add missing visit_enum

[frederik-uni]

fixes surrealdb/surrealdb#4844 and probably dtolnay/serde-yaml#344

[frederik-uni]

@dtolnay is there an issue with this request?

[sagarrabadiya]

I can confirm this PR fixes surrealdb/surrealdb#4844 currently stuck with untagged variant deserialize by adding this method code into crate it works as expected without this method untagged enum with input data doesn’t deserialize and throws an error

when will this PR be merged?

[frederik-uni]

@nicolube as it was mentioned above it doesn’t really make sense. It's only a fix that works for the specific cases mentioned above but won’t work for custom deserializers.

/// Called when deserializing a struct-like variant.
data.struct_variant(fields, visitor);
/// Called when deserializing a variant with a single value.
data.newtype_variant();
/// Called when deserializing a tuple-like variant.
data.tuple_variant(len, visitor);
/// Called when deserializing a variant with no values.
data.unit_variant();

I dont see a way to determine what variant the value is. newtype_variant works out for the issues we are encountering with surrealdb, but there could be cases where the other functions would be needed.

[frederik-uni]

tests for this will be hard because the default behavior doesnt change.

I tested it with a serde-content fork

fn hint(&self) -> Option<VariantHint> {
        Some(match &self.data {
            Some(Value::Map(_)) => VariantHint::Struct(&[]),
            Some(Value::Seq(seq)) => VariantHint::Tuple(seq.len()),
            Some(_) => VariantHint::Newtype,
            None => VariantHint::Unit,
        })
    }

@Mingun how would you write tests for it?

[oli-obk]

tests for this will be hard because the default behavior doesnt change.

if you change the deserializers in https://github.com/serde-rs/serde/blob/master/serde/src/de/value.rs#L264 to support this, is there anything else missing from https://github.com/serde-rs/serde/blob/master/test_suite/tests/test_de.rs that you'd need to write a test?

[frederik-uni]

it doesn’t really make sense. It's only a fix that works for the specific cases mentioned above but won’t work for custom deserializers.

can you elaborate on what doesn't make sense? I am missing context I think

the original code was

Ok(Content::Map(
                [(
                    Content::String(key),
                    tri!(data.newtype_variant::<Self::Value>()),
                )]
                .into(),

I added the type hint so the right function is called

let data = match variant_hint {
                de::VariantHint::Unit => Content::Unit,
                de::VariantHint::Newtype => tri!(data.newtype_variant()),
                de::VariantHint::Tuple(len) => tri!(data.tuple_variant(len, self)),
                de::VariantHint::Struct(fields) => tri!(data.struct_variant(fields, self)),
            };

So its fixed now

[frederik-uni]

tests for this will be hard because the default behavior doesnt change.

if you change the deserializers in https://github.com/serde-rs/serde/blob/master/serde/src/de/value.rs#L264 to support this, is there anything else missing from https://github.com/serde-rs/serde/blob/master/test_suite/tests/test_de.rs that you'd need to write a test?

I dont see me writing tests. I am not that familiar with how serde works internally and I just wanted to fix an issue I had with surrealdb. The implementation was missing completely and the use case of this function is a very specific case that seems to only happen when using flatten. If someone wants to pick it up and write the tests for it please do.

[Mingun]

@frederik-uni, the goal to is write tests to cover all possible code paths in your new code. So you need to figure out what conditions is needed for that and construct corresponding input.

[frederik-uni]

@Mingun the untagged feature has no existing tests. there is a folder in the test_suite labeld regression but there arent any tests. I am not familiar enough with the codebase to be able to create a deserializer in the test_suite that has the new hint. Thats why i said if someone wants to pick it up they are welcome to. I literally dont know how to write the test. the primitive_deserializer has no uses in the test_suite. Thats why i said i wont implement it. I just dont know how and its not worth the time investment.

BTW i dont get how a primitive_deserializer would help test the flatten feature but ok

[Mingun]

@Mingun the untagged feature has no existing tests

No, it has:
https://github.com/serde-rs/serde/blob/b9dbfcb4ac3b7a663d9efc6eb1387c62302a6fb4/test_suite/tests/test_enum_untagged.rs

Most tests define some serializable struct and by using serde_test (de)serializer asserts that:

• struct is serialized to the some list of tokens. Each token roughly represents the serde API call of a Serializer;
• struct is tried to deserialize from some list of tokens, which roughly represents the serde API call of a Deserializer / Visitor. Usually several different representations are allowed (for example, for tagged enums either tag or content can be first), so test tries to deserialize from several representations.

Each token also roughly represents some serialization construct, for example, string, number, or mapping in JSON. So if you aware of the structure that you want to deserialize, you may construct token list corresponding to it and check that serde is able to deserialize from it.

[frederik-uni]

@Mingun it seems like i cant reproduce the bug using assert_tokens. i guess the visit_enum function isnt used internally & serde-content visited the enum differently than it was meant to