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

Mingun · 2022-06-05T16:51:41Z

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

…und byte arrays

codecov-commenter · 2022-06-05T16:59:46Z

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

tests/namespaces.rs

src/name.rs

src/events/mod.rs

src/name.rs

dralley · 2022-06-13T03:51:39Z

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 · 2022-06-13T11:44:46Z

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

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`

Mingun added 3 commits June 5, 2022 21:19

ns: Move pending_pop from NamespaceResolver to Reader

65c5aa5

This is more idiomatic because Reader is decide when to pop scope

ns: Move NamespaceResolver from module reader to module name

d58aa20

This is quite big part of library that needed its own tests, so it is better to it to live in a dedicated module

ns: Add QName, LocalName, Namespace, and Prefix typesafe wrappers aro…

7ad7c43

…und byte arrays

Mingun added bug serde Issues related to mapping from Rust types to XML namespaces Issues related to namespaces support labels Jun 5, 2022

Mingun requested a review from dralley June 5, 2022 16:51

dralley reviewed Jun 12, 2022

View reviewed changes

src/reader.rs Show resolved Hide resolved

dralley reviewed Jun 12, 2022

View reviewed changes

tests/namespaces.rs Outdated Show resolved Hide resolved

dralley reviewed Jun 12, 2022

View reviewed changes

src/name.rs Outdated Show resolved Hide resolved

dralley reviewed Jun 12, 2022

View reviewed changes

src/name.rs Outdated Show resolved Hide resolved