From ff1c60933871d9362d4c1d70cf3f72d6c460a9a0 Mon Sep 17 00:00:00 2001 From: Alex Palladii Date: Tue, 14 Jun 2022 11:08:58 +0200 Subject: [PATCH] Pass HTTP status to Oktakit::Error.from_response() [description below] This fixes https://github.com/Shopify/oktakit/issues/59 by modifying `Oktakit::Error.from_response()` method to accept and process client response and HTTP status. The reason behind these changes: ```ruby response, http_status = client.get_user('example@example.com') ``` This usage method is listed in the repo's README and works in most cases. However, it separates response from HTTP status. `response` is a `Sawyer::Resource` object and doesn't contain any data about the request (HTTP status, URL, method or headers). It only has a Hash with Okta error code, error summary, internal error link, error id and error causes, which causes a `TypeError` if we try to `raise Oktakit::Error.from_response(response) unless http_status == 200` in case the user is not found. With these changes implemented, we will gain the ability to process different response formats without causing errors on the client side. --- lib/oktakit/error.rb | 42 ++++++------- lib/oktakit/response/raise_error.rb | 4 +- spec/error_spec.rb | 91 +++++++++++++++++++++++++++++ 3 files changed, 115 insertions(+), 22 deletions(-) create mode 100644 spec/error_spec.rb diff --git a/lib/oktakit/error.rb b/lib/oktakit/error.rb index bdd69c7..c11ac1f 100644 --- a/lib/oktakit/error.rb +++ b/lib/oktakit/error.rb @@ -6,11 +6,12 @@ class Error < StandardError # # @param [Hash] response HTTP response # @return [Oktakit::Error] - def self.from_response(response) - status = response[:status].to_i - if (klass = error(status)) - klass.new(response) - end + def self.from_response(response, status) + status = status.to_i + + return if response.nil? || status.nil? || status.zero? + + (klass = error(status)) ? klass.new(response, status) : nil end def self.error(status) @@ -33,8 +34,10 @@ def self.error(status) end end - def initialize(response = nil) + def initialize(response = nil, status = nil) @response = response + @status = status + super(build_error_message) end @@ -54,18 +57,16 @@ def data private + attr_reader :response, :status + def parse_data - body = @response[:body] - return if body.empty? - return body unless body.is_a?(String) + return '' if response.empty? + return response unless response.is_a?(String) - headers = @response[:response_headers] + headers = response[:response_headers] content_type = headers && headers[:content_type] || '' - if content_type =~ /json/ - Sawyer::Agent.serializer.decode(body) - else - body - end + + content_type =~ /json/ ? Sawyer::Agent.serializer.decode(body) : body end def response_message @@ -78,12 +79,13 @@ def response_message end def build_error_message - return nil if @response.nil? + message = '' + + message << "#{response[:method].to_s.upcase} " unless response[:method].nil? + message << "#{redact_url(response[:url].to_s)} : " unless response[:url].nil? + message << status.to_s + message << " - #{response_message}" unless response_message.nil? || response_message.empty? - message = "#{@response[:method].to_s.upcase} " - message << redact_url(@response[:url].to_s) + ': ' - message << "#{@response[:status]} - " - message << response_message.to_s unless response_message.nil? message end diff --git a/lib/oktakit/response/raise_error.rb b/lib/oktakit/response/raise_error.rb index 5837a48..b84aba8 100644 --- a/lib/oktakit/response/raise_error.rb +++ b/lib/oktakit/response/raise_error.rb @@ -9,8 +9,8 @@ module Response class RaiseError < Faraday::Response::Middleware private - def on_complete(response) - if (error = Oktakit::Error.from_response(response)) + def on_complete(response, status) + if (error = Oktakit::Error.from_response(response, status)) raise error end end diff --git a/spec/error_spec.rb b/spec/error_spec.rb new file mode 100644 index 0000000..c5805ad --- /dev/null +++ b/spec/error_spec.rb @@ -0,0 +1,91 @@ +require "spec_helper" + +describe Oktakit::Error do + describe "#from_response(response)" do + context "success" do + subject(:from_response) { described_class.from_response(okta_response, 404) } + + context "with response and status" do + let(:okta_response) do + { + errorCode: "00000000", + errorSummary: "Not found: Resource not found: example@example.com (User)", + errorLink: "E0000007", + errorId: "00000000", + errorCauses: [], + } + end + + it "returns an instance of the described class" do + expect(from_response.class).to(eq(Oktakit::NotFound)) + end + + it "returns a meaningful error message" do + expected_message = "404 - Not found: Resource not found: example@example.com (User)" + + expect(from_response.message).to(eq(expected_message)) + end + end + + context "with missing response" do + let(:okta_response) { {} } + + it "returns an instance of the described class" do + expect(from_response.class).to(eq(Oktakit::NotFound)) + end + + it "returns a meaningful error message" do + expect(from_response.message).to(eq("404")) + end + end + + context "with a different response format" do + let(:okta_response) do + { + status: 404, + method: "POST", + url: "http://example.com/api/v1", + response_headers: { + content_type: "json", + }, + body: { + errorCode: "E0000007", + errorSummary: "Not found: Resource not found: abrakadabrasdfskdkf@admin.com (User)", + errorLink: "E0000007", + errorId: "oaeLRic8zbhTBiJ81eJnWTQUg", + errorCauses: [], + }, + } + end + + it "returns an instance of the described class" do + expect(from_response.class).to(eq(Oktakit::NotFound)) + end + + it "returns a meaningful error message" do + expected_message = "POST http://example.com/api/v1 : 404" + + expect(from_response.message).to(eq(expected_message)) + end + end + end + + context "with errors" do + context "with missing status" do + subject(:from_response) { described_class.from_response(okta_response, 0) } + + let(:okta_response) do + { + errorCode: "00000000", + errorSummary: "Not found: Resource not found: example@example.com (User)", + errorLink: "E0000007", + errorId: "00000000", + errorCauses: [], + } + end + + it { is_expected.to(be_nil) } + end + end + end +end