From ff1192fea40c55a11c52e26f22a814d68d058170 Mon Sep 17 00:00:00 2001 From: Godfrey Chan Date: Tue, 5 Nov 2013 20:05:58 -0800 Subject: [PATCH] Eliminate `JSON.{parse,load,generate,dump}` and `def to_json` JSON.{dump,generate} offered by the JSON gem is not compatiable with Rails at the moment and can cause a lot of subtle bugs when passed certain data structures. This changed all direct usage of the JSON gem in internal Rails code to always go through AS::JSON.{decode,encode}. We also shouldn't be implementing `to_json` most of the time, and these occurances are replaced with an equivilent `as_json` implementation to avoid problems down the road. See [1] for all the juicy details. [1]: intridea/multi_json#138 (comment) --- .../journey/gtg/transition_table.rb | 8 ++--- actionpack/test/controller/routing_test.rb | 29 ++++++++++--------- actionpack/test/controller/test_case_test.rb | 5 ++-- actionpack/test/controller/webservice_test.rb | 3 +- .../test/journey/gtg/transition_table_test.rb | 4 +-- actionpack/test/lib/controller/fake_models.rb | 4 +-- .../active_support/core_ext/object/json.rb | 2 +- activesupport/test/json/encoding_test.rb | 8 ++--- 8 files changed, 32 insertions(+), 31 deletions(-) diff --git a/actionpack/lib/action_dispatch/journey/gtg/transition_table.rb b/actionpack/lib/action_dispatch/journey/gtg/transition_table.rb index 971cb3447f..5a79059ed6 100644 --- a/actionpack/lib/action_dispatch/journey/gtg/transition_table.rb +++ b/actionpack/lib/action_dispatch/journey/gtg/transition_table.rb @@ -43,9 +43,7 @@ module ActionDispatch move_string(t, a).concat(move_regexp(t, a)) end - def to_json - require 'json' - + def as_json(options = nil) simple_regexp = Hash.new { |h,k| h[k] = {} } @regexp_states.each do |from, hash| @@ -54,11 +52,11 @@ module ActionDispatch end end - JSON.dump({ + { regexp_states: simple_regexp, string_states: @string_states, accepting: @accepting - }) + } end def to_svg diff --git a/actionpack/test/controller/routing_test.rb b/actionpack/test/controller/routing_test.rb index 46df1a7bd5..2c84e95c6e 100644 --- a/actionpack/test/controller/routing_test.rb +++ b/actionpack/test/controller/routing_test.rb @@ -2,6 +2,7 @@ require 'abstract_unit' require 'controller/fake_controllers' require 'active_support/core_ext/object/with_options' +require 'active_support/core_ext/object/json' class MilestonesController < ActionController::Base def index() head :ok end @@ -86,36 +87,36 @@ class LegacyRouteSetTests < ActiveSupport::TestCase def test_symbols_with_dashes rs.draw do get '/:artist/:song-omg', :to => lambda { |env| - resp = JSON.dump env[ActionDispatch::Routing::RouteSet::PARAMETERS_KEY] + resp = ActiveSupport::JSON.encode env[ActionDispatch::Routing::RouteSet::PARAMETERS_KEY] [200, {}, [resp]] } end - hash = JSON.load get(URI('http://example.org/journey/faithfully-omg')) + hash = ActiveSupport::JSON.decode get(URI('http://example.org/journey/faithfully-omg')) assert_equal({"artist"=>"journey", "song"=>"faithfully"}, hash) end def test_id_with_dash rs.draw do get '/journey/:id', :to => lambda { |env| - resp = JSON.dump env[ActionDispatch::Routing::RouteSet::PARAMETERS_KEY] + resp = ActiveSupport::JSON.encode env[ActionDispatch::Routing::RouteSet::PARAMETERS_KEY] [200, {}, [resp]] } end - hash = JSON.load get(URI('http://example.org/journey/faithfully-omg')) + hash = ActiveSupport::JSON.decode get(URI('http://example.org/journey/faithfully-omg')) assert_equal({"id"=>"faithfully-omg"}, hash) end def test_dash_with_custom_regexp rs.draw do get '/:artist/:song-omg', :constraints => { :song => /\d+/ }, :to => lambda { |env| - resp = JSON.dump env[ActionDispatch::Routing::RouteSet::PARAMETERS_KEY] + resp = ActiveSupport::JSON.encode env[ActionDispatch::Routing::RouteSet::PARAMETERS_KEY] [200, {}, [resp]] } end - hash = JSON.load get(URI('http://example.org/journey/123-omg')) + hash = ActiveSupport::JSON.decode get(URI('http://example.org/journey/123-omg')) assert_equal({"artist"=>"journey", "song"=>"123"}, hash) assert_equal 'Not Found', get(URI('http://example.org/journey/faithfully-omg')) end @@ -123,24 +124,24 @@ class LegacyRouteSetTests < ActiveSupport::TestCase def test_pre_dash rs.draw do get '/:artist/omg-:song', :to => lambda { |env| - resp = JSON.dump env[ActionDispatch::Routing::RouteSet::PARAMETERS_KEY] + resp = ActiveSupport::JSON.encode env[ActionDispatch::Routing::RouteSet::PARAMETERS_KEY] [200, {}, [resp]] } end - hash = JSON.load get(URI('http://example.org/journey/omg-faithfully')) + hash = ActiveSupport::JSON.decode get(URI('http://example.org/journey/omg-faithfully')) assert_equal({"artist"=>"journey", "song"=>"faithfully"}, hash) end def test_pre_dash_with_custom_regexp rs.draw do get '/:artist/omg-:song', :constraints => { :song => /\d+/ }, :to => lambda { |env| - resp = JSON.dump env[ActionDispatch::Routing::RouteSet::PARAMETERS_KEY] + resp = ActiveSupport::JSON.encode env[ActionDispatch::Routing::RouteSet::PARAMETERS_KEY] [200, {}, [resp]] } end - hash = JSON.load get(URI('http://example.org/journey/omg-123')) + hash = ActiveSupport::JSON.decode get(URI('http://example.org/journey/omg-123')) assert_equal({"artist"=>"journey", "song"=>"123"}, hash) assert_equal 'Not Found', get(URI('http://example.org/journey/omg-faithfully')) end @@ -160,14 +161,14 @@ class LegacyRouteSetTests < ActiveSupport::TestCase def test_star_paths_are_greedy_but_not_too_much rs.draw do get "/*path", :to => lambda { |env| - x = JSON.dump env["action_dispatch.request.path_parameters"] + x = ActiveSupport::JSON.encode env["action_dispatch.request.path_parameters"] [200, {}, [x]] } end expected = { "path" => "foo/bar", "format" => "html" } u = URI('http://example.org/foo/bar.html') - assert_equal expected, JSON.parse(get(u)) + assert_equal expected, ActiveSupport::JSON.decode(get(u)) end def test_optional_star_paths_are_greedy @@ -185,7 +186,7 @@ class LegacyRouteSetTests < ActiveSupport::TestCase def test_optional_star_paths_are_greedy_but_not_too_much rs.draw do get "/(*filters)", :to => lambda { |env| - x = JSON.dump env["action_dispatch.request.path_parameters"] + x = ActiveSupport::JSON.encode env["action_dispatch.request.path_parameters"] [200, {}, [x]] } end @@ -193,7 +194,7 @@ class LegacyRouteSetTests < ActiveSupport::TestCase expected = { "filters" => "ne_27.065938,-80.6092/sw_25.489856,-82", "format" => "542794" } u = URI('http://example.org/ne_27.065938,-80.6092/sw_25.489856,-82.542794') - assert_equal expected, JSON.parse(get(u)) + assert_equal expected, ActiveSupport::JSON.decode(get(u)) end def test_regexp_precidence diff --git a/actionpack/test/controller/test_case_test.rb b/actionpack/test/controller/test_case_test.rb index f75c604277..de0476dbde 100644 --- a/actionpack/test/controller/test_case_test.rb +++ b/actionpack/test/controller/test_case_test.rb @@ -1,5 +1,6 @@ require 'abstract_unit' require 'controller/fake_controllers' +require 'active_support/json/decoding' class TestCaseTest < ActionController::TestCase class TestController < ActionController::Base @@ -622,7 +623,7 @@ XML @request.headers['Referer'] = "http://nohost.com/home" @request.headers['Content-Type'] = "application/rss+xml" get :test_headers - parsed_env = JSON.parse(@response.body) + parsed_env = ActiveSupport::JSON.decode(@response.body) assert_equal "http://nohost.com/home", parsed_env["HTTP_REFERER"] assert_equal "application/rss+xml", parsed_env["CONTENT_TYPE"] end @@ -631,7 +632,7 @@ XML @request.headers['HTTP_REFERER'] = "http://example.com/about" @request.headers['CONTENT_TYPE'] = "application/json" get :test_headers - parsed_env = JSON.parse(@response.body) + parsed_env = ActiveSupport::JSON.decode(@response.body) assert_equal "http://example.com/about", parsed_env["HTTP_REFERER"] assert_equal "application/json", parsed_env["CONTENT_TYPE"] end diff --git a/actionpack/test/controller/webservice_test.rb b/actionpack/test/controller/webservice_test.rb index b2dfd96606..d80b0e2da0 100644 --- a/actionpack/test/controller/webservice_test.rb +++ b/actionpack/test/controller/webservice_test.rb @@ -1,4 +1,5 @@ require 'abstract_unit' +require 'active_support/json/decoding' class WebServiceTest < ActionDispatch::IntegrationTest class TestController < ActionController::Base @@ -54,7 +55,7 @@ class WebServiceTest < ActionDispatch::IntegrationTest def test_register_and_use_json_simple with_test_route_set do - with_params_parsers Mime::JSON => Proc.new { |data| JSON.parse(data)['request'].with_indifferent_access } do + with_params_parsers Mime::JSON => Proc.new { |data| ActiveSupport::JSON.decode(data)['request'].with_indifferent_access } do post "/", '{"request":{"summary":"content...","title":"JSON"}}', 'CONTENT_TYPE' => 'application/json' diff --git a/actionpack/test/journey/gtg/transition_table_test.rb b/actionpack/test/journey/gtg/transition_table_test.rb index 33acba8b65..b968780d8d 100644 --- a/actionpack/test/journey/gtg/transition_table_test.rb +++ b/actionpack/test/journey/gtg/transition_table_test.rb @@ -1,5 +1,5 @@ require 'abstract_unit' -require 'json' +require 'active_support/json/decoding' module ActionDispatch module Journey @@ -13,7 +13,7 @@ module ActionDispatch /articles/:id(.:format) } - json = JSON.load table.to_json + json = ActiveSupport::JSON.decode table.to_json assert json['regexp_states'] assert json['string_states'] assert json['accepting'] diff --git a/actionpack/test/lib/controller/fake_models.rb b/actionpack/test/lib/controller/fake_models.rb index 08af187311..b8b51d86c2 100644 --- a/actionpack/test/lib/controller/fake_models.rb +++ b/actionpack/test/lib/controller/fake_models.rb @@ -112,7 +112,7 @@ module Blog end class RenderJsonTestException < Exception - def to_json(options = nil) - return { :error => self.class.name, :message => self.to_s }.to_json + def as_json(options = nil) + { :error => self.class.name, :message => self.to_s } end end diff --git a/activesupport/lib/active_support/core_ext/object/json.rb b/activesupport/lib/active_support/core_ext/object/json.rb index c50cd57c2c..e2a89f5760 100644 --- a/activesupport/lib/active_support/core_ext/object/json.rb +++ b/activesupport/lib/active_support/core_ext/object/json.rb @@ -91,7 +91,7 @@ end class Float # Encoding Infinity or NaN to JSON should return "null". The default returns - # "Infinity" or "NaN" which breaks parsing the JSON. E.g. JSON.parse('[NaN]'). + # "Infinity" or "NaN" which are not valid JSON. def as_json(options = nil) #:nodoc: finite? ? self : nil end diff --git a/activesupport/test/json/encoding_test.rb b/activesupport/test/json/encoding_test.rb index ae8b8052ca..99120f1d2a 100644 --- a/activesupport/test/json/encoding_test.rb +++ b/activesupport/test/json/encoding_test.rb @@ -310,7 +310,7 @@ class TestJSONEncoding < ActiveSupport::TestCase hash = {"foo" => f, "other_hash" => {"foo" => "other_foo", "test" => "other_test"}} assert_equal({"foo"=>{"foo"=>"hello","bar"=>"world"}, - "other_hash" => {"foo"=>"other_foo","test"=>"other_test"}}, JSON.parse(hash.to_json)) + "other_hash" => {"foo"=>"other_foo","test"=>"other_test"}}, ActiveSupport::JSON.decode(hash.to_json)) end def test_struct_encoding @@ -335,13 +335,13 @@ class TestJSONEncoding < ActiveSupport::TestCase assert_equal({"name" => "David", "sub" => { "name" => "David", - "date" => "2010-01-01" }}, JSON.parse(json_custom)) + "date" => "2010-01-01" }}, ActiveSupport::JSON.decode(json_custom)) assert_equal({"name" => "David", "email" => "sample@example.com"}, - JSON.parse(json_strings)) + ActiveSupport::JSON.decode(json_strings)) assert_equal({"name" => "David", "date" => "2010-01-01"}, - JSON.parse(json_string_and_date)) + ActiveSupport::JSON.decode(json_string_and_date)) end def test_opt_out_big_decimal_string_serialization