Skip to content
This repository has been archived by the owner on May 16, 2023. It is now read-only.

Method Optional #33

Closed
jatsrt opened this issue Jan 8, 2021 · 9 comments · Fixed by #34
Closed

Method Optional #33

jatsrt opened this issue Jan 8, 2021 · 9 comments · Fixed by #34
Labels
bug Something isn't working regression

Comments

@jatsrt
Copy link

jatsrt commented Jan 8, 2021

Difference in 0.3 to 0.4, httpMethod/Method should be optional. This change breaks usability for websocket connection based events.

https://github.com/LegNeato/aws-lambda-events/blob/b86ba2835ff062d4771f500f82022fbe7525a33f/aws_lambda_events/src/generated/apigw.rs#L22

@LegNeato
Copy link
Contributor

LegNeato commented Jan 9, 2021

@calavera looks like we might have dropped the optional bit on translation.

@LegNeato LegNeato added bug Something isn't working regression labels Jan 9, 2021
@LegNeato
Copy link
Contributor

LegNeato commented Jan 9, 2021

@LegNeato
Copy link
Contributor

LegNeato commented Jan 9, 2021

Hm, actually this looks wrong on the Go side:

https://github.com/aws/aws-lambda-go/blob/master/events/apigw.go#L9

LegNeato added a commit to LegNeato/aws-lambda-go that referenced this issue Jan 9, 2021
@LegNeato
Copy link
Contributor

LegNeato commented Jan 9, 2021

Before we had a blanket Go string to Rust Option<String> conversion, as Go will coerce a null string to "", as mentioned in the README (go's default behavior is so lame). Because this is no longer a string after #29, we don't do the blanket conversion and thus hit this bug.

While I put up a PR for this case on the go side (aws/aws-lambda-go#350, will take time) we might want to have it always be Option<Method> on the Rust side.

Thoughts?

@LegNeato
Copy link
Contributor

LegNeato commented Jan 9, 2021

(this isn't really handled well by the C# bindings either: https://github.com/aws/aws-lambda-dotnet/blob/master/Libraries/src/Amazon.Lambda.APIGatewayEvents/APIGatewayProxyRequest.cs, "This field is only set for REST API requests.")

@calavera
Copy link
Owner

calavera commented Jan 9, 2021

That makes sense, sorry for breaking that use case. I have also seeing that you can get empty strings, which I fixed by using GET as default: https://github.com/LegNeato/aws-lambda-events/blob/master/aws_lambda_events/src/custom_serde.rs#L234

I really wish they had a schema. Making it optional, and the change in the Go library make sense to me.

@calavera
Copy link
Owner

@jatsrt do you use the same lambda function to receive HTTP events and WS events? or only WS events? The Go library makes a distinction about the kind of APIGateway events you receive by using two different types. The code in this library is still broken either way, but it might make more sense to modify the WS event type, and not the HTTP event type.

Apparently, different language bindings implement this in different ways, so I'm not sure at all about what's the best action to take.

@jatsrt
Copy link
Author

jatsrt commented Jan 19, 2021

Im my use cases they are always separate.

@jatsrt
Copy link
Author

jatsrt commented Mar 6, 2021

Still not optional in the request context, so it causes an error still

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Something isn't working regression
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants