feat(gts): improve API with single-pass validation and reusable trait helpers#102
feat(gts): improve API with single-pass validation and reusable trait helpers#102aviator5 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>
📝 WalkthroughWalkthroughThis PR refactors ChangesGTS Store Refactor and Traits Enforcement
Sequence Diagram(s)sequenceDiagram
participant GtsOps
participant GtsStore
participant resolve_schema_refs_checked
participant EffectiveTraits
participant check_entity_traits
GtsOps->>GtsStore: with_reader(GtsFileReader)
GtsOps->>GtsOps: validate_entity(gts_id)
GtsOps->>GtsStore: validate_schema_refs, validate_schema_traits
GtsStore->>resolve_schema_refs_checked: schema
resolve_schema_refs_checked-->>GtsStore: Ok(inlined) or Err(CircularRef / UnresolvedRefs)
GtsStore->>EffectiveTraits: build_effective_traits(schemas, merged, dialect)
EffectiveTraits-->>GtsStore: validate(check_unresolved)
GtsOps->>check_entity_traits: gts_id
check_entity_traits->>GtsStore: effective_traits, content_is_abstract
check_entity_traits-->>GtsOps: Ok or Err(traits violation)
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested reviewers
🚥 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! |
… helpers - Split GtsStore::new(Option<reader>) into new() and with_reader() for clarity - Export ResolvedTypeSchema and ResolveSchemaRefsError for composable validation - Refactor trait validation into single-pass: validate_and_resolve_type_schema, validate_payload, validate_traits. Each builds effective traits schema once. - Extract inline_local_pointers to resolve JSON Pointer refs against host doc - Split validate_effective_traits into validate_prebuilt_effective (reusable after schema is pre-built) and effective_traits_schema_and_values (builder) - Add extract_gts_refs for discovering schema dependencies before resolution - resolve_schema_refs_checked now returns ResolveSchemaRefsError with CircularRef and UnresolvedRefs variants; preserve unresolved refs in lenient output - Add Default impl for GtsStore - Update all tests to use GtsStore::new() instead of GtsStore::new(None) Signed-off-by: Aviator 5 <ai.agent.tor@gmail.com>
e758186 to
09a04bd
Compare
- Accept gts:// $ref values with supported JSON Pointer fragments while rejecting unsupported fragments. - Compile fully resolved schemas in validate_and_resolve_type_schema to catch malformed bodies deferred by raw GTS refs. - Validate relative x-gts-ref targets with GtsIdPattern so wildcard patterns resolve consistently. Signed-off-by: Aviator 5 <ai.agent.tor@gmail.com>
94e95eb to
18bc635
Compare
- Surface checked resolution failures through StoreError for unresolved and circular refs. - Preserve allOf closedness from referenced schemas and keep non-object ref targets intact. - Reject over-deep ref scans and add regression coverage for chained IDs, URI validation, local pointer overlay, and referenced trait schemas. Signed-off-by: Aviator 5 <ai.agent.tor@gmail.com>
18bc635 to
8afe001
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
gts/src/store.rs (1)
527-614:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUnresolved
allOfitems are silently dropped when other items resolve.When
allOfcontains a mix of resolvable and unresolvable items, theresolved_all_ofvector accumulates the unresolved items (line 549) but is never included in the returnedmerged_schema(lines 582-614). This silently drops unresolved$refs from the lenient resolution output, contradicting the documented behavior that "unresolved refs [are] left intact."Example:
{"allOf": [{"$ref": "gts://missing~"}, {"properties": {"a": {}}}]}would flatten to{"properties": {"a": {}}}, dropping the unresolved ref entirely.For
resolve_schema_refs_checked, this is masked becauseunresolved_refstracking correctly raises an error. But for the lenientresolve_schema_refs, schemas are silently weakened.Proposed fix: include unresolved items in the returned schema
// If we have merged properties, create a single schema instead of allOf if !merged_properties.is_empty() { let mut merged_schema = serde_json::Map::new(); // Copy all properties except allOf for (k, v) in map { if k != "allOf" { merged_schema.insert(k.clone(), v.clone()); } } // Add merged properties and required fields merged_schema .insert("properties".to_owned(), Value::Object(merged_properties)); if let Some(ap) = merged_schema.get("additionalProperties").cloned() { merge_additional_properties_constraint( &mut merged_additional_properties, &ap, ); } if let Some(ap) = merged_additional_properties { merged_schema.insert("additionalProperties".to_owned(), ap); } if !merged_required.is_empty() { merged_schema.insert( "required".to_owned(), Value::Array( merged_required.into_iter().map(Value::String).collect(), ), ); } + + // Preserve unresolved allOf items to avoid silently weakening the schema + if !resolved_all_of.is_empty() { + merged_schema.insert("allOf".to_owned(), Value::Array(resolved_all_of)); + } return Value::Object(merged_schema); }🤖 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 527 - 614, When merging allOf array items in the resolve_schema_refs_inner function, the unresolved items that still contain $ref are correctly accumulated in the resolved_all_of vector, but they are never included in the returned merged_schema. After creating the merged_schema and populating it with merged properties, required fields, and additionalProperties constraints, add logic to include the resolved_all_of items back into the merged_schema by inserting them as an allOf field if the vector is not empty. This ensures that unresolved refs are preserved in the output instead of being silently dropped from the schema.
🤖 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.
Outside diff comments:
In `@gts/src/store.rs`:
- Around line 527-614: When merging allOf array items in the
resolve_schema_refs_inner function, the unresolved items that still contain $ref
are correctly accumulated in the resolved_all_of vector, but they are never
included in the returned merged_schema. After creating the merged_schema and
populating it with merged properties, required fields, and additionalProperties
constraints, add logic to include the resolved_all_of items back into the
merged_schema by inserting them as an allOf field if the vector is not empty.
This ensures that unresolved refs are preserved in the output instead of being
silently dropped from the schema.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b92e65dc-e29d-4431-a736-04fee66f321b
📒 Files selected for processing (9)
gts-id/src/gts_id.rsgts/src/entities.rsgts/src/lib.rsgts/src/ops.rsgts/src/schema_compat.rsgts/src/schema_refs.rsgts/src/schema_traits.rsgts/src/store.rsgts/src/store_test.rs
🚧 Files skipped from review as they are similar to previous changes (7)
- gts/src/lib.rs
- gts/src/schema_compat.rs
- gts/src/entities.rs
- gts/src/schema_refs.rs
- gts/src/ops.rs
- gts/src/schema_traits.rs
- gts/src/store_test.rs
This PR refactors GTS trait validation and schema resolution: it adds a new
schema_refs.rsmodule for extracting external GTS type references, consolidates trait materialization behind anEffectiveTraitsstruct (const+default precedence, JSON Pointer inlining), reworks theGtsStoreconstructor API (new()+with_reader()), adds single-pass validation methods plusCircularRef/UnresolvedRefserror variants, and introducescheck_entity_traits()(OP#13) entity-level validation. It also tightensx-gts-refwildcard handling viaGtsIdPatternand derives entity type IDs purely from the$idchain.Summary by CodeRabbit
Summary
New Features
$refdependencies.Bug Fixes
$refresolution: now errors on circular and unresolved externalgts://references.const/defaultmaterialization precedence andallOfadditionalPropertiesclosedness merging.Tests
$refextraction/resolution, and trait/schema validation edge cases.