[CookieStore] Only set Cookies if they are not already set (#2033)

This changes the behavior of the automatic usage of the `CookieStore` to avoid overwriting already-set `Cookie`s and, instead only sets them if they do not exist yet. Closes #1964 Co-authored-by: Aayush Atharva <aayush@shieldblaze.com>
AsyncHttpClient · Dec 1, 2024 · d5a8336 · d5a8336
1 parent 2af2715
commit d5a8336
Show file tree

Hide file tree

Showing 4 changed files with 55 additions and 9 deletions.
diff --git a/client/src/main/java/org/asynchttpclient/DefaultAsyncHttpClient.java b/client/src/main/java/org/asynchttpclient/DefaultAsyncHttpClient.java
@@ -235,7 +235,7 @@ public <T> ListenableFuture<T> executeRequest(Request request, AsyncHandler<T> h
                 if (!cookies.isEmpty()) {
                     RequestBuilder requestBuilder = request.toBuilder();
                     for (Cookie cookie : cookies) {
-                        requestBuilder.addOrReplaceCookie(cookie);
+                        requestBuilder.addCookieIfUnset(cookie);
                     }
                     request = requestBuilder.build();
                 }

diff --git a/client/src/main/java/org/asynchttpclient/RequestBuilderBase.java b/client/src/main/java/org/asynchttpclient/RequestBuilderBase.java
@@ -323,6 +323,21 @@ public T addCookie(Cookie cookie) {
      * @return this
      */
     public T addOrReplaceCookie(Cookie cookie) {
+        return maybeAddOrReplaceCookie(cookie, true);
+    }
+
+    /**
+     * Add a cookie based on its name, if it does not exist yet. Cookies that
+     * are already set will be ignored.
+     *
+     * @param cookie the new cookie
+     * @return this
+     */
+    public T addCookieIfUnset(Cookie cookie) {
+        return maybeAddOrReplaceCookie(cookie, false);
+    }
+
+    private T maybeAddOrReplaceCookie(Cookie cookie, boolean allowReplace) {
         String cookieKey = cookie.name();
         boolean replace = false;
         int index = 0;
@@ -335,10 +350,10 @@ public T addOrReplaceCookie(Cookie cookie) {
 
             index++;
         }
-        if (replace) {
-            cookies.set(index, cookie);
-        } else {
+        if (!replace) {
             cookies.add(cookie);
+        } else if (allowReplace) {
+            cookies.set(index, cookie);
         }
         return asDerivedType();
     }

diff --git a/client/src/main/java/org/asynchttpclient/netty/handler/intercept/Redirect30xInterceptor.java b/client/src/main/java/org/asynchttpclient/netty/handler/intercept/Redirect30xInterceptor.java
@@ -142,11 +142,8 @@ public boolean exitAfterHandlingRedirect(Channel channel, NettyResponseFuture<?>
                 CookieStore cookieStore = config.getCookieStore();
                 if (cookieStore != null) {
                     // Update request's cookies assuming that cookie store is already updated by Interceptors
-                    List<Cookie> cookies = cookieStore.get(newUri);
-                    if (!cookies.isEmpty()) {
-                        for (Cookie cookie : cookies) {
-                            requestBuilder.addOrReplaceCookie(cookie);
-                        }
+                    for (Cookie cookie : cookieStore.get(newUri)) {
+                        requestBuilder.addCookieIfUnset(cookie);
                     }
                 }
 

diff --git a/client/src/test/java/org/asynchttpclient/RequestBuilderTest.java b/client/src/test/java/org/asynchttpclient/RequestBuilderTest.java
@@ -166,6 +166,40 @@ public void testAddOrReplaceCookies() {
         assertEquals(requestBuilder.cookies.size(), 2, "cookie size must be 2 after adding 1 more cookie i.e. cookie3");
     }
 
+    @RepeatedIfExceptionsTest(repeats = 5)
+    public void testAddIfUnsetCookies() {
+        RequestBuilder requestBuilder = new RequestBuilder();
+        Cookie cookie = new DefaultCookie("name", "value");
+        cookie.setDomain("google.com");
+        cookie.setPath("/");
+        cookie.setMaxAge(1000);
+        cookie.setSecure(true);
+        cookie.setHttpOnly(true);
+        requestBuilder.addCookieIfUnset(cookie);
+        assertEquals(requestBuilder.cookies.size(), 1, "cookies size should be 1 after adding one cookie");
+        assertEquals(requestBuilder.cookies.get(0), cookie, "cookie does not match");
+
+        Cookie cookie2 = new DefaultCookie("name", "value");
+        cookie2.setDomain("google2.com");
+        cookie2.setPath("/path");
+        cookie2.setMaxAge(1001);
+        cookie2.setSecure(false);
+        cookie2.setHttpOnly(false);
+
+        requestBuilder.addCookieIfUnset(cookie2);
+        assertEquals(requestBuilder.cookies.size(), 1, "cookies size should remain 1 as we just ignored cookie2 because of a cookie with same name");
+        assertEquals(requestBuilder.cookies.get(0), cookie, "cookie does not match");
+
+        Cookie cookie3 = new DefaultCookie("name2", "value");
+        cookie3.setDomain("google.com");
+        cookie3.setPath("/");
+        cookie3.setMaxAge(1000);
+        cookie3.setSecure(true);
+        cookie3.setHttpOnly(true);
+        requestBuilder.addCookieIfUnset(cookie3);
+        assertEquals(requestBuilder.cookies.size(), 2, "cookie size must be 2 after adding 1 more cookie i.e. cookie3");
+    }
+
     @RepeatedIfExceptionsTest(repeats = 5)
     public void testSettingQueryParamsBeforeUrlShouldNotProduceNPE() {
         RequestBuilder requestBuilder = new RequestBuilder();