loopback-next
loopback-next copied to clipboard
Fix: hasManyThrough inclusion performance
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
toCustomer
- Added test for bi-directional hasManyThrough relation
(I wrote this test to test the scope magic. Additionally, the current hasManyThrough relation test uses
UserLink
withUser
<>User
, but I found sometimes that issues occur when includingA
<>B
<>A
instead ofA
<>A
- see #7599. )
Possible issues:
- scope magic:
limit
andfields
are emulated and as such may not work properly or keep working properly - unforeseen scope issues: I feel uncertain that
limit
andfields
, even if properly emulated, are enough to satisfy allscope
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 findingItem
with relationCategory
with relationItem
, allItems
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
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?
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
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 Can you please rebase this PR so that we can revive this PR and merge it then ?
@0x0aNL Can you please rebase this PR so that we can revive this PR and merge it then ?
I have rebased the PR!
Rebased once again and fixed performance in cases where no Through
models were found.
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 | |
---|---|
Change from base Build 5749758464: | -0.2% |
Covered Lines: | 9498 |
Relevant Lines: | 12384 |
💛 - 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! 🎉