399173: UpgradeRequest.getParameterMap() should never return null

+ Making api.UpgradeRequest never return null, but also have no logic on
  how to populate the parameter map
+ Using MultiMap in websocket-client for parameter map parsing
+ Using HttpServletRequest.getParameterMap() as-is in websocket-server
+ Adding unit testing for both sides

9 files changed, 360 insertions, 39 deletions

diff --git a/jetty-websocket/websocket-api/src/main/java/org/eclipse/jetty/websocket/api/UpgradeRequest.java b/jetty-websocket/websocket-api/src/main/java/org/eclipse/jetty/websocket/api/UpgradeRequest.java
index ed3bed6883..5d6cc1a653 100644
--- a/jetty-websocket/websocket-api/src/main/java/org/eclipse/jetty/websocket/api/UpgradeRequest.java
+++ b/jetty-websocket/websocket-api/src/main/java/org/eclipse/jetty/websocket/api/UpgradeRequest.java
@@ -21,6 +21,7 @@ package org.eclipse.jetty.websocket.api;
 import java.net.HttpCookie;
 import java.net.URI;
 import java.util.ArrayList;
+import java.util.Collections;
 import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
@@ -35,6 +36,7 @@ public class UpgradeRequest
     private List<ExtensionConfig> extensions = new ArrayList<>();
     private List<HttpCookie> cookies = new ArrayList<>();
     private Map<String, List<String>> headers = new HashMap<>();
+    private Map<String, String[]> parameters = new HashMap<>();
     private Object session;
     private String httpVersion;
     private String method;
@@ -168,10 +170,14 @@ public class UpgradeRequest
         return getHeader("Origin");
     }
 
+    /**
+     * Returns a map of the query parameters of the request.
+     * 
+     * @return a unmodifiable map of query parameters of the request.
+     */
     public Map<String, String[]> getParameterMap()
     {
-        // TODO Auto-generated method stub
-        return null;
+        return Collections.unmodifiableMap(parameters);
     }
 
     public String getQueryString()
@@ -238,11 +244,17 @@ public class UpgradeRequest
         this.method = method;
     }
 
+    protected void setParameterMap(Map<String, String[]> parameters)
+    {
+        this.parameters.clear();
+        this.parameters.putAll(parameters);
+    }
+
     public void setRequestURI(URI uri)
     {
         this.requestURI = uri;
         this.host = this.requestURI.getHost();
-        // TODO: parse parameters
+        this.parameters.clear();
     }
 
     public void setSession(Object session)
diff --git a/jetty-websocket/websocket-client/src/main/java/org/eclipse/jetty/websocket/client/ClientUpgradeRequest.java b/jetty-websocket/websocket-client/src/main/java/org/eclipse/jetty/websocket/client/ClientUpgradeRequest.java
index 248002e7b0..5c662d6ee7 100644
--- a/jetty-websocket/websocket-client/src/main/java/org/eclipse/jetty/websocket/client/ClientUpgradeRequest.java
+++ b/jetty-websocket/websocket-client/src/main/java/org/eclipse/jetty/websocket/client/ClientUpgradeRequest.java
@@ -21,13 +21,17 @@ package org.eclipse.jetty.websocket.client;
 import java.net.CookieStore;
 import java.net.HttpCookie;
 import java.net.URI;
+import java.util.HashMap;
 import java.util.HashSet;
 import java.util.List;
+import java.util.Map;
 import java.util.Random;
 import java.util.Set;
 
 import org.eclipse.jetty.util.B64Code;
+import org.eclipse.jetty.util.MultiMap;
 import org.eclipse.jetty.util.StringUtil;
+import org.eclipse.jetty.util.UrlEncoded;
 import org.eclipse.jetty.util.log.Log;
 import org.eclipse.jetty.util.log.Logger;
 import org.eclipse.jetty.websocket.api.UpgradeRequest;
@@ -39,6 +43,7 @@ import org.eclipse.jetty.websocket.api.extensions.ExtensionConfig;
 public class ClientUpgradeRequest extends UpgradeRequest
 {
     private final static Logger LOG = Log.getLogger(ClientUpgradeRequest.class);
+    private final static int MAX_KEYS = -1; // maximum number of parameter keys to decode
     private static final Set<String> FORBIDDEN_HEADERS;
 
     static
@@ -203,4 +208,37 @@ public class ClientUpgradeRequest extends UpgradeRequest
 
         setCookies(cookieStore.get(getRequestURI()));
     }
+
+    @Override
+    public void setRequestURI(URI uri)
+    {
+        super.setRequestURI(uri);
+
+        // parse parameter map
+        Map<String, String[]> pmap = new HashMap<>();
+
+        String query = uri.getQuery();
+
+        if (StringUtil.isNotBlank(query))
+        {
+            MultiMap<String> params = new MultiMap<String>();
+            UrlEncoded.decodeTo(uri.getQuery(),params,"UTF-8",MAX_KEYS);
+
+            for (String key : params.keySet())
+            {
+                List<String> values = params.getValues(key);
+                if (values == null)
+                {
+                    pmap.put(key,new String[0]);
+                }
+                else
+                {
+                    int len = values.size();
+                    pmap.put(key,values.toArray(new String[len]));
+                }
+            }
+
+            super.setParameterMap(pmap);
+        }
+    }
 }
diff --git a/jetty-websocket/websocket-client/src/test/java/org/eclipse/jetty/websocket/client/WebSocketClientTest.java b/jetty-websocket/websocket-client/src/test/java/org/eclipse/jetty/websocket/client/WebSocketClientTest.java
index c77566f133..34c4dc75d0 100644
--- a/jetty-websocket/websocket-client/src/test/java/org/eclipse/jetty/websocket/client/WebSocketClientTest.java
+++ b/jetty-websocket/websocket-client/src/test/java/org/eclipse/jetty/websocket/client/WebSocketClientTest.java
@@ -22,11 +22,13 @@ import static org.hamcrest.Matchers.*;
 
 import java.net.InetSocketAddress;
 import java.net.URI;
+import java.util.Map;
 import java.util.concurrent.Future;
 import java.util.concurrent.TimeUnit;
 
 import org.eclipse.jetty.toolchain.test.AdvancedRunner;
 import org.eclipse.jetty.websocket.api.Session;
+import org.eclipse.jetty.websocket.api.UpgradeRequest;
 import org.eclipse.jetty.websocket.client.blockhead.BlockheadServer;
 import org.eclipse.jetty.websocket.client.blockhead.BlockheadServer.ServerConnection;
 import org.eclipse.jetty.websocket.common.WebSocketFrame;
@@ -207,4 +209,45 @@ public class WebSocketClientTest
             factSmall.stop();
         }
     }
+
+    @Test
+    public void testParameterMap() throws Exception
+    {
+        WebSocketClient fact = new WebSocketClient();
+        fact.start();
+        try
+        {
+            TrackingSocket wsocket = new TrackingSocket();
+
+            URI wsUri = server.getWsUri().resolve("/test?snack=cashews&amount=handful&brand=off");
+            Future<Session> future = client.connect(wsocket,wsUri);
+
+            ServerConnection ssocket = server.accept();
+            ssocket.upgrade();
+
+            future.get(500,TimeUnit.MILLISECONDS);
+
+            Assert.assertTrue(wsocket.openLatch.await(1,TimeUnit.SECONDS));
+
+            Session session = (Session)wsocket.getConnection();
+            UpgradeRequest req = session.getUpgradeRequest();
+            Assert.assertThat("Upgrade Request",req,notNullValue());
+
+            Map<String, String[]> parameterMap = req.getParameterMap();
+            Assert.assertThat("Parameter Map",parameterMap,notNullValue());
+
+            Assert.assertThat("Parameter[snack]",parameterMap.get("snack"),is(new String[]
+            { "cashews" }));
+            Assert.assertThat("Parameter[amount]",parameterMap.get("amount"),is(new String[]
+            { "handful" }));
+            Assert.assertThat("Parameter[brand]",parameterMap.get("brand"),is(new String[]
+            { "off" }));
+
+            Assert.assertThat("Parameter[cost]",parameterMap.get("cost"),nullValue());
+        }
+        finally
+        {
+            fact.stop();
+        }
+    }
 }
diff --git a/jetty-websocket/websocket-server/src/main/java/org/eclipse/jetty/websocket/server/ServletWebSocketRequest.java b/jetty-websocket/websocket-server/src/main/java/org/eclipse/jetty/websocket/server/ServletWebSocketRequest.java
index e234220d1a..9186c93282 100644
--- a/jetty-websocket/websocket-server/src/main/java/org/eclipse/jetty/websocket/server/ServletWebSocketRequest.java
+++ b/jetty-websocket/websocket-server/src/main/java/org/eclipse/jetty/websocket/server/ServletWebSocketRequest.java
@@ -49,6 +49,9 @@ public class ServletWebSocketRequest extends UpgradeRequest
         setMethod(request.getMethod());
         setHttpVersion(request.getProtocol());
 
+        // Copy parameters
+        super.setParameterMap(request.getParameterMap());
+
         // Copy Cookies
         cookieMap = new HashMap<String, String>();
         for (Cookie cookie : request.getCookies())
@@ -98,6 +101,40 @@ public class ServletWebSocketRequest extends UpgradeRequest
         }
     }
 
+    public Principal getPrincipal()
+    {
+        return req.getUserPrincipal();
+    }
+
+    public StringBuffer getRequestURL()
+    {
+        return req.getRequestURL();
+    }
+
+    public Map<String, Object> getServletAttributes()
+    {
+        Map<String, Object> attributes = new HashMap<String, Object>();
+
+        for (String name : Collections.list(req.getAttributeNames()))
+        {
+            attributes.put(name,req.getAttribute(name));
+        }
+
+        return attributes;
+    }
+
+    public Map<String, List<String>> getServletParameters()
+    {
+        Map<String, List<String>> parameters = new HashMap<String, List<String>>();
+
+        for (String name : Collections.list(req.getParameterNames()))
+        {
+            parameters.put(name,Collections.unmodifiableList(Arrays.asList(req.getParameterValues(name))));
+        }
+
+        return parameters;
+    }
+
     protected String[] parseProtocols(String protocol)
     {
         if (protocol == null)
@@ -121,38 +158,4 @@ public class ServletWebSocketRequest extends UpgradeRequest
     {
         this.req.setAttribute(name,o);
     }
-    
-    public Principal getPrincipal()
-    {
-        return req.getUserPrincipal();
-    }
-    
-    public StringBuffer getRequestURL()
-    {
-        return req.getRequestURL();
-    }
-    
-    public Map<String, Object> getServletAttributes()
-    {
-        Map<String, Object> attributes = new HashMap<String,Object>();
-        
-        for (String name : Collections.list((Enumeration<String>)req.getAttributeNames()))
-        {
-            attributes.put(name, req.getAttribute(name));
-        }
-        
-        return attributes;
-    }
-    
-    public Map<String, List<String>> getServletParameters()
-    {
-        Map<String, List<String>> parameters = new HashMap<String, List<String>>();
-        
-        for (String name : Collections.list((Enumeration<String>)req.getParameterNames()))
-        {
-            parameters.put(name, Collections.unmodifiableList(Arrays.asList(req.getParameterValues(name))));
-        }
-        
-        return parameters;
-    }   
 }
diff --git a/jetty-websocket/websocket-server/src/test/java/org/eclipse/jetty/websocket/server/WebSocketServerSessionTest.java b/jetty-websocket/websocket-server/src/test/java/org/eclipse/jetty/websocket/server/WebSocketServerSessionTest.java
new file mode 100644
index 0000000000..0bfd4c97b3
--- /dev/null
+++ b/jetty-websocket/websocket-server/src/test/java/org/eclipse/jetty/websocket/server/WebSocketServerSessionTest.java
@@ -0,0 +1,92 @@
+//
+//  ========================================================================
+//  Copyright (c) 1995-2013 Mort Bay Consulting Pty. Ltd.
+//  ------------------------------------------------------------------------
+//  All rights reserved. This program and the accompanying materials
+//  are made available under the terms of the Eclipse Public License v1.0
+//  and Apache License v2.0 which accompanies this distribution.
+//
+//      The Eclipse Public License is available at
+//      http://www.eclipse.org/legal/epl-v10.html
+//
+//      The Apache License v2.0 is available at
+//      http://www.opensource.org/licenses/apache2.0.php
+//
+//  You may elect to redistribute this code under either of these licenses.
+//  ========================================================================
+//
+
+package org.eclipse.jetty.websocket.server;
+
+import static org.hamcrest.Matchers.*;
+
+import java.net.URI;
+import java.util.concurrent.TimeUnit;
+
+import org.eclipse.jetty.toolchain.test.AdvancedRunner;
+import org.eclipse.jetty.websocket.api.Session;
+import org.eclipse.jetty.websocket.common.WebSocketFrame;
+import org.eclipse.jetty.websocket.server.blockhead.BlockheadClient;
+import org.eclipse.jetty.websocket.server.helper.IncomingFramesCapture;
+import org.eclipse.jetty.websocket.server.helper.SessionServlet;
+import org.junit.AfterClass;
+import org.junit.Assert;
+import org.junit.BeforeClass;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+
+/**
+ * Testing various aspects of the server side support for WebSocket {@link Session}
+ */
+@RunWith(AdvancedRunner.class)
+public class WebSocketServerSessionTest
+{
+    private static SimpleServletServer server;
+
+    @BeforeClass
+    public static void startServer() throws Exception
+    {
+        server = new SimpleServletServer(new SessionServlet());
+        server.start();
+    }
+
+    @AfterClass
+    public static void stopServer()
+    {
+        server.stop();
+    }
+
+    @Test
+    public void testUpgradeRequestResponse() throws Exception
+    {
+        URI uri = server.getServerUri().resolve("/test?snack=cashews&amount=handful&brand=off");
+        BlockheadClient client = new BlockheadClient(uri);
+        try
+        {
+            client.connect();
+            client.sendStandardRequest();
+            client.expectUpgradeResponse();
+
+            // Ask the server socket for specific parameter map info
+            client.write(WebSocketFrame.text("getParameterMap|snack"));
+            client.write(WebSocketFrame.text("getParameterMap|amount"));
+            client.write(WebSocketFrame.text("getParameterMap|brand"));
+            client.write(WebSocketFrame.text("getParameterMap|cost")); // intentionall invalid
+
+            // Read frame (hopefully text frame)
+            IncomingFramesCapture capture = client.readFrames(4,TimeUnit.MILLISECONDS,500);
+            WebSocketFrame tf = capture.getFrames().pop();
+            Assert.assertThat("Parameter Map[snack]",tf.getPayloadAsUTF8(),is("[cashews]"));
+            tf = capture.getFrames().pop();
+            Assert.assertThat("Parameter Map[amount]",tf.getPayloadAsUTF8(),is("[handful]"));
+            tf = capture.getFrames().pop();
+            Assert.assertThat("Parameter Map[brand]",tf.getPayloadAsUTF8(),is("[off]"));
+            tf = capture.getFrames().pop();
+            Assert.assertThat("Parameter Map[cost]",tf.getPayloadAsUTF8(),is("<null>"));
+        }
+        finally
+        {
+            client.close();
+        }
+    }
+}
diff --git a/jetty-websocket/websocket-server/src/test/java/org/eclipse/jetty/websocket/server/blockhead/BlockheadClient.java b/jetty-websocket/websocket-server/src/test/java/org/eclipse/jetty/websocket/server/blockhead/BlockheadClient.java
index 135ed7ab1c..9eea24986f 100644
--- a/jetty-websocket/websocket-server/src/test/java/org/eclipse/jetty/websocket/server/blockhead/BlockheadClient.java
+++ b/jetty-websocket/websocket-server/src/test/java/org/eclipse/jetty/websocket/server/blockhead/BlockheadClient.java
@@ -130,7 +130,8 @@ public class BlockheadClient implements IncomingFrames, OutgoingFrames
         {
             scheme = "https";
         }
-        this.destHttpURI = new URI(scheme,destWebsocketURI.getSchemeSpecificPart(),destWebsocketURI.getFragment());
+        this.destHttpURI = new URI(scheme,destWebsocketURI.getUserInfo(),destWebsocketURI.getHost(),destWebsocketURI.getPort(),destWebsocketURI.getPath(),
+                destWebsocketURI.getQuery(),destWebsocketURI.getFragment());
 
         this.bufferPool = new MappedByteBufferPool(8192);
         this.generator = new Generator(policy,bufferPool);
@@ -556,7 +557,13 @@ public class BlockheadClient implements IncomingFrames, OutgoingFrames
     public void sendStandardRequest() throws IOException
     {
         StringBuilder req = new StringBuilder();
-        req.append("GET /chat HTTP/1.1\r\n");
+        req.append("GET ");
+        req.append(destHttpURI.getPath());
+        if (StringUtil.isNotBlank(destHttpURI.getQuery()))
+        {
+            req.append('?').append(destHttpURI.getQuery());
+        }
+        req.append(" HTTP/1.1\r\n");
         req.append("Host: ").append(destHttpURI.getHost());
         if (destHttpURI.getPort() > 0)
         {
diff --git a/jetty-websocket/websocket-server/src/test/java/org/eclipse/jetty/websocket/server/helper/SessionServlet.java b/jetty-websocket/websocket-server/src/test/java/org/eclipse/jetty/websocket/server/helper/SessionServlet.java
new file mode 100644
index 0000000000..395eb520f9
--- /dev/null
+++ b/jetty-websocket/websocket-server/src/test/java/org/eclipse/jetty/websocket/server/helper/SessionServlet.java
@@ -0,0 +1,32 @@
+//
+//  ========================================================================
+//  Copyright (c) 1995-2013 Mort Bay Consulting Pty. Ltd.
+//  ------------------------------------------------------------------------
+//  All rights reserved. This program and the accompanying materials
+//  are made available under the terms of the Eclipse Public License v1.0
+//  and Apache License v2.0 which accompanies this distribution.
+//
+//      The Eclipse Public License is available at
+//      http://www.eclipse.org/legal/epl-v10.html
+//
+//      The Apache License v2.0 is available at
+//      http://www.opensource.org/licenses/apache2.0.php
+//
+//  You may elect to redistribute this code under either of these licenses.
+//  ========================================================================
+//
+
+package org.eclipse.jetty.websocket.server.helper;
+
+import org.eclipse.jetty.websocket.servlet.WebSocketServlet;
+import org.eclipse.jetty.websocket.servlet.WebSocketServletFactory;
+
+@SuppressWarnings("serial")
+public class SessionServlet extends WebSocketServlet
+{
+    @Override
+    public void configure(WebSocketServletFactory factory)
+    {
+        factory.register(SessionSocket.class);
+    }
+}
diff --git a/jetty-websocket/websocket-server/src/test/java/org/eclipse/jetty/websocket/server/helper/SessionSocket.java b/jetty-websocket/websocket-server/src/test/java/org/eclipse/jetty/websocket/server/helper/SessionSocket.java
new file mode 100644
index 0000000000..ca12ac9196
--- /dev/null
+++ b/jetty-websocket/websocket-server/src/test/java/org/eclipse/jetty/websocket/server/helper/SessionSocket.java
@@ -0,0 +1,93 @@
+//
+//  ========================================================================
+//  Copyright (c) 1995-2013 Mort Bay Consulting Pty. Ltd.
+//  ------------------------------------------------------------------------
+//  All rights reserved. This program and the accompanying materials
+//  are made available under the terms of the Eclipse Public License v1.0
+//  and Apache License v2.0 which accompanies this distribution.
+//
+//      The Eclipse Public License is available at
+//      http://www.eclipse.org/legal/epl-v10.html
+//
+//      The Apache License v2.0 is available at
+//      http://www.opensource.org/licenses/apache2.0.php
+//
+//  You may elect to redistribute this code under either of these licenses.
+//  ========================================================================
+//
+
+package org.eclipse.jetty.websocket.server.helper;
+
+import java.util.Map;
+
+import org.eclipse.jetty.util.log.Log;
+import org.eclipse.jetty.util.log.Logger;
+import org.eclipse.jetty.websocket.api.Session;
+import org.eclipse.jetty.websocket.api.WebSocketConnection;
+import org.eclipse.jetty.websocket.api.annotations.OnWebSocketConnect;
+import org.eclipse.jetty.websocket.api.annotations.OnWebSocketMessage;
+import org.eclipse.jetty.websocket.api.annotations.WebSocket;
+
+@WebSocket
+public class SessionSocket
+{
+    private static final Logger LOG = Log.getLogger(SessionSocket.class);
+    private Session session;
+
+    @OnWebSocketConnect
+    public void onConnect(WebSocketConnection conn)
+    {
+        this.session = (Session)conn;
+    }
+
+    @OnWebSocketMessage
+    public void onText(String message)
+    {
+        LOG.debug("onText({})",message);
+        if (message == null)
+        {
+            return;
+        }
+
+        try
+        {
+            if (message.startsWith("getParameterMap"))
+            {
+                Map<String, String[]> parameterMap = session.getUpgradeRequest().getParameterMap();
+
+                int idx = message.indexOf('|');
+                String key = message.substring(idx + 1);
+                String values[] = parameterMap.get(key);
+
+                if (values == null)
+                {
+                    session.getRemote().sendStringByFuture("<null>");
+                    return;
+                }
+
+                StringBuilder valueStr = new StringBuilder();
+                valueStr.append('[');
+                boolean delim = false;
+                for (String value : values)
+                {
+                    if (delim)
+                    {
+                        valueStr.append(", ");
+                    }
+                    valueStr.append(value);
+                    delim = true;
+                }
+                valueStr.append(']');
+                session.getRemote().sendStringByFuture(valueStr.toString());
+                return;
+            }
+
+            // echo the message back.
+            this.session.getRemote().sendStringByFuture(message);
+        }
+        catch (Throwable t)
+        {
+            LOG.warn(t);
+        }
+    }
+}
diff --git a/jetty-websocket/websocket-server/src/test/resources/jetty-logging.properties b/jetty-websocket/websocket-server/src/test/resources/jetty-logging.properties
index 26bbba6fc6..523b9ad6c5 100644
--- a/jetty-websocket/websocket-server/src/test/resources/jetty-logging.properties
+++ b/jetty-websocket/websocket-server/src/test/resources/jetty-logging.properties
@@ -10,6 +10,7 @@ org.eclipse.jetty.LEVEL=WARN
 # org.eclipse.jetty.websocket.common.Generator.LEVEL=DEBUG
 # org.eclipse.jetty.websocket.server.ab.Fuzzer.LEVEL=DEBUG
 # org.eclipse.jetty.websocket.server.blockhead.LEVEL=DEBUG
+# org.eclipse.jetty.websocket.server.helper.LEVEL=DEBUG
 
 ### Show state changes on BrowserDebugTool
 # org.eclipse.jetty.websocket.server.browser.LEVEL=DEBUG