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 #21

Merged
merged 3 commits into from
Nov 29, 2016
Merged

Conversation

seanmonstar
Copy link
Owner

@aidanhs I took the commits from your PR, and did as I was saying in #18, removing the Seek bounds on Body. I'm not against finding a solution, but I wanted to get these valuable fixes that you made merged in sooner, and we can find the best solution later.

For anyone curious, it looks better if you add ?w=1 to the end of the diff on Github (removes the whitespace noise).

Closes #9
Closes #18

@aidanhs
Copy link
Contributor

aidanhs commented Nov 23, 2016

I'm fine with going back to closer to my original approach (approximately equal to this PR), but this isn't quite right for the behaviour that you're looking for. Specifically, it seems to me that the reader will be at EOF when trying to make a subsequent request as a result of a 307 or 308 redirect. This seems like really bad behaviour.

I'd propose one of two options to get the incremental fix in:

  1. 307 and 308 are returned as-is (as they are currently) if there's a body, which at least allows redirects without a body (e.g. GET) to work.
  2. The knowledge of whether a body can be reused is exposed to send somehow, and it returns the 307/308 directly if it can't.

If you look back at #18 I've taken this PR and added a single tiny commit to implement suggestion 1.

@seanmonstar
Copy link
Owner Author

@aidanhs Good catch! I've added the ability for send to know if the body can be reset, and return the response if it cannot. And with tests!

body::can_reset(body)
} else {
true
}
Copy link
Contributor

Choose a reason for hiding this comment

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

This is minor personal preference, but you could write this as body.map(body::can_reset).unwrap_or(true).

@aidanhs
Copy link
Contributor

aidanhs commented Nov 29, 2016

Might be worth adding a note to https://docs.rs/reqwest/0.1.0/reqwest/struct.RequestBuilder.html#method.body saying something like "The type of body will affect whether this request can be redirected. For example, since a Reader may not be seekable a 307 or 308 response to a PUT or POST will be returned as-is rather than trying to send the request (and body) again."

@seanmonstar seanmonstar merged commit 409edad into master Nov 29, 2016
@seanmonstar seanmonstar deleted the aidanhs-aphs-307-308 branch November 29, 2016 18:22
@seanmonstar
Copy link
Owner Author

(Travis failure was nightly, there is a bug today's nightly cargo.)

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.

Handle 307 and 308 redirects
2 participants