Add webhook _id and api_version to webhook handler

[lizkenyon]

Description

Fixes #1247

Shopify recommends as a best practice to check the x-shopify-webhook-id to verify you are not processing duplicate webhooks. We currently are not passing this information into the webhook handler. This PR will add the webhook_id and api_version to the webhook handler to bring the ruby library in line with the Javascript library in terms of information the handler has access to.

This could be resolved in a couple of approaches outlined below

Add webhook_id and api_version to handle function

This is what is currently implemented in this PR.
The webhook_id and api_version are added to the webhook request object from the headers. They are then explicitly passed to the handle function.

This would be a breaking change, but would be a straightforward fix. Developers would need to add webhook_id and api_version to their handle implementation.

Add metadata

If we are concerned, that in the future we may need to add even more information to the handle function and would require more breaking changes we could instead change the method signature to include the body and metadata. Where metadata could be a catch all of all the additional information. This would make any future additions easier but would require slight more initial refactoring for developers on the initial change.

      sig do
        abstract.params(body: T::Hash[String, T.untyped], metadata: T::Hash[String, T.untyped]).void
      end
      def handle(body:, metadata); end
    end

Add new handle function

If we are concerned about introducing a breaking change, we could introduce a new handle function. (eg. handle_with_webhook_and_version then developers would be able to implement the new function if they wanted access to the webhook_id and api_version. This could get complicated if we need to add more information in the future. (We would need to add another new function?)

How has this been tested?

Please, describe the tests that you ran to verify your changes.

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.

[paulomarg]

I think we can make this a minor change by adding a static value to the webhook handler - if we add

module Handler
  FLAG_TO_USE_ALL_FIELDS = T.let(false, T::Boolean)

and then do

module TestHelpers
  class FakeWebhookHandler
    include ShopifyAPI::Webhooks::Handler
    FLAG_TO_USE_ALL_FIELDS = true

we can then use if handler.class::USE_ALL_FIELDS in the registry to call the appropriate version of the function.

We can add deprecation logs when using the old format to give folks time to fix it before a major version. We'd need to declare the abstract class in a way that could deal with both cases, but I think that should be possible with some creative typing?

Do you think that would work?

I agree that there is a bit of a deeper problem here. I see two ways that apps can make this future-proof:

• declare their handlers with a **metadata argument
• change the function to take in a hash / object

I'd lean towards the second option because the first one is somewhat easy to forget and can trip people up - the second option is more reliable IMO.

[nelsonwittwer]

I'd only introduce a new breaking change if we have plans for major version on the horizon. If not, I'd prefer to allow users to opt-in to this option in the initializer.

Besides that roll out question, I think everything looks great!

[paulomarg]

This looks great! I had some thoughts mostly around naming things, but the logic makes sense to me.

[paulomarg]

Should we:

• explicitly type data?
• make data an object instead of a hash to keep the API similar to what it was before? Can we use ShopifyAPI::Webhooks::Request?

[paulomarg]

I wonder if this name is clear enough - is there something we can name it that indicates what it does (i.e. that it takes in the metadata object, or the full metadata)?

We could even take it a bit further: if we need a new method, we could simply test for the presence of that method in the class when we're calling, so use_handle_webhook? wouldn't be necessary.

[lizkenyon]

We could even take it a bit further: if we need a new method, we could simply test for the presence of that method in the class when we're calling, so use_handle_webhook? wouldn't be necessary.

This is what I originally tried and a couldn't get this to work, do you know of a way?
This is what I did:
Originally this was an interface file meaning meaning if we add a new method users would need to implement the new method or they get an error (breaking change). So I changed it to an abstract file with a default implementation so users would not have to implement the new method. But then you loose the ability to check with respond_to? because there is and implementation so it always returns true.

[paulomarg]

Hmm right - I guess we can't have an interface with an optional method (unless we create a wholly different interface), which complicates things indeed. Let's go ahead with this approach then.

[lizkenyon]

As discussed, I refactored the handler to have multiple handler modules.
Developers can implement the new handler if they want they additional data.

[paulomarg]

🔥

[paulomarg]

I think these are inverted?

[nelsonwittwer]

I like this method of rolling this out, well done!

[lizkenyon]

Tested this and verified that:

1. The deprecation warning is shown when you use the old handler
2. When using the new handler you have access to the additional fields.