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

[HOTFIX] Release cleanups and fixes #120

Merged
merged 6 commits into from
Apr 3, 2014
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion Gemfile
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ group :test do
gem 'json', '>= 1.8.1', :platforms => [:jruby, :rbx, :ruby_18, :ruby_19]
gem 'mime-types', '~> 1.25', :platforms => [:jruby, :ruby_18]
gem 'rspec', '>= 2.14'
gem 'rubocop', '>= 0.19', :platforms => [:ruby_19, :ruby_20, :ruby_21]
gem 'rubocop', '~> 0.19.0', :platforms => [:ruby_19, :ruby_20, :ruby_21]
gem 'simplecov', :require => false
gem 'yardstick'
end
Expand Down
2 changes: 1 addition & 1 deletion lib/http.rb
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@
require 'http/response'
require 'http/response/body'
require 'http/response/parser'
require 'http/backports' if RUBY_VERSION < '1.9.0'
require 'http/backports'

# HTTP should be easy
module HTTP
Expand Down
4 changes: 2 additions & 2 deletions lib/http/backports.rb
Original file line number Diff line number Diff line change
@@ -1,2 +1,2 @@
require 'http/backports/uri'
require 'http/backports/base64'
require 'http/backports/uri' if RUBY_VERSION < '1.9.3'
Copy link
Member

Choose a reason for hiding this comment

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

Scumbag 1.9.2 😢

Copy link
Member Author

Choose a reason for hiding this comment

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

Just for the record, pasting here same I shared on IRC :D

URI.decode URI.encode_www_form :a => [1,2]

On Ruby >= 1.9.3, this outputs: a=1&b=2, which is pretty expected, on Ruby = 1.9.2 it outputs something absolutely insane: a=[1,+2]

Copy link
Member

Choose a reason for hiding this comment

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

I will note that 1.9.2 is widely considered to be broken in general. This is patching 1.9.2's URI.encode_www_form to have a non-broken behavior. If people are depending on the broken behaviors of a broken Ruby VM, this could be a potentially breaking change. I'm not sure how much I care about breaking the code of people depending on the broken behaviors of a broken VM, however. I would personally prefer to drop support for 1.9.2 entirely.

require 'http/backports/base64' if RUBY_VERSION < '1.9.0'
82 changes: 37 additions & 45 deletions lib/http/client.rb
Original file line number Diff line number Diff line change
Expand Up @@ -19,49 +19,27 @@ def initialize(default_options = {})
end

# Make an HTTP request
def request(verb, uri, options = {})
opts = @default_options.merge(options)
def request(verb, uri, opts = {})
opts = @default_options.merge(opts)
uri = make_request_uri(uri, opts)
headers = opts.headers
proxy = opts.proxy
proxy = opts.proxy
body = make_request_body(opts, headers)

request_body = make_request_body(opts, headers)
uri, opts = normalize_get_params(uri, opts) if verb == :get
req = HTTP::Request.new(verb, uri, headers, proxy, body)
res = perform req, opts

uri = "#{uri}?#{URI.encode_www_form(opts.params)}" if opts.params && !opts.params.empty?

request = HTTP::Request.new(verb, uri, headers, proxy, request_body)
perform request, opts
end

# Perform the HTTP request (following redirects if needed)
def perform(req, options)
res = perform_without_following_redirects req, options

if options.follow
res = Redirector.new(options.follow).perform req, res do |request|
perform_without_following_redirects request, options
if opts.follow
res = Redirector.new(opts.follow).perform req, res do |request|
perform request, opts
end
end

res
end

# Read a chunk of the body
def readpartial(size = BUFFER_SIZE)
return unless @socket

read_more size
chunk = @parser.chunk

finish_response if @parser.finished?

chunk
end

private

# Perform a single (no follow) HTTP request
def perform_without_following_redirects(req, options)
def perform(req, options)
# finish previous response if client was re-used
# TODO: this is pretty wrong, as socket shoud be part of response
# connection, so that re-use of client will not break multiple
Expand Down Expand Up @@ -90,6 +68,20 @@ def perform_without_following_redirects(req, options)
res
end

# Read a chunk of the body
def readpartial(size = BUFFER_SIZE)
return unless @socket

read_more size
chunk = @parser.chunk

finish_response if @parser.finished?

chunk
end

private

# Initialize TLS connection
def start_tls(socket, options)
# TODO: abstract away SSLContexts so we can use other TLS libraries
Expand All @@ -100,6 +92,18 @@ def start_tls(socket, options)
socket
end

# Merges query params if needed
def make_request_uri(uri, options)
uri = URI uri.to_s unless uri.is_a? URI

if options.params && !options.params.empty?
params = CGI.parse(uri.query.to_s).merge(options.params || {})
uri.query = URI.encode_www_form params
end

uri
end

# Create the request body object to send
def make_request_body(opts, headers)
if opts.body
Expand Down Expand Up @@ -128,17 +132,5 @@ def read_more(size)
rescue EOFError
false
end

# Moves uri get params into the opts.params hash
# @return [Array<URI, Hash>]
def normalize_get_params(uri, opts)
uri = URI(uri) unless uri.is_a?(URI)
if uri.query
extracted_params_from_uri = Hash[URI.decode_www_form(uri.query)]
opts = opts.with_params(extracted_params_from_uri.merge(opts.params || {}))
uri.query = nil
end
[uri, opts]
end
end
end
9 changes: 9 additions & 0 deletions spec/http/backports/uri_spec.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
require 'spec_helper'

describe URI do
describe '.encode_www_form' do
it 'properly encodes arrays' do
expect(URI.encode_www_form :a => [:b, :c]).to eq 'a=b&a=c'
end
end
end
39 changes: 30 additions & 9 deletions spec/http/client_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@

describe HTTP::Client do
StubbedClient = Class.new(HTTP::Client) do
def perform_without_following_redirects(request, options)
def perform(request, options)
stubs.fetch(request.uri.to_s) { super(request, options) }
end

Expand Down Expand Up @@ -70,27 +70,48 @@ def simple_response(body, status = 200)
end

describe 'parsing params' do
let(:client) { HTTP::Client.new }
before { allow(client).to receive :perform }

it 'accepts params within the provided URL' do
client = HTTP::Client.new
allow(client).to receive(:perform)
expect(HTTP::Request).to receive(:new) do |_, uri|
params = CGI.parse(URI(uri).query)
expect(params).to eq('foo' => ['bar'])
expect(CGI.parse uri.query).to eq('foo' => %w[bar])
end

client.get('http://example.com/?foo=bar')
end

it 'combines GET params from the URI with the passed in params' do
client = HTTP::Client.new
allow(client).to receive(:perform)
expect(HTTP::Request).to receive(:new) do |_, uri|
params = CGI.parse(URI(uri).query)
expect(params).to eq('foo' => ['bar'], 'baz' => ['quux'])
expect(CGI.parse uri.query).to eq('foo' => %w[bar], 'baz' => %w[quux])
end

client.get('http://example.com/?foo=bar', :params => {:baz => 'quux'})
end

it 'merges duplicate values' do
expect(HTTP::Request).to receive(:new) do |_, uri|
expect(CGI.parse uri.query).to eq('a' => %w[1 2])
end

client.get('http://example.com/?a=1', :params => {:a => 2})
end

it 'does not modifies query part if no params were given' do
expect(HTTP::Request).to receive(:new) do |_, uri|
expect(uri.query).to eq 'deadbeef'
end

client.get('http://example.com/?deadbeef')
end

it 'does not corrupts index-less arrays' do
expect(HTTP::Request).to receive(:new) do |_, uri|
expect(CGI.parse uri.query).to eq 'a[]' => %w[b c], 'd' => %w[e]
end

client.get('http://example.com/?a[]=b&a[]=c', :params => {:d => 'e'})
end
end

describe 'passing json' do
Expand Down