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

Webhooks throwing error when deserializing admin_graphql_api_id #600

Closed
jeffse opened this Issue Jun 18, 2018 · 25 comments

Comments

Projects
None yet
10 participants
@jeffse
Copy link

jeffse commented Jun 18, 2018

We started to receive webhook data including the new admin_graphql_api_id attribute today. This has a value of something akin to gid://shopify/Customer/61476333152.

The default WebhooksController is serializing the entire hash into the job request. When ActiveJob tries to deserialize the object, it's looking at these gid's and trying to deserialize them into ActiveRecord objects. Since these objects don't exist on our system, we get an ActiveJob::DeserializationError.

I think I can work around this by creating a custom WebhooksController that deletes all of the admin_graphql_api_id attributes, but I'd like to see a more long-term fix in shopify_app.

@EiNSTeiN-

This comment has been minimized.

Copy link
Contributor

EiNSTeiN- commented Jun 18, 2018

Which webhook event type is this happening for?

@edavis10

This comment has been minimized.

Copy link

edavis10 commented Jun 18, 2018

I saw it on themes/publish:

ActiveJob::DeserializationError: Error while trying to deserialize arguments: uninitialized constant Theme

I ended up adding a fake Theme class that did nothing but if this happens in more places I'll have to be more heavy-handed with it.

@EiNSTeiN-

This comment has been minimized.

Copy link
Contributor

EiNSTeiN- commented Jun 18, 2018

Thanks. Looks like an unintended side effect caused by a change that was released this morning. I've passed this information on to the responsible team and they will look into it.

@adrianthedev

This comment has been minimized.

@swalkinshaw

This comment has been minimized.

Copy link
Contributor

swalkinshaw commented Jun 18, 2018

@adrianthedev I've been investigating a fix for this and a custom locator was my first thought as well however it doesn't fix the problem for everyone.

Do you have a class defined in your application which matches up the resource of the webhook you're receiving? ie: if its a webhook from products/create, do you have a local Product class?

Another problem is here: https://github.com/rails/globalid/blob/5bfe23a8ab3ab49c9050bc3ad0668f5c01492cf6/lib/global_id/locator.rb#L17

gid.model_class gets called even if you have a custom locator and will cause an exception if that constant doesn't exist.

@adrianthedev

This comment has been minimized.

Copy link

adrianthedev commented Jun 18, 2018

yes. I have a class Product, but it's different for Order. It's namespaced. It started to fail on my end as well. Will update in a sec.

@adrianthedev

This comment has been minimized.

Copy link

adrianthedev commented Jun 18, 2018

The quickest quickfix is to have dummy (empty) models for everything you expect to receive in webhooks until this is fixed properly from shopify.

@swalkinshaw

This comment has been minimized.

Copy link
Contributor

swalkinshaw commented Jun 18, 2018

I'm working on a potential upstream fix to GlobalID which combined with a custom locator (that shopify_app will define) would fix this. It looks like the best solution so far.

@adrianthedev

This comment has been minimized.

Copy link

adrianthedev commented Jun 18, 2018

Sounds good!

@jeffse

This comment has been minimized.

Copy link

jeffse commented Jun 18, 2018

I ended up filtering out all of the Global IDs from the webhook data for now.

class ApplicationJob < ActiveJob::Base

  def deserialize(job_data)
    sanitized_data = recursive_delete_gids(job_data)
    super(sanitized_data)
  end

private

  def recursive_delete_gids(hash)
    hash.each do |key, value|
      case value
      when String
        hash.delete(key) if value.start_with? 'gid://'
      when Hash
        recursive_delete_gids(value)
      when Array
        value.each do |array_value|
          recursive_delete_gids(array_value) if array_value.is_a? Hash
        end
      end
    end
  end
end
@swalkinshaw

This comment has been minimized.

Copy link
Contributor

swalkinshaw commented Jun 18, 2018

There's another alternative solution which may be worthwhile regardless. Shopify webhooks take a fields param (https://help.shopify.com/api/reference/events/webhook#properties) which restricts what fields are sent in the payload.

If you only want a subset of the fields, it's a good practice to use anyway to reduce the payload size. In this case, it could be used to set a whitelist and not include admin_graphql_api_id. You can update your existing webhook subscriptions with that.

@edavis10

This comment has been minimized.

Copy link

edavis10 commented Jun 18, 2018

This is what I'm using for the Theme one. If I had a Theme model then it'd be a much bigger issue

# Class used to prevent ActiveJob and GlobalId from erroring when
# the Shopify API sends GlobalID inside of the GraphQL API
#
# e.g. "admin_graphql_api_id":"gid://shopify/Theme/11810989"
#
class Theme
  include GlobalID::Identification

  # No-op
  def self.find(*args)
    nil
  end
end
@swalkinshaw

This comment has been minimized.

Copy link
Contributor

swalkinshaw commented Jun 18, 2018

I think you could combine two strategies like this:

# config/initializers/global_id.rb

GlobalID::Locator.use :shopify do |gid|
end

Theme = Class.new
# define others as needed

Then the class will exist, but locator should just return the string back.

@adrianthedev

This comment has been minimized.

Copy link

adrianthedev commented Jun 18, 2018

yes, but you still need to define all classes. Order, LineItem, Product, etc.

@talecK

This comment has been minimized.

Copy link

talecK commented Jun 18, 2018

I am seeing a similar issue, woke up to thousands of failed ProductCreateJobs in resque.

We are seeing the errors in ProductsCreateJob, which is weird since we define product.rb < ActiveRecord::Base.

Any advice on how we should proceed in fixing this in the short term?

@adrianthedev

This comment has been minimized.

Copy link

adrianthedev commented Jun 18, 2018

this is the best short term @talecK

#600 (comment)

@adrianthedev

This comment has been minimized.

Copy link

adrianthedev commented Jun 18, 2018

IMHO the best thing would be to have a ignored_fields, similar to fields in shopify_app.rb config. This way Shopify can keep sending those fields and configuration would be easy to use.

@eapache

This comment has been minimized.

Copy link

eapache commented Jun 18, 2018

We are exploring the possibility of removing this field from the webhook payloads we send temporarily, until we can work out the details here.

@lulessa

This comment has been minimized.

Copy link
Contributor

lulessa commented Jun 18, 2018

One solution temporary workaround is to base64 encode the received webhook payload before enqueueing the job with ActiveJob. Of course, this will increase your job argument size by about 20% and add a little processing time in the webhooks controller.

class WebhooksController < ShopifyApp::WebhooksController
[...]
webhook_data = Base64.encode64(webhook_params.to_json)
MyJob.perform_later(shop_domain: shop_domain, webhook: webhook_data)
class ProductsUpdateJob < ActiveJob::Base

  def perform(shop_domain:, webhook:)
    # NOTE: Temporary fix for gid deserialization error
    if webhook.is_a?(String)
      webhook = JSON.parse(Base64.decode64(webhook))
    end
    # Continue on to use webhook var as if it was a Hash
    [...]
  end
end
@eapache

This comment has been minimized.

Copy link

eapache commented Jun 19, 2018

We are no longer sending this field in webhook payloads for now.

@djones

This comment has been minimized.

Copy link
Contributor

djones commented Jun 19, 2018

Thanks @eapache!

@laverick

This comment has been minimized.

Copy link

laverick commented Jun 19, 2018

@eapache If this field will be included again in the future can we get a bit more info on the timelines and recommended handling once available? Thanks.

@eapache

This comment has been minimized.

Copy link

eapache commented Jun 19, 2018

Absolutely, we'll post that information before we expose the field again.

@swalkinshaw

This comment has been minimized.

Copy link
Contributor

swalkinshaw commented Jun 19, 2018

I'm going to close this issue for now as we're no longer including this field for new webhooks since yesterday around ~7 PM EST (23:00 UTC).

The admin_graphql_api_id fields still exist in REST responses. They were added to help with interop/migration with our new Admin GraphQL API.

For consistency (and usefulness), they will ideally be included in webhooks again soon once we come up with a solution. This will likely involve having to update to a newer version of this gem though.

@laverick

This comment has been minimized.

Copy link

laverick commented Jun 20, 2018

Thanks @swalkinshaw & @eapache

If bringing those fields back ends up requiring a gem update for apps to continue working it would be good to have plenty of notice.

What's the best channel to monitor for updates?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment