From cd26a1d5ee080f1a68b874046e5e5b3672267f69 Mon Sep 17 00:00:00 2001 From: Mathias Stearn Date: Tue, 24 Oct 2017 13:00:11 -0400 Subject: [PATCH] SERVER-31629 Support putting unique codes directly into Status constructors --- buildscripts/cpplint.py | 7 +++++++ buildscripts/errorcodes.py | 14 +++----------- src/mongo/base/error_codes.tpl.h | 11 +++++++++++ src/mongo/base/status_test.cpp | 2 +- src/mongo/bson/ugly_bson_integration_test.cpp | 2 +- src/mongo/client/dbclient_rs.cpp | 6 +++--- src/mongo/db/pipeline/pipeline_test.cpp | 4 ++-- src/mongo/db/repl/rs_rollback_no_uuid_test.cpp | 2 +- src/mongo/db/repl/rs_rollback_test.cpp | 2 +- src/mongo/db/repl/storage_interface_impl_test.cpp | 5 +++-- 10 files changed, 33 insertions(+), 22 deletions(-) diff --git a/buildscripts/cpplint.py b/buildscripts/cpplint.py index a9dd9940921..c6aa51b7b92 100755 --- a/buildscripts/cpplint.py +++ b/buildscripts/cpplint.py @@ -1676,6 +1676,12 @@ def CheckForMongoVolatile(filename, clean_lines, linenum, error): 'Illegal use of the volatile storage keyword, use AtomicWord instead ' 'from "mongo/platform/atomic_word.h"') +def CheckForNonMongoAssert(filename, clean_lines, linenum, error): + line = clean_lines.elided[linenum] + if re.search(r'\bassert\s*\(', line): + error(filename, linenum, 'mongodb/assert', 5, + 'Illegal use of the bare assert function, use a function from assert_utils.h instead.') + def CheckForCopyright(filename, lines, error): """Logs an error if no Copyright message appears at the top of the file.""" @@ -5820,6 +5826,7 @@ def ProcessLine(filename, file_extension, clean_lines, line, CheckForMongoPolyfill(filename, clean_lines, line, error) CheckForMongoAtomic(filename, clean_lines, line, error) CheckForMongoVolatile(filename, clean_lines, line, error) + CheckForNonMongoAssert(filename, clean_lines, line, error) if nesting_state.InAsmBlock(): return CheckForFunctionLengths(filename, clean_lines, line, function_state, error) CheckForMultilineCommentsAndStrings(filename, clean_lines, line, error) diff --git a/buildscripts/errorcodes.py b/buildscripts/errorcodes.py index 639c484eeb8..cc46789907f 100755 --- a/buildscripts/errorcodes.py +++ b/buildscripts/errorcodes.py @@ -54,17 +54,16 @@ def assignErrorCodes(): def parseSourceFiles( callback ): """Walks MongoDB sourcefiles and invokes callback for each AssertLocation found.""" - quick = [ "assert" , "Exception"] + quick = ["assert", "Exception", "ErrorCodes::Error"] patterns = [ re.compile( r"(?:u|m(?:sg)?)asser(?:t|ted)(?:NoTrace)?\s*\(\s*(\d+)", re.MULTILINE ) , - re.compile( r"(?:DB|Assertion)Exception\s*\(\s*(\d+)", re.MULTILINE ), + re.compile( r"(?:DB|Assertion)Exception\s*[({]\s*(\d+)", re.MULTILINE ), re.compile( r"fassert(?:Failed)?(?:WithStatus)?(?:NoTrace)?(?:StatusOK)?\s*\(\s*(\d+)", re.MULTILINE ), + re.compile( r"ErrorCodes::Error\s*[({]\s*(\d+)", re.MULTILINE ) ] - bad = [ re.compile( r"^\s*assert *\(" ) ] - for sourceFile in utils.getAllSourceFiles(prefix='src/mongo/'): if list_files: print 'scanning file: ' + sourceFile @@ -75,13 +74,6 @@ def parseSourceFiles( callback ): if not any([zz in text for zz in quick]): continue - # TODO: move check for bad assert type to the linter. - for b in bad: - if b.search(text): - msg = "Bare assert prohibited. Replace with [umwdf]assert" - print( "%s: %s" % (sourceFile, msg) ) - raise Exception(msg) - matchiters = [p.finditer(text) for p in patterns] for matchiter in matchiters: for match in matchiter: diff --git a/src/mongo/base/error_codes.tpl.h b/src/mongo/base/error_codes.tpl.h index ff4d3b0822d..893f61b19a8 100644 --- a/src/mongo/base/error_codes.tpl.h +++ b/src/mongo/base/error_codes.tpl.h @@ -72,6 +72,17 @@ public: */ static Error fromString(StringData name); + /** + * Reuses a unique numeric code in a way that supresses the duplicate code detection. This + * should only be used when testing error cases to ensure that the code under test fails in the + * right place. It should NOT be used in non-test code to either make a new error site (use + * ErrorCodes::Error(CODE) for that) or to see if a specific failure case occurred (use named + * codes for that). + */ + static Error duplicateCodeForTest(int code) { + return static_cast(code); + } + /** * Generic predicate to test if a given error code is in a category. * diff --git a/src/mongo/base/status_test.cpp b/src/mongo/base/status_test.cpp index ba57448939f..121c30f7ff5 100644 --- a/src/mongo/base/status_test.cpp +++ b/src/mongo/base/status_test.cpp @@ -202,7 +202,7 @@ TEST(Parsing, CodeToEnum) { ASSERT_EQUALS(ErrorCodes::TypeMismatch, ErrorCodes::Error(int(ErrorCodes::TypeMismatch))); ASSERT_EQUALS(ErrorCodes::UnknownError, ErrorCodes::Error(int(ErrorCodes::UnknownError))); ASSERT_EQUALS(ErrorCodes::MaxError, ErrorCodes::Error(int(ErrorCodes::MaxError))); - ASSERT_EQUALS(ErrorCodes::OK, ErrorCodes::Error(0)); + ASSERT_EQUALS(ErrorCodes::OK, ErrorCodes::duplicateCodeForTest(0)); } TEST(Transformers, ExceptionToStatus) { diff --git a/src/mongo/bson/ugly_bson_integration_test.cpp b/src/mongo/bson/ugly_bson_integration_test.cpp index a3f6bf11fdb..df2b584884a 100644 --- a/src/mongo/bson/ugly_bson_integration_test.cpp +++ b/src/mongo/bson/ugly_bson_integration_test.cpp @@ -60,7 +60,7 @@ TEST_F(UglyBSONFixture, DuplicateFields) { << BSONArray() << "documents" << BSONArray()), - ErrorCodes::Error(40413)); + ErrorCodes::duplicateCodeForTest(40413)); } } // namespace diff --git a/src/mongo/client/dbclient_rs.cpp b/src/mongo/client/dbclient_rs.cpp index 3f798f95bda..a4e7a5df146 100644 --- a/src/mongo/client/dbclient_rs.cpp +++ b/src/mongo/client/dbclient_rs.cpp @@ -288,7 +288,7 @@ DBClientConnection* DBClientReplicaSet::checkMaster() { return _master.get(); monitor->failedHost(_masterHost, - {ErrorCodes::Error(40332), "Last known master host cannot be reached"}); + {ErrorCodes::Error(40657), "Last known master host cannot be reached"}); h = monitor->getMasterOrUassert(); // old master failed, try again. } @@ -320,7 +320,7 @@ DBClientConnection* DBClientReplicaSet::checkMaster() { const std::string message = str::stream() << "can't connect to new replica set master [" << _masterHost.toString() << "]" << (errmsg.empty() ? "" : ", err: ") << errmsg; - monitor->failedHost(_masterHost, {ErrorCodes::Error(40333), message}); + monitor->failedHost(_masterHost, {ErrorCodes::Error(40659), message}); uasserted(ErrorCodes::FailedToSatisfyReadPreference, message); } @@ -350,7 +350,7 @@ bool DBClientReplicaSet::checkLastHost(const ReadPreferenceSetting* readPref) { // Make sure we don't think the host is down. if (_lastSlaveOkConn->isFailed() || !_getMonitor()->isHostUp(_lastSlaveOkHost)) { _invalidateLastSlaveOkCache( - {ErrorCodes::Error(40334), "Last slave connection is no longer available"}); + {ErrorCodes::Error(40660), "Last slave connection is no longer available"}); return false; } diff --git a/src/mongo/db/pipeline/pipeline_test.cpp b/src/mongo/db/pipeline/pipeline_test.cpp index 4cca4bff29d..31e1dd19ec0 100644 --- a/src/mongo/db/pipeline/pipeline_test.cpp +++ b/src/mongo/db/pipeline/pipeline_test.cpp @@ -2206,7 +2206,7 @@ TEST_F(PipelineInitialSourceNSTest, ChangeStreamIsNotValidIfNotFirstStage) { setMockReplicationCoordinatorOnOpCtx(ctx->opCtx); ctx->ns = NamespaceString("a.collection"); auto parseStatus = Pipeline::parse(rawPipeline, ctx).getStatus(); - ASSERT_EQ(parseStatus, ErrorCodes::Error(40602)); + ASSERT_EQ(parseStatus, ErrorCodes::duplicateCodeForTest(40602)); } TEST_F(PipelineInitialSourceNSTest, ChangeStreamIsNotValidIfNotFirstStageInFacet) { @@ -2217,7 +2217,7 @@ TEST_F(PipelineInitialSourceNSTest, ChangeStreamIsNotValidIfNotFirstStageInFacet setMockReplicationCoordinatorOnOpCtx(ctx->opCtx); ctx->ns = NamespaceString("a.collection"); auto parseStatus = Pipeline::parseFacetPipeline(rawPipeline, ctx).getStatus(); - ASSERT_EQ(parseStatus, ErrorCodes::Error(40600)); + ASSERT_EQ(parseStatus, ErrorCodes::duplicateCodeForTest(40600)); ASSERT(std::string::npos != parseStatus.reason().find("$changeStream")); } diff --git a/src/mongo/db/repl/rs_rollback_no_uuid_test.cpp b/src/mongo/db/repl/rs_rollback_no_uuid_test.cpp index b48ed2596c9..a08d9594033 100644 --- a/src/mongo/db/repl/rs_rollback_no_uuid_test.cpp +++ b/src/mongo/db/repl/rs_rollback_no_uuid_test.cpp @@ -240,7 +240,7 @@ TEST_F(RSRollbackTest, RemoteGetRollbackIdDiffersFromRequiredRBID) { _replicationProcess.get()) .transitional_ignore(), AssertionException, - ErrorCodes::Error(40362)); + ErrorCodes::duplicateCodeForTest(40362)); } TEST_F(RSRollbackTest, BothOplogsAtCommonPoint) { diff --git a/src/mongo/db/repl/rs_rollback_test.cpp b/src/mongo/db/repl/rs_rollback_test.cpp index 9eb4c722388..6e93453ea1c 100644 --- a/src/mongo/db/repl/rs_rollback_test.cpp +++ b/src/mongo/db/repl/rs_rollback_test.cpp @@ -337,7 +337,7 @@ TEST_F(RSRollbackTest, RemoteGetRollbackIdDiffersFromRequiredRBID) { _replicationProcess.get()) .transitional_ignore(), AssertionException, - ErrorCodes::Error(40506)); + ErrorCodes::duplicateCodeForTest(40506)); } TEST_F(RSRollbackTest, BothOplogsAtCommonPoint) { diff --git a/src/mongo/db/repl/storage_interface_impl_test.cpp b/src/mongo/db/repl/storage_interface_impl_test.cpp index 3b2eca9f2f4..a134a16642e 100644 --- a/src/mongo/db/repl/storage_interface_impl_test.cpp +++ b/src/mongo/db/repl/storage_interface_impl_test.cpp @@ -338,7 +338,8 @@ TEST_F(StorageInterfaceImplTest, GetRollbackIDReturnsBadStatusIfDocumentHasBadFi {BSON("_id" << StorageInterfaceImpl::kRollbackIdDocumentId << "bad field" << 3), SnapshotName(0)}}; ASSERT_OK(storage.insertDocuments(opCtx, nss, transformInserts(badDocs))); - ASSERT_EQUALS(ErrorCodes::Error(40415), storage.getRollbackID(opCtx).getStatus()); + ASSERT_EQUALS(ErrorCodes::duplicateCodeForTest(40415), + storage.getRollbackID(opCtx).getStatus()); } TEST_F(StorageInterfaceImplTest, GetRollbackIDReturnsBadStatusIfRollbackIDIsNotInt) { @@ -622,7 +623,7 @@ TEST_F(StorageInterfaceImplTest, } auto status = storage.createCollection(opCtx, nss, CollectionOptions()); - ASSERT_EQUALS(ErrorCodes::Error(28838), status); + ASSERT_EQUALS(ErrorCodes::duplicateCodeForTest(28838), status); ASSERT_STRING_CONTAINS(status.reason(), "cannot create a non-capped oplog collection"); }