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-4999): Write Concern 0 Must Not Affect Read Operations #3541

Merged
merged 22 commits into from
Feb 2, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
9a17960
test(NODE-4999): Convert issue repro to failing test
W-A-James Jan 25, 2023
dc46642
test(NODE-4999): Got repro working
W-A-James Jan 25, 2023
704e3fc
test(NODE-4999): Add tests to check for desired behaviour
W-A-James Jan 26, 2023
fa76f2d
fix(NODE-4999): WIP fix
W-A-James Jan 26, 2023
4d7ff69
fix(NODE-4999): Start implementing fix
W-A-James Jan 27, 2023
1c7c2c2
Merge branch '4.x' into NODE-4999/write_concern_0_must_not_affect_rea…
W-A-James Jan 27, 2023
52f4cc5
fix(NODE-4999): revert unneeded type change and run eslint
W-A-James Jan 27, 2023
a74ebce
Merge branch 'NODE-4999/write_concern_0_must_not_affect_read_operatio…
W-A-James Jan 27, 2023
877cd73
test(NODE-4999): Fix import and extend timeout
W-A-James Jan 27, 2023
7f73b5b
fix(NODE-4999): simplify changes
W-A-James Jan 27, 2023
2c0a14d
test(NODE-4999): Ensure test only runs on replica sets and sharded cl…
W-A-James Jan 27, 2023
82557e3
test(NODE-4999): conditionally execute test
W-A-James Jan 27, 2023
0f06672
fix(NODE-4999): explicitly delete writeConcern key
W-A-James Jan 30, 2023
2f0a3a6
test(NODE-4999): Convert test to typescript
W-A-James Jan 30, 2023
164731c
fix(NODE-4999): ensure that copy is made of options object
W-A-James Jan 30, 2023
9ab6549
test(NODE-4999): extend timeout for changeStream test and change find…
W-A-James Jan 30, 2023
2a26d17
test(NODE-4999): Change test names and ensure that checks in find.tes…
W-A-James Jan 30, 2023
17ebd06
WIP
W-A-James Jan 31, 2023
d77a47b
test(NODE-4999): Fix for failing ci test
W-A-James Feb 1, 2023
0c44d2a
chore(NODE-4999): change mongo-client-encryption version
W-A-James Feb 1, 2023
c5050a8
test(NODE-4999): Fix changeStream test
W-A-James Feb 2, 2023
b1c0c2a
Update test/integration/read-write-concern/write_concern.test.ts
W-A-James Feb 2, 2023
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
2 changes: 1 addition & 1 deletion .evergreen/run-tests.sh
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ else
source "$DRIVERS_TOOLS"/.evergreen/csfle/set-temp-creds.sh
fi

npm install mongodb-client-encryption@"2.4.0-alpha.2"
npm install mongodb-client-encryption@"2.4.0"
npm install @mongodb-js/zstd
npm install snappy

Expand Down
3 changes: 2 additions & 1 deletion src/change_stream.ts
Original file line number Diff line number Diff line change
Expand Up @@ -594,7 +594,8 @@ export class ChangeStream<
super();

this.pipeline = pipeline;
this.options = options;
this.options = { ...options };
delete this.options.writeConcern;

if (parent instanceof Collection) {
this.type = CHANGE_DOMAIN_TYPES.COLLECTION;
Expand Down
4 changes: 3 additions & 1 deletion src/operations/aggregate.ts
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ export class AggregateOperation<T = Document> extends CommandOperation<T> {
constructor(ns: MongoDBNamespace, pipeline: Document[], options?: AggregateOptions) {
super(undefined, { ...options, dbName: ns.db });

this.options = options ?? {};
this.options = { ...options };

// Covers when ns.collection is null, undefined or the empty string, use DB_AGGREGATE_COLLECTION
this.target = ns.collection || DB_AGGREGATE_COLLECTION;
Expand All @@ -65,6 +65,8 @@ export class AggregateOperation<T = Document> extends CommandOperation<T> {

if (this.hasWriteStage) {
this.trySecondaryWrite = true;
} else {
delete this.options.writeConcern;
}

if (this.explain && this.writeConcern) {
Expand Down
3 changes: 2 additions & 1 deletion src/operations/find.ts
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,8 @@ export class FindOperation extends CommandOperation<Document> {
) {
super(collection, options);

this.options = options;
this.options = { ...options };
delete this.options.writeConcern;
this.ns = ns;

if (typeof filter !== 'object' || Array.isArray(filter)) {
Expand Down
3 changes: 2 additions & 1 deletion src/operations/indexes.ts
Original file line number Diff line number Diff line change
Expand Up @@ -394,7 +394,8 @@ export class ListIndexesOperation extends CommandOperation<Document> {
constructor(collection: Collection, options?: ListIndexesOptions) {
super(collection, options);

this.options = options ?? {};
this.options = { ...options };
delete this.options.writeConcern;
this.collectionNamespace = collection.s.namespace;
}

Expand Down
3 changes: 2 additions & 1 deletion src/operations/list_collections.ts
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,8 @@ export class ListCollectionsOperation extends CommandOperation<string[]> {
constructor(db: Db, filter: Document, options?: ListCollectionsOptions) {
super(db, options);

this.options = options ?? {};
this.options = { ...options };
delete this.options.writeConcern;
this.db = db;
this.filter = filter;
this.nameOnly = !!this.options.nameOnly;
Expand Down
75 changes: 0 additions & 75 deletions test/integration/read-write-concern/write_concern.test.js

This file was deleted.

177 changes: 177 additions & 0 deletions test/integration/read-write-concern/write_concern.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,177 @@
import { expect } from 'chai';
import { on, once } from 'events';

import { Collection } from '../../../src/collection';
import { LEGACY_HELLO_COMMAND } from '../../../src/constants';
import { Db } from '../../../src/db';
import { MongoClient } from '../../../src/mongo_client';
import * as mock from '../../tools/mongodb-mock/index';

describe('Write Concern', function () {
it('should respect writeConcern from uri', function (done) {
const client = this.configuration.newClient(
`${this.configuration.url()}&w=0&monitorCommands=true`
);
const events: any[] = [];
client.on('commandStarted', event => {
if (event.commandName === 'insert') {
events.push(event);
}
});

expect(client.writeConcern).to.eql({ w: 0 });
client
.db('test')
.collection('test')
.insertOne({ a: 1 }, (err, result) => {
expect(err).to.not.exist;
expect(result).to.exist;
expect(events).to.be.an('array').with.lengthOf(1);
expect(events[0]).to.containSubset({
commandName: 'insert',
command: {
writeConcern: { w: 0 }
}
});
client.close(done);
});
});

describe('mock server write concern test', () => {
let server;
before(() => {
return mock.createServer().then(s => {
server = s;
});
});

after(() => mock.cleanup());

// TODO(NODE-3816): the mock server response is looking for writeConcern on all messages, but endSessions doesn't have it
it.skip('should pipe writeConcern from client down to API call', function () {
server.setMessageHandler(request => {
if (request.document && request.document[LEGACY_HELLO_COMMAND]) {
return request.reply(mock.HELLO);
}
expect(request.document.writeConcern).to.exist;
expect(request.document.writeConcern.w).to.equal('majority');
return request.reply({ ok: 1 });
});

const uri = `mongodb://${server.uri()}`;
const client = new MongoClient(uri, { writeConcern: { w: 'majority' } });
return client
.connect()
.then(() => {
const db = client.db('wc_test');
const collection = db.collection('wc');

return collection.insertMany([{ a: 2 }]);
})
.then(() => {
return client.close();
});
});
});

context('when performing read operations', function () {
context('when writeConcern = 0', function () {
describe('cursor creating operations with a getMore', function () {
let client: MongoClient;
let db: Db;
let col: Collection;

beforeEach(async function () {
client = this.configuration.newClient({ writeConcern: { w: 0 } });
await client.connect();
db = client.db('writeConcernTest');
col = db.collection('writeConcernTest');

const docs: any[] = [];
for (let i = 0; i < 100; i++) {
docs.push({ a: i, b: i + 1 });
}

await col.insertMany(docs);
});

afterEach(async function () {
await db.dropDatabase();
await client.close();
});

it('succeeds on find', async function () {
const findResult = col.find({}, { batchSize: 2 });
const err = await findResult.toArray().catch(e => e);

expect(err).to.not.be.instanceOf(Error);
});

it('succeeds on listCollections', async function () {
const collections: any[] = [];
for (let i = 0; i < 10; i++) {
collections.push(`writeConcernTestCol${i + 1}`);
}

for (const colName of collections) {
await db.createCollection(colName).catch(() => null);
}

const cols = db.listCollections({}, { batchSize: 2 });

const err = await cols.toArray().catch(e => e);

expect(err).to.not.be.instanceOf(Error);
});

it('succeeds on aggregate', async function () {
const aggResult = col.aggregate([{ $match: { a: { $gte: 0 } } }], { batchSize: 2 });
const err = await aggResult.toArray().catch(e => e);

expect(err).to.not.be.instanceOf(Error);
});

it('succeeds on listIndexes', async function () {
await col.createIndex({ a: 1 });
await col.createIndex({ b: -1 });
await col.createIndex({ a: 1, b: -1 });

const listIndexesResult = col.listIndexes({ batchSize: 2 });
const err = await listIndexesResult.toArray().catch(e => e);

expect(err).to.not.be.instanceOf(Error);
});

it('succeeds on changeStream', {
durran marked this conversation as resolved.
Show resolved Hide resolved
metadata: { requires: { topology: 'replicaset' } },
async test() {
const changeStream = col.watch(undefined, { batchSize: 2 });
const changes = on(changeStream, 'change');
await once(changeStream.cursor, 'init');

await col.insertMany(
[
{ a: 10 },
{ a: 10 },
{ a: 10 },
{ a: 10 },
{ a: 10 },
{ a: 10 },
{ a: 10 },
{ a: 10 },
{ a: 10 },
{ a: 10 },
{ a: 10 },
{ a: 10 }
],
{ writeConcern: { w: 'majority' } }
);

const err = await changes.next().catch(e => e);
expect(err).to.not.be.instanceOf(Error);
}
});
});
});
});
});
6 changes: 3 additions & 3 deletions test/unit/operations/find.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -25,15 +25,15 @@ describe('FindOperation', function () {
const operation = new FindOperation(undefined, namespace, filter, options);

it('sets the namespace', function () {
expect(operation.ns).to.equal(namespace);
expect(operation.ns).to.deep.equal(namespace);
});

it('sets options', function () {
expect(operation.options).to.equal(options);
expect(operation.options).to.deep.equal(options);
});

it('sets filter', function () {
expect(operation.filter).to.equal(filter);
expect(operation.filter).to.deep.equal(filter);
});
});

Expand Down