Skip to content

Commit

Permalink
fix: make sure scope filters are used for each fk on includes
Browse files Browse the repository at this point in the history 
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 loopbackio#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 <[email protected]>
  • Loading branch information
@collaorodrigo7
collaorodrigo7 committed Oct 15, 2021
1 parent ee2ef41 commit b8a5f33
Show file tree
Hide file tree
Showing 6 changed files with 209 additions and 12 deletions.
5 changes: 5 additions & 0 deletions docs/site/Include-filter.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
9 changes: 8 additions & 1 deletion packages/filter/src/query.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
};
}

/**
Expand Down
94 changes: 94 additions & 0 deletions packages/repository/src/__tests__/unit/repositories/legacy-juggler-bridge.unit.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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},
Expand Down
57 changes: 57 additions & 0 deletions ...repository/src/__tests__/unit/repositories/relations-helpers/find-by-foreign-keys.unit.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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%'},
},
});
});
});
5 changes: 4 additions & 1 deletion packages/repository/src/relations/has-many/has-many-through.inclusion-resolver.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
51 changes: 41 additions & 10 deletions packages/repository/src/relations/relation.helpers.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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>;
Expand Down

0 comments on commit b8a5f33

Please sign in to comment.