Add functions for attribute value normalization

dralley · dralley · commit f317d76a2155 · 2023-06-19T18:46:45.000-04:00
closes #371
diff --git a/Changelog.md b/Changelog.md
@@ -14,8 +14,17 @@
 
 ### Bug Fixes
 
+- [#379]: Improved compliance with the XML attribute value normalization process by
+  adding `Attribute::normalized_value()` and `Attribute::normalized_value_with()`,
+  which ought to be used in place of `Attribute::unescape_value()` and
+  `Attribute::unescape_value_with()`
+### Bug Fixes
+
 ### Misc Changes
 
+- [#379]: Added tests for attribute value normalization
+
+[#379]: https://github.com/tafia/quick-xml/pull/379
 
 ## 0.29.0 -- 2023-06-13
 
@@ -111,6 +120,10 @@
 - [#565]: Fix compilation error when build with serde <1.0.139
 
 
+### New Tests
+
+- [#379]: Added tests for attribute value normalization
+
 [externally tagged]: https://serde.rs/enum-representations.html#externally-tagged
 [#490]: https://github.com/tafia/quick-xml/pull/490
 [#510]: https://github.com/tafia/quick-xml/issues/510
diff --git a/benches/macrobenches.rs b/benches/macrobenches.rs
@@ -43,14 +43,13 @@ static INPUTS: &[(&str, &str)] = &[
     ("players.xml", PLAYERS),
 ];
 
-// TODO: use fully normalized attribute values
 fn parse_document_from_str(doc: &str) -> XmlResult<()> {
     let mut r = Reader::from_str(doc);
     loop {
         match criterion::black_box(r.read_event()?) {
             Event::Start(e) | Event::Empty(e) => {
                 for attr in e.attributes() {
-                    criterion::black_box(attr?.decode_and_unescape_value(&r)?);
+                    criterion::black_box(attr?.decode_and_normalize_value(&r)?);
                 }
             }
             Event::Text(e) => {
@@ -67,15 +66,14 @@ fn parse_document_from_str(doc: &str) -> XmlResult<()> {
     Ok(())
 }
 
-// TODO: use fully normalized attribute values
 fn parse_document_from_bytes(doc: &[u8]) -> XmlResult<()> {
     let mut r = Reader::from_reader(doc);
     let mut buf = Vec::new();
     loop {
         match criterion::black_box(r.read_event_into(&mut buf)?) {
             Event::Start(e) | Event::Empty(e) => {
                 for attr in e.attributes() {
-                    criterion::black_box(attr?.decode_and_unescape_value(&r)?);
+                    criterion::black_box(attr?.decode_and_normalize_value(&r)?);
                 }
             }
             Event::Text(e) => {
@@ -93,15 +91,14 @@ fn parse_document_from_bytes(doc: &[u8]) -> XmlResult<()> {
     Ok(())
 }
 
-// TODO: use fully normalized attribute values
 fn parse_document_from_str_with_namespaces(doc: &str) -> XmlResult<()> {
     let mut r = NsReader::from_str(doc);
     loop {
         match criterion::black_box(r.read_resolved_event()?) {
             (resolved_ns, Event::Start(e) | Event::Empty(e)) => {
                 criterion::black_box(resolved_ns);
                 for attr in e.attributes() {
-                    criterion::black_box(attr?.decode_and_unescape_value(&r)?);
+                    criterion::black_box(attr?.decode_and_normalize_value(&r)?);
                 }
             }
             (resolved_ns, Event::Text(e)) => {
@@ -120,7 +117,6 @@ fn parse_document_from_str_with_namespaces(doc: &str) -> XmlResult<()> {
     Ok(())
 }
 
-// TODO: use fully normalized attribute values
 fn parse_document_from_bytes_with_namespaces(doc: &[u8]) -> XmlResult<()> {
     let mut r = NsReader::from_reader(doc);
     let mut buf = Vec::new();
@@ -129,7 +125,7 @@ fn parse_document_from_bytes_with_namespaces(doc: &[u8]) -> XmlResult<()> {
             (resolved_ns, Event::Start(e) | Event::Empty(e)) => {
                 criterion::black_box(resolved_ns);
                 for attr in e.attributes() {
-                    criterion::black_box(attr?.decode_and_unescape_value(&r)?);
+                    criterion::black_box(attr?.decode_and_normalize_value(&r)?);
                 }
             }
             (resolved_ns, Event::Text(e)) => {
diff --git a/benches/microbenches.rs b/benches/microbenches.rs
@@ -1,6 +1,9 @@
+use std::borrow::Cow;
+
 use criterion::{self, criterion_group, criterion_main, Criterion};
 use pretty_assertions::assert_eq;
 use quick_xml::escape::{escape, unescape};
+use quick_xml::events::attributes::Attribute;
 use quick_xml::events::Event;
 use quick_xml::name::QName;
 use quick_xml::reader::{NsReader, Reader};
@@ -242,6 +245,74 @@ fn attributes(c: &mut Criterion) {
             assert_eq!(count, 150);
         })
     });
+
+    group.finish();
+}
+
+/// Benchmarks normalizing attribute values
+fn attribute_value_normalization(c: &mut Criterion) {
+    let mut group = c.benchmark_group("attribute_value_normalization");
+
+    group.bench_function("noop_short", |b| {
+        let attr = Attribute {
+            key: QName(b"foo"),
+            value: Cow::Borrowed(b"foobar"),
+        };
+        b.iter(|| {
+            criterion::black_box(attr.normalized_value()).unwrap();
+        })
+    });
+
+    group.bench_function("noop_long", |b| {
+        let attr = Attribute {
+            key: QName(b"foo"),
+            value: Cow::Borrowed(LOREM_IPSUM_TEXT.as_bytes()),
+        };
+        b.iter(|| {
+            criterion::black_box(attr.normalized_value()).unwrap();
+        })
+    });
+
+    group.bench_function("replacement_chars", |b| {
+        let attr = Attribute {
+            key: QName(b"foo"),
+            value: Cow::Borrowed(b"just a bit\n of text without\tany entities"),
+        };
+        b.iter(|| {
+            criterion::black_box(attr.normalized_value()).unwrap();
+        })
+    });
+
+    group.bench_function("char_reference", |b| {
+        let attr1 = Attribute {
+            key: QName(b"foo"),
+            value: Cow::Borrowed(b"prefix &#34;some stuff&#34;,&#x22;more stuff&#x22;"),
+        };
+        let attr2 = Attribute {
+            key: QName(b"foo"),
+            value: Cow::Borrowed(b"&#38;&#60;"),
+        };
+        b.iter(|| {
+            criterion::black_box(attr1.normalized_value()).unwrap();
+            criterion::black_box(attr2.normalized_value()).unwrap();
+        })
+    });
+
+    group.bench_function("entity_reference", |b| {
+        let attr1 = Attribute {
+            key: QName(b"foo"),
+            value: Cow::Borrowed(b"age &gt; 72 &amp;&amp; age &lt; 21"),
+        };
+        let attr2 = Attribute {
+            key: QName(b"foo"),
+            value: Cow::Borrowed(b"&quot;what&apos;s that?&quot;"),
+        };
+        b.iter(|| {
+            criterion::black_box(attr1.normalized_value()).unwrap();
+            criterion::black_box(attr2.normalized_value()).unwrap();
+        })
+    });
+
     group.finish();
 }
 
@@ -354,6 +425,7 @@ criterion_group!(
     read_resolved_event_into,
     one_event,
     attributes,
+    attribute_value_normalization,
     escaping,
     unescaping,
 );
diff --git a/src/errors.rs b/src/errors.rs
@@ -82,6 +82,7 @@ impl From<EscapeError> for Error {
 }
 
 impl From<AttrError> for Error {
+    /// Creates a new `Error::InvalidAttr` from the given error
     #[inline]
     fn from(error: AttrError) -> Self {
         Error::InvalidAttr(error)
diff --git a/src/escapei.rs b/src/escapei.rs
diff --git a/src/events/attributes.rs b/src/events/attributes.rs

Original file line number	Diff line number	Diff line change
`@@ -43,14 +43,13 @@ static INPUTS: &[(&str, &str)] = &[`
`43`	`43`	`("players.xml", PLAYERS),`
`44`	`44`	`];`
`45`	`45`
`46`		`-// TODO: use fully normalized attribute values`
`47`	`46`	`fn parse_document_from_str(doc: &str) -> XmlResult<()> {`
`48`	`47`	`let mut r = Reader::from_str(doc);`
`49`	`48`	`loop {`
`50`	`49`	`match criterion::black_box(r.read_event()?) {`
`51`	`50`	`Event::Start(e) \| Event::Empty(e) => {`
`52`	`51`	`for attr in e.attributes() {`
`53`		`- criterion::black_box(attr?.decode_and_unescape_value(&r)?);`
	`52`	`+ criterion::black_box(attr?.decode_and_normalize_value(&r)?);`
`54`	`53`	`}`
`55`	`54`	`}`
`56`	`55`	`Event::Text(e) => {`
`@@ -67,15 +66,14 @@ fn parse_document_from_str(doc: &str) -> XmlResult<()> {`
`67`	`66`	`Ok(())`
`68`	`67`	`}`
`69`	`68`
`70`		`-// TODO: use fully normalized attribute values`
`71`	`69`	`fn parse_document_from_bytes(doc: &[u8]) -> XmlResult<()> {`
`72`	`70`	`let mut r = Reader::from_reader(doc);`
`73`	`71`	`let mut buf = Vec::new();`
`74`	`72`	`loop {`
`75`	`73`	`match criterion::black_box(r.read_event_into(&mut buf)?) {`
`76`	`74`	`Event::Start(e) \| Event::Empty(e) => {`
`77`	`75`	`for attr in e.attributes() {`
`78`		`- criterion::black_box(attr?.decode_and_unescape_value(&r)?);`
	`76`	`+ criterion::black_box(attr?.decode_and_normalize_value(&r)?);`
`79`	`77`	`}`
`80`	`78`	`}`
`81`	`79`	`Event::Text(e) => {`
`@@ -93,15 +91,14 @@ fn parse_document_from_bytes(doc: &[u8]) -> XmlResult<()> {`
`93`	`91`	`Ok(())`
`94`	`92`	`}`
`95`	`93`
`96`		`-// TODO: use fully normalized attribute values`
`97`	`94`	`fn parse_document_from_str_with_namespaces(doc: &str) -> XmlResult<()> {`
`98`	`95`	`let mut r = NsReader::from_str(doc);`
`99`	`96`	`loop {`
`100`	`97`	`match criterion::black_box(r.read_resolved_event()?) {`
`101`	`98`	`(resolved_ns, Event::Start(e) \| Event::Empty(e)) => {`
`102`	`99`	`criterion::black_box(resolved_ns);`
`103`	`100`	`for attr in e.attributes() {`
`104`		`- criterion::black_box(attr?.decode_and_unescape_value(&r)?);`
	`101`	`+ criterion::black_box(attr?.decode_and_normalize_value(&r)?);`
`105`	`102`	`}`
`106`	`103`	`}`
`107`	`104`	`(resolved_ns, Event::Text(e)) => {`
`@@ -120,7 +117,6 @@ fn parse_document_from_str_with_namespaces(doc: &str) -> XmlResult<()> {`
`120`	`117`	`Ok(())`
`121`	`118`	`}`
`122`	`119`
`123`		`-// TODO: use fully normalized attribute values`
`124`	`120`	`fn parse_document_from_bytes_with_namespaces(doc: &[u8]) -> XmlResult<()> {`
`125`	`121`	`let mut r = NsReader::from_reader(doc);`
`126`	`122`	`let mut buf = Vec::new();`
`@@ -129,7 +125,7 @@ fn parse_document_from_bytes_with_namespaces(doc: &[u8]) -> XmlResult<()> {`
`129`	`125`	`(resolved_ns, Event::Start(e) \| Event::Empty(e)) => {`
`130`	`126`	`criterion::black_box(resolved_ns);`
`131`	`127`	`for attr in e.attributes() {`
`132`		`- criterion::black_box(attr?.decode_and_unescape_value(&r)?);`
	`128`	`+ criterion::black_box(attr?.decode_and_normalize_value(&r)?);`
`133`	`129`	`}`
`134`	`130`	`}`
`135`	`131`	`(resolved_ns, Event::Text(e)) => {`
Original file line number	Diff line number	Diff line change
`@@ -82,6 +82,7 @@ impl From<EscapeError> for Error {`
`82`	`82`	`}`
`83`	`83`
`84`	`84`	`impl From<AttrError> for Error {`
	`85`	+ /// Creates a new `Error::InvalidAttr` from the given error
`85`	`86`	`#[inline]`
`86`	`87`	`fn from(error: AttrError) -> Self {`
`87`	`88`	`Error::InvalidAttr(error)`