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
Experimental - Add support for a new flavor of json serialization configuration #6209
base: main
Are you sure you want to change the base?
Conversation
Why not make a configuration option which receives the serializer we want to use? That way we could implement one by ourselves, like: class MySerializer
def self.dump(value)
::GraphQL::Subscriptions::Serialize.dump(value)
end
def self.load(value)
::GraphQL::Subscriptions::Serialize.load(value)
end
end
Sidekiq.serializer = MySerializer |
@sobrinho Conventions >>> Configuration. Sidekiq's convention is that all job data is native JSON, period. That also means you can use Redis's JSON functionality with Sidekiq. https://redis.io/docs/data-types/json/ If we allow the user to plug in their own format, we essentially lose all ability to work with jobs outside of Ruby. Some commercial functionality needs to parse/edit job data in Lua inside Redis. I can't do that if the job payload is opaque bytes. |
But the data of the job itself should be ignored by Sidekiq, no? The job metadata I agree but the data passed around for the job (the args) isn't used by Sidekiq internals. |
What I mean is that we could use Marshall/JSON/YAML/whatever to dump/load the args passed to the job to add support for special object types, symbols or whatever is needed. |
Ah, fair enough. Yes, it seems weird to treat the args element separately from the rest of the payload but I see your point. I'm still unlikely to accept this PR, given the limitation around Symbol keys but at least it's good to know the possibilities. |
I really like this implementation and I'm using it more and more in one of our projects here (since we already depend on that gem): https://github.com/rmosolgo/graphql-ruby/blob/master/lib/graphql/subscriptions/serialize.rb It compiles to "JSON" and adds support to a bunch of things. Currently we are using something like this: class MyJob
include Sidekiq::Job
def self.perform_async(*args, **kwargs)
data = GraphQL::Subscriptions::Serialize.dump([args, kwargs])
super(data)
end
def perform(data)
args, kwargs = ::GraphQL::Subscriptions::Serialize.load(data)
_perform(*args, **kwargs)
end
def _perform(myarg, mykwarg:)
# ...
end
end That's why I wonder of why not have support for this natively and keep the JSON implementation as the native one. We could even have other natively if desired such as YAML to support more complex data but that should be enough to not have to do trickery stuff like we did. |
It would be almost trivial to create a SerializedArguments module prepended to Sidekiq::Job which stores the marshaller as a job payload element and runs the job arguments through the marshaller before calling |
This change allows the user to opt-into a more intrusive JSON configuration which can transparently marshal most core Ruby types. This allows the user to use almost any core datatype as an argument to Job#perform.
For example:
Symbols
By far the biggest limitation is that it still does not handle Symbols as Hash keys so we still can't marshal keyword arguments or typical option hashes. It does handle Symbols as plain arguments and Array/Hash values.
ActiveSupport
It does clash with the ActiveSupport JSON monkeypatches in
active_support/core_ext/object/json.rb
. There's not much I can do about that, I don't know how that might affect Rails apps. They should not be monkeypatching core gems but opening issues to fix any problems.Fixes #6208