fix: accept PCPS trees in path validation#743
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThe PR addresses an audit finding by refactoring subtree-existence validation in the get operation. Instead of explicitly matching individual tree variants, ChangesSubTree Existence Validation
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #743 +/- ##
========================================
Coverage 91.43% 91.43%
========================================
Files 236 236
Lines 67111 67111
========================================
Hits 61364 61364
Misses 5747 5747
🚀 New features to boost your workflow:
|
|
✅ Review complete (commit 0dbebe9) |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Small, focused fix that swaps the hand-rolled tree-variant match in check_subtree_exists for the centralized Element::is_any_tree() helper, closing the gap where ProvableCountProvableSumTree paths were rejected. Two regression tests cover the realistic entry points. One nitpick: the explicit into_underlying() before is_any_tree() is redundant since is_any_tree() already unwraps via underlying().
💬 1 nitpick(s)
| match element.map(|e| e.into_underlying()) { | ||
| Ok(Element::Tree(..)) | ||
| | Ok(Element::SumTree(..)) | ||
| | Ok(Element::BigSumTree(..)) | ||
| | Ok(Element::CountTree(..)) | ||
| | Ok(Element::CountSumTree(..)) | ||
| | Ok(Element::ProvableCountTree(..)) | ||
| | Ok(Element::ProvableCountSumTree(..)) | ||
| | Ok(Element::ProvableSumTree(..)) | ||
| | Ok(Element::CommitmentTree(..)) | ||
| | Ok(Element::MmrTree(..)) | ||
| | Ok(Element::BulkAppendTree(..)) | ||
| | Ok(Element::DenseAppendOnlyFixedSizeTree(..)) => Ok(()).wrap_with_cost(cost), | ||
| Ok(e) if e.is_any_tree() => Ok(()).wrap_with_cost(cost), | ||
| Ok(_) | Err(Error::PathKeyNotFound(_)) => Err(error_fn()).wrap_with_cost(cost), | ||
| Err(e) => Err(e).wrap_with_cost(cost), | ||
| } |
There was a problem hiding this comment.
💬 Nitpick: Redundant into_underlying() before is_any_tree()
Element::is_any_tree() already invokes self.underlying() internally to look through NonCounted / NotSummed / NotCountedOrSummed wrappers (see grovedb-element/src/element/helpers.rs:323 and :56-63). Calling into_underlying() first re-does that unwrap, and for wrapped variants it performs an owned *inner boxed-element move on every successful subtree check. Since the subsequent arms no longer pattern-match against a specific unwrapped variant, you can pass the element straight to is_any_tree() and let the helper handle wrapper transparency. The comment about looking through NonCounted remains accurate either way.
| match element.map(|e| e.into_underlying()) { | |
| Ok(Element::Tree(..)) | |
| | Ok(Element::SumTree(..)) | |
| | Ok(Element::BigSumTree(..)) | |
| | Ok(Element::CountTree(..)) | |
| | Ok(Element::CountSumTree(..)) | |
| | Ok(Element::ProvableCountTree(..)) | |
| | Ok(Element::ProvableCountSumTree(..)) | |
| | Ok(Element::ProvableSumTree(..)) | |
| | Ok(Element::CommitmentTree(..)) | |
| | Ok(Element::MmrTree(..)) | |
| | Ok(Element::BulkAppendTree(..)) | |
| | Ok(Element::DenseAppendOnlyFixedSizeTree(..)) => Ok(()).wrap_with_cost(cost), | |
| Ok(e) if e.is_any_tree() => Ok(()).wrap_with_cost(cost), | |
| Ok(_) | Err(Error::PathKeyNotFound(_)) => Err(error_fn()).wrap_with_cost(cost), | |
| Err(e) => Err(e).wrap_with_cost(cost), | |
| } | |
| match element { | |
| Ok(ref e) if e.is_any_tree() => Ok(()).wrap_with_cost(cost), | |
| Ok(_) | Err(Error::PathKeyNotFound(_)) => Err(error_fn()).wrap_with_cost(cost), | |
| Err(e) => Err(e).wrap_with_cost(cost), | |
| } |
source: ['claude']
1 participant
Fix PCPS subtree path validation
Summary
check_subtree_existsaccepts allcentralized
Element::is_any_tree()variants after unwrappingNonCountedwrappers.ProvableCountProvableSumTreepaths throughis_empty_tree.DeleteOptions::validate_tree_at_path_existson PCPS subtrees.Closes #710.
Validation
Summary by CodeRabbit
Tests
Refactor