[#1177] Create new class AtomDBFactory - #1220
Conversation
|
@coderabbitai full review |
✅ Action performedFull review finished. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds ChangesAtomDB factory integration
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant db_loader
participant AtomDBFactory
participant RemoteAtomDB
participant AtomDBSingleton
db_loader->>AtomDBFactory: create(config, context, should_wrap)
AtomDBFactory->>RemoteAtomDB: assemble configured peers and local persistence
RemoteAtomDB-->>AtomDBFactory: return remote backend
AtomDBFactory-->>db_loader: return shared_ptr<AtomDB>
db_loader->>AtomDBSingleton: provide(backend)
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: 2
🧹 Nitpick comments (1)
src/atomdb/AtomDBFactory.cc (1)
8-9: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd the required file-level
stdnamespace directive.
AtomDBFactory.ccusesshared_ptr,string, andmake_sharedwithout a file-levelusing namespace std;. It currently relies onAtomDBFactory.hto export that namespace. Add the directive in this source file.Proposed change
+using namespace std; using namespace atomdb; using namespace commons;As per coding guidelines,
src/**/*.ccmust use file-levelusing namespace stdand domainusing namespacelines.🤖 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` around lines 8 - 9, Add a file-level using namespace std; directive in AtomDBFactory.cc alongside the existing atomdb and commons namespace directives, so shared_ptr, string, and make_shared resolve without relying on AtomDBFactory.h.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/remotedb/RemoteAtomDB.cc`:
- Around line 27-43: Update the context initialization in the RemoteAtomDB peer
creation flow to derive the fallback context from each peer’s uid instead of
using the shared "remotedb_" value, while preserving explicitly configured
contexts. Ensure the derived context is reused for local_persistence and the
remote AtomDBFactory::create call, and add a regression test covering two peers
without configured contexts that verifies their persistence remains isolated.
In `@src/tests/cpp/atomdb_factory_test.cc`:
- Around line 25-65: Extend the AtomDBFactory construction coverage with a valid
morkdb configuration, asserting that create_backend returns a MorkDB instance,
and add the required direct Bazel dependency if MorkDB.h is included. Also add
behavior-focused coverage for direct factory creation and backend compatibility,
reusing the existing test helpers and assertion style.
---
Nitpick comments:
In `@src/atomdb/AtomDBFactory.cc`:
- Around line 8-9: Add a file-level using namespace std; directive in
AtomDBFactory.cc alongside the existing atomdb and commons namespace directives,
so shared_ptr, string, and make_shared resolve without relying on
AtomDBFactory.h.
🪄 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: f91c28d9-81c3-4aa7-a2a2-a301116dee17
📒 Files selected for processing (17)
src/atomdb/AtomDBFactory.ccsrc/atomdb/AtomDBFactory.hsrc/atomdb/AtomDBSingleton.ccsrc/atomdb/BUILDsrc/atomdb/adapterdb/AdapterDB.ccsrc/atomdb/adapterdb/BUILDsrc/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
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/atomdb/remotedb/RemoteAtomDB.cc (2)
73-75: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
this->remote_db_inRemoteAtomDB::is_protected().Line 74 accesses a class member without
this->. Match the existingcomposite_type_enabled()implementation.Proposed change
- for (auto& [uid, peer] : remote_db_) { + for (auto& [uid, peer] : this->remote_db_) {As per coding guidelines, use
this->fieldconsistently in C++. As per path instructions, usethis->for member access insrc/**/*.{h,cc}.🤖 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 73 - 75, Update RemoteAtomDB::is_protected() to access the remote_db_ member through this->remote_db_, matching the style used by composite_type_enabled() and the repository’s C++ member-access convention.Sources: Coding guidelines, Path instructions
73-81: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winAdd focused tests for
RemoteAtomDB::is_protected().Test empty peers, unprotected peers, one protected peer, and mixed protected and unprotected peers. Also verify the result used by
AtomDBFactory::wrap_if_protected().As per path instructions, behavior changes in
src/require matching tests undersrc/tests/cpp/.🤖 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 73 - 81, Add focused C++ tests under src/tests/cpp/ for RemoteAtomDB::is_protected(), covering empty peers, all-unprotected peers, a single protected peer, and mixed peer protection states. Also verify AtomDBFactory::wrap_if_protected() uses the reported protection result correctly, reusing existing test fixtures and helpers where available.Source: 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/main/db_loader.cc`:
- Around line 76-83: Update the AtomDB loading branch around
AtomDBSingleton::provide to preserve AtomDBFactory::create() protection
semantics for remotedb, adapterdb, and composite backends, ensuring configured
storage is wrapped or inspected before registration. Add C++ coverage for
protected and unprotected configurations, or, if the loader is intentionally
privileged, document and test the raw-storage contract.
---
Nitpick comments:
In `@src/atomdb/remotedb/RemoteAtomDB.cc`:
- Around line 73-75: Update RemoteAtomDB::is_protected() to access the
remote_db_ member through this->remote_db_, matching the style used by
composite_type_enabled() and the repository’s C++ member-access convention.
- Around line 73-81: Add focused C++ tests under src/tests/cpp/ for
RemoteAtomDB::is_protected(), covering empty peers, all-unprotected peers, a
single protected peer, and mixed peer protection states. Also verify
AtomDBFactory::wrap_if_protected() uses the reported protection result
correctly, reusing existing test fixtures and helpers where available.
🪄 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: 9cf26528-d8ee-4ff3-9760-055b0ff73942
📒 Files selected for processing (3)
src/atomdb/adapterdb/AdapterDB.ccsrc/atomdb/remotedb/RemoteAtomDB.ccsrc/main/db_loader.cc
🚧 Files skipped from review as they are similar to previous changes (1)
- src/atomdb/adapterdb/AdapterDB.cc
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/atomdb/adapterdb/AdapterDB.cc (1)
38-40: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winPreserve the populated-backend invariant for factory construction.
The only active config constructor calls
initialize(true). This bypasses the check that rejects a non-empty backend for an unpersisted mapping context. A factory-createdAdapterDBcan then write adapter output into existing backend data.Use
initialize(false)for production construction. Keep an explicit test-only opt-out if tests require it. Add a regression test with an unpersisted context and a populated injected 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/adapterdb/AdapterDB.cc` around lines 38 - 40, Change the active AdapterDB constructor to call initialize(false) so factory-created instances reject populated backends for unpersisted mapping contexts. Preserve any explicit test-only opt-out, and add a regression test covering an unpersisted context with a populated injected backend.Source: 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 81-84: Update the remote peer loop in AtomDBFactory so an empty
UID from JsonConfig::at_path("uid") causes RAISE_ERROR(...) instead of silently
continuing. Add a test covering a configured remote peer without uid and assert
that construction or initialization throws, while preserving normal handling for
peers with valid UIDs.
In `@src/atomdb/remotedb/RemoteAtomDB.h`:
- Line 22: Update the RemoteAtomDB class documentation to remove the claim that
construction expects a JSON config. Document AtomDBFactory as the configuration
entry point, and identify the remaining peer-map constructor as the
dependency-injection entry point.
In `@src/tests/cpp/atomdb_factory_test.cc`:
- Around line 104-110: Extend
AtomDBFactoryTest.CreateAdapterDBRequiresBackendType with a valid
adapterdb.atomdb_backend configuration and assert AtomDBFactory::create
succeeds. Exercise a representative operation on the returned AdapterDB to
verify delegation, and add the direct AdapterDB Bazel dependency only if the
test includes its header.
---
Outside diff comments:
In `@src/atomdb/adapterdb/AdapterDB.cc`:
- Around line 38-40: Change the active AdapterDB constructor to call
initialize(false) so factory-created instances reject populated backends for
unpersisted mapping contexts. Preserve any explicit test-only opt-out, and add a
regression test covering an unpersisted context with a populated injected
backend.
🪄 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: 30bc8930-48fb-4904-bd47-97cbb2bdae97
📒 Files selected for processing (18)
src/atomdb/AtomDB.hsrc/atomdb/AtomDBFactory.ccsrc/atomdb/AtomDBFactory.hsrc/atomdb/AtomDBSingleton.ccsrc/atomdb/BUILDsrc/atomdb/adapterdb/AdapterDB.ccsrc/atomdb/adapterdb/AdapterDB.hsrc/atomdb/adapterdb/BUILDsrc/atomdb/remotedb/RemoteAtomDB.ccsrc/atomdb/remotedb/RemoteAtomDB.hsrc/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.ccsrc/tests/cpp/remote_atomdb_test.cc
💤 Files with no reviewable changes (1)
- src/atomdb/adapterdb/BUILD
🚧 Files skipped from review as they are similar to previous changes (4)
- src/tests/cpp/redis_mongodb_test_2.cc
- src/tests/cpp/adapterdb_test.cc
- src/tests/cpp/redis_mongodb_test.cc
- src/atomdb/BUILD
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/atomdb_factory_test.cc`:
- Around line 130-168: Update the AdapterDB integration test setup around
mapping_path, the AtomDBFactory::create context, and the test Node name to
incorporate unique_marker in each resource identifier. Replace the fixed mapping
filename with a marker-based path and use marker-derived factory context and
node names so concurrent or previous runs cannot share mapping files or
persisted MorkDB state.
🪄 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: 20987971-14c3-4cf3-8a7b-db917cf0d4b1
📒 Files selected for processing (3)
src/atomdb/AtomDBFactory.ccsrc/tests/cpp/BUILDsrc/tests/cpp/atomdb_factory_test.cc
🚧 Files skipped from review as they are similar to previous changes (1)
- src/tests/cpp/BUILD
| AtomDBType type = parse_atomdb_type(atomdb_type); | ||
|
|
||
| if (type == AtomDBType::RedisMongoDB) { | ||
| return shared_ptr<AtomDB>(new RedisMongoDB(context, false, config)); |
There was a problem hiding this comment.
Avoid instantiating shared_ptr and unique_ptr using constructor. Use make_shared whenever possible.
Please review this in all the code, I'll not mark this issue anymore in this revision.
|
|
||
| private: | ||
| friend class AtomDBFactory; | ||
| friend class MorkDB; |
There was a problem hiding this comment.
Why does MorkDB need to be a friend class?
There was a problem hiding this comment.
Because of this code snippet
// --> MorkDB : RedisMongoDB(context, skip_redis = true)
MorkDB::MorkDB(const string& context, const JsonConfig& config) : RedisMongoDB(context, true, config) {
mork_setup(config);
}
This causes a compilation error if it is not a friend class
…ith static helpers Remove the should_wrap flag so create() always goes through wrap_if_protected, and expose AtomDB::string_to_type/type_to_string. Update callers and tests accordingly.
2 participants
Summary
AtomDBFactoryto create leaf AtomDB backends (redismongodb,morkdb,inmemorydb) from config.RedisMongoDB/MorkDB/InMemoryDBconstruction with factory calls inAtomDBSingleton,AdapterDB,RemoteAtomDB, anddb_loader.RedisMongoDBconstructor private so backends are created only via the factory (andMorkDB).wrap_if_protected()hook (stub for now) for future authorization wrapping.