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

Gracefully handle missing headers #1810

Merged
merged 6 commits into from
Mar 3, 2023

Conversation

RReverser
Copy link
Contributor

@RReverser RReverser commented Mar 3, 2023

Reports a missing header rejection only if the header's decoder didn't return a default value.

I've also added a regression test by using Cookie as an example: missing Cookie should be treated in the same way as empty Cookie header, but before this PR would reject the entire request. Now it's going to be handled correctly.

Closes #1781.

Adds a failing test for missing Content-Encoding header that should result in an empty-list Content-Encoding instead of an error.
Reports a missing header rejection only if the header's decoder didn't return a default value.

Closes tokio-rs#1781.
Copy link
Member

@jplatte jplatte left a comment

Choose a reason for hiding this comment

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

Looks good!

axum/src/typed_header.rs Show resolved Hide resolved
Copy link
Member

@jplatte jplatte left a comment

Choose a reason for hiding this comment

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

If @davidpdrsn agrees, I think we can merge this as a non-breaking change.

axum/src/typed_header.rs Outdated Show resolved Hide resolved
This avoids relying on Debug representation of the opaque ContentEncoding type.
Copy link
Member

@davidpdrsn davidpdrsn left a comment

Choose a reason for hiding this comment

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

Looks good to me! Wanna update the change log as well?

@RReverser
Copy link
Contributor Author

Looks good to me! Wanna update the change log as well?

Done.

Copy link
Member

@davidpdrsn davidpdrsn left a comment

Choose a reason for hiding this comment

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

Thanks!

@davidpdrsn davidpdrsn merged commit fd96bce into tokio-rs:main Mar 3, 2023
@RReverser RReverser deleted the missing-headers branch March 3, 2023 15:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

TypedHeader shouldn't reject missing headers by default
3 participants