JsonGem and JsonPure adapters should work with the yajl-ruby compatible ::JSON implementations.

[tommyh]

yajl-ruby's "yajl/json_gem" overwrites ::JSON implementation in a "compatible" way, but the options expected are different. yajl-ruby expects :symbolize_keys, while the json gem expects :symbolize_names.

yajl-ruby has patched this issue in HEAD, for v2 of the gem which is unreleased, but it doesn't appear they will backport to v1.1. v1.1.0 was released 2 years ago, so I'm not certain when v2 will be released...

brianmario/yajl-ruby#94
brianmario/yajl-ruby#105

To explain the impact, I just hit this issue last week and here are some other people who have run into it as well: sferik/twitter-ruby#317

Note: I read the contribution guidelines and tried todo open coverage/index.html, but i didn't have a coverage directory - even after running the rspec tests. Not sure if this is a requirement anymore or not.

[rwz]

Weeell, I personally don't like this approach. When you require yajl/json_gem, original Yajl also gets required and I see no point of having/using a separate adapter just for the wrapper. If you have required Yajl, MultiJson will pick it up and use Yajl adapter by default. Also, it won't confuse wrapper with JSON gem since it looks for JSON::JSON_LOADED constant which Yajl wrapper doesn't define

[tommyh]

I understand what you're saying about how yajl and JSON should play nicely together, and under normal circumstances they totally do. But depending on when certain classes get loaded, an issue with symbolize_keys will present itself.

I did a poor job of explaining the issue, so when i get to the office I'll put some example code which explains it better, :).

Also, I didn't create another adapter. I created a "yajl_compatability" spec for the "json_adapter". The naming of my spec is a bit confusing, but I tried to fit it into the "Dir glob" "_adapter_spec.rb" naming scheme, this was possibly a bad idea. My spec is an odd case, so I can try to make that stand out in the naming better.

I'll post a follow up with code later today.

[tommyh]

When multi_json uses the :json_gem adapter with the ::JSON implementation provided by the json gem, everything works fine:

tom:~/workspace/multi_json (master)$ bundle exec irb
1.9.3p448 :001 > require 'json'
 => true 
1.9.3p448 :002 > require 'multi_json'
 => true 
1.9.3p448 :003 > MultiJson.adapter
 => MultiJson::Adapters::JsonGem 
1.9.3p448 :004 > MultiJson.load('{"abc":"def"}', :symbolize_keys => true)
 => {:abc=>"def"} 

But when multi_json uses the :json_gem adapter with the ::JSON implementation provided by the yajl-ruby gem, :symbolize_keys option isn't recognized:

tom:~/workspace/multi_json (master)$ bundle exec irb
1.9.3p448 :001 > require 'json'
 => true 
1.9.3p448 :002 > require 'multi_json'
 => true 
1.9.3p448 :003 > MultiJson.adapter
 => MultiJson::Adapters::JsonGem 
1.9.3p448 :005 > require 'yajl/json_gem'
 => true 
1.9.3p448 :006 > MultiJson.load('{"abc":"def"}', :symbolize_keys => true)
 => {"abc"=>"def"} 

My pull request fixes this issue. If you revert the change I made (tommyh@7285909#L2L15), you'll notice how my compatibility specs will fail.

Note: this is an issue with the yajl-ruby gem because it's "compatible" implementation of ::JSON expects :symbolize_keys, not :symbolize_names. Because this is an issue with yajl-ruby, you might be wondering why I'm not just opening a ticket in that project - which is a fair question. It is patched in the unreleased version 2 branch of yajl-ruby brianmario/yajl-ruby#94, but it doesn't appear it'll be back ported to the stable version 1.1 branch. And the unstable version has been in development for about 2 years (last commit 7 months ago), so this issue probably won't be going away anytime soon.

[rwz]

But when multi_json uses the :json_gem adapter with the ::JSON implementation provided by the yajl-ruby gem, :symbolize_keys option isn't recognized:

You're doing it wrong. You should not use JsonGem adapter with anything, but JSON gem.

If you have Yajl, use Yajl adapter, if you have JSON gem, use JsonGem

[tommyh]

While I understand what you are saying, I disagree. My "steps to reproduce" are a simplified version of what is occurring in my codebase. Most of the code is in external gems.

require 'json' comes from require active_support/core_ext (if my memory is correct, I have to double check exactly which active_support class requires json)

require multi_json comes from being automatically loaded when bundler boots up (multi_json is a dependency of one of our gems, we don't use it directly)

require 'yajl/json_gem' comes from our codebase. We have decided to use this as our parser.

MultiJson.load('{"abc":"def"}', :symbolize_keys => true) this is occurring inside of a gem we use.

Because the multi_json "gem is primarily for library authors" (http://www.intridea.com/blog/2010/6/14/multi-json-the-swappable-json-handler), we're not even using multi_json directly. Everything worked fine until a require json was put early in our application boot, then everything started to break inside of a gem we use.

@sferik said "This seems like a very serious bug in multi_json": sferik/twitter-ruby#317 (comment) - so I'm a bit confused what I'm doing wrong.

Here are 2 other people who are hitting the exact same issue:

• ArgumentError: argument must have an :id key sferik/twitter-ruby#317 (comment) + ArgumentError: argument must have an :id key sferik/twitter-ruby#317 (comment)
• ArgumentError: argument must have an :id key sferik/twitter-ruby#317 (comment)

And my pull request comes with the fix for the issue (failing test first) which was a very small change - no new adapter needed. in json_common.rb when you are already putting the value for symbolize_keys into symbolize_names, I simply leave the symbolize_keys option there too.

[rwz]

This is not a MultiJson bug, this is a result of a misconfiguration I suppose. The steps how something like this might happen are:

1. load AS, which does require 'json' internally
2. require 'multi_json' using Bundler, for example
3. actually use MultiJson for something (calling MultiJson.adapter is sufficient)
4. require yajl/json_gem overwriting JSON constant and putting Yajl there instead

Now, first time MultiJson is used, it looks for already required libs and finds JsonGem and happily sets it as an adapter. After that, you rewrite JSON constant with Yajl and MultiJson is not aware of that.

Here are two possible solutions:

1. Explicitly make MultiJson use Yajl adapter by creating an initializer with code like this: MultiJson.adapter = :yajl
2. Explicitly require yajl/json_gem by Bundler. That'll load plain Yajl as well and make MultiJson pick the correct adapter in the first place. Put this into your Gemfile: gem 'yajl-ruby', require: 'yajl/gson_gem'

[tommyh]

But as an application developer, how would I even know #1 is needed? How would I even know what multi_json is? (It's used by gems which my app uses, I as an application developer don't reference it in my code or even have it specified in my Gemfile).

#2 also requires I have intimate knowledge of multi_json's adapter strategy.

Are you saying #1 or #2 should be performed by every application developer who wants to use the yajl/json_gem? To me that sounds like a violation of the rule of least surprise.

I'm not saying this is a bug in multi_json itself, but this fix alleviates an incompatibility between 2 json parsers. Isn't that the goal of multi_json - to allow library developers to perform json parsing while allowing application developers to choose the json parser.

I'm confused on where the pushback is coming from - is my fix too hard to maintain? Is supporting yajl/json_gem too difficult? Have you had issues with it in the past?

On Sep 12, 2013, at 12:49 AM, Pavel Pravosud notifications@github.com wrote:

This is not a MultiJson bug, this is a result of a misconfiguration I suppose. The steps how something like this might happen are:

1. load AS, which does require 'json' internally
2. require 'multi_json'
3. actually use MultiJson for something (calling MultiJson.adapter is sufficient)
4. require yajl/json_gem overwriting JSON constant and putting Yajl there instead

Now, first time MultiJson is used, it looks for already required libs and finds JsonGem and happily sets it as an adapter. After that, you rewrite JSON constant with Yajl and MultiJson is not aware of that.

Here are two possible solutions to this:

1. Explicitly make MultiJson use Yajl adapter by creating an initializer with code like this: MultiJson.adapter = :yajl
2. Explicitly require yajl/json_gem by Bundler. That'll load plain Yajl as well and make MultiJson oick the correct adapter in the first place.

—
Reply to this email directly or view it on GitHub.

[rwz]

When you choose to use something, that overrides stdlib llibriary constant and changes its implementation, you're expected to know what you're doing.

So, yes, if you want to use yajl/json_gem you should be ready for any kind of very hard to track bugs like this one or worse. Because it is a VERY stupid idea to begin with.

The goal of MultiJson is to provide a minimal thin wrapper layer for different json adapters. MultiJson won't be able to track and guard against any possible way a developer can shoot his own foot and doesn't have such goal.

[tommyh]

I agree with a "thin wrapper for json adapters" - I think my fix fits within that definition - is my fix "too thick"?

You mention "or worse issues" - have any been submitted? Are there any you know about? If not, would it be unreasonable to merge my fix, then wait and see. If people start complaining about yajl/json_gem issues, then you can officially drop support for it.

I'm not asking for the adapter strategy to be changed or something which would have a material affect on people (or leading more people to shoot themselves in the foot). This is just fixing one incompatibility which other people are hitting too.

On Sep 12, 2013, at 1:12 AM, Pavel Pravosud notifications@github.com wrote:

When you choose to use something, that overrides stdlib llibriary constant and changes it's implementation, you're expected to know what you're doing.

So, yes, if you want to use yajl/json_gem you should be ready for any kind of very hard to track bugs like this one or worse. Because it is a VERY stupid idea to begin with.

The goal of MultiJson is to provide a minimal thin wrapper layer for different json adapters. MultiJson won't be able to track and guard against any possible way a developer can shoot his own foot and doesn't have such goal.

—
Reply to this email directly or view it on GitHub.