feat(gts): single-pass type/payload validation and reusable schema helpers#103
Open
aviator5 wants to merge 4 commits into
Open
feat(gts): single-pass type/payload validation and reusable schema helpers#103aviator5 wants to merge 4 commits into
aviator5 wants to merge 4 commits into
Conversation
- Derive Type Schema parent IDs from chained values instead of fallbacks. - Validate x-gts-ref literals and matches through GtsIdPattern for malformed wildcard rejection and segment-aware version matching. Signed-off-by: Aviator 5 <ai.agent.tor@gmail.com>
|
Warning Review limit reached
More reviews will be available in 29 minutes and 11 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?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 credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan refill rate. 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, the refill rate gradually slows as usage increases. The highest same-day bursts are limited more strictly. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (9)
📝 WalkthroughWalkthroughRefactors ChangesGTS Schema Validation & Trait System Overhaul
Sequence Diagram(s)sequenceDiagram
participant Client
participant GtsOps
participant validate_gts_keywords
participant GtsStore
participant SchemaResolver
participant schema_traits
Client->>GtsOps: add_entity(entity, validate=true)
GtsOps->>validate_gts_keywords: validate_gts_keywords(content)
validate_gts_keywords-->>GtsOps: Ok / Err
GtsOps->>GtsStore: register(entity)
GtsStore-->>GtsOps: Ok(gts_id)
GtsOps->>GtsStore: store.validate_schema(gts_id)
GtsStore->>SchemaResolver: try_resolve_schema_refs(schema)
SchemaResolver-->>GtsStore: Ok(resolved_schema) / Err(CircularRef/UnresolvedRefs)
GtsStore->>schema_traits: build_effective_traits(schemas, traits)
schema_traits->>schema_traits: inline_local_pointers(fragment, root)
schema_traits->>schema_traits: materialize_traits_recursive(values)
schema_traits-->>GtsStore: EffectiveTraits
GtsStore->>schema_traits: EffectiveTraits::validate(check_unresolved)
schema_traits-->>GtsStore: Ok / Err(Vec<String>)
GtsStore-->>GtsOps: Ok(ResolvedType) / Err(StoreError)
GtsOps->>GtsOps: check_entity_traits(gts_id)
GtsOps-->>Client: Ok / Err(entity validation failure)
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
|
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
gts/src/store.rs (2)
1199-1208:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRun
x-gts-refchecks against the resolved schema.JSON Schema validation uses the resolved schema, but Line 1244 passes the raw schema to
XGtsRefValidator. Since that validator does not follow$ref,x-gts-refconstraints inside referenced schemas are skipped for registered instance validation;validate_payloadalready uses the resolved schema.🐛 Proposed fix to retain the resolved schema for extension validation
- let schema_with_internal_refs_resolved = self + let resolved_schema = self .try_resolve_schema_refs(&schema) .map_err(|e| StoreError::ValidationError(format!("Schema '{type_id}' has {e}")))?; @@ let schema_with_internal_refs_resolved = - Self::remove_x_gts_ref_fields(&schema_with_internal_refs_resolved); + Self::remove_x_gts_ref_fields(&resolved_schema); @@ // Validate x-gts-ref constraints let validator = crate::x_gts_ref::XGtsRefValidator::new(); - let x_gts_ref_errors = validator.validate_instance(&obj.content, &schema, ""); + let x_gts_ref_errors = validator.validate_instance(&obj.content, &resolved_schema, "");Also applies to: 1243-1246
🤖 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 `@gts/src/store.rs` around lines 1199 - 1208, The XGtsRefValidator should validate against the resolved schema to ensure x-gts-ref constraints inside referenced schemas are checked, not the raw schema. Pass the resolved schema (the result of try_resolve_schema_refs) to the XGtsRefValidator instead of the raw schema parameter. This will make x-gts-ref validation consistent with JSON Schema validation which already uses the resolved schema, and ensure that constraints in referenced schemas are properly validated during registered instance validation.
543-611:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPreserve passthrough
allOfbranches when flattening.When any branch contributes
properties, Line 611 returns the flattened schema but ignoresresolved_all_of. That drops unresolved refs in lenient output and also loses non-mergeable constraints from sibling branches, silently weakening schemas.🐛 Proposed fix to keep non-flattened branches
Value::Object(ref item_map) => { // If this item still has a $ref, keep it in allOf if item_map.contains_key("$ref") { resolved_all_of.push(resolved_item); } else { + let mut passthrough = item_map.clone(); // Merge properties and required fields from resolved items if let Some(Value::Object(props_map)) = item_map.get("properties") { + passthrough.remove("properties"); for (k, v) in props_map { merged_properties.insert(k.clone(), v.clone()); } } if let Some(Value::Array(req_array)) = item_map.get("required") { + passthrough.remove("required"); for v in req_array { if let Value::String(s) = v && !merged_required.contains(s) { merged_required.push(s.to_owned()); @@ if let Some(ap) = item_map.get("additionalProperties") { + passthrough.remove("additionalProperties"); merge_additional_properties_constraint( &mut merged_additional_properties, ap, ); } + passthrough.remove("$id"); + passthrough.remove("$schema"); + if !passthrough.is_empty() { + resolved_all_of.push(Value::Object(passthrough)); + } } } @@ if let Some(ap) = merged_additional_properties { merged_schema.insert("additionalProperties".to_owned(), ap); } + if !resolved_all_of.is_empty() { + merged_schema.insert("allOf".to_owned(), Value::Array(resolved_all_of)); + } if !merged_required.is_empty() {🤖 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 `@gts/src/store.rs` around lines 543 - 611, The code at the return statement (line 611) creates and returns a merged schema but discards the resolved_all_of items that contain unresolved $ref branches and non-mergeable constraints. To fix this, after creating the merged_schema map and before returning it, check if resolved_all_of is not empty. If it contains items, add an allOf field to merged_schema that includes both the merged schema itself and any items from resolved_all_of that couldn't be flattened. This ensures unresolved references and non-mergeable constraints are preserved in the output schema rather than being silently dropped.gts/src/schema_traits.rs (1)
264-272:⚠️ Potential issue | 🟠 MajorPass the effective dialect to trait schema integrity validation.
validate_trait_schema_integrity()at line 101 validates individual trait schema fragments usingjsonschema::validator_for(ts), which defaults to the crate's default draft (Draft 2020-12) per line 234. The dialect pinning at lines 301-305 happens afterward, so trait fragments that use dialect-specific keywords could pass validation under the wrong draft or fail unnecessarily under the correct one. Since EffectiveTraits holds the dialect-pinned schema, pass the dialect to the integrity check, or temporarily inject$schemainto object fragments that don't already declare one.🤖 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 `@gts/src/schema_traits.rs` around lines 264 - 272, The validation of trait schema fragments in the loop over resolved_trait_schemas uses jsonschema::validator_for(ts) which defaults to Draft 2020-12 instead of the effective dialect that is pinned later. Update the validation logic to either accept and pass the effective dialect to jsonschema::validator_for() so trait fragments are validated against the correct draft, or temporarily inject a `$schema` keyword into Value::Object fragments that do not already declare one before calling the validator, ensuring fragments are validated against the intended dialect.
🤖 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 `@gts/src/entities.rs`:
- Around line 239-244: The code in the GtsId parsing block accepts any valid
GtsId through try_new, but for schema entities the $id field should specifically
be a type ID (ending with ~), not an instance-like ID. Add an additional
validation check after GtsId::try_new succeeds to verify that the normalized
value is specifically a type ID by confirming it ends with the type ID suffix.
Only set gts_id, instance_id, and selected_entity_field if both the
GtsId::try_new validation and the type ID suffix validation pass.
In `@gts/src/ops.rs`:
- Around line 704-713: The validation loop for trait schemas currently uses an
if-let pattern that silently skips non-object schemas like booleans, causing
them to incorrectly pass validation. In the loop iterating through
traits.resolved_trait_schemas, modify the condition to explicitly reject
non-object schemas in addition to object schemas without additionalProperties:
false. Only object schemas with additionalProperties: false should be considered
valid; if the schema is not an object (ts.as_object() returns None), or if it's
an object without the required additionalProperties setting, an error should be
returned. Consider using an else clause to handle the non-object case or
restructure the logic to check for closed object schemas in a single validation.
- Around line 350-355: The validate_gts_keywords check is currently executed
after self.store.register has already mutated the store, which allows malformed
schemas to be persisted if validation fails. Move the entire if block containing
the validate_gts_keywords call to execute before self.store.register is invoked.
This ensures that if the GTS keywords validation fails in validate_gts_keywords,
the entity is rejected before any store mutations occur, preventing malformed
schemas from being left in the store.
In `@gts/src/schema_refs.rs`:
- Around line 53-67: The collect_gts_refs function recursively processes all
values in a schema object map without considering which keys they belong to,
causing it to incorrectly extract `$ref` from data-valued keywords like const,
default, examples, and enum. Modify the loop that iterates over map.values() to
skip values whose keys are known data-valued schema keywords. Instead of
iterating directly over map.values(), iterate over map.iter() and check if the
key is a data-valued keyword before recursing; if it is a data-valued keyword,
skip the recursive call for that value.
In `@gts/src/schema_traits.rs`:
- Around line 676-691: In the nested object handling branch of
materialize_traits_recursive, the code is replacing explicitly set but
type-mismatched values with newly materialized objects, which prevents JSON
Schema validation from catching the original error. Instead of unconditionally
replacing the result value with the materialized nested object, check whether
the original input value at that key exists and is not null before replacing it.
If an explicit value is already present in the input (even if it has the wrong
type), preserve it unchanged so that JSON Schema validation can detect and
report the type mismatch later.
In `@gts/src/store.rs`:
- Around line 474-490: The current merging logic in the
resolve_schema_refs_inner function when handling Value::Object(resolved_map)
performs a last-wins merge that can lose schema constraints like required fields
or additionalProperties restrictions from the referenced schema. Instead of
directly merging the resolved schema with sibling properties, detect when a $ref
has sibling properties and wrap them in an allOf array construct that contains
both the referenced schema and an object with the sibling properties. This
preserves constraints from both the referenced schema and the siblings rather
than allowing siblings to overwrite them. Update the code where merged is
created and returned to implement this allOf wrapping strategy.
In `@gts/src/x_gts_ref.rs`:
- Around line 416-424: The resolve_pointer method in the code that handles
x-gts-ref pointer resolution lacks cycle detection, which can cause infinite
recursion and stack overflow when circular references exist in the schema.
Create a new method resolve_pointer_checked that accepts an additional parameter
for tracking visited pointers using a BTreeSet, or modify the existing
resolve_pointer to include cycle detection. Before each pointer traversal, check
if the pointer string is already in the visited set; if it is, return None to
break the cycle. If not, insert it into the visited set and proceed with the
existing traversal logic. Ensure that any recursive calls to resolve pointers
during x-gts-ref hops also use the cycle-checking variant to maintain
consistency throughout the resolution chain.
---
Outside diff comments:
In `@gts/src/schema_traits.rs`:
- Around line 264-272: The validation of trait schema fragments in the loop over
resolved_trait_schemas uses jsonschema::validator_for(ts) which defaults to
Draft 2020-12 instead of the effective dialect that is pinned later. Update the
validation logic to either accept and pass the effective dialect to
jsonschema::validator_for() so trait fragments are validated against the correct
draft, or temporarily inject a `$schema` keyword into Value::Object fragments
that do not already declare one before calling the validator, ensuring fragments
are validated against the intended dialect.
In `@gts/src/store.rs`:
- Around line 1199-1208: The XGtsRefValidator should validate against the
resolved schema to ensure x-gts-ref constraints inside referenced schemas are
checked, not the raw schema. Pass the resolved schema (the result of
try_resolve_schema_refs) to the XGtsRefValidator instead of the raw schema
parameter. This will make x-gts-ref validation consistent with JSON Schema
validation which already uses the resolved schema, and ensure that constraints
in referenced schemas are properly validated during registered instance
validation.
- Around line 543-611: The code at the return statement (line 611) creates and
returns a merged schema but discards the resolved_all_of items that contain
unresolved $ref branches and non-mergeable constraints. To fix this, after
creating the merged_schema map and before returning it, check if resolved_all_of
is not empty. If it contains items, add an allOf field to merged_schema that
includes both the merged schema itself and any items from resolved_all_of that
couldn't be flattened. This ensures unresolved references and non-mergeable
constraints are preserved in the output schema rather than being silently
dropped.
🪄 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: 1c39a5e0-161d-45af-b632-0784969dd8bf
📒 Files selected for processing (13)
gts-id/src/gts_id.rsgts-macros/tests/inheritance_tests.rsgts-macros/tests/integration_tests.rsgts/src/entities.rsgts/src/lib.rsgts/src/ops.rsgts/src/schema_compat.rsgts/src/schema_modifiers.rsgts/src/schema_refs.rsgts/src/schema_traits.rsgts/src/store.rsgts/src/store_test.rsgts/src/x_gts_ref.rs
aviator5
force-pushed
the
gts-improvements
branch
from
604982d to
991b73c
Compare
…lpers API & store: - Split GtsStore::new(Option<reader>) into new() and with_reader(); add Default impl; update all call sites and tests to GtsStore::new() - Add validate_schema(type_id) -> ResolvedType, a self-contained resolved view (inlined schema, chain-merged effective traits, composed traits schema); export ResolvedType - validate_payload/validate_instance reuse the resolved view instead of re-deriving traits per call - try_resolve_schema_refs returns StoreError::CircularRef / UnresolvedRefs; resolve_schema_refs keeps unresolved refs in lenient output Schema helpers: - New schema_refs module: extract_gts_refs + ExtractRefsError for discovering schema dependencies before resolution - schema_traits: extract inline_local_pointers (resolve JSON Pointer refs against the host doc); build_effective_traits / build_effective_traits_schema build the effective traits schema once - schema_compat: add merge_additional_properties_constraint, a closedness- preserving lattice so a permissive allOf overlay can never loosen a closed base; apply it in extract_effective_schema and refine compatibility comments - schema_modifiers: add validate_gts_keywords, a single structural gate for x-gts-final/abstract/traits keyword format + placement Fixes: - x_gts_ref: a relative x-gts-ref resolving to a wildcard pattern is now accepted via GtsIdPattern, matching the absolute branch - entities: chained Type Schema doc fix (type_id is parent, None for standalone) plus coverage - gts-id: reject the gts: form and assert is_valid agrees on gts:// Signed-off-by: Aviator 5 <ai.agent.tor@gmail.com>
aviator5
force-pushed
the
gts-improvements
branch
from
991b73c to
5148eab
Compare
…tion - entities: require schema $id to be a type id (reject instance-like $id) - ops: run validate_gts_keywords before register; reject boolean trait schemas as not closed - schema_refs: skip data-valued keywords (const/default/examples/enum) when extracting $ref dependency edges - schema_traits: don't materialize over a non-object value so type errors still reach validation - store: combine $ref siblings via allOf instead of last-wins merge - x_gts_ref: depth-guard resolve_pointer against cyclic x-gts-ref Signed-off-by: Aviator 5 <ai.agent.tor@gmail.com>
aviator5
force-pushed
the
gts-improvements
branch
from
ddb5218 to
7d24239
Compare
…t trait checks - Extract `$ref` resolution into a `schema_resolve` module built around a narrow `SchemaProvider` trait + `SchemaResolver`; `GtsStore` keeps thin `resolve_schema_refs`/`try_resolve_schema_refs` wrappers and implements the provider. - Resolve `allOf`/`anyOf`/`oneOf` faithfully: inline `$ref`s in place and preserve the combinator (no lossy property flatten). Strip only root-only keys from inlined targets (`$id`/`$schema` and the `x-gts-*` type-level modifiers). - `ResolvedType` gains `id` (GtsTypeId), `is_abstract`, `is_final`. - `validate_schema`: abstract types now type-check the trait values they provide, deferring only the required-trait completeness check. - Entity closedness check detects `additionalProperties: false` through `allOf`. - Tests: cover 2–4 level trait merge (default/const/null), trait-schema `allOf`+`$ref` resolution, and exact `ResolvedType` assertions; move resolver unit tests into `schema_resolve_test` (driven via a mock provider) and keep wrapper smoke tests in `store_test`. Signed-off-by: Aviator 5 <ai.agent.tor@gmail.com>
aviator5
force-pushed
the
gts-improvements
branch
from
fc35374 to
75e482c
Compare
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.
API & store:
Default impl; update all call sites and tests to GtsStore::new()
view (inlined schema, chain-merged effective traits, composed traits
schema); export ResolvedType
re-deriving traits per call
resolve_schema_refs keeps unresolved refs in lenient output
Schema helpers:
discovering schema dependencies before resolution
against the host doc); build_effective_traits / build_effective_traits_schema
build the effective traits schema once
preserving lattice so a permissive allOf overlay can never loosen a closed
base; apply it in extract_effective_schema and refine compatibility comments
x-gts-final/abstract/traits keyword format + placement
Fixes:
accepted via GtsIdPattern, matching the absolute branch
standalone) plus coverage
Summary by CodeRabbit
additionalPropertiesclosedness across composed schemas.x-gts-refvalue/pattern matching, including recursion-depth safeguards and cycle termination.