Fix too early handling of namespace declarations by Mingun · Pull Request #1002 · tafia/quick-xml

Mingun · 2026-08-13T20:23:33Z

I realized, that working with namespaces was entirely incorrect in serde Deserializer. We push and pop namespaces when we take events from the Reader, but we may buffer those events and check for xsi:nil later, when the state of NamespaceResolver was changed.

Now Deserializer holds the NamespaceResolver by itself and calls its methods only when event is consumed by Deserializer::next() call.

Fixes #953
Closes #974

@weiznich, it would be great if you can check this PR on your data. I'm sure that the fix currently is incomplete, so I need some time to find the cases that is not covered yet. You may help with that.

codecov-commenter · 2026-08-13T20:36:26Z

⚠️ Please install the to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

❌ Patch coverage is 79.38144% with 20 lines in your changes missing coverage. Please review.
✅ Project coverage is 56.13%. Comparing base (e00ae5c) to head (a821930).
⚠️ Report is 40 commits behind head on master.

Files with missing lines	Patch %	Lines
src/de/mod.rs	78.20%	17 Missing ⚠️
src/errors.rs	0.00%	3 Missing ⚠️
❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1002      +/-   ##
==========================================
- Coverage   57.31%   56.13%   -1.18%     
==========================================
  Files          46       47       +1     
  Lines       18197    18431     +234     
==========================================
- Hits        10429    10346      -83     
- Misses       7768     8085     +317

Flag	Coverage Δ
unittests	`56.13% <79.38%> (-1.18%)`	⬇️

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

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Mingun · 2026-08-15T15:23:17Z

Ok, it seems that now all possible paths are covered

Mingun · 2026-08-15T17:00:05Z

Wait a bit. Found another suspicious test

Helper will become more complicated in the next commits, so introduce it to keep only one place with such complexness

In next commits `last_peeked()` cannot be used due to starting violating borrow checker rules

…of namespace bindings failures (8): serde-de-xsi (6): as_field::nested_nil::field::nil_on_parent as_field::nested_nil::field::nil_on_self as_field::nested_nil::value::nil_on_parent as_field::nested_nil::value::nil_on_self top_level_option::nested_nil::string top_level_option::nested_nil::unit serde-issues (2): issue953::open_close issue953::self_closed

…it must be 0 failures (20, +14): serde-de (1): unit::excess_element serde-de-seq (10 - only for "overlapped-lists" feature): variable_name::fixed_size::field_after_list::overlapped_with_nested_list variable_name::fixed_size::field_before_list::overlapped_with_nested_list variable_name::fixed_size::two_lists::choice_and_fixed::overlapped::with_nested_list_fixed_after variable_name::fixed_size::two_lists::choice_and_fixed::overlapped::with_nested_list_fixed_before variable_name::fixed_size::two_lists::fixed_and_choice::overlapped::with_nested_list_fixed_after variable_name::fixed_size::two_lists::fixed_and_choice::overlapped::with_nested_list_fixed_before variable_name::variable_size::field_after_list::overlapped_with_nested_list variable_name::variable_size::field_before_list::overlapped_with_nested_list variable_name::variable_size::two_lists::choice_and_fixed::overlapped::with_nested_list_fixed_after variable_name::variable_size::two_lists::fixed_and_choice::overlapped::with_nested_list_fixed_after serde-de-xsi (9, +3): as_field::nested_nil::field::nil_on_parent as_field::nested_nil::field::nil_on_self as_field::nested_nil::value::nil_on_parent as_field::nested_nil::value::nil_on_self as_field::with_element::nested::true_ top_level_option::nested_nil::string top_level_option::nested_nil::unit top_level_option::with_element::ns0::true_ top_level_option::with_element::xsi::true_

Mingun · 2026-08-15T18:20:40Z

Ok, I believe now everything was catched and analysed.

dralley · 2026-08-16T01:55:14Z

+        self.read.pop_front()
+    }
+
+    #[cfg(not(feature = "overlapped-lists"))]


Not immediately actionable, but Rust 1.95 has cfg_select - would be nice to use eventually.

dralley · 2026-08-16T02:14:19Z

        V: Visitor<'de>,
    {
-        let has_nil = self.map.de.reader.reader.has_nil_attr(&self.map.start);
+        // `self.map.start` already was taken from the reader, so its namespace bindings already processed.


"its namespace bindings were already processed"

dralley · 2026-08-16T02:25:28Z

  as `&#13;`, `&#10;`, and `&#9;` respectively, preventing silent data loss from
  XML attribute-value normalization on round-trip. Likewise `Attribute::from`
  performs the same transformation.
+- [#953]: The serde `Deserializer` now correctly handle namespaces. Previously


Deserializer now correctly handles namespaces.

dralley · 2026-08-16T02:26:01Z

  performs the same transformation.
+- [#953]: The serde `Deserializer` now correctly handle namespaces. Previously
+  the namespace bindings might be applied or removed before the event actually
+  was consumed which lead to couple of bugs.


to a couple

dralley · 2026-08-16T02:29:33Z

+    /// Runs action as if all namespaces from the specified `start` element was added
+    /// to the resolver. Actually, resolver state remains unchanged after this call,
+    /// but inside the action resolver have all namespaces from the `start`.
+    pub fn with<F, R>(&mut self, start: &BytesStart, mut action: F) -> Result<R, NamespaceError>


Runs action as if all namespaces from the specified start element were added to the resolver, but without actually changing the resolver state.

dralley · 2026-08-16T02:47:39Z

  `normalized_value_with()` instead.
+- [#1002]: Added `NamespaceResolver::with` that allows temporary apply namespace
+  bindings from the start tag and run query on the resolver. It is useful to check
+  peeked event which is not yet consumed.


[Fix too early handling of namespace declarations #1002]: Added NamespaceResolver::with that allows temporarily applying namespace
bindings from the start tag for the scope of a provided closure F, without making any
persistent change to the resolver.

"It is useful to check a peeked event which is not yet consumed." also sounds like an internal detail - is a user going to do that?

Because NamespaceResolver is public and intended to use with own implementations of a peekable reader, this is a note for such users.

dralley · 2026-08-16T02:50:14Z

+  peeked event which is not yet consumed.
+- [#1002]: Added `Deserializer::resolver` and `Deserializer::resolver_mut` methods
+  to get a namespace resolver used by this deserializer, because its internally no
+  longer uses the `NsReader`.


[Fix too early handling of namespace declarations #1002]: Added Deserializer::resolver and Deserializer::resolver_mut methods
to get a namespace resolver used by this deserializer, because it no longer uses
an NsReader internally.

dralley · 2026-08-16T03:08:58Z

Code looks reasonable so far, still reviewing a bit, but will wait until @weiznich gives the 👍 with his dataset to finish.

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

weiznich

I've verified that this works with our dataset. Also the code looks fine to me.

Thanks for fixing this ❤️

Mingun added bug serde Issues related to mapping from Rust types to XML namespaces Issues related to namespaces support labels Aug 13, 2026

Mingun force-pushed the issue953 branch from 425c4c7 to e69e7cd Compare August 14, 2026 21:25

Unify implementation of read_to_end across overlapped-lists feature

c71c9c2

Mingun force-pushed the issue953 branch from e69e7cd to 9fc148f Compare August 15, 2026 15:04

Mingun marked this pull request as ready for review August 15, 2026 15:21

Mingun requested a review from dralley August 15, 2026 15:23

Mingun force-pushed the issue953 branch from 9fc148f to 7f327c4 Compare August 15, 2026 16:46

Mingun marked this pull request as draft August 15, 2026 16:59

Mingun added 5 commits August 15, 2026 22:23

Rewrite deserialize_option using new helper function

02e6033

Helper will become more complicated in the next commits, so introduce it to keep only one place with such complexness

Get rid of last_peeked(), inline peek()

3711cb2

In next commits `last_peeked()` cannot be used due to starting violating borrow checker rules

Inline skip_next_tree - now it is used only in one place

ce37aca

Mingun force-pushed the issue953 branch 2 times, most recently from d0253d3 to 0cdd7ac Compare August 15, 2026 17:53

Mingun marked this pull request as ready for review August 15, 2026 18:19

dralley reviewed Aug 16, 2026

View reviewed changes

Handle namespaces only when events actually consumed in Deserializer

a821930

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

Mingun force-pushed the issue953 branch from 0cdd7ac to a821930 Compare August 16, 2026 11:32

weiznich approved these changes Aug 17, 2026

View reviewed changes

Conversation

Mingun commented Aug 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov-commenter commented Aug 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Mingun commented Aug 15, 2026

Uh oh!

Mingun commented Aug 15, 2026

Uh oh!

Mingun commented Aug 15, 2026

Uh oh!

dralley Aug 16, 2026

Choose a reason for hiding this comment

Uh oh!

dralley Aug 16, 2026

Choose a reason for hiding this comment

Uh oh!

dralley Aug 16, 2026

Choose a reason for hiding this comment

Uh oh!

dralley Aug 16, 2026

Choose a reason for hiding this comment

Uh oh!

dralley Aug 16, 2026

Choose a reason for hiding this comment

Uh oh!

dralley Aug 16, 2026

Choose a reason for hiding this comment

Uh oh!

Mingun Aug 16, 2026

Choose a reason for hiding this comment

Uh oh!

dralley Aug 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dralley commented Aug 16, 2026

Uh oh!

weiznich left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Mingun commented Aug 13, 2026 •

edited

Loading

codecov-commenter commented Aug 13, 2026 •

edited

Loading

dralley Aug 16, 2026 •

edited

Loading