feat(drive): document filter for state transition subscriptions part 2 #2781
Conversation
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
WalkthroughAdds ValidationResult helpers for combined error/data extraction; extends query conditions with ValueClause, operator eval and shape checks; introduces DriveDocumentQueryFilter with transition/original matching and validation; expands rs-drive public query types and schema validation; adds Value 64-bit fit check; broadens wasm-sdk SdkError→WasmSdkError mapping. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Client
participant Filter as DriveDocumentQueryFilter
participant Trans as DocumentTransition
participant Orig as OriginalDocument
Client->>Filter: matches_document_transition(Trans, batch_owner)
activate Filter
Note right of Filter: Evaluate action-specific clauses on transition payload/meta
alt Clauses pass
Filter-->>Client: TransitionCheckResult::Pass
else Clauses fail
Filter-->>Client: TransitionCheckResult::Fail
else Needs original
Filter-->>Client: TransitionCheckResult::NeedsOriginal
Client->>Filter: matches_original_document(Orig)
Note right of Filter: Evaluate original-dependent clauses
Filter-->>Client: bool (true/false)
end
deactivate Filter
sequenceDiagram
autonumber
participant Caller
participant VR as ValidationResult<TData,E>
Caller->>VR: into_data_with_error()
alt has error(s)
VR-->>Caller: Ok(Err(E))
else has data
VR-->>Caller: Ok(Ok(TData))
else empty
VR-->>Caller: Ok(Err(ProtocolError::CorruptedCodeExecution(...)))
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🧰 Additional context used📓 Path-based instructions (2)**/*.rs📄 CodeRabbit inference engine (CLAUDE.md)
Files:
packages/rs-*/**/*.rs📄 CodeRabbit inference engine (AGENTS.md)
Files:
🧠 Learnings (2)📓 Common learnings📚 Learning: 2025-09-05T06:00:56.491ZApplied to files:
🧬 Code graph analysis (2)packages/rs-drive/src/query/filter.rs (1)
packages/rs-drive/src/query/conditions.rs (1)
🔇 Additional comments (26)
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. 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 |
QuantumExplorer
changed the base branch from
v2.1-dev
to
feat/documentFilter
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
🧹 Nitpick comments (11)
packages/rs-platform-value/src/lib.rs (1)
151-180: Fix typo in method name: should beis_integer_can_fit_64_bitsThe method name
is_integer_can_fit_64_bytesis misleading - it checks if integers fit within 64 bits, not bytes. This should be corrected for clarity.Apply this diff to fix the method name:
- pub fn is_integer_can_fit_64_bytes(&self) -> bool { + pub fn is_integer_can_fit_64_bits(&self) -> bool {Also update the doc comment example at line 159:
- /// assert!(value.is_integer_can_fit_64_bytes()); + /// assert!(value.is_integer_can_fit_64_bits());packages/rs-dpp/src/validation/validation_result.rs (1)
244-255: Consider renaming to clarify the nested Result return patternThe method name
into_data_with_errordoesn't clearly convey that it returns a nestedResult<Result<TData, E>, ProtocolError>. This could lead to confusion about its usage pattern (requiring??to fully unwrap).Consider a more descriptive name like
into_result_or_protocol_errorortry_into_resultto better reflect the nested Result pattern. Also, consider adding a doc comment explaining the usage pattern:/// Converts the validation result into a nested Result. /// /// # Returns /// - `Ok(Err(error))` if validation errors exist (returns first error) /// - `Ok(Ok(data))` if data exists and no errors /// - `Err(ProtocolError)` if neither data nor errors exist (corrupted state) /// /// # Example /// ``` /// let result = validation_result.into_data_with_error()??; /// ``` pub fn into_data_with_error(mut self) -> Result<Result<TData, E>, ProtocolError> {packages/rs-drive/src/query/conditions.rs (1)
1392-1585: Consider simplifying the nested type validation logicThe
validate_against_schemamethod has deeply nested pattern matching for type validation that could be simplified using helper functions or tables.Consider extracting the type compatibility checks into separate functions for better readability:
fn is_compatible_integer_value(value: &Value) -> bool { matches!( value, Value::U128(_) | Value::I128(_) | Value::U64(_) | Value::I64(_) | Value::U32(_) | Value::I32(_) | Value::U16(_) | Value::I16(_) | Value::U8(_) | Value::I8(_) ) } fn is_value_compatible_with_type(value: &Value, property_type: &DocumentPropertyType) -> bool { use DocumentPropertyType as T; match property_type { T::U8 | T::U16 | T::U32 | T::U64 | T::U128 | T::I8 | T::I16 | T::I32 | T::I64 | T::I128 => is_compatible_integer_value(value), T::F64 => matches!(value, Value::Float(_)), T::Date => is_compatible_integer_value(value), T::String(_) => matches!(value, Value::Text(_)), T::Identifier => matches!(value, Value::Identifier(_)), T::ByteArray(_) => matches!(value, Value::Bytes(_)), T::Boolean => matches!(value, Value::Bool(_)), T::Object(_) | T::Array(_) | T::VariableTypeArray(_) => false, } }packages/rs-drive/src/query/mod.rs (1)
309-415: Consider extracting primary key validation logicThe
validate_against_schemamethod onInternalClauseshas good structure but could benefit from extracting the primary key validation into a separate helper method for better readability.Consider extracting lines 372-411 into a helper method:
fn validate_primary_key_clauses(&self) -> QuerySyntaxSimpleValidationResult { if let Some(pk_eq) = &self.primary_key_equal_clause { if pk_eq.operator != WhereOperator::Equal || !matches!(pk_eq.value, Value::Identifier(_)) { return QuerySyntaxSimpleValidationResult::new_with_error( QuerySyntaxError::InvalidWhereClauseComponents( "primary key equality must compare an identifier", ), ); } } if let Some(pk_in) = &self.primary_key_in_clause { // ... rest of the IN clause validation } QuerySyntaxSimpleValidationResult::default() }packages/rs-drive/src/query/filter.rs (7)
1056-1056: Fix type cast syntax.Line 1056 uses
1 as u64which should be just1for clarity and simplicity.- revision: 1 as u64, + revision: 1,
89-89: Consider documenting the clippy allow directive.The
#[allow(clippy::large_enum_variant)]attribute suppresses warnings about enum variants containing large data structures. Consider adding a comment explaining why this is acceptable here (e.g., performance trade-offs, usage patterns).+// Allow large variants as InternalClauses are typically small in practice despite their potential size #[allow(clippy::large_enum_variant)]
300-304: Early return for missing batch owner.When
owner_clauseisSomebutbatch_owner_valueisNone, returning immediately is cleaner than pattern matching. The current approach is functionally correct but could be more readable.let owner_ok = match (owner_clause, batch_owner_value) { (Some(clause), Some(val)) => clause.matches_value(val), - (Some(_), None) => return TransitionCheckResult::Fail, + (Some(_), None) => false, (None, _) => true, };
1102-1102: Fix type cast syntax.Line 1102 uses
10 as Creditswhich should use standard Rust casting syntax or just be10if Credits implements From.- price: 10 as Credits, + price: 10,
1234-1234: Fix type cast syntax.Line 1234 uses
10 as Creditswhich should use standard Rust casting syntax or just be10if Credits implements From.- price: 10 as Credits, + price: 10,
594-611: Consider handling the case wherepathcontains consecutive dots.The
get_value_by_pathfunction splits on '.' but doesn't handle edge cases like consecutive dots (..) or leading/trailing dots which could lead to unexpected behavior.fn get_value_by_path<'a>(root: &'a BTreeMap<String, Value>, path: &str) -> Option<&'a Value> { if path.is_empty() { return None; } + // Reject invalid paths with consecutive dots or leading/trailing dots + if path.contains("..") || path.starts_with('.') || path.ends_with('.') { + return None; + } let mut current: Option<&Value> = None; let mut segments = path.split('.'); if let Some(first) = segments.next() { current = root.get(first); } for seg in segments { + if seg.is_empty() { + return None; + } match current { Some(Value::Map(ref vm)) => { current = vm.get_optional_key(seg); } _ => return None, } } current }
366-366: Avoid underscore match arm when specific variants are expected.The underscore pattern at line 366 catches
Createtransitions, but it's clearer to be explicit about which variants are not expected here.- _ => false, + DocumentActionMatchClauses::Create { .. } => false,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
packages/rs-dpp/src/validation/validation_result.rs(2 hunks)packages/rs-drive/src/query/conditions.rs(13 hunks)packages/rs-drive/src/query/filter.rs(1 hunks)packages/rs-drive/src/query/mod.rs(8 hunks)packages/rs-platform-value/src/lib.rs(1 hunks)packages/wasm-sdk/src/error.rs(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.rs: Format Rust code with cargo fmt
Run Clippy linter for Rust code
Files:
packages/rs-platform-value/src/lib.rspackages/wasm-sdk/src/error.rspackages/rs-drive/src/query/filter.rspackages/rs-dpp/src/validation/validation_result.rspackages/rs-drive/src/query/mod.rspackages/rs-drive/src/query/conditions.rs
packages/rs-*/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
packages/rs-*/**/*.rs: Use rustfmt defaults for Rust code (format via cargo fmt --all)
Keep Rust code clippy-clean (cargo clippy)
Name Rust modules in snake_case
Use PascalCase for Rust types
Use SCREAMING_SNAKE_CASE for Rust constants
Files:
packages/rs-platform-value/src/lib.rspackages/rs-drive/src/query/filter.rspackages/rs-dpp/src/validation/validation_result.rspackages/rs-drive/src/query/mod.rspackages/rs-drive/src/query/conditions.rs
packages/wasm-sdk/**
📄 CodeRabbit inference engine (CLAUDE.md)
Keep WASM SDK docs in sync (run generate_docs.py) when updating the WASM SDK
Files:
packages/wasm-sdk/src/error.rs
🧠 Learnings (2)
📓 Common learnings
Learnt from: QuantumExplorer
PR: dashpay/platform#2257
File: packages/rs-drive-abci/src/mimic/test_quorum.rs:159-164
Timestamp: 2024-11-20T16:16:01.830Z
Learning: QuantumExplorer prefers not to receive auto-generated messages asking to post on social media.
📚 Learning: 2025-09-05T06:00:56.491Z
Learnt from: QuantumExplorer
PR: dashpay/platform#2761
File: packages/rs-drive/src/query/filter.rs:268-272
Timestamp: 2025-09-05T06:00:56.491Z
Learning: For document subscription filters in rs-drive, validation should not require index availability checks like queries do. Subscription filtering has different validation requirements than query optimization.
Applied to files:
packages/rs-drive/src/query/filter.rspackages/rs-drive/src/query/mod.rs
🧬 Code graph analysis (4)
packages/wasm-sdk/src/error.rs (1)
packages/wasm-sdk/src/dpp.rs (1)
new(40-50)
packages/rs-drive/src/query/filter.rs (4)
packages/rs-drive/src/query/mod.rs (6)
query(884-892)clauses(641-652)clauses(695-712)clauses(769-780)from(419-436)from(2353-2409)packages/rs-dpp/src/validation/validation_result.rs (7)
new(83-88)None(86-86)new_with_error(104-109)from(279-281)from(285-290)is_valid(201-203)is_err(205-207)packages/rs-drive/src/query/conditions.rs (2)
from(323-325)from(1589-1591)packages/rs-platform-value/src/lib.rs (7)
from(1579-1585)from(1598-1606)from(1619-1627)from(1634-1641)from(1645-1652)from(1659-1666)from(1673-1680)
packages/rs-drive/src/query/mod.rs (2)
packages/rs-dpp/src/validation/validation_result.rs (4)
None(86-86)new_with_error(104-109)new_with_errors(111-113)default(28-33)packages/rs-drive/src/query/conditions.rs (2)
validate_against_schema(1393-1585)in_values(347-380)
packages/rs-drive/src/query/conditions.rs (2)
packages/rs-drive/src/query/mod.rs (5)
query(884-892)validate_against_schema(311-415)clauses(641-652)clauses(695-712)clauses(769-780)packages/rs-dpp/src/validation/validation_result.rs (9)
None(86-86)new_with_error(104-109)new_with_data(90-95)new_with_errors(111-113)new(83-88)is_err(205-207)first_error(209-211)default(28-33)is_valid(201-203)
⏰ 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). (20)
- GitHub Check: Rust packages (drive) / Tests
- GitHub Check: Rust packages (drive) / Unused dependencies
- GitHub Check: Rust packages (wasm-dpp) / Tests
- GitHub Check: Rust packages (dpp) / Linting
- GitHub Check: Rust packages (dpp) / Formatting
- GitHub Check: Rust packages (dpp) / Check each feature
- GitHub Check: Rust packages (dpp) / Tests
- GitHub Check: Rust packages (dpp) / Unused dependencies
- GitHub Check: Rust packages (wasm-sdk) / Unused dependencies
- GitHub Check: Rust packages (wasm-sdk) / Tests
- GitHub Check: Rust packages (wasm-sdk) / Linting
- GitHub Check: Rust packages (wasm-sdk) / Formatting
- GitHub Check: Rust packages (rs-sdk-ffi) / Tests
- GitHub Check: Rust packages (dash-sdk) / Tests
- GitHub Check: Rust packages (drive-abci) / Tests
- GitHub Check: Build Docker images (Dashmate helper, dashmate-helper, dashmate-helper) / Build Dashmate helper image
- GitHub Check: Build Docker images (DAPI, dapi, dapi) / Build DAPI image
- GitHub Check: Build Docker images (Drive, drive, drive-abci) / Build Drive image
- GitHub Check: Build JS packages / Build JS
- GitHub Check: Rust crates security audit
🔇 Additional comments (12)
packages/wasm-sdk/src/error.rs (1)
172-178: Approve: Sensible fallback for unmapped error variantsAdding a catch-all arm for unhandled
SdkErrorvariants preserves the retriable flag and prevents panics on new error types. This is a defensive improvement.packages/rs-drive/src/query/conditions.rs (4)
177-242: LGTM: Well-structured operator evaluationThe centralized
evalmethod provides a clean, consistent implementation for all operators. The bounds checking for Between variants is properly handled.
347-380: LGTM: Proper error propagation with ValidationResultThe refactoring of
in_valuesto returnQuerySyntaxValidationResultaligns well with the new validation flow, providing better error context.
1678-1704: LGTM: Comprehensive meta field type mappingThe
meta_field_property_typefunction properly maps all system fields to their expected types. This enables proper validation of meta fields in queries.
1594-1609: LGTM: Clean ValueClause implementationThe
ValueClausestruct provides a focused way to evaluate operators against values without field lookups, which is useful for the subscription filtering use case.packages/rs-drive/src/query/mod.rs (3)
106-128: LGTM: Cleaner contract lookup function signatureThe updated
ContractLookupFntype andcontract_lookup_fn_for_contractimplementation now use the importedIdentifieralias and simplified error types, making the API cleaner.
1236-1236: LGTM: Proper error propagation with ValidationResultThe change from
.in_values()?to.in_values().into_data_with_error()??correctly handles the newValidationResultreturn type, maintaining proper error propagation.
150-152: LGTM: Public filter module for subscriptionsThe new
filtermodule appropriately extends the query functionality to support document subscription filtering.packages/rs-drive/src/query/filter.rs (4)
8-32: Well-structured module documentation!The documentation clearly explains the filtering workflow, usage patterns, and validation approach. The two-phase evaluation model (transition-level then original-document) is well described.
131-327: Clean implementation of the two-phase filter evaluation.The
matches_document_transitionmethod efficiently handles all transition types with clear separation of concerns: contract/type validation, transition-specific checks, and the three-state return model (Pass/Fail/NeedsOriginal) enables optimized fetching of original documents.
431-583: Thorough validation logic with clear operator constraints.The
validatemethod correctly enforces structural rules, validates operator compatibility for value clauses (owner/price), and properly delegates schema validation toInternalClauses::validate_against_schema.
613-1700: Comprehensive test coverage!The test suite thoroughly covers:
- All action types and their clause combinations
- Edge cases like missing fields, nested paths, and operator variations
- Validation rules including type constraints and invalid configurations
- The two-phase evaluation model with transition-level and original-document checks
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (8)
packages/rs-dpp/src/validation/validation_result.rs (2)
205-207: Consider renamingis_errfor consistency withis_valid.While the implementation is correct,
is_errmight be confusing alongsideis_valid. Consider renaming it tohas_errorsoris_invalidto make the naming more consistent and clearer about what it checks.- pub fn is_err(&self) -> bool { + pub fn has_errors(&self) -> bool { !self.errors.is_empty() }
244-255: Potential issue:pop()mutates errors but doesn't handle multiple errors well.The method pops a single error from the errors vector, which could be problematic when there are multiple errors. Consider:
- This mutation makes the method consume
selfappropriately, but the remaining errors are lost- The method name doesn't clearly indicate it only handles a single error
- Consider returning all errors or documenting the single-error behavior clearly
Consider this alternative that preserves all errors:
pub fn into_data_with_error(mut self) -> Result<Result<TData, E>, ProtocolError> { - if let Some(error) = self.errors.pop() { - Ok(Err(error)) + if !self.errors.is_empty() { + // Return the first error while preserving the error order + let errors = self.errors; + Ok(Err(errors.into_iter().next().unwrap())) } else { self.data .map(Ok) .ok_or(ProtocolError::CorruptedCodeExecution(format!( "trying to push validation result into data (errors are {:?})", self.errors ))) } }packages/rs-drive/src/query/conditions.rs (1)
245-299: Consider extracting common numeric value check logic.The
value_shape_okmethod has repetitive numeric type checking logic that could be extracted into the existingis_numeric_valuehelper function to improve maintainability.The numeric type checking pattern appears multiple times. You already have
is_numeric_valuedefined at line 1661, consider using it:GreaterThan | GreaterThanOrEquals | LessThan | LessThanOrEquals => { match property_type { DocumentPropertyType::F64 => is_numeric_value(value), DocumentPropertyType::String(_) => { matches!(value, Value::Text(_)) } - _ => matches!( - value, - Value::U128(_) - | Value::I128(_) - | Value::U64(_) - | Value::I64(_) - | Value::U32(_) - | Value::I32(_) - | Value::U16(_) - | Value::I16(_) - | Value::U8(_) - | Value::I8(_) - ), + _ => is_numeric_value(value) && !matches!(value, Value::Float(_)), } }packages/rs-drive/src/query/filter.rs (1)
527-527: Between bounds validation could be more defensive.While the validation checks that bounds are in ascending order at line 527, it would be safer to also verify this during evaluation to prevent issues if the value is modified after validation.
Consider adding a bounds order check in the evaluation path as well:
Between => match right_value { Value::Array(bounds) if bounds.len() == 2 => { match bounds[0].partial_cmp(&bounds[1]) { Some(Ordering::Less) | Some(Ordering::Equal) => { left_value >= &bounds[0] && left_value <= &bounds[1] } - _ => false, + _ => { + // Log or handle invalid bounds + false + } } } _ => false, },packages/rs-drive/src/query/mod.rs (4)
106-107: Clean up type signature: Remove qualifiedErrorimportThe function signature uses the qualified
Errortype but the crate-localErroris available without qualification.-pub type ContractLookupFn<'a> = - dyn Fn(&Identifier) -> Result<Option<Arc<DataContract>>, Error> + 'a; +pub type ContractLookupFn<'a> = + dyn Fn(&Identifier) -> Result<Option<Arc<DataContract>>, Error> + 'a;Since
Erroris imported fromcrate::error::Errorat line 18, you don't need to qualify it.
123-129: Simplified closure implementationThe closure body can be simplified by removing the explicit return statement since it's the last expression in the block.
pub fn contract_lookup_fn_for_contract<'a>( data_contract: Arc<DataContract>, ) -> Box<ContractLookupFn<'a>> { let func = move |id: &Identifier| -> Result<Option<Arc<DataContract>>, Error> { if data_contract.id().ne(id) { - return Ok(None); + Ok(None) + } else { + Ok(Some(Arc::clone(&data_contract))) } - Ok(Some(Arc::clone(&data_contract))) }; Box::new(func) }
327-333: Consider extracting magic string constantThe string literal
"use primary_key_* clauses for $id"appears multiple times in the validation logic (lines 330, 345, 361). Consider extracting it as a constant to ensure consistency.Add a constant at the module level or within the impl block:
const ID_FIELD_ERROR_MSG: &str = "use primary_key_* clauses for $id";Then replace all occurrences with the constant.
374-383: Consider combining redundant identifier type checksThe identifier type validation for primary key IN clause at lines 398-404 duplicates some logic that could be combined with the earlier validation.
Consider extracting a helper method to validate identifier arrays:
fn validate_identifier_array(value: &Value) -> bool { matches!(value, Value::Array(arr) if arr.iter().all(|v| matches!(v, Value::Identifier(_)))) }This would simplify both validation blocks and reduce duplication.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
packages/rs-dpp/src/validation/validation_result.rs(2 hunks)packages/rs-drive/src/query/conditions.rs(13 hunks)packages/rs-drive/src/query/filter.rs(1 hunks)packages/rs-drive/src/query/mod.rs(8 hunks)packages/rs-platform-value/src/lib.rs(1 hunks)packages/wasm-sdk/src/error.rs(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.rs: Format Rust code with cargo fmt
Run Clippy linter for Rust code
Files:
packages/wasm-sdk/src/error.rspackages/rs-platform-value/src/lib.rspackages/rs-drive/src/query/filter.rspackages/rs-dpp/src/validation/validation_result.rspackages/rs-drive/src/query/conditions.rspackages/rs-drive/src/query/mod.rs
packages/wasm-sdk/**
📄 CodeRabbit inference engine (CLAUDE.md)
Keep WASM SDK docs in sync (run generate_docs.py) when updating the WASM SDK
Files:
packages/wasm-sdk/src/error.rs
packages/rs-*/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
packages/rs-*/**/*.rs: Use rustfmt defaults for Rust code (format via cargo fmt --all)
Keep Rust code clippy-clean (cargo clippy)
Name Rust modules in snake_case
Use PascalCase for Rust types
Use SCREAMING_SNAKE_CASE for Rust constants
Files:
packages/rs-platform-value/src/lib.rspackages/rs-drive/src/query/filter.rspackages/rs-dpp/src/validation/validation_result.rspackages/rs-drive/src/query/conditions.rspackages/rs-drive/src/query/mod.rs
🧠 Learnings (2)
📓 Common learnings
Learnt from: QuantumExplorer
PR: dashpay/platform#2257
File: packages/rs-drive-abci/src/mimic/test_quorum.rs:159-164
Timestamp: 2024-11-20T16:16:01.830Z
Learning: QuantumExplorer prefers not to receive auto-generated messages asking to post on social media.
📚 Learning: 2025-09-05T06:00:56.491Z
Learnt from: QuantumExplorer
PR: dashpay/platform#2761
File: packages/rs-drive/src/query/filter.rs:268-272
Timestamp: 2025-09-05T06:00:56.491Z
Learning: For document subscription filters in rs-drive, validation should not require index availability checks like queries do. Subscription filtering has different validation requirements than query optimization.
Applied to files:
packages/rs-drive/src/query/filter.rspackages/rs-drive/src/query/mod.rs
🧬 Code graph analysis (4)
packages/rs-platform-value/src/lib.rs (1)
packages/rs-platform-value/src/btreemap_extensions/mod.rs (2)
v(266-266)v(478-478)
packages/rs-drive/src/query/filter.rs (3)
packages/rs-drive/src/query/mod.rs (6)
query(884-892)clauses(641-652)clauses(695-712)clauses(769-780)from(419-436)from(2353-2409)packages/rs-dpp/src/validation/validation_result.rs (5)
new(83-88)None(86-86)new_with_error(104-109)is_valid(201-203)is_err(205-207)packages/rs-drive/src/query/conditions.rs (2)
from(323-325)from(1589-1591)
packages/rs-drive/src/query/conditions.rs (2)
packages/rs-drive/src/query/mod.rs (5)
query(884-892)validate_against_schema(311-415)clauses(641-652)clauses(695-712)clauses(769-780)packages/rs-dpp/src/validation/validation_result.rs (9)
None(86-86)new_with_error(104-109)new_with_data(90-95)new_with_errors(111-113)new(83-88)is_err(205-207)first_error(209-211)default(28-33)is_valid(201-203)
packages/rs-drive/src/query/mod.rs (2)
packages/rs-dpp/src/validation/validation_result.rs (4)
None(86-86)new_with_error(104-109)new_with_errors(111-113)default(28-33)packages/rs-drive/src/query/conditions.rs (2)
validate_against_schema(1393-1585)in_values(347-380)
🔇 Additional comments (17)
packages/wasm-sdk/src/error.rs (1)
172-177: LGTM! Fallback arm for non-exhaustive error enum.The catch-all arm ensures forward compatibility with new
SdkErrorvariants by mapping them toGeneric, preserving the retriable flag and usingto_string()for the message. This is a good defensive pattern for handling non-exhaustive enums.packages/rs-platform-value/src/lib.rs (1)
151-180: LGTM! Well-implemented 64-bit fit check.The method correctly handles all integer variants, checking 128-bit values against their respective 64-bit bounds while directly accepting smaller integer types. The documentation example clearly demonstrates the expected behavior.
packages/rs-drive/src/query/conditions.rs (5)
177-242: LGTM! Comprehensive operator evaluation logic.The
evalmethod provides a unified evaluation interface for bothWhereClauseandValueClause, correctly handling all operators including complex ones likeBetweenvariants andStartsWith. The boundary checking forBetweenoperators is properly implemented.
858-858: Good use of the newinto_data_with_error()method.The change properly handles the validation result from
in_values(), extracting either the data or propagating the error through the double?operator.
1611-1658: LGTM! Well-structured allowed operators definition.The
allowed_ops_for_typefunction provides a clean mapping of property types to their allowed operators, properly restricting complex types (Object, Array, VariableTypeArray) from any operators.
1680-1704: LGTM! Comprehensive meta field type mapping.The
meta_field_property_typefunction correctly maps all meta/system fields to their appropriate types, including proper handling of block heights (U64 for platform, U32 for core) and timestamps.
1594-1609: LGTM! Clean ValueClause implementation.The
ValueClausestruct and itsmatches_valuemethod provide a simple, focused abstraction for value-only comparisons, delegating to the sharedWhereOperator::evallogic.packages/rs-drive/src/query/filter.rs (5)
1-32: Excellent module documentation!The documentation clearly explains the filtering system, action-specific behaviors, two-phase evaluation pattern, and validation approach. This will greatly help developers understand and use the subscription filtering feature.
145-327: Well-designed two-phase evaluation pattern.The
matches_document_transitionmethod effectively implements a two-phase pattern that:
- Short-circuits on obvious mismatches (contract/type)
- Evaluates transition-level constraints first
- Only requires original document fetching when necessary
This design minimizes unnecessary database lookups and improves performance.
518-519: Good use of the newis_integer_can_fit_in_64_bitsmethod.The validation correctly uses the new platform-value method to ensure price values fit in 64 bits, which is appropriate for the Credits type (u64).
593-611: LGTM! Clean dot-notation path resolver.The
get_value_by_pathfunction correctly handles nested field access with proper error handling for missing segments and non-map intermediates.
613-1700: Comprehensive test coverage!The test suite thoroughly covers:
- All transition types (Create, Replace, Delete, Transfer, UpdatePrice, Purchase)
- Various operators (Equal, In, Range, Between, StartsWith)
- Nested field access
- Edge cases and validation scenarios
- Two-phase evaluation pattern
Excellent testing discipline!
packages/rs-drive/src/query/mod.rs (5)
5-5: LGTM! Addition ofValueClauseto public exportsThe addition of
ValueClauseto the public export set aligns with the PR's document filtering functionality. The change is well-structured alongside related query components.
150-152: LGTM! New document subscription filtering moduleThe new
filtermodule is appropriately gated behindserverorverifyfeatures, consistent with other query modules. The module comment clearly indicates its purpose for document subscription filtering.
157-161: LGTM! Well-structured validation result type aliasesThe type aliases
QuerySyntaxValidationResult<TData>andQuerySyntaxSimpleValidationResultprovide clear abstractions for query syntax validation, improving code readability and maintainability.
309-415: LGTM! Comprehensive schema validation for InternalClausesThe
validate_against_schemamethod provides thorough validation including:
- Composition verification
- Field existence checks
- Proper enforcement of $id field restrictions
- Primary key typing constraints
- Delegation to field-level validation
The validation logic is well-structured and follows a clear sequential pattern.
1236-1236: LGTM! Proper error handling withinto_data_with_error()The change from
in_values()toin_values().into_data_with_error()??properly handles the validation result containing both data and potential errors, ensuring that invalid IN clause values are caught and propagated correctly.
Summary by CodeRabbit
New Features
Improvements
Bug Fixes