From b60b859890572aa8278d3e01cc417ec14336e8eb Mon Sep 17 00:00:00 2001 From: Aleksey V Zapparov Date: Thu, 3 Apr 2014 01:44:39 +0200 Subject: [PATCH 1/6] Get rid of perform_without_follow separation --- lib/http/client.rb | 55 +++++++++++++++++++--------------------- spec/http/client_spec.rb | 2 +- 2 files changed, 27 insertions(+), 30 deletions(-) diff --git a/lib/http/client.rb b/lib/http/client.rb index 34671339..b323814a 100644 --- a/lib/http/client.rb +++ b/lib/http/client.rb @@ -19,49 +19,32 @@ 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) headers = opts.headers - proxy = opts.proxy + proxy = opts.proxy request_body = make_request_body(opts, headers) uri, opts = normalize_get_params(uri, opts) if verb == :get - uri = "#{uri}?#{URI.encode_www_form(opts.params)}" if opts.params && !opts.params.empty? + if opts.params && !opts.params.empty? + uri = "#{uri}?#{URI.encode_www_form(opts.params)}" + end - request = HTTP::Request.new(verb, uri, headers, proxy, request_body) - perform request, opts - end + req = HTTP::Request.new(verb, uri, headers, proxy, request_body) + res = perform req, opts - # 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 @@ -90,6 +73,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 diff --git a/spec/http/client_spec.rb b/spec/http/client_spec.rb index 9d4abcc1..1f6cb41b 100644 --- a/spec/http/client_spec.rb +++ b/spec/http/client_spec.rb @@ -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 From 3161afae663818638b9af56c025e138f6673e21a Mon Sep 17 00:00:00 2001 From: Aleksey V Zapparov Date: Thu, 3 Apr 2014 02:07:42 +0200 Subject: [PATCH 2/6] Fix request params merge --- lib/http/client.rb | 35 +++++++++++++++-------------------- 1 file changed, 15 insertions(+), 20 deletions(-) diff --git a/lib/http/client.rb b/lib/http/client.rb index b323814a..ed997c30 100644 --- a/lib/http/client.rb +++ b/lib/http/client.rb @@ -21,17 +21,12 @@ def initialize(default_options = {}) # Make an HTTP request def request(verb, uri, opts = {}) opts = @default_options.merge(opts) + uri = make_request_uri(uri, opts) headers = opts.headers 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 - - if opts.params && !opts.params.empty? - uri = "#{uri}?#{URI.encode_www_form(opts.params)}" - end - - req = HTTP::Request.new(verb, uri, headers, proxy, request_body) + req = HTTP::Request.new(verb, uri, headers, proxy, body) res = perform req, opts if opts.follow @@ -97,6 +92,18 @@ def start_tls(socket, options) socket end + # Merges query params if needed + def make_request_uri(uri, options) + uri = uri.is_a?(URI) ? uri : URI(uri.to_s) + + 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 @@ -125,17 +132,5 @@ def read_more(size) rescue EOFError false end - - # Moves uri get params into the opts.params hash - # @return [Array] - 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 From be751694500aa6d47993796bf1380cdd1400376c Mon Sep 17 00:00:00 2001 From: Aleksey V Zapparov Date: Thu, 3 Apr 2014 02:40:25 +0200 Subject: [PATCH 3/6] Fix client specs for ruby 1.8.7 --- lib/http/client.rb | 2 +- spec/http/client_spec.rb | 11 +++++------ 2 files changed, 6 insertions(+), 7 deletions(-) diff --git a/lib/http/client.rb b/lib/http/client.rb index ed997c30..18862c17 100644 --- a/lib/http/client.rb +++ b/lib/http/client.rb @@ -94,7 +94,7 @@ def start_tls(socket, options) # Merges query params if needed def make_request_uri(uri, options) - uri = uri.is_a?(URI) ? uri : URI(uri.to_s) + 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 || {}) diff --git a/spec/http/client_spec.rb b/spec/http/client_spec.rb index 1f6cb41b..02a90e4a 100644 --- a/spec/http/client_spec.rb +++ b/spec/http/client_spec.rb @@ -70,11 +70,12 @@ 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) + params = CGI.parse uri.query expect(params).to eq('foo' => ['bar']) end @@ -82,10 +83,8 @@ def simple_response(body, status = 200) 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) + params = CGI.parse uri.query expect(params).to eq('foo' => ['bar'], 'baz' => ['quux']) end From c89e1098aa5ec505b9c26c4d04b8a57cd9faf89d Mon Sep 17 00:00:00 2001 From: Aleksey V Zapparov Date: Thu, 3 Apr 2014 02:51:45 +0200 Subject: [PATCH 4/6] Freeze rubocop version to ~> 0.19.0 --- Gemfile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Gemfile b/Gemfile index b0918ddb..5ccd6995 100644 --- a/Gemfile +++ b/Gemfile @@ -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 From 619b2d5d145798229ce7c3910dfa5887ea1d9cfb Mon Sep 17 00:00:00 2001 From: Aleksey V Zapparov Date: Thu, 3 Apr 2014 03:16:53 +0200 Subject: [PATCH 5/6] Add more specs on params handling --- spec/http/client_spec.rb | 30 ++++++++++++++++++++++++++---- 1 file changed, 26 insertions(+), 4 deletions(-) diff --git a/spec/http/client_spec.rb b/spec/http/client_spec.rb index 02a90e4a..027b0882 100644 --- a/spec/http/client_spec.rb +++ b/spec/http/client_spec.rb @@ -75,8 +75,7 @@ def simple_response(body, status = 200) it 'accepts params within the provided URL' do expect(HTTP::Request).to receive(:new) do |_, uri| - params = CGI.parse 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') @@ -84,12 +83,35 @@ def simple_response(body, status = 200) it 'combines GET params from the URI with the passed in params' do expect(HTTP::Request).to receive(:new) do |_, uri| - params = CGI.parse 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 From 529e6d8c04f91b13a6542446a4bb75d03bebe5ec Mon Sep 17 00:00:00 2001 From: Aleksey V Zapparov Date: Thu, 3 Apr 2014 04:04:55 +0200 Subject: [PATCH 6/6] Use URI backport for rubies < 1.9.3 --- lib/http.rb | 2 +- lib/http/backports.rb | 4 ++-- spec/http/backports/uri_spec.rb | 9 +++++++++ 3 files changed, 12 insertions(+), 3 deletions(-) create mode 100644 spec/http/backports/uri_spec.rb diff --git a/lib/http.rb b/lib/http.rb index dc0c6eaf..f4c8460f 100644 --- a/lib/http.rb +++ b/lib/http.rb @@ -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 diff --git a/lib/http/backports.rb b/lib/http/backports.rb index d1d517ea..2f61bb67 100644 --- a/lib/http/backports.rb +++ b/lib/http/backports.rb @@ -1,2 +1,2 @@ -require 'http/backports/uri' -require 'http/backports/base64' +require 'http/backports/uri' if RUBY_VERSION < '1.9.3' +require 'http/backports/base64' if RUBY_VERSION < '1.9.0' diff --git a/spec/http/backports/uri_spec.rb b/spec/http/backports/uri_spec.rb new file mode 100644 index 00000000..a295cf27 --- /dev/null +++ b/spec/http/backports/uri_spec.rb @@ -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