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>

activemodel/lib/active_model/serialization.rb

@@ -54,27 +54,29 @@ module ActiveModel
   #
   #   person = Person.new
   #   person.serializable_hash   # => {"name"=>nil}
-  #   person.as_json             # => "{\"name\":null}"
+  #   person.as_json             # => {"name"=>nil}
+  #   person.to_json             # => "{\"name\":null}"
   #   person.to_xml              # => "<?xml version=\"1.0\" encoding=\"UTF-8\"?>\n<serial-person...
   #
   #   person.name = "Bob"
   #   person.serializable_hash   # => {"name"=>"Bob"}
-  #   person.as_json             # => "{\"name\":\"Bob\"}"
+  #   person.as_json             # => {"name"=>"Bob"}
+  #   person.to_json             # => "{\"name\":\"Bob\"}"
   #   person.to_xml              # => "<?xml version=\"1.0\" encoding=\"UTF-8\"?>\n<serial-person...
   #
   # Valid options are <tt>:only</tt>, <tt>:except</tt> and <tt>:methods</tt> .
   module Serialization
     def serializable_hash(options = nil)
       options ||= {}
 
-      options[:only]   = Array.wrap(options[:only]).map { |n| n.to_s }
-      options[:except] = Array.wrap(options[:except]).map { |n| n.to_s }
+      only   = Array.wrap(options[:only]).map(&:to_s)
+      except = Array.wrap(options[:except]).map(&:to_s)
 
       attribute_names = attributes.keys.sort
-      if options[:only].any?
-        attribute_names &= options[:only]
-      elsif options[:except].any?
-        attribute_names -= options[:except]
+      if only.any?
+        attribute_names &= only
+      elsif except.any?
+        attribute_names -= except
       end
 
       method_names = Array.wrap(options[:methods]).inject([]) do |methods, name|

activemodel/lib/active_model/serializers/json.rb

@@ -79,18 +79,16 @@ module JSON
       #                    "title": "Welcome to the weblog"},
       #                   {"comments": [{"body": "Don't think too hard"}],
       #                    "title": "So I was thinking"}]}
-      def encode_json(encoder)
-        hash = serializable_hash(encoder.options)
+
+      def as_json(options = nil)
+        hash = serializable_hash(options)
+
         if include_root_in_json
-          custom_root = encoder.options && encoder.options[:root]
+          custom_root = options && options[:root]
           hash = { custom_root || self.class.model_name.element => hash }
         end
 
-        ActiveSupport::JSON.encode(hash)
-      end
-
-      def as_json(options = nil)
-        self
+        hash
       end
 
       def from_json(json)

activemodel/test/cases/serializeration/json_serialization_test.rb

@@ -116,5 +116,32 @@ def @contact.favorite_quote; "Constraints are liberating"; end
     assert_equal hash.to_json, car.errors.to_json
   end
 
+  test "serializable_hash should not modify options passed in argument" do
+    options = { :except => :name }
+    @contact.serializable_hash(options)
+
+    assert_nil options[:only]
+    assert_equal :name, options[:except]
+  end
+
+  test "as_json should return a hash" do
+    json = @contact.as_json
+
+    assert_kind_of Hash, json
+    assert_kind_of Hash, json['contact']
+    %w(name age created_at awesome preferences).each do |field|
+      assert_equal @contact.send(field), json['contact'][field]
+    end
+  end
+
+  test "custom as_json should be honored when generating json" do
+    def @contact.as_json(options); { :name => name, :created_at => created_at }; end
+    json = @contact.to_json
+
+    assert_match %r{"name":"Konata Izumi"}, json
+    assert_match %r{"created_at":#{ActiveSupport::JSON.encode(Time.utc(2006, 8, 1))}}, json
+    assert_no_match %r{"awesome":}, json
+    assert_no_match %r{"preferences":}, json
+  end
 
 end

activerecord/lib/active_record/relation.rb

@@ -76,7 +76,9 @@ def to_a
       @records
     end
 
-    def as_json(options = nil) to_a end #:nodoc:
+    def as_json(options = nil) #:nodoc:
+      to_a.as_json(options)
+    end
 
     # Returns size of the records.
     def size

activerecord/lib/active_record/serialization.rb

@@ -5,7 +5,7 @@ module Serialization
     include ActiveModel::Serializers::JSON
 
     def serializable_hash(options = nil)
-      options ||= {}
+      options = options.try(:clone) || {}
 
       options[:except] = Array.wrap(options[:except]).map { |n| n.to_s }
       options[:except] |= Array.wrap(self.class.inheritance_column)

activerecord/test/cases/json_serialization_test.rb

@@ -82,6 +82,13 @@ def @contact.favorite_quote; "Constraints are liberating"; end
     assert_match %r{"label":"Has cheezburger"}, methods_json
     assert_match %r{"favorite_quote":"Constraints are liberating"}, methods_json
   end
+
+  def test_serializable_hash_should_not_modify_options_in_argument
+    options = { :only => :name }
+    @contact.serializable_hash(options)
+
+    assert_nil options[:except]
+  end
 end
 
 class DatabaseConnectedJsonEncodingTest < ActiveRecord::TestCase

activesupport/lib/active_support/json/encoding.rb

@@ -41,9 +41,26 @@ def initialize(options = nil)
           @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 @@ def as_json(options = nil) to_a end #:nodoc:
 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 @@ def as_json(options = nil) #:nodoc:
     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
 

activesupport/test/json/encoding_test.rb

@@ -108,12 +108,24 @@ def test_non_utf8_string_transcodes
     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 @@ def test_nested_hash_with_float
     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)