Skip to content

Commit

Permalink
fix: relation query count regression
Browse files Browse the repository at this point in the history 
fixes: #8074
Signed-off-by: Rifa Achrinza <25147899+achrinza@users.noreply.github.com>
  • Loading branch information
@achrinza
achrinza committed Dec 17, 2021
1 parent 37a576e commit f5384a0
Show file tree
Hide file tree
Showing 2 changed files with 122 additions and 28 deletions.
129 changes: 107 additions & 22 deletions ...repository/src/__tests__/unit/repositories/relations-helpers/find-by-foreign-keys.unit.ts
View file
Expand Up @@ -3,6 +3,7 @@
// This file is licensed under the MIT License.
// License text available at https://opensource.org/licenses/MIT

import {Filter} from '@loopback/filter';
import {
createStubInstance,
expect,
Expand All @@ -12,6 +13,7 @@ import {
import {cloneDeep} from 'lodash';
import {findByForeignKeys} from '../../../..';
import {
Category,
createProduct,
Product,
ProductRepository,
Expand All @@ -31,7 +33,11 @@ describe('findByForeignKeys', () => {

const fkIds: number[] = [];
await productRepo.create({id: 1, name: 'product', categoryId: 1});
const products = await findByForeignKeys(productRepo, 'categoryId', fkIds);
const products = await findByForeignKeys<Product, Category, 'categoryId'>(
productRepo,
'categoryId',
fkIds,
);
expect(products).to.be.empty();

sinon.assert.notCalled(productRepo.stubs.find);
Expand All @@ -41,7 +47,11 @@ describe('findByForeignKeys', () => {
const find = productRepo.stubs.find;
find.resolves([]);
await productRepo.create({id: 1, name: 'product', categoryId: 1});
const products = await findByForeignKeys(productRepo, 'categoryId', 2);
const products = await findByForeignKeys<Product, Category, 'categoryId'>(
productRepo,
'categoryId',
2,
);
expect(products).to.be.empty();
sinon.assert.calledWithMatch(find, {});
});
Expand All @@ -50,7 +60,11 @@ describe('findByForeignKeys', () => {
const find = productRepo.stubs.find;
find.resolves([]);
await productRepo.create({id: 1, name: 'product', categoryId: 1});
const products = await findByForeignKeys(productRepo, 'categoryId', [2, 3]);
const products = await findByForeignKeys<Product, Category, 'categoryId'>(
productRepo,
'categoryId',
[2, 3],
);
expect(products).to.be.empty();
sinon.assert.calledWithMatch(find, {});
});
Expand All @@ -61,7 +75,11 @@ describe('findByForeignKeys', () => {
const pencil = createProduct({name: 'pencil', categoryId: 1});
find.resolves([pen, pencil]);

const products = await findByForeignKeys(productRepo, 'categoryId', 1);
const products = await findByForeignKeys<Product, Category, 'categoryId'>(
productRepo,
'categoryId',
1,
);
expect(products).to.deepEqual([pen, pencil]);

sinon.assert.calledWithMatch(find, {
Expand All @@ -76,7 +94,11 @@ describe('findByForeignKeys', () => {
const pen = await productRepo.create({name: 'pen', categoryId: 1});
const pencil = await productRepo.create({name: 'pencil', categoryId: 2});
find.resolves([pen]);
const products = await findByForeignKeys(productRepo, 'categoryId', 1);
const products = await findByForeignKeys<Product, Category, 'categoryId'>(
productRepo,
'categoryId',
1,
);
expect(products).to.deepEqual([pen]);
expect(products).to.not.containDeep(pencil);
sinon.assert.calledWithMatch(find, {
Expand All @@ -91,7 +113,11 @@ describe('findByForeignKeys', () => {
const pen = await productRepo.create({name: 'pen', categoryId: 1});
const pencil = await productRepo.create({name: 'pencil', categoryId: 2});
find.resolves([pencil]);
const products = await findByForeignKeys(productRepo, 'categoryId', [2]);
const products = await findByForeignKeys<Product, Category, 'categoryId'>(
productRepo,
'categoryId',
[2],
);
expect(products).to.deepEqual([pencil]);
expect(products).to.not.containDeep(pen);
sinon.assert.calledWithMatch(find, {
Expand All @@ -107,7 +133,11 @@ describe('findByForeignKeys', () => {
const paper = createProduct({name: 'paper', categoryId: 3});
const find = productRepo.stubs.find;
find.resolves([pen, paper]);
const products = await findByForeignKeys(productRepo, 'categoryId', [1, 3]);
const products = await findByForeignKeys<Product, Category, 'categoryId'>(
productRepo,
'categoryId',
[1, 3],
);
expect(products).to.deepEqual([pen, paper]);
expect(products).to.not.containDeep(pencil);
sinon.assert.calledWithMatch(find, {
Expand All @@ -122,7 +152,7 @@ describe('findByForeignKeys', () => {
it('does not throw an error if scope is passed in and is undefined or empty', async () => {
const find = productRepo.stubs.find;
find.resolves([]);
let products = await findByForeignKeys(
let products = await findByForeignKeys<Product, Category, 'categoryId'>(
productRepo,
'categoryId',
[1],
Expand All @@ -131,7 +161,13 @@ describe('findByForeignKeys', () => {
);
expect(products).to.be.empty();
sinon.assert.calledWithMatch(find, {});
products = await findByForeignKeys(productRepo, 'categoryId', 1, {}, {});
products = await findByForeignKeys<Product, Category, 'categoryId'>(
productRepo,
'categoryId',
1,
{},
{},
);
expect(products).to.be.empty();
sinon.assert.calledWithMatch(find, {});
});
Expand All @@ -141,10 +177,15 @@ describe('findByForeignKeys', () => {
find.resolves([]);
await productRepo.create({id: 1, name: 'product', categoryId: 1});
await productRepo.create({id: 2, name: 'product', categoryId: 1});
await findByForeignKeys(productRepo, 'categoryId', 1, {
where: {id: 2},
include: ['nested inclusion'],
});
await findByForeignKeys<Product, Category, 'categoryId'>(
productRepo,
'categoryId',
1,
{
where: {id: 2},
include: ['nested inclusion'],
},
);

sinon.assert.calledWithMatch(find, {
where: {
Expand All @@ -166,7 +207,12 @@ describe('findByForeignKeys', () => {
productRepo.stubs.find.resolves([]);
await productRepo.create({id: 1, name: 'product', categoryId: 1});
await productRepo.create({id: 2, name: 'product', categoryId: 1});
await findByForeignKeys(productRepo, 'categoryId', fkValues, scope);
await findByForeignKeys<Product, Category, 'categoryId'>(
productRepo,
'categoryId',
fkValues,
scope,
);

expect(fkValues).to.deepEqual(fkValuesOriginal);
expect(scope).to.deepEqual(scopeOriginal);
Expand All @@ -179,14 +225,19 @@ describe('findByForeignKeys', () => {
order: ['name ASC'],
where: {name: {like: 'product%'}},
};
const newproducts = [
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]);
await productRepo.create(newProducts[0]);
await productRepo.create(newProducts[1]);
find.resolves([]);
await findByForeignKeys(productRepo, 'categoryId', [1, 2], scope);
await findByForeignKeys<Product, Category, 'categoryId'>(
productRepo,
'categoryId',
[1, 2],
scope,
);
sinon.assert.calledWithMatch(find, {
limit: 4,
order: ['name ASC'],
Expand All @@ -211,14 +262,19 @@ describe('findByForeignKeys', () => {
order: ['name ASC'],
where: {name: {like: 'product%'}},
};
const newproducts = [
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]);
await productRepo.create(newProducts[0]);
await productRepo.create(newProducts[1]);
find.resolves([]);
await findByForeignKeys(productRepo, 'categoryId', [1, 2], scope);
await findByForeignKeys<Product, Category, 'categoryId'>(
productRepo,
'categoryId',
[1, 2],
scope,
);
sinon.assert.calledWithMatch(find, {
limit: 4,
order: ['name ASC'],
Expand All @@ -228,4 +284,33 @@ describe('findByForeignKeys', () => {
},
});
});
it('runs find globally when `limit` is not set', async () => {
const find = productRepo.stubs.find;
const scope: Filter<Product> = {
fields: ['id', 'categoryId'],
offset: 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<Product, Category, 'categoryId'>(
productRepo,
'categoryId',
[1, 2],
scope,
);
sinon.assert.calledWithMatch(find, {
fields: ['id', 'categoryId'],
offset: 4,
order: ['name ASC'],
where: {name: {like: 'product%'}},
});
});
});
21 changes: 15 additions & 6 deletions packages/repository/src/relations/relation.helpers.ts
View file
Expand Up @@ -47,14 +47,23 @@ export async function findByForeignKeys<
value = fkValues;
}
let useScopeFilterGlobally = false;

// 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, not
// on a per-fk basis
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.limit` is not defined, there is no reason to apply the scope to
// each fk. This is to prevent unecessarily high database query counts.
// See: https://github.com/loopbackio/loopback-next/issues/8074
if (!scope?.limit) {
useScopeFilterGlobally = true;
}

// 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;
Expand All @@ -63,7 +72,7 @@ export async function findByForeignKeys<

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
// 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 => {
Expand Down

0 comments on commit f5384a0

Please sign in to comment.