From e1ff1a1b7196b2f22d484be1d0537b0198cfdaed Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Tue, 13 Sep 2022 17:33:11 +0200 Subject: [PATCH 1/3] fix(shell-api): allow pipeline-style bulk update operations MONGOSH-1281 Fix cloning of the update documents to allow arrays as well as objects, so that users can use aggregation pipelines for bulk updates. Also, remove the buggy `hint` and `arrayFilters` implementation that was storing extra properties in the wrong place on the outgoing documents, and instead fully rely on driver helpers to implement these. --- packages/shell-api/src/bulk.spec.ts | 17 +++---- packages/shell-api/src/bulk.ts | 34 ++++--------- packages/shell-api/src/helpers.ts | 5 ++ packages/shell-api/src/integration.spec.ts | 57 ++++++++++++++++++++++ 4 files changed, 78 insertions(+), 35 deletions(-) diff --git a/packages/shell-api/src/bulk.spec.ts b/packages/shell-api/src/bulk.spec.ts index b6b2890777..d15a74bc7d 100644 --- a/packages/shell-api/src/bulk.spec.ts +++ b/packages/shell-api/src/bulk.spec.ts @@ -371,12 +371,10 @@ describe('Bulk API', () => { it('calls serviceProviderBulkOp.update and returns parent when hint/arrayFilter set', () => { bulkFindOp.hint({ hint: 1 }); - // bulkFindOp.arrayFilters(['filter']); + bulkFindOp.arrayFilters([{ x: 1 }]); bulkFindOp.update({ updateDoc: 1 }); expect(innerStub.update).to.have.been.calledWith({ - updateDoc: 1, - hint: { hint: 1 }, - // arrayFilters: [ 'filter' ] + updateDoc: 1 }); expect(bulk._batchCounts.nUpdateOps).to.equal(1); }); @@ -400,12 +398,10 @@ describe('Bulk API', () => { it('calls serviceProviderBulkOp.updateOne and returns parent when hint/arrayFilter set', () => { bulkFindOp.hint({ hint: 1 }); - // bulkFindOp.arrayFilters(['filter']); + bulkFindOp.arrayFilters([{ x: 1 }]); bulkFindOp.updateOne({ updateOneDoc: 1 }); expect(innerStub.updateOne).to.have.been.calledWith({ - updateOneDoc: 1, - hint: { hint: 1 }, - // arrayFilters: [ 'filter' ] + updateOneDoc: 1 }); expect(bulk._batchCounts.nUpdateOps).to.equal(1); }); @@ -432,8 +428,7 @@ describe('Bulk API', () => { bulkFindOp.hint({ hint: 1 }); bulkFindOp.replaceOne({ replaceOneDoc: 1 }); expect(innerStub.replaceOne).to.have.been.calledWith({ - replaceOneDoc: 1, - hint: { hint: 1 } + replaceOneDoc: 1 }); expect(bulk._batchCounts.nUpdateOps).to.equal(1); }); @@ -452,7 +447,7 @@ describe('Bulk API', () => { it('sets the attribute and returns self', () => { const attr = { hint: 1 }; expect(bulkFindOp.hint(attr)).to.equal(bulkFindOp); - expect(bulkFindOp._hint).to.deep.equal(attr); + expect(innerStub.hint).to.have.been.calledWith(attr); }); }); describe('arrayFilters', () => { diff --git a/packages/shell-api/src/bulk.ts b/packages/shell-api/src/bulk.ts index b4c6359448..87ece1dfc7 100644 --- a/packages/shell-api/src/bulk.ts +++ b/packages/shell-api/src/bulk.ts @@ -11,7 +11,7 @@ import { CollationOptions } from '@mongosh/service-provider-core'; import { asPrintable } from './enums'; -import { assertArgsDefinedType } from './helpers'; +import { assertArgsDefinedType, shallowClone } from './helpers'; import { BulkWriteResult } from './result'; import type Collection from './collection'; @@ -19,8 +19,6 @@ import type Collection from './collection'; export class BulkFindOp extends ShellApiWithMongoClass { _serviceProviderBulkFindOp: FindOperators; _parentBulk: Bulk; - _hint: Document | undefined; - _arrayFilters: Document[] | undefined; constructor(innerFind: FindOperators, parentBulk: Bulk) { super(); this._serviceProviderBulkFindOp = innerFind; @@ -53,7 +51,10 @@ export class BulkFindOp extends ShellApiWithMongoClass { @apiVersions([1]) hint(hintDoc: Document): BulkFindOp { assertArgsDefinedType([hintDoc], [true], 'BulkFindOp.hint'); - this._hint = hintDoc; + /* eslint-disable chai-friendly/no-unused-expressions */ + // @ts-expect-error NODE-4634 + this._serviceProviderBulkFindOp.hint?.(hintDoc); + /* eslint-ensable chai-friendly/no-unused-expressions */ return this; } @@ -92,41 +93,26 @@ export class BulkFindOp extends ShellApiWithMongoClass { replaceOne(replacement: Document): Bulk { this._parentBulk._batchCounts.nUpdateOps++; assertArgsDefinedType([replacement], [true], 'BulkFindOp.replacement'); - const op = { ...replacement }; - if (this._hint) { - op.hint = this._hint; - } + const op = shallowClone(replacement); this._serviceProviderBulkFindOp.replaceOne(op); return this._parentBulk; } @returnType('Bulk') @apiVersions([1]) - updateOne(update: Document): Bulk { + updateOne(update: Document | Document[]): Bulk { this._parentBulk._batchCounts.nUpdateOps++; assertArgsDefinedType([update], [true], 'BulkFindOp.update'); - const op = { ...update }; - if (this._hint) { - op.hint = this._hint; - } - if (this._arrayFilters) { - op.arrayFilters = this._arrayFilters; - } + const op = shallowClone(update); this._serviceProviderBulkFindOp.updateOne(op); return this._parentBulk; } @returnType('Bulk') - update(update: Document): Bulk { + update(update: Document | Document[]): Bulk { this._parentBulk._batchCounts.nUpdateOps++; assertArgsDefinedType([update], [true], 'BulkFindOp.update'); - const op = { ...update }; - if (this._hint) { - op.hint = this._hint; - } - if (this._arrayFilters) { - op.arrayFilters = this._arrayFilters; - } + const op = shallowClone(update); this._serviceProviderBulkFindOp.update(op); return this._parentBulk; } diff --git a/packages/shell-api/src/helpers.ts b/packages/shell-api/src/helpers.ts index 842b6a63e3..76a285003e 100644 --- a/packages/shell-api/src/helpers.ts +++ b/packages/shell-api/src/helpers.ts @@ -804,3 +804,8 @@ improvements and to suggest MongoDB products and deployment options to you. To enable free monitoring, run the following command: db.enableFreeMonitoring() To permanently disable this reminder, run the following command: db.disableFreeMonitoring() `; + +export function shallowClone(input: T): T { + if (!input || typeof input !== 'object') return input; + return Array.isArray(input) ? ([...input] as unknown as T) : { ...input }; +} diff --git a/packages/shell-api/src/integration.spec.ts b/packages/shell-api/src/integration.spec.ts index 6b01111ba7..938dbb200b 100644 --- a/packages/shell-api/src/integration.spec.ts +++ b/packages/shell-api/src/integration.spec.ts @@ -1855,6 +1855,63 @@ describe('Shell API (integration)', function() { expect(op.operations.length).to.equal(1); }); }); + describe('update() with pipeline update', () => { + beforeEach(async() => { + bulk = await collection[m](); + for (let i = 0; i < size; i++) { + await collection.insertOne({ x: i }); + } + expect(await collection.countDocuments()).to.equal(size); + expect(await collection.countDocuments({ y: { $exists: true } })).to.equal(0); + bulk.find({ y: 0 }).upsert().update([{ $set: { y: 1 } }]); + await bulk.execute(); + }); + afterEach(async() => { + await collection.drop(); + }); + it('toJSON returns correctly', () => { + expect(bulk.toJSON()).to.deep.equal({ nInsertOps: 0, nUpdateOps: 1, nRemoveOps: 0, nBatches: 1 }); + }); + it('executes', async() => { + expect(await collection.countDocuments()).to.equal(size + 1); + expect(await collection.countDocuments({ y: { $exists: true } })).to.equal(1); + }); + it('getOperations returns correctly', () => { + const ops = bulk.getOperations(); + expect(ops.length).to.equal(1); + const op = ops[0]; + expect(op.originalZeroIndex).to.equal(0); + expect(op.batchType).to.equal(2); + expect(op.operations.length).to.equal(1); + }); + }); + describe('updateOne() with pipeline update', () => { + beforeEach(async() => { + bulk = await collection[m](); + for (let i = 0; i < size; i++) { + await collection.insertOne({ x: i }); + } + expect(await collection.countDocuments()).to.equal(size); + expect(await collection.countDocuments({ y: { $exists: true } })).to.equal(0); + bulk.find({ y: 0 }).upsert().updateOne([{ $set: { y: 1 } }]); + await bulk.execute(); + }); + it('toJSON returns correctly', () => { + expect(bulk.toJSON()).to.deep.equal({ nInsertOps: 0, nUpdateOps: 1, nRemoveOps: 0, nBatches: 1 }); + }); + it('executes', async() => { + expect(await collection.countDocuments()).to.equal(size + 1); + expect(await collection.countDocuments({ y: { $exists: true } })).to.equal(1); + }); + it('getOperations returns correctly', () => { + const ops = bulk.getOperations(); + expect(ops.length).to.equal(1); + const op = ops[0]; + expect(op.originalZeroIndex).to.equal(0); + expect(op.batchType).to.equal(2); + expect(op.operations.length).to.equal(1); + }); + }); describe('error states', () => { it('cannot be executed twice', async() => { bulk = await collection[m](); From 67b3383c942d59ad19f836b19be291a2d63d0b1d Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Wed, 14 Sep 2022 11:09:09 +0200 Subject: [PATCH 2/3] fixup: skip tests on 4.0.x --- packages/shell-api/src/integration.spec.ts | 111 +++++++++++---------- 1 file changed, 57 insertions(+), 54 deletions(-) diff --git a/packages/shell-api/src/integration.spec.ts b/packages/shell-api/src/integration.spec.ts index 938dbb200b..222f6aeb38 100644 --- a/packages/shell-api/src/integration.spec.ts +++ b/packages/shell-api/src/integration.spec.ts @@ -1855,61 +1855,64 @@ describe('Shell API (integration)', function() { expect(op.operations.length).to.equal(1); }); }); - describe('update() with pipeline update', () => { - beforeEach(async() => { - bulk = await collection[m](); - for (let i = 0; i < size; i++) { - await collection.insertOne({ x: i }); - } - expect(await collection.countDocuments()).to.equal(size); - expect(await collection.countDocuments({ y: { $exists: true } })).to.equal(0); - bulk.find({ y: 0 }).upsert().update([{ $set: { y: 1 } }]); - await bulk.execute(); - }); - afterEach(async() => { - await collection.drop(); - }); - it('toJSON returns correctly', () => { - expect(bulk.toJSON()).to.deep.equal({ nInsertOps: 0, nUpdateOps: 1, nRemoveOps: 0, nBatches: 1 }); - }); - it('executes', async() => { - expect(await collection.countDocuments()).to.equal(size + 1); - expect(await collection.countDocuments({ y: { $exists: true } })).to.equal(1); - }); - it('getOperations returns correctly', () => { - const ops = bulk.getOperations(); - expect(ops.length).to.equal(1); - const op = ops[0]; - expect(op.originalZeroIndex).to.equal(0); - expect(op.batchType).to.equal(2); - expect(op.operations.length).to.equal(1); - }); - }); - describe('updateOne() with pipeline update', () => { - beforeEach(async() => { - bulk = await collection[m](); - for (let i = 0; i < size; i++) { - await collection.insertOne({ x: i }); - } - expect(await collection.countDocuments()).to.equal(size); - expect(await collection.countDocuments({ y: { $exists: true } })).to.equal(0); - bulk.find({ y: 0 }).upsert().updateOne([{ $set: { y: 1 } }]); - await bulk.execute(); - }); - it('toJSON returns correctly', () => { - expect(bulk.toJSON()).to.deep.equal({ nInsertOps: 0, nUpdateOps: 1, nRemoveOps: 0, nBatches: 1 }); - }); - it('executes', async() => { - expect(await collection.countDocuments()).to.equal(size + 1); - expect(await collection.countDocuments({ y: { $exists: true } })).to.equal(1); + context('with >= 4.2 server', () => { + skipIfServerVersion(testServer, '< 4.2'); + describe('update() with pipeline update', () => { + beforeEach(async() => { + bulk = await collection[m](); + for (let i = 0; i < size; i++) { + await collection.insertOne({ x: i }); + } + expect(await collection.countDocuments()).to.equal(size); + expect(await collection.countDocuments({ y: { $exists: true } })).to.equal(0); + bulk.find({ y: 0 }).upsert().update([{ $set: { y: 1 } }]); + await bulk.execute(); + }); + afterEach(async() => { + await collection.drop(); + }); + it('toJSON returns correctly', () => { + expect(bulk.toJSON()).to.deep.equal({ nInsertOps: 0, nUpdateOps: 1, nRemoveOps: 0, nBatches: 1 }); + }); + it('executes', async() => { + expect(await collection.countDocuments()).to.equal(size + 1); + expect(await collection.countDocuments({ y: { $exists: true } })).to.equal(1); + }); + it('getOperations returns correctly', () => { + const ops = bulk.getOperations(); + expect(ops.length).to.equal(1); + const op = ops[0]; + expect(op.originalZeroIndex).to.equal(0); + expect(op.batchType).to.equal(2); + expect(op.operations.length).to.equal(1); + }); }); - it('getOperations returns correctly', () => { - const ops = bulk.getOperations(); - expect(ops.length).to.equal(1); - const op = ops[0]; - expect(op.originalZeroIndex).to.equal(0); - expect(op.batchType).to.equal(2); - expect(op.operations.length).to.equal(1); + describe('updateOne() with pipeline update', () => { + beforeEach(async() => { + bulk = await collection[m](); + for (let i = 0; i < size; i++) { + await collection.insertOne({ x: i }); + } + expect(await collection.countDocuments()).to.equal(size); + expect(await collection.countDocuments({ y: { $exists: true } })).to.equal(0); + bulk.find({ y: 0 }).upsert().updateOne([{ $set: { y: 1 } }]); + await bulk.execute(); + }); + it('toJSON returns correctly', () => { + expect(bulk.toJSON()).to.deep.equal({ nInsertOps: 0, nUpdateOps: 1, nRemoveOps: 0, nBatches: 1 }); + }); + it('executes', async() => { + expect(await collection.countDocuments()).to.equal(size + 1); + expect(await collection.countDocuments({ y: { $exists: true } })).to.equal(1); + }); + it('getOperations returns correctly', () => { + const ops = bulk.getOperations(); + expect(ops.length).to.equal(1); + const op = ops[0]; + expect(op.originalZeroIndex).to.equal(0); + expect(op.batchType).to.equal(2); + expect(op.operations.length).to.equal(1); + }); }); }); describe('error states', () => { From c5010d58f23625d025e13cb39364e887a25689c4 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Mon, 19 Sep 2022 19:43:00 +0200 Subject: [PATCH 3/3] fixup: drop ts-expect-error after driver bump on main --- packages/shell-api/src/bulk.ts | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/packages/shell-api/src/bulk.ts b/packages/shell-api/src/bulk.ts index 87ece1dfc7..96e04d54d8 100644 --- a/packages/shell-api/src/bulk.ts +++ b/packages/shell-api/src/bulk.ts @@ -51,10 +51,7 @@ export class BulkFindOp extends ShellApiWithMongoClass { @apiVersions([1]) hint(hintDoc: Document): BulkFindOp { assertArgsDefinedType([hintDoc], [true], 'BulkFindOp.hint'); - /* eslint-disable chai-friendly/no-unused-expressions */ - // @ts-expect-error NODE-4634 - this._serviceProviderBulkFindOp.hint?.(hintDoc); - /* eslint-ensable chai-friendly/no-unused-expressions */ + this._serviceProviderBulkFindOp.hint(hintDoc); return this; }