Skip to content

Consider removing #[serde(deny_unknown_fields)] from request types #750

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.

Sign up for GitHub

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

Jump to bottom
Closed
ramosbugs opened this issue Dec 12, 2023 · 2 comments · Fixed by #753
Closed

Consider removing #[serde(deny_unknown_fields)] from request types #750

ramosbugs opened this issue Dec 12, 2023 · 2 comments · Fixed by #753

Comments

@ramosbugs
Copy link
Contributor

#666 added #[serde(deny_unknown_fields)] to ApiGatewayProxyRequest and other request types:

aws-lambda-rust-runtime/lambda-events/src/event/apigw/mod.rs

Line 16 in d513b13

#[serde(deny_unknown_fields)]

Using this serde attribute causes deserialization to fail if any unexpected fields are included in the request payload. This has two negative consequences:

  1. Due to Bug: sam local includes extraneous version attribute in ApiGatewayLambdaEvent (incompatible with aws-lambda-rust-runtime) aws/aws-sam-cli#6442, Lambda functions written using this library cannot be tested with versions of sam local from the past 3 years. This is a bug in aws-sam-cli, not the aws_lambda_events crate, but it illustrates a downside to strict request parsing.
  2. If the Amazon API Gateway team were to add a new feature that involved a new request field, all deployed Rust Lambda functions using the Lambda proxy integration might immediately break in production. In most APIs, adding a new field is not considered a breaking change. Unless there's a guarantee from that team never to add additional fields, this could be a ticking time bomb.

Unless there's a strong reason to reject request payloads with unknown fields that outweighs these downsides, I'd recommend removing deny_unknown_fields.
@calavera
Copy link
Contributor

calavera commented Dec 13, 2023 •
edited
Loading

I don't really remember why I added those attributes, and I don't have a strong opinion about it. Feel free to open a PR.

ramosbugs added a commit to ramosbugs/aws-lambda-rust-runtime that referenced this issue Dec 14, 2023
@ramosbugs
Remove #[serde(deny_unknown_fields)]
3797151 
Adding new fields to request payloads is typically not considered
to be a breaking change, but using #[serde(deny_unknown_fields)]
would cause all Lambda functions to start failing immediately in
production if a new feature were deployed that added a new request
field.

Fixes awslabs#750.
@ramosbugs ramosbugs mentioned this issue Dec 14, 2023
Merged
2 tasks
calavera pushed a commit that referenced this issue Dec 14, 2023
@ramosbugs
Remove #[serde(deny_unknown_fields)] (#753)
9b21cd0 
Adding new fields to request payloads is typically not considered
to be a breaking change, but using #[serde(deny_unknown_fields)]
would cause all Lambda functions to start failing immediately in
production if a new feature were deployed that added a new request
field.

Fixes #750.
@github-actions GitHub Actions
Copy link

⚠️COMMENT VISIBILITY WARNING⚠️

Comments on closed issues are hard for the maintainers of this repository to see.
If you need more assistance, please open a new issue that references this one.
If you wish to keep having a conversation with other community members under this issue feel free to do so.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Remove #[serde(deny_unknown_fields)] ramosbugs/aws-lambda-rust-runtime
2 participants
@calavera @ramosbugs