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..c8c0383d9650 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,7 +187,107 @@ export function hasManyInclusionResolverAcceptance( expect(toJSON(result)).to.deepEqual(toJSON(expected)); }); - it('throws when navigational properties are present when updating model instance', 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}, + { + ...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', + customerId, + }); + + const found = await customerRepo.findById(customerId, { + include: [{relation: 'orders'}], + }); + 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( + /Navigational properties are not allowed.*"orders"/, + ); + }); + + it('throws when the instance contains navigational property when operates update()', async () => { const created = await customerRepo.create({name: 'customer'}); const customerId = created.id; @@ -202,10 +302,35 @@ export function hasManyInclusionResolverAcceptance( expect(found.orders).to.have.lengthOf(1); found.name = 'updated name'; - await expect(customerRepo.save(found)).to.be.rejectedWith( - 'The `Customer` instance is not valid. Details: `orders` is not defined in the model (value: undefined).', + 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( + // can test with toString for mongo + customerRepo.updateById(customer.id, { + name: 'Luigi', + orders: [{description: 'Nintendo'}], + }), + ).to.be.rejectedWith(/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..b9a7b529cd46 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,67 @@ 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"/); }); 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..35ce40764caa 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,10 +27,15 @@ 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 // setting it up to check if it works fine as well + Order.definition.properties.customerId.type = features.idType; Order.definition.properties.customerId.mongodb = { dataType: 'ObjectID', 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 63de1a675e16..2b2125fc715a 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,232 @@ 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'}], - }); - - 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, + 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: []}, + ]); }); - folderRepo.registerInclusionResolver('author', hasOneResolver); - - const folder = await folderRepo.findOne({ - include: [{relation: 'author'}], + 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(), + }); }); - expect(folder!.toJSON()).to.deepEqual({ - ...folders[0].toJSON(), - author: createdAuthor.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, + }); + + folderRepo.registerInclusionResolver('author', hasOneResolver); + + const folder = await folderRepo.findOne({ + include: [{relation: 'author'}], + }); + + 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; + }; + }); + context( + 'checks navigational properties for create and update operations (ensurePersistable)', + () => { + it('throws if create() tries to create an entity with DataObject with nav properties', async () => { + 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")', + ); + }); + + it('throws if createAll() tries to create all entities that some if them have navigational properties', async () => { + await expect( + folderRepo.createAll([ + {name: 'f1'}, + {name: 'f2', files: [{title: 'nav property'}]}, + ]), + ).to.be.rejectedWith( + 'Navigational properties are not allowed in model data (model "Folder" property "files")', + ); + }); + it('throws if updateAll() tries to update entities with DataObject with nav properties', async () => { + await folderRepo.create({name: 'f1', id: 1}); + await folderRepo.create({name: 'f2', id: 2}); + await expect( + folderRepo.updateAll({ + name: 'unified name', + files: [{title: 'nav property'}], + }), + ).to.be.rejectedWith( + 'Navigational properties are not allowed in model data (model "Folder" property "files")', + ); + }); + + it('throws if updateById() tries to update entities with an entity with nav properties', async () => { + folderRepo.registerInclusionResolver('files', hasManyResolver); + await folderRepo.create({name: 'folder', id: 1}); + + await expect( + folderRepo.updateById(1, { + name: 'update', + files: [{title: 't', id: 1, folderId: 1}], + }), + ).to.be.rejectedWith( + 'Navigational properties are not allowed in model data (model "Folder" property "files")', + ); + }); + + it('throws if update() tries to update entities with Entity.toJSON() with nav properties', async () => { + folderRepo.registerInclusionResolver('files', hasManyResolver); + await folderRepo.create({name: 'folder', id: 1}); + await fileRepo.create({title: 'file title', id: 1, folderId: 1}); + const folder = await folderRepo.findById(1, { + include: [{relation: 'files'}], + }); + folder.name = 'new name'; + + await expect(folderRepo.update(folder)).to.be.rejectedWith( + 'Navigational properties are not allowed in model data (model "Folder" property "files")', + ); + }); + // only test replaceById and save with normal id type here. Tests for MongoDB are in repository-tests + it('implements Repository.save() without id', async () => { + const fol = await folderRepo.create({name: 'folder'}); + const folder = await folderRepo.save(fol); + const result = await folderRepo.findById(folder!.id); + expect(result.toJSON()).to.eql(fol!.toJSON()); + }); + + it('implements Repository.save() with id', async () => { + const fol = await folderRepo.create({name: 'f3', id: 1}); + fol.name = 'new name'; + const saved = await folderRepo.save(fol); + const result = await folderRepo.findById(saved!.id); + expect(result.toJSON()).to.eql(fol.toJSON()); + }); + + it('implements Repository.replaceById() without id property provided in the target data', async () => { + const fol = await folderRepo.create({name: 'f3', id: 1}); + await folderRepo.replaceById(fol.id, { + name: 'new name', + }); + const result = await folderRepo.findById(fol.id); + expect(result.toJSON()).to.eql({ + id: fol.id, + name: 'new name', + }); + }); + + it('throws when Repository.replaceById() with id property provided in the target data', async () => { + const fol = await folderRepo.create({name: 'f3', id: 1}); + fol.name = 'new name'; + await folderRepo.replaceById(fol.id, fol); + const result = await folderRepo.findById(fol.id); + expect(result.toJSON()).to.eql({ + id: fol.id, + name: 'new name', + }); + }); + + 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; + }; + }, + ); }); 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 c2f6de4b666a..069703160f00 100644 --- a/packages/repository/src/repositories/legacy-juggler-bridge.ts +++ b/packages/repository/src/repositories/legacy-juggler-bridge.ts @@ -37,6 +37,8 @@ import { EntityCrudRepository, TransactionalEntityRepository, } from './repository'; +import * as debugFactory from 'debug'; +const debug = debugFactory('loopback:repository:legacy-juggler-bridge'); export namespace juggler { /* eslint-disable @typescript-eslint/no-unused-vars */ @@ -339,13 +341,18 @@ export class DefaultCrudRepository< } async create(entity: DataObject, options?: Options): Promise { - const model = await ensurePromise(this.modelClass.create(entity, options)); + const model = await ensurePromise( + this.modelClass.create(this.ensurePersistable(entity), options), + ); return this.toEntity(model); } async createAll(entities: DataObject[], options?: Options): Promise { const models = await ensurePromise( - this.modelClass.create(entities, options), + this.modelClass.create( + entities.map(e => this.ensurePersistable(e)), + options, + ), ); return this.toEntities(models); } @@ -426,7 +433,7 @@ export class DefaultCrudRepository< ): Promise { where = where || {}; const result = await ensurePromise( - this.modelClass.updateAll(where, data, options), + this.modelClass.updateAll(where, this.ensurePersistable(data), options), ); return {count: result.count}; } @@ -451,7 +458,17 @@ export class DefaultCrudRepository< options?: Options, ): Promise { try { - await ensurePromise(this.modelClass.replaceById(id, data, options)); + const payload = this.ensurePersistable(data, {replacement: true}); + debug( + '%s replaceById', + this.modelClass.modelName, + typeof id, + id, + payload, + 'options', + options, + ); + await ensurePromise(this.modelClass.replaceById(id, payload, options)); } catch (err) { if (err.statusCode === 404) { throw new EntityNotFoundError(this.entityClass, id); @@ -528,6 +545,50 @@ export class DefaultCrudRepository< return includeRelatedModels(this, entities, include, options); } + /** 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. + */ + protected ensurePersistable( + entity: R | DataObject, + options: {replacement?: boolean; idProperty?: string} = {}, + ): 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 dtat + // 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}")`, + ); + } + } + if (options.replacement === true) { + const idProperty = this.entityClass.getIdProperties()[0]; + if (idProperty) { + delete data[idProperty]; + } + } + return data; + } + /** * Removes juggler's "include" filter as it does not apply to LoopBack 4 * relations.