Skip to content

Commit

Permalink
fix: error label checking & insertOne where retryWrites is false
Browse files Browse the repository at this point in the history
  • Loading branch information
Thomas Reggi authored Sep 15, 2020
1 parent 7bbc783 commit d4502aa
Show file tree
Hide file tree
Showing 6 changed files with 26 additions and 6 deletions.
2 changes: 1 addition & 1 deletion src/bulk/common.ts
Original file line number Diff line number Diff line change
Expand Up @@ -550,7 +550,7 @@ function executeCommands(
function resultHandler(err?: AnyError, result?: Document) {
// Error is a driver related error not a bulk op error, return early
if (err && 'message' in err && !(err instanceof MongoWriteConcernError)) {
return callback(err);
return callback(new BulkWriteError(err, new BulkWriteResult(bulkOperation.s.bulkResult)));
}

if (err instanceof MongoWriteConcernError) {
Expand Down
3 changes: 1 addition & 2 deletions src/error.ts
Original file line number Diff line number Diff line change
Expand Up @@ -275,9 +275,8 @@ const RETRYABLE_WRITE_ERROR_CODES = new Set([

export function isRetryableWriteError(error: MongoError): boolean {
if (error instanceof MongoWriteConcernError) {
return RETRYABLE_WRITE_ERROR_CODES.has(error.result?.code);
return RETRYABLE_WRITE_ERROR_CODES.has(error.result?.code ?? error.code ?? 0);
}

return RETRYABLE_WRITE_ERROR_CODES.has(error.code ?? 0);
}

Expand Down
13 changes: 12 additions & 1 deletion src/sdam/server.ts
Original file line number Diff line number Diff line change
Expand Up @@ -558,6 +558,12 @@ function inActiveTransaction(session: ClientSession | undefined, cmd: Document)
return session && session.inTransaction() && !isTransactionCommand(cmd);
}

/** this checks the retryWrites option passed down from the client options, it
* does not check if the server supports retryable writes */
function isRetryableWritesEnabled(topology: Topology) {
return topology.s.options.retryWrites !== false;
}

function makeOperationHandler(
server: Server,
connection: Connection,
Expand All @@ -573,7 +579,11 @@ function makeOperationHandler(
session.serverSession.isDirty = true;
}

if (supportsRetryableWrites(server) && !inActiveTransaction(session, cmd)) {
if (
(isRetryableWritesEnabled(server.s.topology) || isTransactionCommand(cmd)) &&
supportsRetryableWrites(server) &&
!inActiveTransaction(session, cmd)
) {
err.addErrorLabel('RetryableWriteError');
}

Expand All @@ -584,6 +594,7 @@ function makeOperationHandler(
} else {
// if pre-4.4 server, then add error label if its a retryable write error
if (
(isRetryableWritesEnabled(server.s.topology) || isTransactionCommand(cmd)) &&
maxWireVersion(server) < 9 &&
isRetryableWriteError(err) &&
!inActiveTransaction(session, cmd)
Expand Down
8 changes: 7 additions & 1 deletion test/functional/retryable_writes.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@ const parseRunOn = require('../functional/spec-runner').parseRunOn;

describe('Retryable Writes', function () {
let ctx = {};

loadSpecTests('retryable-writes').forEach(suite => {
const environmentRequirementList = parseRunOn(suite.runOn);
environmentRequirementList.forEach(requires => {
Expand Down Expand Up @@ -86,6 +85,10 @@ function executeScenarioTest(test, ctx) {
const args = generateArguments(test);

let result = ctx.collection[test.operation.name].apply(ctx.collection, args);
const outcome = test.outcome && test.outcome.result;
const errorLabelsContain = outcome && outcome.errorLabelsContain;
const errorLabelsOmit = outcome && outcome.errorLabelsOmit;
const hasResult = outcome && !errorLabelsContain && !errorLabelsOmit;
if (test.outcome.error) {
result = result
.then(() => expect(false).to.be.true)
Expand All @@ -94,6 +97,9 @@ function executeScenarioTest(test, ctx) {
expect(err.message, 'expected operations to fail, but they succeeded').to.not.match(
/expected false to be true/
);
if (hasResult) expect(err.result).to.matchMongoSpec(test.outcome.result);
if (errorLabelsContain) expect(err.errorLabels).to.have.members(errorLabelsContain);
if (errorLabelsOmit) expect(err.errorLabels).to.not.have.members(errorLabelsOmit);
});
} else if (test.outcome.result) {
const expected = transformToFixUpsertedId(test.outcome.result);
Expand Down
5 changes: 4 additions & 1 deletion test/spec/retryable-writes/insertOne-serverErrors.json
Original file line number Diff line number Diff line change
Expand Up @@ -966,7 +966,10 @@
],
"writeConcernError": {
"code": 91,
"errmsg": "Replication is being shut down"
"errmsg": "Replication is being shut down",
"errorLabels": [
"RetryableWriteError"
]
}
}
},
Expand Down
1 change: 1 addition & 0 deletions test/spec/retryable-writes/insertOne-serverErrors.yml
Original file line number Diff line number Diff line change
Expand Up @@ -442,6 +442,7 @@ tests:
writeConcernError:
code: 91
errmsg: Replication is being shut down
errorLabels: ["RetryableWriteError"]
operation:
name: "insertOne"
arguments:
Expand Down

0 comments on commit d4502aa

Please sign in to comment.