search

intridea / multi_json

A generic swappable back-end for JSON handling.
http://rdoc.info/projects/intridea/multi_json
MIT License
757 stars 129 forks source link

MultiJSON assumes that the JSON ext is loaded if defined?(::JSON::JSON_LOADED) #196

Closed eregon closed 4 years ago

eregon commented 4 years ago

See https://github.com/intridea/multi_json/blob/66864817eb2fae85e9a93d8823877d75851ad666/lib/multi_json.rb#L50

But that is unfortunately incorrect, because JSON pure also defines that constant: https://github.com/flori/json/blob/b20cdca8eb03297132bcf3b786eba602fa04ca37/lib/json/pure.rb#L14

I think using defined?(::JSON::Ext) would work better instead.

This causes an issue on TruffleRuby where JSON pure is used by default (and require 'json/ext' raises unless the json gem is installed additionally). If json is required before multi_json, then multi_json raises an error: https://github.com/oracle/truffleruby/issues/2032#issuecomment-641884150

On MRI it's quite confusing as it will detect JSON pure as :json_gem (same issue), but still work because it will then require 'json/ext' which will work (but have 2 versions of JSON loaded, so it seems suboptimal).

eregon commented 4 years ago

I'll try to make a PR for this.

eregon commented 4 years ago

An additional confusing thing is that json_pure seems to ship a json_pure-2.3.0/lib/json/ext.rb file which defines JSON::Ext. But I guess if something require 'json/ext' they really meant to use the native JSON, even if json_pure was earlier in the load path and we end up with something like:

$ ruby -d -e 'gem "json_pure"; require "json/pure"; require "json/ext"; puts $"; p JSON::Ext'
Using Pure library for JSON.
Using Ext extension for JSON.
...
/home/eregon/.rubies/ruby-2.6.6/lib/ruby/gems/2.6.0/gems/json_pure-2.3.0/lib/json/pure.rb
/home/eregon/.rubies/ruby-2.6.6/lib/ruby/gems/2.6.0/gems/json-2.3.0/lib/json/ext/parser.so
/home/eregon/.rubies/ruby-2.6.6/lib/ruby/gems/2.6.0/gems/json-2.3.0/lib/json/ext/generator.so
/home/eregon/.rubies/ruby-2.6.6/lib/ruby/gems/2.6.0/gems/json_pure-2.3.0/lib/json/ext.rb
eregon commented 4 years ago

So I guess we should check for JSON::Ext::Parser (or JSON::Ext::Generator). That's not defined, even after require "json/ext" if there is no C extension.

eregon commented 4 years ago

PR to fix this: https://github.com/intridea/multi_json/pull/197