tafia / quick-xml

Rust high performance xml reader and writer
MIT License
1.18k stars 235 forks source link

Rework handling general entity references (`&entity;`) #766

Open Mingun opened 3 months ago

Mingun commented 3 months ago

This is a big change in handling general entity references and character references. Open PR early to get feedback.

With this changes we can correctly parse document

<!DOCTYPE root [
  <!ENTITY root "<root/>">
]>
&root;

as equivalent normalized document

<root/>

The updated custom_entities example shows how it would be possible to implement requirement from the specification about parsed general entities. Serde deserializer did not updated yet, because this is not trivial part and probably that will be done in another PR.

Of course, such change probably makes the performance worse, I didn't measure impact yet.

Closes #667

codecov-commenter commented 3 months ago

:warning: Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

Attention: Patch coverage is 61.13537% with 178 lines in your changes missing coverage. Please review.

Project coverage is 60.13%. Comparing base (7558577) to head (eb90e9f). Report is 94 commits behind head on master.

Files Patch % Lines
examples/custom_entities.rs 0.00% 117 Missing :warning:
src/events/mod.rs 38.98% 36 Missing :warning:
src/reader/buffered_reader.rs 83.09% 12 Missing :warning:
benches/macrobenches.rs 0.00% 4 Missing :warning:
src/de/mod.rs 88.46% 3 Missing :warning:
src/errors.rs 0.00% 3 Missing :warning:
benches/microbenches.rs 0.00% 1 Missing :warning:
src/reader/slice_reader.rs 97.56% 1 Missing :warning:
src/writer/async_tokio.rs 0.00% 1 Missing :warning:

:exclamation: Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #766 +/- ## ========================================== - Coverage 61.81% 60.13% -1.69% ========================================== Files 41 41 Lines 16798 16492 -306 ========================================== - Hits 10384 9917 -467 - Misses 6414 6575 +161 ``` | [Flag](https://app.codecov.io/gh/tafia/quick-xml/pull/766/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None) | Coverage Δ | | |---|---|---| | [unittests](https://app.codecov.io/gh/tafia/quick-xml/pull/766/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None) | `60.13% <61.13%> (-1.69%)` | :arrow_down: | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None#carryforward-flags-in-the-pull-request-comment) to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

Mingun commented 3 months ago

I finished work on the base part of the entities support. In this PR new Event::GeneralRef is added together with new BytesRef struct which represents the any &...; reference, including entity references and character references. Character references can be resolved by call BytesRef::resolve_char_ref(), entity references can be resolved by mapping from content of BytesRef to replacement text. Both usages are shown in the updated custom_entities example.

dralley commented 3 months ago

I won't have a chance to fully review this for a couple of days. Quick question though, am I correct in thinking that this PR will mean that any time a text block contains one or more entity references, instead of the developer receiving one Event::Text containing everything between the opening and closing tags, they will receive a series of Event::Text and Event::GeneralRef which they will then need to merge back together themselves into the original text?

Mingun commented 3 months ago

Yes, you are correct. But that does not mean that he/she will needed to construct the complete text themselves. In the next PR I plan to rename Reader to the RawReader and add make it return borrow-only RawEvents, and in in the another PR introduce new Reader which will automatically merge all consequent Text, CData and GeneralRef events. This should be much more convenient for the average user. RawReader will only be needed for very fine control.

Because renames affects very many places, I want to do that in a separate PR to reduce noise in PR with new Reader.

Borrow-only reader-only events will be useful also in that sense that I plan to add offset member to them to track event position in the stream. When you construct event for writing you are obviously does not have position and I think it is better to not have a dummy value for it, in order to you couldn't mistakenly use writer event in the reading context and get the wrong position.

Because new Reader will have a stack of the RawReaders (in the same way as demonstrated in custom_entities example), it will be simple recreate readers when we will need to change encoding, so I think, that #158 is very close to resolving.

Mingun commented 2 months ago

@dralley, what do you think about this?