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

Add Nori::StringWithAttributes#as_json #100

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

HarlemSquirrel
Copy link

Fixes #99

@pcai
Copy link
Member

pcai commented Jun 14, 2023

Thanks for your contribution. Wouldn’t this cause serialization to lose the attributes? That could be unexpected. It seems odd to support an interface with an intentionally lossy implementation.

If you didn’t care about this, you could explicitly call to_s on the nori::StringWithAttributes before passing it to sidekiq. But it’s not the job of nori to decide to silently truncate data to avoid raising a serialization exception. Sidekiq is telling you there is a real bug in your application because it doesn’t know how to represent a nori string that potentially has attributes.

@HarlemSquirrel
Copy link
Author

That is a good point. Perhaps this should return a Hash instead?

Something like

{ attributes: Array, value: String }

@pcai
Copy link
Member

pcai commented Jun 14, 2023

That might be a start, but I am guessing that would just introduce a problem on the worker side. A complete solution probably requires thinking about deserialization as well. After all, the design of this sidekiq interface is to abstract away the network/process boundary with transparent serialization and deserialization of objects, so the type of the arguments passed by the caller should be the same as the types the receiver gets

@HarlemSquirrel
Copy link
Author

HarlemSquirrel commented Jun 14, 2023

Also good points. Today, with Sidekiq 6 the value is serialized as a String so for anyone moving from Sidekiq 6 to 7 as we just did would expect something like to_s to get the same values at deserialization.

Specifically, we are serializing the body from Response objects from Savon that are Hash objects generated by nori.find
https://github.com/savonrb/savon/blob/v2.14.0/lib/savon/response.rb#L42-L44

Everything in these Hashes are fine to serialize to JSON as is except for Nori::StringWithAttributes objects. These inherit from String which returns self for as_json which is currently misleading. I would expect #as_json to return a type that Sidekiq 7 is happy with (Array, Hash, String, Integer, etc.). It could mean that this is a potentially breaking change and that warrants a minor or major version bump but to_s is closer to the effective result of calling #as_json today on it's parent class.

@HarlemSquirrel
Copy link
Author

HarlemSquirrel commented Jun 14, 2023

I do realize that the #as_json for "simple" types such as String and Integer are is implemented by Rails rather that being from Ruby.
https://github.com/rails/rails/blob/v7.0.5/activesupport/lib/active_support/core_ext/object/json.rb#L92-L96

But I don't expect Rails would patch this class too so this is an interesting use case. Maybe Nori::StringWithAttributes shouldn't inherit from String if it can't serialize like one?

require "rails"
require "nori"
Nori::StringWithAttributes.new("Hello").as_json.class
# => Nori::StringWithAttributes < String

Nori::StringWithAttributes.new("Hello").to_json.class
# => String < Object

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

Successfully merging this pull request may close these issues.

Nori::StringWithAttributes should implement #as_json
2 participants