feat(NODE-5241): add option to return modified document #3710

durran · 2023-06-07T12:43:35Z

Description

Adds a new option to the findOneAnd... family of methods, includeResultMetadata which defaults to true. When set to the default, it will return a ModifyResult, when set to false, it will return the actual modified document.

What is changing?

Adds the includeResultMetadata option.
Update the types of findOneAndReplace, findOneAndDelete, findOneAndUpdate.
Adds new type tests.
Refactors crud api tests and find and modify tests to use async/await.
Removes redundant tests.

Is there new documentation needed for these changes?

Yes

What is the motivation for this change?

NODE-5241

Release Highlight

New `includeResultMetadata` option for `findOneAnd...` family of methods.

This option defaults to true, which will return a ModifyResult type. When set to false, which will
become the default in the next major release, it will return the modified document or null if nothing matched.
This applies to findOneAndDelete, findOneAndUpdate, findOneAndReplace.

// With a document { _id: 1, a: 1 } in the collection
await collection.findOneAndDelete({ a: 1 }, { includeResultMetadata: false }); // returns { _id: 1, a: 1 }
await collection.findOneAndDelete({ a: 2 }, { includeResultMetadata: false }); // returns null
await collection.findOneAndDelete({ a: 1 }, { includeResultMetadata: true }); // returns { ok: 1, lastErrorObject: { n: 1 }, value: { _id: 1, a: 1 }}

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
- Example: feat(NODE-1234)!: rewriting everything in coffeescript
Changes are covered by tests
New TODOs have a related JIRA ticket

dariakp · 2023-06-07T19:54:48Z

src/operations/find_and_modify.ts

@@ -29,6 +29,8 @@ export interface FindOneAndDeleteOptions extends CommandOperationOptions {
  sort?: Sort;
  /** Map of parameter names and values that can be accessed using $$var (requires MongoDB 5.0). */
  let?: Document;
+  /** Return the raw result document instead of the ModifyResult */
+  returnRawResult?: boolean;


Just wanted to bring a couple of points to your attention from the ticket description so that we minimize potential back and forth in review:

Dev note: returnRawResult is not meant to be prescriptive - the server returns a write_ops::FindAndModifyCommandReply, internally the driver returns a ModifyResult but our flag should make it clearer as to what is being returned

API docs should clearly explain the option

It may be worth taking this back to the team for consensus on option name; once the option name is settled, it's important to make sure we set accurate expectations for what the option does. The spec compliant version of these commands returns less information than the current ModifyResult. In the original ticket, returnRawResult = true is intended to return what is currently ModifyResult and returnRawResult = false is intended to return the spec compliant version, which is equal to ModifyResult.value.

durran · 2023-06-12T13:09:37Z

test/integration/crud/find.test.js

-  /**
-   * Test findOneAndUpdate a document
-   */
-  it('shouldCorrectlyfindOneAndUpdateDocument', {


The cases in this giant test case are covered in individual findAndModify tests as well as spec tests.

durran · 2023-06-12T13:10:07Z

test/integration/crud/find.test.js

-  /**
-   * Test findOneAndUpdate a document
-   */
-  it('Should Correctly Handle findOneAndUpdate Duplicate Key Error', {


This was testing server behaviour.

baileympearson · 2023-06-12T17:07:21Z

test/integration/crud/crud_api.test.ts

    metadata: {
      requires: { topology: ['single', 'replicaset', 'sharded', 'ssl', 'heap', 'wiredtiger'] }
    },
+    test: async function () {


I think all of these topology requirements are no longer necessary. Do you want to remove the metadata block entirely?

(same is true for all tests I think)

baileympearson · 2023-06-12T17:10:08Z

test/integration/crud/crud_api.test.ts


-                  findOneAndReplace();


just want to comment on this ... recursive test functions? wow. 🙁

test/integration/crud/find_and_modify.test.ts

src/operations/find_and_modify.ts

baileympearson · 2023-06-12T18:10:26Z

test/types/schema_helpers.test-d.ts

@@ -127,6 +135,41 @@ expectAssignable<SchemaWithIdNumberInterface | null>(
  (await interfaceNumberTestCollection.findOneAndUpdate({ a: 1 }, { a: 5 })).value
 );

+expectAssignable<SchemaWithIdNumberType | null>(


Without helpful errors / describe blocks / test names, it's hard to GROK what these tests are testing. I think they're testing the return type of findOneAnd* methods when includeResultMetadata is false?

should these tests live in findX.test-d.ts? They're not actually related to how we infer the collection schema (which is that this file is for I think)

can we comment them to make it clear what they're testing? Something I've considered in the past is an anonmyous scope with a comment:

// findOneAnd* methods return the modified document when `includeResultMetadata` is false { expectAssignable<SchemaWithIdNumberInterface | null>( await interfaceNumberTestCollection.findOneAndDelete({ a: 1 }, { includeResultMetadata: false }) ); ... }

I've reverted the schema_helpers.test-d.ts changes per your comment, and simplified type testing by just adding 3 tests (1 for each method) in findX-test-d.ts and added a comment to them.

baileympearson · 2023-06-16T15:22:02Z

test/integration/crud/find_and_modify.test.ts

+import { expect } from 'chai';
+
+import { type CommandStartedEvent, MongoServerError, ObjectId } from '../../mongodb';
+import { setupDatabase } from '../shared';


Do we have any tests for includeResultMetadata = false? I see tests for the default behavior, but nothing with includeResultMetadata disabled.

Added tests with the option set to false.

test/types/community/collection/findX.test-d.ts

src/operations/find_and_modify.ts

durran changed the title ~~Node 5241~~ feat!(NODE-5241): return raw result by default with find and modify Jun 7, 2023

dariakp reviewed Jun 7, 2023

View reviewed changes

durran force-pushed the NODE-5241 branch 4 times, most recently from 9a3664c to 7b75770 Compare June 12, 2023 13:07

durran commented Jun 12, 2023

View reviewed changes

durran marked this pull request as ready for review June 12, 2023 16:21

baileympearson changed the title ~~feat!(NODE-5241): return raw result by default with find and modify~~ feat(NODE-5241): return raw result by default with find and modify Jun 12, 2023

durran changed the title ~~feat(NODE-5241): return raw result by default with find and modify~~ feat!(NODE-5241): add option to return modified document Jun 12, 2023

baileympearson requested changes Jun 12, 2023

View reviewed changes

dariakp changed the title ~~feat!(NODE-5241): add option to return modified document~~ feat(NODE-5241): add option to return modified document Jun 12, 2023

dariakp assigned baileympearson Jun 12, 2023

dariakp added the Primary Review label Jun 12, 2023

durran force-pushed the NODE-5241 branch 2 times, most recently from 4cf8db6 to 33023ff Compare June 15, 2023 13:13

durran requested a review from baileympearson June 15, 2023 18:28

baileympearson requested changes Jun 16, 2023

View reviewed changes

durran force-pushed the NODE-5241 branch from 8e0d88e to 1ad018c Compare June 19, 2023 14:15

durran requested a review from baileympearson June 19, 2023 14:29

baileympearson previously approved these changes Jun 20, 2023

View reviewed changes

nbbeeken added Team Review and removed Primary Review labels Jun 20, 2023

nbbeeken requested changes Jun 20, 2023

View reviewed changes

src/operations/find_and_modify.ts Show resolved Hide resolved

durran added 5 commits June 21, 2023 13:38

feat(NODE-5241): allow raw result from find and modify

f2343e2

feat!(NODE-5241): return raw result by default in find and modify

81a4132

fix: fix overloads

713985e

fix: reverse default

Verified

This commit was signed with the committer’s verified signature.

Friendseeker Jerry Tan

GPG key ID: 7DD5039728A27160

Verified
Learn about vigilant mode

474f0fb

fix: tests

61aa1a8

durran added 8 commits June 21, 2023 13:38

test: refactor find and modify test

946ffb6

test: refactor crud api test

4d9a42f

test: update type tests

4be0d02

test: mode fixes

30c8d5a

test: monitor commands

c6f5925

test: monitor commands

a21fdbe

test: update suggestions

710dc0e

test: add null tests

b2b8a72

durran dismissed baileympearson’s stale review via b2b8a72 June 21, 2023 11:43

durran force-pushed the NODE-5241 branch from 1ad018c to b2b8a72 Compare June 21, 2023 11:43

durran requested a review from nbbeeken June 21, 2023 11:44

baileympearson approved these changes Jun 21, 2023

View reviewed changes

baileympearson merged commit d9c2600 into main Jun 21, 2023

baileympearson deleted the NODE-5241 branch June 21, 2023 14:16

github-actions bot mentioned this pull request Jun 21, 2023

chore(main): release 5.7.0 [skip-ci] #3735

Merged

malikj2000 pushed a commit that referenced this pull request Jun 29, 2023

feat(NODE-5241): add option to return modified document (#3710)

151282d

This was referenced Aug 21, 2023

chore(5.x): release 5.8.0 [skip-ci] #3825

Merged

chore(main): release 6.0.0 [skip-ci] #3762

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(NODE-5241): add option to return modified document #3710

feat(NODE-5241): add option to return modified document #3710

durran commented Jun 7, 2023 •

edited

Loading

dariakp Jun 7, 2023

durran Jun 12, 2023

durran Jun 12, 2023

baileympearson Jun 12, 2023

durran Jun 15, 2023

baileympearson Jun 12, 2023

baileympearson Jun 12, 2023

durran Jun 15, 2023

baileympearson Jun 16, 2023

durran Jun 19, 2023

feat(NODE-5241): add option to return modified document #3710

feat(NODE-5241): add option to return modified document #3710

Conversation

durran commented Jun 7, 2023 • edited Loading

Description

What is changing?

Is there new documentation needed for these changes?

What is the motivation for this change?

Release Highlight

New includeResultMetadata option for findOneAnd... family of methods.

Double check the following

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

durran commented Jun 7, 2023 •

edited

Loading

New `includeResultMetadata` option for `findOneAnd...` family of methods.