Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adding support for APIGatewayV2 (Web Sockets) #92

Merged
merged 9 commits into from
Jul 30, 2019
Merged

Adding support for APIGatewayV2 (Web Sockets) #92

merged 9 commits into from
Jul 30, 2019

Conversation

TimGustafson
Copy link
Contributor

Issue #, if available:
N/A

Description of changes:
Added support for APIGatewayV2 events.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@bmoffatt
Copy link
Contributor

bmoffatt commented Jul 23, 2019

Can provide a link to the relevant AWS documentation and sample data?

@bmoffatt bmoffatt requested a review from mukhtar July 23, 2019 13:21
@TimGustafson
Copy link
Contributor Author

The basic AWS documentation about WebSocket APIs is available at:

https://docs.aws.amazon.com/apigateway/latest/developerguide/apigateway-websocket-api.html

Documentation specific to the Lambda Proxy integration can be found at:

https://docs.aws.amazon.com/apigateway/latest/developerguide/api-gateway-create-api-as-simple-proxy-for-lambda.html

Is that what you're looking for?

@bmoffatt
Copy link
Contributor

Going to use aws/aws-lambda-go#159 as a sample data reference

@bmoffatt
Copy link
Contributor

Having 'V2' in the name may be confusing for future readers. Putting Websocket in the type name will be clearer

Copy link
Contributor

@mukhtar mukhtar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the change.

The runtime currently fails to be deserialize the request into APIGatewayV2ProxyRequestEvent. I believe the two inner classes need to be declared static. See APIGatewayProxyRequestEvent.

@mukhtar
Copy link
Contributor

mukhtar commented Jul 23, 2019

Can you also add the remaining fields to APIGatewayV2ProxyRequestEvent? See aws/aws-lambda-go#159.

Modified inner classes to be static
@TimGustafson
Copy link
Contributor Author

I've added the static declaration for the inner classes, and also added the missing properties.

@mukhtar
Copy link
Contributor

mukhtar commented Jul 24, 2019

I'm facing an issue with the authorizer field which is an object in the sample from the aws-lambda-go repo. It seems it can be either a string or an object according to the aws-lambda-go pr. Any reason you selected String?

@TimGustafson
Copy link
Contributor Author

I have changed the authorizer field to be a Map<String,Object> so that it will take just about anything that comes in a map format. I've tested this against an APIGatewayV2 deployment that have an authorizer configured, and one that does not. It appears to work in both cases.

In investigating this, I realized that we also don't have classes for APIGatewayV2Authorizer(Request|Response) objects and started creating these to make authorizers easier to author in Java, but ran into a problem where Java serialization lower-cases the first letter of each property by default, but the policyDocument returned by the custom authorizer needs to have "Effect", "Action" and "Resources" all begin with a capital letter. I'm going to spend a bit more time to see if I can get that to work, as I believe that will provide a superior experience for users, and then I'll also supply some demo code (including a CloudFormation template) to demonstrate how to use these, as doing so right now is non-trivial, and required me to spend quite a bit of time doing trial-and-error until I got it just right.

If I do create those APIGatewayV2Authorizer(Request|Response) classes, should I include them in this fork, or should we close this (since technically they're not required for the APIGatewayV2Proxy(Request|Response)Event objects) and open a new fork for the authorizer classes?

@mukhtar
Copy link
Contributor

mukhtar commented Jul 26, 2019

Thanks! I think it would be great to get the APIGatewayV2Proxy(Request|Response) merged in so if we remove APIGatewayV2Authorizer(Request|Response) we're good to go. I tested this too and LGTM.

@TimGustafson
Copy link
Contributor Author

I have removed the custom authorizer classes. (Sorry, I didn't realize I had added them to the master branch!)

mukhtar added 2 commits July 30, 2019 20:54
To be consistent with APIGatewayProxyRequestEvent.

Also set serialVersionUID.
Copy link
Contributor

@mukhtar mukhtar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Added serialVersionUID to the serializable classes and updated the multivalue header and params value type to List<String> to be consistent with APIGatewayProxyRequestEvent.

@mukhtar mukhtar merged commit 32ffd2d into aws:master Jul 30, 2019
lucasam pushed a commit to lucasam/aws-lambda-java-libs that referenced this pull request Aug 1, 2019
lucasam pushed a commit to lucasam/aws-lambda-java-libs that referenced this pull request Aug 1, 2019
@adamnfish
Copy link

Is there any reason the new APIGatewayV2ProxyResponseEvent class lacks withX methods? The non-v2 version does include withX methods and I think that's the common pattern for the AWS SDKs?

Here's an example of an existing with method on the original APIGatewayProxyResponseEvent class:
https://github.com/aws/aws-lambda-java-libs/blob/master/aws-lambda-java-events/src/main/java/com/amazonaws/services/lambda/runtime/events/APIGatewayProxyResponseEvent.java#L44

Thanks very much this contribution, this will be a great help. Any idea when it will be available in a public release?

raupachz pushed a commit to raupachz/aws-lambda-java-libs that referenced this pull request Dec 1, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants