From 63be78e64bb39ad6a40d4e4706981e7e76238d95 Mon Sep 17 00:00:00 2001 From: Rodrigo Collao Date: Sat, 2 Oct 2021 19:28:30 -0500 Subject: [PATCH] fix: make sure scope filters are used for each fk on includes 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 fix #6832 Signed-off-by: Rodrigo Collao --- .../legacy-juggler-bridge.unit.ts | 47 +++++++++++++++++++ .../find-by-foreign-keys.unit.ts | 33 +++++++++++++ .../src/relations/relation.helpers.ts | 37 +++++++++++---- 3 files changed, 107 insertions(+), 10 deletions(-) diff --git a/packages/repository/src/__tests__/unit/repositories/legacy-juggler-bridge.unit.ts b/packages/repository/src/__tests__/unit/repositories/legacy-juggler-bridge.unit.ts index 05c670dc9251..a78c952f74dc 100644 --- a/packages/repository/src/__tests__/unit/repositories/legacy-juggler-bridge.unit.ts +++ b/packages/repository/src/__tests__/unit/repositories/legacy-juggler-bridge.unit.ts @@ -475,6 +475,53 @@ describe('DefaultCrudRepository', () => { ]); }); + it('implements Repository.find() with included models and scope limit', async () => { + const createdFolders = await folderRepo.createAll([ + {name: 'f1', id: 1}, + {name: 'f2', id: 2}, + {name: 'f3', id: 3}, + {name: 'f4', id: 4}, + ]); + const files = await fileRepo.createAll([ + {id: 1, title: 'file1', folderId: 1}, + {id: 2, title: 'file3.A', folderId: 3}, + {id: 3, title: 'file3.D', folderId: 3}, + {id: 4, title: 'file3.C', folderId: 3}, + {id: 5, title: 'file3.B', folderId: 3}, + {id: 6, title: 'file2.D', folderId: 2}, + {id: 7, title: 'file2.A', folderId: 2}, + {id: 8, title: 'file2.C', folderId: 2}, + {id: 9, title: 'file2.B', folderId: 2}, + ]); + + folderRepo.registerInclusionResolver( + 'files', + folderFiles.inclusionResolver, + ); + + const folders = await folderRepo.find({ + include: [ + {relation: 'files', scope: {limit: 3, order: ['title ASC']}}, + ], + }); + + expect(toJSON(folders)).to.deepEqual([ + {...createdFolders[0].toJSON(), files: [toJSON(files[0])]}, + { + ...createdFolders[1].toJSON(), + files: [toJSON(files[6]), toJSON(files[8]), toJSON(files[7])], + }, + { + ...createdFolders[2].toJSON(), + files: [toJSON(files[1]), toJSON(files[4]), toJSON(files[3])], + }, + { + ...createdFolders[3].toJSON(), + // files: [], //? I would prefer to have files as an empty array but I think that would be a change to flattenTargetsOfOneToManyRelation? + }, + ]); + }); + it('implements Repository.findById() with included models', async () => { const folders = await folderRepo.createAll([ {name: 'f1', id: 1}, diff --git a/packages/repository/src/__tests__/unit/repositories/relations-helpers/find-by-foreign-keys.unit.ts b/packages/repository/src/__tests__/unit/repositories/relations-helpers/find-by-foreign-keys.unit.ts index a5865a7564b6..3cc97466dfab 100644 --- a/packages/repository/src/__tests__/unit/repositories/relations-helpers/find-by-foreign-keys.unit.ts +++ b/packages/repository/src/__tests__/unit/repositories/relations-helpers/find-by-foreign-keys.unit.ts @@ -171,4 +171,37 @@ describe('findByForeignKeys', () => { expect(fkValues).to.deepEqual(fkValuesOriginal); expect(scope).to.deepEqual(scopeOriginal); }); + + it('runs a find for each fkValue to properly respect scope filters', async () => { + const find = productRepo.stubs.find; + const scope = { + limit: 4, + order: ['name ASC'], + where: {name: {like: 'product%'}}, + }; + const newproducts = [ + createProduct({id: 1, name: 'productA', categoryId: 1}), + createProduct({id: 2, name: 'productB', categoryId: 2}), + ]; + await productRepo.create(newproducts[0]); + await productRepo.create(newproducts[1]); + find.resolves([]); + await findByForeignKeys(productRepo, 'categoryId', [1, 2], scope); + sinon.assert.calledWithMatch(find, { + limit: 4, + order: ['name ASC'], + where: { + categoryId: 1, + name: {like: 'product%'}, + }, + }); + sinon.assert.calledWithMatch(find, { + limit: 4, + order: ['name ASC'], + where: { + categoryId: 2, + name: {like: 'product%'}, + }, + }); + }); }); diff --git a/packages/repository/src/relations/relation.helpers.ts b/packages/repository/src/relations/relation.helpers.ts index c1acd88f58f6..7f269efc69a0 100644 --- a/packages/repository/src/relations/relation.helpers.ts +++ b/packages/repository/src/relations/relation.helpers.ts @@ -39,8 +39,6 @@ export async function findByForeignKeys< options?: Options, ): Promise<(Target & TargetRelations)[]> { let value; - scope = cloneDeep(scope); - if (Array.isArray(fkValues)) { if (fkValues.length === 0) return []; value = fkValues.length === 1 ? fkValues[0] : {inq: fkValues}; @@ -48,16 +46,35 @@ export async function findByForeignKeys< value = fkValues; } - const where = {[fkName]: value} as unknown as Where; - - if (scope && !_.isEmpty(scope)) { - // combine where clause to scope filter - scope = new FilterBuilder(scope).impose({where}).filter; + const isScopeSet = scope && !_.isEmpty(scope); + if (isScopeSet && Array.isArray(fkValues)) { + // since there is a scope, there could be a where filter, a limit, an order + // and we should run the scope in multiple queries so we can respect the + // scope filter params + const findPromises = fkValues.map(fk => { + const where = {[fkName]: fk} as unknown as Where; + let localScope = cloneDeep(scope); + // combine where clause to scope filter + localScope = new FilterBuilder(localScope).impose({where}).filter; + return targetRepository.find(localScope, options); + }); + return Promise.all(findPromises).then(findResults => { + //findResults is an array of arrays for each scope result, so we need to flatten it before returning it + return findResults.flat(); + }); } else { - scope = {where} as Filter; - } + scope = cloneDeep(scope); + const where = {[fkName]: value} as unknown as Where; + + if (isScopeSet) { + // combine where clause to scope filter + scope = new FilterBuilder(scope).impose({where}).filter; + } else { + scope = {where} as Filter; + } - return targetRepository.find(scope, options); + return targetRepository.find(scope, options); + } } export type StringKeyOf = Extract;