Skip to content
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

Sidekiq worker arguments do not serialize to JSON safely and make it incompatible with Sidekiq 7's strict_args #79

Open
daneshd opened this issue Oct 28, 2024 · 1 comment

Comments

@daneshd
Copy link

daneshd commented Oct 28, 2024

Sidekiq 7 enforces that arguments be serialized to JSON safely by default. From what I can tell, the Vero::SidekiqWorker class does not meet this requirement. Any time it's called, we see this warning:

WARN: Job arguments to Vero::SidekiqWorker do not serialize to JSON safely. This will raise an error in Sidekiq 7.0. See https://github.com/mperham/sidekiq/wiki/Best-Practices or raise an error today by calling Sidekiq.strict_args! during Sidekiq initialization.

I believe the options arg is causing the issue since the other two args are strings. It's being fed in as a ruby hash where the keys are symbols. I believe it should be converted to JSON before being sent to Sidekiq. Fortunately, BaseApi#options_with_symbolized_keys is being run and converting the strings back to symbols. However we weren't sure how the values in options are used. There's usually a data value that's a hash and that may run into similar issues for missing key references.

We've implemented the below workaround to temporary patch converting options to JSON when queuing the worker up and then apply with_indifferent_access in the worker so that symbol-based keys would get picked up, just in case that is used by the gem.

However, it would be great to see a proper fix for it.

Workaround

Note if you're using this gem outside of a Rails app, with_indifferent_access won't be available.

module Vero
  class SidekiqWorker
    def perform(api_class, domain, options)
      api_class.constantize.new(domain, options.with_indifferent_access).perform
      Vero::App.log(self, "method: #{api_class}, options: #{options.to_json}, response: sidekiq job queued")
    end
  end

  module Senders
    class Sidekiq
      def call(api_class, domain, options)
        response = ::Vero::SidekiqWorker.perform_async(api_class.to_s, domain, options.as_json)
        Vero::App.log(self, "method: #{api_class.name}, options: #{options.to_json}, response: sidekiq job queued")
        response
      end
    end
  end
end
@jylamont
Copy link
Contributor

Thanks for this @daneshd! We are planning to make another release of this gem in the near future and will aim to address warnings like this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants