fixed some issues with JSON encoding

- as_json in ActiveModel should return a hash
  and handle :only/:except/:methods options
- Array and Hash should call as_json on their elements
- json methods should not modify options argument

[#5374 state:committed]

Signed-off-by: Jeremy Kemper <jeremy@bitsweat.net>

2 files changed, 108 insertions, 7 deletions

diff --git a/activesupport/lib/active_support/json/encoding.rb b/activesupport/lib/active_support/json/encoding.rb
index 2f9588e0f4..6e9d62bd16 100644
--- a/activesupport/lib/active_support/json/encoding.rb
+++ b/activesupport/lib/active_support/json/encoding.rb
@@ -41,9 +41,26 @@ module ActiveSupport
           @seen = []
         end
 
-        def encode(value)
+        def encode(value, use_options = true)
           check_for_circular_references(value) do
-            value.as_json(options).encode_json(self)
+            jsonified = use_options ? value.as_json(options_for(value)) : value.as_json
+            jsonified.encode_json(self)
+          end
+        end
+
+        # like encode, but only calls as_json, without encoding to string
+        def as_json(value)
+          check_for_circular_references(value) do
+            value.as_json(options_for(value))
+          end
+        end
+
+        def options_for(value)
+          if value.is_a?(Array) || value.is_a?(Hash)
+            # hashes and arrays need to get encoder in the options, so that they can detect circular references
+            (options || {}).merge(:encoder => self)
+          else
+            options
           end
         end
 
@@ -186,13 +203,22 @@ module Enumerable
 end
 
 class Array
-  def as_json(options = nil) self end #:nodoc:
-  def encode_json(encoder) "[#{map { |v| encoder.encode(v) } * ','}]" end #:nodoc:
+  def as_json(options = nil) #:nodoc:
+    # use encoder as a proxy to call as_json on all elements, to protect from circular references
+    encoder = options && options[:encoder] || ActiveSupport::JSON::Encoding::Encoder.new(options)
+    map { |v| encoder.as_json(v) }
+  end
+
+  def encode_json(encoder) #:nodoc:
+    # we assume here that the encoder has already run as_json on self and the elements, so we run encode_json directly
+    "[#{map { |v| v.encode_json(encoder) } * ','}]"
+  end
 end
 
 class Hash
   def as_json(options = nil) #:nodoc:
-    if options
+    # create a subset of the hash by applying :only or :except
+    subset = if options
       if attrs = options[:only]
         slice(*Array.wrap(attrs))
       elsif attrs = options[:except]
@@ -203,10 +229,22 @@ class Hash
     else
       self
     end
+
+    # use encoder as a proxy to call as_json on all values in the subset, to protect from circular references
+    encoder = options && options[:encoder] || ActiveSupport::JSON::Encoding::Encoder.new(options)
+    pairs = subset.map { |k, v| [k.to_s, encoder.as_json(v)] }
+    result = self.is_a?(ActiveSupport::OrderedHash) ? ActiveSupport::OrderedHash.new : Hash.new
+    pairs.inject(result) { |hash, pair| hash[pair.first] = pair.last; hash }
   end
 
   def encode_json(encoder)
-    "{#{map { |k,v| "#{encoder.encode(k.to_s)}:#{encoder.encode(v)}" } * ','}}"
+    # values are encoded with use_options = false, because we don't want hash representations from ActiveModel to be
+    # processed once again with as_json with options, as this could cause unexpected results (i.e. missing fields);
+
+    # on the other hand, we need to run as_json on the elements, because the model representation may contain fields
+    # like Time/Date in their original (not jsonified) form, etc.
+
+    "{#{map { |k,v| "#{encoder.encode(k.to_s)}:#{encoder.encode(v, false)}" } * ','}}"
   end
 end
 
diff --git a/activesupport/test/json/encoding_test.rb b/activesupport/test/json/encoding_test.rb
index 1527d02d16..e0494de6e4 100644
--- a/activesupport/test/json/encoding_test.rb
+++ b/activesupport/test/json/encoding_test.rb
@@ -108,12 +108,24 @@ class TestJSONEncoding < Test::Unit::TestCase
     end
   end
 
-  def test_exception_raised_when_encoding_circular_reference
+  def test_exception_raised_when_encoding_circular_reference_in_array
     a = [1]
     a << a
     assert_raise(ActiveSupport::JSON::Encoding::CircularReferenceError) { ActiveSupport::JSON.encode(a) }
   end
 
+  def test_exception_raised_when_encoding_circular_reference_in_hash
+    a = { :name => 'foo' }
+    a[:next] = a
+    assert_raise(ActiveSupport::JSON::Encoding::CircularReferenceError) { ActiveSupport::JSON.encode(a) }
+  end
+
+  def test_exception_raised_when_encoding_circular_reference_in_hash_inside_array
+    a = { :name => 'foo', :sub => [] }
+    a[:sub] << a
+    assert_raise(ActiveSupport::JSON::Encoding::CircularReferenceError) { ActiveSupport::JSON.encode(a) }
+  end
+
   def test_hash_key_identifiers_are_always_quoted
     values = {0 => 0, 1 => 1, :_ => :_, "$" => "$", "a" => "a", :A => :A, :A0 => :A0, "A0B" => "A0B"}
     assert_equal %w( "$" "A" "A0" "A0B" "_" "a" "0" "1" ).sort, object_keys(ActiveSupport::JSON.encode(values))
@@ -152,6 +164,57 @@ class TestJSONEncoding < Test::Unit::TestCase
     end
   end
 
+  def test_hash_should_pass_encoding_options_to_children_in_as_json
+    person = {
+      :name => 'John',
+      :address => {
+        :city => 'London',
+        :country => 'UK'
+      }
+    }
+    json = person.as_json :only => [:address, :city]
+
+    assert_equal({ 'address' => { 'city' => 'London' }}, json)
+  end
+
+  def test_hash_should_pass_encoding_options_to_children_in_to_json
+    person = {
+      :name => 'John',
+      :address => {
+        :city => 'London',
+        :country => 'UK'
+      }
+    }
+    json = person.to_json :only => [:address, :city]
+
+    assert_equal(%({"address":{"city":"London"}}), json)
+  end
+
+  def test_array_should_pass_encoding_options_to_children_in_as_json
+    people = [
+      { :name => 'John', :address => { :city => 'London', :country => 'UK' }},
+      { :name => 'Jean', :address => { :city => 'Paris' , :country => 'France' }}
+    ]
+    json = people.as_json :only => [:address, :city]
+    expected = [
+      { 'address' => { 'city' => 'London' }},
+      { 'address' => { 'city' => 'Paris' }}
+    ]
+
+    assert_equal(expected, json)
+  end
+
+  def test_array_should_pass_encoding_options_to_children_in_to_json
+    people = [
+      { :name => 'John', :address => { :city => 'London', :country => 'UK' }},
+      { :name => 'Jean', :address => { :city => 'Paris' , :country => 'France' }}
+    ]
+    json = people.to_json :only => [:address, :city]
+
+    assert_equal(%([{"address":{"city":"London"}},{"address":{"city":"Paris"}}]), json)
+  end
+
+
   protected
 
     def object_keys(json_object)