From 95ab58360097b40ab5d7dc6d8484f22da77f9a0e Mon Sep 17 00:00:00 2001 From: Agnes Lin Date: Mon, 18 Nov 2019 21:29:50 -0500 Subject: [PATCH] feat(repository): rejects create/update operations for navigational properties --- ...-inclusion-resolver.relation.acceptance.ts | 149 ++++++++++++- .../has-many.relation.acceptance.ts | 81 ++++++- .../acceptance/has-one.relation.acceptance.ts | 1 + .../src/crud/relations/helpers.ts | 6 + .../src/crud/replace-by-id.suite.ts | 13 +- .../legacy-juggler-bridge.unit.ts | 200 ++++++++++-------- .../src/repositories/legacy-juggler-bridge.ts | 74 ++++++- 7 files changed, 415 insertions(+), 109 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 06f6914e5802..32e05fbeefda 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,9 +187,142 @@ export function hasManyInclusionResolverAcceptance( expect(toJSON(result)).to.deepEqual(toJSON(expected)); }); - it('throws when navigational properties are present when updating model instance', async () => { - const created = await customerRepo.create({name: 'customer'}); - const customerId = created.id; + skipIf( + features.hasRevisionToken, + it, + 'returns inclusions after running save() operation', + async () => { + // this shows save() works well with func ensurePersistable and ObjectId + // the test skips for Cloudant as it needs the _rev property for replacement. + // see replace-by-id.suite.ts + 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 + // the test skips for Cloudant as it needs the _rev property for replacement. + // see replace-by-id.suite.ts + 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; + // FIXME: [mongo] if pizza obj is converted to JSON obj, it would get an error + // because it tries to convert ObjectId to string type. + // should test with JSON obj once it's fixed. + + await orderRepo.replaceById(pizza.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: [ + { + ...odinPizza, + isShipped: features.emptyValue, + // eslint-disable-next-line @typescript-eslint/camelcase + shipment_id: features.emptyValue, + }, + ], + }, + ]; + expect(toJSON(result)).to.deepEqual(toJSON(expected)); + }, + ); + + it('returns inclusions after running updateById() operation', async () => { + 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; + const reheatedPizza = toJSON(pizza); + + await orderRepo.updateById(pizza.id, reheatedPizza); + 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: [ + { + ...odinPizza, + isShipped: features.emptyValue, + // eslint-disable-next-line @typescript-eslint/camelcase + shipment_id: features.emptyValue, + }, + ], + }, + ]; + 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 + const customer = await customerRepo.create({name: 'customer'}); + const anotherCus = await customerRepo.create({name: 'another customer'}); + const customerId = customer.id; await orderRepo.create({ description: 'pizza', @@ -201,11 +334,19 @@ export function hasManyInclusionResolverAcceptance( }); expect(found.orders).to.have.lengthOf(1); + const wrongOrder = await orderRepo.create({ + description: 'wrong order', + customerId: anotherCus.id, + }); + found.name = 'updated name'; + found.orders.push(wrongOrder); + await expect(customerRepo.save(found)).to.be.rejectedWith( - 'The `Customer` instance is not valid. Details: `orders` is not defined in the model (value: undefined).', + /Navigational properties are not allowed.*"orders"/, ); }); + // scope for inclusion is not supported yet it('throws error if the inclusion query contains a non-empty scope', async () => { const customer = await customerRepo.create({name: 'customer'}); 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 106cc3864049..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 @@ -174,7 +174,7 @@ export function hasManyRelationAcceptance( ); }); - it('does not create an array of the related model', async () => { + it('throws when tries to create() an instance with navigational property', async () => { await expect( customerRepo.create({ name: 'a customer', @@ -184,7 +184,84 @@ export function hasManyRelationAcceptance( }, ], }), - ).to.be.rejectedWith(/`orders` is not defined/); + ).to.be.rejectedWith( + 'Navigational properties are not allowed in model data (model "Customer" property "orders")', + ); + }); + + it('throws when tries to createAll() instancese with navigational properties', async () => { + await expect( + customerRepo.createAll([ + { + name: 'a customer', + orders: [{description: 'order 1'}], + }, + { + name: 'a customer', + address: {street: '1 Amedee Bonnet'}, + }, + ]), + ).to.be.rejectedWith( + 'Navigational properties are not allowed in model data (model "Customer" property "orders")', + ); + }); + + it('throws when the instance contains navigational property when operates update()', async () => { + const created = await customerRepo.create({name: 'customer'}); + await orderRepo.create({ + description: 'pizza', + customerId: created.id, + }); + + const found = await customerRepo.findById(created.id, { + include: [{relation: 'orders'}], + }); + expect(found.orders).to.have.lengthOf(1); + + found.name = 'updated name'; + await expect(customerRepo.update(found)).to.be.rejectedWith( + /Navigational properties are not allowed.*"orders"/, + ); + }); + + it('throws when the instancees contain navigational property when operates updateAll()', async () => { + await customerRepo.create({name: 'Mario'}); + await customerRepo.create({name: 'Luigi'}); + + await expect( + customerRepo.updateAll({ + name: 'Nintendo', + orders: [{description: 'Switch'}], + }), + ).to.be.rejectedWith(/Navigational properties are not allowed.*"orders"/); + }); + + it('throws when the instance contains navigational property when operates updateById()', async () => { + const customer = await customerRepo.create({name: 'Mario'}); + + await expect( + customerRepo.updateById(customer.id, { + name: 'Luigi', + orders: [{description: 'Nintendo'}], + }), + ).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', () => { diff --git a/packages/repository-tests/src/crud/relations/acceptance/has-one.relation.acceptance.ts b/packages/repository-tests/src/crud/relations/acceptance/has-one.relation.acceptance.ts index acd97bf1de6f..e89331d4b71c 100644 --- a/packages/repository-tests/src/crud/relations/acceptance/has-one.relation.acceptance.ts +++ b/packages/repository-tests/src/crud/relations/acceptance/has-one.relation.acceptance.ts @@ -49,6 +49,7 @@ export function hasOneRelationAcceptance( ); beforeEach(async () => { + await customerRepo.deleteAll(); await addressRepo.deleteAll(); existingCustomerId = (await givenPersistedCustomerInstance()).id; }); diff --git a/packages/repository-tests/src/crud/relations/helpers.ts b/packages/repository-tests/src/crud/relations/helpers.ts index 93d02d3105dd..d22534927556 100644 --- a/packages/repository-tests/src/crud/relations/helpers.ts +++ b/packages/repository-tests/src/crud/relations/helpers.ts @@ -8,9 +8,11 @@ import {CrudFeatures, CrudRepositoryCtor} from '../..'; import { Address, AddressRepository, + Customer, CustomerRepository, Order, OrderRepository, + Shipment, ShipmentRepository, } from './fixtures/models'; import { @@ -25,6 +27,10 @@ export function givenBoundCrudRepositories( repositoryClass: CrudRepositoryCtor, features: CrudFeatures, ) { + Order.definition.properties.id.type = features.idType; + Address.definition.properties.id.type = features.idType; + Customer.definition.properties.id.type = features.idType; + 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. // however real-world applications might have such config for MongoDB diff --git a/packages/repository-tests/src/crud/replace-by-id.suite.ts b/packages/repository-tests/src/crud/replace-by-id.suite.ts index ddc80eade8a5..2af747b9649c 100644 --- a/packages/repository-tests/src/crud/replace-by-id.suite.ts +++ b/packages/repository-tests/src/crud/replace-by-id.suite.ts @@ -3,12 +3,17 @@ // This file is licensed under the MIT License. // License text available at https://opensource.org/licenses/MIT -import {Entity, model, property} from '@loopback/repository'; -import {AnyObject, EntityCrudRepository} from '@loopback/repository'; +import { + AnyObject, + Entity, + EntityCrudRepository, + model, + property, +} from '@loopback/repository'; import {expect, toJSON} from '@loopback/testlab'; -import {MixedIdType} from '../helpers.repository-tests'; import { deleteAllModelsInDefaultDataSource, + MixedIdType, withCrudCtx, } from '../helpers.repository-tests'; import { @@ -72,7 +77,6 @@ export function createSuiteForReplaceById( // This important! Not all databases allow `patchById` to set // properties to "undefined", `replaceById` must always work. created.description = undefined; - await repo.replaceById(created.id, created); const found = await repo.findById(created.id); @@ -112,7 +116,6 @@ export function createSuiteForReplaceById( // This important! Not all databases allow `patchById` to set // properties to "undefined", `replaceById` must always work. created.description = undefined; - await repo.replaceById(created.id, created); const found = await repo.findById(created.id); diff --git a/packages/repository/src/__tests__/unit/repositories/legacy-juggler-bridge.unit.ts b/packages/repository/src/__tests__/unit/repositories/legacy-juggler-bridge.unit.ts index 6a30ccd272d5..3dfec058e727 100644 --- a/packages/repository/src/__tests__/unit/repositories/legacy-juggler-bridge.unit.ts +++ b/packages/repository/src/__tests__/unit/repositories/legacy-juggler-bridge.unit.ts @@ -318,7 +318,7 @@ describe('DefaultCrudRepository', () => { }); }); - context('find* methods including relations', () => { + describe('DefaultCrudRepository with relations', () => { @model() class Author extends Entity { @property({id: true}) @@ -394,112 +394,128 @@ describe('DefaultCrudRepository', () => { await fileRepo.deleteAll(); await authorRepo.deleteAll(); }); - - it('implements Repository.find() with included models', async () => { - const createdFolders = await folderRepo.createAll([ - {name: 'f1', id: 1}, - {name: 'f2', id: 2}, - ]); - const files = await fileRepo.createAll([ - {id: 1, title: 'file1', folderId: 1}, - {id: 2, title: 'file2', folderId: 3}, - ]); - - folderRepo.registerInclusionResolver('files', hasManyResolver); - - const folders = await folderRepo.find({include: [{relation: 'files'}]}); - - expect(toJSON(folders)).to.deepEqual([ - {...createdFolders[0].toJSON(), files: [toJSON(files[0])]}, - {...createdFolders[1].toJSON(), files: []}, - ]); - }); - - it('implements Repository.findById() with included models', async () => { - const folders = await folderRepo.createAll([ - {name: 'f1', id: 1}, - {name: 'f2', id: 2}, - ]); - const createdFile = await fileRepo.create({ - id: 1, - title: 'file1', - folderId: 1, - }); - - fileRepo.registerInclusionResolver('folder', belongsToResolver); - - const file = await fileRepo.findById(1, { - include: [{relation: 'folder'}], + context('find* methods with inclusion', () => { + it('implements Repository.find() with included models', async () => { + const createdFolders = await folderRepo.createAll([ + {name: 'f1', id: 1}, + {name: 'f2', id: 2}, + ]); + const files = await fileRepo.createAll([ + {id: 1, title: 'file1', folderId: 1}, + {id: 2, title: 'file2', folderId: 3}, + ]); + + folderRepo.registerInclusionResolver('files', hasManyResolver); + + const folders = await folderRepo.find({include: [{relation: 'files'}]}); + + expect(toJSON(folders)).to.deepEqual([ + {...createdFolders[0].toJSON(), files: [toJSON(files[0])]}, + {...createdFolders[1].toJSON(), files: []}, + ]); }); - expect(file.toJSON()).to.deepEqual({ - ...createdFile.toJSON(), - folder: folders[0].toJSON(), + it('implements Repository.findById() with included models', async () => { + const folders = await folderRepo.createAll([ + {name: 'f1', id: 1}, + {name: 'f2', id: 2}, + ]); + const createdFile = await fileRepo.create({ + id: 1, + title: 'file1', + folderId: 1, + }); + + fileRepo.registerInclusionResolver('folder', belongsToResolver); + + const file = await fileRepo.findById(1, { + include: [{relation: 'folder'}], + }); + + expect(file.toJSON()).to.deepEqual({ + ...createdFile.toJSON(), + folder: folders[0].toJSON(), + }); }); - }); - it('implements Repository.findOne() with included models', async () => { - const folders = await folderRepo.createAll([ - {name: 'f1', id: 1}, - {name: 'f2', id: 2}, - ]); - const createdAuthor = await authorRepo.create({ - id: 1, - name: 'a1', - folderId: 1, + it('implements Repository.findOne() with included models', async () => { + const folders = await folderRepo.createAll([ + {name: 'f1', id: 1}, + {name: 'f2', id: 2}, + ]); + const createdAuthor = await authorRepo.create({ + id: 1, + name: 'a1', + folderId: 1, + }); + + folderRepo.registerInclusionResolver('author', hasOneResolver); + + const folder = await folderRepo.findOne({ + include: [{relation: 'author'}], + }); + + expect(folder!.toJSON()).to.deepEqual({ + ...folders[0].toJSON(), + author: createdAuthor.toJSON(), + }); }); - folderRepo.registerInclusionResolver('author', hasOneResolver); - - const folder = await folderRepo.findOne({ - include: [{relation: 'author'}], + it('throws if the target data passes to CRUD methods contains nav properties', async () => { + // a unit test for entityToData, which is invoked by create() method + // it would be the same of other CRUD methods. + await expect( + folderRepo.create({ + name: 'f1', + files: [{title: 'nav property'}], + }), + ).to.be.rejectedWith( + 'Navigational properties are not allowed in model data (model "Folder" property "files")', + ); }); - expect(folder!.toJSON()).to.deepEqual({ - ...folders[0].toJSON(), - author: createdAuthor.toJSON(), - }); - }); - - // stub resolvers - - const hasManyResolver: InclusionResolver = async entities => { - const files = []; - for (const entity of entities) { - const file = await folderFiles(entity.id).find(); - files.push(file); - } + // stub resolvers + const hasManyResolver: InclusionResolver< + Folder, + File + > = async entities => { + const files = []; + for (const entity of entities) { + const file = await folderFiles(entity.id).find(); + files.push(file); + } - return files; - }; + return files; + }; - const belongsToResolver: InclusionResolver< - File, - Folder - > = async entities => { - const folders = []; + const belongsToResolver: InclusionResolver< + File, + Folder + > = async entities => { + const folders = []; - for (const file of entities) { - const folder = await fileFolder(file.folderId); - folders.push(folder); - } + for (const file of entities) { + const folder = await fileFolder(file.folderId); + folders.push(folder); + } - return folders; - }; + return folders; + }; - const hasOneResolver: InclusionResolver< - Folder, - Author - > = async entities => { - const authors = []; + const hasOneResolver: InclusionResolver< + Folder, + Author + > = async entities => { + const authors = []; - for (const folder of entities) { - const author = await folderAuthor(folder.id).get(); - authors.push(author); - } + for (const folder of entities) { + const author = await folderAuthor(folder.id).get(); + authors.push(author); + } - return authors; - }; + return authors; + }; + }); }); it('implements Repository.delete()', async () => { diff --git a/packages/repository/src/repositories/legacy-juggler-bridge.ts b/packages/repository/src/repositories/legacy-juggler-bridge.ts index 206a0bdc2552..3ac35d12a25b 100644 --- a/packages/repository/src/repositories/legacy-juggler-bridge.ts +++ b/packages/repository/src/repositories/legacy-juggler-bridge.ts @@ -339,14 +339,18 @@ export class DefaultCrudRepository< } async create(entity: DataObject, options?: Options): Promise { - const model = await ensurePromise(this.modelClass.create(entity, options)); + // perform persist hook + const data = await this.entityToData(entity, options); + const model = await ensurePromise(this.modelClass.create(data, options)); return this.toEntity(model); } async createAll(entities: DataObject[], options?: Options): Promise { - const models = await ensurePromise( - this.modelClass.create(entities, options), + // perform persist hook + const data = await Promise.all( + entities.map(e => this.entityToData(e, options)), ); + const models = await ensurePromise(this.modelClass.create(data, options)); return this.toEntities(models); } @@ -415,7 +419,9 @@ export class DefaultCrudRepository< return this.updateById(entity.getId(), entity, options); } - delete(entity: T, options?: Options): Promise { + async delete(entity: T, options?: Options): Promise { + // perform persist hook + await this.entityToData(entity, options); return this.deleteById(entity.getId(), options); } @@ -425,8 +431,9 @@ export class DefaultCrudRepository< options?: Options, ): Promise { where = where ?? {}; + const persistedData = await this.entityToData(data, options); const result = await ensurePromise( - this.modelClass.updateAll(where, data, options), + this.modelClass.updateAll(where, persistedData, options), ); return {count: result.count}; } @@ -451,7 +458,8 @@ export class DefaultCrudRepository< options?: Options, ): Promise { try { - await ensurePromise(this.modelClass.replaceById(id, data, options)); + const payload = await this.entityToData(data, options); + await ensurePromise(this.modelClass.replaceById(id, payload, options)); } catch (err) { if (err.statusCode === 404) { throw new EntityNotFoundError(this.entityClass, id); @@ -528,6 +536,60 @@ export class DefaultCrudRepository< return includeRelatedModels(this, entities, include, options); } + /** + * This function works as a persist hook. + * It converts an entity from the CRUD operations' caller + * to a persistable data that can will be stored in the + * back-end database. + * + * User can extend `DefaultCrudRepository` then override this + * function to execute custom persist hook. + * @param entity The entity passed from CRUD operations' caller. + * @param options + */ + protected async entityToData( + entity: R | DataObject, + options = {}, + ): Promise> { + return this.ensurePersistable(entity, options); + } + + /** Converts an entity object to a JSON object to check if it contains navigational property. + * Throws an error if `entity` contains navigational property. + * + * @param entity The entity passed from CRUD operations' caller. + * @param options + */ + protected ensurePersistable( + entity: R | DataObject, + 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 + // would call replaceId with id *argument* set to ObjectID value but + // id *property* set to string value. + /* + const data: AnyObject = + typeof entity.toJSON === 'function' ? entity.toJSON() : {...entity}; + */ + + // proposal solution from agnes: delete the id property in the + // target data when runs replaceById + + const data: AnyObject = new this.entityClass(entity); + + const def = this.entityClass.definition; + for (const r in def.relations) { + const relName = def.relations[r].name; + if (relName in data) { + throw new Error( + `Navigational properties are not allowed in model data (model "${this.entityClass.modelName}" property "${relName}")`, + ); + } + } + return data; + } + /** * Removes juggler's "include" filter as it does not apply to LoopBack 4 * relations.