From 35c1a04b25895ac1c3d1a71244c2c05ea22af7e1 Mon Sep 17 00:00:00 2001 From: Tormod Gjeitnes Hellen Date: Wed, 10 Apr 2024 15:03:00 +0200 Subject: [PATCH 01/14] Add new subchema merge error type --- typify-impl/src/convert.rs | 7 +++++-- typify-impl/src/merge.rs | 31 ++++++++++++++++++++++++------- 2 files changed, 29 insertions(+), 9 deletions(-) diff --git a/typify-impl/src/convert.rs b/typify-impl/src/convert.rs index ac8a7a02..04199837 100644 --- a/typify-impl/src/convert.rs +++ b/typify-impl/src/convert.rs @@ -8,7 +8,7 @@ use crate::type_entry::{ Variant, VariantDetails, }; use crate::util::{all_mutually_exclusive, recase, ref_key, Case, StringValidator}; -use log::{debug, info}; +use log::{debug, error, info}; use schemars::schema::{ ArrayValidation, InstanceType, Metadata, ObjectValidation, Schema, SchemaObject, SingleOrVec, StringValidation, SubschemaValidation, @@ -555,7 +555,10 @@ impl TypeSpace { Ok((type_entry, &None)) } - Err(_) => self.convert_never(type_name, original_schema), + Err(e) => { + error!("Error in convert_schema_object: {:#?}", e); + self.convert_never(type_name, original_schema) + } } } diff --git a/typify-impl/src/merge.rs b/typify-impl/src/merge.rs index 6ad751da..275d408e 100644 --- a/typify-impl/src/merge.rs +++ b/typify-impl/src/merge.rs @@ -10,6 +10,7 @@ use schemars::schema::{ ArrayValidation, InstanceType, NumberValidation, ObjectValidation, Schema, SchemaObject, SingleOrVec, StringValidation, SubschemaValidation, }; +use thiserror::Error; use crate::{util::ref_key, validate::schema_value_validate, RefKey}; @@ -196,8 +197,10 @@ fn merge_schema_object( // two schemas and then do the appropriate merge with subschemas (i.e. // potentially twice). This is effectively an `allOf` between the merged // "body" schema and the component subschemas. - merged_schema = try_merge_with_subschemas(merged_schema, a.subschemas.as_deref(), defs)?; - merged_schema = try_merge_with_subschemas(merged_schema, b.subschemas.as_deref(), defs)?; + merged_schema = + try_merge_with_subschemas(merged_schema, a.subschemas.as_deref(), defs).map_err(|_| ())?; + merged_schema = + try_merge_with_subschemas(merged_schema, b.subschemas.as_deref(), defs).map_err(|_| ())?; assert_ne!(merged_schema, Schema::Bool(false).into_object()); @@ -266,6 +269,18 @@ fn merge_so_enum_values( } } +#[derive(Error, Debug)] +pub(crate) enum SubschemaMergeError { + #[error("Empty merged AnyOf subschema")] + EmptyMergedAnyOfSubschema, + #[error("Empty merged OneOf subschema")] + EmptyMergedOneOfSubschema, + #[error("try_merge_schema_not error")] + NotSchemaMerge, + #[error("try_merge_schema_not error")] + AllOfSchemaMerge, +} + /// Merge the schema with a subschema validation object. It's important that /// the return value reduces the complexity of the problem so avoid infinite /// recursion. @@ -273,7 +288,7 @@ pub(crate) fn try_merge_with_subschemas( mut schema_object: SchemaObject, maybe_subschemas: Option<&SubschemaValidation>, defs: &BTreeMap, -) -> Result { +) -> Result { let Some(SubschemaValidation { all_of, any_of, @@ -296,13 +311,15 @@ pub(crate) fn try_merge_with_subschemas( .iter() .try_fold(schema_object.into(), |schema, other| { try_merge_schema(&schema, other, defs) - })?; + }) + .map_err(|_| SubschemaMergeError::AllOfSchemaMerge)?; assert_ne!(merged_schema, Schema::Bool(false)); schema_object = merged_schema.into_object(); } if let Some(not) = not { - schema_object = try_merge_schema_not(schema_object, not.as_ref(), defs)?; + schema_object = try_merge_schema_not(schema_object, not.as_ref(), defs) + .map_err(|_| SubschemaMergeError::NotSchemaMerge)?; } // TODO: we should be able to handle a combined one_of and any_of... but @@ -314,7 +331,7 @@ pub(crate) fn try_merge_with_subschemas( let merged_subschemas = try_merge_with_each_subschema(&schema_object, any_of, defs); match merged_subschemas.len() { - 0 => return Err(()), + 0 => return Err(SubschemaMergeError::EmptyMergedAnyOfSubschema), 1 => schema_object = merged_subschemas.into_iter().next().unwrap().into_object(), _ => { schema_object = SchemaObject { @@ -333,7 +350,7 @@ pub(crate) fn try_merge_with_subschemas( let merged_subschemas = try_merge_with_each_subschema(&schema_object, one_of, defs); match merged_subschemas.len() { - 0 => return Err(()), + 0 => return Err(SubschemaMergeError::EmptyMergedOneOfSubschema), 1 => schema_object = merged_subschemas.into_iter().next().unwrap().into_object(), _ => { schema_object = SchemaObject { From 224bfb9aa278d3c06739ccf8e205e2f358045cd7 Mon Sep 17 00:00:00 2001 From: Tormod Gjeitnes Hellen Date: Fri, 3 May 2024 16:45:09 +0200 Subject: [PATCH 02/14] Add NotSchemaMergeError --- typify-impl/src/merge.rs | 30 ++++++++++++++++++++++-------- 1 file changed, 22 insertions(+), 8 deletions(-) diff --git a/typify-impl/src/merge.rs b/typify-impl/src/merge.rs index 275d408e..cae46846 100644 --- a/typify-impl/src/merge.rs +++ b/typify-impl/src/merge.rs @@ -276,7 +276,7 @@ pub(crate) enum SubschemaMergeError { #[error("Empty merged OneOf subschema")] EmptyMergedOneOfSubschema, #[error("try_merge_schema_not error")] - NotSchemaMerge, + NotSchemaMerge(#[from] NotSchemaMergeError), #[error("try_merge_schema_not error")] AllOfSchemaMerge, } @@ -318,8 +318,7 @@ pub(crate) fn try_merge_with_subschemas( } if let Some(not) = not { - schema_object = try_merge_schema_not(schema_object, not.as_ref(), defs) - .map_err(|_| SubschemaMergeError::NotSchemaMerge)?; + schema_object = try_merge_schema_not(schema_object, not.as_ref(), defs)?; } // TODO: we should be able to handle a combined one_of and any_of... but @@ -428,6 +427,16 @@ fn try_merge_with_each_subschema( joined_schemas } +#[derive(Error, Debug)] +pub(crate) enum NotSchemaMergeError { + #[error("Subtracting everything leaves nothing")] + SubtractingEverything, + #[error("Property `{0}` can't be both required and not required")] + ConflictingPropertyRequire(String), + #[error("Error in try_merge_with_subschemas_not")] + NotSubschemaMerge, +} + /// "Subtract" the "not" schema from the schema object. /// /// TODO Exactly where and how we handle not constructions is... tricky! As we @@ -437,7 +446,7 @@ fn try_merge_schema_not( mut schema_object: SchemaObject, not_schema: &Schema, defs: &BTreeMap, -) -> Result { +) -> Result { debug!( "try_merge_schema_not {}\n not:{}", serde_json::to_string_pretty(&schema_object).unwrap(), @@ -445,7 +454,7 @@ fn try_merge_schema_not( ); match not_schema { // Subtracting everything leaves nothing... - Schema::Bool(true) => Err(()), + Schema::Bool(true) => Err(NotSchemaMergeError::SubtractingEverything), // ... whereas subtracting nothing leaves everything. Schema::Bool(false) => Ok(schema_object), @@ -502,7 +511,9 @@ fn try_merge_schema_not( // A property can't be both required and not required // therefore this schema is unsatisfiable. if required.contains(not_required) { - return Err(()); + return Err(NotSchemaMergeError::ConflictingPropertyRequire( + not_required.to_string(), + )); } // Set the property's schema to false i.e. that the // presence of any value would be invalid. We ignore @@ -514,7 +525,8 @@ fn try_merge_schema_not( } if let Some(not_subschemas) = subschemas { - schema_object = try_merge_with_subschemas_not(schema_object, not_subschemas, defs)?; + schema_object = try_merge_with_subschemas_not(schema_object, not_subschemas, defs) + .map_err(|()| NotSchemaMergeError::NotSubschemaMerge)?; } Ok(schema_object) @@ -605,7 +617,9 @@ fn try_merge_with_subschemas_not( then_schema: None, else_schema: None, } => match try_merge_all(all_of, defs) { - Ok(merged_not_schema) => try_merge_schema_not(schema_object, &merged_not_schema, defs), + Ok(merged_not_schema) => { + try_merge_schema_not(schema_object, &merged_not_schema, defs).map_err(|_| ()) + } Err(_) => Ok(schema_object), }, From afb8b8b22e4784fdf28287d5668c0b1d204202ad Mon Sep 17 00:00:00 2001 From: Tormod Gjeitnes Hellen Date: Fri, 3 May 2024 16:48:32 +0200 Subject: [PATCH 03/14] fixup --- typify-impl/src/merge.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/typify-impl/src/merge.rs b/typify-impl/src/merge.rs index cae46846..afcda300 100644 --- a/typify-impl/src/merge.rs +++ b/typify-impl/src/merge.rs @@ -277,7 +277,7 @@ pub(crate) enum SubschemaMergeError { EmptyMergedOneOfSubschema, #[error("try_merge_schema_not error")] NotSchemaMerge(#[from] NotSchemaMergeError), - #[error("try_merge_schema_not error")] + #[error("Error merging AllOf schema")] AllOfSchemaMerge, } From 59acafd0a9a30bab86c1fa8a5a8ba6530cc699b1 Mon Sep 17 00:00:00 2001 From: Tormod Gjeitnes Hellen Date: Fri, 3 May 2024 17:00:33 +0200 Subject: [PATCH 04/14] Add SchemaMergeError --- typify-impl/src/merge.rs | 49 +++++++++++++++++++++++++++++----------- 1 file changed, 36 insertions(+), 13 deletions(-) diff --git a/typify-impl/src/merge.rs b/typify-impl/src/merge.rs index afcda300..48df864d 100644 --- a/typify-impl/src/merge.rs +++ b/typify-impl/src/merge.rs @@ -30,9 +30,9 @@ fn try_merge_all(schemas: &[Schema], defs: &BTreeMap) -> Result< [] => panic!("we should not be trying to merge an empty array of schemas"), [only] => only.clone(), [first, second, rest @ ..] => { - let mut out = try_merge_schema(first, second, defs)?; + let mut out = try_merge_schema(first, second, defs).map_err(|_| ())?; for schema in rest { - out = try_merge_schema(&out, schema, defs)?; + out = try_merge_schema(&out, schema, defs).map_err(|_| ())?; } out } @@ -78,12 +78,26 @@ fn merge_schema(a: &Schema, b: &Schema, defs: &BTreeMap) -> Sche try_merge_schema(a, b, defs).unwrap_or(Schema::Bool(false)) } +#[derive(Error, Debug)] +pub(crate) enum SchemaMergeError { + #[error("Cannot merge any schema with a trivially false schema")] + MergeWithFalse, + #[error("Error in merge_schema_object")] + ObjectSchemaMerge, +} + /// Merge two schemas returning the resulting schema. If the two schemas are /// incompatible (i.e. if there is no data that can satisfy them both /// simultaneously) then this returns Err. -fn try_merge_schema(a: &Schema, b: &Schema, defs: &BTreeMap) -> Result { +fn try_merge_schema( + a: &Schema, + b: &Schema, + defs: &BTreeMap, +) -> Result { match (a, b) { - (Schema::Bool(false), _) | (_, Schema::Bool(false)) => Err(()), + (Schema::Bool(false), _) | (_, Schema::Bool(false)) => { + Err(SchemaMergeError::MergeWithFalse) + } (Schema::Bool(true), other) | (other, Schema::Bool(true)) => Ok(other.clone()), // If we have two references to the same schema, that's easy! @@ -139,7 +153,9 @@ fn try_merge_schema(a: &Schema, b: &Schema, defs: &BTreeMap) -> } } - (Schema::Object(aa), Schema::Object(bb)) => Ok(merge_schema_object(aa, bb, defs)?.into()), + (Schema::Object(aa), Schema::Object(bb)) => Ok(merge_schema_object(aa, bb, defs) + .map_err(|()| SchemaMergeError::ObjectSchemaMerge)? + .into()), } } @@ -584,7 +600,9 @@ fn try_merge_with_subschemas_not( else_schema: None, } => { debug!("not not"); - Ok(try_merge_schema(&schema_object.into(), not.as_ref(), defs)?.into_object()) + Ok(try_merge_schema(&schema_object.into(), not.as_ref(), defs) + .map_err(|_| ())? + .into_object()) } // TODO this is a kludge @@ -830,9 +848,9 @@ fn merge_so_array( (Some(SingleOrVec::Single(aa_single)), _), (Some(SingleOrVec::Single(bb_single)), _), ) => ( - Some(SingleOrVec::Single(Box::new(try_merge_schema( - aa_single, bb_single, defs, - )?))), + Some(SingleOrVec::Single(Box::new( + try_merge_schema(aa_single, bb_single, defs).map_err(|_| ())?, + ))), None, max_items, ), @@ -855,10 +873,15 @@ fn merge_so_array( )?; if allow_additional_items { - let additional_items = additional_items.as_deref().map_or_else( - || Ok(single.as_ref().clone()), - |additional_schema| try_merge_schema(additional_schema, single, defs), - )?; + let additional_items = additional_items + .as_deref() + .map_or_else( + || Ok(single.as_ref().clone()), + |additional_schema| { + try_merge_schema(additional_schema, single, defs) + }, + ) + .map_err(|_| ())?; ( Some(SingleOrVec::Vec(items)), Some(Box::new(additional_items)), From fda93d5e7939ab5e985dab8217077679a738a7ef Mon Sep 17 00:00:00 2001 From: Tormod Gjeitnes Hellen Date: Fri, 3 May 2024 17:21:35 +0200 Subject: [PATCH 05/14] Add NotSubschemaMergeError --- typify-impl/src/merge.rs | 41 ++++++++++++++++++++-------------------- 1 file changed, 20 insertions(+), 21 deletions(-) diff --git a/typify-impl/src/merge.rs b/typify-impl/src/merge.rs index 48df864d..0f550999 100644 --- a/typify-impl/src/merge.rs +++ b/typify-impl/src/merge.rs @@ -449,8 +449,8 @@ pub(crate) enum NotSchemaMergeError { SubtractingEverything, #[error("Property `{0}` can't be both required and not required")] ConflictingPropertyRequire(String), - #[error("Error in try_merge_with_subschemas_not")] - NotSubschemaMerge, + #[error("Error in try_merge_with_subschemas_not: {0}")] + NotSubschemaMerge(#[from] NotSubschemaMergeError), } /// "Subtract" the "not" schema from the schema object. @@ -541,8 +541,7 @@ fn try_merge_schema_not( } if let Some(not_subschemas) = subschemas { - schema_object = try_merge_with_subschemas_not(schema_object, not_subschemas, defs) - .map_err(|()| NotSchemaMergeError::NotSubschemaMerge)?; + schema_object = try_merge_with_subschemas_not(schema_object, not_subschemas, defs)?; } Ok(schema_object) @@ -550,11 +549,21 @@ fn try_merge_schema_not( } } +#[derive(Error, Debug)] +pub(crate) enum NotSubschemaMergeError { + #[error("Error in merge_schema_object")] + ObjectSchemaMerge, + #[error("Error in merging schemas: {0}")] + SchemaMerge(#[from] SchemaMergeError), + #[error("Error in merging schemas: {0}")] + NotSchemaMerge(#[from] Box), +} + fn try_merge_with_subschemas_not( schema_object: SchemaObject, not_subschemas: &SubschemaValidation, defs: &BTreeMap, -) -> Result { +) -> Result { debug!("try_merge_with_subschemas_not"); match not_subschemas { SubschemaValidation { @@ -588,6 +597,7 @@ fn try_merge_with_subschemas_not( ..Default::default() }; merge_schema_object(&schema_object, &new_other, defs) + .map_err(|()| NotSubschemaMergeError::ObjectSchemaMerge) } SubschemaValidation { @@ -600,9 +610,7 @@ fn try_merge_with_subschemas_not( else_schema: None, } => { debug!("not not"); - Ok(try_merge_schema(&schema_object.into(), not.as_ref(), defs) - .map_err(|_| ())? - .into_object()) + Ok(try_merge_schema(&schema_object.into(), not.as_ref(), defs)?.into_object()) } // TODO this is a kludge @@ -635,9 +643,8 @@ fn try_merge_with_subschemas_not( then_schema: None, else_schema: None, } => match try_merge_all(all_of, defs) { - Ok(merged_not_schema) => { - try_merge_schema_not(schema_object, &merged_not_schema, defs).map_err(|_| ()) - } + Ok(merged_not_schema) => try_merge_schema_not(schema_object, &merged_not_schema, defs) + .map_err(|e| Box::new(e).into()), Err(_) => Ok(schema_object), }, @@ -1582,11 +1589,7 @@ mod tests { let ab = try_merge_schema(&a, &b, &Default::default()); - assert!( - ab.is_err(), - "{}", - serde_json::to_string_pretty(&ab).unwrap(), - ); + assert!(ab.is_err(), "{:#?}", &ab,); let a = json!({ "type": "array", @@ -1613,11 +1616,7 @@ mod tests { let ab = try_merge_schema(&a, &b, &Default::default()); - assert!( - ab.is_err(), - "{}", - serde_json::to_string_pretty(&ab).unwrap(), - ); + assert!(ab.is_err(), "{:#?}", &ab,); } #[test] From 58ec269d69b2000a6918ecccf0ede43cc591c439 Mon Sep 17 00:00:00 2001 From: Tormod Gjeitnes Hellen Date: Fri, 7 Jun 2024 17:40:28 +0200 Subject: [PATCH 06/14] Improve try_merge_with_subschemas errors --- typify-impl/src/merge.rs | 46 +++++++++++++++++++++++++++++++--------- 1 file changed, 36 insertions(+), 10 deletions(-) diff --git a/typify-impl/src/merge.rs b/typify-impl/src/merge.rs index 0f550999..5356deda 100644 --- a/typify-impl/src/merge.rs +++ b/typify-impl/src/merge.rs @@ -287,14 +287,25 @@ fn merge_so_enum_values( #[derive(Error, Debug)] pub(crate) enum SubschemaMergeError { - #[error("Empty merged AnyOf subschema")] - EmptyMergedAnyOfSubschema, - #[error("Empty merged OneOf subschema")] - EmptyMergedOneOfSubschema, - #[error("try_merge_schema_not error")] + #[error("Empty merged AnyOf subschema. schema_object: {schema_object:?}, any_of: {any_of:?}, defs: {defs:?}")] + EmptyMergedAnyOfSubschema { + schema_object: SchemaObject, + any_of: Vec, + defs: BTreeMap, + }, + #[error("Empty merged OneOf subschema. schema_object: {schema_object:?}, one_of: {one_of:?}, defs: {defs:?}")] + EmptyMergedOneOfSubschema { + schema_object: SchemaObject, + one_of: Vec, + defs: BTreeMap, + }, + #[error("try_merge_schema_not error: {0}")] NotSchemaMerge(#[from] NotSchemaMergeError), - #[error("Error merging AllOf schema")] - AllOfSchemaMerge, + #[error("Error merging AllOf schema: {error}. Context: {context:?}")] + AllOfSchemaMerge { + error: SchemaMergeError, + context: Vec, + }, } /// Merge the schema with a subschema validation object. It's important that @@ -328,7 +339,10 @@ pub(crate) fn try_merge_with_subschemas( .try_fold(schema_object.into(), |schema, other| { try_merge_schema(&schema, other, defs) }) - .map_err(|_| SubschemaMergeError::AllOfSchemaMerge)?; + .map_err(|e| SubschemaMergeError::AllOfSchemaMerge { + error: e, + context: all_of.clone(), + })?; assert_ne!(merged_schema, Schema::Bool(false)); schema_object = merged_schema.into_object(); } @@ -346,7 +360,13 @@ pub(crate) fn try_merge_with_subschemas( let merged_subschemas = try_merge_with_each_subschema(&schema_object, any_of, defs); match merged_subschemas.len() { - 0 => return Err(SubschemaMergeError::EmptyMergedAnyOfSubschema), + 0 => { + return Err(SubschemaMergeError::EmptyMergedAnyOfSubschema { + schema_object, + any_of: any_of.clone(), + defs: defs.clone(), + }) + } 1 => schema_object = merged_subschemas.into_iter().next().unwrap().into_object(), _ => { schema_object = SchemaObject { @@ -365,7 +385,13 @@ pub(crate) fn try_merge_with_subschemas( let merged_subschemas = try_merge_with_each_subschema(&schema_object, one_of, defs); match merged_subschemas.len() { - 0 => return Err(SubschemaMergeError::EmptyMergedOneOfSubschema), + 0 => { + return Err(SubschemaMergeError::EmptyMergedOneOfSubschema { + schema_object, + one_of: one_of.clone(), + defs: defs.clone(), + }) + } 1 => schema_object = merged_subschemas.into_iter().next().unwrap().into_object(), _ => { schema_object = SchemaObject { From 926ddf172a6e774cfb8caf51d5353d492ba3aee7 Mon Sep 17 00:00:00 2001 From: Tormod Gjeitnes Hellen Date: Fri, 7 Jun 2024 18:11:26 +0200 Subject: [PATCH 07/14] fixup --- typify-impl/src/merge.rs | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/typify-impl/src/merge.rs b/typify-impl/src/merge.rs index 5356deda..9ad4e3d3 100644 --- a/typify-impl/src/merge.rs +++ b/typify-impl/src/merge.rs @@ -287,17 +287,15 @@ fn merge_so_enum_values( #[derive(Error, Debug)] pub(crate) enum SubschemaMergeError { - #[error("Empty merged AnyOf subschema. schema_object: {schema_object:?}, any_of: {any_of:?}, defs: {defs:?}")] + #[error("Empty merged AnyOf subschema. schema_object: {schema_object:?}, any_of: {any_of:?}")] EmptyMergedAnyOfSubschema { schema_object: SchemaObject, any_of: Vec, - defs: BTreeMap, }, - #[error("Empty merged OneOf subschema. schema_object: {schema_object:?}, one_of: {one_of:?}, defs: {defs:?}")] + #[error("Empty merged OneOf subschema. schema_object: {schema_object:?}, one_of: {one_of:?}")] EmptyMergedOneOfSubschema { schema_object: SchemaObject, one_of: Vec, - defs: BTreeMap, }, #[error("try_merge_schema_not error: {0}")] NotSchemaMerge(#[from] NotSchemaMergeError), @@ -364,7 +362,6 @@ pub(crate) fn try_merge_with_subschemas( return Err(SubschemaMergeError::EmptyMergedAnyOfSubschema { schema_object, any_of: any_of.clone(), - defs: defs.clone(), }) } 1 => schema_object = merged_subschemas.into_iter().next().unwrap().into_object(), @@ -389,7 +386,6 @@ pub(crate) fn try_merge_with_subschemas( return Err(SubschemaMergeError::EmptyMergedOneOfSubschema { schema_object, one_of: one_of.clone(), - defs: defs.clone(), }) } 1 => schema_object = merged_subschemas.into_iter().next().unwrap().into_object(), From 5899cac4ed6835c4af8bc0c465e1931029529479 Mon Sep 17 00:00:00 2001 From: Tormod Gjeitnes Hellen Date: Mon, 10 Jun 2024 13:53:00 +0200 Subject: [PATCH 08/14] More context --- typify-impl/src/merge.rs | 23 +++++++++++++++-------- 1 file changed, 15 insertions(+), 8 deletions(-) diff --git a/typify-impl/src/merge.rs b/typify-impl/src/merge.rs index 9ad4e3d3..76c89d3b 100644 --- a/typify-impl/src/merge.rs +++ b/typify-impl/src/merge.rs @@ -80,10 +80,10 @@ fn merge_schema(a: &Schema, b: &Schema, defs: &BTreeMap) -> Sche #[derive(Error, Debug)] pub(crate) enum SchemaMergeError { - #[error("Cannot merge any schema with a trivially false schema")] + #[error("Cannot merge two trivially false schemas")] MergeWithFalse, - #[error("Error in merge_schema_object")] - ObjectSchemaMerge, + #[error("Error when trying to merge the two schema objects {a:?} and {b:?}")] + ObjectSchemaMerge { a: SchemaObject, b: SchemaObject }, } /// Merge two schemas returning the resulting schema. If the two schemas are @@ -154,7 +154,10 @@ fn try_merge_schema( } (Schema::Object(aa), Schema::Object(bb)) => Ok(merge_schema_object(aa, bb, defs) - .map_err(|()| SchemaMergeError::ObjectSchemaMerge)? + .map_err(|()| SchemaMergeError::ObjectSchemaMerge { + a: aa.clone(), + b: bb.clone(), + })? .into()), } } @@ -573,8 +576,8 @@ fn try_merge_schema_not( #[derive(Error, Debug)] pub(crate) enum NotSubschemaMergeError { - #[error("Error in merge_schema_object")] - ObjectSchemaMerge, + #[error("Error when trying to merge the two schema objects {a:?} and {b:?}")] + ObjectSchemaMerge { a: SchemaObject, b: SchemaObject }, #[error("Error in merging schemas: {0}")] SchemaMerge(#[from] SchemaMergeError), #[error("Error in merging schemas: {0}")] @@ -618,8 +621,12 @@ fn try_merge_with_subschemas_not( })), ..Default::default() }; - merge_schema_object(&schema_object, &new_other, defs) - .map_err(|()| NotSubschemaMergeError::ObjectSchemaMerge) + merge_schema_object(&schema_object, &new_other, defs).map_err(|()| { + NotSubschemaMergeError::ObjectSchemaMerge { + a: schema_object, + b: new_other, + } + }) } SubschemaValidation { From 93055d908135ebff6a47b9082a02de64c90785bd Mon Sep 17 00:00:00 2001 From: Tormod Gjeitnes Hellen Date: Mon, 10 Jun 2024 17:22:22 +0200 Subject: [PATCH 09/14] Add ObjectSchemaMergeError --- typify-impl/src/merge.rs | 136 +++++++++++++++++++++++++++++++++------ 1 file changed, 118 insertions(+), 18 deletions(-) diff --git a/typify-impl/src/merge.rs b/typify-impl/src/merge.rs index 76c89d3b..452e1c33 100644 --- a/typify-impl/src/merge.rs +++ b/typify-impl/src/merge.rs @@ -82,8 +82,12 @@ fn merge_schema(a: &Schema, b: &Schema, defs: &BTreeMap) -> Sche pub(crate) enum SchemaMergeError { #[error("Cannot merge two trivially false schemas")] MergeWithFalse, - #[error("Error when trying to merge the two schema objects {a:?} and {b:?}")] - ObjectSchemaMerge { a: SchemaObject, b: SchemaObject }, + #[error("Error when trying to merge the two schema objects {a:?} and {b:?}: {source}")] + ObjectSchemaMerge { + source: Box, + a: SchemaObject, + b: SchemaObject, + }, } /// Merge two schemas returning the resulting schema. If the two schemas are @@ -154,7 +158,8 @@ fn try_merge_schema( } (Schema::Object(aa), Schema::Object(bb)) => Ok(merge_schema_object(aa, bb, defs) - .map_err(|()| SchemaMergeError::ObjectSchemaMerge { + .map_err(|source| SchemaMergeError::ObjectSchemaMerge { + source: Box::new(source), a: aa.clone(), b: bb.clone(), })? @@ -162,11 +167,58 @@ fn try_merge_schema( } } +#[derive(Error, Debug)] +pub(crate) enum ObjectSchemaMergeError { + #[error("Error when trying to merge the two instance types {a:?} and {b:?}")] + InstanceType { + a: Option>, + b: Option>, + }, + #[error("Error when trying to merge the two formats {a:?} and {b:?}")] + Format { + a: Option, + b: Option, + }, + #[error("Error when trying to merge the two numbers {a:?} and {b:?}")] + Number { + a: Option>, + b: Option>, + }, + #[error("Error when trying to merge the two strings {a:?} and {b:?}")] + String { + a: Option>, + b: Option>, + }, + #[error("Error when trying to merge the two arrays {a:?} and {b:?}")] + Array { + a: Option>, + b: Option>, + }, + #[error("Error when trying to merge the two objects {a:?} and {b:?}")] + Object { + a: Option>, + b: Option>, + }, + #[error("Error when trying to merge the two enums {a_enum:?} (with const value {a_const:?}) and {b_enum:?} (with const value {b_const:?})")] + EnumValues { + a_enum: Option>, + a_const: Option, + b_enum: Option>, + b_const: Option, + }, + #[error("Error when trying to merge the schema {schema:?} with the maybe_subschema {maybe_subschema:?}: {source}")] + SubschemaMerge { + source: SubschemaMergeError, + schema: SchemaObject, + maybe_subschema: Option>, + }, +} + fn merge_schema_object( a: &SchemaObject, b: &SchemaObject, defs: &BTreeMap, -) -> Result { +) -> Result { debug!( "merging {}\n{}", serde_json::to_string_pretty(a).unwrap(), @@ -176,20 +228,55 @@ fn merge_schema_object( assert!(a.reference.is_none()); assert!(b.reference.is_none()); - let instance_type = merge_so_instance_type(a.instance_type.as_ref(), b.instance_type.as_ref())?; - let format = merge_so_format(a.format.as_ref(), b.format.as_ref())?; + let instance_type = merge_so_instance_type(a.instance_type.as_ref(), b.instance_type.as_ref()) + .map_err(|()| ObjectSchemaMergeError::InstanceType { + a: a.instance_type.clone(), + b: b.instance_type.clone(), + })?; + let format = merge_so_format(a.format.as_ref(), b.format.as_ref()).map_err(|()| { + ObjectSchemaMergeError::Format { + a: a.format.clone(), + b: b.format.clone(), + } + })?; - let number = merge_so_number(a.number.as_deref(), b.number.as_deref())?; - let string = merge_so_string(a.string.as_deref(), b.string.as_deref())?; - let array = merge_so_array(a.array.as_deref(), b.array.as_deref(), defs)?; - let object = merge_so_object(a.object.as_deref(), b.object.as_deref(), defs)?; + let number = merge_so_number(a.number.as_deref(), b.number.as_deref()).map_err(|()| { + ObjectSchemaMergeError::Number { + a: a.number.clone(), + b: b.number.clone(), + } + })?; + let string = merge_so_string(a.string.as_deref(), b.string.as_deref()).map_err(|()| { + ObjectSchemaMergeError::String { + a: a.string.clone(), + b: b.string.clone(), + } + })?; + let array = merge_so_array(a.array.as_deref(), b.array.as_deref(), defs).map_err(|()| { + ObjectSchemaMergeError::Array { + a: a.array.clone(), + b: b.array.clone(), + } + })?; + let object = merge_so_object(a.object.as_deref(), b.object.as_deref(), defs).map_err(|()| { + ObjectSchemaMergeError::Object { + a: a.object.clone(), + b: b.object.clone(), + } + })?; let enum_values = merge_so_enum_values( a.enum_values.as_ref(), a.const_value.as_ref(), b.enum_values.as_ref(), b.const_value.as_ref(), - )?; + ) + .map_err(|()| ObjectSchemaMergeError::EnumValues { + a_enum: a.enum_values.clone(), + a_const: b.const_value.clone(), + b_enum: b.enum_values.clone(), + b_const: b.const_value.clone(), + })?; // We could clean up this schema to eliminate data irrelevant to the // instance type, but logic in the conversion path should already handle @@ -216,10 +303,18 @@ fn merge_schema_object( // two schemas and then do the appropriate merge with subschemas (i.e. // potentially twice). This is effectively an `allOf` between the merged // "body" schema and the component subschemas. - merged_schema = - try_merge_with_subschemas(merged_schema, a.subschemas.as_deref(), defs).map_err(|_| ())?; - merged_schema = - try_merge_with_subschemas(merged_schema, b.subschemas.as_deref(), defs).map_err(|_| ())?; + merged_schema = try_merge_with_subschemas(merged_schema.clone(), a.subschemas.as_deref(), defs) + .map_err(|source| ObjectSchemaMergeError::SubschemaMerge { + source, + schema: merged_schema, + maybe_subschema: a.subschemas.clone(), + })?; + merged_schema = try_merge_with_subschemas(merged_schema.clone(), b.subschemas.as_deref(), defs) + .map_err(|source| ObjectSchemaMergeError::SubschemaMerge { + source, + schema: merged_schema, + maybe_subschema: a.subschemas.clone(), + })?; assert_ne!(merged_schema, Schema::Bool(false).into_object()); @@ -576,8 +671,12 @@ fn try_merge_schema_not( #[derive(Error, Debug)] pub(crate) enum NotSubschemaMergeError { - #[error("Error when trying to merge the two schema objects {a:?} and {b:?}")] - ObjectSchemaMerge { a: SchemaObject, b: SchemaObject }, + #[error("Error when trying to merge the two schema objects {a:?} and {b:?}: {source}")] + ObjectSchemaMerge { + source: Box, + a: SchemaObject, + b: SchemaObject, + }, #[error("Error in merging schemas: {0}")] SchemaMerge(#[from] SchemaMergeError), #[error("Error in merging schemas: {0}")] @@ -621,8 +720,9 @@ fn try_merge_with_subschemas_not( })), ..Default::default() }; - merge_schema_object(&schema_object, &new_other, defs).map_err(|()| { + merge_schema_object(&schema_object, &new_other, defs).map_err(|source| { NotSubschemaMergeError::ObjectSchemaMerge { + source: Box::new(source), a: schema_object, b: new_other, } From dbd982daf2f369e9df829aab153ed30e1c08a0da Mon Sep 17 00:00:00 2001 From: Tormod Gjeitnes Hellen Date: Mon, 10 Jun 2024 17:35:12 +0200 Subject: [PATCH 10/14] Follow convention --- typify-impl/src/merge.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/typify-impl/src/merge.rs b/typify-impl/src/merge.rs index 452e1c33..8b1cf1ec 100644 --- a/typify-impl/src/merge.rs +++ b/typify-impl/src/merge.rs @@ -397,9 +397,9 @@ pub(crate) enum SubschemaMergeError { }, #[error("try_merge_schema_not error: {0}")] NotSchemaMerge(#[from] NotSchemaMergeError), - #[error("Error merging AllOf schema: {error}. Context: {context:?}")] + #[error("Error merging AllOf schema: {source}. Context: {context:?}")] AllOfSchemaMerge { - error: SchemaMergeError, + source: SchemaMergeError, context: Vec, }, } @@ -435,8 +435,8 @@ pub(crate) fn try_merge_with_subschemas( .try_fold(schema_object.into(), |schema, other| { try_merge_schema(&schema, other, defs) }) - .map_err(|e| SubschemaMergeError::AllOfSchemaMerge { - error: e, + .map_err(|source| SubschemaMergeError::AllOfSchemaMerge { + source, context: all_of.clone(), })?; assert_ne!(merged_schema, Schema::Bool(false)); From 4bbe19a2fdfe0e0cf0b24fe71ce626c132b846c1 Mon Sep 17 00:00:00 2001 From: Tormod Gjeitnes Hellen Date: Mon, 10 Jun 2024 17:54:05 +0200 Subject: [PATCH 11/14] More context --- typify-impl/src/merge.rs | 21 +++++++++++++++------ 1 file changed, 15 insertions(+), 6 deletions(-) diff --git a/typify-impl/src/merge.rs b/typify-impl/src/merge.rs index 8b1cf1ec..6ede839e 100644 --- a/typify-impl/src/merge.rs +++ b/typify-impl/src/merge.rs @@ -395,12 +395,15 @@ pub(crate) enum SubschemaMergeError { schema_object: SchemaObject, one_of: Vec, }, - #[error("try_merge_schema_not error: {0}")] - NotSchemaMerge(#[from] NotSchemaMergeError), - #[error("Error merging AllOf schema: {source}. Context: {context:?}")] + #[error("Error trying to merge in the not schema {not:?}: {source}")] + NotSchemaMerge { + source: NotSchemaMergeError, + not: Box, + }, + #[error("Error trying to merge in the all_of schema {all_of:?}: {source}")] AllOfSchemaMerge { source: SchemaMergeError, - context: Vec, + all_of: Vec, }, } @@ -437,14 +440,20 @@ pub(crate) fn try_merge_with_subschemas( }) .map_err(|source| SubschemaMergeError::AllOfSchemaMerge { source, - context: all_of.clone(), + all_of: all_of.clone(), })?; assert_ne!(merged_schema, Schema::Bool(false)); schema_object = merged_schema.into_object(); } if let Some(not) = not { - schema_object = try_merge_schema_not(schema_object, not.as_ref(), defs)?; + schema_object = + try_merge_schema_not(schema_object, not.as_ref(), defs).map_err(|source| { + SubschemaMergeError::NotSchemaMerge { + source, + not: not.clone(), + } + })?; } // TODO: we should be able to handle a combined one_of and any_of... but From 40bded1a0e51b68a48d0df43a98f2347a4be77c6 Mon Sep 17 00:00:00 2001 From: Tormod Gjeitnes Hellen Date: Mon, 10 Jun 2024 18:16:29 +0200 Subject: [PATCH 12/14] Nice to have --- typify-impl/src/merge.rs | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/typify-impl/src/merge.rs b/typify-impl/src/merge.rs index 6ede839e..fd15c7b4 100644 --- a/typify-impl/src/merge.rs +++ b/typify-impl/src/merge.rs @@ -20,7 +20,10 @@ pub(crate) fn merge_all(schemas: &[Schema], defs: &BTreeMap) -> try_merge_all(schemas, defs).unwrap_or(Schema::Bool(false)) } -fn try_merge_all(schemas: &[Schema], defs: &BTreeMap) -> Result { +fn try_merge_all( + schemas: &[Schema], + defs: &BTreeMap, +) -> Result { debug!( "merge all {}", serde_json::to_string_pretty(schemas).unwrap(), @@ -30,9 +33,9 @@ fn try_merge_all(schemas: &[Schema], defs: &BTreeMap) -> Result< [] => panic!("we should not be trying to merge an empty array of schemas"), [only] => only.clone(), [first, second, rest @ ..] => { - let mut out = try_merge_schema(first, second, defs).map_err(|_| ())?; + let mut out = try_merge_schema(first, second, defs)?; for schema in rest { - out = try_merge_schema(&out, schema, defs).map_err(|_| ())?; + out = try_merge_schema(&out, schema, defs)?; } out } From fa7ce3ce46a16d9a304f5faec7965a56e3c1540a Mon Sep 17 00:00:00 2001 From: Tormod Gjeitnes Hellen Date: Mon, 10 Jun 2024 18:23:14 +0200 Subject: [PATCH 13/14] Get rid of clones --- typify-impl/src/merge.rs | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/typify-impl/src/merge.rs b/typify-impl/src/merge.rs index fd15c7b4..1c9922ff 100644 --- a/typify-impl/src/merge.rs +++ b/typify-impl/src/merge.rs @@ -209,10 +209,9 @@ pub(crate) enum ObjectSchemaMergeError { b_enum: Option>, b_const: Option, }, - #[error("Error when trying to merge the schema {schema:?} with the maybe_subschema {maybe_subschema:?}: {source}")] + #[error("Error when trying to merge in the subschema {maybe_subschema:?}: {source}")] SubschemaMerge { source: SubschemaMergeError, - schema: SchemaObject, maybe_subschema: Option>, }, } @@ -306,16 +305,14 @@ fn merge_schema_object( // two schemas and then do the appropriate merge with subschemas (i.e. // potentially twice). This is effectively an `allOf` between the merged // "body" schema and the component subschemas. - merged_schema = try_merge_with_subschemas(merged_schema.clone(), a.subschemas.as_deref(), defs) + merged_schema = try_merge_with_subschemas(merged_schema, a.subschemas.as_deref(), defs) .map_err(|source| ObjectSchemaMergeError::SubschemaMerge { source, - schema: merged_schema, maybe_subschema: a.subschemas.clone(), })?; - merged_schema = try_merge_with_subschemas(merged_schema.clone(), b.subschemas.as_deref(), defs) + merged_schema = try_merge_with_subschemas(merged_schema, b.subschemas.as_deref(), defs) .map_err(|source| ObjectSchemaMergeError::SubschemaMerge { source, - schema: merged_schema, maybe_subschema: a.subschemas.clone(), })?; From 4fb85d0654de266eb596d9b51549451379a4605f Mon Sep 17 00:00:00 2001 From: Tormod Gjeitnes Hellen Date: Tue, 18 Jun 2024 12:06:25 +0200 Subject: [PATCH 14/14] Address review on error displays --- typify-impl/src/convert.rs | 4 ++-- typify-impl/src/merge.rs | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/typify-impl/src/convert.rs b/typify-impl/src/convert.rs index 04199837..0bdb7111 100644 --- a/typify-impl/src/convert.rs +++ b/typify-impl/src/convert.rs @@ -8,7 +8,7 @@ use crate::type_entry::{ Variant, VariantDetails, }; use crate::util::{all_mutually_exclusive, recase, ref_key, Case, StringValidator}; -use log::{debug, error, info}; +use log::{debug, info}; use schemars::schema::{ ArrayValidation, InstanceType, Metadata, ObjectValidation, Schema, SchemaObject, SingleOrVec, StringValidation, SubschemaValidation, @@ -556,7 +556,7 @@ impl TypeSpace { } Err(e) => { - error!("Error in convert_schema_object: {:#?}", e); + debug!("failed to merge schemas in convert_schema_object: {:#?}", e); self.convert_never(type_name, original_schema) } } diff --git a/typify-impl/src/merge.rs b/typify-impl/src/merge.rs index 1c9922ff..acc96c69 100644 --- a/typify-impl/src/merge.rs +++ b/typify-impl/src/merge.rs @@ -83,7 +83,7 @@ fn merge_schema(a: &Schema, b: &Schema, defs: &BTreeMap) -> Sche #[derive(Error, Debug)] pub(crate) enum SchemaMergeError { - #[error("Cannot merge two trivially false schemas")] + #[error("merging with false")] MergeWithFalse, #[error("Error when trying to merge the two schema objects {a:?} and {b:?}: {source}")] ObjectSchemaMerge { @@ -172,7 +172,7 @@ fn try_merge_schema( #[derive(Error, Debug)] pub(crate) enum ObjectSchemaMergeError { - #[error("Error when trying to merge the two instance types {a:?} and {b:?}")] + #[error("Incompatible instance types {a:?} and {b:?}")] InstanceType { a: Option>, b: Option>,