Follow 307/308 redirects

[jyn514]

This is the simplest part of #187. I'm not sure what the expect-100 discussion was about, but maybe it can be fixed later?

[algesten]

Thank! I need to refresh my memory on why we disabled 307/308. I suspect it's because the spec requires us to resend any body, which in turn means buffer potentially a lot of data to be able to "replay", or break spec.

[jyn514]

Another option is to make this work most of the time by only worrying about GET/HEAD/DELETE requests. Then there's no body to worry about resending, and only PUT/POST will have to deal with 307 responses manually.

[jyn514]

Would love to get some eyes on this :) AFAIK there are a few ways forward:

1. The current situation, don't follow 307/308 and return them to the caller as an error. This is the most flexible for the caller but also the least ergonomic, especially since ureq doesn't currently document that 307 isn't handled.
2. Handle 307/308, but only for GET/HEAD/DELETE (any method without a body). This handles the common case while putting off a decision about bodies for later. However it makes the API pretty inconsistent, which is unfortunate.
3. Handle 307/308 on PUT by resending the request. This requires bounding the API to require Seek, not just Read, which is more restrictive for the caller but prevents spurious copies. Personally I think requiring the caller to use read_to_end first is not a giant deal, but I also don't use ureq for PUT requests so my opinion doesn't matter very much :P
4. possibly other ideas ???

While writing this up I realized I don't actually need this to be fixed on the ureq end to fix deadlinks/cargo-deadlinks#110, I can handle the 307/308 explicitly on my end.

[jsha]

Hi, sorry for the delay in looking at this! I have been meaning to swap back into my head what the semantics of 307/308 are. I think this is the best option for a short term fix:

Handle 307/308, but only for GET/HEAD/DELETE (any method without a body). This handles the common case while putting off a decision about bodies for later. However it makes the API pretty inconsistent, which is unfortunate.

Are you interested in updating this PR to take this approach?

[jyn514]

Yes, definitely! I'll add that tomorrow.

[jsha]

FYI, you're getting a test failure because we do something tricky to make our doctests not hit live websites. If you look at the hidden is_test(true) at the top of the doctest, it sets of a test agent that overrides DNS lookups to point at a TestServer listening on localhost. We have a set of handlers in testserver.rs for various known test URLs. To make your doctest work you'll need to add a handler for /308.

Why, then, do we use httpbin as an example URL? If someone copies and pastes the code, we'd like it to work on the real Internet too.

[jyn514]

@jsha thanks, that fixed it!

[algesten]

I'm happy to go ahead with this change. Thanks both for the work!