Merge pull request #8729 from vespa-engine/bjorncs/jdisc-uri-bindings

Use local port when constructing request uri (MERGEOK)

2 files changed, 39 insertions, 467 deletions

diff --git a/jdisc_http_service/src/main/java/com/yahoo/jdisc/http/server/jetty/HttpRequestFactory.java b/jdisc_http_service/src/main/java/com/yahoo/jdisc/http/server/jetty/HttpRequestFactory.java
index 617e081bd24..1306d72a618 100644
--- a/jdisc_http_service/src/main/java/com/yahoo/jdisc/http/server/jetty/HttpRequestFactory.java
+++ b/jdisc_http_service/src/main/java/com/yahoo/jdisc/http/server/jetty/HttpRequestFactory.java
@@ -5,6 +5,7 @@ import com.yahoo.jdisc.Response;
 import com.yahoo.jdisc.http.HttpRequest;
 import com.yahoo.jdisc.http.servlet.ServletRequest;
 import com.yahoo.jdisc.service.CurrentContainer;
+import org.eclipse.jetty.util.URIUtil;
 import org.eclipse.jetty.util.Utf8Appendable;
 
 import javax.servlet.http.HttpServletRequest;
@@ -37,10 +38,17 @@ class HttpRequestFactory {
         }
     }
 
+    // Implementation based on org.eclipse.jetty.server.Request.getRequestURL(), but with getLocalPort() as port
     public static URI getUri(HttpServletRequest servletRequest) {
-        String query = servletRequest.getQueryString();
         try {
-            return URI.create(servletRequest.getRequestURL() + (query != null ? '?' + query : ""));
+            StringBuffer builder = new StringBuffer(128);
+            URIUtil.appendSchemeHostPort(builder, servletRequest.getScheme(), servletRequest.getServerName(), servletRequest.getLocalPort());
+            builder.append(servletRequest.getRequestURI());
+            String query = servletRequest.getQueryString();
+            if (query != null) {
+                builder.append('?').append(query);
+            }
+            return URI.create(builder.toString());
         } catch (IllegalArgumentException e) {
             throw createBadQueryException(e);
         }
diff --git a/jdisc_http_service/src/test/java/com/yahoo/jdisc/http/server/jetty/HttpRequestFactoryTest.java b/jdisc_http_service/src/test/java/com/yahoo/jdisc/http/server/jetty/HttpRequestFactoryTest.java
index 6a2e35b617c..f0203529d4b 100644
--- a/jdisc_http_service/src/test/java/com/yahoo/jdisc/http/server/jetty/HttpRequestFactoryTest.java
+++ b/jdisc_http_service/src/test/java/com/yahoo/jdisc/http/server/jetty/HttpRequestFactoryTest.java
@@ -12,491 +12,31 @@ import com.yahoo.jdisc.service.CurrentContainer;
 import org.eclipse.jetty.server.HttpConnection;
 import org.testng.annotations.Test;
 
-import javax.servlet.AsyncContext;
-import javax.servlet.DispatcherType;
-import javax.servlet.RequestDispatcher;
-import javax.servlet.ServletContext;
-import javax.servlet.ServletException;
-import javax.servlet.ServletInputStream;
-import javax.servlet.ServletRequest;
-import javax.servlet.ServletResponse;
-import javax.servlet.http.Cookie;
 import javax.servlet.http.HttpServletRequest;
-import javax.servlet.http.HttpServletResponse;
-import javax.servlet.http.HttpSession;
-import javax.servlet.http.HttpUpgradeHandler;
-import javax.servlet.http.Part;
-import java.io.BufferedReader;
-import java.io.IOException;
-import java.io.UnsupportedEncodingException;
 import java.net.URI;
-import java.security.Principal;
-import java.util.Collection;
-import java.util.Enumeration;
-import java.util.Locale;
-import java.util.Map;
 
 import static org.hamcrest.CoreMatchers.equalTo;
 import static org.hamcrest.CoreMatchers.is;
 import static org.hamcrest.MatcherAssert.assertThat;
 import static org.mockito.Mockito.mock;
 import static org.mockito.Mockito.when;
+import static org.testng.AssertJUnit.assertEquals;
 import static org.testng.AssertJUnit.fail;
 
 /**
- *
  * @author Steinar Knutsen
+ * @author bjorncs
  */
 public class HttpRequestFactoryTest {
 
-    private static final class MockRequest implements HttpServletRequest {
-        String queryString = null;
-        StringBuffer requestUrl;
-        final String method = "GET";
-        final String protocol = "HTTP/1.1";
-        final String remoteAddr = "127.0.0.1";
-        final int remotePort = 0;
-
-        public MockRequest(String sortOfUri) {
-            int mark = sortOfUri.indexOf('?');
-            if (mark > 0) {
-                queryString = sortOfUri.substring(mark + 1);
-                requestUrl = new StringBuffer(sortOfUri.substring(0, mark));
-            } else {
-                queryString = null;
-                requestUrl = new StringBuffer(sortOfUri);
-            }
-        }
-
-        @Override
-        public Object getAttribute(String name) {
-            switch (name) {
-                case "org.eclipse.jetty.server.HttpConnection":
-                    HttpConnection connection = mock(HttpConnection.class);
-                    when(connection.getCreatedTimeStamp()).thenReturn(System.currentTimeMillis());
-                    return connection;
-                default:
-                    return null;
-            }
-        }
-
-        @Override
-        public Enumeration<String> getAttributeNames() {
-            // TODO Auto-generated method stub
-            return null;
-        }
-
-        @Override
-        public String getCharacterEncoding() {
-            // TODO Auto-generated method stub
-            return null;
-        }
-
-        @Override
-        public void setCharacterEncoding(String env)
-                throws UnsupportedEncodingException {
-            // TODO Auto-generated method stub
-
-        }
-
-        @Override
-        public int getContentLength() {
-            // TODO Auto-generated method stub
-            return 0;
-        }
-
-        @Override
-        public long getContentLengthLong() {
-            // TODO Auto-generated method stub
-            return 0;
-        }
-
-        @Override
-        public String getContentType() {
-            // TODO Auto-generated method stub
-            return null;
-        }
-
-        @Override
-        public ServletInputStream getInputStream() throws IOException {
-            // TODO Auto-generated method stub
-            return null;
-        }
-
-        @Override
-        public String getParameter(String name) {
-            // TODO Auto-generated method stub
-            return null;
-        }
-
-        @Override
-        public Enumeration<String> getParameterNames() {
-            // TODO Auto-generated method stub
-            return null;
-        }
-
-        @Override
-        public String[] getParameterValues(String name) {
-            // TODO Auto-generated method stub
-            return null;
-        }
-
-        @Override
-        public Map<String, String[]> getParameterMap() {
-            // TODO Auto-generated method stub
-            return null;
-        }
-
-        @Override
-        public String getProtocol() {
-            return protocol;
-        }
-
-        @Override
-        public String getScheme() {
-            // TODO Auto-generated method stub
-            return null;
-        }
-
-        @Override
-        public String getServerName() {
-            // TODO Auto-generated method stub
-            return null;
-        }
-
-        @Override
-        public int getServerPort() {
-            // TODO Auto-generated method stub
-            return 0;
-        }
-
-        @Override
-        public BufferedReader getReader() throws IOException {
-            // TODO Auto-generated method stub
-            return null;
-        }
-
-        @Override
-        public String getRemoteAddr() {
-            return remoteAddr;
-        }
-
-        @Override
-        public String getRemoteHost() {
-            // TODO Auto-generated method stub
-            return null;
-        }
-
-        @Override
-        public void setAttribute(String name, Object o) {
-            // TODO Auto-generated method stub
-
-        }
-
-        @Override
-        public void removeAttribute(String name) {
-            // TODO Auto-generated method stub
-
-        }
-
-        @Override
-        public Locale getLocale() {
-            // TODO Auto-generated method stub
-            return null;
-        }
-
-        @Override
-        public Enumeration<Locale> getLocales() {
-            // TODO Auto-generated method stub
-            return null;
-        }
-
-        @Override
-        public boolean isSecure() {
-            // TODO Auto-generated method stub
-            return false;
-        }
-
-        @Override
-        public RequestDispatcher getRequestDispatcher(String path) {
-            // TODO Auto-generated method stub
-            return null;
-        }
-
-        @Override
-        @Deprecated
-        public String getRealPath(String path) {
-            // TODO Auto-generated method stub
-            return null;
-        }
-
-        @Override
-        public int getRemotePort() {
-            return remotePort;
-        }
-
-        @Override
-        public String getLocalName() {
-            // TODO Auto-generated method stub
-            return null;
-        }
-
-        @Override
-        public String getLocalAddr() {
-            // TODO Auto-generated method stub
-            return null;
-        }
-
-        @Override
-        public int getLocalPort() {
-            // TODO Auto-generated method stub
-            return 0;
-        }
-
-        @Override
-        public ServletContext getServletContext() {
-            // TODO Auto-generated method stub
-            return null;
-        }
-
-        @Override
-        public AsyncContext startAsync() throws IllegalStateException {
-            // TODO Auto-generated method stub
-            return null;
-        }
-
-        @Override
-        public AsyncContext startAsync(ServletRequest servletRequest,
-                ServletResponse servletResponse) throws IllegalStateException {
-            // TODO Auto-generated method stub
-            return null;
-        }
-
-        @Override
-        public boolean isAsyncStarted() {
-            // TODO Auto-generated method stub
-            return false;
-        }
-
-        @Override
-        public boolean isAsyncSupported() {
-            // TODO Auto-generated method stub
-            return false;
-        }
-
-        @Override
-        public AsyncContext getAsyncContext() {
-            // TODO Auto-generated method stub
-            return null;
-        }
-
-        @Override
-        public DispatcherType getDispatcherType() {
-            // TODO Auto-generated method stub
-            return null;
-        }
-
-        @Override
-        public String getAuthType() {
-            // TODO Auto-generated method stub
-            return null;
-        }
-
-        @Override
-        public Cookie[] getCookies() {
-            // TODO Auto-generated method stub
-            return null;
-        }
-
-        @Override
-        public long getDateHeader(String name) {
-            // TODO Auto-generated method stub
-            return 0;
-        }
-
-        @Override
-        public String getHeader(String name) {
-            // TODO Auto-generated method stub
-            return null;
-        }
-
-        @Override
-        public Enumeration<String> getHeaders(String name) {
-            // TODO Auto-generated method stub
-            return null;
-        }
-
-        @Override
-        public Enumeration<String> getHeaderNames() {
-            // TODO Auto-generated method stub
-            return null;
-        }
-
-        @Override
-        public int getIntHeader(String name) {
-            // TODO Auto-generated method stub
-            return 0;
-        }
-
-        @Override
-        public String getMethod() {
-            return method;
-        }
-
-        @Override
-        public String getPathInfo() {
-            // TODO Auto-generated method stub
-            return null;
-        }
-
-        @Override
-        public String getPathTranslated() {
-            // TODO Auto-generated method stub
-            return null;
-        }
-
-        @Override
-        public String getContextPath() {
-            // TODO Auto-generated method stub
-            return null;
-        }
-
-        @Override
-        public String getQueryString() {
-            return queryString;
-        }
-
-        @Override
-        public String getRemoteUser() {
-            // TODO Auto-generated method stub
-            return null;
-        }
-
-        @Override
-        public boolean isUserInRole(String role) {
-            // TODO Auto-generated method stub
-            return false;
-        }
-
-        @Override
-        public Principal getUserPrincipal() {
-            // TODO Auto-generated method stub
-            return null;
-        }
-
-        @Override
-        public String getRequestedSessionId() {
-            // TODO Auto-generated method stub
-            return null;
-        }
-
-        @Override
-        public String getRequestURI() {
-            // TODO Auto-generated method stub
-            return null;
-        }
-
-        @Override
-        public StringBuffer getRequestURL() {
-            return requestUrl;
-        }
-
-        @Override
-        public String getServletPath() {
-            // TODO Auto-generated method stub
-            return null;
-        }
-
-        @Override
-        public HttpSession getSession(boolean create) {
-            // TODO Auto-generated method stub
-            return null;
-        }
-
-        @Override
-        public HttpSession getSession() {
-            // TODO Auto-generated method stub
-            return null;
-        }
-
-        @Override
-        public String changeSessionId() {
-            // TODO Auto-generated method stub
-            return null;
-        }
-
-        @Override
-        public boolean isRequestedSessionIdValid() {
-            // TODO Auto-generated method stub
-            return false;
-        }
-
-        @Override
-        public boolean isRequestedSessionIdFromCookie() {
-            // TODO Auto-generated method stub
-            return false;
-        }
-
-        @Override
-        public boolean isRequestedSessionIdFromURL() {
-            // TODO Auto-generated method stub
-            return false;
-        }
-
-        @Override
-        @Deprecated
-        public boolean isRequestedSessionIdFromUrl() {
-            // TODO Auto-generated method stub
-            return false;
-        }
-
-        @Override
-        public boolean authenticate(HttpServletResponse response)
-                throws IOException, ServletException {
-            // TODO Auto-generated method stub
-            return false;
-        }
-
-        @Override
-        public void login(String username, String password)
-                throws ServletException {
-            // TODO Auto-generated method stub
-
-        }
-
-        @Override
-        public void logout() throws ServletException {
-            // TODO Auto-generated method stub
-
-        }
-
-        @Override
-        public Collection<Part> getParts() throws IOException, ServletException {
-            // TODO Auto-generated method stub
-            return null;
-        }
-
-        @Override
-        public Part getPart(String name) throws IOException, ServletException {
-            // TODO Auto-generated method stub
-            return null;
-        }
-
-        @Override
-        public <T extends HttpUpgradeHandler> T upgrade(Class<T> handlerClass)
-                throws IOException, ServletException {
-            // TODO Auto-generated method stub
-            return null;
-        }
-
-    }
+    private static final int LOCAL_PORT = 8080;
 
     @Test
     public final void testIllegalQuery() {
         try {
             HttpRequestFactory.newJDiscRequest(
                     new MockContainer(),
-                    new MockRequest("http://example.com/search?query=\"contains_quotes\""));
+                    createMockRequest("http", "example.com", "/search", "query=\"contains_quotes\""));
             fail("Above statement should throw");
         } catch (RequestException e) {
             assertThat(e.getResponseStatus(), is(Response.Status.BAD_REQUEST));
@@ -508,7 +48,7 @@ public class HttpRequestFactoryTest {
         try {
             HttpRequestFactory.newJDiscRequest(
                     new MockContainer(),
-                    new MockRequest("http://example.com/search?query=%c0%ae"));
+                    createMockRequest("http", "example.com", "/search", "query=%c0%ae"));
             fail("Above statement should throw");
         } catch (RequestException e) {
             assertThat(e.getResponseStatus(), is(Response.Status.BAD_REQUEST));
@@ -516,6 +56,30 @@ public class HttpRequestFactoryTest {
         }
     }
 
+    @Test
+    public void request_uri_uses_local_port() {
+        HttpRequest request = HttpRequestFactory.newJDiscRequest(
+                new MockContainer(),
+                createMockRequest("http", "example.com", "/search", "query=value"));
+        assertEquals(LOCAL_PORT, request.getUri().getPort());
+    }
+
+    private static HttpServletRequest createMockRequest(String scheme, String serverName, String path, String queryString) {
+        HttpServletRequest request = mock(HttpServletRequest.class);
+        HttpConnection connection = mock(HttpConnection.class);
+        when(connection.getCreatedTimeStamp()).thenReturn(System.currentTimeMillis());
+        when(request.getAttribute("org.eclipse.jetty.server.HttpConnection")).thenReturn(connection);
+        when(request.getProtocol()).thenReturn("HTTP/1.1");
+        when(request.getScheme()).thenReturn(scheme);
+        when(request.getServerName()).thenReturn(serverName);
+        when(request.getRemoteAddr()).thenReturn("127.0.0.1");
+        when(request.getRemotePort()).thenReturn(1234);
+        when(request.getLocalPort()).thenReturn(LOCAL_PORT);
+        when(request.getMethod()).thenReturn("GET");
+        when(request.getQueryString()).thenReturn(queryString);
+        when(request.getRequestURI()).thenReturn(path);
+        return request;
+    }
 
     private static final class MockContainer implements CurrentContainer {