Migrate Serverless-java-container to aws-lambda-java-events #584

mbfreder · 2023-06-11T21:16:08Z

Issue #, if available:

Description of changes:

Migrated request and response classes for APIGateway Rest Api, Http Api and ALB to their equivalents in aws-lambda-java-events library
Update tests to be compatible with aws-lambda-java-events classes.

By submitting this pull request

I confirm that my contribution is made under the terms of the Apache 2.0 license.
I confirm that I've made a best effort attempt to update all relevant documentation.

sapessi · 2023-06-11T21:19:43Z

We identified new issues on unchanged lines of code. Navigate to the Amazon CodeGuru Reviewer console to view the recommendations to fix them.

sapessi · 2023-06-11T21:19:45Z

.../src/main/java/com/amazonaws/serverless/proxy/internal/servlet/AwsAlbHttpServletRequest.java

+import java.util.stream.Collectors;
+import java.util.stream.Stream;
+
+public class AwsAlbHttpServletRequest extends AwsHttpServletRequest{


Recommendation generated by Amazon CodeGuru Reviewer. Leave feedback on this recommendation by replying to the comment or by reacting to the comment using emoji.

This class references 46 other classes. By comparison, 98% of the classes in the CodeGuru reference dataset reference fewer. This indicates that this class is highly coupled with other classes. A class that is highly coupled with other classes is difficult to understand and its behavior might change unexpectedly when one of its referenced classes is updated. High coupling could also increase the integration test complexity, maintenance cost and technical debt. We recommend that you simplify this class or break it into multiple classes.

I think breaking this class into multiple classes will add more confusion and make it difficult to understand. This implementation is quite similar to already existing classes like AwsHttpApiV2ProxyHttpServletRequest and AwsProxyHttpServletRequest.

Can we reduce duplicate code? There is a lot copy/paste from AwsProxyHttpServletRequest. We can run mvn pmd:cpd for a report.

roger-zhangg · 2023-06-12T18:29:11Z

...src/main/java/com/amazonaws/serverless/proxy/internal/jaxrs/AwsHttpApiV2SecurityContext.java

@@ -94,7 +94,7 @@ public String getAuthenticationScheme() {
        if (event.getRequestContext().getAuthorizer() == null) {
            return null;
        }
-        if (event.getRequestContext().getAuthorizer().isJwt()) {
+        if (event.getRequestContext().getAuthorizer().getJwt() != null) { //TODO: Confirm


Check out the TODO here?

Updated. Thanks Roger.

roger-zhangg · 2023-06-12T18:29:35Z

...ore/src/main/java/com/amazonaws/serverless/proxy/internal/jaxrs/AwsProxySecurityContext.java

-                    case ALB:
-                        return event.getMultiValueHeaders().getFirst(ALB_IDENTITY_HEADER);
-                    }
+                    return event.getRequestContext().getAuthorizer().get("principalId").toString(); // TODO: Check later


roger-zhangg · 2023-06-12T18:29:49Z

...ain/java/com/amazonaws/serverless/proxy/internal/servlet/AwsAlbHttpServletRequestReader.java

+        }
+
+        request.setPath(stripBasePath(request.getPath(), config));
+        if (request.getMultiValueHeaders() != null && request.getMultiValueHeaders().get(HttpHeaders.CONTENT_TYPE) != null && request.getMultiValueHeaders().get(HttpHeaders.CONTENT_TYPE).get(0) != null) {   //TODO: check


roger-zhangg · 2023-06-12T18:30:03Z

...rc/main/java/com/amazonaws/serverless/proxy/internal/servlet/AwsProxyHttpServletRequest.java

@@ -195,7 +194,7 @@ public String getQueryString() {
            return this.generateQueryString(
                    request.getMultiValueQueryStringParameters(),
                    // ALB does not automatically decode parameters, so we don't want to re-encode them
-                    request.getRequestSource() != RequestSource.ALB,
+                    true, //request.getRequestSource() != RequestSource.ALB, TODO: check


roger-zhangg · 2023-06-12T18:30:14Z

...n/java/com/amazonaws/serverless/proxy/internal/servlet/AwsProxyHttpServletRequestReader.java

@@ -49,17 +52,17 @@ public HttpServletRequest readRequest(AwsProxyRequest request, SecurityContext s
        }

        request.setPath(stripBasePath(request.getPath(), config));
-        if (request.getMultiValueHeaders() != null && request.getMultiValueHeaders().getFirst(HttpHeaders.CONTENT_TYPE) != null) {
-            String contentType = request.getMultiValueHeaders().getFirst(HttpHeaders.CONTENT_TYPE);
+        if (request.getMultiValueHeaders() != null && request.getMultiValueHeaders().get(HttpHeaders.CONTENT_TYPE) != null && request.getMultiValueHeaders().get(HttpHeaders.CONTENT_TYPE).get(0) != null) {   //TODO: check


roger-zhangg · 2023-06-12T18:30:20Z

...n/java/com/amazonaws/serverless/proxy/internal/servlet/AwsProxyHttpServletRequestReader.java

        }
        AwsProxyHttpServletRequest servletRequest = new AwsProxyHttpServletRequest(request, lambdaContext, securityContext, config);
        servletRequest.setServletContext(servletContext);
        servletRequest.setAttribute(API_GATEWAY_CONTEXT_PROPERTY, request.getRequestContext());
        servletRequest.setAttribute(API_GATEWAY_STAGE_VARS_PROPERTY, request.getStageVariables());
        servletRequest.setAttribute(API_GATEWAY_EVENT_PROPERTY, request);
-        servletRequest.setAttribute(ALB_CONTEXT_PROPERTY, request.getRequestContext().getElb());
+        //servletRequest.setAttribute(ALB_CONTEXT_PROPERTY, request.getRequestContext().getElb());  TODO: elb


roger-zhangg · 2023-06-12T18:30:31Z

...a-container-core/src/test/java/com/amazonaws/serverless/proxy/model/AwsProxyRequestTest.java


-        assertThat(req.getHeaders().get("accept"), is("*"));
+        assertThat(req.getHeaders().get("accept"), is("*")); //TODO: Move this to alb specific test file, if any.


I think I left many of these TODOs after my initial changes. Some were around ALB references that wasn't clear what was the best option at the time

deki · 2023-06-14T06:32:12Z

.../src/main/java/com/amazonaws/serverless/proxy/internal/servlet/AwsAlbHttpServletRequest.java

+import java.util.stream.Collectors;
+import java.util.stream.Stream;
+
+public class AwsAlbHttpServletRequest extends AwsHttpServletRequest{


Can we reduce duplicate code? There is a lot copy/paste from AwsProxyHttpServletRequest. We can run mvn pmd:cpd for a report.

deki · 2023-06-14T06:33:35Z

...ore/src/main/java/com/amazonaws/serverless/proxy/internal/jaxrs/AwsProxySecurityContext.java

-                    case ALB:
-                        return event.getMultiValueHeaders().getFirst(ALB_IDENTITY_HEADER);
-                    }
+                    return event.getRequestContext().getAuthorizer().get("principalId").toString();


Please define a constant for principalId (ideally it should be provided by aws-lambda-events lib).

deki · 2023-06-14T06:33:47Z

...ore/src/main/java/com/amazonaws/serverless/proxy/internal/jaxrs/AwsProxySecurityContext.java

                return null;
            };
        }

        if (getAuthenticationScheme().equals(AUTH_SCHEME_COGNITO_POOL)) {
-            return new CognitoUserPoolPrincipal(event.getRequestContext().getAuthorizer().getClaims());
+            return new CognitoUserPoolPrincipal((Map<String, String>) event.getRequestContext().getAuthorizer().get("claims"));


Please define a constant (ideally it should be provided by aws-lambda-events lib).

deki · 2023-06-14T06:34:00Z

...ore/src/main/java/com/amazonaws/serverless/proxy/internal/jaxrs/AwsProxySecurityContext.java

-            if (event.getMultiValueHeaders().containsKey(ALB_ACESS_TOKEN_HEADER)) {
-                return AUTH_SCHEME_CUSTOM;
-            }
+        if (event.getRequestContext().getAuthorizer() != null && ((Map<String, String>) event.getRequestContext().getAuthorizer().get("claims")).get("sub") != null) {


use constants

deki · 2023-06-14T06:34:12Z

...ore/src/main/java/com/amazonaws/serverless/proxy/internal/jaxrs/AwsProxySecurityContext.java

            claims = c;
        }

        @Override
        public String getName() {
-            return claims.getSubject();
+            return claims.get("sub");


use constant

deki · 2023-06-14T06:35:26Z

.../src/main/java/com/amazonaws/serverless/proxy/internal/servlet/AwsAlbHttpServletRequest.java

+        }
+
+        return new StringBuilder().append("null")
+                .append(".execute-api.")


this is for API-Gateway but not for ALB

I used the same logic and tweaked it to make this test pass, since ALB's request context doesn't have an API ID. Is there a better way to do it?

deki · 2023-06-14T06:38:52Z

...java-container-core/src/main/java/com/amazonaws/serverless/proxy/model/AwsProxyResponse.java

@@ -16,6 +16,7 @@
 import com.fasterxml.jackson.annotation.JsonInclude;
 import com.fasterxml.jackson.annotation.JsonProperty;

+import java.util.List;


AwsProxyResponse should be moved to src/test/java

deki · 2023-06-14T06:43:44Z

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

@@ -17,7 +17,7 @@
    <properties>
        <spring.version>6.0.9</spring.version>
        <springboot.version>3.1.0</springboot.version>
-        <springsecurity.version>6.1.0</springsecurity.version>
+        <springsecurity.version>6.0.3</springsecurity.version>


this downgrade seems to be a merge error

Yes. Thanks for pointing that out.

sapessi · 2023-06-20T12:45:43Z

...c/main/java/com/amazonaws/serverless/proxy/internal/servlet/AwsHttpServletRequestHelper.java

+     * @param valueSeparator The separator to be used for parsing header values
+     * @return A list of SimpleMapEntry objects with all of the possible values for the header.
+     */
+    protected static List<AwsHttpServletRequest.HeaderValue> parseHeaderValue(String headerValue, String valueSeparator, String qualifierSeparator) {


Recommendation generated by Amazon CodeGuru Reviewer. Leave feedback on this recommendation by replying to the comment or by reacting to the comment using emoji.

The cyclomatic complexity of this method is 13. By comparison, 98% of the methods in the CodeGuru reference dataset have a lower cyclomatic complexity. This indicates the method has a high number of decisions and it can make the logic difficult to understand and test.

We recommend that you simplify this method or break it into multiple methods. For example, consider extracting the code block on lines 439-447 into a separate method.

sapessi · 2023-06-20T12:45:43Z

...c/main/java/com/amazonaws/serverless/proxy/internal/servlet/AwsHttpServletRequestHelper.java

+import static com.amazonaws.serverless.proxy.internal.servlet.AwsHttpServletRequest.cleanUri;
+import static com.amazonaws.serverless.proxy.internal.servlet.AwsHttpServletRequest.decodeRequestPath;
+
+public class AwsHttpServletRequestHelper {


Recommendation generated by Amazon CodeGuru Reviewer. Leave feedback on this recommendation by replying to the comment or by reacting to the comment using emoji.

This class references 45 other classes. By comparison, 98% of the classes in the CodeGuru reference dataset reference fewer. This indicates that this class is highly coupled with other classes. A class that is highly coupled with other classes is difficult to understand and its behavior might change unexpectedly when one of its referenced classes is updated. High coupling could also increase the integration test complexity, maintenance cost and technical debt. We recommend that you simplify this class or break it into multiple classes.

deki

Could you please rebase on main?

We could use the opportunity and move some of the helping methods to https://github.com/aws/aws-lambda-java-libs/tree/events-v4-serialization-v2 so that other users of that library benefit from it as well. In addition this would reduce the amount of code here.

deki · 2023-06-28T14:05:32Z

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

@@ -169,7 +176,7 @@
                <version>${dependencyCheck.version}</version>
                <configuration>
                    <skipProvidedScope>true</skipProvidedScope>
-                    <failBuildOnCVSS>7</failBuildOnCVSS>
+                    <failBuildOnCVSS>8</failBuildOnCVSS>


The issue was related to a false positive. The CVE has been updated so it's no longer required.

Hmm just noticed https://nvd.nist.gov/vuln/detail/CVE-2023-35116 came back today. FasterXML/jackson-databind#3972 is closed.
Let me add a suppression for it...

deki · 2023-06-28T14:14:15Z

.../src/main/java/com/amazonaws/serverless/proxy/internal/servlet/AwsAlbHttpServletRequest.java

+
+    @Override
+    public Enumeration<String> getHeaders(String s) {
+        return AwsHttpServletRequestHelper.getHeaders(request.getMultiValueHeaders(), s);


Both AwsAlbHttpServletRequest and AwsProxyHttpServletRequest extend AwsHttpServletRequest. Instead of overriding all methods twice in subclasses and delegating to the helper, this should be done in AwsHttpServletRequest directly.

AwsHttpServletRequest doesn't have access to the underlying request objects. I can't think of a way to move the methods there if I can't access the request objects (APIGatewayProxyRequestEvent and ApplicationLoadBalancerRequestEvent).

deki · 2023-06-28T14:16:14Z

samples/springboot3/pet-store/template.yml

@@ -22,11 +22,11 @@ Resources:
          Type: HttpApi
          Properties:
            TimeoutInMillis: 20000
-            PayloadFormatVersion: '1.0'
+        # PayloadFormatVersion: '1.0'


is that intended?

No, updated.

deki · 2023-06-28T14:17:17Z

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

@@ -265,7 +265,7 @@
                <version>${dependencyCheck.version}</version>
                <configuration>
                    <skipProvidedScope>true</skipProvidedScope>
-                    <failBuildOnCVSS>7</failBuildOnCVSS>
+                    <failBuildOnCVSS>8</failBuildOnCVSS>


HttpApiV2ProxyRequest -> APIGatewayV2HTTPEvent

Migration to aws-lambda-java-events

These frameworks are not supported by 2.0, but we remove references to HttpApiV2ProxyRequest anyway.

- Replaced string literals with constants

…ind vulnerability - Created a helper class to reduce duplicated code in AwsAlbHttpServletRequest

mbfreder · 2023-06-28T23:05:46Z

Could you please rebase on main?

We could use the opportunity and move some of the helping methods to https://github.com/aws/aws-lambda-java-libs/tree/events-v4-serialization-v2 so that other users of that library benefit from it as well. In addition this would reduce the amount of code here.

Rebased on main.
Question: Which helping methods are you referring to exactly? I think we could benefit more by having a parent classes for the request and response classes in the java-events library.

sapessi reviewed Jun 11, 2023

View reviewed changes

mbfreder force-pushed the java-events branch 2 times, most recently from 8430ff4 to a71db17 Compare June 11, 2023 22:41

roger-zhangg reviewed Jun 12, 2023

View reviewed changes

mbfreder force-pushed the java-events branch from 6d816dc to 8d0882e Compare June 12, 2023 22:28

deki requested changes Jun 14, 2023

View reviewed changes

sapessi reviewed Jun 20, 2023

View reviewed changes

mbfreder force-pushed the java-events branch from 98b92ef to 8031f54 Compare June 20, 2023 13:09

mbfreder requested a review from deki June 21, 2023 20:52

deki requested changes Jun 28, 2023

View reviewed changes

valerena and others added 15 commits June 28, 2023 15:12

Migrate to aws-lambda-java-events

33a9812

HttpApiV2ProxyRequest -> APIGatewayV2HTTPEvent

Delete HttpApiV2ProxyRequest and update tests

05e756a

Migration to aws-lambda-java-events

Migrate to APIGatewayV2HTTPEvent in struts and spark

cc202cf

These frameworks are not supported by 2.0, but we remove references to HttpApiV2ProxyRequest anyway.

Continue migration - AwsProxy

0122b16

Rebase on main

7c48254

Fixed failing tests in AwsProxyRequestTest

c53fa81

Update Springboot sample

55a2fdc

Rebase on main

3df931e

fixed springboot3 sample app

8ec979e

removed comments, added tests

295f6c1

Bump aws-lambda-java-events version from 3.11.1 to 3.11.2

49a5cbc

fix samples

caa5101

Removed TODO comments

40ad8a2

- Moved AwsProxyResponse to test directory

300545e

- Replaced string literals with constants

- Temporarily bumped failBuildOnCVSS from 7 to 8 due to Jackson-datab…

80d799a

…ind vulnerability - Created a helper class to reduce duplicated code in AwsAlbHttpServletRequest

mbfreder force-pushed the java-events branch from 8031f54 to 80d799a Compare June 28, 2023 22:52

mbfreder mentioned this pull request Sep 19, 2023

Common response class for ApiGateway and ALB aws/aws-lambda-java-libs#427

Merged

mbfreder closed this Oct 18, 2023


		assertThat(req.getHeaders().get("accept"), is("*"));
		assertThat(req.getHeaders().get("accept"), is("*")); //TODO: Move this to alb specific test file, if any.

Migrate Serverless-java-container to aws-lambda-java-events #584

Migrate Serverless-java-container to aws-lambda-java-events #584

Conversation

mbfreder commented Jun 11, 2023

sapessi commented Jun 11, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

deki left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mbfreder commented Jun 28, 2023