From c945bfc46f977f2b697f9f2910e915a324414e46 Mon Sep 17 00:00:00 2001 From: Bailey Pearson Date: Thu, 28 Jul 2022 14:49:49 -0400 Subject: [PATCH 1/8] fix: pass 'comment' option through to distinct command --- src/operations/distinct.ts | 13 +- test/spec/crud/unified/distinct-comment.json | 186 +++++++++++++++++++ test/spec/crud/unified/distinct-comment.yml | 98 ++++++++++ 3 files changed, 290 insertions(+), 7 deletions(-) create mode 100644 test/spec/crud/unified/distinct-comment.json create mode 100644 test/spec/crud/unified/distinct-comment.yml diff --git a/src/operations/distinct.ts b/src/operations/distinct.ts index d006d31133..00d65cc1ea 100644 --- a/src/operations/distinct.ts +++ b/src/operations/distinct.ts @@ -61,6 +61,12 @@ export class DistinctOperation extends CommandOperation { cmd.maxTimeMS = options.maxTimeMS; } + // we check for undefined specifically here to allow falsy values + // eslint-disable-next-line no-restricted-syntax + if (typeof options.comment !== 'undefined') { + cmd.comment = options.comment; + } + // Do we have a readConcern specified decorateWithReadConcern(cmd, coll, options); @@ -71,13 +77,6 @@ export class DistinctOperation extends CommandOperation { return callback(err); } - if (this.explain && maxWireVersion(server) < 4) { - callback( - new MongoCompatibilityError(`Server ${server.name} does not support explain on distinct`) - ); - return; - } - super.executeCommand(server, session, cmd, (err, result) => { if (err) { callback(err); diff --git a/test/spec/crud/unified/distinct-comment.json b/test/spec/crud/unified/distinct-comment.json new file mode 100644 index 0000000000..11bce9ac9d --- /dev/null +++ b/test/spec/crud/unified/distinct-comment.json @@ -0,0 +1,186 @@ +{ + "description": "distinct-comment", + "schemaVersion": "1.0", + "createEntities": [ + { + "client": { + "id": "client0", + "observeEvents": [ + "commandStartedEvent" + ] + } + }, + { + "database": { + "id": "database0", + "client": "client0", + "databaseName": "distinct-comment-tests" + } + }, + { + "collection": { + "id": "collection0", + "database": "database0", + "collectionName": "coll0" + } + } + ], + "initialData": [ + { + "collectionName": "coll0", + "databaseName": "distinct-comment-tests", + "documents": [ + { + "_id": 1, + "x": 11 + }, + { + "_id": 2, + "x": 22 + }, + { + "_id": 3, + "x": 33 + } + ] + } + ], + "tests": [ + { + "description": "distinct with document comment", + "runOnRequirements": [ + { + "minServerVersion": "4.4.14" + } + ], + "operations": [ + { + "name": "distinct", + "object": "collection0", + "arguments": { + "fieldName": "x", + "filter": {}, + "comment": { + "key": "value" + } + }, + "expectResult": [ + 11, + 22, + 33 + ] + } + ], + "expectEvents": [ + { + "client": "client0", + "events": [ + { + "commandStartedEvent": { + "command": { + "distinct": "coll0", + "key": "x", + "query": {}, + "comment": { + "key": "value" + } + }, + "commandName": "distinct", + "databaseName": "distinct-comment-tests" + } + } + ] + } + ] + }, + { + "description": "distinct with string comment", + "runOnRequirements": [ + { + "minServerVersion": "4.4.0" + } + ], + "operations": [ + { + "name": "distinct", + "object": "collection0", + "arguments": { + "fieldName": "x", + "filter": {}, + "comment": "comment" + }, + "expectResult": [ + 11, + 22, + 33 + ] + } + ], + "expectEvents": [ + { + "client": "client0", + "events": [ + { + "commandStartedEvent": { + "command": { + "distinct": "coll0", + "key": "x", + "query": {}, + "comment": "comment" + }, + "commandName": "distinct", + "databaseName": "distinct-comment-tests" + } + } + ] + } + ] + }, + { + "description": "distinct with document comment - pre 4.4, server error", + "runOnRequirements": [ + { + "minServerVersion": "3.6.0", + "maxServerVersion": "4.4.13" + } + ], + "operations": [ + { + "name": "distinct", + "object": "collection0", + "arguments": { + "fieldName": "x", + "filter": {}, + "comment": { + "key": "value" + } + }, + "expectError": { + "isClientError": false + } + } + ], + "expectEvents": [ + { + "client": "client0", + "events": [ + { + "commandStartedEvent": { + "command": { + "distinct": "coll0", + "key": "x", + "query": {}, + "comment": { + "key": "value" + } + }, + "commandName": "distinct", + "databaseName": "distinct-comment-tests" + } + } + ] + } + ] + } + ] +} diff --git a/test/spec/crud/unified/distinct-comment.yml b/test/spec/crud/unified/distinct-comment.yml new file mode 100644 index 0000000000..c082009943 --- /dev/null +++ b/test/spec/crud/unified/distinct-comment.yml @@ -0,0 +1,98 @@ +description: "distinct-comment" + +schemaVersion: "1.0" + +createEntities: + - client: + id: &client0 client0 + observeEvents: [ commandStartedEvent ] + - database: + id: &database0 database0 + client: *client0 + databaseName: &database0Name distinct-comment-tests + - collection: + id: &collection0 collection0 + database: *database0 + collectionName: &collection0Name coll0 + +initialData: + - collectionName: *collection0Name + databaseName: *database0Name + documents: + - { _id: 1, x: 11 } + - { _id: 2, x: 22 } + - { _id: 3, x: 33 } + +tests: + - description: "distinct with document comment" + runOnRequirements: + # https://jira.mongodb.org/browse/SERVER-44847 + # Server supports distinct with comment of any type for comment starting from 4.4.14. + - minServerVersion: "4.4.14" + operations: + - name: distinct + object: *collection0 + arguments: + fieldName: &fieldName x + filter: &filter {} + comment: &documentComment { key: "value"} + expectResult: [ 11, 22, 33 ] + expectEvents: + - client: *client0 + events: + - commandStartedEvent: + command: + distinct: *collection0Name + key: *fieldName + query: *filter + comment: *documentComment + commandName: distinct + databaseName: *database0Name + + - description: "distinct with string comment" + runOnRequirements: + - minServerVersion: "4.4.0" + operations: + - name: distinct + object: *collection0 + arguments: + fieldName: *fieldName + filter: *filter + comment: &stringComment "comment" + expectResult: [ 11, 22, 33 ] + expectEvents: + - client: *client0 + events: + - commandStartedEvent: + command: + distinct: *collection0Name + key: *fieldName + query: *filter + comment: *stringComment + commandName: distinct + databaseName: *database0Name + + - description: "distinct with document comment - pre 4.4, server error" + runOnRequirements: + - minServerVersion: "3.6.0" + maxServerVersion: "4.4.13" + operations: + - name: distinct + object: *collection0 + arguments: + fieldName: *fieldName + filter: *filter + comment: *documentComment + expectError: + isClientError: false + expectEvents: + - client: *client0 + events: + - commandStartedEvent: + command: + distinct: *collection0Name + key: *fieldName + query: *filter + comment: *documentComment + commandName: distinct + databaseName: *database0Name From 30f5a6e3068be6592a65bbd18500230e40be8945 Mon Sep 17 00:00:00 2001 From: Bailey Pearson Date: Thu, 28 Jul 2022 14:56:27 -0400 Subject: [PATCH 2/8] chore: remove unused imports from distinct.ts --- src/operations/distinct.ts | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/operations/distinct.ts b/src/operations/distinct.ts index 00d65cc1ea..5fe8585e81 100644 --- a/src/operations/distinct.ts +++ b/src/operations/distinct.ts @@ -1,9 +1,8 @@ import type { Document } from '../bson'; import type { Collection } from '../collection'; -import { MongoCompatibilityError } from '../error'; import type { Server } from '../sdam/server'; import type { ClientSession } from '../sessions'; -import { Callback, decorateWithCollation, decorateWithReadConcern, maxWireVersion } from '../utils'; +import { Callback, decorateWithCollation, decorateWithReadConcern } from '../utils'; import { CommandOperation, CommandOperationOptions } from './command'; import { Aspect, defineAspects } from './operation'; From 0ec4c80a11113214857c0473277ef6124a06b4c9 Mon Sep 17 00:00:00 2001 From: Bailey Pearson Date: Mon, 1 Aug 2022 13:18:39 -0400 Subject: [PATCH 3/8] test: add falsy values test to distinct --- .../comment_with_falsy_values.test.ts | 4 +- .../node-specific/distinct.test.ts | 39 +++++++++++++++++++ 2 files changed, 41 insertions(+), 2 deletions(-) create mode 100644 test/integration/node-specific/distinct.test.ts diff --git a/test/integration/node-specific/comment_with_falsy_values.test.ts b/test/integration/node-specific/comment_with_falsy_values.test.ts index 92c86e1145..418e249053 100644 --- a/test/integration/node-specific/comment_with_falsy_values.test.ts +++ b/test/integration/node-specific/comment_with_falsy_values.test.ts @@ -1,8 +1,8 @@ import { Long } from '../../../src'; import { TestBuilder, UnifiedTestSuiteBuilder } from '../../tools/utils'; -const falsyValues = [0, false, '', Long.ZERO, null, NaN] as const; -const falsyToString = (value: typeof falsyValues[number]) => { +export const falsyValues = [0, false, '', Long.ZERO, null, NaN] as const; +export const falsyToString = (value: typeof falsyValues[number]) => { if (Number.isNaN(value)) { return 'NaN'; } diff --git a/test/integration/node-specific/distinct.test.ts b/test/integration/node-specific/distinct.test.ts new file mode 100644 index 0000000000..6deef4f8c7 --- /dev/null +++ b/test/integration/node-specific/distinct.test.ts @@ -0,0 +1,39 @@ +import { expect } from 'chai'; + +import { Collection, CommandStartedEvent, Db, MongoClient } from '../../../src'; +import { falsyToString, falsyValues } from './comment_with_falsy_values.test'; + +context('command distinct', function () { + let client: MongoClient; + let collection: Collection; + let commands: CommandStartedEvent[] = []; + + beforeEach(async function () { + client = this.configuration.newClient({ monitorCommands: true }); + client.on('commandStarted', e => commands.push(e)); + await client.connect(); + collection = await client.db('comment-falsy-values').createCollection('collection'); + commands = []; + }); + + afterEach(async function () { + await collection.drop(); + await client.close(); + }); + + context('comment with falsy values', function () { + for (const falsyValue of falsyValues) { + it(`should send falsy value ${falsyToString(falsyValue)} on the command`, async function () { + await collection.distinct('some-key', {}, { comment: falsyValue }); + + expect(commands).to.have.lengthOf(1); + const distinctCommand = commands.find(command => command.commandName === 'distinct'); + expect(distinctCommand).to.exist; + + // chai does not narrow types, so TS doesn't know the distinct command exists at this point. + // eslint-disable-next-line @typescript-eslint/no-non-null-assertion + expect(distinctCommand!.command).to.haveOwnProperty('comment'); + }); + } + }); +}); From 7dbedb9f082516a3102a6a6df9612b01ea98280d Mon Sep 17 00:00:00 2001 From: Bailey Pearson Date: Mon, 1 Aug 2022 13:19:06 -0400 Subject: [PATCH 4/8] test: swallow errors when distinct fails, because we want to check the command --- test/integration/node-specific/distinct.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/integration/node-specific/distinct.test.ts b/test/integration/node-specific/distinct.test.ts index 6deef4f8c7..434caf2c2f 100644 --- a/test/integration/node-specific/distinct.test.ts +++ b/test/integration/node-specific/distinct.test.ts @@ -24,7 +24,7 @@ context('command distinct', function () { context('comment with falsy values', function () { for (const falsyValue of falsyValues) { it(`should send falsy value ${falsyToString(falsyValue)} on the command`, async function () { - await collection.distinct('some-key', {}, { comment: falsyValue }); + await collection.distinct('some-key', {}, { comment: falsyValue }).catch(() => null); expect(commands).to.have.lengthOf(1); const distinctCommand = commands.find(command => command.commandName === 'distinct'); From b0cabc55cb7ee62dd809dae137dbe3793ad67d80 Mon Sep 17 00:00:00 2001 From: Bailey Pearson Date: Mon, 1 Aug 2022 14:46:45 -0400 Subject: [PATCH 5/8] chore: fix lint --- test/integration/node-specific/distinct.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/integration/node-specific/distinct.test.ts b/test/integration/node-specific/distinct.test.ts index 434caf2c2f..f730e97ce9 100644 --- a/test/integration/node-specific/distinct.test.ts +++ b/test/integration/node-specific/distinct.test.ts @@ -1,6 +1,6 @@ import { expect } from 'chai'; -import { Collection, CommandStartedEvent, Db, MongoClient } from '../../../src'; +import { Collection, CommandStartedEvent, MongoClient } from '../../../src'; import { falsyToString, falsyValues } from './comment_with_falsy_values.test'; context('command distinct', function () { From a096393e12f54b9032f810697c756883011b0157 Mon Sep 17 00:00:00 2001 From: Bailey Pearson Date: Tue, 2 Aug 2022 16:59:00 -0400 Subject: [PATCH 6/8] test: move new distinct comment falsy values tests into comment falsy value test file --- .../comment_with_falsy_values.test.ts | 39 ++++++++++++++++++- .../node-specific/distinct.test.ts | 39 ------------------- 2 files changed, 38 insertions(+), 40 deletions(-) delete mode 100644 test/integration/node-specific/distinct.test.ts diff --git a/test/integration/node-specific/comment_with_falsy_values.test.ts b/test/integration/node-specific/comment_with_falsy_values.test.ts index 418e249053..cc87a99741 100644 --- a/test/integration/node-specific/comment_with_falsy_values.test.ts +++ b/test/integration/node-specific/comment_with_falsy_values.test.ts @@ -1,4 +1,6 @@ -import { Long } from '../../../src'; +import { expect } from 'chai'; + +import { Collection, CommandStartedEvent, Long, MongoClient } from '../../../src'; import { TestBuilder, UnifiedTestSuiteBuilder } from '../../tools/utils'; export const falsyValues = [0, false, '', Long.ZERO, null, NaN] as const; @@ -173,4 +175,39 @@ describe('Comment with falsy values', () => { .test(testsForChangeStreamsAggregate) .test(testsForGetMore) .run(); + + context('Collection.distinct()', function () { + let client: MongoClient; + let collection: Collection; + let commands: CommandStartedEvent[] = []; + + beforeEach(async function () { + client = this.configuration.newClient({ monitorCommands: true }); + client.on('commandStarted', e => commands.push(e)); + await client.connect(); + collection = await client.db('comment-falsy-values').createCollection('collection'); + commands = []; + }); + + afterEach(async function () { + await collection.drop(); + await client.close(); + }); + + for (const falsyValue of falsyValues) { + it(`distinct should send falsy value ${falsyToString( + falsyValue + )} on the command`, async function () { + await collection.distinct('some-key', {}, { comment: falsyValue }).catch(() => null); + + expect(commands).to.have.lengthOf(1); + const distinctCommand = commands.find(command => command.commandName === 'distinct'); + expect(distinctCommand).to.exist; + + // chai does not narrow types, so TS doesn't know the distinct command exists at this point. + // eslint-disable-next-line @typescript-eslint/no-non-null-assertion + expect(distinctCommand!.command).to.haveOwnProperty('comment'); + }); + } + }); }); diff --git a/test/integration/node-specific/distinct.test.ts b/test/integration/node-specific/distinct.test.ts deleted file mode 100644 index f730e97ce9..0000000000 --- a/test/integration/node-specific/distinct.test.ts +++ /dev/null @@ -1,39 +0,0 @@ -import { expect } from 'chai'; - -import { Collection, CommandStartedEvent, MongoClient } from '../../../src'; -import { falsyToString, falsyValues } from './comment_with_falsy_values.test'; - -context('command distinct', function () { - let client: MongoClient; - let collection: Collection; - let commands: CommandStartedEvent[] = []; - - beforeEach(async function () { - client = this.configuration.newClient({ monitorCommands: true }); - client.on('commandStarted', e => commands.push(e)); - await client.connect(); - collection = await client.db('comment-falsy-values').createCollection('collection'); - commands = []; - }); - - afterEach(async function () { - await collection.drop(); - await client.close(); - }); - - context('comment with falsy values', function () { - for (const falsyValue of falsyValues) { - it(`should send falsy value ${falsyToString(falsyValue)} on the command`, async function () { - await collection.distinct('some-key', {}, { comment: falsyValue }).catch(() => null); - - expect(commands).to.have.lengthOf(1); - const distinctCommand = commands.find(command => command.commandName === 'distinct'); - expect(distinctCommand).to.exist; - - // chai does not narrow types, so TS doesn't know the distinct command exists at this point. - // eslint-disable-next-line @typescript-eslint/no-non-null-assertion - expect(distinctCommand!.command).to.haveOwnProperty('comment'); - }); - } - }); -}); From cf79223628971d968d2a73fa112f7625b158ac15 Mon Sep 17 00:00:00 2001 From: Bailey Pearson Date: Tue, 2 Aug 2022 17:00:28 -0400 Subject: [PATCH 7/8] test: unexport variables from comment falsy values test file --- .../node-specific/comment_with_falsy_values.test.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/integration/node-specific/comment_with_falsy_values.test.ts b/test/integration/node-specific/comment_with_falsy_values.test.ts index cc87a99741..d4dfa20301 100644 --- a/test/integration/node-specific/comment_with_falsy_values.test.ts +++ b/test/integration/node-specific/comment_with_falsy_values.test.ts @@ -3,8 +3,8 @@ import { expect } from 'chai'; import { Collection, CommandStartedEvent, Long, MongoClient } from '../../../src'; import { TestBuilder, UnifiedTestSuiteBuilder } from '../../tools/utils'; -export const falsyValues = [0, false, '', Long.ZERO, null, NaN] as const; -export const falsyToString = (value: typeof falsyValues[number]) => { +const falsyValues = [0, false, '', Long.ZERO, null, NaN] as const; +const falsyToString = (value: typeof falsyValues[number]) => { if (Number.isNaN(value)) { return 'NaN'; } From c0753f084c60be19903c68ca4de903e6e405f36e Mon Sep 17 00:00:00 2001 From: Bailey Pearson Date: Wed, 3 Aug 2022 11:24:28 -0400 Subject: [PATCH 8/8] test: assert on falsy value equality --- .../node-specific/comment_with_falsy_values.test.ts | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/test/integration/node-specific/comment_with_falsy_values.test.ts b/test/integration/node-specific/comment_with_falsy_values.test.ts index d4dfa20301..9cc8950cec 100644 --- a/test/integration/node-specific/comment_with_falsy_values.test.ts +++ b/test/integration/node-specific/comment_with_falsy_values.test.ts @@ -206,7 +206,15 @@ describe('Comment with falsy values', () => { // chai does not narrow types, so TS doesn't know the distinct command exists at this point. // eslint-disable-next-line @typescript-eslint/no-non-null-assertion - expect(distinctCommand!.command).to.haveOwnProperty('comment'); + const command = distinctCommand!.command; + + expect(command).to.haveOwnProperty('comment'); + + if (Number.isNaN(falsyValue)) { + expect(command.comment).to.be.NaN; + } else { + expect(command.comment).to.equal(falsyValue); + } }); } });