From d71a37ffeed1aa13a13f95c096ad345d8298f158 Mon Sep 17 00:00:00 2001 From: InvictusMB Date: Thu, 28 May 2020 15:48:49 +0200 Subject: [PATCH] feat(repository): transparently ensure foreign key target in inclusion resolvers --- .../relation.factory.integration.ts | 323 +++++++++++++++++- packages/repository/src/query.ts | 70 +++- .../belongs-to.inclusion-resolver.ts | 15 +- .../has-many/has-many.inclusion-resolver.ts | 9 +- .../has-one/has-one.inclusion-resolver.ts | 15 +- .../src/relations/relation.helpers.ts | 18 +- 6 files changed, 429 insertions(+), 21 deletions(-) diff --git a/packages/repository/src/__tests__/integration/repositories/relation.factory.integration.ts b/packages/repository/src/__tests__/integration/repositories/relation.factory.integration.ts index 13211c5799fc..d8248c945048 100644 --- a/packages/repository/src/__tests__/integration/repositories/relation.factory.integration.ts +++ b/packages/repository/src/__tests__/integration/repositories/relation.factory.integration.ts @@ -81,6 +81,152 @@ describe('HasMany relation', () => { expect(orders).to.deepEqual(persistedOrders); }); + it('can include the related model when the foreign key is omitted in filter', async () => { + const order = await customerOrderRepo.create({ + description: 'an order desc', + }); + + const customer = await customerRepo.findById(existingCustomerId, { + include: [ + { + relation: 'orders', + scope: { + fields: { + description: true, + }, + }, + }, + ], + }); + + withProtoCheck(false, () => { + expect(customer.orders).length(1); + expect(customer.orders).to.matchEach((v: Partial) => { + expect(v).to.deepEqual({ + id: undefined, + description: order.description, + customerId: undefined, + }); + }); + }); + }); + + it('can include the related model when the foreign key is disabled in filter', async () => { + const order = await customerOrderRepo.create({ + description: 'an order desc', + }); + const customer = await customerRepo.findById(existingCustomerId, { + include: [ + { + relation: 'orders', + scope: { + fields: { + customerId: false, + description: true, + }, + }, + }, + ], + }); + + withProtoCheck(false, () => { + expect(customer.orders).length(1); + expect(customer.orders).to.matchEach((v: Partial) => { + expect(v).to.deepEqual({ + id: undefined, + description: order.description, + customerId: undefined, + }); + }); + }); + }); + + it('can include the related model when only the foreign key is disabled in filter', async () => { + const order = await customerOrderRepo.create({ + description: 'an order desc', + }); + const customer = await customerRepo.findById(existingCustomerId, { + include: [ + { + relation: 'orders', + scope: { + fields: { + customerId: false, + }, + }, + }, + ], + }); + + withProtoCheck(false, () => { + expect(customer.orders).length(1); + expect(customer.orders).to.matchEach((v: Partial) => { + expect(v).to.deepEqual({ + id: order.id, + description: order.description, + customerId: undefined, + }); + }); + }); + }); + + it('preserves the foreign key value when set in filter', async () => { + const order = await customerOrderRepo.create({ + description: 'an order desc', + }); + const customer = await customerRepo.findById(existingCustomerId, { + include: [ + { + relation: 'orders', + scope: { + fields: { + customerId: true, + description: true, + }, + }, + }, + ], + }); + + withProtoCheck(false, () => { + expect(customer.orders).length(1); + expect(customer.orders).to.matchEach((v: Partial) => { + expect(v).to.deepEqual({ + id: undefined, + description: order.description, + customerId: order.customerId, + }); + }); + }); + }); + + it('includes only fields set in filter', async () => { + await customerOrderRepo.create({ + description: 'an order desc', + }); + const customer = await customerRepo.findById(existingCustomerId, { + include: [ + { + relation: 'orders', + scope: { + fields: {}, + }, + }, + ], + }); + + withProtoCheck(false, () => { + expect(customer.orders).length(1); + expect(customer.orders).to.matchEach((v: Partial) => { + expect(v).to.deepEqual({ + id: undefined, + description: undefined, + customerId: undefined, + }); + }); + }); + }); + it('finds appropriate related model instances for multiple relations', async () => { // note(shimks): roundabout way of creating reviews with 'approves' // ideally, the review repository should have a approve function @@ -139,6 +285,14 @@ describe('HasMany relation', () => { ); customerOrderRepo = orderFactoryFn(existingCustomerId); + const customerCrud = customerRepo as DefaultCrudRepository< + Customer, + number + >; + customerCrud.registerInclusionResolver( + 'orders', + orderFactoryFn.inclusionResolver, + ); } function givenRepositoryFactoryFunctions() { @@ -154,6 +308,8 @@ describe('HasMany relation', () => { }); describe('BelongsTo relation', () => { + let customer: Customer; + let order: Order; let findCustomerOfOrder: BelongsToAccessor< Customer, typeof Order.prototype.id @@ -168,30 +324,150 @@ describe('BelongsTo relation', () => { reviewRepo.deleteAll(), ]); }); + beforeEach(givenCustomerAndOrder); it('finds an instance of the related model', async () => { - const customer = await customerRepo.create({name: 'Order McForder'}); - const order = await orderRepo.create({ - customerId: customer.id, - description: 'Order from Order McForder, the hoarder of Mordor', - }); - const result = await findCustomerOfOrder(order.id); expect(result).to.deepEqual(customer); }); it('throws EntityNotFound error when the related model does not exist', async () => { - const order = await orderRepo.create({ + const orderToFail = await orderRepo.create({ customerId: 999, // does not exist description: 'Order of a fictional customer', }); - await expect(findCustomerOfOrder(order.id)).to.be.rejectedWith( + await expect(findCustomerOfOrder(orderToFail.id)).to.be.rejectedWith( EntityNotFoundError, ); }); + it('can include the related model when the foreign key is omitted in filter', async () => { + const orderWithRelations = (await orderRepo.findById(order.id, { + include: [ + { + relation: 'customer', + scope: { + fields: { + name: true, + }, + }, + }, + ], + })) as OrderWithRelations; + + withProtoCheck(false, () => { + expect(orderWithRelations.customer).to.deepEqual({ + id: undefined, + name: customer.name, + orders: undefined, + reviewsApproved: undefined, + reviewsAuthored: undefined, + }); + }); + }); + + it('can include the related model when the foreign key is disabled in filter', async () => { + const orderWithRelations = (await orderRepo.findById(order.id, { + include: [ + { + relation: 'customer', + scope: { + fields: { + id: false, + name: true, + }, + }, + }, + ], + })) as OrderWithRelations; + + withProtoCheck(false, () => { + expect(orderWithRelations.customer).to.deepEqual({ + id: undefined, + name: customer.name, + orders: undefined, + reviewsApproved: undefined, + reviewsAuthored: undefined, + }); + }); + }); + + it('can include the related model when only the foreign key is disabled in filter', async () => { + const orderWithRelations = (await orderRepo.findById(order.id, { + include: [ + { + relation: 'customer', + scope: { + fields: { + id: false, + }, + }, + }, + ], + })) as OrderWithRelations; + + withProtoCheck(false, () => { + expect(orderWithRelations.customer).to.deepEqual({ + id: undefined, + name: customer.name, + orders: undefined, + reviewsApproved: undefined, + reviewsAuthored: undefined, + }); + }); + }); + + it('preserves the foreign key value when set in filter', async () => { + const orderWithRelations = (await orderRepo.findById(order.id, { + include: [ + { + relation: 'customer', + scope: { + fields: { + id: true, + name: true, + }, + }, + }, + ], + })) as OrderWithRelations; + + withProtoCheck(false, () => { + expect(orderWithRelations.customer).to.deepEqual({ + id: customer.id, + name: customer.name, + orders: undefined, + reviewsApproved: undefined, + reviewsAuthored: undefined, + }); + }); + }); + + it('includes only fields set in filter', async () => { + const orderWithRelations = (await orderRepo.findById(order.id, { + include: [ + { + relation: 'customer', + scope: { + fields: {}, + }, + }, + ], + })) as OrderWithRelations; + + withProtoCheck(false, () => { + expect(orderWithRelations.customer).to.deepEqual({ + id: undefined, + name: undefined, + orders: undefined, + reviewsApproved: undefined, + reviewsAuthored: undefined, + }); + }); + }); + //--- HELPERS ---// function givenAccessor() { @@ -200,6 +476,22 @@ describe('BelongsTo relation', () => { Getter.fromValue(customerRepo), orderRepo, ); + + const orderCrud = orderRepo as DefaultCrudRepository; + orderCrud.registerInclusionResolver( + 'customer', + findCustomerOfOrder.inclusionResolver, + ); + } + + async function givenCustomerAndOrder() { + customer = await customerRepo.create({ + name: 'Order McForder', + }); + order = await orderRepo.create({ + customerId: customer.id, + description: 'Order from Order McForder, the hoarder of Mordor', + }); } }); @@ -225,6 +517,10 @@ class Order extends Entity { }); } +class OrderWithRelations extends Order { + customer: Customer; +} + class Review extends Entity { id: number; description: string; @@ -284,3 +580,14 @@ function givenCrudRepositories() { orderRepo = new DefaultCrudRepository(Order, db); reviewRepo = new DefaultCrudRepository(Review, db); } + +function withProtoCheck(value: boolean, fn: Function) { + const shouldJs = (expect as unknown) as {config: {checkProtoEql: boolean}}; + const oldValue = shouldJs.config.checkProtoEql; + shouldJs.config.checkProtoEql = value; + try { + fn(); + } finally { + shouldJs.config.checkProtoEql = oldValue; + } +} diff --git a/packages/repository/src/query.ts b/packages/repository/src/query.ts index bbb55d5b7ba4..d7cdb89f0d35 100644 --- a/packages/repository/src/query.ts +++ b/packages/repository/src/query.ts @@ -507,7 +507,7 @@ export class WhereBuilder { } /** - * A builder for Filter. It provides fleunt APIs to add clauses such as + * A builder for Filter. It provides fluent APIs to add clauses such as * `fields`, `order`, `where`, `limit`, `offset`, and `include`. * * @example @@ -698,6 +698,74 @@ export class FilterBuilder { } } +/** + * Checks whether fields array passed as an argument is a + * subset of fields picked by a target filter. + * + * @param fields - array of fields to search in a filter + * @param filter - target filter + * + */ +export function matchesFields( + fields: (keyof T)[], + filter?: Filter, +) { + const normalized = new FilterBuilder(filter).build(); + const targetFields = normalized.fields; + if (!targetFields) { + return true; + } + for (const f of fields) { + if (!targetFields[f]) { + return false; + } + } + return true; +} + +/** + * Ensures that a filter always picks a passed field. + * + * @param targetFields - a field to include + * @param filter - a target filter + */ +export function ensureFields( + targetFields: (keyof T)[], + filter: Filter, +) { + const builder = new FilterBuilder(filter); + const fields = builder.filter.fields; + if (!fields || matchesFields(targetFields, filter)) { + return {filter, pruneFields: {} as PruningMask}; + } + const prune = [] as (keyof T)[]; + let excludedCount = 0; + targetFields.forEach(f => { + if (fields[f] === false) { + excludedCount++; + } + if (!fields[f]) { + prune.push(f); + builder.fields(f); + } + }); + const pruneFields = _.fromPairs( + prune.map(f => [f, undefined]), + ) as PruningMask; + + // if the filter only hides the targetFields, unset the entire fields clause + if (Object.keys(fields).length === excludedCount) { + delete filter.fields; + } + + return { + filter: builder.build(), + pruneFields, + }; +} + +export type PruningMask = {[key in keyof T]: undefined}; + /** * Get nested properties by path * @param value - Value of an object diff --git a/packages/repository/src/relations/belongs-to/belongs-to.inclusion-resolver.ts b/packages/repository/src/relations/belongs-to/belongs-to.inclusion-resolver.ts index 311add19d758..72d9adf29305 100644 --- a/packages/repository/src/relations/belongs-to/belongs-to.inclusion-resolver.ts +++ b/packages/repository/src/relations/belongs-to/belongs-to.inclusion-resolver.ts @@ -5,7 +5,7 @@ import {AnyObject, Options} from '../../common-types'; import {Entity} from '../../model'; -import {Filter, Inclusion} from '../../query'; +import {Filter, Inclusion, ensureFields} from '../../query'; import {EntityCrudRepository} from '../../repositories/repository'; import { deduplicate, @@ -54,15 +54,24 @@ export function createBelongsToInclusionResolver< const targetKey = relationMeta.keyTo as StringKeyOf; const dedupedSourceIds = deduplicate(sourceIds); + const {filter: scope, pruneFields} = ensureFields( + [targetKey], + inclusion.scope as Filter, + ); const targetRepo = await getTargetRepo(); const targetsFound = await findByForeignKeys( targetRepo, targetKey, dedupedSourceIds.filter(e => e), - inclusion.scope as Filter, + scope, options, ); - return flattenTargetsOfOneToOneRelation(sourceIds, targetsFound, targetKey); + return flattenTargetsOfOneToOneRelation( + sourceIds, + targetsFound, + targetKey, + pruneFields, + ); }; } diff --git a/packages/repository/src/relations/has-many/has-many.inclusion-resolver.ts b/packages/repository/src/relations/has-many/has-many.inclusion-resolver.ts index 06ee2c5b5a62..45e01602ba5b 100644 --- a/packages/repository/src/relations/has-many/has-many.inclusion-resolver.ts +++ b/packages/repository/src/relations/has-many/has-many.inclusion-resolver.ts @@ -6,7 +6,7 @@ import debugFactory from 'debug'; import {AnyObject, Options} from '../../common-types'; import {Entity} from '../../model'; -import {Filter, Inclusion} from '../../query'; +import {Filter, Inclusion, ensureFields} from '../../query'; import {EntityCrudRepository} from '../../repositories/repository'; import { findByForeignKeys, @@ -60,12 +60,16 @@ export function createHasManyInclusionResolver< sourceIds.map(i => typeof i), ); + const {filter: scope, pruneFields} = ensureFields( + [targetKey], + inclusion.scope as Filter, + ); const targetRepo = await getTargetRepo(); const targetsFound = await findByForeignKeys( targetRepo, targetKey, sourceIds, - inclusion.scope as Filter, + scope, options, ); @@ -75,6 +79,7 @@ export function createHasManyInclusionResolver< sourceIds, targetsFound, targetKey, + pruneFields, ); debug('fetchHasManyModels result', result); diff --git a/packages/repository/src/relations/has-one/has-one.inclusion-resolver.ts b/packages/repository/src/relations/has-one/has-one.inclusion-resolver.ts index 202d2de14e06..edae32bf6632 100644 --- a/packages/repository/src/relations/has-one/has-one.inclusion-resolver.ts +++ b/packages/repository/src/relations/has-one/has-one.inclusion-resolver.ts @@ -5,7 +5,7 @@ import {AnyObject, Options} from '../../common-types'; import {Entity} from '../../model'; -import {Filter, Inclusion} from '../../query'; +import {Filter, Inclusion, ensureFields} from '../../query'; import {EntityCrudRepository} from '../../repositories/repository'; import { findByForeignKeys, @@ -48,15 +48,24 @@ export function createHasOneInclusionResolver< const sourceIds = entities.map(e => (e as AnyObject)[sourceKey]); const targetKey = relationMeta.keyTo as StringKeyOf; + const {filter: scope, pruneFields} = ensureFields( + [targetKey], + inclusion.scope as Filter, + ); const targetRepo = await getTargetRepo(); const targetsFound = await findByForeignKeys( targetRepo, targetKey, sourceIds, - inclusion.scope as Filter, + scope, options, ); - return flattenTargetsOfOneToOneRelation(sourceIds, targetsFound, targetKey); + return flattenTargetsOfOneToOneRelation( + sourceIds, + targetsFound, + targetKey, + pruneFields, + ); }; } diff --git a/packages/repository/src/relations/relation.helpers.ts b/packages/repository/src/relations/relation.helpers.ts index c40cbe124c8a..bba0ad441f89 100644 --- a/packages/repository/src/relations/relation.helpers.ts +++ b/packages/repository/src/relations/relation.helpers.ts @@ -14,6 +14,7 @@ import { FilterBuilder, Inclusion, Options, + PruningMask, Where, } from '..'; const debug = debugFactory('loopback:repository:relation-helpers'); @@ -149,15 +150,14 @@ function isInclusionAllowed( * @param sourceIds - One value or array of values of the target key * @param targetEntities - target entities that satisfy targetKey's value (ids). * @param targetKey - name of the target key + * @param pruneFields - map of fields to prune after flattening * */ -export function flattenTargetsOfOneToOneRelation< - SourceWithRelations extends Entity, - Target extends Entity ->( +export function flattenTargetsOfOneToOneRelation( sourceIds: unknown[], targetEntities: Target[], targetKey: StringKeyOf, + pruneFields = {} as PruningMask, ): (Target | undefined)[] { const lookup = buildLookupMap( targetEntities, @@ -165,6 +165,10 @@ export function flattenTargetsOfOneToOneRelation< reduceAsSingleItem, ); + targetEntities.forEach((e: Partial) => { + Object.assign(e, pruneFields); + }); + return flattenMapByKeys(sourceIds, lookup); } @@ -176,12 +180,14 @@ export function flattenTargetsOfOneToOneRelation< * @param sourceIds - One value or array of values of the target key * @param targetEntities - target entities that satisfy targetKey's value (ids). * @param targetKey - name of the target key + * @param pruneFields - map of fields to prune after flattening * */ export function flattenTargetsOfOneToManyRelation( sourceIds: unknown[], targetEntities: Target[], targetKey: StringKeyOf, + pruneFields = {} as PruningMask, ): (Target[] | undefined)[] { debug('flattenTargetsOfOneToManyRelation'); debug('sourceIds', sourceIds); @@ -198,6 +204,10 @@ export function flattenTargetsOfOneToManyRelation( reduceAsArray, ); + targetEntities.forEach((e: Partial) => { + Object.assign(e, pruneFields); + }); + debug('lookup map', lookup); return flattenMapByKeys(sourceIds, lookup);