Common response class for ApiGateway and ALB #427
Conversation
There was a problem hiding this comment.
I don't think we need an extra response object. I think we need to pick one, and if they're all the same, remove the others.
There was a problem hiding this comment.
I think customers should still be able to use the current response classes. AwsProxyResponseEvent is just meant simplify things; It would be very helpful for the Serverless-java-container use-case, but I'm not sure everybody would want that. That's why I kept the current classes.
...bda-java-events/src/main/java/com/amazonaws/services/lambda/runtime/events/helper/Utils.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Is there an existing class which already covers this response?
There was a problem hiding this comment.
No. This response class is similar to the one we currently have on aws-serverless-java-container. Having it here allows us to maintain a single response class on the serverless java container, which will remove a lot of duplicated code.
There was a problem hiding this comment.
I'm not sure which package this should be in, but it doesn't feel like it should be in helper.
There was a problem hiding this comment.
Right. Moving it to the events folder.
There was a problem hiding this comment.
Has this class come from aws-serverless-java-container? Why do you think it belongs in the events lib?
There was a problem hiding this comment.
This was added based on this conversation with Dennis. The class does not come from SJC, the the methods inside do. We thought by bringing theses here, other customers might benefit from them.
| /** | ||
| * Parses a header value using the default value separator "," and qualifier separator ";". | ||
| * @param headerValue The value to be parsed | ||
| * @return A list of SimpleMapEntry objects with all of the possible values for the header. |
There was a problem hiding this comment.
what is SimpleMapEntry? I see the method is returning List of HeaderValue
There was a problem hiding this comment.
Old comment that I forgot to update. Fixed.
|
|
||
| // sort list by preference | ||
| values.sort((HeaderValue first, HeaderValue second) -> { | ||
| if ((first.getPriority() - second.getPriority()) < .001f) { |
There was a problem hiding this comment.
where is this magic number coming from?
There was a problem hiding this comment.
That's an epsilon. Instead of comparing 2 float values for equality (with ==), we check whether their difference is within some error bounds or epsilon value, like this: bool isEqual = fabs(f1 – f2) <= epsilon; This calculation allows us to express the concept of two floats being close enough that we want to consider them to be equal. Ref
| return null; | ||
| } | ||
|
|
||
| public static String getFirstQueryParamValue(Map<String, List<String>> queryString, String key, boolean isCaseSensitive) { |
There was a problem hiding this comment.
I'd avoid boolean parameter in this method. Looks like it is easy to split into 2 different methods
| public HeaderValue() { | ||
| attributes = new HashMap<>(); | ||
| } | ||
|
|
There was a problem hiding this comment.
NIT: the file doesn't look properly formatted, 2 blank lines between methods
| /** | ||
| * Class that represents a header value. | ||
| */ | ||
| public static class HeaderValue { |
There was a problem hiding this comment.
if it ever end up in a HashMap, do you think it makes sense to implement equals and hashCode?
There was a problem hiding this comment.
I can't think of a use case for that.
| * This class exposes some utility methods to work with request values such as headers | ||
| * and query string parameters. | ||
| */ | ||
| public class HeaderUtils { |
There was a problem hiding this comment.
This utility class has a lot helper methods - worth covering basic scenarios with unit tests?
| /** | ||
| * Interface to abstract APIGWY and ALB request classes, reduce complexity and increase efficiency. | ||
| */ | ||
| public interface LambdaRequestEvent { |
There was a problem hiding this comment.
Can this be renamed to something like HttpRequestEvent?
| import com.amazonaws.services.lambda.runtime.events.apigateway.RequestSource; | ||
|
|
||
| /** | ||
| * Interface to abstract APIGWY and ALB request classes, reduce complexity and increase efficiency. |
There was a problem hiding this comment.
"A common interface shared by event sources which produce HTTP as JSON"
Issue #, if available:
Description of changes:
Target (OCI, Managed Runtime, both):
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.