search

http-rs / http-types

Common types for HTTP operations
https://docs.rs/http-types
Apache License 2.0
200 stars 83 forks source link

Consider more rigorous handling of http::StatusCode conversion to StatusCode #183

Open khodzha opened 4 years ago

khodzha commented 4 years ago

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

khodzha commented 4 years ago

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

Fishrock123 commented 4 years ago

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?

Fishrock123 commented 4 years ago

Related: making Request::new() or Response::new() return a Result makes the bounds complain that the errors are not Send/Sync (I forget which).

khodzha commented 4 years ago

and current Error already must include valid StatusCode https://github.com/http-rs/http-types/blob/dae51d7cf462ada802bd21cf4bc8c97dbacd7b97/src/error.rs#L15-L18

Fishrock123 commented 4 years ago

and current Error already must include valid StatusCode

That would just be 500 in the case of a Response.

isra17 commented 4 years ago

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.
isra17 commented 4 years ago

This would look like https://github.com/http-rs/http-types/commit/ae64fd0b84f0ac27d71c37e7bdae2972b8571635 , what do you think?

Fishrock123 commented 4 years ago

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.

isra17 commented 4 years ago

You cannot mix custom discriminant values (X = 1) with tuple or struct variants (Y(u16)).

Fishrock123 commented 4 years ago

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?

isra17 commented 4 years ago

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?

mipli commented 4 years ago

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:

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.