Handle the 'relative url' case when follow_redirect is enabled

[yannrouillard]

Here is a proposition of fix is to solve bug #140

[rogerleite]

Hi @yannrouillard!

Thanks for this PR. I looked at http://www.w3.org/Protocols/rfc2616/rfc2616-sec14.html section 14.30 Location, and RFC says that supports only absolute URI. Content-Location header supports relative URI and HTTPI don't look for it. I don't know what to do in this case.

Before we merge, it would be great to have this case on tests. Would you mind to code something like this https://github.com/savonrb/httpi/blob/master/spec/httpi/httpi_spec.rb#L233.

[yannrouillard]

Hi,

RFC2616 has been superseded by RFC7321 and this one mentions the relative url case:
https://tools.ietf.org/html/rfc7231#section-7.1.2

You're right for the test case, I will update the pull request to include such a test.

I also noticed that httpi doesn't properly honor the "303 See Other" response code. It should in that case always issues a GET request even if the original request was a POST.
I wonder if we should implement this behavior for 301 and 302 as it seems to be the usual browser behavior.
Do you want me to open a second PR for that ?

Another problem I had is that cookies are not automatically handled. I had to disable follow_redirect and do it manually so the cookie are taken into account. Do you think this should be done automatically by HTTPI ?

[rogerleite]

Hi, thanks for the link!

HTTPI's follow redirect option was discussed at this issue #67 and implemented because this wasabi's issue savonrb/wasabi#18. I don't think we have to be so "by the book" and change 30x actual behaviour. Maybe @tjarratt can help me here.

About the cookies, i think that HTTPI could have a follow_redirect_action that clients should customize the redirect action with ruby block or something like that. What do you think?

[tjarratt]

I'm completely okay with changing the behavior of things to match specifications. HTTP is a very complicated spec, and historically Savon, HTTPI (and other related gems) have only implemented as much as they needed to, leaving the rest for the community.

It seems like handling cookies would be a nice feature for HTTPI, if that's something you need @yannrouillard. A separate pull request for that would be good.

My only other suggestion would be a small test for this behavior. There's a lot of edge cases in following redirects, and having some tests that assert the correct behavior would benefit anyone that wants to refactor or modify this code later.

[coveralls]

Coverage increased (+0.04%) to 71.59% when pulling 63b6148 on yannrouillard:fix-140-relative_url_support_in_follow_redirect into d6a3825 on savonrb:master.

[yannrouillard]

I updated the pull request to include the relevant test cases and I also fixed the patch to that is work for path relative to the current url (and not only paths relative to the root of the current host).

I created a second branch https://github.com/yannrouillard/httpi/tree/fix-redirection-status-code-behavior for the problem about 303 redirection. I didn't implement a modification for 301 and 302 code status as it is a bit different than 303:

• all requests redirected with a 303 must be done with the GET method,
• whereas only POST requests may be transformed into GET for 301 and 302.

So httpi does respect the standard for 301 and 302 and, as it is was used first for Savon, I wonder if this implement the could break some current cases where POST API call are redirected (don't know if that cases exists that much).

[tjarratt]

Thanks for the pull request @yannrouillard! It is much appreciated. Glad to have some more improvements to HTTPI 😄

I haven't had a chance to look at your second branch -- when you get some time, do you want to submit a second PR for that?