From af3410e8b692c80449f353b0c62b2c3b07d34fab Mon Sep 17 00:00:00 2001 From: Agnes Lin Date: Fri, 22 Nov 2019 16:01:20 -0500 Subject: [PATCH] fix: feedback applied --- ...-inclusion-resolver.relation.acceptance.ts | 154 +++++++++--------- .../has-many.relation.acceptance.ts | 17 ++ .../src/crud/relations/helpers.ts | 3 + .../src/repositories/legacy-juggler-bridge.ts | 23 ++- 4 files changed, 109 insertions(+), 88 deletions(-) diff --git a/packages/repository-tests/src/crud/relations/acceptance/has-many-inclusion-resolver.relation.acceptance.ts b/packages/repository-tests/src/crud/relations/acceptance/has-many-inclusion-resolver.relation.acceptance.ts index 539d5376b45a..ebef96cf5dc9 100644 --- a/packages/repository-tests/src/crud/relations/acceptance/has-many-inclusion-resolver.relation.acceptance.ts +++ b/packages/repository-tests/src/crud/relations/acceptance/has-many-inclusion-resolver.relation.acceptance.ts @@ -187,84 +187,86 @@ export function hasManyInclusionResolverAcceptance( expect(toJSON(result)).to.deepEqual(toJSON(expected)); }); - describe('checks save() and replaceById()', () => { - /* istanbul ignore next */ - (features.hasRevisionToken ? it.skip : it)( - 'skips the tests for Cloudant as it needs the token', - async () => { - it('returns inclusions after running save() operation', async () => { - // this shows save() works well with func ensurePersistable and ObjectId - const thor = await customerRepo.create({name: 'Thor'}); - const odin = await customerRepo.create({name: 'Odin'}); - - const thorOrder = await orderRepo.create({ - customerId: thor.id, - description: 'Pizza', - }); - - const pizza = await orderRepo.findById(thorOrder.id); - pizza.customerId = odin.id; - - await orderRepo.save(pizza); - const odinPizza = await orderRepo.findById(thorOrder.id); - - const result = await customerRepo.findById(odin.id, { - include: [{relation: 'orders'}], - }); - const expected = { - ...odin, - parentId: features.emptyValue, - orders: [ - { - ...odinPizza, - isShipped: features.emptyValue, - // eslint-disable-next-line @typescript-eslint/camelcase - shipment_id: features.emptyValue, - }, - ], - }; - expect(toJSON(result)).to.containEql(toJSON(expected)); - }); - - it('returns inclusions after running replaceById() operation', async () => { - // this shows replaceById() works well with func ensurePersistable and ObjectId - const thor = await customerRepo.create({name: 'Thor'}); - const odin = await customerRepo.create({name: 'Odin'}); - - const thorOrder = await orderRepo.create({ - customerId: thor.id, - description: 'Pizza', - }); - - const pizza = await orderRepo.findById(thorOrder.id); - pizza.customerId = odin.id; - - await orderRepo.replaceById(thorOrder.id, pizza); - const odinPizza = await orderRepo.findById(thorOrder.id); - - const result = await customerRepo.find({ - include: [{relation: 'orders'}], - }); - const expected = [ - {...thor, parentId: features.emptyValue}, + skipIf( + !features.hasRevisionToken, + it, + 'returns inclusions after running save() operation', + async () => { + // this shows save() works well with func ensurePersistable and ObjectId + const thor = await customerRepo.create({name: 'Thor'}); + const odin = await customerRepo.create({name: 'Odin'}); + + const thorOrder = await orderRepo.create({ + customerId: thor.id, + description: 'Pizza', + }); + + const pizza = await orderRepo.findById(thorOrder.id); + pizza.customerId = odin.id; + + await orderRepo.save(pizza); + const odinPizza = await orderRepo.findById(thorOrder.id); + + const result = await customerRepo.findById(odin.id, { + include: [{relation: 'orders'}], + }); + const expected = { + ...odin, + parentId: features.emptyValue, + orders: [ + { + ...odinPizza, + isShipped: features.emptyValue, + // eslint-disable-next-line @typescript-eslint/camelcase + shipment_id: features.emptyValue, + }, + ], + }; + expect(toJSON(result)).to.containEql(toJSON(expected)); + }, + ); + + skipIf( + !features.hasRevisionToken, + it, + 'returns inclusions after running replaceById() operation', + async () => { + // this shows replaceById() works well with func ensurePersistable and ObjectId + const thor = await customerRepo.create({name: 'Thor'}); + const odin = await customerRepo.create({name: 'Odin'}); + + const thorOrder = await orderRepo.create({ + customerId: thor.id, + description: 'Pizza', + }); + + const pizza = await orderRepo.findById(thorOrder.id.toString()); + pizza.customerId = odin.id; + + await orderRepo.replaceById(thorOrder.id, pizza); + const odinPizza = await orderRepo.findById(thorOrder.id); + + const result = await customerRepo.find({ + include: [{relation: 'orders'}], + }); + const expected = [ + {...thor, parentId: features.emptyValue}, + { + ...odin, + parentId: features.emptyValue, + orders: [ { - ...odin, - parentId: features.emptyValue, - orders: [ - { - ...odinPizza, - isShipped: features.emptyValue, - // eslint-disable-next-line @typescript-eslint/camelcase - shipment_id: features.emptyValue, - }, - ], + ...odinPizza, + isShipped: features.emptyValue, + // eslint-disable-next-line @typescript-eslint/camelcase + shipment_id: features.emptyValue, }, - ]; - expect(toJSON(result)).to.deepEqual(toJSON(expected)); - }); - }, - ); - }); + ], + }, + ]; + expect(toJSON(result)).to.deepEqual(toJSON(expected)); + }, + ); it('throws when navigational properties are present when updating model instance with save()', async () => { // save() calls replaceById so the result will be the same for replaceById 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 b9a7b529cd46..e54c4aff15ef 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 @@ -247,6 +247,23 @@ export function hasManyRelationAcceptance( ).to.be.rejectedWith(/Navigational properties are not allowed.*"orders"/); }); + it('throws when the instance contains navigational property when operates delete()', async () => { + const customer = await customerRepo.create({name: 'customer'}); + + await orderRepo.create({ + description: 'pizza', + customerId: customer.id, + }); + + const found = await customerRepo.findById(customer.id, { + include: [{relation: 'orders'}], + }); + + await expect(customerRepo.delete(found)).to.be.rejectedWith( + 'Navigational properties are not allowed in model data (model "Customer" property "orders")', + ); + }); + context('when targeting the source model', () => { it('gets the parent entity through the child entity', async () => { const parent = await customerRepo.create({name: 'parent customer'}); diff --git a/packages/repository-tests/src/crud/relations/helpers.ts b/packages/repository-tests/src/crud/relations/helpers.ts index d22534927556..bf903b15ec0e 100644 --- a/packages/repository-tests/src/crud/relations/helpers.ts +++ b/packages/repository-tests/src/crud/relations/helpers.ts @@ -30,6 +30,9 @@ export function givenBoundCrudRepositories( Order.definition.properties.id.type = features.idType; Address.definition.properties.id.type = features.idType; Customer.definition.properties.id.type = features.idType; + Customer.definition.properties.id.mongodb = { + dataType: 'ObjectID', + }; Shipment.definition.properties.id.type = features.idType; // when running the test suite on MongoDB, we don't really need to setup // this config for mongo connector to pass the test. diff --git a/packages/repository/src/repositories/legacy-juggler-bridge.ts b/packages/repository/src/repositories/legacy-juggler-bridge.ts index 01d8802b80a9..04518c687b22 100644 --- a/packages/repository/src/repositories/legacy-juggler-bridge.ts +++ b/packages/repository/src/repositories/legacy-juggler-bridge.ts @@ -420,7 +420,8 @@ export class DefaultCrudRepository< return this.updateById(entity.getId(), entity, options); } - delete(entity: T, options?: Options): Promise { + async delete(entity: T, options?: Options): Promise { + this.ensurePersistable(entity); return this.deleteById(entity.getId(), options); } @@ -456,7 +457,7 @@ export class DefaultCrudRepository< options?: Options, ): Promise { try { - const payload = this.ensurePersistable(data, {replacement: true}); + const payload = this.ensurePersistable(data); await ensurePromise(this.modelClass.replaceById(id, payload, options)); } catch (err) { if (err.statusCode === 404) { @@ -536,15 +537,13 @@ export class DefaultCrudRepository< /** Converts an entity object to a JSON object to check if it contains navigational property. * Throws an error if `entity` contains navigational property. - * Also removed id property for replaceById operation (mongo use case). * * @param entity - * @param options when attributw replacement is set to true, delete the id property to operate - * replaceById method. + * @param options */ protected ensurePersistable( entity: R | DataObject, - options: {replacement?: boolean; idProperty?: string} = {}, + options: {}, ): legacy.ModelData { // FIXME(bajtos) Ideally, we should call toJSON() to convert R to data object // Unfortunately that breaks replaceById for MongoDB connector, where we @@ -569,12 +568,12 @@ export class DefaultCrudRepository< ); } } - if (options.replacement === true) { - const idProperty = this.entityClass.getIdProperties()[0]; - if (idProperty) { - delete data[idProperty]; - } - } + // if (options.replacement === true) { + // const idProperty = this.entityClass.getIdProperties()[0]; + // if (!data[idProperty] === undefined) { + // throw new Error(`id property should not be included in ${data}`); + // } + // } return data; }