From 6d8fb7f9912873c6ff34785c55094079cf364126 Mon Sep 17 00:00:00 2001 From: Mathias Stearn Date: Tue, 24 Feb 2015 18:52:57 -0500 Subject: [PATCH] SERVER-17370 Fix bugs in storage engine-specific index and collection options --- jstests/core/apitest_db.js | 7 ++- src/mongo/db/catalog/collection_options.cpp | 10 +--- .../db/catalog/collection_options_test.cpp | 8 ++- src/mongo/db/storage/devnull/devnull_init.cpp | 9 ---- .../db/storage/in_memory/in_memory_init.cpp | 9 ---- src/mongo/db/storage/mmap_v1/mmap_v1_init.cpp | 9 ---- src/mongo/db/storage/storage_engine.h | 19 ++++++- .../storage/wiredtiger/wiredtiger_index.cpp | 10 ++-- .../wiredtiger/wiredtiger_index_test.cpp | 43 +++++++-------- .../db/storage/wiredtiger/wiredtiger_init.cpp | 2 +- .../wiredtiger/wiredtiger_record_store.cpp | 54 ++++++++++--------- .../wiredtiger/wiredtiger_record_store.h | 7 +++ .../wiredtiger_record_store_test.cpp | 37 ++++++++++--- 13 files changed, 114 insertions(+), 110 deletions(-) diff --git a/jstests/core/apitest_db.js b/jstests/core/apitest_db.js index f12f44c1aed..70975ea7c84 100644 --- a/jstests/core/apitest_db.js +++ b/jstests/core/apitest_db.js @@ -40,10 +40,13 @@ assert(found, "found test.test in collection infos"); // storageEngine in collection options must: // - be a document -// - contain at least one field of document type with the name of a registered storage engine. +// - all fields of the document: +// -- must have names that are registered storage engines +// -- must be objects db.getCollection('test').drop(); var storageEngineName = db.serverStatus().storageEngine.name; -assert.commandFailed(db.createCollection('test', {storageEngine: {}})); +assert.commandFailed(db.createCollection('test', {storageEngine: 'not a document'})); +assert.commandWorked(db.createCollection('test', {storageEngine: {}})); assert.commandFailed(db.createCollection('test', {storageEngine: {unknownStorageEngine: {}}})); var invalidStorageEngineOptions = {} invalidStorageEngineOptions[storageEngineName] = 12345; diff --git a/src/mongo/db/catalog/collection_options.cpp b/src/mongo/db/catalog/collection_options.cpp index ba2a3ff5b6e..1efdf1b737c 100644 --- a/src/mongo/db/catalog/collection_options.cpp +++ b/src/mongo/db/catalog/collection_options.cpp @@ -144,16 +144,8 @@ namespace mongo { if (e.type() != mongo::Object) { return Status(ErrorCodes::BadValue, "'storageEngine' has to be a document."); } - BSONObjIterator j(e.Obj()); - if (!j.more()) { - return Status(ErrorCodes::BadValue, - "Empty 'storageEngine' options are invalid. " - "Please remove, or include valid options."); - } - // Loop through each provided storageEngine. - while (j.more()) { - BSONElement storageEngineElement = j.next(); + BSONForEach(storageEngineElement, e.Obj()) { StringData storageEngineName = storageEngineElement.fieldNameStringData(); if (storageEngineElement.type() != mongo::Object) { return Status(ErrorCodes::BadValue, str::stream() << "'storageEngine." << diff --git a/src/mongo/db/catalog/collection_options_test.cpp b/src/mongo/db/catalog/collection_options_test.cpp index 4bb94af2478..d0d1086fea0 100644 --- a/src/mongo/db/catalog/collection_options_test.cpp +++ b/src/mongo/db/catalog/collection_options_test.cpp @@ -87,15 +87,13 @@ namespace mongo { TEST(CollectionOptions, InvalidStorageEngineField) { // "storageEngine" field has to be an object if present. - ASSERT_NOT_OK( CollectionOptions().parse(fromjson("{storageEngine: 1}"))); + ASSERT_NOT_OK(CollectionOptions().parse(fromjson("{storageEngine: 1}"))); // Every field under "storageEngine" has to be an object. - ASSERT_NOT_OK( CollectionOptions().parse(fromjson( - "{storageEngine: {storageEngine1: 1}}"))); + ASSERT_NOT_OK(CollectionOptions().parse(fromjson("{storageEngine: {storageEngine1: 1}}"))); // Empty "storageEngine" not allowed - ASSERT_NOT_OK( CollectionOptions().parse(fromjson( - "{storageEngine: {}}"))); + ASSERT_OK(CollectionOptions().parse(fromjson("{storageEngine: {}}"))); } TEST(CollectionOptions, ParseEngineField) { diff --git a/src/mongo/db/storage/devnull/devnull_init.cpp b/src/mongo/db/storage/devnull/devnull_init.cpp index 30478bafa28..604df977d94 100644 --- a/src/mongo/db/storage/devnull/devnull_init.cpp +++ b/src/mongo/db/storage/devnull/devnull_init.cpp @@ -53,15 +53,6 @@ namespace mongo { return "devnull"; } - virtual Status validateCollectionStorageOptions(const BSONObj& options) const - { - return Status::OK(); - } - - virtual Status validateIndexStorageOptions(const BSONObj& options) const { - return Status::OK(); - } - virtual Status validateMetadata(const StorageEngineMetadata& metadata, const StorageGlobalParams& params) const { return Status::OK(); diff --git a/src/mongo/db/storage/in_memory/in_memory_init.cpp b/src/mongo/db/storage/in_memory/in_memory_init.cpp index d026e5b974c..639c017dee1 100644 --- a/src/mongo/db/storage/in_memory/in_memory_init.cpp +++ b/src/mongo/db/storage/in_memory/in_memory_init.cpp @@ -54,15 +54,6 @@ namespace mongo { return "inMemoryExperiment"; } - virtual Status validateCollectionStorageOptions(const BSONObj& options) const - { - return Status::OK(); - } - - virtual Status validateIndexStorageOptions(const BSONObj& options) const { - return Status::OK(); - } - virtual Status validateMetadata(const StorageEngineMetadata& metadata, const StorageGlobalParams& params) const { return Status::OK(); diff --git a/src/mongo/db/storage/mmap_v1/mmap_v1_init.cpp b/src/mongo/db/storage/mmap_v1/mmap_v1_init.cpp index 40a2b82edfa..c16bf35cd1b 100644 --- a/src/mongo/db/storage/mmap_v1/mmap_v1_init.cpp +++ b/src/mongo/db/storage/mmap_v1/mmap_v1_init.cpp @@ -52,15 +52,6 @@ namespace mongo { return "mmapv1"; } - virtual Status validateCollectionStorageOptions(const BSONObj& options) const - { - return Status::OK(); - } - - virtual Status validateIndexStorageOptions(const BSONObj& options) const { - return Status::OK(); - } - virtual Status validateMetadata(const StorageEngineMetadata& metadata, const StorageGlobalParams& params) const { Status status = metadata.validateStorageEngineOption( diff --git a/src/mongo/db/storage/storage_engine.h b/src/mongo/db/storage/storage_engine.h index 7f1cd2140db..3534a8ac9ef 100644 --- a/src/mongo/db/storage/storage_engine.h +++ b/src/mongo/db/storage/storage_engine.h @@ -35,6 +35,7 @@ #include "mongo/base/status.h" #include "mongo/bson/bsonobj.h" +#include "mongo/util/mongoutils/str.h" namespace mongo { @@ -81,14 +82,28 @@ namespace mongo { /** * Validates creation options for a collection in the StorageEngine. * Returns an error if the creation options are not valid. + * + * Default implementation only accepts empty objects (no options). */ - virtual Status validateCollectionStorageOptions(const BSONObj& options) const = 0; + virtual Status validateCollectionStorageOptions(const BSONObj& options) const { + if (options.isEmpty()) return Status::OK(); + return Status(ErrorCodes::InvalidOptions, + str::stream() << "storage engine " << getCanonicalName() + << " does not support any collection storage options"); + } /** * Validates creation options for an index in the StorageEngine. * Returns an error if the creation options are not valid. + * + * Default implementation only accepts empty objects (no options). */ - virtual Status validateIndexStorageOptions(const BSONObj& options) const = 0; + virtual Status validateIndexStorageOptions(const BSONObj& options) const { + if (options.isEmpty()) return Status::OK(); + return Status(ErrorCodes::InvalidOptions, + str::stream() << "storage engine " << getCanonicalName() + << " does not support any index storage options"); + } /** * Validates existing metadata in the data directory against startup options. diff --git a/src/mongo/db/storage/wiredtiger/wiredtiger_index.cpp b/src/mongo/db/storage/wiredtiger/wiredtiger_index.cpp index c86883ea00a..4293a183991 100644 --- a/src/mongo/db/storage/wiredtiger/wiredtiger_index.cpp +++ b/src/mongo/db/storage/wiredtiger/wiredtiger_index.cpp @@ -118,6 +118,7 @@ namespace { // static StatusWith WiredTigerIndex::parseIndexOptions(const BSONObj& options) { + StringBuilder ss; BSONForEach(elem, options) { if (elem.fieldNameStringData() == "configString") { if (elem.type() != String) { @@ -126,11 +127,7 @@ namespace { << "Not adding 'configString' value " << elem << " to index configuration"); } - if (elem.valueStringData().empty()) { - return StatusWith(ErrorCodes::InvalidOptions, - "configString must be not be an empty string."); - } - return StatusWith(elem.String()); + ss << elem.valueStringData() << ','; } else { // Return error on first unrecognized field. @@ -139,8 +136,7 @@ namespace { << " is not a supported option."); } } - return StatusWith(ErrorCodes::BadValue, - "Storage engine options document must not be empty."); + return StatusWith(ss.str()); } // static diff --git a/src/mongo/db/storage/wiredtiger/wiredtiger_index_test.cpp b/src/mongo/db/storage/wiredtiger/wiredtiger_index_test.cpp index 86797c701f7..c6b15b74a3f 100644 --- a/src/mongo/db/storage/wiredtiger/wiredtiger_index_test.cpp +++ b/src/mongo/db/storage/wiredtiger/wiredtiger_index_test.cpp @@ -101,49 +101,42 @@ namespace mongo { } TEST(WiredTigerIndexTest, GenerateCreateStringEmptyDocument) { - BSONObj spec = fromjson("{storageEngine: {wiredTiger: {}}}"); - IndexDescriptor desc(NULL, "", spec); - StatusWith result = WiredTigerIndex::generateCreateString("", desc); - const Status& status = result.getStatus(); - ASSERT_NOT_OK(status); - ASSERT_EQUALS(ErrorCodes::BadValue, status.code()); + BSONObj spec = fromjson("{}"); + StatusWith result = WiredTigerIndex::parseIndexOptions(spec); + ASSERT_OK(result.getStatus()); + ASSERT_EQ(result.getValue(), ""); // "," would also be valid. } TEST(WiredTigerIndexTest, GenerateCreateStringUnknownField) { - BSONObj spec = fromjson("{storageEngine: {wiredTiger: {unknownField: 1}}}"); - IndexDescriptor desc(NULL, "", spec); - StatusWith result = WiredTigerIndex::generateCreateString("", desc); + BSONObj spec = fromjson("{unknownField: 1}"); + StatusWith result = WiredTigerIndex::parseIndexOptions(spec); const Status& status = result.getStatus(); ASSERT_NOT_OK(status); - ASSERT_EQUALS(ErrorCodes::InvalidOptions, status.code()); + ASSERT_EQUALS(ErrorCodes::InvalidOptions, status); } TEST(WiredTigerIndexTest, GenerateCreateStringNonStringConfig) { - BSONObj spec = fromjson("{storageEngine: {wiredTiger: {configString: 12345}}}"); - IndexDescriptor desc(NULL, "", spec); - StatusWith result = WiredTigerIndex::generateCreateString("", desc); + BSONObj spec = fromjson("{configString: 12345}"); + StatusWith result = WiredTigerIndex::parseIndexOptions(spec); const Status& status = result.getStatus(); ASSERT_NOT_OK(status); - ASSERT_EQUALS(ErrorCodes::TypeMismatch, status.code()); + ASSERT_EQUALS(ErrorCodes::TypeMismatch, status); } TEST(WiredTigerIndexTest, GenerateCreateStringEmptyConfigString) { - BSONObj spec = fromjson("{storageEngine: {wiredTiger: {configString: ''}}}"); - IndexDescriptor desc(NULL, "", spec); - StatusWith result = WiredTigerIndex::generateCreateString("", desc); - const Status& status = result.getStatus(); - ASSERT_NOT_OK(status); - ASSERT_EQUALS(ErrorCodes::InvalidOptions, status.code()); + BSONObj spec = fromjson("{configString: ''}"); + StatusWith result = WiredTigerIndex::parseIndexOptions(spec); + ASSERT_OK(result.getStatus()); + ASSERT_EQ(result.getValue(), ","); // "" would also be valid. } TEST(WiredTigerIndexTest, GenerateCreateStringValidConfigFormat) { - BSONObj spec = fromjson("{storageEngine: {wiredTiger: {configString: 'abc=def'}}}"); - IndexDescriptor desc(NULL, "", spec); - StatusWith result = WiredTigerIndex::generateCreateString("", desc); + // TODO eventually this should fail since "abc" is not a valid WT option. + BSONObj spec = fromjson("{configString: 'abc=def'}"); + StatusWith result = WiredTigerIndex::parseIndexOptions(spec); const Status& status = result.getStatus(); ASSERT_OK(status); - const std::string& config = result.getValue(); - ASSERT_NOT_EQUALS(std::string::npos, config.find("abc=def")); + ASSERT_EQ(result.getValue(), "abc=def,"); } } // namespace mongo diff --git a/src/mongo/db/storage/wiredtiger/wiredtiger_init.cpp b/src/mongo/db/storage/wiredtiger/wiredtiger_init.cpp index ec8b05b47fd..148df9d9696 100644 --- a/src/mongo/db/storage/wiredtiger/wiredtiger_init.cpp +++ b/src/mongo/db/storage/wiredtiger/wiredtiger_init.cpp @@ -82,7 +82,7 @@ namespace mongo { } virtual Status validateCollectionStorageOptions(const BSONObj& options) const { - return Status::OK(); + return WiredTigerRecordStore::parseOptionsField(options).getStatus(); } virtual Status validateIndexStorageOptions(const BSONObj& options) const { diff --git a/src/mongo/db/storage/wiredtiger/wiredtiger_record_store.cpp b/src/mongo/db/storage/wiredtiger/wiredtiger_record_store.cpp index 4f203c7f991..11942f5a93b 100644 --- a/src/mongo/db/storage/wiredtiger/wiredtiger_record_store.cpp +++ b/src/mongo/db/storage/wiredtiger/wiredtiger_record_store.cpp @@ -89,6 +89,30 @@ namespace { const long long WiredTigerRecordStore::kCollectionScanOnCreationThreshold = 10000; + StatusWith WiredTigerRecordStore::parseOptionsField(const BSONObj options) { + StringBuilder ss; + BSONForEach(elem, options) { + if (elem.fieldNameStringData() == "configString") { + if (elem.type() != String) { + return StatusWith(ErrorCodes::TypeMismatch, str::stream() + << "storageEngine.wiredTiger.configString " + << "must be a string. " + << "Not adding 'configString' value " + << elem << " to collection configuration"); + } + ss << elem.valueStringData() << ','; + } + else { + // Return error on first unrecognized field. + return StatusWith(ErrorCodes::InvalidOptions, str::stream() + << '\'' << elem.fieldNameStringData() << '\'' + << " is not a supported option in " + << "storageEngine.wiredTiger"); + } + } + return StatusWith(ss.str()); + } + // static StatusWith WiredTigerRecordStore::generateCreateString( StringData ns, @@ -114,30 +138,12 @@ namespace { ss << extraStrings << ","; - // Validate configuration object. - // Warn about unrecognized fields that may be introduced in newer versions of this - // storage engine instead of raising an error. - // Ensure that 'configString' field is a string. Warn if this is not the case. - BSONForEach(elem, options.storageEngine.getObjectField(kWiredTigerEngineName)) { - if (elem.fieldNameStringData() == "configString") { - if (elem.type() != String) { - return StatusWith(ErrorCodes::TypeMismatch, str::stream() - << "storageEngine.wiredTiger.configString " - << "must be a string. " - << "Not adding 'configString' value " - << elem << " to collection configuration"); - continue; - } - ss << elem.valueStringData() << ","; - } - else { - // Return error on first unrecognized field. - return StatusWith(ErrorCodes::InvalidOptions, str::stream() - << '\'' << elem.fieldNameStringData() << '\'' - << " is not a supported option in " - << "storageEngine.wiredTiger"); - } - } + StatusWith customOptions = + parseOptionsField(options.storageEngine.getObjectField(kWiredTigerEngineName)); + if (!customOptions.isOK()) + return customOptions; + + ss << customOptions.getValue(); if ( NamespaceString::oplog(ns) ) { // force file for oplog diff --git a/src/mongo/db/storage/wiredtiger/wiredtiger_record_store.h b/src/mongo/db/storage/wiredtiger/wiredtiger_record_store.h index 1107206ef1e..6defe3552fa 100644 --- a/src/mongo/db/storage/wiredtiger/wiredtiger_record_store.h +++ b/src/mongo/db/storage/wiredtiger/wiredtiger_record_store.h @@ -68,6 +68,13 @@ namespace mongo { */ static const long long kCollectionScanOnCreationThreshold; + /** + * Parses collections options for wired tiger configuration string for table creation. + * The document 'options' is typically obtained from the 'wiredTiger' field of + * CollectionOptions::storageEngine. + */ + static StatusWith parseOptionsField(const BSONObj options); + /** * Creates a configuration string suitable for 'config' parameter in WT_SESSION::create(). * Configuration string is constructed from: diff --git a/src/mongo/db/storage/wiredtiger/wiredtiger_record_store_test.cpp b/src/mongo/db/storage/wiredtiger/wiredtiger_record_store_test.cpp index efecfe86c56..b944e38329d 100644 --- a/src/mongo/db/storage/wiredtiger/wiredtiger_record_store_test.cpp +++ b/src/mongo/db/storage/wiredtiger/wiredtiger_record_store_test.cpp @@ -150,22 +150,43 @@ namespace mongo { return new WiredTigerHarnessHelper(); } + TEST(WiredTigerRecordStoreTest, GenerateCreateStringEmptyDocument) { + BSONObj spec = fromjson("{}"); + StatusWith result = WiredTigerRecordStore::parseOptionsField(spec); + ASSERT_OK(result.getStatus()); + ASSERT_EQ(result.getValue(), ""); // "," would also be valid. + } + TEST(WiredTigerRecordStoreTest, GenerateCreateStringUnknownField) { - CollectionOptions options; - options.storageEngine = fromjson("{wiredTiger: {unknownField: 1}}"); - StatusWith result = WiredTigerRecordStore::generateCreateString("", options, ""); + BSONObj spec = fromjson("{unknownField: 1}"); + StatusWith result = WiredTigerRecordStore::parseOptionsField(spec); const Status& status = result.getStatus(); ASSERT_NOT_OK(status); - ASSERT_EQUALS(ErrorCodes::InvalidOptions, status.code()); + ASSERT_EQUALS(ErrorCodes::InvalidOptions, status); } TEST(WiredTigerRecordStoreTest, GenerateCreateStringNonStringConfig) { - CollectionOptions options; - options.storageEngine = fromjson("{wiredTiger: {configString: 12345}}"); - StatusWith result = WiredTigerRecordStore::generateCreateString("", options, ""); + BSONObj spec = fromjson("{configString: 12345}"); + StatusWith result = WiredTigerRecordStore::parseOptionsField(spec); const Status& status = result.getStatus(); ASSERT_NOT_OK(status); - ASSERT_EQUALS(ErrorCodes::TypeMismatch, status.code()); + ASSERT_EQUALS(ErrorCodes::TypeMismatch, status); + } + + TEST(WiredTigerRecordStoreTest, GenerateCreateStringEmptyConfigString) { + BSONObj spec = fromjson("{configString: ''}"); + StatusWith result = WiredTigerRecordStore::parseOptionsField(spec); + ASSERT_OK(result.getStatus()); + ASSERT_EQ(result.getValue(), ","); // "" would also be valid. + } + + TEST(WiredTigerRecordStoreTest, GenerateCreateStringValidConfigFormat) { + // TODO eventually this should fail since "abc" is not a valid WT option. + BSONObj spec = fromjson("{configString: 'abc=def'}"); + StatusWith result = WiredTigerRecordStore::parseOptionsField(spec); + const Status& status = result.getStatus(); + ASSERT_OK(status); + ASSERT_EQ(result.getValue(), "abc=def,"); } TEST(WiredTigerRecordStoreTest, Isolation1 ) {