Skip to content

Follow 302 redirects #11

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

Closed
wants to merge 2 commits into from
Closed

Follow 302 redirects #11

wants to merge 2 commits into from

Conversation

p8952
Copy link

@p8952 p8952 commented Jul 9, 2013

fixes #10, the request method was not following 302 redirects causing an HTTP status code to be returned rather than marshaled data

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 0b5cb4e on p8952:302-redirect into * on rubygems:master*.

when Net::HTTPSuccess
response.body
when Net::HTTPRedirection
request(method, path, data, content_type, 'https://bundler.rubygems.org')
Copy link
Member

Choose a reason for hiding this comment

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

Why is the host hardcoded to https://bundler.rubygems.org? Shouldn't it use the Location header from the response, as specified in RFC 2616?

Copy link
Author

Choose a reason for hiding this comment

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

That is good point, I have updated the pull request.

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling c6ac76a on p8952:302-redirect into * on rubygems:master*.

when Net::HTTPSuccess
response.body
when Net::HTTPRedirection
request(method, path, data, content_type, response['location'])
Copy link
Member

Choose a reason for hiding this comment

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

The response['location'] will be a fully qualified URI, not just the host part. And the path could be completely different than in the initial request.

If you want me to merge this, you need to add some specs to prove that it works as expected.

Copy link
Contributor

Choose a reason for hiding this comment

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

It should have something like

RequestError = Class.new(StandardError)
BadUrl = Class.new(RequestError)  
def url_to_uri(url)
  begin
    uri = URI.parse(url)
  rescue URI::InvalidURIError, SocketError => e
    raise BadUrl.new(e)
  end
end
uri = url_to_uri(response['location'])
path = uri.path # or uri.request_uri to include the query
host = uri.host

though this won't handle meta content refresh

I would separate out the actual connection.request code to avoid too much recursion

As an aside, I see you're not handling all the Net:HTTP errors

HTTP_ERRORS = [Timeout::Error, Errno::EINVAL, Errno::ECONNRESET,
   Net::HTTPBadResponse, Net::HTTPHeaderSyntaxError, Net::ProtocolError]

I have elsewhere done something like

BadResponse = Class.new(RequestError)
BasicResponse = Struct.new('BasicResponse', :url, :code)
def get_response(url)
  uri = url_to_uri(url)
  Net::HTTP.get_response(uri)
rescue EOFError => e
  BasicResponse.new(url, '200')
rescue *HTTP_ERRORS => e
  log url.inspect + e.inspect
  raise BadResponse.new(e)
end

Also, why VERIFY_NONE ? just add require 'net/https' right?

@bf4 bf4 mentioned this pull request Jul 11, 2013
sferik added a commit that referenced this pull request Jul 12, 2013
@sferik
Copy link
Member

sferik commented Jul 12, 2013

Closed by #13.

@sferik sferik closed this Jul 12, 2013
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.

TypeError: incompatible marshal file format
4 participants