minidom: Make `ElementBuilder::prefix` fail on adding duplicate prefix

Maxime “pep” Buquet created 5 years ago

Signed-off-by: Maxime “pep” Buquet <[e-mail]>

Change summary

minidom-rs/src/element.rs | 10 ++++++----
minidom-rs/src/error.rs   |  5 +++++
minidom-rs/src/tests.rs   | 20 ++++++++++----------
3 files changed, 21 insertions(+), 14 deletions(-)

Detailed changes

minidom-rs/src/element.rs

@@ -194,7 +194,7 @@ impl Element {
             name.into(),
             namespace.into(),
             None,
-            BTreeMap::new(),
+            None,
             BTreeMap::new(),
             Vec::new(),
         )
@@ -987,10 +987,12 @@ pub struct ElementBuilder {
 
 impl ElementBuilder {
     /// Sets a custom prefix. It is not possible to set the same prefix twice.
-    pub fn prefix<S: Into<Namespace>>(mut self, prefix: Prefix, namespace: S) -> ElementBuilder {
-        // TODO: Make this actually send back an error when a duplicate prefix is added.
+    pub fn prefix<S: Into<Namespace>>(mut self, prefix: Prefix, namespace: S) -> Result<ElementBuilder> {
+        if let Some(_) = self.root.prefixes.get(&prefix) {
+            return Err(Error::DuplicatePrefix);
+        }
         self.root.prefixes.insert(prefix, namespace.into());
-        self
+        Ok(self)
     }
 
     /// Sets an attribute.

minidom-rs/src/error.rs

@@ -44,6 +44,9 @@ pub enum Error {
 
     /// An error which is returned when a comment is to be parsed by minidom
     NoComments,
+
+    /// An error which is returned when a prefixed is defined twice
+    DuplicatePrefix,
 }
 
 impl StdError for Error {
@@ -58,6 +61,7 @@ impl StdError for Error {
             Error::InvalidPrefix => None,
             Error::MissingNamespace => None,
             Error::NoComments => None,
+            Error::DuplicatePrefix => None,
         }
     }
 }
@@ -84,6 +88,7 @@ impl std::fmt::Display for Error {
                 fmt,
                 "a comment has been found even though comments are forbidden"
             ),
+            Error::DuplicatePrefix => write!(fmt, "the prefix is already defined"),
         }
     }
 }

minidom-rs/src/tests.rs

@@ -77,8 +77,8 @@ fn test_real_data() {
         .append(correction)
         .build();
     let stream = Element::builder("stream", "http://etherx.jabber.org/streams")
-        .prefix(Some(String::from("stream")), "http://etherx.jabber.org/streams")
-        .prefix(None, "jabber:client")
+        .prefix(Some(String::from("stream")), "http://etherx.jabber.org/streams").unwrap()
+        .prefix(None, "jabber:client").unwrap()
         .append(message)
         .build();
     println!("{}", String::from(&stream));
@@ -109,8 +109,8 @@ fn test_real_data() {
         .append(pubsub)
         .build();
     let stream = Element::builder("stream", "http://etherx.jabber.org/streams")
-        .prefix(Some(String::from("stream")), "http://etherx.jabber.org/streams")
-        .prefix(None, "jabber:client")
+        .prefix(Some(String::from("stream")), "http://etherx.jabber.org/streams").unwrap()
+        .prefix(None, "jabber:client").unwrap()
         .append(iq)
         .build();
 
@@ -141,8 +141,8 @@ fn writer_with_decl_works() {
 #[test]
 fn writer_with_prefix() {
     let root = Element::builder("root", "ns1")
-        .prefix(Some(String::from("p1")), "ns1")
-        .prefix(None, "ns2")
+        .prefix(Some(String::from("p1")), "ns1").unwrap()
+        .prefix(None, "ns2").unwrap()
         .build();
     assert_eq!(String::from(&root),
         r#"<p1:root xmlns="ns2" xmlns:p1="ns1"/>"#,
@@ -168,7 +168,7 @@ fn writer_no_prefix_namespace_child() {
     assert_eq!(String::from(&root), r#"<root xmlns="ns1"><child/></root>"#);
 
     let child = Element::builder("child", "ns2")
-        .prefix(None, "ns3")
+        .prefix(None, "ns3").unwrap()
         .build();
     let root = Element::builder("root", "ns1")
         .append(child)
@@ -181,7 +181,7 @@ fn writer_no_prefix_namespace_child() {
 fn writer_prefix_namespace_child() {
     let child = Element::builder("child", "ns1").build();
     let root = Element::builder("root", "ns1")
-        .prefix(Some(String::from("p1")), "ns1")
+        .prefix(Some(String::from("p1")), "ns1").unwrap()
         .append(child)
         .build();
     assert_eq!(String::from(&root), r#"<p1:root xmlns:p1="ns1"><p1:child/></p1:root>"#);
@@ -193,8 +193,8 @@ fn writer_with_prefix_deduplicate() {
         // .prefix(Some(String::from("p1")), "ns1")
         .build();
     let root = Element::builder("root", "ns1")
-        .prefix(Some(String::from("p1")), "ns1")
-        .prefix(None, "ns2")
+        .prefix(Some(String::from("p1")), "ns1").unwrap()
+        .prefix(None, "ns2").unwrap()
         .append(child)
         .build();
     assert_eq!(String::from(&root),