From c6dba707428790d21053022cc2f1eddc1e2c0ef2 Mon Sep 17 00:00:00 2001 From: Durran Jordan Date: Wed, 20 Oct 2021 00:52:28 +0200 Subject: [PATCH 1/5] fix(NODE-3515): do proper opTime merging in bulk ressults --- src/bulk/common.ts | 70 ++++++++++++++++++----------------- test/unit/bulk/common.test.js | 51 +++++++++++++++++++++++++ 2 files changed, 87 insertions(+), 34 deletions(-) create mode 100644 test/unit/bulk/common.test.js diff --git a/src/bulk/common.ts b/src/bulk/common.ts index 4afb066622..d7d701c036 100644 --- a/src/bulk/common.ts +++ b/src/bulk/common.ts @@ -1,5 +1,12 @@ import { PromiseProvider } from '../promise_provider'; -import { Long, ObjectId, Document, BSONSerializeOptions, resolveBSONOptions } from '../bson'; +import { + Long, + ObjectId, + Document, + BSONSerializeOptions, + resolveBSONOptions, + Timestamp +} from '../bson'; import { MongoWriteConcernError, AnyError, @@ -433,8 +440,13 @@ export class WriteError { } } +/** Converts the number to a Long or returns it. */ +function longOrConvert(value: number | Long | Timestamp): Long | Timestamp { + return typeof value === 'number' ? Long.fromNumber(value) : value; +} + /** Merges results into shared data structure */ -function mergeBatchResults( +export function mergeBatchResults( batch: Batch, bulkResult: BulkResult, err?: AnyError, @@ -471,42 +483,32 @@ function mergeBatchResults( // Deal with opTime if available if (result.opTime || result.lastOp) { - const opTime = result.lastOp || result.opTime; - let lastOpTS = null; - let lastOpT = null; - - // We have a time stamp - if (opTime && opTime._bsontype === 'Timestamp') { - if (bulkResult.opTime == null) { - bulkResult.opTime = opTime; - } else if (opTime.greaterThan(bulkResult.opTime)) { - bulkResult.opTime = opTime; - } - } else { - // Existing TS - if (bulkResult.opTime) { - lastOpTS = - typeof bulkResult.opTime.ts === 'number' - ? Long.fromNumber(bulkResult.opTime.ts) - : bulkResult.opTime.ts; - lastOpT = - typeof bulkResult.opTime.t === 'number' - ? Long.fromNumber(bulkResult.opTime.t) - : bulkResult.opTime.t; + let opTime = result.lastOp || result.opTime; + + if (opTime) { + // If the opTime is a Timestamp, convert it to a consistent format to be + // able to compare easily. Converting to the object from a timestamp is + // much more straightforward than the other direction. + if (opTime._bsontype === 'Timestamp') { + opTime = { ts: opTime, t: Long.ZERO }; } - // Current OpTime TS - const opTimeTS = typeof opTime.ts === 'number' ? Long.fromNumber(opTime.ts) : opTime.ts; - const opTimeT = typeof opTime.t === 'number' ? Long.fromNumber(opTime.t) : opTime.t; - - // Compare the opTime's - if (bulkResult.opTime == null) { - bulkResult.opTime = opTime; - } else if (opTimeTS.greaterThan(lastOpTS)) { + // If there's no lastOp, just set it. + if (!bulkResult.opTime) { bulkResult.opTime = opTime; - } else if (opTimeTS.equals(lastOpTS)) { - if (opTimeT.greaterThan(lastOpT)) { + } else { + // First compare the ts values and set if the opTimeTS value is greater. + const lastOpTS = longOrConvert(bulkResult.opTime.ts); + const opTimeTS = longOrConvert(opTime.ts); + if (opTimeTS.greaterThan(lastOpTS)) { bulkResult.opTime = opTime; + } else if (opTimeTS.equals(lastOpTS)) { + // If the ts values are equal, then compare using the t values. + const lastOpT = longOrConvert(bulkResult.opTime.t); + const opTimeT = longOrConvert(opTime.t); + if (opTimeT.greaterThan(lastOpT)) { + bulkResult.opTime = opTime; + } } } } diff --git a/test/unit/bulk/common.test.js b/test/unit/bulk/common.test.js new file mode 100644 index 0000000000..ce709a0f23 --- /dev/null +++ b/test/unit/bulk/common.test.js @@ -0,0 +1,51 @@ +'use strict'; + +const { expect } = require('chai'); +const { mergeBatchResults } = require('../../../src/bulk/common'); +const { Timestamp, Long } = require('../../../src/bson'); + +describe('bulk/common', function () { + describe('#mergeBatchResults', function () { + context('when opTime is an object', function () { + context('when the opTime on the result is a Timestamp', function () { + const batch = []; + const bulkResult = { + ok: 1, + writeErrors: [], + writeConcernErrors: [], + insertedIds: [], + nInserted: 0, + nUpserted: 0, + nMatched: 0, + nModified: 0, + nRemoved: 1, + upserted: [], + opTime: { + ts: 7020546605669417496, + t: 10 + } + }; + const result = { + n: 8, + nModified: 8, + opTime: Timestamp.fromNumber(8020546605669417496), + electionId: '7fffffff0000000000000028', + ok: 1, + $clusterTime: { + clusterTime: '7020546605669417498', + signature: { + hash: 'AAAAAAAAAAAAAAAAAAAAAAAAAAA=', + keyId: 0 + } + }, + operationTime: '7020546605669417498' + }; + + it('replaces the opTime with the properly formatted timestamp', function () { + mergeBatchResults(batch, bulkResult, null, result); + expect(bulkResult.opTime.t).to.equal(Long.ZERO); + }); + }); + }); + }); +}); From 8d58d915417476b3679ff208d101e9d01ce5df65 Mon Sep 17 00:00:00 2001 From: Durran Jordan Date: Wed, 20 Oct 2021 19:27:22 +0200 Subject: [PATCH 2/5] fix: updating tests, fix typos --- src/bulk/common.ts | 49 ++++++----- test/unit/bulk/common.test.js | 152 +++++++++++++++++++++++++--------- 2 files changed, 142 insertions(+), 59 deletions(-) diff --git a/src/bulk/common.ts b/src/bulk/common.ts index d7d701c036..077fd698f3 100644 --- a/src/bulk/common.ts +++ b/src/bulk/common.ts @@ -481,34 +481,39 @@ export function mergeBatchResults( return; } - // Deal with opTime if available + // The server write command specification states that lastOp is an optional + // mongod only field that has a type of timestamp. Across various scarce specs + // where opTime is mentioned, it is an "opaque" object that can have a "ts" and + // "t" field with Timestamp and Long as their types respectively. + // The "lastOp" field of the bulk write result is never mentioned in the driver + // specifications or the bulk write spec, so we should probably just keep its + // value consistent since it seems to vary. + // See: https://github.com/mongodb/specifications/blob/master/source/driver-bulk-update.rst#results-object if (result.opTime || result.lastOp) { let opTime = result.lastOp || result.opTime; - if (opTime) { - // If the opTime is a Timestamp, convert it to a consistent format to be - // able to compare easily. Converting to the object from a timestamp is - // much more straightforward than the other direction. - if (opTime._bsontype === 'Timestamp') { - opTime = { ts: opTime, t: Long.ZERO }; - } + // If the opTime is a Timestamp, convert it to a consistent format to be + // able to compare easily. Converting to the object from a timestamp is + // much more straightforward than the other direction. + if (opTime._bsontype === 'Timestamp') { + opTime = { ts: opTime, t: Long.ZERO }; + } - // If there's no lastOp, just set it. - if (!bulkResult.opTime) { + // If there's no lastOp, just set it. + if (!bulkResult.opTime) { + bulkResult.opTime = opTime; + } else { + // First compare the ts values and set if the opTimeTS value is greater. + const lastOpTS = longOrConvert(bulkResult.opTime.ts); + const opTimeTS = longOrConvert(opTime.ts); + if (opTimeTS.greaterThan(lastOpTS)) { bulkResult.opTime = opTime; - } else { - // First compare the ts values and set if the opTimeTS value is greater. - const lastOpTS = longOrConvert(bulkResult.opTime.ts); - const opTimeTS = longOrConvert(opTime.ts); - if (opTimeTS.greaterThan(lastOpTS)) { + } else if (opTimeTS.equals(lastOpTS)) { + // If the ts values are equal, then compare using the t values. + const lastOpT = longOrConvert(bulkResult.opTime.t); + const opTimeT = longOrConvert(opTime.t); + if (opTimeT.greaterThan(lastOpT)) { bulkResult.opTime = opTime; - } else if (opTimeTS.equals(lastOpTS)) { - // If the ts values are equal, then compare using the t values. - const lastOpT = longOrConvert(bulkResult.opTime.t); - const opTimeT = longOrConvert(opTime.t); - if (opTimeT.greaterThan(lastOpT)) { - bulkResult.opTime = opTime; - } } } } diff --git a/test/unit/bulk/common.test.js b/test/unit/bulk/common.test.js index ce709a0f23..8f61f50be0 100644 --- a/test/unit/bulk/common.test.js +++ b/test/unit/bulk/common.test.js @@ -6,44 +6,122 @@ const { Timestamp, Long } = require('../../../src/bson'); describe('bulk/common', function () { describe('#mergeBatchResults', function () { - context('when opTime is an object', function () { - context('when the opTime on the result is a Timestamp', function () { - const batch = []; - const bulkResult = { - ok: 1, - writeErrors: [], - writeConcernErrors: [], - insertedIds: [], - nInserted: 0, - nUpserted: 0, - nMatched: 0, - nModified: 0, - nRemoved: 1, - upserted: [], - opTime: { - ts: 7020546605669417496, - t: 10 - } - }; - const result = { - n: 8, - nModified: 8, - opTime: Timestamp.fromNumber(8020546605669417496), - electionId: '7fffffff0000000000000028', - ok: 1, - $clusterTime: { - clusterTime: '7020546605669417498', - signature: { - hash: 'AAAAAAAAAAAAAAAAAAAAAAAAAAA=', - keyId: 0 - } - }, - operationTime: '7020546605669417498' - }; - - it('replaces the opTime with the properly formatted timestamp', function () { + let opTime; + let lastOp; + const bulkResult = { + ok: 1, + writeErrors: [], + writeConcernErrors: [], + insertedIds: [], + nInserted: 0, + nUpserted: 0, + nMatched: 0, + nModified: 0, + nRemoved: 1, + upserted: [] + }; + const result = { + n: 8, + nModified: 8, + electionId: '7fffffff0000000000000028', + ok: 1, + $clusterTime: { + clusterTime: '7020546605669417498', + signature: { + hash: 'AAAAAAAAAAAAAAAAAAAAAAAAAAA=', + keyId: 0 + } + }, + operationTime: '7020546605669417498' + }; + const batch = []; + + context('when lastOp is an object', function () { + context('when the opTime is a Timestamp', function () { + before(function () { + lastOp = { ts: 7020546605669417496, t: 10 }; + opTime = Timestamp.fromNumber(8020546605669417496); + bulkResult.lastOp = lastOp; + result.opTime = opTime; + }); + + it('replaces the lastOp with the properly formatted object', function () { mergeBatchResults(batch, bulkResult, null, result); - expect(bulkResult.opTime.t).to.equal(Long.ZERO); + expect(bulkResult.lastOp).to.deep.equal({ ts: opTime, t: Long.ZERO }); + }); + }); + + context('when the opTime is an object', function () { + context('when the ts is greater', function () { + before(function () { + lastOp = { ts: 7020546605669417496, t: 10 }; + opTime = { ts: 7020546605669417497, t: 10 }; + bulkResult.lastOp = lastOp; + result.opTime = opTime; + }); + + it('replaces the lastOp with the new opTime', function () { + mergeBatchResults(batch, bulkResult, null, result); + expect(bulkResult.lastOp).to.deep.equal(opTime); + }); + }); + + context('when the ts is equal', function () { + context('when the t is greater', function () { + before(function () { + lastOp = { ts: 7020546605669417496, t: 10 }; + opTime = { ts: 7020546605669417496, t: 20 }; + bulkResult.lastOp = lastOp; + result.opTime = opTime; + }); + + it('replaces the lastOp with the new opTime', function () { + mergeBatchResults(batch, bulkResult, null, result); + expect(bulkResult.lastOp).to.deep.equal(opTime); + }); + }); + + context('when the t is equal', function () { + before(function () { + lastOp = { ts: 7020546605669417496, t: 10 }; + opTime = { ts: 7020546605669417496, t: 10 }; + bulkResult.lastOp = lastOp; + result.opTime = opTime; + }); + + it('does not replace the lastOp with the new opTime', function () { + mergeBatchResults(batch, bulkResult, null, result); + expect(bulkResult.lastOp).to.deep.equal(lastOp); + }); + }); + + context('when the t is less', function () { + before(function () { + lastOp = { ts: 7020546605669417496, t: 10 }; + opTime = { ts: 7020546605669417496, t: 5 }; + bulkResult.lastOp = lastOp; + result.opTime = opTime; + }); + + it('does not replace the lastOp with the new opTime', function () { + mergeBatchResults(batch, bulkResult, null, result); + expect(bulkResult.lastOp).to.deep.equal(lastOp); + }); + }); + }); + + context('when the ts is less', function () { + before(function () { + lastOp = { ts: 7020546605669417496, t: 10 }; + opTime = { ts: 7020546605669417495, t: 10 }; + bulkResult.lastOp = lastOp; + result.opTime = opTime; + }); + + it('does not replace the lastOp with the new opTime', function () { + mergeBatchResults(batch, bulkResult, null, result); + expect(bulkResult.lastOp).to.deep.equal(lastOp); + }); }); }); }); From c8da608393d9e4a57f45b6244ec7a7ced55b1655 Mon Sep 17 00:00:00 2001 From: Durran Jordan Date: Wed, 20 Oct 2021 20:40:29 +0200 Subject: [PATCH 3/5] fix: fix test field names --- test/unit/bulk/common.test.js | 28 ++++++++++++++-------------- 1 file changed, 14 insertions(+), 14 deletions(-) diff --git a/test/unit/bulk/common.test.js b/test/unit/bulk/common.test.js index 8f61f50be0..5cf6cb649a 100644 --- a/test/unit/bulk/common.test.js +++ b/test/unit/bulk/common.test.js @@ -5,7 +5,7 @@ const { mergeBatchResults } = require('../../../src/bulk/common'); const { Timestamp, Long } = require('../../../src/bson'); describe('bulk/common', function () { - describe('#mergeBatchResults', function () { + describe.only('#mergeBatchResults', function () { let opTime; let lastOp; const bulkResult = { @@ -41,13 +41,13 @@ describe('bulk/common', function () { before(function () { lastOp = { ts: 7020546605669417496, t: 10 }; opTime = Timestamp.fromNumber(8020546605669417496); - bulkResult.lastOp = lastOp; + bulkResult.opTime = lastOp; result.opTime = opTime; }); - it('replaces the lastOp with the properly formatted object', function () { + it('replaces the opTime with the properly formatted object', function () { mergeBatchResults(batch, bulkResult, null, result); - expect(bulkResult.lastOp).to.deep.equal({ ts: opTime, t: Long.ZERO }); + expect(bulkResult.opTime).to.deep.equal({ ts: opTime, t: Long.ZERO }); }); }); @@ -56,13 +56,13 @@ describe('bulk/common', function () { before(function () { lastOp = { ts: 7020546605669417496, t: 10 }; opTime = { ts: 7020546605669417497, t: 10 }; - bulkResult.lastOp = lastOp; + bulkResult.opTime = lastOp; result.opTime = opTime; }); it('replaces the lastOp with the new opTime', function () { mergeBatchResults(batch, bulkResult, null, result); - expect(bulkResult.lastOp).to.deep.equal(opTime); + expect(bulkResult.opTime).to.deep.equal(opTime); }); }); @@ -71,13 +71,13 @@ describe('bulk/common', function () { before(function () { lastOp = { ts: 7020546605669417496, t: 10 }; opTime = { ts: 7020546605669417496, t: 20 }; - bulkResult.lastOp = lastOp; + bulkResult.opTime = lastOp; result.opTime = opTime; }); it('replaces the lastOp with the new opTime', function () { mergeBatchResults(batch, bulkResult, null, result); - expect(bulkResult.lastOp).to.deep.equal(opTime); + expect(bulkResult.opTime).to.deep.equal(opTime); }); }); @@ -85,13 +85,13 @@ describe('bulk/common', function () { before(function () { lastOp = { ts: 7020546605669417496, t: 10 }; opTime = { ts: 7020546605669417496, t: 10 }; - bulkResult.lastOp = lastOp; + bulkResult.opTime = lastOp; result.opTime = opTime; }); it('does not replace the lastOp with the new opTime', function () { mergeBatchResults(batch, bulkResult, null, result); - expect(bulkResult.lastOp).to.deep.equal(lastOp); + expect(bulkResult.opTime).to.deep.equal(lastOp); }); }); @@ -99,13 +99,13 @@ describe('bulk/common', function () { before(function () { lastOp = { ts: 7020546605669417496, t: 10 }; opTime = { ts: 7020546605669417496, t: 5 }; - bulkResult.lastOp = lastOp; + bulkResult.opTime = lastOp; result.opTime = opTime; }); it('does not replace the lastOp with the new opTime', function () { mergeBatchResults(batch, bulkResult, null, result); - expect(bulkResult.lastOp).to.deep.equal(lastOp); + expect(bulkResult.opTime).to.deep.equal(lastOp); }); }); }); @@ -114,13 +114,13 @@ describe('bulk/common', function () { before(function () { lastOp = { ts: 7020546605669417496, t: 10 }; opTime = { ts: 7020546605669417495, t: 10 }; - bulkResult.lastOp = lastOp; + bulkResult.opTime = lastOp; result.opTime = opTime; }); it('does not replace the lastOp with the new opTime', function () { mergeBatchResults(batch, bulkResult, null, result); - expect(bulkResult.lastOp).to.deep.equal(lastOp); + expect(bulkResult.opTime).to.deep.equal(lastOp); }); }); }); From 51d26af968d9f57c3a74eb81297e785d64718bc4 Mon Sep 17 00:00:00 2001 From: Durran Jordan Date: Wed, 20 Oct 2021 21:08:18 +0200 Subject: [PATCH 4/5] fix: fix lastOp references in test descriptions --- test/unit/bulk/common.test.js | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/test/unit/bulk/common.test.js b/test/unit/bulk/common.test.js index 5cf6cb649a..b5ab465769 100644 --- a/test/unit/bulk/common.test.js +++ b/test/unit/bulk/common.test.js @@ -60,7 +60,7 @@ describe('bulk/common', function () { result.opTime = opTime; }); - it('replaces the lastOp with the new opTime', function () { + it('replaces the opTime with the new opTime', function () { mergeBatchResults(batch, bulkResult, null, result); expect(bulkResult.opTime).to.deep.equal(opTime); }); @@ -75,7 +75,7 @@ describe('bulk/common', function () { result.opTime = opTime; }); - it('replaces the lastOp with the new opTime', function () { + it('replaces the opTime with the new opTime', function () { mergeBatchResults(batch, bulkResult, null, result); expect(bulkResult.opTime).to.deep.equal(opTime); }); @@ -89,7 +89,7 @@ describe('bulk/common', function () { result.opTime = opTime; }); - it('does not replace the lastOp with the new opTime', function () { + it('does not replace the opTime with the new opTime', function () { mergeBatchResults(batch, bulkResult, null, result); expect(bulkResult.opTime).to.deep.equal(lastOp); }); @@ -103,7 +103,7 @@ describe('bulk/common', function () { result.opTime = opTime; }); - it('does not replace the lastOp with the new opTime', function () { + it('does not replace the opTime with the new opTime', function () { mergeBatchResults(batch, bulkResult, null, result); expect(bulkResult.opTime).to.deep.equal(lastOp); }); @@ -118,7 +118,7 @@ describe('bulk/common', function () { result.opTime = opTime; }); - it('does not replace the lastOp with the new opTime', function () { + it('does not replace the opTime with the new opTime', function () { mergeBatchResults(batch, bulkResult, null, result); expect(bulkResult.opTime).to.deep.equal(lastOp); }); From 8c2ee0a8896a1ebf509d97a427216f4eecdd8486 Mon Sep 17 00:00:00 2001 From: Durran Jordan Date: Wed, 20 Oct 2021 21:51:42 +0200 Subject: [PATCH 5/5] fix: remove only on common bulk tests --- test/unit/bulk/common.test.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/unit/bulk/common.test.js b/test/unit/bulk/common.test.js index b5ab465769..5e50a5a202 100644 --- a/test/unit/bulk/common.test.js +++ b/test/unit/bulk/common.test.js @@ -5,7 +5,7 @@ const { mergeBatchResults } = require('../../../src/bulk/common'); const { Timestamp, Long } = require('../../../src/bson'); describe('bulk/common', function () { - describe.only('#mergeBatchResults', function () { + describe('#mergeBatchResults', function () { let opTime; let lastOp; const bulkResult = {