Fix RequestDispatcherRequestWrapper inconsistency between `getRequestURI` and `getRequestURL`

2 files changed, 73 insertions, 3 deletions

diff --git a/extensions/servlet/src/com/google/inject/servlet/ManagedServletPipeline.java b/extensions/servlet/src/com/google/inject/servlet/ManagedServletPipeline.java
index 4ca0f2ee..455551a4 100644
--- a/extensions/servlet/src/com/google/inject/servlet/ManagedServletPipeline.java
+++ b/extensions/servlet/src/com/google/inject/servlet/ManagedServletPipeline.java
@@ -128,7 +128,7 @@ class ManagedServletPipeline {
 
             ServletRequest requestToProcess;
             if (servletRequest instanceof HttpServletRequest) {
-               requestToProcess = new RequestDispatcherRequestWrapper(servletRequest, newRequestUri);
+               requestToProcess = wrapRequest((HttpServletRequest)servletRequest, newRequestUri);
             } else {
               // This should never happen, but instead of throwing an exception
               // we will allow a happy case pass thru for maximum tolerance to
@@ -164,6 +164,11 @@ class ManagedServletPipeline {
     return null;
   }
 
+  // visible for testing
+  static HttpServletRequest wrapRequest(HttpServletRequest request, String newUri) {
+    return new RequestDispatcherRequestWrapper(request, newUri);
+  }
+
   /**
    * A Marker constant attribute that when present in the request indicates to Guice servlet that
    * this request has been generated by a request dispatcher rather than the servlet pipeline.
@@ -174,8 +179,8 @@ class ManagedServletPipeline {
   private static class RequestDispatcherRequestWrapper extends HttpServletRequestWrapper {
     private final String newRequestUri;
 
-    public RequestDispatcherRequestWrapper(ServletRequest servletRequest, String newRequestUri) {
-      super((HttpServletRequest) servletRequest);
+    public RequestDispatcherRequestWrapper(HttpServletRequest servletRequest, String newRequestUri) {
+      super(servletRequest);
       this.newRequestUri = newRequestUri;
     }
 
@@ -183,5 +188,26 @@ class ManagedServletPipeline {
     public String getRequestURI() {
       return newRequestUri;
     }
+
+    @Override
+    public StringBuffer getRequestURL() {
+      StringBuffer url = new StringBuffer();
+      String scheme = getScheme();
+      int port = getServerPort();
+
+      url.append(scheme);
+      url.append("://");
+      url.append(getServerName());
+      // port might be -1 in some cases (see java.net.URL.getPort)
+      if (port > 0 &&
+          (("http".equals(scheme) && (port != 80)) ||
+           ("https".equals(scheme) && (port != 443)))) {
+        url.append(':');
+        url.append(port);
+      }
+      url.append(getRequestURI());
+
+      return (url);
+    }
   }
 }
diff --git a/extensions/servlet/test/com/google/inject/servlet/ServletPipelineRequestDispatcherTest.java b/extensions/servlet/test/com/google/inject/servlet/ServletPipelineRequestDispatcherTest.java
index 6343ac55..91abe9cd 100644
--- a/extensions/servlet/test/com/google/inject/servlet/ServletPipelineRequestDispatcherTest.java
+++ b/extensions/servlet/test/com/google/inject/servlet/ServletPipelineRequestDispatcherTest.java
@@ -266,6 +266,50 @@ public class ServletPipelineRequestDispatcherTest extends TestCase {
     finally {
       verify(injector, mockRequest, mockResponse, mockBinding);
     }
+  }
+
+  public final void testWrappedRequestUriAndUrlConsistency() {
+    final HttpServletRequest mockRequest = createMock(HttpServletRequest.class);
+    expect(mockRequest.getScheme()).andReturn("http");
+    expect(mockRequest.getServerName()).andReturn("the.server");
+    expect(mockRequest.getServerPort()).andReturn(12345);
+    replay(mockRequest);
+    HttpServletRequest wrappedRequest = ManagedServletPipeline.wrapRequest(mockRequest, "/new-uri");
+    assertEquals("/new-uri", wrappedRequest.getRequestURI());
+    assertEquals("http://the.server:12345/new-uri", wrappedRequest.getRequestURL().toString());
+  }
+
+  public final void testWrappedRequestUrlNegativePort() {
+    final HttpServletRequest mockRequest = createMock(HttpServletRequest.class);
+    expect(mockRequest.getScheme()).andReturn("http");
+    expect(mockRequest.getServerName()).andReturn("the.server");
+    expect(mockRequest.getServerPort()).andReturn(-1);
+    replay(mockRequest);
+    HttpServletRequest wrappedRequest = ManagedServletPipeline.wrapRequest(mockRequest, "/new-uri");
+    assertEquals("/new-uri", wrappedRequest.getRequestURI());
+    assertEquals("http://the.server/new-uri", wrappedRequest.getRequestURL().toString());
+  }
+
+  public final void testWrappedRequestUrlDefaultPort() {
+    final HttpServletRequest mockRequest = createMock(HttpServletRequest.class);
+    expect(mockRequest.getScheme()).andReturn("http");
+    expect(mockRequest.getServerName()).andReturn("the.server");
+    expect(mockRequest.getServerPort()).andReturn(80);
+    replay(mockRequest);
+    HttpServletRequest wrappedRequest = ManagedServletPipeline.wrapRequest(mockRequest, "/new-uri");
+    assertEquals("/new-uri", wrappedRequest.getRequestURI());
+    assertEquals("http://the.server/new-uri", wrappedRequest.getRequestURL().toString());
+  }
+
 
+  public final void testWrappedRequestUrlDefaultHttpsPort() {
+    final HttpServletRequest mockRequest = createMock(HttpServletRequest.class);
+    expect(mockRequest.getScheme()).andReturn("https");
+    expect(mockRequest.getServerName()).andReturn("the.server");
+    expect(mockRequest.getServerPort()).andReturn(443);
+    replay(mockRequest);
+    HttpServletRequest wrappedRequest = ManagedServletPipeline.wrapRequest(mockRequest, "/new-uri");
+    assertEquals("/new-uri", wrappedRequest.getRequestURI());
+    assertEquals("https://the.server/new-uri", wrappedRequest.getRequestURL().toString());
   }
 }