ShopifyAPI::Clients::HttpResponse as argument for ShopifyAPI::Errors::HttpResponseError #1040

kaarelss · 2022-10-14T06:26:40Z

ShopifyAPI::Clients::HttpResponse as argument for ShopifyAPI::Errors::HttpResponseError because code can be obtained from response as well as headers for custom retry logic when call limit is exceeded and 429 error occurs.

Description

Fixes #1038

Please, include a summary of what the PR is for:

Ability to get headers from response when error occurs. Previously you can get only code from the error.

How has this been tested?

All tests passed as well as new test is included for this new functionality: test_throttle_error_with_retry_after_header_in_error_object in http_client_test.rb

Checklist:

My commit message follow the pattern described in here
I have performed a self-review of my own code.
I have added tests that prove my fix is effective or that my feature works.
I have updated the project documentation.
I have added a changelog line.

…:HttpResponseError because code can be obtained from response as well as headers for custom retry logic

jbourassa

I don't normally maintain this repo but this looks good to me. I'd like to see this merged if you can sign the CLA and fix the linter.

jbourassa · 2022-11-03T00:59:39Z

test/clients/http_client_test.rb

@@ -179,6 +179,18 @@ def test_throttle_error_no_retry_after_header
        verify_http_request
      end

+


Oops! Rubocop complains on this line. Care to fix it?

nelsonwittwer · 2022-11-03T12:53:53Z

lib/shopify_api/errors/http_response_error.rb

+      attr_reader :response
+
+      sig { params(response: ShopifyAPI::Clients::HttpResponse).void }
+      def initialize(response:)


This idea makes sense to provide the response instead of just the code!

I do think if we add the required keyword of response we'll need to update one more place to make sure we don't introduce a regression with this change:

MaxHttpRetriesExceededError which inherits from this class. We'll need to update any instantiations of this error to reflect this keyword change otherwise we'll see an ArgumentError anytime we raise that retries error.

jbourassa · 2022-11-03T13:10:04Z

Sorbet complains about a missing type annotation, this should fix it:

diff --git lib/shopify_api/errors/http_response_error.rb lib/shopify_api/errors/http_response_error.rb
index 1d536a4..e4d912f 100644
--- lib/shopify_api/errors/http_response_error.rb
+++ lib/shopify_api/errors/http_response_error.rb
@@ -15,7 +15,7 @@ module ShopifyAPI
       sig { params(response: ShopifyAPI::Clients::HttpResponse).void }
       def initialize(response:)
         super
-        @code = response.code.to_i
+        @code = T.let(response.code, Integer)
         @response = response
       end
     end

You can run run the Sorbet locally with bundle exec srb tc.

Make sure to sign the CLA too: https://cla.shopify.com/

kaarelss · 2022-11-03T13:41:48Z

lib/shopify_api/clients/http_client.rb


-            raise ShopifyAPI::Errors::MaxHttpRetriesExceededError.new(code: response.code),
+            raise ShopifyAPI::Errors::MaxHttpRetriesExceededError.new(response: response),


This idea makes sense to provide the response instead of just the code!

I do think if we add the required keyword of response we'll need to update one more place to make sure we don't introduce a regression with this change:

MaxHttpRetriesExceededError which inherits from this class. We'll need to update any instantiations of this error to reflect this keyword change otherwise we'll see an ArgumentError anytime we raise that retries error.

Hi @nelsonwittwer. Thank you for your comment. Do you mean this place?

Added assertion for that - 3312bc0

kaarelss · 2022-11-03T13:47:54Z

Sorbet complains about a missing type annotation, this should fix it:

diff --git lib/shopify_api/errors/http_response_error.rb lib/shopify_api/errors/http_response_error.rb
index 1d536a4..e4d912f 100644
--- lib/shopify_api/errors/http_response_error.rb
+++ lib/shopify_api/errors/http_response_error.rb
@@ -15,7 +15,7 @@ module ShopifyAPI
       sig { params(response: ShopifyAPI::Clients::HttpResponse).void }
       def initialize(response:)
         super
-        @code = response.code.to_i
+        @code = T.let(response.code, Integer)
         @response = response
       end
     end

You can run run the Sorbet locally with bundle exec srb tc.

Make sure to sign the CLA too: https://cla.shopify.com/

Thank you @jbourassa for mentioning this, fixed it. We do not have sorbet in our ruby on rails project, so I'm new to this. About the CLA, my employer has asked Shopify to add additional authorized contributor to the existing Corporate CLA. We are still waiting for a reply from Shopify.

kaarelss · 2022-11-10T16:06:27Z

HI @jbourassa and @nelsonwittwer. I have CLA signed now. What are the next steps?

teddyhwang · 2022-11-16T18:32:14Z

HI @jbourassa and @nelsonwittwer. I have CLA signed now. What are the next steps?

@kaarelss can you rebase your branch with main? that should also get your tests to pass and we'll get this merged in

…:HttpResponseError because code can be obtained from response as well as headers for custom retry logic

kaarelss · 2022-11-28T11:40:20Z

HI @jbourassa and @nelsonwittwer. I have CLA signed now. What are the next steps?

@kaarelss can you rebase your branch with main? that should also get your tests to pass and we'll get this merged in

Done

nelsonwittwer · 2022-11-28T14:47:23Z

One last little 👶 step and we can get this merged in! Will you add a quick line in the change log describing your change?

…y-api-ruby into response_in_error_object * 'response_in_error_object' of github.com:kaarelss/shopify-api-ruby: Added assertion that response is present in MaxHttpRetriesExceededError Added sorbet assertion Removed extra empty line ShopifyAPI::Clients::HttpResponse as argument for ShopifyAPI::Errors::HttpResponseError because code can be obtained from response as well as headers for custom retry logic Remove mentions of private apps (Shopify#1062) Fix ActiveSupport inflector dependency (Shopify#1063) Move Session Storage to `shopify_app` (Shopify#1055) Remove session argument from REST examples (Shopify#1064) Constrain Zeitwerk to 2.6.5 (Shopify#1059) Clarify statement about packages (Shopify#1057)

kaarelss · 2022-11-29T13:24:49Z

One last little 👶 step and we can get this merged in! Will you add a quick line in the change log describing your change?

Added

hypogealgaol · 2023-02-10T08:03:23Z

Hi, not sure if this is related but I wanted to try to create my own error doing the following:

response = ShopifyAPI::Clients::HttpResponse.new(code: 401, body: {}, headers: {})
ShopifyAPI::Errors::HttpResponseError.new(response: response)
Traceback (most recent call last):
        3: from (irb):175
        2: from (irb):176:in `rescue in irb_binding'
        1: from (irb):176:in `new'
ArgumentError (missing keyword: :code)

Am I doing something wrong?

kaarelss · 2023-02-10T08:12:11Z

Hi, not sure if this is related but I wanted to try to create my own error doing the following:

response = ShopifyAPI::Clients::HttpResponse.new(code: 401, body: {}, headers: {})
ShopifyAPI::Errors::HttpResponseError.new(response: response)
Traceback (most recent call last):
        3: from (irb):175
        2: from (irb):176:in `rescue in irb_binding'
        1: from (irb):176:in `new'
ArgumentError (missing keyword: :code)

Am I doing something wrong?

Which version of the gem are you using?

ShopifyAPI::Clients::HttpResponse as argument for ShopifyAPI::Errors:…

f6c6efc

…:HttpResponseError because code can be obtained from response as well as headers for custom retry logic

kaarelss requested a review from a team as a code owner October 14, 2022 06:26

github-actions bot added the cla-needed label Oct 14, 2022

jbourassa approved these changes Nov 3, 2022

View reviewed changes

kaarelss and others added 2 commits November 3, 2022 14:10

Removed extra empty line

053d9db

Merge branch 'main' into response_in_error_object

a98cef3

nelsonwittwer requested changes Nov 3, 2022

View reviewed changes

Added sorbet assertion

285adc7

kaarelss commented Nov 3, 2022

View reviewed changes

nelsonwittwer approved these changes Nov 3, 2022

View reviewed changes

Added assertion that response is present in MaxHttpRetriesExceededError

533ec79

kaarelss force-pushed the response_in_error_object branch from 3312bc0 to 533ec79 Compare November 10, 2022 07:27

github-actions bot removed the cla-needed label Nov 10, 2022

kaarelss added 4 commits November 28, 2022 10:30

ShopifyAPI::Clients::HttpResponse as argument for ShopifyAPI::Errors:…

54be246

…:HttpResponseError because code can be obtained from response as well as headers for custom retry logic

Removed extra empty line

072f2fa

Added sorbet assertion

5cfe410

Added assertion that response is present in MaxHttpRetriesExceededError

c83bf0d

kaarelss force-pushed the response_in_error_object branch from 533ec79 to c83bf0d Compare November 28, 2022 08:30

kaarelss added 2 commits November 29, 2022 15:17

Added line in changelog

df0b31d

Merge branch 'main' into response_in_error_object

ae254a4

nelsonwittwer merged commit bdcffb6 into Shopify:main Dec 2, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ShopifyAPI::Clients::HttpResponse as argument for ShopifyAPI::Errors::HttpResponseError #1040

ShopifyAPI::Clients::HttpResponse as argument for ShopifyAPI::Errors::HttpResponseError #1040

kaarelss commented Oct 14, 2022

jbourassa left a comment

jbourassa Nov 3, 2022

kaarelss Nov 3, 2022

nelsonwittwer Nov 3, 2022

jbourassa commented Nov 3, 2022

kaarelss Nov 3, 2022

kaarelss Nov 3, 2022 •

edited

Loading

kaarelss commented Nov 3, 2022

kaarelss commented Nov 10, 2022

teddyhwang commented Nov 16, 2022

kaarelss commented Nov 28, 2022

nelsonwittwer commented Nov 28, 2022

kaarelss commented Nov 29, 2022

hypogealgaol commented Feb 10, 2023 •

edited

Loading

kaarelss commented Feb 10, 2023

		@@ -179,6 +179,18 @@ def test_throttle_error_no_retry_after_header
		verify_http_request
		end


		raise ShopifyAPI::Errors::MaxHttpRetriesExceededError.new(code: response.code),
		raise ShopifyAPI::Errors::MaxHttpRetriesExceededError.new(response: response),

ShopifyAPI::Clients::HttpResponse as argument for ShopifyAPI::Errors::HttpResponseError #1040

ShopifyAPI::Clients::HttpResponse as argument for ShopifyAPI::Errors::HttpResponseError #1040

Conversation

kaarelss commented Oct 14, 2022

Description

How has this been tested?

Checklist:

jbourassa left a comment

Choose a reason for hiding this comment

jbourassa Nov 3, 2022

Choose a reason for hiding this comment

kaarelss Nov 3, 2022

Choose a reason for hiding this comment

nelsonwittwer Nov 3, 2022

Choose a reason for hiding this comment

jbourassa commented Nov 3, 2022

kaarelss Nov 3, 2022

Choose a reason for hiding this comment

kaarelss Nov 3, 2022 • edited Loading

Choose a reason for hiding this comment

kaarelss commented Nov 3, 2022

kaarelss commented Nov 10, 2022

teddyhwang commented Nov 16, 2022

kaarelss commented Nov 28, 2022

nelsonwittwer commented Nov 28, 2022

kaarelss commented Nov 29, 2022

hypogealgaol commented Feb 10, 2023 • edited Loading

kaarelss commented Feb 10, 2023

kaarelss Nov 3, 2022 •

edited

Loading

hypogealgaol commented Feb 10, 2023 •

edited

Loading