feat(NODE-3699): add support for `comment` field #3167

baileympearson · 2022-03-09T20:35:01Z

Description

What is changing?

This PR adds support for the comment field. It includes changes from three drivers tickets:

Is there new documentation needed for these changes?

No.

Notes

Not modified:

the legacy count comment specified a $comment field of type string. The new comment field is supported in MongoDB 4.4+ and our legacy count command only runs for server versions <=3.2.
There is pre-existing logic in CommandOperation.executeCommand to attach a comment to a command when executing the command if the comment is of type string. All the new operations in the drivers spec attach the comment field to the command prior to calling super.executeOperation, so the logic in CommandOperation.executeCommand does not need to be updated. I also decided to leave it as-it-is (instead of making the change to accept non-string comments in this location specifically) because I didn't want to change the behavior for commands which only support string comments.

Double check the following

Ran npm run check:lint script
Self-review completed using the steps outlined here
PR title follows the correct format: <type>(NODE-xxxx)<!>: <description>
Changes are covered by tests
New TODOs have a related JIRA ticket

src/operations/delete.ts

src/operations/find_and_modify.ts

src/operations/update.ts

src/change_stream.ts

nbbeeken

whoops, didn't start a review before commenting, sorry for all the pings

test/unit/connection_string.test.ts

src/operations/get_more.ts

src/cursor/abstract_cursor.ts

test/unit/operations/get_more.test.ts

dariakp

Leaving feedback on everything except the tests.

src/change_stream.ts

src/cmap/connection.ts

src/change_stream.ts

src/operations/aggregate.ts

src/operations/delete.ts

src/operations/find.ts

src/operations/get_more.ts

src/utils.ts

dariakp

I still need to review the builder and the falsy values test, but here's feedback on the rest of the tests

test/unit/connection_string.test.ts

test/unit/operations/get_more.test.ts

test/integration/unified-test-format/unified_test_format.spec.test.ts

test/tools/unified-spec-runner/match.ts

test/tools/unified-spec-runner/operations.ts

test/tools/unified-spec-runner/schema.ts

test/tools/utils.ts

test/spec/change-streams/unified/change-streams.json

test/tools/utils.ts

…string tests

test/tools/unified-spec-runner/match.ts

nbbeeken

LGTM

dariakp

just a couple of minor but important notes

test/unit/operations/list_collections.test.js

test/integration/enumerate_collections.test.ts

test/integration/enumerate_databases.test.ts

test/integration/enumerate_indexes.test.ts

baileympearson force-pushed the NODE-3699 branch 2 times, most recently from ec05837 to dfcf905 Compare March 14, 2022 19:13

baileympearson commented Mar 14, 2022

View reviewed changes

src/operations/delete.ts Outdated Show resolved Hide resolved

baileympearson marked this pull request as ready for review March 14, 2022 19:52

baileympearson changed the title ~~wip feat(NODE-3699): Add support for comment field~~ feat(NODE-3699): Add support for comment field Mar 14, 2022

nbbeeken reviewed Mar 15, 2022

View reviewed changes

src/operations/find_and_modify.ts Outdated Show resolved Hide resolved

nbbeeken reviewed Mar 15, 2022

View reviewed changes

src/operations/update.ts Outdated Show resolved Hide resolved

nbbeeken reviewed Mar 15, 2022

View reviewed changes

src/change_stream.ts Outdated Show resolved Hide resolved

nbbeeken requested changes Mar 15, 2022

View reviewed changes

nbbeeken added the Primary Review In Review with primary reviewer, not yet ready for team's eyes label Mar 16, 2022

baileympearson force-pushed the NODE-3699 branch from 9b105e8 to 0224de0 Compare March 17, 2022 19:09

baileympearson commented Mar 22, 2022

View reviewed changes

test/unit/connection_string.test.ts Outdated Show resolved Hide resolved

baileympearson requested a review from nbbeeken March 22, 2022 17:17

nbbeeken reviewed Mar 25, 2022

View reviewed changes

src/operations/get_more.ts Outdated Show resolved Hide resolved

nbbeeken reviewed Mar 25, 2022

View reviewed changes

src/operations/get_more.ts Outdated Show resolved Hide resolved

nbbeeken reviewed Mar 25, 2022

View reviewed changes

src/cursor/abstract_cursor.ts Outdated Show resolved Hide resolved

nbbeeken reviewed Mar 25, 2022

View reviewed changes

src/cursor/abstract_cursor.ts Show resolved Hide resolved

nbbeeken requested changes Mar 25, 2022

View reviewed changes

test/unit/operations/get_more.test.ts Show resolved Hide resolved

baileympearson added 12 commits March 28, 2022 11:20

Add 24 new crud test files

bbbd074

Add remaining 4 tests

87546ed

feat: Add support for comment field in update operations

e0eb13f

Sync change streams test with latest specs

49a9c48

fix: bring in latest versions of comment tests

911f5c4

fix: uncomment atomic operators check in update.ts

ac9fd81

resolve merge conflicts

a5e4de6

finish conflict

704764a

fix: skip failing change stream tests

07df766

fix: address linting issues

d7d7d28

fix: skip change streams pre 4.4 test

6bb15ec

docs: add docstring comments to 'comment' option

fec0040

dariakp requested changes Mar 29, 2022

View reviewed changes

wip - address comments pt 1

f1b15f9

nbbeeken added Team Review Needs review from team and removed Primary Review In Review with primary reviewer, not yet ready for team's eyes labels Mar 30, 2022

dariakp requested changes Mar 30, 2022

View reviewed changes

test/tools/utils.ts Outdated Show resolved Hide resolved

baileympearson added 10 commits March 30, 2022 14:45

feat: add comment support to listCollections and listIndexes

a74b530

fix: reorganize tests && add tests for listDatabases

f180064

fix: move sinon stubbing into tests and revert changes to connection …

42147ee

…string tests

fix: address comments

0bcdfc0

chore: add test case for positive and negative zero

a5523e0

fix: run on requireemnts in change stream falsy tests

459344d

fix: use correct run on requirement for list* tests

8aac554

wip

ece725d

fix: don't run chnage stream tests on shareded topology

9def648

fix: remove only

cd2707d

baileympearson requested a review from dariakp March 31, 2022 14:24

nbbeeken reviewed Mar 31, 2022

View reviewed changes

test/tools/unified-spec-runner/match.ts Outdated Show resolved Hide resolved

fix: address Neal's latest comments

8518dcd

baileympearson requested a review from nbbeeken March 31, 2022 18:24

nbbeeken previously approved these changes Mar 31, 2022

View reviewed changes

dariakp requested changes Mar 31, 2022

View reviewed changes

fix: address Daria's comments

157c75d

baileympearson dismissed nbbeeken’s stale review via 157c75d April 1, 2022 14:22

baileympearson requested review from dariakp and nbbeeken April 1, 2022 14:57

dariakp approved these changes Apr 1, 2022

View reviewed changes

nbbeeken changed the title ~~feat(NODE-3699): Add support for comment field~~ feat(NODE-3699): add support for comment field Apr 1, 2022

nbbeeken merged commit 4e2f9bf into main Apr 1, 2022

nbbeeken deleted the NODE-3699 branch April 1, 2022 15:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(NODE-3699): add support for `comment` field #3167

feat(NODE-3699): add support for `comment` field #3167

baileympearson commented Mar 9, 2022 •

edited

Loading

nbbeeken left a comment

dariakp left a comment

dariakp left a comment

nbbeeken left a comment

dariakp left a comment

feat(NODE-3699): add support for comment field #3167

feat(NODE-3699): add support for comment field #3167

Conversation

baileympearson commented Mar 9, 2022 • edited Loading

Description

What is changing?

Is there new documentation needed for these changes?

Notes

Double check the following

nbbeeken left a comment

Choose a reason for hiding this comment

dariakp left a comment

Choose a reason for hiding this comment

dariakp left a comment

Choose a reason for hiding this comment

nbbeeken left a comment

Choose a reason for hiding this comment

dariakp left a comment

Choose a reason for hiding this comment

feat(NODE-3699): add support for `comment` field #3167

feat(NODE-3699): add support for `comment` field #3167

baileympearson commented Mar 9, 2022 •

edited

Loading