feat(drive): document filter for state transition subscriptions part 1 #2761
Conversation
WalkthroughAdds a feature-gated DriveDocumentQueryFilter API with per-action match clauses and TransitionCheckResult (Pass/Fail/NeedsOriginal); implements clause evaluation including dot-path lookups, original-document handling, and validation; extends query conditions with ValueClause, operator evaluation, value-shape and schema-aware validation, updated exports, and tests. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client
participant Filter as DriveDocumentQueryFilter
participant Schema as SchemaValidator
participant Eval as ClauseEvaluator
participant DT as DocumentTransition
participant Orig as OriginalDocument
Client->>Filter: construct(contract, docType, action_clauses)
Client->>Filter: validate()
Filter->>Schema: InternalClauses::validate_against_schema(...)
Schema-->>Filter: Ok / Error
Client->>Filter: matches_document_transition(DT, batch_owner)
Filter->>DT: inspect action & payloads
alt no original required
Filter->>Eval: evaluate_clauses(new/original?, owner/price?)
Eval-->>Filter: Pass / Fail
Filter-->>Client: Pass / Fail
else original required
Filter-->>Client: NeedsOriginal
Client->>Filter: matches_original_document(Orig)
Filter->>Eval: evaluate_clauses(original & new, owner/price?)
Eval-->>Filter: Pass / Fail
Filter-->>Client: Pass / Fail
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 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 (1)
🧰 Additional context used📓 Path-based instructions (2)**/*.rs📄 CodeRabbit inference engine (CLAUDE.md)
Files:
packages/rs-*/**/*.rs📄 CodeRabbit inference engine (AGENTS.md)
Files:
🧠 Learnings (3)📚 Learning: 2025-07-23T08:31:42.268ZApplied to files:
📚 Learning: 2025-02-03T23:34:43.081ZApplied to files:
📚 Learning: 2024-10-29T14:40:54.727ZApplied to files:
🧬 Code graph analysis (1)packages/rs-drive/src/query/conditions.rs (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). (3)
🔇 Additional comments (5)
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 |
pauldelucia
changed the title
pauldelucia
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: 4
🧹 Nitpick comments (1)
packages/rs-drive/src/query/filter.rs (1)
11-27: Align cfg-gating to avoid no-feature build/test failures
- The module is included unconditionally; once mod.rs is gated, this file won't be compiled without features. Still, for belt-and-suspenders:
- Consider guarding the conversion impls that reference
DriveDocumentQuerywith the same#[cfg(any(feature = "server", "verify"))].- Gate tests with features to avoid
cargo testfailures when features are off.#[cfg(test)] -mod tests { +#[cfg(all(test, any(feature = "server", feature = "verify")))] +mod tests {Optionally:
-impl<'a> From<DriveDocumentQueryFilter<'a>> for DriveDocumentQuery<'a> { +#[cfg(any(feature = "server", feature = "verify"))] +impl<'a> From<DriveDocumentQueryFilter<'a>> for DriveDocumentQuery<'a> { ... } -impl<'a> From<DriveDocumentQuery<'a>> for DriveDocumentQueryFilter<'a> { +#[cfg(any(feature = "server", feature = "verify"))] +impl<'a> From<DriveDocumentQuery<'a>> for DriveDocumentQueryFilter<'a> { ... }Also applies to: 43-61, 63-71, 275-275
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
packages/rs-drive/src/query/filter.rs(1 hunks)packages/rs-drive/src/query/mod.rs(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
packages/rs-drive/src/query/filter.rs (1)
packages/rs-drive/src/query/mod.rs (9)
query(768-776)from(303-320)from(2220-2276)contract(924-929)contract(1022-1027)internal_clauses(1216-1221)order_by(421-424)order_by(672-691)is_empty(219-225)
⏰ 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). (14)
- GitHub Check: Rust packages (drive) / Linting
- GitHub Check: Rust packages (drive) / Unused dependencies
- GitHub Check: Rust packages (drive) / Tests
- GitHub Check: Rust packages (dash-sdk) / Unused dependencies
- GitHub Check: Rust packages (dash-sdk) / Linting
- GitHub Check: Rust packages (dash-sdk) / Formatting
- GitHub Check: Rust packages (dash-sdk) / Check each feature
- GitHub Check: Rust packages (drive-abci) / Unused dependencies
- GitHub Check: Rust packages (dash-sdk) / Tests
- GitHub Check: Rust packages (drive-abci) / Check each feature
- GitHub Check: Rust packages (drive-abci) / Tests
- GitHub Check: Rust packages (drive-abci) / Linting
- GitHub Check: Build Docker images (Drive, drive, drive-abci) / Build Drive image
- GitHub Check: Build JS packages / Build JS
🔇 Additional comments (1)
packages/rs-drive/src/query/filter.rs (1)
196-264: Type/ordering caveat: directValuecomparisons may diverge from index ordering
>/<overValuemay not mirror how keys are serialized/ordered in indexes (e.g., mixed numeric types, identifiers). If consistency with query/index semantics is required, consider normalizing types or reusing the same serialization rules used byDocumentType::serialize_value_for_key.Would you like me to sketch a normalization layer that aligns filter comparisons with index key ordering?
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
packages/rs-drive/src/query/filter.rs (1)
494-512: Path walker for nested fields: LGTM (addresses earlier feedback)Correctly traverses Value::Map via ValueMapHelper; returns None on non-map. Good.
🧹 Nitpick comments (4)
packages/rs-drive/src/query/conditions.rs (2)
220-264: Normalize Between bounds to avoid inverted-range false negativesIf the provided bounds are in descending order, current checks will always fail. Sort (lo, hi) before comparison.
- WhereOperator::Between => match &self.value { - Value::Array(bounds) if bounds.len() == 2 => { - value >= &bounds[0] && value <= &bounds[1] - } + WhereOperator::Between => match &self.value { + Value::Array(bounds) if bounds.len() == 2 => { + let (lo, hi) = if bounds[0] <= bounds[1] { (&bounds[0], &bounds[1]) } else { (&bounds[1], &bounds[0]) }; + value >= lo && value <= hi } ... - WhereOperator::BetweenExcludeBounds => match &self.value { - Value::Array(bounds) if bounds.len() == 2 => { - value > &bounds[0] && value < &bounds[1] - } + WhereOperator::BetweenExcludeBounds => match &self.value { + Value::Array(bounds) if bounds.len() == 2 => { + let (lo, hi) = if bounds[0] <= bounds[1] { (&bounds[0], &bounds[1]) } else { (&bounds[1], &bounds[0]) }; + value > lo && value < hi } ... - WhereOperator::BetweenExcludeLeft => match &self.value { - Value::Array(bounds) if bounds.len() == 2 => { - value > &bounds[0] && value <= &bounds[1] - } + WhereOperator::BetweenExcludeLeft => match &self.value { + Value::Array(bounds) if bounds.len() == 2 => { + let (lo, hi) = if bounds[0] <= bounds[1] { (&bounds[0], &bounds[1]) } else { (&bounds[1], &bounds[0]) }; + value > lo && value <= hi } ... - WhereOperator::BetweenExcludeRight => match &self.value { - Value::Array(bounds) if bounds.len() == 2 => { - value >= &bounds[0] && value < &bounds[1] - } + WhereOperator::BetweenExcludeRight => match &self.value { + Value::Array(bounds) if bounds.len() == 2 => { + let (lo, hi) = if bounds[0] <= bounds[1] { (&bounds[0], &bounds[1]) } else { (&bounds[1], &bounds[0]) }; + value >= lo && value < hi }
220-264: Guard against cross-type comparisons producing surprising resultsUsing >, < on Value relies on its PartialOrd across variants. Consider documenting/ensuring numeric normalization (e.g., cast ints to a common type) if mismatched numeric variants may appear at runtime.
packages/rs-drive/src/query/filter.rs (2)
37-41: Drop unused query/index imports after making validation index-agnosticClean up imports once validate() stops calling find_best_index.
-use indexmap::IndexMap; -use crate::query::{DriveDocumentQuery, InternalClauses, WhereClause}; +use crate::query::{InternalClauses, WhereClause}; use dpp::platform_value::ValueMapHelper; -use dpp::version::LATEST_PLATFORM_VERSION; +use dpp::data_contract::document_type::DocumentType;
282-312: Minor: simplify empty-clauses handlingLocal empty is fine. Optionally inline via a small helper to pick the right clause set; no action required.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
packages/rs-drive/src/query/conditions.rs(1 hunks)packages/rs-drive/src/query/filter.rs(1 hunks)
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-07-23T08:31:42.268Z
Learnt from: CR
PR: dashpay/platform#0
File: packages/wasm-sdk/CLAUDE.md:0-0
Timestamp: 2025-07-23T08:31:42.268Z
Learning: Applies to packages/wasm-sdk/src/**/*.rs : The WASM SDK now fully supports where and orderBy clauses for document queries; use the specified JSON array formats and supported operators.
Applied to files:
packages/rs-drive/src/query/conditions.rs
📚 Learning: 2024-10-29T14:40:54.727Z
Learnt from: lklimek
PR: dashpay/platform#2277
File: packages/rs-sdk/src/core/transaction.rs:0-0
Timestamp: 2024-10-29T14:40:54.727Z
Learning: In `packages/rs-sdk/src/platform/document_query.rs` and `packages/rs-sdk/src/core/transaction.rs`, certain places don't implement `IntoInner`, so direct error mappings cannot be simplified using `IntoInner`. A TODO comment has been added to address this in a future PR.
Applied to files:
packages/rs-drive/src/query/filter.rs
📚 Learning: 2025-09-05T06:00:56.444Z
Learnt from: QuantumExplorer
PR: dashpay/platform#2761
File: packages/rs-drive/src/query/filter.rs:268-272
Timestamp: 2025-09-05T06:00:56.444Z
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.rs
🧬 Code graph analysis (1)
packages/rs-drive/src/query/filter.rs (2)
packages/rs-drive/src/query/mod.rs (9)
query(769-777)clauses(526-537)clauses(580-597)clauses(654-665)contract(925-930)contract(1023-1028)from(304-321)from(2221-2277)internal_clauses(1217-1222)packages/rs-drive/src/query/conditions.rs (2)
from(197-199)from(1312-1314)
⏰ 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). (13)
- GitHub Check: Rust packages (dash-sdk) / Linting
- GitHub Check: Rust packages (dash-sdk) / Check each feature
- GitHub Check: Rust packages (dash-sdk) / Tests
- GitHub Check: Rust packages (drive-abci) / Check each feature
- GitHub Check: Rust packages (dash-sdk) / Unused dependencies
- GitHub Check: Rust packages (drive-abci) / Tests
- GitHub Check: Rust packages (drive) / Unused dependencies
- GitHub Check: Rust packages (drive) / Tests
- GitHub Check: Rust packages (drive) / Linting
- GitHub Check: Rust packages (drive-abci) / Linting
- 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 (4)
packages/rs-drive/src/query/filter.rs (4)
119-186: Per-action matching looks solidGood dispatch and constraints (e.g., requiring original doc when clause present). Nice use of matches_value for owner/price.
233-279: Safe fallback for non-data actions with PK-only filters: LGTMPrevents accidental matches when no document data is available. Good guard.
322-389: Dot-path resolution and clause evaluation read cleanCorrect contract/type checks; path lookup via get_value_by_path; early returns. Looks good.
960-1006: Purchase owner context behavior: nice negative/positive coverageGood requirement of batch_owner_value when owner_clause is present.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
packages/rs-drive/src/query/mod.rs (1)
155-157: Good fix: gate the filter moduleMatches prior feedback; avoids no-feature build breaks.
packages/rs-drive/src/query/filter.rs (1)
699-701: Gate tests with the same features as the implementationWithout server/verify features, tests reference gated items and fail to compile.
-#[cfg(test)] -mod tests { +#[cfg(all(test, any(feature = "server", feature = "verify")))] +mod tests {
🧹 Nitpick comments (8)
packages/rs-drive/src/query/conditions.rs (2)
267-299: Minor: O(n^2) duplicate check can be O(n) with a setFor
in_values, using aHashSetavoids quadratic scans; negligible at 100 max, but trivial to simplify.- if (1..in_values.len()).any(|i| in_values[i..].contains(&in_values[i - 1])) { + { + use std::collections::hash_map::Entry; + use std::collections::HashMap; + let mut seen: HashMap<&Value, ()> = HashMap::with_capacity(len); + for v in in_values.iter() { + if let Entry::Vacant(e) = seen.entry(v) { + e.insert(()); + } else { + return Err(Error::Query(QuerySyntaxError::InvalidInClause( + "there should be no duplicates values for In query".to_string(), + ))); + } + } + }
1699-1760: Add coverage for 128-bit ranges and IN(Bytes) on U8Given new validation paths, add tests:
- Range ops on
U128/I128fields (should pass).INwithBytesfor aU8field (should pass) and for non-U8(should fail).I can draft these tests if you share a contract fixture with
U128/I128and aU8field.Also applies to: 1834-1969
packages/rs-drive/src/query/mod.rs (1)
2231-2305: Nice shared tests for StartsWith and IN-duplicatesThese guard recent validation paths well. Consider adding similar tests for the SQL path too.
packages/rs-drive/src/query/filter.rs (5)
251-266: Check contract/type before evaluating owner clause (micro-optimization and clarity)Evaluate target matching first; then run owner checks. Avoids unnecessary work on mismatched transitions.
- // Check owner clause (if any) - let new_owner_value: Value = transfer.recipient_owner_id().into(); - let owner_ok = match owner_clause { - Some(clause) => clause.matches_value(&new_owner_value), - None => true, - }; - if !owner_ok { - return TransitionCheckResult::Fail; - } - - // Ensure transition targets this filter's contract and document type - if transfer.base().data_contract_id() != self.contract.id() - || transfer.base().document_type_name() != &self.document_type_name - { - return TransitionCheckResult::Fail; - } + // Ensure transition targets this filter's contract and document type + if transfer.base().data_contract_id() != self.contract.id() + || transfer.base().document_type_name() != &self.document_type_name + { + return TransitionCheckResult::Fail; + } + + // Check owner clause (if any) + let new_owner_value: Value = transfer.recipient_owner_id().into(); + let owner_ok = match owner_clause { + Some(clause) => clause.matches_value(&new_owner_value), + None => true, + }; + if !owner_ok { + return TransitionCheckResult::Fail; + }
290-305: UpdatePrice: validate target first, then price clauseSame rationale as Transfer; check contract/type before clause eval.
- // Check price clause (if any) - let price_value = Value::U64(update_price.price()); - let price_ok = match price_clause { - Some(clause) => clause.matches_value(&price_value), - None => true, - }; - if !price_ok { - return TransitionCheckResult::Fail; - } - - // Ensure transition targets this filter's contract and document type + // Ensure transition targets this filter's contract and document type if update_price.base().data_contract_id() != self.contract.id() || update_price.base().document_type_name() != &self.document_type_name { return TransitionCheckResult::Fail; } + + // Check price clause (if any) + let price_value = Value::U64(update_price.price()); + let price_ok = match price_clause { + Some(clause) => clause.matches_value(&price_value), + None => true, + }; + if !price_ok { + return TransitionCheckResult::Fail; + }
329-344: Purchase: validate target first, then owner clauseMinor ordering tweak for consistency and early-exit.
- // Check owner clause (if any) - let owner_ok = match (owner_clause, batch_owner_value) { - (Some(clause), Some(val)) => clause.matches_value(val), - (Some(_), None) => return TransitionCheckResult::Fail, // a required context is missing - (None, _) => true, - }; - if !owner_ok { - return TransitionCheckResult::Fail; - } - - // Ensure transition targets this filter's contract and document type + // Ensure transition targets this filter's contract and document type if purchase.base().data_contract_id() != self.contract.id() || purchase.base().document_type_name() != &self.document_type_name { return TransitionCheckResult::Fail; } + + // Check owner clause (if any) + let owner_ok = match (owner_clause, batch_owner_value) { + (Some(clause), Some(val)) => clause.matches_value(val), + (Some(_), None) => return TransitionCheckResult::Fail, // a required context is missing + (None, _) => true, + }; + if !owner_ok { + return TransitionCheckResult::Fail; + }
410-417: Silence clippy: method doesn’t use selfUntil/unless you move this to InternalClauses, add an allow to keep clippy clean.
- #[cfg(any(feature = "server", feature = "verify"))] - fn evaluate_clauses( + #[cfg(any(feature = "server", feature = "verify"))] + #[allow(clippy::unused_self)] + fn evaluate_clauses(
90-130: Gate enum with the same feature flags as the structKeeps API surface consistent when features are disabled.
-#[derive(Debug, PartialEq, Clone)] -pub enum DocumentActionMatchClauses { +#[cfg(any(feature = "server", feature = "verify"))] +#[derive(Debug, PartialEq, Clone)] +pub enum DocumentActionMatchClauses {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/rs-drive/src/query/conditions.rs(3 hunks)packages/rs-drive/src/query/filter.rs(1 hunks)packages/rs-drive/src/query/mod.rs(7 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
**/*.rs: Use 4-space indentation for Rust files
Format Rust code with rustfmt defaults
Keep Rust code clippy-clean (no warnings)
In Rust, modules use snake_case, types use PascalCase, constants use SCREAMING_SNAKE_CASE
Files:
packages/rs-drive/src/query/conditions.rspackages/rs-drive/src/query/filter.rspackages/rs-drive/src/query/mod.rs
packages/**
📄 CodeRabbit inference engine (AGENTS.md)
Place all source packages under packages/* (JS/TS packages and Rust crates)
Files:
packages/rs-drive/src/query/conditions.rspackages/rs-drive/src/query/filter.rspackages/rs-drive/src/query/mod.rs
**/*.{js,jsx,ts,tsx,rs}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{js,jsx,ts,tsx,rs}: Use LF line endings in source files
Ensure a final newline at end of source files
Files:
packages/rs-drive/src/query/conditions.rspackages/rs-drive/src/query/filter.rspackages/rs-drive/src/query/mod.rs
🧠 Learnings (3)
📚 Learning: 2025-07-23T08:31:42.268Z
Learnt from: CR
PR: dashpay/platform#0
File: packages/wasm-sdk/CLAUDE.md:0-0
Timestamp: 2025-07-23T08:31:42.268Z
Learning: Applies to packages/wasm-sdk/src/**/*.rs : The WASM SDK now fully supports where and orderBy clauses for document queries; use the specified JSON array formats and supported operators.
Applied to files:
packages/rs-drive/src/query/conditions.rspackages/rs-drive/src/query/mod.rs
📚 Learning: 2024-10-29T14:40:54.727Z
Learnt from: lklimek
PR: dashpay/platform#2277
File: packages/rs-sdk/src/core/transaction.rs:0-0
Timestamp: 2024-10-29T14:40:54.727Z
Learning: In `packages/rs-sdk/src/platform/document_query.rs` and `packages/rs-sdk/src/core/transaction.rs`, certain places don't implement `IntoInner`, so direct error mappings cannot be simplified using `IntoInner`. A TODO comment has been added to address this in a future PR.
Applied to files:
packages/rs-drive/src/query/filter.rs
📚 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 (3)
packages/rs-drive/src/query/conditions.rs (1)
packages/rs-drive/src/query/mod.rs (6)
from(310-327)from(2309-2365)clauses(532-543)clauses(588-605)clauses(662-673)query(779-787)
packages/rs-drive/src/query/filter.rs (2)
packages/rs-drive/src/query/mod.rs (9)
query(779-787)clauses(532-543)clauses(588-605)clauses(662-673)contract(937-942)contract(1035-1040)internal_clauses(1229-1234)from(310-327)from(2309-2365)packages/rs-drive/src/query/conditions.rs (3)
validate_internal_clauses_against_schema(1612-1694)from(197-199)from(1371-1373)
packages/rs-drive/src/query/mod.rs (1)
packages/rs-drive/src/query/conditions.rs (4)
allowed_ops_for_type(1378-1413)validate_internal_clauses_against_schema(1612-1694)validate_where_clause_against_schema(1490-1608)value_shape_ok(1433-1486)
🔇 Additional comments (6)
packages/rs-drive/src/query/conditions.rs (2)
1488-1609: LGTM: strong schema validation with operator/value-shape checksField existence, operator allow-listing, value-shape/type checks (including IN duplicates/length and StartsWith empties) look solid.
1610-1694: LGTM: internal-clause schema validation and PK-specific typingComposition check plus
$idsegregation and identifier enforcement on PK clauses are correct.packages/rs-drive/src/query/mod.rs (2)
5-9: Re-exports look good and are properly gatedPublicly exposing validation helpers and
ValueClauseunder the same feature flags is consistent.
551-554: Schema validation injected at all constructorsValidating
InternalClausesright after extraction ensures early, consistent failures across inputs.Also applies to: 679-682, 843-846
packages/rs-drive/src/query/filter.rs (2)
132-365: Solid two-phase matching design (Pass/Fail/NeedsOriginal) and nested field supportAction dispatch, primary-key short-circuiting, and dot-path resolution look correct and keep fetches minimal. Nice alignment with index-agnostic validation.
486-667: Validation is correctly index-agnostic; nice use of schema checksThe structural checks and operator/value typing look good and align with subscription semantics.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
packages/rs-drive/src/query/filter.rs (1)
662-664: Gate tests with the same feature flags as the implementationTests call items behind server/verify feature flags; without gating, they fail to compile when those features are off.
-#[cfg(test)] -mod tests { +#[cfg(all(test, any(feature = "server", feature = "verify")))] +mod tests {
🧹 Nitpick comments (4)
packages/rs-drive/src/query/filter.rs (4)
176-207: DRY up repeated “original clauses” decision logicThe conditional sequence (empty → Pass, PK-only → evaluate on id, else → NeedsOriginal) is repeated across actions. Extract into a small helper to reduce duplication and keep semantics consistent.
Apply this change in the Replace arm (apply the same pattern to Delete/Transfer/UpdatePrice/Purchase):
- if original_document_clauses.is_empty() { - return TransitionCheckResult::Pass; - } - if original_document_clauses.is_for_primary_key() { - if self.evaluate_clauses( - original_document_clauses, - &id_value, - &BTreeMap::new(), - ) { - return TransitionCheckResult::Pass; - } - return TransitionCheckResult::Fail; - } - TransitionCheckResult::NeedsOriginal + return self.original_requirement_result(original_document_clauses, &id_value);Add this helper (outside the selected range):
#[cfg(any(feature = "server", feature = "verify"))] impl DriveDocumentQueryFilter<'_> { fn original_requirement_result( &self, clauses: &InternalClauses, id_value: &Value, ) -> TransitionCheckResult { if clauses.is_empty() { TransitionCheckResult::Pass } else if clauses.is_for_primary_key() { if self.evaluate_clauses(clauses, id_value, &BTreeMap::new()) { TransitionCheckResult::Pass } else { TransitionCheckResult::Fail } } else { TransitionCheckResult::NeedsOriginal } } }
642-660: Simplify and tighten path walkingMinor simplification; eliminates the extra Option state and makes early returns explicit.
-fn get_value_by_path<'a>(root: &'a BTreeMap<String, Value>, path: &str) -> Option<&'a Value> { - if path.is_empty() { - 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 { - match current { - Some(Value::Map(ref vm)) => { - current = vm.get_optional_key(seg); - } - _ => return None, - } - } - current -} +fn get_value_by_path<'a>(root: &'a BTreeMap<String, Value>, path: &str) -> Option<&'a Value> { + if path.is_empty() { + return None; + } + let mut segments = path.split('.'); + let first = segments.next()?; + let mut current: &Value = root.get(first)?; + for seg in segments { + current = match current { + Value::Map(ref vm) => vm.get_optional_key(seg)?, + _ => return None, + }; + } + Some(current) +}
1089-1091: Prefer typed literals over casts in testsUse u64-suffixed literals to keep clippy quiet.
- revision: 1 as u64, + revision: 1u64,Also applies to: 1103-1106
1567-1577: Rename misleading variable in test (“invalid_filter” → “non_indexed_filter”)Variable name contradicts the assertion that follows.
- let invalid_filter = DriveDocumentQueryFilter { + let non_indexed_filter = DriveDocumentQueryFilter { contract: &contract, document_type_name: "niceDocument".to_string(), action_clauses: DocumentActionMatchClauses::Create { new_document_clauses: internal_clauses, }, }; assert!( - invalid_filter.validate().is_ok(), + non_indexed_filter.validate().is_ok(), "Structural validate should ignore indexes" );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/rs-drive/src/query/filter.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
**/*.rs: Use 4-space indentation for Rust files
Format Rust code with rustfmt defaults
Keep Rust code clippy-clean (no warnings)
In Rust, modules use snake_case, types use PascalCase, constants use SCREAMING_SNAKE_CASE
Files:
packages/rs-drive/src/query/filter.rs
packages/**
📄 CodeRabbit inference engine (AGENTS.md)
Place all source packages under packages/* (JS/TS packages and Rust crates)
Files:
packages/rs-drive/src/query/filter.rs
**/*.{js,jsx,ts,tsx,rs}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{js,jsx,ts,tsx,rs}: Use LF line endings in source files
Ensure a final newline at end of source files
Files:
packages/rs-drive/src/query/filter.rs
🧠 Learnings (2)
📚 Learning: 2024-10-29T14:40:54.727Z
Learnt from: lklimek
PR: dashpay/platform#2277
File: packages/rs-sdk/src/core/transaction.rs:0-0
Timestamp: 2024-10-29T14:40:54.727Z
Learning: In `packages/rs-sdk/src/platform/document_query.rs` and `packages/rs-sdk/src/core/transaction.rs`, certain places don't implement `IntoInner`, so direct error mappings cannot be simplified using `IntoInner`. A TODO comment has been added to address this in a future PR.
Applied to files:
packages/rs-drive/src/query/filter.rs
📚 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.rs
🧬 Code graph analysis (1)
packages/rs-drive/src/query/filter.rs (1)
packages/rs-drive/src/query/conditions.rs (1)
validate_internal_clauses_against_schema(1612-1694)
🔇 Additional comments (2)
packages/rs-drive/src/query/filter.rs (2)
132-145: Two-phase matching API is clean and avoids unnecessary readsThe Pass/Fail/NeedsOriginal flow is clear and efficient for subscription evaluation. Nice separation of concerns.
438-447: Good: validation is structural-only and returns precise errorsIndex-agnostic validation and explicit QuerySyntaxError mapping are aligned with subscription requirements.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/rs-drive/src/query/mod.rs (2)
506-518: Fix limit validation to use max_query_limit (not default_query_limit)The check rejects values > config.default_query_limit, but the error mentions max limit. Compare against config.max_query_limit to match intent and SQL path behavior.
Apply this diff:
- let limit = maybe_limit - .map_or(Some(config.default_query_limit), |limit_value| { - if limit_value == 0 || limit_value > config.default_query_limit { + let limit = maybe_limit + .map_or(Some(config.default_query_limit), |limit_value| { + if limit_value == 0 || limit_value > config.max_query_limit { None } else { Some(limit_value) } }) .ok_or(Error::Query(QuerySyntaxError::InvalidLimit(format!( "limit greater than max limit {}", config.max_query_limit ))))?;
645-657: Apply the same max limit fix in from_decomposed_valuesMirror the correction here to keep limit validation consistent across entry points.
- let limit = maybe_limit - .map_or(Some(config.default_query_limit), |limit_value| { - if limit_value == 0 || limit_value > config.default_query_limit { + let limit = maybe_limit + .map_or(Some(config.default_query_limit), |limit_value| { + if limit_value == 0 || limit_value > config.max_query_limit { None } else { Some(limit_value) } }) .ok_or(Error::Query(QuerySyntaxError::InvalidLimit(format!( "limit greater than max limit {}", config.max_query_limit ))))?;
♻️ Duplicate comments (1)
packages/rs-drive/src/query/filter.rs (1)
661-665: Gate tests with the same feature flags as the module (optional)The module is only compiled under server/verify, so tests won’t build in no-feature builds anyway. Gating tests similarly avoids accidental breakage if inclusion changes later.
-#[cfg(test)] +#[cfg(all(test, any(feature = "server", feature = "verify")))] mod tests {
🧹 Nitpick comments (3)
packages/rs-drive/src/query/mod.rs (1)
421-436: Micro: simplify primary-key-only order checkAvoid allocating a Vec just to get the first key.
- || (self.internal_clauses.is_empty() - && (self.order_by.is_empty() - || (self.order_by.len() == 1 - && self - .order_by - .keys() - .collect::<Vec<&String>>() - .first() - .unwrap() - .as_str() - == "$id"))) + || (self.internal_clauses.is_empty() + && (self.order_by.is_empty() + || (self.order_by.len() == 1 + && self.order_by.keys().next().map(|k| k.as_str()) == Some("$id"))))packages/rs-drive/src/query/filter.rs (2)
641-659: Edge-case: consider rejecting empty path early (already returns None)Current behavior returns None for empty paths. If you want stricter feedback, you could debug_assert! on empty path in non-release builds.
447-511: Deduplicate owner/price clause validation with small helpersMinor: factor identical typing checks for owner_clause and price_clause into private fns to reduce repetition.
+fn validate_owner_clause(clause: &ValueClause) -> bool { + match clause.operator { + WhereOperator::Equal => matches!(clause.value, Value::Identifier(_)), + WhereOperator::In => matches!(&clause.value, Value::Array(arr) if arr.iter().all(|v| matches!(v, Value::Identifier(_)))), + _ => false, + } +} + +fn validate_price_clause(clause: &ValueClause) -> bool { + match clause.operator { + WhereOperator::Equal + | WhereOperator::GreaterThan + | WhereOperator::GreaterThanOrEquals + | WhereOperator::LessThan + | WhereOperator::LessThanOrEquals => matches!(clause.value, Value::U64(_)|Value::I64(_)|Value::U32(_)|Value::I32(_)|Value::U16(_)|Value::I16(_)|Value::U8(_)|Value::I8(_)), + WhereOperator::Between + | WhereOperator::BetweenExcludeBounds + | WhereOperator::BetweenExcludeLeft + | WhereOperator::BetweenExcludeRight => matches!(&clause.value, Value::Array(arr) if arr.len()==2 && arr.iter().all(|v| matches!(v, Value::U64(_)|Value::I64(_)|Value::U32(_)|Value::I32(_)|Value::U16(_)|Value::I16(_)|Value::U8(_)|Value::I8(_))) && if let Value::Array(a)=&clause.value { a[0] <= a[1] } else { true }), + WhereOperator::In => matches!(&clause.value, Value::Array(arr) if arr.iter().all(|v| matches!(v, Value::U64(_)|Value::I64(_)|Value::U32(_)|Value::I32(_)|Value::U16(_)|Value::I16(_)|Value::U8(_)|Value::I8(_)))), + WhereOperator::StartsWith => false, + } +}Then call these helpers from validate().
Also applies to: 512-593, 594-629
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/rs-drive/src/query/conditions.rs(3 hunks)packages/rs-drive/src/query/filter.rs(1 hunks)packages/rs-drive/src/query/mod.rs(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/rs-drive/src/query/conditions.rs
🧰 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
**/*.rs: Use 4-space indentation for Rust files
Format Rust code with rustfmt defaults
Keep Rust code clippy-clean (no warnings)
In Rust, modules use snake_case, types use PascalCase, constants use SCREAMING_SNAKE_CASE
Files:
packages/rs-drive/src/query/filter.rspackages/rs-drive/src/query/mod.rs
packages/**
📄 CodeRabbit inference engine (AGENTS.md)
Place all source packages under packages/* (JS/TS packages and Rust crates)
Files:
packages/rs-drive/src/query/filter.rspackages/rs-drive/src/query/mod.rs
**/*.{js,jsx,ts,tsx,rs}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{js,jsx,ts,tsx,rs}: Use LF line endings in source files
Ensure a final newline at end of source files
Files:
packages/rs-drive/src/query/filter.rspackages/rs-drive/src/query/mod.rs
🧠 Learnings (3)
📚 Learning: 2024-10-29T14:40:54.727Z
Learnt from: lklimek
PR: dashpay/platform#2277
File: packages/rs-sdk/src/core/transaction.rs:0-0
Timestamp: 2024-10-29T14:40:54.727Z
Learning: In `packages/rs-sdk/src/platform/document_query.rs` and `packages/rs-sdk/src/core/transaction.rs`, certain places don't implement `IntoInner`, so direct error mappings cannot be simplified using `IntoInner`. A TODO comment has been added to address this in a future PR.
Applied to files:
packages/rs-drive/src/query/filter.rs
📚 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
📚 Learning: 2025-07-23T08:31:42.268Z
Learnt from: CR
PR: dashpay/platform#0
File: packages/wasm-sdk/CLAUDE.md:0-0
Timestamp: 2025-07-23T08:31:42.268Z
Learning: Applies to packages/wasm-sdk/src/**/*.rs : The WASM SDK now fully supports where and orderBy clauses for document queries; use the specified JSON array formats and supported operators.
Applied to files:
packages/rs-drive/src/query/mod.rs
🧬 Code graph analysis (2)
packages/rs-drive/src/query/filter.rs (2)
packages/rs-drive/src/query/mod.rs (7)
query(775-783)clauses(532-543)clauses(586-603)clauses(660-671)internal_clauses(1223-1228)from(310-327)from(2227-2283)packages/rs-drive/src/query/conditions.rs (3)
validate_internal_clauses_against_schema(1667-1749)from(197-199)from(1351-1353)
packages/rs-drive/src/query/mod.rs (1)
packages/rs-drive/src/query/conditions.rs (4)
allowed_ops_for_type(1358-1403)validate_internal_clauses_against_schema(1667-1749)validate_where_clause_against_schema(1486-1663)value_shape_ok(1425-1482)
🔇 Additional comments (9)
packages/rs-drive/src/query/mod.rs (3)
5-9: Good public re-exports for filter consumersExporting ValueClause and the schema-validation helpers under the same feature gate looks correct and keeps the public surface coherent.
155-157: Filter module correctly gated and documentedpub mod filter behind cfg(any(feature = "server", feature = "verify")) prevents no-feature build breaks. Doc string is clear.
1220-1280: Error message keeps users on the right pathfind_best_index error texts are specific and helpful; threshold check with defaults::MAX_INDEX_DIFFERENCE looks fine.
packages/rs-drive/src/query/filter.rs (6)
1-32: Excellent high-level docs and two-phase evaluation planClear guidance on Pass/Fail/NeedsOriginal and action semantics makes this easy to adopt.
161-175: Solid short-circuiting and PK-only optimization across actionsEarly rejections and handling PK-only clauses without fetching originals are correct and efficient.
Also applies to: 176-207, 231-262, 263-294, 295-327
371-430: Correct nested-field resolution and value evaluationUsing get_value_by_path plus WhereClause::matches_value keeps clause logic centralized; returning false on missing fields is expected.
432-631: Validation is structural and index-agnostic — goodvalidate() aligns with subscription needs and reuses validate_internal_clauses_against_schema. Owner/price clause typing checks look right.
898-942: Test for nested paths covers Value::Map walkingNice coverage ensuring "meta.status" works with ValueMapHelper.
1262-1360: Great action-specific testsThese verify two-phase matching and owner/price-only scenarios. They’ll be useful for future refactors.
Also applies to: 1361-1464
|
✅ DashSDKFFI.xcframework built for this PR.
SwiftPM (host the zip at a stable URL, then use): .binaryTarget(
name: "DashSDKFFI",
url: "https://your.cdn.example/DashSDKFFI.xcframework.zip",
checksum: "26393ee4aeb7bd1d00c2de1a9cd119667f884c67bb7a9025bd9364271383d60c"
)Xcode manual integration:
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/rs-drive/src/query/conditions.rs (1)
1889-2001: Strengthen assertions for clearer failure signalsWhere possible, assert on the specific error variant/message instead of just
is_err(), to prevent false positives if a different error is thrown.Example:
- let res = validate_where_clause_against_schema(doc_type, &clause); - assert!(res.is_err()); + let res = validate_where_clause_against_schema(doc_type, &clause) + .expect_err("expected validation to fail"); + matches!( + res, + crate::error::Error::Query( + crate::error::query::QuerySyntaxError::InvalidWhereClauseComponents(_) + ) + );Also consider adding positive tests for meta fields to guard the fix above:
@@ fn validate_rejects_in_bytes_for_string_field() { @@ } + + #[test] + fn validate_allows_meta_fields() { + let fixture = get_data_contract_fixture(None, 0, LATEST_PLATFORM_VERSION.protocol_version); + let contract = fixture.data_contract_owned(); + let doc_type = contract + .document_type_for_name("niceDocument") + .expect("doc type exists"); + + // $ownerId equality + let clause_owner = WhereClause { + field: "$ownerId".to_string(), + operator: Equal, + value: Value::Identifier([3u8; 32]), + }; + assert!(validate_where_clause_against_schema(doc_type, &clause_owner).is_ok()); + + // $createdAt range + let clause_created = WhereClause { + field: "$createdAt".to_string(), + operator: GreaterThanOrEquals, + value: Value::U64(1), + }; + assert!(validate_where_clause_against_schema(doc_type, &clause_created).is_ok()); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/rs-drive/src/query/conditions.rs(3 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.rs: Format Rust code with cargo fmt
Run Clippy linter for Rust code
Files:
packages/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-drive/src/query/conditions.rs
🧠 Learnings (2)
📚 Learning: 2025-07-23T08:31:42.268Z
Learnt from: CR
PR: dashpay/platform#0
File: packages/wasm-sdk/CLAUDE.md:0-0
Timestamp: 2025-07-23T08:31:42.268Z
Learning: Applies to packages/wasm-sdk/src/**/*.rs : The WASM SDK now fully supports where and orderBy clauses for document queries; use the specified JSON array formats and supported operators.
Applied to files:
packages/rs-drive/src/query/conditions.rs
📚 Learning: 2025-02-03T23:34:43.081Z
Learnt from: QuantumExplorer
PR: dashpay/platform#2450
File: packages/rs-drive/src/drive/tokens/distribution/queries.rs:57-57
Timestamp: 2025-02-03T23:34:43.081Z
Learning: In Rust query implementations, the `..` operator after a value creates a `RangeFrom` bound rather than performing vector slicing, which is a safe operation for constructing range-based queries.
Applied to files:
packages/rs-drive/src/query/conditions.rs
🧬 Code graph analysis (1)
packages/rs-drive/src/query/conditions.rs (1)
packages/rs-drive/src/query/mod.rs (4)
clauses(531-542)clauses(585-602)clauses(659-670)query(774-782)
⏰ 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)
- GitHub Check: Build JS packages / Build JS
- GitHub Check: Swift SDK and Example build (warnings as errors)
🔇 Additional comments (7)
packages/rs-drive/src/query/conditions.rs (7)
220-227: Solid: in-memory matcher for WhereClauseDelegating to the shared evaluator is clean and keeps SQL/path logic separate from subscription filtering.
1279-1296: Good addition: ValueClause for scalar (non-field) comparisonsThis keeps transition-supplied values first-class without overloading WhereClause.
1297-1354: Operator evaluator looks correct; Bytes IN and bound-order guards coveredCentralizing semantics avoids drift between WhereClause/ValueClause. Nice.
1356-1404: Allowed-ops matrix is consistent with schema expectationsIdentifier/ByteArray limited to {Equal, In}; strings permit StartsWith and ranges. 👍
1405-1421: Numeric-shape helper correctly includes 128‑bit intsAligns with earlier range support for U128/I128.
1423-1482: Value-shape validation is tight and type-awareCovers text ranges, numeric ranges, and Bytes vs Array shapes for IN.
1754-1788: Tests: range grouping coverage is goodCovers allowed/invalid combinations and same‑value incoherence. 👍
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (7)
packages/rs-drive/src/query/conditions.rs (7)
1147-1160: SQL path: normalize numeric literals to the field’s canonical type
sql_value_to_platform_valueemitsValue::I64for integers. For unsigned fields (e.g.,U64) or other integer widths, we should coerce the numeric literal to the canonical variant (or reject on overflow). Today validation lets some mismatches through (fixed by previous comment), but better to normalize here to avoid user confusion.Add a helper and use it in both IN and BinaryOp paths:
fn coerce_for_property_type(prop: &DocumentPropertyType, v: Value) -> Result<Value, Error> { use DocumentPropertyType as T; match (prop, v) { (T::U8, Value::I64(n)) if n >= 0 && n <= u8::MAX as i64 => Ok(Value::U8(n as u8)), (T::U16, Value::I64(n)) if n >= 0 && n <= u16::MAX as i64 => Ok(Value::U16(n as u16)), (T::U32, Value::I64(n)) if n >= 0 => Ok(Value::U32(n as u32)), (T::U64, Value::I64(n)) if n >= 0 => Ok(Value::U64(n as u64)), (T::I8, Value::I64(n)) if n >= i8::MIN as i64 && n <= i8::MAX as i64 => Ok(Value::I8(n as i8)), (T::I16, Value::I64(n)) if n >= i16::MIN as i64 && n <= i16::MAX as i64 => Ok(Value::I16(n as i16)), (T::I32, Value::I64(n)) if n >= i32::MIN as i64 && n <= i32::MAX as i64 => Ok(Value::I32(n as i32)), (T::I64, Value::I64(_)) => Ok(v), (T::U128, Value::I64(n)) if n >= 0 => Ok(Value::U128(n as u128)), (T::I128, Value::I64(n)) => Ok(Value::I128(n as i128)), (T::F64, Value::I64(n)) => Ok(Value::Float(n as f64)), // Strings are already converted via value_from_string; identifiers/bytes handled elsewhere _ => Ok(v), } }Then, when building
in_valuesand singletransformed_value, callcoerce_for_property_type(property_type.as_ref(), platform_value)before pushing/using the value; propagate an error if out of range.Also applies to: 1282-1295, 1297-1311
613-617: Fix error message copy/paste: it’s a range clause, not an IN clauseMessage says “in clause has same field as an equality clause” while handling a range clause; this confuses users.
Apply:
- "in clause has same field as an equality clause", + "range clause has same field as an equality clause",
1083-1089: Avoid potential overflow/panic when computing StartsWith upper bound
*last_char += 1;can overflow (0xFF) and panic in debug builds. Use carry-aware increment; if overflow, treat upper bound as unbounded or append 0xFF sentinel.Apply:
- *last_char += 1; + if *last_char == u8::MAX { + // Carry: append 0x00, and let right bound be prefix + [0x00] (exclusive) + right_key.push(0); + } else { + *last_char = last_char.wrapping_add(1); + }Alternatively, compute a
prefix_upper_bound(&left_key) -> Option<Vec<u8>>and use an open-ended range whenNone.
177-242: Deduplicate Between evaluator branches*The four Between variants repeat the same bound-order guard and comparisons. Factor a small helper to reduce risk of drift.
Example:
impl WhereOperator { + fn bounds_ok(bounds: &[Value], allow_eq: bool) -> bool { + bounds.len() == 2 + && matches!( + bounds[0].partial_cmp(&bounds[1]), + Some(Ordering::Less) | Some(Ordering::Equal) if allow_eq || matches!(bounds[0].partial_cmp(&bounds[1]), Some(Ordering::Less)) + ) + } pub fn eval(&self, probe: &Value, clause_value: &Value) -> bool { match self { @@ - WhereOperator::Between => match clause_value { - Value::Array(bounds) if bounds.len() == 2 => { - match bounds[0].partial_cmp(&bounds[1]) { - Some(Ordering::Less) | Some(Ordering::Equal) => { - probe >= &bounds[0] && probe <= &bounds[1] - } - _ => false, - } - } - _ => false, - }, + WhereOperator::Between => match clause_value { + Value::Array(bounds) if Self::bounds_ok(bounds, true) => { + probe >= &bounds[0] && probe <= &bounds[1] + } + _ => false, + }, // Apply same pattern to the other Between* variants...
1765-1791: Expose meta-field typing helper for reuse
meta_field_property_typeis useful beyond this module (e.g., SQL builder, validators). Considerpub(crate)and reusing it where meta fields are resolved elsewhere to keep mappings in sync.-fn meta_field_property_type(field: &str) -> Option<DocumentPropertyType> { +pub(crate) fn meta_field_property_type(field: &str) -> Option<DocumentPropertyType> {
1199-1244: Avoid panics in parsing: return structured errors instead
panic!("unreachable: ...")appears twice. Prefer returningError::Query(QuerySyntaxError::InvalidSQL(...))to keep this library non-panicking on malformed inputs.Replace both panics with
return Err(Error::Query(QuerySyntaxError::InvalidSQL("expected identifier/value".to_string())));
1931-1976: Add tests to cover equality numeric-type strictness and SQL numeric coercionOnce equality type matching is tightened and numeric coercion added, add tests to ensure:
- Equality on
U64rejectsValue::I64in validation.- SQL
WHERE myU64 = 5yields aWhereClausewithValue::U64(5).I can draft these unit tests once you confirm the desired coercion semantics.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/rs-drive/src/query/conditions.rs(7 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.rs: Format Rust code with cargo fmt
Run Clippy linter for Rust code
Files:
packages/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-drive/src/query/conditions.rs
🧠 Learnings (3)
📚 Learning: 2025-07-23T08:31:42.268Z
Learnt from: CR
PR: dashpay/platform#0
File: packages/wasm-sdk/CLAUDE.md:0-0
Timestamp: 2025-07-23T08:31:42.268Z
Learning: Applies to packages/wasm-sdk/src/**/*.rs : The WASM SDK now fully supports where and orderBy clauses for document queries; use the specified JSON array formats and supported operators.
Applied to files:
packages/rs-drive/src/query/conditions.rs
📚 Learning: 2025-02-03T23:34:43.081Z
Learnt from: QuantumExplorer
PR: dashpay/platform#2450
File: packages/rs-drive/src/drive/tokens/distribution/queries.rs:57-57
Timestamp: 2025-02-03T23:34:43.081Z
Learning: In Rust query implementations, the `..` operator after a value creates a `RangeFrom` bound rather than performing vector slicing, which is a safe operation for constructing range-based queries.
Applied to files:
packages/rs-drive/src/query/conditions.rs
📚 Learning: 2024-10-29T14:40:54.727Z
Learnt from: lklimek
PR: dashpay/platform#2277
File: packages/rs-sdk/src/core/transaction.rs:0-0
Timestamp: 2024-10-29T14:40:54.727Z
Learning: In `packages/rs-sdk/src/platform/document_query.rs` and `packages/rs-sdk/src/core/transaction.rs`, certain places don't implement `IntoInner`, so direct error mappings cannot be simplified using `IntoInner`. A TODO comment has been added to address this in a future PR.
Applied to files:
packages/rs-drive/src/query/conditions.rs
🧬 Code graph analysis (1)
packages/rs-drive/src/query/conditions.rs (1)
packages/rs-drive/src/query/mod.rs (4)
query(772-780)clauses(529-540)clauses(583-600)clauses(657-668)
⏰ 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). (8)
- GitHub Check: Rust packages (dash-sdk) / Tests
- GitHub Check: Rust packages (drive) / Tests
- GitHub Check: Rust packages (drive) / Unused dependencies
- GitHub Check: Rust packages (drive) / Linting
- GitHub Check: Rust packages (drive-abci) / Tests
- GitHub Check: Build Docker images (Drive, drive, drive-abci) / Build Drive image
- GitHub Check: Build JS packages / Build JS
- GitHub Check: Swift SDK and Example build (warnings as errors)
🔇 Additional comments (1)
packages/rs-drive/src/query/conditions.rs (1)
1-3: Run cargo fmt and cargo clippy for packages/rs-drive and attach outputsSandbox verification failed with: "Could not locate working directory: no /proc/self/exe available" — run locally and ensure the crate is clippy-clean:
cargo fmt --all -- --check
cargo clippy -p rs-drive --all-features -- -D warnings
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/rs-drive/src/query/conditions.rs (2)
127-135: Do not treat IN as a “range” operator (removes unnecessary orderBy requirement)
is_range()returning true forInforces an orderBy in downstream logic that doesn’t need it and may reject valid queries. MakeInreturn false here; keepStartsWithas range.Apply:
pub const fn is_range(self) -> bool { match self { Equal => false, - GreaterThan | GreaterThanOrEquals | LessThan | LessThanOrEquals | Between - | BetweenExcludeBounds | BetweenExcludeLeft | BetweenExcludeRight | In | StartsWith => { + GreaterThan | GreaterThanOrEquals | LessThan | LessThanOrEquals + | Between | BetweenExcludeBounds | BetweenExcludeLeft | BetweenExcludeRight + | StartsWith => { true } } }
1003-1070: Bug: BetweenExcludeBounds uses inclusive range in reverse scanIn
to_path_queryforBetweenExcludeBounds, when scanning right‑to‑left andstarts_at_key > right_key, the code inserts an inclusive rangeleft..=right, which violates the operator’s “exclude both bounds” semantics. Useafter_to(exclusive on both sides).Apply:
- } else if starts_at_key > right_key { - query.insert_range_inclusive(left_key..=right_key) + } else if starts_at_key > right_key { + // exclude both bounds even in reverse direction + query.insert_range_after_to(left_key..right_key)Add a unit test asserting no boundary keys are returned in reverse traversal.
♻️ Duplicate comments (3)
packages/rs-drive/src/query/mod.rs (2)
150-152: Feature‑gate forfiltermodule is correctPrevents no‑feature builds from breaking; matches prior guidance.
164-176: Making StartAtDocument public unblocks external compositionAddresses earlier request; doc comments are clear.
packages/rs-drive/src/query/conditions.rs (1)
1512-1567: Equality type-check should match runtime equality semantics (avoid silent non‑matches)
Equalcurrently accepts any integer variant for integer fields; runtimeValueequality is variant‑strict, so some clauses validate but can never match. Reuse the already‑definedvalue_type_matchesclosure for theEqualarm.Apply:
- WhereOperator::Equal => { - use DocumentPropertyType as T; - let ok = match property_type { - // Accept any integer-like value for integer fields (signed/unsigned), reject floats - T::U8 - | T::U16 - | T::U32 - | T::U64 - | T::U128 - | T::I8 - | T::I16 - | T::I32 - | T::I64 - | T::I128 => { - matches!( - self.value, - Value::U128(_) - | Value::I128(_) - | Value::U64(_) - | Value::I64(_) - | Value::U32(_) - | Value::I32(_) - | Value::U16(_) - | Value::I16(_) - | Value::U8(_) - | Value::I8(_) - ) - } - T::F64 => matches!(self.value, Value::Float(_)), - T::Date => matches!( - self.value, - Value::U64(_) - | Value::I64(_) - | Value::U32(_) - | Value::I32(_) - | Value::U16(_) - | Value::I16(_) - | Value::U8(_) - | Value::I8(_) - ), - T::String(_) => matches!(self.value, Value::Text(_)), - T::Identifier => matches!(self.value, Value::Identifier(_)), - T::ByteArray(_) => matches!(self.value, Value::Bytes(_)), - T::Boolean => matches!(self.value, Value::Bool(_)), - // Not applicable for object/array/variable arrays - T::Object(_) | T::Array(_) | T::VariableTypeArray(_) => false, - }; + WhereOperator::Equal => { + let ok = value_type_matches(property_type, &self.value); if !ok { return Err(Error::Query( QuerySyntaxError::InvalidWhereClauseComponents( "invalid value type for equality", ), )); } }
🧹 Nitpick comments (2)
packages/rs-drive/src/query/conditions.rs (1)
95-122: Fix typo casing in error messages (“StartsWith”)Minor polish: error strings use “Startswith” instead of “StartsWith”.
Apply:
- StartsWith => Err(Error::Query(QuerySyntaxError::InvalidWhereClauseOrder( - "Startswith clause order invalid", + StartsWith => Err(Error::Query(QuerySyntaxError::InvalidWhereClauseOrder( + "StartsWith clause order invalid", ))),packages/rs-drive/src/query/mod.rs (1)
1988-1996: OrderBy requirement should not apply to IN clausesThis block derives
left_to_rightbased onis_range(). With the suggested change to excludeInfromis_range(), IN queries won’t unnecessarily requireorderBy.After adjusting
is_range(), run the query tests that useINwithoutorderByto confirm no regressions.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/rs-drive/src/query/conditions.rs(7 hunks)packages/rs-drive/src/query/filter.rs(1 hunks)packages/rs-drive/src/query/mod.rs(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/rs-drive/src/query/filter.rs
🧰 Additional context used
📓 Path-based instructions (2)
**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.rs: Format Rust code with cargo fmt
Run Clippy linter for Rust code
Files:
packages/rs-drive/src/query/conditions.rspackages/rs-drive/src/query/mod.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-drive/src/query/conditions.rspackages/rs-drive/src/query/mod.rs
🧠 Learnings (3)
📚 Learning: 2025-07-23T08:31:42.268Z
Learnt from: CR
PR: dashpay/platform#0
File: packages/wasm-sdk/CLAUDE.md:0-0
Timestamp: 2025-07-23T08:31:42.268Z
Learning: Applies to packages/wasm-sdk/src/**/*.rs : The WASM SDK now fully supports where and orderBy clauses for document queries; use the specified JSON array formats and supported operators.
Applied to files:
packages/rs-drive/src/query/conditions.rspackages/rs-drive/src/query/mod.rs
📚 Learning: 2025-02-03T23:34:43.081Z
Learnt from: QuantumExplorer
PR: dashpay/platform#2450
File: packages/rs-drive/src/drive/tokens/distribution/queries.rs:57-57
Timestamp: 2025-02-03T23:34:43.081Z
Learning: In Rust query implementations, the `..` operator after a value creates a `RangeFrom` bound rather than performing vector slicing, which is a safe operation for constructing range-based queries.
Applied to files:
packages/rs-drive/src/query/conditions.rs
📚 Learning: 2024-10-29T14:40:54.727Z
Learnt from: lklimek
PR: dashpay/platform#2277
File: packages/rs-sdk/src/core/transaction.rs:0-0
Timestamp: 2024-10-29T14:40:54.727Z
Learning: In `packages/rs-sdk/src/platform/document_query.rs` and `packages/rs-sdk/src/core/transaction.rs`, certain places don't implement `IntoInner`, so direct error mappings cannot be simplified using `IntoInner`. A TODO comment has been added to address this in a future PR.
Applied to files:
packages/rs-drive/src/query/conditions.rs
🧬 Code graph analysis (2)
packages/rs-drive/src/query/conditions.rs (1)
packages/rs-drive/src/query/mod.rs (5)
query(866-874)validate_against_schema(305-397)clauses(623-634)clauses(677-694)clauses(751-762)
packages/rs-drive/src/query/mod.rs (3)
packages/rs-drive/src/query/conditions.rs (1)
validate_against_schema(1394-1583)packages/rs-drive/src/query/vote_polls_by_document_type_query.rs (1)
document_type(266-271)packages/rs-drive/src/drive/document/index_uniqueness/internal/validate_uniqueness_of_data/v0/mod.rs (1)
document_type(62-224)
⏰ 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)
- GitHub Check: Rust crates security audit
- GitHub Check: Swift SDK and Example build (warnings as errors)
🔇 Additional comments (8)
packages/rs-drive/src/query/conditions.rs (6)
176-241: Good abstraction: single evaluator centralizes operator semanticsConsolidating WHERE operator evaluation into
WhereOperator::evalreduces duplication and keepsWhereClause/ValueClausein sync. Looks solid, includingIN(Bytes)and bound‑order checks.
243-302: Value shape checks are comprehensive (incl. 128‑bit ints)Covers F64 vs. integral paths and string ranges correctly;
is_numeric_valuealso includes I128/U128.
1458-1476: Validation: nice guard for BETWEEN bound orderingAscending‑bounds check prevents surprising matches on reversed tuples.
1207-1220: Meta/system fields typing in SQL path is correctResolving
$ownerId,$createdAt,$revision, etc., viameta_field_property_typebefore conversions avoids false “property not in document type” errors.Also applies to: 1342-1355
1592-1607: ValueClause evaluation reuse is goodLeveraging
WhereOperator::evalhere ensures parity withWhereClause.
1676-1702: Meta field map looks complete and consistent with query rulesCovers identifiers, dates, block heights, revision, protocol version, and
$typeas string.If
$protocolVersionis intentionally exposed to queries, confirm it should be U64 (not U32) for all supported platform versions.packages/rs-drive/src/query/mod.rs (2)
5-5: Re‑exporting ValueClause is correctPublic surface now exposes
ValueClausewithWhereClause/WhereOperator; aligns with new filter module usage.
303-397: Schema validation on InternalClauses is well‑scopedGood separation: forbids
$idoutside primary‑key clauses, enforces IN/EQ typing and shape, and reusesWhereClause::validate_against_schema.Please add a test that
primary_key_in_clausewith an empty array is rejected viain_values()in this path.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
packages/rs-drive/src/query/filter.rs (1)
646-648: Gate tests behind the same feature flags as the implementationTests reference items gated by
server/verify. Without those features, the crate’s tests won’t compile. This was previously flagged; looks like a regression.Apply this diff:
-#[cfg(test)] +#[cfg(all(test, any(feature = "server", feature = "verify")))] mod tests {
🧹 Nitpick comments (2)
packages/rs-drive/src/query/filter.rs (2)
534-555: Avoid cross‑type Value comparisons in Between validation
arr[0] <= arr[1]relies onValue’s PartialOrd across mixed integer variants, which can be surprising. Normalize to a single integer type first.Apply this diff inside the
Between*arm to ensure robust numeric comparisons:- | WhereOperator::BetweenExcludeRight => match &price.value { - Value::Array(arr) => { - arr.len() == 2 - && arr.iter().all(|v| { - matches!( - v, - Value::U64(_) - | Value::I64(_) - | Value::U32(_) - | Value::I32(_) - | Value::U16(_) - | Value::I16(_) - | Value::U8(_) - | Value::I8(_) - ) - }) - && arr[0] <= arr[1] - } - _ => false, - }, + | WhereOperator::BetweenExcludeRight => match &price.value { + Value::Array(arr) => { + if arr.len() != 2 { + return false; + } + let to_i128 = |v: &Value| -> Option<i128> { + match v { + Value::U64(n) => Some(*n as i128), + Value::I64(n) => Some(*n as i128), + Value::U32(n) => Some(*n as i128), + Value::I32(n) => Some(*n as i128), + Value::U16(n) => Some(*n as i128), + Value::I16(n) => Some(*n as i128), + Value::U8(n) => Some(*n as i128), + Value::I8(n) => Some(*n as i128), + _ => None, + } + }; + match (to_i128(&arr[0]), to_i128(&arr[1])) { + (Some(a), Some(b)) => a <= b, + _ => false, + } + } + _ => false, + },
181-205: DRY the original‑clauses decision (empty / primary‑key / needs original)The same triage pattern repeats across Replace/Delete/Transfer/UpdatePrice/Purchase. Extract a small helper to reduce duplication and risk of drift.
Example helper and usage:
impl DriveDocumentQueryFilter<'_> { + #[cfg(any(feature = "server", feature = "verify"))] + fn decide_with_original_clauses( + &self, + clauses: &InternalClauses, + id_value: &Value, + ) -> TransitionCheckResult { + if clauses.is_empty() { + return TransitionCheckResult::Pass; + } + if clauses.is_for_primary_key() { + if self.evaluate_clauses(clauses, id_value, &BTreeMap::new()) { + return TransitionCheckResult::Pass; + } + return TransitionCheckResult::Fail; + } + TransitionCheckResult::NeedsOriginal + }Then replace the repeated blocks with:
- if original_document_clauses.is_empty() { - return TransitionCheckResult::Pass; - } - if original_document_clauses.is_for_primary_key() { - if self.evaluate_clauses( - original_document_clauses, - &id_value, - &BTreeMap::new(), - ) { - return TransitionCheckResult::Pass; - } - return TransitionCheckResult::Fail; - } - TransitionCheckResult::NeedsOriginal + self.decide_with_original_clauses(original_document_clauses, &id_value)Also applies to: 212-229, 244-261, 276-293, 308-325
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/rs-drive/src/query/filter.rs(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.rs: Format Rust code with cargo fmt
Run Clippy linter for Rust code
Files:
packages/rs-drive/src/query/filter.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-drive/src/query/filter.rs
🧠 Learnings (2)
📚 Learning: 2024-10-29T14:40:54.727Z
Learnt from: lklimek
PR: dashpay/platform#2277
File: packages/rs-sdk/src/core/transaction.rs:0-0
Timestamp: 2024-10-29T14:40:54.727Z
Learning: In `packages/rs-sdk/src/platform/document_query.rs` and `packages/rs-sdk/src/core/transaction.rs`, certain places don't implement `IntoInner`, so direct error mappings cannot be simplified using `IntoInner`. A TODO comment has been added to address this in a future PR.
Applied to files:
packages/rs-drive/src/query/filter.rs
📚 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.rs
🧬 Code graph analysis (1)
packages/rs-drive/src/query/filter.rs (2)
packages/rs-drive/src/query/mod.rs (6)
query(866-874)clauses(623-634)clauses(677-694)clauses(751-762)from(401-418)from(2335-2391)packages/rs-drive/src/query/conditions.rs (2)
from(326-328)from(1587-1589)
⏰ 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). (6)
- GitHub Check: Build Docker images (Dashmate helper, dashmate-helper, dashmate-helper) / Build Dashmate helper image
- GitHub Check: Build Docker images (Drive, drive, drive-abci) / Build Drive image
- GitHub Check: Build Docker images (DAPI, dapi, dapi) / Build DAPI image
- GitHub Check: Build JS packages / Build JS
- GitHub Check: Rust crates security audit
- GitHub Check: Swift SDK and Example build (warnings as errors)
🔇 Additional comments (2)
packages/rs-drive/src/query/filter.rs (2)
20-31: Solid two-phase matching and structural (index-agnostic) validationThe Pass/Fail/NeedsOriginal split and validation focus look good and align with subscription semantics.
619-644: Good dot‑path resolver for nested mapsThe
get_value_by_pathwalker usingValueMapHelpercovers nested object access cleanly.
QuantumExplorer
changed the title
Issue being fixed or feature implemented
This PR introduces the
DriveDocumentQueryFilterstruct, which is used to filter document state transitions based on specified criteria. It includes methods to check if a state transition or document matches the filter, as well as utility functions for evaluating where clauses.The filter will be primarily used in the context of clients subscribing to document events in Platform.
What was done?
Create filter module with DriveDocumentQueryFilter, related functions, and tests.
How Has This Been Tested?
Unit tests
Breaking Changes
Checklist:
For repository code-owners and collaborators only
Summary by CodeRabbit
New Features
Validation
Tests
Documentation