Issue with JSON serialization #7145

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
6 participants

Suppose we have simple model with custom as_json:

class TestModel < ActiveRecord::Base

  def some_method
    'test output'
  end

  def as_json(options={})
    options[:methods] ||= []
    options[:methods].push(:some_method)
    super options
  end
end

And we want to serialize array of 10 records:

([TestModel.new] * 10).as_json

Expected behaviour (for me) is that some_method will be called just 10 times. But I get 55 instead 😄. This is because of modification of options variable, which passed in super

I've tested it with rails 3.2, 3.0.9 and latest master. With ActiveRecord and MongoMapper.

activesupport/lib/active_support/json/encoding.rb
@@ -62,7 +62,7 @@ def options_for(value)
# hashes and arrays need to get encoder in the options, so that they can detect circular references
options.merge(:encoder => self)
else
- options
+ options.try(:clone) || {}
@dmathieu

dmathieu Jul 24, 2012

Contributor

options is defined in the initializer. It should never be nil at this point.
So it's not necessary to use try, nor || here.

activesupport/lib/active_support/json/encoding.rb
@@ -62,7 +62,7 @@ def options_for(value)
# hashes and arrays need to get encoder in the options, so that they can detect circular references
options.merge(:encoder => self)
else
- options
+ options.clone
@@ -85,6 +85,20 @@ def sorted_json(json)
end
end
+ class TestClass
+
@carlosantoniodasilva

carlosantoniodasilva Jul 25, 2012

Owner

Please remove whitespace ✂️

activesupport/test/json/encoding_test.rb
+
+ def test_as_json_doesnt_modify_options_var
+ options = {}
+ ([TestClass.new] * 10).as_json(options)
@carlosantoniodasilva

carlosantoniodasilva Jul 25, 2012

Owner

I think 2 should prove the point, not?

@maxprokopiev

maxprokopiev Jul 25, 2012

yes, 2 is quite enough - fixed

Contributor

jfirebaugh commented Jul 27, 2012

IMHO this is a bug in TestModel#as_json, not Rails. Instead of modifying the passed options, it should be defined like this:

  def as_json(options={})
    super options.merge(:methods => (options[:methods] || []).dup << :some_method)
  end
Member

steveklabnik commented Nov 17, 2012

This needs to be rebased, @Juggler .

This has been fixed by #8185, closing here, thanks!

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