🐛 Guard against orphaning an alias when deleting an anchor target#185
Conversation
frenck
added
the
bugfix
📝 WalkthroughWalkthroughThis change adds delete-time protection against orphaning YAML aliases in both the single-document and nested-view deletion paths of the round-trip document implementation. New internal helper functions detect aliases, compute dangling alias sets before and after a deletion, and roll back the deletion with a raised error if it would newly orphan an alias. Corresponding tests validate that deleting referenced anchor targets fails, deleting aliases themselves succeeds, nested anchor targets are protected, and unrelated deletions proceed despite pre-existing dangling aliases. No public signatures changed. Sequence Diagram(s)Included above in the hidden review stack artifact. Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
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 |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #185 +/- ##
==========================================
+ Coverage 92.31% 92.34% +0.03%
==========================================
Files 40 40
Lines 11444 11504 +60
==========================================
+ Hits 10564 10623 +59
- Misses 880 881 +1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This PR fixes a mutation-API bug in the round-trip document layer (src/roundtrip/document.rs): deleting a node that carried an &anchor still referenced by a *alias elsewhere produced YAML that no longer parsed. Both YAMLRocksDocument.__delitem__ and YAMLRocksDocumentView.__delitem__ now snapshot the node tree before a delete (only when the document actually contains aliases, keeping the common no-alias case allocation-free), perform the delete, and roll back with a descriptive ValueError if the delete leaves a dangling alias. Deleting the alias itself remains allowed.
Changes:
- Added a snapshot-and-rollback guard to the single-document top-level delete and the nested-view delete paths.
- Added four helper functions (
collect_alias_names,document_has_aliases,first_dangling_alias,dangling_alias_error) to detect orphaned aliases and build the error. - Added Python tests covering rejection of deleting a referenced anchor target and allowing deletion of the alias itself.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
src/roundtrip/document.rs |
Adds the dangling-alias guard to both delete paths plus the supporting helper functions and error message. |
tests/core/test_anchors.py |
Adds tests for the top-level delete guard (target rejected, alias deletion allowed). |
Notes from the review:
- The rollback correctly preserves round-trip fidelity, since the snapshot is cloned before
del_childcallsmark_modified, so the restored nodes keep their original modified flags. - The post-delete check is absolute (any dangling alias) rather than differential, which can misattribute a pre-existing dangling alias (round-trip loading does not validate aliases) to an unrelated delete and emit a misleading error.
- The nested-view guard path is new but untested; only the top-level path has coverage.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Deleting a node that carries an `&anchor` still referenced by a `*alias` elsewhere in the document left the alias dangling, so the re-emitted YAML no longer parsed: silent corruption produced by a mutation the API allowed. `__delitem__` (both the top-level document and a node view) now snapshots the node tree before the delete, but only when the document actually references an alias, so the common no-anchor case stays allocation-free. The check is differential: it records which aliases were already dangling before the delete (the round-trip/compose path does not validate alias targets at load time, so a document may already carry one) and rejects only an alias the delete *newly* orphans, restoring the snapshot and raising a `ValueError` that names the anchor. Removing the alias itself (rather than its target) stays allowed.
frenck
force-pushed
the
frenck/roundtrip-guard-dangling-alias
branch
from
96182d6 to
a5c5270
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/roundtrip/document.rs (2)
174-188: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winGuard logic is correct. Snapshotting before
del_child(pre-mark_modified) means rollback restores a fully clean state, soto_yaml()replays the original bytes verbatim — matching the test that asserts byte-for-byte preservation on a blocked delete.Note: this snapshot/rollback block is duplicated almost verbatim in
YAMLRocksDocumentView.__delitem__(Lines 613-629). Consider extracting a small helper (e.g. taking&mut Vec<YamlNode>plus a deletion closure) so the two guards can't drift out of sync.
1492-1501: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win
document_has_aliasesfully traverses and allocates aHashSetjust to test emptiness, which undercuts the doc comment's claim of keeping the no-alias case cheap. A short-circuiting existence check avoids both the allocation and the full walk on the common path.♻️ Short-circuiting existence check
-fn document_has_aliases(nodes: &[YamlNode]) -> bool { - let mut aliases = std::collections::HashSet::new(); - for node in nodes { - collect_alias_names(node, &mut aliases); - } - !aliases.is_empty() -} +fn document_has_aliases(nodes: &[YamlNode]) -> bool { + nodes.iter().any(node_contains_alias) +} + +fn node_contains_alias(node: &YamlNode) -> bool { + match &node.kind { + YamlNodeKind::Alias(_) => true, + YamlNodeKind::Mapping(pairs) => pairs + .iter() + .any(|(k, v)| node_contains_alias(k) || node_contains_alias(v)), + YamlNodeKind::Sequence(items) => items.iter().any(node_contains_alias), + _ => false, + } +}
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 49442fc8-83a6-4ba4-990f-0d35e3f72e2d
📒 Files selected for processing (2)
src/roundtrip/document.rstests/core/test_anchors.py
Breaking change
None. This only rejects a delete that would produce YAML that no longer parses; it does not change the result of any delete that was already valid.
Proposed change
Deleting a node that carries an
&anchorstill referenced by a*aliaselsewhere in the document left the alias dangling, so the re-emitted YAML no longer parsed. The mutation API allowed it, which turned a valid document into a broken one with no warning.__delitem__(both the top-level document and a node view) now snapshots the node tree before the delete, but only when the document actually references an alias, so the common no-anchor case stays allocation-free. After the delete it checks for a*aliaswith no matching&anchor; if one is found it restores the snapshot and raises aValueErrornaming the anchor, leaving the document untouched. Removing the alias itself (rather than its target) stays allowed.Type of change
Additional information
Checklist
uv run pytestpasses locally. A pull request cannot be merged unless CI is green.uv run ruff check .anduv run ruff format --check .pass.cargo fmt --checkandcargo clippy --all-targets -- -D warningspass.If the change is user-facing:
docs/is added or updated, anddocs/verify_examples.pystill passes.