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

Add omitempty to HTTPMethod in APIGatewayProxyRequest #350

Closed
wants to merge 1 commit into from

Conversation

LegNeato
Copy link
Contributor

@LegNeato LegNeato commented Jan 9, 2021

This can be empty for websocket events, see calavera/aws-lambda-events#33

Issue #, if available:

Description of changes:

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@codecov-io
Copy link

Codecov Report

Merging #350 (6b77cd7) into master (b3dd246) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #350   +/-   ##
=======================================
  Coverage   72.22%   72.22%           
=======================================
  Files          19       19           
  Lines         738      738           
=======================================
  Hits          533      533           
  Misses        138      138           
  Partials       67       67           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b3dd246...6b77cd7. Read the comment docs.

@harrisonhjones
Copy link
Contributor

I'm just curious: why do you want this? Are you marshalling this struct to JSON? Why?

@LegNeato
Copy link
Contributor Author

I generate rust code from the go code: calavera/aws-lambda-events#33

@bmoffatt
Copy link
Collaborator

did you mean to modify APIGatewayWebsocketProxyRequest instead?

Since it's unused, I think we could deprecate the field, and remove the json tag. Would that play well with your code generator?

@LegNeato
Copy link
Contributor Author

I'm pretty sure I modified the correct struct. If you look at the corresponding C# class (https://github.com/aws/aws-lambda-dotnet/blob/master/Libraries/src/Amazon.Lambda.APIGatewayEvents/APIGatewayProxyRequest.cs) they point out that different fields can be null for this event depending on if the source is http or websockets.

I just look at the structs, so if there is additional marshalling logic elsewhere that chooses between the two structs please let me know and I'll see what I can do on my side.

@bmoffatt
Copy link
Collaborator

There's no marshaling logic, it's up the the author of the function to select the correct struct for the event source. The java events project also has a separate type for web socket events

@calavera
Copy link
Contributor

@LegNeato it might make more sense to modify the Websocket structure like @bmoffatt suggests. Leaving up to the user is kind of unfortunate because for them both requests come from origin. It will probably be reasonable to supplement with specific examples of usage for each one of the structs.

@bmoffatt
Copy link
Collaborator

bmoffatt commented Mar 2, 2021

superseded by #355

@bmoffatt bmoffatt closed this Mar 2, 2021
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.

5 participants