From c093a4536a75799fa1b5f5fe8b8b9c3a9ba12dbc Mon Sep 17 00:00:00 2001 From: Bailey Pearson Date: Thu, 7 Mar 2024 11:02:28 -0700 Subject: [PATCH 1/4] add tests for pkFactory in bulk write --- test/integration/crud/bulk.test.ts | 71 +++++++++++++++++++++++++++++- 1 file changed, 70 insertions(+), 1 deletion(-) diff --git a/test/integration/crud/bulk.test.ts b/test/integration/crud/bulk.test.ts index a5f3f9c468..44d2c2437d 100644 --- a/test/integration/crud/bulk.test.ts +++ b/test/integration/crud/bulk.test.ts @@ -3,6 +3,7 @@ import * as crypto from 'crypto'; import { type Collection, + Double, Long, MongoBatchReExecutionError, MongoBulkWriteError, @@ -65,16 +66,84 @@ describe('Bulk', function () { context('when called with a valid operation', function () { it('should not throw a MongoInvalidArgument error', async function () { try { - client.db('test').collection('test').initializeUnorderedBulkOp().raw({ insertOne: {} }); + client + .db('test') + .collection('test') + .initializeUnorderedBulkOp() + .raw({ insertOne: { document: {} } }); } catch (error) { expect(error).not.to.exist; } }); }); + + it('supports the legacy specification (no nested document field)', async function () { + await client + .db('test') + .collection('test') + .initializeUnorderedBulkOp() + // @ts-expect-error Not allowed in TS, but allowed for legacy compat + .raw({ insertOne: { name: 'john doe' } }) + .execute(); + const result = await client.db('test').collection('test').findOne({ name: 'john doe' }); + expect(result).to.exist; + }); }); }); describe('Collection', function () { + describe('when a pkFactory is set on the client', function () { + let client: MongoClient; + const pkFactory = { + count: 0, + createPk: function () { + return new Double(this.count++); + } + }; + let collection: Collection; + + beforeEach(async function () { + client = this.configuration.newClient({}, { pkFactory, promoteValues: false }); + collection = client.db('integration').collection('pk_factory_tests'); + await collection.deleteMany({}); + }); + + afterEach(() => client.close()); + + it('insertMany() generates _ids using the pkFactory', async function () { + await collection.insertMany([{ name: 'john doe' }]); + const result = await collection.findOne({ name: 'john doe' }); + expect(result).to.have.property('_id').to.be.instanceOf(Double); + }); + + it('bulkWrite() generates _ids using the pkFactory', async function () { + await collection.bulkWrite([{ insertOne: { document: { name: 'john doe' } } }]); + const result = await collection.findOne({ name: 'john doe' }); + expect(result).to.have.property('_id').to.be.instanceOf(Double); + }); + + it('ordered bulk operations generate _ids using pkFactory', async function () { + await collection.initializeOrderedBulkOp().insert({ name: 'john doe' }).execute(); + const result = await collection.findOne({ name: 'john doe' }); + expect(result).to.have.property('_id').to.be.instanceOf(Double); + }); + + it('unordered bulk operations generate _ids using pkFactory', async function () { + await collection.initializeUnorderedBulkOp().insert({ name: 'john doe' }).execute(); + const result = await collection.findOne({ name: 'john doe' }); + expect(result).to.have.property('_id').to.be.instanceOf(Double); + }); + + it('bulkOperation.raw() with the legacy syntax (no nested document field) generates _ids using pkFactory', async function () { + await collection + .initializeOrderedBulkOp() + // @ts-expect-error Not allowed by TS, but still permitted. + .raw({ insertOne: { name: 'john doe' } }) + .execute(); + const result = await collection.findOne({ name: 'john doe' }); + expect(result).to.have.property('_id').to.be.instanceOf(Double); + }); + }); describe('#insertMany()', function () { context('when passed an invalid docs argument', function () { it('should throw a MongoInvalidArgument error', async function () { From eded423470516be0539da768a208ad6915bd68e3 Mon Sep 17 00:00:00 2001 From: Bailey Pearson Date: Thu, 7 Mar 2024 11:02:45 -0700 Subject: [PATCH 2/4] fix _id generation in bulk writes && misc cleanups --- src/bulk/common.ts | 56 ++++++++++++++---------------- src/operations/common_functions.ts | 21 ++++++++--- src/operations/insert.ts | 8 +++-- 3 files changed, 47 insertions(+), 38 deletions(-) diff --git a/src/bulk/common.ts b/src/bulk/common.ts index f891cfbdf1..82dbef9209 100644 --- a/src/bulk/common.ts +++ b/src/bulk/common.ts @@ -1,6 +1,6 @@ import { promisify } from 'util'; -import { type BSONSerializeOptions, type Document, ObjectId, resolveBSONOptions } from '../bson'; +import { type BSONSerializeOptions, type Document, resolveBSONOptions } from '../bson'; import type { Collection } from '../collection'; import { type AnyError, @@ -12,6 +12,7 @@ import { } from '../error'; import type { Filter, OneOrMore, OptionalId, UpdateFilter, WithoutId } from '../mongo_types'; import type { CollationOptions, CommandOperationOptions } from '../operations/command'; +import { maybeAddIdToDocuments } from '../operations/common_functions'; import { DeleteOperation, type DeleteStatement, makeDeleteStatement } from '../operations/delete'; import { executeOperation } from '../operations/execute_operation'; import { InsertOperation } from '../operations/insert'; @@ -917,7 +918,7 @@ export abstract class BulkOperationBase { * Create a new OrderedBulkOperation or UnorderedBulkOperation instance * @internal */ - constructor(collection: Collection, options: BulkWriteOptions, isOrdered: boolean) { + constructor(private collection: Collection, options: BulkWriteOptions, isOrdered: boolean) { // determine whether bulkOperation is ordered or unordered this.isOrdered = isOrdered; @@ -1032,9 +1033,9 @@ export abstract class BulkOperationBase { * ``` */ insert(document: Document): BulkOperationBase { - if (document._id == null && !shouldForceServerObjectId(this)) { - document._id = new ObjectId(); - } + maybeAddIdToDocuments(this.collection, document, { + forceServerObjectId: this.shouldForceServerObjectId() + }); return this.addToOperationsList(BatchType.INSERT, document); } @@ -1093,21 +1094,16 @@ export abstract class BulkOperationBase { throw new MongoInvalidArgumentError('Operation must be an object with an operation key'); } if ('insertOne' in op) { - const forceServerObjectId = shouldForceServerObjectId(this); - if (op.insertOne && op.insertOne.document == null) { - // NOTE: provided for legacy support, but this is a malformed operation - if (forceServerObjectId !== true && (op.insertOne as Document)._id == null) { - (op.insertOne as Document)._id = new ObjectId(); - } - - return this.addToOperationsList(BatchType.INSERT, op.insertOne); - } + const forceServerObjectId = this.shouldForceServerObjectId(); + const document = + op.insertOne && op.insertOne.document == null + ? // NOTE: provided for legacy support, but this is a malformed operation + (op.insertOne as Document) + : op.insertOne.document; - if (forceServerObjectId !== true && op.insertOne.document._id == null) { - op.insertOne.document._id = new ObjectId(); - } + maybeAddIdToDocuments(this.collection, document, { forceServerObjectId }); - return this.addToOperationsList(BatchType.INSERT, op.insertOne.document); + return this.addToOperationsList(BatchType.INSERT, document); } if ('replaceOne' in op || 'updateOne' in op || 'updateMany' in op) { @@ -1268,6 +1264,18 @@ export abstract class BulkOperationBase { batchType: BatchType, document: Document | UpdateStatement | DeleteStatement ): this; + + private shouldForceServerObjectId(): boolean { + if (typeof this.s.options.forceServerObjectId === 'boolean') { + return this.s.options.forceServerObjectId; + } + + if (typeof this.s.collection.s.db.options?.forceServerObjectId === 'boolean') { + return this.s.collection.s.db.options?.forceServerObjectId; + } + + return false; + } } Object.defineProperty(BulkOperationBase.prototype, 'length', { @@ -1277,18 +1285,6 @@ Object.defineProperty(BulkOperationBase.prototype, 'length', { } }); -function shouldForceServerObjectId(bulkOperation: BulkOperationBase): boolean { - if (typeof bulkOperation.s.options.forceServerObjectId === 'boolean') { - return bulkOperation.s.options.forceServerObjectId; - } - - if (typeof bulkOperation.s.collection.s.db.options?.forceServerObjectId === 'boolean') { - return bulkOperation.s.collection.s.db.options?.forceServerObjectId; - } - - return false; -} - function isInsertBatch(batch: Batch): boolean { return batch.batchType === BatchType.INSERT; } diff --git a/src/operations/common_functions.ts b/src/operations/common_functions.ts index 6f79db71d3..785ecbaf12 100644 --- a/src/operations/common_functions.ts +++ b/src/operations/common_functions.ts @@ -43,11 +43,21 @@ export async function indexInformation( return info; } -export function prepareDocs( +export function maybeAddIdToDocuments( coll: Collection, docs: Document[], options: { forceServerObjectId?: boolean } -): Document[] { +): Document[]; +export function maybeAddIdToDocuments( + coll: Collection, + docs: Document, + options: { forceServerObjectId?: boolean } +): Document; +export function maybeAddIdToDocuments( + coll: Collection, + docOrDocs: Document[] | Document, + options: { forceServerObjectId?: boolean } +): Document[] | Document { const forceServerObjectId = typeof options.forceServerObjectId === 'boolean' ? options.forceServerObjectId @@ -55,14 +65,15 @@ export function prepareDocs( // no need to modify the docs if server sets the ObjectId if (forceServerObjectId === true) { - return docs; + return docOrDocs; } - return docs.map(doc => { + const transform = (doc: Document): Document => { if (doc._id == null) { doc._id = coll.s.pkFactory.createPk(); } return doc; - }); + }; + return Array.isArray(docOrDocs) ? docOrDocs.map(transform) : transform(docOrDocs); } diff --git a/src/operations/insert.ts b/src/operations/insert.ts index 27e84debf2..1788a06c1a 100644 --- a/src/operations/insert.ts +++ b/src/operations/insert.ts @@ -9,7 +9,7 @@ import type { MongoDBNamespace } from '../utils'; import { WriteConcern } from '../write_concern'; import { BulkWriteOperation } from './bulk_write'; import { CommandOperation, type CommandOperationOptions } from './command'; -import { prepareDocs } from './common_functions'; +import { maybeAddIdToDocuments } from './common_functions'; import { AbstractOperation, Aspect, defineAspects } from './operation'; /** @internal */ @@ -69,7 +69,7 @@ export interface InsertOneResult { export class InsertOneOperation extends InsertOperation { constructor(collection: Collection, doc: Document, options: InsertOneOptions) { - super(collection.s.namespace, prepareDocs(collection, [doc], options), options); + super(collection.s.namespace, maybeAddIdToDocuments(collection, [doc], options), options); } override async execute( @@ -131,7 +131,9 @@ export class InsertManyOperation extends AbstractOperation { const writeConcern = WriteConcern.fromOptions(options); const bulkWriteOperation = new BulkWriteOperation( coll, - prepareDocs(coll, this.docs, options).map(document => ({ insertOne: { document } })), + maybeAddIdToDocuments(coll, this.docs, options).map(document => ({ + insertOne: { document } + })), options ); From ab6b600ed0d9b0159974ed0d783f779cf2caa8ed Mon Sep 17 00:00:00 2001 From: Bailey Pearson Date: Fri, 8 Mar 2024 09:27:44 -0700 Subject: [PATCH 3/4] address comments --- src/bulk/common.ts | 15 +++---- src/mongo_client.ts | 2 +- src/operations/insert.ts | 2 +- test/integration/crud/bulk.test.ts | 1 + test/integration/crud/insert.test.js | 29 +++++++++++++- test/integration/crud/pk_factory.test.js | 51 ------------------------ 6 files changed, 36 insertions(+), 64 deletions(-) delete mode 100644 test/integration/crud/pk_factory.test.js diff --git a/src/bulk/common.ts b/src/bulk/common.ts index 82dbef9209..5de005449e 100644 --- a/src/bulk/common.ts +++ b/src/bulk/common.ts @@ -1097,7 +1097,7 @@ export abstract class BulkOperationBase { const forceServerObjectId = this.shouldForceServerObjectId(); const document = op.insertOne && op.insertOne.document == null - ? // NOTE: provided for legacy support, but this is a malformed operation + ? // TODO(NODE-6003): remove support for omitting the `documents` subdocument in bulk inserts (op.insertOne as Document) : op.insertOne.document; @@ -1266,15 +1266,10 @@ export abstract class BulkOperationBase { ): this; private shouldForceServerObjectId(): boolean { - if (typeof this.s.options.forceServerObjectId === 'boolean') { - return this.s.options.forceServerObjectId; - } - - if (typeof this.s.collection.s.db.options?.forceServerObjectId === 'boolean') { - return this.s.collection.s.db.options?.forceServerObjectId; - } - - return false; + return ( + this.s.options.forceServerObjectId === true || + this.s.collection.s.db.options?.forceServerObjectId === true + ); } } diff --git a/src/mongo_client.ts b/src/mongo_client.ts index 5ab24eee9b..57cff6753f 100644 --- a/src/mongo_client.ts +++ b/src/mongo_client.ts @@ -86,7 +86,7 @@ export interface Auth { /** @public */ export interface PkFactory { - createPk(): any; // TODO: when js-bson is typed, function should return some BSON type + createPk(): any; } /** @public */ diff --git a/src/operations/insert.ts b/src/operations/insert.ts index 1788a06c1a..9b3ba4b24b 100644 --- a/src/operations/insert.ts +++ b/src/operations/insert.ts @@ -131,7 +131,7 @@ export class InsertManyOperation extends AbstractOperation { const writeConcern = WriteConcern.fromOptions(options); const bulkWriteOperation = new BulkWriteOperation( coll, - maybeAddIdToDocuments(coll, this.docs, options).map(document => ({ + this.docs.map(document => ({ insertOne: { document } })), options diff --git a/test/integration/crud/bulk.test.ts b/test/integration/crud/bulk.test.ts index 44d2c2437d..d705e9ba69 100644 --- a/test/integration/crud/bulk.test.ts +++ b/test/integration/crud/bulk.test.ts @@ -144,6 +144,7 @@ describe('Bulk', function () { expect(result).to.have.property('_id').to.be.instanceOf(Double); }); }); + describe('#insertMany()', function () { context('when passed an invalid docs argument', function () { it('should throw a MongoInvalidArgument error', async function () { diff --git a/test/integration/crud/insert.test.js b/test/integration/crud/insert.test.js index 7f6ade3ca7..117858570c 100644 --- a/test/integration/crud/insert.test.js +++ b/test/integration/crud/insert.test.js @@ -71,6 +71,31 @@ describe('crud - insert', function () { await client.close(); }); + describe('when a pkFactory is set on the client', function () { + let client; + const pkFactory = { + count: 0, + createPk: function () { + return new Double(this.count++); + } + }; + let collection; + + beforeEach(async function () { + client = this.configuration.newClient({}, { pkFactory, promoteValues: false }); + collection = client.db('integration').collection('pk_factory_tests'); + await collection.deleteMany({}); + }); + + afterEach(() => client.close()); + + it('insertOne() generates _ids using the pkFactory', async function () { + await collection.insertOne({ name: 'john doe' }); + const result = await collection.findOne({ name: 'john doe' }); + expect(result).to.have.property('_id').to.be.instanceOf(Double); + }); + }); + it('Should correctly execute Collection.prototype.insertOne', function (done) { const configuration = this.configuration; let url = configuration.url(); @@ -135,6 +160,7 @@ describe('crud - insert', function () { it('insertMany returns the insertedIds and we can look up the documents', async function () { const db = client.db(); const collection = db.collection('test_multiple_insert'); + await collection.deleteMany({}); const docs = [{ a: 1 }, { a: 2 }]; const r = await collection.insertMany(docs); @@ -839,6 +865,7 @@ describe('crud - insert', function () { const db = client.db(); const collection = db.collection('Should_correctly_insert_object_with_timestamps'); + await collection.deleteMany({}); const { insertedId } = await collection.insertOne(doc); expect(insertedId.equals(doc._id)).to.be.true; @@ -1700,7 +1727,7 @@ describe('crud - insert', function () { try { db.collection(k.toString()); test.fail(false); - } catch (err) { } // eslint-disable-line + } catch (err) {} // eslint-disable-line client.close(done); }); diff --git a/test/integration/crud/pk_factory.test.js b/test/integration/crud/pk_factory.test.js deleted file mode 100644 index b81ef8b7c1..0000000000 --- a/test/integration/crud/pk_factory.test.js +++ /dev/null @@ -1,51 +0,0 @@ -'use strict'; -const { expect } = require('chai'); -const { setupDatabase } = require('../../integration/shared'); -const { ObjectId } = require('../../mongodb'); - -describe('PkFactory', function () { - before(function () { - return setupDatabase(this.configuration); - }); - - it('should create records with custom PK factory', { - metadata: { - requires: { topology: ['single', 'replicaset', 'sharded', 'ssl', 'heap', 'wiredtiger'] } - }, - - test: function (done) { - var configuration = this.configuration; - - // Custom factory (need to provide a 12 byte array); - var CustomPKFactory = { - createPk() { - return new ObjectId('a'.repeat(24)); - } - }; - - var client = configuration.newClient( - { - writeConcern: { w: 1 }, - maxPoolSize: 1 - }, - { - pkFactory: CustomPKFactory - } - ); - - client.connect(function (err, client) { - var db = client.db(configuration.db); - var collection = db.collection('test_custom_key'); - - collection.insert({ a: 1 }, { writeConcern: { w: 1 } }, function (err) { - expect(err).to.not.exist; - collection.find({ _id: new ObjectId('a'.repeat(24)) }).toArray(function (err, items) { - expect(items.length).to.equal(1); - - client.close(done); - }); - }); - }); - } - }); -}); From afcacffce8b55de9db8fd655f509a5402bbcfec9 Mon Sep 17 00:00:00 2001 From: Bailey Pearson Date: Fri, 8 Mar 2024 09:38:37 -0700 Subject: [PATCH 4/4] last comment --- test/integration/crud/bulk.test.ts | 10 +++++----- test/integration/crud/insert.test.js | 2 +- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/test/integration/crud/bulk.test.ts b/test/integration/crud/bulk.test.ts index d705e9ba69..7a33edbabe 100644 --- a/test/integration/crud/bulk.test.ts +++ b/test/integration/crud/bulk.test.ts @@ -113,25 +113,25 @@ describe('Bulk', function () { it('insertMany() generates _ids using the pkFactory', async function () { await collection.insertMany([{ name: 'john doe' }]); const result = await collection.findOne({ name: 'john doe' }); - expect(result).to.have.property('_id').to.be.instanceOf(Double); + expect(result).to.have.property('_id').to.have.property('_bsontype').to.equal('Double'); }); it('bulkWrite() generates _ids using the pkFactory', async function () { await collection.bulkWrite([{ insertOne: { document: { name: 'john doe' } } }]); const result = await collection.findOne({ name: 'john doe' }); - expect(result).to.have.property('_id').to.be.instanceOf(Double); + expect(result).to.have.property('_id').to.have.property('_bsontype').to.equal('Double'); }); it('ordered bulk operations generate _ids using pkFactory', async function () { await collection.initializeOrderedBulkOp().insert({ name: 'john doe' }).execute(); const result = await collection.findOne({ name: 'john doe' }); - expect(result).to.have.property('_id').to.be.instanceOf(Double); + expect(result).to.have.property('_id').to.have.property('_bsontype').to.equal('Double'); }); it('unordered bulk operations generate _ids using pkFactory', async function () { await collection.initializeUnorderedBulkOp().insert({ name: 'john doe' }).execute(); const result = await collection.findOne({ name: 'john doe' }); - expect(result).to.have.property('_id').to.be.instanceOf(Double); + expect(result).to.have.property('_id').to.have.property('_bsontype').to.equal('Double'); }); it('bulkOperation.raw() with the legacy syntax (no nested document field) generates _ids using pkFactory', async function () { @@ -141,7 +141,7 @@ describe('Bulk', function () { .raw({ insertOne: { name: 'john doe' } }) .execute(); const result = await collection.findOne({ name: 'john doe' }); - expect(result).to.have.property('_id').to.be.instanceOf(Double); + expect(result).to.have.property('_id').to.have.property('_bsontype').to.equal('Double'); }); }); diff --git a/test/integration/crud/insert.test.js b/test/integration/crud/insert.test.js index 117858570c..e0f7971a9d 100644 --- a/test/integration/crud/insert.test.js +++ b/test/integration/crud/insert.test.js @@ -92,7 +92,7 @@ describe('crud - insert', function () { it('insertOne() generates _ids using the pkFactory', async function () { await collection.insertOne({ name: 'john doe' }); const result = await collection.findOne({ name: 'john doe' }); - expect(result).to.have.property('_id').to.be.instanceOf(Double); + expect(result).to.have.property('_id').to.have.property('_bsontype').to.equal('Double'); }); });