[#1177] Add ProtectedAtomDB - #1221
Conversation
WalkthroughThe change adds ChangesProtection status propagation
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant AtomDBFactory
participant Backend
participant ProtectedAtomDB
participant Caller
AtomDBFactory->>Backend: query is_protected()
AtomDBFactory->>ProtectedAtomDB: wrap protected backend
Caller->>ProtectedAtomDB: invoke database operation
ProtectedAtomDB-->>Caller: require public key or report not implemented
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/atomdb/redis_mongodb/RedisMongoDB.h (1)
44-57: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winKeep backend names instance-scoped.
MONGODB_DB_NAMEand context-derived collection names are class statics, while AtomDB construction can update them through the singleton factory path. Store these names as private instance state instead, and remove the corresponding static definition if converted. This affectsMorkDBas well because it derives fromRedisMongoDB.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/atomdb/redis_mongodb/RedisMongoDB.h` around lines 44 - 57, Convert MONGODB_DB_NAME and all context-derived collection names in RedisMongoDB from class statics to private instance members, initialize them per backend instance in initialize_statics or the corresponding constructor path, and update all accesses accordingly so singleton factory updates do not overwrite other instances. In src/atomdb/redis_mongodb/RedisMongoDB.h lines 44-57, change the declarations and initialization; in src/atomdb/redis_mongodb/RedisMongoDB.cc lines 34 and 1238-1243, remove static definitions and update the affected references. Preserve compatible access for the MorkDB subclass, which derives from RedisMongoDB.
🧹 Nitpick comments (6)
src/atomdb/remotedb/RemoteAtomDBPeer.cc (1)
43-46: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winApply
this->consistently to the new member accesses.Both new methods omit explicit qualification for instance members. Use the repository's required form.
src/atomdb/remotedb/RemoteAtomDBPeer.cc#L43-L46: qualifylocal_persistence_andatomdb_withthis->.src/atomdb/remotedb/RemoteAtomDB.cc#L69-L75: qualifyremote_db_withthis->.As per coding guidelines: access class members with
this->fieldconsistently in C++.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/atomdb/remotedb/RemoteAtomDBPeer.cc` around lines 43 - 46, Qualify all new member accesses with this-> to follow the C++ coding guideline: update local_persistence_ and atomdb_ in RemoteAtomDBPeer::is_protected in src/atomdb/remotedb/RemoteAtomDBPeer.cc lines 43-46, and update remote_db_ in src/atomdb/remotedb/RemoteAtomDB.cc lines 69-75.Source: Coding guidelines
src/atomdb/AtomDB.h (1)
26-26: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument the new
is_protected()contract in every production header.The new public method is declared in six production headers without a brief Doxygen block. Add a short description on the base method and
@copydoc AtomDB::is_protectedon each override.
src/atomdb/AtomDB.h#L26-L26: document the base contract.src/atomdb/adapterdb/AdapterDB.h#L65-L65: document the adapter override.src/atomdb/inmemorydb/InMemoryDB.h#L26-L26: document the in-memory override.src/atomdb/redis_mongodb/RedisMongoDB.h#L35-L35: document the MongoDB override.src/atomdb/remotedb/RemoteAtomDB.h#L32-L32: document the federation override.src/atomdb/remotedb/RemoteAtomDBPeer.h#L33-L33: document the peer override.As per coding guidelines and path instructions: public C++ header methods require brief Doxygen documentation.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/atomdb/AtomDB.h` at line 26, Document the public is_protected() contract in src/atomdb/AtomDB.h:26-26 with a brief Doxygen description, and add `@copydoc` AtomDB::is_protected to the overrides in src/atomdb/adapterdb/AdapterDB.h:65-65, src/atomdb/inmemorydb/InMemoryDB.h:26-26, src/atomdb/redis_mongodb/RedisMongoDB.h:35-35, src/atomdb/remotedb/RemoteAtomDB.h:32-32, and src/atomdb/remotedb/RemoteAtomDBPeer.h:33-33.Sources: Coding guidelines, Path instructions
src/atomdb/auth/ProtectedAtomDB.cc (1)
3-4: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDefine
LOG_LEVELbefore includingLogger.h.The file uses
LOG_INFOat line 16 but does not defineLOG_LEVELbefore#include "Logger.h". Neighboring.ccfiles insrc/atomdbset the level explicitly. Without the define, the log level depends on whateverLogger.hdefaults to.♻️ Proposed fix
`#include` "ProtectedAtomDB.h" +#define LOG_LEVEL INFO_LEVEL `#include` "Logger.h" `#include` "Utils.h"As per coding guidelines: "Set
#define LOG_LEVEL INFO_LEVEL(orDEBUG_LEVEL) before#include "Logger.h"in C++, then useLOG_DEBUG,LOG_INFO,LOG_ERROR".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/atomdb/auth/ProtectedAtomDB.cc` around lines 3 - 4, Define LOG_LEVEL as INFO_LEVEL before including Logger.h in ProtectedAtomDB.cc, keeping the existing LOG_INFO usage unchanged and matching the neighboring atomdb .cc files.Source: Coding guidelines
src/atomdb/AtomDBFactory.cc (1)
46-46: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valuePrefer
make_sharedhere to save one allocation.
ProtectedAtomDBhas a public constructor, somake_sharedworks and allocates the control block together with the object. Movingbackendalso avoids oneshared_ptrcopy with its atomic refcount increment.♻️ Proposed fix
- return shared_ptr<AtomDB>(new ProtectedAtomDB(backend)); + return make_shared<ProtectedAtomDB>(std::move(backend));🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/atomdb/AtomDBFactory.cc` at line 46, Update the AtomDB factory return to use make_shared for the public ProtectedAtomDB constructor, forwarding backend with move semantics to avoid an unnecessary shared_ptr copy while preserving the returned shared_ptr type and behavior.src/tests/cpp/adapterdb_test.cc (1)
254-259: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThis assertion passes even if
AdapterDB::is_protected()returned a hardcoded value.In the test environment the
RedisMongoDBbackend reportsfalse, so the comparison reduces tofalse == false. A hardcodedreturn falseinAdapterDB::is_protected()would still pass.Add a case with a backend that reports
true, following theProtectedInMemoryDBpattern added insrc/tests/cpp/remote_atomdb_test.ccat lines 542-548. That proves the delegation instead of the ambient default.As per path instructions: "Prioritize tests for real behavior: error paths, boundary conditions, thread/proxy interactions, and regressions — not trivial getters or coverage padding."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/tests/cpp/adapterdb_test.cc` around lines 254 - 259, Strengthen AdapterDBTest.IsProtectedDelegatesToBackend by exercising a backend whose is_protected() returns true, following the ProtectedInMemoryDB setup pattern from the referenced test. Assert that AdapterDB delegates and preserves the true result, while retaining the existing false-case coverage if appropriate.Source: Path instructions
src/tests/cpp/protected_atomdb_test.cc (1)
51-74: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winExtend the rejection coverage, and test through a
shared_ptr<AtomDB>handle.Two gaps:
The tests exercise 6 of the roughly 29 no-key methods. The untested ones include
get_node,get_link,get_matching_atoms,query_for_pattern,query_for_targets,query_for_incoming_set,node_exists,link_exists,atoms_exist,nodes_exist,links_exist,add_atom,add_atoms,add_nodes,add_links,delete_node,delete_link,delete_atoms,delete_nodes,delete_links,re_index_patterns,node_count,link_count. A missedoverrideon any of them silently delegates to the backend and leaks data. The 70% line coverage gate also counts each stub.Every call goes through the concrete
ProtectedAtomDBtype. Real callers hold ashared_ptr<AtomDB>fromAtomDBSingleton. A test through the base pointer proves the virtual dispatch reaches the rejection, which is the property that actually protects the data.💚 Suggested additional test
TEST(ProtectedAtomDBTest, RejectsAccessThroughBaseInterface) { shared_ptr<AtomDB> db = make_protected_db("protected_base_iface_"); Node node("Symbol", "\"x\""); Link link("Expression", {"a", "b"}); vector<string> handles = {"handle"}; LinkSchema link_schema({"LINK_TEMPLATE", "Expression", "1", "VARIABLE", "V"}); EXPECT_THROW(db->get_atom("handle"), runtime_error); EXPECT_THROW(db->get_node("handle"), runtime_error); EXPECT_THROW(db->get_link("handle"), runtime_error); EXPECT_THROW(db->query_for_pattern(link_schema), runtime_error); EXPECT_THROW(db->query_for_targets("handle"), runtime_error); EXPECT_THROW(db->query_for_incoming_set("handle"), runtime_error); EXPECT_THROW(db->node_exists("handle"), runtime_error); EXPECT_THROW(db->link_exists("handle"), runtime_error); EXPECT_THROW(db->atoms_exist(handles), runtime_error); EXPECT_THROW(db->nodes_exist(handles), runtime_error); EXPECT_THROW(db->links_exist(handles), runtime_error); EXPECT_THROW(db->add_node(&node), runtime_error); EXPECT_THROW(db->add_link(&link), runtime_error); EXPECT_THROW(db->delete_node("handle"), runtime_error); EXPECT_THROW(db->delete_link("handle"), runtime_error); EXPECT_THROW(db->delete_atoms(handles), runtime_error); EXPECT_THROW(db->delete_nodes(handles), runtime_error); EXPECT_THROW(db->delete_links(handles), runtime_error); EXPECT_THROW(db->re_index_patterns(), runtime_error); EXPECT_THROW(db->node_count(), runtime_error); EXPECT_THROW(db->link_count(), runtime_error); EXPECT_THROW(db->atom_count(), runtime_error); }
LinkSchemaneeds the matching include and BUILD dep.I can generate the full test additions if that helps.
As per coding guidelines: "Write C++ unit tests in
src/tests/cpp/*_test.ccbuilt with Bazel, testing real behavior not trivial coverage".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/tests/cpp/protected_atomdb_test.cc` around lines 51 - 74, Expand ProtectedAtomDBTest rejection coverage to exercise every listed no-key AtomDB method, including node/link retrieval, queries, existence checks, bulk mutations, re_index_patterns, and node/link counts, using representative handles, collections, Node, Link, and LinkSchema inputs. Add the required LinkSchema include and Bazel dependency. Also add coverage through a shared_ptr<AtomDB> base-interface handle, preserving runtime_error expectations to verify virtual dispatch rejects every operation.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/main/db_loader.cc`:
- Around line 216-218: Define a shared loader_atomdb pointer immediately after
the ProtectedAtomDB unwrapping branch, documenting that it narrowly bypasses
protection for trusted loader operations. Update both the file loader and
generated data loader worker lambdas to capture and use loader_atomdb instead of
calling AtomDBSingleton::get_instance(), including add_atoms, add_nodes,
add_links, and query_for_pattern.
---
Outside diff comments:
In `@src/atomdb/redis_mongodb/RedisMongoDB.h`:
- Around line 44-57: Convert MONGODB_DB_NAME and all context-derived collection
names in RedisMongoDB from class statics to private instance members, initialize
them per backend instance in initialize_statics or the corresponding constructor
path, and update all accesses accordingly so singleton factory updates do not
overwrite other instances. In src/atomdb/redis_mongodb/RedisMongoDB.h lines
44-57, change the declarations and initialization; in
src/atomdb/redis_mongodb/RedisMongoDB.cc lines 34 and 1238-1243, remove static
definitions and update the affected references. Preserve compatible access for
the MorkDB subclass, which derives from RedisMongoDB.
---
Nitpick comments:
In `@src/atomdb/AtomDB.h`:
- Line 26: Document the public is_protected() contract in
src/atomdb/AtomDB.h:26-26 with a brief Doxygen description, and add `@copydoc`
AtomDB::is_protected to the overrides in src/atomdb/adapterdb/AdapterDB.h:65-65,
src/atomdb/inmemorydb/InMemoryDB.h:26-26,
src/atomdb/redis_mongodb/RedisMongoDB.h:35-35,
src/atomdb/remotedb/RemoteAtomDB.h:32-32, and
src/atomdb/remotedb/RemoteAtomDBPeer.h:33-33.
In `@src/atomdb/AtomDBFactory.cc`:
- Line 46: Update the AtomDB factory return to use make_shared for the public
ProtectedAtomDB constructor, forwarding backend with move semantics to avoid an
unnecessary shared_ptr copy while preserving the returned shared_ptr type and
behavior.
In `@src/atomdb/auth/ProtectedAtomDB.cc`:
- Around line 3-4: Define LOG_LEVEL as INFO_LEVEL before including Logger.h in
ProtectedAtomDB.cc, keeping the existing LOG_INFO usage unchanged and matching
the neighboring atomdb .cc files.
In `@src/atomdb/remotedb/RemoteAtomDBPeer.cc`:
- Around line 43-46: Qualify all new member accesses with this-> to follow the
C++ coding guideline: update local_persistence_ and atomdb_ in
RemoteAtomDBPeer::is_protected in src/atomdb/remotedb/RemoteAtomDBPeer.cc lines
43-46, and update remote_db_ in src/atomdb/remotedb/RemoteAtomDB.cc lines 69-75.
In `@src/tests/cpp/adapterdb_test.cc`:
- Around line 254-259: Strengthen AdapterDBTest.IsProtectedDelegatesToBackend by
exercising a backend whose is_protected() returns true, following the
ProtectedInMemoryDB setup pattern from the referenced test. Assert that
AdapterDB delegates and preserves the true result, while retaining the existing
false-case coverage if appropriate.
In `@src/tests/cpp/protected_atomdb_test.cc`:
- Around line 51-74: Expand ProtectedAtomDBTest rejection coverage to exercise
every listed no-key AtomDB method, including node/link retrieval, queries,
existence checks, bulk mutations, re_index_patterns, and node/link counts, using
representative handles, collections, Node, Link, and LinkSchema inputs. Add the
required LinkSchema include and Bazel dependency. Also add coverage through a
shared_ptr<AtomDB> base-interface handle, preserving runtime_error expectations
to verify virtual dispatch rejects every operation.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 2d9ba24d-eea4-4c9d-a266-6c78842911e3
📒 Files selected for processing (24)
src/atomdb/AtomDB.hsrc/atomdb/AtomDBFactory.ccsrc/atomdb/BUILDsrc/atomdb/adapterdb/AdapterDB.ccsrc/atomdb/adapterdb/AdapterDB.hsrc/atomdb/auth/BUILDsrc/atomdb/auth/ProtectedAtomDB.ccsrc/atomdb/auth/ProtectedAtomDB.hsrc/atomdb/inmemorydb/InMemoryDB.hsrc/atomdb/redis_mongodb/RedisMongoDB.ccsrc/atomdb/redis_mongodb/RedisMongoDB.hsrc/atomdb/remotedb/BUILDsrc/atomdb/remotedb/RemoteAtomDB.ccsrc/atomdb/remotedb/RemoteAtomDB.hsrc/atomdb/remotedb/RemoteAtomDBPeer.ccsrc/atomdb/remotedb/RemoteAtomDBPeer.hsrc/main/BUILDsrc/main/db_loader.ccsrc/tests/cpp/BUILDsrc/tests/cpp/adapterdb_test.ccsrc/tests/cpp/atomdb_factory_test.ccsrc/tests/cpp/protected_atomdb_test.ccsrc/tests/cpp/remote_atomdb_test.ccsrc/tests/cpp/test_commons/mocks/MockAtomDB.h
💤 Files with no reviewable changes (1)
- src/atomdb/remotedb/BUILD
|
@coderabbitai full review |
|
|
@coderabbitai full review |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (5)
src/atomdb/ProtectedAtomDB.cc (1)
3-7: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSet the logging level in this source file.
Define
LOG_LEVELbefore includingLogger.h. Add the required file-levelusing namespace std;.Proposed fix
+#define LOG_LEVEL INFO_LEVEL `#include` "Logger.h" `#include` "Utils.h" +using namespace std; using namespace atomdb; using namespace commons;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/atomdb/ProtectedAtomDB.cc` around lines 3 - 7, Define LOG_LEVEL before including Logger.h in ProtectedAtomDB.cc, and add the required file-level using namespace std; alongside the existing namespace declarations.Sources: Coding guidelines, Path instructions
src/atomdb/AtomDB.h (1)
53-53: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument the protection contract.
Add a brief Doxygen block for
is_protected(). Define whattruemeans for composed backends and factory-created databases.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/atomdb/AtomDB.h` at line 53, Add a brief Doxygen comment immediately above the virtual method is_protected() in AtomDB, documenting what a true result means for composed backends and factory-created databases. Keep the protection contract concise and limited to this method.Sources: Coding guidelines, Path instructions
src/atomdb/remotedb/RemoteAtomDBPeer.cc (1)
43-46: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winQualify the new member accesses with
this->.Use explicit member qualification for
local_persistence_andatomdb_. This keeps the new method consistent with the required C++ member-access convention.Proposed fix
bool RemoteAtomDBPeer::is_protected() const { - return (local_persistence_ && local_persistence_->is_protected()) || - (atomdb_ && atomdb_->is_protected()); + return (this->local_persistence_ && this->local_persistence_->is_protected()) || + (this->atomdb_ && this->atomdb_->is_protected()); }As per coding guidelines, access class members with
this->fieldconsistently in C++.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/atomdb/remotedb/RemoteAtomDBPeer.cc` around lines 43 - 46, Update RemoteAtomDBPeer::is_protected() to explicitly qualify both member accesses as this->local_persistence_ and this->atomdb_, preserving the existing protection checks and short-circuit behavior.Sources: Coding guidelines, Path instructions
src/atomdb/adapterdb/AdapterDB.cc (1)
67-70: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a positive delegation test for
AdapterDB::is_protected().The current
src/tests/cpp/adapterdb_test.cchas nois_protected()assertion. A hardcodedfalseimplementation could pass the existing tests. Add a backend test double that returnstrueand assert thatAdapterDB::is_protected()also returnstrue. (raw.githubusercontent.com)As per path instructions, prioritize tests for real behavior and regressions, not trivial coverage padding.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/atomdb/adapterdb/AdapterDB.cc` around lines 67 - 70, Add a positive delegation test in the existing AdapterDB test suite using a backend test double configured to return true from is_protected(), then assert AdapterDB::is_protected() returns true. Reuse the suite’s existing setup and test-double patterns, ensuring the assertion exercises the AdapterDB::is_protected() delegation rather than a hardcoded result.Sources: Path instructions, MCP tools
src/atomdb/inmemorydb/InMemoryDB.h (1)
26-26: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument each new public override.
Add a brief Doxygen block above every new
is_protected()declaration. Use@copydoc AtomDB::is_protectedfor overrides so the public contract remains consistent.
src/atomdb/inmemorydb/InMemoryDB.h#L26-L26: documentInMemoryDB::is_protected().src/atomdb/redis_mongodb/RedisMongoDB.h#L35-L35: documentRedisMongoDB::is_protected().src/atomdb/remotedb/RemoteAtomDB.h#L30-L30: documentRemoteAtomDB::is_protected().src/atomdb/remotedb/RemoteAtomDBPeer.h#L33-L33: documentRemoteAtomDBPeer::is_protected().As per coding guidelines, use brief Doxygen
/** ... */blocks above public API methods in C++ header files.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/atomdb/inmemorydb/InMemoryDB.h` at line 26, Document each public is_protected() override with a brief Doxygen block using `@copydoc` AtomDB::is_protected immediately above the declaration. Apply this to InMemoryDB::is_protected() in src/atomdb/inmemorydb/InMemoryDB.h (lines 26-26), RedisMongoDB::is_protected() in src/atomdb/redis_mongodb/RedisMongoDB.h (lines 35-35), RemoteAtomDB::is_protected() in src/atomdb/remotedb/RemoteAtomDB.h (lines 30-30), and RemoteAtomDBPeer::is_protected() in src/atomdb/remotedb/RemoteAtomDBPeer.h (lines 33-33).Sources: Coding guidelines, Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/atomdb/AtomDBFactory.cc`:
- Around line 118-124: The authorization-aware AtomDB contract is incomplete, so
factory-returned protected instances currently expose only failing no-key/keyed
operations. In src/atomdb/AtomDBFactory.cc lines 118-124, do not wrap backends
until the contract is usable through shared_ptr<AtomDB>; in src/atomdb/AtomDB.h
lines 53-53, add the public authorization context or keyed-operation interface;
in src/atomdb/ProtectedAtomDB.h lines 18-20 and src/atomdb/ProtectedAtomDB.cc
lines 22-168, implement authorization and delegation through the selected
backend; and in src/tests/cpp/protected_atomdb_test.cc lines 57-67, replace
not-implemented assertions with successful authorized delegation and invalid-key
error coverage.
In `@src/atomdb/redis_mongodb/RedisMongoDB.h`:
- Line 44: Convert all context-derived database names, collection names, and
Redis prefixes in RedisMongoDB to private instance members so each backend
retains its own namespace: update src/atomdb/redis_mongodb/RedisMongoDB.h:44 and
:57 to declare and initialize them from context, remove their static definitions
in src/atomdb/redis_mongodb/RedisMongoDB.cc:34, and update protection-state
loading at src/atomdb/redis_mongodb/RedisMongoDB.cc:1238-1244 to use the
instance names; keep only truly global constants static.
In `@src/tests/cpp/BUILD`:
- Around line 853-869: Expand protected_atomdb_test.cc to exercise every no-key
AtomDB method, asserting each rejects access through ProtectedAtomDB. Add a
separate test that stores ProtectedAtomDB in shared_ptr<AtomDB> and invokes the
no-key API through the base interface, verifying virtual dispatch reaches the
rejection overrides; retain the existing rejection expectations and prioritize
missing-override regressions.
---
Nitpick comments:
In `@src/atomdb/adapterdb/AdapterDB.cc`:
- Around line 67-70: Add a positive delegation test in the existing AdapterDB
test suite using a backend test double configured to return true from
is_protected(), then assert AdapterDB::is_protected() returns true. Reuse the
suite’s existing setup and test-double patterns, ensuring the assertion
exercises the AdapterDB::is_protected() delegation rather than a hardcoded
result.
In `@src/atomdb/AtomDB.h`:
- Line 53: Add a brief Doxygen comment immediately above the virtual method
is_protected() in AtomDB, documenting what a true result means for composed
backends and factory-created databases. Keep the protection contract concise and
limited to this method.
In `@src/atomdb/inmemorydb/InMemoryDB.h`:
- Line 26: Document each public is_protected() override with a brief Doxygen
block using `@copydoc` AtomDB::is_protected immediately above the declaration.
Apply this to InMemoryDB::is_protected() in src/atomdb/inmemorydb/InMemoryDB.h
(lines 26-26), RedisMongoDB::is_protected() in
src/atomdb/redis_mongodb/RedisMongoDB.h (lines 35-35),
RemoteAtomDB::is_protected() in src/atomdb/remotedb/RemoteAtomDB.h (lines
30-30), and RemoteAtomDBPeer::is_protected() in
src/atomdb/remotedb/RemoteAtomDBPeer.h (lines 33-33).
In `@src/atomdb/ProtectedAtomDB.cc`:
- Around line 3-7: Define LOG_LEVEL before including Logger.h in
ProtectedAtomDB.cc, and add the required file-level using namespace std;
alongside the existing namespace declarations.
In `@src/atomdb/remotedb/RemoteAtomDBPeer.cc`:
- Around line 43-46: Update RemoteAtomDBPeer::is_protected() to explicitly
qualify both member accesses as this->local_persistence_ and this->atomdb_,
preserving the existing protection checks and short-circuit behavior.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 4b1adccb-0e73-41e8-adbe-774cc5fa4701
📒 Files selected for processing (19)
src/atomdb/AtomDB.hsrc/atomdb/AtomDBFactory.ccsrc/atomdb/BUILDsrc/atomdb/ProtectedAtomDB.ccsrc/atomdb/ProtectedAtomDB.hsrc/atomdb/adapterdb/AdapterDB.ccsrc/atomdb/adapterdb/AdapterDB.hsrc/atomdb/inmemorydb/InMemoryDB.hsrc/atomdb/redis_mongodb/RedisMongoDB.ccsrc/atomdb/redis_mongodb/RedisMongoDB.hsrc/atomdb/remotedb/BUILDsrc/atomdb/remotedb/RemoteAtomDB.hsrc/atomdb/remotedb/RemoteAtomDBPeer.ccsrc/atomdb/remotedb/RemoteAtomDBPeer.hsrc/tests/cpp/BUILDsrc/tests/cpp/atomdb_factory_test.ccsrc/tests/cpp/protected_atomdb_test.ccsrc/tests/cpp/remote_atomdb_test.ccsrc/tests/cpp/test_commons/mocks/MockAtomDB.h
💤 Files with no reviewable changes (1)
- src/atomdb/remotedb/BUILD
1 participant
Summary
ProtectedAtomDBas an authorization wrapper (no-key ops reject; keyed ops stubbed for a follow-up).is_protected()to the AtomDB interface and wire it through RedisMongoDB / peers / AdapterDB ...