Skip to content

Commit b8485e1

Browse files
kaloianmkaloianm
committed
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.
1 parent 5156e62 commit b8485e1

File tree

5 files changed

+70
-117
lines changed

5 files changed

+70
-117
lines changed

‎src/mongo/db/dbhelpers.cpp‎

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -54,14 +54,12 @@
5454
#include "mongo/db/query/query_planner.h"
5555
#include "mongo/db/repl/repl_client_info.h"
5656
#include "mongo/db/repl/replication_coordinator.h"
57-
#include "mongo/db/s/sharding_state.h"
5857
#include "mongo/db/service_context.h"
5958
#include "mongo/db/storage/data_protector.h"
6059
#include "mongo/db/storage/encryption_hooks.h"
6160
#include "mongo/db/storage/storage_options.h"
6261
#include "mongo/db/write_concern.h"
6362
#include "mongo/db/write_concern_options.h"
64-
#include "mongo/s/shard_key_pattern.h"
6563
#include "mongo/util/log.h"
6664
#include "mongo/util/mongoutils/str.h"
6765
#include "mongo/util/scopeguard.h"

‎src/mongo/db/s/config/configsvr_shard_collection_command.cpp‎

Lines changed: 0 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -140,14 +140,6 @@ void validateAndDeduceFullRequestOptions(OperationContext* opCtx,
140140
uassert(
141141
ErrorCodes::InvalidOptions, "cannot have empty shard key", !request->getKey().isEmpty());
142142

143-
// Ensure the proposed shard key is valid.
144-
uassert(ErrorCodes::InvalidOptions,
145-
str::stream()
146-
<< "Unsupported shard key pattern "
147-
<< shardKeyPattern.toString()
148-
<< ". Pattern must either be a single hashed field, or a list of ascending fields",
149-
shardKeyPattern.isValid());
150-
151143
// Ensure that hashed and unique are not both set.
152144
uassert(ErrorCodes::InvalidOptions,
153145
"Hashed shard keys cannot be declared unique. It's possible to ensure uniqueness on "

‎src/mongo/s/shard_key_pattern.cpp‎

Lines changed: 20 additions & 38 deletions
Original file line numberDiff line numberDiff line change
@@ -66,32 +66,38 @@ bool isHashedPatternEl(const BSONElement& el) {
6666
* ii) a compound list of ascending, potentially-nested field paths, e.g. { a : 1 , b.c : 1 }
6767
*/
6868
std::vector<std::unique_ptr<FieldRef>> parseShardKeyPattern(const BSONObj& keyPattern) {
69+
uassert(ErrorCodes::BadValue, "Shard key is empty", !keyPattern.isEmpty());
70+
6971
std::vector<std::unique_ptr<FieldRef>> parsedPaths;
7072

7173
for (const auto& patternEl : keyPattern) {
7274
auto newFieldRef(stdx::make_unique<FieldRef>(patternEl.fieldNameStringData()));
7375

7476
// Empty path
75-
if (newFieldRef->numParts() == 0)
76-
return {};
77+
uassert(ErrorCodes::BadValue,
78+
str::stream() << "Field " << patternEl.fieldNameStringData() << " is empty",
79+
newFieldRef->numParts() > 0);
7780

7881
// Extra "." in path?
79-
if (newFieldRef->dottedField() != patternEl.fieldNameStringData())
80-
return {};
82+
uassert(ErrorCodes::BadValue,
83+
str::stream() << "Field " << patternEl.fieldNameStringData()
84+
<< " contains extra dot",
85+
newFieldRef->dottedField() == patternEl.fieldNameStringData());
8186

8287
// Empty parts of the path, ".."?
8388
for (size_t i = 0; i < newFieldRef->numParts(); ++i) {
84-
if (newFieldRef->getPart(i).empty())
85-
return {};
89+
uassert(ErrorCodes::BadValue,
90+
str::stream() << "Field " << patternEl.fieldNameStringData()
91+
<< " contains empty parts",
92+
!newFieldRef->getPart(i).empty());
8693
}
8794

8895
// Numeric and ascending (1.0), or "hashed" and single field
89-
if (!patternEl.isNumber()) {
90-
if (keyPattern.nFields() != 1 || !isHashedPatternEl(patternEl))
91-
return {};
92-
} else if (patternEl.numberInt() != 1) {
93-
return {};
94-
}
96+
uassert(ErrorCodes::BadValue,
97+
str::stream() << "Field " << patternEl.fieldNameStringData()
98+
<< " can only be 1 or 'hashed'",
99+
(patternEl.isNumber() && patternEl.numberInt() == 1) ||
100+
(keyPattern.nFields() == 1 && isHashedPatternEl(patternEl)));
95101

96102
parsedPaths.emplace_back(std::move(newFieldRef));
97103
}
@@ -129,17 +135,13 @@ Status ShardKeyPattern::checkShardKeySize(const BSONObj& shardKey) {
129135
}
130136

131137
ShardKeyPattern::ShardKeyPattern(const BSONObj& keyPattern)
132-
: _keyPatternPaths(parseShardKeyPattern(keyPattern)),
133-
_keyPattern(_keyPatternPaths.empty() ? BSONObj() : keyPattern),
138+
: _keyPattern(keyPattern),
139+
_keyPatternPaths(parseShardKeyPattern(keyPattern)),
134140
_hasId(keyPattern.hasField("_id"_sd)) {}
135141

136142
ShardKeyPattern::ShardKeyPattern(const KeyPattern& keyPattern)
137143
: ShardKeyPattern(keyPattern.toBSON()) {}
138144

139-
bool ShardKeyPattern::isValid() const {
140-
return !_keyPattern.toBSON().isEmpty();
141-
}
142-
143145
bool ShardKeyPattern::isHashedPattern() const {
144146
return isHashedPatternEl(_keyPattern.toBSON().firstElement());
145147
}
@@ -161,11 +163,6 @@ string ShardKeyPattern::toString() const {
161163
}
162164

163165
bool ShardKeyPattern::isShardKey(const BSONObj& shardKey) const {
164-
// Shard keys are always of the form: { 'nested.path' : value, 'nested.path2' : value }
165-
166-
if (!isValid())
167-
return false;
168-
169166
const auto& keyPatternBSON = _keyPattern.toBSON();
170167

171168
for (const auto& patternEl : keyPatternBSON) {
@@ -179,12 +176,6 @@ bool ShardKeyPattern::isShardKey(const BSONObj& shardKey) const {
179176
}
180177

181178
BSONObj ShardKeyPattern::normalizeShardKey(const BSONObj& shardKey) const {
182-
// Shard keys are always of the form: { 'nested.path' : value, 'nested.path2' : value }
183-
// and in the same order as the key pattern
184-
185-
if (!isValid())
186-
return BSONObj();
187-
188179
// We want to return an empty key if users pass us something that's not a shard key
189180
if (shardKey.nFields() > _keyPattern.toBSON().nFields())
190181
return BSONObj();
@@ -225,9 +216,6 @@ static BSONElement extractKeyElementFromMatchable(const MatchableDocument& match
225216
}
226217

227218
BSONObj ShardKeyPattern::extractShardKeyFromMatchable(const MatchableDocument& matchable) const {
228-
if (!isValid())
229-
return BSONObj();
230-
231219
BSONObjBuilder keyBuilder;
232220

233221
BSONObjIterator patternIt(_keyPattern.toBSON());
@@ -277,9 +265,6 @@ static BSONElement findEqualityElement(const EqualityMatches& equalities, const
277265

278266
StatusWith<BSONObj> ShardKeyPattern::extractShardKeyFromQuery(OperationContext* opCtx,
279267
const BSONObj& basicQuery) const {
280-
if (!isValid())
281-
return StatusWith<BSONObj>(BSONObj());
282-
283268
auto qr = stdx::make_unique<QueryRequest>(NamespaceString(""));
284269
qr->setFilter(basicQuery);
285270

@@ -299,9 +284,6 @@ StatusWith<BSONObj> ShardKeyPattern::extractShardKeyFromQuery(OperationContext*
299284
}
300285

301286
BSONObj ShardKeyPattern::extractShardKeyFromQuery(const CanonicalQuery& query) const {
302-
if (!isValid())
303-
return BSONObj();
304-
305287
// Extract equalities from query.
306288
EqualityMatches equalities;
307289
// TODO: Build the path set initially?

‎src/mongo/s/shard_key_pattern.h‎

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -87,8 +87,6 @@ class ShardKeyPattern {
8787
*/
8888
explicit ShardKeyPattern(const KeyPattern& keyPattern);
8989

90-
bool isValid() const;
91-
9290
bool isHashedPattern() const;
9391

9492
const KeyPattern& getKeyPattern() const;
@@ -233,11 +231,11 @@ class ShardKeyPattern {
233231
};
234232

235233
private:
234+
KeyPattern _keyPattern;
235+
236236
// Ordered, parsed paths
237237
std::vector<std::unique_ptr<FieldRef>> _keyPatternPaths;
238238

239-
KeyPattern _keyPattern;
240-
241239
bool _hasId;
242240
};
243241

‎src/mongo/s/shard_key_pattern_test.cpp‎

Lines changed: 48 additions & 65 deletions
Original file line numberDiff line numberDiff line change
@@ -25,85 +25,66 @@
2525
* then also delete it in the license file.
2626
*/
2727

28+
#include "mongo/platform/basic.h"
29+
2830
#include "mongo/s/shard_key_pattern.h"
2931

3032
#include "mongo/db/hasher.h"
3133
#include "mongo/db/json.h"
3234
#include "mongo/db/query/query_test_service_context.h"
3335
#include "mongo/unittest/unittest.h"
3436

37+
namespace mongo {
3538
namespace {
3639

3740
using std::string;
3841

39-
using namespace mongo;
40-
41-
TEST(ShardKeyPattern, ValidShardKeyPatternSingle) {
42-
BSONObj empty;
43-
ASSERT(!ShardKeyPattern(empty).isValid());
44-
45-
//
46-
// Single field ShardKeyPatterns
47-
//
48-
49-
ASSERT(ShardKeyPattern(BSON("a" << 1)).isValid());
50-
ASSERT(ShardKeyPattern(BSON("a" << 1)).isValid());
51-
ASSERT(ShardKeyPattern(BSON("a" << 1.0f)).isValid());
52-
ASSERT(ShardKeyPattern(BSON("a" << (long long)1L)).isValid());
53-
54-
ASSERT(!ShardKeyPattern(BSON("a" << -1)).isValid());
55-
ASSERT(!ShardKeyPattern(BSON("a" << -1.0)).isValid());
56-
ASSERT(!ShardKeyPattern(BSON("a"
57-
<< "1"))
58-
.isValid());
59-
60-
ASSERT(ShardKeyPattern(BSON("a"
61-
<< "hashed"))
62-
.isValid());
63-
ASSERT(!ShardKeyPattern(BSON("a"
64-
<< "hash"))
65-
.isValid());
66-
ASSERT(!ShardKeyPattern(BSON("" << 1)).isValid());
67-
ASSERT(!ShardKeyPattern(BSON("." << 1)).isValid());
42+
TEST(ShardKeyPattern, SingleFieldShardKeyPatternsValidityCheck) {
43+
ShardKeyPattern(BSON("a" << 1));
44+
ShardKeyPattern(BSON("a" << 1.0f));
45+
ShardKeyPattern(BSON("a" << (long long)1L));
46+
ShardKeyPattern(BSON("a"
47+
<< "hashed"));
48+
49+
ASSERT_THROWS(ShardKeyPattern({}), DBException);
50+
ASSERT_THROWS(ShardKeyPattern(BSON("a" << -1)), DBException);
51+
ASSERT_THROWS(ShardKeyPattern(BSON("a" << -1.0)), DBException);
52+
ASSERT_THROWS(ShardKeyPattern(BSON("a"
53+
<< "1")),
54+
DBException);
55+
ASSERT_THROWS(ShardKeyPattern(BSON("a"
56+
<< "hash")),
57+
DBException);
58+
ASSERT_THROWS(ShardKeyPattern(BSON("" << 1)), DBException);
59+
ASSERT_THROWS(ShardKeyPattern(BSON("." << 1)), DBException);
6860
}
6961

70-
TEST(ShardKeyPattern, ValidShardKeyPatternComposite) {
71-
//
72-
// Composite ShardKeyPatterns
73-
//
74-
75-
ASSERT(ShardKeyPattern(BSON("a" << 1 << "b" << 1)).isValid());
76-
ASSERT(ShardKeyPattern(BSON("a" << 1.0f << "b" << 1.0)).isValid());
77-
ASSERT(!ShardKeyPattern(BSON("a" << 1 << "b" << -1)).isValid());
78-
ASSERT(!ShardKeyPattern(BSON("a" << 1 << "b"
79-
<< "1"))
80-
.isValid());
81-
82-
ASSERT(ShardKeyPattern(BSON("a" << 1 << "b" << 1.0 << "c" << 1.0f)).isValid());
83-
ASSERT(!ShardKeyPattern(BSON("a" << 1 << "b." << 1.0)).isValid());
84-
ASSERT(!ShardKeyPattern(BSON("a" << 1 << "" << 1.0)).isValid());
62+
TEST(ShardKeyPattern, CompositeShardKeyPatternsValidityCheck) {
63+
ShardKeyPattern(BSON("a" << 1 << "b" << 1));
64+
ShardKeyPattern(BSON("a" << 1.0f << "b" << 1.0));
65+
ShardKeyPattern(BSON("a" << 1 << "b" << 1.0 << "c" << 1.0f));
66+
67+
ASSERT_THROWS(ShardKeyPattern(BSON("a" << 1 << "b" << -1)), DBException);
68+
ASSERT_THROWS(ShardKeyPattern(BSON("a" << 1 << "b"
69+
<< "1")),
70+
DBException);
71+
ASSERT_THROWS(ShardKeyPattern(BSON("a" << 1 << "b." << 1.0)), DBException);
72+
ASSERT_THROWS(ShardKeyPattern(BSON("a" << 1 << "" << 1.0)), DBException);
8573
}
8674

87-
TEST(ShardKeyPattern, ValidShardKeyPatternNested) {
88-
//
89-
// Nested ShardKeyPatterns
90-
//
91-
92-
ASSERT(ShardKeyPattern(BSON("a.b" << 1)).isValid());
93-
ASSERT(!ShardKeyPattern(BSON("a.b" << -1)).isValid());
94-
ASSERT(ShardKeyPattern(BSON("a.b.c.d" << 1.0)).isValid());
95-
96-
ASSERT(!ShardKeyPattern(BSON("a" << BSON("b" << 1))).isValid());
97-
98-
ASSERT(!ShardKeyPattern(BSON("a.b." << 1)).isValid());
99-
ASSERT(!ShardKeyPattern(BSON("a.b.." << 1)).isValid());
100-
ASSERT(!ShardKeyPattern(BSON("a..b" << 1)).isValid());
101-
102-
ASSERT(ShardKeyPattern(BSON("a" << 1 << "c.d" << 1.0 << "e.f.g" << 1.0f)).isValid());
103-
ASSERT(ShardKeyPattern(BSON("a" << 1 << "a.b" << 1.0 << "a.b.c" << 1.0f)).isValid());
104-
105-
ASSERT(!ShardKeyPattern(BSON("a" << 1 << "a.b." << 1.0)).isValid());
106-
ASSERT(!ShardKeyPattern(BSON("a" << BSON("b" << 1) << "c.d" << 1.0)).isValid());
75+
TEST(ShardKeyPattern, NestedShardKeyPatternsValidtyCheck) {
76+
ShardKeyPattern(BSON("a.b" << 1));
77+
ShardKeyPattern(BSON("a.b.c.d" << 1.0));
78+
ShardKeyPattern(BSON("a" << 1 << "c.d" << 1.0 << "e.f.g" << 1.0f));
79+
ShardKeyPattern(BSON("a" << 1 << "a.b" << 1.0 << "a.b.c" << 1.0f));
80+
81+
ASSERT_THROWS(ShardKeyPattern(BSON("a.b" << -1)), DBException);
82+
ASSERT_THROWS(ShardKeyPattern(BSON("a" << BSON("b" << 1))), DBException);
83+
ASSERT_THROWS(ShardKeyPattern(BSON("a.b." << 1)), DBException);
84+
ASSERT_THROWS(ShardKeyPattern(BSON("a.b.." << 1)), DBException);
85+
ASSERT_THROWS(ShardKeyPattern(BSON("a..b" << 1)), DBException);
86+
ASSERT_THROWS(ShardKeyPattern(BSON("a" << 1 << "a.b." << 1.0)), DBException);
87+
ASSERT_THROWS(ShardKeyPattern(BSON("a" << BSON("b" << 1) << "c.d" << 1.0)), DBException);
10788
}
10889

10990
TEST(ShardKeyPattern, IsShardKey) {
@@ -501,4 +482,6 @@ TEST(ShardKeyPattern, UniqueIndexCompatibleHashed) {
501482
ASSERT(!indexComp(pattern, BSON("c" << 1)));
502483
ASSERT(!indexComp(pattern, BSON("c" << -1 << "a.b" << 1)));
503484
}
504-
}
485+
486+
} // namespace
487+
} // namespace mongo

0 commit comments

Comments
 (0)