fix: error label checking & insertOne where retryWrites is false

NODE-2742
mongodb · Sep 15, 2020 · d4502aa · d4502aa
1 parent 7bbc783
commit d4502aa
Show file tree

Hide file tree

Showing 6 changed files with 26 additions and 6 deletions.
diff --git a/src/bulk/common.ts b/src/bulk/common.ts
@@ -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) {

diff --git a/src/error.ts b/src/error.ts
@@ -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);
 }
 

diff --git a/src/sdam/server.ts b/src/sdam/server.ts
@@ -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,
@@ -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');
         }
 
@@ -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)

diff --git a/test/functional/retryable_writes.test.js b/test/functional/retryable_writes.test.js
@@ -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 => {
@@ -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)
@@ -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);

diff --git a/test/spec/retryable-writes/insertOne-serverErrors.json b/test/spec/retryable-writes/insertOne-serverErrors.json
@@ -966,7 +966,10 @@
           ],
           "writeConcernError": {
             "code": 91,
-            "errmsg": "Replication is being shut down"
+            "errmsg": "Replication is being shut down",
+            "errorLabels": [
+              "RetryableWriteError"
+            ]
           }
         }
       },

diff --git a/test/spec/retryable-writes/insertOne-serverErrors.yml b/test/spec/retryable-writes/insertOne-serverErrors.yml
@@ -442,6 +442,7 @@ tests:
                 writeConcernError:
                     code: 91
                     errmsg: Replication is being shut down
+                    errorLabels: ["RetryableWriteError"]
         operation:
             name: "insertOne"
             arguments: