From 6be6ed8326acc06963ee272364f2deccde35c7a6 Mon Sep 17 00:00:00 2001
From: Jo Liss <joliss42@gmail.com>
Date: Mon, 29 Oct 2012 17:20:29 +0100
Subject: [PATCH 1/4] Extract Associations module into separate file

---
 lib/active_model/serializer.rb              | 172 -------------------
 lib/active_model/serializer/associations.rb | 175 ++++++++++++++++++++
 lib/active_model_serializers.rb             |   1 +
 3 files changed, 176 insertions(+), 172 deletions(-)
 create mode 100644 lib/active_model/serializer/associations.rb

diff --git a/lib/active_model/serializer.rb b/lib/active_model/serializer.rb
index 82ee0481..03a8fa79 100644
--- a/lib/active_model/serializer.rb
+++ b/lib/active_model/serializer.rb
@@ -52,178 +52,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 = {}
 
diff --git a/lib/active_model/serializer/associations.rb b/lib/active_model/serializer/associations.rb
new file mode 100644
index 00000000..64c68b20
--- /dev/null
+++ b/lib/active_model/serializer/associations.rb
@@ -0,0 +1,175 @@
+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
+        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
+  end
+end
diff --git a/lib/active_model_serializers.rb b/lib/active_model_serializers.rb
index 33aa902b..b190e7d7 100644
--- a/lib/active_model_serializers.rb
+++ b/lib/active_model_serializers.rb
@@ -5,6 +5,7 @@ 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)

From 28ee88ca9a09d8afa3b190ddff623657f2098310 Mon Sep 17 00:00:00 2001
From: Jo Liss <joliss42@gmail.com>
Date: Mon, 29 Oct 2012 22:01:56 +0100
Subject: [PATCH 2/4] In the test, use the same :hash across serializers

Otherwise, `include!` will not remember the unique_values of
already-sideloaded hashes across serializer calls.
---
 test/association_test.rb | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/test/association_test.rb b/test/association_test.rb
index d174e196..a87e9936 100644
--- a/test/association_test.rb
+++ b/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)
-
-    @hash = {}
-    @root_hash = {}
+    @post_serializer = @post_serializer_class.new(@post, :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)

From ee3cec3d0c0d4da6bbf92b428c022a9b1058ba91 Mon Sep 17 00:00:00 2001
From: Jo Liss <joliss42@gmail.com>
Date: Mon, 29 Oct 2012 22:06:50 +0100
Subject: [PATCH 3/4] When objects are sideloaded multiple times, serialize
 them only once

To achieve this, we make the following change when sideloading: Instead
of serializing associations and discarding duplicate *hashes*, we
memorize the *objects* (records) that we have already serialized, and
only serialize those that are new.

This change is mostly transparent, and brings down serialization time
from 3.1 seconds to 1.0 seconds on my set of sample data.

There is one change in the behavior: If you sideload the same object
multiple times, and it yields different hashes, like so:

    embed :ids, include: true
    has_many :comments
    has_many :recent_comments, root: comments, serializer: CommentShortSerializer

then previously, it would be included multiple times, whereas now, the
first hash wins. (I haven't actually tested this.) I don't know that
either option is preferable. It's not covered by the test suite, and I
think it's an edge case that is OK to ignore entirely.
---
 lib/active_model/serializer.rb              | 18 +++++++++-------
 lib/active_model/serializer/associations.rb | 11 +++++++---
 test/association_test.rb                    | 23 +++++++++++++++++++++
 test/serializer_test.rb                     |  6 ++++--
 4 files changed, 45 insertions(+), 13 deletions(-)

diff --git a/lib/active_model/serializer.rb b/lib/active_model/serializer.rb
index 03a8fa79..74374455 100644
--- a/lib/active_model/serializer.rb
+++ b/lib/active_model/serializer.rb
@@ -310,7 +310,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
@@ -326,13 +326,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)
-      if current_value = unique_values[key]
-        current_value.merge! value
-        hash[key] = current_value.to_a
-      elsif value
-        hash[key] = value
-        unique_values[key] = OrderedSet.new(value)
+    def merge_association(hash, key, serializables, unique_values)
+      already_serialized = (unique_values[key] ||= {})
+      serializable_hashes = (hash[key] ||= [])
+
+      serializables.each do |serializable|
+        unless already_serialized.include? serializable.object
+          already_serialized[serializable.object] = true
+          serializable_hashes << serializable.serializable_hash
+        end
       end
     end
 
diff --git a/lib/active_model/serializer/associations.rb b/lib/active_model/serializer/associations.rb
index 64c68b20..74afc1ca 100644
--- a/lib/active_model/serializer/associations.rb
+++ b/lib/active_model/serializer/associations.rb
@@ -99,7 +99,12 @@ module ActiveModel
             find_serializable(item).serializable_hash
           end
         end
-        alias serialize_many serialize
+
+        def serializables
+          associated_object.map do |item|
+            find_serializable(item)
+          end
+        end
 
         def serialize_ids
           # Use pluck or select_columns if available
@@ -149,9 +154,9 @@ module ActiveModel
           end
         end
 
-        def serialize_many
+        def serializables
           object = associated_object
-          value = object && find_serializable(object).serializable_hash
+          value = object && find_serializable(object)
           value ? [value] : []
         end
 
diff --git a/test/association_test.rb b/test/association_test.rb
index a87e9936..f766b5ab 100644
--- a/test/association_test.rb
+++ b/test/association_test.rb
@@ -284,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
diff --git a/test/serializer_test.rb b/test/serializer_test.rb
index 941d371c..1d8e3d69 100644
--- a/test/serializer_test.rb
+++ b/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)

From c767d7f5e4f0caa35b411e03dfe9ca284c2a5fbf Mon Sep 17 00:00:00 2001
From: Jo Liss <joliss42@gmail.com>
Date: Mon, 29 Oct 2012 22:50:54 +0100
Subject: [PATCH 4/4] Remove newly-redundant OrderedSet

---
 lib/active_model/ordered_set.rb | 25 -------------------------
 lib/active_model/serializer.rb  |  1 -
 lib/active_model_serializers.rb |  1 -
 3 files changed, 27 deletions(-)
 delete mode 100644 lib/active_model/ordered_set.rb

diff --git a/lib/active_model/ordered_set.rb b/lib/active_model/ordered_set.rb
deleted file mode 100644
index dd76ed41..00000000
--- a/lib/active_model/ordered_set.rb
+++ /dev/null
@@ -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
diff --git a/lib/active_model/serializer.rb b/lib/active_model/serializer.rb
index 74374455..38c0b222 100644
--- a/lib/active_model/serializer.rb
+++ b/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
diff --git a/lib/active_model_serializers.rb b/lib/active_model_serializers.rb
index b190e7d7..55709e0e 100644
--- a/lib/active_model_serializers.rb
+++ b/lib/active_model_serializers.rb
@@ -2,7 +2,6 @@ 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"