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

[DaveEshopGuide]

Fixes #1311
ensuring that the response body is always a Hash, even if ShopifyAPI.Context.response_as_struct is true.

Description

Had to add a Utility (ShopifyAPI::Utils::OstructHashUtils) to handle the conversion since a simple .to_h and even JSON.parse(response.body.to_json) did not work as expected (nested Keys and Array handling failed).

How has this been tested?

Wrote new Specs to test the registry process with ShopifyAPI::Context.response_as_struct = true

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.

[DaveEshopGuide]

I have signed the CLA!

[lizkenyon]

Hi there 👋

Thank you for flagging this issue, and taking the time to dig into this!

I was thinking instead of converting the openstruct object back to a hash. (After we have converter a hash to an openstruct), instead in the cases of the webhook registry we stop this conversion from happening in the first place.

We could do this by adding a default param to the client.query that allows us to override the context.response_as_struct value for the cases of the webhook registration.

#client.rb
        sig do
          params(
            query: String,
            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::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)

#registry.rb
        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)

If you would like to make these changes you are welcome to! Otherwise I am happy to put up a PR myself with these changes shortly. Thanks again!

[lizkenyon]

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

[DaveEshopGuide]

Hi there 👋

Thank you for flagging this issue, and taking the time to dig into this!

I was thinking instead of converting the openstruct object back to a hash. (After we have converter a hash to an openstruct), instead in the cases of the webhook registry we stop this conversion from happening in the first place.

We could do this by adding a default param to the client.query that allows us to override the context.response_as_struct value for the cases of the webhook registration.

#client.rb
        sig do
          params(
            query: String,
            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::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)

#registry.rb
        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)

If you would like to make these changes you are welcome to! Otherwise I am happy to put up a PR myself with these changes shortly. Thanks again!

@lizkenyon Thanks for the feedback. Yes, that seems much more reasonable. I wasn't quite happy with introducing a new Util for this anyways. I'll see what I can do.

best, Dave

[DaveEshopGuide]

@lizkenyon Feedback applied ;)

[lizkenyon]

@DaveEshopGuide Thank you for this fix!

Could you add a post to the CHANGELOG.md and then this should be good to go!

[lizkenyon]

Thank you so much for your contribution! 🌟 After a rebase/merge let get this PR merged in!

[Iben1993]

Good

[lizkenyon]

Hey @DaveEshopGuide Could you rebase/ resolve the merge conflicts for this PR?