(fix) cqlengine: handle missing table metadata after schema change in… - #744
(fix) cqlengine: handle missing table metadata after schema change in…#744mykaul wants to merge 2 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
Adds resilience to cqlengine schema management by retrying table metadata lookups after forcing a targeted metadata refresh, and introduces unit tests for the _get_table_metadata retry behavior.
Changes:
- Add refresh-and-retry handling for missing table metadata in
_sync_table. - Add refresh-and-retry handling for missing table metadata in
_get_table_metadata. - Add new unit tests covering
_get_table_metadatasuccess/refresh/failure paths.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
cassandra/cqlengine/management.py |
Retries table metadata lookup after cluster.refresh_table_metadata(...) in both _sync_table and _get_table_metadata. |
tests/unit/cqlengine/test_management.py |
New unit tests validating _get_table_metadata behavior when metadata is missing and becomes available (or stays missing). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
6f1be93 to
d5c3f18
Compare
30a8fd7 to
59408e4
Compare
59408e4 to
0b48dd1
Compare
📝 WalkthroughWalkthrough
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Comment |
|
Rebased onto current `master` (no conflicts; the only cqlengine-adjacent changes on master since this PR was branched — the SimpleStrategy→NetworkTopologyStrategy rename and an unused-imports cleanup — don't touch the modified lines). Re-verified the outstanding thread from @dkropachev: the test module docstring was fully genericized to Test results: Re: CI — all required checks (Docs build, Integration tests across libev/asyncio/asyncore, Python 3.11–3.14) pass. The "Test wheels building" workflow shows Force-pushed the rebased branch (same code changes, updated commit base) to keep the two logical commits intact per review history. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (2)
tests/unit/cqlengine/test_management.py:157
- As above, this patch target does not exist because
issubclasscomes from builtins. The decorator therefore raisesAttributeErrorbefore the exception-propagation behavior is exercised.
@patch("cassandra.cqlengine.management.issubclass", return_value=True)
tests/unit/cqlengine/test_management.py:122
- This test never reaches
_sync_table:issubclassis resolved from Python builtins and is not an attribute ofcassandra.cqlengine.management, so this patch raisesAttributeErrorwhen the test starts. Create the module-level override explicitly (or use a realModelsubclass).
This issue also appears on line 157 of the same file.
@patch("cassandra.cqlengine.management.issubclass", return_value=True)
![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
🤖 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 `@cassandra/cqlengine/management.py`:
- Around line 439-447: The refresh-and-lookup flow around
cluster.refresh_table_metadata must convert driver refresh failures into
contextual CQLEngineException errors, and the final metadata KeyError must be
re-raised with exception chaining via from exc. Update the affected
management.py block accordingly, and add a test in
tests/unit/cqlengine/test_management.py covering a refresh-time DriverException
and asserting the contextual CQLEngineException.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: ec041138-c781-49bd-88a8-5cdaadefcc5d
📒 Files selected for processing (2)
cassandra/cqlengine/management.pytests/unit/cqlengine/test_management.py
… sync_table After CREATE TABLE or ALTER TABLE, the local metadata cache may not yet contain the new table if schema agreement timed out or the automatic metadata refresh was skipped. _sync_table() and _get_table_metadata() unconditionally accessed cluster.metadata.keyspaces[ks].tables[table], which raised KeyError in this case. Wrap both lookups in try/except KeyError. On miss, force a targeted cluster.refresh_table_metadata() call and retry once. If the table is still not present after the forced refresh, raise a descriptive CQLEngineException instead of a bare KeyError. This follows the same defensive pattern already used in _sync_type(), which calls cluster.refresh_user_type_metadata() after CREATE TYPE. Add unit tests for _get_table_metadata verifying: immediate hit (no refresh), successful retry after refresh, and failure after refresh. Also wrap the refresh_table_metadata() call itself in try/except: it can raise a DriverException (e.g. if schema agreement fails), which was previously left to propagate uncaught. Catch it and re-raise as a contextual CQLEngineException, matching the exception path used for the "still missing after refresh" case. Chain both re-raises with `from exc` so the original driver error/KeyError is preserved in the traceback instead of being replaced by a bare raise. Add a unit test verifying that a DriverException raised during the refresh itself results in a CQLEngineException, not an unhandled driver error.
Drop the detailed description from the test_management.py docstring, keeping only the general module-level summary.
0b48dd1 to
d028b2b
Compare
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 `@tests/unit/cqlengine/test_management.py`:
- Around line 67-79: The retry test around _get_table_metadata must make
metadata appear only as a consequence of refresh_table_metadata, not merely on
the second property access; update the refresh mock to mutate the mocked tables
state, and assert the existing refresh call. In the create-path test at
tests/unit/cqlengine/test_management.py:155-168, make execute() mutate metadata
similarly and assert execute() occurs before _get_table_metadata().
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: aec3857d-c8e0-4d4c-89a0-9246e4ffc409
📒 Files selected for processing (2)
cassandra/cqlengine/management.pytests/unit/cqlengine/test_management.py
🚧 Files skipped from review as they are similar to previous changes (1)
- cassandra/cqlengine/management.py
| # First lookup: table not in tables dict. After refresh: table is there. | ||
| tables_first = {} | ||
| tables_after = {"test_table": table_meta} | ||
| ks_meta = MagicMock() | ||
| type(ks_meta).tables = PropertyMock(side_effect=[tables_first, tables_after]) | ||
| cluster.metadata.keyspaces = {"test_ks": ks_meta} | ||
| mock_get_cluster.return_value = cluster | ||
|
|
||
| model = self._make_model() | ||
| result = _get_table_metadata(model) | ||
|
|
||
| self.assertIs(result, table_meta) | ||
| cluster.refresh_table_metadata.assert_called_once_with("test_ks", "test_table") |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Make metadata availability depend on the mocked operation.
The retry test returns tables_after on the second property access, independent of refresh_table_metadata(). A regression that performs the second lookup before refresh can pass. The create-path test also does not assert that execute() occurs before _get_table_metadata().
Make the refresh and create mocks mutate the metadata state. Then assert the required call order.
📍 Affects 1 file
tests/unit/cqlengine/test_management.py#L67-L79(this comment)tests/unit/cqlengine/test_management.py#L155-L168
🤖 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 `@tests/unit/cqlengine/test_management.py` around lines 67 - 79, The retry test
around _get_table_metadata must make metadata appear only as a consequence of
refresh_table_metadata, not merely on the second property access; update the
refresh mock to mutate the mocked tables state, and assert the existing refresh
call. In the create-path test at
tests/unit/cqlengine/test_management.py:155-168, make execute() mutate metadata
similarly and assert execute() occurs before _get_table_metadata().
… sync_table
After CREATE TABLE or ALTER TABLE, the local metadata cache may not yet contain the new table if schema agreement timed out or the automatic metadata refresh was skipped. _sync_table() and _get_table_metadata() unconditionally accessed cluster.metadata.keyspaces[ks].tables[table], which raised KeyError in this case.
Wrap both lookups in try/except KeyError. On miss, force a targeted cluster.refresh_table_metadata() call and retry once. If the table is still not present after the forced refresh, raise a descriptive CQLEngineException instead of a bare KeyError.
This follows the same defensive pattern already used in _sync_type(), which calls cluster.refresh_user_type_metadata() after CREATE TYPE.
Add unit tests for _get_table_metadata verifying: immediate hit (no refresh), successful retry after refresh, and failure after refresh.
Pre-review checklist
./docs/source/.Fixes:annotations to PR description.