Make cookie encoding conform better to RFC 6265 in STRICT mode.

Motivation:

- On the client, cookies should be sorted in decreasing order of path
  length. From RFC 6265:

      5.4.2. The user agent SHOULD sort the cookie-list in the following
      order:

        *  Cookies with longer paths are listed before cookies with
           shorter paths.

        *  Among cookies that have equal-length path fields, cookies with
           earlier creation-times are listed before cookies with later
           creation-times.

      NOTE: Not all user agents sort the cookie-list in this order, but
      this order reflects common practice when this document was
      written, and, historically, there have been servers that
      (erroneously) depended on this order.

  Note that the RFC does not define the path length of cookies without a
  path. We sort pathless cookies before cookies with the longest path,
  since pathless cookies inherit the request path (and setting a path
  that is longer than the request path is of limited use, since it cannot
  be read from the context in which it is written).

- On the server, if there are multiple cookies of the same name, only one
  of them should be encoded. RFC 6265 says:

      Servers SHOULD NOT include more than one Set-Cookie header field in
      the same response with the same cookie-name.

  Note that the RFC does not define which cookie should be set in the case
  of multiple cookies with the same name; we arbitrarily pick the last one.

Modifications:

- Changed the visibility of the 'strict' field to 'protected' in
  CookieEncoder.

- Modified ClientCookieEncoder to sort cookies in decreasing order of path
  length when in strict mode.

- Modified ServerCookieEncoder to return only the last cookie of a given
  name when in strict mode.

- Added a fast path for both strict mode in both client and server code
  for cases with only one cookie, in order avoid the overhead of sorting
  and memory allocation.

- Added unit tests for the new cases.

Result:

- Cookie generation on client and server is now more conformant to RFC 6265.

codec-http/src/main/java/io/netty/handler/codec/http/cookie/ClientCookieEncoder.java

@@ -15,16 +15,24 @@
  */
 package io.netty.handler.codec.http.cookie;
 
-import static io.netty.handler.codec.http.cookie.CookieUtil.*;
+import static io.netty.handler.codec.http.cookie.CookieUtil.add;
+import static io.netty.handler.codec.http.cookie.CookieUtil.addQuoted;
+import static io.netty.handler.codec.http.cookie.CookieUtil.stringBuilder;
+import static io.netty.handler.codec.http.cookie.CookieUtil.stripTrailingSeparator;
+import static io.netty.handler.codec.http.cookie.CookieUtil.stripTrailingSeparatorOrNull;
 import static io.netty.util.internal.ObjectUtil.checkNotNull;
-
-import java.util.Iterator;
-
 import io.netty.handler.codec.http.HttpRequest;
 
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.Collection;
+import java.util.Comparator;
+import java.util.Iterator;
+import java.util.List;
+
 /**
- * A <a href="http://tools.ietf.org/html/rfc6265">RFC6265</a> compliant cookie encoder to be used client side,
+ * A <a href="http://tools.ietf.org/html/rfc6265">RFC6265</a> compliant cookie encoder to be used client side, so
- * so only name=value pairs are sent.
+ * only name=value pairs are sent.
  *
  * User-Agents are not supposed to interpret cookies, so, if present, {@link Cookie#rawValue()} will be used.
  * Otherwise, {@link Cookie#value()} will be used unquoted.
@@ -42,13 +50,14 @@ import io.netty.handler.codec.http.HttpRequest;
 public final class ClientCookieEncoder extends CookieEncoder {
 
     /**
-     * Strict encoder that validates that name and value chars are in the valid scope
+     * Strict encoder that validates that name and value chars are in the valid scope and (for methods that accept
-     * defined in RFC6265
+     * multiple cookies) sorts cookies into order of decreasing path length, as specified in RFC6265.
      */
     public static final ClientCookieEncoder STRICT = new ClientCookieEncoder(true);
 
     /**
-     * Lax instance that doesn't validate name and value
+     * Lax instance that doesn't validate name and value, and (for methods that accept multiple cookies) keeps
+     * cookies in the order in which they were given.
      */
     public static final ClientCookieEncoder LAX = new ClientCookieEncoder(false);
 
@@ -59,8 +68,10 @@ public final class ClientCookieEncoder extends CookieEncoder {
     /**
      * Encodes the specified cookie into a Cookie header value.
      *
-     * @param name the cookie name
+     * @param name
-     * @param value the cookie value
+     *            the cookie name
+     * @param value
+     *            the cookie value
      * @return a Rfc6265 style Cookie header value
      */
     public String encode(String name, String value) {
@@ -70,7 +81,8 @@ public final class ClientCookieEncoder extends CookieEncoder {
     /**
      * Encodes the specified cookie into a Cookie header value.
      *
-     * @param specified the cookie
+     * @param specified
+     *            the cookie
      * @return a Rfc6265 style Cookie header value
      */
     public String encode(Cookie cookie) {
@@ -79,10 +91,37 @@ public final class ClientCookieEncoder extends CookieEncoder {
         return stripTrailingSeparator(buf);
     }
 
+    /**
+     * Sort cookies into decreasing order of path length, breaking ties by sorting into increasing chronological
+     * order of creation time, as recommended by RFC 6265.
+     */
+    private static final Comparator<Cookie> COOKIE_COMPARATOR = new Comparator<Cookie>() {
+        @Override
+        public int compare(Cookie c1, Cookie c2) {
+            String path1 = c1.path();
+            String path2 = c2.path();
+            // Cookies with unspecified path default to the path of the request. We don't
+            // know the request path here, but we assume that the length of an unspecified
+            // path is longer than any specified path (i.e. pathless cookies come first),
+            // because setting cookies with a path longer than the request path is of
+            // limited use.
+            int len1 = path1 == null ? Integer.MAX_VALUE : path1.length();
+            int len2 = path2 == null ? Integer.MAX_VALUE : path2.length();
+            int diff = len2 - len1;
+            if (diff != 0) {
+                return diff;
+            }
+            // Rely on Java's sort stability to retain creation order in cases where
+            // cookies have same path length.
+            return -1;
+        }
+    };
+
     /**
      * Encodes the specified cookies into a single Cookie header value.
      *
-     * @param cookies some cookies
+     * @param cookies
+     *            some cookies
      * @return a Rfc6265 style Cookie header value, null if no cookies are passed.
      */
     public String encode(Cookie... cookies) {
@@ -91,13 +130,52 @@ public final class ClientCookieEncoder extends CookieEncoder {
         }
 
         StringBuilder buf = stringBuilder();
+        if (strict) {
+            if (cookies.length == 1) {
+                encode(buf, cookies[0]);
+            } else {
+                Cookie[] cookiesSorted = Arrays.copyOf(cookies, cookies.length);
+                Arrays.sort(cookiesSorted, COOKIE_COMPARATOR);
+                for (Cookie c : cookiesSorted) {
+                    encode(buf, c);
+                }
+            }
+        } else {
             for (Cookie c : cookies) {
-            if (c == null) {
+                encode(buf, c);
-                break;
+            }
+        }
+        return stripTrailingSeparatorOrNull(buf);
     }
 
+    /**
+     * Encodes the specified cookies into a single Cookie header value.
+     *
+     * @param cookies
+     *            some cookies
+     * @return a Rfc6265 style Cookie header value, null if no cookies are passed.
+     */
+    public String encode(Collection<? extends Cookie> cookies) {
+        if (checkNotNull(cookies, "cookies").isEmpty()) {
+            return null;
+        }
+
+        StringBuilder buf = stringBuilder();
+        if (strict) {
+            if (cookies.size() == 1) {
+                encode(buf, cookies.iterator().next());
+            } else {
+                Cookie[] cookiesSorted = cookies.toArray(new Cookie[cookies.size()]);
+                Arrays.sort(cookiesSorted, COOKIE_COMPARATOR);
+                for (Cookie c : cookiesSorted) {
                     encode(buf, c);
                 }
+            }
+        } else {
+            for (Cookie c : cookies) {
+                encode(buf, c);
+            }
+        }
         return stripTrailingSeparatorOrNull(buf);
     }
 
@@ -114,14 +192,27 @@ public final class ClientCookieEncoder extends CookieEncoder {
         }
 
         StringBuilder buf = stringBuilder();
+        if (strict) {
+            Cookie firstCookie = cookiesIt.next();
+            if (!cookiesIt.hasNext()) {
+                encode(buf, firstCookie);
+            } else {
+                List<Cookie> cookiesList = new ArrayList<Cookie>();
+                cookiesList.add(firstCookie);
                 while (cookiesIt.hasNext()) {
-            Cookie c = cookiesIt.next();
+                    cookiesList.add(cookiesIt.next());
-            if (c == null) {
-                break;
                 }
-
+                Cookie[] cookiesSorted = cookiesList.toArray(new Cookie[cookiesList.size()]);
+                Arrays.sort(cookiesSorted, COOKIE_COMPARATOR);
+                for (Cookie c : cookiesSorted) {
                     encode(buf, c);
                 }
+            }
+        } else {
+            while (cookiesIt.hasNext()) {
+                encode(buf, cookiesIt.next());
+            }
+        }
         return stripTrailingSeparatorOrNull(buf);
     }
 

codec-http/src/main/java/io/netty/handler/codec/http/cookie/CookieEncoder.java

@@ -24,7 +24,7 @@ import static io.netty.handler.codec.http.cookie.CookieUtil.unwrapValue;
  */
 public abstract class CookieEncoder {
 
-    private final boolean strict;
+    protected final boolean strict;
 
     protected CookieEncoder(boolean strict) {
         this.strict = strict;

codec-http/src/main/java/io/netty/handler/codec/http/cookie/ServerCookieEncoder.java

@@ -15,9 +15,11 @@
  */
 package io.netty.handler.codec.http.cookie;
 
-import static io.netty.handler.codec.http.cookie.CookieUtil.*;
+import static io.netty.handler.codec.http.cookie.CookieUtil.add;
+import static io.netty.handler.codec.http.cookie.CookieUtil.addQuoted;
+import static io.netty.handler.codec.http.cookie.CookieUtil.stringBuilder;
+import static io.netty.handler.codec.http.cookie.CookieUtil.stripTrailingSeparator;
 import static io.netty.util.internal.ObjectUtil.checkNotNull;
-
 import io.netty.handler.codec.http.HttpHeaderDateFormat;
 import io.netty.handler.codec.http.HttpRequest;
 
@@ -25,7 +27,10 @@ import java.util.ArrayList;
 import java.util.Collection;
 import java.util.Collections;
 import java.util.Date;
+import java.util.HashMap;
+import java.util.Iterator;
 import java.util.List;
+import java.util.Map;
 
 /**
  * A <a href="http://tools.ietf.org/html/rfc6265">RFC6265</a> compliant cookie encoder to be used server side,
@@ -47,12 +52,15 @@ public final class ServerCookieEncoder extends CookieEncoder {
 
     /**
      * Strict encoder that validates that name and value chars are in the valid scope
-     * defined in RFC6265
+     * defined in RFC6265, and (for methods that accept multiple cookies) that only
+     * one cookie is encoded with any given name. (If multiple cookies have the same
+     * name, the last one is the one that is encoded.)
      */
     public static final ServerCookieEncoder STRICT = new ServerCookieEncoder(true);
 
     /**
-     * Lax instance that doesn't validate name and value
+     * Lax instance that doesn't validate name and value, and that allows multiple
+     * cookies with the same name.
      */
     public static final ServerCookieEncoder LAX = new ServerCookieEncoder(false);
 
@@ -114,6 +122,26 @@ public final class ServerCookieEncoder extends CookieEncoder {
         return stripTrailingSeparator(buf);
     }
 
+    /** Deduplicate a list of encoded cookies by keeping only the last instance with a given name.
+     *
+     * @param encoded The list of encoded cookies.
+     * @param nameToLastIndex A map from cookie name to index of last cookie instance.
+     * @return The encoded list with all but the last instance of a named cookie.
+     */
+    private List<String> dedup(List<String> encoded, Map<String, Integer> nameToLastIndex) {
+        boolean[] isLastInstance = new boolean[encoded.size()];
+        for (int idx : nameToLastIndex.values()) {
+            isLastInstance[idx] = true;
+        }
+        List<String> dedupd = new ArrayList<String>(nameToLastIndex.size());
+        for (int i = 0, n = encoded.size(); i < n; i++) {
+            if (isLastInstance[i]) {
+                dedupd.add(encoded.get(i));
+            }
+        }
+        return dedupd;
+    }
+
     /**
      * Batch encodes cookies into Set-Cookie header values.
      *
@@ -126,13 +154,16 @@ public final class ServerCookieEncoder extends CookieEncoder {
         }
 
         List<String> encoded = new ArrayList<String>(cookies.length);
-        for (Cookie c : cookies) {
+        Map<String, Integer> nameToIndex = strict && cookies.length > 1 ? new HashMap<String, Integer>() : null;
-            if (c == null) {
+        boolean hasDupdName = false;
-                break;
+        for (int i = 0; i < cookies.length; i++) {
-            }
+            Cookie c = cookies[i];
             encoded.add(encode(c));
+            if (nameToIndex != null) {
+                hasDupdName |= nameToIndex.put(c.name(), i) != null;
             }
-        return encoded;
+        }
+        return hasDupdName ? dedup(encoded, nameToIndex) : encoded;
     }
 
     /**
@@ -147,13 +178,16 @@ public final class ServerCookieEncoder extends CookieEncoder {
         }
 
         List<String> encoded = new ArrayList<String>(cookies.size());
+        Map<String, Integer> nameToIndex = strict && cookies.size() > 1 ? new HashMap<String, Integer>() : null;
+        int i = 0;
+        boolean hasDupdName = false;
         for (Cookie c : cookies) {
-            if (c == null) {
-                break;
-            }
             encoded.add(encode(c));
+            if (nameToIndex != null) {
+                hasDupdName |= nameToIndex.put(c.name(), i++) != null;
             }
-        return encoded;
+        }
+        return hasDupdName ? dedup(encoded, nameToIndex) : encoded;
     }
 
     /**
@@ -163,17 +197,24 @@ public final class ServerCookieEncoder extends CookieEncoder {
      * @return the corresponding bunch of Set-Cookie headers
      */
     public List<String> encode(Iterable<? extends Cookie> cookies) {
-        if (!checkNotNull(cookies, "cookies").iterator().hasNext()) {
+        Iterator<? extends Cookie> cookiesIt = checkNotNull(cookies, "cookies").iterator();
+        if (!cookiesIt.hasNext()) {
             return Collections.emptyList();
         }
 
         List<String> encoded = new ArrayList<String>();
-        for (Cookie c : cookies) {
+        Cookie firstCookie = cookiesIt.next();
-            if (c == null) {
+        Map<String, Integer> nameToIndex = strict && cookiesIt.hasNext() ? new HashMap<String, Integer>() : null;
-                break;
+        int i = 0;
-            }
+        encoded.add(encode(firstCookie));
+        boolean hasDupdName = nameToIndex != null ? nameToIndex.put(firstCookie.name(), i++) != null : false;
+        while (cookiesIt.hasNext()) {
+            Cookie c = cookiesIt.next();
             encoded.add(encode(c));
-        }
+            if (nameToIndex != null) {
-        return encoded;
+                hasDupdName |= nameToIndex.put(c.name(), i++) != null;
+            }
+        }
+        return hasDupdName ? dedup(encoded, nameToIndex) : encoded;
     }
 }

codec-http/src/test/java/io/netty/handler/codec/http/cookie/ClientCookieEncoderTest.java

@@ -23,21 +23,23 @@ public class ClientCookieEncoderTest {
 
     @Test
     public void testEncodingMultipleClientCookies() {
-        String c1 = "myCookie=myValue; ";
+        String c1 = "myCookie=myValue";
-        String c2 = "myCookie2=myValue2; ";
+        String c2 = "myCookie2=myValue2";
         String c3 = "myCookie3=myValue3";
-        Cookie cookie = new DefaultCookie("myCookie", "myValue");
+        Cookie cookie1 = new DefaultCookie("myCookie", "myValue");
-        cookie.setDomain(".adomainsomewhere");
+        cookie1.setDomain(".adomainsomewhere");
-        cookie.setMaxAge(50);
+        cookie1.setMaxAge(50);
-        cookie.setPath("/apathsomewhere");
+        cookie1.setPath("/apathsomewhere");
-        cookie.setSecure(true);
+        cookie1.setSecure(true);
         Cookie cookie2 = new DefaultCookie("myCookie2", "myValue2");
         cookie2.setDomain(".anotherdomainsomewhere");
         cookie2.setPath("/anotherpathsomewhere");
         cookie2.setSecure(false);
         Cookie cookie3 = new DefaultCookie("myCookie3", "myValue3");
-        String encodedCookie = ClientCookieEncoder.STRICT.encode(cookie, cookie2, cookie3);
+        String encodedCookie = ClientCookieEncoder.STRICT.encode(cookie1, cookie2, cookie3);
-        assertEquals(c1 + c2 + c3, encodedCookie);
+        // Cookies should be sorted into decreasing order of path length, as per RFC6265.
+        // When no path is provided, we assume maximum path length (so cookie3 comes first).
+        assertEquals(c3 + "; " + c2 + "; " + c1, encodedCookie);
     }
 
     @Test

codec-http/src/test/java/io/netty/handler/codec/http/cookie/ServerCookieEncoderTest.java

@@ -15,17 +15,20 @@
  */
 package io.netty.handler.codec.http.cookie;
 
-import org.junit.Test;
+import static org.junit.Assert.assertEquals;
-
+import static org.junit.Assert.assertNotNull;
+import static org.junit.Assert.assertNull;
+import static org.junit.Assert.assertTrue;
 import io.netty.handler.codec.http.HttpHeaderDateFormat;
 
 import java.text.ParseException;
+import java.util.ArrayList;
 import java.util.Date;
 import java.util.List;
 import java.util.regex.Matcher;
 import java.util.regex.Pattern;
 
-import static org.junit.Assert.*;
+import org.junit.Test;
 
 public class ServerCookieEncoderTest {
 
@@ -60,4 +63,29 @@ public class ServerCookieEncoderTest {
         assertNotNull(encodedCookie2);
         assertTrue(encodedCookie2.isEmpty());
     }
+
+    @Test
+    public void testEncodingMultipleCookiesStrict() {
+        List<String> result = new ArrayList<String>();
+        result.add("cookie2=value2");
+        result.add("cookie1=value3");
+        Cookie cookie1 = new DefaultCookie("cookie1", "value1");
+        Cookie cookie2 = new DefaultCookie("cookie2", "value2");
+        Cookie cookie3 = new DefaultCookie("cookie1", "value3");
+        List<String> encodedCookies = ServerCookieEncoder.STRICT.encode(cookie1, cookie2, cookie3);
+        assertEquals(result, encodedCookies);
+    }
+
+    @Test
+    public void testEncodingMultipleCookiesLax() {
+        List<String> result = new ArrayList<String>();
+        result.add("cookie1=value1");
+        result.add("cookie2=value2");
+        result.add("cookie1=value3");
+        Cookie cookie1 = new DefaultCookie("cookie1", "value1");
+        Cookie cookie2 = new DefaultCookie("cookie2", "value2");
+        Cookie cookie3 = new DefaultCookie("cookie1", "value3");
+        List<String> encodedCookies = ServerCookieEncoder.LAX.encode(cookie1, cookie2, cookie3);
+        assertEquals(result, encodedCookies);
+    }
 }