Skip to content
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.

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

Split out namespace aware reader to a struct that manage the internal namespaces buffer #437

Merged
merged 11 commits into from
Jul 24, 2022

Conversation

Mingun
Copy link
Collaborator

@Mingun Mingun commented Jul 23, 2022

This is PR for low-level namespaces support in the quick-xml, on which high-level serde support will be implemented.

The goals of this PR is to make a safe interface and leave the option of not paying for namespace support if it is not needed. For that the buffer with readed namespaces now managed by the dedicated NsReader reader, which should be used instead of Reader when you need namespaces. Because now buffer is stored inside the reader, the user cannot accidently clear it and get a panic or wrong results later, when it would try to resolve namespace. In addition, it is impossible now to mix read methods with and without namespace awareness, which could lead to hard-to-catch bugs.

Originally, I've planned to include the serde part into this PR, but number of commits for support serde grows and I've decided to made a dedicated PR for that. At least, the serde part slightly depends on #434, because those changes will break some removed tests.


I would like to merge that before #425, because:

This commit only moves code without significant changes (the only changes is:
- corrected imports
- add imports to the doc comments which have become inaccessible
)
The following commit will not allow to call `reader.decoder()` while
reference to ResolveResult is valid, so we'll need to save decoder
before the loop and update it when it could be changed
@Mingun Mingun added enhancement namespaces Issues related to namespaces support labels Jul 23, 2022
@Mingun Mingun requested a review from dralley July 23, 2022 20:24
@codecov-commenter
Copy link

codecov-commenter commented Jul 23, 2022

Codecov Report

Merging #437 (45be217) into master (ebbcce0) will increase coverage by 1.99%.
The diff coverage is 72.72%.

@@            Coverage Diff             @@
##           master     #437      +/-   ##
==========================================
+ Coverage   49.51%   51.50%   +1.99%     
==========================================
  Files          22       25       +3     
  Lines       13847    13310     -537     
==========================================
- Hits         6856     6855       -1     
+ Misses       6991     6455     -536     
Flag Coverage Δ
unittests 51.50% <72.72%> (+1.99%) ⬆️

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

Impacted Files Coverage Δ
benches/microbenches.rs 0.00% <0.00%> (ø)
src/lib.rs 12.26% <0.00%> (ø)
src/name.rs 88.37% <ø> (ø)
src/reader/ns_reader.rs 60.37% <60.37%> (ø)
src/reader/buffered_reader.rs 65.06% <65.06%> (ø)
src/reader/mod.rs 90.42% <84.00%> (ø)
src/reader/slice_reader.rs 100.00% <100.00%> (ø)
src/de/mod.rs 77.77% <0.00%> (-0.46%) ⬇️
src/de/map.rs 72.55% <0.00%> (+0.46%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ebbcce0...45be217. Read the comment docs.

src/reader/mod.rs Outdated Show resolved Hide resolved
src/reader/ns_reader.rs Outdated Show resolved Hide resolved
src/reader/ns_reader.rs Outdated Show resolved Hide resolved
/// [`End`]: Event::End
/// [`read_event()`]: Self::read_event
#[inline]
pub fn read_event_ns(&mut self) -> Result<(ResolveResult, Event<'i>)> {
Copy link
Collaborator

@dralley dralley Jul 24, 2022

Choose a reason for hiding this comment

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

I'm not sure this naming is an improvment, it feels too brief. Perhaps read_event_with_ns() or read_event_and_ns() would be better.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I agree, not a better name. May be read_resolved_event?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Changed to read_resolved_event / read_resolved_event_into

src/reader/mod.rs Outdated Show resolved Hide resolved
Mingun and others added 7 commits July 24, 2022 17:39
… to it

This commit only moves code and do not introduce any improvements
This commit only moves code
`Deserializer` implementation can buffer events and it will not be able
to buffer the `resolve` result because it is bound to the `NsReader`
Co-authored-by: Daniel Alley <dalley@redhat.com>
Copy link
Collaborator

@dralley dralley left a comment

Choose a reason for hiding this comment

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

I think the PR still needs to be pushed, changes aren't live yet. read_resolved_event() still doesn't feel like the best name - but I have no suggestions that feel clearly better.

…ll non-namespace-aware mutation methods

Because `impl DerefMut<Target=Reader> for NsReader` is removed, all Reader's
methods that accept `&mut self` won't be available for the NsReader, including
configuration methods.

Because we need a way to configure our reader, we need to reimplement necessary
methods that accepts `&mut self`. In order to not duplicate the code and the doc
of this configure methods, they was put into the macro `configure_methods!` and
embed to both readers.
@@ -77,6 +77,19 @@ macro_rules! configure_methods {

/// Changes whether mismatched closing tag names should be detected.
///
/// Note, that start and end tags [should match literally][spec], they cannot
/// have different prefixes even if both prefixes resolves to the same namespace.
/// The
Copy link
Collaborator

@dralley dralley Jul 24, 2022

Choose a reason for hiding this comment

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

This should be either

The XML

or

This XML:

or

This:

Copy link
Collaborator

Choose a reason for hiding this comment

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

Also "both prefixes resolves" -> "both prefixes resolve"

Copy link
Collaborator Author

@Mingun Mingun Jul 24, 2022

Choose a reason for hiding this comment

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

Done ("The XML") and done!

Co-authored-by: Daniel Alley <dalley@redhat.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement namespaces Issues related to namespaces support
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants