getvero / vero

Ruby gem for Vero
http://www.getvero.com
18 stars 22 forks source link

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

Open daneshd opened 1 month ago

daneshd commented 1 month ago

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 commented 3 weeks ago

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.