fix(typeorm, #954): Filtering on relations with pagination

[mwoelk]

This pull request fixes the issues described in #954.

Contents

• 2 new e2e test cases in the "basic" example:
  • Test that filters on a relation without paging
  • Test that filters on a relation with additional paging (the breaking example)
• Fixed the filter-query-builder to use skip/take whenever joins are present. Otherwise it should stick with limit/offset for performance reasons
• Added a note in the docs (under /concepts/queries#paging) stating that there might be performance issues when filtering and paging relations with a large result set and a link to the issue for further reading.

Tests
All relevant tests passed on my machine.

Side Note
Just checked on current master (beb96c3) those unrelated tests fail on my machine:

• TypeOrmQueryService › #aggregate › call select with the aggregate columns and return the result
• TypeOrmQueryService › #aggregate › call select with the aggregate columns and return the result with a filter
• SequelizeQueryService › #aggregate › call select with the aggregate columns and return the result
• SequelizeQueryService › #aggregate › call select with the aggregate columns and return the result with a filter

they all have the same reason:

-     "dateType": StringMatching /2020-02-03/,
+     "dateType": "2020-02-02 23:00:00.000 +00:00",

[coveralls]

Pull Request Test Coverage Report for Build 638948748

• 11 of 11 (100.0%) changed or added relevant lines in 1 file are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage increased (+0.003%) to 97.257%

---

Totals
Change from base Build 636952663:	0.003%
Covered Lines:	4245
Relevant Lines:	4307

---

💛 - Coveralls

[mwoelk]

Wrong alert... Tried out the example by hand in the playground and had overlooked that one of the sub-items were moved during the tests 🤦 . Should work fine 😄

[mwoelk]

Just found out that there is another issue in typeorm that occurs when adding ordering to the whole pagination and filtering (see typeorm/typeorm#4742 (comment)):

"Something worth mentioning as well is the issue of using .orderBy() with .skip() and .take() which is also partially related to typeorm/typeorm#747. When you use .offset() and .limit() you should use .orderBy('entity.created_at', 'DESC') where created_at is the raw field name. When you decide to use .orderBy() with .skip() and .take() you should use .orderBy('entity.createdAt', 'DESC') where createdAt refers to the entity field name"

Question is whether this is a bug that needs to be fixed by typeorm or a workaround that should be included in this PR.

edit
Another recent typeorm issue related to this behaviour: typeorm/typeorm#5670

[doug-martin]

@mwoelk thank you for opening an issue and quickly diving in to create a PR!

I think this should be looked into this some more, as I think this would break some of our stuff internally because we do work with large result sets (well into the millions for some of our datasets), I haven't pulled it down yet to see the generated SQL but after reading through the issues it sounds like this removes the limit and offset and does it in memory?

I'd really like to see this fixed in TypeORM or look at how the query is being generated to account for it.

[mwoelk]

@doug-martin I think you should look into the request a bit. It fixes a a bug that breaks or-filtering on relation fields which is a quite common use-case. Otherwise or-filters should be disabled by default for relations because the behaviour can be very strange in combination with pagination.

I only changed the behaviour to skip/take when the filters lead to join-clauses being added. I think I could refine it even further to only use the skip/take when there are relations present in combination with or-filters and pagination.

The way I read it in the TypeORM discussion I mentioned it seems to be an issue they won't/can't fix because it comes down to the query itself (applying limit/offset in a way that joins with multiple result rows per top-entity are filtered somehow contradicts the way limit/offset works in queries).

[mwoelk]

Another way to handle it could be to use subqueries on the relation and limit them to just one result. Because only the existence of an item in a relation determines if the base entity should be in the result set. This could be overall faster than the join approach because it can stop as soon as one match was found. It also prevents the issue in a way that each entity should occur only once in the query result. But it might increase the complexity of the queries by a lot. I would have to take a closer look into it and also compare performance.

[doug-martin]

@mwoelk thank you so much for opening this PR. I pulled your changes down and tested everything out, your solution works great! I had a small misunderstanding, going back and reading through the issue again I see that it just fetches the primary keys with paging applied, while slightly inefficient I think this is a fine solution and keeps this package from having to having to create any more custom queries.

I cleaned it up just a bit and moved the tests to typeorm-query.service.spec.ts for each relation type oneToOne, manyToOne, oneToMany, and manyToMany. One the tests pass Ill merge and release.

Thanks again!

[doug-martin]

Fixed under ##977