SERVER-75473 Disallow document shard key updates outside of a retryable write or transaction

6 files changed, 104 insertions, 51 deletions

diff --git a/jstests/concurrency/fsm_workloads/write_without_shard_key_base.js b/jstests/concurrency/fsm_workloads/write_without_shard_key_base.js
index c16623de4c5..f601c9990d8 100644
--- a/jstests/concurrency/fsm_workloads/write_without_shard_key_base.js
+++ b/jstests/concurrency/fsm_workloads/write_without_shard_key_base.js
@@ -172,7 +172,7 @@ var $config = extendWorkload($config, function($config, $super) {
 
         const duplicateKeyInChangeShardKeyMsg = "Failed to update document's shard key field";
         const wouldChangeOwningShardMsg =
-            "Must run update to document shard key in a transaction or as a retryable write.";
+            "Must run update to shard key field in a multi-statement transaction";
         const otherErrorsInChangeShardKeyMsg = "was converted into a distributed transaction";
         const failureInRetryableWriteToTxnConversionMsg =
             "Cannot retry a retryable write that has been converted";
diff --git a/jstests/sharding/server_status_crud_metrics.js b/jstests/sharding/server_status_crud_metrics.js
index 89dfff0aa35..af763cabfbc 100644
--- a/jstests/sharding/server_status_crud_metrics.js
+++ b/jstests/sharding/server_status_crud_metrics.js
@@ -83,9 +83,9 @@ if (WriteWithoutShardKeyTestUtil.isWriteWithoutShardKeyFeatureEnabled(st.s)) {
         // If a WouldChangeOwningShard update is performed not as a retryable write or in a
         // transaction, expect an error.
         assert.eq(updateRes.getWriteError().code, ErrorCodes.IllegalOperation);
-        assert.eq(
-            updateRes.getWriteError().errmsg,
-            "Must run update to document shard key in a transaction or as a retryable write.");
+        assert(updateRes.getWriteError().errmsg.includes(
+            "Must run update to shard key field in a multi-statement transaction or with " +
+            "retryWrites: true."));
     }
 
     // Shouldn't increment the metrics for unsharded collection.
diff --git a/jstests/sharding/updateOne_without_shard_key/cluster_write_without_shard_key_basic.js b/jstests/sharding/updateOne_without_shard_key/cluster_write_without_shard_key_basic.js
index 632e2384266..e2ef51ad8f9 100644
--- a/jstests/sharding/updateOne_without_shard_key/cluster_write_without_shard_key_basic.js
+++ b/jstests/sharding/updateOne_without_shard_key/cluster_write_without_shard_key_basic.js
@@ -369,46 +369,6 @@ function runAndVerifyCommand(testCase) {
         targetDocId: {_id: _id},
     };
     assert.commandFailedWithCode(mongosConn.runCommand(cmdObj), ErrorCodes.IllegalOperation);
-
-    // Cannot specify $_allowShardKeyUpdatesWithoutFullShardKeyInQuery for an update from an
-    // external client.
-    cmdObj = {
-        _clusterWriteWithoutShardKey: 1,
-        writeCmd: {
-            update: collName,
-            updates: [
-                {q: {}, u: {$set: {x: 90}}, $_allowShardKeyUpdatesWithoutFullShardKeyInQuery: true},
-            ],
-        },
-        shardId: shard0Name,
-        targetDocId: {_id: _id},
-        txnNumber: NumberLong(1),
-        lsid: {id: UUID()},
-        startTransaction: true,
-        autocommit: false
-    };
-    assert.commandFailedWithCode(mongosConn.runCommand(cmdObj), ErrorCodes.InvalidOptions);
-
-    // Cannot specify $_allowShardKeyUpdatesWithoutFullShardKeyInQuery for a findAndModify from an
-    // external client.
-    cmdObj = {
-        _clusterWriteWithoutShardKey: 1,
-        writeCmd: {
-            findAndModify: collName,
-            query: {},
-            update: [
-                {$set: {a: aFieldValue}},
-            ],
-            $_allowShardKeyUpdatesWithoutFullShardKeyInQuery: true
-        },
-        shardId: shard0Name,
-        targetDocId: {_id: _id},
-        txnNumber: NumberLong(1),
-        lsid: {id: UUID()},
-        startTransaction: true,
-        autocommit: false
-    };
-    assert.commandFailedWithCode(mongosConn.runCommand(cmdObj), ErrorCodes.InvalidOptions);
 })();
 
 st.stop();
diff --git a/jstests/sharding/updateOne_without_shard_key/updateOne_without_shard_key_basic.js b/jstests/sharding/updateOne_without_shard_key/updateOne_without_shard_key_basic.js
index aba60f182ac..43190e150d1 100644
--- a/jstests/sharding/updateOne_without_shard_key/updateOne_without_shard_key_basic.js
+++ b/jstests/sharding/updateOne_without_shard_key/updateOne_without_shard_key_basic.js
@@ -321,6 +321,7 @@ const testCases = [
 
         replacementDocTest: true,  // Replacement tests validate that the final replacement
         // operation was only applied once.
+        mustBeInRetryableWriteOrTransaction: true,
         cmdObj: {
             update: collName,
             updates:
@@ -342,6 +343,7 @@ const testCases = [
 
         replacementDocTest: true,  // Replacement tests validate that the final replacement
         // operation was only applied once.
+        mustBeInRetryableWriteOrTransaction: true,
         cmdObj: {
             update: collName,
             updates: [
@@ -365,9 +367,9 @@ const testCases = [
             {_id: 1, x: xFieldValShard1_1, y: yFieldVal}
         ],
 
-        replacementDocTest: true,      // Replacement tests validate that the final replacement
-                                       // operation was only applied once.
-        wouldChangeOwningShard: true,  // Can only run in retryable write or transaction.
+        replacementDocTest: true,  // Replacement tests validate that the final replacement
+                                   // operation was only applied once.
+        mustBeInRetryableWriteOrTransaction: true,
         cmdObj: {
             update: collName,
             updates: [{q: {y: yFieldVal}, u: {x: xFieldValShard0_2, y: yFieldVal, a: setFieldVal}}]
@@ -404,6 +406,7 @@ const testCases = [
                 {q: {y: 6}, u: {x: -1, _id: 6}, upsert: true}
             ],
         },
+        mustBeInRetryableWriteOrTransaction: true,
         options: [{ordered: true}, {ordered: false}],
         expectedMods: [{_id: 0, x: xFieldValShard0_1, y: yFieldVal + 1}, {_id: 6, y: 6, x: -1}],
         expectedResponse: {n: 2, nModified: 1, upserted: [{"index": 1, _id: 6}]},
@@ -425,7 +428,7 @@ const isTxnApiEnabled = FeatureFlagUtil.isEnabled(
 configurations.forEach(config => {
     let conn = WriteWithoutShardKeyTestUtil.getClusterConnection(st, config);
     testCases.forEach(testCase => {
-        if (!isTxnApiEnabled && testCase.wouldChangeOwningShard &&
+        if (!isTxnApiEnabled && testCase.mustBeInRetryableWriteOrTransaction &&
             (config === WriteWithoutShardKeyTestUtil.Configurations.noSession ||
              config === WriteWithoutShardKeyTestUtil.Configurations.sessionNotRetryableWrite)) {
             return;
diff --git a/jstests/sharding/updateOne_without_shard_key/write_without_shard_key_update_shard_key_errors.js b/jstests/sharding/updateOne_without_shard_key/write_without_shard_key_update_shard_key_errors.js
new file mode 100644
index 00000000000..9b9fca7f006
--- /dev/null
+++ b/jstests/sharding/updateOne_without_shard_key/write_without_shard_key_update_shard_key_errors.js
@@ -0,0 +1,90 @@
+/**
+ * Test that updateOne and findAndModify without shard key fails to update a document shard key if
+ * it was not run as a retryable write or transaction.
+ *
+ * TODO: SERVER-67429 Remove or amend this test since we can update document shard key outside of a
+ * retryable write or transaction.
+ *
+ * @tags: [
+ *    requires_sharding,
+ *    requires_fcv_70,
+ *    uses_transactions,
+ *    uses_multi_shard_transaction,
+ *    featureFlagUpdateOneWithoutShardKey,
+ * ]
+ */
+
+(function() {
+"use strict";
+
+load("jstests/sharding/updateOne_without_shard_key/libs/write_without_shard_key_test_util.js");
+
+// Make sure we're testing with no implicit session.
+TestData.disableImplicitSessions = true;
+
+// 2 shards single node, 1 mongos, 1 config server 3-node.
+const st = new ShardingTest({});
+const dbName = "testDb";
+const collName = "testColl";
+const nss = dbName + "." + collName;
+const splitPoint = 0;
+const docsToInsert =
+    [{_id: 0, x: -2, y: 1}, {_id: 1, x: -1, y: 1}, {_id: 2, x: 1, y: 1}, {_id: 3, x: 2, y: 1}];
+
+// Sets up a 2 shard cluster using 'x' as a shard key where Shard 0 owns x <
+// splitPoint and Shard 1 splitPoint >= 0.
+WriteWithoutShardKeyTestUtil.setupShardedCollection(
+    st, nss, {x: 1}, [{x: splitPoint}], [{query: {x: splitPoint}, shard: st.shard1.shardName}]);
+
+assert.commandWorked(st.getDB(dbName).getCollection(collName).insert(docsToInsert));
+
+const updateShardKeyErrMsg = "Must run update to shard key field in a multi-statement transaction";
+
+let testCases = [
+    {
+        logMessage: "Running update document shard key error test for updateOne.",
+        cmdObj: {
+            update: collName,
+            updates: [{q: {y: 1}, u: {x: 5, y: 1}}],
+        },
+    },
+    {
+        logMessage: "Running update document shard key error test for findAndModify",
+        cmdObj: {
+            findAndModify: collName,
+            query: {y: 1},
+            update: {x: 5, y: 1},
+        },
+    },
+];
+
+const configurations = [
+    WriteWithoutShardKeyTestUtil.Configurations.noSession,
+    WriteWithoutShardKeyTestUtil.Configurations.sessionNotRetryableWrite,
+];
+
+configurations.forEach(config => {
+    testCases.forEach(testCase => {
+        jsTestLog(testCase.logMessage);
+        let conn = WriteWithoutShardKeyTestUtil.getClusterConnection(st, config);
+        let dbConn;
+        if (config === WriteWithoutShardKeyTestUtil.Configurations.noSession) {
+            dbConn = conn.getDB(dbName);
+        } else {
+            dbConn = conn.getDatabase(dbName);
+        }
+
+        let res = assert.commandFailedWithCode(dbConn.runCommand(testCase.cmdObj),
+                                               ErrorCodes.IllegalOperation);
+        let errmsg;
+        if (res.writeErrors) {
+            errmsg = res.writeErrors[0].errmsg;
+        } else {
+            errmsg = res.errmsg;
+        }
+        assert(errmsg.includes(updateShardKeyErrMsg));
+    });
+});
+
+st.stop();
+})();
diff --git a/jstests/sharding/update_compound_shard_key.js b/jstests/sharding/update_compound_shard_key.js
index 866d8fc7301..76bdaa8c5a1 100644
--- a/jstests/sharding/update_compound_shard_key.js
+++ b/jstests/sharding/update_compound_shard_key.js
@@ -195,9 +195,9 @@ if (WriteWithoutShardKeyTestUtil.isWriteWithoutShardKeyFeatureEnabled(st.s)) {
         // If a WouldChangeOwningShard update is performed not as a retryable write or in a
         // transaction, expect an error.
         assert.eq(updateRes.getWriteError().code, ErrorCodes.IllegalOperation);
-        assert.eq(
-            updateRes.getWriteError().errmsg,
-            "Must run update to document shard key in a transaction or as a retryable write.");
+        assert(updateRes.getWriteError().errmsg.includes(
+            "Must run update to shard key field in a multi-statement transaction or with " +
+            "retryWrites: true."));
     }
 
 } else {