Unexpected JSON serialisation behaviour

[clupprich]

I ran into a weird issue when upgrading a Rails service recently.

Long story short, jbuilder has changed the way a template is transformed to JSON over the last couple of versions. See https://github.com/rails/jbuilder/blob/master/lib/jbuilder.rb#L250-L252 and the history on that line for details.

# jbuilder v2.8.0
JSON.load Jbuilder.new { |json| json.config Config::Options.new(foo: :bar) }.target!
# => {"config"=>{"foo"=>"bar"}}

# jbuilder v2.9.1
JSON.load Jbuilder.new { |json| json.config Config::Options.new(foo: :bar) }.target!
# => {"config"=>[["foo", "bar"]]}

That was kind of unexpected. The reason for that is, that Config::Options includes Enumerable and Enumerable#as_json calls to_a before serialising into JSON.

My current workaround would be to call to_h/to_hash on the Config::Options that's rendered or maybe even monkey-patching Config::Options#as_json.

I think it would be a good idea to implement Config::Options#as_json, maybe even removing Config::Options#to_json? Though that's a breaking change and would require a major version release? What do you all think?

[pkuczynski]

@Fryguy @cjlarose @rdubya what you think?

[cjlarose]

It's a little weird to add support for as_json since that's an ActiveSupport thing and config doesn't explicitly promise that it'll work, but it makes sense to want this behavior since Config::Options more-or-less behaves like a Hash where you'd expect it to.

I think just delegating calls from Config::Options#as_json(options) to to_hash.as_json(options) seems appropriate. I can open a PR.

[rdubya]

It looks like they try to skip that for OpenStructs but the check is too specific. Is there anything we can do to work around that? https://github.com/rails/jbuilder/blob/master/lib/jbuilder.rb#L313

[rdubya]

I'm not sure I am seeing where the issue is coming into play, but does this change help us at all? rails/jbuilder@e2e8623#diff-8be890cee05d282eadfd5ce2da69682cae8b9b87bb5c1e8d04004c39d7375a62

[cjlarose]

@rdubya I tested jbuilder at e2e8623b08078ad6a2323ce8ecaf642b7afe1166 and I still get the array-of-pairs representation {"config"=>[["foo", "bar"]]}

On commit 07d31ca6b8791b1767f9186a372ae3d3cbf16cf5 (current master), I get the same thing.

[cjlarose]

The change in behavior introduced with jbuilder 2.9.1 (rails/jbuilder@e2e8623) is interesting because it seems like the intent was to start respecting .to_json, which Config::Options explicitly implements. I feel like jbuilder expects objects to implement to_json if they want control over how they're represented, so it seems like we're doing the right thing already by defining that method. I'll dig into this more, but there's definitely something weird going on.

[cjlarose]

So in OP's example, Jbuilder 2.9.1 ends up doing something like this:

hash = { config: Config::Options.new(foo: :bar) }
hash.to_json

which produces the string {"config":[["foo","bar"]]}.

Before 2.9.1, Jbuilder did this:

hash = { config: Config::Options.new(foo: :bar) }
JSON.dump(hash)

which produces {"config":{"foo":"bar"}}

This is all assuming you're using ActiveSupport::ToJsonWithActiveSupportEncoder, which modifies the behavior of calling .to_json directly on objects, but not when using JSON.dump.

The JSON.dump approach appears to call Config::Options#to_json, where the Hash#to_json approach does not.

Calling Hash#to_json (again, assuming the ActiveSupport version is loaded), will call #as_json on the values, which in our case is defined by Enumerable since Config::Options includes that module.

One interesting side-effect of all of this is that if you're just using the config gem and jbuilder without the activesupport stuff loaded, OP's original example

JSON.load Jbuilder.new { |json| json.config Config::Options.new(foo: :bar) }.target!

produces the desired result, even in Jbuilder 2.9.1

{"config"=>{"foo"=>"bar"}}

[cjlarose]

Given all of that, I think OP's original suggestion of making an explicit implementation of as_json in Config::Options is the way to go. I'll keep the to_json in there, too, as to not introduce any breaking changes.

[cjlarose]

Opened #292 which adds Config::Options#as_json, which I think shouldn't introduce any major breaking changes.

Longer term, I think a more robust solution would be to remove include Enumerable from Config::Options and just explicitly handle the methods we want to expose (like #each). The default behavior for ActiveSupport's Object#as_json is exactly what we want.

https://github.com/rails/rails/blob/5384bbc4ce937b4d539518b4e3cefb91e03d4f80/activesupport/lib/active_support/core_ext/object/json.rb#L52-L60

But removing include Enumerable that would likely be a breaking change from a semver perspective, so we can hold off on that.