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

New Module For Retrieving Headers #1283

Closed
gsquire opened this issue Apr 16, 2020 · 5 comments
Closed

New Module For Retrieving Headers #1283

gsquire opened this issue Apr 16, 2020 · 5 comments
Labels
suggestion A suggestion to change functionality

Comments

@gsquire
Copy link
Contributor

gsquire commented Apr 16, 2020

Idea

While working through a small application, I wanted to fetch a request header to do some inspection and respond accordingly. I looked through existing issues and comments in this repository saying to use the FromRequest trait for the desired header. This was pretty straightforward and I was wondering if it would be useful to have a new module in contrib that implemented this as a macro for various common headers? This way, developers could use them as request guards to fetch headers that they want to use.

Here is how I used it to fetch the user agent. In this case, I failed if the header wasn't there but in the implementation, we can use an Option and always succeed.

Implementation Idea

This is roughly what I think it could look like. Keep in mind this won't compile as is.

// Perhaps have a macro to make types too.
#[derive(Debug)]
struct $ty {
    value: String,
}

// This could be inside of a macro.
impl<'a, 'r> FromRequest<'a, 'r> for $ty {
    type Error = ();

    fn from_request(request: &'a Request<'r>) -> request::Outcome<Option<Self>, Self::Error> {
        const HEADER: &str = $k; // The macro would expand the key 'k'.

        let value = request.headers().get_one(HEADER);
        match value {
            Some(v) => Outcome::Success(Some($ty {
                value: v.to_string(),
            })),
            None => Outcome::Success(None),
        }
    }
}
@jebrosen
Copy link
Collaborator

Although this example is useful as-is and makes sense in a utility module or crate, I think for inclusion in rocket_contrib I would rather see a solution using a typed header API. Note also that in 0.4 we do have the reverse (typed header from hyper -> rocket Header), but in the async branch we have lost typed headers altogether (see #1067).


A feature like this could also be a really nice application of const generics (still quite a ways away from being stable, I fear):

const CONTENT_LENGTH: &str = "Content-Length";
const CONTENT_TYPE: &str = "Content-Type";

struct Header<'a, const NAME: &str>(value: &'a str);

impl<'a, 'r, const NAME> FromRequest<'a, 'r> for Header<'a, NAME> {
    type Error = ();
    fn from_request(request: &'a Request<'r>) -> request::Outcome<Self, Self::Error> {
        request.headers().get_one(NAME).expect("TODO: example")
    }
}

#[get("/")]
fn index(ct: Header<CONTENT_TYPE>) -> String {
    ct.0.to_string()
}

One upside of an approach like this is that hyper (0.13) already has constants defined for most or all headers so this is "free". A significant downside is that it can't support typed headers in any way.

@jebrosen jebrosen added the suggestion A suggestion to change functionality label Apr 18, 2020
@gsquire
Copy link
Contributor Author

gsquire commented Apr 18, 2020

I agree that using constant generics would be nice. I suppose you could close this issue or let things settle with the async migration and go from there?

Thanks for the response.

@gsquire
Copy link
Contributor Author

gsquire commented Jan 26, 2021

I know Rust 1.51 is still a ways out but it sounds like the goal is to get the min_const_generics feature stabilized. If that is the case, would this feature request still be viable? Feel free to close the issue if you think this could go in another direction too. Thanks for your time.

@jebrosen
Copy link
Collaborator

I know Rust 1.51 is still a ways out but it sounds like the goal is to get the min_const_generics feature stabilized. If that is the case, would this feature request still be viable?

Unfortunately not as far as that design: <const NAME: &'static str> requires const_generics, not min_const_generics.


I am still in favor of this idea in general. #1067 already tracks the conversion from a typed header library to Rocket's header type which is most useful for responses. I think we should either 1) keep this issue open for implementing FromRequest for header types, which is most useful for requests, or 2) solve it alongside #1067.

@SergioBenitez
Copy link
Member

Closing in favor of #1067.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
suggestion A suggestion to change functionality
Projects
None yet
Development

No branches or pull requests

3 participants