From d11e189a0d6addb665d66ad7d163abd3ba809291 Mon Sep 17 00:00:00 2001 From: Guillaume Gautreau Date: Tue, 12 Sep 2023 16:44:46 +0200 Subject: [PATCH 01/10] fix(datasource-mongoose): fix errors when creating or retrieving records from collections created with asModel on an object field Linked to CU-860rq6mt1 --- .../datasource-mongoose/src/collection.ts | 61 ++++++++++++++++--- .../src/utils/pipeline/lookup.ts | 44 ++++++++++--- .../flattener/collection_create.test.ts | 30 +++++++-- .../flattener/collection_list.test.ts | 34 +++++++++++ 4 files changed, 146 insertions(+), 23 deletions(-) diff --git a/packages/datasource-mongoose/src/collection.ts b/packages/datasource-mongoose/src/collection.ts index cb8ade4d50..01dffeca65 100644 --- a/packages/datasource-mongoose/src/collection.ts +++ b/packages/datasource-mongoose/src/collection.ts @@ -113,16 +113,31 @@ export default class MongooseCollection extends BaseCollection { // Only array fields can create subdocuments (the others should use update) const schema = MongooseSchema.fromModel(this.model).applyStack(this.stack); - if (!schema.isArray) - throw new ValidationError('Trying to create subrecords on a non-array field'); - // Transform list of subrecords to a list of modifications that we'll apply to the root record. + if (schema.isArray) { + return this._createForArraySubfield(data, flatData, schema); + } + + return this._createForObjectSubfield(data, flatData); + } + + private computeSubFieldName() { const lastStackStep = this.stack[this.stack.length - 1]; const fieldName = this.stack.length > 2 ? lastStackStep.prefix.substring(this.stack[this.stack.length - 2].prefix.length + 1) : lastStackStep.prefix; + return fieldName; + } + + private async _createForArraySubfield( + data: RecordData[], + flatData: RecordData[], + schema: MongooseSchema, + ) { + const fieldName = this.computeSubFieldName(); + const updates: Record = {}; const results = []; @@ -146,10 +161,12 @@ export default class MongooseCollection extends BaseCollection { // Apply the modifications to the root document. const promises = Object.values(updates).map(({ rootId, path, records }) => - this.model.updateOne( - { _id: rootId }, - { $push: { [path]: { $position: 0, $each: records } } }, - ), + schema.isArray + ? this.model.updateOne( + { _id: rootId }, + { $push: { [path]: { $position: 0, $each: records } } }, + ) + : this.model.updateOne({ _id: rootId }, { $set: { [path]: records[0] } }), ); await Promise.all(promises); @@ -157,6 +174,36 @@ export default class MongooseCollection extends BaseCollection { return results; } + private async _createForObjectSubfield(data: RecordData[], flatData: RecordData[]) { + if (data.length > 1) throw new ValidationError('Trying to create multiple subrecords at once'); + + const fieldName = this.computeSubFieldName(); + + const entry = data[0]; + const flatEntry = flatData[0]; + const { parentId, ...rest } = entry; + + if (!parentId) throw new ValidationError('Trying to create a subrecord with no parent'); + + const [rootId, path] = splitId(`${parentId}.${fieldName}`); + + await this.model.updateOne( + { _id: rootId }, + { + $set: { + [path]: rest, + }, + }, + ); + + return [ + { + _id: `${rootId}.${path}`, + ...flatEntry, + }, + ]; + } + private async _update(caller: Caller, filter: Filter, flatPatch: RecordData): Promise { const { asFields } = this.stack[this.stack.length - 1]; const patch = unflattenRecord(flatPatch, asFields, true); diff --git a/packages/datasource-mongoose/src/utils/pipeline/lookup.ts b/packages/datasource-mongoose/src/utils/pipeline/lookup.ts index 7ed3344f05..50359d69e7 100644 --- a/packages/datasource-mongoose/src/utils/pipeline/lookup.ts +++ b/packages/datasource-mongoose/src/utils/pipeline/lookup.ts @@ -10,21 +10,33 @@ import { Stack } from '../../types'; */ export default class LookupGenerator { static lookup(model: Model, stack: Stack, projection: Projection): PipelineStage[] { - const childSchema = MongooseSchema.fromModel(model).applyStack(stack, true).fields; + const schemaStack = stack.reduce( + (acc, stackElement) => { + const lastModel = acc[acc.length - 1]; - return this.lookupProjection(model.db.models, null, childSchema, projection); + return [...acc, lastModel.applyStack([stackElement], true)]; + }, + [MongooseSchema.fromModel(model)], + ); + + return this.lookupProjection( + model.db.models, + null, + schemaStack.map(s => s.fields), + projection, + ); } private static lookupProjection( models: Record>, currentPath: string, - schema: SchemaNode, + schemaStack: SchemaNode[], projection: Projection, ): PipelineStage[] { const pipeline = []; for (const [name, subProjection] of Object.entries(projection.relations)) - pipeline.push(...this.lookupRelation(models, currentPath, schema, name, subProjection)); + pipeline.push(...this.lookupRelation(models, currentPath, schemaStack, name, subProjection)); return pipeline; } @@ -32,16 +44,23 @@ export default class LookupGenerator { private static lookupRelation( models: Record>, currentPath: string, - schema: SchemaNode, + schemaStack: SchemaNode[], name: string, subProjection: Projection, ): PipelineStage[] { const as = currentPath ? `${currentPath}.${name}` : name; + if (!schemaStack.length) { + throw new Error(`Unexpected relation: '${name}' (empty schema stack)`); + } + + const lastSchema = schemaStack[schemaStack.length - 1]; + const previousSchemas = schemaStack.slice(0, schemaStack.length - 1); + // Native many to one relation if (name.endsWith('__manyToOne')) { const foreignKeyName = name.substring(0, name.length - '__manyToOne'.length); - const model = models[schema[foreignKeyName].options.ref]; + const model = models[lastSchema[foreignKeyName].options.ref]; const from = model.collection.collectionName; const localField = currentPath ? `${currentPath}.${foreignKeyName}` : foreignKeyName; @@ -55,13 +74,18 @@ export default class LookupGenerator { { $unwind: { path: `$${as}`, preserveNullAndEmptyArrays: true } }, // Recurse to get relations of relations - ...this.lookupProjection(models, as, subSchema, subProjection), + ...this.lookupProjection(models, as, [...schemaStack, subSchema], subProjection), ]; } - // Fake relation or inverse of fake relation - if (name === 'parent' || schema[name]) { - return this.lookupProjection(models, as, schema[name], subProjection); + // Inverse of fake relation + if (name === 'parent' && previousSchemas.length) { + return this.lookupProjection(models, as, previousSchemas, subProjection); + } + + // Fake relation + if (lastSchema[name]) { + return this.lookupProjection(models, as, [...schemaStack, lastSchema[name]], subProjection); } // We should have handled all possible cases. diff --git a/packages/datasource-mongoose/test/integration/flattener/collection_create.test.ts b/packages/datasource-mongoose/test/integration/flattener/collection_create.test.ts index 74c2f013fe..8f3004ad4e 100644 --- a/packages/datasource-mongoose/test/integration/flattener/collection_create.test.ts +++ b/packages/datasource-mongoose/test/integration/flattener/collection_create.test.ts @@ -169,7 +169,7 @@ describe('Complex flattening', () => { ); }); - it('creating a subModel should fail', async () => { + it('creating a subModel should work', async () => { connection = await setupFlattener('collection_flattener_create'); const dataSource = new MongooseDatasource(connection, { @@ -180,10 +180,28 @@ describe('Complex flattening', () => { .getCollection('cars') .create(caller, [{ name: 'my fiesta', wheelSize: 12 }]); - await expect( - dataSource - .getCollection('cars_engine') - .create(caller, [{ parentId: car._id, horsePower: '12' }]), - ).rejects.toThrow('Trying to create subrecords on a non-array field'); + const result = await dataSource + .getCollection('cars_engine') + .create(caller, [{ parentId: car._id, horsePower: '12' }]); + + const doc = await connection.model('cars').findOne({ _id: car._id }); + + expect(result).toEqual( + expect.arrayContaining([ + expect.objectContaining({ + horsePower: '12', + }), + ]), + ); + + expect(doc).toEqual( + expect.objectContaining({ + name: 'my fiesta', + wheelSize: 12, + engine: expect.objectContaining({ + horsePower: '12', + }), + }), + ); }); }); diff --git a/packages/datasource-mongoose/test/integration/flattener/collection_list.test.ts b/packages/datasource-mongoose/test/integration/flattener/collection_list.test.ts index 3da7d48a4f..c6322aad50 100644 --- a/packages/datasource-mongoose/test/integration/flattener/collection_list.test.ts +++ b/packages/datasource-mongoose/test/integration/flattener/collection_list.test.ts @@ -145,6 +145,40 @@ describe('Complex flattening', () => { }), ]); }); + + it('should correctly retrieve one nested record', async () => { + connection = await setupFlattener('collection_flattener_list'); + const dataSource = new MongooseDatasource(connection, { + flattenMode: 'manual', + flattenOptions: { + cars: { asModels: ['engine'] }, + }, + }); + + const [car] = await dataSource + .getCollection('cars') + .create(caller, [{ name: 'my fiesta', wheelSize: 12, engine: { horsePower: 98 } }]); + + const records = await dataSource.getCollection('cars_engine').list( + caller, + new Filter({ + conditionTree: new ConditionTreeLeaf('_id', 'Equal', `${car._id}.engine`), + }), + new Projection('_id', 'horsePower', 'parentId', 'parent:_id', 'parent:engine:horsePower'), + ); + + expect(records).toEqual([ + expect.objectContaining({ + horsePower: '98', + parent: { + _id: car._id, + engine: { + horsePower: '98', + }, + }, + }), + ]); + }); }); }); }); From 50fa4a0804b5a7adf74dac2d0448453c35a2b0fe Mon Sep 17 00:00:00 2001 From: Guillaume Gautreau Date: Wed, 13 Sep 2023 09:05:34 +0200 Subject: [PATCH 02/10] fix: create elements in nested asModels/asFields --- .../datasource-mongoose/src/utils/pipeline/lookup.ts | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/packages/datasource-mongoose/src/utils/pipeline/lookup.ts b/packages/datasource-mongoose/src/utils/pipeline/lookup.ts index 50359d69e7..9190a33ca9 100644 --- a/packages/datasource-mongoose/src/utils/pipeline/lookup.ts +++ b/packages/datasource-mongoose/src/utils/pipeline/lookup.ts @@ -11,10 +11,11 @@ import { Stack } from '../../types'; export default class LookupGenerator { static lookup(model: Model, stack: Stack, projection: Projection): PipelineStage[] { const schemaStack = stack.reduce( - (acc, stackElement) => { - const lastModel = acc[acc.length - 1]; - - return [...acc, lastModel.applyStack([stackElement], true)]; + (acc, _, index) => { + return [ + ...acc, + MongooseSchema.fromModel(model).applyStack(stack.slice(0, index + 1), true), + ]; }, [MongooseSchema.fromModel(model)], ); From c06d28e0370c09c093079fbbb08cf8c6fe246fcc Mon Sep 17 00:00:00 2001 From: Guillaume Gautreau Date: Wed, 13 Sep 2023 10:38:33 +0200 Subject: [PATCH 03/10] refactor: remove useless case --- packages/datasource-mongoose/src/utils/pipeline/lookup.ts | 4 ---- 1 file changed, 4 deletions(-) diff --git a/packages/datasource-mongoose/src/utils/pipeline/lookup.ts b/packages/datasource-mongoose/src/utils/pipeline/lookup.ts index 9190a33ca9..2a7cea5f90 100644 --- a/packages/datasource-mongoose/src/utils/pipeline/lookup.ts +++ b/packages/datasource-mongoose/src/utils/pipeline/lookup.ts @@ -51,10 +51,6 @@ export default class LookupGenerator { ): PipelineStage[] { const as = currentPath ? `${currentPath}.${name}` : name; - if (!schemaStack.length) { - throw new Error(`Unexpected relation: '${name}' (empty schema stack)`); - } - const lastSchema = schemaStack[schemaStack.length - 1]; const previousSchemas = schemaStack.slice(0, schemaStack.length - 1); From a65a8d53992e8a4f64c23393fa39b375b7ac4941 Mon Sep 17 00:00:00 2001 From: Guillaume Gautreau Date: Wed, 13 Sep 2023 11:20:44 +0200 Subject: [PATCH 04/10] refactor: return early --- packages/datasource-mongoose/src/collection.ts | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/packages/datasource-mongoose/src/collection.ts b/packages/datasource-mongoose/src/collection.ts index 01dffeca65..e6bffbbeab 100644 --- a/packages/datasource-mongoose/src/collection.ts +++ b/packages/datasource-mongoose/src/collection.ts @@ -123,12 +123,10 @@ export default class MongooseCollection extends BaseCollection { private computeSubFieldName() { const lastStackStep = this.stack[this.stack.length - 1]; - const fieldName = - this.stack.length > 2 - ? lastStackStep.prefix.substring(this.stack[this.stack.length - 2].prefix.length + 1) - : lastStackStep.prefix; - return fieldName; + return this.stack.length > 2 + ? lastStackStep.prefix.substring(this.stack[this.stack.length - 2].prefix.length + 1) + : lastStackStep.prefix; } private async _createForArraySubfield( From e059fdb13b9c199910d1f578f71b7ccf1bec2b7c Mon Sep 17 00:00:00 2001 From: Guillaume Gautreau Date: Wed, 13 Sep 2023 11:21:42 +0200 Subject: [PATCH 05/10] refactor: remove useless variables --- packages/datasource-mongoose/src/collection.ts | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/packages/datasource-mongoose/src/collection.ts b/packages/datasource-mongoose/src/collection.ts index e6bffbbeab..a5a0899691 100644 --- a/packages/datasource-mongoose/src/collection.ts +++ b/packages/datasource-mongoose/src/collection.ts @@ -177,9 +177,7 @@ export default class MongooseCollection extends BaseCollection { const fieldName = this.computeSubFieldName(); - const entry = data[0]; - const flatEntry = flatData[0]; - const { parentId, ...rest } = entry; + const { parentId, ...rest } = data[0]; if (!parentId) throw new ValidationError('Trying to create a subrecord with no parent'); @@ -197,7 +195,7 @@ export default class MongooseCollection extends BaseCollection { return [ { _id: `${rootId}.${path}`, - ...flatEntry, + ...flatData[0], }, ]; } @@ -266,7 +264,6 @@ export default class MongooseCollection extends BaseCollection { return; } - const schema = MongooseSchema.fromModel(this.model).applyStack(this.stack); const idsByPath = groupIdsByPath(ids); if (schema.isArray) { From 2049e5b16dd26259e73796b1852062fc08148a37 Mon Sep 17 00:00:00 2001 From: Guillaume Gautreau Date: Wed, 13 Sep 2023 11:22:45 +0200 Subject: [PATCH 06/10] refactor: revert unwanted change --- packages/datasource-mongoose/src/collection.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/packages/datasource-mongoose/src/collection.ts b/packages/datasource-mongoose/src/collection.ts index a5a0899691..55153e9397 100644 --- a/packages/datasource-mongoose/src/collection.ts +++ b/packages/datasource-mongoose/src/collection.ts @@ -264,6 +264,7 @@ export default class MongooseCollection extends BaseCollection { return; } + const schema = MongooseSchema.fromModel(this.model).applyStack(this.stack); const idsByPath = groupIdsByPath(ids); if (schema.isArray) { From 2aa2e4942d639acc5ecf0425a70f7c58057b652f Mon Sep 17 00:00:00 2001 From: alban bertolini Date: Wed, 13 Sep 2023 11:25:11 +0200 Subject: [PATCH 07/10] refactor: win two lines --- packages/datasource-mongoose/src/collection.ts | 16 ++-------------- 1 file changed, 2 insertions(+), 14 deletions(-) diff --git a/packages/datasource-mongoose/src/collection.ts b/packages/datasource-mongoose/src/collection.ts index 55153e9397..1abedc098d 100644 --- a/packages/datasource-mongoose/src/collection.ts +++ b/packages/datasource-mongoose/src/collection.ts @@ -183,21 +183,9 @@ export default class MongooseCollection extends BaseCollection { const [rootId, path] = splitId(`${parentId}.${fieldName}`); - await this.model.updateOne( - { _id: rootId }, - { - $set: { - [path]: rest, - }, - }, - ); + await this.model.updateOne({ _id: rootId }, { $set: { [path]: rest } }); - return [ - { - _id: `${rootId}.${path}`, - ...flatData[0], - }, - ]; + return [{ _id: `${rootId}.${path}`, ...flatData[0] }]; } private async _update(caller: Caller, filter: Filter, flatPatch: RecordData): Promise { From e6d46430063ac59944da5f361838e5f7185c5300 Mon Sep 17 00:00:00 2001 From: Guillaume Gautreau Date: Wed, 13 Sep 2023 11:30:12 +0200 Subject: [PATCH 08/10] refactor: check that creation is valid --- .../datasource-mongoose/src/collection.ts | 1 + .../flattener/collection_create.test.ts | 37 ++++++++++++++++++- 2 files changed, 37 insertions(+), 1 deletion(-) diff --git a/packages/datasource-mongoose/src/collection.ts b/packages/datasource-mongoose/src/collection.ts index 55153e9397..b1674834e9 100644 --- a/packages/datasource-mongoose/src/collection.ts +++ b/packages/datasource-mongoose/src/collection.ts @@ -174,6 +174,7 @@ export default class MongooseCollection extends BaseCollection { private async _createForObjectSubfield(data: RecordData[], flatData: RecordData[]) { if (data.length > 1) throw new ValidationError('Trying to create multiple subrecords at once'); + if (data.length === 0) throw new ValidationError('Trying to create without data'); const fieldName = this.computeSubFieldName(); diff --git a/packages/datasource-mongoose/test/integration/flattener/collection_create.test.ts b/packages/datasource-mongoose/test/integration/flattener/collection_create.test.ts index 8f3004ad4e..290278b9d6 100644 --- a/packages/datasource-mongoose/test/integration/flattener/collection_create.test.ts +++ b/packages/datasource-mongoose/test/integration/flattener/collection_create.test.ts @@ -169,7 +169,7 @@ describe('Complex flattening', () => { ); }); - it('creating a subModel should work', async () => { + it('should create a model', async () => { connection = await setupFlattener('collection_flattener_create'); const dataSource = new MongooseDatasource(connection, { @@ -204,4 +204,39 @@ describe('Complex flattening', () => { }), ); }); + + it('should throw an error when creating a model with an empty list of elements', async () => { + connection = await setupFlattener('collection_flattener_create'); + + const dataSource = new MongooseDatasource(connection, { + asModels: { cars: ['engine', 'engine.fuel'] }, + }); + + await dataSource.getCollection('cars').create(caller, [{ name: 'my fiesta', wheelSize: 12 }]); + + await expect(dataSource.getCollection('cars_engine').create(caller, [])).rejects.toThrow( + 'Trying to create without data', + ); + }); + + it('should throw an error when creating multiple objects in an object model', async () => { + connection = await setupFlattener('collection_flattener_create'); + + const dataSource = new MongooseDatasource(connection, { + asModels: { cars: ['engine', 'engine.fuel'] }, + }); + + const [car] = await dataSource + .getCollection('cars') + .create(caller, [{ name: 'my fiesta', wheelSize: 12 }]); + + await dataSource.getCollection('cars').create(caller, [{ name: 'my fiesta', wheelSize: 12 }]); + + await expect( + dataSource.getCollection('cars_engine').create(caller, [ + { parentId: car._id, horsePower: '12' }, + { parentId: car._id, horsePower: '13' }, + ]), + ).rejects.toThrow('Trying to create multiple subrecords at once'); + }); }); From 9080db5333a4268fca8224c739f055525886810c Mon Sep 17 00:00:00 2001 From: Guillaume Gautreau Date: Wed, 13 Sep 2023 11:34:51 +0200 Subject: [PATCH 09/10] refactor: remove useless code --- packages/datasource-mongoose/src/collection.ts | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/packages/datasource-mongoose/src/collection.ts b/packages/datasource-mongoose/src/collection.ts index 65814e6dbb..09a30723d1 100644 --- a/packages/datasource-mongoose/src/collection.ts +++ b/packages/datasource-mongoose/src/collection.ts @@ -159,12 +159,10 @@ export default class MongooseCollection extends BaseCollection { // Apply the modifications to the root document. const promises = Object.values(updates).map(({ rootId, path, records }) => - schema.isArray - ? this.model.updateOne( - { _id: rootId }, - { $push: { [path]: { $position: 0, $each: records } } }, - ) - : this.model.updateOne({ _id: rootId }, { $set: { [path]: records[0] } }), + this.model.updateOne( + { _id: rootId }, + { $push: { [path]: { $position: 0, $each: records } } }, + ), ); await Promise.all(promises); From f518cbbfc8e0f55df1665ede0133e7258fe3a7be Mon Sep 17 00:00:00 2001 From: Guillaume Gautreau Date: Wed, 13 Sep 2023 11:37:11 +0200 Subject: [PATCH 10/10] refactor: remove prefix before private methods --- packages/datasource-mongoose/src/collection.ts | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/packages/datasource-mongoose/src/collection.ts b/packages/datasource-mongoose/src/collection.ts index 09a30723d1..1857d505d2 100644 --- a/packages/datasource-mongoose/src/collection.ts +++ b/packages/datasource-mongoose/src/collection.ts @@ -115,10 +115,10 @@ export default class MongooseCollection extends BaseCollection { const schema = MongooseSchema.fromModel(this.model).applyStack(this.stack); if (schema.isArray) { - return this._createForArraySubfield(data, flatData, schema); + return this.createForArraySubfield(data, flatData, schema); } - return this._createForObjectSubfield(data, flatData); + return this.createForObjectSubfield(data, flatData); } private computeSubFieldName() { @@ -129,7 +129,7 @@ export default class MongooseCollection extends BaseCollection { : lastStackStep.prefix; } - private async _createForArraySubfield( + private async createForArraySubfield( data: RecordData[], flatData: RecordData[], schema: MongooseSchema, @@ -170,7 +170,7 @@ export default class MongooseCollection extends BaseCollection { return results; } - private async _createForObjectSubfield(data: RecordData[], flatData: RecordData[]) { + private async createForObjectSubfield(data: RecordData[], flatData: RecordData[]) { if (data.length > 1) throw new ValidationError('Trying to create multiple subrecords at once'); if (data.length === 0) throw new ValidationError('Trying to create without data');