Repository navigation
feat(platform)!: skipIfAbsent may skip on a derived index property (PV14) - #5318
Conversation
…V14)
An index property read through a reference ("quoteId.$ownerId") can now
be a skip property, so an index leaves out a document whose reference
or referenced field is absent instead of filing it under null.
- Only the skipIfAbsent array names one; `true` keeps meaning the
type's own optional properties, since whether a derived value can be
absent depends on the referenced type.
- Registration refuses a derived skip property that is never absent (a
required reference reading $ownerId, $creatorId or a field required
with every object around it) and a byte array that may be empty.
- Drive's walkers, update and the SDK cost walker count a null skip
value as absent, which a derived value is when its reference or
field is.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 16 minutes. View limit detailsLimit details: You’ve used the included review currently available. Review configuration: ⚙️ Run configuration
📒 Files selected for processing (1)
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
📒 Files selected for processing (12)
Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThis change allows derived index properties in array-form ChangesDerived Index Skip Behavior
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~25 minutes Change: Feature Sequence Diagram(s)sequenceDiagram
participant DocumentWrite
participant Context
participant Carries
participant DocumentCarries
participant IndexLevel
DocumentWrite->>Context: evaluate index participation and reachability
Context->>Carries: check skip-property presence
Carries->>DocumentCarries: check document value and reference property
Context->>IndexLevel: determine index participation and reachability
Suggested reviewers: Merge Risk: ⚪ Minimal · up to This change lets contracts skip indexing a document when a value read through a reference is absent. No blocking issue was identified, and the reported tests cover the new behavior. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 fb440ba) · triage: critical |
|
📖 Book Preview built successfully. Download the preview from the workflow artifacts. Updated at 2026-10-07T09:03:33.254Z |
|
Waiting for bot review — coderabbitai ✓ · thepastaclaw not yet. Wait for the missing reviews, or a writer can post |
github-actions
Bot
added
the
waiting-bots
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Phase 1 + Phase 2
Verified the supplied findings against head 4edc967. The two Claude findings identify the same valid, non-blocking coverage gap in the client cost estimator and are combined below; no blocking issue was confirmed. Validation was static only: diff whitespace checks passed, no builds or tests were run, and the supplied CI snapshot had no completed validation results.
🟡 1 suggestion(s)
Review provenance
Source: reviewer 1: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 2: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: platform-versioning); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 5: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); reviewer 6: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 7: muse-spark-1.3-contributor (agent: phase1-reviewer, role: rust-quality); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
criticalbygpt-6.1-sol(effort low) — The diff intricately changes consensus-visible contract validation and index participation across reference-derived values, notably resolve_derived_index_properties in rs-dpp and document_carries in rs-drive, affecting create, update, delete, and moderation behavior. - Phase 1 reviewers:
muse-spark-1.3-contributor— general (completed, effort xhigh); agentphase1-reviewer,muse-spark-1.3-contributor— rust-quality (completed, effort xhigh); agentphase1-reviewer - Phase 1 model:
muse-spark-1.3-contributor— not quota-gated; passed overgemini-3.8-flash-high(antigravity below 15% reserve: weekly 15% left, 5h 100% left),glm-5.3-flash(not used above high effort; tier asks max) - Single stage: Phase 1 and Phase 2 reviewed this head side by side, with no blocker gate between them (triage tier)
- Fresh verifier:
gpt-6.1-sol— final-verifier; agentsol-verifier - Phase 2 reviewers:
gpt-6.1-sol— general (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— platform-versioning (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort xhigh); agentphase2-reviewer
🤖 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/drive/document/cost/writes.rs`:
- [SUGGESTION] packages/rs-drive/src/drive/document/cost/writes.rs:201-207: Cover derived skip-value fallback in client cost estimates
The new fallback distinguishes an unresolved derived value from an explicitly supplied null: a missing derived entry participates when its reference is present, while an explicit Value::Null skips even with that reference present. The added drive-abci lifecycle test exercises server-side resolution but never calls document_create_cost, and the existing cost tests cover ordinary skip properties rather than this derived-value fallback. Add a table-driven regression through document_create_cost covering an omitted derived value with its reference present and absent, an explicit null with the reference present, and a supplied non-null value. Assert that skipped indexes receive neither own nor shared storage allocation, that a participating sibling retains its allocation, and that the unresolved, reference-present case still prices the derived index. This protects both conservative estimation and correct shared-layer attribution.
The SDK cost walker counts a derived value the caller did not resolve as present while its reference is, and a null one as absent. A table of four replies (unresolved with and without a quote, resolved to null, resolved) checks that a skipped index takes neither its own layer nor a share of the sibling's, that the sibling keeps its entry, and that an unresolved owner is priced as a resolved one. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Waiting for bot review — coderabbitai not yet · thepastaclaw ✓. Wait for the missing reviews, or a writer can post |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Re-review — Final validation — Phase 1 + Phase 2
Independently reviewed the complete diff at fb440ba and found no actionable in-scope defects. The derived skip behavior remains gated to PV14, and the new client-cost regression addresses the prior coverage finding. This was static verification only; the supplied exact-head CI snapshot still shows checks pending or queued, so build and test success is not established.
🔴 0 blocking | 🟡 0 suggestion(s) | 💬 0 nitpick(s)
Review provenance
Source: reviewer 1: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 2: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: platform-versioning); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 5: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); reviewer 6: gpt-6-astra (agent: phase2-reviewer, role: general); reviewer 7: gpt-6-astra (agent: phase2-reviewer, role: architecture-layering); reviewer 8: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 9: gpt-6-astra (agent: phase2-reviewer, role: platform-versioning); reviewer 10: gpt-6-astra (agent: phase2-reviewer, role: rust-quality); reviewer 11: gpt-6-astra (agent: phase2-reviewer, role: security-auditor); reviewer 12: muse-spark-1.3-contributor (agent: phase1-reviewer, role: rust-quality); reviewer 13: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 14: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 15: gpt-6.1-sol (agent: phase2-reviewer, role: platform-versioning); reviewer 16: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 17: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
criticalbygpt-6.1-sol(effort low) — The diff intricately changes consensus-visible contract validation and index membership across parsing, cost estimation, and document lifecycle operations, notably in resolve_derived_index_properties and document_carries in packages/rs-drive/src/drive/document/index_level_tree_types.rs. - Phase 1 reviewers:
muse-spark-1.3-contributor— general (completed, effort xhigh); agentphase1-reviewer,muse-spark-1.3-contributor— rust-quality (completed, effort xhigh); agentphase1-reviewer - Phase 1 model:
muse-spark-1.3-contributor— not quota-gated; passed overgemini-3.8-flash-high(antigravity below 15% reserve: weekly 15% left, 5h 100% left),glm-5.3-flash(not used above high effort; tier asks max) - Single stage: Phase 1 and Phase 2 reviewed this head side by side, with no blocker gate between them (triage tier)
- Fresh final gate: an independent Phase-2 review ran after iterative findings were reconciled
- Fresh verifier:
gpt-6.1-sol— final-verifier; agentsol-verifier - Phase 2 reviewers:
gpt-6.1-sol— general (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— platform-versioning (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort xhigh); agentphase2-reviewer,gpt-6-astra— general (completed, effort xhigh); agentphase2-reviewer,gpt-6-astra— architecture-layering (completed, effort xhigh); agentphase2-reviewer,gpt-6-astra— platform-versioning (completed, effort xhigh); agentphase2-reviewer,gpt-6-astra— rust-quality (completed, effort xhigh); agentphase2-reviewer,gpt-6-astra— security-auditor (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— general (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— platform-versioning (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort xhigh); agentphase2-reviewer - Model comparison: every Phase-2 reviewer also ran on
gpt-6-astra; the verifier weighed both sets without knowing which model wrote which
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify the current code and confirm that no unresolved issues remain.
No unresolved findings remain from the prior review on this head.
|
Bots are done — your move: post |
github-actions
Bot
added
waiting-self-review
Basic explanation
What this does: A document type can index a value it does not store, read from the document one of its references points at. For example, a reply indexed by
quoteId.$ownerIdis filed under the owner of the post it quotes. Until now, a reply quoting no post was still written into that index under an empty (null) value. With this change, the index can instead leave such a reply out entirely, the same wayskipIfAbsentalready leaves out documents missing one of their own properties.Value: Apps get indexes that hold only the documents that actually have the value. "Replies quoting my posts" or "replies to posts tagged X" then never store or walk entries for replies that quote nothing or whose post has no tag. Counts and rankings over such an index count only the documents that have the value.
Risks: Low to medium. This is consensus code at protocol version 14, which is not released yet, so no live data or earlier protocol version is affected. The new rule in Drive is that a null skip value counts as absent. It applies to every skip property, but a stored document never holds null for one of its own properties, so in practice it only changes behaviour for derived values. A wrong skip decision would leave an index entry behind or look for one that is not there, failing the delete or replace. The new end-to-end test covers create, edit, delete, moderator removal of the referenced post, and moderator removal and restore of the reply, and it fails when the null rule is reverted.
Issue being fixed or feature implemented
Derived index properties (#5216, #5223) were refused as
skipIfAbsentproperties. A document whose reference was absent, or whose referenced document had no value for the field, could only be filed under null.What was done?
Before: registration refused the index.
{ "name": "byQuotedOwner", "properties": [{ "quoteId.$ownerId": "asc" }, { "$createdAt": "asc" }], "skipIfAbsent": ["quoteId.$ownerId"] }(Past that check, the derived-property rules refused it too.)
After: the index registers. A reply quoting no post writes nothing into
byQuotedOwner, and its delete looks for nothing there. A reply quoting a post is filed under that post's owner, as before.Parser (dpp)
skipIfAbsentarray may name a derived index property.skip_if_absent_index_errornow admits a dotted name that reads through a reference (new helperreads_through_reference, which also replaces an equivalentparse_derived_index_property_name(..) != NotDerivedcheck in index property validation).skipIfAbsent: truestill means the type's own optional properties and leaves derived properties out. Whether a derived value can be absent depends on the referenced type, which the per-type parse that resolvestruecannot see. An index whose only optional properties are derived is refused undertrue, and the message points to the array form.resolve_derived_index_properties(full validation, every build) refuses a derived skip property that:$ownerId,$creatorId, or a schema property required together with every object around it);minItems0 on the referenced type, since an empty byte array is keyed like a missing value (the same rule a stored type's own skip properties follow).Drive
document_carries(index_level_tree_types.rs) decides whether a document carries a skip property: present and not null. Drive already puts a derived value into the document before keying it, as null when the reference or the field is absent, so that null now skips. The v2 insert and delete walkers and update v1 all go through it.cost/writes.rs) uses the same rule. A derived value the caller did not supply counts as carried while its reference is present, which keeps the estimate an upper bound, and as absent when the reference is absent.Docs
derived-index-properties.md, and theskipIfAbsentrules inindexes.mdupdated.skipIfAbsentdescription and v14 note 65.In-place changes to shipped generations
create_document_types_from_document_schemas1 (selected at PV9 to PV14): the new checks sit inresolve_derived_index_properties, which acts only on derived index properties. Only parser generation 3 (PV14) declares them, so the function's output is unchanged at every shipped version.try_from_schema/common(used by the shipped generations): theskipIfAbsent: trueresolution runs only whenindex.skip_if_absentis set, which only generation 3's grammar admits. Thereads_through_referencecheck is behindadmit_derived_index_properties, also generation 3 only, and is equivalent to the check it replaces.update_document_for_contract_operations1: selected only at PV14 (unreleased). In addition, every index of an earlier contract has an empty skip set, sotakes_part_in_index_byreturns true without reading the document.How Has This Been Tested?
derived_index_property_tests.rs, covering the array form admitted on every parse,$ownerIdthrough an optional vs a required reference, never-absent fields (a nested required field inside an optional object is admitted, and refused once the object is required), empty byte arrays, andtrueleaving derived properties out. The old skip refusal test is narrowed to the unique-index case.cargo test -p dpp --features all_features --lib data_contract: 2798 passed.should_skip_a_reply_whose_derived_value_is_absent. It checks the GroveDB index levels directly:byTopicandbyQuotedOwnerhold no null value, while the non-skippingbyTagdoes. It also checks queries and proofs, edits, deletion of a skipped reply after its post was removed (value read from the removal record), and moderator removal and restore of a reply. Mutation check: with the oldcontains_keyrule,byTopicgains a null key and the test fails.cargo test -p drive-abci --lib -- derived_index skip index_only document_cost: 49 passed.cargo test -p drive --lib -- skip cost derived layout index_only: 271 passed.cargo clippy -p dpp -p drive -p drive-abci --all-targets -- -D warnings: clean.cargo fmt --all: clean.Breaking Changes
Consensus change at protocol version 14 (unreleased): contracts that registration used to refuse are now accepted, and Drive skips documents holding a null skip value. No effect on any earlier protocol version.
Checklist:
structure.rs, regeneratedgrovedb-structure.json, and checked the structure viewer link posted on this pull requestFor repository code-owners and collaborators only
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
skipIfAbsent.startAtorstartAftermust fix derived properties with==.Bug Fixes
Documentation
PR Hygiene ·
fb440ba/self-reviewedWhen every merge requirement is met, the
PR Hygienecheck passes. Reviewer limits do not block merging; other required GitHub checks and protections still apply.