Fix RequestDispatcherRequestWrapper inconsistency between `getRequestURI` and `getRequestURL`
diff --git a/extensions/servlet/src/com/google/inject/servlet/ManagedServletPipeline.java b/extensions/servlet/src/com/google/inject/servlet/ManagedServletPipeline.java
index 4ca0f2e..455551a 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 @@
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 @@
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 @@
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 @@
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 6343ac5..91abe9c 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 @@
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());
}
}
