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

JSON gem compatibility #12862

Merged
merged 1 commit into from Nov 15, 2013
Merged

JSON gem compatibility #12862

merged 1 commit into from Nov 15, 2013

Conversation

@chancancode
Copy link
Member

@chancancode chancancode commented Nov 12, 2013

This is extracted from #12183 as an isolated PR. I documented the current state of JSON-gem-vs-active-support incompatibility here. Clearly, you cannot currently use the JSON gem's dump or generate method in conjunction with ActiveSupport's JSON encoder, otherwise, things may break unexpectedly on certain input at runtime with a cryptic error. This limitation is not currently very well known. It's probably safe to assume that the situation is going to get worse as we move to Ruby 1.9+ (where the JSON gem ships with the stdlib) and as multi_json is being EOL'ed and removed from Rails 4.1 (many gem authors simply replaced MultiJson.dump with ::JSON.{dump,generate}.

To improve the situation, we can do one of the following things:

  1. Do nothing: This feels irresponsible, especially when we know that this is going to fail in subtle and non-obvious way in seemingly (to the user) unpredictable manner.
  2. Document this behaviour and warn people about using JSON gem together with Rails: this doesn't really help, who read documentations? In practice, this would have the same effect as 1.
  3. Boycott JSON gem, override ::JSON.{dump,generate} to raise an exception when ActiveSupport::JSON is loaded: while this might be ideal from a purist perspective, I'm afraid this is a non-starter – even some of our dependencies are currently using ::JSON.{dump,generate} in their codebase.
  4. Make it possible for AS::JSON and JSON gem to coexist in peace: this is what I propose to do. It seems like the only reasonable and responsible thing to do.

With this PR, calling ::JSON.{generate,dump} will invoke the JSON gem's encoder. It will not invoke any of the ActiveSupport::JSON logic, i.e. it will bypass as_json and ignore any options that it does not natively understand (e.g. only, except, etc). Instead of using the ad-hoc approach in #9295, this PR changes our version of to_json to add some triage code. The end result is that invoking ::JSON.{generate,dump} will yield the same result before and after ActiveSupport::JSON is loaded.

If I get a 👍 from core, I'll can clean things up, add the changelog, etc.

cc @jeremy

@jeremy
Copy link
Member

@jeremy jeremy commented Nov 12, 2013

Good idea. This works how everyone would expect.

@chancancode
Copy link
Member Author

@chancancode chancancode commented Nov 12, 2013

👍 Good to merge

[Object, Array, FalseClass, Float, Hash, Integer, NilClass, String, TrueClass].each do |klass|
klass.class_eval do
# Dumps object in JSON (JavaScript Object Notation). See www.json.org for more info.

This comment has been minimized.

@chancancode

chancancode Nov 12, 2013
Author Member

Is this important? Is there a way to preserve the documentation when using AMC?

Previously, calling `::JSON.{generate,dump}` sometimes causes
unexpected failures such as intridea/multi_json#86.

`::JSON.{generate,dump}` now bypasses the ActiveSupport JSON encoder
completely and yields the same result with or without ActiveSupport.
This means that it will **not** call `as_json` and will ignore any
options that the JSON gem does not natively understand. To invoke
ActiveSupport's JSON encoder instead, use `obj.to_json(options)` or
`ActiveSupport::JSON.encode(obj, options)`.
jeremy added a commit that referenced this pull request Nov 15, 2013
JSON gem compatibility
@jeremy jeremy merged commit bed324c into rails:master Nov 15, 2013
1 check was pending
1 check was pending
default The Travis CI build is in progress
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.