I think that the json_safe? method should return true for symbols, to avoid the "Job arguments to **** must be native JSON types" error

[ndbroadbent]

Ruby version: 2.7.1
Rails version: 6.0.4.6
Sidekiq / Pro / Enterprise version(s): Sidekiq 6.4.0

I had to hunt down why the Job arguments to #{item["class"]} must be native JSON types error was happening in lib/sidekiq/job_util.rb. It turned out that json_safe?(item) was returning false because I passing in a symbol as the first job argument. I have some Shrine uploader jobs that use symbols for the action arg (:promote, :delete):

class TemplateUploader < Shrine
  Attacher.promote do |data|
    ShrineJob.perform_async(:promote, data)
  end
  Attacher.delete do |data|
    ShrineJob.perform_async(:delete, data)
  end

  # ...

I believe it's generally safe to convert symbols to strings, so I was wondering if json_safe? could be a bit more lenient. Here's the original method:

    def json_safe?(item)
      JSON.parse(JSON.dump(item["args"])) == item["args"]
    end

What do you think about this change?

    def json_safe?(item)
      stringified_args = item["args"].map { |arg| arg.is_a?(Symbol) ? arg.to_s : arg }
      JSON.parse(JSON.dump(item["args"])) == stringified_args
    end

(Could even go a little further to ignore symbols in hash keys: arg.is_a?(Hash) ? arg.with_indifferent_access : arg)

I don't feel too strongly about it, but it would have just saved me a little bit of time. Would you accept a PR with this change? (No worries if not, I'm happy to switch them to strings.)

[ndbroadbent]

I've just noticed another place where this is happening in my code:

      InternalSlackNotificationJob.perform_async(
        channel: '#customers',
        text: slack_message,
        attachments: [
          {
            title: "View Account: #{account.name_or_email_or_uid}",
            title_link: Rails.application.routes.url_helpers.admin_account_url(account),
          },
        ]
      )

I'm actually using symbols as hash keys all over my app for this InternalSlackNotificationJob class, so it will be a bit annoying for me to change all of these into strings.

What do you think about this change?

    def json_safe?(item)
      stringified_args = item["args"].map do |arg|
        case arg
        when Symbol
          arg.to_s
        when Hash
          arg.with_indifferent_access
        else
          arg
        end
      end
      JSON.parse(JSON.dump(item["args"])) == stringified_args
    end

[ndbroadbent]

Sorry for the noise, I read through the discussion in #5071. I can see that symbols are intentionally banned for this reason (with strict args):

def perform(hash)
    id = hash[:id] # will never exist, but hash['id'] will!

I suppose I will need to use Sidekiq.strict_args!(false) for now. But I think it would be more user-friendly to just call with_indifferent_access behind the scenes for any arguments that are hashes, so we can actually pass in a hash with symbols as keys, and then continue to use symbols to reference values inside #perform. Just my two cents! But I'm happy with Sidekiq.strict_args!(false) since you mentioned that it will always be supported for backwards compatibility. Thanks!

---

EDIT: Just another 2 cents... I was thinking some more, and symbols are such a big part of Ruby that it would be even nicer if you could use some different marshaling format that actually preserved the difference between symbols and strings. It feels really weird to use strings as args or hash keys when the rest of my app uses symbols everywhere else. It feels like the bug here is using JSON, which doesn't fully support Ruby data types (since Sidekiq is Ruby on Rails job processor, and symbols are a big part of Ruby code.)

Maybe a future Sidekiq version could use msgpack with an extension type for symbols. This would also be great for dates and times.

Anyway, that's enough from me. Hope you're having a good week!

[adamniedzielski]

“with_indifferent_access” is an ActiveSupport thing. Sidekiq doesn’t depend on ActiveSupport. You can write your own middleware, see #5246

…

On Tue 22. Mar 2022 at 01:40, Nathan Broadbent ***@***.***> wrote: Closed #5252 <#5252>. — Reply to this email directly, view it on GitHub <#5252 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AASQMEXNDG2BP35Y4XPUIULVBEJG7ANCNFSM5RJHVOMA> . You are receiving this because you are subscribed to this thread.Message ID: ***@***.***>