feat!: add should_add_parent_tree_at_path feature to PathQuery#379
Conversation
WalkthroughA new feature was introduced to support conditionally adding parent tree information to query results in GroveDB. This involved adding a new boolean field to query-related structs, updating versioning, implementing logic to check this condition during proof verification, and extending tests to cover the new behavior with and without aggregation. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant GroveDB
participant Query
participant ProofVerifier
Client->>GroveDB: Submit query with subqueries (may set add_parent_tree_on_subquery)
GroveDB->>Query: Build PathQuery with subqueries and flags
GroveDB->>ProofVerifier: Generate proof for query
ProofVerifier->>Query: Call should_add_parent_tree_at_path(path, version)
alt Should add parent tree
ProofVerifier->>Result: Add parent tree info to result set
end
ProofVerifier->>Result: Continue verification
Result-->>Client: Return query results (with or without parent tree info)
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
QuantumExplorer
changed the title
![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
🔭 Outside diff range comments (1)
merk/src/proofs/query/mod.rs (1)
162-164: Critical: Missing encoding for the new field.The
Encodeimplementation encodesleft_to_rightbut doesn't encode the newadd_parent_tree_on_subqueryfield. However, bothDecodeandBorrowDecodeimplementations expect to decode this field, which will cause decoding failures.Add the encoding for the new field:
// Encode the left_to_right boolean self.left_to_right.encode(encoder)?; + // Encode the add_parent_tree_on_subquery boolean + self.add_parent_tree_on_subquery.encode(encoder)?; + Ok(())
🧹 Nitpick comments (4)
grovedb/src/tests/query_tests.rs (1)
3429-3437: Avoid scattering the same default value across hundreds of testsEvery
SubqueryBranchliteral in this file now repeatsadd_parent_tree_on_subquery: false,to satisfy the new field. When the default is
false, a helper/ctor (orDefault+ struct-update syntax) keeps the tests resilient to future signature changes and removes boilerplate:-SubqueryBranch { - subquery_path: None, - subquery: Some(Query::new_range_full().into()), - add_parent_tree_on_subquery: false, -} +SubqueryBranch::new(None, Some(Query::new_range_full()))(where
newcould internally set the flag tofalse).
This single change would eliminate hundreds of literal updates and make the intent clearer.merk/src/proofs/query/mod.rs (2)
194-196: Fix the misleading comment.The comment says "Decode the left_to_right boolean" but this code is actually decoding the
add_parent_tree_on_subqueryfield.- // Decode the left_to_right boolean + // Decode the add_parent_tree_on_subquery boolean let add_parent_tree_on_subquery = bool::decode(decoder)?;
235-237: Fix the misleading comment.The comment says "Decode the left_to_right boolean" but this code is actually decoding the
add_parent_tree_on_subqueryfield.- // Decode the left_to_right boolean + // Decode the add_parent_tree_on_subquery boolean let add_parent_tree_on_subquery = bool::borrow_decode(decoder)?;grovedb/src/query/mod.rs (1)
308-428: Consider adding documentation for complex path traversal logic.The implementation is correct, but the recursive path matching logic is complex. Consider adding inline comments to explain:
- The path length comparison strategy in the main function
- The subquery path matching logic in the recursive function
- Examples of how the method behaves with different path configurations
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
grovedb-version/src/version/grovedb_versions.rs(1 hunks)grovedb-version/src/version/v1.rs(1 hunks)grovedb-version/src/version/v2.rs(1 hunks)grovedb/src/operations/proof/verify.rs(1 hunks)grovedb/src/query/mod.rs(20 hunks)grovedb/src/tests/count_tree_tests.rs(1 hunks)grovedb/src/tests/mod.rs(1 hunks)grovedb/src/tests/query_tests.rs(30 hunks)merk/src/proofs/query/mod.rs(4 hunks)merk/src/proofs/query/verify.rs(2 hunks)
🧰 Additional context used
🧠 Learnings (10)
📓 Common learnings
Learnt from: fominok
PR: dashpay/grovedb#345
File: grovedb/src/operations/delete/v1.rs:19-35
Timestamp: 2025-02-18T08:17:30.690Z
Learning: Refactoring suggestions for legacy code should be avoided as they are considered out of scope for feature PRs. This includes the function `delete_internal_on_transaction` in `grovedb/src/operations/delete/v1.rs` which is legacy code.
grovedb-version/src/version/grovedb_versions.rs (1)
Learnt from: fominok
PR: dashpay/grovedb#345
File: grovedb/src/operations/delete/v1.rs:19-35
Timestamp: 2025-02-18T08:17:30.690Z
Learning: Refactoring suggestions for legacy code should be avoided as they are considered out of scope for feature PRs. This includes the function `delete_internal_on_transaction` in `grovedb/src/operations/delete/v1.rs` which is legacy code.
grovedb-version/src/version/v1.rs (1)
Learnt from: fominok
PR: dashpay/grovedb#345
File: grovedb/src/operations/delete/v1.rs:19-35
Timestamp: 2025-02-18T08:17:30.690Z
Learning: Refactoring suggestions for legacy code should be avoided as they are considered out of scope for feature PRs. This includes the function `delete_internal_on_transaction` in `grovedb/src/operations/delete/v1.rs` which is legacy code.
grovedb-version/src/version/v2.rs (1)
Learnt from: fominok
PR: dashpay/grovedb#345
File: grovedb/src/operations/delete/v1.rs:19-35
Timestamp: 2025-02-18T08:17:30.690Z
Learning: Refactoring suggestions for legacy code should be avoided as they are considered out of scope for feature PRs. This includes the function `delete_internal_on_transaction` in `grovedb/src/operations/delete/v1.rs` which is legacy code.
grovedb/src/operations/proof/verify.rs (3)
Learnt from: pauldelucia
PR: dashpay/grovedb#366
File: grovedb/src/error.rs:7-7
Timestamp: 2025-04-18T06:12:25.554Z
Learning: The `Error::InvalidProofError` in grovedb/src/operations/proof/util.rs is from the merk error handling system, distinct from grovedb's own `Error::InvalidProof` variant. These are separate error types from different crates.
Learnt from: pauldelucia
PR: dashpay/grovedb#366
File: grovedb/src/error.rs:7-7
Timestamp: 2025-04-18T06:12:25.554Z
Learning: The `Error::InvalidProofError` in grovedb/src/operations/proof/util.rs is from the merk error handling system, distinct from grovedb's own `Error::InvalidProof` variant. These are separate error types from different crates.
Learnt from: fominok
PR: dashpay/grovedb#345
File: grovedb/src/operations/delete/v1.rs:19-35
Timestamp: 2025-02-18T08:17:30.690Z
Learning: Refactoring suggestions for legacy code should be avoided as they are considered out of scope for feature PRs. This includes the function `delete_internal_on_transaction` in `grovedb/src/operations/delete/v1.rs` which is legacy code.
grovedb/src/tests/mod.rs (1)
undefined
<retrieved_learning>
Learnt from: fominok
PR: #345
File: grovedb/src/operations/delete/v1.rs:19-35
Timestamp: 2025-02-18T08:17:30.690Z
Learning: Refactoring suggestions for legacy code should be avoided as they are considered out of scope for feature PRs. This includes the function delete_internal_on_transaction in grovedb/src/operations/delete/v1.rs which is legacy code.
</retrieved_learning>
grovedb/src/tests/count_tree_tests.rs (1)
Learnt from: fominok
PR: dashpay/grovedb#345
File: grovedb/src/operations/delete/v1.rs:19-35
Timestamp: 2025-02-18T08:17:30.690Z
Learning: Refactoring suggestions for legacy code should be avoided as they are considered out of scope for feature PRs. This includes the function `delete_internal_on_transaction` in `grovedb/src/operations/delete/v1.rs` which is legacy code.
grovedb/src/tests/query_tests.rs (1)
undefined
<retrieved_learning>
Learnt from: fominok
PR: #345
File: grovedb/src/operations/delete/v1.rs:19-35
Timestamp: 2025-02-18T08:17:30.690Z
Learning: Refactoring suggestions for legacy code should be avoided as they are considered out of scope for feature PRs. This includes the function delete_internal_on_transaction in grovedb/src/operations/delete/v1.rs which is legacy code.
</retrieved_learning>
merk/src/proofs/query/verify.rs (2)
Learnt from: pauldelucia
PR: dashpay/grovedb#366
File: grovedb/src/error.rs:7-7
Timestamp: 2025-04-18T06:12:25.554Z
Learning: The `Error::InvalidProofError` in grovedb/src/operations/proof/util.rs is from the merk error handling system, distinct from grovedb's own `Error::InvalidProof` variant. These are separate error types from different crates.
Learnt from: pauldelucia
PR: dashpay/grovedb#366
File: grovedb/src/error.rs:7-7
Timestamp: 2025-04-18T06:12:25.554Z
Learning: The `Error::InvalidProofError` in grovedb/src/operations/proof/util.rs is from the merk error handling system, distinct from grovedb's own `Error::InvalidProof` variant. These are separate error types from different crates.
grovedb/src/query/mod.rs (1)
Learnt from: fominok
PR: dashpay/grovedb#345
File: grovedb/src/operations/delete/v1.rs:19-35
Timestamp: 2025-02-18T08:17:30.690Z
Learning: Refactoring suggestions for legacy code should be avoided as they are considered out of scope for feature PRs. This includes the function `delete_internal_on_transaction` in `grovedb/src/operations/delete/v1.rs` which is legacy code.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Compilation errors
- GitHub Check: Linting
- GitHub Check: Tests
- GitHub Check: Code Coverage
🔇 Additional comments (12)
grovedb-version/src/version/grovedb_versions.rs (1)
17-18: Verify all manual instantiations now set the new field
should_add_parent_tree_at_pathis added toGroveDBPathQueryMethodVersions.
Any place where this struct is constructed with a literal (e.g.GroveDBPathQueryMethodVersions { terminal_keys: …, merge: …, ..Default::default() }) will still compile, but full literals without a trailing..Default::default()will now fail. Please double-check that all such sites (especially in version constants likeGROVE_V1,GROVE_V2) have been updated or switched to use the spread syntax to avoid compilation breaks.grovedb/src/tests/mod.rs (1)
3989-3989: LGTM: Proper test compatibility updateThe addition of
add_parent_tree_on_subquery: falsecorrectly maintains backward compatibility for this test case. Setting it tofalseis appropriate here since this test focuses on reference verification without needing parent tree inclusion complexity.grovedb-version/src/version/v1.rs (1)
178-184: Confirm version‐number semantics for the new method flag
should_add_parent_tree_at_pathis introduced with value0, which is consistent with how brand-new method IDs are usually initialised in this table.
However, if the feature is conditionally compiled or back-ported, the first release that actually supports the logic might need the number1to avoid ambiguity with nodes built against older code that implicitly had “feature missing = 0”. Please make sure the chosen value is compatible with any on-disk/versioned data you already produced, otherwise consider bumping it.grovedb-version/src/version/v2.rs (1)
180-184: Parallel concern: value0reused in V2Same note as for V1: double-check that assigning
0here does not collide with pre-existing binaries that interpret0as “method absent”.
If V2 is the first protocol that can ever call the method, keeping0is fine; if not, increment to1and document the change.merk/src/proofs/query/verify.rs (2)
388-392:Clonederive addition looks goodAdding
ClonetoProvedKeyOptionalValueis harmless and unblocks users who need to duplicate the result set. No further issues spotted.
448-452: Likewise forProvedKeyValue
Cloneis now derived; implementation stays POD and safe. 👍merk/src/proofs/query/mod.rs (3)
130-131: LGTM! Well-documented field addition.The new
add_parent_tree_on_subqueryfield is properly declared with a clear comment explaining its purpose. The boolean type is appropriate for this flag-like behavior.
202-202: LGTM! Consistent field initialization.The new field is properly included in both
DecodeandBorrowDecodestruct constructors with consistent placement.Also applies to: 243-243
680-680: LGTM! Appropriate default value.Setting
add_parent_tree_on_subquerytofalseby default is the correct behavior, maintaining backward compatibility and requiring explicit opt-in for the new feature.grovedb/src/tests/count_tree_tests.rs (2)
109-209: LGTM! Test correctly validates query behavior without aggregation.The test properly verifies that when
add_parent_tree_on_subqueryis not set, the query returns only the child items without including the parent count tree element.
211-318: LGTM! Test correctly validates query behavior with aggregation enabled.The test properly verifies that when
add_parent_tree_on_subqueryis set to true, the query returns the parent count tree element along with the child items. The count tree element correctly shows a count of 3 and the last key as "innerkey2".grovedb/src/query/mod.rs (1)
1413-1413: LGTM! Test updates maintain backward compatibility.All Query struct initializations have been correctly updated to include
add_parent_tree_on_subquery: false, maintaining the existing test behavior while supporting the new field.Also applies to: 1431-1431, 1596-1596, 1668-1668, 1718-1718, 1736-1736, 1754-1754, 1771-1771, 1900-1900, 1923-1923, 1946-1946, 1978-1978, 2022-2022, 2045-2045, 2070-2070, 2083-2083, 2096-2096, 2123-2123, 2128-2128, 2151-2151
2 participants
Issue being fixed or feature implemented
Added a new feature to the
PathQuerythat determines whether to include the parent tree in the results based on the query path.What was done?
should_add_parent_tree_at_pathinGroveDBPathQueryMethodVersions.GROVE_V1andGROVE_V2constants to initialize this new field.should_add_parent_tree_at_pathmethod in thePathQuerystruct to evaluate whether the parent tree should be included based on the query path and the current grove version.How Has This Been Tested?
New unit tests were added to cover the functionality of the
should_add_parent_tree_at_pathmethod and its integration with existing query functionalities.Breaking Changes
None
Checklist
For repository code-owners and collaborators only
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Refactor