Fix: hasManyThrough inclusion performance #8251

0x0aNL · 2022-01-19T22:28:12Z

As described in my proposal in #8250, this PR changes the hasManyThrough inclusion resolver from performing at least one query per through model, to performing a single query per inclusion level.

Changes in this PR:
hasManyThrough inclusion resolver:

collect all target model keys from the through models
perform a single findByForeignKeys call
do some magic to adhere to scope limit and order
"rearrange" the result in the proper format and return

repository-tests:

Added hasManyThrough relation back from CartItem to Customer
Added test for bi-directional hasManyThrough relation
(I wrote this test to test the scope magic. Additionally, the current hasManyThrough relation test uses UserLink with User<>User, but I found sometimes that issues occur when including A<>B<>A instead of A<>A - see Regression: secondary HasManyThrough inclusion fails with circular dependency #7599. )

Possible issues:

scope magic: limit and fields are emulated and as such may not work properly or keep working properly
unforeseen scope issues: I feel uncertain that limit and fields, even if properly emulated, are enough to satisfy all scope cases, despite all tests passing
repository tests: I fear I may have broken some guidelines. Perhaps the commit should be discarded

Notes:

Doesn't fix the "issue" of re-querying an Item in an inclusion of depth 3 with bi-directional hasManyThrough (e.g., when finding Item with relation Category with relation Item, all Items will be queried twice)

Checklist

DCO (Developer Certificate of Origin) signed in all commits
npm test passes on your machine
New tests added or existing tests modified to cover all changes
Code conforms with the style guide (not 100% sure)
API Documentation in code was updated
Documentation in /docs/site was updated
Affected artifact templates in packages/cli were updated
Affected example projects in examples/* were updated

achrinza

The changes LGTM; If possible, I'd like another maintainer's approval before merging.

@dhmlau @raymondfeng

nabdelgadir

LGTM, just some minor comments

nabdelgadir · 2022-02-02T23:37:31Z

...sitory-tests/src/crud/relations/acceptance/has-many-through-inclusion-resolver.acceptance.ts

@@ -185,19 +231,27 @@ export function hasManyThroughInclusionResolverAcceptance(
        expect(toJSON(result)).to.deepEqual(toJSON(expected));
      });

-      it('honours limit scope when returning a model', async () => {
+      it('honours limit and order scope when returning a model', async () => {


question - how is this test showing it's honouring the order?

There are 3 customers of which only 2 are selected due to the limit. The two that are expected to be returned are Zelda and Voldemort (in that order). Since that order does not correlate to either the creation sequence of the customers nor the linking sequence of the carts to the customers, it can only be explained by successful usage of the order clause. (Although it could technically be caused by unexpected random order returned by the memory connnector ..)

Perhaps I should add a comment with this explanation to the test?

I just noticed my response was about a different section of the changeset.

In this test, I added an order clause which should put the second cart item (shield) as the first of the included cart items. The last expect tests whether that is the case.

nabdelgadir · 2022-02-02T23:39:43Z

packages/repository/src/relations/has-many/has-many-through.inclusion-resolver.ts

+
+    // get target ids from the through entities by foreign key
+    const allIds = _.uniq(
+      throughResult.map(x => x?.map(entity => entity[throughKeyTo])).flat(),


nitpick: would it be possible to use descriptive variables instead of x for here and below?

I'll change x to throughEntity.

nabdelgadir · 2022-02-02T23:59:47Z

packages/repository/src/relations/has-many/has-many-through.inclusion-resolver.ts

+    const pickFieldList = Array.isArray(scope?.fields)
+      ? scope?.fields
+      : undefined;
+    const omitFieldList = scope?.fields


Nice 👍 I think it might be nice to include a comment explaining what this does, but not necessary.

scope.fields is undefined -> []

scope.fields is an array of properties e.g. ["id", "name"] -> []

scope.fields is an object = {id: true, name: false} -> ['name']

I'll add a comment or two.

0x0aNL · 2022-02-03T17:52:54Z

Questions about updating the PR:

Should I commit changes in a single commit with a message like "fix: add explanatory comments"?
Alternatively, should I squash with existing commits?
Should I rebase on master?

nabdelgadir · 2022-02-05T18:08:58Z

Questions about updating the PR:

Should I commit changes in a single commit with a message like "fix: add explanatory comments"?

Alternatively, should I squash with existing commits?

During the review process, it is easier for us if you do separate commits to address comments so we can see the difference. But since this is a small PR, you can just squash it right away to the relevant commit.

Should I rebase on master?

I would recommend rebasing, but the final commits will be rebased when it's merged even if you don't rebase locally.

Here's some docs for reference https://loopback.io/doc/en/lb4/submitting_a_pr.html#5-pr-review-process

0x0aNL · 2022-03-09T12:36:12Z

Finally gotten around to update this PR. I did rebase and squash however, so I'll describe the changes:

Next to adding several comments and renaming some variables based on the review so far, there is one bigger change: instead of manually building a pickFieldList and omitFieldList and emulating the field filter, the field filter is now normalized using FilterBuilder. Only the targetKey field is manipulated: making sure it is in the result set, and making sure it is removed from the final response if it is supposed to be omitted. Readability and functionality should be improved!

samarpanB · 2023-04-06T09:05:30Z

@0x0aNL Can you please rebase this PR so that we can revive this PR and merge it then ?

0x0aNL · 2023-04-07T12:40:08Z

@0x0aNL Can you please rebase this PR so that we can revive this PR and merge it then ?

I have rebased the PR!

Signed-off-by: Roderik van Heijst <rvanheijst@0x0a.nl>

…ation inclusion Signed-off-by: Roderik van Heijst <rvanheijst@0x0a.nl>

0x0aNL · 2023-08-03T11:40:04Z

Rebased once again and fixed performance in cases where no Through models were found.

coveralls · 2023-08-03T12:25:56Z

Pull Request Test Coverage Report for Build 5750231698

2 of 21 (9.52%) changed or added relevant lines in 1 file are covered.
No unchanged relevant lines lost coverage.
Overall coverage decreased (-0.2%) to 69.385%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
packages/repository/src/relations/has-many/has-many-through.inclusion-resolver.ts	2	21	9.52%

Totals
Change from base Build 5749758464:	-0.2%
Covered Lines:	9498
Relevant Lines:	12384

💛 - Coveralls

dhmlau · 2023-10-16T12:39:59Z

It seems like this PR has been approved for a while. Merging it now.

@0x0aNL, thanks for your contribution. Your PR has been landed! 🎉

0x0aNL requested review from agnes512 and nabdelgadir as code owners January 19, 2022 22:28

achrinza self-requested a review January 22, 2022 08:32

achrinza approved these changes Jan 22, 2022

View reviewed changes

nabdelgadir approved these changes Feb 3, 2022

View reviewed changes

0x0aNL force-pushed the fix/hasManyThrough-inclusion-performance branch from 6b050bd to d84bc11 Compare March 9, 2022 12:08

0x0aNL force-pushed the fix/hasManyThrough-inclusion-performance branch from d84bc11 to d47e056 Compare April 7, 2023 12:33

0x0aNL added 2 commits August 3, 2023 13:35

fix(repository): improve HasManyThrough inclusion performance

35fbb4e

Signed-off-by: Roderik van Heijst <rvanheijst@0x0a.nl>

test(repository-tests): add test for bi-drectional hasManyThrough rel…

765157c

…ation inclusion Signed-off-by: Roderik van Heijst <rvanheijst@0x0a.nl>

0x0aNL force-pushed the fix/hasManyThrough-inclusion-performance branch from d47e056 to 765157c Compare August 3, 2023 11:37

dhmlau merged commit 001d970 into loopbackio:master Oct 16, 2023
13 checks passed

dhmlau mentioned this pull request Dec 12, 2023

@loopback/repository 6.1.4 broke hasManyThrough relations #10259

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix: hasManyThrough inclusion performance #8251

Fix: hasManyThrough inclusion performance #8251

0x0aNL commented Jan 19, 2022

achrinza left a comment

nabdelgadir left a comment

nabdelgadir Feb 2, 2022

0x0aNL Feb 3, 2022

0x0aNL Feb 9, 2022

nabdelgadir Feb 2, 2022

0x0aNL Feb 3, 2022

nabdelgadir Feb 2, 2022

0x0aNL Feb 3, 2022

0x0aNL commented Feb 3, 2022 •

edited

nabdelgadir commented Feb 5, 2022

0x0aNL commented Mar 9, 2022

samarpanB commented Apr 6, 2023

0x0aNL commented Apr 7, 2023

0x0aNL commented Aug 3, 2023

coveralls commented Aug 3, 2023

dhmlau commented Oct 16, 2023

Fix: hasManyThrough inclusion performance #8251

Fix: hasManyThrough inclusion performance #8251

Conversation

0x0aNL commented Jan 19, 2022

Checklist

achrinza left a comment

Choose a reason for hiding this comment

nabdelgadir left a comment

Choose a reason for hiding this comment

nabdelgadir Feb 2, 2022

Choose a reason for hiding this comment

0x0aNL Feb 3, 2022

Choose a reason for hiding this comment

0x0aNL Feb 9, 2022

Choose a reason for hiding this comment

nabdelgadir Feb 2, 2022

Choose a reason for hiding this comment

0x0aNL Feb 3, 2022

Choose a reason for hiding this comment

nabdelgadir Feb 2, 2022

Choose a reason for hiding this comment

0x0aNL Feb 3, 2022

Choose a reason for hiding this comment

0x0aNL commented Feb 3, 2022 • edited

nabdelgadir commented Feb 5, 2022

0x0aNL commented Mar 9, 2022

samarpanB commented Apr 6, 2023

0x0aNL commented Apr 7, 2023

0x0aNL commented Aug 3, 2023

coveralls commented Aug 3, 2023

Pull Request Test Coverage Report for Build 5750231698

💛 - Coveralls

dhmlau commented Oct 16, 2023

0x0aNL commented Feb 3, 2022 •

edited