Fix shouldUseSkipTake to consider nested relations #378
Conversation
|
View your CI Pipeline Execution ↗ for commit ec39883.
☁️ Nx Cloud last updated this comment at |
| return false | ||
| } | ||
| const nestedFilter = filter[field] as Filter<unknown> | ||
|
|
||
| if (!relation || relation.isOneToOne || relation.isManyToOne) { | ||
| return false | ||
| // } else if (relation.isOneToMany) { | ||
| // TODO | ||
| // return false | ||
| } else { | ||
| return true | ||
| } | ||
| }).length > 0 | ||
| ) | ||
| return this.shouldUseSkipTake(nestedFilter, relation.inverseEntityMetadata.relations) | ||
| }) |
There was a problem hiding this comment.
This looks like a potential issue? What if the filter is flat, maybe we need to check if there is a nested filter?
There was a problem hiding this comment.
If the filter is flat I believe it would fall into the no relation case and exit above, we know this key represents a relational filter.
(I would also imagine this flat case is tested somewhere in the existing test suite and existing tests go green)
I think the easiest way to check on these would be to write the tests. Just point me at the best place to add something and maybe if we have tests with similar setups and I'll wrtie some up.
There was a problem hiding this comment.
That should be here: packages/query-typeorm/__tests__/query/filter-query.builder.spec.ts
There was a problem hiding this comment.
Added some tests.
Rebased the code to add tests, generate snapshots against the old code first.
See the diff after the change here: ec39883
If you'd like some more test cases describe them and let me know.
|
Thanks for the contribution! |
2 participants
Fixes: #377
@TriPSs Can you let me know where you think a test for this case should live and I'll add something to the repo.
Thanks to @AngusLeck for working on this solution.