Deserialize custom entities inside attributes

[mmirate]

As shown by the new tests, the result is that deserialization will process custom entities in attributes as well as in text content.

This seems to require some breaking changes to the public API.

[codecov-commenter]

⚠️ Please install the to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

❌ Patch coverage is 81.25000% with 12 lines in your changes missing coverage. Please review.
✅ Project coverage is 56.44%. Comparing base (a759d65) to head (88ba6fc).
⚠️ Report is 12 commits behind head on master.

Files with missing lines	Patch %	Lines
src/de/attributes.rs	0.00%	5 Missing ⚠️
src/de/map.rs	84.00%	4 Missing ⚠️
src/de/mod.rs	80.00%	2 Missing ⚠️
src/de/text.rs	0.00%	1 Missing ⚠️
❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #951      +/-   ##
==========================================
+ Coverage   55.08%   56.44%   +1.35%     
==========================================
  Files          44       44              
  Lines       16911    17627     +716     
==========================================
+ Hits         9316     9949     +633     
- Misses       7595     7678      +83     

Flag	Coverage Δ
unittests	56.44% <81.25%> (+1.35%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[Mingun]

It seems to me that current implementation will apply entity resolving twice for the text case:

#[test]
fn need_to_give_reasonable_name() {
    let value: String = from_str("<root>&amp;lt;</root>").unwrap();
    assert_eq!(value, "&lt;");
}

That test should be added.

Probably it would be better to create separate deserializer for attribute values rather that reuse SimpleTypeDeserializer. It is hard to say without trying.

[Mingun]

SimpleTypeDeserializer created from text content cannot contain entities, as noted in the docs, so it is redundant to specify entity resolver here. Text contains already merged text and expanded entities:

quick-xml/src/de/mod.rs

Lines 2382 to 2434 in 6238d8a

	/// Read all consequent [`Text`] and [`CData`] events until non-text event
	/// occurs. Content of all events would be appended to `result` and returned
	/// as [`DeEvent::Text`].
	///
	/// [`Text`]: PayloadEvent::Text
	/// [`CData`]: PayloadEvent::CData
	fn drain_text(&mut self, mut result: Cow<'i, str>) -> Result<DeEvent<'i>, DeError> {
	loop {
	if self.current_event_is_last_text() {
	break;
	}
	match self.next_impl()? {
	PayloadEvent::Text(e) => result
	.to_mut()
	.push_str(&e.xml_content(self.reader.xml_version())?),
	PayloadEvent::CData(e) => result
	.to_mut()
	.push_str(&e.xml_content(self.reader.xml_version())?),
	PayloadEvent::GeneralRef(e) => self.resolve_reference(result.to_mut(), e)?,
	// SAFETY: current_event_is_last_text checks that event is Text, CData or GeneralRef
	_ => unreachable!("Only `Text`, `CData` or `GeneralRef` events can come here"),
	}
	}
	Ok(DeEvent::Text(Text::new(result)))
	}
	/// Return an input-borrowing event.
	fn next(&mut self) -> Result<DeEvent<'i>, DeError> {
	loop {
	return match self.next_impl()? {
	PayloadEvent::Start(e) => Ok(DeEvent::Start(e)),
	PayloadEvent::End(e) => Ok(DeEvent::End(e)),
	PayloadEvent::Text(e) => self.drain_text(e.xml_content(self.reader.xml_version())?),
	PayloadEvent::CData(e) => {
	self.drain_text(e.xml_content(self.reader.xml_version())?)
	}
	PayloadEvent::DocType(e) => {
	self.entity_resolver
	.capture(e)
	.map_err(|err| DeError::Custom(format!("cannot parse DTD: {}", err)))?;
	continue;
	}
	PayloadEvent::GeneralRef(e) => {
	let mut text = String::new();
	self.resolve_reference(&mut text, e)?;
	self.drain_text(text.into())
	}
	PayloadEvent::Eof => Ok(DeEvent::Eof),
	};
	}
	}

Please add the following test:

#[test]
fn need_to_give_reasonable_name() {
    let value: String = from_str("<root>&amp;lt;</root>").unwrap();
    assert_eq!(value, "&lt;");
}

[mmirate]

The abovementioned test will pass.

This test will also pass:

#[test]
fn need_to_give_reasonable_name() {
    let value: BTreeMap<String, String> = from_str(r#"<root attr="&amp;lt;" />"#).unwrap();
    assert_eq!(value, BTreeMap::from_iter([
        (String::from("@attr"), String::from("&lt;"))
    ]));
}

The tests where instead of & there is a custom entity resolver that supports a custom analogue of & are more longwinded. These tests pass for < in element text but produce an UnterminatedEntity error for < as attribute content. I'm not sure whether that is a bug or an inevitable consequence of the recursive nature of "attribute normalization".

[Mingun]

entity_resolver should be stored by value.

[mmirate]

This would require EntityResolver to have Clone as a supertrait - is this desirable?

[Mingun]

To keep as least one validity constraint of XML, use the name as the root tag:

Suggested change

	<!DOCTYPE dict[ <!ENTITY unc "unclassified"> ]>
	<!DOCTYPE root[ <!ENTITY unc "unclassified"> ]>

[Mingun]

And here

Suggested change

	<!DOCTYPE dict[ <!ENTITY unc "unclassified"> ]>
	<!DOCTYPE root[ <!ENTITY unc "unclassified"> ]>

[Mingun]

Probably

impl AttributesDeserializer {
  fn with_resolver<E: EntityResolver>(self, resolver) -> AttributesDeserializer<E> {
    // replace entity_resolver with new one
  }
}

would be more practical, because I think, in most cases PredefinedEntityResolver will be used. So if you need use specific resolver, you may write a chain:

attributes.into_map_access("@").with_resolver(my_resolver)