feat(NODE-4034)!: make internal bulk result private #3515

durran · 2023-01-05T18:29:30Z

Description

Makes the internal BulkResult on the BulkWriteResult private.

What is changing?

Marks the BulkResult interface as internal.
Makes the result property of the BulkWriteResult private and non-enumerable.
Makes all properties of the BulkWriteResult per the crud spec definition actual properties instead of getters.
Removes toJSON().
DRYs up the generation of the inserted/upserted id maps.
Eliminates insertedIds from the result enumerable in unified tests.

mongosh CI failures fix in this PR: mongodb-js/mongosh#1386

Is there new documentation needed for these changes?

None

What is the motivation for this change?

NODE-4034/NODE-4393/NODE-4973

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

durran · 2023-01-11T11:14:35Z

src/bulk/common.ts

    const mergeResult = mergeBatchResults(batch, bulkOperation.s.bulkResult, err, result);
+    const writeResult = new BulkWriteResult(bulkOperation.s.bulkResult);


mergeBatchResults() mutates the provided bulkResult so we move the construction of the BulkWriteResult until afterwards so it acts as immutable.

Just clarification for further reviewers.

Before the changes in this PR, BulkWriteResult used getters to provide access to each property on the raw bulk result, so constructing the BulkWriteResult before modifying the raw result was okay.

Now, we assign the properties directly in the constructor, so we need to create the BulkWriteResult after we process the raw result.

baileympearson · 2023-01-11T15:08:11Z

src/bulk/common.ts

+   * @internal
+   */
+  constructor(bulkResult: BulkResult) {
+    this.result = bulkResult;


Is there a reason to assign result here and then override it in line 207?

The override was to set it as non-enumerable but typescript complained if I did it all in one go here. (It couldn't figure out that Object.defineProperty was setting this.result.

src/bulk/common.ts

baileympearson · 2023-01-11T15:48:44Z

src/bulk/common.ts

    const mergeResult = mergeBatchResults(batch, bulkOperation.s.bulkResult, err, result);
+    const writeResult = new BulkWriteResult(bulkOperation.s.bulkResult);


Just clarification for further reviewers.

Before the changes in this PR, BulkWriteResult used getters to provide access to each property on the raw bulk result, so constructing the BulkWriteResult before modifying the raw result was okay.

Now, we assign the properties directly in the constructor, so we need to create the BulkWriteResult after we process the raw result.

test/integration/retryable-writes/retryable_writes.spec.test.ts

dariakp · 2023-01-12T16:01:25Z

src/bulk/common.ts

+    this.deletedCount = this.result.nRemoved ?? 0;
+    this.upsertedCount = this.result.upserted.length ?? 0;
+    this.upsertedIds = BulkWriteResult.generateIdMap(this.result.upserted);
+    this.insertedIds = BulkWriteResult.generateIdMap(this.result.insertedIds);


Have you had a chance to check with any other drivers to see why they aren't running into the same insertedIds issues? It may be good to know if they are also implementing an internal work around, or aren't spec compliant themselves, or perhaps interpreting the spec differently.

Ruby iterates over the keys in the expected result and checks the actual object for that specific property, so it does not error when the actual result has more properties than the expected. Python does the same. It seems we are probably the only driver looking for an exact match of the actual and expected object properties, which makes us less flexible on adding other properties to our result classes.

The CRUD spec is clear about required and optional fields in the result objects, but does not say if additional properties on the result classes are forbidden. It seems like Node took the spec strictly but other drivers allowed a bit more flexibility - neither is incorrect in my opinion.

@dariakp Are you okay with this answer? This is the last outstanding thread on this PR.

The "Evaluating Matches" section of the unified runner spec is very explicit about asserting that the properties of a subdocument inside an expected result property must match exactly:
https://github.com/mongodb/specifications/blob/master/source/unified-test-format/unified-test-format.rst#evaluating-matches

also

Test runners MUST NOT permit additional fields in nested documents.

here: https://github.com/mongodb/specifications/blob/master/source/unified-test-format/unified-test-format.rst#allowing-extra-fields-in-root-level-documents

So, per the spec, we are correct, and the spec tests must account for allowed optional properties via special test operators (basically, what you have filed in the DRIVERS-2523 is addressing a bug in the test implementation for RewrapManyDataKey; it is also an omission for the unified spec that we don't have the rewrapManyDataKey operation defined in the unified format spec and are also not referencing the spec where that operation is expected to be defined). This is an issue for insertMany tests, too, and for the "acknowledged" property in general (which perhaps no driver implements); surprised that is not coming up here (unless we already have the logic to handle it). I don't think we should treat BulkWriteResult as a root level document, because we do know what optional fields are allowed to exist on it, and we do not want it to have arbitrary fields beyond the ones defined by the spec.

I added a comment to the drivers ticket, we don't need to keep this PR open on that account

Since the test updates merged for DRIVERS-2523 I just went ahead and added them here.

@durran Should that ticket be updated to "teams implementing"? (That way we can tag the generated node ticket number in this PR and it'll auto-link in jira)

I did update it but for some reason it didn't generate any individual language tickets. Hmm.

Description updated with NODE-4973 now.

Also linked all 3 tickets to NODE-4034 now in JIRA.

durran force-pushed the NODE-4034 branch from 103e78a to d006adc Compare January 5, 2023 19:16

durran changed the title ~~feat(NODE-4034): make internal bulk result private~~ feat(NODE-4034)!: make internal bulk result private Jan 5, 2023

durran force-pushed the NODE-4034 branch 3 times, most recently from 96a8512 to 51873e2 Compare January 11, 2023 10:40

durran commented Jan 11, 2023

View reviewed changes

durran marked this pull request as ready for review January 11, 2023 11:28

baileympearson requested changes Jan 11, 2023

View reviewed changes

durran force-pushed the NODE-4034 branch from 2e49926 to 6ac961a Compare January 12, 2023 10:14

durran requested a review from baileympearson January 12, 2023 10:16

durran mentioned this pull request Jan 12, 2023

fix(service-provider-core): bulk result now marked internal NODE-4034 mongodb-js/mongosh#1386

Merged

durran added the Primary Review In Review with primary reviewer, not yet ready for team's eyes label Jan 12, 2023

baileympearson requested changes Jan 12, 2023

View reviewed changes

test/integration/retryable-writes/retryable_writes.spec.test.ts Outdated Show resolved Hide resolved

test/integration/retryable-writes/retryable_writes.spec.test.ts Outdated Show resolved Hide resolved

durran requested a review from baileympearson January 12, 2023 13:48

dariakp reviewed Jan 12, 2023

View reviewed changes

baileympearson added Team Review Needs review from team and removed Primary Review In Review with primary reviewer, not yet ready for team's eyes labels Jan 13, 2023

durran added 9 commits January 17, 2023 17:32

feat(NODE-4034): make internal bulk result private

6625a45

feat: add properties to bulk write result

d592a0f

test: fix missing insertedIds in spec tests

8ea38e5

chore: update migration guide

80bdaa3

chore: link drivers ticket

cb834ea

fix: add suggestions

4971fee

test: fix test

4b6bea5

test: fix tests

1badb1b

test: add insertedIds to csfle bulk results

318ffcb

durran force-pushed the NODE-4034 branch from 2888300 to 318ffcb Compare January 17, 2023 16:32

baileympearson approved these changes Jan 17, 2023

View reviewed changes

baileympearson requested a review from dariakp January 17, 2023 20:27

baileympearson merged commit ebac1f5 into main Jan 18, 2023

baileympearson deleted the NODE-4034 branch January 18, 2023 20:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(NODE-4034)!: make internal bulk result private #3515

feat(NODE-4034)!: make internal bulk result private #3515

durran commented Jan 5, 2023 •

edited

Loading

durran Jan 11, 2023

baileympearson Jan 11, 2023

baileympearson Jan 11, 2023

durran Jan 12, 2023

baileympearson Jan 11, 2023

dariakp Jan 12, 2023

durran Jan 12, 2023

baileympearson Jan 13, 2023

dariakp Jan 13, 2023

dariakp Jan 13, 2023

durran Jan 17, 2023

dariakp Jan 17, 2023

durran Jan 18, 2023

durran Jan 18, 2023

durran Jan 18, 2023

		const mergeResult = mergeBatchResults(batch, bulkOperation.s.bulkResult, err, result);
		const writeResult = new BulkWriteResult(bulkOperation.s.bulkResult);

feat(NODE-4034)!: make internal bulk result private #3515

feat(NODE-4034)!: make internal bulk result private #3515

Conversation

durran commented Jan 5, 2023 • edited Loading

Description

What is changing?

Is there new documentation needed for these changes?

What is the motivation for this change?

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

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 Jan 5, 2023 •

edited

Loading