Skip to content

Fix shouldUseSkipTake to consider nested relations #378

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
May 13, 2025
Merged

Fix shouldUseSkipTake to consider nested relations #378

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
90e4074
fix: describe indentation
Smtih May 12, 2025
c884e9e
test: add tests for nested relation cases
Smtih May 13, 2025
ec39883
fix: shouldUseSkipTake to handle nested cases
Smtih May 9, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
81 changes: 77 additions & 4 deletions packages/query-typeorm/__tests__/query/__snapshots__/filter-query.builder.spec.ts.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -113,7 +113,7 @@ OFFSET
11"
`;

exports[`FilterQueryBuilder #select with paging should use limit/offset when filtering on many to one relation 1`] = `
exports[`FilterQueryBuilder #select with paging skip/take - limit/offset should use limit/offset when filtering on many to one relation 1`] = `
"SELECT
"TestEntity"."test_entity_pk" AS "TestEntity_test_entity_pk",
"TestEntity"."string_type" AS "TestEntity_string_type",
Expand All @@ -132,7 +132,7 @@ OFFSET
3"
`;

exports[`FilterQueryBuilder #select with paging should use limit/offset when filtering on one to one relation 1`] = `
exports[`FilterQueryBuilder #select with paging skip/take - limit/offset should use limit/offset when filtering on nested many to one relation 1`] = `
"SELECT
"TestEntity"."test_entity_pk" AS "TestEntity_test_entity_pk",
"TestEntity"."string_type" AS "TestEntity_string_type",
Expand All @@ -145,13 +145,14 @@ exports[`FilterQueryBuilder #select with paging should use limit/offset when fil
FROM
"test_entity" "TestEntity"
LEFT JOIN "test_relation" "oneTestRelation" ON "oneTestRelation"."test_relation_pk" = "TestEntity"."oneTestRelationTestRelationPk"
LEFT JOIN "test_entity" "testEntity" ON "testEntity"."test_entity_pk" = "oneTestRelation"."test_entity_id"
LIMIT
10
OFFSET
3"
`;

exports[`FilterQueryBuilder #select with paging should use skip/take when filtering on one to many relation 1`] = `
exports[`FilterQueryBuilder #select with paging skip/take - limit/offset should use limit/offset when filtering on nested one to one relation 1`] = `
"SELECT
"TestEntity"."test_entity_pk" AS "TestEntity_test_entity_pk",
"TestEntity"."string_type" AS "TestEntity_string_type",
Expand All @@ -163,7 +164,31 @@ exports[`FilterQueryBuilder #select with paging should use skip/take when filter
"TestEntity"."oneTestRelationTestRelationPk" AS "TestEntity_oneTestRelationTestRelationPk"
FROM
"test_entity" "TestEntity"
LEFT JOIN "test_relation" "testRelations" ON "testRelations"."test_entity_id" = "TestEntity"."test_entity_pk""
LEFT JOIN "test_relation" "oneTestRelation" ON "oneTestRelation"."test_relation_pk" = "TestEntity"."oneTestRelationTestRelationPk"
LEFT JOIN "test_entity" "oneTestEntity" ON "oneTestEntity"."oneTestRelationTestRelationPk" = "oneTestRelation"."test_relation_pk"
LIMIT
10
OFFSET
3"
`;

exports[`FilterQueryBuilder #select with paging skip/take - limit/offset should use limit/offset when filtering on one to one relation 1`] = `
"SELECT
"TestEntity"."test_entity_pk" AS "TestEntity_test_entity_pk",
"TestEntity"."string_type" AS "TestEntity_string_type",
"TestEntity"."bool_type" AS "TestEntity_bool_type",
"TestEntity"."number_type" AS "TestEntity_number_type",
"TestEntity"."date_type" AS "TestEntity_date_type",
"TestEntity"."many_to_one_relation_id" AS "TestEntity_many_to_one_relation_id",
"TestEntity"."many_to_one_soft_delete_relation_id" AS "TestEntity_many_to_one_soft_delete_relation_id",
"TestEntity"."oneTestRelationTestRelationPk" AS "TestEntity_oneTestRelationTestRelationPk"
FROM
"test_entity" "TestEntity"
LEFT JOIN "test_relation" "oneTestRelation" ON "oneTestRelation"."test_relation_pk" = "TestEntity"."oneTestRelationTestRelationPk"
LIMIT
10
OFFSET
3"
`;

exports[`FilterQueryBuilder #select with paging skip/take - limit/offset should use skip/take when filtering on many to many relation 1`] = `
Expand All @@ -182,6 +207,54 @@ FROM
LEFT JOIN "test_relation" "manyTestRelations" ON "manyTestRelations"."test_relation_pk" = "TestEntity_manyTestRelations"."testRelationTestRelationPk""
`;

exports[`FilterQueryBuilder #select with paging skip/take - limit/offset should use skip/take when filtering on nested many to many relation 1`] = `
"SELECT
"TestEntity"."test_entity_pk" AS "TestEntity_test_entity_pk",
"TestEntity"."string_type" AS "TestEntity_string_type",
"TestEntity"."bool_type" AS "TestEntity_bool_type",
"TestEntity"."number_type" AS "TestEntity_number_type",
"TestEntity"."date_type" AS "TestEntity_date_type",
"TestEntity"."many_to_one_relation_id" AS "TestEntity_many_to_one_relation_id",
"TestEntity"."many_to_one_soft_delete_relation_id" AS "TestEntity_many_to_one_soft_delete_relation_id",
"TestEntity"."oneTestRelationTestRelationPk" AS "TestEntity_oneTestRelationTestRelationPk"
FROM
"test_entity" "TestEntity"
LEFT JOIN "test_relation" "oneTestRelation" ON "oneTestRelation"."test_relation_pk" = "TestEntity"."oneTestRelationTestRelationPk"
LEFT JOIN "test_entity_many_test_relations_test_relation" "manyTestEntities_oneTestRelation" ON "manyTestEntities_oneTestRelation"."testRelationTestRelationPk" = "oneTestRelation"."test_relation_pk"
LEFT JOIN "test_entity" "manyTestEntities" ON "manyTestEntities"."test_entity_pk" = "manyTestEntities_oneTestRelation"."testEntityTestEntityPk""
`;

exports[`FilterQueryBuilder #select with paging skip/take - limit/offset should use skip/take when filtering on nested one to many relation 1`] = `
"SELECT
"TestEntity"."test_entity_pk" AS "TestEntity_test_entity_pk",
"TestEntity"."string_type" AS "TestEntity_string_type",
"TestEntity"."bool_type" AS "TestEntity_bool_type",
"TestEntity"."number_type" AS "TestEntity_number_type",
"TestEntity"."date_type" AS "TestEntity_date_type",
"TestEntity"."many_to_one_relation_id" AS "TestEntity_many_to_one_relation_id",
"TestEntity"."many_to_one_soft_delete_relation_id" AS "TestEntity_many_to_one_soft_delete_relation_id",
"TestEntity"."oneTestRelationTestRelationPk" AS "TestEntity_oneTestRelationTestRelationPk"
FROM
"test_entity" "TestEntity"
LEFT JOIN "test_relation" "oneTestRelation" ON "oneTestRelation"."test_relation_pk" = "TestEntity"."oneTestRelationTestRelationPk"
LEFT JOIN "test_entity_relation_entity" "testEntityRelation" ON "testEntityRelation"."test_relation_id" = "oneTestRelation"."test_relation_pk""
`;

exports[`FilterQueryBuilder #select with paging skip/take - limit/offset should use skip/take when filtering on one to many relation 1`] = `
"SELECT
"TestEntity"."test_entity_pk" AS "TestEntity_test_entity_pk",
"TestEntity"."string_type" AS "TestEntity_string_type",
"TestEntity"."bool_type" AS "TestEntity_bool_type",
"TestEntity"."number_type" AS "TestEntity_number_type",
"TestEntity"."date_type" AS "TestEntity_date_type",
"TestEntity"."many_to_one_relation_id" AS "TestEntity_many_to_one_relation_id",
"TestEntity"."many_to_one_soft_delete_relation_id" AS "TestEntity_many_to_one_soft_delete_relation_id",
"TestEntity"."oneTestRelationTestRelationPk" AS "TestEntity_oneTestRelationTestRelationPk"
FROM
"test_entity" "TestEntity"
LEFT JOIN "test_relation" "testRelations" ON "testRelations"."test_entity_id" = "TestEntity"."test_entity_pk""
`;

exports[`FilterQueryBuilder #select with relation should select and map relation 1`] = `
"SELECT
"TestEntity"."test_entity_pk" AS "TestEntity_test_entity_pk",
Expand Down
132 changes: 99 additions & 33 deletions packages/query-typeorm/__tests__/query/filter-query.builder.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -314,45 +314,111 @@ describe('FilterQueryBuilder', (): void => {
instance(mockWhereBuilder)
)
})
})

it('should use skip/take when filtering on one to many relation', () => {
const mockWhereBuilder = mock<WhereBuilder<TestEntity>>(WhereBuilder)
when(mockWhereBuilder.build(anything(), anything(), anything(), anything())).thenCall((qb: WhereExpressionBuilder) => qb)
it('should use skip/take when filtering on nested many to many relation', () => {
const mockWhereBuilder = mock<WhereBuilder<TestEntity>>(WhereBuilder)
when(mockWhereBuilder.build(anything(), anything(), anything(), anything())).thenCall(
(qb: WhereExpressionBuilder) => qb
)

expectSelectSQLSnapshot(
{
paging: { limit: 10, offset: 3 },
filter: { testRelations: { testRelationPk: { eq: 'test' } } }
},
instance(mockWhereBuilder)
)
})
expectSelectSQLSnapshot(
{
paging: { limit: 10, offset: 3 },
filter: { oneTestRelation: { manyTestEntities: { testEntityPk: { eq: 'test' } } } }
},
instance(mockWhereBuilder)
)
})

it('should use limit/offset when filtering on one to one relation', () => {
const mockWhereBuilder = mock<WhereBuilder<TestEntity>>(WhereBuilder)
when(mockWhereBuilder.build(anything(), anything(), anything(), anything())).thenCall((qb: WhereExpressionBuilder) => qb)
it('should use skip/take when filtering on one to many relation', () => {
const mockWhereBuilder = mock<WhereBuilder<TestEntity>>(WhereBuilder)
when(mockWhereBuilder.build(anything(), anything(), anything(), anything())).thenCall(
(qb: WhereExpressionBuilder) => qb
)

expectSelectSQLSnapshot(
{
paging: { limit: 10, offset: 3 },
filter: { oneTestRelation: { testRelationPk: { eq: 'test' } } }
},
instance(mockWhereBuilder)
)
})
expectSelectSQLSnapshot(
{
paging: { limit: 10, offset: 3 },
filter: { testRelations: { testRelationPk: { eq: 'test' } } }
},
instance(mockWhereBuilder)
)
})

it('should use limit/offset when filtering on many to one relation', () => {
const mockWhereBuilder = mock<WhereBuilder<TestEntity>>(WhereBuilder)
when(mockWhereBuilder.build(anything(), anything(), anything(), anything())).thenCall((qb: WhereExpressionBuilder) => qb)
it('should use skip/take when filtering on nested one to many relation', () => {
const mockWhereBuilder = mock<WhereBuilder<TestEntity>>(WhereBuilder)
when(mockWhereBuilder.build(anything(), anything(), anything(), anything())).thenCall(
(qb: WhereExpressionBuilder) => qb
)

expectSelectSQLSnapshot(
{
paging: { limit: 10, offset: 3 },
filter: { manyToOneRelation: { testRelationPk: { eq: 'test' } } }
},
instance(mockWhereBuilder)
)
expectSelectSQLSnapshot(
{
paging: { limit: 10, offset: 3 },
filter: { oneTestRelation: { testEntityRelation: { testRelationId: { eq: 'test' } } } }
},
instance(mockWhereBuilder)
)
})

it('should use limit/offset when filtering on one to one relation', () => {
const mockWhereBuilder = mock<WhereBuilder<TestEntity>>(WhereBuilder)
when(mockWhereBuilder.build(anything(), anything(), anything(), anything())).thenCall(
(qb: WhereExpressionBuilder) => qb
)

expectSelectSQLSnapshot(
{
paging: { limit: 10, offset: 3 },
filter: { oneTestRelation: { testRelationPk: { eq: 'test' } } }
},
instance(mockWhereBuilder)
)
})

it('should use limit/offset when filtering on nested one to one relation', () => {
const mockWhereBuilder = mock<WhereBuilder<TestEntity>>(WhereBuilder)
when(mockWhereBuilder.build(anything(), anything(), anything(), anything())).thenCall(
(qb: WhereExpressionBuilder) => qb
)

expectSelectSQLSnapshot(
{
paging: { limit: 10, offset: 3 },
filter: { oneTestRelation: { oneTestEntity: { testEntityPk: { eq: 'test' } } } }
},
instance(mockWhereBuilder)
)
})

it('should use limit/offset when filtering on many to one relation', () => {
const mockWhereBuilder = mock<WhereBuilder<TestEntity>>(WhereBuilder)
when(mockWhereBuilder.build(anything(), anything(), anything(), anything())).thenCall(
(qb: WhereExpressionBuilder) => qb
)

expectSelectSQLSnapshot(
{
paging: { limit: 10, offset: 3 },
filter: { manyToOneRelation: { testRelationPk: { eq: 'test' } } }
},
instance(mockWhereBuilder)
)
})

it('should use limit/offset when filtering on nested many to one relation', () => {
const mockWhereBuilder = mock<WhereBuilder<TestEntity>>(WhereBuilder)
when(mockWhereBuilder.build(anything(), anything(), anything(), anything())).thenCall(
(qb: WhereExpressionBuilder) => qb
)

expectSelectSQLSnapshot(
{
paging: { limit: 10, offset: 3 },
filter: { oneTestRelation: { testEntity: { testEntityPk: { eq: 'test' } } } }
},
instance(mockWhereBuilder)
)
})
})
})

Expand Down
37 changes: 16 additions & 21 deletions packages/query-typeorm/src/query/filter-query.builder.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ import {
UpdateQueryBuilder,
WhereExpressionBuilder
} from 'typeorm'
import { RelationMetadata } from 'typeorm/metadata/RelationMetadata'
import { SoftDeleteQueryBuilder } from 'typeorm/query-builder/SoftDeleteQueryBuilder'

import { AggregateBuilder } from './aggregate.builder'
Expand Down Expand Up @@ -327,31 +328,25 @@ export class FilterQueryBuilder<Entity> {
}

/**
* Checks if the query should use skip/take instead of limit/offset
* Checks if the query should use skip/take instead of limit/offset.
*
* We need to use Skip/Take instead of Limit/Offset when the query involves a join that might be (one|many)-to-many.
* This method looks for any n-to-many relations in the filter and if it finds any, it returns true.
*
* Recursively traverses the filter so we can detect nested n-to-many relations.
*/
private shouldUseSkipTake(filter?: Filter<Entity>): boolean {
if (!filter) {
return false
}
private shouldUseSkipTake<T>(filter?: Filter<T>, relations: RelationMetadata[] = this.repo.metadata.relations): boolean {
if (!filter) return false

return (
getFilterFields(filter).filter((field) => {
const relation = this.repo.metadata.relations.find(({ propertyName }) => propertyName === field)
return getFilterFields(filter).some((field) => {
const relation = relations.find(({ propertyName }) => propertyName === field)
if (!relation) return false
if (!relation.isOneToOne && !relation.isManyToOne) return true

if (!relation) {
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)
})
Comment on lines -342 to +349
Copy link
Owner

@TriPSs TriPSs May 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks like a potential issue? What if the filter is flat, maybe we need to check if there is a nested filter?

Copy link
Contributor Author

@Smtih Smtih May 11, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Owner

@TriPSs TriPSs May 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That should be here: packages/query-typeorm/__tests__/query/filter-query.builder.spec.ts

Copy link
Contributor Author

@Smtih Smtih May 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}

public getReferencedRelationsWithAliasRecursive(
Expand Down
Loading