Skip redundant queries when include_data is disabled

Bajena · Jan 15, 2020 · 4333617 · 4333617
1 parent 0d42078
commit 4333617
Show file tree

Hide file tree

Showing 6 changed files with 219 additions and 7 deletions.
diff --git a/README.md b/README.md
@@ -64,7 +64,7 @@ class UserSerializer < BaseSerializer
   # The previous one is a shorthand for the following lines:
   lazy_relationship :blog_posts, loader: AmsLazyRelationships::Loaders::Association.new("User", :blog_posts)
   has_many :blog_posts, serializer: BlogPostSerializer do |serializer|
-    serializer.lazy_blog_posts
+    -> { serializer.lazy_blog_posts }
   end
 
   lazy_has_one :poro_model, loader: AmsLazyRelationships::Loaders::Direct.new(:poro_model) { |object| PoroModel.new(object) }
@@ -119,7 +119,32 @@ Sometimes it may happen that you need to process the relationship before renderi
 ```ruby
 class BlogPostSerializer < BaseSerializer
   lazy_has_many :comments do |serializer|
-    serializer.lazy_comments.map(&:decorate)
+    -> { serializer.lazy_comments.map(&:decorate) }
+   end
+end
+```
+
+Keep attention that custom finder should be returned in a form of lambda,
+in order to avoid redundant SQL queries when `include_data` AMS setting appears to be `false`:
+
+```ruby
+class BlogPostSerializer < BaseSerializer
+  lazy_has_many :comments do |serializer|
+    include_data :if_sideloaded
+    -> { serializer.lazy_comments.map(&:decorate) }
+   end
+end
+```
+
+Feel free to skip custom lazy finder for association if your goal is just to
+define `include_data` setting nor some links/metas:
+
+```ruby
+class BlogPostSerializer < BaseSerializer
+  lazy_has_many :comments do
+    include_data :if_sideloaded
+    link :self, 'a link'
+    meta name: 'Dan Brown'
    end
 end
 ```

diff --git a/lib/ams_lazy_relationships.rb b/lib/ams_lazy_relationships.rb
@@ -4,6 +4,7 @@
 require "active_model_serializers"
 
 require "ams_lazy_relationships/version"
+require "ams_lazy_relationships/extensions"
 require "ams_lazy_relationships/loaders"
 require "ams_lazy_relationships/core"
 

diff --git a/lib/ams_lazy_relationships/core/relationship_wrapper_methods.rb b/lib/ams_lazy_relationships/core/relationship_wrapper_methods.rb
@@ -19,7 +19,7 @@ def define_relationship_wrapper_methods
         define_singleton_method(
           "lazy_#{relationship_type}"
         ) do |relationship_name, options = {}, &block|
-          send(:define_lazy_association, relationship_type, relationship_name, options, block)
+          define_lazy_association(relationship_type, relationship_name, options, block)
         end
       end
     end
@@ -29,11 +29,20 @@ def define_lazy_association(type, name, options, block)
 
       real_relationship_options = options.except(*lazy_relationship_option_keys)
 
-      block ||= lambda do |serializer|
-        serializer.public_send("lazy_#{name}")
-      end
+      public_send(type, name.to_sym, real_relationship_options) do |serializer|
+        block_value = instance_exec(serializer, &block) if block
 
-      public_send(type, name.to_sym, real_relationship_options, &block)
+        if block && block_value != :nil
+          # respect the custom finder for lazy association
+          # @see https://github.com/rails-api/active_model_serializers/blob/v0.10.10/lib/active_model/serializer/reflection.rb#L165-L168
+          block_value
+        else
+          # provide default lazy association finder in a form of lambda,
+          # in order to play nice with possible `include_data` setting.
+          # @see lib/ams_lazy_relationships/extensions/reflection.rb
+          serializer.method("lazy_#{name}")
+        end
+      end
 
       lazy_relationship(name, options.slice(*lazy_relationship_option_keys))
     end

diff --git a/lib/ams_lazy_relationships/extensions.rb b/lib/ams_lazy_relationships/extensions.rb
@@ -0,0 +1,8 @@
+# frozen_string_literal: true
+
+require "ams_lazy_relationships/extensions/reflection"
+
+module AmsLazyRelationships
+  module Extensions
+  end
+end
diff --git a/lib/ams_lazy_relationships/extensions/reflection.rb b/lib/ams_lazy_relationships/extensions/reflection.rb
@@ -0,0 +1,71 @@
+# frozen_string_literal: true
+
+# There is a general problem inside AMS related to custom association finder
+# combined with `include_data` setting:
+#
+#   class BlogPost3Serializer < BaseTestSerializer
+#     belongs_to :category do
+#       include_data :if_sideloaded
+#       object.categories.last
+#     end
+#   end
+#
+# The problem is that `belongs_to` block will be fully evaluated each time for
+# each object, and only after that AMS is able to take into account
+# `include_data` mode -
+# https://github.com/rails-api/active_model_serializers/blob/v0.10.10/lib/active_model/serializer/reflection.rb#L162-L163
+#
+#   def value(serializer, include_slice)
+#     # ...
+#     block_value = instance_exec(serializer, &block) if block
+#     return unless include_data?(include_slice)
+#     # ...
+#   end
+#
+# That causing redundant (and so huge potentially!) SQL queries and AR objects
+# allocation when `include_data` appears to be `false` but `belongs_to` block
+# defines instant (not a kind of AR::Relation) custom association finder.
+#
+# Described problem is a very specific use case for pure AMS applications.
+# The bad news is that `ams_lazy_relationships` is always utilize the
+# association block -
+# https://github.com/Bajena/ams_lazy_relationships/blob/v0.2.0/lib/ams_lazy_relationships/core/relationship_wrapper_methods.rb#L32-L36
+#
+# def define_lazy_association(type, name, options, block)
+#   #...
+#   block ||= lambda do |serializer|
+#     serializer.public_send("lazy_#{name}")
+#   end
+#
+#   public_send(type, name.to_sym, real_relationship_options, &block)
+#   #...
+# end
+#
+# This way we break `include_data` optimizations for the host application.
+#
+# In order to overcome that we are forced to monkey-patch
+# `AmsLazyRelationships::Extensions::Reflection#value` method and make it to be
+# ready for Proc returned by association block. This way we will use a kind of
+#
+#   block ||= lambda do |serializer|
+#     -> { serializer.public_send("lazy_#{name}") }
+#   end
+#
+# as association block, then AMS will evaluate it, get the value of `include_data`
+# setting, make a decision do we need to continue with that association, if so -
+# will finally evaluate the proc with lazy relationship inside it.
+
+module AmsLazyRelationships
+  module Extensions
+    module Reflection
+      def value(*)
+        case(block_value = super)
+        when Proc, Method then block_value.call
+        else block_value
+        end
+      end
+    end
+  end
+end
+
+::ActiveModel::Serializer::Reflection.prepend AmsLazyRelationships::Extensions::Reflection
diff --git a/spec/core_spec.rb b/spec/core_spec.rb
@@ -866,4 +866,102 @@ class UserSerializer < BaseTestSerializer
       end
     end
   end
+
+  describe 'include_data AMS setting' do
+    next unless AMS_VERSION >= Gem::Version.new("0.10.3")
+
+    shared_examples 'lazy loader when include_data option is set' do
+      let(:adapter_class) { json_api_adapter_class }
+      let(:includes) { ['blog_posts'] }
+      let(:category_data) { json.dig(:included, 0, :relationships, :category, :data) }
+
+      it 'does not fire unnecessary SQL query' do
+        expect { json }
+          .to make_database_queries(count: 1, matching: 'blog_posts')
+          .and make_database_queries(count: 0, matching: 'categories')
+
+        expect(category_data).to be_nil
+      end
+
+      context 'when sideloaded' do
+        let(:includes) { ['blog_posts.category'] }
+
+        it 'fires single SQL query' do
+          expect { json }
+            .to make_database_queries(count: 1, matching: 'blog_posts')
+            .and make_database_queries(count: 1, matching: 'categories')
+
+          expect(category_data).to be_present
+        end
+      end
+    end
+
+    context 'include_data disabled globally' do
+      let(:level0_serializer_class) do
+        module Serializer14
+          class BlogPost1Serializer < BaseTestSerializer
+            lazy_belongs_to :category
+          end
+
+          class User1Serializer < BaseTestSerializer
+            lazy_has_many :blog_posts, serializer: BlogPost1Serializer
+          end
+        end
+
+        Serializer14::User1Serializer
+      end
+
+      around do |example|
+        backup = ActiveModel::Serializer.config.include_data_default
+        ActiveModel::Serializer.config.include_data_default = :if_sideloaded
+
+        example.run
+
+        ActiveModel::Serializer.config.include_data_default = backup
+      end
+
+      include_examples 'lazy loader when include_data option is set'
+    end
+
+    context 'include_data disabled locally with custom finder' do
+      let(:level0_serializer_class) do
+        module Serializer14
+          class BlogPost2Serializer < BaseTestSerializer
+            lazy_belongs_to :category do |serializer|
+              include_data :if_sideloaded
+              -> { serializer.lazy_category }
+            end
+          end
+
+          class User2Serializer < BaseTestSerializer
+            lazy_has_many :blog_posts, serializer: BlogPost2Serializer
+          end
+        end
+
+        Serializer14::User2Serializer
+      end
+
+      include_examples 'lazy loader when include_data option is set'
+    end
+
+    context 'include_data disabled locally without custom finder' do
+      let(:level0_serializer_class) do
+        module Serializer14
+          class BlogPost3Serializer < BaseTestSerializer
+            lazy_belongs_to :category do
+              include_data :if_sideloaded
+            end
+          end
+
+          class User3Serializer < BaseTestSerializer
+            lazy_has_many :blog_posts, serializer: BlogPost3Serializer
+          end
+        end
+
+        Serializer14::User3Serializer
+      end
+
+      include_examples 'lazy loader when include_data option is set'
+    end
+  end
 end