Skip to content

Fix #435: replay only events from time of creating SeqAccess struct #453

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

Merged
merged 3 commits into from
Aug 7, 2022
Merged

Fix #435: replay only events from time of creating SeqAccess struct #453

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
f2b9441
Move replay starting to the `Drop` implementation of SeqAccess implem…
Mingun Aug 6, 2022
9ecbec4
Add tests for replaying only part of skipped events
Mingun Jul 23, 2022
656081b
#435: Fix incorrect results when nesting `start_replay()` calls
Mingun Aug 6, 2022
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
26 changes: 21 additions & 5 deletions src/de/map.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -553,13 +553,13 @@ where
} else {
TagFilter::Exclude(self.map.fields)
};
let seq = visitor.visit_seq(MapValueSeqAccess {
visitor.visit_seq(MapValueSeqAccess {
#[cfg(feature = "overlapped-lists")]
checkpoint: self.map.de.skip_checkpoint(),

map: self.map,
filter,
});
#[cfg(feature = "overlapped-lists")]
self.map.de.start_replay();
seq
})
}

#[inline]
Expand Down Expand Up @@ -588,6 +588,22 @@ where
/// When feature `overlapped-lists` is activated, all tags, that not pass
/// this check, will be skipped.
filter: TagFilter<'de>,

/// Checkpoint after which all skipped events should be returned. All events,
/// that was skipped before creating this checkpoint, will still stay buffered
/// and will not be returned
#[cfg(feature = "overlapped-lists")]
checkpoint: usize,
}

#[cfg(feature = "overlapped-lists")]
impl<'de, 'a, 'm, R> Drop for MapValueSeqAccess<'de, 'a, 'm, R>
where
R: XmlRead<'de>,
{
fn drop(&mut self) {
self.map.de.start_replay(self.checkpoint);
}
}

impl<'de, 'a, 'm, R> SeqAccess<'de> for MapValueSeqAccess<'de, 'a, 'm, R>
Expand Down
248 changes: 236 additions & 12 deletions src/de/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -483,6 +483,14 @@ where
self.reader.next()
}

/// Returns the mark after which all events, skipped by [`Self::skip()`] call,
/// should be replayed after calling [`Self::start_replay()`].
#[cfg(feature = "overlapped-lists")]
#[inline]
fn skip_checkpoint(&self) -> usize {
self.write.len()
}

/// Extracts XML tree of events from and stores them in the skipped events
/// buffer from which they can be retrieved later. You MUST call
/// [`Self::start_replay()`] after calling this to give access to the skipped
Expand Down Expand Up @@ -530,8 +538,8 @@ where
Ok(())
}

/// Moves all buffered events to the end of [`Self::write`] buffer and swaps
/// read and write buffers.
/// Moves buffered events, skipped after given `checkpoint` from [`Self::write`]
/// skip buffer to [`Self::read`] buffer.
///
/// After calling this method, [`Self::peek()`] and [`Self::next()`] starts
/// return events that was skipped previously by calling [`Self::skip()`],
Expand All @@ -541,9 +549,15 @@ where
/// This method MUST be called if any number of [`Self::skip()`] was called
/// after [`Self::new()`] or `start_replay()` or you'll lost events.
#[cfg(feature = "overlapped-lists")]
fn start_replay(&mut self) {
self.write.append(&mut self.read);
std::mem::swap(&mut self.read, &mut self.write);
fn start_replay(&mut self, checkpoint: usize) {
if checkpoint == 0 {
self.write.append(&mut self.read);
std::mem::swap(&mut self.read, &mut self.write);
} else {
let mut read = self.write.split_off(checkpoint);
read.append(&mut self.read);
self.read = read;
}
}

fn next_start(&mut self) -> Result<Option<BytesStart<'de>>, DeError> {
Expand Down Expand Up @@ -828,10 +842,7 @@ where
where
V: Visitor<'de>,
{
let seq = visitor.visit_seq(seq::TopLevelSeqAccess::new(self)?);
#[cfg(feature = "overlapped-lists")]
self.start_replay();
seq
visitor.visit_seq(seq::TopLevelSeqAccess::new(self)?)
}

fn deserialize_map<V>(self, visitor: V) -> Result<V::Value, DeError>
Expand Down Expand Up @@ -1024,6 +1035,10 @@ mod tests {
assert_eq!(de.next().unwrap(), Start(BytesStart::new("root")));
assert_eq!(de.peek().unwrap(), &Start(BytesStart::new("inner")));

// Mark that start_replay() should begin replay from this point
let checkpoint = de.skip_checkpoint();
assert_eq!(checkpoint, 0);

// Should skip first <inner> tree
de.skip().unwrap();
assert_eq!(de.read, vec![]);
Expand Down Expand Up @@ -1060,7 +1075,7 @@ mod tests {
//
// <target/>
// </root>
de.start_replay();
de.start_replay(checkpoint);
assert_eq!(
de.read,
vec![
Expand All @@ -1074,6 +1089,10 @@ mod tests {
assert_eq!(de.write, vec![]);
assert_eq!(de.next().unwrap(), Start(BytesStart::new("inner")));

// Mark that start_replay() should begin replay from this point
let checkpoint = de.skip_checkpoint();
assert_eq!(checkpoint, 0);

// Skip `#text` node and consume <inner/> after it
de.skip().unwrap();
assert_eq!(
Expand Down Expand Up @@ -1105,7 +1124,7 @@ mod tests {
//
// <target/>
// </root>
de.start_replay();
de.start_replay(checkpoint);
assert_eq!(
de.read,
vec![
Expand All @@ -1119,6 +1138,7 @@ mod tests {
assert_eq!(de.next().unwrap(), Start(BytesStart::new("target")));
assert_eq!(de.next().unwrap(), End(BytesEnd::new("target")));
assert_eq!(de.next().unwrap(), End(BytesEnd::new("root")));
assert_eq!(de.next().unwrap(), Eof);
}

/// Checks that `read_to_end()` behaves correctly after `skip()`
Expand All @@ -1144,6 +1164,10 @@ mod tests {

assert_eq!(de.next().unwrap(), Start(BytesStart::new("root")));

// Mark that start_replay() should begin replay from this point
let checkpoint = de.skip_checkpoint();
assert_eq!(checkpoint, 0);

// Skip the <skip> tree
de.skip().unwrap();
assert_eq!(de.read, vec![]);
Expand Down Expand Up @@ -1189,7 +1213,7 @@ mod tests {
// and after that stream that messages:
//
// </root>
de.start_replay();
de.start_replay(checkpoint);
assert_eq!(
de.read,
vec![
Expand All @@ -1206,6 +1230,206 @@ mod tests {
de.read_to_end(QName(b"skip")).unwrap();

assert_eq!(de.next().unwrap(), End(BytesEnd::new("root")));
assert_eq!(de.next().unwrap(), Eof);
}

/// Checks that replay replayes only part of events
/// Test for https://github.com/tafia/quick-xml/issues/435
#[test]
fn partial_replay() {
let mut de = Deserializer::from_str(
r#"
<root>
<skipped-1/>
<skipped-2/>
<inner>
<skipped-3/>
<skipped-4/>
<target-2/>
</inner>
<target-1/>
</root>
"#,
);

// Initial conditions - both are empty
assert_eq!(de.read, vec![]);
Copy link
Collaborator

@dralley dralley Aug 6, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just .is_empty() is cleaner

Copy link
Collaborator Author

@Mingun Mingun Aug 7, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would like to keep comparing with empty vector for two reasons:

  • keep uniformity of checks in the test
  • if check failed, I would see actual content of a field

assert_eq!(de.write, vec![]);

assert_eq!(de.next().unwrap(), Start(BytesStart::new("root")));

// start_replay() should start replay from this point
let checkpoint1 = de.skip_checkpoint();
assert_eq!(checkpoint1, 0);

// Should skip first and second <skipped-N/> elements
de.skip().unwrap(); // skipped-1
de.skip().unwrap(); // skipped-2
assert_eq!(de.read, vec![]);
assert_eq!(
de.write,
vec![
Start(BytesStart::new("skipped-1")),
End(BytesEnd::new("skipped-1")),
Start(BytesStart::new("skipped-2")),
End(BytesEnd::new("skipped-2")),
]
);

////////////////////////////////////////////////////////////////////////////////////////

assert_eq!(de.next().unwrap(), Start(BytesStart::new("inner")));
assert_eq!(de.peek().unwrap(), &Start(BytesStart::new("skipped-3")));
assert_eq!(
de.read,
vec![
// This comment here to keep the same formatting of both arrays
// otherwise rustfmt suggest one-line it
Start(BytesStart::new("skipped-3")),
]
);
assert_eq!(
de.write,
vec![
Start(BytesStart::new("skipped-1")),
End(BytesEnd::new("skipped-1")),
Start(BytesStart::new("skipped-2")),
End(BytesEnd::new("skipped-2")),
]
);

// start_replay() should start replay from this point
let checkpoint2 = de.skip_checkpoint();
assert_eq!(checkpoint2, 4);

// Should skip third and forth <skipped-N/> elements
de.skip().unwrap(); // skipped-3
de.skip().unwrap(); // skipped-4
assert_eq!(de.read, vec![]);
assert_eq!(
de.write,
vec![
// checkpoint 1
Start(BytesStart::new("skipped-1")),
End(BytesEnd::new("skipped-1")),
Start(BytesStart::new("skipped-2")),
End(BytesEnd::new("skipped-2")),
// checkpoint 2
Start(BytesStart::new("skipped-3")),
End(BytesEnd::new("skipped-3")),
Start(BytesStart::new("skipped-4")),
End(BytesEnd::new("skipped-4")),
]
);
assert_eq!(de.next().unwrap(), Start(BytesStart::new("target-2")));
assert_eq!(de.next().unwrap(), End(BytesEnd::new("target-2")));
assert_eq!(de.peek().unwrap(), &End(BytesEnd::new("inner")));
assert_eq!(
de.read,
vec![
// This comment here to keep the same formatting of both arrays
// otherwise rustfmt suggest one-line it
End(BytesEnd::new("inner")),
]
);
assert_eq!(
de.write,
vec![
// checkpoint 1
Start(BytesStart::new("skipped-1")),
End(BytesEnd::new("skipped-1")),
Start(BytesStart::new("skipped-2")),
End(BytesEnd::new("skipped-2")),
// checkpoint 2
Start(BytesStart::new("skipped-3")),
End(BytesEnd::new("skipped-3")),
Start(BytesStart::new("skipped-4")),
End(BytesEnd::new("skipped-4")),
]
);

// Start replay events from checkpoint 2
de.start_replay(checkpoint2);
assert_eq!(
de.read,
vec![
Start(BytesStart::new("skipped-3")),
End(BytesEnd::new("skipped-3")),
Start(BytesStart::new("skipped-4")),
End(BytesEnd::new("skipped-4")),
End(BytesEnd::new("inner")),
]
);
assert_eq!(
de.write,
vec![
Start(BytesStart::new("skipped-1")),
End(BytesEnd::new("skipped-1")),
Start(BytesStart::new("skipped-2")),
End(BytesEnd::new("skipped-2")),
]
);

// Replayed events
assert_eq!(de.next().unwrap(), Start(BytesStart::new("skipped-3")));
assert_eq!(de.next().unwrap(), End(BytesEnd::new("skipped-3")));
assert_eq!(de.next().unwrap(), Start(BytesStart::new("skipped-4")));
assert_eq!(de.next().unwrap(), End(BytesEnd::new("skipped-4")));

assert_eq!(de.next().unwrap(), End(BytesEnd::new("inner")));
assert_eq!(de.read, vec![]);
assert_eq!(
de.write,
vec![
Start(BytesStart::new("skipped-1")),
End(BytesEnd::new("skipped-1")),
Start(BytesStart::new("skipped-2")),
End(BytesEnd::new("skipped-2")),
]
);

////////////////////////////////////////////////////////////////////////////////////////

// New events
assert_eq!(de.next().unwrap(), Start(BytesStart::new("target-1")));
assert_eq!(de.next().unwrap(), End(BytesEnd::new("target-1")));

assert_eq!(de.read, vec![]);
assert_eq!(
de.write,
vec![
Start(BytesStart::new("skipped-1")),
End(BytesEnd::new("skipped-1")),
Start(BytesStart::new("skipped-2")),
End(BytesEnd::new("skipped-2")),
]
);

// Start replay events from checkpoint 1
de.start_replay(checkpoint1);
assert_eq!(
de.read,
vec![
Start(BytesStart::new("skipped-1")),
End(BytesEnd::new("skipped-1")),
Start(BytesStart::new("skipped-2")),
End(BytesEnd::new("skipped-2")),
]
);
assert_eq!(de.write, vec![]);

// Replayed events
assert_eq!(de.next().unwrap(), Start(BytesStart::new("skipped-1")));
assert_eq!(de.next().unwrap(), End(BytesEnd::new("skipped-1")));
assert_eq!(de.next().unwrap(), Start(BytesStart::new("skipped-2")));
assert_eq!(de.next().unwrap(), End(BytesEnd::new("skipped-2")));

assert_eq!(de.read, vec![]);
assert_eq!(de.write, vec![]);

// New events
assert_eq!(de.next().unwrap(), End(BytesEnd::new("root")));
assert_eq!(de.next().unwrap(), Eof);
}

/// Checks that limiting buffer size works correctly
Expand Down
Loading