feat(drive)!: multiple IN clauses on consecutive index properties in document queries - #4391
feat(drive)!: multiple IN clauses on consecutive index properties in document queries#4391QuantumExplorer wants to merge 2 commits into
Conversation
…document queries Drive's document-query grammar historically allowed at most one IN clause per query, treating it as a range-class operator. But an IN over several consecutive properties of a compound index is just a bounded cross-product of point lookups, a shape grovedb path queries natively express (a key set at one path level with per-key subqueries carrying another key set at the next level), proofs included. This lifts the grammar restriction for plain document queries (SELECT documents), not the grouped-aggregate surfaces. Grammar: `InternalClauses.in_clause: Option<WhereClause>` becomes `in_clauses: Vec<WhereClause>`; `WhereClause::group_clauses` groups any number of IN clauses structurally (still rejecting duplicate and equality-overlapping fields), and the count/sum aggregate validator keeps rejecting more than one explicitly. Consensus gate: acceptance is decided at path-query lowering, the choke point shared by execution, proof generation, and proof verification. A new `DriveDocumentQueryMethodVersions.non_primary_key_path_query` feature version dispatches `get_non_primary_key_path_query`: v0 (all tables through protocol version 13) rejects multiple IN clauses with the historical `MultipleInClauses` error, v1 (protocol version 14, unreleased) lowers them to multi-level key-set path queries. Single-IN shapes lower through the v0 body under both versions, byte-identically. v1 semantics (conservative): - The IN clauses must sit on consecutive index properties immediately after the equality prefix, with an optional single range clause right after the last IN; index selection only considers conforming indexes. - Every IN'd property and the range property need an orderBy entry; results come back in index traversal order with per-level direction. - The product of IN list sizes is capped at 100 (the single-IN worst case); each list keeps its existing 100-value cap. - startAt/startAfter with more than one IN clause is rejected: the cross-branch cursor machinery bakes the cursor's start keys into the default subquery applied to every sibling branch, which is only correct under a single-branch ancestry. Processing fees derive from the operations of the actual grovedb traversal, so cost scales with the enumerated branches automatically. Tests: grammar acceptance/rejection units, execution + proof round-trips against the live root hash on the family compound indexes (2-IN, 3-IN, equality prefix, trailing range, descending levels, cross-product cap, consecutiveness, cursor rejection), a protocol version 13 rejection on both the no-proof and prove paths, wire-level drive-abci getDocuments v1 tests at both protocol versions, and a version-table freeze test pinning the gate to v14. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
📝 WalkthroughWalkthroughThe query system replaces the singular ChangesMulti-IN document queries
Estimated code review effort: 4 (Complex) | ~45 minutes Mergeability Score: 🟠 High · up to The PR expands multi-IN handling across document queries, proof verification, and existing withdrawal and aggregate paths, but current code can misclassify an IN filter, broaden malformed verifier input, and bypass aggregate validation. These defects may produce incorrect results or inconsistent query behavior, so merge should wait for fixes. Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
|
⛔ Blockers found — Opus deferred (commit ec4d6e2) |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## v4.2-dev #4391 +/- ##
============================================
- Coverage 86.81% 86.46% -0.36%
============================================
Files 2647 2647
Lines 340850 342564 +1714
============================================
+ Hits 295913 296190 +277
- Misses 44937 46374 +1437
🚀 New features to boost your workflow:
|
The multi-IN lowering is a pure function of the contract, so the storage-backed integration tests in query_tests.rs have lib-target twins here: nested key-set structure for two IN levels, the equality prefix + two IN levels + trailing range shape, the protocol version 13 rejection, and the cross-product cap, consecutiveness, cursor, and missing-order-by rejections. Raises patch coverage where the PR coverage phase only runs the lib target. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/rs-drive/src/query/drive_document_count_query/drive_dispatcher.rs (1)
254-266: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winRoute sum and average prove paths through the validator
Drive::execute_document_sum_requestcallsdetect_sum_modewith raw clauses.Drive::execute_document_average_provealso uses raw clauses. Route both paths throughvalidate_and_canonicalize_where_clausesbefore mode detection and index selection.having.rsdefines unsupported types and has no execution path.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/rs-drive/src/query/drive_document_count_query/drive_dispatcher.rs` around lines 254 - 266, Update Drive::execute_document_sum_request and Drive::execute_document_average_prove to call validate_and_canonicalize_where_clauses before detect_sum_mode, mode detection, or index selection. Use the validated and canonicalized clauses throughout both prove paths, preserving existing handling for supported clauses and avoiding changes to having.rs.
🧹 Nitpick comments (1)
packages/rs-drive/src/query/mod.rs (1)
4096-4115: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winLoose error assertions for the missing-
orderBybranch inpackages/rs-drive/src/query/mod.rsandpackages/rs-drive/tests/query_tests.rs. Both tests cover the same lowering branch and both assert onlyError::Query(_), so an unrelated query error, such as an index-selection failure, would keep them green. The lowering returnsQuerySyntaxError::MissingOrderByForRangefor this shape.
packages/rs-drive/src/query/mod.rs#L4096-L4115: assertError::Query(QuerySyntaxError::MissingOrderByForRange(_))inmissing_order_by_on_an_in_field_is_rejected.packages/rs-drive/tests/query_tests.rs#L8282-L8316: assertError::Query(QuerySyntaxError::MissingOrderByForRange(_))intest_multiple_in_clauses_require_order_by_on_each_in_field.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/rs-drive/src/query/mod.rs` around lines 4096 - 4115, Strengthen the error assertions in `missing_order_by_on_an_in_field_is_rejected` at packages/rs-drive/src/query/mod.rs:4096-4115 and `test_multiple_in_clauses_require_order_by_on_each_in_field` at packages/rs-drive/tests/query_tests.rs:8282-8316 to match `Error::Query(QuerySyntaxError::MissingOrderByForRange(_))` rather than any `Error::Query(_)`, preserving each test’s existing setup and failure message.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/rs-drive/SECONDARY_INDEX_QUERIES.md`:
- Line 35: Update the `WhereClause` documentation and the corresponding
description at the additional referenced section to identify these as
non-primary-key `IN` clauses allowed in plain document queries from protocol
version 14, while explicitly retaining rejection of multiple `IN` clauses for
grouped aggregate queries. Keep the wording aligned with the
`DriveDocumentQuery` lowering contract.
In
`@packages/rs-drive/src/drive/identity/withdrawals/document/find_withdrawal_documents_by_status_and_transaction_indices/v0/mod.rs`:
- Around line 74-80: Update the InternalClauses initializer in the
withdrawal-document query to place the transaction-index WhereOperator::In
clause in in_clauses, while leaving only the status equality clause in
equal_clauses. Use the existing transaction-index clause construction and keep
unrelated clause fields unchanged.
In `@packages/wasm-drive-verify/src/document/verify_proof.rs`:
- Around line 168-176: Validate that in_clauses is an actual array with
Array::is_array before converting or iterating it in the parsers in
packages/wasm-drive-verify/src/document/verify_proof.rs (lines 168-176),
packages/wasm-drive-verify/src/document/verify_proof_keep_serialized.rs (lines
157-165), and
packages/wasm-drive-verify/src/document/verify_start_at_document_in_proof.rs
(lines 172-180); reject non-array values rather than allowing Array::from to
silently produce an empty array, while preserving parsing of valid arrays.
---
Outside diff comments:
In `@packages/rs-drive/src/query/drive_document_count_query/drive_dispatcher.rs`:
- Around line 254-266: Update Drive::execute_document_sum_request and
Drive::execute_document_average_prove to call
validate_and_canonicalize_where_clauses before detect_sum_mode, mode detection,
or index selection. Use the validated and canonicalized clauses throughout both
prove paths, preserving existing handling for supported clauses and avoiding
changes to having.rs.
---
Nitpick comments:
In `@packages/rs-drive/src/query/mod.rs`:
- Around line 4096-4115: Strengthen the error assertions in
`missing_order_by_on_an_in_field_is_rejected` at
packages/rs-drive/src/query/mod.rs:4096-4115 and
`test_multiple_in_clauses_require_order_by_on_each_in_field` at
packages/rs-drive/tests/query_tests.rs:8282-8316 to match
`Error::Query(QuerySyntaxError::MissingOrderByForRange(_))` rather than any
`Error::Query(_)`, preserving each test’s existing setup and failure message.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: e1f24f04-0b43-4c37-a589-8ef4753408ad
📒 Files selected for processing (29)
packages/rs-drive-abci/src/execution/platform_events/fee_pool_outwards_distribution/fetch_reward_shares_list_for_masternode/v0/mod.rspackages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/batch/data_triggers/triggers/dpns/v0/mod.rspackages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/batch/data_triggers/triggers/dpns/v1/mod.rspackages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/batch/data_triggers/triggers/withdrawals/v0/mod.rspackages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/batch/data_triggers/triggers/withdrawals/v1/mod.rspackages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/batch/state/v0/fetch_documents.rspackages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/batch/tests/document/dpns.rspackages/rs-drive-abci/src/query/document_query/v0/mod.rspackages/rs-drive-abci/src/query/document_query/v1/tests.rspackages/rs-drive/SECONDARY_INDEX_QUERIES.mdpackages/rs-drive/src/drive/document/index_uniqueness/internal/validate_uniqueness_of_data/v0/mod.rspackages/rs-drive/src/drive/document/index_uniqueness/internal/validate_uniqueness_of_data/v1/mod.rspackages/rs-drive/src/drive/identity/withdrawals/document/fetch_oldest_withdrawal_documents_by_status/v0/mod.rspackages/rs-drive/src/drive/identity/withdrawals/document/find_withdrawal_documents_by_status_and_transaction_indices/v0/mod.rspackages/rs-drive/src/query/conditions.rspackages/rs-drive/src/query/defaults.rspackages/rs-drive/src/query/drive_document_count_query/drive_dispatcher.rspackages/rs-drive/src/query/filter.rspackages/rs-drive/src/query/mod.rspackages/rs-drive/tests/query_tests.rspackages/rs-platform-version/src/version/drive_versions/drive_document_method_versions/mod.rspackages/rs-platform-version/src/version/drive_versions/drive_document_method_versions/v1.rspackages/rs-platform-version/src/version/drive_versions/drive_document_method_versions/v2.rspackages/rs-platform-version/src/version/drive_versions/drive_document_method_versions/v3.rspackages/rs-platform-version/src/version/drive_versions/drive_document_method_versions/v4.rspackages/rs-platform-version/src/version/v14.rspackages/wasm-drive-verify/src/document/verify_proof.rspackages/wasm-drive-verify/src/document/verify_proof_keep_serialized.rspackages/wasm-drive-verify/src/document/verify_start_at_document_in_proof.rs
| pub primary_key_in_clause: Option<WhereClause>, // IN clause on $id | ||
| pub primary_key_equal_clause: Option<WhereClause>, // == clause on $id | ||
| pub in_clause: Option<WhereClause>, // Single IN clause on indexed field | ||
| pub in_clauses: Vec<WhereClause>, // IN clauses on indexed fields (several allowed from protocol version 14) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Scope the v14 allowance to plain document queries.
The text says that several IN clauses are allowed from protocol version 14 without excluding grouped aggregate queries. The PR contract retains multiple-IN rejection for grouped aggregate queries. The text also should identify these as non-primary-key IN clauses.
Update both descriptions to state the plain-document scope and retain the grouped-aggregate rejection. This keeps the documentation aligned with the PR objective and the versioned DriveDocumentQuery lowering contract.
Proposed wording
- pub in_clauses: Vec<WhereClause>, // IN clauses on indexed fields (several allowed from protocol version 14)
+ pub in_clauses: Vec<WhereClause>, // Non-primary-key IN clauses on plain document queries
-**IN clauses per query**: One before protocol version 14. From protocol
+**Non-primary-key IN clauses in plain document queries**: One before protocol
version 14, and several from protocol version 14 when they sit on consecutive
properties of one compound index ...
+Grouped aggregate queries continue to reject multiple IN clauses.Also applies to: 241-246
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/rs-drive/SECONDARY_INDEX_QUERIES.md` at line 35, Update the
`WhereClause` documentation and the corresponding description at the additional
referenced section to identify these as non-primary-key `IN` clauses allowed in
plain document queries from protocol version 14, while explicitly retaining
rejection of multiple `IN` clauses for grouped aggregate queries. Keep the
wording aligned with the `DriveDocumentQuery` lowering contract.
| // Parse in_clauses (array form; protocol version 14+ accepts several) | ||
| if let Ok(clauses) = Reflect::get(&obj, &JsValue::from_str("in_clauses")) { | ||
| if !clauses.is_null() && !clauses.is_undefined() { | ||
| let clauses_array = Array::from(&clauses); | ||
| for i in 0..clauses_array.length() { | ||
| internal_clauses | ||
| .in_clauses | ||
| .push(parse_where_clause(&clauses_array.get(i))?); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
for file in \
packages/wasm-drive-verify/src/document/verify_proof.rs \
packages/wasm-drive-verify/src/document/verify_proof_keep_serialized.rs \
packages/wasm-drive-verify/src/document/verify_start_at_document_in_proof.rs
do
rg -n -C 3 'in_clauses|Array::from|parse_internal_clauses' "$file"
done
rg -n -C 3 --glob '*.rs' \
'in_clauses.*(array|Array)|parse_internal_clauses' \
packages/wasm-drive-verifyRepository: dashpay/platform
Length of output: 11289
🏁 Script executed:
#!/bin/bash
set -euo pipefail
for file in \
packages/wasm-drive-verify/src/document/verify_proof.rs \
packages/wasm-drive-verify/src/document/verify_proof_keep_serialized.rs \
packages/wasm-drive-verify/src/document/verify_start_at_document_in_proof.rs
do
echo "=== $file ==="
sed -n '120,205p' "$file"
done
echo "=== Array::is_array and parser tests/usages ==="
rg -n -C 4 'Array::is_array|Array::from|in_clauses|in_clause|parse_where_clause' \
packages/wasm-drive-verify Cargo.toml packages --glob '*.rs' --glob '*.toml' | head -n 500Repository: dashpay/platform
Length of output: 50372
🏁 Script executed:
node - <<'JS'
const inputs = [
{},
{0: "x", length: 1},
{length: 0},
"abc",
7,
null,
undefined,
["x"]
];
for (const value of inputs) {
let result;
try {
result = Array.from(value);
} catch (error) {
result = `${error.name}: ${error.message}`;
}
console.log(JSON.stringify(value), "=>", result);
}
JSRepository: dashpay/platform
Length of output: 457
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Relevant parser tails and imports ==="
for file in \
packages/wasm-drive-verify/src/document/verify_proof.rs \
packages/wasm-drive-verify/src/document/verify_proof_keep_serialized.rs \
packages/wasm-drive-verify/src/document/verify_start_at_document_in_proof.rs
do
echo "--- $file ---"
sed -n '1,35p' "$file"
sed -n '200,330p' "$file"
done
echo "=== in_clauses references outside the three parsers ==="
rg -n --glob '*.rs' --glob '*.ts' --glob '*.js' \
'in_clauses|in_clause' packages | grep -vE \
'packages/wasm-drive-verify/src/document/(verify_proof|verify_proof_keep_serialized|verify_start_at_document_in_proof)\.rs' \
| head -n 300
echo "=== InternalClauses definitions and consumers ==="
rg -n -C 5 --glob '*.rs' 'struct InternalClauses|in_clauses:' packages | head -n 300
echo "=== js-sys metadata/cache for Array::from ==="
rg -n -C 5 'pub fn from|is_array' \
"$HOME/.cargo/registry/src" "$HOME/.cargo/git/checkouts" 2>/dev/null \
| grep -E 'js-sys|array\.rs' | head -n 120 || trueRepository: dashpay/platform
Length of output: 50372
🏁 Script executed:
node - <<'JS'
const values = [
{ name: 'plain object', value: {} },
{ name: 'array-like object', value: {0: 'clause', length: 1} },
{ name: 'string', value: 'clause' },
{ name: 'number', value: 1 },
{ name: 'array', value: [] },
{ name: 'typed array', value: new Uint8Array([1, 2]) },
];
for (const {name, value} of values) {
console.log(name, {
isArray: Array.isArray(value),
arrayFromLength: Array.from(value).length,
});
}
JSRepository: dashpay/platform
Length of output: 450
🌐 Web query:
js-sys Array::from JsValue Array::is_array Rust wasm-bindgen documentation
💡 Result:
In the js-sys crate for Rust and wasm-bindgen, you can interact with JavaScript arrays and check their types using the following methods: 1. Checking if a value is an Array: You can determine if a JsValue is a JavaScript Array using the static method Array::is_array [1]. This is the equivalent of the JavaScript Array.isArray method [1]. use js_sys::Array; use wasm_bindgen::JsValue; let val: JsValue = //... obtained from JS if Array::is_array(&val) { // It is an array } Alternatively, you can use the JsCast trait (available in wasm-bindgen) to check types dynamically [2]: if val.has_type:: { // It is an array } 2. Creating an Array from a JsValue: The Array::from static method is used to create a new, shallow-copied Array instance from an array-like or iterable object [3][4][5]. use js_sys::Array; use wasm_bindgen::JsValue; let val: JsValue = //... let array = Array::from(&val); Note that for generic or typed operations, js-sys also provides variants such as from_iterable and from_iterable_map [3][4][5].
Citations:
- 1: https://github.com/rustwasm/wasm-bindgen/blob/main/crates/js-sys/src/lib.rs
- 2: https://wasm-bindgen.github.io/wasm-bindgen/api/wasm_bindgen/trait.JsCast.html
- 3: https://wasm-bindgen.github.io/wasm-bindgen/api/js_sys/struct.Array.html
- 4: https://docs.rs/js-sys/latest/js_sys/struct.Array.html
- 5: https://wasm-bindgen.github.io/wasm-bindgen/api/src/js_sys/lib.rs.html
Reject non-array in_clauses values before calling Array::from.
Array::from accepts array-like values, so {} becomes an empty array. The verifier then drops the supplied IN constraints and proceeds with a broader query. Add Array::is_array(&clauses) validation in all three parsers: verify_proof.rs, verify_proof_keep_serialized.rs, and verify_start_at_document_in_proof.rs.
📍 Affects 3 files
packages/wasm-drive-verify/src/document/verify_proof.rs#L168-L176(this comment)packages/wasm-drive-verify/src/document/verify_proof_keep_serialized.rs#L157-L165packages/wasm-drive-verify/src/document/verify_start_at_document_in_proof.rs#L172-L180
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/wasm-drive-verify/src/document/verify_proof.rs` around lines 168 -
176, Validate that in_clauses is an actual array with Array::is_array before
converting or iterating it in the parsers in
packages/wasm-drive-verify/src/document/verify_proof.rs (lines 168-176),
packages/wasm-drive-verify/src/document/verify_proof_keep_serialized.rs (lines
157-165), and
packages/wasm-drive-verify/src/document/verify_start_at_document_in_proof.rs
(lines 172-180); reject non-array values rather than allowing Array::from to
silently produce an empty array, while preserving parsing of valid arrays.
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
The protocol-v14 multi-IN lowering has four blocking issues: it ignores descending order on the first leftover index property, changes historical v13 error precedence before versioned dispatch, rejects cursor combinations only after storage/proof work, and keeps consensus-versioned implementations inline rather than in immutable version modules. Two additional suggestions address misleading scope documentation and permissive WASM parsing that can silently broaden a proof query.
Source: reviewer backend model gpt-5.6-sol; final verifier backend model gpt-5.6-sol; openclaw-agent/cliproxy/gpt-5.6-sol is orchestration-only and not reviewer evidence.
Validated blockers were found in the Codex precheck. Opus is deferred until a fresh Codex revalidation clears the blocker gate.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet: not run (deferred by blocker gate)
🔴 4 blocking | 🟡 2 suggestion(s)
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/rs-drive/src/query/mod.rs`:
- [BLOCKING] packages/rs-drive/src/query/mod.rs:2371-2378: Descending order on the first leftover index property is ignored
The v1 multi-IN path delegates the index tail to `recursive_insert_on_query`. That helper computes the requested direction for the first leftover property at lines 1984-1991, but its no-cursor branch constructs that level with `Query::new_with_direction(first.ascending)` at line 2086 instead. An accepted query over `[a, b, c]`, such as `a IN (...) AND b IN (...) ORDER BY a ASC, b ASC, c DESC`, therefore traverses `c` in the index's ascending direction. `Index::matches` permits this shape because the deepest IN field is penultimate and the three order fields are continuous. Implement the requested tail direction in the v1 lowering without editing historical v0 behavior in place, and add execution/proof coverage for descending order on the leftover property.
- [BLOCKING] packages/rs-drive/src/query/mod.rs:2286-2292: Cursor rejection occurs after cursor storage or proof processing
The multi-IN cursor check runs only inside path-query lowering. `construct_path_query_operations` first reads and deserializes `self.start_at` from GroveDB at lines 1212-1268, while proof verification first calls `verify_start_at_document_in_proof` before constructing the query. Consequently, a v14 request for an unsupported multi-IN cursor shape can return `StartDocumentNotFound` or a proof error instead of `Unsupported`, and an existing cursor performs unnecessary state/proof work. Add a shared version-aware shape preflight before cursor lookup or proof extraction. It must retain the historical v0 precedence by returning `MultipleInClauses` before processing a cursor.
- [BLOCKING] packages/rs-drive/src/query/mod.rs:2220-2259: Consensus-critical versions are implemented inline instead of versioned modules
The dispatcher selects feature versions correctly, but `get_non_primary_key_path_query_v0`, `get_non_primary_key_path_query_v1`, and the v1-specific lowering remain in the monolithic `query/mod.rs`. Consensus-versioned Drive methods must keep dispatch in `mod.rs` and implementations in separate `v0/mod.rs` and `v1/mod.rs` modules so already-live behavior is isolated from later edits. Move both implementations into version directories and leave only dispatch at the parent boundary.
In `packages/rs-drive/src/query/conditions.rs`:
- [BLOCKING] packages/rs-drive/src/query/conditions.rs:609-623: The unversioned grammar changes v13 multi-IN errors
Before this PR, `group_clauses` checked the number of non-primary-key IN clauses before checking field overlap, so every query with more than one such clause returned `MultipleInClauses`. The new unversioned loop instead returns `DuplicateNonGroupableClauseSameField` when two IN clauses share a field or when one overlaps an equality clause. Parsing completes before `get_non_primary_key_path_query_v0` dispatches, so the v0 length guard cannot preserve the historical protocol-v13 result. Preserve duplicate and overlap information structurally until version-aware validation: v0 must reject any multi-IN shape with `MultipleInClauses`, while v1 can apply the new duplicate/overlap checks.
In `packages/wasm-drive-verify/src/document/verify_proof.rs`:
- [SUGGESTION] packages/wasm-drive-verify/src/document/verify_proof.rs:168-176: Reject non-array in_clauses values in WASM proof parsers
`Array::from` accepts array-like values rather than requiring a JavaScript array; for example, `{}` becomes an empty array. A caller that supplies malformed `in_clauses` can therefore have its constraints silently discarded, causing the verifier to reconstruct and verify a broader query than requested. Require `Array::is_array(&clauses)` before conversion in this parser and in `verify_proof_keep_serialized.rs` and `verify_start_at_document_in_proof.rs`, returning an invalid-input error for non-array values.
In `packages/rs-drive/SECONDARY_INDEX_QUERIES.md`:
- [SUGGESTION] packages/rs-drive/SECONDARY_INDEX_QUERIES.md:35: Scope the documented v14 multi-IN allowance to plain document queries
The documentation currently says several indexed-field IN clauses are allowed from protocol version 14 without limiting that statement to plain document queries. The PR intentionally keeps count, sum, average, and ranked/grouped aggregate surfaces on the single-IN contract. Identify these as non-primary-key IN clauses for plain document queries here and in the restrictions section at lines 241-246, and explicitly state that grouped aggregate queries continue to reject multiple IN clauses.
| Self::recursive_insert_on_query( | ||
| &mut child_query, | ||
| left_over_index_properties.as_slice(), | ||
| index.unique, | ||
| None, | ||
| deepest_left_to_right, | ||
| Some(&self.order_by), | ||
| platform_version, |
There was a problem hiding this comment.
🔴 Blocking: Descending order on the first leftover index property is ignored
The v1 multi-IN path delegates the index tail to recursive_insert_on_query. That helper computes the requested direction for the first leftover property at lines 1984-1991, but its no-cursor branch constructs that level with Query::new_with_direction(first.ascending) at line 2086 instead. An accepted query over [a, b, c], such as a IN (...) AND b IN (...) ORDER BY a ASC, b ASC, c DESC, therefore traverses c in the index's ascending direction. Index::matches permits this shape because the deepest IN field is penultimate and the three order fields are continuous. Implement the requested tail direction in the v1 lowering without editing historical v0 behavior in place, and add execution/proof coverage for descending order on the leftover property.
source: ['codex']
| let in_clauses = in_clauses_array | ||
| .into_iter() | ||
| .map(|clause| { | ||
| if known_fields.contains(&clause.field) { | ||
| Err(Error::Query( | ||
| QuerySyntaxError::DuplicateNonGroupableClauseSameField( | ||
| "in clause has same field as an equality clause", | ||
| "in clause has same field as an equality or in clause", | ||
| ), | ||
| )) | ||
| } else { | ||
| known_fields.insert(clause.field.clone()); | ||
| Ok(Some(clause.clone())) | ||
| Ok(clause) | ||
| } | ||
| } | ||
| _ => Err(Error::Query(QuerySyntaxError::MultipleInClauses( | ||
| "There should only be one in clause", | ||
| ))), | ||
| }?; | ||
| }) | ||
| .collect::<Result<Vec<WhereClause>, Error>>()?; |
There was a problem hiding this comment.
🔴 Blocking: The unversioned grammar changes v13 multi-IN errors
Before this PR, group_clauses checked the number of non-primary-key IN clauses before checking field overlap, so every query with more than one such clause returned MultipleInClauses. The new unversioned loop instead returns DuplicateNonGroupableClauseSameField when two IN clauses share a field or when one overlaps an equality clause. Parsing completes before get_non_primary_key_path_query_v0 dispatches, so the v0 length guard cannot preserve the historical protocol-v13 result. Preserve duplicate and overlap information structurally until version-aware validation: v0 must reject any multi-IN shape with MultipleInClauses, while v1 can apply the new duplicate/overlap checks.
source: ['codex']
| // Conservative v1: the cross-branch cursor machinery is not wired | ||
| // for key-set branching at more than one level, so reject cursors | ||
| // instead of shipping silently wrong pagination. | ||
| if starts_at_document.is_some() || self.start_at.is_some() { | ||
| return Err(Error::Query(QuerySyntaxError::Unsupported( | ||
| "startAt/startAfter is not supported with multiple in clauses".to_string(), | ||
| ))); |
There was a problem hiding this comment.
🔴 Blocking: Cursor rejection occurs after cursor storage or proof processing
The multi-IN cursor check runs only inside path-query lowering. construct_path_query_operations first reads and deserializes self.start_at from GroveDB at lines 1212-1268, while proof verification first calls verify_start_at_document_in_proof before constructing the query. Consequently, a v14 request for an unsupported multi-IN cursor shape can return StartDocumentNotFound or a proof error instead of Unsupported, and an existing cursor performs unnecessary state/proof work. Add a shared version-aware shape preflight before cursor lookup or proof extraction. It must retain the historical v0 precedence by returning MultipleInClauses before processing a cursor.
source: ['codex']
| pub fn get_non_primary_key_path_query( | ||
| &self, | ||
| document_type_path: Vec<Vec<u8>>, | ||
| starts_at_document: Option<(Document, bool)>, | ||
| platform_version: &PlatformVersion, | ||
| ) -> Result<PathQuery, Error> { | ||
| match platform_version | ||
| .drive | ||
| .methods | ||
| .document | ||
| .query | ||
| .non_primary_key_path_query | ||
| { | ||
| 0 => self.get_non_primary_key_path_query_v0( | ||
| document_type_path, | ||
| starts_at_document, | ||
| platform_version, | ||
| ), | ||
| 1 => self.get_non_primary_key_path_query_v1( | ||
| document_type_path, | ||
| starts_at_document, | ||
| platform_version, | ||
| ), | ||
| version => Err(Error::Drive(DriveError::UnknownVersionMismatch { | ||
| method: "DriveDocumentQuery::get_non_primary_key_path_query".to_string(), | ||
| known_versions: vec![0, 1], | ||
| received: version, | ||
| })), | ||
| } | ||
| } | ||
|
|
||
| #[cfg(any(feature = "server", feature = "verify"))] | ||
| /// v1 of the non-primary-key path query lowering (protocol version 14): | ||
| /// accepts multiple `In` clauses. Single-`In` shapes lower exactly as v0. | ||
| fn get_non_primary_key_path_query_v1( | ||
| &self, | ||
| document_type_path: Vec<Vec<u8>>, | ||
| starts_at_document: Option<(Document, bool)>, | ||
| platform_version: &PlatformVersion, | ||
| ) -> Result<PathQuery, Error> { |
There was a problem hiding this comment.
🔴 Blocking: Consensus-critical versions are implemented inline instead of versioned modules
The dispatcher selects feature versions correctly, but get_non_primary_key_path_query_v0, get_non_primary_key_path_query_v1, and the v1-specific lowering remain in the monolithic query/mod.rs. Consensus-versioned Drive methods must keep dispatch in mod.rs and implementations in separate v0/mod.rs and v1/mod.rs modules so already-live behavior is isolated from later edits. Move both implementations into version directories and leave only dispatch at the parent boundary.
source: ['codex']
| // Parse in_clauses (array form; protocol version 14+ accepts several) | ||
| if let Ok(clauses) = Reflect::get(&obj, &JsValue::from_str("in_clauses")) { | ||
| if !clauses.is_null() && !clauses.is_undefined() { | ||
| let clauses_array = Array::from(&clauses); | ||
| for i in 0..clauses_array.length() { | ||
| internal_clauses | ||
| .in_clauses | ||
| .push(parse_where_clause(&clauses_array.get(i))?); | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: Reject non-array in_clauses values in WASM proof parsers
Array::from accepts array-like values rather than requiring a JavaScript array; for example, {} becomes an empty array. A caller that supplies malformed in_clauses can therefore have its constraints silently discarded, causing the verifier to reconstruct and verify a broader query than requested. Require Array::is_array(&clauses) before conversion in this parser and in verify_proof_keep_serialized.rs and verify_start_at_document_in_proof.rs, returning an invalid-input error for non-array values.
source: ['coderabbit']
| pub primary_key_in_clause: Option<WhereClause>, // IN clause on $id | ||
| pub primary_key_equal_clause: Option<WhereClause>, // == clause on $id | ||
| pub in_clause: Option<WhereClause>, // Single IN clause on indexed field | ||
| pub in_clauses: Vec<WhereClause>, // IN clauses on indexed fields (several allowed from protocol version 14) |
There was a problem hiding this comment.
🟡 Suggestion: Scope the documented v14 multi-IN allowance to plain document queries
The documentation currently says several indexed-field IN clauses are allowed from protocol version 14 without limiting that statement to plain document queries. The PR intentionally keeps count, sum, average, and ranked/grouped aggregate surfaces on the single-IN contract. Identify these as non-primary-key IN clauses for plain document queries here and in the restrictions section at lines 241-246, and explicitly state that grouped aggregate queries continue to reject multiple IN clauses.
source: ['coderabbit']
2 participants
Issue being fixed or feature implemented
Drive's document-query grammar allows at most one
INclause per query —WhereClause::group_clausesrejects a second one withMultipleInClauses, treatingINas a range-class operator. But anINover several consecutive properties of a compound index is not a range at all: it is a bounded cross-product of point lookups —|list1| × |list2|subtrees of the index — a shape grovedb path queries natively express (a key set at one path level, per-key subqueries carrying another key set at the next level), proof generation included. The single-INcap was a drive grammar restriction, not a storage limitation.This PR relaxes the grammar so queries like
work on a compound index over those properties. Plain document queries only (
SELECTdocuments) — the grouped-aggregate surfaces (count/sum/average/ranked) keep rejecting multipleINs.What was done?
Grammar (structural, unversioned).
InternalClauses.in_clause: Option<WhereClause>becamein_clauses: Vec<WhereClause>, andgroup_clausesnow groups any number ofINclauses (still rejecting duplicate fields and equality/INoverlap). The field rename forced a compile-time audit of every consumer: filters, uniqueness validation, withdrawal queries, data triggers, wasm-drive-verify (whose JSin_clausekey stays accepted for back-compat, with a newin_clausesarray form), and the CBOR/gRPC round-tripFromimpl, which now emits allINclauses.Consensus gate (protocol version 14). Which query shapes are accepted is part of the consensus query contract, so acceptance is decided at path-query lowering — the single choke point shared by execution, proof generation, and client proof verification (
construct_path_query*→get_non_primary_key_path_query). Following the repo's versioned-module convention, a newDriveDocumentQueryMethodVersions.non_primary_key_path_queryfeature version dispatches the lowering:INwith the historicalMultipleInClauseserror.DRIVE_DOCUMENT_METHOD_VERSIONS_V4, protocol version 14, unreleased): lowers multipleINs to a multi-level key-set path query. Single-INqueries route through the v0 body under both versions, byte-identically.v1 semantics (deliberately conservative):
INclauses must sit on consecutive index properties immediately after the equality prefix; an optional single range clause may follow the lastIN. Index selection only considers conforming indexes (the existingIndex::matchestail and order-by continuity rules still apply, with the deepestINfield playing the in-field role).IN'd property and the trailing range property require anorderByentry; results return in index traversal order with per-level direction (grovedb supports mixed asc/desc per level).Π |list_i| ≤ 100(defaults::MAX_IN_CROSS_PRODUCT_SIZE) — the same worst-case branch enumeration as one maximal singleIN; each list keeps its 100-value cap.startAt/startAfterwith more than oneINis rejected (Unsupported) rather than shipped broken: the existing cross-branch cursor machinery bakes the cursor's per-level start keys into the default subquery applied to every sibling branch, which is only correct under a single-branch (equality) ancestry.The count/sum dispatcher's shared validator gained an explicit multi-
INguard so the aggregate surfaces keep their existing contract.How Has This Been Tested?
conditions.rs): multipleINs on distinct fields group structurally; same-fieldINs and equality overlap still reject.query_tests.rs, family compound indexes): 2-IN, 3-IN, equality prefix + 2-IN(on the 4-property index), 2-IN+ trailing range, and a descending first level — each cross-checked against a brute-force filter over all stored documents and round-tripped throughexecute_with_proof_only_get_elements, asserting the verified root hash equals the live grovedb root hash and proof results equal no-proof results.MultipleInClauseson both the no-proof and prove paths (and v14 accepts it); 120-branch cross product; non-consecutiveINproperties; cursor pagination; missingorderByon anINfield.drive-abcigetDocumentsv1 handler): multi-INdocuments select returns the expected documents and a proof at protocol version 14, and surfacesMultipleInClausesas a query error at protocol version 13.non_primary_key_path_queryto 0 at v13 and 1 at v14.cargo check --workspace --all-targets, fulldrivetests (server,verify,cbor_query),drive-abcidocument-query tests,drive-proof-verifier,dash-sdklib tests,platform-versiontests, and clippy ondrive— all clean.Breaking Changes
Consensus query contract: protocol version 14 nodes accept a query shape (multiple
INclauses) that v13 nodes reject, gated behind the new versioned lowering so mixed-version networks agree until the upgrade activates. Rust API:InternalClauses.in_clauseis nowin_clauses: Vec<WhereClause>(wasm-drive-verify keeps accepting the JSin_clausekey).Checklist:
For repository code-owners and collaborators only
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
INclauses in compound document queries starting with protocol version 14.INquery inputs.Bug Fixes
INcombinations.Documentation
INclauses.