diff --git a/src/schema.rs b/src/schema.rs index 4948939..e6d68c8 100644 --- a/src/schema.rs +++ b/src/schema.rs @@ -1,13 +1,16 @@ -// Copyright 2025 Oxide Computer Company +// Copyright 2026 Oxide Computer Company use std::fmt; -use openapiv3::{AdditionalProperties, ArrayType, ObjectType, ReferenceOr, Schema, SchemaData}; +use openapiv3::{ + AdditionalProperties, ArrayType, ObjectType, ReferenceOr, Schema, SchemaData, SchemaKind, +}; use crate::{ ChangeClass, ChangeComparison, ChangeDetails, compare::Compare, - context::{Contextual, ToContext}, + context::{Context, Contextual, ToContext}, + resolve::ReferenceOrResolver, setops::SetCompare, }; @@ -292,29 +295,27 @@ impl Compare { &mut self, comparison: SchemaComparison, dry_run: bool, - old_schema_kind: Contextual<'_, &openapiv3::SchemaKind>, - new_schema_kind: Contextual<'_, &openapiv3::SchemaKind>, + old_schema_kind: Contextual<'_, &SchemaKind>, + new_schema_kind: Contextual<'_, &SchemaKind>, ) -> anyhow::Result { match (old_schema_kind.as_ref(), new_schema_kind.as_ref()) { - (&openapiv3::SchemaKind::Type(old_type), &openapiv3::SchemaKind::Type(new_type)) => { - self.compare_schema_type( - comparison, - dry_run, - old_schema_kind.subcomponent(old_type), - new_schema_kind.subcomponent(new_type), - ) - } + (&SchemaKind::Type(old_type), &SchemaKind::Type(new_type)) => self.compare_schema_type( + comparison, + dry_run, + old_schema_kind.subcomponent(old_type), + new_schema_kind.subcomponent(new_type), + ), ( - openapiv3::SchemaKind::OneOf { one_of: old_one_of }, - openapiv3::SchemaKind::OneOf { one_of: new_one_of }, + SchemaKind::OneOf { one_of: old_one_of }, + SchemaKind::OneOf { one_of: new_one_of }, ) => { let old_one_of = old_schema_kind.append_deref(old_one_of, "oneOf"); let new_one_of = new_schema_kind.append_deref(new_one_of, "oneOf"); self.compare_schema_type_one_of(comparison, dry_run, old_one_of, new_one_of) } ( - openapiv3::SchemaKind::AllOf { all_of: old_all_of }, - openapiv3::SchemaKind::AllOf { all_of: new_all_of }, + SchemaKind::AllOf { all_of: old_all_of }, + SchemaKind::AllOf { all_of: new_all_of }, ) => { let old_all_of = old_schema_kind.append_deref(old_all_of, "allOf"); let new_all_of = new_schema_kind.append_deref(new_all_of, "allOf"); @@ -322,8 +323,8 @@ impl Compare { } ( - openapiv3::SchemaKind::AnyOf { any_of: old_any_of }, - openapiv3::SchemaKind::AnyOf { any_of: new_any_of }, + SchemaKind::AnyOf { any_of: old_any_of }, + SchemaKind::AnyOf { any_of: new_any_of }, ) => { if old_any_of != new_any_of { self.schema_push_change( @@ -339,15 +340,12 @@ impl Compare { Ok(true) } } - ( - openapiv3::SchemaKind::Not { not: old_not }, - openapiv3::SchemaKind::Not { not: new_not }, - ) => { + (SchemaKind::Not { not: old_not }, SchemaKind::Not { not: new_not }) => { let old_not = old_schema_kind.append_deref(old_not.as_ref(), "not"); let new_not = new_schema_kind.append_deref(new_not.as_ref(), "not"); self.compare_schema_ref_helper(dry_run, comparison, old_not, new_not) } - (&openapiv3::SchemaKind::Any(old_any), &openapiv3::SchemaKind::Any(new_any)) => { + (&SchemaKind::Any(old_any), &SchemaKind::Any(new_any)) => { if old_any == new_any { Ok(true) } else { @@ -363,6 +361,37 @@ impl Compare { } } _ => { + // Consider the case where both old and new are -- effectively + // -- an enum of values. This might be an enum (with or without + // a type), or a oneOf where each subschema is an enum (again, + // with or without a type). + if let (Some(old_enum), Some(new_enum)) = ( + extract_enum_values(old_schema_kind.context(), old_schema_kind.as_ref()), + extract_enum_values(new_schema_kind.context(), new_schema_kind.as_ref()), + ) { + // We don't care about the order of enumerated values; yes, + // this is an inefficient way to check, but with + // serde_json::Value not implementing Hash or Ord... meh. + if old_enum.len() == new_enum.len() + && old_enum.iter().all(|v| new_enum.contains(v)) + { + let old_tag = SchemaKindTag::new(&old_schema_kind); + let new_tag = SchemaKindTag::new(&new_schema_kind); + return self.schema_push_change( + dry_run, + format!( + "schema kind changed from {} to {} with equivalent enum values", + old_tag, new_tag + ), + &old_schema_kind, + &new_schema_kind, + comparison, + ChangeClass::Trivial, + ChangeDetails::Metadata, + ); + } + } + let old_tag = SchemaKindTag::new(&old_schema_kind); let new_tag = SchemaKindTag::new(&new_schema_kind); self.schema_push_change( @@ -821,14 +850,14 @@ impl fmt::Display for SchemaKindTag { } impl SchemaKindTag { - fn new(kind: &openapiv3::SchemaKind) -> Self { + fn new(kind: &SchemaKind) -> Self { match kind { - openapiv3::SchemaKind::Type(_) => Self::Type, - openapiv3::SchemaKind::OneOf { .. } => Self::OneOf, - openapiv3::SchemaKind::AllOf { .. } => Self::AllOf, - openapiv3::SchemaKind::AnyOf { .. } => Self::AnyOf, - openapiv3::SchemaKind::Not { .. } => Self::Not, - openapiv3::SchemaKind::Any { .. } => Self::Any, + SchemaKind::Type(_) => Self::Type, + SchemaKind::OneOf { .. } => Self::OneOf, + SchemaKind::AllOf { .. } => Self::AllOf, + SchemaKind::AnyOf { .. } => Self::AnyOf, + SchemaKind::Not { .. } => Self::Not, + SchemaKind::Any { .. } => Self::Any, } } } @@ -859,31 +888,25 @@ fn classify_schema_ref(schema_ref: &ReferenceOr) -> SchemaRefKind<'_> { match schema_ref { ReferenceOr::Reference { .. } => SchemaRefKind::BareRef, ReferenceOr::Item(schema) => match &schema.schema_kind { - openapiv3::SchemaKind::Type(_) - | openapiv3::SchemaKind::Not { .. } - | openapiv3::SchemaKind::Any(_) => SchemaRefKind::InlineType, - openapiv3::SchemaKind::AllOf { all_of } if all_of.len() == 1 => { - SchemaRefKind::SingleElement { - inner: all_of.first().unwrap(), - metadata: &schema.schema_data, - } - } - openapiv3::SchemaKind::AnyOf { any_of } if any_of.len() == 1 => { - SchemaRefKind::SingleElement { - inner: any_of.first().unwrap(), - metadata: &schema.schema_data, - } - } - openapiv3::SchemaKind::OneOf { one_of } if one_of.len() == 1 => { - SchemaRefKind::SingleElement { - inner: one_of.first().unwrap(), - metadata: &schema.schema_data, - } + SchemaKind::Type(_) | SchemaKind::Not { .. } | SchemaKind::Any(_) => { + SchemaRefKind::InlineType } + SchemaKind::AllOf { all_of } if all_of.len() == 1 => SchemaRefKind::SingleElement { + inner: all_of.first().unwrap(), + metadata: &schema.schema_data, + }, + SchemaKind::AnyOf { any_of } if any_of.len() == 1 => SchemaRefKind::SingleElement { + inner: any_of.first().unwrap(), + metadata: &schema.schema_data, + }, + SchemaKind::OneOf { one_of } if one_of.len() == 1 => SchemaRefKind::SingleElement { + inner: one_of.first().unwrap(), + metadata: &schema.schema_data, + }, // Multi-element wrappers - not semantically equivalent to single schemas. - openapiv3::SchemaKind::AllOf { .. } - | openapiv3::SchemaKind::AnyOf { .. } - | openapiv3::SchemaKind::OneOf { .. } => SchemaRefKind::MultiElement, + SchemaKind::AllOf { .. } | SchemaKind::AnyOf { .. } | SchemaKind::OneOf { .. } => { + SchemaRefKind::MultiElement + } }, } } @@ -893,3 +916,71 @@ fn classify_schema_ref(schema_ref: &ReferenceOr) -> SchemaRefKind<'_> { fn has_meaningful_metadata(data: &SchemaData) -> bool { *data != SchemaData::default() } + +/// For a schema that is **exclusively** enumerated values, this returns a Some +/// of those enumerated values. Note that this will never return Some of an +/// empty Vec--there must always be at least a single enumerated value. +fn extract_enum_values(context: &Context<'_>, kind: &SchemaKind) -> Option> { + match kind { + // For untyped schemas, we can use the enum field as-is. + SchemaKind::Any(any) if !any.enumeration.is_empty() => Some(any.enumeration.clone()), + + // For typed schemas, convert them to Values of the appropriate type. + SchemaKind::Type(openapiv3::Type::String(s)) if !s.enumeration.is_empty() => Some( + s.enumeration + .iter() + .map(|v| match v { + Some(s) => serde_json::Value::String(s.clone()), + None => serde_json::Value::Null, + }) + .collect(), + ), + SchemaKind::Type(openapiv3::Type::Integer(i)) if !i.enumeration.is_empty() => Some( + i.enumeration + .iter() + .map(|v| match v { + Some(n) => serde_json::Value::Number((*n).into()), + None => serde_json::Value::Null, + }) + .collect(), + ), + SchemaKind::Type(openapiv3::Type::Number(n)) if !n.enumeration.is_empty() => Some( + n.enumeration + .iter() + .map(|v| match v { + Some(f) => serde_json::Number::from_f64(*f) + .map(serde_json::Value::Number) + .unwrap_or(serde_json::Value::Null), + None => serde_json::Value::Null, + }) + .collect(), + ), + SchemaKind::Type(openapiv3::Type::Boolean(b)) if !b.enumeration.is_empty() => Some( + b.enumeration + .iter() + .map(|v| match v { + Some(b) => serde_json::Value::Bool(*b), + None => serde_json::Value::Null, + }) + .collect(), + ), + + // A oneOf may be composed exclusively of enums in which case we can + // flatten them into a single collection. A oneOf isn't supposed to be + // empty... but we'll still check. + SchemaKind::OneOf { one_of } if !one_of.is_empty() => Some( + one_of + .iter() + .map(|schema_ref| { + let (schema, _) = schema_ref.resolve(context).ok()?; + extract_enum_values(context, &schema.schema_kind) + }) + .collect::>>()? + .into_iter() + .flatten() + .collect(), + ), + + _ => None, + } +} diff --git a/tests/cases/simple/base.json b/tests/cases/simple/base.json index 8318c57..278c828 100644 --- a/tests/cases/simple/base.json +++ b/tests/cases/simple/base.json @@ -395,6 +395,12 @@ "properties": { "name": { "type": "string" + }, + "kind": { + "$ref": "#/components/schemas/ItemKind" + }, + "type": { + "$ref": "#/components/schemas/ItemType" } }, "required": [ @@ -500,6 +506,39 @@ }, "RefChainB": { "$ref": "#/components/schemas/SubType" + }, + "ItemKind": { + "enum": [ + "compassionate", + "thoughtful", + "caring", + "considerate" + ] + }, + "ItemType": { + "oneOf": [ + { + "description": "verb; to enter (data, text, etc.) by means of a keyboard", + "type": "string", + "enum": [ + "keyboard" + ] + }, + { + "description": "verb; to enter (data) into a computer or data processing system", + "type": "string", + "enum": [ + "input" + ] + }, + { + "description": "verb; to form letters, words, or symbols on a surface (such as paper or a screen) using an instrument like a pen, pencil, or keyboard to communicate, record information, or create literature", + "type": "string", + "enum": [ + "write" + ] + } + ] } } } diff --git a/tests/cases/simple/output/add-type-extension.out b/tests/cases/simple/output/add-type-extension.out index 306f21f..e225bcf 100644 --- a/tests/cases/simple/output/add-type-extension.out +++ b/tests/cases/simple/output/add-type-extension.out @@ -10,8 +10,8 @@ + "mumble": "frotz" + } }, - "MultiAllOf": { - "allOf": [ + "ItemKind": { + "enum": [ Result for patch: diff --git a/tests/cases/simple/output/enum-to-oneof.out b/tests/cases/simple/output/enum-to-oneof.out new file mode 100644 index 0000000..402250f --- /dev/null +++ b/tests/cases/simple/output/enum-to-oneof.out @@ -0,0 +1,60 @@ +--- enum-to-oneof.json ++++ patched +@@ + "type": "object" + }, + "ItemKind": { +- "enum": [ +- "compassionate", +- "thoughtful", +- "caring", +- "considerate" ++ "oneOf": [ ++ { ++ "enum": [ ++ "compassionate" ++ ], ++ "type": "string" ++ }, ++ { ++ "enum": [ ++ "thoughtful" ++ ], ++ "type": "string" ++ }, ++ { ++ "enum": [ ++ "caring" ++ ], ++ "type": "string" ++ }, ++ { ++ "enum": [ ++ "considerate" ++ ], ++ "type": "string" ++ } + ] + }, + "ItemType": { + + +Result for patch: +[ + Change { + message: "schema kind changed from any to oneOf with equivalent enum values", + old_path: [ + "#/components/schemas/ItemKind", + "#/components/schemas/CreateItem/properties/kind/$ref", + "#/paths/~1items/post/request_body/content/application~1json/schema/$ref", + ], + new_path: [ + "#/components/schemas/ItemKind", + "#/components/schemas/CreateItem/properties/kind/$ref", + "#/paths/~1items/post/request_body/content/application~1json/schema/$ref", + ], + comparison: Input, + class: Trivial, + details: Metadata, + }, +] diff --git a/tests/cases/simple/output/oneof-to-enum.out b/tests/cases/simple/output/oneof-to-enum.out new file mode 100644 index 0000000..1baa069 --- /dev/null +++ b/tests/cases/simple/output/oneof-to-enum.out @@ -0,0 +1,56 @@ +--- oneof-to-enum.json ++++ patched +@@ + ] + }, + "ItemType": { +- "oneOf": [ +- { +- "description": "verb; to enter (data, text, etc.) by means of a keyboard", +- "enum": [ +- "keyboard" +- ], +- "type": "string" +- }, +- { +- "description": "verb; to enter (data) into a computer or data processing system", +- "enum": [ +- "input" +- ], +- "type": "string" +- }, +- { +- "description": "verb; to form letters, words, or symbols on a surface (such as paper or a screen) using an instrument like a pen, pencil, or keyboard to communicate, record information, or create literature", +- "enum": [ +- "write" +- ], +- "type": "string" +- } ++ "enum": [ ++ "keyboard", ++ "input", ++ "write" + ] + }, + "MultiAllOf": { + + +Result for patch: +[ + Change { + message: "schema kind changed from oneOf to any with equivalent enum values", + old_path: [ + "#/components/schemas/ItemType", + "#/components/schemas/CreateItem/properties/type/$ref", + "#/paths/~1items/post/request_body/content/application~1json/schema/$ref", + ], + new_path: [ + "#/components/schemas/ItemType", + "#/components/schemas/CreateItem/properties/type/$ref", + "#/paths/~1items/post/request_body/content/application~1json/schema/$ref", + ], + comparison: Input, + class: Trivial, + details: Metadata, + }, +] diff --git a/tests/cases/simple/output/type-indirection.out b/tests/cases/simple/output/type-indirection.out index 48bdd43..0aed074 100644 --- a/tests/cases/simple/output/type-indirection.out +++ b/tests/cases/simple/output/type-indirection.out @@ -21,9 +21,9 @@ + "jank": true + } + }, - "MultiAllOf": { - "allOf": [ - { + "ItemKind": { + "enum": [ + "compassionate", Result for patch: diff --git a/tests/cases/simple/output/untyped-to-typed-enum.out b/tests/cases/simple/output/untyped-to-typed-enum.out new file mode 100644 index 0000000..48a734f --- /dev/null +++ b/tests/cases/simple/output/untyped-to-typed-enum.out @@ -0,0 +1,33 @@ +--- untyped-to-typed-enum.json ++++ patched +@@ + "thoughtful", + "caring", + "considerate" +- ] ++ ], ++ "type": "string" + }, + "ItemType": { + "oneOf": [ + + +Result for patch: +[ + Change { + message: "schema kind changed from any to regular type with equivalent enum values", + old_path: [ + "#/components/schemas/ItemKind", + "#/components/schemas/CreateItem/properties/kind/$ref", + "#/paths/~1items/post/request_body/content/application~1json/schema/$ref", + ], + new_path: [ + "#/components/schemas/ItemKind", + "#/components/schemas/CreateItem/properties/kind/$ref", + "#/paths/~1items/post/request_body/content/application~1json/schema/$ref", + ], + comparison: Input, + class: Trivial, + details: Metadata, + }, +] diff --git a/tests/cases/simple/patch/enum-to-oneof.json b/tests/cases/simple/patch/enum-to-oneof.json new file mode 100644 index 0000000..cbb24e7 --- /dev/null +++ b/tests/cases/simple/patch/enum-to-oneof.json @@ -0,0 +1,34 @@ +[ + { + "op": "replace", + "path": "/components/schemas/ItemKind", + "value": { + "oneOf": [ + { + "type": "string", + "enum": [ + "compassionate" + ] + }, + { + "type": "string", + "enum": [ + "thoughtful" + ] + }, + { + "type": "string", + "enum": [ + "caring" + ] + }, + { + "type": "string", + "enum": [ + "considerate" + ] + } + ] + } + } +] diff --git a/tests/cases/simple/patch/oneof-to-enum.json b/tests/cases/simple/patch/oneof-to-enum.json new file mode 100644 index 0000000..82cc0d1 --- /dev/null +++ b/tests/cases/simple/patch/oneof-to-enum.json @@ -0,0 +1,13 @@ +[ + { + "op": "replace", + "path": "/components/schemas/ItemType", + "value": { + "enum": [ + "keyboard", + "input", + "write" + ] + } + } +] diff --git a/tests/cases/simple/patch/untyped-to-typed-enum.json b/tests/cases/simple/patch/untyped-to-typed-enum.json new file mode 100644 index 0000000..fa534b4 --- /dev/null +++ b/tests/cases/simple/patch/untyped-to-typed-enum.json @@ -0,0 +1,7 @@ +[ + { + "op": "add", + "path": "/components/schemas/ItemKind/type", + "value": "string" + } +] diff --git a/tests/test_changes.rs b/tests/test_changes.rs index 59d565b..5088847 100644 --- a/tests/test_changes.rs +++ b/tests/test_changes.rs @@ -34,6 +34,7 @@ fn test_change() { // Iterate through every file in the "patch" subdirectory. let patch_dir = path.join("patch"); let output_dir = path.join("output"); + let mut aok = true; for patch_entry in std::fs::read_dir(&patch_dir).unwrap() { let patch_entry = patch_entry.unwrap(); println!(" Considering patch {:?}", patch_entry.file_name()); @@ -79,9 +80,19 @@ fn test_change() { let output = format!("{udiff}\n\nResult for patch:\n{:#?}\n", result); let mut output_path = output_dir.join(patch_entry.file_name()); output_path.set_extension("out"); - expectorate::assert_contents(output_path, &output); + + if std::panic::catch_unwind(|| { + expectorate::assert_contents(output_path, &output) + }) + .is_err() + { + aok = false; + } } } + if !aok { + panic!("One or more patches didn't match their output files; see above"); + } } } }