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

Add handling of 307 and 308 redirects #18

Closed
wants to merge 2 commits into from

Conversation

aidanhs
Copy link
Contributor

@aidanhs aidanhs commented Nov 17, 2016

Fixes #9

I've tested and it seems to work on cloudfront-hosted assets I've been struggling to download with hyper (they were returning 307 for a GET). I've not tested it with PUT or POST.

This PR also makes Method::Delete be transformed into Method::Get by 301, 302 and 303 responses since that's the described behaviour for a 303 response on https://en.wikipedia.org/wiki/List_of_HTTP_status_codes#3xx_Redirection.

@aidanhs
Copy link
Contributor Author

aidanhs commented Nov 17, 2016

Hmm, there's a complexity here - what happens if the body is a reader? This may need to be more invasive.

@aidanhs
Copy link
Contributor Author

aidanhs commented Nov 17, 2016

Ok, split into two commits. The first alters Kind::Reader bodies to always seek to the beginning. This is a breaking change because it adds a Seek bound on the reader if you're calling Body::new. It also fixes a bug where using f.metadata().map(|m| m.len()).ok() as the length is wrong (the File may not be at the beginning).

The second implements the 307/308 redirects.

Copy link
Owner

@seanmonstar seanmonstar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! This is definitely the right direction.

@@ -10,7 +13,7 @@ pub struct Body {

impl Body {
/// Instantiate a `Body` from a reader.
pub fn new<R: Read + 'static>(reader: R) -> Body {
pub fn new<R: Read + Seek + 'static>(reader: R) -> Body {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I'd rather hold off on requiring Seek. There are use cases where a body could be a reader that cannot seek. Looking at golang, it seems they instead have a check if the body can be reused, and if not, just returns the 30x response as is. I think we could probably do the same for now.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a bit ambivalent about that - I'd be a bit perplexed if two identically typed Body structs behaved completely differently when faced with a redirect.

It would be particularly annoying to deal with that in my case as cloudflare only intermittently returns the redirect depending on what it has cached - the prospect of trying to debug a failure that's intermittent in two dimensions (cloudflare, Body construction details) makes me nervous.

How about a Body<Reusable> and a Body<Oneshot>? I'd have to implement it to see what using it would look like, but it seems doable.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They wouldn't be identical Body structs.

Again, looking into other libraries, it doesn't look like it's easy to always do this. For case where the body is string, it is handled, and reqwest can do so also. But for streams, there isn't a way to reset the stream, and so the 307 response is returned directly. I don't know how I'd feel about Reusable and Oneshot; it'd all depend on if it was explicit, and not confusing.

Since everyone usually always checks the status code of a response, it should seem like people would notice when they receive a 307.

},
StatusCode::TemporaryRedirect |
StatusCode::PermanentRedirect => (),
_ => panic!(),
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Prefer unreachable!() in this instance.

repi pushed a commit to EmbarkStudios/reqwest that referenced this pull request Dec 20, 2019
expose log function and support tags & extra fields
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants