From 366898ced513d2ec07e62be330fa8d17b0dee1fa Mon Sep 17 00:00:00 2001 From: Nobuhiro Nikushi Date: Fri, 30 Nov 2018 11:18:49 +0900 Subject: [PATCH 1/5] Add failed test to reproduce bug raising NoMethodError Failures: 1) Mrkt::Authentication#authenticate on a unexpected non json response should raise an Error Failure/Error: expect { subject }.to raise_error(Mrkt::Errors::Error) expected Mrkt::Errors::Unknown, got # with backtrace: # ./lib/mrkt/concerns/authentication.rb:34:in `block in authenticate' # ./lib/mrkt/concerns/authentication.rb:31:in `tap' # ./lib/mrkt/concerns/authentication.rb:31:in `authenticate' # ./spec/concerns/authentication_spec.rb:5:in `block (3 levels) in ' # ./spec/concerns/authentication_spec.rb:22:in `block (5 levels) in ' # ./spec/concerns/authentication_spec.rb:22:in `block (4 levels) in ' # ./spec/concerns/authentication_spec.rb:22:in `block (4 levels) in ' --- spec/concerns/authentication_spec.rb | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/spec/concerns/authentication_spec.rb b/spec/concerns/authentication_spec.rb index 80d5653..9140807 100644 --- a/spec/concerns/authentication_spec.rb +++ b/spec/concerns/authentication_spec.rb @@ -8,6 +8,22 @@ it { is_expected.to be_success } end + context 'on a unexpected non json response' do + before do + @authentication_request_stub = stub_request(:get, "https://#{host}/identity/oauth/token") + .with(query: { client_id: client_id, client_secret: client_secret, grant_type: 'client_credentials' }) + .to_return( + status: 500, + headers: { content_type: 'text/plain' }, + body: 'something wrong', + ) + end + + it 'should raise an Error' do + expect { subject }.to raise_error(Mrkt::Errors::Unknown) + end + end + context 'when the token is invalid' do let(:authentication_stub) do { From f795d34ae0a4feb18c46f65ce411b3c4b5a83af8 Mon Sep 17 00:00:00 2001 From: Nobuhiro Nikushi Date: Fri, 30 Nov 2018 11:11:26 +0900 Subject: [PATCH 2/5] Add middleware to handle unexpected non-json response for edge case --- lib/mrkt/concerns/connection.rb | 1 + lib/mrkt/faraday_middleware.rb | 2 + .../exceptional_response.rb | 25 +++++++++++ .../exceptional_response_spec.rb | 41 +++++++++++++++++++ 4 files changed, 69 insertions(+) create mode 100644 lib/mrkt/faraday_middleware/exceptional_response.rb create mode 100644 spec/faraday_middleware/exceptional_response_spec.rb diff --git a/lib/mrkt/concerns/connection.rb b/lib/mrkt/concerns/connection.rb index c73380d..c9d23df 100644 --- a/lib/mrkt/concerns/connection.rb +++ b/lib/mrkt/concerns/connection.rb @@ -13,6 +13,7 @@ def init_connection conn.response :logger, @logger, (@log_options || {}) if @debug conn.response :mkto, content_type: /\bjson$/ + conn.response :mkto_exceptional_response conn.options.timeout = @options[:read_timeout] if @options.key?(:read_timeout) conn.options.open_timeout = @options[:open_timeout] if @options.key?(:open_timeout) diff --git a/lib/mrkt/faraday_middleware.rb b/lib/mrkt/faraday_middleware.rb index f87f47d..a841afd 100644 --- a/lib/mrkt/faraday_middleware.rb +++ b/lib/mrkt/faraday_middleware.rb @@ -3,9 +3,11 @@ module Mrkt module FaradayMiddleware autoload :Response, 'mrkt/faraday_middleware/response' + autoload :ExceptionalResponse, 'mrkt/faraday_middleware/exceptional_response' end if Faraday::Middleware.respond_to? :register_middleware Faraday::Response.register_middleware mkto: -> { Mrkt::FaradayMiddleware::Response } + Faraday::Response.register_middleware mkto_exceptional_response: -> { Mrkt::FaradayMiddleware::ExceptionalResponse } end end diff --git a/lib/mrkt/faraday_middleware/exceptional_response.rb b/lib/mrkt/faraday_middleware/exceptional_response.rb new file mode 100644 index 0000000..a5a9b6c --- /dev/null +++ b/lib/mrkt/faraday_middleware/exceptional_response.rb @@ -0,0 +1,25 @@ +module Mrkt + module FaradayMiddleware + # A middleware to handle exceptional non-json responses. + # In some cases, for example in trouble of marketo servers, we confirmed + # there is possibility that servers could respond with non json response + # unexpectedly. + class ExceptionalResponse < Faraday::Response::Middleware + ServerErrorStatuses = 500...600 + + def on_complete(env) + # Nothing to do by this handler if response content type is like json + return if env[:response_headers]['Content-Type'] =~ /\bjson$/ + + case env[:status] + when ServerErrorStatuses + raise Mrkt::Errors::Unknown, response_values(env) + end + end + + def response_values(env) + {:status => env.status, :headers => env.response_headers, :body => env.body} + end + end + end +end diff --git a/spec/faraday_middleware/exceptional_response_spec.rb b/spec/faraday_middleware/exceptional_response_spec.rb new file mode 100644 index 0000000..3965f13 --- /dev/null +++ b/spec/faraday_middleware/exceptional_response_spec.rb @@ -0,0 +1,41 @@ +describe Mrkt::FaradayMiddleware::ExceptionalResponse do + before do + response = lambda { |env| + [status, {'Content-Type' => content_type}, body] + } + @conn = Faraday.new do |b| + b.use Mrkt::FaradayMiddleware::ExceptionalResponse + b.adapter :test do |stub| + stub.get('/', &response) + end + end + end + + context 'with application/json and with server error' do + let(:content_type) { 'application/json' } + let(:status) { 500 } + let(:body) { '{}' } + + it 'does nothing' do + expect(@conn.get('/').body).to eq body + end + end + + context 'with non json content-type and with server error' do + let(:content_type) { 'text/plain' } + let(:status) { 500 } + let(:body) { 'something wrong' } + + it { expect { @conn.get('/') }.to raise_error(Mrkt::Errors::Unknown) } + end + + context 'with non json content-type but with successful response' do + let(:content_type) { 'text/html' } + let(:status) { 200 } + let(:body) { 'ok' } + + it 'does nothing' do + expect(@conn.get('/').body).to eq body + end + end +end From 5dbc28d0babd210599ba53a25db0285a4a941939 Mon Sep 17 00:00:00 2001 From: Nobuhiro Nikushi Date: Fri, 30 Nov 2018 11:26:00 +0900 Subject: [PATCH 3/5] Revert "Add failed test to reproduce bug raising NoMethodError" This reverts commit 366898ced513d2ec07e62be330fa8d17b0dee1fa. --- spec/concerns/authentication_spec.rb | 16 ---------------- 1 file changed, 16 deletions(-) diff --git a/spec/concerns/authentication_spec.rb b/spec/concerns/authentication_spec.rb index 9140807..80d5653 100644 --- a/spec/concerns/authentication_spec.rb +++ b/spec/concerns/authentication_spec.rb @@ -8,22 +8,6 @@ it { is_expected.to be_success } end - context 'on a unexpected non json response' do - before do - @authentication_request_stub = stub_request(:get, "https://#{host}/identity/oauth/token") - .with(query: { client_id: client_id, client_secret: client_secret, grant_type: 'client_credentials' }) - .to_return( - status: 500, - headers: { content_type: 'text/plain' }, - body: 'something wrong', - ) - end - - it 'should raise an Error' do - expect { subject }.to raise_error(Mrkt::Errors::Unknown) - end - end - context 'when the token is invalid' do let(:authentication_stub) do { From 04d3e154d3597f5edab1e8cc50badf56da373b90 Mon Sep 17 00:00:00 2001 From: Nobuhiro Nikushi Date: Fri, 30 Nov 2018 12:21:05 +0900 Subject: [PATCH 4/5] 4XX error as well --- lib/mrkt/faraday_middleware/exceptional_response.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/mrkt/faraday_middleware/exceptional_response.rb b/lib/mrkt/faraday_middleware/exceptional_response.rb index a5a9b6c..9f7c621 100644 --- a/lib/mrkt/faraday_middleware/exceptional_response.rb +++ b/lib/mrkt/faraday_middleware/exceptional_response.rb @@ -5,7 +5,7 @@ module FaradayMiddleware # there is possibility that servers could respond with non json response # unexpectedly. class ExceptionalResponse < Faraday::Response::Middleware - ServerErrorStatuses = 500...600 + ServerErrorStatuses = 400...600 def on_complete(env) # Nothing to do by this handler if response content type is like json From 46482c03ab9227d472b8a1b618ee76dc69d386c8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?KARASZI=20Istv=C3=A1n?= Date: Mon, 3 Dec 2018 12:23:01 +0900 Subject: [PATCH 5/5] Reflect review Co-Authored-By: nikushi --- spec/faraday_middleware/exceptional_response_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/faraday_middleware/exceptional_response_spec.rb b/spec/faraday_middleware/exceptional_response_spec.rb index 3965f13..9c89394 100644 --- a/spec/faraday_middleware/exceptional_response_spec.rb +++ b/spec/faraday_middleware/exceptional_response_spec.rb @@ -1,5 +1,5 @@ describe Mrkt::FaradayMiddleware::ExceptionalResponse do - before do + before do response = lambda { |env| [status, {'Content-Type' => content_type}, body] }