Merge pull request #142 from joliss/sideloading-complexity

When objects are sideloaded multiple times, serialize them only once

lib/active_model/ordered_set.rb

@@ -1,25 +0,0 @@
-module ActiveModel
-  class OrderedSet
-    def initialize(array)
-      @array = array
-      @hash = {}
-
-      array.each do |item|
-        @hash[item] = true
-      end
-    end
-
-    def merge!(other)
-      other.each do |item|
-        next if @hash.key?(item)
-
-        @hash[item] = true
-        @array.push item
-      end
-    end
-
-    def to_a
-      @array
-    end
-  end
-end

lib/active_model/serializer.rb

@@ -1,6 +1,5 @@
 require "active_support/core_ext/class/attribute"
 require "active_support/core_ext/module/anonymous"
-require "set"
 
 module ActiveModel
   # Active Model Serializer
@@ -52,178 +51,6 @@ module ActiveModel
       end
     end
 
-    module Associations #:nodoc:
-      class Config #:nodoc:
-        class_attribute :options
-
-        def self.refine(name, class_options)
-          current_class = self
-
-          Class.new(self) do
-            singleton_class.class_eval do
-              define_method(:to_s) do
-                "(subclass of #{current_class.name})"
-              end
-
-              alias inspect to_s
-            end
-
-            self.options = class_options
-          end
-        end
-
-        self.options = {}
-
-        def initialize(name, source, options={})
-          @name = name
-          @source = source
-          @options = options
-        end
-
-        def option(key, default=nil)
-          if @options.key?(key)
-            @options[key]
-          elsif self.class.options.key?(key)
-            self.class.options[key]
-          else
-            default
-          end
-        end
-
-        def target_serializer
-          option(:serializer)
-        end
-
-        def source_serializer
-          @source
-        end
-
-        def key
-          option(:key) || @name
-        end
-
-        def root
-          option(:root) || plural_key
-        end
-
-        def name
-          option(:name) || @name
-        end
-
-        def associated_object
-          option(:value) || source_serializer.send(name)
-        end
-
-        def embed_ids?
-          option(:embed, source_serializer._embed) == :ids
-        end
-
-        def embed_objects?
-          option(:embed, source_serializer._embed) == :objects
-        end
-
-        def embed_in_root?
-          option(:include, source_serializer._root_embed)
-        end
-
-        def embeddable?
-          !associated_object.nil?
-        end
-
-      protected
-
-        def find_serializable(object)
-          if target_serializer
-            target_serializer.new(object, source_serializer.options)
-          elsif object.respond_to?(:active_model_serializer) && (ams = object.active_model_serializer)
-            ams.new(object, source_serializer.options)
-          else
-            object
-          end
-        end
-      end
-
-      class HasMany < Config #:nodoc:
-        alias plural_key key
-
-        def serialize
-          associated_object.map do |item|
-            find_serializable(item).serializable_hash
-          end
-        end
-        alias serialize_many serialize
-
-        def serialize_ids
-          # Use pluck or select_columns if available
-          # return collection.ids if collection.respond_to?(:ids)
-
-          associated_object.map do |item|
-            item.read_attribute_for_serialization(:id)
-          end
-        end
-      end
-
-      class HasOne < Config #:nodoc:
-        def embeddable?
-          if polymorphic? && associated_object.nil?
-            false
-          else
-            true
-          end
-        end
-
-        def polymorphic?
-          option :polymorphic
-        end
-
-        def polymorphic_key
-          associated_object.class.to_s.demodulize.underscore.to_sym
-        end
-
-        def plural_key
-          if polymorphic?
-            associated_object.class.to_s.pluralize.demodulize.underscore.to_sym
-          else
-            key.to_s.pluralize.to_sym
-          end
-        end
-
-        def serialize
-          object = associated_object
-
-          if object && polymorphic?
-            { 
-              :type => polymorphic_key,
-              polymorphic_key => find_serializable(object).serializable_hash
-            }
-          elsif object
-            find_serializable(object).serializable_hash
-          end
-        end
-
-        def serialize_many
-          object = associated_object
-          value = object && find_serializable(object).serializable_hash
-          value ? [value] : []
-        end
-
-        def serialize_ids
-          object = associated_object
-
-          if object && polymorphic?
-            { 
-              :type => polymorphic_key,
-              :id => object.read_attribute_for_serialization(:id)
-            }
-          elsif object
-            object.read_attribute_for_serialization(:id)
-          else
-            nil
-          end
-        end
-      end
-    end
-
     class_attribute :_attributes
     self._attributes = {}
 
@@ -482,7 +309,7 @@ module ActiveModel
         if association.embed_in_root? && hash.nil?
           raise IncludeError.new(self.class, association.name)
         elsif association.embed_in_root? && association.embeddable?
-          merge_association hash, association.root, association.serialize_many, unique_values
+          merge_association hash, association.root, association.serializables, unique_values
         end
       elsif association.embed_objects?
         node[association.key] = association.serialize
@@ -498,13 +325,15 @@ module ActiveModel
     # a unique list of all of the objects that are already in the Array. This
     # avoids the need to scan through the Array looking for entries every time
     # we want to merge a new list of values.
-    def merge_association(hash, key, value, unique_values)
+    def merge_association(hash, key, serializables, unique_values)
-      if current_value = unique_values[key]
+      already_serialized = (unique_values[key] ||= {})
-        current_value.merge! value
+      serializable_hashes = (hash[key] ||= [])
-        hash[key] = current_value.to_a
+
-      elsif value
+      serializables.each do |serializable|
-        hash[key] = value
+        unless already_serialized.include? serializable.object
-        unique_values[key] = OrderedSet.new(value)
+          already_serialized[serializable.object] = true
+          serializable_hashes << serializable.serializable_hash
+        end
       end
     end
 

lib/active_model/serializer/associations.rb

@@ -0,0 +1,180 @@
+module ActiveModel
+  class Serializer
+    module Associations #:nodoc:
+      class Config #:nodoc:
+        class_attribute :options
+
+        def self.refine(name, class_options)
+          current_class = self
+
+          Class.new(self) do
+            singleton_class.class_eval do
+              define_method(:to_s) do
+                "(subclass of #{current_class.name})"
+              end
+
+              alias inspect to_s
+            end
+
+            self.options = class_options
+          end
+        end
+
+        self.options = {}
+
+        def initialize(name, source, options={})
+          @name = name
+          @source = source
+          @options = options
+        end
+
+        def option(key, default=nil)
+          if @options.key?(key)
+            @options[key]
+          elsif self.class.options.key?(key)
+            self.class.options[key]
+          else
+            default
+          end
+        end
+
+        def target_serializer
+          option(:serializer)
+        end
+
+        def source_serializer
+          @source
+        end
+
+        def key
+          option(:key) || @name
+        end
+
+        def root
+          option(:root) || plural_key
+        end
+
+        def name
+          option(:name) || @name
+        end
+
+        def associated_object
+          option(:value) || source_serializer.send(name)
+        end
+
+        def embed_ids?
+          option(:embed, source_serializer._embed) == :ids
+        end
+
+        def embed_objects?
+          option(:embed, source_serializer._embed) == :objects
+        end
+
+        def embed_in_root?
+          option(:include, source_serializer._root_embed)
+        end
+
+        def embeddable?
+          !associated_object.nil?
+        end
+
+      protected
+
+        def find_serializable(object)
+          if target_serializer
+            target_serializer.new(object, source_serializer.options)
+          elsif object.respond_to?(:active_model_serializer) && (ams = object.active_model_serializer)
+            ams.new(object, source_serializer.options)
+          else
+            object
+          end
+        end
+      end
+
+      class HasMany < Config #:nodoc:
+        alias plural_key key
+
+        def serialize
+          associated_object.map do |item|
+            find_serializable(item).serializable_hash
+          end
+        end
+
+        def serializables
+          associated_object.map do |item|
+            find_serializable(item)
+          end
+        end
+
+        def serialize_ids
+          # Use pluck or select_columns if available
+          # return collection.ids if collection.respond_to?(:ids)
+
+          associated_object.map do |item|
+            item.read_attribute_for_serialization(:id)
+          end
+        end
+      end
+
+      class HasOne < Config #:nodoc:
+        def embeddable?
+          if polymorphic? && associated_object.nil?
+            false
+          else
+            true
+          end
+        end
+
+        def polymorphic?
+          option :polymorphic
+        end
+
+        def polymorphic_key
+          associated_object.class.to_s.demodulize.underscore.to_sym
+        end
+
+        def plural_key
+          if polymorphic?
+            associated_object.class.to_s.pluralize.demodulize.underscore.to_sym
+          else
+            key.to_s.pluralize.to_sym
+          end
+        end
+
+        def serialize
+          object = associated_object
+
+          if object && polymorphic?
+            {
+              :type => polymorphic_key,
+              polymorphic_key => find_serializable(object).serializable_hash
+            }
+          elsif object
+            find_serializable(object).serializable_hash
+          end
+        end
+
+        def serializables
+          object = associated_object
+          value = object && find_serializable(object)
+          value ? [value] : []
+        end
+
+        def serialize_ids
+          object = associated_object
+
+          if object && polymorphic?
+            {
+              :type => polymorphic_key,
+              :id => object.read_attribute_for_serialization(:id)
+            }
+          elsif object
+            object.read_attribute_for_serialization(:id)
+          else
+            nil
+          end
+        end
+      end
+    end
+  end
+end

lib/active_model_serializers.rb

@@ -2,9 +2,9 @@ require "active_support"
 require "active_support/core_ext/string/inflections"
 require "active_support/notifications"
 require "active_model"
-require "active_model/ordered_set"
 require "active_model/array_serializer"
 require "active_model/serializer"
+require "active_model/serializer/associations"
 require "set"
 
 if defined?(Rails)

test/association_test.rb

@@ -30,6 +30,9 @@ class AssociationTest < ActiveModel::TestCase
   end
 
   def setup
+    @hash = {}
+    @root_hash = {}
+
     @post = Model.new(:title => "New Post", :body => "Body")
     @comment = Model.new(:id => 1, :body => "ZOMG A COMMENT")
     @post.comments = [ @comment ]
@@ -43,17 +46,13 @@ class AssociationTest < ActiveModel::TestCase
       attributes :title, :body
     end
 
-    @post_serializer = @post_serializer_class.new(@post)
+    @post_serializer = @post_serializer_class.new(@post, :hash => @root_hash)
-
-    @hash = {}
-    @root_hash = {}
   end
 
   def include!(key, options={})
     @post_serializer.include! key, {
       :embed => :ids,
       :include => true,
-      :hash => @root_hash,
       :node => @hash,
       :serializer => @comment_serializer_class
     }.merge(options)
@@ -61,7 +60,6 @@ class AssociationTest < ActiveModel::TestCase
 
   def include_bare!(key, options={})
     @post_serializer.include! key, {
-      :hash => @root_hash,
       :node => @hash,
       :serializer => @comment_serializer_class
     }.merge(options)
@@ -286,6 +284,29 @@ class AssociationTest < ActiveModel::TestCase
         ]
       }, @root_hash)
     end
+
+    def test_embed_ids_include_true_does_not_serialize_multiple_times
+      @post.recent_comment = @comment
+
+      @post_serializer_class.class_eval do
+        has_one :comment, :embed => :ids, :include => true
+        has_one :recent_comment, :embed => :ids, :include => true, :root => :comments
+      end
+
+      # Count how often the @comment record is serialized.
+      serialized_times = 0
+      @comment.class_eval do
+        define_method :read_attribute_for_serialization, lambda { |name|
+          serialized_times += 1 if name == :body
+          super(name)
+        }
+      end
+
+      include_bare! :comment
+      include_bare! :recent_comment
+
+      assert_equal 1, serialized_times
+    end
   end
 
   class InclusionTest < AssociationTest

test/serializer_test.rb

@@ -73,11 +73,13 @@ class SerializerTest < ActiveModel::TestCase
 
   class CommentSerializer
     def initialize(comment, options={})
-      @comment = comment
+      @object = comment
     end
 
+    attr_reader :object
+
     def serializable_hash
-      { :title => @comment.read_attribute_for_serialization(:title) }
+      { :title => @object.read_attribute_for_serialization(:title) }
     end
 
     def as_json(options=nil)