Enums don't serialize correctly

[antonsmetanin]

Enum serialization doesn't seem to work correctly. I've prepared a test that demonstrates the issue, but basically I couldn't find a way to serialize an enum to match the following schema, other than manually building a Value, and even then schema resolution fails to find the correct union variant:

{
    "name": "Root",
    "type": "record",
    "fields": [
        {"name": "field_union", "type": [
            {
                "name": "A",
                "type": "record",
                "fields": []
            },
            {
                "name": "B",
                "type": "record",
                "fields": []
            },
            {
                "name": "C",
                "type": "record",
                "fields": [
                    {"name": "field_a", "type": "long"},
                    {"name": "field_b", "type": ["null", "string"]}
                ]
            },
            {
                "name": "D",
                "type": "record",
                "fields": [
                    {"name": "field_a", "type": "float"},
                    {"name": "field_b", "type": "int"}
                ]
            }
        ]},
        {"name": "field_f", "type": "string"}
    ]
}

Here's the enum definition that's supposed to work with this schema, but it doesn't currently:

#[derive(Serialize)]
struct Root {
    field_union: Enum,
    field_f: String,
}

#[derive(Serialize)]
enum Enum {
    A {},
    B {},
    C {
        field_a: i64,
        field_b: Option<String>,
    },
    D {
        field_a: f32,
        field_b: i32
    },
}

So I've looked into the implementation, and found two issues:

1. For some reason enum variants get serialized as records with two fields:

• "type" with an Avro enum for the discriminator,
• "value" with a union variant containing the fields.
  instead of just serializing union variant directly. Because of this bytecode ends up being different from what's expected (there's an additional enum byte for "type"), but also schema resolution fails because it expects to find actual fields at the top level, while they are under "value".

2. Separately from the first issue, even if you try to use resolve() on a Value built manually, schema resolution ignores the variant number coming from Value::Union and tries to find a schema by simply matching the fields, which causes serialization to lose field data in some cases, depending on the order in which these variants are listed in the schema. In the example above, A is tested first, and since it doesn't have any fields, it matches any value.

[antonsmetanin]

I've prepared a PR that fixes this with a test to validate the fix: #366
Would be grateful if you could take a look.

[martin-g]

This sounds a lot like apache/avro#1921

[antonsmetanin]

@martin-g Thanks. I didn't read the whole thread, but I must note that serde distinguishes between unit variants

enum E {
    A
}

and empty struct variants

enum E {
    A {}
}

So I don't think mixed variants like

enum E {
    A,
    B { field: i64 }
}

should be supported (except maybe with a single unit variant treated as null, as a special case, to make it consistent with Option<T>, but I'm not sure about this), but for most cases

enum E {
    A {},
    B { field: i64 }
}

would be equivalent to the above and should be supported.

I think in general we should agree on a specification of how serde maps to Avro, and then test the implementation against it. I can try to work on a proposal when I have some free time.

[Kriskras99]

I wrote a small program to test how Serde serializes enums: https://gist.github.com/Kriskras99/553e6a29777038359e24252df61fb959
It prints the function name and arguments for every serialize function called.

I think we can support mixed variants with SchemaAwareSerializer but not with to_value.
And this isn't the only time where this is an issue with to_value, I think its function definition should be changed from pub fn to_value<S: Serialize>(value: S) -> Result<Value, Error> to pub fn to_value<S: Serialize + AvroSchema>(value: S) -> Result<Value, Error> (and do the same with from_value). Without having a schema it's all guess work and very brittle.

[Kriskras99]

What schema should we generate for the following enum?:

#[derive(AvroSchema, Serialize, Deserialize)]
struct Bar {
    a: String
};

#[derive(AvroSchema, Serialize, Deserialize)]
enum Foo {
    One(Bar),
    Two(Bar),
}

The derive for Foo can only control the namespace in the schema returned by Bar, so both variants will have the same Record schema which is not allowed by spec. We could add the enum variant to the namespace giving something like this:

[
    {
        "type": "record",
        "name": "Bar",
        "namespace": "A",
        "fields": [{"name": "a", "type": "string"}]
    },
    {
        "type": "record",
        "name": "Bar",
        "namespace": "B",
        "fields": [{"name": "a", "type": "string"}]
    }
]

And when the union is already in a namespace:

{
    "type": "record",
    "name": "Outer",
    "namespace": "some.name.space",
    "fields": [
        {
            "name": "enum", 
            "type": [
                {
                    "type": "record",
                    "name": "Bar",
                    "namespace": "some.name.space.A",
                    "fields": [{"name": "a", "type": "string"}]
                },
                {
                    "type": "record",
                    "name": "Bar",
                    "namespace": "some.name.space.B",
                    "fields": [{"name": "a", "type": "string"}]
                }
            ]
        }
    ]
}

If we don't want to touch the namespace, the the current implementation (record with enum and union) is the only option left I think.

[antonsmetanin]

@Kriskras99 Can you elaborate on why you think it's guess work? I'm not sure I'm following. As far as I understand, the problem with mixed enums is that when serialize_unit_variant is called, we don't know whether other variants are units or structs:

• if all others are also unit variants, then it must be serialized as Avro enum string;
• but if at least one of them is a struct variant, then this one must be serialized as Avro union variant.

But as your program demonstrated, for empty struct variants serde_derive generates a serialize_unit_variant call. That's why I'm saying that true mixed enums should be explicitly prohibited, but enums that use empty struct variants in place of unit ones, which are semantically equivalent, should be supported. And actually the test from my PR (https://github.com/apache/avro-rs/blob/a4a430fc44deca1fcdd1df1b86d03ca6929fd07a/avro/tests/union_serialization.rs) shows that they are working fine without requiring SchemaAwareSerializer or adding the AvroSchema trait bound to to_value.

As for deriving schema from Rust type definition, yes, it's a whole other can of worms, but we can probably control that with custom annotations. For example, #[avro_union(variant_name)] in:

#[derive(AvroSchema, Serialize, Deserialize)]
#[avro_union(variant_name)]
enum Foo {
    One(Bar),
    Two(Bar),
}

could indicate that it should use variant name as type name:

[
    {
        "type": "record",
        "name": "One",
        "fields": [{"name": "a", "type": "string"}]
    },
    {
        "type": "record",
        "name": "Two",
        "fields": [{"name": "a", "type": "string"}]
    }
]

Or #[avro_union_type(name = "Baz")] in:

#[derive(AvroSchema, Serialize, Deserialize)]
#[avro_union(variant_name)]
enum Foo {
    One(Bar),
    #[avro_union_type(name = "Baz")]
    Two(Bar),
}

could mean that it should generate (or re-use if it's been previously defined) type Baz for this specific variant:

[
    {
        "type": "record",
        "name": "Bar",
        "fields": [{"name": "a", "type": "string"}]
    },
    {
        "type": "record",
        "name": "Baz",
        "fields": [{"name": "a", "type": "string"}]
    }
]

There are some questions to be considered, though, like how should primitives be handled. Since using variant names for types or specifying the type manually makes no sense for them:

enum Foo {
    One(Bar),
    Two(String),
}

On the other hand, struct variants like the following:

enum Foo {
    One(Bar),
    Two { a: String },
}

can only use variant name for type because the struct is anonymous.

But in general I believe it's a separate issue. That is, it would be perfectly fine if derive didn't support such enums, as long as to_value supported them, since you can always fall back to manually defining the schema.

[Kriskras99]

You're right, if you forbid mixing unit variants and struct/newtype variants then to_value can just work.

[jdarais]

@antonsmetanin @Kriskras99 regarding mixing of unit variants with struct variants in rust enum types, it should work if you treat the rust unit type as avro null. I think this is also the mapping used for serialize_unit in the Serializer implementation in both ser.rs and ser_schema.rs.

With this, having a rust enum type with two unit variants and a struct variant (e.g. enum MyEnum { A, B, C {} } would be invalid since both A and B would be mapped to null, and avro union can't have duplicate types, but having a single unit variant combined with struct variants would be valid. This also works with rust's Option type without modification, which would map None to null.