Lazy dig #44

stokarenko · 2020-01-09T15:30:11Z

It introduces lazy_dig serializer instance method, in order to dig through nested associations keeping laziness and N+1-free evaluation.

class AuthorSerializer < BaseSerializer
  lazy_relationship :address
end

class BlogPostSerializer < BaseSerializer
  lazy_relationship :author

  attribute :author_address do
    lazy_dig(:author, :address)&.full_address
  end
end

Also we turn all primary keys into UUIDs within a test suite, that is much stronger. Indeed, with auto-increment keys when we assert the things like

  expect(json).to eq(blog_posts.map(&:id))

it's possible to face with false-positive result when not BlogPosts but, say, Comments was JSONed, just because

blog_posts = 3.times.map { BlogPost.create! }
comments = 3.times.map { Comment.create! }

blog_posts.map(&:id) == comments.map(&:id)
# => true, both of them equal to `[1,2,3]`

stokarenko · 2020-01-09T15:32:51Z

@Bajena Is that good enough in general? ) Checking CC issues in meanwhile )

stokarenko · 2020-01-09T19:46:44Z

@Bajena fixed CC, ready for review )

please release it with as a new version if possible, that is the last piece of puzzle for me )

Bajena · 2020-01-10T06:37:35Z

What's the most typical use case for lazy_dig in your project? I can't really think of any possible uses in my project, but I guess you haven't invented this only for fun? :)

Bajena

Looks fine overall. I posted some minor comments.

Bajena · 2020-01-10T06:39:14Z

lib/ams_lazy_relationships/core/evaluation.rb

    # Recursively loads the tree of lazy relationships
    # The nesting is limited to 3 levels.
    #
    # @param object [Object] Lazy relationships will be loaded for this record.
    # @param level [Integer] Current nesting level
-    def load_all_lazy_relationships(object, level = NESTING_START_LEVEL)
+    def init_all_lazy_relationships(object, level = NESTING_START_LEVEL)


Good idea with renaming this. I'd even consider renaming it to sth like collect_all_lazy_relationships or touch_all_lazy_relationships, but this is good enough as well 👍

Bajena · 2020-01-10T06:43:51Z

lib/ams_lazy_relationships/core/lazy_dig_method.rb

+
+module AmsLazyRelationships::Core
+  module LazyDigMethod
+    def lazy_dig(*relation_names)


It'd be good to add some documentation to this method - what args it accepts, what it returns and an example of use

Bajena · 2020-01-10T06:54:34Z

lib/ams_lazy_relationships/core/lazy_dig_method.rb

+        next_objects = lazy_dig_next_objects!(relation_name, serializer, object)
+        next unless next_objects
+
+        relationships[:multiple] ||= next_objects.respond_to?(:to_ary)


Using ||= on boolean values is a bit risky, because it'll get overwritten even if the value was set previously but is false or nil.
Do we even need this :multiple key anyways?

well, we always initialize this flag by false at the beginning

def lazy_dig(*relation_names) relationships = { multiple: false, # ... }

The idea is to maintain the singular/plural nature of lazy_dig returned value based on singular/plural nature of the chain of requested relation names. I.e.

lazy_dig(:company, :logo) # returns single AR record or nil lazy_dig(:companies, :logo) # returns the array of AR records lazy_dig(:company, :logos) # returns the array of AR records

In general, lazy_dig will return single AR object (or nil) if every relationship in a chain was a kind of singular association, and will return an array otherwise. So highlighted expression is hunting for any single plural relationship

relationships[:multiple] ||= next_objects.respond_to?(:to_ary)

. Then we decide what kind of return value will be as follows

relationships[:multiple] ? objects : objects.first

Bajena · 2020-01-10T06:56:58Z

lib/ams_lazy_relationships/core/lazy_dig_method.rb

+    end
+
+    def lazy_dig_next_objects!(relation_name, serializer, object)
+      serializer&.send(


I'd even consider raising an ArgumentError exception here if there's no such (lazy) relationship in the serializer. It'd prevent nils, because of typos. On the other hand standard dig doesn't care neither 🤔

Honestly I don't have a strong opinion on this, even don't know is that good or bad for me personally )

I just wanted to simulate the Hash#dig behavior which silently ignores any kind of misses )

My vote is to leave that as it is ) If we need more strict method (I'm not sure that we are though) - let it have a bang name like lazy_dig! )

I'd even consider raising an ArgumentError exception here if there's no such (lazy) relationship in the serializer.

Hm, thinking about raising the ArgumentError widely

module AmsLazyRelationships::Core module Evaluation def load_lazy_relationship(relation_name, object) lrm = lazy_relationships[relation_name] # Raise ArgumentError ?. return unless lrm

@Bajena Yeah, did that, added tests, please take a look )

stokarenko · 2020-01-10T09:23:43Z

What's the most typical use case for lazy_dig in your project? I can't really think of any possible uses in my project, but I guess you haven't invented this only for fun? :)

The quote right from my project, very similar to example posted to README.md:

class OrganizationSerializer
    lazy_belongs_to :logo
end

class UserSerializer
    lazy_belongs_to :organization

    attribute(:organization_logo_url) { lazy_dig(:organization, :logo)&.url }
end

I.e. our motivation is the same as described in Example 5

Sometimes you may just want to make use of lazy relationship without rendering the whole nested record. For example imagine that your BlogPost serializer is supposed to render author_name attribute.

, but we are going several steps deeper through nested relationships.

Bajena

Great work, nothing more to add🎉 Thanks for your hard work!

* Use uuids as primary keys within test suite * Add `lazy_dig` as serializer instance method

Bajena · 2020-01-11T07:12:54Z

Released v0.2.0

stokarenko · 2020-01-11T08:32:37Z

@Bajena thank you a lot! )

Use uuids as primary keys within test suite

e19c5cd

stokarenko requested a review from Bajena January 9, 2020 15:30

stokarenko force-pushed the lazy-dig-vol3 branch 3 times, most recently from 60143d6 to 42f30bd Compare January 9, 2020 16:39

Bajena reviewed Jan 10, 2020

View reviewed changes

stokarenko force-pushed the lazy-dig-vol3 branch 2 times, most recently from 6b61a25 to b712485 Compare January 10, 2020 11:52

Add lazy_dig as serializer instance method

2381a13

stokarenko force-pushed the lazy-dig-vol3 branch from b712485 to 2381a13 Compare January 10, 2020 11:54

Bajena approved these changes Jan 11, 2020

View reviewed changes

Bajena merged commit 951a773 into Bajena:master Jan 11, 2020

Bajena pushed a commit that referenced this pull request Jan 11, 2020

Lazy dig (#44)

2487912

* Use uuids as primary keys within test suite * Add `lazy_dig` as serializer instance method

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Lazy dig #44

Lazy dig #44

stokarenko commented Jan 9, 2020

stokarenko commented Jan 9, 2020

stokarenko commented Jan 9, 2020

Bajena commented Jan 10, 2020

Bajena left a comment

Bajena Jan 10, 2020

Bajena Jan 10, 2020

stokarenko Jan 10, 2020

Bajena Jan 10, 2020

stokarenko Jan 10, 2020 •

edited

Loading

Bajena Jan 10, 2020

stokarenko Jan 10, 2020

stokarenko Jan 10, 2020 •

edited

Loading

stokarenko Jan 10, 2020

stokarenko commented Jan 10, 2020

Bajena left a comment

Bajena commented Jan 11, 2020

stokarenko commented Jan 11, 2020

Lazy dig #44

Lazy dig #44

Conversation

stokarenko commented Jan 9, 2020

stokarenko commented Jan 9, 2020

stokarenko commented Jan 9, 2020

Bajena commented Jan 10, 2020

Bajena left a comment

Choose a reason for hiding this comment

Bajena Jan 10, 2020

Choose a reason for hiding this comment

Bajena Jan 10, 2020

Choose a reason for hiding this comment

stokarenko Jan 10, 2020

Choose a reason for hiding this comment

Bajena Jan 10, 2020

Choose a reason for hiding this comment

stokarenko Jan 10, 2020 • edited Loading

Choose a reason for hiding this comment

Bajena Jan 10, 2020

Choose a reason for hiding this comment

stokarenko Jan 10, 2020

Choose a reason for hiding this comment

stokarenko Jan 10, 2020 • edited Loading

Choose a reason for hiding this comment

stokarenko Jan 10, 2020

Choose a reason for hiding this comment

stokarenko commented Jan 10, 2020

Bajena left a comment

Choose a reason for hiding this comment

Bajena commented Jan 11, 2020

stokarenko commented Jan 11, 2020

stokarenko Jan 10, 2020 •

edited

Loading

stokarenko Jan 10, 2020 •

edited

Loading