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 layer to validate requests #289
Add layer to validate requests #289
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! I think this makes sense to have.
@davidpdrsn @Nehliin thanks for your comments! I've updated the PR |
CI sometimes fails on
|
tower-http/src/validate_request.rs
Outdated
|
||
/// Type that performs validation of the Accept header. | ||
pub struct AcceptHeader<ResBody> { | ||
header_value: Mime, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should use Arc<Mime>
since Mime
might contain allocated collections like String
and Vec
. Just as a small perf optimization.
header_value: Mime, | |
header_value: Arc<Mime>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, thanks. What kind of performance gains does this achieve?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Services tend to be cloned a lot. So wrapping this in an Arc
makes it cheaper to clone.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Getting closer 😊 wanna update the changelog as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Thanks for working on this 😃
.to_str() | ||
.ok() | ||
.into_iter() | ||
.flat_map(|s| s.split(",").map(|typ| typ.trim())) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Worth noting that this is incorrect for the same reason as this issue http-rs/http-types#349
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh thanks for catching that! Would you mind opening an issue?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I might not be the best at doing this — I don't really know Rust and just stumbled on this when giving a colleague advice on how to port something from a Node project to their Rust project. But I can if you would prefer an issue from somebody who doesn't actually directly use your project or know Rust :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alright. I've opened #291. Thanks for reporting this.
This commit adds a new layer to customize validating requests.
Motivation
The new layer adds support for custom validation of requests.
In smithy-rs, we allow requests based on the
Accept
header. Our use-case could be common to others using tower and more general than just validating a header, hence this layer to validate any request, with headers being a special case.Solution
The new layer takes from the
RequireAuthorization
layer and generalizes it for any request. If this is accepted, we can have the authorization layer be like theAcceptHeader
layer and use this general validator.Alternatively, we could have a layer to validate headers specifically or only for the Accept header.