Add `content::ContentLocation` header by pepoviola · Pull Request #256 · http-rs/http-types

pepoviola · 2020-10-08T15:02:55Z

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.

src/content/content_location.rs

yoshuawuyts · 2020-10-08T21:30:12Z

Also ref #99

pepoviola · 2020-10-09T17:10:38Z

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.

src/content/content_location.rs

Co-authored-by: Jeremiah Senkpiel <fishrock123@rocketmail.com>

src/content/content_location.rs

Fishrock123

This looks good to me aside from one question.

pepoviola · 2020-10-12T18:05:22Z

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

src/content/content_location.rs

Co-authored-by: Jeremiah Senkpiel <fishrock123@rocketmail.com>

Fishrock123 · 2020-10-12T19:07:30Z

src/content/content_location.rs

+        // 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()?;


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

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

@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!

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!

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 `>::Error` in the current scope --> src/content/content_location.rs:58:15 | 58 | e.set_status(400); | ^^^^^^^^^^ method not found in `>::Error` error[E0277]: the trait bound `>::Error: std::error::Error` is not satisfied --> src/content/content_location.rs:60:11 | 60 | })?; | ^ the trait `std::error::Error` is not implemented for `>::Error` | = note: required because of the requirements on the impl of `std::convert::From<>::Error>` for `anyhow::Error` = note: required because of the requirements on the impl of `std::convert::Into<anyhow::Error>` for `>::Error` = note: required because of the requirements on the impl of `std::convert::From<>::Error>` for `error::Error` = note: required by `std::convert::From::from` help: consider further restricting the associated type | 46 | U::Error: std::fmt::Debug, >::Error: std::error::Error | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ error[E0277]: `>::Error` cannot be sent between threads safely --> src/content/content_location.rs:60:11 | 60 | })?; | ^ `>::Error` cannot be sent between threads safely | = help: the trait `std::marker::Send` is not implemented for `>::Error` = note: required because of the requirements on the impl of `std::convert::From<>::Error>` for `anyhow::Error` = note: required because of the requirements on the impl of `std::convert::Into<anyhow::Error>` for `>::Error` = note: required because of the requirements on the impl of `std::convert::From<>::Error>` for `error::Error` = note: required by `std::convert::From::from` help: consider further restricting the associated type | 46 | U::Error: std::fmt::Debug, >::Error: std::marker::Send | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ error[E0277]: `>::Error` cannot be shared between threads safely --> src/content/content_location.rs:60:11 | 60 | })?; | ^ `>::Error` cannot be shared between threads safely | = help: the trait `std::marker::Sync` is not implemented for `>::Error` = note: required because of the requirements on the impl of `std::convert::From<>::Error>` for `anyhow::Error` = note: required because of the requirements on the impl of `std::convert::Into<anyhow::Error>` for `>::Error` = note: required because of the requirements on the impl of `std::convert::From<>::Error>` for `error::Error` = note: required by `std::convert::From::from` help: consider further restricting the associated type | 46 | U::Error: std::fmt::Debug, >::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!

src/content/content_location.rs

yoshuawuyts · 2020-10-17T15:43:17Z

src/content/content_location.rs

+        // 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()?;


@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

Co-authored-by: Yoshua Wuyts <yoshuawuyts+github@gmail.com>

src/content/content_location.rs

…tion

yoshuawuyts

💯 this looks great; thank you!

pepoviola · 2020-11-03T22:55:37Z

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

pepoviola added 2 commits October 7, 2020 12:32

initial work to review

aa54f4b

use to_string and change ref to ietf

96052dd

yoshuawuyts reviewed Oct 8, 2020

View reviewed changes

src/content/content_location.rs Outdated Show resolved Hide resolved

src/content/content_location.rs Show resolved Hide resolved

src/content/content_location.rs Outdated Show resolved Hide resolved

pepoviola added 2 commits October 9, 2020 10:31

changes from feedback, use Url type for url field

b504712

fix test

05c5cf3

changes from clippy suggestions

b537b58

Fishrock123 reviewed Oct 9, 2020

View reviewed changes

src/content/content_location.rs Outdated Show resolved Hide resolved

src/content/content_location.rs Outdated Show resolved Hide resolved

src/content/content_location.rs Outdated Show resolved Hide resolved

src/content/content_location.rs Outdated Show resolved Hide resolved

pepoviola and others added 2 commits October 9, 2020 20:31

Update src/content/content_location.rs

09fb048

Co-authored-by: Jeremiah Senkpiel <fishrock123@rocketmail.com>

use Url as type for struct and fmt

fdcb1a6

Fishrock123 reviewed Oct 12, 2020

View reviewed changes

src/content/content_location.rs Show resolved Hide resolved

Fishrock123 approved these changes Oct 12, 2020

View reviewed changes

pepoviola marked this pull request as ready for review October 12, 2020 18:05

Fishrock123 reviewed Oct 12, 2020

View reviewed changes

src/content/content_location.rs Outdated Show resolved Hide resolved

Update src/content/content_location.rs

ccb085f

Co-authored-by: Jeremiah Senkpiel <fishrock123@rocketmail.com>

Fishrock123 reviewed Oct 12, 2020

View reviewed changes

use match to check error

f83e425

yoshuawuyts reviewed Oct 17, 2020

View reviewed changes

pepoviola and others added 3 commits October 17, 2020 13:06

Update src/content/content_location.rs

f4abf11

Co-authored-by: Yoshua Wuyts <yoshuawuyts+github@gmail.com>

Merge branch 'main' into header-content-location

348b7d9

use create new error approach

cc1c8e9

pepoviola commented Oct 22, 2020

View reviewed changes

src/content/content_location.rs Outdated Show resolved Hide resolved

Fishrock123 mentioned this pull request Nov 2, 2020

Add content::ContentLocation header #268

Closed

pepoviola added 2 commits November 2, 2020 22:57

Merge remote-tracking branch 'upstream/main' into header-content-loca…

63a7be9

…tion

changes from feedback, use bail_status

85a5089

yoshuawuyts changed the title ~~Header content location~~ Add content::ContentLocation header Nov 3, 2020

yoshuawuyts approved these changes Nov 3, 2020

View reviewed changes

yoshuawuyts merged commit de7afbb into http-rs:main Nov 3, 2020

Conversation

pepoviola commented Oct 8, 2020

Uh oh!

yoshuawuyts left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

yoshuawuyts commented Oct 8, 2020

Uh oh!

pepoviola commented Oct 9, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Fishrock123 left a comment

Choose a reason for hiding this comment

Uh oh!

pepoviola commented Oct 12, 2020

Uh oh!

Uh oh!

Fishrock123 Oct 12, 2020

Choose a reason for hiding this comment

Uh oh!

pepoviola Oct 13, 2020

Choose a reason for hiding this comment

Uh oh!

yoshuawuyts Oct 17, 2020

Choose a reason for hiding this comment

Uh oh!

pepoviola Oct 17, 2020

Choose a reason for hiding this comment

Uh oh!

pepoviola Oct 17, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

yoshuawuyts Oct 17, 2020

Choose a reason for hiding this comment

Uh oh!

yoshuawuyts left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

yoshuawuyts left a comment

Choose a reason for hiding this comment

Uh oh!

pepoviola commented Nov 3, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants