SERVER-34644 Assert that ShardKeyPattern is always constructed with a valid key

There is never a reason to construct ShardKeyPattern with an invalid
shard key because this information comes from the sharded metadata.
Asserting that this is the case allows us to also get rid of all the
ShardKeyPattern::isValid checks.

5 files changed, 70 insertions, 117 deletions

diff --git a/src/mongo/db/dbhelpers.cpp b/src/mongo/db/dbhelpers.cpp
index cecdbccf6fb..2c29260a9e9 100644
--- a/src/mongo/db/dbhelpers.cpp
+++ b/src/mongo/db/dbhelpers.cpp
@@ -54,14 +54,12 @@
 #include "mongo/db/query/query_planner.h"
 #include "mongo/db/repl/repl_client_info.h"
 #include "mongo/db/repl/replication_coordinator.h"
-#include "mongo/db/s/sharding_state.h"
 #include "mongo/db/service_context.h"
 #include "mongo/db/storage/data_protector.h"
 #include "mongo/db/storage/encryption_hooks.h"
 #include "mongo/db/storage/storage_options.h"
 #include "mongo/db/write_concern.h"
 #include "mongo/db/write_concern_options.h"
-#include "mongo/s/shard_key_pattern.h"
 #include "mongo/util/log.h"
 #include "mongo/util/mongoutils/str.h"
 #include "mongo/util/scopeguard.h"
diff --git a/src/mongo/db/s/config/configsvr_shard_collection_command.cpp b/src/mongo/db/s/config/configsvr_shard_collection_command.cpp
index 52c802db5dc..5aecda95945 100644
--- a/src/mongo/db/s/config/configsvr_shard_collection_command.cpp
+++ b/src/mongo/db/s/config/configsvr_shard_collection_command.cpp
@@ -140,14 +140,6 @@ void validateAndDeduceFullRequestOptions(OperationContext* opCtx,
     uassert(
         ErrorCodes::InvalidOptions, "cannot have empty shard key", !request->getKey().isEmpty());
 
-    // Ensure the proposed shard key is valid.
-    uassert(ErrorCodes::InvalidOptions,
-            str::stream()
-                << "Unsupported shard key pattern "
-                << shardKeyPattern.toString()
-                << ". Pattern must either be a single hashed field, or a list of ascending fields",
-            shardKeyPattern.isValid());
-
     // Ensure that hashed and unique are not both set.
     uassert(ErrorCodes::InvalidOptions,
             "Hashed shard keys cannot be declared unique. It's possible to ensure uniqueness on "
diff --git a/src/mongo/s/shard_key_pattern.cpp b/src/mongo/s/shard_key_pattern.cpp
index f8e7e5b4395..01b87312e41 100644
--- a/src/mongo/s/shard_key_pattern.cpp
+++ b/src/mongo/s/shard_key_pattern.cpp
@@ -66,32 +66,38 @@ bool isHashedPatternEl(const BSONElement& el) {
  * ii) a compound list of ascending, potentially-nested field paths, e.g. { a : 1 , b.c : 1 }
  */
 std::vector<std::unique_ptr<FieldRef>> parseShardKeyPattern(const BSONObj& keyPattern) {
+    uassert(ErrorCodes::BadValue, "Shard key is empty", !keyPattern.isEmpty());
+
     std::vector<std::unique_ptr<FieldRef>> parsedPaths;
 
     for (const auto& patternEl : keyPattern) {
         auto newFieldRef(stdx::make_unique<FieldRef>(patternEl.fieldNameStringData()));
 
         // Empty path
-        if (newFieldRef->numParts() == 0)
-            return {};
+        uassert(ErrorCodes::BadValue,
+                str::stream() << "Field " << patternEl.fieldNameStringData() << " is empty",
+                newFieldRef->numParts() > 0);
 
         // Extra "." in path?
-        if (newFieldRef->dottedField() != patternEl.fieldNameStringData())
-            return {};
+        uassert(ErrorCodes::BadValue,
+                str::stream() << "Field " << patternEl.fieldNameStringData()
+                              << " contains extra dot",
+                newFieldRef->dottedField() == patternEl.fieldNameStringData());
 
         // Empty parts of the path, ".."?
         for (size_t i = 0; i < newFieldRef->numParts(); ++i) {
-            if (newFieldRef->getPart(i).empty())
-                return {};
+            uassert(ErrorCodes::BadValue,
+                    str::stream() << "Field " << patternEl.fieldNameStringData()
+                                  << " contains empty parts",
+                    !newFieldRef->getPart(i).empty());
         }
 
         // Numeric and ascending (1.0), or "hashed" and single field
-        if (!patternEl.isNumber()) {
-            if (keyPattern.nFields() != 1 || !isHashedPatternEl(patternEl))
-                return {};
-        } else if (patternEl.numberInt() != 1) {
-            return {};
-        }
+        uassert(ErrorCodes::BadValue,
+                str::stream() << "Field " << patternEl.fieldNameStringData()
+                              << " can only be 1 or 'hashed'",
+                (patternEl.isNumber() && patternEl.numberInt() == 1) ||
+                    (keyPattern.nFields() == 1 && isHashedPatternEl(patternEl)));
 
         parsedPaths.emplace_back(std::move(newFieldRef));
     }
@@ -129,17 +135,13 @@ Status ShardKeyPattern::checkShardKeySize(const BSONObj& shardKey) {
 }
 
 ShardKeyPattern::ShardKeyPattern(const BSONObj& keyPattern)
-    : _keyPatternPaths(parseShardKeyPattern(keyPattern)),
-      _keyPattern(_keyPatternPaths.empty() ? BSONObj() : keyPattern),
+    : _keyPattern(keyPattern),
+      _keyPatternPaths(parseShardKeyPattern(keyPattern)),
       _hasId(keyPattern.hasField("_id"_sd)) {}
 
 ShardKeyPattern::ShardKeyPattern(const KeyPattern& keyPattern)
     : ShardKeyPattern(keyPattern.toBSON()) {}
 
-bool ShardKeyPattern::isValid() const {
-    return !_keyPattern.toBSON().isEmpty();
-}
-
 bool ShardKeyPattern::isHashedPattern() const {
     return isHashedPatternEl(_keyPattern.toBSON().firstElement());
 }
@@ -161,11 +163,6 @@ string ShardKeyPattern::toString() const {
 }
 
 bool ShardKeyPattern::isShardKey(const BSONObj& shardKey) const {
-    // Shard keys are always of the form: { 'nested.path' : value, 'nested.path2' : value }
-
-    if (!isValid())
-        return false;
-
     const auto& keyPatternBSON = _keyPattern.toBSON();
 
     for (const auto& patternEl : keyPatternBSON) {
@@ -179,12 +176,6 @@ bool ShardKeyPattern::isShardKey(const BSONObj& shardKey) const {
 }
 
 BSONObj ShardKeyPattern::normalizeShardKey(const BSONObj& shardKey) const {
-    // Shard keys are always of the form: { 'nested.path' : value, 'nested.path2' : value }
-    // and in the same order as the key pattern
-
-    if (!isValid())
-        return BSONObj();
-
     // We want to return an empty key if users pass us something that's not a shard key
     if (shardKey.nFields() > _keyPattern.toBSON().nFields())
         return BSONObj();
@@ -225,9 +216,6 @@ static BSONElement extractKeyElementFromMatchable(const MatchableDocument& match
 }
 
 BSONObj ShardKeyPattern::extractShardKeyFromMatchable(const MatchableDocument& matchable) const {
-    if (!isValid())
-        return BSONObj();
-
     BSONObjBuilder keyBuilder;
 
     BSONObjIterator patternIt(_keyPattern.toBSON());
@@ -277,9 +265,6 @@ static BSONElement findEqualityElement(const EqualityMatches& equalities, const
 
 StatusWith<BSONObj> ShardKeyPattern::extractShardKeyFromQuery(OperationContext* opCtx,
                                                               const BSONObj& basicQuery) const {
-    if (!isValid())
-        return StatusWith<BSONObj>(BSONObj());
-
     auto qr = stdx::make_unique<QueryRequest>(NamespaceString(""));
     qr->setFilter(basicQuery);
 
@@ -299,9 +284,6 @@ StatusWith<BSONObj> ShardKeyPattern::extractShardKeyFromQuery(OperationContext*
 }
 
 BSONObj ShardKeyPattern::extractShardKeyFromQuery(const CanonicalQuery& query) const {
-    if (!isValid())
-        return BSONObj();
-
     // Extract equalities from query.
     EqualityMatches equalities;
     // TODO: Build the path set initially?
diff --git a/src/mongo/s/shard_key_pattern.h b/src/mongo/s/shard_key_pattern.h
index 1ceed1a8147..b37eb6ff09a 100644
--- a/src/mongo/s/shard_key_pattern.h
+++ b/src/mongo/s/shard_key_pattern.h
@@ -87,8 +87,6 @@ public:
      */
     explicit ShardKeyPattern(const KeyPattern& keyPattern);
 
-    bool isValid() const;
-
     bool isHashedPattern() const;
 
     const KeyPattern& getKeyPattern() const;
@@ -233,11 +231,11 @@ public:
     };
 
 private:
+    KeyPattern _keyPattern;
+
     // Ordered, parsed paths
     std::vector<std::unique_ptr<FieldRef>> _keyPatternPaths;
 
-    KeyPattern _keyPattern;
-
     bool _hasId;
 };
 
diff --git a/src/mongo/s/shard_key_pattern_test.cpp b/src/mongo/s/shard_key_pattern_test.cpp
index 2b4ddb22fae..18bbe3233f5 100644
--- a/src/mongo/s/shard_key_pattern_test.cpp
+++ b/src/mongo/s/shard_key_pattern_test.cpp
@@ -25,6 +25,8 @@
  *    then also delete it in the license file.
  */
 
+#include "mongo/platform/basic.h"
+
 #include "mongo/s/shard_key_pattern.h"
 
 #include "mongo/db/hasher.h"
@@ -32,78 +34,57 @@
 #include "mongo/db/query/query_test_service_context.h"
 #include "mongo/unittest/unittest.h"
 
+namespace mongo {
 namespace {
 
 using std::string;
 
-using namespace mongo;
-
-TEST(ShardKeyPattern, ValidShardKeyPatternSingle) {
-    BSONObj empty;
-    ASSERT(!ShardKeyPattern(empty).isValid());
-
-    //
-    // Single field ShardKeyPatterns
-    //
-
-    ASSERT(ShardKeyPattern(BSON("a" << 1)).isValid());
-    ASSERT(ShardKeyPattern(BSON("a" << 1)).isValid());
-    ASSERT(ShardKeyPattern(BSON("a" << 1.0f)).isValid());
-    ASSERT(ShardKeyPattern(BSON("a" << (long long)1L)).isValid());
-
-    ASSERT(!ShardKeyPattern(BSON("a" << -1)).isValid());
-    ASSERT(!ShardKeyPattern(BSON("a" << -1.0)).isValid());
-    ASSERT(!ShardKeyPattern(BSON("a"
-                                 << "1"))
-                .isValid());
-
-    ASSERT(ShardKeyPattern(BSON("a"
-                                << "hashed"))
-               .isValid());
-    ASSERT(!ShardKeyPattern(BSON("a"
-                                 << "hash"))
-                .isValid());
-    ASSERT(!ShardKeyPattern(BSON("" << 1)).isValid());
-    ASSERT(!ShardKeyPattern(BSON("." << 1)).isValid());
+TEST(ShardKeyPattern, SingleFieldShardKeyPatternsValidityCheck) {
+    ShardKeyPattern(BSON("a" << 1));
+    ShardKeyPattern(BSON("a" << 1.0f));
+    ShardKeyPattern(BSON("a" << (long long)1L));
+    ShardKeyPattern(BSON("a"
+                         << "hashed"));
+
+    ASSERT_THROWS(ShardKeyPattern({}), DBException);
+    ASSERT_THROWS(ShardKeyPattern(BSON("a" << -1)), DBException);
+    ASSERT_THROWS(ShardKeyPattern(BSON("a" << -1.0)), DBException);
+    ASSERT_THROWS(ShardKeyPattern(BSON("a"
+                                       << "1")),
+                  DBException);
+    ASSERT_THROWS(ShardKeyPattern(BSON("a"
+                                       << "hash")),
+                  DBException);
+    ASSERT_THROWS(ShardKeyPattern(BSON("" << 1)), DBException);
+    ASSERT_THROWS(ShardKeyPattern(BSON("." << 1)), DBException);
 }
 
-TEST(ShardKeyPattern, ValidShardKeyPatternComposite) {
-    //
-    // Composite ShardKeyPatterns
-    //
-
-    ASSERT(ShardKeyPattern(BSON("a" << 1 << "b" << 1)).isValid());
-    ASSERT(ShardKeyPattern(BSON("a" << 1.0f << "b" << 1.0)).isValid());
-    ASSERT(!ShardKeyPattern(BSON("a" << 1 << "b" << -1)).isValid());
-    ASSERT(!ShardKeyPattern(BSON("a" << 1 << "b"
-                                     << "1"))
-                .isValid());
-
-    ASSERT(ShardKeyPattern(BSON("a" << 1 << "b" << 1.0 << "c" << 1.0f)).isValid());
-    ASSERT(!ShardKeyPattern(BSON("a" << 1 << "b." << 1.0)).isValid());
-    ASSERT(!ShardKeyPattern(BSON("a" << 1 << "" << 1.0)).isValid());
+TEST(ShardKeyPattern, CompositeShardKeyPatternsValidityCheck) {
+    ShardKeyPattern(BSON("a" << 1 << "b" << 1));
+    ShardKeyPattern(BSON("a" << 1.0f << "b" << 1.0));
+    ShardKeyPattern(BSON("a" << 1 << "b" << 1.0 << "c" << 1.0f));
+
+    ASSERT_THROWS(ShardKeyPattern(BSON("a" << 1 << "b" << -1)), DBException);
+    ASSERT_THROWS(ShardKeyPattern(BSON("a" << 1 << "b"
+                                           << "1")),
+                  DBException);
+    ASSERT_THROWS(ShardKeyPattern(BSON("a" << 1 << "b." << 1.0)), DBException);
+    ASSERT_THROWS(ShardKeyPattern(BSON("a" << 1 << "" << 1.0)), DBException);
 }
 
-TEST(ShardKeyPattern, ValidShardKeyPatternNested) {
-    //
-    // Nested ShardKeyPatterns
-    //
-
-    ASSERT(ShardKeyPattern(BSON("a.b" << 1)).isValid());
-    ASSERT(!ShardKeyPattern(BSON("a.b" << -1)).isValid());
-    ASSERT(ShardKeyPattern(BSON("a.b.c.d" << 1.0)).isValid());
-
-    ASSERT(!ShardKeyPattern(BSON("a" << BSON("b" << 1))).isValid());
-
-    ASSERT(!ShardKeyPattern(BSON("a.b." << 1)).isValid());
-    ASSERT(!ShardKeyPattern(BSON("a.b.." << 1)).isValid());
-    ASSERT(!ShardKeyPattern(BSON("a..b" << 1)).isValid());
-
-    ASSERT(ShardKeyPattern(BSON("a" << 1 << "c.d" << 1.0 << "e.f.g" << 1.0f)).isValid());
-    ASSERT(ShardKeyPattern(BSON("a" << 1 << "a.b" << 1.0 << "a.b.c" << 1.0f)).isValid());
-
-    ASSERT(!ShardKeyPattern(BSON("a" << 1 << "a.b." << 1.0)).isValid());
-    ASSERT(!ShardKeyPattern(BSON("a" << BSON("b" << 1) << "c.d" << 1.0)).isValid());
+TEST(ShardKeyPattern, NestedShardKeyPatternsValidtyCheck) {
+    ShardKeyPattern(BSON("a.b" << 1));
+    ShardKeyPattern(BSON("a.b.c.d" << 1.0));
+    ShardKeyPattern(BSON("a" << 1 << "c.d" << 1.0 << "e.f.g" << 1.0f));
+    ShardKeyPattern(BSON("a" << 1 << "a.b" << 1.0 << "a.b.c" << 1.0f));
+
+    ASSERT_THROWS(ShardKeyPattern(BSON("a.b" << -1)), DBException);
+    ASSERT_THROWS(ShardKeyPattern(BSON("a" << BSON("b" << 1))), DBException);
+    ASSERT_THROWS(ShardKeyPattern(BSON("a.b." << 1)), DBException);
+    ASSERT_THROWS(ShardKeyPattern(BSON("a.b.." << 1)), DBException);
+    ASSERT_THROWS(ShardKeyPattern(BSON("a..b" << 1)), DBException);
+    ASSERT_THROWS(ShardKeyPattern(BSON("a" << 1 << "a.b." << 1.0)), DBException);
+    ASSERT_THROWS(ShardKeyPattern(BSON("a" << BSON("b" << 1) << "c.d" << 1.0)), DBException);
 }
 
 TEST(ShardKeyPattern, IsShardKey) {
@@ -501,4 +482,6 @@ TEST(ShardKeyPattern, UniqueIndexCompatibleHashed) {
     ASSERT(!indexComp(pattern, BSON("c" << 1)));
     ASSERT(!indexComp(pattern, BSON("c" << -1 << "a.b" << 1)));
 }
-}
+
+}  // namespace
+}  // namespace mongo