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

Introduce typified wrappers for names and fix couple of bugs #393

Merged
merged 12 commits into from
Jun 13, 2022

Conversation

Mingun
Copy link
Collaborator

@Mingun Mingun commented Jun 5, 2022

Fixed bugs:

  • not declared prefix in the tags resulted in a namespace resolution, as if the tag were not bound to any namespace:
    <unknown:element/>
    was considered as
    <element/>
  • elements/attributes with reserved names (started with "xml"i) erroneously was treated as error, although XML spec explicitly requires that fatal error MUST NOT be raised. Because quick-xml does not check names validity, now such names does not produce any error.

To fix this kind of errors and avoid messing up different names, a typified wrappers for names was introduced.

This PR is the first step to properly support namespaces in quick-xml.

This is more idiomatic because Reader is decide when to pop scope
This is quite big part of library that needed its own tests,
so it is better to it to live in a dedicated module
@Mingun Mingun added bug serde Issues related to mapping from Rust types to XML namespaces Issues related to namespaces support labels Jun 5, 2022
@Mingun Mingun requested a review from dralley June 5, 2022 16:51
@codecov-commenter
Copy link

Codecov Report

Merging #393 (ad8a9f3) into master (0a42987) will increase coverage by 0.62%.
The diff coverage is 84.52%.

@@            Coverage Diff             @@
##           master     #393      +/-   ##
==========================================
+ Coverage   60.79%   61.41%   +0.62%     
==========================================
  Files          19       20       +1     
  Lines        9843    10160     +317     
==========================================
+ Hits         5984     6240     +256     
- Misses       3859     3920      +61     
Flag Coverage Δ
unittests 61.41% <84.52%> (+0.62%) ⬆️

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

Impacted Files Coverage Δ
examples/custom_entities.rs 0.00% <0.00%> (ø)
examples/issue68.rs 0.00% <0.00%> (ø)
examples/nested_readers.rs 0.00% <0.00%> (ø)
examples/read_texts.rs 0.00% <0.00%> (ø)
src/errors.rs 12.37% <0.00%> (-0.54%) ⬇️
src/writer.rs 90.33% <ø> (ø)
src/lib.rs 25.96% <25.00%> (ø)
src/de/map.rs 88.07% <66.66%> (-0.67%) ⬇️
src/de/mod.rs 75.45% <66.66%> (-0.63%) ⬇️
src/reader.rs 87.60% <74.28%> (-0.62%) ⬇️
... and 8 more

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 0a42987...ad8a9f3. Read the comment docs.

src/reader.rs Show resolved Hide resolved
tests/namespaces.rs Outdated Show resolved Hide resolved
src/name.rs Outdated Show resolved Hide resolved
src/name.rs Outdated Show resolved Hide resolved
src/name.rs Outdated Show resolved Hide resolved
src/name.rs Outdated Show resolved Hide resolved
src/name.rs Outdated Show resolved Hide resolved
src/name.rs Outdated Show resolved Hide resolved
src/events/mod.rs Outdated Show resolved Hide resolved
src/events/mod.rs Outdated Show resolved Hide resolved
src/events/mod.rs Outdated Show resolved Hide resolved
src/name.rs Outdated Show resolved Hide resolved
@dralley
Copy link
Collaborator

dralley commented Jun 13, 2022

Code looks fine, review finished. One remaining general question:

Could the namespace buffer be made an internal part of NamespaceResolver? Unlike the event buffer that the user provides, the namespace buffer actually carries around important data between events, essentially for the whole lifetime of the reader, and NamespaceResolver also carries a lot of internal details about the buffer such as indexes (via NamespaceEntry).

It's also normal to clear the event buffer, and a user might think they ought to do the same with the namespace buffer, but that would probably not end well for them. There's some obvious footguns and not a lot of obvious benefits (unless it's necessary to make the lifetimes work, or something).

@Mingun
Copy link
Collaborator Author

Mingun commented Jun 13, 2022

Yes, I totally agree, that namespace buffer should lay in the namespace resolver. I plan to implement that in the next PR

Mingun and others added 8 commits June 13, 2022 17:28
Co-authored-by: Daniel Alley <dalley@redhat.com>
Add `ResolveResult` with results of namespace resolution and replace
two-state `Option<Namespace>` with three-state `ResolveResult`.

Since now in that cases `ResolveResult::Unknown` is returned, which
can be converted to an `Error::UnknownPrefix` using `TryInto<Option<Namespace>>`
failures (1):
    reserved_name

Co-authored-by: Daniel Alley <dalley@redhat.com>
…me test

Co-authored-by: Daniel Alley <dalley@redhat.com>
and `BytesStart::local_name()` and `BytesEnd::local_name()` to return `LocalName`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug namespaces Issues related to namespaces support serde Issues related to mapping from Rust types to XML
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants