v1.3.6: Cascade reach through special-field references (RedisSet/RedisPriorityQueue of ForeignKey) - #289
Open
yedidyakfir wants to merge 11 commits into
Open
v1.3.6: Cascade reach through special-field references (RedisSet/RedisPriorityQueue of ForeignKey)#289yedidyakfir wants to merge 11 commits into
yedidyakfir wants to merge 11 commits into
Conversation
…ntainer discriminator to CascadeEdge + SF-held-ref fixtures - CascadeEdge gains sf_container: str | None = None as its last field, dropped by _drop_none_values/cascade_plan_json so non-SF plan bytes stay identical - Add SF-held-ref test fixtures (RedisSet/RedisPriorityQueue holding Reference[T]) covering per-field, blanket, opt-out, and ttl=None fail-fast cases in tests/models/cascade_types.py - Register the four ttl-carrying fixtures in ALL_CASCADE_MODELS and CASCADE_PLANNER_MODELS
…ef discovery pass in build_cascade_plan + edge tests - Add _static_walk_sf_fk_edges: a dedicated pass over _special_field_names that emits a distinct CascadeEdge (sf_container="set"/"zset") for each cascade-enabled RedisSet/RedisPriorityQueue field holding a Reference[T], honoring field > global > off precedence via _classify_edge - Wire the new pass into build_cascade_plan right after _static_walk_fk_edges, appending to the same entry.fks list - Lazy-import RedisSet/RedisPriorityQueue inside the pass: a module-top import reintroduces a real cycle (priority_queue -> special -> scripts.loader -> planner), contrary to the plan's cycle-safety assumption - Add tests/unit/cascade/test_cascade_sf_held_ref_plan.py covering the new edge shape (set/zset), coexistence with the refresh-only special suffix, precedence, and the None-drop hash-stability guarantee for non-SF edges - Fix: mark the three deliberately-ttl-less SF fail-fast fixtures (CascadeSetRefNoTtlTarget/CascadeSetRefToNoTtl/CascadeSetRefRootNoTtl, added in the prior task-1 commit) init_with_rapyer=False so they are excluded from REDIS_MODELS and don't trip init_rapyer()'s full-model-set ttl validation in unrelated tests
…nested sub-model exclusion in SF-held-ref pass Clarify that _static_walk_sf_fk_edges intentionally handles only direct SF fields and does not recurse into nested inline sub-models (WR-01 from code review). Nested-case traversal is deferred to the server-side work. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Qot6HrjzLLCBhmHaH1CLRM
…verage matrix and worked example - Add Cascade-Eligible Shapes coverage-matrix table (5 shapes) to ttl-cascade.md - Add worked RedisSet cascade example mirroring CascadeSetRefParent fixture - Extend real-Redis-7+/fakeredis divergence note for SF-held-ref cascade
… fixtures - CascadeSetRefSelfNode/CascadePQRefSelfNode: self-ref-in-SET/PQ cycle-safety - CascadeMixedEdgeSharedChild(Root): inline+SF shared-child max-budget-wins - CascadeSfDiamondChild/CascadeSfDiamondRoot: SF-only dual-edge diamond - All six registered in ALL_CASCADE_MODELS; none added to CASCADE_PLANNER_MODELS
…nch in push_edges - push_sf_edge reads SMEMBERS/ZRANGE keyed on edge.sf_container, decodes each member with a guarded pcall(cjson.decode), and feeds valid string target keys through the existing push_child/next_hop/visited machinery - push_edges now splits per-node edges into SF (dispatched immediately) vs inline (still batched through the single JSON.GET); SF edge paths never enter the inline paths array - next_hop is called once per SF edge per node-walk, not per member
…scadeSetRefParent (RedisSet) - Proves CASF-09's fakeredis leg for the SET-shaped SF-held-ref fixture - Real fakeredis, real aset_ttl(cascade=True) call, no mocking - Root main key + refs SF container key refresh; SET member (author) untouched
…scadePQRefParent (RedisPriorityQueue) - Mirrors the RedisSet proof for the ZSET-backed SF-held-ref fixture - Proves both SF container kinds (SET and ZSET) that Phase 1 classified correctly fall back to root-own-EXPIRE on fakeredis
…aversal proof (CASF-04..08) - New test_cascade_sf_held_ref_apply.py: 8 tests (A-H) proving SET reach, PQ reach, dangling-count reuse, self-ref-in-SET/PQ termination, mixed inline+SF max-budget-wins, SF-only dual-edge diamond convergence, and malformed/non-string SF member tolerance -- all against real Redis :6370 - test_cascade_graph_shapes.py, test_cascade_depth_and_gate.py, and tests/unit/cascade/ pass unmodified (CASF-08 byte-for-byte proof) Two auto-fixed bugs surfaced by the new self-ref-in-SET/PQ fixtures (Rule 1/3 -- blocking, discovered exercising this plan's own fixtures for the first time): - [Rule 3] _unwrap_relational_target returned a raw typing.ForwardRef for a self-referencing FK target baked into an SF container's dynamic subclass generic args (RedisConverter._build_redis_subclass captures it at class-body time, before pydantic's model_rebuild can resolve it). Added _resolve_forward_ref, looking the name up in the global model registry, in rapyer/cascade/planner.py. - [Rule 1] RedisSet/RedisPriorityQueue._dump_members called dump_python directly on raw native-Python input (e.g. a bare FK target-key string) without validating first; dump_python never validates, so a ForeignKey element reached its serializer as a plain string and crashed on `_target_key`. Fixed by validating via the adapter before dumping in rapyer/types/redis_set.py and rapyer/types/priority_queue.py -- no behavior change for existing scalar-typed SF fields (verified via the full test suite).
…r gate for SF-only parents - Add class_declares_cascade_enabled_sf_ref_edge() to planner.py, reusing _static_walk_sf_fk_edges's field>global>off classification verbatim - Add lazily-cached _has_cascade_enabled_sf_ref_edge() classmethod + OR it into _contains_foreign_key(), so refresh_ttl/aset_ttl now fire the cascade Function for SF-only cascade-enabled parents - contains_fk_field() and __init_subclass__ left byte-for-byte unmodified - Mock-based unit proof (test_cascade_sf_only_trigger_gate.py): run_fcall fires for CascadeSetRefParent/CascadePQRefParent, still plain pipe.expire for the cascade-disabled CascadeSetRefOptOut
…of for SF-only cascade parents - Test A/C: asave() on CascadeSetRefParent/CascadePQRefParent re-arms an SF-held child's own Meta.ttl through the public save path - Test B: aset_ttl(ttl, cascade=True) re-arms the child directly, returning a CascadeResult with dangling_children=0 - No _apply_cascade/run_fcall helper used anywhere in this file -- proves the fix through the literal public API (asave/aset_ttl), closing CASF-04/05/06 end-to-end (02-01 proved reach only at the direct-FCALL level)
📝 WalkthroughWalkthroughAdds TTL cascade support for foreign-key references stored in ChangesSF-held reference cascade
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant ModelAPI
participant CascadePlanner
participant CascadeLua
participant RedisContainer
ModelAPI->>CascadePlanner: build SF-held FK cascade plan
CascadePlanner-->>ModelAPI: return container-aware CascadeEdge
ModelAPI->>CascadeLua: refresh TTL with cascade
CascadeLua->>RedisContainer: read SET/ZSET members
RedisContainer-->>CascadeLua: return referenced keys
CascadeLua-->>ModelAPI: refresh reachable targets and report results
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
Coverage reportTotal coverage: 99% Full report |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
tests/integration/foreign_keys/test_cascade_sf_held_ref_apply.py (1)
199-205: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winMake the diamond test detect duplicate traversal.
The current TTL and
[0, 0]assertions also pass if the shared child is walked twice. Give the shared child a dangling descendant and assert it contributes exactly one dangling count.🤖 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/integration/foreign_keys/test_cascade_sf_held_ref_apply.py` around lines 199 - 205, Update the diamond test around _apply_cascade to add a dangling descendant beneath the shared child, then assert the result includes exactly one dangling reference from that descendant. Keep the existing TTL assertions and ensure the expected result distinguishes single traversal from duplicate visits.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 `@docs/documentation/special-fields/ttl-cascade.md`:
- Around line 94-128: Convert the Python example in the TTL cascade
documentation from a fenced code block to the repository’s required indented
code-block style, preserving the example content and surrounding explanation
unchanged.
In `@rapyer/scripts/lua/cascade/library.lua`:
- Around line 226-253: Update push_sf_edge to read the SET/ZSET container with
redis.pcall instead of redis.call, preserving the existing command selection.
Treat any failed SMEMBERS or ZRANGE result as an empty member list and return
without aborting traversal, while retaining the current per-member JSON decoding
and push_child behavior for successful reads.
In `@tests/unit/cascade/test_cascade_sf_held_ref_fakeredis_fallback.py`:
- Line 38: Update the TTL assertions for persisted author keys in the fakeredis
fallback test to require exactly -1, including the assertion at the second
occurrence. Remove -2 from the accepted values so child-key deletion cannot be
treated as successful behavior.
---
Nitpick comments:
In `@tests/integration/foreign_keys/test_cascade_sf_held_ref_apply.py`:
- Around line 199-205: Update the diamond test around _apply_cascade to add a
dangling descendant beneath the shared child, then assert the result includes
exactly one dangling reference from that descendant. Keep the existing TTL
assertions and ensure the expected result distinguishes single traversal from
duplicate visits.
🪄 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: e1a8b9ea-a3df-408d-8af9-f6e2f1b1c59b
📒 Files selected for processing (13)
docs/documentation/special-fields/ttl-cascade.mdrapyer/base.pyrapyer/cascade/planner.pyrapyer/scripts/lua/cascade/library.luarapyer/types/priority_queue.pyrapyer/types/redis_set.pytests/integration/foreign_keys/test_cascade_sf_held_ref_apply.pytests/integration/foreign_keys/test_cascade_sf_held_ref_public_api.pytests/models/cascade_types.pytests/unit/cascade/conftest.pytests/unit/cascade/test_cascade_sf_held_ref_fakeredis_fallback.pytests/unit/cascade/test_cascade_sf_held_ref_plan.pytests/unit/cascade/test_cascade_sf_only_trigger_gate.py
Comment on lines
+94
to
+128
| ```python | ||
| from typing import Annotated, ClassVar | ||
|
|
||
| from pydantic import Field | ||
| from rapyer import AtomicRedisModel | ||
| from rapyer.cascade import CascadeTTL | ||
| from rapyer.config import RedisConfig | ||
| from rapyer.types import Reference, RedisSet | ||
|
|
||
|
|
||
| class Author(AtomicRedisModel): | ||
| name: str = "anon" | ||
|
|
||
| Meta: ClassVar[RedisConfig] = RedisConfig(ttl=3600) | ||
|
|
||
|
|
||
| class Library(AtomicRedisModel): | ||
| name: str = "main" | ||
| authors: Annotated[RedisSet[Reference[Author]], CascadeTTL()] = Field( | ||
| default_factory=RedisSet | ||
| ) | ||
|
|
||
| Meta: ClassVar[RedisConfig] = RedisConfig(ttl=3600) | ||
|
|
||
|
|
||
| library = await Library(name="main").asave() | ||
| author = await Author(name="Jane").asave() | ||
| await library.authors.aadd(author.key) | ||
|
|
||
| # Every member of the set is re-armed to its own Meta.ttl, exactly like an | ||
| # inline collection-of-FK field, whenever the parent's TTL is (re)set: | ||
| await library.asave() | ||
| # ...or explicitly: | ||
| await library.aset_ttl(3600, cascade=True) | ||
| ``` |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Use an indented code block for this example.
This new fenced block triggers the active MD046 warning; convert it to the repository’s required indented style.
🧰 Tools
🪛 markdownlint-cli2 (0.23.0)
[warning] 94-94: Code block style
Expected: indented; Actual: fenced
(MD046, code-block-style)
🤖 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 `@docs/documentation/special-fields/ttl-cascade.md` around lines 94 - 128,
Convert the Python example in the TTL cascade documentation from a fenced code
block to the repository’s required indented code-block style, preserving the
example content and surrounding explanation unchanged.
Source: Linters/SAST tools
Comment on lines
+226
to
+253
| -- SF-held-ref edge: read the field's own SET/ZSET key (not the inline | ||
| -- JSON document) and feed each decoded member through push_child. | ||
| -- next_hop is called ONCE per edge here, never per member. | ||
| local function push_sf_edge(parent_key, edge, remaining_budget, established) | ||
| local follow, budget = next_hop(edge, remaining_budget, established) | ||
| if not follow then | ||
| return | ||
| end | ||
| if not edge.recurse_into_target then | ||
| budget = 0 | ||
| end | ||
| local sf_key = special_prefix .. ':' .. parent_key .. ':' .. edge.path | ||
| local members | ||
| if edge.sf_container == 'set' then | ||
| members = redis.call('SMEMBERS', sf_key) | ||
| else | ||
| members = redis.call('ZRANGE', sf_key, 0, -1) | ||
| end | ||
| for _, raw_member in ipairs(members) do | ||
| -- A malformed (non-JSON) or non-string-decoded member is a dead | ||
| -- end for that member only -- never aborts the atomic FCALL. | ||
| local ok, target_key = pcall(cjson.decode, raw_member) | ||
| if ok and type(target_key) == 'string' then | ||
| push_child(target_key, edge, budget) | ||
| end | ||
| end | ||
| end | ||
|
|
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
SET/ZSET reads aren't error-tolerant like the inline JSON.GET path.
push_sf_edge uses redis.call('SMEMBERS'/'ZRANGE', ...) while the inline path (read_reference_paths) deliberately uses redis.pcall('JSON.GET', ...) so a WRONGTYPE/corrupt target becomes "a dead end for further traversal, not an aborted cascade." A WRONGTYPE (or any Redis-level error) on an SF container key here raises and aborts the whole atomic FCALL during the read phase — before any EXPIRE runs — so nothing gets refreshed at all, not even the root. That breaks the same resilience guarantee the rest of the file is careful to uphold.
🔒️ Proposed fix: mirror read_reference_paths' pcall tolerance
local sf_key = special_prefix .. ':' .. parent_key .. ':' .. edge.path
- local members
+ local members
if edge.sf_container == 'set' then
- members = redis.call('SMEMBERS', sf_key)
+ members = redis.pcall('SMEMBERS', sf_key)
else
- members = redis.call('ZRANGE', sf_key, 0, -1)
+ members = redis.pcall('ZRANGE', sf_key, 0, -1)
+ end
+ if type(members) ~= 'table' or members.err then
+ return -- WRONGTYPE/corrupt SF key: dead end for this edge only
end
for _, raw_member in ipairs(members) do📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| -- SF-held-ref edge: read the field's own SET/ZSET key (not the inline | |
| -- JSON document) and feed each decoded member through push_child. | |
| -- next_hop is called ONCE per edge here, never per member. | |
| local function push_sf_edge(parent_key, edge, remaining_budget, established) | |
| local follow, budget = next_hop(edge, remaining_budget, established) | |
| if not follow then | |
| return | |
| end | |
| if not edge.recurse_into_target then | |
| budget = 0 | |
| end | |
| local sf_key = special_prefix .. ':' .. parent_key .. ':' .. edge.path | |
| local members | |
| if edge.sf_container == 'set' then | |
| members = redis.call('SMEMBERS', sf_key) | |
| else | |
| members = redis.call('ZRANGE', sf_key, 0, -1) | |
| end | |
| for _, raw_member in ipairs(members) do | |
| -- A malformed (non-JSON) or non-string-decoded member is a dead | |
| -- end for that member only -- never aborts the atomic FCALL. | |
| local ok, target_key = pcall(cjson.decode, raw_member) | |
| if ok and type(target_key) == 'string' then | |
| push_child(target_key, edge, budget) | |
| end | |
| end | |
| end | |
| -- SF-held-ref edge: read the field's own SET/ZSET key (not the inline | |
| -- JSON document) and feed each decoded member through push_child. | |
| -- next_hop is called ONCE per edge here, never per member. | |
| local function push_sf_edge(parent_key, edge, remaining_budget, established) | |
| local follow, budget = next_hop(edge, remaining_budget, established) | |
| if not follow then | |
| return | |
| end | |
| if not edge.recurse_into_target then | |
| budget = 0 | |
| end | |
| local sf_key = special_prefix .. ':' .. parent_key .. ':' .. edge.path | |
| local members | |
| if edge.sf_container == 'set' then | |
| members = redis.pcall('SMEMBERS', sf_key) | |
| else | |
| members = redis.pcall('ZRANGE', sf_key, 0, -1) | |
| end | |
| if type(members) ~= 'table' or members.err then | |
| return -- WRONGTYPE/corrupt SF key: dead end for this edge only | |
| end | |
| for _, raw_member in ipairs(members) do | |
| -- A malformed (non-JSON) or non-string-decoded member is a dead | |
| -- end for that member only -- never aborts the atomic FCALL. | |
| local ok, target_key = pcall(cjson.decode, raw_member) | |
| if ok and type(target_key) == 'string' then | |
| push_child(target_key, edge, budget) | |
| end | |
| end | |
| end |
🤖 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 `@rapyer/scripts/lua/cascade/library.lua` around lines 226 - 253, Update
push_sf_edge to read the SET/ZSET container with redis.pcall instead of
redis.call, preserving the existing command selection. Treat any failed SMEMBERS
or ZRANGE result as an empty member list and return without aborting traversal,
while retaining the current per-member JSON decoding and push_child behavior for
successful reads.
| assert result == CascadeResult(dangling_children=0, dangling_special=0) | ||
| assert await fake_redis_client.ttl(parent.key) > 0 | ||
| assert await fake_redis_client.ttl(refs_key) > 0 | ||
| assert await fake_redis_client.ttl(author.key) in (-1, -2) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Do not accept deletion as successful fallback behavior.
Each author key was persisted, so its unchanged TTL must be -1. Allowing -2 masks an erroneous child-key deletion.
Proposed fix
- assert await fake_redis_client.ttl(author.key) in (-1, -2)
+ assert await fake_redis_client.ttl(author.key) == -1Also applies to: 63-63
🤖 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/cascade/test_cascade_sf_held_ref_fakeredis_fallback.py` at line
38, Update the TTL assertions for persisted author keys in the fakeredis
fallback test to require exactly -1, including the assertion at the second
occurrence. Remove -2 from the accepted values so child-key deletion cannot be
treated as successful behavior.
Source: Path instructions
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Milestone v1.3.6 — Cascade Reach Through Special-Field References
Extends the v1.3.5 TTL cascade so the shared walker also discovers and re-arms
ForeignKeyreferences held inside special-field containers —RedisSet[ForeignKey[T]]andRedisPriorityQueue[ForeignKey[T]]— which live under their own SET/ZSET keys and were never read by the inline-only (JSON.GET) traversal. Traversal-reach only; the per-child cascading-TTL-refresh apply layer is reused unchanged.Phase 1 — Classify SF-held FK references into the cascade plan
sf_containerdiscriminator onCascadeEdge; SF-held-ref discovery pass inbuild_cascade_plan/_static_walk_sf_fk_edges(precedence: field > global > off; fail-fast on a missing targetMeta.ttl).Phase 2 — Traverse SF-held references server-side and re-arm children
library.lua: newpush_sf_edgebranch reads SET (SMEMBERS) / ZSET (ZRANGE) members server-side, decodes each into its target key, and feeds them through the existingpush_child/next_hop/visitedbudget machinery — so SF-reached children re-arm to their ownMeta.ttlin the same atomic FCALL as inline-reached children.rapyer/base.py):_contains_foreign_key()now ORs in_has_cascade_enabled_sf_ref_edge()soasave/aset_ttl(cascade=True)/refresh_ttlactually fire the cascade Function for parents whose only cascade edge is SF-held (previously they silently took the native-EXPIREfast path).RedisSet/RedisPriorityQueue._dump_membersnow validate before dumping, so a raw FK key string round-trips correctly.Verification
EXPIREfallback.Follow-ups (non-blocking, from code review)
init_with_rapyer=Falsetarget) is silently dropped — no edge, no startup error, cascade silently off. Consider wiring into the fail-fast path.RedisSetvalidates members without the redis context whileRedisPriorityQueuevalidates with it; harmless today, risks divergence for a future context-sensitive inner type.🤖 Generated with Claude Code
https://claude.ai/code/session_01XWA2KXKFrqLo5qnhDC3ACi
Summary by CodeRabbit
New Features
Bug Fixes
Documentation