Repository navigation
Add database handle for sync database access - #419
srikar-naidu wants to merge 5 commits into
Conversation
|
👋 Hi @srikar-naidu, thanks for contributing! For the review process to begin, please verify that the following is satisfied:
📢✨ Before we assign a reviewer:
Also check that issue requirements are satisfied & you ran Pull requests that don't follow the guidelines will be closed. Reviewer assignment can take up to 2 weeks. |
🟡 Waiting for changesOutcome: reviewed Last updated: 2026-11-09 17:03 UTC |
rtibblesbot
left a comment
rtibblesbot
left a comment
There was a problem hiding this comment.
PR #419: SyncDatabase covers #365 and keeps the default handle's behaviour. CI passing.
- suggestion: the diff carries 10 commits from open PRs #410, #411 and #415, because the base is
release-v0.9.x. Merging this first lands their changes without their own review. Retarget tonet-characterization-363so the diff shows only984f435. - suggestion: a pinned handle's backend keeps the first reading thread's connection (inline).
- suggestion: guard violations in controller-driven syncs surface only as
ERRORED(inline). - suggestion: pinned transaction test never runs the non-isolated branch (inline).
- nitpick: module
begin_transactionrebuilds the backend per call (inline).
Next step: Needs a full code review by a human engineer.
- The author is outside the team: confirm this should be built at all.
- Decide whether pinned
SyncDatabasehandles may cross threads, since the sync stack (#366/#368) will copy this API. - Confirm the stacked PRs #410, #411 and #415 merge before this, or retarget it so only
984f435lands.
Written by rtibblesbot, an LLM-based coding agent.
rtibblesbot
left a comment
rtibblesbot
left a comment
There was a problem hiding this comment.
PR #419: all 4 prior findings are resolved. No new findings. CI is passing.
Prior-finding status
RESOLVED — morango/sync/db.py — pinned backend keeps first thread's connection
RESOLVED — morango/sync/db.py — begin_transaction rebuilds default backend on every call
RESOLVED — tests/testapp/tests/helpers.py — guard violation inside controller-driven sync shows only ERRORED
RESOLVED — tests/testapp/tests/sync/test_db.py — non-isolated branch never run
Next step: Needs a full code review by a human engineer.
- The author is outside the team: confirm this should be built at all.
- Confirm the
SyncDatabasehandle API is the pattern the sync refactor (#365) should build on, since later sync code will copy it. - Check the module-global default backend relying on Django's per-thread
connectionproxy suits how sync runs across threads.
Written by rtibblesbot, an LLM-based coding agent.
bjesterbot
left a comment
bjesterbot
left a comment
There was a problem hiding this comment.
This covers the #365 contract well. The routed default handle keeps today's behaviour: backend returns the module DBBackend, so existing patches of morango.sync.db.DBBackend still work. The pinned handle sends ORM calls, raw SQL, and transactions to its own alias. The diff now holds only the three intended files, because the base is db-handle-365. I found no blockers. The two suggestions below matter mostly because #366/#368 will copy these test patterns.
⚠️ Should-fix:PinnedSyncDatabaseTestCasekeepsTestingRouteractive. Spec §5 asks pinned-handle tests to run withDATABASE_ROUTERS=[](inline).- 💡 Suggestion: the guard has two blind spots that its docstring should state: it only watches the calling thread, and on PostgreSQL it does not see a top-level
atomic()ondefault(inline). - 💡 Nit: the parameter docs for
begin_transactionwere dropped in the move (inline). - ✅ The per-thread backend cache, which rebuilds when
connections[alias]changes, fixes the cross-thread problem without needingConnectionProxy. The guard re-raises a rejected query even when the code under test catches it, which keeps the SQL visible insideSessionController.proceed_to.
Verification
pytest tests/testapp/tests/sync/test_db.py: 10 passed, 1 skipped (the existing PostgreSQL-only isolation test).- Full SQLite suite (
pytest tests/testapp/tests): 666 passed, 2 skipped. This includes the async and legacy network characterization tests, unedited. - The 4 pinned tests also pass with
@override_settings(DATABASE_ROUTERS=[])added locally. So the first suggestion is a one-line change. - I did not run PostgreSQL locally.
Generated by Claude Code using model Claude Opus 5.5
rtibblesbot
left a comment
rtibblesbot
left a comment
There was a problem hiding this comment.
PR #419: all 4 prior findings resolved. One new nitpick inline. CI passing.
Prior-finding status
RESOLVED — morango/sync/db.py — pinned backend keeps the first thread's connection
RESOLVED — morango/sync/db.py — begin_transaction rebuilds the default backend per call
RESOLVED — tests/testapp/tests/helpers.py — guard violation in controller sync shows only ERRORED
RESOLVED — tests/testapp/tests/sync/test_db.py — non-isolated branch never exercised
Next step: Needs a full code review by a human engineer.
- The author is outside the team: confirm this should be built at all.
- Check that legacy-instance matching on
_calculate_0_4_uuidinmorango/models/core.pyis right for every 0.4/0.5 instance already stored on deployed devices. - Check that
SyncDatabaseis the pattern other sync callers should copy for pinned-alias access.
Written by rtibblesbot, an LLM-based coding agent.
bjester
left a comment
bjester
left a comment
There was a problem hiding this comment.
Overall, looks great! Although I'm requesting the removal of the resolution of the MAC-less pathway. I'm aware of those failures, but haven't quite determined the right change for it. For now, we can ignore it.
Thank you for your assistance!
| instance = InstanceIDModel.objects.get(current=True, **kwargs) | ||
| # The legacy fields can also match a modern instance when no MAC is available. | ||
| instance = InstanceIDModel.objects.get( | ||
| current=True, id=_calculate_0_4_uuid(kwargs), **kwargs |
There was a problem hiding this comment.
I suspect this was to resolve the failing test in CI? We already have an issue tracked for it. Unfortunately, this change isn't quite appropriate. Please remove it. The tests are flip-flopping so I'll just keep running them until we get a green.
|
|
||
| @mock.patch("uuid.getnode", return_value=0x010000000000) | ||
| @mock.patch("ifcfg.interfaces", return_value={}) | ||
| def test_system_id_change_without_a_mac_address(self, *args): |
There was a problem hiding this comment.
Obviously, this can be removed as well without corresponding change to its target.
bjester
changed the title
Summary
Adds
SyncDatabaseso sync code can explicitly choose which database it uses. The default handle keeps Django’s existing ORM routing, while a pinned handle uses the specified alias for ORM queries, raw SQL, transactions, and instance counter updates.Also adds a test guard that reports the SQL when a query accidentally reaches the default database.
TODO
Reviewer guidance
The main thing to check is the difference between the two handles. The default handle leaves ORM routing to Django and uses
defaultfor raw SQL and transactions. A pinned handle keeps those operations on its own alias, even when the router points elsewhere.All seven new contract tests and all four existing async and legacy network characterization tests passed. The characterization tests ran without edits. Full SQLite and PostgreSQL verification is still outstanding.
Issues addressed
Closes #365.
Documentation
The
SyncDatabaseand query guard docstrings describe the new behavior. There’s no separate documentation page for this change.AI usage
I used Codex to help map the issue requirements to the existing sync code, implement the handle and query guard, and run the tests. I provided the maintainer’s branch and scope requirements, reviewed the proposed approach, and decided to proceed after discussing how to preserve default routing and verify that pinned operations stay on their alias.