From a218c003e52775509e89c16b18e1cba98c7ae765 Mon Sep 17 00:00:00 2001 From: Nathan Woodhull Date: Tue, 16 Jan 2024 22:59:18 -0500 Subject: [PATCH] Add typed exceptions for 403 and 5xx responses Raise AuthorizationError on 403 and a new ServerError on any 5xx, so callers can distinguish an unauthorized API key from a transient server-side failure worth retrying. ServerError subclasses ResponseError so existing rescues continue to catch 5xx responses. Refactor on_complete's status dispatch into helper methods to satisfy RuboCop's complexity cops, and tidy the spec's layout offenses. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../response/raise_error.rb | 49 ++++++++++++------- spec/response/raise_error_spec.rb | 36 ++++++++++++-- 2 files changed, 63 insertions(+), 22 deletions(-) diff --git a/lib/action_network_rest/response/raise_error.rb b/lib/action_network_rest/response/raise_error.rb index 0aa8416..1b376e2 100644 --- a/lib/action_network_rest/response/raise_error.rb +++ b/lib/action_network_rest/response/raise_error.rb @@ -3,34 +3,42 @@ module ActionNetworkRest module Response class RaiseError < Faraday::Middleware - # rubocop:disable Style/GuardClause def on_complete(response) status_code = response[:status].to_i + return unless (400...600).cover?(status_code) - if (400...600).cover? status_code - if status_code == 400 && response[:body].include?('error') - error_hsh = JSON.parse(response[:body]) - error_message = error_hsh['error'] - - if error_message == 'You must specify a valid person id' - raise MustSpecifyValidPersonId, error_message(response) - end - elsif status_code == 404 - raise NotFoundError, error_message(response) - elsif status_code == 429 - raise TooManyRequests, error_message(response) - else - raise ResponseError, error_message(response) - end - end + raise error_class_for(status_code, response), error_message(response) end - # rubocop:enable Style/GuardClause def error_message(response) "#{response[:method].to_s.upcase} #{response[:url]}: #{response[:status]} \n\n #{response[:body]}" end + + private + + def error_class_for(status_code, response) + case status_code + when 400 then bad_request_error_class(response) + when 403 then AuthorizationError + when 404 then NotFoundError + when 429 then TooManyRequests + when 500..599 then ServerError + else ResponseError + end + end + + def bad_request_error_class(response) + return ResponseError unless response[:body].include?('error') + + # A few specific 400 messages map to a dedicated exception; any other + # 400 still raises the generic ResponseError. + body_error = JSON.parse(response[:body])['error'] + body_error == 'You must specify a valid person id' ? MustSpecifyValidPersonId : ResponseError + end end + class AuthorizationError < StandardError; end + class MissingActionNetworkId < StandardError; end class MustSpecifyValidPersonId < StandardError; end @@ -39,6 +47,11 @@ class NotFoundError < StandardError; end class ResponseError < StandardError; end + # Raised for 5xx responses. Subclasses ResponseError so existing rescues + # continue to catch server errors while allowing callers to distinguish + # transient server-side failures (worth retrying) from client errors. + class ServerError < ResponseError; end + class TooManyRequests < StandardError; end class UsedAllRequestTries < StandardError; end diff --git a/spec/response/raise_error_spec.rb b/spec/response/raise_error_spec.rb index a3f6fa2..73cf36a 100644 --- a/spec/response/raise_error_spec.rb +++ b/spec/response/raise_error_spec.rb @@ -15,6 +15,20 @@ .to(raise_error(ActionNetworkRest::Response::MustSpecifyValidPersonId, /You must specify a valid person id/)) end + it 'should raise ResponseError for a 400 whose error message is not specially handled' do + response = { status: '400', body: { error: 'Some other validation error' }.to_json } + + expect { subject.on_complete(response) } + .to(raise_error(ActionNetworkRest::Response::ResponseError, /Some other validation error/)) + end + + it 'should raise AuthorizationError if status is 403' do + response = { status: '403', body: { error: 'API Key invalid or not present' }.to_json } + + expect { subject.on_complete(response) } + .to(raise_error(ActionNetworkRest::Response::AuthorizationError, /API Key invalid/)) + end + it 'should raise NotFoundError if status is 404' do response = { status: '404', body: { error: 'Not found' }.to_json } @@ -29,13 +43,27 @@ end.to raise_error(ActionNetworkRest::Response::TooManyRequests, /Too many requests/) end - %w[418 500].each do |generic_error_code| - it "should raise ResponseError for generic error with status #{generic_error_code}" do - response = { status: generic_error_code, body: { error: 'Something went wrong' }.to_json } + %w[500 502 503].each do |server_error_code| + it "should raise ServerError for server error with status #{server_error_code}" do + response = { status: server_error_code, body: { error: 'Something went wrong' }.to_json } expect { subject.on_complete(response) } - .to(raise_error(ActionNetworkRest::Response::ResponseError, /Something went wrong/)) + .to(raise_error(ActionNetworkRest::Response::ServerError, /Something went wrong/)) end end + + it 'ServerError should be a ResponseError so existing rescues still catch 5xx' do + response = { status: '500', body: { error: 'Something went wrong' }.to_json } + + expect { subject.on_complete(response) } + .to(raise_error(ActionNetworkRest::Response::ResponseError)) + end + + it 'should raise ResponseError for a generic client error with status 418' do + response = { status: '418', body: { error: 'Something went wrong' }.to_json } + + expect { subject.on_complete(response) } + .to(raise_error(ActionNetworkRest::Response::ResponseError, /Something went wrong/)) + end end end