loopback-next icon indicating copy to clipboard operation
loopback-next copied to clipboard

Fix: hasManyThrough inclusion performance

Open 0x0aNL opened this issue 2 years ago • 3 comments

Fixes #8250

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

  • [x] DCO (Developer Certificate of Origin) signed in all commits
  • [x] npm test passes on your machine
  • [x] New tests added or existing tests modified to cover all changes
  • [x] 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

0x0aNL avatar Jan 19 '22 22:01 0x0aNL

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?

0x0aNL avatar Feb 03 '22 17:02 0x0aNL

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

nabdelgadir avatar Feb 05 '22 18:02 nabdelgadir

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!

0x0aNL avatar Mar 09 '22 12:03 0x0aNL

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

samarpanB avatar Apr 06 '23 09:04 samarpanB

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

I have rebased the PR!

0x0aNL avatar Apr 07 '23 12:04 0x0aNL

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

0x0aNL avatar Aug 03 '23 11:08 0x0aNL

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%
<!-- Total: 2 21
Totals Coverage Status
Change from base Build 5749758464: -0.2%
Covered Lines: 9498
Relevant Lines: 12384

💛 - Coveralls

coveralls avatar Aug 03 '23 12:08 coveralls

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

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

dhmlau avatar Oct 16 '23 12:10 dhmlau