refactor(dpp): extract shared try_from_schema parsing helpers - #4276
Conversation
The document-type parser generations (v0/v1/v2) carried near-identical copies of name validation, schema depth/size accounting, meta-schema selection + compilation, keeps*History flag extraction, the core property/index parsing body, and the doctype-level aggregate keyword handling. Extract them into try_from_schema/common as parameter-pure helpers: nothing in common reads the platform-version tables; each generation passes its own constants (v1 supplies the one genuinely version-varying input, the document_type_schema >= 2 keeps*History gate, since upstream v2 delegates core parsing to v1). Zero behavior change: the full dpp suite passes with an identical test-name set (3806 passed before and after), no test was added or removed, and the only test-module edits pin each generation's tests to that generation's own protocol versions (v0->PV8, v1->PV11 with the meta-schema axis at PV12, v2->PV13) instead of latest(), so the guards keep targeting the generation they were written for as new protocol versions land. Net -1115 lines across the parser generations. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
🕓 Ready for review — 15 ahead in queue (commit d2200de) |
|
Warning Review limit reached
Next review available in: 51 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe PR adds shared, generation-aware document-type schema parsing utilities. Generations 0 and 1 use shared validation and core parsing. Generation 2 uses shared aggregate parsing and validation. Parser tests now use fixed protocol versions. ChangesDocument-type schema parsing
Estimated code review effort: 4 (Complex) | ~60 minutes 🚥 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 |
# Conflicts: # packages/rs-dpp/src/data_contract/document_type/class_methods/try_from_schema/v2/mod.rs
Builds on the try_from_schema shared-helpers extraction (#4276): the generation-3 parser becomes a thin driver over common's parameter-pure helpers instead of a fourth copy of the parsing body. The ranked layer common gains here is the part that cannot exist on the base branch: the ParserGeneration ranked fields (admit_meta_schema_v3, admit_ranked, ranked_index_key_length_check), the meta-schema v3 selection arm, the ranked-aware Index::try_from_value_map call, and the per-property ranked key-length hook that v3 fills with its 247/239-byte ceilings. Generation v3 now passes admit_history: true as a literal, removing the always-true document_type_schema >= 2 conditional it previously carried; v1/v2 pick up only their generation-range doc comments (on this branch generation 2 ends at PV13, where the base branch has it extending to latest). No behavior change: full dpp suite at baseline (3846 passed, 0 failed), drive suite 3316/0, workspace check clean. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/rs-dpp/src/data_contract/document_type/class_methods/try_from_schema/v1/mod.rs (1)
609-636: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winPoint this generation-1 test at the generation-1 parser.
Line 625 calls
DocumentTypeV0::try_from_schema, insidev1/mod.rs. The test now also passesgeneration_1_platform_version()(PV11) to it. PV11 never selects generation 0, so this assertion drives the generation-0 parser with a protocol version that does not belong to it. The new helper's doc comment at lines 107-111 states the pin exists to stop exactly this mis-targeting.The effect is a coverage gap, not a production defect: generation 1 is never tested against a document type name containing a space. Only the
invalid&namecase at line 653 reachesDocumentTypeV1.Note that
DocumentTypeV1::try_from_schematakes the extratoken_configurationsargument, so the call needs&BTreeMap::new()added.💚 Proposed fix
- let result = DocumentTypeV0::try_from_schema( + let result = DocumentTypeV1::try_from_schema( Identifier::new([1; 32]), 1, config.version(), "invalid name", schema.clone(), None, + &BTreeMap::new(), &config, true, &mut vec![], platform_version, );The
use crate::data_contract::document_type::DocumentTypeV0;import at line 103 then becomes unused and should be removed.🤖 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-dpp/src/data_contract/document_type/class_methods/try_from_schema/v1/mod.rs` around lines 609 - 636, Update the test in v1/mod.rs to call DocumentTypeV1::try_from_schema instead of DocumentTypeV0::try_from_schema, preserving generation_1_platform_version() and adding the required &BTreeMap::new() token_configurations argument. Remove the now-unused DocumentTypeV0 import.
🧹 Nitpick comments (1)
packages/rs-dpp/src/data_contract/document_type/class_methods/try_from_schema/v2/mod.rs (1)
130-140: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueCorrect the version claim in the helper doc comment.
Line 133 states PV13 is the first protocol version whose
try_from_schemaselects generation 2. Line 1 and lines 35-36 of this file state generation 2 covers protocol version 12 onward. PV12 is the first version selecting generation 2; PV13 is the first version wheredocument_type_schemais 2. The compressed sentence reads as a contradiction of the module doc.The pinned value PV13 itself is correct for these tests, because they exercise the aggregate keywords under meta-schema 2.
Line 211 is also affected: the
expectmessage still says "default config available on latest platform version", but the version is now pinned.📝 Proposed wording fix
/// Generation-specific tests must pin a protocol version that actually /// selects their own generation: `PlatformVersion::latest()` silently /// retargets these tests onto a different parser generation and a - /// different document meta-schema whenever LATEST moves. PV13 is the first - /// protocol version whose `try_from_schema` selects generation 2 with - /// `document_type_schema: 2` — the meta-schema `latest()` resolves to - /// today, so behavior here is unchanged by the pin. + /// different document meta-schema whenever LATEST moves. Generation 2 + /// serves PV12 onward; PV13 is the first of those versions with + /// `document_type_schema: 2`, which is the meta-schema these aggregate + /// tests need. It is also the meta-schema `latest()` resolves to today, so + /// behavior here is unchanged by the pin.- .expect("default config available on latest platform version"); + .expect("default config available on the pinned generation-2 platform version");🤖 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-dpp/src/data_contract/document_type/class_methods/try_from_schema/v2/mod.rs` around lines 130 - 140, Correct the documentation for generation_2_platform_version so it states that PV12 is the first protocol version selecting generation 2, while PV13 is the first using document_type_schema 2; keep the pinned PV13 value unchanged. Also update the expect message in the helper’s call site to describe protocol version 13 rather than a default/latest platform version.
🤖 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-dpp/src/data_contract/document_type/class_methods/try_from_schema/common/mod.rs`:
- Around line 502-520: Move the countable and range-countable index admission
checks from the protocol-version conditions in the shared parser logic into
ParserGeneration. Add an admit_count_indexes field, set it to false for
generation 1 and true for generation 2 alongside admit_history, and use this
field for both countable checks while preserving the existing
UnsupportedFeatureError behavior.
---
Outside diff comments:
In
`@packages/rs-dpp/src/data_contract/document_type/class_methods/try_from_schema/v1/mod.rs`:
- Around line 609-636: Update the test in v1/mod.rs to call
DocumentTypeV1::try_from_schema instead of DocumentTypeV0::try_from_schema,
preserving generation_1_platform_version() and adding the required
&BTreeMap::new() token_configurations argument. Remove the now-unused
DocumentTypeV0 import.
---
Nitpick comments:
In
`@packages/rs-dpp/src/data_contract/document_type/class_methods/try_from_schema/v2/mod.rs`:
- Around line 130-140: Correct the documentation for
generation_2_platform_version so it states that PV12 is the first protocol
version selecting generation 2, while PV13 is the first using
document_type_schema 2; keep the pinned PV13 value unchanged. Also update the
expect message in the helper’s call site to describe protocol version 13 rather
than a default/latest platform version.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 7e30705f-96ad-44f0-a3a3-f1acd23e4fff
📒 Files selected for processing (5)
packages/rs-dpp/src/data_contract/document_type/class_methods/try_from_schema/common/mod.rspackages/rs-dpp/src/data_contract/document_type/class_methods/try_from_schema/mod.rspackages/rs-dpp/src/data_contract/document_type/class_methods/try_from_schema/v0/mod.rspackages/rs-dpp/src/data_contract/document_type/class_methods/try_from_schema/v1/mod.rspackages/rs-dpp/src/data_contract/document_type/class_methods/try_from_schema/v2/mod.rs
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## v4.2-dev #4276 +/- ##
============================================
+ Coverage 87.52% 87.54% +0.01%
============================================
Files 2678 2679 +1
Lines 341047 341312 +265
============================================
+ Hits 298518 298798 +280
+ Misses 42529 42514 -15
🚀 New features to boost your workflow:
|
Review follow-up: parse_document_type_core still read platform_version.protocol_version at the countable/rangeCountable admission checks, contradicting the module invariant that grammar admission comes only from ParserGeneration. The gate is now an admit_count_indexes field computed by the v1 driver alongside admit_history (the entry point serves generations 1 and 2, so both sides of the PV12 boundary flow through it). Also points the generation-1 space-in-name test at the generation-1 parser: it called DocumentTypeV0::try_from_schema at PV11, which never selects generation 0, leaving DocumentTypeV1 untested for that case. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Resolves the inherited "TODO: Split into multiple functions": the ~620-line core is now a ~140-line pipeline over private stage functions (schema validation, flags, properties, indices, per-index property validation, paths/key requirements, token costs), with a CoreParseContext bundle carrying the invariant inputs instead of threading them loose. Pure code motion: no clone was introduced at any seam, error orderings are unchanged, and the count-index admission checks plus the index grammar stay together in the index stage. The blanket #[allow(unused_variables)] on the monolith is gone; the remaining allows are conditional on validation being off. Full dpp suite unchanged (3806 passed, identical test-name set), drive 3258/0, workspace check and all-features clippy clean. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…to claude/try-from-schema-dedup
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/rs-dpp/src/data_contract/document_type/class_methods/try_from_schema/v1/mod.rs (1)
68-100: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAdd a protocol-12 count-index regression test in the v2 parser.
DocumentTypeV2::try_from_schemadelegates throughDocumentTypeV1::try_from_schema, so the count-index rejection at PV11 and admission at PV12 can bypass v2 tests. Add a v2 regression at protocol version 12 that exercisesindex.countable/rangeCountableand asserts the parsed index metadata flows into the finalDocumentTypeV2.🤖 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-dpp/src/data_contract/document_type/class_methods/try_from_schema/v1/mod.rs` around lines 68 - 100, Add a regression test in the DocumentTypeV2 parser tests using protocol version 12 that defines indexes with countable and rangeCountable metadata, parses them through DocumentTypeV2::try_from_schema, and asserts the resulting index metadata is present on the final DocumentTypeV2. Ensure the test specifically covers the protocol-12 admission path and complements the protocol-11 rejection behavior.Source: Learnings
🤖 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-dpp/src/data_contract/document_type/class_methods/try_from_schema/v1/mod.rs`:
- Around line 68-100: Add a regression test in the DocumentTypeV2 parser tests
using protocol version 12 that defines indexes with countable and rangeCountable
metadata, parses them through DocumentTypeV2::try_from_schema, and asserts the
resulting index metadata is present on the final DocumentTypeV2. Ensure the test
specifically covers the protocol-12 admission path and complements the
protocol-11 rejection behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 2498f05c-1811-47c2-a602-1c597900c5ad
📒 Files selected for processing (2)
packages/rs-dpp/src/data_contract/document_type/class_methods/try_from_schema/common/mod.rspackages/rs-dpp/src/data_contract/document_type/class_methods/try_from_schema/v1/mod.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/rs-dpp/src/data_contract/document_type/class_methods/try_from_schema/common/mod.rs
Review follow-up: the countable/rangeCountable admission flip at PV12 reaches generation 2 only by delegation through DocumentTypeV1, so no v2 test exercised it. Adds a pair of regression tests: admission at PV12 with the parsed index metadata asserted on the final DocumentTypeV2, and rejection of the same schema under full validation at PV11. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
CodeRabbit's nitpick (count-index boundary untested through the v2 parser) is addressed in d2200de: 🤖 Addressed by Claude Code |
2 participants
Issue being fixed or feature implemented
The document-type parser generations under
packages/rs-dpp/.../class_methods/try_from_schema/(v0,v1,v2) carry near-identical copies of the same parsing sub-steps. Every new parser generation (e.g. the upcoming one in #4266) has to copy ~1,900 lines and mutate a few dozen of them, which makes review hard and lets the copies drift.What was done?
Extracted the shared sub-steps into a new
try_from_schema/commonmodule of parameter-pure helpers — nothing incommonreads the platform-version tables; all variability comes in as parameters, and each generation passes its own constants:validate_document_type_name,validate_schema_depth_and_account_for_size,select_document_meta_schema,validate_against_meta_schema_and_compile— verbatim extractions of the previously inline code (the two per-generation method-name strings are now a parameter).parse_keeps_history_flags(props, admit_history)— thekeeps*Historyextraction.v1passes the one genuinely version-varying input (document_type_schema >= 2), sincev2has always delegated its core parsing tov1and that body serves generations 1 and 2 across PV12/PV13 schema values.parse_document_type_core(…, &ParserGeneration, …)— the main property/index parsing body shared by generations 1+2 (token-identical to the previous inline body; verified by comment/whitespace-stripped diff).parse_doctype_aggregate_keywords/apply_doctype_aggregates— the doctype-leveldocumentsCountable/documentsSummable/range*handlingv2layers on top.v0routes through the four sub-step helpers but keeps its own core, which genuinely diverges (DocumentTypeV0output, no token costs, no countable gate).Net −1,115 lines across the generations. This is also the base for #4266, whose new parser generation becomes a thin driver over these helpers instead of a fourth copy.
Test modules: each generation's tests now pin that generation's own protocol versions (v0→PV8, v1→PV11 + the meta-schema axis at PV12, v2→PV13) instead of
PlatformVersion::latest(), so the guards keep targeting the generation they were written for as new protocol versions land. No test added, removed, or otherwise changed.How Has This Been Tested?
Refactor purity was the acceptance bar:
cargo test -p dpp --lib: 3806 passed / 0 failed / 6 ignored — identical result and identical sorted test-name set before and after the change (diffed, zero delta).cargo test -p drive --lib(contract parsing flows through this code): 3258 passed / 0 failed.cargo check --workspace --all-targets,cargo check -p dpp --no-default-features --features state-transitions,cargo fmt --all --check: clean.v1body: the only differences are thekeeps*Historyextraction and the impl-block brace.Breaking Changes
None — no behavior change, no public API change (all helpers are
pub(super)).Checklist:
For repository code-owners and collaborators only
🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Compatibility
Tests