xso: reject duplicate children

Jonas Schäfer created 2 years ago

This was an oversight. Even though we apparently don't have tests for
this anywhere, it is what the old functional macros do.

Change summary

parsers/src/forwarding.rs       | 32 ++++++++++++++++++++++++++++++++
parsers/src/private.rs          | 23 +++++++++++++++++++++++
parsers/src/util/macro_tests.rs | 13 +++++++++++++
xso-proc/src/error_message.rs   | 12 ++++++++++++
xso-proc/src/field.rs           | 13 ++++++++++++-
5 files changed, 92 insertions(+), 1 deletion(-)

Detailed changes

parsers/src/forwarding.rs

@@ -96,4 +96,36 @@ mod tests {
         let serialized: Element = forwarded.into();
         assert_eq!(serialized, reference);
     }
+
+    #[test]
+    fn test_invalid_duplicate_delay() {
+        let elem: Element = "<forwarded xmlns='urn:xmpp:forward:0'><delay xmlns='urn:xmpp:delay' from='capulet.com' stamp='2002-09-10T23:08:25+00:00'/><delay xmlns='urn:xmpp:delay' from='capulet.com' stamp='2002-09-10T23:08:25+00:00'/><message xmlns='jabber:client' to='[e-mail]/balcony' from='[e-mail]/home'/></forwarded>"
+            .parse()
+            .unwrap();
+        let error = Forwarded::try_from(elem).unwrap_err();
+        let message = match error {
+            FromElementError::Invalid(Error::Other(string)) => string,
+            _ => panic!(),
+        };
+        assert_eq!(
+            message,
+            "Element forwarded must not have more than one delay child."
+        );
+    }
+
+    #[test]
+    fn test_invalid_duplicate_message() {
+        let elem: Element = "<forwarded xmlns='urn:xmpp:forward:0'><delay xmlns='urn:xmpp:delay' from='capulet.com' stamp='2002-09-10T23:08:25+00:00'/><message xmlns='jabber:client' to='[e-mail]/balcony' from='[e-mail]/home'/><message xmlns='jabber:client' to='[e-mail]/balcony' from='[e-mail]/home'/></forwarded>"
+            .parse()
+            .unwrap();
+        let error = Forwarded::try_from(elem).unwrap_err();
+        let message = match error {
+            FromElementError::Invalid(Error::Other(string)) => string,
+            _ => panic!(),
+        };
+        assert_eq!(
+            message,
+            "Element forwarded must not have more than one message child."
+        );
+    }
 }

parsers/src/private.rs

@@ -39,3 +39,26 @@ pub struct Query {
 impl IqSetPayload for Query {}
 impl IqGetPayload for Query {}
 impl IqResultPayload for Query {}
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+    use minidom::Element;
+    use xso::error::{Error, FromElementError};
+
+    #[test]
+    fn test_invalid_duplicate_child() {
+        let elem: Element = "<query xmlns='jabber:iq:private'><storage xmlns='storage:bookmarks'/><storage xmlns='storage:bookmarks'/></query>"
+            .parse()
+            .unwrap();
+        let error = Query::try_from(elem).unwrap_err();
+        let message = match error {
+            FromElementError::Invalid(Error::Other(string)) => string,
+            _ => panic!(),
+        };
+        assert_eq!(
+            message,
+            "Query element must not have more than one child in field 'storage'."
+        );
+    }
+}

parsers/src/util/macro_tests.rs

@@ -522,6 +522,19 @@ fn parent_positive() {
     assert_eq!(v.child.foo, "hello world!");
 }
 
+#[test]
+fn parent_negative_duplicate_child() {
+    #[allow(unused_imports)]
+    use std::{
+        option::Option::{None, Some},
+        result::Result::{Err, Ok},
+    };
+    match parse_str::<Parent>("<parent xmlns='urn:example:ns1'><attr foo='hello world!'/><attr foo='hello world!'/></parent>") {
+        Err(::xso::error::FromElementError::Invalid(::xso::error::Error::Other(e))) if e.contains("must not have more than one") => (),
+        other => panic!("unexpected result: {:?}", other),
+    }
+}
+
 #[derive(FromXml, AsXml, PartialEq, Debug, Clone)]
 #[xml(namespace = NS1, name = "parent")]
 struct OptionalChild {

xso-proc/src/error_message.rs

@@ -87,3 +87,15 @@ pub(super) fn on_missing_attribute(parent_name: &ParentRef, field: &Member) -> S
 pub(super) fn on_missing_child(parent_name: &ParentRef, field: &Member) -> String {
     format!("Missing child {} in {}.", FieldName(&field), parent_name)
 }
+
+/// Create a string error message for a duplicate child element.
+///
+/// `parent_name` should point at the compound which is being parsed and
+/// `field` should be the field to which the child belongs.
+pub(super) fn on_duplicate_child(parent_name: &ParentRef, field: &Member) -> String {
+    format!(
+        "{} must not have more than one child in {}.",
+        parent_name,
+        FieldName(&field)
+    )
+}

xso-proc/src/field.rs

@@ -393,6 +393,8 @@ impl FieldDef {
                     AmountConstraint::FixedSingle(_) => {
                         let missing_msg =
                             error_message::on_missing_child(container_name, &self.member);
+                        let duplicate_msg =
+                            error_message::on_duplicate_child(container_name, &self.member);
 
                         let on_absent = match default_ {
                             Flag::Absent => quote! {
@@ -411,7 +413,16 @@ impl FieldDef {
                                 init: quote! { ::std::option::Option::None },
                                 ty: option_ty(self.ty.clone()),
                             },
-                            matcher,
+                            matcher: quote! {
+                                match #matcher {
+                                    ::core::result::Result::Ok(v) => if #field_access.is_some() {
+                                        ::core::result::Result::Err(::xso::error::FromEventsError::Invalid(::xso::error::Error::Other(#duplicate_msg)))
+                                    } else {
+                                        ::core::result::Result::Ok(v)
+                                    },
+                                    ::core::result::Result::Err(e) => ::core::result::Result::Err(e),
+                                }
+                            },
                             builder,
                             collect: quote! {
                                 #field_access = ::std::option::Option::Some(#substate_result);