http-types
http-types copied to clipboard
http-rs
Consider more rigorous handling of http::StatusCode conversion to StatusCode
Right now conversion from http::StatusCode (which is just an u16 in 100..=600 range) to StatusCode just expect()s the result for example here:
https://github.com/http-rs/http-types/blob/539638273de768a24354b65999ad9317b6659204/src/response.rs#L101-L103
this could cause a crash in response conversion for example here: https://github.com/http-rs/http-types/blob/539638273de768a24354b65999ad9317b6659204/src/hyperium_http.rs#L123
if some server responds erroneously with some status that belongs to 100..=600 range but is not implemented in StatusCode enum (for example 229), the conversion From<http::Response<Body>> for Response will panic despite it being From and not TryFrom
also this is just plain unwrap() which can fail in the same way https://github.com/http-rs/http-types/blob/539638273de768a24354b65999ad9317b6659204/src/hyperium_http.rs#L21-L23
Related: Request does the same thing but for Url.
https://github.com/http-rs/http-types/blob/539638273de768a24354b65999ad9317b6659204/src/request.rs#L47-L54
I'm not sure what the correct answer is here - do we make ::new() return a Result? Do this in a way that swallows errors?
Related: making Request::new() or Response::new() return a Result makes the bounds complain that the errors are not Send/Sync (I forget which).
and current Error already must include valid StatusCode https://github.com/http-rs/http-types/blob/dae51d7cf462ada802bd21cf4bc8c97dbacd7b97/src/error.rs#L15-L18
and current Error already must include valid StatusCode
That would just be 500 in the case of a Response.
This is kind of a blocker for anything that tries to download arbitrary resource. Would it make sense to fallback to X00 code in case of error as described in the RFC or perhaps set a fallback code Invalid = 0 value:
For example, if an unrecognized status code of 471 is received by a
client, the client can assume that there was something wrong with its
request and treat the response as if it had received a 400 (Bad
Request) status code.
This would look like https://github.com/http-rs/http-types/commit/ae64fd0b84f0ac27d71c37e7bdae2972b8571635 , what do you think?
Hmm, I'm not sure if that is the correct way to go. However, something occurred to me while reading that: we can take better advantage of Rust enums' ability to store different things.
That is we could have something like:
pub enum StatusCode {
Unknown(u16),
// ...
}
and in the match:
code => StatusCode::Unknown(code),
I think that would be better than coercing to 400. It would handle any arbitrary value the server could realistically send, let us match on that in specific, and yet still let us inspect the value for logging/debugging.
You cannot mix custom discriminant values (X = 1) with tuple or struct variants (Y(u16)).
Hmm, That's unfortunate. Perhaps StatusCode should look like:
pub enum StatusCode {
Unknown(u16),
Continue,
BadRequest,
// ... etc
}
With custom overloads for as, etc to make it act like a discriminant enum? It doesn't need to be C-compatible so that seems like an ok thing to do, even if it's a bit more code?
Or to keep it sized as u16, perhaps:
pub struct StatusCode(u16);
#[allow(non_upper_case_globals)]
impl StatusCode {
pub const Continue: Self = StatusCode(100);
pub const Ok: Self = StatusCode(200);
// ...
}
Actually this might be the most efficient way to not lose information and not alias any values?
Has there been any more thought into how this should be solved? Had an issue with this when integrating with an external API, so would love to help solve this bug.
I've done a first pass on implementation of both of the solutions suggested above to get an idea of how they might look in practice:
- Enum solution: https://github.com/http-rs/http-types/compare/master...mipli:unknown-statuscode-as-enum
- Struct solution: https://github.com/http-rs/http-types/compare/master...mipli:unkown-statuscode-as-struct
With custom overloads for
as, etc to make it act like a discriminant enum?
@Fishrock123 Is it possible to do a custom overload for as? As far as I could find that was not possible, which means that this change would require new major version release, since StatusCode::Ok as u16 will no longer work.
If either of these solutions is something you would like as a solution for this, I can make a proper PR and we can finalize the solution there. If not, please let me know how what I can do to help find a solution for this problem.
