[#1177] ProtectedAtomDB - #1216
Conversation
… is_protected() in the AtomDB and crontect classes.
|
Warning Review limit reached
Next review available in: 46 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
WalkthroughChangesThe AtomDB interface now exposes Protected AtomDB support
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant AtomDBSingleton
participant AtomDBFactory
participant Backend
participant ProtectedAtomDB
AtomDBSingleton->>AtomDBFactory: create(config, context)
AtomDBFactory->>Backend: create_backend(config, context)
Backend-->>AtomDBFactory: return backend
AtomDBFactory->>Backend: query is_protected()
AtomDBFactory->>ProtectedAtomDB: wrap protected backend
ProtectedAtomDB-->>AtomDBSingleton: return final backend
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (4)
src/atomdb/AtomDB.h (1)
26-26: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument the new public
is_protected()declarations.These declarations add one public API contract, but none has a brief Doxygen block. Add a concise contract at every site.
src/atomdb/AtomDB.h#L26-L26: document the base method and define the meaning oftrue.src/atomdb/adapterdb/AdapterDB.h#L65-L65: document delegated backend status.src/atomdb/inmemorydb/InMemoryDB.h#L26-L26: document the constant unprotected result.src/atomdb/redis_mongodb/RedisMongoDB.h#L36-L36: document the MongoDB-loaded status.src/atomdb/remotedb/RemoteAtomDB.h#L32-L32: document aggregate peer semantics.src/atomdb/remotedb/RemoteAtomDBPeer.h#L33-L33: document local-persistence and remote-backend composition.As per coding guidelines, "Use brief Doxygen
/** ... */blocks above public API methods in C++ header files."Suggested base contract
+ /** + * `@brief` Returns whether this backend requires protected access. + */ virtual bool is_protected() const = 0;🤖 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, Add brief Doxygen blocks above each public is_protected() declaration: in src/atomdb/AtomDB.h lines 26-26 define true as the database being protected; in src/atomdb/adapterdb/AdapterDB.h lines 65-65 describe delegation to the backend; in src/atomdb/inmemorydb/InMemoryDB.h lines 26-26 document the constant unprotected result; in src/atomdb/redis_mongodb/RedisMongoDB.h lines 36-36 describe the MongoDB-loaded status; in src/atomdb/remotedb/RemoteAtomDB.h lines 32-32 document aggregate peer semantics; and in src/atomdb/remotedb/RemoteAtomDBPeer.h lines 33-33 document the combination of local persistence and remote-backend protection.Source: Coding guidelines
src/atomdb/remotedb/RemoteAtomDB.cc (1)
90-97: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse explicit member access for
remote_db_.Change Line 91 to use
this->remote_db_.As per coding guidelines, "Access class members with
this->fieldconsistently in C++."Proposed change
- for (auto& [uid, peer] : remote_db_) { + for (auto& [uid, peer] : this->remote_db_) {🤖 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/RemoteAtomDB.cc` around lines 90 - 97, Update the range-based loop in RemoteAtomDB::is_protected() to access the remote_db_ member explicitly as this->remote_db_, following the project’s C++ member-access convention.Source: Coding guidelines
src/atomdb/auth/ProtectedAtomDB.cc (2)
164-166: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse
this->backendfor member access.The constructor parameter at line 11 shares the member name, so explicit
this->also removes the shadowing ambiguity.-bool ProtectedAtomDB::allow_nested_indexing() { return backend->allow_nested_indexing(); } +bool ProtectedAtomDB::allow_nested_indexing() { return this->backend->allow_nested_indexing(); } -bool ProtectedAtomDB::composite_type_enabled() const { return backend->composite_type_enabled(); } +bool ProtectedAtomDB::composite_type_enabled() const { return this->backend->composite_type_enabled(); }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/auth/ProtectedAtomDB.cc` around lines 164 - 166, Update ProtectedAtomDB::allow_nested_indexing and ProtectedAtomDB::composite_type_enabled to access the backend member through this->backend, following the C++ member-access convention and avoiding ambiguity with the constructor parameter.Source: Coding guidelines
168-168: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a Bazel-backed test for the wrapper behavior.
This cohort adds a production class with no
*_test.cc.AtomDBMockalready mocksis_protected, so meaningful cases are cheap:
is_protected()returns true regardless of the backend value.allow_nested_indexing()andcomposite_type_enabled()delegate to the mock backend and return its value.- Each keyless overload throws, and the message names the method and mentions
public_key.AtomDBSingleton::initwraps a protected backend and leaves an unprotected backend unwrapped.Do you want me to generate the
src/tests/cpp/protected_atomdb_test.ccfile and thecc_testtarget?As per coding guidelines: "Test updates are required when production code changes: Add or update C++ tests (*_test.cc under src/tests/cpp/) ... when production code under src/ ... changes behavior".
🤖 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` at line 168, Add Bazel-backed coverage in src/tests/cpp/protected_atomdb_test.cc for ProtectedAtomDB and its AtomDBSingleton::init integration. Test that is_protected() always returns true, allow_nested_indexing() and composite_type_enabled() delegate backend values, every keyless overload throws an error naming the method and public_key, and init wraps protected backends but leaves unprotected ones unchanged. Register the test with an appropriate cc_test target.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/atomdb/AtomDBSingleton.cc`:
- Around line 42-46: Update AtomDBSingleton::init so adapterdb backends are made
ready before calling is_protected(), preventing initialization from failing
during the protection check. Also update AtomDBSingleton::get_instance and
singleton assignment so a ProtectedAtomDB without an authorization key is never
exposed through the existing keyless API; retain the usable underlying instance
or reject protected initialization explicitly.
In `@src/atomdb/auth/ProtectedAtomDB.cc`:
- Line 11: Update the ProtectedAtomDB constructor to validate that the incoming
backend is non-null before storing it, rejecting invalid input immediately while
preserving ownership transfer via std::move. Use the existing project convention
for signaling invalid constructor arguments.
In `@src/atomdb/auth/ProtectedAtomDB.h`:
- Around line 76-107: Update the six keyless add_* declarations in
src/atomdb/auth/ProtectedAtomDB.h: replace throw_if_exists with a defaulted
const atoms::Merger* merger parameter, and order batch parameters as
is_transactional then merger. Apply the same parameter-list changes to the six
keyless definitions in src/atomdb/auth/ProtectedAtomDB.cc so they match the
AtomDB virtual API and declarations.
- Around line 34-44: Update the protected-database access path around
AtomDBSingleton::get_instance and ProtectedAtomDB so callers that provide a
public key can reach the keyed get_atom, get_node, get_link, and
get_matching_atoms operations without requiring a downcast. Expose these
operations through the AtomDB contract or a dedicated protected-access interface
returned by the singleton, and ensure the keyless virtual methods no longer
block the intended protected access flow.
- Line 148: Mark the ProtectedAtomDB::raise_public_key_required declaration as
[[noreturn]] so the compiler recognizes that the helper always throws and
keyless non-void overloads have no fallthrough path.
In `@src/atomdb/redis_mongodb/RedisMongoDB.cc`:
- Around line 1237-1243: Add Bazel-backed C++ tests under src/tests/cpp covering
RedisMongoDB::load_protected_flag() for matching, missing, and non-true
documents; RemoteAtomDB::is_protected() for no, unprotected, and mixed peers;
RemoteAtomDBPeer::is_protected() and AdapterDB::is_protected() delegation;
InMemoryDB::is_protected() returning false; and singleton wrapping plus
ProtectedAtomDB keyless versus authorized access. Register the tests with the
appropriate Bazel target and reuse existing test fixtures or helpers where
available.
In `@src/atomdb/remotedb/RemoteAtomDB.cc`:
- Around line 90-97: Update RemoteAtomDB protection handling so mixed peer
configurations preserve per-peer authorization instead of treating any protected
peer as protecting the entire aggregate. In RemoteAtomDB’s routed operations and
RemoteAtomDBPeer delegation, add and propagate the public_key/protected access
token to the selected peer, requiring authorization for protected peers while
retaining no-key access for unprotected peers. Keep is_protected() consistent
with the resulting routing behavior.
---
Nitpick comments:
In `@src/atomdb/AtomDB.h`:
- Line 26: Add brief Doxygen blocks above each public is_protected()
declaration: in src/atomdb/AtomDB.h lines 26-26 define true as the database
being protected; in src/atomdb/adapterdb/AdapterDB.h lines 65-65 describe
delegation to the backend; in src/atomdb/inmemorydb/InMemoryDB.h lines 26-26
document the constant unprotected result; in
src/atomdb/redis_mongodb/RedisMongoDB.h lines 36-36 describe the MongoDB-loaded
status; in src/atomdb/remotedb/RemoteAtomDB.h lines 32-32 document aggregate
peer semantics; and in src/atomdb/remotedb/RemoteAtomDBPeer.h lines 33-33
document the combination of local persistence and remote-backend protection.
In `@src/atomdb/auth/ProtectedAtomDB.cc`:
- Around line 164-166: Update ProtectedAtomDB::allow_nested_indexing and
ProtectedAtomDB::composite_type_enabled to access the backend member through
this->backend, following the C++ member-access convention and avoiding ambiguity
with the constructor parameter.
- Line 168: Add Bazel-backed coverage in src/tests/cpp/protected_atomdb_test.cc
for ProtectedAtomDB and its AtomDBSingleton::init integration. Test that
is_protected() always returns true, allow_nested_indexing() and
composite_type_enabled() delegate backend values, every keyless overload throws
an error naming the method and public_key, and init wraps protected backends but
leaves unprotected ones unchanged. Register the test with an appropriate cc_test
target.
In `@src/atomdb/remotedb/RemoteAtomDB.cc`:
- Around line 90-97: Update the range-based loop in RemoteAtomDB::is_protected()
to access the remote_db_ member explicitly as this->remote_db_, following the
project’s C++ member-access convention.
🪄 Autofix (Beta)
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: efc09d00-3a9a-40f5-87aa-03063a5d8770
📒 Files selected for processing (16)
src/atomdb/AtomDB.hsrc/atomdb/AtomDBSingleton.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/RemoteAtomDB.ccsrc/atomdb/remotedb/RemoteAtomDB.hsrc/atomdb/remotedb/RemoteAtomDBPeer.ccsrc/atomdb/remotedb/RemoteAtomDBPeer.hsrc/tests/cpp/test_commons/mocks/MockAtomDB.h
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 9-10: Add the file-level using namespace std; directive alongside
the existing atomdb and commons namespace imports in AtomDBFactory.cc.
In `@src/atomdb/remotedb/RemoteAtomDB.cc`:
- Around line 21-43: Add a behavior test covering RemoteAtomDB construction with
a protected peer and an explicitly empty local_persistence context, using the
relevant RemoteAtomDB factory setup. Assert that the operation is routed through
the protected peer and that local persistence uses the peer context as its
fallback when its context is empty.
In `@src/tests/cpp/atomdb_factory_test.cc`:
- Around line 62-74: Extend WrapIfProtectedWrapsOnce by configuring a backend
capability such as AtomDBMock::composite_type_enabled() or
allow_nested_indexing(), then assert the same capability through wrapped after
AtomDBFactory::wrap_if_protected(backend). Keep the existing wrapping,
protection, and idempotence assertions unchanged.
🪄 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: 1b8ac24a-1652-4f7f-b030-2c523c416f5e
📒 Files selected for processing (19)
src/atomdb/AtomDBFactory.ccsrc/atomdb/AtomDBFactory.hsrc/atomdb/AtomDBSingleton.ccsrc/atomdb/BUILDsrc/atomdb/adapterdb/AdapterDB.ccsrc/atomdb/adapterdb/BUILDsrc/atomdb/auth/ProtectedAtomDB.ccsrc/atomdb/auth/ProtectedAtomDB.hsrc/atomdb/redis_mongodb/RedisMongoDB.hsrc/atomdb/remotedb/BUILDsrc/atomdb/remotedb/RemoteAtomDB.ccsrc/main/BUILDsrc/main/db_loader.ccsrc/tests/benchmark/atomdb/atomdb_main.ccsrc/tests/cpp/BUILDsrc/tests/cpp/adapterdb_test.ccsrc/tests/cpp/atomdb_factory_test.ccsrc/tests/cpp/redis_mongodb_test.ccsrc/tests/cpp/redis_mongodb_test_2.cc
🚧 Files skipped from review as they are similar to previous changes (3)
- src/atomdb/redis_mongodb/RedisMongoDB.h
- src/atomdb/auth/ProtectedAtomDB.cc
- src/atomdb/auth/ProtectedAtomDB.h
Cover is_protected() across backends, ProtectedAtomDB access rules, and RemoteAtomDB factory construction with context fallback.
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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/tests/cpp/adapterdb_test.cc`:
- Around line 219-223: Update AdapterDBTest.IsProtectedDelegatesToBackend to
construct the adapter with an injected AtomDBMock configured to return true for
is_protected(), then assert db->is_protected() is true; retain the test’s focus
on delegation rather than the default fixture state.
In `@src/tests/cpp/atomdb_protection_test.cc`:
- Around line 55-62: Replace the deny-all expectations in
AuthorizedOverloadsNotImplementedYet with tests for the documented keyed-access
path: verify ProtectedAtomDB authorizes and delegates get_atom, atom_exists, and
node_count to AtomDBMock, including meaningful returned results. Add coverage
for protection wrapping and unauthorized/error behavior, using the keyed
overloads declared by ProtectedAtomDB rather than expecting runtime_error for
every call.
In `@src/tests/cpp/remote_atomdb_test.cc`:
- Around line 796-798: The test allocates Node objects using new but does not
release them, creating memory leaks. In both test cases where new Node is called
(around the allocations followed by db.add_node), add a delete node statement
immediately after the db.add_node call returns, following the cleanup pattern
demonstrated in chain_operator_test.cc Lines 54-82 where caller-owned nodes are
deleted after being passed to add_node.
- Around line 803-806: Before calling cleanup->drop_all() in the cleanup block
(after line 805), add an assertion that cleanup->atom_exists(handle) returns
true. This validates that the atom was actually persisted in the fallback
RedisMongoDB backend and not just cached in the remote InMemoryDB or peer cache,
confirming the fallback-context behavior works correctly.
🪄 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: 7d32b1b0-d494-45de-9aa6-58420dc9c36a
📒 Files selected for processing (9)
src/atomdb/auth/ProtectedAtomDB.ccsrc/atomdb/auth/ProtectedAtomDB.hsrc/tests/cpp/BUILDsrc/tests/cpp/adapterdb_test.ccsrc/tests/cpp/atomdb_factory_test.ccsrc/tests/cpp/atomdb_protection_test.ccsrc/tests/cpp/inmemorydb_test.ccsrc/tests/cpp/redis_mongodb_test.ccsrc/tests/cpp/remote_atomdb_test.cc
🚧 Files skipped from review as they are similar to previous changes (3)
- src/atomdb/auth/ProtectedAtomDB.cc
- src/tests/cpp/atomdb_factory_test.cc
- src/atomdb/auth/ProtectedAtomDB.h
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/tests/cpp/remote_atomdb_test.cc (1)
832-858: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winExercise the factory-created peer during lookup.
At Lines 832-837, the test only checks status on
factory_db. At Lines 847-856, it reads through a separate manually constructedRemoteAtomDBPeer. The test can pass if JSON construction drops or misconfigureslocal_persistencewhile direct peer routing remains correct.Configure a separate unprotected RedisMongoDB local-persistence context in
peer_json, seed it, and callfactory_db.get_atom(handle). A successful keyless lookup then proves that the factory-installed local persistence is selected beforeProtectedAtomDB::get_atom(handle)rejects the unkeyed remote read.Proposed test structure
const string peer_context = "remote_factory_prot_"; + const string local_context = "remote_factory_prot_local_"; seed_protected_flag(peer_context, true); + string handle = seed_redis_node(local_context, "\"factory_routed\""); + ASSERT_FALSE(handle.empty()); auto peer_json = redis_mongodb_fields(); peer_json["uid"] = "protected_peer"; peer_json["context"] = peer_context; - peer_json["local_persistence"] = {{"type", "inmemorydb"}, {"context", ""}}; + peer_json["local_persistence"] = redis_mongodb_fields(); + peer_json["local_persistence"]["context"] = local_context; RemoteAtomDB factory_db(JsonConfig(nlohmann::json::array({peer_json}))); auto* factory_peer = factory_db.get_peer("protected_peer"); ASSERT_NE(factory_peer, nullptr); EXPECT_FALSE(factory_peer->is_readonly()); EXPECT_TRUE(factory_peer->is_protected()); EXPECT_TRUE(factory_db.is_protected()); - // Manually constructed peer and lookup. + EXPECT_EQ(factory_peer->get_cached_atom(handle), nullptr); + auto got = factory_db.get_atom(handle); + ASSERT_NE(got, nullptr); + EXPECT_EQ(got->handle(), handle); + + drop_redis_context(local_context); drop_redis_context(peer_context);As per coding guidelines, C++ tests must test real behavior rather than trivial coverage. As per path instructions, behavior changes need tests for the changed routing path.
🤖 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/remote_atomdb_test.cc` around lines 832 - 858, Extend the factory-created peer test around RemoteAtomDB factory_db so peer_json configures a separate unprotected RedisMongoDB local-persistence context, seeds the requested atom there, and then calls factory_db.get_atom(handle). Remove reliance on the separately constructed RemoteAtomDB db/peer lookup for this assertion, while preserving checks that factory_db installs the protected peer and returns the seeded atom through its factory-configured local persistence.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/tests/cpp/remote_atomdb_test.cc`:
- Around line 755-760: Update seed_redis_node after the EXPECT_NE check to
return an empty handle immediately when seeder is nullptr, before constructing
the node or calling add_node; preserve the existing successful seeding path for
valid RedisMongoDB backends.
---
Nitpick comments:
In `@src/tests/cpp/remote_atomdb_test.cc`:
- Around line 832-858: Extend the factory-created peer test around RemoteAtomDB
factory_db so peer_json configures a separate unprotected RedisMongoDB
local-persistence context, seeds the requested atom there, and then calls
factory_db.get_atom(handle). Remove reliance on the separately constructed
RemoteAtomDB db/peer lookup for this assertion, while preserving checks that
factory_db installs the protected peer and returns the seeded atom through its
factory-configured local persistence.
🪄 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: 095b7865-2f30-4960-ac0c-ed2786dc769d
📒 Files selected for processing (3)
src/tests/cpp/BUILDsrc/tests/cpp/adapterdb_test.ccsrc/tests/cpp/remote_atomdb_test.cc
🚧 Files skipped from review as they are similar to previous changes (2)
- src/tests/cpp/BUILD
- src/tests/cpp/adapterdb_test.cc
Starts AtomDB authorization support.
is_protected()AtomDBinterfaceRedisMongoDBreads from the Mongo config collection (protected: true)InMemoryDBalways returnsfalseRemoteAtomDB/ peer /AdapterDBdelegate to the underlying backendProtectedAtomDBpublic_key: blockedpublic_key: signature is ready, but the real implementation (authorize + delegate) comes in the next PRAtomDBFactoryredismongodb,morkdb,inmemorydb)is_protected()is true, wraps withProtectedAtomDBRedisMongoDBconstructor is private: creation goes through the factory (andMorkDB)new RedisMongoDB(...)and forgetting the wrapper