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

Webhook Registry: Ensuring that the response body is always a Hash, even if ShopifyAPI.Context.response_as_struct is true. #1313

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

## 14.3.0
- [#1312](https://github.com/Shopify/shopify-api-ruby/pull/1312) Use same leeway for `exp` and `nbf` when parsing JWT
- [#1313](https://github.com/Shopify/shopify-api-ruby/pull/1313) Fix: Webhook Registry now working with response_as_struct enabled
- [#1314](https://github.com/Shopify/shopify-api-ruby/pull/1314)
- Add new session util method `SessionUtils::session_id_from_shopify_id_token`
- `SessionUtils::current_session_id` now accepts shopify Id token in the format of `Bearer this_token` or just `this_token`
Expand Down
5 changes: 3 additions & 2 deletions lib/shopify_api/clients/graphql/client.rb
Original file line number Diff line number Diff line change
Expand Up @@ -28,9 +28,10 @@ def initialize(session:, base_path:, api_version: nil)
variables: T.nilable(T::Hash[T.any(Symbol, String), T.untyped]),
headers: T.nilable(T::Hash[T.any(Symbol, String), T.untyped]),
tries: Integer,
response_as_struct: T.nilable(T::Boolean),
).returns(HttpResponse)
end
def query(query:, variables: nil, headers: nil, tries: 1)
def query(query:, variables: nil, headers: nil, tries: 1, response_as_struct: Context.response_as_struct)
body = { query: query, variables: variables }
@http_client.request(
HttpRequest.new(
Expand All @@ -42,7 +43,7 @@ def query(query:, variables: nil, headers: nil, tries: 1)
body_type: "application/json",
tries: tries,
),
response_as_struct: Context.response_as_struct || false,
response_as_struct: response_as_struct || false,
)
end
end
Expand Down
6 changes: 4 additions & 2 deletions lib/shopify_api/clients/graphql/storefront.rb
Original file line number Diff line number Diff line change
Expand Up @@ -45,11 +45,13 @@ def initialize(shop, storefront_access_token = nil, private_token: nil, public_t
variables: T.nilable(T::Hash[T.any(Symbol, String), T.untyped]),
headers: T.nilable(T::Hash[T.any(Symbol, String), T.untyped]),
tries: Integer,
response_as_struct: T.nilable(T::Boolean),
).returns(HttpResponse)
end
def query(query:, variables: nil, headers: {}, tries: 1)
def query(query:, variables: nil, headers: {}, tries: 1, response_as_struct: Context.response_as_struct)
T.must(headers).merge!({ @storefront_auth_header => @storefront_access_token })
super(query: query, variables: variables, headers: headers, tries: tries)
super(query: query, variables: variables, headers: headers, tries: tries,
response_as_struct: response_as_struct)
end
end
end
Expand Down
9 changes: 5 additions & 4 deletions lib/shopify_api/webhooks/registry.rb
Original file line number Diff line number Diff line change
Expand Up @@ -138,7 +138,7 @@ def unregister(topic:, session:)
}
MUTATION

delete_response = client.query(query: delete_mutation)
delete_response = client.query(query: delete_mutation, response_as_struct: false)
raise Errors::WebhookRegistrationError,
"Failed to delete webhook from Shopify" unless delete_response.ok?
result = T.cast(delete_response.body, T::Hash[String, T.untyped])
Expand Down Expand Up @@ -170,7 +170,7 @@ def get_webhook_id(topic:, client:)
}
QUERY

fetch_id_response = client.query(query: fetch_id_query)
fetch_id_response = client.query(query: fetch_id_query, response_as_struct: false)
raise Errors::WebhookRegistrationError,
"Failed to fetch webhook from Shopify" unless fetch_id_response.ok?
body = T.cast(fetch_id_response.body, T::Hash[String, T.untyped])
Expand Down Expand Up @@ -216,7 +216,7 @@ def process(request)
).returns(T::Hash[Symbol, T.untyped])
end
def webhook_registration_needed?(client, registration)
check_response = client.query(query: registration.build_check_query)
check_response = client.query(query: registration.build_check_query, response_as_struct: false)
raise Errors::WebhookRegistrationError,
"Failed to check if webhook was already registered" unless check_response.ok?
parsed_check_result = registration.parse_check_result(T.cast(check_response.body, T::Hash[String, T.untyped]))
Expand All @@ -233,7 +233,8 @@ def webhook_registration_needed?(client, registration)
).returns(T::Hash[String, T.untyped])
end
def send_register_request(client, registration, webhook_id)
register_response = client.query(query: registration.build_register_query(webhook_id: webhook_id))
register_response = client.query(query: registration.build_register_query(webhook_id: webhook_id),
response_as_struct: false)

raise Errors::WebhookRegistrationError, "Failed to register webhook with Shopify" unless register_response.ok?

Expand Down
23 changes: 23 additions & 0 deletions test/webhooks/registry_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,29 @@ def test_process
assert(handler_called)
end

def test_process_with_response_as_struct
Copy link
Contributor

Choose a reason for hiding this comment

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

We could keep this test! Thank you for adding it. This would help us catch any regressions in the future.

modify_context(response_as_struct: true)

handler_called = false

handler = TestHelpers::FakeWebhookHandler.new(
lambda do |topic, shop, body,|
assert_equal(@topic, topic)
assert_equal(@shop, shop)
assert_equal({}, body)
handler_called = true
end,
)

ShopifyAPI::Webhooks::Registry.add_registration(
topic: @topic, path: "path", delivery_method: :http, handler: handler,
)

ShopifyAPI::Webhooks::Registry.process(@webhook_request)

assert(handler_called)
end

def test_process_new_handler
handler_called = false

Expand Down
Loading