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

Open
wants to merge 31 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 26 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
12 changes: 5 additions & 7 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 */
moreToCome?: boolean;
}

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

if (options.noResponse || message.moreToCome) {
if (message.moreToCome) {
yield MongoDBResponse.empty;
return;
}
Expand Down Expand Up @@ -527,11 +529,7 @@ export class Connection extends TypedEventEmitter<ConnectionEvents> {
new CommandSucceededEvent(
this,
message,
options.noResponse
? undefined
: message.moreToCome
? { ok: 1 }
: (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 @@ -647,7 +647,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
2 changes: 1 addition & 1 deletion src/operations/run_command.ts
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ export class RunAdminCommandOperation<T = Document> extends AbstractOperation<T>
constructor(
public command: Document,
public override options: RunCommandOptions & {
noResponse?: boolean;
moreToCome?: boolean;
bypassPinningCheck?: boolean;
}
) {
Expand Down
46 changes: 44 additions & 2 deletions test/integration/collection-management/collection.test.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,13 @@
import { expect } from 'chai';

import { Collection, type Db, type MongoClient, MongoServerError } from '../../mongodb';
import * as sinon from 'sinon';

import {
Collection,
CreateIndexesOperation,
type Db,
type MongoClient,
MongoServerError
} from '../../mongodb';
import { type FailPoint } from '../../tools/utils';
import { setupDatabase } from '../shared';

Expand Down Expand Up @@ -422,6 +429,41 @@ describe('Collection', function () {
});
});

describe('#createIndex', () => {
nbbeeken marked this conversation as resolved.
Show resolved Hide resolved
let client: MongoClient;
let db: Db;
let coll: Collection<{ a: string }>;
const ERROR_RESPONSE = {
ok: 0,
errmsg:
'WiredTigerIndex::insert: key too large to index, failing 1470 { : "56f37cb8e4b089e98d52ab0e", : "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa..." }',
code: 17280
};

beforeEach(async function () {
client = configuration.newClient({ w: 1 });
db = client.db(configuration.db);
coll = db.collection('test_coll');
await client.connect();
sinon.stub(CreateIndexesOperation.prototype, 'execute').callsFake(() => {
throw ERROR_RESPONSE;
});
});

afterEach(async function () {
await coll.drop();
await client.close();
sinon.restore();
});

it('should error when createIndex fails', async function () {
const e = await coll.createIndex({ a: 1 }).catch(e => e);
expect(e).to.have.property('ok', ERROR_RESPONSE.ok);
expect(e).to.have.property('errmsg', ERROR_RESPONSE.errmsg);
expect(e).to.have.property('code', ERROR_RESPONSE.code);
});
});

describe('countDocuments()', function () {
let client: MongoClient;
let collection: Collection<{ test: string }>;
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 } }); // moreToCome = true
});

context('when server selection would return no servers', () => {
Expand Down
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
49 changes: 0 additions & 49 deletions test/unit/collection.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,55 +14,6 @@ describe('Collection', function () {
await cleanup();
});

context('#createIndex', () => {
it('should error when createIndex fails', function (done) {
const ERROR_RESPONSE = {
ok: 0,
errmsg:
'WiredTigerIndex::insert: key too large to index, failing 1470 { : "56f37cb8e4b089e98d52ab0e", : "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa..." }',
code: 17280
};

server.setMessageHandler(request => {
const doc = request.document;

if (isHello(doc)) {
return request.reply(Object.assign({}, HELLO));
}

if (doc.createIndexes) {
return request.reply(ERROR_RESPONSE);
}

if (doc.insert === 'system.indexes') {
return request.reply(ERROR_RESPONSE);
}
});

const client = new MongoClient(`mongodb://${server.uri()}`);

const close = e => client.close().then(() => done(e));

client
.connect()
.then(() => client.db('foo').collection('bar'))
.then(coll => coll.createIndex({ a: 1 }))
.then(
() => close('Expected createIndex to fail, but it succeeded'),
e => {
try {
expect(e).to.have.property('ok', ERROR_RESPONSE.ok);
expect(e).to.have.property('errmsg', ERROR_RESPONSE.errmsg);
expect(e).to.have.property('code', ERROR_RESPONSE.code);
close(null);
} catch (err) {
close(err);
}
}
);
});
});

context('#aggregate', () => {
// general test for aggregate function
function testAggregate(config, done) {
Expand Down