fix: document query start to support pagination #2284
Conversation
WalkthroughThe pull request introduces significant enhancements to the document querying functionality across multiple modules. Key changes include improved error handling for query parameters, the addition of new utility functions, and expanded test coverage for various scenarios, particularly focusing on proof generation and validation. New methods for storing documents and modifications to existing query methods enhance flexibility and robustness. Overall, these changes aim to improve the reliability and accuracy of document queries within the platform. Changes
Suggested labels
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: 5
🧹 Outside diff range and nitpick comments (10)
packages/rs-drive/tests/supporting_files/contract/withdrawals/withdrawals.json (4)
7-10: Document the creationRestrictionMode values.The
creationRestrictionModeis set to 2, but there's no documentation explaining what different modes represent. Consider adding a comment or description explaining the available modes and their implications.
11-72: Consider index optimization opportunities.The indices are well-structured for pagination support, but consider:
- The
transactionindex might benefit from being unique sincetransactionIndexshould be unique per status.- The
identityStatusindex includes$createdAtwhich might not be necessary if you're already tracking$updatedAtinidentityRecent.
109-115: Document outputScript constraints.The
outputScriptbyte array size constraints (23-25 bytes) seem very specific. Please add documentation explaining these size requirements and their relationship to the underlying protocol.
116-127: Enhance status enum documentation.While the status values have basic descriptions, consider adding:
- Documentation for state transitions (which status can transition to which)
- Timeout/expiration conditions for status changes
- System behavior expectations for each status
packages/rs-sdk/src/platform/document_query.rs (1)
Line range hint
329-362: Consider adding documentation and performance guidelinesThe pagination implementation would benefit from:
- Documentation explaining:
- The pagination behavior and limitations
- The expected format and source of start position values
- The difference between StartAt and StartAfter modes
- Performance considerations:
- Recommended page sizes
- Impact on memory usage when fetching large result sets
Consider adding documentation to the
DocumentQuerystruct:/// Request that is used to query documents from the Dash Platform. /// /// This is an abstraction layer built on top of [GetDocumentsRequest] to address issues with missing details /// required to correctly verify proofs returned by the Dash Platform. /// /// # Pagination /// The query supports pagination through the `start` field which can be used in two modes: /// - `StartAt`: Includes the document at the start position /// - `StartAfter`: Starts from the document after the given position /// /// The start position must be a 32-byte identifier. For optimal performance, /// it's recommended to limit page sizes to X documents.packages/rs-drive-abci/src/query/document_query/v0/mod.rs (2)
729-745: Add documentation for the serialize_vec_to_cbor functionThe function looks well-implemented but lacks documentation explaining its purpose and usage.
Add rustdoc comments:
+/// Serializes a vector of values into CBOR format +/// +/// # Arguments +/// * `input` - Vector of values that implement `Into<Value>` +/// +/// # Returns +/// * `Result<Vec<u8>, Error>` - Serialized CBOR bytes or error +/// +/// # Errors +/// Returns a Protocol error if serialization fails fn serialize_vec_to_cbor<T: Into<Value>>(input: Vec<T>) -> Result<Vec<u8>, Error> {
1400-1401: Address TODO comment for ignored testThere's a TODO comment indicating that this functionality should be possible, but the test is currently ignored.
Would you like help implementing this test case or should I create an issue to track this enhancement?
packages/rs-drive/src/query/mod.rs (2)
1278-1310: LGTM with a note about the TODO comment.The changes to handle optional start keys improve the method's flexibility and robustness. The logic for both directions is well-structured and handles edge cases appropriately.
Consider documenting the behavior when
start_at_keyisNonefor both directions, and investigate the TODO comment on line 1306 to ensure the behavior is correct.
2723-2820: Enhance test case and remove debug prints.The test case effectively verifies the handling of missing transactionIndex field, but could be improved:
- Remove debug print statements
- Add more assertions to verify the complete query structure
Apply these changes:
- println!("{:?}", &drive_document_query); - println!("Generated PathQuery: {}", result); assert_eq!( result .clone() .query .query .default_subquery_branch .subquery .expect("expected subquery") .items, Query::new_range_full().items ); + // Add assertions for other query properties + assert_eq!(result.query.limit, Some(3)); + assert!(result.query.offset.is_none());packages/rs-drive/tests/query_tests.rs (1)
4451-4452: Remove unnecessary debug print statementThe debug statement
println!("{:?}", query);is present in the test code. It's advisable to remove such statements to prevent cluttering the test output.Apply this diff to remove the debug print statement:
- println!("{:?}", query);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (7)
- packages/rs-drive-abci/src/query/document_query/v0/mod.rs (3 hunks)
- packages/rs-drive-abci/src/query/mod.rs (2 hunks)
- packages/rs-drive/src/query/mod.rs (7 hunks)
- packages/rs-drive/src/verify/document/verify_proof_keep_serialized/v0/mod.rs (1 hunks)
- packages/rs-drive/tests/query_tests.rs (3 hunks)
- packages/rs-drive/tests/supporting_files/contract/withdrawals/withdrawals.json (1 hunks)
- packages/rs-sdk/src/platform/document_query.rs (3 hunks)
🔇 Additional comments (9)
packages/rs-drive/src/verify/document/verify_proof_keep_serialized/v0/mod.rs (1)
42-42: Verify pagination parameters in path_query.Since this PR implements pagination support, let's verify that the path_query correctly includes pagination parameters.
✅ Verification successful
Let me gather more specific information about the path_query construction and pagination parameters.
Let me search for the path query construction method and pagination parameters.
Let me check one more thing about the DriveDocumentQuery implementation.
Based on the code analysis, I can now provide a final response.
The path_query output is expected and pagination is correctly handled.
The
println!statement inverify_proof_keep_serialized_v0is showing the constructed path query which is normal behavior. The pagination parameters are handled through:
- The
SizedQuerystructure which encapsulates limit and offset parameters- The path query construction process that properly sets limits based on the query type:
- For single document queries, limit is set to 1 when needed
- For multiple document queries, limits are inherited from the DriveDocumentQuery configuration
The debug print is useful for development and verification purposes, showing the exact query being executed.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for path_query construction to verify pagination parameters ast-grep --pattern 'construct_path_query($_,$_)' # Search for pagination-related tests rg -l 'test.*pagination' --type rust # Look for pagination parameter definitions rg 'limit|offset|page|cursor' --type rustLength of output: 187636
Script:
#!/bin/bash # Search for path_query construction in the specific file rg -A 5 'path_query' packages/rs-drive/src/verify/document/verify_proof_keep_serialized/v0/mod.rs # Look for query construction related code in the same file rg -A 10 'query' packages/rs-drive/src/verify/document/verify_proof_keep_serialized/v0/mod.rs # Look for the struct/type definition of path_query ast-grep --pattern 'struct PathQuery'Length of output: 3129
Script:
#!/bin/bash # Look for construct_path_query implementation ast-grep --pattern 'fn construct_path_query' # Look for DriveDocumentQuery struct definition and its fields ast-grep --pattern 'struct DriveDocumentQuery' # Search for where pagination parameters are set in query construction rg -B 5 -A 10 'limit.*=.*Some' packages/rs-drive/src/query/Length of output: 4389
Script:
#!/bin/bash # Look for DriveDocumentQuery implementation and its methods rg -A 20 'impl.*DriveDocumentQuery' packages/rs-drive/src/query/ # Look for where the path_query is used in verify_proof_keep_serialized rg -B 5 -A 10 'verify_proof_keep_serialized' packages/rs-drive/src/verify/document/Length of output: 10130
packages/rs-drive/tests/supporting_files/contract/withdrawals/withdrawals.json (3)
129-141: LGTM! Schema constraints are well-defined.The schema constraints appropriately enforce:
- Required system timestamps ($createdAt, $updatedAt)
- Essential withdrawal fields (amount, fees, status, etc.)
- No additional properties allowed
86-91: Review amount constraints.The
amountproperty has a minimum value of 1000 without explanation. Consider:
- Adding a comment explaining why 1000 is the minimum
- Adding a maximum value constraint to prevent overflow
#!/bin/bash # Search for other amount constraints in the codebase rg -A 2 '"minimum":\s*1000' packages/rs-drive/tests/
1-5: Verify schema metadata values.A few points need verification:
- The
$format_versionis set to "0" - is this intentional for a new schema?- The
idandownerIdare identical - please confirm if this is the expected behavior.✅ Verification successful
Schema metadata values are consistent with other contracts
Based on the examination of other schema files in the codebase:
- The
$format_versionbeing "0" is the standard version used across all contract schemas- Having identical
idandownerIdis a common pattern seen in several contract definitions🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other schema files to compare format versions and ID patterns fd -e json -x sh -c 'echo "=== {} ==="; jq -r \ "\"format_version: \" + (.\"$format_version\" | tostring), \ \"id matches ownerId: \" + (.id == .ownerId | tostring)" {}' \ . packages/rs-drive/tests/supporting_files/contract/Length of output: 506
Script:
#!/bin/bash # Let's try a simpler approach to find and examine schema files fd -e json . packages/rs-drive/tests/supporting_files/contract/ -x cat {}Length of output: 147597
packages/rs-drive-abci/src/query/mod.rs (2)
35-42: LGTM: Imports are well-organized and necessary.The new imports are properly structured and required for the document storage functionality.
117-149: Add documentation to explain the function's purpose and parameters.The implementation looks good and follows the established patterns. Consider adding documentation to explain:
- The function's role in pagination testing
- The choice of SingleEpoch(0) for storage flags
- The implications of setting owner_id to None
Add documentation like this:
+ /// Stores a document in the platform for testing purposes. + /// + /// # Parameters + /// * `platform` - The platform instance + /// * `data_contract` - The data contract associated with the document + /// * `document_type` - Reference to the document type + /// * `document` - The document to store + /// * `platform_version` - The platform version + /// + /// Uses SingleEpoch(0) for storage flags as this is intended for testing. pub fn store_document(Let's verify this function is being used in pagination-related tests:
packages/rs-sdk/src/platform/document_query.rs (1)
22-22: LGTM: Import addition for vector size validationThe addition of
InvalidVectorSizeErrorimport is appropriate for the new pagination validation logic.packages/rs-drive-abci/src/query/document_query/v0/mod.rs (1)
496-1398: LGTM! Comprehensive test coverageThe test suite provides excellent coverage of pagination functionality including:
- Single item proofs
- Range proofs
- Secondary index queries
- Start after functionality
packages/rs-drive/tests/query_tests.rs (1)
Line range hint
4433-4437: LGTMThe query parameters with
"limit": 3and the specified"orderBy"clause are correct and align with the test objectives.
packages/rs-drive/src/verify/document/verify_proof_keep_serialized/v0/mod.rs
Outdated
Show resolved
Hide resolved
packages/rs-drive/src/verify/document/verify_proof_keep_serialized/v0/mod.rs
Outdated
Show resolved
Hide resolved
packages/rs-drive/tests/supporting_files/contract/withdrawals/withdrawals.json
Outdated
Show resolved
Hide resolved
shumkov
changed the title
shumkov
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (4)
packages/rs-drive-abci/src/query/document_query/v0/mod.rs (3)
Line range hint
73-77: Consider adding a minimum limit validationWhile the code validates the upper bound of the limit parameter, it might be beneficial to also validate the minimum value to prevent potential issues with negative values being cast to u16.
Add a check for negative values:
if limit > u16::MAX as u32 { return Ok(QueryValidationResult::new_with_error(QueryError::Query( QuerySyntaxError::InvalidLimit(format!("limit {} out of bounds", limit)), ))); } +if limit < 0 { + return Ok(QueryValidationResult::new_with_error(QueryError::Query( + QuerySyntaxError::InvalidLimit("limit cannot be negative".to_string()), + ))); +}
727-743: Add documentation for the serialize_vec_to_cbor functionThis helper function would benefit from documentation explaining its purpose, parameters, and return value. Consider adding a doc comment.
Add documentation:
+/// Serializes a vector of values into CBOR format. +/// +/// # Arguments +/// * `input` - A vector of values that can be converted into `Value` +/// +/// # Returns +/// * `Result<Vec<u8>, Error>` - The CBOR-serialized bytes or an error +/// +/// # Errors +/// Returns an error if the conversion to CBOR values or serialization fails fn serialize_vec_to_cbor<T: Into<Value>>(input: Vec<T>) -> Result<Vec<u8>, Error> {
1398-1401: Address the TODO comment and ignored testThe comment suggests that this functionality should be possible, but the test is currently ignored. Consider implementing the missing functionality or documenting why it's not currently possible.
Would you like me to help analyze what's preventing this functionality from working and propose a solution? I can help create a GitHub issue to track this enhancement.
packages/rs-drive/src/query/mod.rs (1)
1278-1310: LGTM! Consider adding documentation for pagination behavior.The changes to
inner_query_starts_from_keyimprove pagination support by making the start key optional and handling both forward and backward pagination correctly. The implementation properly handles edge cases and inclusion/exclusion of the start key.Consider adding documentation to explain:
- The pagination behavior in both directions
- The significance of the
includedparameter- The edge case handling when
start_at_keyis None- The TODO comment on line 1305 about the correctness of using
insert_key(vec![])for backward pagination/// Returns a `Query` that either starts at or after the given key. + /// + /// # Arguments + /// + /// * `start_at_key` - Optional starting key for pagination + /// * `left_to_right` - Direction of pagination (forward/backward) + /// * `included` - Whether to include the start key in results + /// + /// # Behavior + /// + /// - Forward pagination (left_to_right = true): + /// - With start key: Returns items >= start_key (included=true) or > start_key (included=false) + /// - Without start key: Returns all items + /// + /// - Backward pagination (left_to_right = false): + /// - With start key: Returns items <= start_key (included=true) or < start_key (included=false) + /// - Without start key: Returns only empty key fn inner_query_starts_from_key(
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- packages/rs-drive-abci/src/query/document_query/v0/mod.rs (2 hunks)
- packages/rs-drive/src/query/mod.rs (7 hunks)
🔇 Additional comments (2)
packages/rs-drive/src/query/mod.rs (2)
Line range hint
1868-1883: LGTM! Improved error handling for missing paths.The changes enhance robustness by gracefully handling cases where the queried path doesn't exist, returning empty results instead of errors. This is a better approach for queries that might legitimately return no results.
2725-2818: LGTM! Good test coverage for missing field handling.The new test case thoroughly verifies that the query system can handle documents with missing fields, particularly in the context of pagination and ordering. This is important for maintaining backward compatibility and robustness.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- packages/rs-drive/src/verify/document/verify_proof_keep_serialized/v0/mod.rs (1 hunks)
- packages/rs-drive/tests/query_tests.rs (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/rs-drive/src/verify/document/verify_proof_keep_serialized/v0/mod.rs
Issue being fixed or feature implemented
Documents pagination issues
What was done?
How Has This Been Tested?
Breaking Changes
None
Checklist:
For repository code-owners and collaborators only
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Tests