Snapshot cookies in continuing request to prevent mutation side effects
diff --git a/extensions/servlet/src/com/google/inject/servlet/ContinuingHttpServletRequest.java b/extensions/servlet/src/com/google/inject/servlet/ContinuingHttpServletRequest.java
index 36c3477..9c473bb 100644
--- a/extensions/servlet/src/com/google/inject/servlet/ContinuingHttpServletRequest.java
+++ b/extensions/servlet/src/com/google/inject/servlet/ContinuingHttpServletRequest.java
@@ -38,9 +38,29 @@
// We clear out the attributes as they are mutable and not thread-safe.
private final Map<String, Object> attributes = Maps.newHashMap();
+ private final Cookie[] cookies;
public ContinuingHttpServletRequest(HttpServletRequest request) {
super(request);
+
+ Cookie[] originalCookies = request.getCookies();
+ if (originalCookies != null) {
+ int numberOfCookies = originalCookies.length;
+ cookies = new Cookie[numberOfCookies];
+ for (int i = 0; i < numberOfCookies; i++) {
+ Cookie originalCookie = originalCookies[i];
+
+ // Snapshot each cookie + freeze.
+ // No snapshot is required if this is a snapshot of a snapshot(!)
+ if (originalCookie instanceof ImmutableCookie) {
+ cookies[i] = originalCookie;
+ } else {
+ cookies[i] = new ImmutableCookie(originalCookie);
+ }
+ }
+ } else {
+ cookies = null;
+ }
}
@Override public HttpSession getSession() {
@@ -68,10 +88,56 @@
}
@Override public Cookie[] getCookies() {
- if (super.getCookies() == null) {
- return null;
+ // NOTE(dhanji): Cookies themselves are mutable. However a ContinuingHttpServletRequest
+ // snapshots the original set of cookies it received and imprisons them in immutable
+ // form. Unfortunately, the cookie array itself is mutable and there is no way for us
+ // to avoid this. At worst, however, mutation effects are restricted within the scope
+ // of a single request. Continued requests are not affected after snapshot time.
+ return cookies;
+ }
+
+ private static final class ImmutableCookie extends Cookie {
+ public ImmutableCookie(Cookie original) {
+ super(original.getName(), original.getValue());
+
+ super.setMaxAge(original.getMaxAge());
+ super.setPath(original.getPath());
+ super.setComment(original.getComment());
+ super.setSecure(original.getSecure());
+ super.setValue(original.getValue());
+ super.setVersion(original.getVersion());
+
+ if (original.getDomain() != null) {
+ super.setDomain(original.getDomain());
+ }
}
- // TODO(dhanji): Cookies themselves are mutable. Is this a problem?
- return super.getCookies().clone();
+
+ @Override public void setComment(String purpose) {
+ throw new UnsupportedOperationException("Cannot modify cookies on a continued request");
+ }
+
+ @Override public void setDomain(String pattern) {
+ throw new UnsupportedOperationException("Cannot modify cookies on a continued request");
+ }
+
+ @Override public void setMaxAge(int expiry) {
+ throw new UnsupportedOperationException("Cannot modify cookies on a continued request");
+ }
+
+ @Override public void setPath(String uri) {
+ throw new UnsupportedOperationException("Cannot modify cookies on a continued request");
+ }
+
+ @Override public void setSecure(boolean flag) {
+ throw new UnsupportedOperationException("Cannot modify cookies on a continued request");
+ }
+
+ @Override public void setValue(String newValue) {
+ throw new UnsupportedOperationException("Cannot modify cookies on a continued request");
+ }
+
+ @Override public void setVersion(int v) {
+ throw new UnsupportedOperationException("Cannot modify cookies on a continued request");
+ }
}
}
diff --git a/extensions/servlet/test/com/google/inject/servlet/ContinuingHttpServletRequestTest.java b/extensions/servlet/test/com/google/inject/servlet/ContinuingHttpServletRequestTest.java
index 6063cb8..65a6333 100644
--- a/extensions/servlet/test/com/google/inject/servlet/ContinuingHttpServletRequestTest.java
+++ b/extensions/servlet/test/com/google/inject/servlet/ContinuingHttpServletRequestTest.java
@@ -15,6 +15,7 @@
*/
package com.google.inject.servlet;
+import junit.framework.AssertionFailedError;
import static org.easymock.EasyMock.createMock;
import static org.easymock.EasyMock.expect;
import static org.easymock.EasyMock.replay;
@@ -50,9 +51,48 @@
replay(delegate);
- assertTrue(Arrays.equals(cookies,
- new ContinuingHttpServletRequest(delegate).getCookies()));
+ ContinuingHttpServletRequest continuingRequest = new ContinuingHttpServletRequest(
+ delegate);
+
+ assertCookieArraysEqual(cookies, continuingRequest.getCookies());
+
+ // Now mutate the original cookies, this shouldnt be reflected in the continued request.
+ cookies[0].setValue("INVALID");
+ cookies[1].setValue("INVALID");
+ cookies[1].setMaxAge(123);
+
+ try {
+ assertCookieArraysEqual(cookies, continuingRequest.getCookies());
+ fail();
+ } catch (AssertionFailedError e) {
+ // Expected.
+ }
+
+ // Perform a snapshot of the snapshot.
+ ContinuingHttpServletRequest furtherContinuingRequest = new ContinuingHttpServletRequest(
+ continuingRequest);
+
+ // The cookies should be fixed.
+ assertCookieArraysEqual(continuingRequest.getCookies(), furtherContinuingRequest.getCookies());
verify(delegate);
}
-}
\ No newline at end of file
+
+ private static void assertCookieArraysEqual(Cookie[] one, Cookie[] two) {
+ assertEquals(one.length, two.length);
+ for (int i = 0; i < one.length; i++) {
+ Cookie cookie = one[i];
+ assertCookiequality(cookie, two[i]);
+ }
+ }
+
+ private static void assertCookiequality(Cookie one, Cookie two) {
+ assertEquals(one.getName(), two.getName());
+ assertEquals(one.getComment(), two.getComment());
+ assertEquals(one.getDomain(), two.getDomain());
+ assertEquals(one.getPath(), two.getPath());
+ assertEquals(one.getValue(), two.getValue());
+ assertEquals(one.getMaxAge(), two.getMaxAge());
+ assertEquals(one.getSecure(), two.getSecure());
+ }
+}