Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Warn if stdlib json is used. #107

Merged
merged 1 commit into from

3 participants

@rwz
Collaborator

MultiJson loads stdlib version of json gem by default. It's broken in many ways, doesn't support some of the options we pass there and segfaults on nil input (see flori/json#165).

I think MultiJson should check the version of json gem loaded and complain loudly if the version is not the latest one.

What do you think, @sferik?

@rwz rwz was assigned
@rwz
Collaborator

Working on that I've studied json gem deeply enough to realize three things:

  • Right now we have no idea which code (json/ext or json/pure) is being used when. Usually it's the code that was required last.
  • json/pure is buggy
  • The only way to surely switch between them I've managed to figure is quite slow.

Here's the work-in-progress branch I ended up with which has this switching implemented. It randomly hits some json/pure bugs from time to time.

@sferik what do you think about all this?

@sferik
Collaborator

The bugs you've referenced seem to be fixed in the latest patchlevels of Ruby 1.9.3 and 2.0.0. If someone reports a bug with an old version of stdlib JSON, we can politely ask them to either upgrade their Ruby version to the latest patchlevel or to use the JSON gem. I don't think we need to make any changes to MultiJSON.

I don't want to get into the business of requiring specific gem versions. Perhaps we could issue a warning if we can detect that someone is using a known-bad version of stdlib Ruby but I wouldn't want to go any farther than that.

@rwz
Collaborator

Well, I've hit this bug using latest json_pure 1.7.7. The reason I've not submitted an issue is that I can't isolate it and recreate outside of MultiJson. :(

Anyway, I got your point and will present a pull-request soon.

@rwz
Collaborator
rwz commented

@sferik Please review.

@coveralls

Coverage Status

Coverage decreased (-0.04%) when pulling 557aedc on stdlib-warn into 035efe5 on master.

@coveralls

Coverage Status

Coverage increased (+0.55%) when pulling e7438e7 on stdlib-warn into d441ec6 on master.

@sferik
Collaborator

If you've run into this bug using the latest json_pure 1.7.7 is this actually a good solution? I don't think it makes sense to warn users to update to the latest version if that doesn't actually fix the problem.

@rwz
Collaborator
rwz commented

The bugs mentioned there only happen with stdlib gem. 1.7.7 is fine

@sferik sferik merged commit dd9e8e6 into master

1 check passed

Details default The Travis build passed
@sferik sferik deleted the stdlib-warn branch
@l8nite l8nite referenced this pull request from a commit in practicefusion/figaro
@laserlemon laserlemon Pull in the latest json gem to avoid MultiJSON warnings
…about using an outdated JSON library. The warning produces unexpected
output in the Cucumber suite's Rails commands.

See: intridea/multi_json#107
2b157ff
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on May 4, 2013
  1. @rwz
This page is out of date. Refresh to see the latest.
View
4 lib/multi_json.rb
@@ -87,7 +87,9 @@ def adapter
# * <tt>:gson</tt> (JRuby only)
# * <tt>:jr_jackson</tt> (JRuby only)
def use(new_adapter)
- @adapter = load_adapter(new_adapter)
+ adapter = load_adapter(new_adapter)
+ adapter.activate! if adapter.respond_to?(:activate!)
+ @adapter = adapter
end
alias adapter= use
alias engine= use
View
10 lib/multi_json/adapters/json_common.rb
@@ -5,6 +5,16 @@ module Adapters
class JsonCommon < Adapter
defaults :load, :create_additions => false, :quirks_mode => true
+ GEM_VERSION = '1.7.7'
+
+ def self.activate!
+ if JSON::VERSION < GEM_VERSION
+ Kernel.warn "You are using an old or stdlib version of #{gem_name} gem\n" +
+ "Please upgrade to the recent version by adding this to your Gemfile:\n\n" +
+ " gem '#{gem_name}', '~> #{GEM_VERSION}'\n"
+ end
+ end
+
def load(string, options={})
string = string.read if string.respond_to?(:read)
View
4 lib/multi_json/adapters/json_gem.rb
@@ -6,6 +6,10 @@ module Adapters
# Use the JSON gem to dump/load.
class JsonGem < JsonCommon
ParseError = ::JSON::ParserError
+
+ def self.gem_name
+ 'json'
+ end
end
end
end
View
4 lib/multi_json/adapters/json_pure.rb
@@ -6,6 +6,10 @@ module Adapters
# Use JSON pure to dump/load.
class JsonPure < JsonCommon
ParseError = ::JSON::ParserError
+
+ def self.gem_name
+ 'json_pure'
+ end
end
end
end
View
19 spec/multi_json_spec.rb
@@ -50,6 +50,25 @@
end
end
+ context 'with stdlib version' do
+ around do |example|
+ version = JSON::VERSION
+ silence_warnings{ JSON::VERSION = '1.5.5' }
+ example.call
+ silence_warnings{ JSON::VERSION = version }
+ end
+
+ it 'should warn about json' do
+ Kernel.should_receive(:warn).with(/'json', '~> 1.7.7'/)
+ MultiJson.use :json_gem
+ end
+
+ it 'should warb about json/pure' do
+ Kernel.should_receive(:warn).with(/'json_pure', '~> 1.7.7'/)
+ MultiJson.use :json_pure
+ end
+ end
+
it 'defaults to the best available gem' do
# Clear cache variable already set by previous tests
MultiJson.send(:remove_instance_variable, :@adapter)
Something went wrong with that request. Please try again.