`#as_json` isolates options when encoding a hash. Closes #8182 #8185

Merged
merged 1 commit into from Nov 13, 2012

Conversation

Projects
None yet
4 participants
@senny
Member

senny commented Nov 12, 2012

I modified the Encoder so that duplicates of the original options hash are passed around. I'm not sure if there are cases where we actually wan't the side effects but all the tests passed.

This is a fix #8182

@senny

This comment has been minimized.

Show comment Hide comment
@senny

senny Nov 12, 2012

Member

@rafaelfranca @carlosantoniodasilva can you take a look?

Member

senny commented Nov 12, 2012

@rafaelfranca @carlosantoniodasilva can you take a look?

@carlosantoniodasilva

View changes

activesupport/lib/active_support/json/encoding.rb
@@ -66,7 +66,7 @@ def options_for(value)
options.merge(:encoder => self)
else
options
- end
+ end.dup

This comment has been minimized.

Show comment Hide comment
@carlosantoniodasilva

carlosantoniodasilva Nov 12, 2012

Member

I think you can dup only in the else clause, since options.merge already creates a "duped" object.

@carlosantoniodasilva

carlosantoniodasilva Nov 12, 2012

Member

I think you can dup only in the else clause, since options.merge already creates a "duped" object.

This comment has been minimized.

Show comment Hide comment
@senny

senny Nov 12, 2012

Member

good catch. updated.

@senny

senny Nov 12, 2012

Member

good catch. updated.

@carlosantoniodasilva

This comment has been minimized.

Show comment Hide comment
@carlosantoniodasilva

carlosantoniodasilva Nov 12, 2012

Member

Yeah, in this case I don't think we want the options to cause this side effect, since options within the object are affecting the other external hash. Thanks!

Yeah, in this case I don't think we want the options to cause this side effect, since options within the object are affecting the other external hash. Thanks!

@carlosantoniodasilva

This comment has been minimized.

Show comment Hide comment
@carlosantoniodasilva

carlosantoniodasilva Nov 12, 2012

Member

Looks good, I'll have to ask you to expand the commit message with a description of the problem, and maybe some example code, otherwise @drogus will kill us 😄

Looks good, I'll have to ask you to expand the commit message with a description of the problem, and maybe some example code, otherwise @drogus will kill us 😄

@senny

This comment has been minimized.

Show comment Hide comment
@senny

senny Nov 12, 2012

Member

I added a more detailed description. I did not add an example though. Example code would look exactly like the test case and I think the diff is very short to understand the situation.

@carlosantoniodasilva is that ok?

Member

senny commented Nov 12, 2012

I added a more detailed description. I did not add an example though. Example code would look exactly like the test case and I think the diff is very short to understand the situation.

@carlosantoniodasilva is that ok?

@carlosantoniodasilva

This comment has been minimized.

Show comment Hide comment
@carlosantoniodasilva

carlosantoniodasilva Nov 12, 2012

Member

I think so, looks fine, just found a typo in the commit message: in the has. :) Thanks

I think so, looks fine, just found a typo in the commit message: in the has. :) Thanks

`#as_json` isolates options when encoding a hash. Closes #8182
Setting options in a custom `#as_json` method had side effects.
Modifications of the `options` hash leaked outside and influenced
the conversion of other objects contained in the hash.

carlosantoniodasilva added a commit that referenced this pull request Nov 13, 2012

Merge pull request #8185 from senny/8182_as_json_options_stick_around
`#as_json` isolates options when encoding a hash. Closes #8182

@carlosantoniodasilva carlosantoniodasilva merged commit d5c4370 into rails:master Nov 13, 2012

@carlosantoniodasilva

This comment has been minimized.

Show comment Hide comment
@carlosantoniodasilva

carlosantoniodasilva Nov 13, 2012

Member

Thanks!

Thanks!

senny added a commit to senny/rails that referenced this pull request Nov 13, 2012

backport #8185, `#as_json` isolates options when encoding a hash.
Setting options in a custom `#as_json` method had side effects.
Modifications of the `options` hash leaked outside and influenced
the conversion of other objects contained in the hash.

Conflicts:

	activesupport/CHANGELOG.md

carlosantoniodasilva added a commit that referenced this pull request Nov 13, 2012

Merge pull request #8199 from senny/backport_8182
backport #8185, `#as_json` isolates options when encoding a hash.
@slbug

This comment has been minimized.

Show comment Hide comment
@slbug

slbug Dec 27, 2012

Contributor

@senny, test fails on rails master. But fails only sometimes. I've run tests about 10 times and got 3 failures. Using ruby 1.9.3p327 (resulting json is the same, just options order messed up)

test_to_json_should_not_keep_options_around(TestJSONEncoding) [rails/activesupport/test/json/encoding_test.rb:279]:
--- expected
+++ actual
@@ -1 +1 @@
-"{\"foo\":{\"foo\":\"hello\",\"bar\":\"world\"},\"other_hash\":{\"foo\":\"other_foo\",\"test\":\"other_test\"}}"
+"{\"foo\":{\"bar\":\"world\",\"foo\":\"hello\"},\"other_hash\":{\"foo\":\"other_foo\",\"test\":\"other_test\"}}"

bundle list

  * actionpack (4.0.0.beta)
  * activemodel (4.0.0.beta)
  * activerecord (4.0.0.beta)
  * activerecord-deprecated_finders (0.0.1 2125c7b)
  * activesupport (4.0.0.beta)
  * arel (3.0.2.20120819075748 38d0a22)
  * atomic (1.0.1)
  * bcrypt-ruby (3.0.1)
  * benchmark-ips (1.2.0)
  * builder (3.1.4)
  * bundler (1.2.2)
  * coffee-rails (4.0.0.beta 052634e)
  * coffee-script (2.2.0)
  * coffee-script-source (1.4.0)
  * columnize (0.3.6)
  * dalli (2.6.0)
  * debugger (1.2.2)
  * debugger-linecache (1.1.2)
  * debugger-ruby_core_source (1.1.5)
  * erubis (2.7.0)
  * execjs (1.4.0)
  * hike (1.2.1)
  * i18n (0.6.1)
  * jquery-rails (2.1.4 cf47e71)
  * json (1.7.5)
  * kindlerb (0.1.1)
  * mail (2.5.3)
  * metaclass (0.0.1)
  * mime-types (1.19)
  * minitest (4.3.3)
  * mocha (0.13.1)
  * multi_json (1.5.0)
  * mustache (0.99.4)
  * mysql (2.9.0)
  * mysql2 (0.3.11)
  * nokogiri (1.5.6)
  * pg (0.14.1)
  * polyglot (0.3.3)
  * racc (1.4.9)
  * rack (1.4.1)
  * rack-cache (1.2)
  * rack-test (0.6.2 1b1e730)
  * rails (4.0.0.beta 99bcb42)
  * railties (4.0.0.beta)
  * rake (10.0.3)
  * rdoc (3.12)
  * redcarpet (2.2.2)
  * ruby-prof (0.11.2)
  * sdoc (0.3.20 87ad100)
  * sprockets (2.8.2)
  * sprockets-rails (2.0.0.rc1 0228520)
  * sqlite3 (1.3.6)
  * thor (0.16.0)
  * thread_safe (0.1.0)
  * tilt (1.3.3)
  * treetop (1.4.12)
  * turbolinks (0.6.1)
  * tzinfo (0.3.35)
  * uglifier (1.3.0)
  * w3c_validators (1.2)
  * yajl-ruby (1.1.0)
Contributor

slbug commented Dec 27, 2012

@senny, test fails on rails master. But fails only sometimes. I've run tests about 10 times and got 3 failures. Using ruby 1.9.3p327 (resulting json is the same, just options order messed up)

test_to_json_should_not_keep_options_around(TestJSONEncoding) [rails/activesupport/test/json/encoding_test.rb:279]:
--- expected
+++ actual
@@ -1 +1 @@
-"{\"foo\":{\"foo\":\"hello\",\"bar\":\"world\"},\"other_hash\":{\"foo\":\"other_foo\",\"test\":\"other_test\"}}"
+"{\"foo\":{\"bar\":\"world\",\"foo\":\"hello\"},\"other_hash\":{\"foo\":\"other_foo\",\"test\":\"other_test\"}}"

bundle list

  * actionpack (4.0.0.beta)
  * activemodel (4.0.0.beta)
  * activerecord (4.0.0.beta)
  * activerecord-deprecated_finders (0.0.1 2125c7b)
  * activesupport (4.0.0.beta)
  * arel (3.0.2.20120819075748 38d0a22)
  * atomic (1.0.1)
  * bcrypt-ruby (3.0.1)
  * benchmark-ips (1.2.0)
  * builder (3.1.4)
  * bundler (1.2.2)
  * coffee-rails (4.0.0.beta 052634e)
  * coffee-script (2.2.0)
  * coffee-script-source (1.4.0)
  * columnize (0.3.6)
  * dalli (2.6.0)
  * debugger (1.2.2)
  * debugger-linecache (1.1.2)
  * debugger-ruby_core_source (1.1.5)
  * erubis (2.7.0)
  * execjs (1.4.0)
  * hike (1.2.1)
  * i18n (0.6.1)
  * jquery-rails (2.1.4 cf47e71)
  * json (1.7.5)
  * kindlerb (0.1.1)
  * mail (2.5.3)
  * metaclass (0.0.1)
  * mime-types (1.19)
  * minitest (4.3.3)
  * mocha (0.13.1)
  * multi_json (1.5.0)
  * mustache (0.99.4)
  * mysql (2.9.0)
  * mysql2 (0.3.11)
  * nokogiri (1.5.6)
  * pg (0.14.1)
  * polyglot (0.3.3)
  * racc (1.4.9)
  * rack (1.4.1)
  * rack-cache (1.2)
  * rack-test (0.6.2 1b1e730)
  * rails (4.0.0.beta 99bcb42)
  * railties (4.0.0.beta)
  * rake (10.0.3)
  * rdoc (3.12)
  * redcarpet (2.2.2)
  * ruby-prof (0.11.2)
  * sdoc (0.3.20 87ad100)
  * sprockets (2.8.2)
  * sprockets-rails (2.0.0.rc1 0228520)
  * sqlite3 (1.3.6)
  * thor (0.16.0)
  * thread_safe (0.1.0)
  * tilt (1.3.3)
  * treetop (1.4.12)
  * turbolinks (0.6.1)
  * tzinfo (0.3.35)
  * uglifier (1.3.0)
  * w3c_validators (1.2)
  * yajl-ruby (1.1.0)
@senny

This comment has been minimized.

Show comment Hide comment
@senny

senny Dec 27, 2012

Member

@slbug It seems something weird is going on with the ordering of the hash. I'll take a look and rewrite the tests if necessary.

Member

senny commented Dec 27, 2012

@slbug It seems something weird is going on with the ordering of the hash. I'll take a look and rewrite the tests if necessary.

@jeremy

This comment has been minimized.

Show comment Hide comment
@jeremy

jeremy Dec 27, 2012

Member

Because Object#as_json uses #instance_values which uses #instance_variables which isn't guaranteed to be in sorted order

Member

jeremy commented Dec 27, 2012

Because Object#as_json uses #instance_values which uses #instance_variables which isn't guaranteed to be in sorted order

senny added a commit to senny/rails that referenced this pull request Dec 27, 2012

rewrite order dependent test case. #8185
As reported (rails#8185 (comment))
this test relied on the order a hash was serialized. Comparing the parsed
hash makes the test no longer order dependent.
@senny

This comment has been minimized.

Show comment Hide comment
@senny

senny Dec 27, 2012

Member

@slbug I rewrote the testcase so that it's no longer order dependent. You can rebase against rails/master and your tests should be fine now.

Member

senny commented Dec 27, 2012

@slbug I rewrote the testcase so that it's no longer order dependent. You can rebase against rails/master and your tests should be fine now.

@carlosantoniodasilva

This comment has been minimized.

Show comment Hide comment
@carlosantoniodasilva

carlosantoniodasilva Dec 28, 2012

Member

Just as a side note, these tests are only failing when applying falcon patch (at least to me). With 1.9.3-p327, everything is green.

Just as a side note, these tests are only failing when applying falcon patch (at least to me). With 1.9.3-p327, everything is green.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment