fix: make sure scope filters are used for each fk on includes

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

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<AnyObject>;
+  scope?: Filter<AnyObject> & {
+    /**
+     * 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;
+  };
 }
 
 /**

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},

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%'},
+      },
+    });
+  });
 });

packages/repository/src/relations/has-many/has-many-through.inclusion-resolver.ts

@@ -118,7 +118,10 @@ export function createHasManyThroughInclusionResolver<
           Target,
           TargetRelations,
           StringKeyOf<Target>
-        >(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

packages/repository/src/relations/relation.helpers.ts

@@ -35,29 +35,60 @@ export async function findByForeignKeys<
   targetRepository: EntityCrudRepository<Target, unknown, TargetRelations>,
   fkName: ForeignKey,
   fkValues: Target[ForeignKey][] | Target[ForeignKey],
-  scope?: Filter<Target>,
+  scope?: Filter<Target> & {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<Target>;
-
-  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<Target>;
+      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<Target>;
-  }
+    const where = {[fkName]: value} as unknown as Where<Target>;
 
-  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<Target>;
+    }
+
+    return targetRepository.find(scope, options);
+  }
 }
 
 export type StringKeyOf<T> = Extract<keyof T, string>;