fix: make sure scope filters are used for each fk on includes #7965

collaorodrigo7 · 2021-10-03T00:40:22Z

If a scope filter is passed, the filter should be applied for each foreign key and not globally.
This implies a query for each fk, and may impact performance. However, is the only way to respect
the scope filter. Example, and include with scope.limit:2 should include 2 instances of the related
model for each fk. Previously scope.limit:2 will cause to "only" include 2 related models.
Signed-off-by:Rodrigo Collao collao.rodrigo7@gmail.com

fix #6832

Signed-off-by: Rodrigo Collao collao.rodrigo7@gmail.com

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

👉 Check out how to submit a PR 👈

collaorodrigo7 · 2021-10-03T03:02:52Z

Could someone help me with these failing tests? I am not able to see any logs about them.

mrmodise · 2021-10-03T04:00:01Z

Could someone help me with these failing tests? I am not able to see any logs about them.

Couldnt see them either. I re-ran the pipeline

collaorodrigo7 · 2021-10-03T04:31:24Z

Thanks, I can see a log from AppVeyor now, and I can recreate it locally.
I'll try to fix it

collaorodrigo7 · 2021-10-03T05:37:54Z

I think I solved the issue @mrmodise in case you have a chance to re-run the pipeline.
I don't know if that's the best way to deal with it though. I ended up passing a flag through the options parameter, to be able to honor the filter on the hasManyThrough.
So now, the filter should be honor no matter what kind of inclusion it is.

collaorodrigo7 · 2021-10-03T05:52:47Z

So, I think that commit definetly solved the previous problem. But I have no clue what is going on with the ubuntu-latest,10 pipeline since I can not see any logs.

achrinza · 2021-10-03T09:15:40Z

Not sure what's causing problems with rendering the error logs (I'm able to view them fine on Firefox).

Here's the logs:

Run faketty npm test --ignore-scripts

> loopback-next@0.1.0 test
> node packages/build/bin/run-nyc npm run mocha


> loopback-next@0.1.0 mocha
> node packages/build/bin/run-mocha --lang en_US.UTF-8 "packages/*/dist/__tests__/**/*.js" "extensions/*/dist/__tests__/**/*.js" "examples/*/dist/__tests__/**/*.js" "packages/cli/test/**/*.js" "packages/build/test/*/*.js"


................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................,......................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................,,,,.......................................................,........,.,.........!!..................................................................................................................................................................................................................................!............................!!..............................................................................................................................................................................................................................................................................................................................................................................................,...................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................,........................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................

  4006 passing (3m)
  10 pending
  5 failing

  1) DefaultCrudRepository + memory connector
       CRUD Repository operations
         Multi relations inclusion resolvers - acceptance
           custom scope with `where` clause:
     TypeError: findResults.flat is not a function
      at Promise.all.then.findResults (packages/repository/src/relations/relation.helpers.ts:29:4)
      at process._tickCallback (internal/process/next_tick.js:68:7)

  2) DefaultCrudRepository + memory connector
       CRUD Repository operations
         Multi relations inclusion resolvers - acceptance
           custom scope with nested inclusion and where clause:
     TypeError: findResults.flat is not a function
      at Promise.all.then.findResults (packages/repository/src/relations/relation.helpers.ts:29:4)
      at process._tickCallback (internal/process/next_tick.js:68:7)

  3) DefaultCrudRepository
       DefaultCrudRepository with relations
         find* methods with inclusion
           implements Repository.find() with included models and scope limit:
     TypeError: findResults.flat is not a function
      at Promise.all.then.findResults (packages/repository/src/relations/relation.helpers.ts:29:4)
      at process._tickCallback (internal/process/next_tick.js:68:7)

  4) findByForeignKeys
       does not manipulate non-primitive params:
     TypeError: findResults.flat is not a function
      at Promise.all.then.findResults (packages/repository/src/relations/relation.helpers.ts:29:4)

  5) findByForeignKeys
       runs a find for each fkValue to properly respect scope filters:
     TypeError: findResults.flat is not a function
      at Promise.all.then.findResults (packages/repository/src/relations/relation.helpers.ts:29:4)



Error: Process completed with exit code 5.

collaorodrigo7 · 2021-10-03T18:33:58Z

Not sure what's causing problems with rendering the error logs (I'm able to view them fine on Firefox).

Here's the logs:

Run faketty npm test --ignore-scripts

> loopback-next@0.1.0 test
> node packages/build/bin/run-nyc npm run mocha


> loopback-next@0.1.0 mocha
> node packages/build/bin/run-mocha --lang en_US.UTF-8 "packages/*/dist/__tests__/**/*.js" "extensions/*/dist/__tests__/**/*.js" "examples/*/dist/__tests__/**/*.js" "packages/cli/test/**/*.js" "packages/build/test/*/*.js"


................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................,......................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................,,,,.......................................................,........,.,.........!!..................................................................................................................................................................................................................................!............................!!..............................................................................................................................................................................................................................................................................................................................................................................................,...................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................,........................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................

  4006 passing (3m)
  10 pending
  5 failing

  1) DefaultCrudRepository + memory connector
       CRUD Repository operations
         Multi relations inclusion resolvers - acceptance
           custom scope with `where` clause:
     TypeError: findResults.flat is not a function
      at Promise.all.then.findResults (packages/repository/src/relations/relation.helpers.ts:29:4)
      at process._tickCallback (internal/process/next_tick.js:68:7)

  2) DefaultCrudRepository + memory connector
       CRUD Repository operations
         Multi relations inclusion resolvers - acceptance
           custom scope with nested inclusion and where clause:
     TypeError: findResults.flat is not a function
      at Promise.all.then.findResults (packages/repository/src/relations/relation.helpers.ts:29:4)
      at process._tickCallback (internal/process/next_tick.js:68:7)

  3) DefaultCrudRepository
       DefaultCrudRepository with relations
         find* methods with inclusion
           implements Repository.find() with included models and scope limit:
     TypeError: findResults.flat is not a function
      at Promise.all.then.findResults (packages/repository/src/relations/relation.helpers.ts:29:4)
      at process._tickCallback (internal/process/next_tick.js:68:7)

  4) findByForeignKeys
       does not manipulate non-primitive params:
     TypeError: findResults.flat is not a function
      at Promise.all.then.findResults (packages/repository/src/relations/relation.helpers.ts:29:4)

  5) findByForeignKeys
       runs a find for each fkValue to properly respect scope filters:
     TypeError: findResults.flat is not a function
      at Promise.all.then.findResults (packages/repository/src/relations/relation.helpers.ts:29:4)



Error: Process completed with exit code 5.

Thank you!. So what do you recommend me to flatten the array? Array.flat is not available on node 10.
Should I just use lodash, or write a helper function for it?

collaorodrigo7 · 2021-10-04T01:11:53Z

I end up using lodash to flatten the array. Tha should solve the problem

collaorodrigo7 · 2021-10-04T14:35:41Z

The only way I can recreate the timeout error reported by AppVeyor is if I run my laptop on battery and power-saving mode.
Could someone help me, or re-run the AppVeyor pipeline?

collaorodrigo7 · 2021-10-05T01:43:25Z

The macos pipeline is giving that same Error: Timeout of 30000ms exceeded. and now AppVeyor test now is passing without any changes. Is this common, or am I doing something wrong?

achrinza · 2021-10-05T13:32:18Z

Apologies for the confusion; I re-ran the Appveyor test earlier today. I've just re-ran the GitHub Actions workflows as well.

The macOS test does timeout sometimes; We've recently increased this timeout recently to see if this reduces the likelihood of it occurring and will continue to tune it over time.

coveralls · 2021-10-05T13:47:04Z

Pull Request Test Coverage Report for Build 1331204950

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

For more information on this, see Tracking coverage changes with pull request builds.
To avoid this issue with future PRs, see these Recommended CI Configurations.
For a quick fix, rebase this PR at GitHub. Your next report should be accurate.

Details

21 of 21 (100.0%) changed or added relevant lines in 1 file are covered.
No unchanged relevant lines lost coverage.
Overall coverage increased (+0.3%) to 76.31%

Totals
Change from base Build 1300113386:	0.3%
Covered Lines:	16869
Relevant Lines:	17724

💛 - Coveralls

mrbatista · 2021-10-06T13:48:34Z

@achrinza I personally don't agree with the idea to add property totalLimit for backward compatibility.
Every LB3 user sees this functionality as a bug.
@raymondfeng What is your opinion?

collaorodrigo7 · 2021-10-06T16:30:15Z

As a long time LB3 user, I completely agree with @mrbatista
Just my 2 cents

achrinza

@mrbatista @collaorodrigo7 I'm open to leaving totalLimit out of this PR if that's preferred; Do you see a downside with adding totalLimit for backwards-compatibility?

I would like to get input from the other maintainers as well @dhmlau @raymondfeng

achrinza · 2021-10-07T12:07:40Z

packages/repository/src/relations/relation.helpers.ts

-  scope = cloneDeep(scope);
-


[If we're keeping totalLimit] Since there's delete scope.totalLimit, can we keep the deep clone to prevent mutations to the user input?

oh yes, Good catch. That totally makes sense.

I pushed the change you suggested

collaorodrigo7 · 2021-10-13T02:31:59Z

Do you guys have any updates?

dhmlau · 2021-10-13T23:28:29Z

I'm not too familiar with how totalLimit works (or not work) in LoopBack3, so maybe @raymondfeng is a better person to comment.
IMHO, I think we can leave that option for now and land the PR if it's good to go. We can always add that option later.

dhmlau · 2021-10-14T18:09:26Z

@achrinza, do you have more review comments before landing this PR?
@collaorodrigo7, I noticed that DCO status is pending. I'm not sure if it's because one of the commits isn't signed (but i'd expect DCO to fail in that case).

collaorodrigo7 · 2021-10-14T18:13:01Z

Thanks, @dhmlau. I am confused about it too. I see that merge commit w/o the signature, but also it shows as no changes on that commit, so I am not sure what happened there.
Any suggestions?

dhmlau · 2021-10-14T18:15:04Z

It might make sense to squash the commits and see. WDYT?

collaorodrigo7 · 2021-10-14T18:32:16Z

It might make sense to squash the commits and see. WDYT?

I am not too familiar with squash, but it's not letting me squash it. I was able to change the commit message, does that help?

dhmlau · 2021-10-14T19:51:33Z

Seems like DCO is running now. Not sure if it's because of your changes. 🤷
Kicking off CI.

dhmlau · 2021-10-14T19:57:02Z

Actually there are quite a few commits in this PR, I'd suggest to squash. To do that, here is what I normally do:

In your branch

git remote add upstream https://github.com/loopbackio/loopback-next.git
git remote -v // optional - used to verify remotes you have setup
git fetch upstream
git rebase -i upstream/master 
// rebase + re-word / change commit message as usual and exit edit mode
// this is when you can change the action from 'pick' to 'fixup' 
git push origin +6832-include-scope-limit

If you grant me the permission, I can help you on this too.

If a scope filter is passed, the filter should be applied for each foreign key and not globally. This implies a query for each fk, and may impact performance. However, is the only way to respect the scope filter. Example, and include with scope.limit:2 should include 2 instances of the related model for each fk. Previously scope.limit:2 will cause to "only" include 2 related models. fix loopbackio#6832 Allow to set a totalLimit filter in scope to apply a limit globally in case someone relied on this bug thinking it was a feature. docs: add note to include with filters about totalLimit option Signed-off-by: Rodrigo Collao <collao.rodrigo7@gmail.com>

collaorodrigo7 · 2021-10-15T05:04:37Z

Thanks for the detail explanation @dhmlau, that was helpful.
I think I managed to do it, please check it whenever you have a chance.
In case I missed something, I already sent you an invite for the access so you can help me with please.

dhmlau · 2021-10-17T18:28:18Z

@collaorodrigo7, thanks for working through this. Your PR has landed! 🎉

collaorodrigo7 requested review from agnes512 and nabdelgadir as code owners October 3, 2021 00:40

collaorodrigo7 force-pushed the 6832-include-scope-limit branch from b256370 to 1407de6 Compare October 3, 2021 05:33

collaorodrigo7 force-pushed the 6832-include-scope-limit branch from 31bdfea to 4cb6999 Compare October 3, 2021 22:28

achrinza reviewed Oct 7, 2021

View reviewed changes

collaorodrigo7 requested a review from achrinza October 13, 2021 02:34

raymondfeng approved these changes Oct 14, 2021

View reviewed changes

collaorodrigo7 force-pushed the 6832-include-scope-limit branch 2 times, most recently from 4bce293 to a95d984 Compare October 14, 2021 18:31

collaorodrigo7 force-pushed the 6832-include-scope-limit branch from a95d984 to b8a5f33 Compare October 15, 2021 05:01

achrinza approved these changes Oct 16, 2021

View reviewed changes

achrinza requested a review from dhmlau October 16, 2021 06:20

dhmlau merged commit 5d1a8c7 into loopbackio:master Oct 17, 2021

dhmlau added the hacktoberfest-accepted label Oct 17, 2021

collaorodrigo7 deleted the 6832-include-scope-limit branch October 17, 2021 21:13

fix: make sure scope filters are used for each fk on includes #7965

fix: make sure scope filters are used for each fk on includes #7965

Uh oh!

Conversation

collaorodrigo7 commented Oct 3, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

collaorodrigo7 commented Oct 3, 2021

Uh oh!

mrmodise commented Oct 3, 2021

Uh oh!

collaorodrigo7 commented Oct 3, 2021

Uh oh!

collaorodrigo7 commented Oct 3, 2021

Uh oh!

collaorodrigo7 commented Oct 3, 2021

Uh oh!

achrinza commented Oct 3, 2021

Uh oh!

collaorodrigo7 commented Oct 3, 2021

Uh oh!

collaorodrigo7 commented Oct 4, 2021

Uh oh!

collaorodrigo7 commented Oct 4, 2021

Uh oh!

collaorodrigo7 commented Oct 5, 2021

Uh oh!

achrinza commented Oct 5, 2021

Uh oh!

coveralls commented Oct 5, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Test Coverage Report for Build 1331204950

Warning: This coverage report may be inaccurate.

Details

💛 - Coveralls

Uh oh!

mrbatista commented Oct 6, 2021

Uh oh!

collaorodrigo7 commented Oct 6, 2021

Uh oh!

achrinza left a comment

Choose a reason for hiding this comment

Uh oh!

achrinza Oct 7, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

collaorodrigo7 Oct 7, 2021

Choose a reason for hiding this comment

Uh oh!

collaorodrigo7 Oct 12, 2021

Choose a reason for hiding this comment

Uh oh!

collaorodrigo7 commented Oct 13, 2021

Uh oh!

dhmlau commented Oct 13, 2021

Uh oh!

dhmlau commented Oct 14, 2021

Uh oh!

collaorodrigo7 commented Oct 14, 2021

Uh oh!

dhmlau commented Oct 14, 2021

Uh oh!

collaorodrigo7 commented Oct 14, 2021

Uh oh!

dhmlau commented Oct 14, 2021

Uh oh!

dhmlau commented Oct 14, 2021

Uh oh!

collaorodrigo7 commented Oct 15, 2021

Uh oh!

dhmlau commented Oct 17, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

collaorodrigo7 commented Oct 3, 2021 •

edited

Loading

coveralls commented Oct 5, 2021 •

edited

Loading

achrinza Oct 7, 2021 •

edited

Loading