Fix serialization of structs with only text content as attributes

[Kakadus]

close #906

allows structs that contain only one $text field to be serialized as an attribute list.

As this is my first contribution, I may have overlooked something. The tests seem to pass though :)

[codecov-commenter]

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

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 58.34%. Comparing base (d1acdb5) to head (d18409b).
⚠️ Report is 5 commits behind head on master.
❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #907      +/-   ##
==========================================
+ Coverage   58.08%   58.34%   +0.25%     
==========================================
  Files          42       42              
  Lines       15513    15514       +1     
==========================================
+ Hits         9011     9051      +40     
+ Misses       6502     6463      -39     

Flag	Coverage Δ
unittests	58.34% <100.00%> (+0.25%)	⬆️

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.

🚀 New features to boost your workflow:

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

[Mingun]

I think, that makes sense.

[Mingun]

When polishing this PR I found, that such implementation creates inconsistency: you cannot parse struct back from the attribute value. That may be solved if consider _fields here:

quick-xml/src/de/text.rs

Lines 124 to 138 in 655691c

	#[inline]
	fn deserialize_struct<V>(
	self,
	_name: &'static str,
	_fields: &'static [&'static str],
	visitor: V,
	) -> Result<V::Value, Self::Error>
	where
	V: Visitor<'de>,
	{
	// Deserializer methods are only hints, if deserializer could not satisfy
	// request, it should return the data that it has. It is responsibility
	// of a Visitor to return an error if it does not understand the data
	self.deserialize_str(visitor)
	}

but I prefer not to do that. As you known, some serde attributes may change struct representation to map representation and therefore we will lose our hack here, because maps does not report their keys.

It seems, that in that case it is better to use #[serde(with)] and convert you struct into/from string manually. What do you think?

Below the adapted test which failed even with this PR:

/// Regression test for https://github.com/tafia/quick-xml/issues/906.
#[test]
fn issue906() {
    #[derive(Debug, PartialEq, Deserialize, Serialize)]
    struct AsElement {
        #[serde(rename = "a-list")]
        a_list: TextContent,
    }

    #[derive(Debug, PartialEq, Deserialize, Serialize)]
    struct AsAttribute {
        #[serde(rename = "@a-list")]
        a_list: TextContent,
    }

    #[derive(Debug, PartialEq, Deserialize, Serialize)]
    struct TextContent {
        #[serde(rename = "$text")]
        content: Vec<String>,
        #[serde(skip)]
        ignored: (),
    }

    let foo = AsElement {
        a_list: TextContent {
            content: vec!["A".to_string(), "B".to_string()],
            ignored: (),
        },
    };
    let bar = AsAttribute {
        a_list: TextContent {
            content: vec!["A".to_string(), "B".to_string()],
            ignored: (),
        },
    };

    let buffer = to_string_with_root("test", &foo).unwrap();
    assert_eq!(buffer, "<test><a-list>A B</a-list></test>");
    let foo2: AsElement = from_str(&buffer).unwrap();
    assert_eq!(foo2, foo);

    let buffer = to_string_with_root("test", &bar).unwrap();
    assert_eq!(buffer, "<test a-list=\"A B\"/>");

    // thread 'issue906' panicked at tests\serde-issues.rs:756:47:
    // called `Result::unwrap()` on an `Err` value: Custom("invalid type: string \"A B\", expected struct TextContent")
    let bar2: AsAttribute = from_str(&buffer).unwrap();
    assert_eq!(bar2, bar);
}

For element there is no problem, because TextContent are deserialized from <a-list>A B</a-list> and not just from "A B".

[Kakadus]

Thanks for taking a look! It seems that I simply forgot about deserialization :)

Writing custom (de)serializers always work, although I'd really like to see first party support for all usages of the text structs. Each struct would need their own implementation, and you'd have to track their usage and apply the with appropriately. Coming from code generators like https://docs.rs/xsd-parser/latest/xsd_parser/, this makes it very complicated IMHO. What do you think about the SimpleTypeDeserializer extension I commited instead?

As you known, some serde attributes may change struct representation to map representation and therefore we will lose our hack here, because maps does not report their keys.

Do you mean e.g. #[serde(flatten)]? A short experiment shows that deserialization of AsElement fails already (which is not affected by this PR):

        /// Regression test for https://github.com/tafia/quick-xml/issues/906.
        #[test]
        fn issue906() {
            #[derive(Debug, PartialEq, Deserialize, Serialize)]
            struct AsElement {
                #[serde(rename = "a-list")]
                a_list: TextContent,
            }

            #[derive(Debug, PartialEq, Deserialize, Serialize)]
            pub struct TextContent {
                #[serde(flatten)]
                content: Inner,
            }

            #[derive(Debug, PartialEq, Deserialize, Serialize)]
            pub struct Inner {
                #[serde(default, rename = "$text")]
                content: Vec<String>,
            }

            let foo = AsElement {
                a_list: TextContent {
                    content: Inner {
                        content: vec!["A".to_string(), "B".to_string()],
                    },
                },
            };

            let buffer = to_string_with_root("test", &foo).unwrap();
            assert_eq!(buffer, "<test><a-list>A B</a-list></test>");
            // thread 'with_root::list::issue906' panicked at tests/serde-se.rs:2347:53:
            // called `Result::unwrap()` on an `Err` value: Custom("invalid type: string \"A B\", expected a sequence")
            let foo2: AsElement = from_str(&buffer).unwrap();
            assert_eq!(foo2, foo);
        }

[Kakadus]

build error with old serde version should be fixed now

I'm afraid the current approach requires a bump of serde to have your IntoDeserializer for SeqAccessDeserializer from serde 1.0.214...

[Kakadus]

@Mingun implementing IntoDeserializer for ListIter directly allows this to compile with serde<1.0.214

[Mingun]

I squashed commits and slightly rewrite implementation, but I'm still unsure if this should be merged. My main concern that this introduces some "magic" behavior which does not smoothly combined with other features. I prefer to use something more explicit in such cases, so that the reader does not have unreasonable hopes.

Before that PR everything is clear: you cannot (de)serialize structs to/from strings, because it is unclear how you should represent keys and values. This PR introduces special behavior for some structs, reusing another special behavior for $text fields. The $text fields was introduced for very clear purpose: if you have element with mixed text/nested elements, you somehow should understand, where to store textual value, keeping the Map interface. In contradiction, attribute values are always strings, so if you want to store some struct in it, you should manually define (de)serialization rules. We don't require you to name the fields $text because it doesn't make sense for the attributes. So it is unclear, why we should specially process them.