diff --git a/codex-rs/schema-evolution/src/compare.rs b/codex-rs/schema-evolution/src/compare.rs index 1c035f358e..ce875bb186 100644 --- a/codex-rs/schema-evolution/src/compare.rs +++ b/codex-rs/schema-evolution/src/compare.rs @@ -1,3 +1,5 @@ +mod array; +mod object; mod value; use crate::ApiSchema; @@ -152,6 +154,8 @@ fn compare_rules( ) -> Result<()> { value::compare_values(base.values.as_ref(), current.values.as_ref(), cx, path); value::compare_types(base.types.as_ref(), current.types.as_ref(), cx, path); + object::compare_optional(base.object.as_ref(), current.object.as_ref(), cx, path)?; + array::compare_optional(base.array.as_ref(), current.array.as_ref(), cx, path)?; value::compare_constraints(&base.constraints, ¤t.constraints, cx, path); Ok(()) } diff --git a/codex-rs/schema-evolution/src/compare/array.rs b/codex-rs/schema-evolution/src/compare/array.rs new file mode 100644 index 0000000000..4129c9d6ba --- /dev/null +++ b/codex-rs/schema-evolution/src/compare/array.rs @@ -0,0 +1,117 @@ +use super::CompareCx; +use super::CompareNarrowing; +use crate::AdditionalItems; +use crate::ArraySchema; +use crate::Items; +use crate::SchemaId; +use crate::SchemaPath; +use anyhow::Result; +use serde_json::Value; + +pub(super) fn compare_optional( + base: Option<&ArraySchema>, + current: Option<&ArraySchema>, + cx: &mut CompareCx<'_>, + path: &SchemaPath, +) -> Result<()> { + if base.is_none() && current.is_none() { + return Ok(()); + } + let unconstrained = ArraySchema { + items: Items::Any, + additional_items: AdditionalItems::Any, + }; + compare( + base.unwrap_or(&unconstrained), + current.unwrap_or(&unconstrained), + cx, + path, + ) +} + +fn compare( + base: &ArraySchema, + current: &ArraySchema, + cx: &mut CompareCx<'_>, + path: &SchemaPath, +) -> Result<()> { + let tuple_len = tuple_len(&base.items).max(tuple_len(¤t.items)); + for index in 0..tuple_len { + compare_element( + element_at(base, index), + element_at(current, index), + cx, + &path.tuple_item(index), + )?; + } + compare_element(tail(base), tail(current), cx, &path.items()) +} + +#[derive(Clone, Copy)] +enum Element { + Any, + Forbidden, + Schema(SchemaId), +} + +fn tuple_len(items: &Items) -> usize { + match items { + Items::Tuple(items) => items.len(), + Items::Any | Items::Each(_) => 0, + } +} + +fn element_at(array: &ArraySchema, index: usize) -> Element { + match &array.items { + Items::Any => Element::Any, + Items::Each(schema) => Element::Schema(*schema), + Items::Tuple(items) => items + .get(index) + .copied() + .map_or_else(|| additional(array.additional_items), Element::Schema), + } +} + +fn tail(array: &ArraySchema) -> Element { + match array.items { + Items::Any => Element::Any, + Items::Each(schema) => Element::Schema(schema), + Items::Tuple(_) => additional(array.additional_items), + } +} + +fn additional(items: AdditionalItems) -> Element { + match items { + AdditionalItems::Any => Element::Any, + AdditionalItems::Forbidden => Element::Forbidden, + AdditionalItems::Schema(schema) => Element::Schema(schema), + } +} + +fn compare_element( + base: Element, + current: Element, + cx: &mut CompareCx<'_>, + path: &SchemaPath, +) -> Result<()> { + match (base, current) { + (Element::Forbidden, _) | (_, Element::Any) => Ok(()), + (Element::Schema(base), Element::Schema(current)) => base.compare(¤t, cx, path), + (Element::Any, Element::Forbidden) => { + cx.constraint_changed(path, Value::Bool(true), Value::Bool(false)); + Ok(()) + } + (Element::Any, Element::Schema(current)) => { + cx.constraint_changed(path, Value::Bool(true), cx.current.snapshot(current)?); + Ok(()) + } + (Element::Schema(base), Element::Forbidden) => { + cx.constraint_changed(path, cx.base.snapshot(base)?, Value::Bool(false)); + Ok(()) + } + } +} + +#[cfg(test)] +#[path = "array_tests.rs"] +mod tests; diff --git a/codex-rs/schema-evolution/src/compare/array_tests.rs b/codex-rs/schema-evolution/src/compare/array_tests.rs new file mode 100644 index 0000000000..399467d041 --- /dev/null +++ b/codex-rs/schema-evolution/src/compare/array_tests.rs @@ -0,0 +1,64 @@ +use crate::ViolationKind; +use crate::test_support::breakage; +use crate::test_support::compare; +use crate::test_support::request_schema; +use pretty_assertions::assert_eq; +use serde_json::json; + +#[test] +fn walks_homogeneous_and_tuple_items() -> anyhow::Result<()> { + let base = request_schema(json!({ + "items": [{ "type": "string" }, { "type": "number" }], + "type": "array" + })); + let current = request_schema(json!({ + "items": [{ "type": "string" }, { "type": "integer" }], + "type": "array" + })); + assert_eq!( + compare(&base, ¤t)?, + vec![breakage( + ViolationKind::TypeNarrowed, + "params[1]", + json!("number"), + json!("integer"), + )] + ); + + let base = request_schema(json!({ "items": { "type": "number" }, "type": "array" })); + let current = request_schema(json!({ "items": { "type": "integer" }, "type": "array" })); + assert_eq!( + compare(&base, ¤t)?, + vec![breakage( + ViolationKind::TypeNarrowed, + "params[]", + json!("number"), + json!("integer"), + )] + ); + Ok(()) +} + +#[test] +fn detects_shortening_a_closed_tuple() -> anyhow::Result<()> { + let base = request_schema(json!({ + "additionalItems": false, + "items": [{ "type": "string" }, { "type": "number" }], + "type": "array" + })); + let current = request_schema(json!({ + "additionalItems": false, + "items": [{ "type": "string" }], + "type": "array" + })); + assert_eq!( + compare(&base, ¤t)?, + vec![breakage( + ViolationKind::ConstraintChanged, + "params[1]", + json!({ "type": "number" }), + json!(false), + )] + ); + Ok(()) +} diff --git a/codex-rs/schema-evolution/src/compare/object.rs b/codex-rs/schema-evolution/src/compare/object.rs new file mode 100644 index 0000000000..36eafb6283 --- /dev/null +++ b/codex-rs/schema-evolution/src/compare/object.rs @@ -0,0 +1,95 @@ +use super::CompareCx; +use super::CompareNarrowing; +use crate::AdditionalProperties; +use crate::AdditionalPropertiesValue; +use crate::ObjectSchema; +use crate::SchemaPath; +use crate::Violation; +use anyhow::Result; +use std::collections::BTreeMap; +use std::collections::BTreeSet; + +pub(super) fn compare_optional( + base: Option<&ObjectSchema>, + current: Option<&ObjectSchema>, + cx: &mut CompareCx<'_>, + path: &SchemaPath, +) -> Result<()> { + if base.is_none() && current.is_none() { + return Ok(()); + } + let unconstrained = ObjectSchema { + properties: BTreeMap::new(), + required: BTreeSet::new(), + additional_properties: AdditionalProperties::Any, + }; + base.unwrap_or(&unconstrained) + .compare(current.unwrap_or(&unconstrained), cx, path) +} + +impl CompareNarrowing for ObjectSchema { + fn compare(&self, current: &Self, cx: &mut CompareCx<'_>, path: &SchemaPath) -> Result<()> { + for (name, property) in &self.properties { + let property_path = path.property(name); + if let Some(current) = current.properties.get(name) { + property + .schema + .compare(¤t.schema, cx, &property_path)?; + } else { + cx.violations.push(Violation::PropertyRemoved { + at: cx.location(&property_path), + }); + } + } + for name in current.required.difference(&self.required) { + cx.violations.push(Violation::RequiredPropertyAdded { + at: cx.location(&path.property(name)), + }); + } + compare_additional_properties( + self.additional_properties, + current.additional_properties, + cx, + &path.additional_properties(), + ) + } +} + +fn compare_additional_properties( + base: AdditionalProperties, + current: AdditionalProperties, + cx: &mut CompareCx<'_>, + path: &SchemaPath, +) -> Result<()> { + match (base, current) { + (AdditionalProperties::Forbidden, _) | (_, AdditionalProperties::Any) => Ok(()), + (AdditionalProperties::Schema(base), AdditionalProperties::Schema(current)) => { + base.compare(¤t, cx, path) + } + (base, current) => { + let before = snapshot(base, cx.base)?; + let after = snapshot(current, cx.current)?; + cx.violations.push(Violation::AdditionalPropertiesNarrowed { + at: cx.location(path), + before, + after, + }); + Ok(()) + } + } +} + +fn snapshot( + value: AdditionalProperties, + schema: &crate::ApiSchema, +) -> Result { + Ok(match value { + AdditionalProperties::Any => AdditionalPropertiesValue::Any, + AdditionalProperties::Forbidden => AdditionalPropertiesValue::Forbidden, + AdditionalProperties::Schema(id) => AdditionalPropertiesValue::Schema(schema.snapshot(id)?), + }) +} + +#[cfg(test)] +#[path = "object_tests.rs"] +mod tests; diff --git a/codex-rs/schema-evolution/src/compare/object_tests.rs b/codex-rs/schema-evolution/src/compare/object_tests.rs new file mode 100644 index 0000000000..43ba884df1 --- /dev/null +++ b/codex-rs/schema-evolution/src/compare/object_tests.rs @@ -0,0 +1,102 @@ +use super::*; +use crate::ViolationKind; +use crate::test_support::breakage; +use crate::test_support::compare; +use crate::test_support::request_schema; +use crate::test_support::sorted; +use pretty_assertions::assert_eq; +use serde_json::json; + +#[test] +fn detects_property_required_and_additional_property_narrowing() -> Result<()> { + let base = request_schema(json!({ + "properties": { + "removed": { "type": "string" }, + "requiredLater": { "type": "string" } + }, + "type": "object" + })); + let current = request_schema(json!({ + "additionalProperties": false, + "properties": { "requiredLater": { "type": "string" } }, + "required": ["requiredLater"], + "type": "object" + })); + + assert_eq!( + compare(&base, ¤t)?, + sorted(vec![ + breakage( + ViolationKind::AdditionalPropertiesNarrowed, + "params.*", + json!(true), + json!(false), + ), + breakage( + ViolationKind::PropertyRemoved, + "params.removed", + json!(true), + json!(false), + ), + breakage( + ViolationKind::RequiredPropertyAdded, + "params.requiredLater", + json!(false), + json!(true), + ), + ]) + ); + Ok(()) +} + +#[test] +fn allows_object_widening() -> Result<()> { + let base = request_schema(json!({ + "additionalProperties": false, + "properties": { "value": { "type": "string" } }, + "required": ["value"], + "type": "object" + })); + let current = request_schema(json!({ + "properties": { + "newOptional": { "type": "boolean" }, + "value": { "type": "string" } + }, + "type": "object" + })); + assert_eq!(compare(&base, ¤t)?, vec![]); + Ok(()) +} + +#[test] +fn reports_each_path_that_uses_a_narrowed_shared_definition() -> Result<()> { + let mut base = request_schema(json!({ + "properties": { + "first": { "$ref": "#/definitions/Shared" }, + "second": { "$ref": "#/definitions/Shared" } + }, + "type": "object" + })); + base["definitions"] = json!({ "Shared": { "type": "number" } }); + let mut current = base.clone(); + current["definitions"]["Shared"]["type"] = json!("integer"); + + assert_eq!( + compare(&base, ¤t)?, + sorted(vec![ + breakage( + ViolationKind::TypeNarrowed, + "params.first", + json!("number"), + json!("integer"), + ), + breakage( + ViolationKind::TypeNarrowed, + "params.second", + json!("number"), + json!("integer"), + ), + ]) + ); + Ok(()) +} diff --git a/codex-rs/schema-evolution/src/compare/value_tests.rs b/codex-rs/schema-evolution/src/compare/value_tests.rs index 08f651892b..4e0f94a12b 100644 --- a/codex-rs/schema-evolution/src/compare/value_tests.rs +++ b/codex-rs/schema-evolution/src/compare/value_tests.rs @@ -2,58 +2,59 @@ use crate::ViolationKind; use crate::test_support::breakage; use crate::test_support::compare; use crate::test_support::request_schema; +use crate::test_support::sorted; use pretty_assertions::assert_eq; use serde_json::json; #[test] fn detects_enum_type_and_bound_narrowing() -> anyhow::Result<()> { + let base = request_schema(json!({ + "properties": { + "choice": { "enum": ["a", "b"], "type": "string" }, + "length": { "minLength": 1, "type": "string" }, + "numeric": { "type": "number" }, + "value": { "type": ["string", "null"] } + }, + "type": "object" + })); + let current = request_schema(json!({ + "properties": { + "choice": { "enum": ["a"], "type": "string" }, + "length": { "minLength": 2, "type": "string" }, + "numeric": { "type": "integer" }, + "value": { "type": "string" } + }, + "type": "object" + })); + assert_eq!( - compare( - &request_schema(json!({ "enum": ["a", "b"], "type": "string" })), - &request_schema(json!({ "enum": ["a"], "type": "string" })), - )?, - vec![breakage( - ViolationKind::EnumNarrowed, - "params", - json!(["a", "b"]), - json!(["a"]), - )] - ); - assert_eq!( - compare( - &request_schema(json!({ "type": "number" })), - &request_schema(json!({ "type": "integer" })), - )?, - vec![breakage( - ViolationKind::TypeNarrowed, - "params", - json!("number"), - json!("integer"), - )] - ); - assert_eq!( - compare( - &request_schema(json!({ "type": ["string", "null"] })), - &request_schema(json!({ "type": "string" })), - )?, - vec![breakage( - ViolationKind::TypeNarrowed, - "params", - json!(["null", "string"]), - json!("string"), - )] - ); - assert_eq!( - compare( - &request_schema(json!({ "minLength": 1, "type": "string" })), - &request_schema(json!({ "minLength": 2, "type": "string" })), - )?, - vec![breakage( - ViolationKind::ConstraintChanged, - "params", - json!({ "minLength": 1 }), - json!({ "minLength": 2 }), - )] + compare(&base, ¤t)?, + sorted(vec![ + breakage( + ViolationKind::EnumNarrowed, + "params.choice", + json!(["a", "b"]), + json!(["a"]), + ), + breakage( + ViolationKind::ConstraintChanged, + "params.length", + json!({ "minLength": 1 }), + json!({ "minLength": 2 }), + ), + breakage( + ViolationKind::TypeNarrowed, + "params.numeric", + json!("number"), + json!("integer"), + ), + breakage( + ViolationKind::TypeNarrowed, + "params.value", + json!(["null", "string"]), + json!("string"), + ), + ]) ); Ok(()) } diff --git a/codex-rs/schema-evolution/src/compare_tests.rs b/codex-rs/schema-evolution/src/compare_tests.rs index 1698fc1d7d..dce17fe06d 100644 --- a/codex-rs/schema-evolution/src/compare_tests.rs +++ b/codex-rs/schema-evolution/src/compare_tests.rs @@ -39,22 +39,38 @@ fn compares_typed_arguments_with_the_full_request_schema() -> Result<()> { )] ); - Ok(()) -} - -#[test] -fn retains_request_level_constraints_in_the_typed_envelope() -> Result<()> { let base = request_schema(json!({ "type": "null" })); let mut current = request_schema(json!({ "type": "null" })); - current["oneOf"][0]["minProperties"] = json!(3); - + current["oneOf"][0]["properties"]["id"] = json!({ "type": "string" }); let mut expected = breakage( - ViolationKind::ConstraintChanged, - "request", - json!({}), - json!({ "minProperties": 3 }), + ViolationKind::TypeNarrowed, + "id", + json!(["integer", "string"]), + json!("string"), ); expected.method = "*".to_string(); assert_eq!(compare(&base, ¤t)?, vec![expected]); Ok(()) } + +#[test] +fn retains_method_and_request_level_constraints_in_the_typed_envelope() -> Result<()> { + let mut base = request_schema(json!({ "type": "null" })); + base["oneOf"][0]["required"] = json!(["id", "params"]); + let mut current = request_schema(json!({ "type": "null" })); + current["oneOf"][0]["minProperties"] = json!(3); + + let violations = compare(&base, ¤t)?; + assert_eq!(violations.len(), 2); + assert!(violations.iter().any(|violation| { + violation.kind == ViolationKind::RequiredPropertyAdded + && violation.method == "*" + && violation.path == "method" + })); + assert!(violations.iter().any(|violation| { + violation.kind == ViolationKind::ConstraintChanged + && violation.method == "*" + && violation.path == "request" + })); + Ok(()) +} diff --git a/codex-rs/schema-evolution/src/lib.rs b/codex-rs/schema-evolution/src/lib.rs index fd4c606ecb..9b20caa383 100644 --- a/codex-rs/schema-evolution/src/lib.rs +++ b/codex-rs/schema-evolution/src/lib.rs @@ -7,10 +7,13 @@ mod violation; mod test_support; use compare::compare_api_schemas; +pub(crate) use model::AdditionalItems; +pub(crate) use model::AdditionalProperties; pub use model::ApiSchema; pub(crate) use model::Arguments; pub(crate) use model::ArraySchema; pub(crate) use model::ConstraintSet; +pub(crate) use model::Items; pub(crate) use model::JsonType; pub(crate) use model::Method; pub(crate) use model::ObjectSchema; @@ -21,6 +24,7 @@ pub(crate) use model::TypeSet; pub(crate) use model::UnionKind; pub(crate) use model::UnionSchema; pub(crate) use model::ValueSet; +pub(crate) use violation::AdditionalPropertiesValue; pub(crate) use violation::Location; pub use violation::SchemaBreakage; pub(crate) use violation::SchemaPath; @@ -30,7 +34,7 @@ pub use violation::ViolationKind; use anyhow::Result; -/// Finds values that the current request schema no longer accepts. +/// Finds request values accepted by `base` that are rejected by `current`. pub fn find_request_narrowing( base: &ApiSchema, current: &ApiSchema, diff --git a/codex-rs/schema-evolution/src/model.rs b/codex-rs/schema-evolution/src/model.rs index a1f2e9b0fb..0e8aaec020 100644 --- a/codex-rs/schema-evolution/src/model.rs +++ b/codex-rs/schema-evolution/src/model.rs @@ -5,7 +5,9 @@ mod object; mod union; mod value; +pub(crate) use array::AdditionalItems; pub(crate) use array::ArraySchema; +pub(crate) use array::Items; pub(crate) use constraints::ConstraintSet; pub(crate) use constraints::annotation; pub(crate) use constraints::normalized; @@ -13,6 +15,7 @@ pub(crate) use constraints::normalized; pub(crate) use method::Argument; pub(crate) use method::Arguments; pub(crate) use method::Method; +pub(crate) use object::AdditionalProperties; pub(crate) use object::ObjectSchema; pub(crate) use union::UnionKind; pub(crate) use union::UnionSchema; diff --git a/codex-rs/schema-evolution/src/test_support.rs b/codex-rs/schema-evolution/src/test_support.rs index 1fff6af22b..4235cc8a6e 100644 --- a/codex-rs/schema-evolution/src/test_support.rs +++ b/codex-rs/schema-evolution/src/test_support.rs @@ -43,3 +43,8 @@ pub(crate) fn breakage( after, } } + +pub(crate) fn sorted(mut violations: Vec) -> Vec { + violations.sort_by_key(|violation| serde_json::to_string(violation).unwrap()); + violations +} diff --git a/codex-rs/schema-evolution/src/violation.rs b/codex-rs/schema-evolution/src/violation.rs index c7a2f1f95f..98eb78937d 100644 --- a/codex-rs/schema-evolution/src/violation.rs +++ b/codex-rs/schema-evolution/src/violation.rs @@ -39,6 +39,11 @@ pub enum Violation { before: Option, after: ValueSet, }, + AdditionalPropertiesNarrowed { + at: Location, + before: AdditionalPropertiesValue, + after: AdditionalPropertiesValue, + }, ConstraintChanged { at: Location, before: SchemaSnapshot, @@ -64,6 +69,16 @@ pub struct SchemaPath(Vec); #[derive(Clone, Debug, Eq, Hash, PartialEq)] enum PathSegment { Property(String), + Items, + TupleItem(usize), + AdditionalProperties, +} + +#[derive(Clone, Debug, Eq, PartialEq)] +pub enum AdditionalPropertiesValue { + Any, + Forbidden, + Schema(Value), } #[derive(Clone, Debug, Eq, PartialEq)] @@ -95,6 +110,24 @@ impl SchemaPath { path } + pub(crate) fn items(&self) -> Self { + let mut path = self.clone(); + path.0.push(PathSegment::Items); + path + } + + pub(crate) fn tuple_item(&self, index: usize) -> Self { + let mut path = self.clone(); + path.0.push(PathSegment::TupleItem(index)); + path + } + + pub(crate) fn additional_properties(&self) -> Self { + let mut path = self.clone(); + path.0.push(PathSegment::AdditionalProperties); + path + } + pub(crate) fn starts_with_params(&self) -> bool { matches!(self.0.first(), Some(PathSegment::Property(name)) if name == "params") } @@ -106,12 +139,26 @@ impl fmt::Display for SchemaPath { match segment { PathSegment::Property(name) if index == 0 => formatter.write_str(name)?, PathSegment::Property(name) => write!(formatter, ".{name}")?, + PathSegment::Items => formatter.write_str("[]")?, + PathSegment::TupleItem(index) => write!(formatter, "[{index}]")?, + PathSegment::AdditionalProperties if index == 0 => formatter.write_str("*")?, + PathSegment::AdditionalProperties => formatter.write_str(".*")?, } } Ok(()) } } +impl AdditionalPropertiesValue { + fn to_json(&self) -> Value { + match self { + Self::Any => Value::Bool(true), + Self::Forbidden => Value::Bool(false), + Self::Schema(value) => value.clone(), + } + } +} + impl Violation { pub fn breakage(&self) -> SchemaBreakage { match self { @@ -146,6 +193,12 @@ impl Violation { before.as_ref().map_or(Value::Null, ValueSet::to_json), after.to_json(), ), + Self::AdditionalPropertiesNarrowed { at, before, after } => at_location( + ViolationKind::AdditionalPropertiesNarrowed, + at, + before.to_json(), + after.to_json(), + ), Self::ConstraintChanged { at, before, after } => at_location( ViolationKind::ConstraintChanged, at, @@ -162,6 +215,7 @@ impl Violation { | Self::RequiredPropertyAdded { at } | Self::TypeNarrowed { at, .. } | Self::EnumNarrowed { at, .. } + | Self::AdditionalPropertiesNarrowed { at, .. } | Self::ConstraintChanged { at, .. } => Some(at), } } @@ -179,6 +233,7 @@ impl Violation { | Self::RequiredPropertyAdded { at } | Self::TypeNarrowed { at, .. } | Self::EnumNarrowed { at, .. } + | Self::AdditionalPropertiesNarrowed { at, .. } | Self::ConstraintChanged { at, .. } => Some(at), } } @@ -199,3 +254,7 @@ fn at_location(kind: ViolationKind, at: &Location, before: Value, after: Value) after, } } + +#[cfg(test)] +#[path = "violation_tests.rs"] +mod tests; diff --git a/codex-rs/schema-evolution/src/violation_tests.rs b/codex-rs/schema-evolution/src/violation_tests.rs new file mode 100644 index 0000000000..46085bb69b --- /dev/null +++ b/codex-rs/schema-evolution/src/violation_tests.rs @@ -0,0 +1,31 @@ +use super::*; +use pretty_assertions::assert_eq; +use serde_json::json; + +#[test] +fn breakage_projection_renders_typed_scope_paths_and_payloads() { + let at = Location::method( + "test/method", + SchemaPath::default() + .property("params") + .property("items") + .tuple_item(/*index*/ 1) + .additional_properties(), + ); + let violation = Violation::ConstraintChanged { + at, + before: SchemaSnapshot(json!({ "minimum": 0 })), + after: SchemaSnapshot(json!({ "minimum": 1 })), + }; + + assert_eq!( + violation.breakage(), + SchemaBreakage { + kind: ViolationKind::ConstraintChanged, + method: "test/method".to_string(), + path: "params.items[1].*".to_string(), + before: json!({ "minimum": 0 }), + after: json!({ "minimum": 1 }), + } + ); +}