nori icon indicating copy to clipboard operation
nori copied to clipboard

Add Nori::StringWithAttributes#as_json

Open HarlemSquirrel opened this issue 2 years ago • 5 comments

Fixes #99

HarlemSquirrel avatar Jun 14 '23 16:06 HarlemSquirrel

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.

pcai avatar Jun 14 '23 16:06 pcai

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

Something like

{ attributes: Array, value: String }

HarlemSquirrel avatar Jun 14 '23 16:06 HarlemSquirrel

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

pcai avatar Jun 14 '23 17:06 pcai

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 avatar Jun 14 '23 17:06 HarlemSquirrel

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

HarlemSquirrel avatar Jun 14 '23 18:06 HarlemSquirrel