Skip to content

Skip empty text events after trim_text_end #986

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
JerryNee wants to merge 1 commit into
base: master
Choose a base branch
from
+120 −29
Open

Skip empty text events after trim_text_end #986

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions Changelog.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,8 @@

### Bug Fixes

- [#984]: Readers no longer emit empty `Text` events when `trim_text_end`
removes whitespace-only text before markup or a general reference.
- [#977]: `NamespaceResolver::push` (and hence every `NsReader` `Start`/`Empty`
event) now returns the new `NamespaceError::TooDeeplyNested` when a document
nests elements deeper than `u16::MAX`, instead of overflowing the internal
Expand All @@ -29,6 +31,7 @@

- [#983]: Adopted an AI use and contribution policy for new upstream contributions.

[#984]: https://github.com/tafia/quick-xml/issues/984
[#977]: https://github.com/tafia/quick-xml/issues/977
[#983]: https://github.com/tafia/quick-xml/issues/983

Expand Down
17 changes: 15 additions & 2 deletions src/reader/buffered_reader.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,7 @@ macro_rules! impl_buffered_source {
&mut self,
buf: &'b mut Vec<u8>,
position: &mut u64,
trim_text_end: bool,
) -> ReadTextResult<'b, &'b mut Vec<u8>> {
let mut read = 0;
let start = buf.len();
Expand Down Expand Up @@ -83,7 +84,13 @@ macro_rules! impl_buffered_source {
read += i as u64;

*position += read;
return ReadTextResult::UpToMarkup(&buf[start..]);
return if trim_text_end
&& buf[start..].iter().all(|&b| is_whitespace(b))
{
ReadTextResult::Markup(buf)
} else {
ReadTextResult::UpToMarkup(&buf[start..])
};
}
Some(i) => {
buf.extend_from_slice(&available[..i]);
Expand All @@ -92,7 +99,13 @@ macro_rules! impl_buffered_source {
read += i as u64;

*position += read;
return ReadTextResult::UpToRef(&buf[start..]);
return if trim_text_end
&& buf[start..].iter().all(|&b| is_whitespace(b))
{
ReadTextResult::Ref(buf)
} else {
ReadTextResult::UpToRef(&buf[start..])
};
}
None => {
buf.extend_from_slice(available);
Expand Down
108 changes: 86 additions & 22 deletions src/reader/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -326,7 +326,11 @@ macro_rules! read_event_impl {
$reader.skip_whitespace(&mut $self.state.offset) $(.$await)? ?;
}

match $reader.read_text($buf, &mut $self.state.offset) $(.$await)? {
match $reader.read_text(
$buf,
&mut $self.state.offset,
$self.state.config.trim_text_end,
) $(.$await)? {
ReadTextResult::Markup(buf) => {
$self.state.state = ParseState::InsideMarkup;
// Pass `buf` to the next next iteration of parsing loop
Expand All @@ -341,15 +345,10 @@ macro_rules! read_event_impl {
}
ReadTextResult::UpToMarkup(bytes) => {
$self.state.state = ParseState::InsideMarkup;
// FIXME: Can produce an empty event if:
// - event contains only spaces
// - trim_text_start = false
// - trim_text_end = true
Ok(Event::Text($self.state.emit_text(bytes)))
}
ReadTextResult::UpToRef(bytes) => {
$self.state.state = ParseState::InsideRef;
// Return Text event with `bytes` content or Eof if bytes is empty
Ok(Event::Text($self.state.emit_text(bytes)))
}
ReadTextResult::UpToEof(bytes) => {
Expand Down Expand Up @@ -494,17 +493,19 @@ macro_rules! read_to_end {
// it is important that this position indicates beginning of the End event.
// If between last event and the End event would be only spaces, then we
// take position before the spaces, but spaces would be skipped without
// generating event if `trim_text_start` is set to `true`. To prevent that
// we temporary disable start text trimming.
// generating event if either text trimming option is set to `true`. To
// prevent that we temporarily disable both text trimming options.
//
// We also cannot take position after getting End event, because if
// `trim_markup_names_in_closing_tags` is set to `true` (which is the default),
// we do not known the real size of the End event that it is occupies in
// the source and cannot correct the position after the End event.
// So, we in any case should tweak parser configuration.
let config = $self.config_mut();
let trim = config.trim_text_start;
let trim_start = config.trim_text_start;
let trim_end = config.trim_text_end;
config.trim_text_start = false;
config.trim_text_end = false;

let start = $self.buffer_position();
let mut depth = 0;
Expand All @@ -513,20 +514,26 @@ macro_rules! read_to_end {
let end = $self.buffer_position();
match $self.$read_event($buf) $(.$await)? {
Err(e) => {
$self.config_mut().trim_text_start = trim;
let config = $self.config_mut();
config.trim_text_start = trim_start;
config.trim_text_end = trim_end;
return Err(e);
}

Ok(Event::Start(e)) if e.name() == $end => depth += 1,
Ok(Event::End(e)) if e.name() == $end => {
if depth == 0 {
$self.config_mut().trim_text_start = trim;
let config = $self.config_mut();
config.trim_text_start = trim_start;
config.trim_text_end = trim_end;
break start..end;
}
depth -= 1;
}
Ok(Event::Eof) => {
$self.config_mut().trim_text_start = trim;
let config = $self.config_mut();
config.trim_text_start = trim_start;
config.trim_text_end = trim_end;
return Err(Error::missed_end($end, $self.decoder()));
}
_ => (),
Expand Down Expand Up @@ -997,11 +1004,11 @@ impl<R> Reader<R> {
/// Result of an attempt to read XML textual data from the source.
#[derive(Debug)]
enum ReadTextResult<'r, B> {
/// Start of markup (`<` character) was found in the first byte. `<` was consumed.
/// No text should be emitted before the start of markup (`<` character).
/// Contains buffer that should be returned back to the next iteration cycle
/// to satisfy borrow checker requirements.
Markup(B),
/// Start of reference (`&` character) was found in the first byte.
/// No text should be emitted before the start of reference (`&` character).
/// `&` was not consumed.
/// Contains buffer that should be returned back to the next iteration cycle
/// to satisfy borrow checker requirements.
Expand Down Expand Up @@ -1068,9 +1075,17 @@ trait XmlSource<'r, B> {
/// - `buf`: Buffer that could be filled from an input (`Self`) and
/// from which [events] could borrow their data
/// - `position`: Will be increased by amount of bytes consumed
/// - `trim_text_end`: Whether trailing whitespace should be trimmed. When
/// enabled, whitespace-only text before markup or a reference is skipped
/// so that `buf` can be reused by the next parser iteration
///
/// [events]: crate::events::Event
fn read_text(&mut self, buf: B, position: &mut u64) -> ReadTextResult<'r, B>;
fn read_text(
&mut self,
buf: B,
position: &mut u64,
trim_text_end: bool,
) -> ReadTextResult<'r, B>;

/// Read input until end of general reference (the `;`) is found, start of
/// another general reference (the `&`) is found or end of input is reached.
Expand Down Expand Up @@ -1645,7 +1660,7 @@ mod test {
let mut input = b"".as_ref();
// ^= 1

match $source(&mut input).read_text(buf, &mut position) $(.$await)? {
match $source(&mut input).read_text(buf, &mut position, false) $(.$await)? {
ReadTextResult::UpToEof(bytes) => assert_eq!(Bytes(bytes), Bytes(b"")),
x => panic!("Expected `UpToEof(_)`, but got `{:?}`", x),
}
Expand All @@ -1659,7 +1674,7 @@ mod test {
let mut input = b"<".as_ref();
// ^= 1

match $source(&mut input).read_text(buf, &mut position) $(.$await)? {
match $source(&mut input).read_text(buf, &mut position, false) $(.$await)? {
ReadTextResult::Markup(b) => assert_eq!(b, $buf),
x => panic!("Expected `Markup(_)`, but got `{:?}`", x),
}
Expand All @@ -1673,7 +1688,7 @@ mod test {
let mut input = b"&".as_ref();
// ^= 1

match $source(&mut input).read_text(buf, &mut position) $(.$await)? {
match $source(&mut input).read_text(buf, &mut position, false) $(.$await)? {
ReadTextResult::Ref(b) => assert_eq!(b, $buf),
x => panic!("Expected `Ref(_)`, but got `{:?}`", x),
}
Expand All @@ -1687,7 +1702,7 @@ mod test {
let mut input = b"a<".as_ref();
// ^= 2

match $source(&mut input).read_text(buf, &mut position) $(.$await)? {
match $source(&mut input).read_text(buf, &mut position, false) $(.$await)? {
ReadTextResult::UpToMarkup(bytes) => assert_eq!(Bytes(bytes), Bytes(b"a")),
x => panic!("Expected `UpToMarkup(_)`, but got `{:?}`", x),
}
Expand All @@ -1701,7 +1716,7 @@ mod test {
let mut input = b"a&".as_ref();
// ^= 2

match $source(&mut input).read_text(buf, &mut position) $(.$await)? {
match $source(&mut input).read_text(buf, &mut position, false) $(.$await)? {
ReadTextResult::UpToRef(bytes) => assert_eq!(Bytes(bytes), Bytes(b"a")),
x => panic!("Expected `UpToRef(_)`, but got `{:?}`", x),
}
Expand All @@ -1715,7 +1730,7 @@ mod test {
let mut input = b"a".as_ref();
// ^= 2

match $source(&mut input).read_text(buf, &mut position) $(.$await)? {
match $source(&mut input).read_text(buf, &mut position, false) $(.$await)? {
ReadTextResult::UpToEof(bytes) => assert_eq!(Bytes(bytes), Bytes(b"a")),
x => panic!("Expected `UpToEof(_)`, but got `{:?}`", x),
}
Expand Down Expand Up @@ -2059,7 +2074,7 @@ mod test {

/// Ensures, that no empty `Text` events are generated
mod $read_event {
use crate::events::{BytesCData, BytesDecl, BytesEnd, BytesPI, BytesStart, BytesText, Event};
use crate::events::{BytesCData, BytesDecl, BytesEnd, BytesPI, BytesRef, BytesStart, BytesText, Event};
use crate::reader::Reader;
use pretty_assertions::assert_eq;

Expand Down Expand Up @@ -2169,6 +2184,55 @@ mod test {
);
}

#[$test]
$($async)? fn trim_text_end_skips_empty_text_before_markup() {
let mut reader = Reader::from_str(" <tag/>");
reader.config_mut().trim_text_end = true;

assert_eq!(
reader.$read_event($buf) $(.$await)? .unwrap(),
Event::Empty(BytesStart::new("tag"))
);
assert_eq!(
reader.$read_event($buf) $(.$await)? .unwrap(),
Event::Eof
);
}

#[$test]
$($async)? fn trim_text_end_skips_empty_text_before_reference() {
let mut reader = Reader::from_str(" &amp;");
reader.config_mut().trim_text_end = true;

assert_eq!(
reader.$read_event($buf) $(.$await)? .unwrap(),
Event::GeneralRef(BytesRef::new("amp"))
);
assert_eq!(
reader.$read_event($buf) $(.$await)? .unwrap(),
Event::Eof
);
}

#[$test]
$($async)? fn trim_text_end_preserves_non_empty_text() {
let mut reader = Reader::from_str(" text <tag/>");
reader.config_mut().trim_text_end = true;

assert_eq!(
reader.$read_event($buf) $(.$await)? .unwrap(),
Event::Text(BytesText::new(" text"))
);
assert_eq!(
reader.$read_event($buf) $(.$await)? .unwrap(),
Event::Empty(BytesStart::new("tag"))
);
assert_eq!(
reader.$read_event($buf) $(.$await)? .unwrap(),
Event::Eof
);
}

#[$test]
$($async)? fn cdata() {
let mut reader = Reader::from_str("<![CDATA[]]>");
Expand Down
19 changes: 16 additions & 3 deletions src/reader/slice_reader.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -264,7 +264,12 @@ impl<'a> XmlSource<'a, ()> for &'a [u8] {
}

#[inline]
fn read_text(&mut self, _buf: (), position: &mut u64) -> ReadTextResult<'a, ()> {
fn read_text(
&mut self,
_buf: (),
position: &mut u64,
trim_text_end: bool,
) -> ReadTextResult<'a, ()> {
// Search for start of markup or an entity or character reference
match memchr::memchr2(b'<', b'&', self) {
Some(0) if self[0] == b'<' => ReadTextResult::Markup(()),
Expand All @@ -275,13 +280,21 @@ impl<'a> XmlSource<'a, ()> for &'a [u8] {
let (bytes, rest) = self.split_at(i);
*self = rest;
*position += i as u64;
ReadTextResult::UpToMarkup(bytes)
if trim_text_end && bytes.iter().all(|&b| is_whitespace(b)) {
ReadTextResult::Markup(())
} else {
ReadTextResult::UpToMarkup(bytes)
}
}
Some(i) => {
let (bytes, rest) = self.split_at(i);
*self = rest;
*position += i as u64;
ReadTextResult::UpToRef(bytes)
if trim_text_end && bytes.iter().all(|&b| is_whitespace(b)) {
ReadTextResult::Ref(())
} else {
ReadTextResult::UpToRef(bytes)
}
}
None => {
let bytes = &self[..];
Expand Down
2 changes: 0 additions & 2 deletions tests/reader-config.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -895,9 +895,7 @@ mod trim_text_end {
assert_eq!(reader.read_event().unwrap(), Event::Eof);
}

// TODO: Enable test after rewriting parser
#[test]
#[ignore = "currently it is hard to fix incorrect behavior, but this will much easy after parser rewrite"]
fn true_() {
let mut reader = Reader::from_str(XML);
reader.config_mut().trim_text_end = true;
Expand Down
Loading