From b5648804d6cdbd1ed01baa6fecea4c5e92629c7e Mon Sep 17 00:00:00 2001 From: John Kastner <130772734+john-h-kastner-aws@users.noreply.github.com> Date: Thu, 10 Oct 2024 14:48:46 -0400 Subject: [PATCH] Fix JSON schema parsing error mesage on error in nested types (#1270) Signed-off-by: John Kastner --- cedar-policy-validator/src/json_schema.rs | 188 ++++++++++++++-------- cedar-policy/CHANGELOG.md | 4 + 2 files changed, 127 insertions(+), 65 deletions(-) diff --git a/cedar-policy-validator/src/json_schema.rs b/cedar-policy-validator/src/json_schema.rs index 9224c8d8f..dc6962676 100644 --- a/cedar-policy-validator/src/json_schema.rs +++ b/cedar-policy-validator/src/json_schema.rs @@ -1080,16 +1080,11 @@ impl<'de, N: Deserialize<'de> + From> Visitor<'de> for TypeVisitor { { use TypeFields::{AdditionalAttributes, Attributes, Element, Name, Type as TypeField}; - // We keep field values wrapped in a `Result` initially so that we do - // not report errors due the contents of a field when the field is not - // expected for a particular type variant. We instead report that the - // field so not exist at all, so that the schema author can delete the - // field without wasting time fixing errors in the value. - let mut type_name: Option> = None; - let mut element: Option, M::Error>> = None; - let mut attributes: Option> = None; - let mut additional_attributes: Option> = None; - let mut name: Option> = None; + let mut type_name: Option = None; + let mut element: Option> = None; + let mut attributes: Option = None; + let mut additional_attributes: Option = None; + let mut name: Option = None; // Gather all the fields in the object. Any fields that are not one of // the possible fields for some schema type will have been reported by @@ -1100,19 +1095,19 @@ impl<'de, N: Deserialize<'de> + From> Visitor<'de> for TypeVisitor { if type_name.is_some() { return Err(serde::de::Error::duplicate_field(TypeField.as_str())); } - type_name = Some(map.next_value()); + type_name = Some(map.next_value()?); } Element => { if element.is_some() { return Err(serde::de::Error::duplicate_field(Element.as_str())); } - element = Some(map.next_value()); + element = Some(map.next_value()?); } Attributes => { if attributes.is_some() { return Err(serde::de::Error::duplicate_field(Attributes.as_str())); } - attributes = Some(map.next_value()); + attributes = Some(map.next_value()?); } AdditionalAttributes => { if additional_attributes.is_some() { @@ -1120,13 +1115,13 @@ impl<'de, N: Deserialize<'de> + From> Visitor<'de> for TypeVisitor { AdditionalAttributes.as_str(), )); } - additional_attributes = Some(map.next_value()); + additional_attributes = Some(map.next_value()?); } Name => { if name.is_some() { return Err(serde::de::Error::duplicate_field(Name.as_str())); } - name = Some(map.next_value()); + name = Some(map.next_value()?); } } } @@ -1141,11 +1136,11 @@ impl<'de, N: Deserialize<'de> + From> TypeVisitor { /// which is not used for a particular type to be `Some` when building that /// type. fn build_schema_type( - type_name: Option>, - element: Option, M::Error>>, - attributes: Option>, - additional_attributes: Option>, - name: Option>, + type_name: Option, + element: Option>, + attributes: Option, + additional_attributes: Option, + name: Option, ) -> std::result::Result, M::Error> where M: MapAccess<'de>, @@ -1164,7 +1159,7 @@ impl<'de, N: Deserialize<'de> + From> TypeVisitor { .map(|(field, _)| field) .collect::>(); - match type_name.transpose()?.as_ref() { + match type_name.as_ref() { Some(s) => { // We've concluded that type exists remaining_fields.remove(&TypeField); @@ -1204,7 +1199,7 @@ impl<'de, N: Deserialize<'de> + From> TypeVisitor { match element { Some(element) => Ok(Type::Type(TypeVariant::Set { - element: Box::new(element?), + element: Box::new(element), })), None => Err(serde::de::Error::missing_field(Element.as_str())), } @@ -1220,9 +1215,9 @@ impl<'de, N: Deserialize<'de> + From> TypeVisitor { if let Some(attributes) = attributes { let additional_attributes = - additional_attributes.unwrap_or(Ok(partial_schema_default())); + additional_attributes.unwrap_or(partial_schema_default()); Ok(Type::Type(TypeVariant::Record(RecordType { - attributes: attributes? + attributes: attributes .0 .into_iter() .map(|(k, TypeOfAttribute { ty, required })| { @@ -1235,7 +1230,7 @@ impl<'de, N: Deserialize<'de> + From> TypeVisitor { ) }) .collect(), - additional_attributes: additional_attributes?, + additional_attributes: additional_attributes, }))) } else { Err(serde::de::Error::missing_field(Attributes.as_str())) @@ -1247,18 +1242,15 @@ impl<'de, N: Deserialize<'de> + From> TypeVisitor { &[type_field_name!(Name)], )?; match name { - Some(name) => { - let name = name?; - Ok(Type::Type(TypeVariant::Entity { - name: RawName::from_normalized_str(&name) - .map_err(|err| { - serde::de::Error::custom(format!( - "invalid entity type `{name}`: {err}" - )) - })? - .into(), - })) - } + Some(name) => Ok(Type::Type(TypeVariant::Entity { + name: RawName::from_normalized_str(&name) + .map_err(|err| { + serde::de::Error::custom(format!( + "invalid entity type `{name}`: {err}" + )) + })? + .into(), + })), None => Err(serde::de::Error::missing_field(Name.as_str())), } } @@ -1268,18 +1260,15 @@ impl<'de, N: Deserialize<'de> + From> TypeVisitor { &[type_field_name!(Name)], )?; match name { - Some(name) => { - let name = name?; - Ok(Type::Type(TypeVariant::EntityOrCommon { - type_name: RawName::from_normalized_str(&name) - .map_err(|err| { - serde::de::Error::custom(format!( - "invalid entity or common type `{name}`: {err}" - )) - })? - .into(), - })) - } + Some(name) => Ok(Type::Type(TypeVariant::EntityOrCommon { + type_name: RawName::from_normalized_str(&name) + .map_err(|err| { + serde::de::Error::custom(format!( + "invalid entity or common type `{name}`: {err}" + )) + })? + .into(), + })), None => Err(serde::de::Error::missing_field(Name.as_str())), } } @@ -1290,18 +1279,13 @@ impl<'de, N: Deserialize<'de> + From> TypeVisitor { )?; match name { - Some(name) => { - let name = name?; - Ok(Type::Type(TypeVariant::Extension { - name: UnreservedId::from_normalized_str(&name).map_err( - |err| { - serde::de::Error::custom(format!( - "invalid extension type `{name}`: {err}" - )) - }, - )?, - })) - } + Some(name) => Ok(Type::Type(TypeVariant::Extension { + name: UnreservedId::from_normalized_str(&name).map_err(|err| { + serde::de::Error::custom(format!( + "invalid extension type `{name}`: {err}" + )) + })?, + })), None => Err(serde::de::Error::missing_field(Name.as_str())), } } @@ -2041,10 +2025,9 @@ mod test { } #[test] - #[should_panic(expected = "unknown field `attributes`")] fn schema_file_unexpected_malformed_attribute() { let src = serde_json::json!( - { + { "": { "entityTypes": { "User": { "shape": { @@ -2061,9 +2044,84 @@ mod test { } }, "actions": {} + }}); + let schema = ValidatorSchema::from_json_value(src, Extensions::all_available()); + assert_matches!(schema, Err(e) => { + expect_err( + "", + &miette::Report::new(e), + &ExpectedErrorMessageBuilder::error(r#"unknown field `foo`, expected one of `type`, `element`, `attributes`, `additionalAttributes`, `name`"#).build() + ); + }); + } + + #[test] + fn error_in_nested_attribute_fails_fast_top_level_attr() { + let src = serde_json::json!( + { + "": { + "entityTypes": { + "User": { + "shape": { + "type": "Record", + "attributes": { + "foo": { + "type": "Record", + // Parsing should fail here when `element` is not expected instead of failing later on `"bar"` + "element": { "type": "Long" } + }, + "bar": { "type": "Long" } + } + } + } + }, + "actions": {} + } + } + ); + + let schema = ValidatorSchema::from_json_value(src, Extensions::all_available()); + assert_matches!(schema, Err(e) => { + expect_err( + "", + &miette::Report::new(e), + &ExpectedErrorMessageBuilder::error(r#"unknown field `element`, expected `attributes` or `additionalAttributes`"#).build() + ); + }); + } + + #[test] + fn error_in_nested_attribute_fails_fast_nested_attr() { + let src = serde_json::json!( + { "": { + "entityTypes": { + "a": { + "shape": { + "type": "Record", + "attributes": { + "foo": { "type": "Entity", "name": "b" }, + "baz": { "type": "Record", + "attributes": { + // Parsing should fail here instead of continuing and failing on the `"b"` as in #417 + "z": "Boolean" + } + } + } + } + }, + "b": {} + } + } } + ); + + let schema = ValidatorSchema::from_json_value(src, Extensions::all_available()); + assert_matches!(schema, Err(e) => { + expect_err( + "", + &miette::Report::new(e), + &ExpectedErrorMessageBuilder::error(r#"invalid type: string "Boolean", expected struct TypeOfAttribute"#).build() + ); }); - let schema: NamespaceDefinition = serde_json::from_value(src).unwrap(); - println!("{:#?}", schema); } #[test] diff --git a/cedar-policy/CHANGELOG.md b/cedar-policy/CHANGELOG.md index 0d67ea855..4553f546f 100644 --- a/cedar-policy/CHANGELOG.md +++ b/cedar-policy/CHANGELOG.md @@ -12,6 +12,10 @@ Starting with version 3.2.4, changes marked with a star (*) are _language breaki ## [Unreleased] Cedar Language Version: TBD +### Fixed + +- Some misleading parser errors for JSON schema with mistakes in nested attribute definitions (#1270, resolving #417) + ## [4.2.0] - 2024-10-07 Cedar Language version: 4.1