Issue #4808 - Review HttpClient Request header APIs.

For some reason, Request.getHeaders() returned HttpFields, but HttpRequest.getHeaders() returned HttpFields.Mutable, and it was obviously wrong. Fixed WebSocket code that was relying on this API error. Signed-off-by: Simone Bordet <[email protected]>
jetty · Jul 13, 2020 · 7b05567 · 7b05567
1 parent fc0f4b2
commit 7b05567
Show file tree

Hide file tree

Showing 6 changed files with 32 additions and 31 deletions.
diff --git a/jetty-client/src/main/java/org/eclipse/jetty/client/HttpRequest.java b/jetty-client/src/main/java/org/eclipse/jetty/client/HttpRequest.java
@@ -372,7 +372,7 @@ public Map<String, Object> getAttributes()
     }
 
     @Override
-    public HttpFields.Mutable getHeaders()
+    public HttpFields getHeaders()
     {
         return headers;
     }

diff --git a/...re-client/src/main/java/org/eclipse/jetty/websocket/core/client/ClientUpgradeRequest.java b/...re-client/src/main/java/org/eclipse/jetty/websocket/core/client/ClientUpgradeRequest.java
@@ -38,7 +38,6 @@
 import org.eclipse.jetty.client.api.Response;
 import org.eclipse.jetty.client.api.Result;
 import org.eclipse.jetty.http.HttpField;
-import org.eclipse.jetty.http.HttpFields;
 import org.eclipse.jetty.http.HttpHeader;
 import org.eclipse.jetty.http.HttpScheme;
 import org.eclipse.jetty.http.HttpStatus;
@@ -154,22 +153,26 @@ public List<String> getSubProtocols()
 
     public void setSubProtocols(String... protocols)
     {
-        HttpFields.Mutable headers = getHeaders();
-        headers.remove(HttpHeader.SEC_WEBSOCKET_SUBPROTOCOL);
-        for (String protocol : protocols)
+        headers(headers ->
         {
-            headers.add(HttpHeader.SEC_WEBSOCKET_SUBPROTOCOL, protocol);
-        }
+            headers.remove(HttpHeader.SEC_WEBSOCKET_SUBPROTOCOL);
+            for (String protocol : protocols)
+            {
+                headers.add(HttpHeader.SEC_WEBSOCKET_SUBPROTOCOL, protocol);
+            }
+        });
     }
 
     public void setSubProtocols(List<String> protocols)
     {
-        HttpFields.Mutable headers = getHeaders();
-        headers.remove(HttpHeader.SEC_WEBSOCKET_SUBPROTOCOL);
-        for (String protocol : protocols)
+        headers(headers ->
         {
-            headers.add(HttpHeader.SEC_WEBSOCKET_SUBPROTOCOL, protocol);
-        }
+            headers.remove(HttpHeader.SEC_WEBSOCKET_SUBPROTOCOL);
+            for (String protocol : protocols)
+            {
+                headers.add(HttpHeader.SEC_WEBSOCKET_SUBPROTOCOL, protocol);
+            }
+        });
     }
 
     @Override
@@ -282,7 +285,7 @@ void requestComplete()
             .collect(Collectors.joining(","));
 
         if (!StringUtil.isEmpty(extensionString))
-            getHeaders().add(HttpHeader.SEC_WEBSOCKET_EXTENSIONS, extensionString);
+            headers(headers -> headers.add(HttpHeader.SEC_WEBSOCKET_EXTENSIONS, extensionString));
 
         // Notify the listener which may change the headers directly.
         notifyUpgradeListeners((listener) -> listener.onHandshakeRequest(this));

diff --git a/...t-core-tests/src/test/java/org/eclipse/jetty/websocket/core/WebSocketNegotiationTest.java b/...t-core-tests/src/test/java/org/eclipse/jetty/websocket/core/WebSocketNegotiationTest.java
@@ -413,13 +413,12 @@ public void testListenerExtensionSelectionError() throws Exception
         upgradeRequest.setSubProtocols("test");
         upgradeRequest.addExtensions("permessage-deflate;server_no_context_takeover");
 
-        CompletableFuture<String> extensionHeader = new CompletableFuture<>();
         upgradeRequest.addListener(new UpgradeListener()
         {
             @Override
             public void onHandshakeRequest(HttpRequest request)
             {
-                request.getHeaders().put(HttpHeader.SEC_WEBSOCKET_EXTENSIONS, "permessage-deflate");
+                request.headers(headers -> headers.put(HttpHeader.SEC_WEBSOCKET_EXTENSIONS, "permessage-deflate"));
             }
         });
 

diff --git a/...t/src/main/java/org/eclipse/jetty/websocket/javax/client/internal/JsrUpgradeListener.java b/...t/src/main/java/org/eclipse/jetty/websocket/javax/client/internal/JsrUpgradeListener.java
@@ -33,7 +33,7 @@
 
 public class JsrUpgradeListener implements UpgradeListener
 {
-    private Configurator configurator;
+    private final Configurator configurator;
 
     public JsrUpgradeListener(Configurator configurator)
     {
@@ -46,7 +46,7 @@ public void onHandshakeRequest(HttpRequest request)
         if (configurator == null)
             return;
 
-        HttpFields.Mutable fields = request.getHeaders();
+        HttpFields fields = request.getHeaders();
         Map<String, List<String>> originalHeaders = new HashMap<>();
         fields.forEach(field ->
         {
@@ -59,8 +59,11 @@ public void onHandshakeRequest(HttpRequest request)
         configurator.beforeRequest(originalHeaders);
 
         // Reset headers on HttpRequest per configurator
-        fields.clear();
-        originalHeaders.forEach(fields::put);
+        request.headers(headers ->
+        {
+            headers.clear();
+            originalHeaders.forEach(headers::put);
+        });
     }
 
     @Override

diff --git a/...cket-javax-tests/src/main/java/org/eclipse/jetty/websocket/javax/tests/NetworkFuzzer.java b/...cket-javax-tests/src/main/java/org/eclipse/jetty/websocket/javax/tests/NetworkFuzzer.java
@@ -30,7 +30,6 @@
 import java.util.concurrent.LinkedBlockingQueue;
 import java.util.concurrent.TimeUnit;
 
-import org.eclipse.jetty.http.HttpFields;
 import org.eclipse.jetty.io.EndPoint;
 import org.eclipse.jetty.util.BufferUtil;
 import org.eclipse.jetty.util.Callback;
@@ -72,11 +71,13 @@ public NetworkFuzzer(LocalServer server, URI wsURI, Map<String, String> requestH
         this.upgradeRequest = new RawUpgradeRequest(client, wsURI);
         if (requestHeaders != null)
         {
-            HttpFields.Mutable fields = this.upgradeRequest.getHeaders();
-            requestHeaders.forEach((name, value) ->
+            this.upgradeRequest.headers(fields ->
             {
-                fields.remove(name);
-                fields.put(name, value);
+                requestHeaders.forEach((name, value) ->
+                {
+                    fields.remove(name);
+                    fields.put(name, value);
+                });
             });
         }
         this.client.start();

diff --git a/...ient/src/main/java/org/eclipse/jetty/websocket/client/impl/JettyClientUpgradeRequest.java b/...ient/src/main/java/org/eclipse/jetty/websocket/client/impl/JettyClientUpgradeRequest.java
@@ -24,7 +24,6 @@
 import java.util.stream.Collectors;
 
 import org.eclipse.jetty.client.HttpResponse;
-import org.eclipse.jetty.http.HttpFields;
 import org.eclipse.jetty.http.HttpHeader;
 import org.eclipse.jetty.http.HttpVersion;
 import org.eclipse.jetty.io.EndPoint;
@@ -49,18 +48,14 @@ public JettyClientUpgradeRequest(WebSocketCoreClient coreClient, UpgradeRequest
         if (request != null)
         {
             // Copy request details into actual request
-            HttpFields.Mutable fields = getHeaders();
-            request.getHeaders().forEach(fields::put);
+            headers(fields -> request.getHeaders().forEach(fields::put));
 
             // Copy manually created Cookies into place
             List<HttpCookie> cookies = request.getCookies();
             if (cookies != null)
             {
                 // TODO: remove existing Cookie header (if set)?
-                for (HttpCookie cookie : cookies)
-                {
-                    fields.add(HttpHeader.COOKIE, cookie.toString());
-                }
+                headers(fields -> cookies.forEach(cookie -> fields.add(HttpHeader.COOKIE, cookie.toString())));
             }
 
             // Copy sub-protocols