test(test-suite): indexOnly document lifecycle functional spec - #4510
Conversation
The legacy DocumentFactory mapped every delete to a by-id delete transition, which the network rejects for indexOnly document types. Branch on the doctype's storage mode and build DocumentIndexOnlyDeleteTransition (delete-by-values) instead, skipping the revision gate since synthesized index-only documents carry none. The by-id path is unchanged for stored types.
… reads execute_raw_results_no_proof returned the raw grovedb elements for indexOnly types — 32-byte row commitments, not documents — so the non-proof GetDocuments endpoint served bytes no client could parse. Route indexOnly queries through the synthesis executor and serialize the synthesized documents. A subset-index projection lacks required properties the serialized document format cannot omit, so it is refused with guidance; projections still travel the proved read surface, where the client synthesizes them from the proof.
PastaPastaPasta
changed the title
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe PR adds index-only document lifecycle coverage. It creates index-only delete transitions without revisions, serializes synthesized documents from covering indexes, rejects incomplete projections, and tests creation, querying, deletion, duplicate prevention, and owner authorization. ChangesIndex-only document behavior
Estimated code review effort: 4 (Complex) | ~45 minutes Merge Risk: 🔵 Low · up to The PR adds index-only document deletion and changes client-visible query serialization; lifecycle and unauthorized-deletion tests cover the main paths, but signer-to-owner authorization binding and atomic recovery across deletion-related updates still need explicit owner awareness or follow-up. The PR is mergeable with that bounded security and reliability risk acknowledged. Sequence Diagram(s)sequenceDiagram
participant PlatformClient
participant DocumentFactoryV0
participant DocumentIndexOnlyDeleteTransition
participant Drive
PlatformClient->>DocumentFactoryV0: request deletion of synthesized indexOnly document
DocumentFactoryV0->>DocumentIndexOnlyDeleteTransition: build transition from document properties
DocumentIndexOnlyDeleteTransition-->>Drive: broadcast value-based deletion
Drive-->>PlatformClient: deletion result
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Full details: Title checkExplanation The title accurately identifies the added functional test coverage for the indexOnly document lifecycle. It does not mention the supporting DPP and Drive fixes, but the title need not cover every change. ✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
✅ Final review complete — no blockers (commit 15dcbcf) |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Codex/Sol only (Phase 2 disabled)
The delete-transition and covering-index serialization changes match the PR's goal, but raw no-proof validation remains result-dependent for non-covering indexes. Empty or fully offset result sets succeed even though the identical query is rejected once it returns a row, so this remains an in-scope query-correctness suggestion. Source: reviewer backends — Codex (exact backend model ID unavailable in the supplied evidence) and Claude (exact backend model ID unavailable in the supplied evidence); final verifier backend — grok-4.5; orchestration only — openclaw-agent/cliproxy/gpt-5.6-sol (not reviewer evidence).
Validated zero-blocker Codex/Sol precheck evidence was promoted to final because Phase 2 (Sonnet/Opus) is temporarily disabled. This is Codex/Sol-only final validation, not Codex + Sonnet/Opus coverage.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed),gpt-5.6-sol— rust-quality (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet/Opus: not run (Phase 2 disabled — temporary Codex/Sol-only final)
- Secondary pass: disabled (
temporary_phase2_sonnet_disable)
🟡 1 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`:
- [SUGGESTION] packages/rs-drive/src/query/mod.rs:2539-2557: Reject non-covering indexes before examining query results
Index coverage is currently validated only when serializing each synthesized document and converting `MissingRequiredKey` into `QuerySyntaxError::Unsupported`. If the storage query returns no documents—because there are no matches, the relevant path is absent, or the offset skips all matches—the iterator is empty and `collect()` succeeds. The same subset-index query therefore returns an empty successful response until a matching row becomes visible, at which point it starts failing. Validate the selected index's ability to reconstruct every required serializable field independently of the returned rows, and add an empty-result regression case so query validity remains state-independent.
| let serialized = documents | ||
| .into_iter() | ||
| .map(|document| { | ||
| document | ||
| .serialize(self.document_type, self.contract, platform_version) | ||
| .map_err(|error| match error { | ||
| ProtocolError::DataContractError( | ||
| dpp::data_contract::errors::DataContractError::MissingRequiredKey(_), | ||
| ) => Error::Query(QuerySyntaxError::Unsupported( | ||
| "this indexOnly query's index does not cover every required \ | ||
| property, so the documents it synthesizes cannot be \ | ||
| serialized into a non-proof response; query through an \ | ||
| index covering all properties, or use a proved query" | ||
| .to_string(), | ||
| )), | ||
| other => other.into(), | ||
| }) | ||
| }) | ||
| .collect::<Result<Vec<_>, Error>>()?; |
There was a problem hiding this comment.
🟡 Suggestion: Reject non-covering indexes before examining query results
Index coverage is currently validated only when serializing each synthesized document and converting MissingRequiredKey into QuerySyntaxError::Unsupported. If the storage query returns no documents—because there are no matches, the relevant path is absent, or the offset skips all matches—the iterator is empty and collect() succeeds. The same subset-index query therefore returns an empty successful response until a matching row becomes visible, at which point it starts failing. Validate the selected index's ability to reconstruct every required serializable field independently of the returned rows, and add an empty-result regression case so query validity remains state-independent.
source: ['codex']
3 participants
Issue being fixed or feature implemented
indexOnly document types (protocol v14) had full e2e coverage at the Rust layers (rs-drive
index_only_e2e_tests, rs-drive-abcibatch/tests/document/index_only), but no JS functional spec inplatform-test-suite— the layer that exercises the whole stack (js-dash-sdk → wasm-dpp → DAPI → drive-abci → drive) against a live network. Two gaps blocked writing one, and the spec surfaced a third:DocumentFactory(the construction path wasm-dpp / js-dash-sdk use) mapped every delete to a by-idDocumentDeleteTransition, which the structure gates reject for indexOnly types — deletion of an indexOnly document is its own transition kind,DocumentIndexOnlyDeleteTransition(delete-by-values).DriveDocumentQuery::execute_raw_results_no_proof— which the non-proofGetDocumentshandler calls directly — returned the raw grovedb elements for indexOnly types. Those are 32-byte row commitments, not documents, so clients blew up parsing them (Document::from_bytes: unknown version, received: 111).What was done?
DocumentFactoryV0::document_delete_transitionsnow picks the delete kind from the doctype's storage mode, mirroring the dispatch already in the batch-transition v0/v1 builders:index_only()→DocumentIndexOnlyDeleteTransition::from_document(no revision gate — synthesized index-only documents carry none), stored types → the by-id path, byte-identical to before. Unit test pins the dispatch, the carried data, and that$createdAtstays out when the doctype doesn't require it. No wasm-dpp changes were needed — its transition wrappers already handle the kind.execute_raw_results_no_proof_internalroutes indexOnly queries through the same synthesis executorDrive::query_documentsuses and serializes the synthesized documents into the wire bytes clients expect. A subset-index projection lacks required properties the serialized document format cannot omit (serialize_v3has no presence flag for required fields), so it is refused with guidance to query through a covering index — projections still travel the proved read surface, where the client synthesizes them from the proof itself. New rs-drive e2e test pins the covering-indexfrom_bytesround trip and the subset refusal.test/functional/platform/IndexOnlyDocument.spec.js, modeled onDocument.spec.js/contacts.spec.js. It registers an inline yappr-style contract (storedpost+ indexOnlylikewithrefersTo/propertyAgreement) and covers, in order:ReferencedEntityNotFoundError, 40120);propertyAgreement,ReferencedDocumentPropertyMismatchError, 40127);DuplicateUniqueIndexError, 40105 — any existing entry collides);[hashtag, postId]index returns documents synthesized from index positions, carrying the created values and owner, with a deterministic$idthat is not the create's id;postId ==, byPost) is refused on the non-proof wire with covering guidance;$idis rejected with guidance (no primary-key tree);documents.broadcast({ delete: [...] })succeeds and the entry disappears from queries;DocumentNotFoundError(40101).Every broadcast in the suite also verifies its executed-transition proof via the EvoSDK-backed proof verifier, which for indexOnly batches takes the
ExecutionNotProved→waitForAffectedStatepath — so the spec exercises the indexOnly executed-proof surface (outcome alwaysAffectedState, neverExecutionProved) on each create and delete. The skipped/brokenwaitForStateTransitionResult.spec.jswas deliberately not extended (it isdescribe.skip'd and stale).How Has This Been Tested?
IndexOnlyDocument.spec.js: 10/10 passing against a local dashmate network (yarn setup+yarn start) built from this branch.Document.spec.js: 8/8 passing on the same network (stored-type deletes/queries unaffected).cargo test -p dpp(4018 passed) andcargo test -p drive --lib(3478 passed, includes the 33 indexOnly e2e tests and the new raw-reads test).cargo clippy --all-targets -- -D warningsclean fordppanddrive.yarn lintclean inplatform-test-suite.Breaking Changes
None. The by-id delete path is byte-identical for stored types; the raw no-proof read path previously returned unparseable bytes for indexOnly types (nothing could have depended on it).
Checklist:
For repository code-owners and collaborators only
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes