From 68ed2dc79998048067da4072d1c8ba1daf2ebc4f Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Armando=20Rodr=C3=ADguez?=
 <127134616+armando-rodriguez-cko@users.noreply.github.com>
Date: Mon, 18 Nov 2024 19:03:42 +0100
Subject: [PATCH] Enhance `parse_response` Method to Improve Error Handling and
 Logging

---
 lib/checkout_sdk/api_client.rb                | 109 +++++++-------
 lib/checkout_sdk/client.rb                    |   7 +-
 lib/checkout_sdk/error.rb                     |  32 +++-
 spec/checkout_sdk/api_client_spec.rb          | 141 ++++++++++++++++++
 .../payments/contexts/contexts_helper.rb      |  36 +++++
 .../contexts/contexts_integration_spec.rb     |  35 +----
 .../reports/reports_integration_spec.rb       |   3 +-
 7 files changed, 271 insertions(+), 92 deletions(-)
 create mode 100644 spec/checkout_sdk/api_client_spec.rb

diff --git a/lib/checkout_sdk/api_client.rb b/lib/checkout_sdk/api_client.rb
index b168df3..0cf604b 100644
--- a/lib/checkout_sdk/api_client.rb
+++ b/lib/checkout_sdk/api_client.rb
@@ -1,11 +1,11 @@
 # frozen_string_literal: true
 
+require 'csv'
+
 module CheckoutSdk
   class ApiClient
     attr_accessor :client, :multipart_client, :log
 
-    # @param [CheckoutConfiguration] configuration
-    # @param [String] uri
     def initialize(configuration, uri)
       @client = configuration.http_client.clone
       @client.url_prefix = uri
@@ -14,42 +14,27 @@ def initialize(configuration, uri)
       @log = configuration.logger
     end
 
-    # @param [String] path
-    # @param [SdkAuthorization] authorization
-    # @param [Object] params
-    def invoke_get(path,
-                   authorization,
-                   params = nil)
+    def invoke_get(path, authorization, params = nil)
       invoke(:get, path, authorization, params: params)
     end
 
-    def invoke_post(path,
-                    authorization,
-                    request = nil,
-                    idempotency_key = nil)
+    def invoke_post(path, authorization, request = nil, idempotency_key = nil)
       invoke(:post, path, authorization, request, idempotency_key)
     end
 
-    def invoke_put(path,
-                   authorization,
-                   request)
+    def invoke_put(path, authorization, request)
       invoke(:put, path, authorization, request)
     end
 
-    def invoke_patch(path,
-                     authorization,
-                     request = nil)
+    def invoke_patch(path, authorization, request = nil)
       invoke(:patch, path, authorization, request)
     end
 
-    def invoke_delete(path,
-                      authorization)
+    def invoke_delete(path, authorization)
       invoke(:delete, path, authorization)
     end
 
-    def submit_file(path,
-                    authorization,
-                    request)
+    def submit_file(path, authorization, request)
       upload(path, authorization, request)
     end
 
@@ -58,7 +43,7 @@ def submit_file(path,
     def invoke(method, path, authorization, body = nil, idempotency_key = nil, params: nil)
       path = append_params(path, params) unless params.nil?
 
-      headers = get_default_headers authorization
+      headers = default_headers(authorization)
       headers[:'Content-Type'] = 'application/json'
       headers[:'Cko-Idempotency-Key'] = idempotency_key unless idempotency_key.nil?
 
@@ -71,26 +56,23 @@ def invoke(method, path, authorization, body = nil, idempotency_key = nil, param
         raise CheckoutApiException, e.response
       end
 
-      parse_response response
+      parse_response(response)
     end
 
-    def get_default_headers(authorization)
-      {
-        'User-Agent': "checkout-sdk-ruby/#{VERSION}",
-        Accept: 'application/json',
-        Authorization: authorization.authorization_header
-      }
+    def default_headers(authorization)
+      { 'User-Agent': "checkout-sdk-ruby/#{VERSION}", Accept: 'application/json',
+        Authorization: authorization.authorization_header }
     end
 
     def append_params(path, input_params)
       raise CheckoutArgumentException, 'Query parameters were not provided' if input_params.nil?
 
-      if input_params.is_a? String
-        params = input_params
-      else
-        hash = CheckoutSdk::JsonSerializer.to_custom_hash(input_params)
-        params = URI.encode_www_form(hash)
-      end
+      params = if input_params.is_a? String
+                 input_params
+               else
+                 hash = CheckoutSdk::JsonSerializer.to_custom_hash(input_params)
+                 URI.encode_www_form(hash)
+               end
 
       "#{path}?#{params}"
     end
@@ -103,16 +85,16 @@ def build_multipart_request(file_request, file)
           MIME::Types.type_for(file_request.file).first,
           File.basename(file_request.file)
         ),
-        :purpose => file_request.purpose
+        purpose: file_request.purpose
       }
     end
 
     def upload(path, authorization, file_request)
-      headers = get_default_headers authorization
+      headers = default_headers(authorization)
 
       file = File.open(file_request.file)
 
-      form = build_multipart_request file_request, file
+      form = build_multipart_request(file_request, file)
 
       begin
         @log.info "post: /#{path}"
@@ -123,28 +105,55 @@ def upload(path, authorization, file_request)
         file.close
       end
 
-      parse_response response
+      parse_response(response)
     end
 
     def parse_response(response)
-      raise CheckoutApiException, response if response.status < 200 || response.status >= 400
+      raise CheckoutApiException, response if response.status < 200 || response.status >= 300
 
       metadata = CheckoutUtils.map_to_http_metadata(response)
-      body = parse_json_or_contents(response)
-      body = OpenStruct.new if body.nil?
-      body = OpenStruct.new(items: body) if body.is_a? Array
-      body.http_metadata = metadata if body.is_a? OpenStruct
+      body = parse_body(response)
+
+      if body.is_a?(Array)
+        body = OpenStruct.new(items: body)
+      elsif !body.is_a?(OpenStruct)
+        body = OpenStruct.new(contents: body)
+      end
+
+      body.http_metadata = metadata if body.is_a?(OpenStruct)
+
       body
+    rescue JSON::ParserError => e
+      raise CheckoutApiException.new(response, "Error parsing JSON: #{e.message}")
+    rescue StandardError => e
+      @log&.error("Unexpected error occurred: #{e.message}")
+      raise
     end
 
-    def parse_json_or_contents(response)
-      return if response.body.nil? || response.body == ''
+    def parse_body(response)
+      content_type = response.headers['Content-Type']
+      return OpenStruct.new if response.body.nil? || response.body.empty?
 
-      if response.body.start_with?('{', '[')
-        JSON.parse(response.body, object_class: OpenStruct)
+      if content_type&.include?('application/json')
+        parsed_value = JSON.parse(response.body)
+        deep_convert_to_ostruct(parsed_value)
+      elsif content_type&.include?('text/csv')
+        csv_data = CSV.parse(response.body, headers: true)
+        OpenStruct.new(csv: csv_data)
       else
         OpenStruct.new(contents: response.body)
       end
     end
+
+    def deep_convert_to_ostruct(obj)
+      case obj
+      when Hash
+        OpenStruct.new(obj.transform_values { |value| deep_convert_to_ostruct(value) })
+      when Array
+        obj.map { |item| deep_convert_to_ostruct(item) }
+      else
+        obj
+      end
+    end
   end
 end
diff --git a/lib/checkout_sdk/client.rb b/lib/checkout_sdk/client.rb
index 4584b8a..a6f33c6 100644
--- a/lib/checkout_sdk/client.rb
+++ b/lib/checkout_sdk/client.rb
@@ -11,7 +11,12 @@ class Client
     attr_reader :api_client,
                 :authorization_type,
                 :configuration
-    protected :api_client, :authorization_type, :configuration
+
+    protected
+
+    attr_reader :api_client,
+                :authorization_type,
+                :configuration
 
     # @param [CheckoutSdk::ApiClient] api_client
     # @param [CheckoutConfiguration] configuration
diff --git a/lib/checkout_sdk/error.rb b/lib/checkout_sdk/error.rb
index de6bab0..3527089 100644
--- a/lib/checkout_sdk/error.rb
+++ b/lib/checkout_sdk/error.rb
@@ -1,30 +1,46 @@
 # frozen_string_literal: true
 
 module CheckoutSdk
-  class CheckoutException < StandardError
-  end
+  class CheckoutException < StandardError; end
 
   class CheckoutArgumentException < CheckoutException; end
 
   class CheckoutAuthorizationException < CheckoutException
     def self.invalid_authorization(authorization_type)
-      CheckoutAuthorizationException.new("Operation requires #{authorization_type} authorization type")
+      new("Operation requires #{authorization_type} authorization type.")
     end
 
     def self.invalid_key(key_type)
-      CheckoutAuthorizationException.new("#{key_type} is required for this operation.")
+      new("#{key_type} is required for this operation.")
     end
   end
 
   class CheckoutApiException < CheckoutException
     attr_reader :http_metadata, :error_details
 
-    def initialize(response)
+    def initialize(response, message = nil)
       @http_metadata = CheckoutUtils.map_to_http_metadata(response)
-      if !http_metadata.body.nil? && http_metadata.body != ''
-        @error_details = JSON.parse(http_metadata.body, object_class: OpenStruct)
+      @error_details = parse_error_details(http_metadata.body)
+      super(message || build_error_message)
+    end
+
+    private
+
+    def parse_error_details(body)
+      return if body.nil? || body.empty?
+
+      JSON.parse(body, object_class: OpenStruct)
+    rescue JSON::ParserError
+      nil
+    end
+
+    def build_error_message
+      message = "The API response status code (#{http_metadata.status_code}) does not indicate success."
+      if @error_details && !@error_details.to_h.empty?
+        details = @error_details.to_h.map { |key, value| "#{key}: #{value}" }.join(', ')
+        message += " Details: #{details}."
       end
-      super("The API response status code (#{http_metadata.status_code}) does not indicate success.")
+      message
     end
   end
 end
diff --git a/spec/checkout_sdk/api_client_spec.rb b/spec/checkout_sdk/api_client_spec.rb
new file mode 100644
index 0000000..3cc7a95
--- /dev/null
+++ b/spec/checkout_sdk/api_client_spec.rb
@@ -0,0 +1,141 @@
+# frozen_string_literal: true
+
+RSpec.describe CheckoutSdk::ApiClient do
+  let(:configuration) do
+    double(
+      'CheckoutConfiguration',
+      http_client: Faraday.new,
+      multipart_http_client: Faraday.new,
+      logger: Logger.new(STDOUT)
+    )
+  end
+
+  let(:api_client) { CheckoutSdk::ApiClient.new(configuration, 'https://api.sandbox.checkout.com') }
+
+  describe '#parse_response' do
+    context 'when the response is successful' do
+      it 'parses the response correctly for valid JSON' do
+        response = double('Response', status: 200, body: '{"key":"value"}', headers: { 'Content-Type' => 'application/json' })
+        allow(CheckoutSdk::CheckoutUtils).to receive(:map_to_http_metadata).with(response).and_return(
+          OpenStruct.new(status_code: 200, body: response.body)
+        )
+        parsed_response = api_client.send(:parse_response, response)
+        expect(parsed_response.key).to eq('value')
+        expect(parsed_response.http_metadata.status_code).to eq(200)
+      end
+
+      it 'returns an OpenStruct object with items for an array response' do
+        response = double('Response', status: 200, body: '[{"item":"value"}]', headers: { 'Content-Type' => 'application/json' })
+        allow(CheckoutSdk::CheckoutUtils).to receive(:map_to_http_metadata).with(response).and_return(
+          OpenStruct.new(status_code: 200, body: response.body)
+        )
+        parsed_response = api_client.send(:parse_response, response)
+        expect(parsed_response.items).to be_an(Array)
+        expect(parsed_response.items.first.item).to eq('value')
+        expect(parsed_response.http_metadata.status_code).to eq(200)
+      end
+
+      it 'wraps primitive JSON values in OpenStruct with contents key' do
+        response = double('Response', status: 200, body: '123', headers: { 'Content-Type' => 'application/json' })
+        allow(CheckoutSdk::CheckoutUtils).to receive(:map_to_http_metadata).with(response).and_return(
+          OpenStruct.new(status_code: 200, body: response.body)
+        )
+        parsed_response = api_client.send(:parse_response, response)
+        expect(parsed_response.contents).to eq(123)
+        expect(parsed_response.http_metadata.status_code).to eq(200)
+      end
+    end
+
+    context 'when the response body is nil' do
+      it 'returns an empty OpenStruct' do
+        response = double('Response', status: 200, body: nil, headers: { 'Content-Type' => 'application/json' })
+        allow(CheckoutSdk::CheckoutUtils).to receive(:map_to_http_metadata).with(response).and_return(
+          OpenStruct.new(status_code: 200)
+        )
+        parsed_response = api_client.send(:parse_response, response)
+        expect(parsed_response).to be_a(OpenStruct)
+        expect(parsed_response.to_h.keys).to contain_exactly(:http_metadata)
+        expect(parsed_response.http_metadata.status_code).to eq(200)
+      end
+    end
+
+    context 'when the response body is an empty string' do
+      it 'returns an empty OpenStruct' do
+        response = double('Response', status: 200, body: '', headers: { 'Content-Type' => 'application/json' })
+        allow(CheckoutSdk::CheckoutUtils).to receive(:map_to_http_metadata).with(response).and_return(
+          OpenStruct.new(status_code: 200)
+        )
+        parsed_response = api_client.send(:parse_response, response)
+        expect(parsed_response).to be_a(OpenStruct)
+        expect(parsed_response.to_h.keys).to contain_exactly(:http_metadata)
+        expect(parsed_response.http_metadata.status_code).to eq(200)
+      end
+    end
+
+    context 'when the response status is not in the 2xx range' do
+      it 'raises a CheckoutApiException for status code less than 200' do
+        response = double('Response', status: 199, body: '{}')
+        allow(CheckoutSdk::CheckoutUtils).to receive(:map_to_http_metadata).with(response).and_return(
+          OpenStruct.new(status_code: 199, body: response.body)
+        )
+        expect do
+          api_client.send(:parse_response, response)
+        end.to raise_error(CheckoutSdk::CheckoutApiException) do |error|
+          expect(error.message).to eq("The API response status code (199) does not indicate success.")
+          expect(error.http_metadata.status_code).to eq(199)
+        end
+      end
+
+      it 'raises a CheckoutApiException for client errors (4xx)' do
+        response = double('Response', status: 400, body: '{"error":"Bad Request"}')
+        allow(CheckoutSdk::CheckoutUtils).to receive(:map_to_http_metadata).with(response).and_return(
+          OpenStruct.new(status_code: 400, body: response.body)
+        )
+        expect do
+          api_client.send(:parse_response, response)
+        end.to raise_error(CheckoutSdk::CheckoutApiException) do |error|
+          expect(error.message).to include("The API response status code (400) does not indicate success.")
+          expect(error.error_details.error).to eq('Bad Request')
+          expect(error.http_metadata.status_code).to eq(400)
+        end
+      end
+
+      it 'raises a CheckoutApiException for server errors (5xx)' do
+        response = double('Response', status: 500, body: '{"error":"Server Error"}')
+        allow(CheckoutSdk::CheckoutUtils).to receive(:map_to_http_metadata).with(response).and_return(
+          OpenStruct.new(status_code: 500, body: response.body)
+        )
+        expect do
+          api_client.send(:parse_response, response)
+        end.to raise_error(CheckoutSdk::CheckoutApiException) do |error|
+          expect(error.message).to include("The API response status code (500) does not indicate success.")
+          expect(error.error_details.error).to eq('Server Error')
+          expect(error.http_metadata.status_code).to eq(500)
+        end
+      end
+    end
+
+    context 'when the response body is invalid JSON' do
+      it 'raises a CheckoutApiException with JSON parsing error' do
+        response = double('Response', status: 200, body: '{invalid_json}', headers: { 'Content-Type' => 'application/json' })
+        allow(CheckoutSdk::CheckoutUtils).to receive(:map_to_http_metadata).with(response).and_return(
+          OpenStruct.new(status_code: 200, body: response.body)
+        )
+        expect do
+          api_client.send(:parse_response, response)
+        end.to raise_error(CheckoutSdk::CheckoutApiException, /Error parsing JSON: .*/)
+      end
+    end
+
+    context 'when an unexpected exception occurs' do
+      it 'logs the error and re-raises the exception' do
+        response = double('Response', status: 200, body: '{"key":"value"}')
+        allow(CheckoutSdk::CheckoutUtils).to receive(:map_to_http_metadata).and_raise(StandardError.new("Unexpected failure"))
+        expect(api_client.log).to receive(:error).with("Unexpected error occurred: Unexpected failure")
+        expect do
+          api_client.send(:parse_response, response)
+        end.to raise_error(StandardError, "Unexpected failure")
+      end
+    end
+  end
+end
\ No newline at end of file
diff --git a/spec/checkout_sdk/payments/contexts/contexts_helper.rb b/spec/checkout_sdk/payments/contexts/contexts_helper.rb
index 42cba44..df944ca 100644
--- a/spec/checkout_sdk/payments/contexts/contexts_helper.rb
+++ b/spec/checkout_sdk/payments/contexts/contexts_helper.rb
@@ -31,4 +31,40 @@ def create_payment_contexts_paypal
     response
   end
 
+  def create_payment_contexts_klarna
+    request = {
+      'source' => {
+        'type' => 'klarna',
+        'account_holder' => {
+          'billing_address' => {
+            'country' => 'DE'
+          }
+        }
+      },
+      'amount' => 1000,
+      'currency' => CheckoutSdk::Common::Currency::EUR,
+      'payment_type' => CheckoutSdk::Payments::PaymentType::REGULAR,
+      'processing_channel_id' => ENV.fetch('CHECKOUT_PROCESSING_CHANNEL_ID', nil),
+      'items' => [
+        {
+          'name' => 'mask',
+          'unit_price' => 1000,
+          'quantity' => 1,
+          'total_amount' => 1000,
+          'reference' => 'BA67A'
+        }
+      ],
+      'processing' => {
+        'locale' => 'en-GB'
+      }
+    }
+
+    response = default_sdk.contexts.create_payment_contexts(request)
+    expect(response).not_to be nil
+    expect(response.id).not_to be nil
+    expect(response.partner_metadata.client_token).not_to be nil
+    expect(response.partner_metadata.session_id).not_to be nil
+    response
+  end
+
 end
diff --git a/spec/checkout_sdk/payments/contexts/contexts_integration_spec.rb b/spec/checkout_sdk/payments/contexts/contexts_integration_spec.rb
index d93def6..64822cb 100644
--- a/spec/checkout_sdk/payments/contexts/contexts_integration_spec.rb
+++ b/spec/checkout_sdk/payments/contexts/contexts_integration_spec.rb
@@ -5,6 +5,7 @@
 
   before(:all) do
     @payment_context_paypal = create_payment_contexts_paypal
+    @payment_context_klarna = create_payment_contexts_klarna
   end
 
   describe '.create_payment_contexts' do
@@ -12,36 +13,7 @@
       it { is_valid_payment_context @payment_context_paypal }
     end
     context 'when creating a payment contexts klarna with valid data' do
-      it 'raises an error (apm_service_unavailable)' do
-        request = {
-          'source' => {
-            'type' => 'klarna',
-            'account_holder' => {
-              'billing_address' => {
-                'country' => 'DE'
-              }
-            }
-          },
-          'amount' => 1000,
-          'currency' => CheckoutSdk::Common::Currency::EUR,
-          'payment_type' => CheckoutSdk::Payments::PaymentType::REGULAR,
-          'processing_channel_id' => ENV.fetch('CHECKOUT_PROCESSING_CHANNEL_ID', nil),
-          'items' => [
-            {
-              'name' => 'mask',
-              'unit_price' => 1000,
-              'quantity' => 1,
-              'total_amount' => 1000,
-              'reference' => 'BA67A'
-            }
-          ],
-          'processing' => {
-            'locale' => 'en-GB'
-          }
-        }
-        expect { default_sdk.contexts.create_payment_contexts(request) }
-            .to raise_error(CheckoutSdk::CheckoutApiException) { |e| expect(e.error_details[:error_codes].first).to eq 'apm_service_unavailable' }
-      end
+      it { is_valid_payment_context @payment_context_klarna }
     end
   end
 
@@ -54,8 +26,7 @@
 
 def is_valid_payment_context(payment_context)
   assert_response payment_context, %w[id
-                                      partner_metadata
-                                      partner_metadata.order_id]
+                                      partner_metadata]
 end
 
 def is_valid_payment_context_details(payment_context_details_response)
diff --git a/spec/checkout_sdk/reports/reports_integration_spec.rb b/spec/checkout_sdk/reports/reports_integration_spec.rb
index d649795..0e88eda 100644
--- a/spec/checkout_sdk/reports/reports_integration_spec.rb
+++ b/spec/checkout_sdk/reports/reports_integration_spec.rb
@@ -72,7 +72,8 @@
       it 'should retrieve report file contents' do
         response = default_sdk.reports.get_report_file report.id, report.files[0].id
         expect(response).not_to be_nil
-        expect(response.contents).not_to be_nil
+        expect(response.contents).to be_nil
+        expect(response.csv).not_to be_nil
         expect(response.http_metadata.status_code).to eq 200
       end