Skip redundant queries when include_data is disabled #47

stokarenko · 2020-01-14T18:10:15Z

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 -

  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 -

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.

In additional, now we allow skipping the custom lazy finder when our goal is just to define include_data setting nor some links/metas:

  lazy_has_many :comments do
    include_data :if_sideloaded
    link :self, 'a link'
    meta name: 'Dan Brown'
   end

stokarenko · 2020-01-14T18:16:41Z

@Bajena

lazy_dig is the last piece of puzzle for me )

Well, found one more missing piece, please take a look )
Also please check these two PRs:

I merged them without your approval but hope that is not an issue since they are about documentation and tests. They blocked include_data stuff a bit )

stokarenko · 2020-01-14T19:12:22Z

Have no ideas how to overcome that Cognitive Complexity of 6 CC issue..
Will try again tomorrow if that is strongly required but as I feel that is exact case when consider refactoring will be worse than original )

Bajena · 2020-01-14T19:22:03Z

Hey again;)
Thanks for another good finding - I'll do my best to review and ship it this week.

Regarding #46 - it'd be cool to have 0.10.0rc4 tests back as my we're using it in our company :D
If that's a problem for you I can try to add it back myself.

stokarenko · 2020-01-14T19:25:44Z

Hey again;)
Thanks for another good finding - I'll do my best to review and ship it this week.

Regarding #46 - it'd be cool to have 0.10.0rc4 tests back as my we're using it in our company :D
If that's a problem for you I can try to add it back myself.

No problem, I'll add )
0.10.0rc4 has the issue on its dependencies and raises undefined method "underscore" on String right on pure require "active_model_serializers". Need to require active_support before it, that's it )

stokarenko · 2020-01-14T21:01:00Z

it'd be cool to have 0.10.0rc4 tests back as my we're using it in our company :D

Here we go ) #48

stokarenko · 2020-01-14T21:05:25Z

Rebased to master with testing against v0.10.0.rc4, pushed )

stokarenko · 2020-01-15T11:43:51Z

Method define_lazy_association has a Cognitive Complexity of 6

ugh, no chance to escape..

Bajena

Sorry that it took some time. I did a first round of review - let me know what you think about my comments.
Btw, good to know that include_data option exists in newer AMS versions. I'd never notice it, because we're using 0.10.0rc4 in my company 🗡

Bajena · 2020-01-16T08:29:46Z

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 }


It can still be called without using a proc, right?
Maybe we could keep the old version for simplicity and just mention the proc form in the include_data example you added in https://github.com/Bajena/ams_lazy_relationships/pull/47/files#diff-04c6e90faac2675aa89e2176d2eec7d8R134 ?

Suggested change

-> { serializer.lazy_blog_posts }

serializer.lazy_blog_posts

It can still be called without using a proc, right?

sure, Reflection monkey-patch will forward non-proc value as it is.
But I believe that in readme is better to propose the best possible sample by default.
Just imagine:

engineer_1 adds non-proc custom finders all over the services & serializers;

engineer_2 turns ActiveModel::Serializer.config.include_data_default = false globally

local naive tests will assert successfully that json response is free from data, but

production servers will continue to fail due to OOM and that will be quite tricky to discover why

I think we need to leave -> { serializer.lazy_blog_posts } as a very default sample, and add a small hint that serializer.lazy_blog_posts will work as well but can produce redundant queries.

I think we need to leave -> { serializer.lazy_blog_posts } as a very default sample, and add a small hint that serializer.lazy_blog_posts will work as well but can produce redundant queries.

Ok, I must admit that your explanation makes sense to me. Let's leave proc, but also let's mention the non-proc version as you suggested 👍

roger that! )

Bajena · 2020-01-16T08:32:40Z

README.md

+end
+```
+
+Keep attention that custom finder should be returned in a form of lambda,


Suggested change

Keep attention that custom finder should be returned in a form of lambda,

If you're using `include_data` AMS setting make sure that your association blocks return a lambda in order to avoid redundant SQL queries when `include_data` AMS setting evaluates to `false`:

Again, typically include_data will be turned off a way further from the project beginning, on the very last phase of make it work, make it right, make it fast ;) I think it's good to make an accent how to do the things better right from beginning )

lib/ams_lazy_relationships/extensions/reflection.rb

Bajena · 2020-01-16T08:40:20Z

lib/ams_lazy_relationships/extensions/reflection.rb

+# 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


I'd avoid adding links to specific versions in the code. Let's get rid of lines 32-44 - they may quickly get out of date.

That is exactly why I pinned v0.2.0 tag here ))

Bajena · 2020-01-16T08:47:07Z

lib/ams_lazy_relationships/extensions/reflection.rb

+  end
+end
+
+::ActiveModel::Serializer::Reflection.prepend AmsLazyRelationships::Extensions::Reflection


I'd consider applying this patch only to AMS versions >= 0.10.3.
Nobody using lower versions of AMS is probably going to return lambdas from their association blocks anyways as they won't benefit from that anyhow.

I thought a lot about that from the beginning, but this way we need to apply the same condition in default define_lazy_association block.

Even worse, readme description will be more complex as well since for lower AMS versions the proc-like custom finder will instantly fail. I.e. we propose the snippet of code which will crush on AMS version still accepted by gemspec file..

As an option, we can move all include_data stuff in separate lib file. require 'ams_lazy_relationships/efficient_finders' will engage monkey-patch and proc-like default finder, without said require nothing will be applied.

How you think?

As an option, we can move all include_data stuff in separate lib file.

Notice that this way ams_lazy_relationships will still silently produce a huge problems to consumers due to cancelled effect from include_data optimizations in sense of DB hits and memory usage.

Ok, you're right, doing it the way I suggested generates more problems than it solves. Let's keep it then. But in such case let's make sure that all AMS versions are tested for proc evaluation (specs are skipped for lower AMS versions in https://github.com/Bajena/ams_lazy_relationships/pull/47/files#diff-6e72b60a2fb8b930cc9741785114a948R871).

They are tested already due to the new default define_lazy_association block, but I'll add an accented test with proc-like custom finder )

Bajena · 2020-01-16T08:48:09Z

README.md

+```ruby
+class BlogPostSerializer < BaseSerializer
+  lazy_has_many :comments do
+    include_data :if_sideloaded


Wasn't it supposed to be:

Suggested change

include_data :if_sideloaded

include_data false

?

:if_sideloaded is the most complex example, this way data will be included for explicit ?include=association and rejected without it.

In second case we need to suppress any kind of finders but we need to keep the finder alive for the time when first case will happen.

i.e.

lazy_has_many :comments do include_data false nil end

is completely ok and ams_lazy_relationship will behave like that without this PR.

Ok, thanks for explanation - so this will work correctly, without N+1s, both when the resource is included and when it isn't 👍

Bajena · 2020-01-16T08:48:32Z

README.md

+```
+
+Feel free to skip custom lazy finder for association if your goal is just to
+define `include_data` setting nor some links/metas:


Suggested change

define `include_data` setting nor some links/metas:

define `include_data` setting to render some links/metas:

?

include_data is not about links and metas, but mostly about skipping so-called json relationships.

By default AMS will add to response the primary keys for each association defined in serializer. I.e.

class OrganizationSerializer has_many :users end OrganizationSerializer.new(org).as_json # {data: {relationships: { users: { [{id: 1}, {id: 2}, ..., {id: 123_567_89} ] } } } }

That happens always even without explicit ?include=users, that produces SELECT * FROM users query on the way and memory allocation for 123_567_89 AR objects.

include_data = false or :if_sideloaded is a way to survive.

I never tried but believe that include_data = :if_sideloaded can be combined with links and metas

True, you're right, I missed the point :) But TBH here I mostly meant that this sentence is not a correct english sentence and tried to rework it a bit. Would you be able to rephrase it a bit?

will try, thanks! )

Co-Authored-By: Jan Bajena <bajena3@gmail.com>

stokarenko · 2020-01-16T10:19:26Z

@Bajena Ready for the second round of review )

Bajena

Just the specs and some minor doc changes left and we should be able to !

Bajena · 2020-01-16T19:08:37Z

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 }


I think we need to leave -> { serializer.lazy_blog_posts } as a very default sample, and add a small hint that serializer.lazy_blog_posts will work as well but can produce redundant queries.

Ok, I must admit that your explanation makes sense to me. Let's leave proc, but also let's mention the non-proc version as you suggested 👍

Bajena · 2020-01-16T19:08:59Z

README.md

+end
+```
+
+Keep attention that custom finder should be returned in a form of lambda,


Bajena · 2020-01-16T19:12:32Z

README.md

+```
+
+Feel free to skip custom lazy finder for association if your goal is just to
+define `include_data` setting nor some links/metas:


True, you're right, I missed the point :) But TBH here I mostly meant that this sentence is not a correct english sentence and tried to rework it a bit. Would you be able to rephrase it a bit?

Bajena · 2020-01-16T19:14:24Z

README.md

+```ruby
+class BlogPostSerializer < BaseSerializer
+  lazy_has_many :comments do
+    include_data :if_sideloaded


Ok, thanks for explanation - so this will work correctly, without N+1s, both when the resource is included and when it isn't 👍

Bajena · 2020-01-16T19:18:41Z

lib/ams_lazy_relationships/extensions/reflection.rb

+  end
+end
+
+::ActiveModel::Serializer::Reflection.prepend AmsLazyRelationships::Extensions::Reflection


Ok, you're right, doing it the way I suggested generates more problems than it solves. Let's keep it then. But in such case let's make sure that all AMS versions are tested for proc evaluation (specs are skipped for lower AMS versions in https://github.com/Bajena/ams_lazy_relationships/pull/47/files#diff-6e72b60a2fb8b930cc9741785114a948R871).

stokarenko · 2020-01-16T20:47:09Z

@Bajena Everything is done I hope ) Please release a new gem version with this stuff )

Bajena

Great stuff!

stokarenko requested a review from Bajena January 14, 2020 18:17

stokarenko force-pushed the skip-redundant-queries-when-include-data-is-disabled-vol2 branch 4 times, most recently from 941ca34 to 3fdda71 Compare January 14, 2020 19:08

stokarenko force-pushed the skip-redundant-queries-when-include-data-is-disabled-vol2 branch from 3fdda71 to b0ad1df Compare January 14, 2020 21:04

stokarenko force-pushed the skip-redundant-queries-when-include-data-is-disabled-vol2 branch from b0ad1df to 4333617 Compare January 15, 2020 10:34

Skip redundant queries when include_data is disabled

a87b378

stokarenko force-pushed the skip-redundant-queries-when-include-data-is-disabled-vol2 branch from 4333617 to a87b378 Compare January 15, 2020 10:42

Bajena reviewed Jan 16, 2020

View reviewed changes

stokarenko and others added 2 commits January 16, 2020 12:31

Update lib/ams_lazy_relationships/extensions/reflection.rb

8f115b0

Co-Authored-By: Jan Bajena <bajena3@gmail.com>

Update lib/ams_lazy_relationships/extensions/reflection.rb

2f3fe92

Co-Authored-By: Jan Bajena <bajena3@gmail.com>

stokarenko mentioned this pull request Jan 16, 2020

Fix association duplicates after accepts_nested_attributes_for assignation #50

Merged

Bajena reviewed Jan 16, 2020

View reviewed changes

Adjust include_data documentation and tests

3289c9f

Bajena approved these changes Jan 16, 2020

View reviewed changes

Bajena merged commit e6d09b0 into Bajena:master Jan 16, 2020

	Keep attention that custom finder should be returned in a form of lambda,
	If you're using `include_data` AMS setting make sure that your association blocks return a lambda in order to avoid redundant SQL queries when `include_data` AMS setting evaluates to `false`:

	define `include_data` setting nor some links/metas:
	define `include_data` setting to render some links/metas:

Skip redundant queries when include_data is disabled #47

Skip redundant queries when include_data is disabled #47

Conversation

stokarenko commented Jan 14, 2020

stokarenko commented Jan 14, 2020

stokarenko commented Jan 14, 2020

Bajena commented Jan 14, 2020

stokarenko commented Jan 14, 2020

stokarenko commented Jan 14, 2020

stokarenko commented Jan 14, 2020

stokarenko commented Jan 15, 2020

Bajena left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stokarenko Jan 16, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stokarenko Jan 16, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stokarenko Jan 16, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stokarenko commented Jan 16, 2020

Bajena left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stokarenko commented Jan 16, 2020

Bajena left a comment

Choose a reason for hiding this comment

stokarenko Jan 16, 2020 •

edited

Loading

stokarenko Jan 16, 2020 •

edited

Loading

stokarenko Jan 16, 2020 •

edited

Loading