From a8c53ce28de020e0f43eec7cb5e69d1444e19928 Mon Sep 17 00:00:00 2001 From: Selva Date: Mon, 5 Sep 2011 22:34:01 +0530 Subject: [PATCH] [HTTP] Added HTTP client agnostic implementation * Get rid of dependency on RestClient implementation * Implemented Tire::HTTP::Response * Made default implementation(RestClient) to use Tire::HTTP::Response * All API calls returns false on failure instead of raising exceptions Closes #98. --- .gitignore | 3 ++- lib/tire.rb | 1 + lib/tire/client.rb | 23 ++++++++++++++++----- lib/tire/http/response.rb | 10 ++++++++++ lib/tire/index.rb | 27 ++++++++++--------------- lib/tire/search.rb | 11 +++++----- test/integration/index_store_test.rb | 14 +++++++++---- test/integration/percolator_test.rb | 4 +--- test/unit/index_test.rb | 30 ++++++++++++---------------- test/unit/model_persistence_test.rb | 3 ++- test/unit/search_test.rb | 6 +++--- test/unit/tire_http_response_test.rb | 19 ++++++++++++++++++ 12 files changed, 96 insertions(+), 55 deletions(-) create mode 100644 lib/tire/http/response.rb create mode 100644 test/unit/tire_http_response_test.rb diff --git a/.gitignore b/.gitignore index 92745cfb..e47d58dd 100644 --- a/.gitignore +++ b/.gitignore @@ -9,4 +9,5 @@ coverage/ scratch/ examples/*.html *.log -.rvmrc \ No newline at end of file +.rvmrc +tags diff --git a/lib/tire.rb b/lib/tire.rb index 868804d1..3dfc162f 100644 --- a/lib/tire.rb +++ b/lib/tire.rb @@ -6,6 +6,7 @@ require 'tire/rubyext/symbol' require 'tire/logger' require 'tire/configuration' +require 'tire/http/response' require 'tire/client' require 'tire/search' require 'tire/search/query' diff --git a/lib/tire/client.rb b/lib/tire/client.rb index bc992965..51ac56e5 100644 --- a/lib/tire/client.rb +++ b/lib/tire/client.rb @@ -3,20 +3,33 @@ module Tire module Client class RestClient + def self.get(url, data=nil) - ::RestClient::Request.new(:method => :get, :url => url, :payload => data).execute + tire_http_response ::RestClient::Request.new(:method => :get, :url => url, :payload => data).execute(&just_response) end + def self.post(url, data) - ::RestClient.post url, data + tire_http_response ::RestClient.post(url, data, &just_response) end + def self.put(url, data) - ::RestClient.put url, data + tire_http_response ::RestClient.put(url, data, &just_response) end + def self.delete(url) - ::RestClient.delete url rescue nil + tire_http_response ::RestClient.delete(url, &just_response) end + def self.head(url) - ::RestClient.head url + tire_http_response ::RestClient.head(url, &just_response) + end + + def self.tire_http_response(response) + Tire::HTTP::Response.new response.body, response.code, response.headers + end + + def self.just_response + lambda {|response, request, result| response } end end diff --git a/lib/tire/http/response.rb b/lib/tire/http/response.rb new file mode 100644 index 00000000..8d3c237c --- /dev/null +++ b/lib/tire/http/response.rb @@ -0,0 +1,10 @@ +module Tire + module HTTP + class Response + attr_reader :body, :code, :headers + def initialize(body, code, headers = {}) + @body, @code, @headers = body, code, headers + end + end + end +end diff --git a/lib/tire/index.rb b/lib/tire/index.rb index fab785e9..ad8b8380 100644 --- a/lib/tire/index.rb +++ b/lib/tire/index.rb @@ -9,30 +9,24 @@ def initialize(name, &block) end def exists? - !!Configuration.client.head("#{Configuration.url}/#{@name}") - rescue Exception => error - false + Configuration.client.head("#{Configuration.url}/#{@name}").code == 200 end def delete - # FIXME: RestClient does not return response for DELETE requests? @response = Configuration.client.delete "#{Configuration.url}/#{@name}" - return @response.body =~ /error/ ? false : true - rescue Exception => error - false + return true if @response.code == 200 ensure curl = %Q|curl -X DELETE "#{Configuration.url}/#{@name}"| - logged(error, 'DELETE', curl) + logged(@response.body, 'DELETE', curl) end def create(options={}) @options = options @response = Configuration.client.post "#{Configuration.url}/#{@name}", MultiJson.encode(options) - rescue Exception => error - false + @response.code == 200 ? @response : false ensure curl = %Q|curl -X POST "#{Configuration.url}/#{@name}" -d '#{MultiJson.encode(options)}'| - logged(error, 'CREATE', curl) + logged(@response.body, 'CREATE', curl) end def mapping @@ -83,16 +77,17 @@ def bulk_store documents count = 0 begin - Configuration.client.post("#{Configuration.url}/_bulk", payload.join("\n")) + response = Configuration.client.post("#{Configuration.url}/_bulk", payload.join("\n")) + raise response.body if response.code != 200 + response rescue Exception => error if count < tries count += 1 - STDERR.puts "[ERROR] #{error.message}:#{error.http_body rescue nil}, retrying (#{count})..." + STDERR.puts "[ERROR] #{error.message}, retrying (#{count})..." retry else STDERR.puts "[ERROR] Too many exceptions occured, giving up..." - STDERR.puts "Response: #{error.http_body rescue nil}" - raise + STDERR.puts "Response: #{error.message}" end ensure curl = %Q|curl -X POST "#{Configuration.url}/_bulk" -d '{... data omitted ...}'| @@ -135,7 +130,7 @@ def remove(*args) raise ArgumentError, "Please pass a document ID" unless id result = Configuration.client.delete "#{Configuration.url}/#{@name}/#{type}/#{id}" - MultiJson.decode(result) if result + MultiJson.decode(result.body) if result.code == 200 end def retrieve(type, id) diff --git a/lib/tire/search.rb b/lib/tire/search.rb index 4614c666..070cc782 100644 --- a/lib/tire/search.rb +++ b/lib/tire/search.rb @@ -68,14 +68,15 @@ def fields(*fields) def perform @response = Configuration.client.get(@url, self.to_json) + if(@response.code != 200) + STDERR.puts "[REQUEST FAILED] #{self.to_curl}\n" + return false + end @json = MultiJson.decode(@response.body) @results = Results::Collection.new(@json, @options) - self - rescue Exception => error - STDERR.puts "[REQUEST FAILED] #{self.to_curl}\n" - raise + return self ensure - logged(error) + logged(@response.body) end def to_curl diff --git a/test/integration/index_store_test.rb b/test/integration/index_store_test.rb index 5f3b25e2..0ac1dfee 100644 --- a/test/integration/index_store_test.rb +++ b/test/integration/index_store_test.rb @@ -7,10 +7,7 @@ class IndexStoreIntegrationTest < Test::Unit::TestCase context "Storing the documents in index" do - teardown { Tire.index('articles-test-ids').delete } - - should "store hashes under their IDs" do - + setup do Tire.index 'articles-test-ids' do delete create @@ -23,7 +20,16 @@ class IndexStoreIntegrationTest < Test::Unit::TestCase refresh end + end + teardown { Tire.index('articles-test-ids').delete } + + should "exists if looked up by the index" do + assert Tire.index("articles-test-ids").exists? + assert !Tire.index("invalid-index").exists? + end + + should "store hashes under their IDs" do s = Tire.search('articles-test-ids') { query { string '*' } } assert_equal 4, s.results.count diff --git a/test/integration/percolator_test.rb b/test/integration/percolator_test.rb index 949b1560..ee8fc4fc 100644 --- a/test/integration/percolator_test.rb +++ b/test/integration/percolator_test.rb @@ -43,9 +43,7 @@ class PercolatorIntegrationTest < Test::Unit::TestCase assert @index.unregister_percolator_query('alert') Tire.index('_percolator').refresh - assert_raise(RestClient::ResourceNotFound) do - Configuration.client.get("#{Configuration.url}/_percolator/percolator-test/alert") - end + assert Configuration.client.get("#{Configuration.url}/_percolator/percolator-test/alert").code != 200 end end diff --git a/test/unit/index_test.rb b/test/unit/index_test.rb index 6d1493e6..149fe88f 100644 --- a/test/unit/index_test.rb +++ b/test/unit/index_test.rb @@ -20,7 +20,7 @@ class IndexTest < Test::Unit::TestCase end should "return false when does not exist" do - Configuration.client.expects(:head).raises(RestClient::ResourceNotFound) + Configuration.client.expects(:head).returns(mock_response('', 404)) assert ! @index.exists? end @@ -30,7 +30,7 @@ class IndexTest < Test::Unit::TestCase end should "not raise exception and just return false when trying to create existing index" do - Configuration.client.expects(:post).raises(RestClient::BadRequest) + Configuration.client.expects(:post).returns(mock_response('{"error":"IndexAlreadyExistsException[\'dummy\']"}', 400)) assert_nothing_raised { assert ! @index.create } end @@ -40,9 +40,7 @@ class IndexTest < Test::Unit::TestCase end should "not raise exception and just return false when deleting non-existing index" do - Configuration.client.expects(:delete).returns(mock_response('{"error":"[articles] missing"}')) - assert_nothing_raised { assert ! @index.delete } - Configuration.client.expects(:delete).raises(RestClient::BadRequest) + Configuration.client.expects(:delete).returns(mock_response('{"error":"[articles] missing"}', 404)) assert_nothing_raised { assert ! @index.delete } end @@ -229,33 +227,33 @@ class MyDocument;end; document = MyDocument.new should "get type from document" do Configuration.client.expects(:delete).with("#{Configuration.url}/dummy/article/1"). - returns('{"ok":true,"_id":"1"}').twice + returns(mock_response('{"ok":true,"_id":"1"}')).twice @index.remove :id => 1, :type => 'article', :title => 'Test' @index.remove :id => 1, :type => 'article', :title => 'Test' end should "set default type" do Configuration.client.expects(:delete).with("#{Configuration.url}/dummy/document/1"). - returns('{"ok":true,"_id":"1"}') + returns(mock_response('{"ok":true,"_id":"1"}')) @index.remove :id => 1, :title => 'Test' end should "get ID from hash" do Configuration.client.expects(:delete).with("#{Configuration.url}/dummy/document/1"). - returns('{"ok":true,"_id":"1"}') + returns(mock_response('{"ok":true,"_id":"1"}')) @index.remove :id => 1 end should "get ID from method" do document = stub('document', :id => 1) Configuration.client.expects(:delete).with("#{Configuration.url}/dummy/document/1"). - returns('{"ok":true,"_id":"1"}') + returns(mock_response('{"ok":true,"_id":"1"}')) @index.remove document end should "get type and ID from arguments" do Configuration.client.expects(:delete).with("#{Configuration.url}/dummy/article/1"). - returns('{"ok":true,"_id":"1"}') + returns(mock_response('{"ok":true,"_id":"1"}')) @index.remove :article, 1 end @@ -288,7 +286,7 @@ class MyDocument;end; document = MyDocument.new json =~ /"id":"2"/ && json =~ /"title":"One"/ && json =~ /"title":"Two"/ - end.returns('{}') + end.returns(mock_response('{}'), 200) @index.bulk_store [ {:id => '1', :title => 'One'}, {:id => '2', :title => 'Two'} ] @@ -305,7 +303,7 @@ class MyDocument;end; document = MyDocument.new json =~ /"id":"2"/ && json =~ /"title":"One"/ && json =~ /"title":"Two"/ - end.returns('{}') + end.returns(mock_response('{}', 200)) one = ActiveModelArticle.new 'title' => 'One'; one.id = '1' two = ActiveModelArticle.new 'title' => 'Two'; two.id = '2' @@ -315,15 +313,13 @@ class MyDocument;end; document = MyDocument.new end should "try again when an exception occurs" do - Configuration.client.expects(:post).raises(RestClient::RequestFailed).at_least(2) + Configuration.client.expects(:post).returns(mock_response('', 404)).at_least(2) - assert_raise(RestClient::RequestFailed) do - @index.bulk_store [ {:id => '1', :title => 'One'}, {:id => '2', :title => 'Two'} ] - end + assert !@index.bulk_store([ {:id => '1', :title => 'One'}, {:id => '2', :title => 'Two'} ]) end should "display error message when collection item does not have ID" do - Configuration.client.expects(:post).with { |url, json| url == "#{Configuration.url}/_bulk" } + Configuration.client.expects(:post).with{ |url, json| url == "#{Configuration.url}/_bulk" }.returns(mock_response('success', 200)) STDERR.expects(:puts).once documents = [ { :title => 'Bogus' }, { :title => 'Real', :id => 1 } ] diff --git a/test/unit/model_persistence_test.rb b/test/unit/model_persistence_test.rb index c8bfb479..54be96f1 100644 --- a/test/unit/model_persistence_test.rb +++ b/test/unit/model_persistence_test.rb @@ -411,7 +411,8 @@ class << a end.returns(mock_response('{"ok":true,"_id":"123"}')) Configuration.client.expects(:delete). - with("#{Configuration.url}/persistent_articles/persistent_article/123") + with("#{Configuration.url}/persistent_articles/persistent_article/123"). + returns(mock_response('{"ok":true,"acknowledged":true}', 200)) article = PersistentArticle.new :id => '123', :title => 'Test' article.save diff --git a/test/unit/search_test.rb b/test/unit/search_test.rb index cd48ad2d..4cce8c0a 100644 --- a/test/unit/search_test.rb +++ b/test/unit/search_test.rb @@ -110,12 +110,12 @@ def foo; 'bar'; end assert_not_nil s.response end - should "print debugging information on exception and re-raise it" do - Configuration.client.expects(:get).raises(RestClient::InternalServerError) + should "print debugging information on exception and retun false" do + Configuration.client.expects(:get).returns(mock_response('failed', '404')) STDERR.expects(:puts) s = Search::Search.new('index') - assert_raise(RestClient::InternalServerError) { s.perform } + assert !s.perform end should "log request, but not response, when logger is set" do diff --git a/test/unit/tire_http_response_test.rb b/test/unit/tire_http_response_test.rb new file mode 100644 index 00000000..66affe4b --- /dev/null +++ b/test/unit/tire_http_response_test.rb @@ -0,0 +1,19 @@ +require 'test_helper' + +module Tire + module HTTP + class ResponseTest < Test::Unit::TestCase + context "construction" do + should "take response body, code and headers" do + response = Response.new "http response body", + 200, + :content_length => 20, + :content_encoding => 'gzip' + + assert_equal "http response body", response.body + assert_equal 200, response.code + end + end + end + end +end