Add content::ContentLocation header

[pepoviola]

Hi All, this is an attempt to make my small contribution to this awesome project 🙌 . This PR is inspired by #253 ( I try to following the same approach ) and also ref to one of the headers listed on #99.

I have some question related to the field for the ContentLocation struct. Is ok to be a String and parse with the Url crate or is better to use another type ( like to be directly an url and just format in the value ) ?

I'm new to rust and my code could be partial ( or totally 😄 ) wrong, so any feedback is welcome.

[yoshuawuyts]

Thanks so much for this PR! -- this is really good progress!

I have some question related to the field for the ContentLocation struct. Is ok to be a String and parse with the Url crate or is better to use another type ( like to be directly an url and just format in the value ) ?

I think we should be operating on a Url and then converting that to a string internally. This does mean that from_headers may need to take the form of from_headers(base_url: impl TryInto<Url>, headers: impl AsRef<Headers>) so that a valid Url can always be constructed, because Content-Location may send either a full URL or just a pathname. That way end-users will always get access to a full URL, which removes the difference entirely.

[yoshuawuyts]

Also ref #99

[pepoviola]

Hi @yoshuawuyts, Thanks for the feedback! I make some of the required changes ( the small ones ) and also change the struct to use Url type internally.

I'm not sure if this is ok, since an invalid base_url should return a 400 error right?

        let base = base_url.try_into().expect("Could not convert into a valid url");
        let url = base.join(value.as_str().trim()).status(400)?;

Again thanks for the feedback, and let me know if the approach isn't good.

[Fishrock123]

This looks good to me aside from one question.

[pepoviola]

Thanks @Fishrock123! I will open the pr for review 👍

[Fishrock123]

(This probably requires some kind of map_err() as noted by the build failures.)

[pepoviola]

Hi @Fishrock123, I use ok here and a match later. I don't know if it's the best approach.
Thx!

[yoshuawuyts]

@pepoviola I think what @Fishrock123 meant is that if you call base_url.try_into()? then by default a status code of 500 will be set on the error. The right solution would indeed be to call map_err and convert the error to a 400 status code. Returning None ignores the error, which is less good than having a 500 status code.

Instead the way to address this would be:

        // If we successfully parsed the header then there's always at least one
        // entry. We want the last entry.
        let value = headers.iter().last().unwrap();
        let base = base_url.try_into().map_err(|mut e| {
            e.set_status(400);
            e
        })?;

Hope that's helpful!

[pepoviola]

Hi @yoshuawuyts, thanks for the feedback!! Yes, I wasn't sure how to mapping the errors but now make a lot of sense.
I will change to your approach. Thanks for the help!

[pepoviola]

Hi @yoshuawuyts, sorry to bother you but I changed to the above approach and now I getting this errors related to the Error type and the set_status method.

error[E0599]: no method named `set_status` found for associated type `<U as std::convert::TryInto<url::Url>>::Error` in the current scope
  --> src/content/content_location.rs:58:15
   |
58 |             e.set_status(400);
   |               ^^^^^^^^^^ method not found in `<U as std::convert::TryInto<url::Url>>::Error`

error[E0277]: the trait bound `<U as std::convert::TryInto<url::Url>>::Error: std::error::Error` is not satisfied
  --> src/content/content_location.rs:60:11
   |
60 |         })?;
   |           ^ the trait `std::error::Error` is not implemented for `<U as std::convert::TryInto<url::Url>>::Error`
   |
   = note: required because of the requirements on the impl of `std::convert::From<<U as std::convert::TryInto<url::Url>>::Error>` for `anyhow::Error`
   = note: required because of the requirements on the impl of `std::convert::Into<anyhow::Error>` for `<U as std::convert::TryInto<url::Url>>::Error`
   = note: required because of the requirements on the impl of `std::convert::From<<U as std::convert::TryInto<url::Url>>::Error>` for `error::Error`
   = note: required by `std::convert::From::from`
help: consider further restricting the associated type
   |
46 |         U::Error: std::fmt::Debug, <U as std::convert::TryInto<url::Url>>::Error: std::error::Error
   |                                  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

error[E0277]: `<U as std::convert::TryInto<url::Url>>::Error` cannot be sent between threads safely
  --> src/content/content_location.rs:60:11
   |
60 |         })?;
   |           ^ `<U as std::convert::TryInto<url::Url>>::Error` cannot be sent between threads safely
   |
   = help: the trait `std::marker::Send` is not implemented for `<U as std::convert::TryInto<url::Url>>::Error`
   = note: required because of the requirements on the impl of `std::convert::From<<U as std::convert::TryInto<url::Url>>::Error>` for `anyhow::Error`
   = note: required because of the requirements on the impl of `std::convert::Into<anyhow::Error>` for `<U as std::convert::TryInto<url::Url>>::Error`
   = note: required because of the requirements on the impl of `std::convert::From<<U as std::convert::TryInto<url::Url>>::Error>` for `error::Error`
   = note: required by `std::convert::From::from`
help: consider further restricting the associated type
   |
46 |         U::Error: std::fmt::Debug, <U as std::convert::TryInto<url::Url>>::Error: std::marker::Send
   |                                  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

error[E0277]: `<U as std::convert::TryInto<url::Url>>::Error` cannot be shared between threads safely
  --> src/content/content_location.rs:60:11
   |
60 |         })?;
   |           ^ `<U as std::convert::TryInto<url::Url>>::Error` cannot be shared between threads safely
   |
   = help: the trait `std::marker::Sync` is not implemented for `<U as std::convert::TryInto<url::Url>>::Error`
   = note: required because of the requirements on the impl of `std::convert::From<<U as std::convert::TryInto<url::Url>>::Error>` for `anyhow::Error`
   = note: required because of the requirements on the impl of `std::convert::Into<anyhow::Error>` for `<U as std::convert::TryInto<url::Url>>::Error`
   = note: required because of the requirements on the impl of `std::convert::From<<U as std::convert::TryInto<url::Url>>::Error>` for `error::Error`
   = note: required by `std::convert::From::from`
help: consider further restricting the associated type
   |
46 |         U::Error: std::fmt::Debug, <U as std::convert::TryInto<url::Url>>::Error: std::marker::Sync
   |                                  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

error: aborting due to 4 previous errors

I try adding the restrictions but don't seems like the best path, right?

Thanks!

[yoshuawuyts]

@pepoviola I think what @Fishrock123 meant is that if you call base_url.try_into()? then by default a status code of 500 will be set on the error. The right solution would indeed be to call map_err and convert the error to a 400 status code. Returning None ignores the error, which is less good than having a 500 status code.

Instead the way to address this would be:

        // If we successfully parsed the header then there's always at least one
        // entry. We want the last entry.
        let value = headers.iter().last().unwrap();
        let base = base_url.try_into().map_err(|mut e| {
            e.set_status(400);
            e
        })?;

Hope that's helpful!

[yoshuawuyts]

a few more notes

[yoshuawuyts]

💯 this looks great; thank you!

[pepoviola]

awesome!, thanks for the guide and all the help!! 🙌