Count issue with related models using though model #1795 #1801
Conversation
regevbr
requested review from
b-admike,
dhmlau,
emonddr,
jannyHou and
zbarbuto
as code owners
|
Can one of the admins verify this patch? To accept patch and trigger a build add comment ".ok\W+to\W+test." |
regevbr
force-pushed
the
1795
branch
2 times, most recently
from
a59fbe7
to
b693a57
Compare
|
@bajtos can you please review? |
|
@slnode test please |
|
Thank you @regevbr for the pull request. This is a large change, I need to set aside some time to review it in detail. I'll try to get it done by the end of next week, before I leave for vacation. @raymondfeng can you please take a look too? |
|
@bajtos sure I will try to get on it soon, I have a lot on my plate :-) |
|
@bajtos I fixed the tests (which actually got me to find bugs I introduced so I fixed them as well) |
|
Thank you @regevbr for the updates, the tests look much better now! I am afraid I got stuck in other tasks and did not find enough time to review your changes. If nobody from @strongloop/loopback-3x-maintainers and/or @strongloop/loopback-maintainers steps up to review this pull request, then I am afraid you will have to wait until January. Sorry for the delay 😢 |
|
@slnode test please |
|
Sorry @regevbr for the long silence on our side. The pull request is quite involved, I find it difficult to find enough time (and energy) to dive into the changes. Is there any chance to split this patch into smaller pull requests, where each pull request would be a meaningful change on its own? The smaller the patch, the easier it is to review for us. |
| patients.should.have.lengthOf(0); | ||
| }); | ||
|
|
||
| it('should find scoped record with promises based on related model id', async function() { |
There was a problem hiding this comment.
I think there are some redundant tests. For example, this test, find , and find explicit look the same to me.
If just to keep one test, I think should find scoped record with promises based on related model id is the best title cause the change is related to scope.
Same for the rest tests.
| } | ||
|
|
||
| return definition; | ||
| } | ||
|
|
||
| function smartMergeRelatedModelQuery(findData, queryRelated, keyFrom, IdKey) { |
There was a problem hiding this comment.
Please add API documentation for this function.
| let scopeOnRelatedModel = false; | ||
| let queryRelated, keyFrom, relatedModel, IdKey, fieldsRelated; | ||
| if (this._scope && this._scope.collect && | ||
| where !== null && typeof where === 'object') { | ||
| queryRelated = { | ||
| relation: this._scope.collect, | ||
| scope: { | ||
| where: where, | ||
| }, | ||
| }; | ||
| scopeOnRelatedModel = true; | ||
| relatedModel = targetModel.relations[queryRelated.relation].modelTo; | ||
| IdKey = idName(relatedModel); | ||
| keyFrom = targetModel.relations[queryRelated.relation].keyFrom || IdKey; | ||
| fieldsRelated = [keyFrom]; | ||
| if (where[IdKey]) { | ||
| where = { | ||
| [keyFrom]: where[IdKey], | ||
| }; | ||
| } else { | ||
| where = {}; | ||
| } | ||
| } |
There was a problem hiding this comment.
I found repeated code in these CRUD methods. Maybe we can refactor them to have a helper function.
|
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
|
This issue has been closed due to continued inactivity. Thank you for your understanding. If you believe this to be in error, please contact one of the code owners, listed in the |
Fixes #1795 - fixed wrong behaviour of
count,findOne,updateAll,destroyAllon related models that use through models.Fixes #1796 - Optimized the bloated query being used on through models when performing
find, making it more efficientAdded optimization when querying related models using the id in the filter, to make the call to the through model more efficient.
Applied the fix from strongloop/loopback/issues/1076 to the explicit call of
findon a related modelChecklist
npm testpasses on your machine