From 14c52dff5d7a93a9b00057c29874d8496f3042b3 Mon Sep 17 00:00:00 2001 From: Aviator 5 Date: Wed, 24 Jun 2026 13:00:05 +0300 Subject: [PATCH 1/7] refactor(schema): remove redundant GTS retriever - Rely on strict pre-resolution before compiling schemas for validation. - Restrict best-effort schema reference resolution to crate internals and silence dead-code warnings. Signed-off-by: Aviator 5 --- gts/src/schema_resolver.rs | 6 ++-- gts/src/store.rs | 74 ++++---------------------------------- 2 files changed, 10 insertions(+), 70 deletions(-) diff --git a/gts/src/schema_resolver.rs b/gts/src/schema_resolver.rs index 70ea165..1641784 100644 --- a/gts/src/schema_resolver.rs +++ b/gts/src/schema_resolver.rs @@ -38,8 +38,8 @@ impl<'a> SchemaResolver<'a> { /// Best-effort `$ref` resolution: resolvable `gts://` `$ref`s are replaced /// with the referenced schema content; external refs that cannot be - /// resolved are preserved in the returned value rather than removed. Use - /// [`Self::try_resolve`] when unresolved refs must be treated as an error. + /// resolved are preserved in the returned value rather than removed. + #[allow(dead_code)] pub(crate) fn resolve(&self, schema: &Value) -> Value { let mut visited = std::collections::HashSet::new(); let mut cycle_found = false; @@ -47,7 +47,7 @@ impl<'a> SchemaResolver<'a> { self.resolve_inner(schema, &mut visited, &mut cycle_found, &mut unresolved_refs) } - /// Like [`Self::resolve`] but returns an error if any external `$ref` + /// Strict `$ref` resolution that returns an error if any external `$ref` /// cannot be resolved or a circular `$ref` is detected. /// /// Uses DFS-path cycle detection: a `$ref` target is held in the seen-set diff --git a/gts/src/store.rs b/gts/src/store.rs index d3a2fba..df30717 100644 --- a/gts/src/store.rs +++ b/gts/src/store.rs @@ -1,71 +1,12 @@ use serde::{Deserialize, Serialize}; use serde_json::Value; use std::collections::HashMap; -use std::sync::{Arc, RwLock}; use thiserror::Error; use crate::entities::GtsEntity; -use crate::gts::{GTS_URI_PREFIX, GtsId, GtsIdError, GtsIdPattern}; +use crate::gts::{GtsId, GtsIdError, GtsIdPattern}; use crate::schema_cast::GtsEntityCastResult; -/// Custom retriever for resolving gts:// URI scheme references in JSON Schema validation -struct GtsRetriever { - store: Arc>>, -} - -impl GtsRetriever { - fn new(store_map: &HashMap) -> Self { - let mut schemas = HashMap::new(); - - // Pre-populate with all schemas from the store - for (id, entity) in store_map { - if entity.is_schema { - // Store with gts:// URI format - let uri = format!("{GTS_URI_PREFIX}{id}"); - schemas.insert(uri, entity.content.clone()); - } - } - - Self { - store: Arc::new(RwLock::new(schemas)), - } - } -} - -impl jsonschema::Retrieve for GtsRetriever { - #[allow(clippy::cognitive_complexity)] - fn retrieve( - &self, - uri: &jsonschema::Uri, - ) -> Result> { - let uri_str = uri.as_str(); - - tracing::debug!("GtsRetriever: Attempting to retrieve URI: {uri_str}"); - - // Only handle gts:// URIs - if !uri_str.starts_with(GTS_URI_PREFIX) { - tracing::warn!("GtsRetriever: Unknown scheme for URI: {uri_str}"); - return Err(format!("Unknown scheme for URI: {uri_str}").into()); - } - - let store = self.store.read().map_err(|e| format!("Lock error: {e}"))?; - - tracing::debug!("GtsRetriever: Store contains {} schemas", store.len()); - - if let Some(schema) = store.get(uri_str) { - tracing::debug!("GtsRetriever: Successfully retrieved schema for {uri_str}"); - Ok(schema.clone()) - } else { - tracing::warn!("GtsRetriever: Schema not found: {uri_str}"); - tracing::debug!( - "GtsRetriever: Available URIs: {:?}", - store.keys().collect::>() - ); - Err(format!("Schema not found: {uri_str}").into()) - } - } -} - #[derive(Debug, Error)] pub enum StoreError { #[error("GTS instance with ID '{0}' not found in store")] @@ -306,12 +247,13 @@ impl GtsStore { /// `$ref`s are inlined, unresolved external refs are left intact. See /// [`crate::schema_resolver::SchemaResolver`]. #[must_use] - pub fn resolve_schema_refs(&self, schema: &Value) -> Value { + #[allow(dead_code)] + pub(crate) fn resolve_schema_refs(&self, schema: &Value) -> Value { crate::schema_resolver::SchemaResolver::new(self).resolve(schema) } - /// Like [`Self::resolve_schema_refs`] but errors on an unresolved external - /// `$ref` or a circular `$ref`. + /// Strict `$ref` resolution that errors on an unresolved external `$ref` or + /// a circular `$ref`. /// /// # Errors /// [`StoreError::UnresolvedRefs`] or [`StoreError::CircularRef`]. @@ -773,12 +715,10 @@ impl GtsStore { .try_resolve_schema_refs(&content) .map_err(|e| StoreError::ValidationError(format!("Schema '{type_id}' has {e}")))?; - // Strip x-gts-ref before compiling (unknown keyword to jsonschema); keep - // a retriever for any residual gts:// refs, mirroring validate_instance. + // Strip x-gts-ref before compiling; try_resolve_schema_refs has already + // inlined all resolvable external gts:// refs or returned an error. let schema_for_validation = Self::remove_x_gts_ref_fields(&resolved_schema); - let retriever = GtsRetriever::new(&self.by_id); let validator = jsonschema::options() - .with_retriever(retriever) .build(&schema_for_validation) .map_err(|e| { StoreError::ValidationError(format!("Invalid schema for '{type_id}': {e}")) From 744269b81e8a04871b366bb599fb58099f9498cf Mon Sep 17 00:00:00 2001 From: Aviator 5 Date: Wed, 24 Jun 2026 14:24:04 +0300 Subject: [PATCH 2/7] fix(gts): align entity validation with schema resolution - Replace the lenient schema reference resolver surface with strict resolution that reports unresolved refs and cycles. - Let validate-entity rely on schema validation for trait completeness instead of enforcing additional closedness policy. Signed-off-by: Aviator 5 --- gts-macros/tests/inheritance_tests.rs | 4 +- gts-macros/tests/integration_tests.rs | 13 +- gts/src/ops.rs | 235 +++++--------------------- gts/src/schema_refs.rs | 4 +- gts/src/schema_resolver.rs | 15 +- gts/src/schema_resolver_test.rs | 72 +++----- gts/src/schema_traits.rs | 5 +- gts/src/store.rs | 33 ++-- gts/src/store_test.rs | 36 +--- 9 files changed, 93 insertions(+), 324 deletions(-) diff --git a/gts-macros/tests/inheritance_tests.rs b/gts-macros/tests/inheritance_tests.rs index c3e8b0c..abcaea0 100644 --- a/gts-macros/tests/inheritance_tests.rs +++ b/gts-macros/tests/inheritance_tests.rs @@ -448,7 +448,9 @@ mod tests { .register_schema(BaseEventV1::<()>::gts_type_id().as_ref(), &base_schema) .unwrap(); - let base_inline = store.resolve_schema_refs(&BaseEventV1::<()>::gts_schema_with_refs()); + let base_inline = store + .resolve_schema_refs(&BaseEventV1::<()>::gts_schema_with_refs()) + .expect("base schema refs should resolve"); // Base has no $refs to resolve, so inline should be same as with_refs assert!( diff --git a/gts-macros/tests/integration_tests.rs b/gts-macros/tests/integration_tests.rs index a99f4b4..1d2e2f8 100644 --- a/gts-macros/tests/integration_tests.rs +++ b/gts-macros/tests/integration_tests.rs @@ -960,8 +960,9 @@ fn test_schema_inline_inheritance_with_parent() { .unwrap(); // Base type can use inline resolution - let inlined = - store.resolve_schema_refs(&inheritance_tests::BaseEventV1::<()>::gts_schema_with_refs()); + let inlined = store + .resolve_schema_refs(&inheritance_tests::BaseEventV1::<()>::gts_schema_with_refs()) + .expect("base schema refs should resolve"); assert!( inlined.get("properties").is_some(), "Inlined schema should have properties" @@ -990,7 +991,9 @@ fn test_runtime_schema_inline_resolution() { .unwrap(); // Generate the inlined schema using runtime resolution (only for base type) - let inlined = store.resolve_schema_refs(&base_schema); + let inlined = store + .resolve_schema_refs(&base_schema) + .expect("base schema refs should resolve"); let inlined_str = inlined.to_string(); // Verify that no external $ref references remain (only internal schema refs should remain) @@ -1058,7 +1061,9 @@ fn test_runtime_schema_inline_resolution_single_segment() { .unwrap(); // Generate the inlined schema - let inlined = store.resolve_schema_refs(&EventTopicV1::gts_schema_with_refs()); + let inlined = store + .resolve_schema_refs(&EventTopicV1::gts_schema_with_refs()) + .expect("event topic schema refs should resolve"); // For single-segment schemas, the result should be essentially the same assert_eq!(inlined["$id"], "gts://gts.x.core.events.topic.v1~"); diff --git a/gts/src/ops.rs b/gts/src/ops.rs index 4346a81..fcb23d3 100644 --- a/gts/src/ops.rs +++ b/gts/src/ops.rs @@ -652,98 +652,6 @@ impl GtsOps { } } - /// OP#13 **entity-level** check for `/validate-entity`, stricter than the - /// `/validate-type-schema` conformance run (`GtsStore::validate_schema`). - /// A GTS Type Schema is a valid standalone entity only if, on top of trait - /// conformance: - /// - /// 1. every `x-gts-traits-schema` in the chain is **closed** - /// (`additionalProperties: false`) — an open trait surface means the type - /// is designed to be extended, not deployed standalone; and - /// 2. if any trait-schema is declared, concrete `x-gts-traits` values are - /// actually provided somewhere in the chain. - /// - /// Abstract types are exempt (templates — descendants close the surface). - /// This delta is pinned by the gts-spec conformance suite (the - /// `/validate-entity`-only failure cases); the store stays the trait - /// resolution/conformance engine and this entity policy lives in ops. - /// - /// # Errors - /// Returns the human-readable reason the type is not a valid standalone - /// entity (or a trait-resolution error from the store). - fn check_entity_traits(&mut self, gts_id: &str) -> Result<(), String> { - // Abstract types are templates — exempt from the standalone-entity check. - if self - .store - .get(gts_id) - .is_some_and(|e| GtsStore::content_is_abstract(&e.content)) - { - return Ok(()); - } - - let traits = self - .store - .effective_traits(gts_id) - .map_err(|e| e.to_string())?; - - if traits.resolved_trait_schemas.is_empty() { - return Ok(()); - } - - // If trait schemas exist but no trait values are provided, the entity - // is incomplete. - if traits - .merged_traits - .as_object() - .is_none_or(serde_json::Map::is_empty) - { - return Err( - "Entity defines x-gts-traits-schema but no x-gts-traits values are provided" - .to_owned(), - ); - } - - // Each trait schema must be closed: `additionalProperties: false` at the - // top level or within any `allOf` branch. A boolean schema is not closed - // and must be rejected, not silently skipped by an `as_object()` guard. - for ts in &traits.resolved_trait_schemas { - if !Self::trait_schema_is_closed(ts) { - return Err("Entity trait schema must set additionalProperties: false \ - to be a valid standalone entity" - .to_owned()); - } - } - - Ok(()) - } - - /// `true` when a resolved trait schema closes its surface with - /// `additionalProperties: false` — at the top level or in any `allOf` branch - /// (the resolver preserves `allOf`, so closedness reached via - /// `allOf: [{$ref closed_type}]` lives inside a branch). - fn trait_schema_is_closed(schema: &Value) -> bool { - Self::trait_schema_is_closed_rec(schema, 0) - } - - fn trait_schema_is_closed_rec(schema: &Value, depth: usize) -> bool { - // Bound recursion to guard against a pathological `allOf` nesting. - const MAX_DEPTH: usize = 64; - if depth >= MAX_DEPTH { - return false; - } - let Some(obj) = schema.as_object() else { - return false; - }; - if obj.get("additionalProperties") == Some(&Value::Bool(false)) { - return true; - } - matches!( - obj.get("allOf"), - Some(Value::Array(branches)) - if branches.iter().any(|b| Self::trait_schema_is_closed_rec(b, depth + 1)) - ) - } - pub fn validate_entity(&mut self, gts_id: &str) -> GtsEntityValidationResult { if gts_id.ends_with('~') { // Validate GTS extension keywords (x-gts-final/x-gts-abstract format @@ -769,18 +677,6 @@ impl GtsOps { }; } - // Beyond type-schema validation, a standalone entity must have a - // closed, resolved trait surface (pinned by the conformance suite's - // /validate-entity cases). - if let Err(e) = self.check_entity_traits(gts_id) { - return GtsEntityValidationResult { - id: gts_id.to_owned(), - ok: false, - entity_type: "schema".to_owned(), - error: e, - }; - } - GtsEntityValidationResult { id: result.id, ok: true, @@ -3734,11 +3630,10 @@ mod tests { } #[test] - fn test_op13_entity_traits_abstract_base_skips_completeness() { - // gts-spec §9.7.5 / §9.11.4 (ADR-0003): a type marked `x-gts-abstract: true` - // is exempt from the entity-level check. It may declare an - // `x-gts-traits-schema` without resolving any `x-gts-traits` values — - // concrete descendants close the required traits. + fn test_validate_entity_accepts_abstract_base_with_unresolved_required_trait() { + // gts-spec §9.7.5 / §9.11.4 (ADR-0003): a type marked + // `x-gts-abstract: true` is exempt from the required-trait completeness + // check. `/validate-entity` uses the same schema validation pipeline. let mut ops = GtsOps::new(None, None, 0); ops.store .register_schema( @@ -3750,24 +3645,25 @@ mod tests { "x-gts-abstract": true, "x-gts-traits-schema": { "type": "object", - "additionalProperties": false, - "properties": {"topicRef": {"type": "string"}} + "properties": {"topicRef": {"type": "string"}}, + "required": ["topicRef"] }, "properties": {"id": {"type": "string"}} }), ) .expect("register abstract base"); + let result = ops.validate_entity("gts.x.test13.abs.base.v1~"); assert!( - ops.check_entity_traits("gts.x.test13.abs.base.v1~").is_ok(), - "Abstract base must be exempt from the entity-level check" + result.ok, + "abstract base must defer completeness: {result:?}" ); } #[test] - fn test_op13_entity_traits_non_abstract_base_without_values_fails() { - // A non-abstract type that declares a trait schema but supplies no - // `x-gts-traits` values anywhere in its chain is not a standalone entity. + fn test_validate_entity_accepts_optional_trait_schema_without_values() { + // OP#13 completeness is about required properties in the effective + // trait-schema, not about requiring any `x-gts-traits` object to exist. let mut ops = GtsOps::new(None, None, 0); ops.store .register_schema( @@ -3778,7 +3674,6 @@ mod tests { "type": "object", "x-gts-traits-schema": { "type": "object", - "additionalProperties": false, "properties": {"topicRef": {"type": "string"}} }, "properties": {"id": {"type": "string"}} @@ -3786,18 +3681,17 @@ mod tests { ) .expect("register concrete base"); + let result = ops.validate_entity("gts.x.test13.conc.base.v1~"); assert!( - ops.check_entity_traits("gts.x.test13.conc.base.v1~") - .is_err(), - "Non-abstract base with no trait values must fail the entity check" + result.ok, + "optional trait schema without values must be valid: {result:?}" ); } #[test] - fn test_op13_entity_traits_open_schema_not_standalone() { - // A trait schema that is not closed (no `additionalProperties: false`) - // signals the type is designed to be extended, not deployed standalone — - // even when concrete trait values are supplied. + fn test_validate_entity_accepts_open_trait_schema_with_values() { + // `x-gts-traits-schema` is an ordinary JSON Schema subschema. The spec + // does not require `additionalProperties: false` for `/validate-entity`. let mut ops = GtsOps::new(None, None, 0); ops.store .register_schema( @@ -3816,63 +3710,17 @@ mod tests { ) .expect("register open base"); + let result = ops.validate_entity("gts.x.test13.open.base.v1~"); assert!( - ops.check_entity_traits("gts.x.test13.open.base.v1~") - .is_err(), - "Open trait schema (no additionalProperties:false) is not a valid standalone entity" - ); - } - - #[test] - fn test_op13_entity_traits_closedness_via_referenced_schema() { - // The closed trait surface comes through an allOf + $ref to another GTS - // type whose body carries `additionalProperties: false`. The resolver - // preserves the `allOf` (inlining the `$ref` in place), so the - // standalone-entity closedness check must see through `allOf` branches - // rather than expecting a flattened top-level `additionalProperties`. - let mut ops = GtsOps::new(None, None, 0); - ops.store - .register_schema( - "gts.x.test13.refclosed.traitdef.v1~", - &json!({ - "$id": "gts://gts.x.test13.refclosed.traitdef.v1~", - "$schema": "http://json-schema.org/draft-07/schema#", - "type": "object", - "properties": {"topicRef": {"type": "string"}}, - "additionalProperties": false - }), - ) - .expect("register trait def"); - ops.store - .register_schema( - "gts.x.test13.refclosed.base.v1~", - &json!({ - "$id": "gts://gts.x.test13.refclosed.base.v1~", - "$schema": "http://json-schema.org/draft-07/schema#", - "type": "object", - "x-gts-traits-schema": { - "type": "object", - "allOf": [{"$ref": "gts://gts.x.test13.refclosed.traitdef.v1~"}] - }, - "x-gts-traits": {"topicRef": "events"}, - "properties": {"id": {"type": "string"}} - }), - ) - .expect("register base"); - - assert!( - ops.check_entity_traits("gts.x.test13.refclosed.base.v1~") - .is_ok(), - "closed trait schema reached via allOf + $ref must be accepted" + result.ok, + "open trait schema with conforming values must be valid: {result:?}" ); } #[test] - fn test_op13_entity_traits_boolean_schema_rejected() { - // A boolean `x-gts-traits-schema` (here `true`) carries no - // `additionalProperties: false`, so it is not closed and must be - // rejected — not silently skipped by the `as_object()` guard in - // `trait_schema_is_closed_rec`. + fn test_validate_entity_accepts_boolean_trait_schema_true() { + // ADR-0002 explicitly admits boolean subschemas. `true` permits + // arbitrary trait values. let mut ops = GtsOps::new(None, None, 0); ops.store .register_schema( @@ -3888,48 +3736,43 @@ mod tests { ) .expect("register boolean-trait base"); - let err = ops - .check_entity_traits("gts.x.test13.boolean.base.v1~") - .expect_err("a boolean trait schema is not closed and must be rejected"); + let result = ops.validate_entity("gts.x.test13.boolean.base.v1~"); assert!( - err.contains("additionalProperties"), - "error must explain the closedness requirement, got: {err}" + result.ok, + "boolean `true` trait schema must be valid: {result:?}" ); } #[test] - fn test_validate_entity_reports_trait_failure() { - // The public `validate_entity` path must surface a `check_entity_traits` - // failure as `ok: false` with the explanatory error — not just return - // `ok: true` for the happy path. An open trait schema (no - // `additionalProperties: false`) with concrete trait values passes - // type-schema validation but is not a valid standalone entity. + fn test_validate_entity_reports_missing_required_trait_failure() { + // `/validate-entity` still surfaces OP#13 failures from validate_schema: + // non-abstract types must resolve required traits. let mut ops = GtsOps::new(None, None, 0); ops.store .register_schema( - "gts.x.test13.validate.open.v1~", + "gts.x.test13.validate.required.v1~", &json!({ - "$id": "gts://gts.x.test13.validate.open.v1~", + "$id": "gts://gts.x.test13.validate.required.v1~", "$schema": "http://json-schema.org/draft-07/schema#", "type": "object", "x-gts-traits-schema": { "type": "object", - "properties": {"topicRef": {"type": "string"}} + "properties": {"topicRef": {"type": "string"}}, + "required": ["topicRef"] }, - "x-gts-traits": {"topicRef": "events"}, "properties": {"id": {"type": "string"}} }), ) - .expect("register open base"); + .expect("register base with required trait"); - let result = ops.validate_entity("gts.x.test13.validate.open.v1~"); + let result = ops.validate_entity("gts.x.test13.validate.required.v1~"); assert!( !result.ok, - "an open trait schema must fail validate_entity, got ok=true" + "missing required trait must fail validate_entity, got ok=true" ); assert!( - result.error.contains("additionalProperties"), - "validate_entity must surface the closedness error, got: {}", + result.error.contains("trait validation failed"), + "validate_entity must surface the OP#13 error, got: {}", result.error ); } diff --git a/gts/src/schema_refs.rs b/gts/src/schema_refs.rs index 354de41..934ede9 100644 --- a/gts/src/schema_refs.rs +++ b/gts/src/schema_refs.rs @@ -64,8 +64,8 @@ fn classify_ref(ref_uri: &str) -> Result, InvalidRefReason> { // A GTS `$ref` may carry a JSON Pointer fragment selecting a sub-schema of // the target document (e.g. `gts://...~#/x-gts-traits-schema`). Validate the // id portion as a type id and require any fragment to be empty or a - // `/`-prefixed JSON Pointer - the exact shapes `resolve_schema_refs_inner` - // is able to dereference. + // `/`-prefixed JSON Pointer - the exact shapes `SchemaResolver` is able to + // dereference. let (id, fragment) = match rest.split_once('#') { Some((id, frag)) => (id, Some(frag)), None => (rest, None), diff --git a/gts/src/schema_resolver.rs b/gts/src/schema_resolver.rs index 1641784..8f75b96 100644 --- a/gts/src/schema_resolver.rs +++ b/gts/src/schema_resolver.rs @@ -7,7 +7,7 @@ //! //! [`SchemaResolver`] depends only on the narrow [`SchemaProvider`] lookup, not //! on `GtsStore` directly; the store implements `SchemaProvider` and exposes -//! `resolve_schema_refs`/`try_resolve_schema_refs` as thin wrappers. +//! `try_resolve_schema_refs` as a thin wrapper. use serde_json::Value; @@ -36,17 +36,6 @@ impl<'a> SchemaResolver<'a> { Self { provider } } - /// Best-effort `$ref` resolution: resolvable `gts://` `$ref`s are replaced - /// with the referenced schema content; external refs that cannot be - /// resolved are preserved in the returned value rather than removed. - #[allow(dead_code)] - pub(crate) fn resolve(&self, schema: &Value) -> Value { - let mut visited = std::collections::HashSet::new(); - let mut cycle_found = false; - let mut unresolved_refs = Vec::new(); - self.resolve_inner(schema, &mut visited, &mut cycle_found, &mut unresolved_refs) - } - /// Strict `$ref` resolution that returns an error if any external `$ref` /// cannot be resolved or a circular `$ref` is detected. /// @@ -61,7 +50,7 @@ impl<'a> SchemaResolver<'a> { /// Returns [`StoreError::UnresolvedRefs`] if any external `$ref` cannot be /// resolved, or [`StoreError::CircularRef`] if a circular `$ref` is /// detected. - pub(crate) fn try_resolve(&self, schema: &Value) -> Result { + pub(crate) fn resolve(&self, schema: &Value) -> Result { let mut visited = std::collections::HashSet::new(); let mut cycle_found = false; let mut unresolved_refs = Vec::new(); diff --git a/gts/src/schema_resolver_test.rs b/gts/src/schema_resolver_test.rs index 8cd97e9..7529c3b 100644 --- a/gts/src/schema_resolver_test.rs +++ b/gts/src/schema_resolver_test.rs @@ -1,8 +1,8 @@ //! Unit tests for [`SchemaResolver`]. They drive the resolver directly against //! a tiny in-memory [`SchemaProvider`] mock (`MapProvider`) — no `GtsStore` -//! involved — so they exercise `SchemaResolver::resolve` / `try_resolve` in -//! isolation. End-to-end coverage of the `GtsStore` wrappers and provider -//! lookup semantics lives in `store_test.rs`. +//! involved — so they exercise `SchemaResolver::try_resolve` in isolation. +//! End-to-end coverage of the `GtsStore` wrapper and provider lookup semantics +//! lives in `store_test.rs`. use std::collections::HashMap; @@ -37,12 +37,9 @@ impl SchemaProvider for MapProvider { // By-value `json!(...)` literals read cleaner at the call sites. #[allow(clippy::needless_pass_by_value)] fn resolve(provider: &MapProvider, schema: Value) -> Value { - SchemaResolver::new(provider).resolve(&schema) -} - -#[allow(clippy::needless_pass_by_value)] -fn try_resolve(provider: &MapProvider, schema: Value) -> Result { - SchemaResolver::new(provider).try_resolve(&schema) + SchemaResolver::new(provider) + .resolve(&schema) + .expect("schema should resolve") } #[test] @@ -458,34 +455,10 @@ fn test_resolve_ref_with_object_siblings_composes_into_allof() { } #[test] -fn test_resolve_keeps_unresolved_bare_ref() { +fn test_resolve_errors_on_unresolved_ref() { let p = MapProvider::new(); - let schema = json!({"$ref": "gts://gts.x.core.events.missing.v1~"}); - assert_eq!(resolve(&p, schema.clone()), schema); -} - -#[test] -fn test_resolve_keeps_unresolved_ref_with_siblings() { - let p = MapProvider::new(); - let schema = json!({ - "type": "object", - "properties": { - "event": { - "$ref": "gts://gts.x.core.events.missing.v1~", - "description": "missing dependency must not be dropped" - } - } - }); - let resolved = resolve(&p, schema.clone()); - assert_eq!(resolved, schema); -} - -#[test] -fn test_try_resolve_errors_on_unresolved_ref() { - let p = MapProvider::new(); - let err = try_resolve( - &p, - json!({ + let err = SchemaResolver::new(&p) + .resolve(&json!({ "type": "object", "properties": { "event": { @@ -493,9 +466,8 @@ fn test_try_resolve_errors_on_unresolved_ref() { "description": "strict mode should reject this" } } - }), - ) - .expect_err("missing external ref should fail checked resolution"); + })) + .expect_err("missing external ref should fail checked resolution"); assert!(matches!( &err, @@ -505,7 +477,7 @@ fn test_try_resolve_errors_on_unresolved_ref() { } #[test] -fn test_try_resolve_allows_duplicate_ref_in_allof() { +fn test_resolve_allows_duplicate_ref_in_allof() { // Redundant manual aggregation (the same $ref appearing more than once in an // allOf composition) uses DFS-path cycle detection, so independent duplicate // $refs are not flagged as cycles. @@ -527,7 +499,7 @@ fn test_try_resolve_allows_duplicate_ref_in_allof() { ] }); - assert!(try_resolve(&p, schema.clone()).is_ok()); + assert!(SchemaResolver::new(&p).resolve(&schema.clone()).is_ok()); assert!(resolve(&p, schema).is_object()); } @@ -546,22 +518,19 @@ fn test_resolve_pointer_to_boolean_with_siblings() { }), ); - let resolved = try_resolve( - &p, - json!({ + let resolved = SchemaResolver::new(&p) + .resolve(&json!({ "$ref": "gts://gts.x.core.events.flag.v1~#/$defs/closed", "description": "extra" - }), - ) - .expect("resolved non-object ref with siblings must not be reported unresolved"); + })) + .expect("resolved non-object ref with siblings must not be reported unresolved"); assert_eq!(resolved, json!(false)); } #[test] fn test_resolve_circular_ref_does_not_hang() { - // A refs B, B refs A. Lenient resolve must terminate; checked resolution - // reports the cycle. + // A refs B, B refs A. Strict resolution must terminate and report the cycle. let p = MapProvider::new() .with( "gts.x.test.circ.a.v1~", @@ -591,9 +560,10 @@ fn test_resolve_circular_ref_does_not_hang() { "properties": {"id": {"type": "string"}} }); - assert!(resolve(&p, schema.clone()).is_object()); assert!(matches!( - try_resolve(&p, schema).expect_err("circular ref must fail checked resolution"), + SchemaResolver::new(&p) + .resolve(&schema) + .expect_err("circular ref must fail checked resolution"), StoreError::CircularRef )); } diff --git a/gts/src/schema_traits.rs b/gts/src/schema_traits.rs index ae933db..eb88ba0 100644 --- a/gts/src/schema_traits.rs +++ b/gts/src/schema_traits.rs @@ -395,9 +395,8 @@ fn effective_schema_is_false_recursive(schema: &Value, depth: usize) -> bool { /// /// Only pointers that actually resolve against `root` are inlined; anything /// else (notably `gts://` refs and the synthetic `#/$defs/GtsInstanceId` family -/// that the macro emits and [`crate::store::GtsStore::resolve_schema_refs`] -/// special-cases) is left untouched. Recursion is bounded by -/// [`MAX_RECURSION_DEPTH`]. +/// that schema resolution special-cases) is left untouched. Recursion is +/// bounded by [`MAX_RECURSION_DEPTH`]. pub(crate) fn inline_local_pointers(fragment: &Value, root: &Value) -> Value { inline_local_pointers_recursive(fragment, root, 0) } diff --git a/gts/src/store.rs b/gts/src/store.rs index df30717..607cff5 100644 --- a/gts/src/store.rs +++ b/gts/src/store.rs @@ -243,22 +243,13 @@ impl GtsStore { self.by_id.iter() } - /// Best-effort `$ref` resolution for a JSON Schema: resolvable `gts://` - /// `$ref`s are inlined, unresolved external refs are left intact. See - /// [`crate::schema_resolver::SchemaResolver`]. - #[must_use] - #[allow(dead_code)] - pub(crate) fn resolve_schema_refs(&self, schema: &Value) -> Value { - crate::schema_resolver::SchemaResolver::new(self).resolve(schema) - } - /// Strict `$ref` resolution that errors on an unresolved external `$ref` or /// a circular `$ref`. /// /// # Errors /// [`StoreError::UnresolvedRefs`] or [`StoreError::CircularRef`]. - pub fn try_resolve_schema_refs(&self, schema: &Value) -> Result { - crate::schema_resolver::SchemaResolver::new(self).try_resolve(schema) + pub fn resolve_schema_refs(&self, schema: &Value) -> Result { + crate::schema_resolver::SchemaResolver::new(self).resolve(schema) } fn remove_x_gts_ref_fields(schema: &Value) -> Value { @@ -459,13 +450,11 @@ impl GtsStore { })?; let base_resolved = self - .try_resolve_schema_refs(&base_content) + .resolve_schema_refs(&base_content) .map_err(|e| StoreError::ValidationError(format!("Schema '{base_id}' has {e}")))?; - let derived_resolved = self - .try_resolve_schema_refs(&derived_content) - .map_err(|e| { - StoreError::ValidationError(format!("Schema '{derived_id}' has {e}")) - })?; + let derived_resolved = self.resolve_schema_refs(&derived_content).map_err(|e| { + StoreError::ValidationError(format!("Schema '{derived_id}' has {e}")) + })?; // Extract effective schemas and compare via schema_compat module let base_eff = crate::schema_compat::extract_effective_schema(&base_resolved); @@ -514,8 +503,8 @@ impl GtsStore { /// `type_id` by walking its `$id` chain (root → leaf). /// /// Collects `x-gts-traits-schema` subschemas and `x-gts-traits` values from - /// each level's **raw** content (before `resolve_schema_refs` flattens - /// `allOf` and drops the `x-gts-*` extension keys), inlines JSON Pointer + /// each level's **raw** content (before `$ref` resolution inlines external + /// schemas and drops the `x-gts-*` extension keys), inlines JSON Pointer /// `$ref`s against their host document, resolves any `gts://` `$ref`s inside /// the collected subschemas, RFC 7396-merges the values (descendant /// last-wins for scalars/arrays, recursive merge for objects, `null` deletes @@ -573,7 +562,7 @@ impl GtsStore { let mut resolved_trait_schemas: Vec = Vec::with_capacity(trait_schemas.len()); for ts in &trait_schemas { - let resolved = self.try_resolve_schema_refs(ts).map_err(|e| { + let resolved = self.resolve_schema_refs(ts).map_err(|e| { StoreError::ValidationError(format!("Schema '{type_id}' trait schema has {e}")) })?; resolved_trait_schemas.push(resolved); @@ -649,7 +638,7 @@ impl GtsStore { // Resolve schema references let resolved_schema = self - .try_resolve_schema_refs(&content) + .resolve_schema_refs(&content) .map_err(|e| StoreError::ValidationError(format!("Schema '{type_id}' has {e}")))?; // Meta-validate the fully-resolved schema. Registration only checks @@ -712,7 +701,7 @@ impl GtsStore { // schema-level metadata (§9.7) and never appear in instances, so the // effective-traits build is deliberately skipped here. let resolved_schema = self - .try_resolve_schema_refs(&content) + .resolve_schema_refs(&content) .map_err(|e| StoreError::ValidationError(format!("Schema '{type_id}' has {e}")))?; // Strip x-gts-ref before compiling; try_resolve_schema_refs has already diff --git a/gts/src/store_test.rs b/gts/src/store_test.rs index e5ba2f4..c41c059 100644 --- a/gts/src/store_test.rs +++ b/gts/src/store_test.rs @@ -5413,39 +5413,17 @@ fn test_validate_and_resolve_accepts_well_formed_gts_ref_schema() { } // --------------------------------------------------------------------------- -// `GtsStore` `$ref`-resolution wrappers (`resolve_schema_refs` / -// `try_resolve_schema_refs`) and the store-as-`SchemaProvider` integration. +// `GtsStore` `$ref`-resolution wrapper (`try_resolve_schema_refs`) and the +// store-as-`SchemaProvider` integration. // Resolver semantics themselves are unit-tested in `schema_resolver_test.rs`; // these are smoke/integration tests for the store-level surface. // --------------------------------------------------------------------------- #[test] fn test_resolve_schema_refs_wrapper_smoke() { - let mut store = GtsStore::new(); - store - .register_schema( - "gts.x.core.events.type.v1~", - &json!({ - "$id": "gts://gts.x.core.events.type.v1~", - "$schema": "http://json-schema.org/draft-07/schema#", - "type": "object", - "properties": {"id": {"type": "string"}} - }), - ) - .expect("register target"); - - let resolved = store.resolve_schema_refs(&json!({"$ref": "gts://gts.x.core.events.type.v1~"})); - assert_eq!( - resolved, - json!({"type": "object", "properties": {"id": {"type": "string"}}}) - ); -} - -#[test] -fn test_try_resolve_schema_refs_wrapper_smoke() { let store = GtsStore::new(); let err = store - .try_resolve_schema_refs(&json!({"$ref": "gts://gts.x.core.events.missing.v1~"})) + .resolve_schema_refs(&json!({"$ref": "gts://gts.x.core.events.missing.v1~"})) .expect_err("unresolved external ref must fail checked resolution"); assert!(matches!( &err, @@ -5472,14 +5450,8 @@ fn test_resolve_schema_refs_uses_exact_gts_uri_lookup_without_minor_fallback() { .expect("register target schema"); let schema = json!({"$ref": "gts://gts.x.core.events.type.v1~"}); - assert_eq!( - store.resolve_schema_refs(&schema), - schema, - "v1~ must not resolve against a stored v1.0~ schema" - ); - let err = store - .try_resolve_schema_refs(&schema) + .resolve_schema_refs(&schema) .expect_err("checked resolution should reject the unresolved v1~ ref"); assert!(matches!( &err, From ddfbf08f32c94f6d23fc451709f7ee2cf63b860e Mon Sep 17 00:00:00 2001 From: Aviator 5 Date: Wed, 24 Jun 2026 17:28:57 +0300 Subject: [PATCH 3/7] fix(gts): align validation with parsed GTS IDs - Route entity validation through GtsId parsing before schema or instance checks. - Stop reporting type status for wildcard ID validation and parsing results. - Remove instance modifier rejection outside schema validation. Signed-off-by: Aviator 5 --- gts/src/ops.rs | 196 +++++------------------------------- gts/src/schema_modifiers.rs | 112 --------------------- 2 files changed, 27 insertions(+), 281 deletions(-) diff --git a/gts/src/ops.rs b/gts/src/ops.rs index fcb23d3..9e3fde1 100644 --- a/gts/src/ops.rs +++ b/gts/src/ops.rs @@ -12,8 +12,8 @@ use crate::schema_cast::GtsEntityCastResult; use crate::store::{GtsStore, GtsStoreQueryResult}; /// `is_schema` is `Some(true)` for schema/type IDs (ending with `~`), -/// `Some(false)` for instance IDs, and `None` when the input couldn't be -/// parsed (unknown). +/// `Some(false)` for instance IDs and wildcard patterns that match instances, +/// and `None` when the input couldn't be parsed (unknown). #[derive(Debug, Clone, Serialize, Deserialize)] pub struct GtsIdValidationResult { pub id: String, @@ -391,18 +391,6 @@ impl GtsOps { // Instance validation when requested. if validate && !entity.is_schema { - if let Err(e) = crate::schema_modifiers::validate_instance_modifiers(&entity.content) { - return GtsAddEntityResult { - ok: false, - id: String::new(), - type_id: None, - is_type_schema: entity.is_schema, - error: format!( - "Instance validation failed: {e}\n{}", - self.get_details(&entity) - ), - }; - } if let Err(e) = self.store.validate_instance(&entity_id) { return GtsAddEntityResult { ok: false, @@ -473,11 +461,11 @@ impl GtsOps { // - '*' must be at end (ending with '.*' or '~*') // - No '*' in the middle of segments match GtsIdPattern::try_new(gts_id) { - Ok(w) => GtsIdValidationResult { + Ok(_) => GtsIdValidationResult { id: gts_id.to_owned(), valid: true, error: String::new(), - is_type: Some(w.pattern().ends_with('~')), + is_type: Some(false), is_wildcard: true, }, Err(e) => GtsIdValidationResult { @@ -516,13 +504,12 @@ impl GtsOps { match GtsIdPattern::try_new(gts_id) { Ok(w) => { let segments = w.segments().iter().map(GtsIdSegmentInfo::from).collect(); - GtsIdParseResult { id: gts_id.to_owned(), ok: true, segments, error: String::new(), - is_type: Some(w.pattern().ends_with('~')), + is_type: Some(false), is_wildcard: true, } } @@ -653,57 +640,29 @@ impl GtsOps { } pub fn validate_entity(&mut self, gts_id: &str) -> GtsEntityValidationResult { - if gts_id.ends_with('~') { - // Validate GTS extension keywords (x-gts-final/x-gts-abstract format - // and mutual exclusion; x-gts-traits/x-gts-traits-schema placement). - if let Some(entity) = self.store.get(gts_id) - && let Err(e) = crate::schema_modifiers::validate_gts_keywords(&entity.content) - { - return GtsEntityValidationResult { - id: gts_id.to_owned(), - ok: false, - entity_type: "schema".to_owned(), - error: e, - }; - } - - let result = self.validate_schema(gts_id); - if !result.ok { - return GtsEntityValidationResult { - id: result.id, - ok: false, - entity_type: "schema".to_owned(), - error: result.error, - }; - } - - GtsEntityValidationResult { - id: result.id, - ok: true, - entity_type: "schema".to_owned(), - error: String::new(), - } + let parsed_id = GtsId::try_new(gts_id); + if parsed_id.is_err() { + return GtsEntityValidationResult { + id: gts_id.to_owned(), + ok: false, + error: parsed_id.unwrap_err().to_string(), + entity_type: String::new(), + }; + } + let result: GtsValidationResult; + let entity_type: String; + if parsed_id.unwrap().is_type() { + result = self.validate_schema(gts_id); + entity_type = "schema".to_owned(); } else { - // Check that schema-only keywords do not appear in instance content. - if let Some(entity) = self.store.get(gts_id) - && let Err(e) = - crate::schema_modifiers::validate_instance_modifiers(&entity.content) - { - return GtsEntityValidationResult { - id: gts_id.to_owned(), - ok: false, - entity_type: "instance".to_owned(), - error: e, - }; - } - - let result = self.validate_instance(gts_id); - GtsEntityValidationResult { - id: result.id, - ok: result.ok, - entity_type: "instance".to_owned(), - error: result.error, - } + result = self.validate_instance(gts_id); + entity_type = "instance".to_owned(); + } + GtsEntityValidationResult { + id: result.id, + ok: result.ok, + error: result.error, + entity_type, } } @@ -865,7 +824,6 @@ mod tests { fn test_validate_id_schema() { let result = GtsOps::validate_id("gts.vendor.package.namespace.type.v1.0~"); assert!(result.valid); - assert!(result.id.ends_with('~')); } #[test] @@ -3320,106 +3278,6 @@ mod tests { ); } - #[test] - fn test_add_entity_validate_rejects_instance_with_schema_only_keyword() { - // Registration with validate=true MUST reject an instance whose body - // contains schema-only keywords (x-gts-final / x-gts-abstract / - // x-gts-traits-schema / x-gts-traits). - let mut ops = GtsOps::new(None, None, 0); - - let schema = json!({ - "$id": "gts://gts.x.testkw.host.thing.v1~", - "$schema": "http://json-schema.org/draft-07/schema#", - "type": "object", - "properties": {"name": {"type": "string"}}, - "required": ["name"], - }); - assert!(ops.add_entity(&schema, false).ok); - - let bad_instance = json!({ - "id": "gts.x.testkw.host.thing.v1~x.testkw._.item.v1", - "name": "leak", - "x-gts-final": true, - }); - let result = ops.add_entity(&bad_instance, true); - assert!( - !result.ok, - "instance with schema-only keyword must be rejected" - ); - assert!( - result.error.contains("x-gts-final"), - "error should name the offending keyword, got: {}", - result.error - ); - } - - #[test] - fn test_add_entity_validate_rejects_instance_with_traits_keyword() { - // Per GTS spec § 9.7.1, x-gts-traits is a schema-only keyword and MUST - // be rejected when present in an instance body. - let mut ops = GtsOps::new(None, None, 0); - - let schema = json!({ - "$id": "gts://gts.x.testtrkw.host.thing.v1~", - "$schema": "http://json-schema.org/draft-07/schema#", - "type": "object", - "properties": {"name": {"type": "string"}}, - "required": ["name"], - }); - assert!(ops.add_entity(&schema, false).ok); - - let bad_instance = json!({ - "id": "gts.x.testtrkw.host.thing.v1~x.testtrkw._.item.v1", - "name": "leak", - "x-gts-traits": {"retention": "P30D"}, - }); - let result = ops.add_entity(&bad_instance, true); - assert!( - !result.ok, - "instance with x-gts-traits keyword must be rejected" - ); - assert!( - result.error.contains("x-gts-traits"), - "error should name the offending keyword, got: {}", - result.error - ); - } - - #[test] - fn test_add_entity_validate_rejects_instance_with_traits_schema_keyword() { - // Per GTS spec § 9.7.1, x-gts-traits-schema is a schema-only keyword - // and MUST be rejected when present in an instance body. - let mut ops = GtsOps::new(None, None, 0); - - let schema = json!({ - "$id": "gts://gts.x.testtskw.host.thing.v1~", - "$schema": "http://json-schema.org/draft-07/schema#", - "type": "object", - "properties": {"name": {"type": "string"}}, - "required": ["name"], - }); - assert!(ops.add_entity(&schema, false).ok); - - let bad_instance = json!({ - "id": "gts.x.testtskw.host.thing.v1~x.testtskw._.item.v1", - "name": "leak", - "x-gts-traits-schema": { - "type": "object", - "properties": {"retention": {"type": "string"}}, - }, - }); - let result = ops.add_entity(&bad_instance, true); - assert!( - !result.ok, - "instance with x-gts-traits-schema keyword must be rejected" - ); - assert!( - result.error.contains("x-gts-traits-schema"), - "error should name the offending keyword, got: {}", - result.error - ); - } - #[test] fn test_validate_id_with_wildcard_valid() { // Test wildcard validation for valid patterns diff --git a/gts/src/schema_modifiers.rs b/gts/src/schema_modifiers.rs index 46509ea..e9030e5 100644 --- a/gts/src/schema_modifiers.rs +++ b/gts/src/schema_modifiers.rs @@ -5,16 +5,6 @@ use crate::schema_traits::{X_GTS_TRAITS, X_GTS_TRAITS_SCHEMA}; pub const X_GTS_FINAL: &str = "x-gts-final"; pub const X_GTS_ABSTRACT: &str = "x-gts-abstract"; -/// All `x-gts-*` keywords that are valid only on JSON Schema documents and -/// MUST be rejected when found inside instance documents (GTS spec § 9.7.1, -/// § 9.11.1). -const SCHEMA_ONLY_KEYWORDS: &[&str] = &[ - X_GTS_FINAL, - X_GTS_ABSTRACT, - X_GTS_TRAITS_SCHEMA, - X_GTS_TRAITS, -]; - fn contains_key_recursive(value: &Value, key: &str) -> bool { match value { Value::Object(map) => { @@ -143,26 +133,6 @@ pub fn validate_gts_keywords(content: &Value) -> Result<(), String> { Ok(()) } -/// Check that schema-only keywords (`x-gts-final`, `x-gts-abstract`, -/// `x-gts-traits-schema`, `x-gts-traits`) do not appear anywhere in instance -/// content. Per GTS spec § 9.7.1 and § 9.11.1 these annotations are only -/// valid on JSON Schema documents and implementations MUST reject instances -/// that contain them. -/// -/// # Errors -/// Returns an error naming the first schema-only keyword found in the content -/// (top-level or nested). -pub fn validate_instance_modifiers(content: &Value) -> Result<(), String> { - for keyword in SCHEMA_ONLY_KEYWORDS { - if contains_key_recursive(content, keyword) { - return Err(format!( - "{keyword} is a schema-only keyword and must not appear in instances" - )); - } - } - Ok(()) -} - #[cfg(test)] #[allow(clippy::unwrap_used, clippy::expect_used)] mod tests { @@ -423,88 +393,6 @@ mod tests { ); } - // ========================================================================= - // validate_instance_modifiers unit tests - // ========================================================================= - - #[test] - fn test_instance_clean() { - assert!(validate_instance_modifiers(&json!({"id": "test", "name": "foo"})).is_ok()); - } - - #[test] - fn test_instance_has_final() { - let result = validate_instance_modifiers(&json!({"id": "test", "x-gts-final": true})); - assert!(result.is_err()); - assert!(result.unwrap_err().contains("x-gts-final")); - } - - #[test] - fn test_instance_has_abstract() { - let result = validate_instance_modifiers(&json!({"id": "test", "x-gts-abstract": true})); - assert!(result.is_err()); - assert!(result.unwrap_err().contains("x-gts-abstract")); - } - - #[test] - fn test_instance_nested_final_rejected() { - let result = validate_instance_modifiers(&json!({ - "id": "test", - "metadata": {"flags": {"x-gts-final": true}}, - })); - assert!(result.is_err()); - assert!(result.unwrap_err().contains("x-gts-final")); - } - - #[test] - fn test_instance_nested_abstract_in_array_rejected() { - let result = validate_instance_modifiers(&json!({ - "id": "test", - "items": [ - {"name": "ok"}, - {"name": "bad", "x-gts-abstract": true}, - ], - })); - assert!(result.is_err()); - assert!(result.unwrap_err().contains("x-gts-abstract")); - } - - #[test] - fn test_instance_has_traits_rejected() { - let result = validate_instance_modifiers(&json!({ - "id": "test", - "x-gts-traits": {"retention": "P30D"}, - })); - assert!(result.is_err()); - let msg = result.unwrap_err(); - assert!(msg.contains("x-gts-traits"), "msg: {msg}"); - assert!(msg.contains("schema-only"), "msg: {msg}"); - } - - #[test] - fn test_instance_has_traits_schema_rejected() { - let result = validate_instance_modifiers(&json!({ - "id": "test", - "x-gts-traits-schema": { - "type": "object", - "properties": {"retention": {"type": "string"}}, - }, - })); - assert!(result.is_err()); - let msg = result.unwrap_err(); - assert!(msg.contains("x-gts-traits-schema"), "msg: {msg}"); - } - - #[test] - fn test_instance_nested_traits_rejected() { - let result = validate_instance_modifiers(&json!({ - "id": "test", - "metadata": {"x-gts-traits": {"foo": "bar"}}, - })); - assert!(result.is_err()); - assert!(result.unwrap_err().contains("x-gts-traits")); - } - // ========================================================================= // Integration tests via store // ========================================================================= From 184ce11cf56a0e190ce5f69bbcc8342c3bdb5ee0 Mon Sep 17 00:00:00 2001 From: Aviator 5 Date: Thu, 25 Jun 2026 01:13:18 +0300 Subject: [PATCH 4/7] fix: validate closed descendant schema branches - Reject descendant schemas that set additionalProperties: false while orphaning ancestor properties under allOf composition. - Validate x-gts-traits-schema compatibility across ancestor prefixes so abstract schemas fail on structural conflicts even without values. - Add regression coverage for nested schema and trait-schema compatibility cases. Signed-off-by: Aviator 5 --- gts/src/ops.rs | 64 ++--- gts/src/schema_compat.rs | 455 ++++++++++++++++++++++---------- gts/src/schema_resolver_test.rs | 2 +- gts/src/schema_traits.rs | 311 ++++++++++++++++++++++ gts/src/store.rs | 8 +- gts/src/store_test.rs | 357 +++++++++++++++++++++++++ 6 files changed, 1013 insertions(+), 184 deletions(-) diff --git a/gts/src/ops.rs b/gts/src/ops.rs index 9e3fde1..9708bb6 100644 --- a/gts/src/ops.rs +++ b/gts/src/ops.rs @@ -390,19 +390,20 @@ impl GtsOps { } // Instance validation when requested. - if validate && !entity.is_schema { - if let Err(e) = self.store.validate_instance(&entity_id) { - return GtsAddEntityResult { - ok: false, - id: String::new(), - type_id: None, - is_type_schema: entity.is_schema, - error: format!( - "Instance validation failed: {e}\n{}", - self.get_details(&entity) - ), - }; - } + if validate + && !entity.is_schema + && let Err(e) = self.store.validate_instance(&entity_id) + { + return GtsAddEntityResult { + ok: false, + id: String::new(), + type_id: None, + is_type_schema: entity.is_schema, + error: format!( + "Instance validation failed: {e}\n{}", + self.get_details(&entity) + ), + }; } // println!("submitted: {}", self.get_content_pretty(&entity)); @@ -640,32 +641,31 @@ impl GtsOps { } pub fn validate_entity(&mut self, gts_id: &str) -> GtsEntityValidationResult { - let parsed_id = GtsId::try_new(gts_id); - if parsed_id.is_err() { - return GtsEntityValidationResult { - id: gts_id.to_owned(), - ok: false, - error: parsed_id.unwrap_err().to_string(), - entity_type: String::new(), - }; - } - let result: GtsValidationResult; - let entity_type: String; - if parsed_id.unwrap().is_type() { - result = self.validate_schema(gts_id); - entity_type = "schema".to_owned(); + let parsed_id = match GtsId::try_new(gts_id) { + Ok(parsed_id) => parsed_id, + Err(e) => { + return GtsEntityValidationResult { + id: gts_id.to_owned(), + ok: false, + entity_type: String::new(), + error: e.to_string(), + }; + } + }; + + let (result, entity_type) = if parsed_id.is_type() { + (self.validate_schema(gts_id), "schema".to_owned()) } else { - result = self.validate_instance(gts_id); - entity_type = "instance".to_owned(); - } + (self.validate_instance(gts_id), "instance".to_owned()) + }; + GtsEntityValidationResult { id: result.id, ok: result.ok, - error: result.error, entity_type, + error: result.error, } } - pub fn schema_graph(&mut self, gts_id: &str) -> GtsSchemaGraphResult { let graph = self.store.build_schema_graph(gts_id); GtsSchemaGraphResult { graph } diff --git a/gts/src/schema_compat.rs b/gts/src/schema_compat.rs index fb54ad7..044b403 100644 --- a/gts/src/schema_compat.rs +++ b/gts/src/schema_compat.rs @@ -13,6 +13,8 @@ use serde_json::Value; use std::collections::{HashMap, HashSet}; +const MAX_RECURSION_DEPTH: usize = 64; + /// Represents the effective (flattened) schema used for compatibility comparison. pub(crate) struct EffectiveSchema { pub properties: HashMap, @@ -91,7 +93,31 @@ pub(crate) fn extract_effective_schema(schema: &Value) -> EffectiveSchema { eff } -/// Validates that a derived schema is compatible with its base schema. +/// Validates that a derived JSON Schema value is compatible with its base. +/// +/// This is the high-level compatibility entry point. It combines the flattened +/// effective-schema comparison with raw-branch checks that need `allOf` +/// ownership information. +pub(crate) fn validate_schema_compatibility( + base_schema: &Value, + derived_schema: &Value, + base_id: &str, + derived_id: &str, +) -> Vec { + let base = extract_effective_schema(base_schema); + let derived = extract_effective_schema(derived_schema); + + let mut errors = validate_effective_schema_compatibility(&base, &derived, base_id, derived_id); + errors.extend(validate_closed_descendant_branches( + base_schema, + derived_schema, + base_id, + derived_id, + )); + errors +} + +/// Validates that a derived effective schema is compatible with its base. /// /// Rules checked: /// - Derived cannot add properties if base has `additionalProperties: false` @@ -104,9 +130,9 @@ pub(crate) fn extract_effective_schema(schema: &Value) -> EffectiveSchema { /// - Derived cannot remove fields from `required` /// - Derived cannot change array `items` type /// -/// Returns an empty `Vec` when the schemas are compatible, otherwise a list of -/// human-readable error descriptions. -pub(crate) fn validate_schema_compatibility( +/// Returns an empty `Vec` when the effective schemas are compatible, otherwise +/// a list of human-readable error descriptions. +pub(crate) fn validate_effective_schema_compatibility( base: &EffectiveSchema, derived: &EffectiveSchema, base_id: &str, @@ -167,6 +193,122 @@ pub(crate) fn validate_schema_compatibility( errors } +/// Validates branch-scoped `additionalProperties: false` in a descendant schema. +/// +/// Flattened compatibility catches closed ancestors that reject new descendant +/// properties, but it cannot see the inverse `allOf` hazard: a descendant +/// branch can set `additionalProperties: false` without restating an ancestor +/// property at the same object path, making that ancestor property unusable in +/// the composed schema. This walks the raw/resolved descendant branches so that +/// branch ownership is preserved. +pub(crate) fn validate_closed_descendant_branches( + ancestor_schema: &Value, + descendant_schema: &Value, + ancestor_label: &str, + descendant_label: &str, +) -> Vec { + let mut errors = Vec::new(); + collect_closed_descendant_branch_errors( + ancestor_schema, + descendant_schema, + "", + 0, + ancestor_label, + descendant_label, + &mut errors, + ); + errors +} + +fn collect_closed_descendant_branch_errors( + ancestor_schema: &Value, + descendant_schema: &Value, + path: &str, + depth: usize, + ancestor_label: &str, + descendant_label: &str, + errors: &mut Vec, +) { + if depth >= MAX_RECURSION_DEPTH { + return; + } + + let ancestor = extract_effective_schema(ancestor_schema); + let Some(descendant_obj) = descendant_schema.as_object() else { + return; + }; + let descendant_props = descendant_obj.get("properties").and_then(Value::as_object); + + if descendant_obj.get("additionalProperties") == Some(&Value::Bool(false)) { + let mut orphaned: Vec<&str> = ancestor + .properties + .keys() + .filter(|name| !descendant_props.is_some_and(|props| props.contains_key(name.as_str()))) + .map(String::as_str) + .collect(); + orphaned.sort_unstable(); + for name in orphaned { + let property_path = join_schema_path(path, name); + errors.push(format!( + "property '{property_path}': descendant schema '{descendant_label}' sets \ + additionalProperties: false but does not restate property defined in \ + ancestor '{ancestor_label}', making it unusable under allOf composition" + )); + } + } + + if let Some(props) = descendant_props { + let mut common: Vec<&str> = props + .keys() + .filter(|name| ancestor.properties.contains_key(name.as_str())) + .map(String::as_str) + .collect(); + common.sort_unstable(); + + for name in common { + let Some(ancestor_prop) = ancestor.properties.get(name) else { + continue; + }; + let Some(descendant_prop) = props.get(name) else { + continue; + }; + + let next_path = join_schema_path(path, name); + collect_closed_descendant_branch_errors( + ancestor_prop, + descendant_prop, + &next_path, + depth + 1, + ancestor_label, + descendant_label, + errors, + ); + } + } + + if let Some(Value::Array(all_of)) = descendant_obj.get("allOf") { + for item in all_of { + collect_closed_descendant_branch_errors( + ancestor_schema, + item, + path, + depth + 1, + ancestor_label, + descendant_label, + errors, + ); + } + } +} + +fn join_schema_path(prefix: &str, name: &str) -> String { + if prefix.is_empty() { + name.to_owned() + } else { + format!("{prefix}.{name}") + } +} + // --------------------------------------------------------------------------- // Constraint comparison helpers // --------------------------------------------------------------------------- @@ -247,8 +389,12 @@ fn compare_property_constraints( let base_nested = extract_effective_schema(base_prop); let derived_nested = extract_effective_schema(derived_prop); - let nested_errors = - validate_schema_compatibility(&base_nested, &derived_nested, "base", "derived"); + let nested_errors = validate_effective_schema_compatibility( + &base_nested, + &derived_nested, + "base", + "derived", + ); for err in nested_errors { errors.push(format!("in nested object '{prop_name}': {err}")); } @@ -639,130 +785,170 @@ mod tests { #[test] fn test_compatible_tightening() { - let base = extract_effective_schema(&json!({ + let base = json!({ "type": "object", "properties": { "v": {"type": "string", "maxLength": 100} } - })); - let derived = extract_effective_schema(&json!({ + }); + let derived = json!({ "type": "object", "properties": { "v": {"type": "string", "maxLength": 50} } - })); + }); let errs = validate_schema_compatibility(&base, &derived, "base~", "derived~"); assert!(errs.is_empty(), "tightening should be ok: {errs:?}"); } #[test] fn test_incompatible_loosening_max_length() { - let base = extract_effective_schema(&json!({ + let base = json!({ "type": "object", "properties": { "v": {"type": "string", "maxLength": 100} } - })); - let derived = extract_effective_schema(&json!({ + }); + let derived = json!({ "type": "object", "properties": { "v": {"type": "string", "maxLength": 200} } - })); + }); let errs = validate_schema_compatibility(&base, &derived, "base~", "derived~"); assert!(!errs.is_empty()); } #[test] fn test_incompatible_loosening_maximum() { - let base = extract_effective_schema(&json!({ + let base = json!({ "type": "object", "properties": { "n": {"type": "integer", "maximum": 100} } - })); - let derived = extract_effective_schema(&json!({ + }); + let derived = json!({ "type": "object", "properties": { "n": {"type": "integer", "maximum": 200} } - })); + }); let errs = validate_schema_compatibility(&base, &derived, "b", "d"); assert!(!errs.is_empty()); } #[test] fn test_incompatible_loosening_minimum() { - let base = extract_effective_schema(&json!({ + let base = json!({ "type": "object", "properties": { "n": {"type": "integer", "minimum": 10} } - })); - let derived = extract_effective_schema(&json!({ + }); + let derived = json!({ "type": "object", "properties": { "n": {"type": "integer", "minimum": 5} } - })); + }); let errs = validate_schema_compatibility(&base, &derived, "b", "d"); assert!(!errs.is_empty()); } #[test] fn test_enum_expansion_fails() { - let base = extract_effective_schema(&json!({ + let base = json!({ "type": "object", "properties": { "s": {"type": "string", "enum": ["a", "b"]} } - })); - let derived = extract_effective_schema(&json!({ + }); + let derived = json!({ "type": "object", "properties": { "s": {"type": "string", "enum": ["a", "b", "c"]} } - })); + }); let errs = validate_schema_compatibility(&base, &derived, "b", "d"); assert!(!errs.is_empty()); } #[test] fn test_enum_subset_ok() { - let base = extract_effective_schema(&json!({ + let base = json!({ "type": "object", "properties": { "s": {"type": "string", "enum": ["a", "b", "c"]} } - })); - let derived = extract_effective_schema(&json!({ + }); + let derived = json!({ "type": "object", "properties": { "s": {"type": "string", "enum": ["a"]} } - })); + }); let errs = validate_schema_compatibility(&base, &derived, "b", "d"); assert!(errs.is_empty(), "{errs:?}"); } #[test] fn test_additional_properties_false_blocks_new_prop() { - let base = extract_effective_schema(&json!({ + let base = json!({ "type": "object", "properties": {"a": {"type": "string"}}, "additionalProperties": false - })); - let derived = extract_effective_schema(&json!({ + }); + let derived = json!({ "type": "object", "properties": { "a": {"type": "string"}, "b": {"type": "string"} } - })); + }); let errs = validate_schema_compatibility(&base, &derived, "b", "d"); assert!(!errs.is_empty()); } + #[test] + fn test_value_compatibility_catches_closed_descendant_branch_orphan() { + let base = json!({ + "type": "object", + "properties": { + "routing": { + "type": "object", + "properties": { + "source": {"type": "string"} + } + } + } + }); + let derived = json!({ + "type": "object", + "allOf": [ + base, + { + "type": "object", + "properties": { + "routing": { + "type": "object", + "additionalProperties": false, + "properties": { + "target": {"type": "string"} + } + } + } + } + ] + }); + + let errs = validate_schema_compatibility(&base, &derived, "base", "derived"); + assert!( + errs.iter() + .any(|e| e.contains("routing.source") && e.contains("additionalProperties")), + "closed descendant branch should not orphan an ancestor property: {errs:?}" + ); + } + #[test] fn test_additional_properties_inherited_via_allof_not_loosening() { // Derived omits `additionalProperties` at its own root but its @@ -774,15 +960,15 @@ mod tests { // Per JSON Schema allOf composition, the base's // `additionalProperties: false` is inherited via $ref, so this // shape is **not** loosening and OP#12 must not flag it. - let base = extract_effective_schema(&json!({ + let base = json!({ "type": "object", "properties": {"a": {"type": "string"}}, "additionalProperties": false - })); - let derived = extract_effective_schema(&json!({ + }); + let derived = json!({ "type": "object", "properties": {"a": {"type": "string"}} - })); + }); let errs = validate_schema_compatibility(&base, &derived, "b", "d"); assert!( errs.is_empty(), @@ -794,16 +980,16 @@ mod tests { fn test_additional_properties_explicit_true_still_loosens() { // A direct derived schema that has no inherited closed branch and // explicitly says `additionalProperties: true` loosens a closed base. - let base = extract_effective_schema(&json!({ + let base = json!({ "type": "object", "properties": {"a": {"type": "string"}}, "additionalProperties": false - })); - let derived = extract_effective_schema(&json!({ + }); + let derived = json!({ "type": "object", "properties": {"a": {"type": "string"}}, "additionalProperties": true - })); + }); let errs = validate_schema_compatibility(&base, &derived, "b", "d"); assert!( errs.iter() @@ -814,42 +1000,39 @@ mod tests { #[test] fn test_open_base_allows_new_prop() { - let base = extract_effective_schema(&json!({ + let base = json!({ "type": "object", "properties": {"a": {"type": "string"}} - })); - let derived = extract_effective_schema(&json!({ + }); + let derived = json!({ "type": "object", "properties": { "a": {"type": "string"}, "b": {"type": "string"} } - })); + }); let errs = validate_schema_compatibility(&base, &derived, "b", "d"); assert!(errs.is_empty(), "{errs:?}"); } #[test] fn test_property_disabled_fails() { - let base = extract_effective_schema(&json!({ + let base = json!({ "type": "object", "required": ["x"], "properties": {"x": {"type": "string"}} - })); - let mut derived_props = HashMap::new(); - derived_props.insert("x".to_owned(), Value::Bool(false)); - let derived = EffectiveSchema { - properties: derived_props, - required: HashSet::new(), - additional_properties: None, - }; + }); + let derived = json!({ + "type": "object", + "properties": {"x": false} + }); let errs = validate_schema_compatibility(&base, &derived, "b", "d"); assert!(!errs.is_empty()); } #[test] fn test_nested_object_loosening_caught() { - let base = extract_effective_schema(&json!({ + let base = json!({ "type": "object", "properties": { "inner": { @@ -859,8 +1042,8 @@ mod tests { } } } - })); - let derived = extract_effective_schema(&json!({ + }); + let derived = json!({ "type": "object", "properties": { "inner": { @@ -870,7 +1053,7 @@ mod tests { } } } - })); + }); let errs = validate_schema_compatibility(&base, &derived, "b", "d"); assert!(!errs.is_empty()); } @@ -879,24 +1062,18 @@ mod tests { fn test_boolean_true_schema_loosens_constrained_property() { // Derived replaces a constrained property with boolean `true` schema // (which accepts anything), silently loosening the contract. - let base = EffectiveSchema { - properties: { - let mut m = HashMap::new(); - m.insert("age".to_owned(), json!({"type": "integer", "maximum": 120})); - m - }, - required: HashSet::new(), - additional_properties: None, - }; - let derived = EffectiveSchema { - properties: { - let mut m = HashMap::new(); - m.insert("age".to_owned(), Value::Bool(true)); - m - }, - required: HashSet::new(), - additional_properties: None, - }; + let base = json!({ + "type": "object", + "properties": { + "age": {"type": "integer", "maximum": 120} + } + }); + let derived = json!({ + "type": "object", + "properties": { + "age": true + } + }); let errs = validate_schema_compatibility(&base, &derived, "b", "d"); assert!( !errs.is_empty(), @@ -905,33 +1082,21 @@ mod tests { } #[test] - fn test_boolean_true_schema_ok_when_base_unconstrained() { - // If base property is also unconstrained (no special keywords), - // derived using boolean true is not loosening anything meaningful. - let base = EffectiveSchema { - properties: { - let mut m = HashMap::new(); - m.insert("name".to_owned(), json!({"type": "string"})); - m - }, - required: HashSet::new(), - additional_properties: None, - }; - let derived = EffectiveSchema { - properties: { - let mut m = HashMap::new(); - m.insert("name".to_owned(), Value::Bool(true)); - m - }, - required: HashSet::new(), - additional_properties: None, - }; + fn test_boolean_true_schema_loosens_typed_property() { + // A boolean `true` derived property removes the base type constraint. + let base = json!({ + "type": "object", + "properties": { + "name": {"type": "string"} + } + }); + let derived = json!({ + "type": "object", + "properties": { + "name": true + } + }); let errs = validate_schema_compatibility(&base, &derived, "b", "d"); - // Base only has "type" constraint. Boolean true does remove that. - // However, dropping type information is still loosening. - // For unconstrained base (no type even), this would be OK. - // Currently this WILL flag because type in base exists but derived is not an object. - // This behavior is acceptable – boolean true schemas should not appear in valid GTS. assert!( !errs.is_empty(), "Boolean true schema replaces typed property - should flag" @@ -942,18 +1107,18 @@ mod tests { fn test_enum_tightening_allows_omitting_bounds() { // Derived introduces enum, which is strictly tighter than maxLength. // Omitting maxLength when adding enum is NOT loosening. - let base = extract_effective_schema(&json!({ + let base = json!({ "type": "object", "properties": { "tier": {"type": "string", "maxLength": 100} } - })); - let derived = extract_effective_schema(&json!({ + }); + let derived = json!({ "type": "object", "properties": { "tier": {"type": "string", "enum": ["gold", "platinum"]} } - })); + }); let errs = validate_schema_compatibility(&base, &derived, "b", "d"); assert!( errs.is_empty(), @@ -965,18 +1130,18 @@ mod tests { fn test_const_tightening_allows_omitting_bounds_and_pattern() { // Derived introduces const, which is the tightest possible constraint. // Omitting bounds and pattern when adding const is NOT loosening. - let base = extract_effective_schema(&json!({ + let base = json!({ "type": "object", "properties": { "v": {"type": "string", "maxLength": 100, "pattern": "^[a-z]+$"} } - })); - let derived = extract_effective_schema(&json!({ + }); + let derived = json!({ "type": "object", "properties": { "v": {"type": "string", "const": "hello"} } - })); + }); let errs = validate_schema_compatibility(&base, &derived, "b", "d"); assert!( errs.is_empty(), @@ -987,18 +1152,18 @@ mod tests { #[test] fn test_enum_tightening_allows_omitting_numeric_bounds() { // Derived introduces enum for an integer property, omitting min/max. - let base = extract_effective_schema(&json!({ + let base = json!({ "type": "object", "properties": { "priority": {"type": "integer", "minimum": 0, "maximum": 100} } - })); - let derived = extract_effective_schema(&json!({ + }); + let derived = json!({ "type": "object", "properties": { "priority": {"type": "integer", "enum": [1, 5, 10]} } - })); + }); let errs = validate_schema_compatibility(&base, &derived, "b", "d"); assert!( errs.is_empty(), @@ -1009,18 +1174,18 @@ mod tests { #[test] fn test_omitting_bounds_without_enum_or_const_still_fails() { // Derived omits maxLength without adding enum or const — still loosening. - let base = extract_effective_schema(&json!({ + let base = json!({ "type": "object", "properties": { "code": {"type": "string", "maxLength": 100} } - })); - let derived = extract_effective_schema(&json!({ + }); + let derived = json!({ "type": "object", "properties": { "code": {"type": "string"} } - })); + }); let errs = validate_schema_compatibility(&base, &derived, "b", "d"); assert!( !errs.is_empty(), @@ -1031,27 +1196,27 @@ mod tests { #[test] fn test_derived_const_must_be_in_base_enum() { // Base has enum, derived narrows to const — but const value must be in base enum. - let base = extract_effective_schema(&json!({ + let base = json!({ "type": "object", "properties": { "status": {"type": "string", "enum": ["active", "inactive"]} } - })); - let derived_ok = extract_effective_schema(&json!({ + }); + let derived_ok = json!({ "type": "object", "properties": { "status": {"type": "string", "const": "active"} } - })); + }); let errs = validate_schema_compatibility(&base, &derived_ok, "b", "d"); assert!(errs.is_empty(), "const in base enum should be ok: {errs:?}"); - let derived_bad = extract_effective_schema(&json!({ + let derived_bad = json!({ "type": "object", "properties": { "status": {"type": "string", "const": "deleted"} } - })); + }); let errs = validate_schema_compatibility(&base, &derived_bad, "b", "d"); assert!(!errs.is_empty(), "const NOT in base enum should fail"); } @@ -1059,18 +1224,18 @@ mod tests { #[test] fn test_const_violates_minimum() { // Base has minimum 42, derived sets const 32 — must fail. - let base = extract_effective_schema(&json!({ + let base = json!({ "type": "object", "properties": { "score": {"type": "integer", "minimum": 42} } - })); - let derived = extract_effective_schema(&json!({ + }); + let derived = json!({ "type": "object", "properties": { "score": {"type": "integer", "const": 32} } - })); + }); let errs = validate_schema_compatibility(&base, &derived, "base~", "derived~"); assert!( !errs.is_empty(), @@ -1086,18 +1251,18 @@ mod tests { #[test] fn test_const_satisfies_minimum() { // Base has minimum 42, derived sets const 50 — should pass. - let base = extract_effective_schema(&json!({ + let base = json!({ "type": "object", "properties": { "score": {"type": "integer", "minimum": 42} } - })); - let derived = extract_effective_schema(&json!({ + }); + let derived = json!({ "type": "object", "properties": { "score": {"type": "integer", "const": 50} } - })); + }); let errs = validate_schema_compatibility(&base, &derived, "base~", "derived~"); assert!( errs.is_empty(), @@ -1108,18 +1273,18 @@ mod tests { #[test] fn test_enum_value_violates_maximum() { // Base has maximum 100, derived enum includes 200 — must fail. - let base = extract_effective_schema(&json!({ + let base = json!({ "type": "object", "properties": { "score": {"type": "integer", "maximum": 100} } - })); - let derived = extract_effective_schema(&json!({ + }); + let derived = json!({ "type": "object", "properties": { "score": {"type": "integer", "enum": [10, 50, 200]} } - })); + }); let errs = validate_schema_compatibility(&base, &derived, "base~", "derived~"); assert!( !errs.is_empty(), @@ -1130,18 +1295,18 @@ mod tests { #[test] fn test_enum_values_within_bounds() { // Base has minimum 10 and maximum 100, all enum values within range — should pass. - let base = extract_effective_schema(&json!({ + let base = json!({ "type": "object", "properties": { "score": {"type": "integer", "minimum": 10, "maximum": 100} } - })); - let derived = extract_effective_schema(&json!({ + }); + let derived = json!({ "type": "object", "properties": { "score": {"type": "integer", "enum": [10, 50, 100]} } - })); + }); let errs = validate_schema_compatibility(&base, &derived, "base~", "derived~"); assert!( errs.is_empty(), @@ -1152,18 +1317,18 @@ mod tests { #[test] fn test_const_string_violates_max_length() { // Base has maxLength 5, derived const is "toolong" (7 chars) — must fail. - let base = extract_effective_schema(&json!({ + let base = json!({ "type": "object", "properties": { "code": {"type": "string", "maxLength": 5} } - })); - let derived = extract_effective_schema(&json!({ + }); + let derived = json!({ "type": "object", "properties": { "code": {"type": "string", "const": "toolong"} } - })); + }); let errs = validate_schema_compatibility(&base, &derived, "base~", "derived~"); assert!( !errs.is_empty(), diff --git a/gts/src/schema_resolver_test.rs b/gts/src/schema_resolver_test.rs index 7529c3b..a033f4d 100644 --- a/gts/src/schema_resolver_test.rs +++ b/gts/src/schema_resolver_test.rs @@ -499,7 +499,7 @@ fn test_resolve_allows_duplicate_ref_in_allof() { ] }); - assert!(SchemaResolver::new(&p).resolve(&schema.clone()).is_ok()); + assert!(SchemaResolver::new(&p).resolve(&schema).is_ok()); assert!(resolve(&p, schema).is_object()); } diff --git a/gts/src/schema_traits.rs b/gts/src/schema_traits.rs index eb88ba0..ae85420 100644 --- a/gts/src/schema_traits.rs +++ b/gts/src/schema_traits.rs @@ -99,6 +99,7 @@ impl EffectiveTraits { /// `false` with values present, or if values don't conform. pub(crate) fn validate(&self, check_unresolved: bool) -> Result<(), Vec> { validate_trait_schema_integrity(&self.resolved_trait_schemas)?; + validate_trait_schema_compatibility(&self.resolved_trait_schemas)?; if !self.has_schema() { if self.has_explicit_values() { @@ -281,6 +282,53 @@ fn validate_trait_schema_integrity(resolved_trait_schemas: &[Value]) -> Result<( Ok(()) } +/// Validate that each descendant trait-schema contribution remains compatible +/// with the effective ancestor trait-schema prefix. +/// +/// Chain aggregation via `allOf` means values that satisfy the descendant's +/// effective trait-schema also satisfy every ancestor branch. This check catches +/// structural incompatibilities early, even when an abstract type provides no +/// concrete `x-gts-traits` values for JSON Schema validation to exercise. +fn validate_trait_schema_compatibility( + resolved_trait_schemas: &[Value], +) -> Result<(), Vec> { + let mut errors = Vec::new(); + + for i in 1..resolved_trait_schemas.len() { + let ancestor_schema = build_effective_traits_schema(&resolved_trait_schemas[..i]); + let descendant_schema = build_effective_traits_schema(&resolved_trait_schemas[..=i]); + + let ancestor = crate::schema_compat::extract_effective_schema(&ancestor_schema); + let descendant = crate::schema_compat::extract_effective_schema(&descendant_schema); + let pair_errors = crate::schema_compat::validate_effective_schema_compatibility( + &ancestor, + &descendant, + "ancestor trait schema", + "descendant trait schema", + ); + + errors.extend(pair_errors.into_iter().map(|err| { + format!("x-gts-traits-schema[{i}] is incompatible with ancestor trait schema: {err}") + })); + + let branch_errors = crate::schema_compat::validate_closed_descendant_branches( + &ancestor_schema, + &resolved_trait_schemas[i], + "ancestor trait schema", + "descendant trait schema", + ); + errors.extend(branch_errors.into_iter().map(|err| { + format!("x-gts-traits-schema[{i}] is incompatible with ancestor trait schema: {err}") + })); + } + + if errors.is_empty() { + Ok(()) + } else { + Err(errors) + } +} + /// Build the dialect-pinned effective traits schema and the materialized /// effective-traits object from chain-collected inputs, returning a /// self-contained [`EffectiveTraits`] (the inputs are retained so the result @@ -1268,6 +1316,269 @@ mod tests { ); } + #[test] + fn test_closed_ancestor_blocks_descendant_trait_schema_property_without_values() { + let schemas = vec![ + json!({ + "type": "object", + "additionalProperties": false, + "properties": { + "retention": {"type": "string"} + } + }), + json!({ + "type": "object", + "properties": { + "topicRef": {"type": "string"} + } + }), + ]; + + let traits = build_effective_traits(&schemas, &json!({}), None); + let err = traits + .validate(false) + .expect_err("abstract types must still reject incompatible trait schemas"); + assert!( + err.iter() + .any(|e| e.contains("topicRef") || e.contains("additionalProperties")), + "closed ancestor should reject descendant trait property: {err:?}" + ); + } + + #[test] + fn test_trait_schema_type_conflict_fails_without_values() { + let schemas = vec![ + json!({ + "type": "object", + "properties": { + "retention": {"type": "string"} + } + }), + json!({ + "type": "object", + "properties": { + "retention": {"type": "integer"} + } + }), + ]; + + let traits = build_effective_traits(&schemas, &json!({}), None); + let err = traits + .validate(false) + .expect_err("trait schema conflicts must fail without concrete values"); + assert!( + err.iter() + .any(|e| e.contains("retention") && e.contains("type")), + "type conflict should be reported: {err:?}" + ); + } + + #[test] + fn test_abstract_valid_narrowing_trait_schema_passes_without_values() { + // Guard against over-rejection: genuine narrowing (open string → enum + // subset) plus a new optional property under an open ancestor must pass. + let schemas = vec![ + json!({ + "type": "object", + "properties": {"priority": {"type": "string"}} + }), + json!({ + "type": "object", + "properties": { + "priority": {"type": "string", "enum": ["low", "high"]}, + "note": {"type": "string"} + } + }), + ]; + + let traits = build_effective_traits(&schemas, &json!({}), None); + assert!( + traits.validate(false).is_ok(), + "valid narrowing must pass without values: {:?}", + traits.validate(false) + ); + } + + #[test] + fn test_descendant_closed_trait_schema_orphans_ancestor_property_fails() { + // Closed descendant that drops an ancestor property orphans it under + // allOf; must fail even with no values present. + let schemas = vec![ + json!({ + "type": "object", + "properties": {"retention": {"type": "string"}} + }), + json!({ + "type": "object", + "additionalProperties": false, + "properties": {"topicRef": {"type": "string"}} + }), + ]; + + let traits = build_effective_traits(&schemas, &json!({}), None); + let err = traits + .validate(false) + .expect_err("a closed descendant orphaning an ancestor trait must fail"); + assert!( + err.iter() + .any(|e| e.contains("retention") && e.contains("additionalProperties")), + "orphaned ancestor property should be reported: {err:?}" + ); + } + + #[test] + fn test_descendant_closed_trait_schema_restating_ancestor_property_passes() { + // Escape hatch: closing the surface but restating the ancestor property + // keeps it usable, so it must pass. + let schemas = vec![ + json!({ + "type": "object", + "properties": {"retention": {"type": "string"}} + }), + json!({ + "type": "object", + "additionalProperties": false, + "properties": { + "retention": {"type": "string"}, + "topicRef": {"type": "string"} + } + }), + ]; + + let traits = build_effective_traits(&schemas, &json!({}), None); + assert!( + traits.validate(false).is_ok(), + "restating the ancestor property keeps it usable: {:?}", + traits.validate(false) + ); + } + + #[test] + fn test_descendant_nested_closed_trait_schema_orphans_ancestor_property_fails() { + let schemas = vec![ + json!({ + "type": "object", + "properties": { + "routing": { + "type": "object", + "properties": { + "source": {"type": "string"} + } + } + } + }), + json!({ + "type": "object", + "properties": { + "routing": { + "type": "object", + "additionalProperties": false, + "properties": { + "target": {"type": "string"} + } + } + } + }), + ]; + + let traits = build_effective_traits(&schemas, &json!({}), None); + let err = traits + .validate(false) + .expect_err("a closed nested descendant orphaning an ancestor trait must fail"); + assert!( + err.iter() + .any(|e| { e.contains("routing.source") && e.contains("additionalProperties") }), + "orphaned nested ancestor property should be reported: {err:?}" + ); + } + + #[test] + fn test_descendant_nested_closed_trait_schema_restating_ancestor_property_passes() { + let schemas = vec![ + json!({ + "type": "object", + "properties": { + "routing": { + "type": "object", + "properties": { + "source": {"type": "string"} + } + } + } + }), + json!({ + "type": "object", + "properties": { + "routing": { + "type": "object", + "additionalProperties": false, + "properties": { + "source": {"type": "string"}, + "target": {"type": "string"} + } + } + } + }), + ]; + + let traits = build_effective_traits(&schemas, &json!({}), None); + assert!( + traits.validate(false).is_ok(), + "restating the nested ancestor property keeps it usable: {:?}", + traits.validate(false) + ); + } + + #[test] + fn test_descendant_nested_allof_closed_branch_orphans_ancestor_property_fails() { + let schemas = vec![ + json!({ + "type": "object", + "properties": { + "routing": { + "type": "object", + "properties": { + "source": {"type": "string"} + } + } + } + }), + json!({ + "type": "object", + "properties": { + "routing": { + "type": "object", + "allOf": [ + { + "type": "object", + "additionalProperties": false, + "properties": { + "target": {"type": "string"} + } + }, + { + "type": "object", + "properties": { + "source": {"type": "string"} + } + } + ] + } + } + }), + ]; + + let traits = build_effective_traits(&schemas, &json!({}), None); + let err = traits + .validate(false) + .expect_err("a closed allOf branch orphaning an ancestor trait must fail"); + assert!( + err.iter() + .any(|e| { e.contains("routing.source") && e.contains("additionalProperties") }), + "orphaned nested allOf ancestor property should be reported: {err:?}" + ); + } + #[test] fn test_override_in_chain() { let chain = vec![ diff --git a/gts/src/store.rs b/gts/src/store.rs index 607cff5..be08a7e 100644 --- a/gts/src/store.rs +++ b/gts/src/store.rs @@ -456,13 +456,9 @@ impl GtsStore { StoreError::ValidationError(format!("Schema '{derived_id}' has {e}")) })?; - // Extract effective schemas and compare via schema_compat module - let base_eff = crate::schema_compat::extract_effective_schema(&base_resolved); - let derived_eff = crate::schema_compat::extract_effective_schema(&derived_resolved); - let errors = crate::schema_compat::validate_schema_compatibility( - &base_eff, - &derived_eff, + &base_resolved, + &derived_resolved, &base_id, &derived_id, ); diff --git a/gts/src/store_test.rs b/gts/src/store_test.rs index c41c059..7773e9f 100644 --- a/gts/src/store_test.rs +++ b/gts/src/store_test.rs @@ -3510,6 +3510,62 @@ fn test_op12_derived_omits_additional_properties_inherits_closedness() { ); } +#[test] +fn test_op12_descendant_nested_closed_schema_orphans_ancestor_property() { + let mut store = GtsStore::new(); + + let base = json!({ + "$id": "gts://gts.x.test12.nested_orphan.event.v1~", + "$schema": "http://json-schema.org/draft-07/schema#", + "type": "object", + "properties": { + "routing": { + "type": "object", + "properties": { + "source": {"type": "string"} + } + } + } + }); + store + .register_schema("gts.x.test12.nested_orphan.event.v1~", &base) + .expect("register base"); + + let derived = json!({ + "$id": "gts://gts.x.test12.nested_orphan.event.v1~x.test12._.child.v1~", + "$schema": "http://json-schema.org/draft-07/schema#", + "type": "object", + "allOf": [ + {"$ref": "gts://gts.x.test12.nested_orphan.event.v1~"}, + { + "type": "object", + "properties": { + "routing": { + "type": "object", + "additionalProperties": false, + "properties": { + "target": {"type": "string"} + } + } + } + } + ] + }); + store + .register_schema( + "gts.x.test12.nested_orphan.event.v1~x.test12._.child.v1~", + &derived, + ) + .expect("register derived"); + + let result = + store.validate_schema_chain("gts.x.test12.nested_orphan.event.v1~x.test12._.child.v1~"); + assert!( + result.is_err(), + "closed nested derived branch should not orphan ancestor routing.source: {result:?}" + ); +} + #[test] fn test_op12_derived_omits_const() { let mut store = GtsStore::new(); @@ -5195,6 +5251,307 @@ fn test_op13_abstract_rejects_wrong_typed_trait_value() { ); } +#[test] +fn test_op13_abstract_rejects_incompatible_trait_schema_without_values() { + // Abstract types may defer required trait values, but their own + // x-gts-traits-schema contribution must still be compatible with ancestors. + // This catches schema conflicts even when there are no materialized values + // for JSON Schema validation to exercise. + let mut store = GtsStore::new(); + let base = "gts.x.abst.tr.incomp.v1~"; + let child = "gts.x.abst.tr.incomp.v1~x.abst._.child.v1~"; + + register_chain_schema( + &mut store, + base, + json!({ + "x-gts-abstract": true, + "x-gts-traits-schema": { + "type": "object", + "properties": {"retention": {"type": "string"}} + } + }), + ); + register_chain_schema( + &mut store, + child, + json!({ + "x-gts-abstract": true, + "x-gts-traits-schema": { + "type": "object", + "properties": {"retention": {"type": "integer"}} + } + }), + ); + + let err = store.validate_schema(child).unwrap_err(); + assert!( + format!("{err}").contains("x-gts-traits-schema") && format!("{err}").contains("retention"), + "abstract child must reject incompatible trait schema without values: {err}" + ); +} + +#[test] +fn test_op13_abstract_closed_trait_schema_blocks_new_schema_property_without_values() { + let mut store = GtsStore::new(); + let base = "gts.x.abst.tr.closed.v1~"; + let child = "gts.x.abst.tr.closed.v1~x.abst._.child.v1~"; + + register_chain_schema( + &mut store, + base, + json!({ + "x-gts-abstract": true, + "x-gts-traits-schema": { + "type": "object", + "additionalProperties": false, + "properties": {"retention": {"type": "string"}} + } + }), + ); + register_chain_schema( + &mut store, + child, + json!({ + "x-gts-abstract": true, + "x-gts-traits-schema": { + "type": "object", + "properties": {"topicRef": {"type": "string"}} + } + }), + ); + + let err = store.validate_schema(child).unwrap_err(); + assert!( + format!("{err}").contains("topicRef") || format!("{err}").contains("additionalProperties"), + "closed ancestor trait schema must block new descendant property without values: {err}" + ); +} + +#[test] +fn test_op13_abstract_descendant_closed_trait_schema_orphans_ancestor_property() { + // A descendant x-gts-traits-schema with additionalProperties:false that + // drops an ancestor trait orphans it under allOf; must fail even though the + // abstract child provides no values. + let mut store = GtsStore::new(); + let base = "gts.x.abst.tr.orphan.v1~"; + let child = "gts.x.abst.tr.orphan.v1~x.abst._.child.v1~"; + + register_chain_schema( + &mut store, + base, + json!({ + "x-gts-abstract": true, + "x-gts-traits-schema": { + "type": "object", + "properties": {"retention": {"type": "string"}} + } + }), + ); + register_chain_schema( + &mut store, + child, + json!({ + "x-gts-abstract": true, + "x-gts-traits-schema": { + "type": "object", + "additionalProperties": false, + "properties": {"topicRef": {"type": "string"}} + } + }), + ); + + let err = store.validate_schema(child).unwrap_err(); + assert!( + format!("{err}").contains("retention") && format!("{err}").contains("additionalProperties"), + "closed descendant trait schema must not orphan an ancestor trait: {err}" + ); +} + +#[test] +fn test_op13_abstract_descendant_closed_trait_schema_restating_ancestor_ok() { + // Escape hatch: a closed descendant that restates the ancestor trait keeps + // it usable and must validate. + let mut store = GtsStore::new(); + let base = "gts.x.abst.tr.restate.v1~"; + let child = "gts.x.abst.tr.restate.v1~x.abst._.child.v1~"; + + register_chain_schema( + &mut store, + base, + json!({ + "x-gts-abstract": true, + "x-gts-traits-schema": { + "type": "object", + "properties": {"retention": {"type": "string"}} + } + }), + ); + register_chain_schema( + &mut store, + child, + json!({ + "x-gts-abstract": true, + "x-gts-traits-schema": { + "type": "object", + "additionalProperties": false, + "properties": { + "retention": {"type": "string"}, + "topicRef": {"type": "string"} + } + } + }), + ); + + assert!( + store.validate_schema(child).is_ok(), + "restating the ancestor trait under a closed descendant must pass" + ); +} + +#[test] +fn test_op13_abstract_descendant_nested_closed_trait_schema_orphans_ancestor_property() { + let mut store = GtsStore::new(); + let base = "gts.x.abst.tr.nested_orphan.v1~"; + let child = "gts.x.abst.tr.nested_orphan.v1~x.abst._.child.v1~"; + + register_chain_schema( + &mut store, + base, + json!({ + "x-gts-abstract": true, + "x-gts-traits-schema": { + "type": "object", + "properties": { + "routing": { + "type": "object", + "properties": { + "source": {"type": "string"} + } + } + } + } + }), + ); + register_chain_schema( + &mut store, + child, + json!({ + "x-gts-abstract": true, + "x-gts-traits-schema": { + "type": "object", + "properties": { + "routing": { + "type": "object", + "additionalProperties": false, + "properties": { + "target": {"type": "string"} + } + } + } + } + }), + ); + + let err = store.validate_schema(child).unwrap_err(); + assert!( + format!("{err}").contains("routing.source") + && format!("{err}").contains("additionalProperties"), + "closed nested descendant trait schema must not orphan an ancestor trait: {err}" + ); +} + +#[test] +fn test_op13_abstract_descendant_nested_closed_trait_schema_restating_ancestor_ok() { + let mut store = GtsStore::new(); + let base = "gts.x.abst.tr.nested_restate.v1~"; + let child = "gts.x.abst.tr.nested_restate.v1~x.abst._.child.v1~"; + + register_chain_schema( + &mut store, + base, + json!({ + "x-gts-abstract": true, + "x-gts-traits-schema": { + "type": "object", + "properties": { + "routing": { + "type": "object", + "properties": { + "source": {"type": "string"} + } + } + } + } + }), + ); + register_chain_schema( + &mut store, + child, + json!({ + "x-gts-abstract": true, + "x-gts-traits-schema": { + "type": "object", + "properties": { + "routing": { + "type": "object", + "additionalProperties": false, + "properties": { + "source": {"type": "string"}, + "target": {"type": "string"} + } + } + } + } + }), + ); + + assert!( + store.validate_schema(child).is_ok(), + "restating the nested ancestor trait under a closed descendant must pass" + ); +} + +#[test] +fn test_op13_abstract_descendant_valid_narrowing_trait_schema_ok() { + // Guard against over-rejection: an abstract descendant narrowing an ancestor + // trait (open string -> enum subset) plus a new optional property must pass. + let mut store = GtsStore::new(); + let base = "gts.x.abst.tr.narrow.v1~"; + let child = "gts.x.abst.tr.narrow.v1~x.abst._.child.v1~"; + + register_chain_schema( + &mut store, + base, + json!({ + "x-gts-abstract": true, + "x-gts-traits-schema": { + "type": "object", + "properties": {"priority": {"type": "string"}} + } + }), + ); + register_chain_schema( + &mut store, + child, + json!({ + "x-gts-abstract": true, + "x-gts-traits-schema": { + "type": "object", + "properties": { + "priority": {"type": "string", "enum": ["low", "high"]}, + "note": {"type": "string"} + } + } + }), + ); + + assert!( + store.validate_schema(child).is_ok(), + "valid abstract narrowing must pass without values" + ); +} + #[test] fn test_op13_abstract_skips_required_completeness() { // A required trait with no default and no value is allowed on an abstract From 6b29be693f5aacee4d8377fdca42f7f45c703879 Mon Sep 17 00:00:00 2001 From: Aviator 5 Date: Thu, 25 Jun 2026 11:08:23 +0300 Subject: [PATCH 5/7] chore: bump gts-spec version for conformance testing Signed-off-by: Aviator 5 --- .gts-spec-version | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.gts-spec-version b/.gts-spec-version index 87a1cf5..9ca265d 100644 --- a/.gts-spec-version +++ b/.gts-spec-version @@ -1 +1 @@ -v0.12.0 +v0.12.1 From 02ca1ac9d20ea12ffd3d643cf22f06daccabcab0 Mon Sep 17 00:00:00 2001 From: Aviator 5 Date: Thu, 25 Jun 2026 13:28:54 +0300 Subject: [PATCH 6/7] fix: fail closed on schema compatibility depth overflow - Report an error when closed-descendant branch validation reaches the recursion limit. - Cover the depth guard path so overly nested schemas cannot silently pass compatibility checks. - Update resolver comments to match the current resolve_schema_refs naming. Signed-off-by: Aviator 5 --- gts/src/schema_compat.rs | 31 +++++++++++++++++++++++++++++++ gts/src/schema_resolver.rs | 2 +- gts/src/schema_resolver_test.rs | 2 +- gts/src/store.rs | 2 +- gts/src/store_test.rs | 2 +- 5 files changed, 35 insertions(+), 4 deletions(-) diff --git a/gts/src/schema_compat.rs b/gts/src/schema_compat.rs index 044b403..f67fb1f 100644 --- a/gts/src/schema_compat.rs +++ b/gts/src/schema_compat.rs @@ -230,6 +230,11 @@ fn collect_closed_descendant_branch_errors( errors: &mut Vec, ) { if depth >= MAX_RECURSION_DEPTH { + errors.push(format!( + "schema compatibility check exceeded maximum nesting depth of \ + {MAX_RECURSION_DEPTH} at '{path}' between ancestor '{ancestor_label}' \ + and descendant '{descendant_label}'" + )); return; } @@ -949,6 +954,32 @@ mod tests { ); } + #[test] + fn test_closed_descendant_branch_fails_when_depth_guard_is_hit() { + fn nested_object(depth: usize) -> Value { + let mut schema = json!({"type": "object", "properties": {}}); + for _ in 0..depth { + schema = json!({ + "type": "object", + "properties": { + "child": schema + } + }); + } + schema + } + + let base = nested_object(MAX_RECURSION_DEPTH); + let derived = nested_object(MAX_RECURSION_DEPTH); + + let errs = validate_closed_descendant_branches(&base, &derived, "base", "derived"); + assert!( + errs.iter() + .any(|err| err.contains("exceeded maximum nesting depth")), + "depth guard should fail closed instead of silently accepting: {errs:?}" + ); + } + #[test] fn test_additional_properties_inherited_via_allof_not_loosening() { // Derived omits `additionalProperties` at its own root but its diff --git a/gts/src/schema_resolver.rs b/gts/src/schema_resolver.rs index 8f75b96..0843a22 100644 --- a/gts/src/schema_resolver.rs +++ b/gts/src/schema_resolver.rs @@ -7,7 +7,7 @@ //! //! [`SchemaResolver`] depends only on the narrow [`SchemaProvider`] lookup, not //! on `GtsStore` directly; the store implements `SchemaProvider` and exposes -//! `try_resolve_schema_refs` as a thin wrapper. +//! `resolve_schema_refs` as a thin wrapper. use serde_json::Value; diff --git a/gts/src/schema_resolver_test.rs b/gts/src/schema_resolver_test.rs index a033f4d..4ccf1c2 100644 --- a/gts/src/schema_resolver_test.rs +++ b/gts/src/schema_resolver_test.rs @@ -1,6 +1,6 @@ //! Unit tests for [`SchemaResolver`]. They drive the resolver directly against //! a tiny in-memory [`SchemaProvider`] mock (`MapProvider`) — no `GtsStore` -//! involved — so they exercise `SchemaResolver::try_resolve` in isolation. +//! involved — so they exercise `SchemaResolver::resolve` in isolation. //! End-to-end coverage of the `GtsStore` wrapper and provider lookup semantics //! lives in `store_test.rs`. diff --git a/gts/src/store.rs b/gts/src/store.rs index be08a7e..5894b9a 100644 --- a/gts/src/store.rs +++ b/gts/src/store.rs @@ -700,7 +700,7 @@ impl GtsStore { .resolve_schema_refs(&content) .map_err(|e| StoreError::ValidationError(format!("Schema '{type_id}' has {e}")))?; - // Strip x-gts-ref before compiling; try_resolve_schema_refs has already + // Strip x-gts-ref before compiling; resolve_schema_refs has already // inlined all resolvable external gts:// refs or returned an error. let schema_for_validation = Self::remove_x_gts_ref_fields(&resolved_schema); let validator = jsonschema::options() diff --git a/gts/src/store_test.rs b/gts/src/store_test.rs index 7773e9f..d7b6489 100644 --- a/gts/src/store_test.rs +++ b/gts/src/store_test.rs @@ -5770,7 +5770,7 @@ fn test_validate_and_resolve_accepts_well_formed_gts_ref_schema() { } // --------------------------------------------------------------------------- -// `GtsStore` `$ref`-resolution wrapper (`try_resolve_schema_refs`) and the +// `GtsStore` `$ref`-resolution wrapper (`resolve_schema_refs`) and the // store-as-`SchemaProvider` integration. // Resolver semantics themselves are unit-tested in `schema_resolver_test.rs`; // these are smoke/integration tests for the store-level surface. From fdcbc0a069f100e7f694a3336d67c92e5b4430a2 Mon Sep 17 00:00:00 2001 From: Aviator 5 Date: Fri, 26 Jun 2026 11:01:36 +0300 Subject: [PATCH 7/7] fix(schema-resolver): resolve local refs in remote fragments - Track the active local root while resolving GTS pointer fragments. - Resolve supported local JSON Pointer refs strictly and preserve sibling handling through allOf. - Add regression coverage for remote fragment roots, nested remote refs, missing local targets, and caller-root siblings. Signed-off-by: Aviator 5 --- gts/src/schema_resolver.rs | 187 ++++++++++++++++++-------- gts/src/schema_resolver_test.rs | 226 ++++++++++++++++++++++++++++++++ gts/src/store.rs | 4 +- 3 files changed, 358 insertions(+), 59 deletions(-) diff --git a/gts/src/schema_resolver.rs b/gts/src/schema_resolver.rs index 0843a22..7167791 100644 --- a/gts/src/schema_resolver.rs +++ b/gts/src/schema_resolver.rs @@ -36,8 +36,9 @@ impl<'a> SchemaResolver<'a> { Self { provider } } - /// Strict `$ref` resolution that returns an error if any external `$ref` - /// cannot be resolved or a circular `$ref` is detected. + /// Strict `$ref` resolution that returns an error if any supported local + /// JSON Pointer or external GTS `$ref` cannot be resolved, or a circular + /// `$ref` is detected. /// /// Uses DFS-path cycle detection: a `$ref` target is held in the seen-set /// only while its resolution is in progress on the current DFS stack and @@ -47,15 +48,20 @@ impl<'a> SchemaResolver<'a> { /// aggregation across an `$id` chain is allowed. /// /// # Errors - /// Returns [`StoreError::UnresolvedRefs`] if any external `$ref` cannot be - /// resolved, or [`StoreError::CircularRef`] if a circular `$ref` is - /// detected. + /// Returns [`StoreError::UnresolvedRefs`] if any supported local JSON + /// Pointer or external GTS `$ref` cannot be resolved, or + /// [`StoreError::CircularRef`] if a circular `$ref` is detected. pub(crate) fn resolve(&self, schema: &Value) -> Result { let mut visited = std::collections::HashSet::new(); let mut cycle_found = false; let mut unresolved_refs = Vec::new(); - let resolved = - self.resolve_inner(schema, &mut visited, &mut cycle_found, &mut unresolved_refs); + let resolved = self.resolve_inner( + schema, + schema, + &mut visited, + &mut cycle_found, + &mut unresolved_refs, + ); if cycle_found { Err(StoreError::CircularRef) } else if !unresolved_refs.is_empty() { @@ -69,6 +75,7 @@ impl<'a> SchemaResolver<'a> { fn resolve_inner( &self, schema: &Value, + local_root: &Value, visited: &mut std::collections::HashSet, cycle_found: &mut bool, unresolved_refs: &mut Vec, @@ -86,13 +93,49 @@ impl<'a> SchemaResolver<'a> { "#/$defs/GtsTypeId" | "#/$defs/GtsSchemaId" => { return crate::GtsTypeId::json_schema_value(); } - s if s.starts_with("#/") => { - // Other internal references - keep as-is + s if s == "#" || s.starts_with("#/") => { + if let Some(pointer) = ref_uri.strip_prefix('#') { + let local_ref_key = + format!("local:{:p}:{ref_uri}", std::ptr::from_ref(local_root)); + if visited.contains(&local_ref_key) { + *cycle_found = true; + return Value::Object(map.clone()); + } + if let Some(target) = local_root.pointer(pointer) { + visited.insert(local_ref_key.clone()); + let resolved = self.resolve_inner( + target, + local_root, + visited, + cycle_found, + unresolved_refs, + ); + visited.remove(&local_ref_key); + return self.resolved_ref_with_siblings( + map, + resolved, + local_root, + visited, + cycle_found, + unresolved_refs, + ); + } + unresolved_refs.push(ref_uri.clone()); + } + + // Unsupported or missing internal references are + // kept in the lenient output after being reported. let mut new_map = serde_json::Map::new(); for (k, v) in map { new_map.insert( k.clone(), - self.resolve_inner(v, visited, cycle_found, unresolved_refs), + self.resolve_inner( + v, + local_root, + visited, + cycle_found, + unresolved_refs, + ), ); } return Value::Object(new_map); @@ -129,7 +172,13 @@ impl<'a> SchemaResolver<'a> { if k == "$ref" { v.clone() } else { - self.resolve_inner(v, visited, cycle_found, unresolved_refs) + self.resolve_inner( + v, + local_root, + visited, + cycle_found, + unresolved_refs, + ) }, ); } @@ -151,6 +200,7 @@ impl<'a> SchemaResolver<'a> { // Recursively resolve refs in the referenced schema let mut resolved = self.resolve_inner( target_content, + content, visited, cycle_found, unresolved_refs, @@ -173,49 +223,14 @@ impl<'a> SchemaResolver<'a> { resolved_map.remove(crate::schema_traits::X_GTS_TRAITS_SCHEMA); } - // If the original object has only $ref, return the resolved schema - if map.len() == 1 { - return resolved; - } - - // Otherwise combine the resolved schema with the - // siblings via `allOf`. A last-wins merge would let a - // sibling drop or loosen the target's constraints - // (e.g. `required`, `additionalProperties`). - match resolved { - Value::Object(resolved_map) => { - let mut siblings = serde_json::Map::new(); - for (k, v) in map { - if k != "$ref" { - siblings.insert( - k.clone(), - self.resolve_inner( - v, - visited, - cycle_found, - unresolved_refs, - ), - ); - } - } - if siblings.is_empty() { - return Value::Object(resolved_map); - } - let mut merged = serde_json::Map::new(); - merged.insert( - "allOf".to_owned(), - Value::Array(vec![ - Value::Object(resolved_map), - Value::Object(siblings), - ]), - ); - return Value::Object(merged); - } - // Non-object target (e.g. a boolean schema via - // a pointer fragment) with siblings: `$ref` - // wins per JSON Schema precedence. - other => return other, - } + return self.resolved_ref_with_siblings( + map, + resolved, + local_root, + visited, + cycle_found, + unresolved_refs, + ); } } if !ref_uri.starts_with('#') { @@ -232,7 +247,13 @@ impl<'a> SchemaResolver<'a> { if k == "$ref" { v.clone() } else { - self.resolve_inner(v, visited, cycle_found, unresolved_refs) + self.resolve_inner( + v, + local_root, + visited, + cycle_found, + unresolved_refs, + ) }, ); } @@ -252,19 +273,71 @@ impl<'a> SchemaResolver<'a> { for (k, v) in map { new_map.insert( k.clone(), - self.resolve_inner(v, visited, cycle_found, unresolved_refs), + self.resolve_inner(v, local_root, visited, cycle_found, unresolved_refs), ); } Value::Object(new_map) } Value::Array(arr) => Value::Array( arr.iter() - .map(|v| self.resolve_inner(v, visited, cycle_found, unresolved_refs)) + .map(|v| { + self.resolve_inner(v, local_root, visited, cycle_found, unresolved_refs) + }) .collect(), ), _ => schema.clone(), } } + + fn resolved_ref_with_siblings( + &self, + map: &serde_json::Map, + resolved: Value, + local_root: &Value, + visited: &mut std::collections::HashSet, + cycle_found: &mut bool, + unresolved_refs: &mut Vec, + ) -> Value { + // If the original object has only $ref, return the resolved schema. + if map.len() == 1 { + return resolved; + } + + // Otherwise combine the resolved schema with the siblings via `allOf`. + // A last-wins merge would let a sibling drop or loosen the target's + // constraints (e.g. `required`, `additionalProperties`). + match resolved { + Value::Object(resolved_map) => { + let mut siblings = serde_json::Map::new(); + for (k, v) in map { + if k != "$ref" { + siblings.insert( + k.clone(), + self.resolve_inner( + v, + local_root, + visited, + cycle_found, + unresolved_refs, + ), + ); + } + } + if siblings.is_empty() { + return Value::Object(resolved_map); + } + let mut merged = serde_json::Map::new(); + merged.insert( + "allOf".to_owned(), + Value::Array(vec![Value::Object(resolved_map), Value::Object(siblings)]), + ); + Value::Object(merged) + } + // Non-object target (e.g. a boolean schema via a pointer fragment) + // with siblings: `$ref` wins per JSON Schema precedence. + other => other, + } + } } #[cfg(test)] diff --git a/gts/src/schema_resolver_test.rs b/gts/src/schema_resolver_test.rs index 4ccf1c2..9439792 100644 --- a/gts/src/schema_resolver_test.rs +++ b/gts/src/schema_resolver_test.rs @@ -528,6 +528,232 @@ fn test_resolve_pointer_to_boolean_with_siblings() { assert_eq!(resolved, json!(false)); } +#[test] +fn test_resolve_remote_pointer_fragment_resolves_local_refs_against_remote_root() { + // A fragment selected from a remote GTS document can contain local refs that + // are scoped to that remote document root. The inlined fragment must not + // keep a dangling `#/$defs/...` pointer after the root `$defs` is dropped. + let p = MapProvider::new().with( + "gts.x.core.events.named.v1~", + json!({ + "$id": "gts://gts.x.core.events.named.v1~", + "$schema": "http://json-schema.org/draft-07/schema#", + "$defs": {"Name": {"type": "string"}}, + "properties": { + "name": {"$ref": "#/$defs/Name"} + } + }), + ); + + let resolved = SchemaResolver::new(&p) + .resolve(&json!({"$ref": "gts://gts.x.core.events.named.v1~#/properties/name"})) + .expect("remote pointer fragment should resolve"); + + assert_eq!(resolved, json!({"type": "string"})); +} + +#[test] +fn test_resolve_remote_fragment_uses_remote_root_not_caller_root() { + // Regression guard: the same `#/$defs/Name` path exists in both the caller + // schema and the remote document, but with different types. The fragment's + // local ref must resolve against the *remote* root, not the caller's root. + let p = MapProvider::new().with( + "gts.x.core.events.named.v1~", + json!({ + "$id": "gts://gts.x.core.events.named.v1~", + "$schema": "http://json-schema.org/draft-07/schema#", + "$defs": {"Name": {"type": "string"}}, + "properties": { + "name": {"$ref": "#/$defs/Name"} + } + }), + ); + + let schema = json!({ + "$defs": {"Name": {"type": "integer"}}, + "properties": { + "test": {"$ref": "gts://gts.x.core.events.named.v1~#/properties/name"} + } + }); + + let resolved = SchemaResolver::new(&p) + .resolve(&schema) + .expect("remote pointer fragment should resolve"); + + assert_eq!( + resolved, + json!({ + "$defs": {"Name": {"type": "integer"}}, + "properties": { + "test": {"type": "string"} + } + }) + ); +} + +#[test] +fn test_resolve_remote_fragment_missing_local_ref_errors() { + // A remote fragment contains a local ref whose target does not exist in the + // remote document root. Strict resolution must report it as unresolved. + let p = MapProvider::new().with( + "gts.x.core.events.broken.v1~", + json!({ + "$id": "gts://gts.x.core.events.broken.v1~", + "$schema": "http://json-schema.org/draft-07/schema#", + "$defs": {"Other": {"type": "string"}}, + "properties": { + "name": {"$ref": "#/$defs/Missing"} + } + }), + ); + + let err = SchemaResolver::new(&p) + .resolve(&json!({"$ref": "gts://gts.x.core.events.broken.v1~#/properties/name"})) + .expect_err("missing local ref in remote fragment should fail"); + + assert!(matches!( + &err, + StoreError::UnresolvedRefs(refs) if refs == &["#/$defs/Missing".to_owned()] + )); +} + +#[test] +fn test_resolve_nested_remote_fragments_resolve_local_refs_against_correct_root() { + // Remote doc A's fragment contains a gts:// ref to remote doc B, and B's + // fragment contains a local #/$defs/... ref. The local ref must resolve + // against B's root, not A's or the caller's. + let p = MapProvider::new() + .with( + "gts.x.core.events.outer.v1~", + json!({ + "$id": "gts://gts.x.core.events.outer.v1~", + "$schema": "http://json-schema.org/draft-07/schema#", + "properties": { + "inner": {"$ref": "gts://gts.x.core.events.inner.v1~#/properties/value"} + } + }), + ) + .with( + "gts.x.core.events.inner.v1~", + json!({ + "$id": "gts://gts.x.core.events.inner.v1~", + "$schema": "http://json-schema.org/draft-07/schema#", + "$defs": {"Value": {"type": "boolean"}}, + "properties": { + "value": {"$ref": "#/$defs/Value"} + } + }), + ); + + let resolved = SchemaResolver::new(&p) + .resolve(&json!({"$ref": "gts://gts.x.core.events.outer.v1~#/properties/inner"})) + .expect("nested remote fragments should resolve"); + + assert_eq!(resolved, json!({"type": "boolean"})); +} + +#[test] +fn test_resolve_remote_fragment_siblings_resolve_against_caller_root() { + // A $ref to a remote fragment has siblings that contain local refs. The + // fragment's local refs resolve against the remote root, but the siblings' + // local refs must resolve against the caller's root. + let p = MapProvider::new().with( + "gts.x.core.events.remote.v1~", + json!({ + "$id": "gts://gts.x.core.events.remote.v1~", + "$schema": "http://json-schema.org/draft-07/schema#", + "$defs": {"Shared": {"type": "string"}}, + "properties": { + "name": {"$ref": "#/$defs/Shared"} + } + }), + ); + + let schema = json!({ + "$defs": {"Shared": {"type": "integer"}}, + "properties": { + "combined": { + "$ref": "gts://gts.x.core.events.remote.v1~#/properties/name", + "properties": { + "local": {"$ref": "#/$defs/Shared"} + } + } + } + }); + + let resolved = SchemaResolver::new(&p) + .resolve(&schema) + .expect("remote fragment with siblings should resolve"); + + assert_eq!( + resolved, + json!({ + "$defs": {"Shared": {"type": "integer"}}, + "properties": { + "combined": { + "allOf": [ + {"type": "string"}, + {"properties": {"local": {"type": "integer"}}} + ] + } + } + }) + ); +} + +#[test] +fn test_resolve_root_self_ref_is_circular() { + // `$ref: "#"` is a JSON Pointer to the document root. Since the root + // contains the ref itself, inlining is inherently recursive and must be + // detected as a cycle — NOT reported as an unresolved external ref (which + // was the bug before `#` was handled as a local pointer). + let p = MapProvider::new(); + + let schema = json!({ + "$id": "gts://gts.x.test.self.v1~", + "$schema": "http://json-schema.org/draft-07/schema#", + "type": "object", + "properties": { + "id": {"type": "string"}, + "self_ref": {"$ref": "#"} + } + }); + + assert!(matches!( + SchemaResolver::new(&p) + .resolve(&schema) + .expect_err("root self-ref must be detected as circular"), + StoreError::CircularRef + )); +} + +#[test] +fn test_resolve_root_self_ref_with_siblings_is_circular() { + // `$ref: "#"` with siblings is still circular — the root contains the + // ref, so inlining recurses. Cycle detection must fire, not UnresolvedRefs. + let p = MapProvider::new(); + + let schema = json!({ + "$id": "gts://gts.x.test.self2.v1~", + "$schema": "http://json-schema.org/draft-07/schema#", + "type": "object", + "properties": { + "id": {"type": "string"}, + "wrapped": { + "$ref": "#", + "properties": {"extra": {"type": "boolean"}} + } + } + }); + + assert!(matches!( + SchemaResolver::new(&p) + .resolve(&schema) + .expect_err("root self-ref with siblings must be circular"), + StoreError::CircularRef + )); +} + #[test] fn test_resolve_circular_ref_does_not_hang() { // A refs B, B refs A. Strict resolution must terminate and report the cycle. diff --git a/gts/src/store.rs b/gts/src/store.rs index 5894b9a..ec06874 100644 --- a/gts/src/store.rs +++ b/gts/src/store.rs @@ -243,8 +243,8 @@ impl GtsStore { self.by_id.iter() } - /// Strict `$ref` resolution that errors on an unresolved external `$ref` or - /// a circular `$ref`. + /// Strict `$ref` resolution that errors on an unresolved supported local + /// JSON Pointer or external GTS `$ref`, or a circular `$ref`. /// /// # Errors /// [`StoreError::UnresolvedRefs`] or [`StoreError::CircularRef`].