fix: document query start after #2347
Conversation
WalkthroughThe changes introduce a new struct Changes
Possibly related PRs
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Outside diff range and nitpick comments (12)
packages/rs-drive/tests/supporting_files/contract/withdrawals/withdrawals-contract.json (4)
2-9: Document the versioning strategy and restriction modesThe schema lacks documentation for several important fields:
- The meaning of
$format_version"0"- The purpose of the
versionfield- The implications of
creationRestrictionMode: 2Adding documentation for these fields would improve maintainability.
11-72: Consider documenting index usage patterns and reviewing index performanceThe schema defines multiple indices but lacks documentation about:
- The intended use case for each index
- Expected query patterns
- Performance implications
The
poolingindex includes 4 fields which might impact query performance. Consider:
- Documenting the necessity of each field in this composite index
- Analyzing if all fields are required for the common query patterns
86-91: Document the business logic behind amount constraintsThe minimum amount of 1000 seems arbitrary. Please document:
- The reason for this specific minimum value
- The unit of measurement (is it in Duffs?)
99-108: Add descriptions for pooling enum valuesWhile the
statusfield's enum values are documented, thepoolingfield's enum values (0,1,2) lack descriptions. Please document what each pooling level represents.packages/rs-drive/src/query/mod.rs (2)
152-163: Consider using anenumfor theincludedflag to improve clarityReplacing the
included: boolfield with anenum(e.g.,Inclusion) can enhance readability and reduce the chance of misuse by making the code more self-documenting.Apply this diff to introduce the
Inclusionenum:+#[derive(Debug, Clone)] +pub enum Inclusion { + Include, + Exclude, +} pub struct StartAtDocument<'a> { pub document: Document, pub document_type: DocumentTypeRef<'a>, - pub included: bool, + pub inclusion: Inclusion, }
Line range hint
1510-1773: Consider refactoring recursive functions to improve readability and maintainabilityThe
recursive_insert_on_queryandrecursive_conditional_insert_on_queryfunctions contain complex nested logic and lengthy code blocks. Refactoring them into smaller, modular helper functions can enhance readability and make the code easier to maintain and test.packages/rs-drive/tests/query_tests.rs (6)
774-775: Handle potential errors duringto_valueconversionIn the
add_withdrawals_to_contractfunction, the conversion fromdomaintovalueusingplatform_value::to_value(domain)assumes success without error handling. It's good practice to handle potential errors to make the code more robust.Consider handling the Result:
-for domain in withdrawals { - let value = platform_value::to_value(domain).expect("expected value"); +for withdrawal in withdrawals { + let value = match platform_value::to_value(withdrawal) { + Ok(v) => v, + Err(e) => { + // Handle the error appropriately + eprintln!("Error converting withdrawal to value: {}", e); + continue; + } + };
870-875: Parameterize the contract JSON file pathThe path
"tests/supporting_files/contract/withdrawals/withdrawals-contract.json"is hardcoded in thesetup_withdrawal_testsfunction. Consider parameterizing this path or adding error handling to make the code more flexible and to handle cases where the file might not be found.You could modify the function signature to accept a path parameter:
-pub fn setup_withdrawal_tests( +pub fn setup_withdrawal_tests<P: AsRef<Path>>( count: u32, total_owners: Option<u32>, seed: u64, + contract_path: P, ) -> (Drive, DataContract) { // ... let contract = setup_contract( &drive, - "tests/supporting_files/contract/withdrawals/withdrawals-contract.json", + contract_path.as_ref(), None, Some(&db_transaction), ); // ...
4682-4684: Avoid unnecessary cloning of valuesIn the code snippet, cloning the value may be unnecessary if a reference suffices. Cloning can have performance implications, especially in tight loops or with large data structures.
Consider using a reference instead:
-let normalized_label_value = document - .get("normalizedLabel") - .cloned() - .unwrap_or(Value::Null); +let normalized_label_value = document + .get("normalizedLabel") + .unwrap_or(&Value::Null);
5022-5201: Refactor duplicated code in test functionsThe test functions
test_withdrawals_query_by_owner_idandtest_withdrawals_query_start_after_query_by_owner_idcontain similar setup and query execution code. Refactoring common code into helper functions would reduce duplication and improve maintainability.For example, you can create a helper function:
fn execute_withdrawal_query( drive: &Drive, contract: &DataContract, query_value: serde_json::Value, transaction: TransactionArg, ) -> Result<Vec<Document>, Error> { let query_cbor = cbor_serializer::serializable_value_to_cbor(&query_value, None)?; let document_type = contract .document_type_for_name("withdrawal") .expect("contract should have a withdrawal document type"); let query = DriveDocumentQuery::from_cbor( &query_cbor, contract, document_type, &drive.config, )?; let (results, _, _) = query.execute_raw_results_no_proof( drive, None, Some(transaction), PlatformVersion::latest(), )?; let documents = results .iter() .map(|bytes| Document::from_bytes(bytes, document_type, PlatformVersion::latest())) .collect::<Result<Vec<_>, _>>()?; Ok(documents) }Then, in your test functions, you can call this helper:
fn test_withdrawals_query_by_owner_id() { // ... let query_value = json!({ "where": [ ["$ownerId", "==", "A8GdKdMT7eDvtjnmMXe1Z3YaTtJzZdxNDRkeLb8goFrZ"] ], "limit": 2 }); let documents = execute_withdrawal_query(&drive, &contract, query_value, &db_transaction) .expect("query should be executed"); // Assertions... }
694-695: Avoid mutating the timestamp in a non-monotonic wayIn the
random_withdrawalsfunction,next_timestampis incremented by a potentially zero value (rng.gen_range(0..3) * 2000), which might result in duplicatecreated_attimestamps.Consider ensuring that
next_timestampalways increases by at least 1 to maintain uniqueness:-next_timestamp += rng.gen_range(0..3) * 2000; +next_timestamp += 2000 + rng.gen_range(0..2) * 2000;
867-867: Handle potential errors ingrove_apply_batchWhile applying the batch in
setup_withdrawal_tests, the code usesexpectto handle errors. For test setups, it's acceptable, but consider proper error handling to prevent panics in production code.Consider returning a
Resultfrom the function and handling the error appropriately.-drive +let result = drive .grove_apply_batch(batch, false, Some(&db_transaction), &platform_version.drive) - .expect("expected to create contracts tree successfully"); + .map_err(|e| { + eprintln!("Error applying batch: {}", e); + e + })?;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
packages/rs-drive/src/query/mod.rs(12 hunks)packages/rs-drive/tests/query_tests.rs(15 hunks)packages/rs-drive/tests/supporting_files/contract/withdrawals/withdrawals-contract.json(1 hunks)
🔇 Additional comments (5)
packages/rs-drive/src/query/mod.rs (4)
Line range hint 921-993: Error handling logic is properly implemented
The error handling correctly maps different GroveDB errors to more descriptive QuerySyntaxError variants, providing informative messages based on the context.
1275-1284: Updated function correctly utilizes StartAtDocument
The change to accept Option<&StartAtDocument> and the pattern matching are appropriate, enhancing code clarity and reducing parameter complexity.
1340-1351: Function now properly accepts StartAtDocument for improved maintainability
Updating the inner_query_from_starts_at function to use Option<&StartAtDocument> simplifies the interface and makes the code more maintainable.
1384-1423: Updated recursive_create_query function signature enhances clarity
Changing the starts_at_document parameter to Option<&StartAtDocument> streamlines the function signature and improves readability by encapsulating related parameters into a single struct.
packages/rs-drive/tests/query_tests.rs (1)
645-712: Ensure generated status values correspond to defined statuses
In the random_withdrawals function, the status is generated using rng.gen_range(1..=4) for statuses other than 0 (Pending). Ensure that all generated status values correspond to valid statuses defined in the status field's enum to prevent undefined behavior.
You can verify the status values used in the codebase with the following script:
✅ Verification successful
Let me search for any enum definitions or constants that define these status values to ensure they are properly documented and used consistently.
Let me search specifically in the test file and surrounding files for any comments or documentation about the withdrawal status values.
Let me check if there are any actual usages of these status values in test assertions or conditions to understand the valid status values better.
Status values appear to be valid but would benefit from explicit documentation
Based on the verification results:
- The code comment indicates status is an enum with
0 - Pendingand other values - The implementation correctly handles status
0asPendingwith specific logic - All generated status values are within 0-4 range as confirmed by grep
- No conflicting status values were found in the codebase
While the values are valid, the specific meaning of status values 1-4 should be documented in the code comments for better maintainability.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that all status values correspond to defined statuses (0 to 4)
# Expected statuses are between 0 and 4 inclusive
grep -E 'status\s*:\s*u8\s*,' packages/rs-drive/tests/query_tests.rs
grep -E 'status\s*=\s*(\d+)' packages/rs-drive/tests/query_tests.rs | awk '{print $NF}' | grep -Ev '^[0-4]$' || echo "All status values are within the valid range."
Length of output: 376
Script:
#!/bin/bash
# Search for enum or constant definitions related to withdrawal status
rg -A 5 "enum.*Status|const.*STATUS" packages/rs-drive/
Length of output: 695
Script:
#!/bin/bash
# Search for comments or documentation about withdrawal status values
rg -B 2 -A 2 "Status.*[0-4]|[0-4].*Status" packages/rs-drive/tests/
Length of output: 514
Script:
#!/bin/bash
# Search for test assertions or conditions using status values
rg -B 2 -A 2 "status.*==|assert.*status" packages/rs-drive/tests/query_tests.rs
Length of output: 361
packages/rs-drive/tests/supporting_files/contract/withdrawals/withdrawals-contract.json
Show resolved
Hide resolved
packages/rs-drive/tests/supporting_files/contract/withdrawals/withdrawals-contract.json
Show resolved
Hide resolved
packages/rs-drive/tests/supporting_files/contract/withdrawals/withdrawals-contract.json
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (7)
packages/rs-drive/tests/query_tests.rs (2)
438-460: Consider using an enum for the status fieldThe status field is currently defined as
u8with a comment indicating possible values (0 - Pending, 1 - Signed, etc.). While using primitive types in test code is acceptable (as per learnings), defining an enum would still improve code readability and make the valid status values more explicit in the code.pub enum WithdrawalStatus { Pending = 0, Signed = 1, // Add other statuses (2,3,4) with descriptive names } pub struct Withdrawal { // ... pub status: WithdrawalStatus, }
663-694: Consider using a more descriptive timestamp baseThe base timestamp (1732192259000) is hardcoded without explanation. Consider using a constant with a descriptive name or adding a comment explaining the significance of this timestamp.
-let mut next_timestamp = 1732192259000; +// Base timestamp: 2024-11-21 12:30:59 UTC +const BASE_WITHDRAWAL_TIMESTAMP: i64 = 1732192259000; +let mut next_timestamp = BASE_WITHDRAWAL_TIMESTAMP;packages/rs-drive/src/query/mod.rs (5)
144-163: SimplifyStartAtDocumentby removing unnecessary lifetimesThe
StartAtDocument<'a>struct uses a lifetime parameter'adue todocument_type: DocumentTypeRef<'a>. IfDocumentTypeRefdoes not require a lifetime parameter (e.g., if it's an owned type or if the data does not reference external content), consider removing the lifetime to simplify the struct and reduce complexity.
Line range hint
921-936: Provide a more descriptive error whenstartAtDocumentis missingCurrently, if
grove_getreturnsNone, the code results in aDriveError::CorruptedCodeExecution("expected a value"). This could be confusing during debugging. Consider providing a more descriptive error message or handling theNonecase explicitly. For example:.ok_or(Error::Drive(DriveError::CorruptedCodeExecution( - "expected a value", + "startAt or startAfter document not found in GroveDB", )))?;Alternatively, handle the
Nonecase separately:let start_at_document = match start_at_document { Some(document) => document, None => { let error_message = if self.start_at_included { "startAt document not found" } else { "startAfter document not found" }; return Err(Error::Query(QuerySyntaxError::StartDocumentNotFound(error_message))); } };
1275-1284: Simplify pattern matching ininner_query_from_starts_at_for_idYou can avoid dereferencing
*includedby matchingincludedby value in the pattern:fn inner_query_from_starts_at_for_id( starts_at_document: Option<&StartAtDocument>, left_to_right: bool, ) -> Query { // We only need items after the start at document let mut inner_query = Query::new_with_direction(left_to_right); - if let Some(StartAtDocument { - document, included, .. - }) = starts_at_document + if let Some(StartAtDocument { + document, + included: &included, + .. + }) = starts_at_document { let start_at_key = document.id().to_vec(); if included { inner_query.insert_range_from(start_at_key..) } else { inner_query.insert_range_after(start_at_key..) } } else { // No starts at document, take all NULL items inner_query.insert_all(); } inner_query }This adjustment eliminates the need to dereference
*included, making the code cleaner.
1340-1351: Simplify pattern matching ininner_query_from_starts_atSimilarly, you can adjust the pattern matching in
inner_query_from_starts_atto avoid dereferencing*included:fn inner_query_from_starts_at( starts_at_document: Option<&StartAtDocument>, indexed_property: &IndexProperty, left_to_right: bool, platform_version: &PlatformVersion, ) -> Result<Query, Error> { let mut inner_query = Query::new_with_direction(left_to_right); if let Some(StartAtDocument { document, document_type, - included, + included: &included, }) = starts_at_document { // We only need items after the start at document let start_at_key = document.get_raw_for_document_type( indexed_property.name.as_str(), *document_type, None, platform_version, )?; // We want to get items starting at the start key if let Some(start_at_key) = start_at_key { if left_to_right { if included { inner_query.insert_range_from(start_at_key..) } else { inner_query.insert_range_after(start_at_key..) } } else if included { inner_query.insert_range_to_inclusive(..=start_at_key) } else { inner_query.insert_range_to(..start_at_key) } } else if left_to_right { inner_query.insert_all(); } else { inner_query.insert_key(vec![]); } } else { // No starts at document, take all NULL items inner_query.insert_all(); } Ok(inner_query) }This change simplifies the code by removing the need to dereference
*included.
Line range hint
1384-1773: Refactor lengthy methods for improved maintainabilityThe methods
recursive_create_queryandrecursive_insert_on_queryspan several hundred lines with deep nesting and complex control flow. This can make the code difficult to read, understand, and maintain.Consider refactoring these methods by:
- Breaking down complex functions: Split large functions into smaller, well-named helper functions that handle specific tasks.
- Reducing nesting levels: Simplify nested
matchandifstatements to flatten the structure where possible.- Extracting repeated logic: Identify and extract repeated patterns into reusable functions or methods.
- Adding documentation: Provide clear comments and documentation for complex logic to aid future developers.
Refactoring will enhance code readability, make debugging easier, and reduce the potential for bugs.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
packages/rs-drive/src/query/mod.rs(11 hunks)packages/rs-drive/tests/query_tests.rs(15 hunks)
🧰 Additional context used
📓 Learnings (1)
packages/rs-drive/tests/query_tests.rs (2)
<retrieved_learning>
Learnt from: QuantumExplorer
PR: #2347
File: packages/rs-drive/tests/query_tests.rs:674-678
Timestamp: 2024-11-25T01:17:20.306Z
Learning: In test code, it's acceptable to assign status field random values without explicitly defining all possible values.
</retrieved_learning>
<retrieved_learning>
Learnt from: QuantumExplorer
PR: #2347
File: packages/rs-drive/tests/query_tests.rs:438-460
Timestamp: 2024-11-25T01:17:02.001Z
Learning: In Rust test files (packages/rs-drive/tests/query_tests.rs), when code is used only in tests, defining explicit enums for fields (like the status field in the Withdrawal struct) may not be necessary; using primitive types is acceptable.
</retrieved_learning>
🔇 Additional comments (6)
packages/rs-drive/tests/query_tests.rs (6)
681-687: LGTM: Well-structured transaction index logic
The transaction index and sign height are correctly handled based on the withdrawal status. The implementation ensures:
- Pending withdrawals (status 0) have no transaction details
- Non-pending withdrawals get sequential indices and random sign heights
- Sign heights are within a reasonable range (1-500000)
847-884: LGTM: Well-structured test setup function
The setup function follows the established pattern for test initialization:
- Properly initializes drive with default config
- Sets up contract structure in a transaction
- Adds test data with appropriate error handling
- Commits transaction only after successful setup
5024-5100: LGTM: Comprehensive withdrawal query test
The test thoroughly verifies:
- Basic querying by owner ID
- Proper limit handling
- Result verification including root hash checks
5104-5201: LGTM: Well-structured pagination test
The test effectively verifies:
- Pagination using startAfter parameter
- Proper handling of status-based filtering
- Correct ordering of results
5205-5305: LGTM: Thorough descending order test
The test comprehensively verifies:
- Descending order with startAfter
- Proper handling of timestamps
- Correct result ordering
702-703: Verify hardcoded values align with business requirements
The core_fee_per_byte and pooling fields are hardcoded to 0 in the random generation. Ensure this aligns with the actual business requirements and consider adding a comment explaining why these values are fixed to 0.
✅ Verification successful
Based on the search results, I can now generate the final response:
Hardcoded values are aligned with business requirements
The core_fee_per_byte and pooling values being set to 0 in the test are correct and align with the business requirements:
core_fee_per_byte: 0- This is valid for testing as the codebase shows that core fee per byte must be a Fibonacci number (1, 2, 3, 5, 8, etc.) in production, with 0 being used only in testspooling: 0- This maps toPooling::Neverenum value which is a valid business state, as seen in multiple test cases and implementations across the codebase
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if there are any non-zero values used for these fields in tests or implementation
# Search for core_fee_per_byte usage
rg -A 5 "core_fee_per_byte.*[1-9]"
# Search for pooling usage
rg -A 5 "pooling.*[1-9]"
Length of output: 76771
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
packages/rs-drive/src/query/mod.rs (1)
Line range hint
1506-1673: Consider breaking down the recursive query implementation.The recursive_insert_on_query function is quite complex and handles multiple responsibilities. Consider:
- Breaking it down into smaller, focused functions
- Adding debug assertions for key invariants
- Improving error messages with more context
This would enhance maintainability and debugging.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
packages/rs-drive/src/query/mod.rs(12 hunks)
🔇 Additional comments (3)
packages/rs-drive/src/query/mod.rs (3)
144-163: Well-structured and documented StartAtDocument struct!
The struct effectively encapsulates the starting conditions for document queries with clear field documentation and appropriate lifetime parameters.
1275-1284: Clean function signature updates using StartAtDocument!
The function signatures have been improved to use the new StartAtDocument struct, enhancing type safety and code clarity while maintaining backward compatibility.
Also applies to: 1340-1351
Line range hint 1427-1632: Excellent documentation with clear examples!
The documentation effectively explains complex hierarchical querying concepts using:
- Clear visual tree structure examples
- Detailed explanations of conditional subqueries
- Practical examples that illustrate the concepts
Issue being fixed or feature implemented
Fixing issue #2303
What was done?
When using an index that had subfields we needed to use conditional subqueries on the corresponding values of the start at/after document.
Consider a hierarchical index structure representing car models:
In this example, suppose we are querying for SUVs starting after the document 29f8e... under the Escape car type of Ford. The areas marked with * denote where conditional subqueries are needed. We need to apply conditional subqueries at each level (brand_name, car_type) to ensure that the query correctly starts after the specified document.
By applying conditional subqueries:
• At the brand_name level: We include Ford and apply a condition to start after the specific car_type and document.
• At the car_type level: We include Escape and apply a condition to start after the document 29f8e....
• At deeper levels: The query continues without the start at condition, as it has been applied at the necessary levels.
This ensures that the query retrieves all SUVs of brand Ford and car type Escape starting after the specified document, accurately reflecting the user’s request.
How Has This Been Tested?
Added unit tests.
Breaking Changes
This should not be breaking.
Checklist:
For repository code-owners and collaborators only
Summary by CodeRabbit
Summary by CodeRabbit
New Features
StartAtDocumentstruct for improved query definitions based on specific documents.Withdrawalstruct to manage withdrawal transactions with relevant fields.Bug Fixes
Documentation
Refactor