Fix registration of event_bridge and pub_sub webhooks

[kirillplatonov]

What this PR does

Right now when you try to add event_bridge webhooks the gem generates an error:

/Users/kirill/Code/shopify_app/lib/shopify_app/managers/webhooks_manager.rb:78:in `webhook_job_klass': ShopifyApp::MissingWebhookJobError 

The error is caused by the webhook's handle attribute which is used only by http webhooks. This PR makes this attribute empty for event_bridge and pub_sub webhooks.

Resolves #1505
Resolves #1297

Reviewer's guide to testing

Try adding a webhook with the delivery method event_bridge. It shouldn't generate an error anymore. Example:

config.webhooks = [{
  delivery_method: :event_bridge,
  topic: "app/uninstalled",
  path: "arn:aws:events....",
}]

Checklist

Before submitting the PR, please consider if any of the following are needed:

• Update CHANGELOG.md if the changes would impact users
• Update README.md, if appropriate.
• Update any relevant pages in /docs, if necessary
• For security fixes, the Disclosure Policy must be followed.

[kirillplatonov]

Hi @nelsonwittwer @paulomarg, I hope you're well. I noticed that currently, it's impossible to use the gem with event_bridge or pub_sub webhooks. I wrote a small fix that resolves this problem. I hope you will consider merging it quickly. Thank you for your time and attention to this matter.

[kirillplatonov]

@jeremywrowe @minichate Could anyone review and merge it, please? Due to this bug, it's impossible to use non-HTTP webhooks with shopify_app gem.

[jeremywrowe]

This seems reasonable to me, @nelsonwittwer would you mind having a look?

[nelsonwittwer]

Thanks for researching what would be needed to bring these subscriptions into this gem! It doesn't seem like that big of a shift to support this which is encouraging.

My one question is about the handler. Can you provide an example? From what I remember the naming of those files have to be exactly precise to work out of the box. I wouldn't think they would need to be different than http handlers but it sounds like they might be?

[nelsonwittwer]

Do you have an example you could provide here?

[kcamcam]

Hey @nelsonwittwer, I opened one of the issues being addressed here #1505. If it's ok I'm going to jump in here to try and speed this up, since its been causing my team some issues.

In our GCP implementation, all Shopify events (webhooks) are sent to the same subscriber endpoint (ex: /shopify/events) which dispatches (handles) the jobs. The naming conventions for the jobs are the same for HTTP and Pub/Sub. Here is an example:

module Shopify::EventsController < ApplicationController
    def dispatch
      event_job_klass.perform_later(topic: event_topic, shop: event_shop, body: event_body)
      head :created
    end

    private

    def event_topic
      params[:message][:attributes]['X-Shopify-Topic']
    end

    def event_shop
      params[:message][:attributes]['X-Shopify-Shop-Domain']
    end

    def event_body
      JSON.parse(Base64.decode64(params[:message][:data])).to_h
    end

    def event_job_klass
      event_job_klass_name.constantize
    end

    def event_job_klass_name
      "#{event_jobs_namespace}/#{event_topic.sub('/', '_')}_job".classify
    end

    def event_jobs_namespace
      ShopifyApp.configuration.webhook_jobs_namespace
    end
  end
end

Thanks for the fix @kirillplatonov 🙏 Hopefully this makes it into the next release 🤞

[kirillplatonov]

@nelsonwittwer Unfortunately, I can't share publicly the code we use for the EventBridge dispatcher. But the way it works is by pulling events from the SQS and delegating them to webhook jobs using the same convention as HTTP webhooks. The example above from @kcamcam for pub_sub is similar.

It would be very helpful to create built-in event_bridge and pub_sub handlers for that gem. But that's a topic for another pull request. For now, it would be great if we could merge this tiny fix that will allow everyone to at least start using their own handlers with shopify_app.

[kirillplatonov]

@nelsonwittwer this PR is rather a bug than a feature request. I can create a separate issue for adding built-in handlers for the event_bridge and pub_sub webhooks if you want.

And I hope this bugfix could be included in the next release. If I can do anything to make it easier for you to merge this PR please let me know.

[ahayman-diff]

Hi @nelsonwittwer, we're also running into this bug on our side. Is there anything we can help with to expedite getting this merged?

We use event bridge and have a similar sol'n to @kcamcam and @kirillplatonov.

This PR would fix our issue, but in the meantime we're going to be creating empty classes that follow the handler naming convention since the handler is not used by the shopify_api gem when the delivery_method is pubsub or eventbridge https://github.com/Shopify/shopify-api-ruby/blob/main/lib/shopify_api/webhooks/registry.rb#L23. Obviously its not ideal to have these empty classes floating around, so the sooner we can move this forward the better! Thanks ❤️

[kirillplatonov]

@nelsonwittwer any chances it will be merged until the next release? It's really tiny fix that doesn't affect HTTP webhooks users. If you could provide some feedback on what need to be changed in the PR in order to be merged I will appreciate that 🙏

[kirillplatonov]

Until PR is merged the following monkey patch can be used:

# config/initializers/shopify_app.rb
Rails.application.config.after_initialize do
  ShopifyAPI::Context.setup(...)

  # patch
  class ShopifyApp::WebhooksManager
    def self.add_registrations
      return unless ShopifyApp.configuration.has_webhooks?

      ShopifyApp::Logger.debug("Adding registrations to webhooks")
      ShopifyApp.configuration.webhooks.each do |attributes|
        webhook_path = path(attributes)
        delivery_method = attributes[:delivery_method] || :http

        ShopifyAPI::Webhooks::Registry.add_registration(
          topic: attributes[:topic],
          delivery_method: delivery_method,
          path: webhook_path,
          handler: (delivery_method == :http) ? webhook_job_klass(webhook_path) : nil,
          fields: attributes[:fields]
        )
      end
    end
  end

  # should be called after patch
  ShopifyApp::WebhooksManager.add_registrations
end

[gcaprio]

@nelsonwittwer Running into this too. Any chance we can get this merged in and not have to monkey patch?

[key88sf]

@nelsonwittwer Can you PLEASE merge this? It's been months for what is basically a 1-line fix.

[kirillplatonov]

Thank you! 🙌

[nelsonwittwer]

So sorry for the delay :( We are working on improving our priorities/processes for open source work. We know we can do better here and we appreciate your contributions and patience on this.