Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(NODE-6377): remove noResponse option #4240

Closed
wants to merge 31 commits into from
Closed
Show file tree
Hide file tree
Changes from 21 commits
Commits
Show all changes
31 commits
Select commit Hold shift + click to select a range
791029f
initial commit
aditi-khare-mongoDB Sep 3, 2024
d1a1931
enforce no response when moreToCome is set
aditi-khare-mongoDB Sep 3, 2024
687e66c
Merge branch 'main' into NODE-6060/fire-and-forget
aditi-khare-mongoDB Sep 3, 2024
ebd4b44
ready for review
aditi-khare-mongoDB Sep 6, 2024
ec5a53a
Merge branch 'main' into NODE-6060/fire-and-forget
aditi-khare-mongoDB Sep 6, 2024
86e1a91
tsdoc updateg
aditi-khare-mongoDB Sep 6, 2024
d892ccc
lint fix
aditi-khare-mongoDB Sep 6, 2024
1cdc9df
Merge branch 'main' into NODE-6060/fire-and-forget
aditi-khare-mongoDB Sep 6, 2024
5df9384
fix failing test
aditi-khare-mongoDB Sep 9, 2024
1e52263
fix failing tests
aditi-khare-mongoDB Sep 9, 2024
41641a5
Merge branch 'main' into NODE-6060/fire-and-forget
aditi-khare-mongoDB Sep 9, 2024
7ca5e03
requested changes 1
aditi-khare-mongoDB Sep 12, 2024
8243f99
remove moreToCome as an option on the request - only on the reply
aditi-khare-mongoDB Sep 12, 2024
7c607fd
add check to remove wordiness
aditi-khare-mongoDB Sep 12, 2024
fbcfcfb
Merge branch 'main' into NODE-6060/fire-and-forget
aditi-khare-mongoDB Sep 12, 2024
8b59ecd
remove extraneous change
aditi-khare-mongoDB Sep 13, 2024
6422402
deprecate noResponse
aditi-khare-mongoDB Sep 13, 2024
ee93834
add moreToCome back on the request
aditi-khare-mongoDB Sep 13, 2024
56772b7
update deprecation comment
aditi-khare-mongoDB Sep 13, 2024
6104c57
remove noResponse deprecation - do in node-6377
aditi-khare-mongoDB Sep 16, 2024
419a848
fix(NODE-6377): Remove noResponse option
aditi-khare-mongoDB Sep 16, 2024
4a94e4e
lint fix
aditi-khare-mongoDB Sep 16, 2024
bcf1244
Merge branch 'main' into NODE-6377/remove-noResponse
aditi-khare-mongoDB Sep 18, 2024
3787250
move unit to integration test to get correct maxWireVersion for opMsg
aditi-khare-mongoDB Sep 19, 2024
4eb7c12
requested changes team review
aditi-khare-mongoDB Sep 24, 2024
90a3e78
Merge branch 'main' into NODE-6377/remove-noResponse
aditi-khare-mongoDB Sep 25, 2024
b133f86
fix failing tests
aditi-khare-mongoDB Sep 25, 2024
29d8330
fix test
aditi-khare-mongoDB Sep 26, 2024
b4b0866
fix failing noauth tests
aditi-khare-mongoDB Sep 27, 2024
d1f620d
Merge branch 'main' into NODE-6377/remove-noResponse
aditi-khare-mongoDB Sep 27, 2024
3d03253
Merge branch 'main' into NODE-6377/remove-noResponse
baileympearson Nov 4, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 18 additions & 8 deletions src/cmap/commands.ts
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,8 @@ export class OpQueryRequest {
awaitData: boolean;
exhaust: boolean;
partial: boolean;
/** moreToCome is an OP_MSG only concept */
moreToCome = false;

constructor(
public databaseName: string,
Expand Down Expand Up @@ -407,13 +409,21 @@ const OPTS_EXHAUST_ALLOWED = 1 << 16;

/** @internal */
export interface OpMsgOptions {
requestId: number;
serializeFunctions: boolean;
ignoreUndefined: boolean;
checkKeys: boolean;
maxBsonSize: number;
moreToCome: boolean;
exhaustAllowed: boolean;
socketTimeoutMS?: number;
session?: ClientSession;
numberToSkip?: number;
numberToReturn?: number;
returnFieldSelector?: Document;
pre32Limit?: number;
serializeFunctions?: boolean;
ignoreUndefined?: boolean;
maxBsonSize?: number;
checkKeys?: boolean;
secondaryOk?: boolean;

requestId?: number;
moreToCome?: boolean;
exhaustAllowed?: boolean;
readPreference: ReadPreference;
}

Expand Down Expand Up @@ -465,7 +475,7 @@ export class OpMsgRequest {

// flags
this.checksumPresent = false;
this.moreToCome = options.moreToCome || false;
this.moreToCome = options.moreToCome ?? command.writeConcern?.w === 0;
this.exhaustAllowed =
typeof options.exhaustAllowed === 'boolean' ? options.exhaustAllowed : false;
}
Expand Down
8 changes: 5 additions & 3 deletions src/cmap/connection.ts
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,6 @@ export interface CommandOptions extends BSONSerializeOptions {
/** Session to use for the operation */
session?: ClientSession;
documentsReturnedIn?: string;
noResponse?: boolean;
omitReadPreference?: boolean;

// TODO(NODE-2802): Currently the CommandOptions take a property willRetryWrite which is a hint
Expand All @@ -94,6 +93,9 @@ export interface CommandOptions extends BSONSerializeOptions {
writeConcern?: WriteConcern;

directConnection?: boolean;

// Triggers fire-and-forget protocol for commands that don't support WriteConcern
nbbeeken marked this conversation as resolved.
Show resolved Hide resolved
moreToCome?: boolean;
}

/** @public */
Expand Down Expand Up @@ -439,7 +441,7 @@ export class Connection extends TypedEventEmitter<ConnectionEvents> {
zlibCompressionLevel: this.description.zlibCompressionLevel
});

if (options.noResponse) {
if (message.moreToCome) {
yield MongoDBResponse.empty;
return;
}
Expand Down Expand Up @@ -527,7 +529,7 @@ export class Connection extends TypedEventEmitter<ConnectionEvents> {
new CommandSucceededEvent(
this,
message,
options.noResponse ? undefined : (object ??= document.toObject(bsonOptions)),
message.moreToCome ? { ok: 1 } : (object ??= document.toObject(bsonOptions)),
started,
this.description.serverConnectionId
)
Expand Down
2 changes: 1 addition & 1 deletion src/mongo_client.ts
Original file line number Diff line number Diff line change
Expand Up @@ -628,7 +628,7 @@ export class MongoClient extends TypedEventEmitter<MongoClientEvents> implements
this,
new RunAdminCommandOperation(
{ endSessions },
{ readPreference: ReadPreference.primaryPreferred, noResponse: true }
{ readPreference: ReadPreference.primaryPreferred, moreToCome: true }
durran marked this conversation as resolved.
Show resolved Hide resolved
)
);
} catch (error) {
Expand Down
1 change: 0 additions & 1 deletion src/operations/command.ts
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,6 @@ export interface CommandOperationOptions
// Admin command overrides.
dbName?: string;
authdb?: string;
noResponse?: boolean;
}

/** @internal */
Expand Down
7 changes: 4 additions & 3 deletions src/operations/run_command.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,9 @@ import { type TODO_NODE_3286 } from '../mongo_types';
import type { ReadPreferenceLike } from '../read_preference';
import type { Server } from '../sdam/server';
import type { ClientSession } from '../sessions';
import type { WriteConcern } from '../write_concern';
import { MongoDBNamespace } from '../utils';
import { AbstractOperation } from './operation';
import { Aspect, defineAspects, AbstractOperation } from './operation';

/** @public */
export type RunCommandOptions = {
Expand Down Expand Up @@ -51,7 +52,7 @@ export class RunAdminCommandOperation<T = Document> extends AbstractOperation<T>
constructor(
public command: Document,
public override options: RunCommandOptions & {
noResponse?: boolean;
moreToCome?: boolean;
bypassPinningCheck?: boolean;
}
) {
Expand All @@ -72,4 +73,4 @@ export class RunAdminCommandOperation<T = Document> extends AbstractOperation<T>
});
return res;
}
}
}
5 changes: 4 additions & 1 deletion src/write_concern.ts
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,10 @@ interface CommandWriteConcernOptions {
* @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. */
/**
* Request acknowledgment that the write operation has propagated to a specified number of mongod instances or to mongod instances with specified tags.
* If w is 0 and is set on a write operation, the server will not send a response.
*/
readonly w?: W;
/** Request acknowledgment that the write operation has been written to the on-disk journal */
readonly journal?: boolean;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -144,6 +144,21 @@ describe('Connection', function () {
}
}
});

nbbeeken marked this conversation as resolved.
Show resolved Hide resolved
it('supports fire-and-forget messages', async function () {
const options: ConnectionOptions = {
...commonConnectOptions,
connectionType: Connection,
...this.configuration.options,
metadata: makeClientMetadata({ driverInfo: {} }),
extendedMetadata: addContainerMetadata(makeClientMetadata({ driverInfo: {} }))
};

const conn = await connect(options);
const readSpy = sinon.spy(conn, 'readMany');
await conn.command(ns('$admin.cmd'), { ping: 1 }, { moreToCome: true });
expect(readSpy).to.not.have.been.called;
});
});

describe('Connection - functional', function () {
Expand Down
4 changes: 2 additions & 2 deletions test/integration/node-specific/mongo_client.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -682,7 +682,7 @@ describe('class MongoClient', function () {
expect(result2).to.have.property('ok', 1);
});

it('sends endSessions with noResponse set', async () => {
it('sends endSessions with writeConcern w = 0 set', async () => {
const session = client.startSession(); // make a session to be ended
await client.db('test').command({ ping: 1 }, { session });
await session.endSession();
Expand All @@ -698,7 +698,7 @@ describe('class MongoClient', function () {
expect(startedEvents).to.have.lengthOf(1);
expect(startedEvents[0]).to.have.property('commandName', 'endSessions');
expect(endEvents).to.have.lengthOf(1);
expect(endEvents[0]).to.have.property('reply', undefined); // noReponse: true
expect(endEvents[0]).to.containSubset({ reply: { ok: 1 } }); // writeConcern.w = 0
nbbeeken marked this conversation as resolved.
Show resolved Hide resolved
});

context('when server selection would return no servers', () => {
Expand Down
140 changes: 138 additions & 2 deletions test/integration/read-write-concern/write_concern.test.ts
Original file line number Diff line number Diff line change
@@ -1,12 +1,16 @@
import { expect } from 'chai';
import { on, once } from 'events';
import { gte } from 'semver';
import * as sinon from 'sinon';

import {
type Collection,
type CommandStartedEvent,
type CommandSucceededEvent,
type Db,
LEGACY_HELLO_COMMAND,
MongoClient
MongoClient,
OpMsgRequest
} from '../../mongodb';
import * as mock from '../../tools/mongodb-mock/index';
import { filterForCommands } from '../shared';
Expand Down Expand Up @@ -93,7 +97,7 @@ describe('Write Concern', function () {
});

afterEach(async function () {
await db.dropDatabase();
await db.dropDatabase({ writeConcern: { w: 'majority' } });
await client.close();
});

Expand Down Expand Up @@ -168,4 +172,136 @@ describe('Write Concern', function () {
});
});
});

describe('fire-and-forget protocol', function () {
context('when writeConcern = 0 and OP_MSG is used', function () {
const writeOperations: { name: string; command: any; expectedReturnVal: any }[] = [
{
name: 'insertOne',
command: client => client.db('test').collection('test').insertOne({ a: 1 }),
expectedReturnVal: { acknowledged: false }
},
{
name: 'insertMany',
command: client =>
client
.db('test')
.collection('test')
.insertMany([{ a: 1 }, { b: 2 }]),
expectedReturnVal: { acknowledged: false }
},
{
name: 'updateOne',
command: client =>
client
.db('test')
.collection('test')
.updateOne({ i: 128 }, { $set: { c: 2 } }),
expectedReturnVal: { acknowledged: false }
},
{
name: 'updateMany',
command: client =>
client
.db('test')
.collection('test')
.updateMany({ name: 'foobar' }, { $set: { name: 'fizzbuzz' } }),
expectedReturnVal: { acknowledged: false }
},
{
name: 'deleteOne',
command: client => client.db('test').collection('test').deleteOne({ a: 1 }),
expectedReturnVal: { acknowledged: false }
},
{
name: 'deleteMany',
command: client => client.db('test').collection('test').deleteMany({ name: 'foobar' }),
expectedReturnVal: { acknowledged: false }
},
{
name: 'replaceOne',
command: client => client.db('test').collection('test').replaceOne({ a: 1 }, { b: 2 }),
expectedReturnVal: { acknowledged: false }
},
{
name: 'removeUser',
command: client => client.db('test').removeUser('albert'),
expectedReturnVal: true
},
{
name: 'findAndModify',
command: client =>
client
.db('test')
.collection('test')
.findOneAndUpdate({}, { $setOnInsert: { a: 1 } }, { upsert: true }),
expectedReturnVal: null
},
{
name: 'dropDatabase',
command: client => client.db('test').dropDatabase(),
expectedReturnVal: true
},
{
name: 'dropCollection',
command: client => client.db('test').dropCollection('test'),
expectedReturnVal: true
},
{
name: 'dropIndexes',
command: client => client.db('test').collection('test').dropIndex('a'),
expectedReturnVal: { ok: 1 }
},
{
name: 'createIndexes',
command: client => client.db('test').collection('test').createIndex({ a: 1 }),
expectedReturnVal: 'a_1'
},
{
name: 'createCollection',
command: client => client.db('test').createCollection('test'),
expectedReturnVal: {}
}
];

for (const op of writeOperations) {
context(`when the write operation ${op.name} is run`, function () {
let client;
let spy;

beforeEach(async function () {
if (gte('3.6.0', this.configuration.version)) {
this.currentTest.skipReason = 'Test requires OP_MSG, needs to be on MongoDB 3.6+';
this.skip();
}
spy = sinon.spy(OpMsgRequest.prototype, 'toBin');
client = this.configuration.newClient({ monitorCommands: true, w: 0 });
await client.connect();
});

afterEach(function () {
sinon.restore();
client.close();
});

it('the request should have moreToCome bit set', async function () {
await op.command(client);
expect(spy.returnValues[spy.returnValues.length - 1][0][16]).to.equal(2);
});

it('the return value of the command should be nullish', async function () {
const result = await op.command(client);
expect(result).to.containSubset(op.expectedReturnVal);
});

it('commandSucceededEvent should have reply with only {ok: 1}', async function () {
const events: CommandSucceededEvent[] = [];
client.on('commandSucceeded', event => events.push(event));
await op.command(client);
expect(events[0]).to.containSubset({ reply: { ok: 1 } });
});
});
}
});
});
});
23 changes: 0 additions & 23 deletions test/unit/cmap/connection.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -28,29 +28,6 @@ describe('new Connection()', function () {

before(() => mock.createServer().then(s => (server = s)));

it('supports fire-and-forget messages', async function () {
server.setMessageHandler(request => {
const doc = request.document;
if (isHello(doc)) {
request.reply(mock.HELLO);
}

// black hole all other requests
});

const options = {
...connectionOptionsDefaults,
connectionType: Connection,
hostAddress: server.hostAddress(),
authProviders: new MongoClientAuthProviders()
};

const conn = await connect(options);
const readSpy = sinon.spy(conn, 'readMany');
await conn.command(ns('$admin.cmd'), { ping: 1 }, { noResponse: true });
expect(readSpy).to.not.have.been.called;
});

it('destroys streams which time out', async function () {
server.setMessageHandler(request => {
const doc = request.document;
Expand Down
11 changes: 11 additions & 0 deletions test/unit/commands.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -109,3 +109,14 @@ describe('class OpCompressedRequest', () => {
}
});
});

describe('OpMsgRequest', () => {
describe('#constructor', () => {
context('when writeConcern = 0', () => {
it('moreToCome is set to true', async () => {
const request = new OpMsgRequest('db', { a: 1, writeConcern: { w: 0 } }, {});
expect(request.moreToCome).to.be.true;
});
});
});
});