-
Notifications
You must be signed in to change notification settings - Fork 271
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
support decoding byte slices #387
base: master
Are you sure you want to change the base?
Conversation
All of the backend infrastructure was in place to support &[u8], and some functions already take `impl AsRef<[u8]>`. However the main decode() and decode_header() functions require a &str. This change updates a few internal signatures and adds a _bytes() version of decode and decode_header. When you're doing many requests per second, the cost of doing an extra utf-8 check over header payloads is significant. By supporting a &[u8] decode, users can let base64 and the crypto implementation in question handle its own bytewise validity. They already do this today in addition to the extra utf-8 scan.
src/decoding.rs
Outdated
/// // Claims is a struct that implements Deserialize | ||
/// let token_message = decode_bytes::<Claims>(token, &DecodingKey::from_secret("secret".as_ref()), &Validation::new(Algorithm::HS256)); | ||
/// ``` | ||
pub fn decode_bytes<T: DeserializeOwned>( |
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.
any reason to not change decode
to take token: impl AsRef<[u8]>
and have a single fn?
Same question for decode_header
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.
Honestly, I think that would be better. It might be a minor breaking change for your users who use clippy. I was trying to avoid any potential for an api break, but that may not be the right thing to optimize for.
If you're willing to permit it, I'd be happy to see how taking token as AsRef feels!
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 it looks good, I'm going to wait a bit to see if there are more breaking changes to add for a new major version though
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.
Yep, seems like there are some other reasonable candidates for breaking changes in your issue queue.
Thanks for reviewing!
All of the backend infrastructure was in place to support &[u8], and some functions already take
impl AsRef<[u8]>
. However the main decode() and decode_header() functions require a &str. This change updates a few internal signatures and adds a _bytes() version of decode and decode_header.When you're doing many requests per second, the cost of doing an extra utf-8 check over header payloads is significant. By supporting a &[u8] decode, users can let base64 and the crypto implementation in question handle its own bytewise validity. They already do this today in addition to the extra utf-8 scan.