Properly use `xsi:nil` to deserialize null values via serde by weiznich · Pull Request #842 · tafia/quick-xml

weiznich · 2025-01-27T10:14:37Z

This commit fixes an issue that causes quick-xml trying to deserialize empty tags via the serde interface even if these tags were explicitly marked as xsi:nil="true"

For example the following XML failed to deserialize before this commit:

<bar>
    <foo xsi:nil="true"/>
</bar>

into the following rust type:

#[derive(Deserialize)]
struct Bar {
    foo: Option<Inner>,
}

#[derive(Deserialize)]
struct Foo {
    baz: String,
}

Before this commit this failed to deserialize with an error message that complained that the baz field was missing. After this commit this uses the xsi:nil attribute to deserialize this into foo: None instead. The standard (https://www.w3.org/TR/xmlschema-1/#xsi_nil) seems to support this behaviour.

Fix #497

codecov-commenter · 2025-01-27T10:37:32Z

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

Codecov Report

Attention: Patch coverage is 77.63158% with 17 lines in your changes missing coverage. Please review.

Project coverage is 60.73%. Comparing base (a9391f3) to head (ad89a00).
Report is 26 commits behind head on master.

Files with missing lines	Patch %	Lines
src/de/mod.rs	72.54%	14 Missing ⚠️
src/events/mod.rs	82.35%	3 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #842      +/-   ##
==========================================
+ Coverage   60.21%   60.73%   +0.51%     
==========================================
  Files          41       41              
  Lines       16021    16013       -8     
==========================================
+ Hits         9647     9725      +78     
+ Misses       6374     6288      -86

Flag	Coverage Δ
unittests	`60.73% <77.63%> (+0.51%)`	⬆️

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

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

Mingun

Good work, and it's nice to see such a person in committers. I think, that we should try to improve it to cover all possible scenarios, but if that would be too many work (because maybe we need to switch to namespace-aware parser), the current implementation could be merged as the first step.

In any case, we need a changelog entry, and in case of using only xsi prefix this should be explicitly noted in the documentation somewhere here.

If you're not prepared to dive so deep to explore the things required, I can finish this PR myself, but that can take some time.

Mingun · 2025-01-27T10:43:53Z

src/events/attributes.rs

+
+    pub(crate) fn is_xsi_nil(&self) -> bool {
+        self.clone().any(|attr| {
+            matches!(attr, Ok(crate::events::attributes::Attribute{
+                        key: QName(b"xsi:nil"), value
+                    }) if &*value == b"true")
+        })
+    }


While the xsi prefix usually bound to http://www.w3.org/2001/XMLSchema-instance namespace, this is not necessary true. I would like to resolve actual namespace and check against it.

Also, need to check is the true the only thing that represents truth value.

I think, it would be better to place such method to the BytesStart, because then we can avoid cloning and the name would better fit its purpose (currently it actually checks has_xsi_nil).

While the xsi prefix usually bound to http://www.w3.org/2001/XMLSchema-instance namespace, this is not necessary true. I would like to resolve actual namespace and check against it.

I will try to have a look at that, if the idea I have works it shouldn't be too hard. Just to verify that I have the right understanding how this namespace thing works:

I need to collect all xmlns:xyz attributes from any parent tag

I then need to check if one or more of these attributes is set to http://www.w3.org/2001/XMLSchema-instance

That then will me give the namespace name, so xyz in the above examle

Finally I can use that name to check if an nil attribute exists and then use that in the check

Also, need to check is the true the only thing that represents truth value.

Unless I miss something this already checks for the true value via a guard expression (line 242)

I think, it would be better to place such method to the BytesStart, because then we can avoid cloning and the name would better fit its purpose (currently it actually checks has_xsi_nil).

I'm not sure I can follow here. Attributes just borrows the buffer from BytesStart so no cloning should be happening here.

I need to collect all xmlns:xyz attributes from any parent tag

Basically we need to use NsReader instead of a Reader in Deserializer, so here:

quick-xml/src/de/mod.rs

Lines 3080 to 3084 in 8f91a9c

pub struct IoReader<R: BufRead> {

reader: Reader<R>,

start_trimmer: StartTrimmer,

buf: Vec<u8>,

}

...and here:

quick-xml/src/de/mod.rs

Lines 3149 to 3152 in 8f91a9c

pub struct SliceReader<'de> {

reader: Reader<&'de [u8]>,

start_trimmer: StartTrimmer,

}

Then make any necessary changes to make code compilable and then you may use resolve methods to resolve prefix. And it seems to me that that refactoring may be not so trivial.

Unless I miss something this already checks for the true value via a guard expression (line 242)

I mean, maybe not only the string "true" represents the true value. For example, XML Schema defines "true" and "1" as true and "false" and "0" as false. It seems that only "true" should be used, as stated here, but I did not check all occurrences of the nil word in the specification. The XML spec is a hard thing to read, so we must be carefull. I would like also have link to the specification (maybe the mentioned link is the right link) near to the check so everyone could verify that check is correct without much efforts.

I'm not sure I can follow here. Attributes just borrows the buffer from BytesStart so no cloning should be happening here.

Yes, I think, actually no cloning is made here but that not obvious from the code. You need to look at the Attributes definition to be sure. As this method used only in start.attributes().is_xsi_nil() chain, it makes sence to move it to the BytesStart for clarity and call it as start.is_xsi_nil().

Then make any necessary changes to make code compilable and then you may use resolve methods to resolve prefix. And it seems to me that that refactoring may be not so trivial.

Maybe I miss something obvious but that wasn't complicated. In fact it literally required changing these two types + adjusting my new added code to get access to the resolve methods.

I mean, maybe not only the string "true" represents the true value. For example, XML Schema defines "true" and "1" as true and "false" and "0" as false. It seems that only "true" should be used, as stated here, but I did not check all occurrences of the nil word in the specification. The XML spec is a hard thing to read, so we must be carefull. I would like also have link to the specification (maybe the mentioned link is the right link) near to the check so everyone could verify that check is correct without much efforts.

Done

Yes, I think, actually no cloning is made here but that not obvious from the code. You need to look at the Attributes definition to be sure. As this method used only in start.attributes().is_xsi_nil() chain, it makes sence to move it to the BytesStart for clarity and call it as start.is_xsi_nil().

Done

Mingun · 2025-01-27T10:49:43Z

src/de/mod.rs

    }
+
+    #[test]
+    fn deserialize_nil() {


I prefer to group tests using modules. This have some advantages, for example, code folding in the editor. In that case it seems reasonable to split this test to several in the nil module:

mod nil { // <foo xsi:nil="true"/> // <foo xsi:nil="true" attr="value"/> // <foo xsi:nil="true"><tag/></foo> fn true_() {} // <foo xsi:nil="false"/> // <foo xsi:nil="false" attr="value"/> // <foo xsi:nil="false"><tag/></foo> fn false_() {} } // and the same for the case of nested element with nil

Those tests are better to place to the serde-de tests to reduce overall time of compiling library (which already not so good), because they do not use internal details.

Moved the tests + added some more variants. What's still not tested is using a different namespace prefix, but I can also add tests for that if you want.

Mingun · 2025-01-27T10:51:11Z

src/de/mod.rs

+        let data = r#"<foo xsi:nil="true"/>"#;
+
+        let res = super::from_str::<Option<Foo>>(data).unwrap();
+        assert!(res.is_none());


I prefer to use assert_eq checks where possible because it will output diff in case of failed check (project uses pretty_assertions crate for diffs).

This commit fixes an issue that causes `quick-xml` trying to deserialize empty tags via the serde interface even if these tags were explicitly marked as `xsi:nil="true"` For example the following XML failed to deserialize before this commit: ```xml <bar> <foo xsi:nil="true"/> </bar> ``` into the following rust type: ```rust #[derive(Deserialize)] struct Bar { foo: Option<Inner>, } #[derive(Deserialize)] struct Foo { baz: String, } ``` Before this commit this failed to deserialize with an error message that complained that the `baz` field was missing. After this commit this uses the `xsi:nil` attribute to deserialize this into `foo: None` instead. The standard (https://www.w3.org/TR/xmlschema-1/#xsi_nil) seems to support this behaviour. Fix tafia#497

Mingun · 2025-02-02T13:14:41Z

Just want to inform you, that this PR is not abandoned, I working on improving testcases to ensure that everything is consistent.

Mingun · 2025-02-09T11:08:50Z

tests/serde-de.rs

+    #[derive(Debug, PartialEq, serde::Deserialize)]
+    struct Helper {
+        #[serde(rename = "@attr")]
+        attr: String,
+        #[serde(rename = "$value")]
+        inner: Option<Foo>,
+    }


Unfortunately, this is incorrect representation for <foo attr="..." xsi:nil="..."/>. Currently, you even cannot use $value name for annotating fields with non-enum type, the serialization of such fields would fail. So currently I searching a correct way to represent such XML by the Rust type.

quick-xml/src/se/content.rs

Lines 351 to 360 in 8f91a9c

#[inline]

fn serialize_struct(

self,

name: &'static str,

_len: usize,

) -> Result<Self::SerializeStruct, Self::Error> {

Err(SeError::Unsupported(

format!("serialization of struct `{name}` is not supported in `$value` field").into(),

))

}

Mingun · 2025-02-09T12:09:12Z

I think, that the correct handling of xsi:nil="true" is to set to None of every Option field of the struct on which xsi:nil="true" is encountered.

<!-- XML 1 -->
<any-tag xsi:nil="true" attr="value">
  <element1>value 1</element1>
  <element2>value 2</element2>
</any-tag>

<!-- XML 2 -->
<any-tag xsi:nil="false" attr="value">
  <element1>value 1</element1>
  <element2>value 2</element2>
  <element3>value 3</element3>
</any-tag>

should be mapped to

#[derive(Deserialize)]
struct AnyName {
  #[serde(rename = "@attr")]
  attr: String,

  element1: String,
  element2: Option<String>,
  element3: Option<String>,
  element4: Option<String>,
}

// XML 1
let xml1 = AnyName {
  // xsi:nil does not influence to attributes, only to nested elements
  attr: "value",

  // Non-Option fields deserialized as usual
  element1: "value 1",
  // Note: even while element is presented in XML,
  // it is not deserialized because of xsi:nil="true".
  // Without it it would be `Some("value 2")`
  element2: None,
  element3: None,
  element4: None,
};

// XML 2
let xml2 = AnyName {
  attr: "value",

  element1: "value 1",
  element2: Some("value 2"),
  element3: Some("value 3"),
  element4: None,
};

Mingun · 2025-02-09T12:23:33Z

@weiznich, I plan to finish this PR myself, because I already done some prepare work of writing tests thats cover all possible (I hope) variants. So nothing from you is required here, but of course, you can give feedback, is that solution would fit your needs?

I hope I can finish PR in next week.

weiznich · 2025-02-11T11:05:18Z

I'm happy with whatever works best for you. If you want to address things on your own that's fine for me, it's also fine to spend a bit more work on my side to implement this as long as I known what to do and what are the expectations.

Mingun · 2025-03-21T16:29:40Z

Because I cannot push my changes here, I've opened #850

Mingun added the serde Issues related to mapping from Rust types to XML label Jan 27, 2025

Mingun requested changes Jan 27, 2025

View reviewed changes

weiznich force-pushed the fix/honor_xsi_nil branch from 8df645b to fa3df2a Compare January 28, 2025 16:07

weiznich force-pushed the fix/honor_xsi_nil branch from fa3df2a to ad89a00 Compare January 29, 2025 06:31

Mingun reviewed Feb 9, 2025

View reviewed changes

Mingun mentioned this pull request Mar 21, 2025

Properly use xsi:nil to deserialize null values via serde #850

Merged

Mingun closed this Mar 21, 2025

	pub struct IoReader<R: BufRead> {
	reader: Reader<R>,
	start_trimmer: StartTrimmer,
	buf: Vec<u8>,
	}

	pub struct SliceReader<'de> {
	reader: Reader<&'de [u8]>,
	start_trimmer: StartTrimmer,
	}

	#[inline]
	fn serialize_struct(
	self,
	name: &'static str,
	_len: usize,
	) -> Result<Self::SerializeStruct, Self::Error> {
	Err(SeError::Unsupported(
	format!("serialization of struct `{name}` is not supported in `$value` field").into(),
	))
	}

Conversation

weiznich commented Jan 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov-commenter commented Jan 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Mingun left a comment

Choose a reason for hiding this comment

Uh oh!

Mingun Jan 27, 2025

Choose a reason for hiding this comment

Uh oh!

weiznich Jan 27, 2025

Choose a reason for hiding this comment

Uh oh!

Mingun Jan 27, 2025 • 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!

weiznich Jan 28, 2025

Choose a reason for hiding this comment

Uh oh!

Mingun Jan 27, 2025

Choose a reason for hiding this comment

Uh oh!

weiznich Jan 28, 2025

Choose a reason for hiding this comment

Uh oh!

Mingun Jan 27, 2025

Choose a reason for hiding this comment

Uh oh!

weiznich Jan 28, 2025

Choose a reason for hiding this comment

Uh oh!

Mingun commented Feb 2, 2025

Uh oh!

Mingun Feb 9, 2025 • 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!

Mingun commented Feb 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Mingun commented Feb 9, 2025

Uh oh!

weiznich commented Feb 11, 2025

Uh oh!

Mingun commented Mar 21, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

weiznich commented Jan 27, 2025 •

edited

Loading

codecov-commenter commented Jan 27, 2025 •

edited

Loading

Mingun Jan 27, 2025 •

edited

Loading

Mingun Feb 9, 2025 •

edited

Loading

Mingun commented Feb 9, 2025 •

edited

Loading