Skip to content

[#1177] ProtectedAtomDB - #1216

Open
marcocapozzoli wants to merge 9 commits into
masterfrom
masc/1177-atomdb-auth
Open

[#1177] ProtectedAtomDB#1216
marcocapozzoli wants to merge 9 commits into
masterfrom
masc/1177-atomdb-auth

Conversation

@marcocapozzoli

@marcocapozzoli marcocapozzoli commented Aug 3, 2026

Copy link
Copy Markdown
Collaborator

Starts AtomDB authorization support.

  1. is_protected()
    • New method on the AtomDB interface
    • Each backend answers in its own way:
      • RedisMongoDB reads from the Mongo config collection (protected: true)
      • InMemoryDB always returns false
      • RemoteAtomDB / peer / AdapterDB delegate to the underlying backend
  2. ProtectedAtomDB
    • Wrapper around any protected AtomDB
    • Without public_key: blocked
    • With public_key: signature is ready, but the real implementation (authorize + delegate) comes in the next PR
    • For now, keyed methods still raise "not implemented yet"
  3. AtomDBFactory
    • Single entry point to create backends (redismongodb, morkdb, inmemorydb)
    • If is_protected() is true, wraps with ProtectedAtomDB
    • That's why the RedisMongoDB constructor is private: creation goes through the factory (and MorkDB)
    • Avoids scattering new RedisMongoDB(...) and forgetting the wrapper

@marcocapozzoli marcocapozzoli self-assigned this Aug 3, 2026
@coderabbitai

coderabbitai Bot commented Aug 3, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Warning

Review limit reached

@marcocapozzoli, you've reached your PR review limit, so we couldn't start this review.

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 @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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 configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 4127b80e-626a-4caa-ae0c-9f93e7e07ae4

📥 Commits

Reviewing files that changed from the base of the PR and between feb1b4c and a00b4f1.

📒 Files selected for processing (1)
  • src/tests/cpp/remote_atomdb_test.cc
  • Adds AtomDB::is_protected() and implements protection-state reporting across in-memory, Redis/MongoDB, adapter, and remote backends. Redis/MongoDB loads the state from configuration data during initialization.

  • Adds AtomDBFactory and ProtectedAtomDB. The factory centralizes backend creation and wraps protected backends once during setup. ProtectedAtomDB rejects unkeyed access, but keyed operations also raise runtime_error instead of delegating, so authorized access is incomplete.

  • Code-quality risks: Protection state can differ between remote peers without synchronization or validation. Factory errors and null-backend rejection can fail initialization. Shared-pointer wrapping adds ownership and indirection overhead.

  • Hot-path impact: Backend wrapping occurs during initialization. Protected operations add virtual delegation and wrapper checks, but the PR does not introduce repeated backend allocation or copying on unprotected hot paths.

  • Test coverage: Tests cover factory behavior, protection-state loading, remote protection propagation, wrapper idempotence, and rejection of keyless and currently unimplemented keyed access. Tests do not cover successful keyed authorization, protected data operations, or client request integration.

Walkthrough

Changes

The AtomDB interface now exposes is_protected(). Backends propagate protection state. ProtectedAtomDB enforces key-based access. AtomDBFactory centralizes backend creation and wrapping. Singleton, loader, remote, adapter, benchmark, and test paths use the factory.

Protected AtomDB support

Layer / File(s) Summary
Protection-status contract and backend propagation
src/atomdb/AtomDB.h, src/atomdb/adapterdb/*, src/atomdb/inmemorydb/*, src/atomdb/redis_mongodb/*, src/atomdb/remotedb/*, src/tests/cpp/test_commons/mocks/*
AtomDB implementations expose protection status. RedisMongoDB loads the status from its configuration collection. Remote backends aggregate status from their peers and persistence backends.
ProtectedAtomDB wrapper
src/atomdb/auth/*
ProtectedAtomDB adds keyed overloads, rejects keyless operations, delegates backend capabilities, and reports protection.
Centralized backend creation
src/atomdb/AtomDBFactory.*, src/atomdb/adapterdb/*, src/atomdb/remotedb/*, src/atomdb/BUILD
AtomDBFactory creates supported backends and applies protection wrapping. AdapterDB and RemoteAtomDB use the shared factory.
Initialization, loader, and test integration
src/atomdb/AtomDBSingleton.cc, src/main/*, src/tests/*
Singleton and loader initialization use factory-based construction. Benchmarks and database tests use factory-created backends. Tests cover creation, wrapping, protection propagation, and context fallback.

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
Loading

Possibly related PRs

  • singnet/das#1180: Modifies overlapping AtomDB capability interfaces and implementations.
  • singnet/das#1181: Modifies overlapping AtomDB capability interfaces and backend delegation.
  • singnet/das#1206: Modifies AtomDB protection handling and ProtectedAtomDB wrapping.

Suggested reviewers: ccgsnet

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Tests For Behavior Changes ✅ Passed Production behavior changes are covered by new and updated C++ tests, including atomdb_factory_test, atomdb_protection_test, remote_atomdb_test, adapterdb_test, and redis_mongodb_test.
Title check ✅ Passed The title clearly identifies the main change: adding ProtectedAtomDB authorization support.
Description check ✅ Passed The description directly explains the new protection interface, wrapper, factory, backend behavior, and planned authorization work.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch masc/1177-atomdb-auth

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 7

🧹 Nitpick comments (4)
src/atomdb/AtomDB.h (1)

26-26: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Document 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 of true.
  • 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 win

Use explicit member access for remote_db_.

Change Line 91 to use this->remote_db_.

As per coding guidelines, "Access class members with this->field consistently 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 value

Use this->backend for 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->field consistently 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 win

Add a Bazel-backed test for the wrapper behavior.

This cohort adds a production class with no *_test.cc. AtomDBMock already mocks is_protected, so meaningful cases are cheap:

  • is_protected() returns true regardless of the backend value.
  • allow_nested_indexing() and composite_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::init wraps a protected backend and leaves an unprotected backend unwrapped.

Do you want me to generate the src/tests/cpp/protected_atomdb_test.cc file and the cc_test target?

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

📥 Commits

Reviewing files that changed from the base of the PR and between 1be99f7 and eb51342.

📒 Files selected for processing (16)
  • src/atomdb/AtomDB.h
  • src/atomdb/AtomDBSingleton.cc
  • src/atomdb/BUILD
  • src/atomdb/adapterdb/AdapterDB.cc
  • src/atomdb/adapterdb/AdapterDB.h
  • src/atomdb/auth/BUILD
  • src/atomdb/auth/ProtectedAtomDB.cc
  • src/atomdb/auth/ProtectedAtomDB.h
  • src/atomdb/inmemorydb/InMemoryDB.h
  • src/atomdb/redis_mongodb/RedisMongoDB.cc
  • src/atomdb/redis_mongodb/RedisMongoDB.h
  • src/atomdb/remotedb/RemoteAtomDB.cc
  • src/atomdb/remotedb/RemoteAtomDB.h
  • src/atomdb/remotedb/RemoteAtomDBPeer.cc
  • src/atomdb/remotedb/RemoteAtomDBPeer.h
  • src/tests/cpp/test_commons/mocks/MockAtomDB.h

Comment thread src/atomdb/AtomDBSingleton.cc Outdated
Comment thread src/atomdb/auth/ProtectedAtomDB.cc Outdated
Comment thread src/atomdb/auth/ProtectedAtomDB.h
Comment thread src/atomdb/auth/ProtectedAtomDB.h Outdated
Comment thread src/atomdb/auth/ProtectedAtomDB.h Outdated
Comment thread src/atomdb/redis_mongodb/RedisMongoDB.cc
Comment thread src/atomdb/remotedb/RemoteAtomDB.cc

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

📥 Commits

Reviewing files that changed from the base of the PR and between eb51342 and c75e72d.

📒 Files selected for processing (19)
  • src/atomdb/AtomDBFactory.cc
  • src/atomdb/AtomDBFactory.h
  • src/atomdb/AtomDBSingleton.cc
  • src/atomdb/BUILD
  • src/atomdb/adapterdb/AdapterDB.cc
  • src/atomdb/adapterdb/BUILD
  • src/atomdb/auth/ProtectedAtomDB.cc
  • src/atomdb/auth/ProtectedAtomDB.h
  • src/atomdb/redis_mongodb/RedisMongoDB.h
  • src/atomdb/remotedb/BUILD
  • src/atomdb/remotedb/RemoteAtomDB.cc
  • src/main/BUILD
  • src/main/db_loader.cc
  • src/tests/benchmark/atomdb/atomdb_main.cc
  • src/tests/cpp/BUILD
  • src/tests/cpp/adapterdb_test.cc
  • src/tests/cpp/atomdb_factory_test.cc
  • src/tests/cpp/redis_mongodb_test.cc
  • src/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

Comment thread src/atomdb/AtomDBFactory.cc
Comment thread src/atomdb/remotedb/RemoteAtomDB.cc
Comment thread src/tests/cpp/atomdb_factory_test.cc
Cover is_protected() across backends, ProtectedAtomDB access rules, and RemoteAtomDB factory construction with context fallback.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

📥 Commits

Reviewing files that changed from the base of the PR and between c75e72d and f6377ca.

📒 Files selected for processing (9)
  • src/atomdb/auth/ProtectedAtomDB.cc
  • src/atomdb/auth/ProtectedAtomDB.h
  • src/tests/cpp/BUILD
  • src/tests/cpp/adapterdb_test.cc
  • src/tests/cpp/atomdb_factory_test.cc
  • src/tests/cpp/atomdb_protection_test.cc
  • src/tests/cpp/inmemorydb_test.cc
  • src/tests/cpp/redis_mongodb_test.cc
  • src/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

Comment thread src/tests/cpp/adapterdb_test.cc
Comment thread src/tests/cpp/atomdb_protection_test.cc
Comment thread src/tests/cpp/remote_atomdb_test.cc Outdated
Comment thread src/tests/cpp/remote_atomdb_test.cc Outdated

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (1)
src/tests/cpp/remote_atomdb_test.cc (1)

832-858: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win

Exercise 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 constructed RemoteAtomDBPeer. The test can pass if JSON construction drops or misconfigures local_persistence while direct peer routing remains correct.

Configure a separate unprotected RedisMongoDB local-persistence context in peer_json, seed it, and call factory_db.get_atom(handle). A successful keyless lookup then proves that the factory-installed local persistence is selected before ProtectedAtomDB::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

📥 Commits

Reviewing files that changed from the base of the PR and between f6377ca and feb1b4c.

📒 Files selected for processing (3)
  • src/tests/cpp/BUILD
  • src/tests/cpp/adapterdb_test.cc
  • src/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

Comment thread src/tests/cpp/remote_atomdb_test.cc
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant