Skip to content

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

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.

Sign up for GitHub

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

Merged
merged 9 commits into from
Jan 18, 2023
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
6625a45
feat(NODE-4034): make internal bulk result private
durran Jan 5, 2023
d592a0f
feat: add properties to bulk write result
durran Jan 5, 2023
8ea38e5
test: fix missing insertedIds in spec tests
durran Jan 5, 2023
80bdaa3
chore: update migration guide
durran Jan 11, 2023
cb834ea
chore: link drivers ticket
durran Jan 11, 2023
4971fee
fix: add suggestions
durran Jan 12, 2023
4b6bea5
test: fix test
durran Jan 12, 2023
1badb1b
test: fix tests
durran Jan 12, 2023
318ffcb
test: add insertedIds to csfle bulk results
durran Jan 5, 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
18 changes: 18 additions & 0 deletions etc/notes/CHANGES_5.0.0.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -191,3 +191,21 @@ await collection.insertMany([{ name: 'fido' }, { name: 'luna' }])

The `keepGoing` option was a legacy name for setting `ordered` to `false` for bulk inserts.
It was only supported by the legacy `collection.insert()` method which is now removed as noted above.

### `BulkWriteResult` no longer contains a publicly enumerable `result` property.

To access the raw result, please use `bulkWriteResult.getRawResponse()`.

### `BulkWriteResult` now contains individual ressult properties.

These can be accessed via:

```ts
bulkWriteResult.insertedCount;
bulkWriteResult.matchedCount;
bulkWriteResult.modifiedCount;
bulkWriteResult.deletedCount;
bulkWriteResult.upsertedCount;
bulkWriteResult.upsertedIds;
bulkWriteResult.insertedIds;
```
88 changes: 32 additions & 56 deletions src/bulk/common.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -123,11 +123,7 @@ export type AnyBulkWriteOperation<TSchema extends Document = Document> =
| { deleteOne: DeleteOneModel<TSchema> }
| { deleteMany: DeleteManyModel<TSchema> };

/**
* @public
*
* @deprecated Will be made internal in 5.0
*/
/** @internal */
export interface BulkResult {
ok: number;
writeErrors: WriteError[];
Expand Down Expand Up @@ -172,54 +168,44 @@ export class Batch<T = Document> {
* The result of a bulk write.
*/
export class BulkWriteResult {
/** @deprecated Will be removed in 5.0 */
result: BulkResult;

/**
* Create a new BulkWriteResult instance
* @internal
*/
constructor(bulkResult: BulkResult) {
this.result = bulkResult;
}

private readonly result: BulkResult;
/** Number of documents inserted. */
get insertedCount(): number {
return this.result.nInserted ?? 0;
}
readonly insertedCount: number;
/** Number of documents matched for update. */
get matchedCount(): number {
return this.result.nMatched ?? 0;
}
readonly matchedCount: number;
/** Number of documents modified. */
get modifiedCount(): number {
return this.result.nModified ?? 0;
}
readonly modifiedCount: number;
/** Number of documents deleted. */
get deletedCount(): number {
return this.result.nRemoved ?? 0;
}
readonly deletedCount: number;
/** Number of documents upserted. */
get upsertedCount(): number {
return this.result.upserted.length ?? 0;
}

readonly upsertedCount: number;
/** Upserted document generated Id's, hash key is the index of the originating operation */
get upsertedIds(): { [key: number]: any } {
const upserted: { [index: number]: any } = {};
for (const doc of this.result.upserted ?? []) {
upserted[doc.index] = doc._id;
readonly upsertedIds: { [key: number]: any };
/** Inserted document generated Id's, hash key is the index of the originating operation */
readonly insertedIds: { [key: number]: any };

private static generateIdMap(ids: Document[]): { [key: number]: any } {
const idMap: { [index: number]: any } = {};
for (const doc of ids) {
idMap[doc.index] = doc._id;
}
return upserted;
return idMap;
}

/** Inserted document generated Id's, hash key is the index of the originating operation */
get insertedIds(): { [key: number]: any } {
const inserted: { [index: number]: any } = {};
for (const doc of this.result.insertedIds ?? []) {
inserted[doc.index] = doc._id;
}
return inserted;
/**
* Create a new BulkWriteResult instance
* @internal
*/
constructor(bulkResult: BulkResult) {
this.result = bulkResult;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

this.insertedCount = this.result.nInserted ?? 0;
this.matchedCount = this.result.nMatched ?? 0;
this.modifiedCount = this.result.nModified ?? 0;
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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Description updated with NODE-4973 now.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Object.defineProperty(this, 'result', { value: this.result, enumerable: false });
}

/** Evaluates to true if the bulk operation correctly executes */
Expand Down Expand Up @@ -314,13 +300,8 @@ export class BulkWriteResult {
}
}

/* @deprecated Will be removed in 5.0 release */
toJSON(): BulkResult {
return this.result;
}

toString(): string {
return `BulkWriteResult(${this.toJSON()})`;
return `BulkWriteResult(${this.result})`;
}

isOk(): boolean {
Expand Down Expand Up @@ -550,12 +531,8 @@ function executeCommands(
}

// Merge the results together
mergeBatchResults(batch, bulkOperation.s.bulkResult, err, result);
const writeResult = new BulkWriteResult(bulkOperation.s.bulkResult);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

const mergeResult = mergeBatchResults(batch, bulkOperation.s.bulkResult, err, result);
if (mergeResult != null) {
return callback(undefined, writeResult);
}

if (bulkOperation.handleWriteError(callback, writeResult)) return;

// Execute the next command in line
Expand Down Expand Up @@ -1246,7 +1223,6 @@ export abstract class BulkOperationBase {
this.s.executed = true;
const finalOptions = { ...this.s.options, ...options };
const operation = new BulkWriteShimOperation(this, finalOptions);

return executeOperation(this.s.collection.s.db.s.client, operation);
}, callback);
}
Expand Down
8 changes: 6 additions & 2 deletions test/integration/retryable-writes/retryable_writes.spec.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -150,8 +150,12 @@ async function executeScenarioTest(test, ctx: RetryableWriteTestContext) {
const result = resultOrError;
const expected = test.outcome.result;

// TODO(NODE-4034): Make CRUD results spec compliant
expect(result.value ?? result).to.deep.include(expected);
const actual = result.value ?? result;
// Some of our result classes contain the optional 'acknowledged' property which is
// not part of the test expectations.
for (const property in expected) {
expect(actual).to.have.deep.property(property, expected[property]);
}
}

if (test.outcome.collection) {
Expand Down
30 changes: 24 additions & 6 deletions test/spec/client-side-encryption/tests/unified/rewrapManyDataKey.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -321,7 +321,10 @@
"modifiedCount": 4,
"deletedCount": 0,
"upsertedCount": 0,
"upsertedIds": {}
"upsertedIds": {},
"insertedIds": {
"$$unsetOrMatches": {}
}
}
}
}
Expand Down Expand Up @@ -503,7 +506,10 @@
"modifiedCount": 4,
"deletedCount": 0,
"upsertedCount": 0,
"upsertedIds": {}
"upsertedIds": {},
"insertedIds": {
"$$unsetOrMatches": {}
}
}
}
}
Expand Down Expand Up @@ -687,7 +693,10 @@
"modifiedCount": 4,
"deletedCount": 0,
"upsertedCount": 0,
"upsertedIds": {}
"upsertedIds": {},
"insertedIds": {
"$$unsetOrMatches": {}
}
}
}
}
Expand Down Expand Up @@ -873,7 +882,10 @@
"modifiedCount": 4,
"deletedCount": 0,
"upsertedCount": 0,
"upsertedIds": {}
"upsertedIds": {},
"insertedIds": {
"$$unsetOrMatches": {}
}
}
}
}
Expand Down Expand Up @@ -1055,7 +1067,10 @@
"modifiedCount": 4,
"deletedCount": 0,
"upsertedCount": 0,
"upsertedIds": {}
"upsertedIds": {},
"insertedIds": {
"$$unsetOrMatches": {}
}
}
}
}
Expand Down Expand Up @@ -1218,7 +1233,10 @@
"modifiedCount": 5,
"deletedCount": 0,
"upsertedCount": 0,
"upsertedIds": {}
"upsertedIds": {},
"insertedIds": {
"$$unsetOrMatches": {}
}
}
}
},
Expand Down
6 changes: 6 additions & 0 deletions test/spec/client-side-encryption/tests/unified/rewrapManyDataKey.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -130,6 +130,7 @@ tests:
deletedCount: 0
upsertedCount: 0
upsertedIds: {}
insertedIds: { $$unsetOrMatches: {} }
expectEvents:
- client: *client0
events:
Expand Down Expand Up @@ -182,6 +183,7 @@ tests:
deletedCount: 0
upsertedCount: 0
upsertedIds: {}
insertedIds: { $$unsetOrMatches: {} }
expectEvents:
- client: *client0
events:
Expand Down Expand Up @@ -236,6 +238,7 @@ tests:
deletedCount: 0
upsertedCount: 0
upsertedIds: {}
insertedIds: { $$unsetOrMatches: {} }
expectEvents:
- client: *client0
events:
Expand Down Expand Up @@ -285,6 +288,7 @@ tests:
deletedCount: 0
upsertedCount: 0
upsertedIds: {}
insertedIds: { $$unsetOrMatches: {} }
expectEvents:
- client: *client0
events:
Expand Down Expand Up @@ -334,6 +338,7 @@ tests:
deletedCount: 0
upsertedCount: 0
upsertedIds: {}
insertedIds: { $$unsetOrMatches: {} }
expectEvents:
- client: *client0
events:
Expand Down Expand Up @@ -381,6 +386,7 @@ tests:
deletedCount: 0
upsertedCount: 0
upsertedIds: {}
insertedIds: { $$unsetOrMatches: {} }
- name: find
object: *collection0
arguments:
Expand Down
17 changes: 1 addition & 16 deletions test/tools/unified-spec-runner/operations.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -659,22 +659,7 @@ operations.set('rewrapManyDataKey', async ({ entities, operation }) => {
const clientEncryption = entities.getEntity('clientEncryption', operation.object);
const { filter, opts } = operation.arguments!;

const rewrapManyDataKeyResult = await clientEncryption.rewrapManyDataKey(filter, opts);

if (rewrapManyDataKeyResult.bulkWriteResult != null) {
// TODO(NODE-4393): refactor BulkWriteResult to not have a 'result' property
//
// The unified spec runner match function will assert that documents have no extra
// keys. For `rewrapManyDataKey` operations, our unifed tests will fail because
// our BulkWriteResult class has an extra property - "result". We explicitly make it
// non-enumerable for the purposes of testing so that the tests can pass.
const { bulkWriteResult } = rewrapManyDataKeyResult;
Object.defineProperty(bulkWriteResult, 'result', {
value: bulkWriteResult.result,
enumerable: false
});
}
return rewrapManyDataKeyResult;
return await clientEncryption.rewrapManyDataKey(filter, opts);
});

operations.set('deleteKey', async ({ entities, operation }) => {
Expand Down