Migrate to java events #657

mbfreder · 2023-10-18T17:42:06Z

Issue #, if available:

Description of changes:
Migrated to Java-events lib.
The build fails because the V4 of Java-events is not released yet.

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.

mbfreder · 2023-10-18T17:44:04Z

The build fails because the V4 of Java-events is not released yet.

sapessi · 2023-10-18T17:45: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-10-18T17:45:45Z

...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 44 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.

sapessi · 2023-10-18T17:45:45Z

...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 438-446 into a separate method.

sapessi · 2023-10-18T23:28:00Z

.../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 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.

I think breaking this class into multiple classes will increase complexity, making the code very hard to understand.

deki · 2023-10-19T06:49:32Z

Thanks for all the work you put into it @mbfreder! Could you please rebase on main to resolve the conflicts? I will go through it in detail afterwards...

mbfreder · 2023-10-19T23:23:45Z

Rebased.

deki

We are almost there :-)

Looking at the code it would be helpful to have not just com.amazonaws.services.lambda.runtime.events.apigateway.HttpRequestEvent but also something like a V1Payload that bundles some properties of ApplicationLoadBalancerRequestEvent and APIGatewayProxyRequestEvent. Just an idea to reduce copy/ paste, not sure if the team would be open for it...

deki · 2023-10-26T16:07:16Z

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

+import java.util.List;
+import java.util.Map;
+
+public class AwsAlbExceptionHandler implements ExceptionHandler<AwsProxyResponseEvent>{


Why do we need that class? Can't we use AwsProxyExceptionHandler? Unittests ran fine without it.

deki · 2023-10-26T16:18:27Z

.../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 {


Please add a Javadoc comment to the class

deki · 2023-10-26T16:23:02Z

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

+
+    @Override
+    public String getProtocol() {
+        return "";


is that a valid return value?

deki · 2023-10-26T16:23:29Z

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

+            }
+        }
+
+        return new StringBuilder().append("null")


this looks wrong, see also #647

deki · 2023-10-26T16:24:42Z

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

+
+        if (request.getMultiValueHeaders() != null && request.getMultiValueHeaders().containsKey(HOST_HEADER_NAME)) {
+            String hostHeader = getFirst(request.getMultiValueHeaders(), HOST_HEADER_NAME);
+            if (SecurityUtils.isValidHost(hostHeader, "null", region)) { // ALB doesn't have apiId.


null instead of "null"?

deki · 2023-10-26T16:27:11Z

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

+
+    @Override
+    public String getRequestId() {
+        return "";


why is that empty?

deki · 2023-10-26T16:35:55Z

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

+    public HttpServletRequest readRequest(ApplicationLoadBalancerRequestEvent request, SecurityContext securityContext, Context lambdaContext, ContainerConfig config) throws InvalidRequestEventException {
+        // Expect the HTTP method and context to be populated. If they are not, we are handling an
+        // unsupported event type.
+        if (request.getHttpMethod() == null || request.getHttpMethod().equals("") || request.getRequestContext() == null) {


replace equals("") with isEmpty()

deki · 2023-10-26T16:37:43Z

...in/java/com/amazonaws/serverless/proxy/internal/servlet/AwsAlbHttpServletResponseWriter.java

+import java.util.HashMap;
+import java.util.Map;
+
+public class AwsAlbHttpServletResponseWriter extends ResponseWriter<AwsHttpServletResponse, AwsProxyResponseEvent> {


please create a Javadoc comment on class level

deki · 2023-10-26T16:46:02Z

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

@@ -35,36 +35,38 @@ public class AwsProxySecurityContext
    // Constants - Package
    //-------------------------------------------------------------

-    static final String AUTH_SCHEME_CUSTOM = "CUSTOM_AUTHORIZER";
+    public static final String AUTH_SCHEME_CUSTOM = "CUSTOM_AUTHORIZER";


instead of making in public you may wanted to use the identical constant from AwsAlbSecurityContext. either that or have the constants just once (in a base class?).

deki · 2023-10-26T16:54:16Z

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

+                <groupId>org.apache.maven.plugins</groupId>
+                <artifactId>maven-compiler-plugin</artifactId>
+                <configuration>
+                    <source>10</source>


should be 17

sapessi reviewed Oct 18, 2023

View reviewed changes

mbfreder force-pushed the migrate-java-events branch 2 times, most recently from 3b4a0ae to 3e3965a Compare October 18, 2023 23:48

mbfreder requested a review from deki October 19, 2023 00:11

mbfreder added 7 commits October 19, 2023 15:53

Added java-events v4 dependency

df70d56

migrate to APIGatewayV2HTTPEvent

ebd5acd

Migrate to APIGatewayV2HTTPEvent in struts and spark

6086216

migrated to APIGatewayProxyRequestEvent and ALB

abc09a8

Migrated to AwsProxyResponseEvent

905aa4b

updated samples

5a0b1b3

remove helper class

a00d71f

mbfreder force-pushed the migrate-java-events branch from 3e3965a to a00d71f Compare October 19, 2023 23:21

deki added this to the Release 2.0 milestone Oct 23, 2023

Fix samples

239bc0a

deki requested changes Oct 26, 2023

View reviewed changes

deki modified the milestones: Release 2.0, Release 3.0 Nov 16, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate to java events #657

Migrate to java events #657

mbfreder commented Oct 18, 2023 •

edited

mbfreder commented Oct 18, 2023

sapessi commented Oct 18, 2023

sapessi Oct 18, 2023

mbfreder Oct 19, 2023

sapessi Oct 18, 2023

mbfreder Oct 19, 2023

sapessi Oct 18, 2023

mbfreder Oct 19, 2023

deki commented Oct 19, 2023

mbfreder commented Oct 19, 2023 •

edited

deki left a comment

deki Oct 26, 2023

deki Oct 26, 2023

deki Oct 26, 2023

deki Oct 26, 2023

deki Oct 26, 2023

deki Oct 26, 2023

deki Oct 26, 2023

deki Oct 26, 2023

deki Oct 26, 2023

deki Oct 26, 2023

Migrate to java events #657

Are you sure you want to change the base?

Migrate to java events #657

Conversation

mbfreder commented Oct 18, 2023 • edited

mbfreder commented Oct 18, 2023

sapessi commented Oct 18, 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

deki commented Oct 19, 2023

mbfreder commented Oct 19, 2023 • edited

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mbfreder commented Oct 18, 2023 •

edited

mbfreder commented Oct 19, 2023 •

edited