SERVER-24174: Inconsistent rule for storing dotted field names

12 files changed, 255 insertions, 194 deletions

diff --git a/jstests/core/basic3.js b/jstests/core/basic3.js
deleted file mode 100644
index 2fa26627bf9..00000000000
--- a/jstests/core/basic3.js
+++ /dev/null
@@ -1,40 +0,0 @@
-// Tests that "." cannot be in field names
-t = db.getCollection("foo_basic3");
-t.drop();
-
-// more diagnostics on bad save, if exception fails
-doBadSave = function(param) {
-    print("doing save with " + tojson(param));
-    var res = t.save(param);
-    // Should not get here.
-    printjson(res);
-};
-
-// more diagnostics on bad save, if exception fails
-doBadUpdate = function(query, update) {
-    print("doing update with " + tojson(query) + " " + tojson(update));
-    var res = t.update(query, update);
-    // Should not get here.
-    printjson(res);
-};
-
-assert.throws(doBadSave, [{"a.b": 5}], ". in names aren't allowed doesn't work");
-
-assert.throws(doBadSave, [{"x": {"a.b": 5}}], ". in embedded names aren't allowed doesn't work");
-
-// following tests make sure update keys are checked
-t.save({"a": 0, "b": 1});
-
-assert.throws(doBadUpdate, [{a: 0}, {"b.b": 1}], "must deny '.' in key of update");
-
-// upsert with embedded doc
-assert.throws(doBadUpdate, [{a: 10}, {c: {"b.b": 1}}], "must deny embedded '.' in key of update");
-
-// if it is a modifier, it should still go through
-t.update({"a": 0}, {$set: {"c.c": 1}});
-t.update({"a": 0}, {$inc: {"c.c": 1}});
-
-// edge cases
-assert.throws(
-    doBadUpdate, [{a: 0}, {"": {"b.b": 1}}], "must deny '' embedded '.' in key of update");
-t.update({"a": 0}, {});
diff --git a/jstests/core/basic9.js b/jstests/core/basic9.js
deleted file mode 100644
index 6d368f46e3b..00000000000
--- a/jstests/core/basic9.js
+++ /dev/null
@@ -1,18 +0,0 @@
-// Tests that $<prefix> field names are not allowed, but you can use a $ anywhere else.
-t = db.getCollection("foo_basic9");
-t.drop();
-
-// more diagnostics on bad save, if exception fails
-doBadSave = function(param) {
-    print("doing save with " + tojson(param));
-    var res = t.save(param);
-    // Should not get here.
-    print('Should have errored out: ' + tojson(res));
-};
-
-t.save({foo$foo: 5});
-t.save({foo$: 5});
-
-assert.throws(doBadSave, [{$foo: 5}], "key names aren't allowed to start with $ doesn't work");
-assert.throws(
-    doBadSave, [{x: {$foo: 5}}], "embedded key names aren't allowed to start with $ doesn't work");
diff --git a/jstests/core/basicb.js b/jstests/core/basicb.js
deleted file mode 100644
index 65531d706a1..00000000000
--- a/jstests/core/basicb.js
+++ /dev/null
@@ -1,7 +0,0 @@
-
-t = db.basicb;
-t.drop();
-
-assert.throws(function() {
-    t.insert({'$a': 5});
-});
diff --git a/jstests/core/bulk_api_ordered.js b/jstests/core/bulk_api_ordered.js
index 4efdd7c319b..45450eee388 100644
--- a/jstests/core/bulk_api_ordered.js
+++ b/jstests/core/bulk_api_ordered.js
@@ -20,16 +20,13 @@ jsTest.log("Starting bulk api ordered tests...");
  *******************************************************/
 var executeTests = function() {
     /**
-     * find() requires selector and $key is disallowed as field name
+     * find() requires selector.
      */
     var bulkOp = coll.initializeOrderedBulkOp();
 
     assert.throws(function() {
         bulkOp.find();
     });
-    assert.throws(function() {
-        bulkOp.insert({$key: 1});
-    });
 
     /**
      * Single successful ordered bulk operation
diff --git a/jstests/core/bulk_api_unordered.js b/jstests/core/bulk_api_unordered.js
index 8e2ca7a7157..2cbbeef5920 100644
--- a/jstests/core/bulk_api_unordered.js
+++ b/jstests/core/bulk_api_unordered.js
@@ -23,16 +23,13 @@ var executeTests = function() {
     coll.remove({});
 
     /**
-     * find() requires selector and $key is disallowed as field name
+     * find() requires selector.
      */
     var bulkOp = coll.initializeUnorderedBulkOp();
 
     assert.throws(function() {
         bulkOp.find();
     });
-    assert.throws(function() {
-        bulkOp.insert({$key: 1});
-    });
 
     /**
      * Single successful unordered bulk operation
diff --git a/jstests/core/field_name_validation.js b/jstests/core/field_name_validation.js
new file mode 100644
index 00000000000..656e01886cc
--- /dev/null
+++ b/jstests/core/field_name_validation.js
@@ -0,0 +1,182 @@
+/**
+ * Test the behavior of field names containing special characters, including:
+ * - dots
+ * - $-prefixed
+ * - reserved $-prefixed ($ref, $id, $db)
+ * - regex
+ *
+ * contained in a top-level element, embedded element, and within _id.
+ *
+ * @tags: [assumes_unsharded_collection]
+ */
+(function() {
+    "use strict";
+
+    const coll = db.field_name_validation;
+    coll.drop();
+
+    //
+    // Insert command field name validation.
+    //
+
+    // Test that dotted field names are allowed.
+    assert.writeOK(coll.insert({"a.b": 1}));
+    assert.writeOK(coll.insert({"_id.a": 1}));
+    assert.writeOK(coll.insert({a: {"a.b": 1}}));
+    assert.writeOK(coll.insert({_id: {"a.b": 1}}));
+
+    // Test that _id cannot be a regex.
+    assert.writeError(coll.insert({_id: /a/}));
+
+    // Test that _id cannot be an array.
+    assert.writeError(coll.insert({_id: [9]}));
+
+    // Test that $-prefixed field names are allowed in embedded objects.
+    assert.writeOK(coll.insert({a: {$b: 1}}));
+    assert.eq(1, coll.find({"a.$b": 1}).itcount());
+
+    // Test that $-prefixed field names are not allowed at the top level.
+    assert.writeErrorWithCode(coll.insert({$a: 1}), ErrorCodes.BadValue);
+    assert.writeErrorWithCode(coll.insert({valid: 1, $a: 1}), ErrorCodes.BadValue);
+
+    // Test that reserved $-prefixed field names are also not allowed.
+    assert.writeErrorWithCode(coll.insert({$ref: 1}), ErrorCodes.BadValue);
+    assert.writeErrorWithCode(coll.insert({$id: 1}), ErrorCodes.BadValue);
+    assert.writeErrorWithCode(coll.insert({$db: 1}), ErrorCodes.BadValue);
+
+    // Test that _id cannot be an object with an element that has a $-prefixed field name.
+    assert.writeErrorWithCode(coll.insert({_id: {$b: 1}}), ErrorCodes.DollarPrefixedFieldName);
+    assert.writeErrorWithCode(coll.insert({_id: {a: 1, $b: 1}}),
+                              ErrorCodes.DollarPrefixedFieldName);
+
+    // Should not enforce the same restrictions on an embedded _id field.
+    assert.writeOK(coll.insert({a: {_id: [9]}}));
+    assert.writeOK(coll.insert({a: {_id: /a/}}));
+    assert.writeOK(coll.insert({a: {_id: {$b: 1}}}));
+
+    //
+    // Update command field name validation.
+    //
+    coll.drop();
+
+    // Dotted fields are allowed in an update.
+    assert.writeOK(coll.update({}, {"a.b": 1}, {upsert: true}));
+    assert.eq(0, coll.find({"a.b": 1}).itcount());
+    assert.eq(1, coll.find({}).itcount());
+
+    // Dotted fields represent paths in $set.
+    assert.writeOK(coll.update({}, {$set: {"a.b": 1}}, {upsert: true}));
+    assert.eq(1, coll.find({"a.b": 1}).itcount());
+
+    // Dotted fields represent paths in the query object.
+    assert.writeOK(coll.update({"a.b": 1}, {$set: {"a.b": 2}}));
+    assert.eq(1, coll.find({"a.b": 2}).itcount());
+    assert.eq(1, coll.find({a: {b: 2}}).itcount());
+
+    assert.writeOK(coll.update({"a.b": 2}, {"a.b": 3}));
+    assert.eq(0, coll.find({"a.b": 3}).itcount());
+
+    // $-prefixed field names are not allowed.
+    assert.writeErrorWithCode(coll.update({"a.b": 1}, {$c: 1}, {upsert: true}),
+                              ErrorCodes.FailedToParse);
+    assert.writeErrorWithCode(coll.update({"a.b": 1}, {$set: {$c: 1}}, {upsert: true}),
+                              ErrorCodes.DollarPrefixedFieldName);
+    assert.writeErrorWithCode(coll.update({"a.b": 1}, {$set: {c: {$d: 1}}}, {upsert: true}),
+                              ErrorCodes.DollarPrefixedFieldName);
+
+    // Reserved $-prefixed field names are also not allowed.
+    assert.writeErrorWithCode(coll.update({"a.b": 1}, {$ref: 1}), ErrorCodes.FailedToParse);
+    assert.writeErrorWithCode(coll.update({"a.b": 1}, {$id: 1}), ErrorCodes.FailedToParse);
+    assert.writeErrorWithCode(coll.update({"a.b": 1}, {$db: 1}), ErrorCodes.FailedToParse);
+
+    //
+    // FindAndModify field name validation.
+    //
+    coll.drop();
+
+    // Dotted fields are allowed in update object.
+    coll.findAndModify({query: {_id: 0}, update: {_id: 0, "a.b": 1}, upsert: true});
+    assert.eq([{_id: 0, "a.b": 1}], coll.find({_id: 0}).toArray());
+
+    // Dotted fields represent paths in $set.
+    coll.findAndModify({query: {_id: 1}, update: {$set: {_id: 1, "a.b": 1}}, upsert: true});
+    assert.eq([{_id: 1, a: {b: 1}}], coll.find({_id: 1}).toArray());
+
+    // Dotted fields represent paths in the query object.
+    coll.findAndModify({query: {_id: 0, "a.b": 1}, update: {"a.b": 2}});
+    assert.eq([{_id: 0, "a.b": 1}], coll.find({_id: 0}).toArray());
+
+    coll.findAndModify({query: {_id: 1, "a.b": 1}, update: {$set: {_id: 1, "a.b": 2}}});
+    assert.eq([{_id: 1, a: {b: 2}}], coll.find({_id: 1}).toArray());
+
+    // $-prefixed field names are not allowed.
+    assert.throws(function() {
+        coll.findAndModify({query: {_id: 1}, update: {_id: 1, $invalid: 1}});
+    });
+    assert.throws(function() {
+        coll.findAndModify({query: {_id: 1}, update: {$set: {_id: 1, $invalid: 1}}});
+    });
+
+    // Reserved $-prefixed field names are also not allowed.
+    assert.throws(function() {
+        coll.findAndModify({query: {_id: 1}, update: {_id: 1, $ref: 1}});
+    });
+    assert.throws(function() {
+        coll.findAndModify({query: {_id: 1}, update: {_id: 1, $id: 1}});
+    });
+    assert.throws(function() {
+        coll.findAndModify({query: {_id: 1}, update: {_id: 1, $db: 1}});
+    });
+
+    //
+    // Aggregation field name validation.
+    //
+    coll.drop();
+
+    assert.writeOK(coll.insert({_id: {a: 1, b: 2}, "c.d": 3}));
+
+    // Dotted fields represent paths in an aggregation pipeline.
+    assert.eq(coll.aggregate([{$match: {"_id.a": 1}}, {$project: {"_id.b": 1}}]).toArray(),
+              [{_id: {b: 2}}]);
+    assert.eq(coll.aggregate([{$match: {"c.d": 3}}, {$project: {"_id.b": 1}}]).toArray(), []);
+
+    assert.eq(coll.aggregate([{$project: {"_id.a": 1}}]).toArray(), [{_id: {a: 1}}]);
+    assert.eq(coll.aggregate([{$project: {"c.d": 1, _id: 0}}]).toArray(), [{}]);
+
+    assert.eq(coll.aggregate([
+                      {$addFields: {"new.field": {$multiply: ["$c.d", "$_id.a"]}}},
+                      {$project: {"new.field": 1, _id: 0}}
+                  ])
+                  .toArray(),
+              [{new: {field: null}}]);
+
+    assert.eq(coll.aggregate([{$group: {_id: "$_id.a", e: {$sum: "$_id.b"}}}]).toArray(),
+              [{_id: 1, e: 2}]);
+    assert.eq(coll.aggregate([{$group: {_id: "$_id.a", e: {$sum: "$c.d"}}}]).toArray(),
+              [{_id: 1, e: 0}]);
+
+    // Accumulation statements cannot have a dotted field name.
+    assert.commandFailed(db.runCommand({
+        aggregate: coll.getName(),
+        pipeline: [{$group: {_id: "$_id.a", "e.f": {$sum: "$_id.b"}}}]
+    }));
+
+    // $-prefixed field names are not allowed in an aggregation pipeline.
+    assert.commandFailed(
+        db.runCommand({aggregate: coll.getName(), pipeline: [{$match: {"$invalid": 1}}]}));
+
+    assert.commandFailed(db.runCommand({
+        aggregate: coll.getName(),
+        pipeline: [{$project: {"_id.a": 1, "$newField": {$multiply: ["$_id.b", "$_id.a"]}}}]
+    }));
+
+    assert.commandFailed(db.runCommand({
+        aggregate: coll.getName(),
+        pipeline: [{$addFields: {"_id.a": 1, "$newField": {$multiply: ["$_id.b", "$_id.a"]}}}]
+    }));
+
+    assert.commandFailed(db.runCommand({
+        aggregate: coll.getName(),
+        pipeline: [{$group: {_id: "$_id.a", "$invalid": {$sum: "$_id.b"}}}]
+    }));
+})();
diff --git a/jstests/core/insert2.js b/jstests/core/insert2.js
index 5da0fc5b7f5..3b3b147af0e 100644
--- a/jstests/core/insert2.js
+++ b/jstests/core/insert2.js
@@ -5,7 +5,6 @@
 // Create a new connection object so it won't affect the global connection when we modify
 // it's settings.
 var conn = new Mongo(db.getMongo().host);
-conn._skipValidation = true;
 conn.forceWriteMode(db.getMongo().writeMode());
 
 t = conn.getDB(db.getName()).insert2;
diff --git a/jstests/core/remove_undefined.js b/jstests/core/remove_undefined.js
index 3f07acfe1c3..af7dce1e409 100644
--- a/jstests/core/remove_undefined.js
+++ b/jstests/core/remove_undefined.js
@@ -1,35 +1,30 @@
+(function() {
+    "use strict";
 
-t = db.drop_undefined.js;
+    const coll = db.remove_undefined;
+    coll.drop();
 
-t.insert({_id: 1});
-t.insert({_id: 2});
-t.insert({_id: null});
+    assert.writeOK(coll.insert({_id: 1}));
+    assert.writeOK(coll.insert({_id: 2}));
+    assert.writeOK(coll.insert({_id: null}));
 
-z = {
-    foo: 1,
-    x: null
-};
+    const obj = {foo: 1, nullElem: null};
 
-t.remove({x: z.bar});
-assert.eq(3, t.count(), "A1");
+    coll.remove({x: obj.bar});
+    assert.eq(3, coll.count());
 
-t.remove({x: undefined});
-assert.eq(3, t.count(), "A2");
+    coll.remove({x: undefined});
+    assert.eq(3, coll.count());
 
-assert.throws(function() {
-    t.remove({_id: z.bar});
-}, [], "B1");
-assert.throws(function() {
-    t.remove({_id: undefined});
-}, [], "B2");
+    assert.writeErrorWithCode(coll.remove({_id: obj.bar}), ErrorCodes.BadValue);
+    assert.writeErrorWithCode(coll.remove({_id: undefined}), ErrorCodes.BadValue);
 
-t.remove({_id: z.x});
-assert.eq(2, t.count(), "C1");
+    coll.remove({_id: obj.nullElem});
+    assert.eq(2, coll.count());
 
-t.insert({_id: null});
-assert.eq(3, t.count(), "C2");
+    assert.writeOK(coll.insert({_id: null}));
+    assert.eq(3, coll.count());
 
-assert.throws(function() {
-    t.remove({_id: undefined});
-}, [], "C3");
-assert.eq(3, t.count(), "C4");
+    assert.writeErrorWithCode(coll.remove({_id: undefined}), ErrorCodes.BadValue);
+    assert.eq(3, coll.count());
+})();
diff --git a/jstests/core/update_replace.js b/jstests/core/update_replace.js
deleted file mode 100644
index e62d03eb09f..00000000000
--- a/jstests/core/update_replace.js
+++ /dev/null
@@ -1,52 +0,0 @@
-// This test checks validation of the replaced doc (on the server) for dots, $prefix and _id
-
-// Create a new connection object so it won't affect the global connection when we modify
-// it's settings.
-var conn = new Mongo(db.getMongo().host);
-conn.forceWriteMode(db.getMongo().writeMode());
-t = conn.getDB(db.getName()).jstests_update_replace;
-t.drop();
-
-var myDB = t.getDB();
-var res;
-
-// Bypass validation in shell so we can test the server.
-conn._skipValidation = true;
-
-// Allow "." in field names
-res = t.save({_id: 1, "a.a": 1});
-assert(!res.hasWriteError(), "a.a");
-
-// Allow "." in field names, embedded
-res = t.save({_id: 1, a: {"a.a": 1}});
-assert(!res.hasWriteError(), "a: a.a");
-
-// Should not allow "$"-prefixed field names, caught before "." check
-res = t.save({_id: 1, $a: {"a.a": 1}});
-assert(res.hasWriteError(), "$a: a.a");
-
-// Should not allow "$"-prefixed field names
-res = t.save({_id: 1, $a: 1});
-assert(res.hasWriteError(), "$a");
-
-// _id validation checks
-
-// Should not allow regex _id
-res = t.save({_id: /a/});
-assert(res.hasWriteError(), "_id regex");
-
-// Should not allow regex _id, even if not first
-res = t.save({a: 2, _id: /a/});
-assert(res.hasWriteError(), "a _id regex");
-
-// Should not allow array _id
-res = t.save({_id: [9]});
-assert(res.hasWriteError(), "_id array");
-
-// This is fine since _id isn't a top level field
-res = t.save({a: {_id: [9]}});
-assert(!res.hasWriteError(), "embedded _id array");
-
-// This is fine since _id isn't a top level field
-res = t.save({b: 1, a: {_id: [9]}});
-assert(!res.hasWriteError(), "b embedded _id array");
diff --git a/jstests/sharding/shard_collection_basic.js b/jstests/sharding/shard_collection_basic.js
index 26ed855f393..330df5cf39c 100644
--- a/jstests/sharding/shard_collection_basic.js
+++ b/jstests/sharding/shard_collection_basic.js
@@ -81,6 +81,16 @@
     assert.commandFailed(
         mongos.adminCommand({shardCollection: kDbName + '.foo', key: {aKey: "hahahashed"}}));
 
+    // Shard key cannot contain embedded objects.
+    assert.commandFailed(
+        mongos.adminCommand({shardCollection: kDbName + '.foo', key: {_id: {a: 1}}}));
+    assert.commandFailed(
+        mongos.adminCommand({shardCollection: kDbName + '.foo', key: {_id: {'a.b': 1}}}));
+
+    // Shard key can contain dotted path to embedded element.
+    assert.commandWorked(mongos.adminCommand(
+        {shardCollection: kDbName + '.shard_key_dotted_path', key: {'_id.a': 1}}));
+
     //
     // Test shardCollection's idempotency
     //
diff --git a/jstests/sharding/shard_key_immutable.js b/jstests/sharding/shard_key_immutable.js
index 76a648d8811..bbc2a259e51 100644
--- a/jstests/sharding/shard_key_immutable.js
+++ b/jstests/sharding/shard_key_immutable.js
@@ -703,29 +703,21 @@ doc = dotColl.findOne();
 delete doc._id;
 assert(friendlyEqual(doc, {x: {a: 100, b: 2}}), 'doc did not change: ' + tojson(doc));
 
-// Dotted field names in the resulting objects should not be allowed.
-// This check currently resides in the client drivers.
+// Dotted field names in the update object do not represent paths. The path must be expanded
+// since single document updates require a match on the shard key.
+// (e.g. 'x.a' will not work, but x: {a: ...} will work).
 dotColl.remove({}, false);
 dotColl.insert({x: {a: 100}});
-assert.throws(function() {
-    dotColl.update({'x.a': 100}, {x: {'a.z': 100}});
-});
+assert.writeErrorWithCode(dotColl.update({'x.a': 100}, {x: {'a.z': 100}}),
+                          ErrorCodes.ShardKeyNotFound);
 
-// Dotted field names in the resulting objects should not be allowed.
-// This check currently resides in the client drivers.
 dotColl.remove({}, false);
 dotColl.insert({x: {a: 100}});
-assert.throws(function() {
-    dotColl.update({'x.a': 100}, {'x.a': 100});
-});
+assert.writeErrorWithCode(dotColl.update({'x.a': 100}, {'x.a': 100}), ErrorCodes.ShardKeyNotFound);
 
-// Dotted field names in the resulting objects should not be allowed.
-// This check currently resides in the client drivers.
 dotColl.remove({}, false);
 dotColl.insert({x: {a: 100}});
-assert.throws(function() {
-    dotColl.update({'x.a': 100}, {'x.a.z': 100});
-});
+assert.writeOK(dotColl.update({'x.a': 100}, {x: {a: 100}, b: 1}));
 
 dotColl.remove({}, false);
 dotColl.insert({x: {a: 100}});
@@ -762,6 +754,7 @@ doc = dotColl.findOne();
 delete doc._id;
 assert(friendlyEqual(doc, {x: {a: 100}}), 'doc changed: ' + tojson(doc));
 
+// Dotted field names within $set are treated as paths.
 dotColl.remove({}, false);
 dotColl.insert({x: {a: 100}});
 assert.writeOK(dotColl.update({'x.a': 100}, {$set: {'x.a': 100, b: 2}}, false, true));
@@ -806,26 +799,14 @@ assert.writeOK(dotColl.update({'x.a': 100}, {x: {a: 100, b: 2}}, true));
 doc = dotColl.findOne();
 assert(doc != null, 'doc was not upserted: ' + tojson(doc));
 
-// Dotted field names in the resulting objects should not be allowed.
-// This check currently resides in the client drivers.
 dotColl.remove({}, false);
-assert.throws(function() {
-    dotColl.update({'x.a': 100}, {x: {'a.z': 100}}, true);
-});
+assert.writeError(dotColl.update({'x.a': 100}, {x: {z: 100}}, true));
 
-// Dotted field names in the resulting objects should not be allowed.
-// This check currently resides in the client drivers.
 dotColl.remove({}, false);
-assert.throws(function() {
-    dotColl.update({'x.a': 100}, {'x.a': 100}, true);
-});
+assert.writeError(dotColl.update({'x.a': 100}, {'x.a': 100}, true));
 
-// Dotted field names in the resulting objects should not be allowed.
-// This check currently resides in the client drivers.
 dotColl.remove({}, false);
-assert.throws(function() {
-    dotColl.update({'x.a': 100}, {'x.a.z': 100}, true);
-});
+assert.writeError(dotColl.update({'x.a': 100}, {'x.a.z': 100}, true));
 
 dotColl.remove({}, false);
 assert.writeError(dotColl.update({'x.a': 100}, {x: 100}, true));
diff --git a/jstests/sharding/tag_range.js b/jstests/sharding/tag_range.js
index 0dec96f52d8..fba599e896b 100644
--- a/jstests/sharding/tag_range.js
+++ b/jstests/sharding/tag_range.js
@@ -2,7 +2,7 @@
 (function() {
     'use strict';
 
-    var st = new ShardingTest({shards: 2, mongos: 1});
+    const st = new ShardingTest({shards: 2, mongos: 1});
 
     assert.commandWorked(st.s0.adminCommand({enableSharding: 'test'}));
     st.ensurePrimaryShard('test', 'shard0001');
@@ -16,17 +16,18 @@
 
     st.addShardTag('shard0000', 'a');
     st.addShardTag('shard0000', 'b');
+    st.addShardTag('shard0000', 'c');
 
     // add two ranges, verify the additions
 
-    st.addTagRange('test.tag_range', {_id: 5}, {_id: 10}, 'a');
-    st.addTagRange('test.tag_range', {_id: 10}, {_id: 15}, 'b');
+    assert.commandWorked(st.addTagRange('test.tag_range', {_id: 5}, {_id: 10}, 'a'));
+    assert.commandWorked(st.addTagRange('test.tag_range', {_id: 10}, {_id: 15}, 'b'));
 
     countTags(2, 'tag ranges were not successfully added');
 
     // remove the second range, should be left with one
 
-    st.removeTagRange('test.tag_range', {_id: 10}, {_id: 15}, 'b');
+    assert.commandWorked(st.removeTagRange('test.tag_range', {_id: 10}, {_id: 15}, 'b'));
 
     countTags(1, 'tag range not removed successfully');
 
@@ -38,38 +39,54 @@
         countTags(1, 'tag range should not have been added');
     }
 
+    // Test that a dotted path is allowed in a tag range if it includes the shard key.
+    assert.commandWorked(
+        st.s0.adminCommand({shardCollection: 'test.tag_range_dotted', key: {"_id.a": 1}}));
+    assert.commandWorked(st.addTagRange('test.tag_range_dotted', {"_id.a": 5}, {"_id.a": 10}, 'c'));
+    countTags(2, 'Dotted path tag range not successfully added.');
+
+    assert.commandWorked(
+        st.removeTagRange('test.tag_range_dotted', {"_id.a": 5}, {"_id.a": 10}, 'c'));
+    assert.commandFailed(st.addTagRange('test.tag_range_dotted', {"_id.b": 5}, {"_id.b": 10}, 'c'));
+    countTags(1, 'Incorrectly added tag range.');
+
+    // Test that ranges on embedded fields of the shard key are not allowed.
+    assert.commandFailed(
+        st.addTagRange('test.tag_range_dotted', {_id: {a: 5}}, {_id: {a: 10}}, 'c'));
+    countTags(1, 'Incorrectly added embedded field tag range');
+
     // removeTagRange tests for tag ranges that do not exist
 
     // Bad namespace
-    st.removeTagRange('badns', {_id: 5}, {_id: 11}, 'a');
+    assert.commandFailed(st.removeTagRange('badns', {_id: 5}, {_id: 11}, 'a'));
     countTags(1, 'Bad namespace: tag range does not exist');
 
     // Bad tag
-    st.removeTagRange('test.tag_range', {_id: 5}, {_id: 11}, 'badtag');
+    assert.commandWorked(st.removeTagRange('test.tag_range', {_id: 5}, {_id: 11}, 'badtag'));
     countTags(1, 'Bad tag: tag range does not exist');
 
     // Bad min
-    st.removeTagRange('test.tag_range', {_id: 0}, {_id: 11}, 'a');
+    assert.commandWorked(st.removeTagRange('test.tag_range', {_id: 0}, {_id: 11}, 'a'));
     countTags(1, 'Bad min: tag range does not exist');
 
     // Bad max
-    st.removeTagRange('test.tag_range', {_id: 5}, {_id: 12}, 'a');
+    assert.commandWorked(st.removeTagRange('test.tag_range', {_id: 5}, {_id: 12}, 'a'));
     countTags(1, 'Bad max: tag range does not exist');
 
     // Invalid namesapce
-    st.removeTagRange(35, {_id: 5}, {_id: 11}, 'a');
+    assert.commandFailed(st.removeTagRange(35, {_id: 5}, {_id: 11}, 'a'));
     countTags(1, 'Invalid namespace: tag range does not exist');
 
     // Invalid tag
-    st.removeTagRange('test.tag_range', {_id: 5}, {_id: 11}, 35);
+    assert.commandWorked(st.removeTagRange('test.tag_range', {_id: 5}, {_id: 11}, 35));
     countTags(1, 'Invalid tag: tag range does not exist');
 
     // Invalid min
-    st.removeTagRange('test.tag_range', 35, {_id: 11}, 'a');
+    assert.commandFailed(st.removeTagRange('test.tag_range', 35, {_id: 11}, 'a'));
     countTags(1, 'Invalid min: tag range does not exist');
 
     // Invalid max
-    st.removeTagRange('test.tag_range', {_id: 5}, 35, 'a');
+    assert.commandFailed(st.removeTagRange('test.tag_range', {_id: 5}, 35, 'a'));
     countTags(1, 'Invalid max: tag range does not exist');
 
     st.stop();