From 1f880eaea6e3bc2aa0bf48a70d020a4f9271286d Mon Sep 17 00:00:00 2001 From: Durran Jordan Date: Thu, 6 Jul 2023 19:37:51 +0200 Subject: [PATCH] feat(NODE-5274): deprecate write concern options (#3752) --- src/gridfs/upload.ts | 21 +-- src/operations/aggregate.ts | 3 +- src/operations/command.ts | 2 +- src/sessions.ts | 5 +- src/write_concern.ts | 81 +++++++++--- test/integration/crud/find_and_modify.test.ts | 28 ++-- .../node-specific/mongo_client.test.ts | 5 +- test/integration/uri-options/uri.test.js | 2 +- test/unit/write_concern.test.ts | 124 ++++++++++++++++++ 9 files changed, 215 insertions(+), 56 deletions(-) create mode 100644 test/unit/write_concern.test.ts diff --git a/src/gridfs/upload.ts b/src/gridfs/upload.ts index a2109af4ed..8ef76e1a57 100644 --- a/src/gridfs/upload.ts +++ b/src/gridfs/upload.ts @@ -263,9 +263,8 @@ async function checkChunksIndex(stream: GridFSBucketWriteStream): Promise }); if (!hasChunksIndex) { - const writeConcernOptions = getWriteOptions(stream); await stream.chunks.createIndex(index, { - ...writeConcernOptions, + ...stream.writeConcern, background: true, unique: true }); @@ -292,7 +291,7 @@ function checkDone(stream: GridFSBucketWriteStream, callback?: Callback): boolea return false; } - stream.files.insertOne(filesDoc, getWriteOptions(stream)).then( + stream.files.insertOne(filesDoc, { writeConcern: stream.writeConcern }).then( () => { stream.emit(GridFSBucketWriteStream.FINISH, filesDoc); stream.emit(GridFSBucketWriteStream.CLOSE); @@ -423,7 +422,7 @@ function doWrite( return false; } - stream.chunks.insertOne(doc, getWriteOptions(stream)).then( + stream.chunks.insertOne(doc, { writeConcern: stream.writeConcern }).then( () => { --stream.state.outstandingRequests; --outstandingRequests; @@ -453,18 +452,6 @@ function doWrite( return false; } -function getWriteOptions(stream: GridFSBucketWriteStream): WriteConcernOptions { - const obj: WriteConcernOptions = {}; - if (stream.writeConcern) { - obj.writeConcern = { - w: stream.writeConcern.w, - wtimeout: stream.writeConcern.wtimeout, - j: stream.writeConcern.j - }; - } - return obj; -} - function waitForIndexes( stream: GridFSBucketWriteStream, callback: (res: boolean) => boolean @@ -499,7 +486,7 @@ function writeRemnant(stream: GridFSBucketWriteStream, callback?: Callback): boo return false; } - stream.chunks.insertOne(doc, getWriteOptions(stream)).then( + stream.chunks.insertOne(doc, { writeConcern: stream.writeConcern }).then( () => { --stream.state.outstandingRequests; checkDone(stream); diff --git a/src/operations/aggregate.ts b/src/operations/aggregate.ts index 301f39cfd3..a742b6ce9f 100644 --- a/src/operations/aggregate.ts +++ b/src/operations/aggregate.ts @@ -3,6 +3,7 @@ import { MongoInvalidArgumentError } from '../error'; import type { Server } from '../sdam/server'; import type { ClientSession } from '../sessions'; import { type Callback, maxWireVersion, type MongoDBNamespace } from '../utils'; +import { WriteConcern } from '../write_concern'; import { type CollationOptions, CommandOperation, type CommandOperationOptions } from './command'; import { Aspect, defineAspects, type Hint } from './operation'; @@ -102,7 +103,7 @@ export class AggregateOperation extends CommandOperation { } if (this.hasWriteStage && this.writeConcern) { - Object.assign(command, { writeConcern: this.writeConcern }); + WriteConcern.apply(command, this.writeConcern); } if (options.bypassDocumentValidation === true) { diff --git a/src/operations/command.ts b/src/operations/command.ts index 0fb804aeb4..7880ad95a2 100644 --- a/src/operations/command.ts +++ b/src/operations/command.ts @@ -135,7 +135,7 @@ export abstract class CommandOperation extends AbstractCallbackOperation { } if (this.writeConcern && this.hasAspect(Aspect.WRITE_OPERATION) && !inTransaction) { - Object.assign(cmd, { writeConcern: this.writeConcern }); + WriteConcern.apply(cmd, this.writeConcern); } if ( diff --git a/src/sessions.ts b/src/sessions.ts index 0f24cea71c..544ab53936 100644 --- a/src/sessions.ts +++ b/src/sessions.ts @@ -45,6 +45,7 @@ import { now, uuidV4 } from './utils'; +import { WriteConcern } from './write_concern'; const minWireVersionForShardedTransactions = 8; @@ -703,11 +704,11 @@ function endTransaction( } if (txnState === TxnState.TRANSACTION_COMMITTED) { - writeConcern = Object.assign({ wtimeout: 10000 }, writeConcern, { w: 'majority' }); + writeConcern = Object.assign({ wtimeoutMS: 10000 }, writeConcern, { w: 'majority' }); } if (writeConcern) { - Object.assign(command, { writeConcern }); + WriteConcern.apply(command, writeConcern); } if (commandName === 'commitTransaction' && session.transaction.options.maxTimeMS) { diff --git a/src/write_concern.ts b/src/write_concern.ts index 0d57cacd8d..e3a3f5510e 100644 --- a/src/write_concern.ts +++ b/src/write_concern.ts @@ -1,3 +1,5 @@ +import { type Document } from './bson'; + /** @public */ export type W = number | 'majority'; @@ -17,16 +19,35 @@ export interface WriteConcernSettings { journal?: boolean; // legacy options - /** The journal write concern */ + /** + * The journal write concern. + * @deprecated Will be removed in the next major version. Please use the journal option. + */ j?: boolean; - /** The write concern timeout */ + /** + * The write concern timeout. + * @deprecated Will be removed in the next major version. Please use the wtimeoutMS option. + */ wtimeout?: number; - /** The file sync write concern */ + /** + * The file sync write concern. + * @deprecated Will be removed in the next major version. Please use the journal option. + */ fsync?: boolean | 1; } export const WRITE_CONCERN_KEYS = ['w', 'wtimeout', 'j', 'journal', 'fsync']; +/** The write concern options that decorate the server command. */ +interface CommandWriteConcernOptions { + /** The write concern */ + w?: W; + /** The journal write concern. */ + j?: boolean; + /** The write concern timeout. */ + wtimeout?: number; +} + /** * A MongoDB WriteConcern, which describes the level of acknowledgement * requested from MongoDB for write operations. @@ -35,23 +56,36 @@ export const WRITE_CONCERN_KEYS = ['w', 'wtimeout', 'j', 'journal', 'fsync']; * @see https://www.mongodb.com/docs/manual/reference/write-concern/ */ export class WriteConcern { - /** request acknowledgment that the write operation has propagated to a specified number of mongod instances or to mongod instances with specified tags. */ - w?: W; - /** specify a time limit to prevent write operations from blocking indefinitely */ + /** Request acknowledgment that the write operation has propagated to a specified number of mongod instances or to mongod instances with specified tags. */ + readonly w?: W; + /** Request acknowledgment that the write operation has been written to the on-disk journal */ + readonly journal?: boolean; + /** Specify a time limit to prevent write operations from blocking indefinitely */ + readonly wtimeoutMS?: number; + /** + * Specify a time limit to prevent write operations from blocking indefinitely. + * @deprecated Will be removed in the next major version. Please use wtimeoutMS. + */ wtimeout?: number; - /** request acknowledgment that the write operation has been written to the on-disk journal */ + /** + * Request acknowledgment that the write operation has been written to the on-disk journal. + * @deprecated Will be removed in the next major version. Please use journal. + */ j?: boolean; - /** equivalent to the j option */ + /** + * Equivalent to the j option. + * @deprecated Will be removed in the next major version. Please use journal. + */ fsync?: boolean | 1; /** * Constructs a WriteConcern from the write concern properties. * @param w - request acknowledgment that the write operation has propagated to a specified number of mongod instances or to mongod instances with specified tags. - * @param wtimeout - specify a time limit to prevent write operations from blocking indefinitely - * @param j - request acknowledgment that the write operation has been written to the on-disk journal - * @param fsync - equivalent to the j option + * @param wtimeoutMS - specify a time limit to prevent write operations from blocking indefinitely + * @param journal - request acknowledgment that the write operation has been written to the on-disk journal + * @param fsync - equivalent to the j option. Is deprecated and will be removed in the next major version. */ - constructor(w?: W, wtimeout?: number, j?: boolean, fsync?: boolean | 1) { + constructor(w?: W, wtimeoutMS?: number, journal?: boolean, fsync?: boolean | 1) { if (w != null) { if (!Number.isNaN(Number(w))) { this.w = Number(w); @@ -59,17 +93,30 @@ export class WriteConcern { this.w = w; } } - if (wtimeout != null) { - this.wtimeout = wtimeout; + if (wtimeoutMS != null) { + this.wtimeoutMS = this.wtimeout = wtimeoutMS; } - if (j != null) { - this.j = j; + if (journal != null) { + this.journal = this.j = journal; } if (fsync != null) { - this.fsync = fsync; + this.journal = this.j = fsync ? true : false; } } + /** + * Apply a write concern to a command document. Will modify and return the command. + */ + static apply(command: Document, writeConcern: WriteConcern): Document { + const wc: CommandWriteConcernOptions = {}; + // The write concern document sent to the server has w/wtimeout/j fields. + if (writeConcern.w != null) wc.w = writeConcern.w; + if (writeConcern.wtimeoutMS != null) wc.wtimeout = writeConcern.wtimeoutMS; + if (writeConcern.journal != null) wc.j = writeConcern.j; + command.writeConcern = wc; + return command; + } + /** Construct a WriteConcern given an options object. */ static fromOptions( options?: WriteConcernOptions | WriteConcern | W, diff --git a/test/integration/crud/find_and_modify.test.ts b/test/integration/crud/find_and_modify.test.ts index 2ecb02c178..8131242503 100644 --- a/test/integration/crud/find_and_modify.test.ts +++ b/test/integration/crud/find_and_modify.test.ts @@ -119,8 +119,8 @@ describe('Collection (#findOneAnd...)', function () { }); it('passes through the writeConcern', async function () { - await collection.findOneAndDelete({}, { writeConcern: { fsync: 1 } }); - expect(started[0].command.writeConcern).to.deep.equal({ fsync: 1 }); + await collection.findOneAndDelete({}, { writeConcern: { j: 1 } }); + expect(started[0].command.writeConcern).to.deep.equal({ j: 1 }); }); }); @@ -128,27 +128,27 @@ describe('Collection (#findOneAnd...)', function () { beforeEach(async function () { collection = client .db('test') - .collection('findAndModifyTest', { writeConcern: { fsync: 1 } }); + .collection('findAndModifyTest', { writeConcern: { j: 1 } }); await collection.insertMany([{ a: 1, b: 1 }], { writeConcern: { w: 1 } }); }); it('passes through the writeConcern', async function () { await collection.findOneAndDelete({}); - expect(started[0].command.writeConcern).to.deep.equal({ fsync: 1 }); + expect(started[0].command.writeConcern).to.deep.equal({ j: 1 }); }); }); context('when provided at the db level', function () { beforeEach(async function () { collection = client - .db('test', { writeConcern: { fsync: 1 } }) + .db('test', { writeConcern: { j: 1 } }) .collection('findAndModifyTest'); await collection.insertMany([{ a: 1, b: 1 }], { writeConcern: { w: 1 } }); }); it('passes through the writeConcern', async function () { await collection.findOneAndDelete({}); - expect(started[0].command.writeConcern).to.deep.equal({ fsync: 1 }); + expect(started[0].command.writeConcern).to.deep.equal({ j: 1 }); }); }); }); @@ -297,8 +297,8 @@ describe('Collection (#findOneAnd...)', function () { }); it('passes through the writeConcern', async function () { - await collection.findOneAndUpdate({}, { $set: { a: 1 } }, { writeConcern: { fsync: 1 } }); - expect(started[0].command.writeConcern).to.deep.equal({ fsync: 1 }); + await collection.findOneAndUpdate({}, { $set: { a: 1 } }, { writeConcern: { j: 1 } }); + expect(started[0].command.writeConcern).to.deep.equal({ j: 1 }); }); }); @@ -306,27 +306,27 @@ describe('Collection (#findOneAnd...)', function () { beforeEach(async function () { collection = client .db('test') - .collection('findAndModifyTest', { writeConcern: { fsync: 1 } }); + .collection('findAndModifyTest', { writeConcern: { j: 1 } }); await collection.insertMany([{ a: 1, b: 1 }], { writeConcern: { w: 1 } }); }); it('passes through the writeConcern', async function () { await collection.findOneAndUpdate({}, { $set: { a: 1 } }); - expect(started[0].command.writeConcern).to.deep.equal({ fsync: 1 }); + expect(started[0].command.writeConcern).to.deep.equal({ j: 1 }); }); }); context('when provided at the db level', function () { beforeEach(async function () { collection = client - .db('test', { writeConcern: { fsync: 1 } }) + .db('test', { writeConcern: { j: 1 } }) .collection('findAndModifyTest'); await collection.insertMany([{ a: 1, b: 1 }], { writeConcern: { w: 1 } }); }); it('passes through the writeConcern', async function () { await collection.findOneAndUpdate({}, { $set: { a: 1 } }); - expect(started[0].command.writeConcern).to.deep.equal({ fsync: 1 }); + expect(started[0].command.writeConcern).to.deep.equal({ j: 1 }); }); }); }); @@ -468,8 +468,8 @@ describe('Collection (#findOneAnd...)', function () { }); it('passes through the writeConcern', async function () { - await collection.findOneAndReplace({}, { b: 1 }, { writeConcern: { fsync: 1 } }); - expect(started[0].command.writeConcern).to.deep.equal({ fsync: 1 }); + await collection.findOneAndReplace({}, { b: 1 }, { writeConcern: { j: 1 } }); + expect(started[0].command.writeConcern).to.deep.equal({ j: 1 }); }); }); diff --git a/test/integration/node-specific/mongo_client.test.ts b/test/integration/node-specific/mongo_client.test.ts index 03278dc3ce..b0bdfd7161 100644 --- a/test/integration/node-specific/mongo_client.test.ts +++ b/test/integration/node-specific/mongo_client.test.ts @@ -52,9 +52,8 @@ describe('class MongoClient', function () { expect(db).to.have.property('writeConcern'); expect(db.writeConcern).to.have.property('w', 1); - expect(db.writeConcern).to.have.property('wtimeout', 1000); - expect(db.writeConcern).to.have.property('fsync', true); - expect(db.writeConcern).to.have.property('j', true); + expect(db.writeConcern).to.have.property('wtimeoutMS', 1000); + expect(db.writeConcern).to.have.property('journal', true); expect(db).to.have.property('s'); expect(db.s).to.have.property('readPreference'); diff --git a/test/integration/uri-options/uri.test.js b/test/integration/uri-options/uri.test.js index 293c65ee1f..ef62c3408b 100644 --- a/test/integration/uri-options/uri.test.js +++ b/test/integration/uri-options/uri.test.js @@ -66,7 +66,7 @@ describe('URI', function () { const client = this.configuration.newClient('mongodb://127.0.0.1:27017/?fsync=true'); client.connect((err, client) => { var db = client.db(this.configuration.db); - expect(db.writeConcern.fsync).to.be.true; + expect(db.writeConcern.journal).to.be.true; client.close(done); }); } diff --git a/test/unit/write_concern.test.ts b/test/unit/write_concern.test.ts new file mode 100644 index 0000000000..0f59a2c3b0 --- /dev/null +++ b/test/unit/write_concern.test.ts @@ -0,0 +1,124 @@ +import { expect } from 'chai'; + +import { WriteConcern } from '../mongodb'; + +describe('WriteConcern', function () { + describe('#constructor', function () { + context('when w is provided', function () { + context('when w is a number', function () { + const writeConcern = new WriteConcern(1); + + it('sets the w property', function () { + expect(writeConcern.w).to.equal(1); + }); + }); + + context('when w is a string number', function () { + const writeConcern = new WriteConcern('10'); + + it('sets the w property to a number', function () { + expect(writeConcern.w).to.equal(10); + }); + }); + + context('when w is a string', function () { + const writeConcern = new WriteConcern('majority'); + + it('sets the w property to the string', function () { + expect(writeConcern.w).to.equal('majority'); + }); + }); + }); + + context('when wtimeoutMS is provided', function () { + const writeConcern = new WriteConcern(1, 50); + + it('sets the wtimeoutMS property', function () { + expect(writeConcern.wtimeoutMS).to.equal(50); + }); + + it('sets the wtimeout property', function () { + expect(writeConcern.wtimeout).to.equal(50); + }); + }); + + context('when journal is provided', function () { + const writeConcern = new WriteConcern(1, 50, true); + + it('sets the journal property', function () { + expect(writeConcern.journal).to.be.true; + }); + + it('sets the j property', function () { + expect(writeConcern.j).to.be.true; + }); + }); + + context('when fsync is provided', function () { + const writeConcern = new WriteConcern(1, 50, false, true); + + it('sets the journal property', function () { + expect(writeConcern.journal).to.be.true; + }); + + it('sets the j property', function () { + expect(writeConcern.j).to.be.true; + }); + }); + }); + + describe('.apply', function () { + context('when no options are set', function () { + const document = {}; + const writeConcern = new WriteConcern(); + + it('returns an empty write concern', function () { + expect(WriteConcern.apply(document, writeConcern)).to.deep.equal({ writeConcern: {} }); + }); + }); + + context('when w is in the write concern', function () { + const document = {}; + const writeConcern = new WriteConcern(2); + + it('adds w to the write concern document', function () { + expect(WriteConcern.apply(document, writeConcern)).to.deep.equal({ + writeConcern: { w: 2 } + }); + }); + }); + + context('when wtimeoutMS is in the write concern', function () { + const document = {}; + const writeConcern = new WriteConcern(2, 30); + + it('adds wtimeout to the write concern document', function () { + expect(WriteConcern.apply(document, writeConcern)).to.deep.equal({ + writeConcern: { w: 2, wtimeout: 30 } + }); + }); + }); + + context('when journal is in the write concern', function () { + const document = {}; + const writeConcern = new WriteConcern(2, 30, true); + + it('adds j to the write concern document', function () { + expect(WriteConcern.apply(document, writeConcern)).to.deep.equal({ + writeConcern: { w: 2, wtimeout: 30, j: true } + }); + }); + }); + + context('when fsync is in the write concern', function () { + const document = {}; + const writeConcern = new WriteConcern(2, 30, true, false); + + it('overwrites j to the write concern document', function () { + expect(WriteConcern.apply(document, writeConcern)).to.deep.equal({ + writeConcern: { w: 2, wtimeout: 30, j: false } + }); + }); + }); + }); +});