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.

[Kriskras99]

Over the past month I've done a lot of thinking about how to improve the enum support in Avro and I think I finally have something that can work. It's something that's workable in both the derive code and the general code, and remains compatible with current schemas.

Current state of Rust enum support in Avro

Plain enums

Plain enums are serialized as Avro enums (only enum support by #[derive(AvroSchema)]):

pub enum Foo {
    A,
    B,
    C,
}

{
  "name": "Foo",
  "type": "enum",
  "symbols": ["A", "B", "C"]
}

Data enums

Data enums are serialized as a record with a discriminator field (Avro enum) and a value field (Avro union):

pub struct Bar {
    integer: i32,
}
pub enum Foo {
    A {
        field: String,
    },
    B(Bar)
}

{
  "name": "Foo",
  "type": "record",
  "fields": [
    {
      "name": "type",
      "type": {
        "type": "enum",
        "symbols": ["A", "B"]
      }
    },
    {
      "name": "value",
      "type": [
        {
          "type": "record",
          "fields": [
            {
              "name": "field",
              "type": "string"
            }
          ]
        },
        {
          "name": "Bar",
          "type": "record",
          "fields": [
            {
              "name": "integer",
              "type": "int"
            }
          ]
        }
      ]
    }
  ]
}

The advantage of this approach is that it works for enums where multiple variants of the same type. It does not currently
work with mixed enums as a unit variant will always be encoded as an Avro enum.

Options

Options have special support in the encoding logic to always produce a bare union.

type Foo = Option<String>;

[
  "null",
  "string"
]

Alternative representations

These alternative representations support mixed enums.

Bare union

If all types are unique, then a regular union can be used:

pub enum Foo {
    A {
        field: String,
    },
    B(Bar),
    C,
}

[
  {
    "type": "record",
    "name": "A",
    "fields": [
      {
        "name": "field",
        "type": "string"
      }
    ]
  },
  {
    "name": "Bar",
    "type": "record",
    "fields": [
      {
        "name": "integer",
        "type": "int"
      }
    ]
  },
  "null"
]

Using the variant name as the namespace can prevent collisions for named schema types, this however doesn't work for
unnamed types.

Union with a record for every variant

pub enum Foo {
    A {
        field: String,
    },
    B(Bar),
    C,
}

[
  {
    "type": "record",
    "name": "A",
    "fields": [
      {
        "name": "field",
        "type": "string"
      }
    ]
  },
  {
    "type": "record",
    "name": "B",
    "fields": [
      {
        "name": "inner",
        "type": {
          "name": "Bar",
          "type": "record",
          "fields": [
            {
              "name": "integer",
              "type": "int"
            }
          ]
        }
      }
    ]
  },
  {
    "type": "record",
    "name": "C",
    "fields": [
      {
        "name": "inner",
        "type": "null"
      }
    ]
  }
]

This representation will always work and is always as efficient when binary encoded compared to the bare union. However,
the schema definition does become larger inflating the JSON but also the in-memory representation.

Proposal

Deriving

It would be good to broaden the support of enums we can (de)serialize, I suggest the following schema derive strategy:

1. If it is a plain enum, emit an Avro enum
2. If it is a mixed/data enum, try to emit an Avro union
3. If that fails, emit an Avro record with a type and value field

Encoding/decoding

When encoding or decoding we would just look at the schema to see what is expected. This does mean changing the signature
of to_value(value: S) -> Result<Value, Error> to to_value(value: S, schema: &Schema) -> Result<Value, Error>. We could
also put a bound on S that it has to implement AvroSchema, but that's not good performance wise as it cannot be cached.

P.S. While thinking about this problem, it occurred to me that it's possible that the AvroSchema derive can produce
invalid schemas (fields with the same name because of a rename, Option<T> where T's schema is null). Users who
are creating a schema by hand (either completely or generating schemas in code) can of course also have this issue. I
think it would be a good idea to add a Schema::validate(&self) -> Result<(), Error> function so we can validate the
generated schema in the derive, and users can check their own schemas.

P.P.S. An addition consideration for the derive implementation is directly nested data enums, that would only work with the Avro record with a type and value field.

[jdarais]

I like the idea of using a union with a record for every variant. You could also represent unit variants as a record with no fields, so no need for a null-type inner field. And tuple variants could be represented as a struct with fields named something like _1, _2, _3, etc, so those would be covered, too.

I also like the idea of being able to provide the schema when calling to_value, though we might also be able to get away with adding a new function, like to_value_with_schema<S: Serialize>(value: S, schema: &Schema) -> Result<Value, Error>, and having the regular to_value function just do whatever the schema produced by the derived AvroSchema trait would have dictated.

[Kriskras99]

My main concern is that the Union of Records has quite a lot of indirection:
Union (Vec) -> Record: Fields (Vec) -> Schema
Compared to the bare union:
Union (Vec) -> Schema
But it has the same amount of indirection as the Record with tag and union:
Record: Fields (Vec) -> Union (Vec) -> Schema

So I think we should still prefer the bare union, but also support either Union of Records or Record with tag and union. We already support Record with tag and union, so that would be the easiest option.

I do like your suggestion on supporting tuples, although that could also be achieved with the bare union.

[Kriskras99]

One advantage of the Union of Records, is that it allows for nested unions (as they are indirectly nested via a record).