mirror
/
mongo
mirror of https://github.com/mongodb/mongo
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

SERVER-111590 Disallow introduction of new usages of CollectionCatalog in the query module (#43133) 

Co-authored-by: Mariano Shaar <47160550+shaargtz@users.noreply.github.com>
GitOrigin-RevId: d96ce1515fb63a0c4384a2db8fb963fb0684119a
This commit is contained in:
Jordi Serra Torrens 2025-10-28 14:59:20 +01:00 committed by MongoDB Bot
parent 3de2ff2a58
commit 8286444418
18 changed files with 67 additions and 31 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

2
.clang-tidy.in
View File

@ -65,7 +65,7 @@ Checks: '-*,
modernize-unary-static-assert,
modernize-use-override,
mongo-assert-check,
mongo-banned-auto-get-usage-check,
mongo-banned-catalog-access-from-query-code-check,
mongo-banned-names-check,
mongo-bypass-database-metadata-access-check,
mongo-cctype-check,

4
.github/CODEOWNERS vendored
View File

@ -3082,7 +3082,7 @@ WORKSPACE.bazel @10gen/devprod-build @svc-auto-approve-bot
/src/mongo/tools/mongo_tidy_checks/**/MongoMacroDefinitionLeaksCheck.* @10gen/server-networking-and-observability @svc-auto-approve-bot
/src/mongo/tools/mongo_tidy_checks/**/MongoTraceCheck.* @10gen/server-networking-and-observability @svc-auto-approve-bot
/src/mongo/tools/mongo_tidy_checks/**/MongoUnstructuredLogCheck.* @10gen/server-networking-and-observability @svc-auto-approve-bot
/src/mongo/tools/mongo_tidy_checks/**/MongoBannedAutoGetUsageCheck.* @10gen/server-catalog-and-routing @svc-auto-approve-bot
/src/mongo/tools/mongo_tidy_checks/**/MongoBannedCatalogAccessFromQueryCodeCheck.* @10gen/server-catalog-and-routing @svc-auto-approve-bot
# The following patterns are parsed from ./src/mongo/tools/mongo_tidy_checks/tests/OWNERS.yml
/src/mongo/tools/mongo_tidy_checks/tests/**/* @10gen/server-programmability @svc-auto-approve-bot
@ -3090,7 +3090,7 @@ WORKSPACE.bazel @10gen/devprod-build @svc-auto-approve-bot
/src/mongo/tools/mongo_tidy_checks/tests/**/test_MongoMacroDefinitionLeaksCheck.* @10gen/server-networking-and-observability @svc-auto-approve-bot
/src/mongo/tools/mongo_tidy_checks/tests/**/test_MongoTraceCheck.* @10gen/server-networking-and-observability @svc-auto-approve-bot
/src/mongo/tools/mongo_tidy_checks/tests/**/test_MongoUnstructuredLogCheck.* @10gen/server-networking-and-observability @svc-auto-approve-bot
/src/mongo/tools/mongo_tidy_checks/tests/**/test_MongoBannedAutoGetUsageCheck.* @10gen/server-catalog-and-routing @svc-auto-approve-bot
/src/mongo/tools/mongo_tidy_checks/tests/**/test_MongoBannedCatalogAccessFromQueryCodeCheck.* @10gen/server-catalog-and-routing @svc-auto-approve-bot
# The following patterns are parsed from ./src/mongo/tools/mongobridge_tool/OWNERS.yml
/src/mongo/tools/mongobridge_tool/**/* @10gen/server-networking-and-observability @svc-auto-approve-bot

4
src/mongo/db/exec/classic/requires_collection_stage.cpp
View File

@ -83,4 +83,8 @@ void RequiresCollectionStage::doRestoreState(const RestoreContext& context) {
doRestoreStateRequiresCollection();
}
uint64_t RequiresCollectionStage::getCatalogEpoch() const {
return CollectionCatalog::get(opCtx())->getEpoch();
}
} // namespace mongo

4
src/mongo/db/exec/classic/requires_collection_stage.h
View File

@ -96,9 +96,7 @@ protected:
private:
// This can only be called when the plan stage is attached to an operation context.
uint64_t getCatalogEpoch() const {
return CollectionCatalog::get(opCtx())->getEpoch();
}
uint64_t getCatalogEpoch() const;
// Pointer to a CollectionPtr that is stored at a high level in a AutoGetCollection or other
// helper. It needs to stay valid until the PlanExecutor saves its state. To avoid this pointer

2
src/mongo/db/query/plan_cache/plan_cache_classic_bm.cpp
View File

@ -367,8 +367,6 @@ void BM_PlanCacheClassic(benchmark::State& state) {
auto collection =
std::make_shared<CollectionMock>(UUID::gen(), kNss, std::make_unique<IndexCatalogMock>());
auto catalog = CollectionCatalog::get(opCtx.get());
catalog->onCreateCollection(opCtx.get(), collection);
// The initialization of the CollectionPtr is SAFE. The lifetime of the Mocked Collection
// instance is managed by the test and guaranteed to be valid for the entire duration of the
// test.

3
src/mongo/db/query/plan_insert_listener.cpp
View File

@ -100,7 +100,8 @@ std::unique_ptr<Notifier> getCappedInsertNotifier(OperationContext* opCtx,
RecoveryUnit::kMajorityCommitted) {
return std::make_unique<MajorityCommittedPointNotifier>();
} else {
auto collection = CollectionCatalog::get(opCtx)->lookupCollectionByNamespace(opCtx, nss);
auto collCatalog = CollectionCatalog::get(opCtx); // NOLINT TODO SERVER-112937 Remove this.
auto collection = collCatalog->lookupCollectionByNamespace(opCtx, nss);
invariant(collection);
return std::make_unique<LocalCappedInsertNotifier>(

2
src/mongo/db/query/search/search_index_process_shard.cpp
View File

@ -52,7 +52,7 @@ std::pair<boost::optional<UUID>, boost::optional<ResolvedView>>
SearchIndexProcessShard::fetchCollectionUUIDAndResolveView(OperationContext* opCtx,
const NamespaceString& nss,
bool failOnTsColl) {
auto catalog = CollectionCatalog::get(opCtx);
auto catalog = CollectionCatalog::get(opCtx); // NOLINT TODO: SERVER-104335 Remove this.
auto coll = catalog->lookupCollectionByNamespace(opCtx, nss);
auto view = catalog->lookupView(opCtx, nss);

4
src/mongo/tools/mongo_tidy_checks/BUILD.bazel
View File

@ -11,7 +11,7 @@ cc_library(
name = "mongo_tidy_checks_static",
srcs = [
"MongoAssertCheck.cpp",
"MongoBannedAutoGetUsageCheck.cpp",
"MongoBannedCatalogAccessFromQueryCodeCheck.cpp",
"MongoBannedNamesCheck.cpp",
"MongoBypassDatabaseMetadataAccessCheck.cpp",
"MongoCctypeCheck.cpp",
@ -36,7 +36,7 @@ cc_library(
],
hdrs = [
"MongoAssertCheck.h",
"MongoBannedAutoGetUsageCheck.h",
"MongoBannedCatalogAccessFromQueryCodeCheck.h",
"MongoBannedNamesCheck.h",
"MongoBypassDatabaseMetadataAccessCheck.h",
"MongoCctypeCheck.h",

27
src/mongo/tools/mongo_tidy_checks/MongoBannedAutoGetUsageCheck.cpp → src/mongo/tools/mongo_tidy_checks/MongoBannedCatalogAccessFromQueryCodeCheck.cpp
View File

@ -27,7 +27,7 @@
* it in the license file.
*/
#include "MongoBannedAutoGetUsageCheck.h"
#include "MongoBannedCatalogAccessFromQueryCodeCheck.h"
#include <array>
#include <string_view>
@ -37,24 +37,29 @@ using namespace clang::ast_matchers;
namespace mongo::tidy {
MongoBannedAutoGetUsageCheck::MongoBannedAutoGetUsageCheck(clang::StringRef Name,
clang::tidy::ClangTidyContext* Context)
MongoBannedCatalogAccessFromQueryCodeCheck::MongoBannedCatalogAccessFromQueryCodeCheck(
clang::StringRef Name, clang::tidy::ClangTidyContext* Context)
: ClangTidyCheck(Name, Context) {}
void MongoBannedAutoGetUsageCheck::registerMatchers(MatchFinder* Finder) {
void MongoBannedCatalogAccessFromQueryCodeCheck::registerMatchers(MatchFinder* Finder) {
Finder->addMatcher(
varDecl(hasType(cxxRecordDecl(hasName("AutoGetCollection")))).bind("AutoGetCollectionDec"),
this);
Finder->addMatcher(
callExpr(callee(cxxMethodDecl(ofClass(cxxRecordDecl(hasName("CollectionCatalog"))),
isStaticStorageClass())))
.bind("CollectionCatalogUsage"),
this);
}
void MongoBannedAutoGetUsageCheck::check(const MatchFinder::MatchResult& Result) {
void MongoBannedCatalogAccessFromQueryCodeCheck::check(const MatchFinder::MatchResult& Result) {
const auto& sourceManager = *Result.SourceManager;
// Get the location of the current source file
const auto filePath = sourceManager.getFilename(
sourceManager.getLocForStartOfFile(sourceManager.getMainFileID()));
// Only check on the file paths belonging to modules that are forbidden to use
// AutoGetCollection.
// AutoGetCollection and CollectionCatalog.
static constexpr std::array<std::string_view, 2> forbiddenDirs = {
"src/mongo/db/query/", "src/mongo/tools/mongo_tidy_checks/tests/"};
if (std::none_of(forbiddenDirs.begin(),
@ -63,11 +68,21 @@ void MongoBannedAutoGetUsageCheck::check(const MatchFinder::MatchResult& Result)
return;
}
// Ignore unit tests.
if (filePath.ends_with("_test.cpp"))
return;
if (const auto matchedVar = Result.Nodes.getNodeAs<VarDecl>("AutoGetCollectionDec")) {
diag(matchedVar->getBeginLoc(),
"AutoGetCollection is not allowed to be used from the query modules. Use ShardRole "
"CollectionAcquisitions instead.");
}
if (const auto matchedCall = Result.Nodes.getNodeAs<CallExpr>("CollectionCatalogUsage")) {
diag(matchedCall->getBeginLoc(),
"CollectionCatalog is not allowed to be used from the query modules. Use ShardRole "
"CollectionAcquisitions instead.");
}
}
} // namespace mongo::tidy

8
src/mongo/tools/mongo_tidy_checks/MongoBannedAutoGetUsageCheck.h → src/mongo/tools/mongo_tidy_checks/MongoBannedCatalogAccessFromQueryCodeCheck.h
View File

@ -35,11 +35,13 @@
namespace mongo::tidy {
/**
* A clang-tidy check that bans the usage of AutoGetCollection in certain modules.
* A clang-tidy check that bans the usage of internal catalog classes such as AutoGetCollection or
* CollectionCatalog from certain modules.
*/
class MongoBannedAutoGetUsageCheck : public clang::tidy::ClangTidyCheck {
class MongoBannedCatalogAccessFromQueryCodeCheck : public clang::tidy::ClangTidyCheck {
public:
MongoBannedAutoGetUsageCheck(clang::StringRef Name, clang::tidy::ClangTidyContext* Context);
MongoBannedCatalogAccessFromQueryCodeCheck(clang::StringRef Name,
clang::tidy::ClangTidyContext* Context);
void registerMatchers(clang::ast_matchers::MatchFinder* Finder) override;
void check(const clang::ast_matchers::MatchFinder::MatchResult& Result) override;
};

6
src/mongo/tools/mongo_tidy_checks/MongoTidyModule.cpp
View File

@ -28,7 +28,7 @@
*/
#include "MongoAssertCheck.h"
#include "MongoBannedAutoGetUsageCheck.h"
#include "MongoBannedCatalogAccessFromQueryCodeCheck.h"
#include "MongoBannedNamesCheck.h"
#include "MongoBypassDatabaseMetadataAccessCheck.h"
#include "MongoCctypeCheck.h"
@ -90,8 +90,8 @@ public:
"mongo-invariant-ddl-coordinator-check");
CheckFactories.registerCheck<MongoBypassDatabaseMetadataAccessCheck>(
"mongo-bypass-database-metadata-access-check");
CheckFactories.registerCheck<MongoBannedAutoGetUsageCheck>(
"mongo-banned-auto-get-usage-check");
CheckFactories.registerCheck<MongoBannedCatalogAccessFromQueryCodeCheck>(
"mongo-banned-catalog-access-from-query-code-check");
}
};

2
src/mongo/tools/mongo_tidy_checks/OWNERS.yml
View File

@ -15,6 +15,6 @@ filters:
- "MongoUnstructuredLogCheck.*":
approvers:
- 10gen/server-networking-and-observability
- "MongoBannedAutoGetUsageCheck.*":
- "MongoBannedCatalogAccessFromQueryCodeCheck.*":
approvers:
- 10gen/server-catalog-and-routing

2
src/mongo/tools/mongo_tidy_checks/tests/BUILD.bazel
View File

@ -33,7 +33,7 @@ tests = [
"test_MongoMacroDefinitionLeaksCheck",
"test_MongoRandCheck",
"test_MongoRWMutexCheck",
"test_MongoBannedAutoGetUsageCheck",
"test_MongoBannedCatalogAccessFromQueryCodeCheck",
"test_MongoBannedNamesCheck",
"test_MongoNoUniqueAddressCheck",
"test_MongoStringDataConstRefCheck1",

7
src/mongo/tools/mongo_tidy_checks/tests/MongoTidyCheck_unittest.py
View File

@ -317,8 +317,11 @@ class MongoTidyTests(unittest.TestCase):
self.run_clang_tidy()
def test_MongoBannedAutoGetUsageCheck(self):
self.expected_output = "AutoGetCollection is not allowed to be used from the query modules. Use ShardRole CollectionAcquisitions instead."
def test_MongoBannedCatalogAccessFromQueryCodeCheck(self):
self.expected_output = [
"AutoGetCollection is not allowed to be used from the query modules. Use ShardRole CollectionAcquisitions instead.",
"CollectionCatalog is not allowed to be used from the query modules. Use ShardRole CollectionAcquisitions instead.",
]
self.run_clang_tidy()

2
src/mongo/tools/mongo_tidy_checks/tests/OWNERS.yml
View File

@ -15,6 +15,6 @@ filters:
- "test_MongoUnstructuredLogCheck.*":
approvers:
- 10gen/server-networking-and-observability
- "test_MongoBannedAutoGetUsageCheck.*":
- "test_MongoBannedCatalogAccessFromQueryCodeCheck.*":
approvers:
- 10gen/server-catalog-and-routing

2
src/mongo/tools/mongo_tidy_checks/tests/test_MongoBannedAutoGetUsageCheck.tidy_config
View File

@ -1,2 +0,0 @@
Checks: '-*,mongo-banned-auto-get-usage-check'
WarningsAsErrors: '*'

15
src/mongo/tools/mongo_tidy_checks/tests/test_MongoBannedAutoGetUsageCheck.cpp → src/mongo/tools/mongo_tidy_checks/tests/test_MongoBannedCatalogAccessFromQueryCodeCheck.cpp
View File

@ -27,12 +27,27 @@
* it in the license file.
*/
#include <memory>
namespace mongo {
class AutoGetCollection {};
class CollectionCatalog {
public:
static std::shared_ptr<CollectionCatalog> get() {
return std::make_shared<CollectionCatalog>();
}
void foo() {};
};
int fun() {
AutoGetCollection agc;
}
int fun2() {
CollectionCatalog::get()->foo();
}
} // namespace mongo

2
src/mongo/tools/mongo_tidy_checks/tests/test_MongoBannedCatalogAccessFromQueryCodeCheck.tidy_config Normal file
View File

@ -0,0 +1,2 @@
Checks: '-*,mongo-banned-catalog-access-from-query-code-check'
WarningsAsErrors: '*'