Follow relative locations correctly. Fixes #228 #230
Conversation
|
Thanks @ioquatix! I looked at your implementation, it looks good. Please fix the minor issues reported by Travis also: https://travis-ci.org/rack-test/rack-test/jobs/363348025 and I'll approve this change |
|
I don't know why but Travis didn't seem to re-run the tests. However I think I fixed all (most?) of the issues. |
|
@perlun Are we good to go with this? |
lib/rack/test.rb
Outdated
|
|
||
| # Compute the next location by appending the location header with the | ||
| # last request, as per https://tools.ietf.org/html/rfc7231#section-7.1.2 | ||
| next_location = URI.parse(last_request.url) + URI.parse(last_response['Location']) |
There was a problem hiding this comment.
I thought about this a bit more: does this work if Location is an absolute URL? It feels like it wouldn't. So please add a test case for that, and if it works => fine, but if not I think we'll need to adjust the implementation a bit.
There was a problem hiding this comment.
When you add two absolute locations the RHS is returned. That's the behaviour of URI RFC.
There was a problem hiding this comment.
2.5.0 :001 > require 'uri'
=> false
2.5.0 :002 > URI.parse("http://www.google.com") + URI.parse("http://www.yahoo.com")
=> #<URI::HTTP http://www.yahoo.com>
2.5.0 :003 >
There was a problem hiding this comment.
OK, sounds great! Would you mind adding a test that documents these semantics? You could even write a comment saying that '# Adding two absolute locations returns the right-hand location or something to the like.
|
@ioquatix Thanks for the update. I realized there might be some more issues with this - noted above. |
|
Okay done. |
|
Can we merge this? |
There was still a Travis issue, I've fixed that now and Travis is green again. Merging - thanks for your contributions! 👍 |
No description provided.