From 887d3b72f71a0614b341d9087df9adf4daa7614a Mon Sep 17 00:00:00 2001 From: Agnes Lin Date: Tue, 30 Jun 2020 16:30:28 -0400 Subject: [PATCH] fix(repository): hasManyThrough can delete correct target n through based on filter --- .../has-many-through.relation.acceptance.ts | 48 ++++++++++++++---- .../has-many.relation.acceptance.ts | 10 ++-- .../relation.factory.integration.ts | 30 ++++++++++++ .../resolve-has-many-through-metadata.unit.ts | 25 ++++++++++ .../has-many-through-repository.factory.ts | 6 ++- .../has-many/has-many-through.helpers.ts | 49 +++++++++++++++++++ .../has-many/has-many-through.repository.ts | 35 +++++++++---- 7 files changed, 179 insertions(+), 24 deletions(-) diff --git a/packages/repository-tests/src/crud/relations/acceptance/has-many-through.relation.acceptance.ts b/packages/repository-tests/src/crud/relations/acceptance/has-many-through.relation.acceptance.ts index a29425278187..fa2f8308378d 100644 --- a/packages/repository-tests/src/crud/relations/acceptance/has-many-through.relation.acceptance.ts +++ b/packages/repository-tests/src/crud/relations/acceptance/has-many-through.relation.acceptance.ts @@ -66,7 +66,7 @@ export function hasManyThroughRelationAcceptance( existingCustomerId = (await givenPersistedCustomerInstance()).id; }); - it('can create an instance of the related model alone with a through model', async () => { + it('creates an instance of the related model alone with a through model', async () => { const item = await customerRepo .cartItems(existingCustomerId) .create( @@ -93,7 +93,7 @@ export function hasManyThroughRelationAcceptance( ); }); - it('can find instances of the related model', async () => { + it('finds instances of the related model', async () => { const item = await customerRepo .cartItems(existingCustomerId) .create( @@ -108,7 +108,7 @@ export function hasManyThroughRelationAcceptance( expect(toJSON(result[0])).to.not.containEql(toJSON(notMyItem)); }); - it('can patch instances', async () => { + it('patches instances', async () => { const item1 = await customerRepo .cartItems(existingCustomerId) .create({description: 'group 1'}); @@ -129,7 +129,7 @@ export function hasManyThroughRelationAcceptance( ); }); - it('can patch an instance based on the filter', async () => { + it('patches an instance based on the filter', async () => { const item1 = await customerRepo .cartItems(existingCustomerId) .create({description: 'group 1'}); @@ -162,7 +162,7 @@ export function hasManyThroughRelationAcceptance( ).to.be.rejectedWith(/Property "id" cannot be changed!/); }); - it('can delete many instances and their through models', async () => { + it('deletes many instances and their through models', async () => { await customerRepo .cartItems(existingCustomerId) .create({description: 'group 1'}); @@ -182,7 +182,7 @@ export function hasManyThroughRelationAcceptance( expect(cartItems).have.length(0); }); - it('can delete corresponding through models when the target gets deleted', async () => { + it('deletes corresponding through models when the target gets deleted', async () => { const item = await customerRepo .cartItems(existingCustomerId) .create({description: 'group 1'}); @@ -205,9 +205,39 @@ export function hasManyThroughRelationAcceptance( expect(cartItems).have.length(0); }); - //FIXME(Agnes): should be able to do deletion based on filters + it('deletes instances based on the filter', async () => { + const item1 = await customerRepo + .cartItems(existingCustomerId) + .create({description: 'group 1'}); + await customerRepo + .cartItems(existingCustomerId) + .create({description: 'group 2'}); + + let links = await customerCartItemLinkRepo.find(); + let cartItems = await cartItemRepo.find(); + expect(links).have.length(2); + expect(cartItems).have.length(2); + + await customerRepo + .cartItems(existingCustomerId) + .delete({description: 'does not exist'}); + links = await customerCartItemLinkRepo.find(); + cartItems = await cartItemRepo.find(); + expect(links).have.length(2); + expect(cartItems).have.length(2); + + await customerRepo + .cartItems(existingCustomerId) + .delete({description: 'group 2'}); + links = await customerCartItemLinkRepo.find(); + cartItems = await cartItemRepo.find(); + expect(links).have.length(1); + expect(toJSON(cartItems)).to.containDeep( + toJSON([{id: item1.id, description: 'group 1'}]), + ); + }); - it('can link a target model to a source model', async () => { + it('links a target model to a source model', async () => { const item = await cartItemRepo.create({description: 'an item'}); let targets = await customerRepo.cartItems(existingCustomerId).find(); @@ -224,7 +254,7 @@ export function hasManyThroughRelationAcceptance( ); }); - it('can unlink a target model from a source model', async () => { + it('unlinks a target model from a source model', async () => { const item = await customerRepo .cartItems(existingCustomerId) .create({description: 'an item'}); diff --git a/packages/repository-tests/src/crud/relations/acceptance/has-many.relation.acceptance.ts b/packages/repository-tests/src/crud/relations/acceptance/has-many.relation.acceptance.ts index be2e84c9e8f0..0c77ff6401c6 100644 --- a/packages/repository-tests/src/crud/relations/acceptance/has-many.relation.acceptance.ts +++ b/packages/repository-tests/src/crud/relations/acceptance/has-many.relation.acceptance.ts @@ -58,7 +58,7 @@ export function hasManyRelationAcceptance( existingCustomerId = (await givenPersistedCustomerInstance()).id; }); - it('can create an instance of the related model', async () => { + it('creates an instance of the related model', async () => { const order = await customerRepo.orders(existingCustomerId).create({ description: 'order 1', }); @@ -80,7 +80,7 @@ export function hasManyRelationAcceptance( ); }); - it('can find instances of the related model', async () => { + it('finds instances of the related model', async () => { const order = await createCustomerOrders(existingCustomerId, { description: 'an order desc', }); @@ -104,7 +104,7 @@ export function hasManyRelationAcceptance( expect(toJSON(persisted)).to.deepEqual(toJSON(foundOrders)); }); - it('can find an instance of the related model with non-id property as a source key(keyFrom)', async () => { + it('finds an instance of the related model with non-id property as a source key(keyFrom)', async () => { const shipment = await shipmentRepo.create({ name: 'non-id prop as keyFrom relation', // eslint-disable-next-line @typescript-eslint/naming-convention @@ -139,7 +139,7 @@ export function hasManyRelationAcceptance( ); }); - it('can patch many instances', async () => { + it('patches many instances', async () => { await createCustomerOrders(existingCustomerId, { description: 'order 1', isShipped: false, @@ -182,7 +182,7 @@ export function hasManyRelationAcceptance( ).to.be.rejectedWith(/Property "customerId" cannot be changed!/); }); - it('can delete many instances', async () => { + it('deletes many instances', async () => { await createCustomerOrders(existingCustomerId, { description: 'order 1', }); 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 3f71aa3137db..83768048488e 100644 --- a/packages/repository/src/__tests__/integration/repositories/relation.factory.integration.ts +++ b/packages/repository/src/__tests__/integration/repositories/relation.factory.integration.ts @@ -342,6 +342,36 @@ describe('HasManyThrough relation', () => { expect(links[0]).has.property('customerId', existingCustomerId + 1); }); + it('deletes instances based on the filter', async () => { + await customerCartItemRepo.create({ + description: 'customer 1', + }); + const item2 = await customerCartItemRepo.create({ + description: 'customer 2', + }); + + let items = await cartItemRepo.find(); + let links = await customerCartItemLinkRepo.find(); + expect(items).have.length(2); + expect(links).have.length(2); + + await customerCartItemRepo.delete({description: 'does not exist'}); + items = await cartItemRepo.find(); + links = await customerCartItemLinkRepo.find(); + expect(items).have.length(2); + expect(links).have.length(2); + + await customerCartItemRepo.delete({description: 'customer 1'}); + items = await cartItemRepo.find(); + links = await customerCartItemLinkRepo.find(); + + expect(items).have.length(1); + expect(links).have.length(1); + expect(items).to.deepEqual([item2]); + expect(links[0]).has.property('itemId', item2.id); + expect(links[0]).has.property('customerId', existingCustomerId); + }); + it('patches instances that belong to the same source model (same source fk)', async () => { const item1 = await customerCartItemRepo.create({ description: 'group 1', diff --git a/packages/repository/src/__tests__/unit/repositories/relations-helpers/resolve-has-many-through-metadata.unit.ts b/packages/repository/src/__tests__/unit/repositories/relations-helpers/resolve-has-many-through-metadata.unit.ts index 998698157ebf..94536faf946e 100644 --- a/packages/repository/src/__tests__/unit/repositories/relations-helpers/resolve-has-many-through-metadata.unit.ts +++ b/packages/repository/src/__tests__/unit/repositories/relations-helpers/resolve-has-many-through-metadata.unit.ts @@ -16,6 +16,7 @@ import { createTargetConstraintFromThrough, createThroughConstraintFromSource, createThroughConstraintFromTarget, + getTargetIdsFromTargetModels, getTargetKeysFromThroughModels, HasManyThroughResolvedDefinition, resolveHasManyThroughMetadata, @@ -103,6 +104,27 @@ describe('HasManyThroughHelpers', () => { expect(result).to.containEql({id: 9}); }); }); + + context('getTargetIdsFromTargetModels', () => { + it('returns an empty array if the given target array is empty', () => { + const result = getTargetIdsFromTargetModels(relationMetaData, []); + expect(result).to.containDeep([]); + }); + it('creates constraint with a given fk', () => { + const result = getTargetIdsFromTargetModels(relationMetaData, [ + createProduct({id: 1}), + ]); + expect(result).to.containDeep([1]); + }); + it('creates constraint with given fks', () => { + const result = getTargetIdsFromTargetModels(relationMetaData, [ + createProduct({id: 1}), + createProduct({id: 2}), + ]); + expect(result).to.containDeep([1, 2]); + }); + }); + context('createThroughConstraintFromTarget', () => { it('creates constraint with a given fk', () => { const result = createThroughConstraintFromTarget(relationMetaData, [1]); @@ -396,4 +418,7 @@ describe('HasManyThroughHelpers', () => { function createCategoryProductLink(properties: Partial) { return new CategoryProductLink(properties); } + function createProduct(properties: Partial) { + return new Product(properties); + } }); diff --git a/packages/repository/src/relations/has-many/has-many-through-repository.factory.ts b/packages/repository/src/relations/has-many/has-many-through-repository.factory.ts index f254cc56d515..7c4be06b56ea 100644 --- a/packages/repository/src/relations/has-many/has-many-through-repository.factory.ts +++ b/packages/repository/src/relations/has-many/has-many-through-repository.factory.ts @@ -13,6 +13,7 @@ import { createTargetConstraintFromThrough, createThroughConstraintFromSource, createThroughConstraintFromTarget, + getTargetIdsFromTargetModels, getTargetKeysFromThroughModels, resolveHasManyThroughMetadata, } from './has-many-through.helpers'; @@ -69,7 +70,9 @@ export function createHasManyThroughRepositoryFactory< >(meta, fkValue); return constraint; } - + function getTargetIds(targetInstances: Target[]): TargetID[] { + return getTargetIdsFromTargetModels(meta, targetInstances); + } function getThroughConstraintFromTarget( fkValues: TargetID[], ): DataObject { @@ -93,6 +96,7 @@ export function createHasManyThroughRepositoryFactory< getTargetConstraintFromThroughModels, getTargetKeys, getThroughConstraintFromSource, + getTargetIds, getThroughConstraintFromTarget, ); }; diff --git a/packages/repository/src/relations/has-many/has-many-through.helpers.ts b/packages/repository/src/relations/has-many/has-many-through.helpers.ts index 0d8e8a5c7552..ea3ba7862732 100644 --- a/packages/repository/src/relations/has-many/has-many-through.helpers.ts +++ b/packages/repository/src/relations/has-many/has-many-through.helpers.ts @@ -174,6 +174,55 @@ export function createThroughConstraintFromSource< const constraint: any = {[sourceFkName]: fkValue}; return constraint; } + +/** + * Returns an array of target ids of the given target instances. + * + * @param relationMeta - resolved hasManyThrough metadata + * @param targetInstances - an array of target instances + * + * @example + * ```ts + * const resolvedMetadata = { + * // .. other props + * keyFrom: 'id', + * keyTo: 'id', + * through: { + * model: () => CategoryProductLink, + * keyFrom: 'categoryId', + * keyTo: 'productId', + * }, + * }; + * getTargetKeysFromTargetModels(resolvedMetadata,[{ + id: 2, + des: 'a target', + }]); + * >>> [2] + * getTargetKeysFromTargetModels(resolvedMetadata, [ + { + id: 2, + des: 'a target', + }, { + id: 1, + des: 'a target', + } + ]); + >>> [2, 1] + */ +export function getTargetIdsFromTargetModels( + relationMeta: HasManyThroughResolvedDefinition, + targetInstances: Target[], +): TargetID[] { + const targetId = relationMeta.keyTo; + // eslint-disable-next-line @typescript-eslint/no-explicit-any + let ids = [] as any; + ids = targetInstances.map( + (targetInstance: Target) => targetInstance[targetId as keyof Target], + ); + ids = deduplicate(ids); + return ids as TargetID[]; +} + /** * Creates through constraint based on the target foreign key * diff --git a/packages/repository/src/relations/has-many/has-many-through.repository.ts b/packages/repository/src/relations/has-many/has-many-through.repository.ts index 17c67f5cdf9b..c4a81a6706e5 100644 --- a/packages/repository/src/relations/has-many/has-many-through.repository.ts +++ b/packages/repository/src/relations/has-many/has-many-through.repository.ts @@ -135,6 +135,7 @@ export class DefaultHasManyThroughRepository< ) => DataObject, public getTargetKeys: (throughInstances: ThroughEntity[]) => TargetID[], public getThroughConstraintFromSource: () => DataObject, + public getTargetIds: (targetInstances: TargetEntity[]) => TargetID[], public getThroughConstraintFromTarget: ( targetID: TargetID[], ) => DataObject, @@ -191,15 +192,31 @@ export class DefaultHasManyThroughRepository< constrainFilter(undefined, sourceConstraint), options?.throughOptions, ); - const targetFkValues = this.getTargetKeys(throughInstances); - // delete through instances that have the targets that are going to be deleted - const throughFkConstraint = this.getThroughConstraintFromTarget( - targetFkValues, - ); - await throughRepository.deleteAll( - constrainWhereOr({}, [sourceConstraint, throughFkConstraint]), - ); - + if (where) { + // only delete related through models + // TODO(Agnes): this performance can be improved by only fetching related data + // TODO: add target ids to the `where` constraint + const targets = await targetRepository.find({where}); + const targetIds = this.getTargetIds(targets); + if (targetIds.length > 0) { + const targetConstraint = this.getThroughConstraintFromTarget(targetIds); + const constraints = {...targetConstraint, ...sourceConstraint}; + await throughRepository.deleteAll( + constrainDataObject({}, constraints as DataObject), + options?.throughOptions, + ); + } + } else { + // otherwise, delete through models that relate to the sourceId + const targetFkValues = this.getTargetKeys(throughInstances); + // delete through instances that have the targets that are going to be deleted + const throughFkConstraint = this.getThroughConstraintFromTarget( + targetFkValues, + ); + await throughRepository.deleteAll( + constrainWhereOr({}, [sourceConstraint, throughFkConstraint]), + ); + } // delete target(s) const targetConstraint = this.getTargetConstraintFromThroughModels( throughInstances,