Support multiple handlers for the same webhook topic

[mhkafadar]

Overview

I would like to propose some improvements to the webhook registration logic of the library. These changes aim to address the following issues:

1. Deletion of unrelated topics during individual webhook registration: Currently, when registering a new topic individually, the library removes existing topics with different names. For example, if a COLLECTIONS_UPDATE topic exists and you register an APP_SUBSCRIPTIONS_UPDATE topic in a separate registration call, the COLLECTIONS_UPDATE webhook will be deleted. This behavior is not desired, as developers may want to subscribe to multiple topics individually. It is worth noting that subscribing to multiple topics at once does not cause this issue.

2. Limitation on registering the same topic with different URLs: The library currently does not allow registering the same topic with different URLs. For instance, if you register an APP_SUBSCRIPTIONS_UPDATE topic with domain.com/webhook, trying to register the same topic with domain.com/webhook-new will result in the deletion of the previous one. This limitation has already been discussed in issue #427, but no changes have been made. According to a response from SBD_ on this Shopify community post, Shopify's API allows multiple subscriptions to the same webhook topic with different URLs.

To address these issues, I have implemented the necessary logic and introduced a new parameter allowMultipleHandlers, which is false by default to maintain the library's current behavior. To enable multiple handlers, you can use the following code:

api.webhooks.register({
  session,
  allowMultipleHandlers: true,
});

I am happy to add tests and submit a PR if these changes align with the goals of the core contributors. You can review my code changes in my repository here. Your feedback is much appreciated. Thank you.

[DavidWittness]

Hi @mhkafadar!

I'm taking a look at the webhook registration logic using the latest API version and app template.

I added a test function for the PRODUCTS_UPDATE topic that is registered using shopify.processWebhooks() when the app is installed.

I setup a button to register a new topic PRODUCTS_CREATE using addHandlers(), followed by register().

Here's what I found:

When clicking the button, the new topic is registered, and the other topic remain. I can query for the webhooks and see both PRODUCTS_CREATE and PRODUCTS_UPDATE.

To test issue 2, I can change the button logic to use the same topic, but a different URL. The button registers PRODUCT_UPDATE with a url of /webhooks-different. Querying for all webhooks shows that both PRODUCT_UPDATE hooks are still registered. When i tried to update a product, both webhook callback functions fired.

Let me know what your code shows and I can see if we can track down why we're seeing different results.

[mhkafadar]

Hello @DavidWittness, and thank you for your response and efforts in reproducing the issue. I'd like to provide more details to help clarify the situation.

I am running the app on two different servers, both using the same API key and secret.

Issue 1:
On Server 1:

// Subscribe to topics and bind handlers
const webhookPath = '/webhook-url-1';
const webhooks = [
  [
    WebhookTopic.PRODUCTS_UPDATE,
    this.webhooksService.handleProductUpdate.bind(this.webhooksService),
  ],
];

for (const [topic, handler] of webhooks) {
  await this.shopifyInitService.shopify.api.webhooks.addHandlers({
    [topic]: [
      {
        deliveryMethod: DeliveryMethod.Http,
        callbackUrl: webhookPath,
        callback: handler,
      },
    ],
  });
}

// Register webhooks for a store
await this.shopifyInitService.shopify.api.webhooks.register({ session });

After completing these steps, let's move on to Server 2:

// Subscribe to topics and bind handlers (using a different url for the 2nd server)
const webhookPath = '/webhook-url-2';
const webhooks = [
  [
    WebhookTopic.UNINSTALL,
    this.webhooksService.uninstall.bind(this.webhooksService),
  ],
];

for (const [topic, handler] of webhooks) {
  await this.shopifyInitService.shopify.api.webhooks.addHandlers({
    [topic]: [
      {
        deliveryMethod: DeliveryMethod.Http,
        callbackUrl: webhookPath,
        callback: handler,
      },
    ],
  });
}

// Register webhooks for a store
await this.shopifyInitService.shopify.api.webhooks.register({ session });

Now, if you query the active webhooks for the store, you will only see the UNINSTALL webhook, and the PRODUCTS_UPDATE webhook will have been removed automatically.

Issue 2:
Follow the same steps as in Issue 1, but on the second server, add the PRODUCTS_UPDATE subscription instead of the UNINSTALL subscription. You will notice that the subscription from the first server has been removed, and only the PRODUCTS_UPDATE webhook with '/webhook-path-2' remains active.

[DavidWittness]

Thanks for adding some context to the issue. I would think running on two different servers wouldn't matter.

Inside the webhooks.register() call, we get the existing handlers based on the config and current session. From that list of topics, we'll add new ones and delete any leftovers. It's possible there is something in the stack causing these handlers to fall out of sync with each other.

@paulomarg, I'm not sure what the current intended functionality is right now. The platform supports multiple handlers per topic, but this test would suggest that we'd only want one handler. Any thoughts on a path forward with this issue?

[paulomarg]

Hmm, so there is an assumption in the current code that there would only be one webhook configuration for the entire app. In this case we're using 2 of them (one for each server), which creates a bit of awkwardness with the current code.

It would usually be ok to run on multiple servers if the configurations were the same - we rely on Shopify to keep the state of the current webhooks for that store, so that state would be consistent across all servers. However, since server passes in its own separate set of webhooks, that no longer applies.

I believe the current code will work for multiple addresses per webhook, but all of those addresses would need to be passed in a single configuration object.

I think there are 2 potential solutions here:

1. You continue to use multiple servers with different paths, but both servers would need to pass in the same configuration containing all the handlers across the different servers. This may not be ideal if it means you'd have to duplicate the configuration.
2. We add a flag to the registration method to prevent deleting webhooks it doesn't find. The downside of this approach is that the app would need to handle deleting webhook subscriptions in Shopify when their webhooks change.

I feel like option 2 a better compromise, since this is not a very common case, and if the app is signalling it doesn't want us to delete the webhooks, the developer will be aware that they need to manage that to keep their subscriptions in check.

[DavidWittness]

I think option 2 is covered in @mhkafadar's proposed solution.

Can you setup a PR with your proposed changes? I'll put it down and run a few tests, then update the docs.

[mhkafadar]

@DavidWittness Hi! Thank you!
I think I have to write tests before creating the PR, I need some time to write tests then I will open PR.

[mhkafadar]

Hello @DavidWittness I created a PR. I can update it after your feedbacks. Thanks

[mhkafadar]

Hello @DavidWittness any updates on your side? Thanks.