Skip to content

Commit 43300c3

Browse files
durrandurran
authored
fix(NODE-3515): do proper opTime merging in bulk results (#3012)
1 parent ee7f095 commit 43300c3

File tree

2 files changed

+169
-33
lines changed

2 files changed

+169
-33
lines changed

‎src/bulk/common.ts

+40-33
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,12 @@
11
import { PromiseProvider } from '../promise_provider';
2-
import { Long, ObjectId, Document, BSONSerializeOptions, resolveBSONOptions } from '../bson';
2+
import {
3+
Long,
4+
ObjectId,
5+
Document,
6+
BSONSerializeOptions,
7+
resolveBSONOptions,
8+
Timestamp
9+
} from '../bson';
310
import {
411
MongoWriteConcernError,
512
AnyError,
@@ -433,8 +440,13 @@ export class WriteError {
433440
}
434441
}
435442

443+
/** Converts the number to a Long or returns it. */
444+
function longOrConvert(value: number | Long | Timestamp): Long | Timestamp {
445+
return typeof value === 'number' ? Long.fromNumber(value) : value;
446+
}
447+
436448
/** Merges results into shared data structure */
437-
function mergeBatchResults(
449+
export function mergeBatchResults(
438450
batch: Batch,
439451
bulkResult: BulkResult,
440452
err?: AnyError,
@@ -469,42 +481,37 @@ function mergeBatchResults(
469481
return;
470482
}
471483

472-
// Deal with opTime if available
484+
// The server write command specification states that lastOp is an optional
485+
// mongod only field that has a type of timestamp. Across various scarce specs
486+
// where opTime is mentioned, it is an "opaque" object that can have a "ts" and
487+
// "t" field with Timestamp and Long as their types respectively.
488+
// The "lastOp" field of the bulk write result is never mentioned in the driver
489+
// specifications or the bulk write spec, so we should probably just keep its
490+
// value consistent since it seems to vary.
491+
// See: https://github.com./mongodb/specifications/blob/master/source/driver-bulk-update.rst#results-object
473492
if (result.opTime || result.lastOp) {
474-
const opTime = result.lastOp || result.opTime;
475-
let lastOpTS = null;
476-
let lastOpT = null;
477-
478-
// We have a time stamp
479-
if (opTime && opTime._bsontype === 'Timestamp') {
480-
if (bulkResult.opTime == null) {
481-
bulkResult.opTime = opTime;
482-
} else if (opTime.greaterThan(bulkResult.opTime)) {
483-
bulkResult.opTime = opTime;
484-
}
485-
} else {
486-
// Existing TS
487-
if (bulkResult.opTime) {
488-
lastOpTS =
489-
typeof bulkResult.opTime.ts === 'number'
490-
? Long.fromNumber(bulkResult.opTime.ts)
491-
: bulkResult.opTime.ts;
492-
lastOpT =
493-
typeof bulkResult.opTime.t === 'number'
494-
? Long.fromNumber(bulkResult.opTime.t)
495-
: bulkResult.opTime.t;
496-
}
493+
let opTime = result.lastOp || result.opTime;
497494

498-
// Current OpTime TS
499-
const opTimeTS = typeof opTime.ts === 'number' ? Long.fromNumber(opTime.ts) : opTime.ts;
500-
const opTimeT = typeof opTime.t === 'number' ? Long.fromNumber(opTime.t) : opTime.t;
495+
// If the opTime is a Timestamp, convert it to a consistent format to be
496+
// able to compare easily. Converting to the object from a timestamp is
497+
// much more straightforward than the other direction.
498+
if (opTime._bsontype === 'Timestamp') {
499+
opTime = { ts: opTime, t: Long.ZERO };
500+
}
501501

502-
// Compare the opTime's
503-
if (bulkResult.opTime == null) {
504-
bulkResult.opTime = opTime;
505-
} else if (opTimeTS.greaterThan(lastOpTS)) {
502+
// If there's no lastOp, just set it.
503+
if (!bulkResult.opTime) {
504+
bulkResult.opTime = opTime;
505+
} else {
506+
// First compare the ts values and set if the opTimeTS value is greater.
507+
const lastOpTS = longOrConvert(bulkResult.opTime.ts);
508+
const opTimeTS = longOrConvert(opTime.ts);
509+
if (opTimeTS.greaterThan(lastOpTS)) {
506510
bulkResult.opTime = opTime;
507511
} else if (opTimeTS.equals(lastOpTS)) {
512+
// If the ts values are equal, then compare using the t values.
513+
const lastOpT = longOrConvert(bulkResult.opTime.t);
514+
const opTimeT = longOrConvert(opTime.t);
508515
if (opTimeT.greaterThan(lastOpT)) {
509516
bulkResult.opTime = opTime;
510517
}

‎test/unit/bulk/common.test.js

+129
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,129 @@
1+
'use strict';
2+
3+
const { expect } = require('chai');
4+
const { mergeBatchResults } = require('../../../src/bulk/common');
5+
const { Timestamp, Long } = require('../../../src/bson');
6+
7+
describe('bulk/common', function () {
8+
describe('#mergeBatchResults', function () {
9+
let opTime;
10+
let lastOp;
11+
const bulkResult = {
12+
ok: 1,
13+
writeErrors: [],
14+
writeConcernErrors: [],
15+
insertedIds: [],
16+
nInserted: 0,
17+
nUpserted: 0,
18+
nMatched: 0,
19+
nModified: 0,
20+
nRemoved: 1,
21+
upserted: []
22+
};
23+
const result = {
24+
n: 8,
25+
nModified: 8,
26+
electionId: '7fffffff0000000000000028',
27+
ok: 1,
28+
$clusterTime: {
29+
clusterTime: '7020546605669417498',
30+
signature: {
31+
hash: 'AAAAAAAAAAAAAAAAAAAAAAAAAAA=',
32+
keyId: 0
33+
}
34+
},
35+
operationTime: '7020546605669417498'
36+
};
37+
const batch = [];
38+
39+
context('when lastOp is an object', function () {
40+
context('when the opTime is a Timestamp', function () {
41+
before(function () {
42+
lastOp = { ts: 7020546605669417496, t: 10 };
43+
opTime = Timestamp.fromNumber(8020546605669417496);
44+
bulkResult.opTime = lastOp;
45+
result.opTime = opTime;
46+
});
47+
48+
it('replaces the opTime with the properly formatted object', function () {
49+
mergeBatchResults(batch, bulkResult, null, result);
50+
expect(bulkResult.opTime).to.deep.equal({ ts: opTime, t: Long.ZERO });
51+
});
52+
});
53+
54+
context('when the opTime is an object', function () {
55+
context('when the ts is greater', function () {
56+
before(function () {
57+
lastOp = { ts: 7020546605669417496, t: 10 };
58+
opTime = { ts: 7020546605669417497, t: 10 };
59+
bulkResult.opTime = lastOp;
60+
result.opTime = opTime;
61+
});
62+
63+
it('replaces the opTime with the new opTime', function () {
64+
mergeBatchResults(batch, bulkResult, null, result);
65+
expect(bulkResult.opTime).to.deep.equal(opTime);
66+
});
67+
});
68+
69+
context('when the ts is equal', function () {
70+
context('when the t is greater', function () {
71+
before(function () {
72+
lastOp = { ts: 7020546605669417496, t: 10 };
73+
opTime = { ts: 7020546605669417496, t: 20 };
74+
bulkResult.opTime = lastOp;
75+
result.opTime = opTime;
76+
});
77+
78+
it('replaces the opTime with the new opTime', function () {
79+
mergeBatchResults(batch, bulkResult, null, result);
80+
expect(bulkResult.opTime).to.deep.equal(opTime);
81+
});
82+
});
83+
84+
context('when the t is equal', function () {
85+
before(function () {
86+
lastOp = { ts: 7020546605669417496, t: 10 };
87+
opTime = { ts: 7020546605669417496, t: 10 };
88+
bulkResult.opTime = lastOp;
89+
result.opTime = opTime;
90+
});
91+
92+
it('does not replace the opTime with the new opTime', function () {
93+
mergeBatchResults(batch, bulkResult, null, result);
94+
expect(bulkResult.opTime).to.deep.equal(lastOp);
95+
});
96+
});
97+
98+
context('when the t is less', function () {
99+
before(function () {
100+
lastOp = { ts: 7020546605669417496, t: 10 };
101+
opTime = { ts: 7020546605669417496, t: 5 };
102+
bulkResult.opTime = lastOp;
103+
result.opTime = opTime;
104+
});
105+
106+
it('does not replace the opTime with the new opTime', function () {
107+
mergeBatchResults(batch, bulkResult, null, result);
108+
expect(bulkResult.opTime).to.deep.equal(lastOp);
109+
});
110+
});
111+
});
112+
113+
context('when the ts is less', function () {
114+
before(function () {
115+
lastOp = { ts: 7020546605669417496, t: 10 };
116+
opTime = { ts: 7020546605669417495, t: 10 };
117+
bulkResult.opTime = lastOp;
118+
result.opTime = opTime;
119+
});
120+
121+
it('does not replace the opTime with the new opTime', function () {
122+
mergeBatchResults(batch, bulkResult, null, result);
123+
expect(bulkResult.opTime).to.deep.equal(lastOp);
124+
});
125+
});
126+
});
127+
});
128+
});
129+
});

0 commit comments

Comments
 (0)