Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 12 commits into from
Dec 2, 2022
Merged
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
Note: For changes to the API, see https://shopify.dev/changelog?filter=api

## Unreleased
- [#1040](https://github.com/Shopify/shopify-api-ruby/pull/1040) `ShopifyAPI::Clients::HttpResponse` as argument for `ShopifyAPI::Errors::HttpResponseError`
- [#1055](https://github.com/Shopify/shopify-api-ruby/pull/1055) Makes session_storage optional. Configuring the API with session_storage is now deprecated. Session persistence is handled by the [shopify_app gem](https://github.com/Shopify/shopify_app) if using Rails.
- [#1063](https://github.com/Shopify/shopify-api-ruby/pull/1063) Fix ActiveSupport inflector dependency

Expand Down
6 changes: 3 additions & 3 deletions lib/shopify_api/clients/http_client.rb
Original file line number Diff line number Diff line change
Expand Up @@ -69,13 +69,13 @@ def request(request)
error_message = error_messages.join("\n")

unless [429, 500].include?(response.code)
raise ShopifyAPI::Errors::HttpResponseError.new(code: response.code.to_i), error_message
raise ShopifyAPI::Errors::HttpResponseError.new(response: response), error_message
end

if tries == request.tries
raise ShopifyAPI::Errors::HttpResponseError.new(code: response.code), error_message if request.tries == 1
raise ShopifyAPI::Errors::HttpResponseError.new(response: response), error_message if request.tries == 1

raise ShopifyAPI::Errors::MaxHttpRetriesExceededError.new(code: response.code),
raise ShopifyAPI::Errors::MaxHttpRetriesExceededError.new(response: response),
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

@kaarelss kaarelss Nov 3, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added assertion for that - 3312bc0

"Exceeded maximum retry count of #{request.tries}. Last message: #{error_message}"
end

Expand Down
10 changes: 7 additions & 3 deletions lib/shopify_api/errors/http_response_error.rb
Original file line number Diff line number Diff line change
Expand Up @@ -9,10 +9,14 @@ class HttpResponseError < StandardError
sig { returns(Integer) }
attr_reader :code

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

sig { params(response: ShopifyAPI::Clients::HttpResponse).void }
def initialize(response:)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

super
@code = code
@code = T.let(response.code, Integer)
@response = response
end
end
end
Expand Down
14 changes: 13 additions & 1 deletion test/clients/http_client_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -162,7 +162,8 @@ def test_retries_exceeded
.with(body: @request.body.to_json, query: @request.query, headers: @expected_headers)
.to_return(body: { errors: "Something very not good" }.to_json, headers: @response_headers, status: 500)

assert_raises(ShopifyAPI::Errors::MaxHttpRetriesExceededError) { @client.request(@request) }
error = assert_raises(ShopifyAPI::Errors::MaxHttpRetriesExceededError) { @client.request(@request) }
assert_instance_of(ShopifyAPI::Clients::HttpResponse, error.response)
end

def test_throttle_error_no_retry_after_header
Expand All @@ -179,6 +180,17 @@ def test_throttle_error_no_retry_after_header
verify_http_request
end

def test_throttle_error_with_retry_after_header_in_error_object
stub_request(@request.http_method, "https://#{@shop}#{@base_path}/#{@request.path}")
.with(body: @request.body.to_json, query: @request.query, headers: @expected_headers)
.to_return(body: { errors: "Call limit exceeded" }.to_json,
headers: @response_headers.merge("Retry-After" => "2.0"), status: 429)

error = assert_raises(ShopifyAPI::Errors::HttpResponseError) { @client.request(@request) }
assert_equal(429, error.code)
assert_equal("2.0", error.response.headers["retry-after"][0])
end

def test_warns_on_deprecation_header
reader, writer = IO.pipe
modify_context(logger: Logger.new(writer), api_version: "unstable")
Expand Down
8 changes: 6 additions & 2 deletions test/rest/base_errors_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,12 @@ def test_outputs_error_message

def test_outputs_error_code
errors = ShopifyAPI::Rest::BaseErrors.new
errors.errors << ShopifyAPI::Errors::HttpResponseError.new(code: 404)
errors.errors << ShopifyAPI::Errors::HttpResponseError.new(code: 405)

response = ShopifyAPI::Clients::HttpResponse.new(code: 404, body: {}, headers: {})
errors.errors << ShopifyAPI::Errors::HttpResponseError.new(response: response)

response = ShopifyAPI::Clients::HttpResponse.new(code: 405, body: {}, headers: {})
errors.errors << ShopifyAPI::Errors::HttpResponseError.new(response: response)

assert_equal(errors.codes, [404, 405])
end
Expand Down