Merge pull request #60 from awslabs/servlet-improvements

Merging servlet improvements fixes for release 0.7

Author: sapessi

aws-serverless-java-container-core/pom.xml

@@ -73,7 +73,6 @@
             <version>4.5.3</version>
             <scope>test</scope>
         </dependency>
-
     </dependencies>
 
 </project>

aws-serverless-java-container-core/src/main/java/com/amazonaws/serverless/proxy/internal/AwsProxyExceptionHandler.java

@@ -73,6 +73,7 @@ public class AwsProxyExceptionHandler
 
     @Override
     public AwsProxyResponse handle(Throwable ex) {
+        log.error("Called exception handler for:", ex);
         if (ex instanceof InvalidRequestEventException) {
             return new AwsProxyResponse(500, headers, getErrorJson(INTERNAL_SERVER_ERROR));
         } else {

aws-serverless-java-container-core/src/main/java/com/amazonaws/serverless/proxy/internal/model/ContainerConfig.java

@@ -2,13 +2,16 @@
 
 
 /**
- * Configuration paramters used by the <code>RequestReader</code> and <code>ResponseWriter</code> objects.
+ * Configuration parameters for the framework
  */
 public class ContainerConfig {
+    public static final String DEFAULT_URI_ENCODING = "UTF-8";
 
     public static ContainerConfig defaultConfig() {
         ContainerConfig configuration = new ContainerConfig();
         configuration.setStripBasePath(false);
+        configuration.setUriEncoding(DEFAULT_URI_ENCODING);
+        configuration.setConsolidateSetCookieHeaders(true);
 
         return configuration;
     }
@@ -19,6 +22,8 @@ public static ContainerConfig defaultConfig() {
 
     private String serviceBasePath;
     private boolean stripBasePath;
+    private String uriEncoding;
+    private boolean consolidateSetCookieHeaders;
 
 
     //-------------------------------------------------------------
@@ -30,6 +35,11 @@ public String getServiceBasePath() {
     }
 
 
+    /**
+     * Configures a base path  that can be strippped from the request path before passing it to the frameowkr-specific implementation. This can be used to
+     * remove API Gateway's base path mappings from the request.
+     * @param serviceBasePath The base path mapping to be removed.
+     */
     public void setServiceBasePath(String serviceBasePath) {
         // clean up base path before setting it, we want a "/" at the beginning but not at the end.
         String finalBasePath = serviceBasePath;
@@ -48,9 +58,45 @@ public boolean isStripBasePath() {
     }
 
 
+    /**
+     * Whether this framework should strip the base path mapping specified with the {@link #setServiceBasePath(String)} method from a request before
+     * passing it to the framework-specific implementations
+     * @param stripBasePath
+     */
     public void setStripBasePath(boolean stripBasePath) {
         this.stripBasePath = stripBasePath;
     }
 
 
+    public String getUriEncoding() {
+        return uriEncoding;
+    }
+
+
+    /**
+     * Sets the charset used to URLEncode and Decode request paths.
+     * @param uriEncoding The charset. By default this is set to UTF-8
+     */
+    public void setUriEncoding(String uriEncoding) {
+        this.uriEncoding = uriEncoding;
+    }
+
+
+    public boolean isConsolidateSetCookieHeaders() {
+        return consolidateSetCookieHeaders;
+    }
+
+
+    /**
+     * Tells the library to consolidate multiple Set-Cookie headers into a single Set-Cookie header with multiple, comma-separated values. This is allowed
+     * by the RFC 2109 (https://tools.ietf.org/html/rfc2109). However, since not all clients support this, we consider it optional. When this value is set
+     * to true the framework will consolidate all Set-Cookie headers into a single header, when it's set to false, the framework will only return the first
+     * Set-Cookie header specified in a response.
+     *
+     * Because API Gateway needs header keys to be unique, we give an option to configure this.
+     * @param consolidateSetCookieHeaders Whether to consolidate the cookie headers or not.
+     */
+    public void setConsolidateSetCookieHeaders(boolean consolidateSetCookieHeaders) {
+        this.consolidateSetCookieHeaders = consolidateSetCookieHeaders;
+    }
 }

aws-serverless-java-container-core/src/main/java/com/amazonaws/serverless/proxy/internal/servlet/AwsHttpServletRequest.java

@@ -12,6 +12,7 @@
  */
 package com.amazonaws.serverless.proxy.internal.servlet;
 
+import com.amazonaws.serverless.proxy.internal.model.ContainerConfig;
 import com.amazonaws.services.lambda.runtime.Context;
 
 import org.slf4j.Logger;
@@ -24,6 +25,7 @@
 import javax.servlet.http.HttpServletRequest;
 import javax.servlet.http.HttpSession;
 import java.io.UnsupportedEncodingException;
+import java.net.URLDecoder;
 import java.net.URLEncoder;
 import java.nio.charset.StandardCharsets;
 import java.util.AbstractMap;
@@ -320,4 +322,15 @@ protected List<Map.Entry<String, String>> parseHeaderValue(String headerValue) {
         }
         return values;
     }
+
+    protected String decodeRequestPath(String requestPath, ContainerConfig config) {
+        try {
+            return URLDecoder.decode(requestPath, config.getUriEncoding());
+        } catch (UnsupportedEncodingException ex) {
+            log.error("Could not URL decode the request path, configured encoding not supported: " + config.getUriEncoding(), ex);
+            // we do not fail at this.
+            return requestPath;
+        }
+
+    }
 }

aws-serverless-java-container-core/src/main/java/com/amazonaws/serverless/proxy/internal/servlet/AwsHttpServletResponse.java

@@ -12,6 +12,8 @@
  */
 package com.amazonaws.serverless.proxy.internal.servlet;
 
+import com.amazonaws.serverless.proxy.internal.LambdaContainerHandler;
+
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
@@ -94,15 +96,18 @@ public void addCookie(Cookie cookie) {
         if (cookie.getDomain() != null && !"".equals(cookie.getDomain().trim())) {
             cookieData += "; Domain=" + cookie.getDomain();
         }
-        cookieData += "; Max-Age=" + cookie.getMaxAge();
 
-        // we always set the timezone to GMT
-        TimeZone gmtTimeZone = TimeZone.getTimeZone(COOKIE_DEFAULT_TIME_ZONE);
-        Calendar currentTimestamp = Calendar.getInstance(gmtTimeZone);
-        currentTimestamp.add(Calendar.SECOND, cookie.getMaxAge());
-        SimpleDateFormat cookieDateFormatter = new SimpleDateFormat(HEADER_DATE_PATTERN);
-        cookieDateFormatter.setTimeZone(gmtTimeZone);
-        cookieData += "; Expires=" + cookieDateFormatter.format(currentTimestamp.getTime());
+        if (cookie.getMaxAge() > 0) {
+            cookieData += "; Max-Age=" + cookie.getMaxAge();
+
+            // we always set the timezone to GMT
+            TimeZone gmtTimeZone = TimeZone.getTimeZone(COOKIE_DEFAULT_TIME_ZONE);
+            Calendar currentTimestamp = Calendar.getInstance(gmtTimeZone);
+            currentTimestamp.add(Calendar.SECOND, cookie.getMaxAge());
+            SimpleDateFormat cookieDateFormatter = new SimpleDateFormat(HEADER_DATE_PATTERN);
+            cookieDateFormatter.setTimeZone(gmtTimeZone);
+            cookieData += "; Expires=" + cookieDateFormatter.format(currentTimestamp.getTime());
+        }
 
         setHeader(HttpHeaders.SET_COOKIE, cookieData, false);
     }
@@ -410,7 +415,22 @@ byte[] getAwsResponseBodyBytes() {
     Map<String, String> getAwsResponseHeaders() {
         Map<String, String> responseHeaders = new HashMap<>();
         for (String header : getHeaderNames()) {
-            responseHeaders.put(header, headers.getFirst(header));
+            // special behavior for set cookie
+            // RFC 2109 allows for a comma separated list of cookies in one Set-Cookie header: https://tools.ietf.org/html/rfc2109
+            if (header.equals(HttpHeaders.SET_COOKIE) && LambdaContainerHandler.getContainerConfig().isConsolidateSetCookieHeaders()) {
+                StringBuilder cookieHeader = new StringBuilder();
+                for (String cookieValue : headers.get(header)) {
+                    if (cookieHeader.length() > 0) {
+                        cookieHeader.append(",");
+                    }
+
+                    cookieHeader.append(" ").append(cookieValue);
+                }
+
+                responseHeaders.put(header, cookieHeader.toString());
+            } else {
+                responseHeaders.put(header, headers.getFirst(header));
+            }
         }
 
         return responseHeaders;

aws-serverless-java-container-core/src/main/java/com/amazonaws/serverless/proxy/internal/servlet/AwsProxyHttpServletRequest.java

@@ -13,6 +13,7 @@
 package com.amazonaws.serverless.proxy.internal.servlet;
 
 
+import com.amazonaws.serverless.proxy.internal.LambdaContainerHandler;
 import com.amazonaws.serverless.proxy.internal.model.AwsProxyRequest;
 import com.amazonaws.services.lambda.runtime.Context;
 
@@ -184,7 +185,7 @@ public String getPathInfo() {
         if (!pathInfo.startsWith("/")) {
             pathInfo = "/" + pathInfo;
         }
-        return pathInfo;
+        return decodeRequestPath(pathInfo, LambdaContainerHandler.getContainerConfig());
     }
 
 
@@ -248,7 +249,7 @@ public StringBuffer getRequestURL() {
 
     @Override
     public String getServletPath() {
-        return request.getPath();
+        return decodeRequestPath(request.getPath(), LambdaContainerHandler.getContainerConfig());
     }
 
     @Override

aws-serverless-java-container-core/src/main/java/com/amazonaws/serverless/proxy/internal/servlet/FilterChainHolder.java

@@ -12,6 +12,9 @@
  */
 package com.amazonaws.serverless.proxy.internal.servlet;
 
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
 import javax.servlet.FilterChain;
 import javax.servlet.ServletException;
 import javax.servlet.ServletRequest;
@@ -33,7 +36,9 @@ public class FilterChainHolder implements FilterChain {
     //-------------------------------------------------------------
 
     private List<FilterHolder> filters;
-    private int currentFilter;
+    int currentFilter;
+
+    private Logger log = LoggerFactory.getLogger(FilterChainHolder.class);
 
 
     //-------------------------------------------------------------
@@ -43,7 +48,7 @@ public class FilterChainHolder implements FilterChain {
     /**
      * Creates a new empty <code>FilterChainHolder</code>
      */
-    public FilterChainHolder() {
+    FilterChainHolder() {
         this(new ArrayList<>());
     }
 
@@ -52,9 +57,9 @@ public FilterChainHolder() {
      * Creates a new instance of a filter chain holder
      * @param allFilters A populated list of <code>FilterHolder</code> objects
      */
-    public FilterChainHolder(List<FilterHolder> allFilters) {
+    FilterChainHolder(List<FilterHolder> allFilters) {
         filters = allFilters;
-        currentFilter = -1;
+        resetHolder();
     }
 
 
@@ -66,16 +71,20 @@ public FilterChainHolder(List<FilterHolder> allFilters) {
     public void doFilter(ServletRequest servletRequest, ServletResponse servletResponse) throws IOException, ServletException {
         currentFilter++;
         if (filters == null || filters.size() == 0 || currentFilter > filters.size() - 1) {
+            log.debug("Could not find filters to execute, returning");
             return;
         }
         // TODO: We do not check for async filters here
 
         FilterHolder holder = filters.get(currentFilter);
 
+        // lazily initialize filters when they are needed
         if (!holder.isFilterInitialized()) {
             holder.init();
         }
+        log.debug("Starting filter " + holder.getFilterName());
         holder.getFilter().doFilter(servletRequest, servletResponse, this);
+        log.debug("Executed filter " + holder.getFilterName());
     }
 
 
@@ -87,7 +96,7 @@ public void doFilter(ServletRequest servletRequest, ServletResponse servletRespo
      * Add a filter to the chain.
      * @param newFilter The filter to be added at the end of the chain
      */
-    public void addFilter(FilterHolder newFilter) {
+    void addFilter(FilterHolder newFilter) {
         filters.add(newFilter);
     }
 
@@ -96,7 +105,7 @@ public void addFilter(FilterHolder newFilter) {
      * Returns the number of filters loaded in the chain holder
      * @return The number of filters in the chain holder. If the filter chain is null then this will return 0
      */
-    public int filterCount() {
+    int filterCount() {
         if (filters == null) {
             return 0;
         } else {
@@ -110,11 +119,29 @@ public int filterCount() {
      * @param idx The index in the chain. Use the <code>filterCount</code> method to get the filter count
      * @return A populated FilterHolder object
      */
-    public FilterHolder getFilter(int idx) {
+    FilterHolder getFilter(int idx) {
         if (filters == null) {
             return null;
         } else {
             return filters.get(idx);
         }
     }
+
+
+    /**
+     * Returns the list of filters in this chain.
+     * @return The list of filters
+     */
+    public List<FilterHolder> getFilters() {
+        return filters;
+    }
+
+
+    /**
+     * Resets the chain holder to the beginning of the filter chain. This method is used from the constructor as well as when
+     * the {@link FilterChainManager} return a holder from the cache.
+     */
+    private void resetHolder() {
+        currentFilter = -1;
+    }
 }