serde-rs
add missing visit_enum
I can confirm this PR fixes https://github.com/surrealdb/surrealdb/issues/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?
@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.
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?
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
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?
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
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.
@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.
@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 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.
@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
@Mingun is there a test that specifically tests the visit functions for Content? I couldn't reproduce the issue because its for a specific case where the enum type is unknown.
What specific methods you mean? If you want to figure out which tests covers methods, just add panic in them and see what tests failed. Then you can examine those tests to understand how they can be modified to reach lines you want.
As I say, you can just insert panic!(); here and see that there is no tests for this method:
https://github.com/serde-rs/serde/blob/a685dcf6801884fb7dc638faf95f093d0e1943ec/serde/src/private/de.rs#L528-L535
Because error message say us that this error is expected in context of untagged or internally tagged enums, start looking at tests for that representations. Search for untagged and tag, because this is the words used in serde attributes to annotate enum as internally tagged or untagged.
Also, you could try to panic inside another method of ContentVisitor, for example here:
https://github.com/serde-rs/serde/blob/a685dcf6801884fb7dc638faf95f093d0e1943ec/serde/src/private/de.rs#L514-L526
This gives you following failures (cargo +nightly test --features unstable in ./test_suite):
failures (in test_annotations):
flatten::enum_::adjacently_tagged::newtype
flatten::enum_::adjacently_tagged::struct_
flatten::enum_::externally_tagged::newtype
flatten::enum_::externally_tagged::struct_from_map
flatten::enum_::untagged::struct_
flatten::unknown_field
test_partially_untagged_enum
test_partially_untagged_enum_desugared
test_partially_untagged_enum_generic
test_partially_untagged_internally_tagged_enum
So look at those tests and think how they could be modified to reach visit_enum instead of visit_map. General considerations tell us that most likely somewhere in the deserialized type there should be reference to an enum type. You can also note, that this approach was more effective, because ContentVisitor used also for #[serde(flatten)] fields and that is another place for experiments.