Merge pull request #240 from dimas/master

Make AccessEvent.prepareForDeferredProcessing() to create a copy of the ...

logback-access/src/main/java/ch/qos/logback/access/spi/AccessEvent.java

@@ -65,6 +65,7 @@ public class AccessEvent implements Serializable, IAccessEvent {
   Map<String, String> requestHeaderMap;
   Map<String, String[]> requestParameterMap;
   Map<String, String> responseHeaderMap;
+  Map<String, Object> attributeMap;
 
   long contentLength = SENTINEL;
   int statusCode = SENTINEL;
@@ -292,21 +293,49 @@ public Map<String, String[]> getRequestParameterMap() {
     return requestParameterMap;
   }
 
-  /**
-   * Attributes are not serialized
-   *
-   * @param key
-   */
   public String getAttribute(String key) {
-    if (httpRequest != null) {
-      Object value = httpRequest.getAttribute(key);
-      if (value == null) {
-        return NA;
-      } else {
-        return value.toString();
+    Object value = null;
+    if (attributeMap != null) {
+      // Event was prepared for deferred processing so we have a copy of attribute map and must use that copy
+      value = attributeMap.get(key);
+    } else if (httpRequest != null) {
+      // We have original request so take attribute from it
+      value = httpRequest.getAttribute(key);
+    }
+
+    return value != null ? value.toString() : NA;
+  }
+
+  private void copyAttributeMap() {
+
+    if (httpRequest == null) {
+      return;
+    }
+
+    attributeMap = new HashMap<String, Object>();
+
+    Enumeration<String> names = httpRequest.getAttributeNames();
+    while (names.hasMoreElements()) {
+      String name = names.nextElement();
+
+      Object value = httpRequest.getAttribute(name);
+      if (shouldCopyAttribute(name, value)) {
+        attributeMap.put(name, value);
       }
+    }
+  }
+
+  private boolean shouldCopyAttribute(String name, Object value) {
+    if (AccessConstants.LB_INPUT_BUFFER.equals(name) || AccessConstants.LB_OUTPUT_BUFFER.equals(name)) {
+      // Do not copy attributes used by logback internally - these are available via other getters anyway
+      return false;
+    } else if (value == null) {
+      // No reasons to copy nulls - Map.get() will return null for missing keys and the list of attribute
+      // names is not available through IAccessEvent
+      return false;
     } else {
-      return NA;
+      // Only copy what is serializable
+      return value instanceof Serializable;
     }
   }
 
@@ -500,5 +529,7 @@ public void prepareForDeferredProcessing() {
     getContentLength();
     getRequestContent();
     getResponseContent();
+
+    copyAttributeMap();
   }
 }

logback-access/src/main/java/ch/qos/logback/access/spi/IAccessEvent.java

@@ -92,11 +92,6 @@ public interface IAccessEvent extends DeferredProcessingAware {
 
   Map<String, String[]> getRequestParameterMap();
 
-  /**
-   * Attributes are not serialized
-   *
-   * @param key
-   */
   String getAttribute(String key);
 
   String[] getRequestParameter(String key);

logback-access/src/test/java/ch/qos/logback/access/dummy/DummyRequest.java

@@ -13,33 +13,42 @@
  */
 package ch.qos.logback.access.dummy;
 
+import ch.qos.logback.access.AccessConstants;
+
+import javax.servlet.*;
+import javax.servlet.http.*;
 import java.io.BufferedReader;
 import java.io.IOException;
 import java.io.UnsupportedEncodingException;
 import java.security.Principal;
 import java.util.*;
 
-import javax.servlet.*;
-import javax.servlet.http.*;
-
-import ch.qos.logback.access.AccessConstants;
-
 public class DummyRequest implements HttpServletRequest {
 
   public final static String  DUMMY_CONTENT_STRING = "request contents";
-  public final static byte[] DUMMY_CONTENT_BYTES = DUMMY_CONTENT_STRING.getBytes(); 
+  public final static byte[] DUMMY_CONTENT_BYTES = DUMMY_CONTENT_STRING.getBytes();
+
+  public static final Map<String, Object> DUMMY_DEFAULT_ATTR_MAP = new HashMap<String, Object>();
 
-
   public static final String DUMMY_RESPONSE_CONTENT_STRING = "response contents";
   public static final byte[] DUMMY_RESPONSE_CONTENT_BYTES =DUMMY_RESPONSE_CONTENT_STRING.getBytes();
 
   Hashtable<String, String> headerNames;
   String uri;
+  Map<String, Object> attributes;
+
+  static {
+    DUMMY_DEFAULT_ATTR_MAP.put("testKey", "testKey");
+    DUMMY_DEFAULT_ATTR_MAP.put(AccessConstants.LB_INPUT_BUFFER, DUMMY_CONTENT_BYTES);
+    DUMMY_DEFAULT_ATTR_MAP.put(AccessConstants.LB_OUTPUT_BUFFER, DUMMY_RESPONSE_CONTENT_BYTES);
+  }
 
   public DummyRequest() {
     headerNames = new Hashtable<String, String>();
     headerNames.put("headerName1", "headerValue1");
     headerNames.put("headerName2", "headerValue2");
+
+    attributes = new HashMap<String, Object>(DUMMY_DEFAULT_ATTR_MAP);
   }
 
   public String getAuthType() {
@@ -164,19 +173,11 @@ public boolean isUserInRole(String arg0) {
   }
 
   public Object getAttribute(String key) {
-    if (key.equals("testKey")) {
-      return "testKey";
-    } else if (AccessConstants.LB_INPUT_BUFFER.equals(key)) {
-      return DUMMY_CONTENT_BYTES;
-    } else if (AccessConstants.LB_OUTPUT_BUFFER.equals(key)) {
-      return DUMMY_RESPONSE_CONTENT_BYTES;
-    } else {
-      return null;
-    }
+    return attributes.get(key);
   }
 
   public Enumeration getAttributeNames() {
-    return null;
+    return Collections.enumeration(attributes.keySet());
   }
 
   public String getCharacterEncoding() {
@@ -306,7 +307,8 @@ public boolean isSecure() {
   public void removeAttribute(String arg0) {
   }
 
-  public void setAttribute(String arg0, Object arg1) {
+  public void setAttribute(String name, Object value) {
+    attributes.put(name, value);
   }
 
   public void setCharacterEncoding(String arg0)

logback-access/src/test/java/ch/qos/logback/access/spi/AccessEventSerializationTest.java

@@ -13,20 +13,16 @@
  */
 package ch.qos.logback.access.spi;
 
-import static org.junit.Assert.assertEquals;
-import static org.junit.Assert.assertNotNull;
-
-import java.io.ByteArrayInputStream;
-import java.io.ByteArrayOutputStream;
-import java.io.IOException;
-import java.io.ObjectInputStream;
-import java.io.ObjectOutputStream;
-
-import org.junit.Test;
-
 import ch.qos.logback.access.dummy.DummyAccessEventBuilder;
 import ch.qos.logback.access.dummy.DummyRequest;
 import ch.qos.logback.access.dummy.DummyResponse;
+import ch.qos.logback.access.dummy.DummyServerAdapter;
+import org.junit.Test;
+
+import java.io.*;
+
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertNotNull;
 
 public class AccessEventSerializationTest  {
 
@@ -70,7 +66,30 @@ public void testSerialization() throws IOException, ClassNotFoundException {
 
     assertEquals(DummyRequest.DUMMY_RESPONSE_CONTENT_STRING, aeBack
         .getResponseContent());
-
+
+    assertEquals(DummyRequest.DUMMY_DEFAULT_ATTR_MAP.get("testKey"), aeBack
+        .getAttribute("testKey"));
   }
 
+  // Web containers may (and will) recycle requests objects. So we must make sure that after
+  // we prepared an event for deferred processing it won't be using data from the original
+  // HttpRequest object which may at that time represent another request
+  @Test
+  public void testAttributesAreNotTakenFromRecycledRequestWhenProcessingDeferred() {
+
+    DummyRequest request = new DummyRequest();
+    DummyResponse response = new DummyResponse();
+    DummyServerAdapter adapter = new DummyServerAdapter(request, response);
+
+    IAccessEvent event = new AccessEvent(request, response, adapter);
+
+    request.setAttribute("testKey", "ORIGINAL");
+
+    event.prepareForDeferredProcessing();
+
+    request.setAttribute("testKey", "NEW");
+
+    // Event should capture the original value
+    assertEquals("ORIGINAL", event.getAttribute("testKey"));
+  }
 }