From b8a5f33b8bac7b3544c3f78ea6946d4caf9ff8cd 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. fix #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 --- docs/site/Include-filter.md | 5 + packages/filter/src/query.ts | 9 +- .../legacy-juggler-bridge.unit.ts | 94 +++++++++++++++++++ .../find-by-foreign-keys.unit.ts | 57 +++++++++++ .../has-many-through.inclusion-resolver.ts | 5 +- .../src/relations/relation.helpers.ts | 51 ++++++++-- 6 files changed, 209 insertions(+), 12 deletions(-) diff --git a/docs/site/Include-filter.md b/docs/site/Include-filter.md index 55b9c6bf763e..3dda16f8d187 100644 --- a/docs/site/Include-filter.md +++ b/docs/site/Include-filter.md @@ -284,6 +284,11 @@ await postRepository.findById('123', { }); ``` +{% include important.html content="The `limit` filter will be applied on a per parent record basis. So each parent record will include a max of `limit` number of records. +Previously, we had a bug where `limit` will be applied globally, so not all parent's records will include related objects depending on the `limit` value. +To keep backward compatability with this, in the weird case it is needed, you can use `totalLimit` instead. For more details [see here](https://github.com/loopbackio/loopback-next/issues/6832) +" %} + #### Access included objects In the Node.js API, you can simply access the returned model instance with diff --git a/packages/filter/src/query.ts b/packages/filter/src/query.ts index 417ec0263330..a1a63c31b913 100644 --- a/packages/filter/src/query.ts +++ b/packages/filter/src/query.ts @@ -181,7 +181,14 @@ export interface Inclusion { // but that won't handle second-level (and deeper) inclusions. // To keep things simple, we allow any filter here, effectively turning off // compiler checks. - scope?: Filter; + scope?: Filter & { + /** + * Global maximum number of inclusions. This is just to remain backward + * compatability. This totalLimit props takes precedence over limit + * https://github.com/loopbackio/loopback-next/issues/6832 + */ + totalLimit?: number; + }; } /** 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..f7664e05a8c0 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,100 @@ 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.find() with included models and scope totalLimit', 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: {totalLimit: 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])], + }, + { + ...createdFolders[2].toJSON(), + // files: [], + }, + { + ...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..6b86fcf1c16c 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,61 @@ 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%'}, + }, + }); + }); + it('runs find globally because totalLimit is set in scope', async () => { + const find = productRepo.stubs.find; + const scope = { + totalLimit: 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: {inq: [1, 2]}, + name: {like: 'product%'}, + }, + }); + }); }); diff --git a/packages/repository/src/relations/has-many/has-many-through.inclusion-resolver.ts b/packages/repository/src/relations/has-many/has-many-through.inclusion-resolver.ts index de51ef99dc19..97ae55f318df 100644 --- a/packages/repository/src/relations/has-many/has-many-through.inclusion-resolver.ts +++ b/packages/repository/src/relations/has-many/has-many-through.inclusion-resolver.ts @@ -118,7 +118,10 @@ export function createHasManyThroughInclusionResolver< Target, TargetRelations, StringKeyOf - >(targetRepo, targetKey, targetIds as unknown as [], scope, options); + >(targetRepo, targetKey, targetIds as unknown as [], scope, { + ...options, + isThroughModelInclude: true, + }); result.push(targetEntityList); } else { // no entities found, add undefined to results diff --git a/packages/repository/src/relations/relation.helpers.ts b/packages/repository/src/relations/relation.helpers.ts index c1acd88f58f6..318df29f116a 100644 --- a/packages/repository/src/relations/relation.helpers.ts +++ b/packages/repository/src/relations/relation.helpers.ts @@ -35,29 +35,60 @@ export async function findByForeignKeys< targetRepository: EntityCrudRepository, fkName: ForeignKey, fkValues: Target[ForeignKey][] | Target[ForeignKey], - scope?: Filter, + scope?: Filter & {totalLimit?: number}, 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}; } else { value = fkValues; } + let useScopeFilterGlobally = false; + if (options) { + useScopeFilterGlobally = options.isThroughModelInclude; + //if its an include from a through model, fkValues will be an array + //however, in this case we DO want to use the scope in the entire query + //no in a per fk basis + } + //This code is to keep backward compatability. See https://github.com/loopbackio/loopback-next/issues/6832 + //for more info + if (scope?.totalLimit) { + scope.limit = scope.totalLimit; + useScopeFilterGlobally = true; + delete scope.totalLimit; + } - 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) && !useScopeFilterGlobally) { + // 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 _.flatten(findResults); + }); } else { - scope = {where} as Filter; - } + const where = {[fkName]: value} as unknown as Where; - return targetRepository.find(scope, options); + 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); + } } export type StringKeyOf = Extract;