refactor: structure v1 getDocuments where/order_by/having as typed proto messages#3654
Open
QuantumExplorer wants to merge 16 commits into
Open
refactor: structure v1 getDocuments where/order_by/having as typed proto messages#3654QuantumExplorer wants to merge 16 commits into
QuantumExplorer wants to merge 16 commits into
Conversation
…oto messages Replace the CBOR-bytes-shaped `where` / `order_by` / `having` fields on `GetDocumentsRequestV1` with typed `repeated WhereClause` / `repeated OrderClause` / `repeated WhereClause`. v0 keeps its CBOR shape — only the not-yet-released v1 surface is restructured. - proto: new nested `WhereOperator`, `DocumentFieldValue`, `WhereClause`, `OrderClause` messages under `GetDocumentsRequest`. Regenerated bindings for Rust / nodejs / Objective-C / python / web. - rs-drive: `DriveDocumentQuery::from_typed_clauses` (typed twin of `from_decomposed_values`); `DocumentCountRequest` now carries structured `Vec<WhereClause>` / `Vec<OrderClause>` directly; extracted `validate_and_canonicalize_where_clauses` so both CBOR and typed entry points share validation. 4 count tests + 1 insert_contract test + bench updated. - rs-drive-abci: new `v1/conversions.rs` (proto → drive, schema- agnostic). v0 `query_documents_v0` split into CBOR-decode plus a shared `query_documents_typed` helper that the v1 documents path reuses without a CBOR round-trip. v1 handler rewired to consume typed proto clauses end-to-end. 28 v1 tests rewritten with `wc()` / `oc()` / `pv()` helpers. - rs-sdk: `TryFrom<DocumentQuery> for GetDocumentsRequest` now builds typed proto messages via `where_clause_to_proto` / `order_clause_to_proto` / `value_to_proto`. `DocumentQuery.having` is now `Vec<WhereClause>`; the old CBOR `serialize_vec_to_cbor` helper is gone. Tests: 28 v1 abci + 26 v0 abci + 48 drive count = 102 passing. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughReplace CBOR-encoded GetDocumentsRequest query blobs with typed protobuf clause messages; update generated client bindings, add v1 proto→drive conversions, refactor v0/v1 handlers and drive query construction to use typed Where/Order/Having clauses, and update SDK, tests, and benches. ChangesDocument query structured clauses and request handlers
Sequence Diagram(s)sequenceDiagram
participant Client
participant V1Handler as query_documents_v1
participant Conversions
participant Validator as validate_and_route
participant Dispatcher
participant Drive as DriveDocumentQuery
Client->>V1Handler: Send GetDocumentsRequestV1
V1Handler->>Conversions: decode where/order_by/having selects
Conversions-->>V1Handler: typed clauses & SelectProjection
V1Handler->>Validator: validate_and_route(select, clauses, limit, group_by, having_non_empty)
Validator-->>V1Handler: route decision (documents/count + mode)
V1Handler->>Dispatcher: dispatch_documents_v1 or dispatch_count_v1 with typed clauses
Dispatcher->>Drive: from_typed_clauses / execute
Drive-->>Dispatcher: results
Dispatcher-->>V1Handler: response
V1Handler-->>Client: reply
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches🧪 Generate unit tests (beta)
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## v3.1-dev #3654 +/- ##
============================================
- Coverage 88.15% 88.08% -0.08%
============================================
Files 2515 2521 +6
Lines 307979 308654 +675
============================================
+ Hits 271507 271868 +361
- Misses 36472 36786 +314
🚀 New features to boost your workflow:
|
Collaborator
Review GateCommit:
|
`serialize_vec_to_cbor` was removed when v1 `DocumentQuery` switched to typed proto messages — `ciborium` is now unused. Caught by `cargo machete` on macOS CI. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
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 (2)
packages/rs-drive/src/query/drive_document_count_query/drive_dispatcher.rs (1)
231-239:⚠️ Potential issue | 🟠 Major | ⚡ Quick winThis helper validates ranges but never canonicalizes them.
WhereClause::group_clauses()is only used as a yes/no check here, so a same-field pair likefield > a AND field < bcomes back unchanged as two range clauses. The count dispatcher then routes on a different shape than the regular document-query path, which can misclassify or reject valid range-count requests after the typed-clause migration.Use the grouped output on the
Ok(...)path and only fall back to the original list for the intentionalMultipleRangeClausesexception.🤖 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 231 - 239, The helper validate_and_canonicalize_where_clauses currently calls WhereClause::group_clauses only as a check and returns the original clauses unchanged; change it to use the grouped/canonicalized output when group_clauses returns Ok(grouped) and only fall back to returning the original clauses when group_clauses returns Err(Error::Query(QuerySyntaxError::MultipleRangeClauses(_))); keep propagating other errors as before. Ensure you reference validate_and_canonicalize_where_clauses, WhereClause::group_clauses, and the MultipleRangeClauses error variant in the change.packages/rs-drive/src/query/mod.rs (1)
784-801:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't treat malformed
order_byas "no ordering".This now accepts successfully decoded but invalid
orderBypayloads by silently dropping bad entries (filter_map(... .ok())) or the whole field (Vec::new()). The previous path rejected malformed order clauses; now the same request can run with partial/default ordering and return different results or proof bytes instead of a validation error.Suggested fix
- let order_by_clauses: Vec<OrderClause> = order_by - .map(|id_cbor| { - if let Value::Array(clauses) = id_cbor { - clauses - .iter() - .filter_map(|order_clause| { - if let Value::Array(clauses_components) = order_clause { - OrderClause::from_components(clauses_components).ok() - } else { - None - } - }) - .collect() - } else { - Vec::new() - } - }) - .unwrap_or_default(); + let order_by_clauses: Vec<OrderClause> = match order_by { + None => Vec::new(), + Some(Value::Array(clauses)) => clauses + .iter() + .map(|order_clause| { + if let Value::Array(clauses_components) = order_clause { + OrderClause::from_components(clauses_components).map_err(Error::from) + } else { + Err(Error::Query(QuerySyntaxError::InvalidOrderByProperties( + "order clauses must be an array", + ))) + } + }) + .collect::<Result<Vec<_>, _>>()?, + Some(_) => { + return Err(Error::Query(QuerySyntaxError::InvalidOrderByProperties( + "order clauses must be an array", + ))) + } + };🤖 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 784 - 801, The code silently accepts malformed order_by payloads by using filter_map(... .ok()) and returning Vec::new() for non-array inputs; change this so any decoding/validation error turns into a validation error instead of being dropped. Replace the current mapping that builds order_by_clauses from order_by with logic that: checks order_by is a Value::Array (else return an error), iterates each clause and calls OrderClause::from_components returning a Result (do not use filter_map to discard errors), collect into a Result<Vec<OrderClause>, E> (propagate the first error upward), and return/convert that error into the existing request validation/parse error path so malformed order_by fails the request. Ensure you update the code around order_by_clauses, OrderClause::from_components, and the handling of order_by to use the Result propagation (or ? operator) instead of dropping invalid entries.
🧹 Nitpick comments (2)
packages/rs-sdk/src/platform/documents/document_query.rs (1)
345-362: ⚡ Quick winAvoid cloning owned clause vectors in request conversion.
TryFrom<DocumentQuery>ownsdapi_request, so cloningwhere_clauses,order_by_clauses,having, andgroup_byhere is unnecessary allocation/copy overhead on every request.Proposed refactor
fn try_from(dapi_request: DocumentQuery) -> Result<Self, Self::Error> { - let where_clauses = dapi_request - .where_clauses - .clone() + let DocumentQuery { + select, + data_contract, + document_type_name, + where_clauses, + group_by, + having, + order_by_clauses, + limit, + start, + .. + } = dapi_request; + + let where_clauses = where_clauses .into_iter() .map(where_clause_to_proto) .collect::<Result<Vec<_>, _>>()?; - let order_by = dapi_request - .order_by_clauses - .clone() + let order_by = order_by_clauses .into_iter() .map(order_clause_to_proto) .collect(); - let having = dapi_request - .having - .clone() + let having = having .into_iter() .map(where_clause_to_proto) .collect::<Result<Vec<_>, _>>()?; - let limit = if dapi_request.limit == 0 { + let limit = if limit == 0 { None } else { - Some(dapi_request.limit) + Some(limit) }; - let start_v1 = dapi_request.start.clone().map(|s| match s { + let start_v1 = start.map(|s| match s { Start::StartAfter(b) => V1Start::StartAfter(b), Start::StartAt(b) => V1Start::StartAt(b), }); Ok(GetDocumentsRequest { version: Some(V1(GetDocumentsRequestV1 { - data_contract_id: dapi_request.data_contract.id().to_vec(), - document_type: dapi_request.document_type_name.clone(), + data_contract_id: data_contract.id().to_vec(), + document_type: document_type_name, where_clauses, order_by, limit, prove: true, start: start_v1, - select: dapi_request.select as i32, - group_by: dapi_request.group_by.clone(), + select: select as i32, + group_by, having, })), }) }Also applies to: 383-401
🤖 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-sdk/src/platform/documents/document_query.rs` around lines 345 - 362, The conversion currently clones the owned clause vectors (where_clauses, order_by_clauses, having, group_by) from dapi_request inside the TryFrom<DocumentQuery> impl, causing unnecessary allocations; instead consume the owned fields directly by removing the .clone() calls and call into_iter() on dapi_request.where_clauses, dapi_request.order_by_clauses, dapi_request.having, and dapi_request.group_by so you map with where_clause_to_proto/order_clause_to_proto and collect as before (preserving the Result handling for where/having), and apply the same change to the other occurrences referenced around the 383-401 block.packages/rs-drive-abci/src/query/document_query/v1/tests.rs (1)
27-52: ⚡ Quick winKeep
pvbehavior aligned with its documented contract.The comment says this mirrors SDK wire mapping, but
pvcurrently handles only a subset ofValuevariants. Either implement the SDK-supported variants here too, or narrow the comment to explicitly state it supports only the subset used in these tests.🤖 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-abci/src/query/document_query/v1/tests.rs` around lines 27 - 52, The pv helper currently claims to mirror the SDK wire conversion but only handles a subset of Value variants; update pv (in tests) to either fully mirror rs-sdk's value_to_proto mapping or change its doc comment to state it only supports the test subset. To fully fix: open the pv function and add match arms for the remaining dpp::platform_value::Value variants the SDK supports (e.g., Null/NullValue, Object/Map -> document_field_value::Variant::Document or equivalent, Decimal -> Double/Bytes per SDK mapping, Date/Time -> appropriate proto variant, and any other enum cases), producing a ProtoDocumentFieldValue with the matching document_field_value::Variant as done for existing cases; alternatively, if you choose the narrower approach, update the function comment above pv to explicitly list the supported Value variants so it no longer claims to mirror the full SDK wire conversion. Ensure you reference pv and document_field_value::Variant (and keep behavior consistent with rs-sdk/src/platform/documents/document_query.rs::value_to_proto).
🤖 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-abci/src/query/document_query/v0/mod.rs`:
- Around line 96-106: The current parsing of order_by_value silently drops
malformed clauses (order_by_clauses) via filter_map and
OrderClause::from_components(...).ok(), which turns bad-CBOR into a different
ordering instead of rejecting the request; change the parsing to validate and
fail fast: when order_by_value is Some(Value::Array(clauses)) iterate through
clauses and return an error immediately if any clause is not an array or
OrderClause::from_components(&components) returns Err (propagate or convert that
Err into the request validation error) rather than skipping it; update the
surrounding code that uses order_by_clauses (the match on order_by_value and any
callers) to handle/propagate the Result<Vec<OrderClause>, Error> so malformed
order_by inputs cause a request rejection instead of silently using Vec::new().
In `@packages/rs-drive-abci/src/query/document_query/v1/conversions.rs`:
- Around line 74-95: value_from_proto currently cannot produce Value::Null which
prevents sending null operands over v1; update the match in value_from_proto to
handle the null variant from the proto (e.g. document_field_value::Variant::Null
or the proto's equivalent) and return Value::Null, and ensure list recursion
already handles nullable entries by allowing value_from_proto to return
Value::Null for list elements so queries like
test_valid_query_timestamp_field_with_null_value can be expressed over the v1
wire format.
In `@packages/rs-drive-abci/src/query/document_query/v1/mod.rs`:
- Around line 80-86: validate_and_route currently rejects the shape (group_by =
[in_field] + range clause) as not_yet_implemented, which prevents
Drive::execute_document_count_request's RangeAggregateCarrierProof path from
being used; update validate_and_route to return a RoutingDecision that includes
the prove path (e.g., RoutingDecision::Prove or the equivalent variant your enum
uses) for the shape where group_by.len() == 1 && group_by[0] == in_field && a
range where_clause exists, or alternatively remove this early rejection here and
push the check into detect_mode so Drive::execute_document_count_request can
select RangeAggregateCarrierProof; reference validate_and_route, detect_mode,
and RangeAggregateCarrierProof in your change.
---
Outside diff comments:
In `@packages/rs-drive/src/query/drive_document_count_query/drive_dispatcher.rs`:
- Around line 231-239: The helper validate_and_canonicalize_where_clauses
currently calls WhereClause::group_clauses only as a check and returns the
original clauses unchanged; change it to use the grouped/canonicalized output
when group_clauses returns Ok(grouped) and only fall back to returning the
original clauses when group_clauses returns
Err(Error::Query(QuerySyntaxError::MultipleRangeClauses(_))); keep propagating
other errors as before. Ensure you reference
validate_and_canonicalize_where_clauses, WhereClause::group_clauses, and the
MultipleRangeClauses error variant in the change.
In `@packages/rs-drive/src/query/mod.rs`:
- Around line 784-801: The code silently accepts malformed order_by payloads by
using filter_map(... .ok()) and returning Vec::new() for non-array inputs;
change this so any decoding/validation error turns into a validation error
instead of being dropped. Replace the current mapping that builds
order_by_clauses from order_by with logic that: checks order_by is a
Value::Array (else return an error), iterates each clause and calls
OrderClause::from_components returning a Result (do not use filter_map to
discard errors), collect into a Result<Vec<OrderClause>, E> (propagate the first
error upward), and return/convert that error into the existing request
validation/parse error path so malformed order_by fails the request. Ensure you
update the code around order_by_clauses, OrderClause::from_components, and the
handling of order_by to use the Result propagation (or ? operator) instead of
dropping invalid entries.
---
Nitpick comments:
In `@packages/rs-drive-abci/src/query/document_query/v1/tests.rs`:
- Around line 27-52: The pv helper currently claims to mirror the SDK wire
conversion but only handles a subset of Value variants; update pv (in tests) to
either fully mirror rs-sdk's value_to_proto mapping or change its doc comment to
state it only supports the test subset. To fully fix: open the pv function and
add match arms for the remaining dpp::platform_value::Value variants the SDK
supports (e.g., Null/NullValue, Object/Map ->
document_field_value::Variant::Document or equivalent, Decimal -> Double/Bytes
per SDK mapping, Date/Time -> appropriate proto variant, and any other enum
cases), producing a ProtoDocumentFieldValue with the matching
document_field_value::Variant as done for existing cases; alternatively, if you
choose the narrower approach, update the function comment above pv to explicitly
list the supported Value variants so it no longer claims to mirror the full SDK
wire conversion. Ensure you reference pv and document_field_value::Variant (and
keep behavior consistent with
rs-sdk/src/platform/documents/document_query.rs::value_to_proto).
In `@packages/rs-sdk/src/platform/documents/document_query.rs`:
- Around line 345-362: The conversion currently clones the owned clause vectors
(where_clauses, order_by_clauses, having, group_by) from dapi_request inside the
TryFrom<DocumentQuery> impl, causing unnecessary allocations; instead consume
the owned fields directly by removing the .clone() calls and call into_iter() on
dapi_request.where_clauses, dapi_request.order_by_clauses, dapi_request.having,
and dapi_request.group_by so you map with
where_clause_to_proto/order_clause_to_proto and collect as before (preserving
the Result handling for where/having), and apply the same change to the other
occurrences referenced around the 383-401 block.
🪄 Autofix (Beta)
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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1ea375d7-8478-4ea1-8ac4-70f50b50b8cc
📒 Files selected for processing (20)
packages/dapi-grpc/build.rspackages/dapi-grpc/clients/drive/v0/nodejs/drive_pbjs.jspackages/dapi-grpc/clients/platform/v0/nodejs/platform_pbjs.jspackages/dapi-grpc/clients/platform/v0/nodejs/platform_protoc.jspackages/dapi-grpc/clients/platform/v0/objective-c/Platform.pbobjc.hpackages/dapi-grpc/clients/platform/v0/objective-c/Platform.pbobjc.mpackages/dapi-grpc/clients/platform/v0/python/platform_pb2.pypackages/dapi-grpc/clients/platform/v0/web/platform_pb.d.tspackages/dapi-grpc/clients/platform/v0/web/platform_pb.jspackages/dapi-grpc/protos/platform/v0/platform.protopackages/rs-drive-abci/src/query/document_query/v0/mod.rspackages/rs-drive-abci/src/query/document_query/v1/conversions.rspackages/rs-drive-abci/src/query/document_query/v1/mod.rspackages/rs-drive-abci/src/query/document_query/v1/tests.rspackages/rs-drive/benches/document_count_worst_case.rspackages/rs-drive/src/drive/contract/insert/insert_contract/v0/mod.rspackages/rs-drive/src/query/drive_document_count_query/drive_dispatcher.rspackages/rs-drive/src/query/drive_document_count_query/tests.rspackages/rs-drive/src/query/mod.rspackages/rs-sdk/src/platform/documents/document_query.rs
`yarn build` regenerates the gRPC service stubs that `cargo build`
alone doesn't touch:
- platform_pb2_grpc.py (Python)
- Platform.pbrpc.{h,m} (Objective-C)
- PlatformGrpc.java (Java)
And drops the now-stale `ciborium` entry from `dash-sdk`'s
`Cargo.lock` (the dep was removed in 3f27c2b but Cargo.lock
wasn't refreshed in that commit).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- v1 wire: add `DocumentFieldValue.null_value` variant so the typed surface can carry `null` operands the v0 CBOR shape supports (previously dropped). Conversions on both sides (rs-drive-abci `value_from_proto`, rs-sdk `value_to_proto`) + the test-only `pv` helper handle the variant. - v1 routing: allow `group_by=[in_field]` and `group_by=[range_field]` alongside the other constraint — drive's `detect_mode` picks `RangeAggregateCarrierProof` (grovedb #663) on the prove path and `RangeNoProof` / `RangeDistinctProof` on the no-prove path. Both produce entries that match the caller's single-field GROUP BY. The two `reject_*` tests become `accept_*` tests. - v0 abci handler: propagate `order_by` clause-parse errors instead of silently dropping malformed components. - rs-drive `from_decomposed_values`: same tightening for the legacy CBOR `order_by` parse path — malformed clauses now reject the request rather than silently producing partial / default ordering. - drive_dispatcher: fix `validate_and_canonicalize_where_clauses` docstring to accurately describe validation-only behavior on the worktree base (no `> AND <` → `between*` merge step). - rs-sdk: destructure `dapi_request` in `TryFrom<DocumentQuery>` to drop the per-field `.clone()` calls. - tests `pv` helper: narrow its docstring to the explicit subset of `Value` variants it handles, with a panic on others. Regenerated dapi-grpc bindings: 8 files (web / nodejs / objc / python) pick up the new `null_value` field. Tests: 54 abci document_query + 48 drive count tests passing. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
|
✅ 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: "016562bd016903014e15cbdab40317ee84c99e5deaebc96fefc80b0ac1ee5df8"
)Xcode manual integration:
|
Comments should read as a static description of the contract, not as a snapshot in time. "Phase 1 supported shapes" / "Phase 2 reserved" wording goes stale the moment a future phase lands — replace with "Currently supported shapes" / "reserved for a future server capability" / "until then only the empty/non-empty discriminant is checked" so the comments stay accurate when more HAVING / GROUP BY shapes get wired up later. Scope: only files this PR introduced / touched: - `platform.proto` GetDocumentsRequestV1 docstring - `rs-drive-abci/src/query/document_query/v1/mod.rs` module + function docstrings - `rs-drive-abci/src/query/document_query/v1/tests.rs` reject_having test comment Regenerated ObjC bindings pick up the proto docstring changes. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…P/BOTTOM
HAVING in SQL evaluates against per-group aggregates, not row
values, so the previous `repeated WhereClause having` shape was
wrong on the wire: it described the operand as a field value when
it should describe it as an aggregate function applied to a field.
Replace it with a proper aggregate-filter type tree:
- `HavingAggregate { function, field }` — left operand. Functions:
- COUNT (with empty field → COUNT(*); non-empty → COUNT(field))
- SUM(field), AVG(field), MIN(field), MAX(field)
- TOP(field) / BOTTOM(field) — N-th-element aggregates whose `N`
argument lives in the clause's right-hand `value` slot
- `HavingClause { aggregate, operator, value }` — full clause.
Operators are scalar-comparison only (`=`, `!=`, `>`, `>=`,
`<`, `<=`); `IN`/`BETWEEN`/`STARTS_WITH` from `WhereOperator`
have no natural meaning against a single aggregate result.
- Multiple `HavingClause` entries on `GetDocumentsRequestV1.having`
combine with implicit AND (same shape contract as
`where_clauses`). A future `OR` capability would land as a
separate logical-group message rather than overloading this
type.
Rust types live in `drive::query::having` (new module) so the SDK
and rs-drive-abci share the canonical structures. Conversions:
- `rs-drive-abci/src/query/document_query/v1/conversions.rs` gains
`having_clause_from_proto` / `having_aggregate_from_proto` /
`having_function_from_proto` / `having_operator_from_proto`.
Server still rejects non-empty HAVING with the existing
`not_yet_implemented` path; the conversion runs first so
wire-malformed HAVING surfaces as `InvalidArgument` rather than
being masked by the wholesale "not yet implemented" rejection.
- `rs-sdk/src/platform/documents/document_query.rs::TryFrom`
emits typed `HavingClause` proto messages via
`having_clause_to_proto`. `DocumentQuery.having` switches from
`Vec<WhereClause>` to `Vec<HavingClause>`; `with_having`
signature follows.
Tests + regenerated dapi-grpc bindings (web / nodejs / objc /
python) updated for the new wire shape. 54 abci + 48 drive count
tests passing; full workspace compiles.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…OM N onto the aggregate
Mirror the full WhereOperator set on HavingOperator (minus
STARTS_WITH — prefix matching has no meaning against a scalar
aggregate, even a string-typed one):
- IN
- BETWEEN
- BETWEEN_EXCLUDE_BOUNDS
- BETWEEN_EXCLUDE_LEFT
- BETWEEN_EXCLUDE_RIGHT
`BETWEEN*` operand semantics match `WhereOperator`: a 2-element
`DocumentFieldValue.list` carrying `[lower, upper]`; `IN` expects
a list of candidate values.
Move the TOP/BOTTOM N-th-rank argument off `HavingClause.value`
onto a new `optional uint64 n` field on `HavingAggregate`. The
previous design put `N` in the value slot, which left no room for
the comparison target — that worked for the scalar-only operator
set but breaks the moment you write `HAVING TOP(score, 3) BETWEEN
80 AND 100`. With `n` on the aggregate, `value` always carries the
comparison target and every operator works with every function.
Updates:
- Proto: `HavingAggregate.n` (optional uint64, jstype = JS_STRING
for browser bigint safety); operator enum extended through 10.
- Drive `having.rs`: `HavingAggregate.n: Option<u64>`,
`HavingOperator::{In, Between, BetweenExcludeBounds,
BetweenExcludeLeft, BetweenExcludeRight}`.
- Conversions both sides updated; `having_clause_from_proto`
reworded — `value` is now always the comparison target.
- Test `hc()` helper defaults `n` to None; docstring notes
that TOP/BOTTOM tests should build the proto inline.
Regenerated dapi-grpc bindings (web / nodejs / objc / python)
include the new variants. 54 abci + 48 drive count tests passing;
full workspace compiles.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/rs-drive/src/query/mod.rs (1)
801-815: 💤 Low valueInconsistent error variant for order_by failures.
The existing
from_btree_map_valuemethod (line 728) usesQuerySyntaxError::InvalidOrderByPropertiesfor invalid order_by array shapes, but this refactored code usesInvalidFormatWhereClause. Using consistent error types helps API consumers handle errors predictably.♻️ Proposed fix for consistent error types
- Error::Query(QuerySyntaxError::InvalidFormatWhereClause(format!( - "invalid order_by clause components: {e}" + Error::Query(QuerySyntaxError::InvalidOrderByProperties( + "invalid order_by clause components" ))) ... - _ => Err(Error::Query(QuerySyntaxError::InvalidFormatWhereClause( - "order_by clause must be an array".to_string(), + _ => Err(Error::Query(QuerySyntaxError::InvalidOrderByProperties( + "order_by clause must be an array", ))), ... - Some(_) => { - return Err(Error::Query(QuerySyntaxError::InvalidFormatWhereClause( - "order_by must be an array".to_string(), - ))); + Some(_) => { + return Err(Error::Query(QuerySyntaxError::InvalidOrderByProperties( + "order_by must be an array", + ))); }🤖 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 801 - 815, The refactor in from_btree_map_value introduced inconsistent error variants for order_by failures: locate the order_by handling in the from_btree_map_value function and replace uses of QuerySyntaxError::InvalidFormatWhereClause (e.g., the "order_by clause must be an array" and "order_by must be an array" error branches and the mapping of invalid order_by components) with QuerySyntaxError::InvalidOrderByProperties so the method consistently returns InvalidOrderByProperties for invalid order_by shapes and components.
🤖 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.
Nitpick comments:
In `@packages/rs-drive/src/query/mod.rs`:
- Around line 801-815: The refactor in from_btree_map_value introduced
inconsistent error variants for order_by failures: locate the order_by handling
in the from_btree_map_value function and replace uses of
QuerySyntaxError::InvalidFormatWhereClause (e.g., the "order_by clause must be
an array" and "order_by must be an array" error branches and the mapping of
invalid order_by components) with QuerySyntaxError::InvalidOrderByProperties so
the method consistently returns InvalidOrderByProperties for invalid order_by
shapes and components.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1d13eb79-2cb2-4038-97f4-03a4a9f8d48d
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (22)
packages/dapi-grpc/clients/drive/v0/nodejs/drive_pbjs.jspackages/dapi-grpc/clients/platform/v0/java/org/dash/platform/dapi/v0/PlatformGrpc.javapackages/dapi-grpc/clients/platform/v0/nodejs/platform_pbjs.jspackages/dapi-grpc/clients/platform/v0/nodejs/platform_protoc.jspackages/dapi-grpc/clients/platform/v0/objective-c/Platform.pbobjc.hpackages/dapi-grpc/clients/platform/v0/objective-c/Platform.pbobjc.mpackages/dapi-grpc/clients/platform/v0/objective-c/Platform.pbrpc.hpackages/dapi-grpc/clients/platform/v0/objective-c/Platform.pbrpc.mpackages/dapi-grpc/clients/platform/v0/python/platform_pb2.pypackages/dapi-grpc/clients/platform/v0/python/platform_pb2_grpc.pypackages/dapi-grpc/clients/platform/v0/web/platform_pb.d.tspackages/dapi-grpc/clients/platform/v0/web/platform_pb.jspackages/dapi-grpc/protos/platform/v0/platform.protopackages/rs-drive-abci/src/query/document_query/v0/mod.rspackages/rs-drive-abci/src/query/document_query/v1/conversions.rspackages/rs-drive-abci/src/query/document_query/v1/mod.rspackages/rs-drive-abci/src/query/document_query/v1/tests.rspackages/rs-drive/src/query/drive_document_count_query/drive_dispatcher.rspackages/rs-drive/src/query/having.rspackages/rs-drive/src/query/mod.rspackages/rs-sdk/Cargo.tomlpackages/rs-sdk/src/platform/documents/document_query.rs
💤 Files with no reviewable changes (4)
- packages/rs-sdk/Cargo.toml
- packages/dapi-grpc/clients/platform/v0/objective-c/Platform.pbrpc.m
- packages/dapi-grpc/clients/platform/v0/java/org/dash/platform/dapi/v0/PlatformGrpc.java
- packages/dapi-grpc/clients/platform/v0/objective-c/Platform.pbrpc.h
✅ Files skipped from review due to trivial changes (2)
- packages/dapi-grpc/clients/platform/v0/python/platform_pb2_grpc.py
- packages/dapi-grpc/clients/platform/v0/objective-c/Platform.pbobjc.h
🚧 Files skipped from review as they are similar to previous changes (7)
- packages/rs-drive/src/query/drive_document_count_query/drive_dispatcher.rs
- packages/rs-sdk/src/platform/documents/document_query.rs
- packages/dapi-grpc/clients/platform/v0/web/platform_pb.js
- packages/dapi-grpc/clients/platform/v0/nodejs/platform_protoc.js
- packages/rs-drive-abci/src/query/document_query/v0/mod.rs
- packages/rs-drive-abci/src/query/document_query/v1/tests.rs
- packages/rs-drive-abci/src/query/document_query/v1/mod.rs
Match the error variant the existing `from_btree_map_value`
already uses for invalid order_by shapes. Two call sites:
- `rs-drive/src/query/mod.rs::from_decomposed_values` —
CBOR-shaped legacy entry point.
- `rs-drive-abci/src/query/document_query/v0/mod.rs` — new
CBOR-decode helper introduced earlier in this PR.
Both were emitting `QuerySyntaxError::InvalidFormatWhereClause`
for order_by problems, which is misleading on two counts: it
points consumers at the wrong field name in the error log, and
it diverges from the variant `from_btree_map_value` returns for
structurally identical failures.
`InvalidOrderByProperties` takes `&'static str`, so the
per-component error detail is dropped — the variant itself plus
the constant message ("invalid order_by clause components" /
"order_by clause must be an array" / "order_by must be an array")
gives consumers enough to localize the failure.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
`MIN` / `MAX` / `TOP` / `BOTTOM` aren't per-group aggregate
functions — they're meta-aggregates computed *across* the set of
group-aggregate results, used to express "this group's count is
the largest" (`COUNT(*) EQ MAX`) or "this group's count is in the
top 5" (`COUNT(*) IN TOP(5)`). Putting them on the left side
conflated two different semantic layers.
Resolve by splitting the type:
- `HavingAggregate.Function` keeps only the per-group aggregates:
`COUNT`, `SUM`, `AVG`. The `n` field is gone — it only existed
to thread the TOP/BOTTOM N-th-rank argument through, and those
functions no longer live here.
- New `HavingRanking { kind: { MIN | MAX | TOP | BOTTOM }, n }`
carries the cross-group reference.
- `HavingClause.right` becomes a `oneof { DocumentFieldValue
value, HavingRanking ranking }` so a clause carries exactly
one of "literal value" or "ranking" on the right side.
The wire boundary now rejects half-built clauses (operator says
`IN`, right side has both `value` and `ranking` set, etc.) at
proto-decode time rather than letting them reach the evaluator.
Operator compatibility (server validates at evaluation time, the
wire is permissive):
- Scalar operators + ranking: works for `MIN` / `MAX` /
`TOP(1)` / `BOTTOM(1)` (single-value rankings).
- `IN` + ranking: works for `TOP(N)` / `BOTTOM(N)` (set
membership).
- `BETWEEN*` + ranking: rejected at evaluation (no meaningful
composition).
Drive Rust types in `drive::query::having`:
- `HavingAggregateFunction` trimmed to `Count` / `Sum` / `Avg`.
- `HavingAggregate.n` removed.
- New `HavingRankingKind` / `HavingRanking` / `HavingRightOperand`
types; `HavingClause.value: Value` → `HavingClause.right:
HavingRightOperand`.
Both conversion directions (`rs-drive-abci`'s
`having_clause_from_proto`, `rs-sdk`'s `having_clause_to_proto`)
updated to thread the new oneof. Test `hc()` helper still builds
literal-value clauses; ranking-right tests should construct the
proto inline with `having_clause::Right::Ranking`.
Regenerated dapi-grpc bindings (web / nodejs / objc / python)
pick up the trimmed `Function` enum, new `HavingRanking` message,
and `oneof right`. 54 abci + 48 drive count tests passing; full
workspace compiles.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
`SELECT SUM(amount)` and `SELECT AVG(score)` are real projection
modes the v1 surface should be able to carry. Adding them to the
existing `enum Select` was insufficient on its own because the
function-only shape has no slot for the field name to aggregate
on — `SELECT SUM` is meaningless without naming the field.
Restructure `Select` from an enum into a message with the same
`(function, field)` shape `HavingAggregate` uses:
```
message Select {
enum Function { DOCUMENTS = 0; COUNT = 1; SUM = 2; AVG = 3; }
Function function = 1;
string field = 2;
}
```
Per-function `field` requirements:
- `DOCUMENTS`: must be empty (rejected with `InvalidArgument` if
caller sets it).
- `COUNT`: empty → `COUNT(*)` (current behavior); non-empty →
`COUNT(field)` (currently rejected with `not_yet_implemented`).
- `SUM` / `AVG`: required (numeric field); currently rejected
with `not_yet_implemented` until the server gains numeric
aggregate evaluation.
The wire-format change from `enum` to `message` (varint → LEN
encoding) breaks the field's wire compat, but v1 is pre-release
so no deployed callers exist. Future projection types can land
without another field-number change because the `(function,
field)` shape is extensible.
Drive Rust types live in new module `drive::query::projection`:
- `SelectFunction { Documents, Count, Sum, Avg }`
- `SelectProjection { function, field }` with convenience
constructors (`documents()`, `count_star()`, `count_field()`,
`sum()`, `avg()`)
Conversions both sides: `rs-drive-abci`'s `select_from_proto` /
`rs-sdk`'s `select_to_proto`. `validate_and_route` now takes a
`&SelectProjection` (decoded once at the handler entry) and
rejects the unimplemented combinations with the existing
`not_yet_implemented` machinery — same contract as `HAVING`.
Downstream callers updated:
- `rs-sdk` callers in dpns / dashpay / count helpers switch from
the proto `Select::Documents` / `Select::Count` enum to
`SelectProjection::documents()` / `count_star()`.
- `rs-sdk-ffi`, `wasm-sdk`, `rs-platform-wallet` likewise.
- ABCI tests: 22 sites that built the request with `select:
V1Select::Documents as i32` switch to `select:
select_documents()` / `select_count_star()` helpers; the
garbage-discriminant test builds the proto message inline with
`function: 42`.
Regenerated dapi-grpc bindings (web / nodejs / objc / python).
54 abci + 48 drive count tests passing; full workspace compiles.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Clippy's `needless_return` lint flags `return Err(...)` as the final expression of a match arm. Drop the `return` and the trailing semicolon so the match arms expression-out directly, matching the surrounding arm style. Caught by macOS CI clippy gate after c275ea8. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
`rs-sdk/tests/fetch/document_count.rs` was still using the removed `Select::Count` proto enum. Switch to `SelectProjection::count_star()` matching the rest of the SDK callers; update the docstring at the top of the file to point at the new builder. `cargo check -p dash-sdk` only builds the library — the integration-test compile lives behind `--tests` and slipped past the local pre-push smoke check. Caught by macOS CI. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
thepastaclaw
suggested changes
May 16, 2026
Collaborator
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Two convergent findings from Claude and Codex hold up under verification: a clause-order-dependent routing bug in validate_and_route that rejects valid two-range GROUP BY counts (real correctness regression — drive's executor explicitly supports this shape per outer_range_plus_inner_range_with_prove_and_group_by_range_routes_to_carrier_proof), and a v0 wire-tightening on order_by parse failures that's intentional per its dedicated commit but worth surfacing in the PR description because v0 is the production-deployed surface. Two style-level findings from Claude (HAVING decoded then discarded, duplicate error arms) are valid but low-impact. No false positives.
Reviewed commit: ef216a6
🔴 1 blocking | 🟡 1 suggestion(s) | 💬 2 nitpick(s)
1 additional finding
🔴 blocking: GROUP BY routing depends on where-clause order when two range clauses are present
packages/rs-drive-abci/src/query/document_query/v1/mod.rs (lines 152-229)
validate_and_route derives range_field via where_clauses.iter().find(...), capturing only the first range clause. This breaks the valid GroupByRange + prove + two-range shape that drive's executor already supports — see outer_range_plus_inner_range_with_prove_and_group_by_range_routes_to_carrier_proof at packages/rs-drive/src/query/drive_document_count_query/tests.rs:2473, which asserts detect_mode(vec![gt_clause("brand"), gt_clause("color")], GroupByRange, true) == RangeAggregateCarrierProof.
With the current router, WHERE brand > X AND color > Y GROUP BY brand routes to GroupByRange (range_field is the first match — "brand"), but the semantically identical WHERE color > Y AND brand > X GROUP BY brand is rejected with "GROUP BY on field 'brand' which is not constrained by an In or range where clause" because range_field is now "color". Request semantics must not depend on clause ordering on the wire.
The routing should check whether any range clause matches group_by[0] (and likewise for In), e.g. by replacing find(...).map(|wc| wc.field.as_str()) == Some(field) with any(|wc| matches!(...) && wc.field == *field) for the GROUP BY routing decision. The same concern applies to the In lookup but is currently masked because downstream validate_and_canonicalize_where_clauses rejects multiple-In via MultipleInClauses before the routing mismatch surfaces — so the user gets a confusing error rather than the right one.
🤖 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-abci/src/query/document_query/v1/mod.rs`:
- [BLOCKING] lines 152-229: GROUP BY routing depends on where-clause order when two range clauses are present
`validate_and_route` derives `range_field` via `where_clauses.iter().find(...)`, capturing only the first range clause. This breaks the valid `GroupByRange + prove + two-range` shape that drive's executor already supports — see `outer_range_plus_inner_range_with_prove_and_group_by_range_routes_to_carrier_proof` at `packages/rs-drive/src/query/drive_document_count_query/tests.rs:2473`, which asserts `detect_mode(vec![gt_clause("brand"), gt_clause("color")], GroupByRange, true) == RangeAggregateCarrierProof`.
With the current router, `WHERE brand > X AND color > Y GROUP BY brand` routes to `GroupByRange` (range_field is the first match — "brand"), but the semantically identical `WHERE color > Y AND brand > X GROUP BY brand` is rejected with `"GROUP BY on field 'brand' which is not constrained by an `In` or range where clause"` because range_field is now "color". Request semantics must not depend on clause ordering on the wire.
The routing should check whether *any* range clause matches `group_by[0]` (and likewise for `In`), e.g. by replacing `find(...).map(|wc| wc.field.as_str()) == Some(field)` with `any(|wc| matches!(...) && wc.field == *field)` for the GROUP BY routing decision. The same concern applies to the `In` lookup but is currently masked because downstream `validate_and_canonicalize_where_clauses` rejects multiple-In via `MultipleInClauses` before the routing mismatch surfaces — so the user gets a confusing error rather than the right one.
In `packages/rs-drive-abci/src/query/document_query/v0/mod.rs`:
- [SUGGESTION] lines 96-122: v0 order_by parse path is now strict where it used to silently drop malformed clauses
The pre-PR v0 path piped `order_by` through `DriveDocumentQuery::from_decomposed_values`, which used `filter_map(...ok())` for inner clauses and treated any non-array top-level value as an empty `order_by` (default index-natural ordering). The new v0 handler parses `order_by` eagerly and returns `InvalidOrderByProperties` for the same inputs.
Commit 27371f91ed ("refactor: use InvalidOrderByProperties for order_by parse failures") makes this change deliberately — failing loudly is arguably better than silently ignoring malformed input — but it is an observable wire-contract change on the v0 endpoint, which is the production-deployed `getDocuments` surface. A v0 caller that was previously sending a malformed `order_by` payload (e.g. a CBOR value that decoded to `Value::Text`, or an inner clause whose direction component didn't parse) used to receive documents in the natural order and will now receive `InvalidOrderByProperties`. Not consensus-breaking (read-only path, deterministic), and the intent is reasonable, but please call this out in the PR description / changelog so v0 consumers can spot it before it manifests as test or integration breakage.
…_proto
Two code-review-driven cleanups:
- v1 handler: don't decode HavingClauses just to discard them.
The server rejects non-empty HAVING wholesale today, and the
downstream dispatchers don't accept the decoded vec yet, so
the decode was pure overhead. Swap to an `!having.is_empty()`
short-circuit before decoding and document that wire-malformed
HAVING will start surfacing as `InvalidArgument` (instead of
the current `not_yet_implemented`) the moment execution lands
and the decode comes back. The `having_*_from_proto` helpers
in `conversions.rs` stay in tree, marked `#[allow(dead_code)]`
per function so any future addition outside the family still
trips the lint.
- SDK `value_to_proto`: collapse the duplicate `Value::Map | Enum*`
arm and the trailing `_` catch-all. Both produced the
identical error (same `{value:?}` formatting), so the explicit
arm added no precision and didn't make the match exhaustive
(`Value` is `#[non_exhaustive]` either way). Single `_` arm
now carries the explanation for both the listed variants and
any future upstream additions.
PR description also amended (separately, via `gh pr edit`) to
call out the v0 `order_by` strictness change from 27371f9 —
the read-only contract tightening is intentional but observable.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
`validate_and_route` was deriving `range_field` (and `in_field`) from a single `where_clauses.iter().find(...)` — capturing only the first matching clause. That broke the valid two-range GROUP BY shape drive's executor explicitly supports (`RangeAggregateCarrierProof`, see `outer_range_plus_inner_range_with_prove_and_group_by_range_routes_to_carrier_proof` in `rs-drive`): `WHERE brand > X AND color > Y GROUP BY brand` routed correctly, but the semantically identical `WHERE color > Y AND brand > X GROUP BY brand` was rejected with "GROUP BY on field 'brand' which is not constrained by an `In` or range where clause". Replace the single-`find` lookups with `is_in_field(field)` / `is_range_field(field)` membership predicates (`any(... && wc.field == field)`) so the routing decision sees every where clause, not just the first match. The `In` lookup was technically masked today because `validate_and_canonicalize_where_clauses` rejects multiple `In` clauses upstream — but using the `any` shape there too keeps the routing logic from baking in an assumption that could go stale if that validator's contract ever relaxes. Regression test: `group_by_routing_is_independent_of_two_range_clause_order` runs the same request with both clause orderings and pins the routing to `count_entries_via_range_field` regardless. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Member
Author
|
@thepastaclaw addressed the blocking finding in 693b546:
The suggestion (v0 |
thepastaclaw
approved these changes
May 16, 2026
Collaborator
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Rechecked the current head after the follow-up commits. The order_by error variant nit, HAVING short-circuit, duplicate value_to_proto arm, v0 contract note, and GROUP BY routing blocker are addressed; the new routing uses match-any predicates and has regression coverage for two-range clause ordering. Checks are green on the current head.
…urface
Extend the v1 `getDocuments` SQL-shaped wire with four items that
round out the SQL surface. Three are wire-only (proto + drive
types + conversions land; routing rejects with the standard
`not_yet_implemented` contract); one (MIN/MAX) is hooked up
through the routing gate and stays in the same rejection family
as SUM/AVG because drive doesn't yet have a numeric-aggregate
executor.
A tracker issue collects every `not_yet_implemented` site this
PR introduces (HAVING, SUM/AVG, MIN/MAX, COUNT(field), OFFSET,
multi-projection SELECT, ORDER BY aggregate, GROUP BY shape
gaps, …) so each one can land as a focused follow-up.
**(1) MIN/MAX in `Select.Function`** — per-group / global
minimum / maximum. Distinct from `HavingRanking::Min` / `Max`
(which are cross-group meta-aggregates). New `SelectFunction`
variants, conversion arms both sides, SDK constructors
(`SelectProjection::min(field)` / `max(field)`). Routing
rejects with the same not-yet-implemented contract as SUM/AVG.
**(3) `offset: optional uint32 = 12;` on `GetDocumentsRequestV1`** —
row-based pagination layered on top of cursor pagination. The
handler rejects non-`None` offset before any other routing.
**(4) `repeated Select selects = 9;` (was single `Select`)** —
multi-projection `SELECT COUNT(*), SUM(amount), AVG(rating)`.
`selects.len() > 1` is rejected at routing; empty defaults to
`SelectProjection::documents()` for v0-style fallback. SDK's
single-projection path wraps in `vec![s]` on the way out. When
multi-projection lands the response shape will need a parallel
`repeated AggregateValue values` field.
**(7) `OrderClause.target` `oneof { field, aggregate }`** —
`ORDER BY COUNT(*)` / `ORDER BY SUM(amount)`. Drive's
`OrderClause` keeps its `field: String` shape today; the
proto→drive converter rejects the aggregate variant at decode
time. Overlaps `HavingRanking::Top` / `Bottom` (consolidation
deferred).
Tests + regenerated dapi-grpc bindings (web / nodejs / objc /
python). 59 abci + 48 drive count tests passing; full workspace
compiles.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Member
Author
|
Created tracker issue #3655 for every |
Member
Author
|
Heads up: my local Docker daemon hung mid-build and I wasn't able to regenerate the non-Rust dapi-grpc bindings (Java / ObjC / Python / Web) for the new MIN/MAX / OFFSET / repeated-Select / ORDER BY-aggregate proto changes in d5f5b3a. The Rust bindings are fine (regenerated at Will regenerate as a follow-up commit once Docker is back; whoever picks this up can also run |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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-abci/src/query/document_query/v1/conversions.rs`:
- Around line 97-102: The conversion for document_field_value::Variant::List in
value_from_proto currently recurses into list.values without guarding against
nested List variants, enabling unbounded recursion; update value_from_proto (or
the helper that handles document_field_value::Variant::List) to reject/return an
error if any element in list.values is itself a
DocumentFieldValue::Variant::List (or enforce a maximum depth parameter and
decrement it on each recursive call), so the method refuses nested List variants
before recursing and prevents infinite/deep recursion.
In `@packages/rs-sdk/src/platform/documents/count_proof_helpers.rs`:
- Around line 41-46: The check in the DocumentCount/DocumentSplitCounts guard
currently allows COUNT(field) but verify_count_query() rebuilds the
DriveDocumentCountQuery without the selected field, causing COUNT(field) to be
treated as COUNT(*) for nullable fields; update the condition around
request.select (in count_proof_helpers.rs) to only accept COUNT when the
selected projection is a star/no-field (i.e., ensure request.select.function ==
SelectFunction::Count AND the select has no specific field), and otherwise
return the existing RequestError guidance; mention the related symbols
request.select, SelectFunction::Count, verify_count_query(), and the
DocumentCount/DocumentSplitCounts check so the change is applied where the guard
is implemented.
In `@packages/rs-sdk/src/platform/documents/document_query.rs`:
- Around line 383-394: The conversion helpers where_clause_to_proto and
having_clause_to_proto are now fallible but execute_transport() still calls
self.try_into().expect("DocumentQuery should always be valid"), which will panic
on bad input; change execute_transport() to propagate/convert the try_into()
error into a proper Result/Transport error instead of unwrapping (or
alternatively perform validation earlier when building the DocumentQuery so
try_into() cannot fail). Specifically, replace the expect(...) call in
execute_transport() with code that returns an Err mapped from the TryInto error
(or validate and filter unsupported WhereClause/HavingClause variants before
calling where_clause_to_proto/having_clause_to_proto) and apply the same change
for the other occurrence referenced around the 726-769 range so invalid queries
produce request errors rather than aborting the process.
🪄 Autofix (Beta)
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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 702d8181-1e6c-41db-93bd-23f95fb620ea
📒 Files selected for processing (26)
packages/dapi-grpc/clients/drive/v0/nodejs/drive_pbjs.jspackages/dapi-grpc/clients/platform/v0/nodejs/platform_pbjs.jspackages/dapi-grpc/clients/platform/v0/nodejs/platform_protoc.jspackages/dapi-grpc/clients/platform/v0/objective-c/Platform.pbobjc.hpackages/dapi-grpc/clients/platform/v0/objective-c/Platform.pbobjc.mpackages/dapi-grpc/clients/platform/v0/python/platform_pb2.pypackages/dapi-grpc/clients/platform/v0/web/platform_pb.d.tspackages/dapi-grpc/clients/platform/v0/web/platform_pb.jspackages/dapi-grpc/protos/platform/v0/platform.protopackages/rs-drive-abci/src/query/document_query/v0/mod.rspackages/rs-drive-abci/src/query/document_query/v1/conversions.rspackages/rs-drive-abci/src/query/document_query/v1/mod.rspackages/rs-drive-abci/src/query/document_query/v1/tests.rspackages/rs-drive/src/query/having.rspackages/rs-drive/src/query/mod.rspackages/rs-drive/src/query/projection.rspackages/rs-platform-wallet/src/wallet/identity/network/profile.rspackages/rs-sdk-ffi/src/document/queries/count.rspackages/rs-sdk/src/platform/dashpay/contact_request_queries.rspackages/rs-sdk/src/platform/documents/count_proof_helpers.rspackages/rs-sdk/src/platform/documents/document_query.rspackages/rs-sdk/src/platform/dpns_usernames/mod.rspackages/rs-sdk/src/platform/dpns_usernames/queries.rspackages/rs-sdk/tests/fetch/document_count.rspackages/wasm-sdk/src/dpns.rspackages/wasm-sdk/src/queries/document.rs
✅ Files skipped from review due to trivial changes (1)
- packages/dapi-grpc/clients/platform/v0/objective-c/Platform.pbobjc.h
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/rs-drive-abci/src/query/document_query/v0/mod.rs
- packages/dapi-grpc/clients/platform/v0/web/platform_pb.d.ts
- packages/rs-drive-abci/src/query/document_query/v1/tests.rs
Three valid findings from the round-3 review:
- **conversions.rs `value_from_proto`**: `Variant::List` recursed
unconditionally — `list(list(list(...)))` could blow the call
stack before schema validation. Add a depth cap: the query
surface only needs one level of nesting (`IN` / `BETWEEN*`
candidate lists of scalars), and a `list` encountered at
`depth >= 1` is rejected as malformed wire input.
- **count_proof_helpers.rs `assert_select_is_count`**: guard
accepted `COUNT(field)` but `verify_count_query()` rebuilds a
`DriveDocumentCountQuery` without the selected field, so
`COUNT(field)` against a nullable field would silently verify
as `COUNT(*)` (different total). Tighten the SDK-side guard to
require `function == Count && field.is_empty()` — matches the
not-yet-implemented gate the server already enforces for the
`COUNT(field)` case.
- **document_query.rs `execute_transport`**: `TryFrom<DocumentQuery>
for GetDocumentsRequest` became fallible when the SDK started
rejecting unmappable `Value` variants (`Map`, future
`#[non_exhaustive]` additions, …). The transport stub was still
`.expect("DocumentQuery should always be valid")` — bad input
panicked the process. Propagate the conversion failure as a
`TransportError::Grpc(Status::invalid_argument(...))` so the
SDK surfaces a normal request error.
Tests: 59 abci document_query + 48 drive count, workspace
compiles.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Issue being fixed or feature implemented
The v1
getDocumentsrequest (GetDocumentsRequestV1) was shipping itswhere/order_by/havingoperands as CBOR-encodedbytes— the same shape v0 uses. That meant:DriveDocumentQuery::from_decomposed_values), so the bytes shape was paying for itself twice — once on the SDK encode side and once on the server decode side — without buying any layout independence the typed shape doesn't already provide.This PR moves the v1 wire to typed
repeated WhereClause/repeated OrderClause/repeated WhereClause(HAVING) messages while leaving the v0 endpoint's CBOR shape untouched.What was done?
Proto / bindings (
packages/dapi-grpc/)GetDocumentsRequest:WhereOperatorenum (11 variants —EQUALthroughSTARTS_WITH),DocumentFieldValue(oneof: bool / int64 / uint64 / double / text / bytes / list),WhereClause(field, operator, value),OrderClause(field, ascending).GetDocumentsRequestV1:bytes where→repeated WhereClause where_clauses,bytes order_by→repeated OrderClause order_by,bytes having→repeated WhereClause having.build.rs: serdebytesshim narrowed toGetDocumentsRequestV0.where/GetDocumentsRequestV0.order_byonly.Drive (
packages/rs-drive/)DriveDocumentQuery::from_typed_clauses— new typed twin offrom_decomposed_values(limit semantics identical).DocumentCountRequestnow carrieswhere_clauses: Vec<WhereClause>/order_clauses: Vec<OrderClause>directly (noValue::Arrayre-parse).validate_and_canonicalize_where_clausesso the CBOR-shaped legacy entry (where_clauses_from_value) and the typed entry share validation. Bothwhere_clauses_from_valueandorder_clauses_from_valueare nowpubso benches/tests can keep their compactValue::Arrayfixture vocabulary.insert_contracttest + 1 bench updated to construct typedWhereClauses.ABCI (
packages/rs-drive-abci/)v1/conversions.rsmodule — schema-agnostic proto → drive conversion (where_clause_from_proto,order_clause_from_proto,value_from_proto). Schema-driven type coercion still runs downstream indocument_type.serialize_value_for_key.query_documents_v0split into CBOR-decode plus a sharedquery_documents_typedhelper.query_documents_typeddirectly (no CBOR round-trip). Count path constructsDocumentCountRequestfrom the typed conversions.validate_and_routere-shaped to take field-by-field args so it works against the new typed envelope; HAVING still rejects unconditionally with the existing "not yet implemented"Unsupportedmessage.SDK (
packages/rs-sdk/)TryFrom<DocumentQuery> for GetDocumentsRequestnow builds typed proto messages viawhere_clause_to_proto/order_clause_to_proto/value_to_proto. Schema-agnostic primitive mapping (Bool→BoolValue,Identifier/Bytes32→BytesValue,U128/I128→Text(decimal),Array→List, etc.); unsupported variants (Null,Map,EnumU8,EnumString) surface asEncodingErrorat the wire boundary.DocumentQuery.havingis nowVec<WhereClause>(wasVec<u8>).serialize_vec_to_cborhelper deleted.How Has This Been Tested?
All test suites compile and pass on the worktree:
cargo test -p drive-abci --lib query::document_query::v1) — the 28 ported-v0-count tests + the v1-routing + e2e tests, all rewritten withwc()/oc()/pv()helpers to construct the new typed proto messages.cargo test -p drive-abci --lib query::document_query::v0) — unchanged behavior, v0 wire still CBOR.cargo test -p drive --lib drive_document_count_query) — bench + tests construct typedWhereClauses directly.cargo check --workspacegreen.cargo fmt --allapplied.Breaking Changes
No consensus-breaking changes. The v1
getDocumentswire is changed shape, but v1 is pre-release (not yet on DAPI mainnet) so there are no deployed clients to migrate.Observable v0 contract tightening (read-only path, not consensus-breaking): the v0
getDocumentsorder_byparse used to silently drop malformed clauses viafilter_map(... .ok())and treat any non-array top-level CBOR value as "no ordering" (default index-natural order). This PR's v0 refactor (which lifts the parse fromDriveDocumentQuery::from_decomposed_valuesinto the abci layer to share with v1) propagates clause-parse errors asQuerySyntaxError::InvalidOrderByProperties. The same tightening applies to the legacyfrom_decomposed_valuespath. A v0 caller that was relying on the silent-drop behavior (e.g. anorder_byCBOR that decoded toValue::Text, or an inner clause whose direction component didn't parse) will now receive a typed error instead of partial/default ordering. Intentional — silently mutating result order on the prove path could change proof bytes without telling the caller — but called out here so v0 consumers can spot it.Checklist:
For repository code-owners and collaborators only
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes / Validation
Tests