seq: Allow to have an ordinary elements together with a $value field

Example of a struct, that should be a valid definition:

    enum Choice { one, two, three }
    struct Root {
        #[serde(rename = "$value")]
        item: [Choice; 3],
        node: (),
    }

and this struct should be able to deserialized from

    <root>
        <one/>
        <two/>
        <three/>
        <node/>
    </root>

The following tests were fixed (10):
    seq::variable_name::fixed_size::field_after_list::after
    seq::variable_name::fixed_size::field_after_list::before
    seq::variable_name::fixed_size::field_before_list::after
    seq::variable_name::fixed_size::field_before_list::before
    seq::variable_name::fixed_size::two_lists::choice_and_fixed::fixed_after
    seq::variable_name::fixed_size::two_lists::choice_and_fixed::fixed_before
    seq::variable_name::fixed_size::two_lists::fixed_and_choice::fixed_after
    seq::variable_name::fixed_size::two_lists::fixed_and_choice::fixed_before
    seq::variable_name::variable_size::field_after_list::before
    seq::variable_name::variable_size::field_before_list::before

failures (26):
    seq::fixed_name::fixed_size::field_after_list::overlapped
    seq::fixed_name::fixed_size::field_before_list::overlapped
    seq::fixed_name::fixed_size::two_lists::overlapped
    seq::fixed_name::fixed_size::unknown_items::overlapped
    seq::fixed_name::variable_size::field_after_list::overlapped
    seq::fixed_name::variable_size::field_before_list::overlapped
    seq::fixed_name::variable_size::two_lists::overlapped
    seq::fixed_name::variable_size::unknown_items::overlapped
    seq::variable_name::fixed_size::field_after_list::overlapped
    seq::variable_name::fixed_size::field_before_list::overlapped
    seq::variable_name::fixed_size::two_lists::choice_and_fixed::overlapped_fixed_after
    seq::variable_name::fixed_size::two_lists::choice_and_fixed::overlapped_fixed_before
    seq::variable_name::fixed_size::two_lists::fixed_and_choice::overlapped_fixed_after
    seq::variable_name::fixed_size::two_lists::fixed_and_choice::overlapped_fixed_before
    seq::variable_name::variable_size::field_after_list::after
    seq::variable_name::variable_size::field_after_list::overlapped
    seq::variable_name::variable_size::field_before_list::after
    seq::variable_name::variable_size::field_before_list::overlapped
    seq::variable_name::variable_size::two_lists::choice_and_fixed::fixed_after
    seq::variable_name::variable_size::two_lists::choice_and_fixed::fixed_before
    seq::variable_name::variable_size::two_lists::choice_and_fixed::overlapped_fixed_after
    seq::variable_name::variable_size::two_lists::choice_and_fixed::overlapped_fixed_before
    seq::variable_name::variable_size::two_lists::fixed_and_choice::fixed_after
    seq::variable_name::variable_size::two_lists::fixed_and_choice::fixed_before
    seq::variable_name::variable_size::two_lists::fixed_and_choice::overlapped_fixed_after
    seq::variable_name::variable_size::two_lists::fixed_and_choice::overlapped_fixed_before

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

Author: Mingun

Changelog.md

@@ -14,6 +14,7 @@
 
 - [#9]: Deserialization erroneously was successful in some cases where error is expected.
   This broke deserialization of untagged enums which rely on error if variant cannot be parsed
+- [#387]: Allow to have an ordinary elements together with a `$value` field
 
 ### Misc Changes
 

src/de/map.rs

@@ -2,6 +2,7 @@
 
 use crate::{
     de::escape::EscapedDeserializer,
+    de::seq::not_in,
     de::{deserialize_bool, DeEvent, Deserializer, XmlRead, INNER_VALUE, UNFLATTEN_PREFIX},
     errors::serialize::DeError,
     events::attributes::IterState,
@@ -43,12 +44,14 @@ enum ValueSource {
     /// </any-tag>
     /// ```
     Text,
-    /// Next value should be deserialized from an element with an any name.
-    /// Corresponding tag name will always be associated with a field with name
-    /// [`INNER_VALUE`].
+    /// Next value should be deserialized from an element with an any name, except
+    /// elements with a name matching one of the struct fields. Corresponding tag
+    /// name will always be associated with a field with name [`INNER_VALUE`].
     ///
-    /// That state is set when call to [`peek()`] returns a [`Start`] event
-    /// _and_ struct has a field with a special name [`INNER_VALUE`].
+    /// That state is set when call to [`peek()`] returns a [`Start`] event, which
+    /// [`name()`] is not listed in the [list of known fields] (which for a struct
+    /// is a list of field names, and for a map that is an empty list), _and_
+    /// struct has a field with a special name [`INNER_VALUE`].
     ///
     /// When in this state, next event, returned by [`next()`], will be a [`Start`],
     /// which represents both a key, and a value. Value would be deserialized from
@@ -97,7 +100,9 @@ enum ValueSource {
     /// [`Start`]: DeEvent::Start
     /// [`peek()`]: Deserializer::peek()
     /// [`next()`]: Deserializer::next()
+    /// [`name()`]: BytesStart::name()
     /// [`Text`]: Self::Text
+    /// [list of known fields]: MapAccess::fields
     Content,
     /// Next value should be deserialized from an element with a dedicated name.
     ///
@@ -135,7 +140,24 @@ enum ValueSource {
     Nested,
 }
 
-/// A deserializer for `Attributes`
+/// A deserializer that extracts map-like structures from an XML. This deserializer
+/// represents a one XML tag:
+///
+/// ```xml
+/// <tag>...</tag>
+/// ```
+///
+/// Name of this tag is stored in a [`Self::start`] property.
+///
+/// # Lifetimes
+///
+/// - `'de` lifetime represents a buffer, from which deserialized values can
+///   borrow their data. Depending on the underlying reader, there can be an
+///   internal buffer of deserializer (i.e. deserializer itself) or an input
+///   (in that case it is possible to approach zero-copy deserialization).
+///
+/// - `'a` lifetime represents a parent deserializer, which could own the data
+///   buffer.
 pub(crate) struct MapAccess<'de, 'a, R>
 where
     R: XmlRead<'de>,
@@ -149,6 +171,8 @@ where
     /// Current state of the accessor that determines what next call to API
     /// methods should return.
     source: ValueSource,
+    /// List of field names of the struct. It is empty for maps
+    fields: &'static [&'static str],
     /// list of fields yet to unflatten (defined as starting with $unflatten=)
     unflatten_fields: Vec<&'static [u8]>,
 }
@@ -161,13 +185,14 @@ where
     pub fn new(
         de: &'a mut Deserializer<'de, R>,
         start: BytesStart<'de>,
-        fields: &[&'static str],
+        fields: &'static [&'static str],
     ) -> Result<Self, DeError> {
         Ok(MapAccess {
             de,
             start,
             iter: IterState::new(0, false),
             source: ValueSource::Unknown,
+            fields,
             unflatten_fields: fields
                 .iter()
                 .filter(|f| f.starts_with(UNFLATTEN_PREFIX))
@@ -230,7 +255,7 @@ where
                 // }
                 // TODO: This should be handled by #[serde(flatten)]
                 // See https://github.com/serde-rs/serde/issues/1905
-                DeEvent::Start(_) if has_value_field => {
+                DeEvent::Start(e) if has_value_field && not_in(self.fields, e, decoder)? => {
                     self.source = ValueSource::Content;
                     seed.deserialize(INNER_VALUE.into_deserializer()).map(Some)
                 }

src/de/seq.rs

@@ -1,18 +1,74 @@
 use crate::de::{DeError, DeEvent, Deserializer, XmlRead};
 use crate::events::BytesStart;
+use crate::reader::Decoder;
 use serde::de::{self, DeserializeSeed};
+#[cfg(not(feature = "encoding"))]
+use std::borrow::Cow;
 
+/// Check if tag `start` is included in the `fields` list. `decoder` is used to
+/// get a string representation of a tag.
+///
+/// Returns `true`, if `start` is not in the `fields` list and `false` otherwise.
+pub fn not_in(
+    fields: &'static [&'static str],
+    start: &BytesStart,
+    decoder: Decoder,
+) -> Result<bool, DeError> {
+    #[cfg(not(feature = "encoding"))]
+    let tag = Cow::Borrowed(decoder.decode(start.name())?);
+
+    #[cfg(feature = "encoding")]
+    let tag = decoder.decode(start.name());
+
+    Ok(fields.iter().all(|&field| field != tag.as_ref()))
+}
+
+/// A filter that determines, what tags should form a sequence.
+///
+/// There are two types of sequences:
+/// - sequence where each element represented by tags with the same name
+/// - sequence where each element can have a different tag
+///
+/// The first variant could represent a collection of structs, the second --
+/// a collection of enum variants.
+///
+/// In the second case we don't know what tag name should be expected as a
+/// sequence element, so we accept any element. Since the sequence are flattened
+/// into maps, we skip elements which have dedicated fields in a struct by using an
+/// `Exclude` filter that filters out elements with names matching field names
+/// from the struct.
+///
+/// # Lifetimes
+///
+/// `'de` represents a lifetime of the XML input, when filter stores the
+/// dedicated tag name
 #[derive(Debug)]
-enum Names {
-    Unknown,
-    Peek(Vec<u8>),
+enum TagFilter<'de> {
+    /// A `SeqAccess` interested only in tags with specified name to deserialize
+    /// an XML like this:
+    ///
+    /// ```xml
+    /// <...>
+    ///   <tag/>
+    ///   <tag/>
+    ///   <tag/>
+    ///   ...
+    /// </...>
+    /// ```
+    ///
+    /// The tag name is stored inside (`b"tag"` for that example)
+    Include(BytesStart<'de>), //TODO: Need to store only name instead of a whole tag
+    /// A `SeqAccess` interested in tags with any name, except explicitly listed.
+    /// Excluded tags are used as struct field names and therefore should not
+    /// fall into a `$value` category
+    Exclude(&'static [&'static str]),
 }
 
-impl Names {
-    fn is_valid(&self, start: &BytesStart) -> bool {
+impl<'de> TagFilter<'de> {
+    fn is_suitable(&self, start: &BytesStart, decoder: Decoder) -> Result<bool, DeError> {
         match self {
-            Names::Unknown => true,
-            Names::Peek(n) => n == start.name(),
+            Self::Include(n) => Ok(n.name() == start.name()),
+            Self::Exclude(fields) => not_in(fields, start, decoder),
         }
     }
 }
@@ -22,8 +78,10 @@ pub struct SeqAccess<'de, 'a, R>
 where
     R: XmlRead<'de>,
 {
+    /// Deserializer used to deserialize sequence items
     de: &'a mut Deserializer<'de, R>,
-    names: Names,
+    /// Filter that determines whether a tag is a part of this sequence.
+    filter: TagFilter<'de>,
 }
 
 impl<'a, 'de, R> SeqAccess<'de, 'a, R>
@@ -32,16 +90,17 @@ where
 {
     /// Get a new SeqAccess
     pub fn new(de: &'a mut Deserializer<'de, R>) -> Result<Self, DeError> {
-        let names = if de.has_value_field {
-            Names::Unknown
+        let filter = if de.has_value_field {
+            TagFilter::Exclude(&[])
         } else {
             if let DeEvent::Start(e) = de.peek()? {
-                Names::Peek(e.name().to_vec())
+                // Clone is cheap if event borrows from the input
+                TagFilter::Include(e.clone())
             } else {
-                Names::Unknown
+                TagFilter::Exclude(&[])
             }
         };
-        Ok(SeqAccess { de, names })
+        Ok(Self { de, filter })
     }
 }
 
@@ -55,10 +114,26 @@ where
     where
         T: DeserializeSeed<'de>,
     {
+        let decoder = self.de.reader.decoder();
         match self.de.peek()? {
             DeEvent::Eof | DeEvent::End(_) => Ok(None),
-            DeEvent::Start(e) if !self.names.is_valid(e) => Ok(None),
+            DeEvent::Start(e) if !self.filter.is_suitable(e, decoder)? => Ok(None),
             _ => seed.deserialize(&mut *self.de).map(Some),
         }
     }
 }
+
+#[test]
+fn test_not_in() {
+    let tag = BytesStart::borrowed_name(b"tag");
+
+    assert_eq!(not_in(&[], &tag, Decoder::utf8()).unwrap(), true);
+    assert_eq!(
+        not_in(&["no", "such", "tags"], &tag, Decoder::utf8()).unwrap(),
+        true
+    );
+    assert_eq!(
+        not_in(&["some", "tag", "included"], &tag, Decoder::utf8()).unwrap(),
+        false
+    );
+}

src/reader.rs

@@ -1669,6 +1669,23 @@ impl Decoder {
     }
 }
 
+/// This implementation is required for tests of other parts of the library
+#[cfg(test)]
+#[cfg(feature = "serialize")]
+impl Decoder {
+    #[cfg(not(feature = "encoding"))]
+    pub(crate) fn utf8() -> Self {
+        Decoder
+    }
+
+    #[cfg(feature = "encoding")]
+    pub(crate) fn utf8() -> Self {
+        Decoder {
+            encoding: encoding_rs::UTF_8,
+        }
+    }
+}
+
 #[cfg(test)]
 mod test {
     macro_rules! check {