martin-g commented on code in PR #569: URL: https://github.com/apache/avro-rs/pull/569#discussion_r3489781181
########## avro_derive/src/enums/bare_union.rs: ########## @@ -0,0 +1,107 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +use crate::attributes::{NamedTypeOptions, Repr, VariantOptions}; +use crate::enums::{FieldVariants, SchemaType, variant_to_schema_expr}; +use crate::implementation::Implementation; +use crate::utils::json_value_expr; +use proc_macro2::Ident; +use quote::quote; +use std::collections::HashSet; +use syn::spanned::Spanned; +use syn::{DataEnum, Generics}; + +pub fn to_implementation( + ident: Ident, + generics: Generics, + container_attrs: NamedTypeOptions, + data: DataEnum, +) -> Result<Implementation, Vec<syn::Error>> { + let Some(Repr::BareUnion { untagged }) = container_attrs.repr else { + unreachable!() + }; + + let mut errors = Vec::new(); + let mut variant_exprs = Vec::new(); + + // Used for checking that the resulting schema is usefull Review Comment: ```suggestion // Used for checking that the resulting schema is useful ``` ########## avro_derive/src/attributes/mod.rs: ########## @@ -24,14 +24,117 @@ use syn::{AttrStyle, Attribute, Expr, Ident, Path, spanned::Spanned}; mod avro; mod serde; +/// What `Schema` representation to generate for a type. +#[derive(Debug, PartialEq)] +pub enum Repr { + /// Generate a `Schema::Enum` for a `enum`. + /// + /// Only works for unit variants. + Enum, + /// Generate a `Schema::Union` for a `enum`. + /// + /// There can only be one unit variant and every newtype/struct/tuple variant must be unique. + BareUnion { untagged: bool }, + /// Generate a `Schema::Union` with a `Schema::Record` for a `enum`. + /// + /// This works for every enum as the records will have unique names for every variant. + UnionOfRecords, + /// Generate a `Schema::Record` with a tag and content field for a `enum`. + /// + /// Requires `#[serde(tag = "...", content = "...")]`. + RecordTagContent { tag: String, content: String }, + /// Generate a `Schema::Record` with a tag field and flattened variant fields for a `enum`. + /// + /// Requires `#[serde(tag = "...")]` + RecordInternallyTagged { tag: String }, +} + +impl Repr { + fn from_avro_and_serde( + avro: Option<avro::Repr>, + tag: Option<String>, + content: Option<String>, + untagged: bool, + span: Span, + ) -> Result<Option<Self>, syn::Error> { + match avro { + Some(avro::Repr::Enum) => { + if tag.is_some() || content.is_some() || untagged { Review Comment: nit: IMO it would be friendlier for the user if each condition has its own error. This way (s)he will know exactly what the problem is instead of checking which one from the listed incompatibilities is the issue. If you agree then please also consider returning a `Vec<syn::Error>` ########## avro_derive/src/enums/record_internally_tagged.rs: ########## @@ -0,0 +1,215 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +use crate::attributes::{FieldOptions, NamedTypeOptions, Repr, VariantOptions, With}; +use crate::enums::newtype_extra_attribute_checks; +use crate::fields::{field_to_record_fields_expr, named_fields_to_record_fields}; +use crate::implementation::Implementation; +use crate::utils::{aliases, json_value_expr, preserve_optional}; +use proc_macro2::Ident; +use quote::quote; +use syn::spanned::Spanned; +use syn::{DataEnum, Expr, Fields, Generics}; + +pub fn to_implementation( + ident: Ident, + generics: Generics, + container_attrs: NamedTypeOptions, + data: DataEnum, +) -> Result<Implementation, Vec<syn::Error>> { + let Some(Repr::RecordInternallyTagged { tag }) = container_attrs.repr else { + unreachable!() + }; + let mut errors = Vec::new(); + let mut field_exprs = Vec::new(); + + for variant in data.variants { + let variant_span = variant.span(); + let variant_attrs = match VariantOptions::new(&variant.attrs, variant_span) { + Ok(attrs) => attrs, + Err(errs) => { + errors.extend(errs); + continue; + } + }; + + if variant_attrs.skip { + continue; + } else if !variant_attrs.only_skip_rename_and_alias_can_be_set() { Review Comment: The error message at line 55 says `On unit variants...` but there is no check here that the variant type is a `unit` here. ```suggestion } else if matches!(&variant.fields, Fields::Unit) && !variant_attrs.only_skip_rename_and_alias_can_be_set() { ``` ########## avro_derive/tests/derive.rs: ########## @@ -1791,7 +1791,7 @@ fn avro_rs_247_serde_flatten_support_duplicate_field_name() { a: i32, } - Foo::get_schema(); + panic!("{:?}", Foo::get_schema()); Review Comment: debug leftover ? ########## avro_derive/src/enums/bare_union.rs: ########## @@ -0,0 +1,107 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +use crate::attributes::{NamedTypeOptions, Repr, VariantOptions}; +use crate::enums::{FieldVariants, SchemaType, variant_to_schema_expr}; +use crate::implementation::Implementation; +use crate::utils::json_value_expr; +use proc_macro2::Ident; +use quote::quote; +use std::collections::HashSet; +use syn::spanned::Spanned; +use syn::{DataEnum, Generics}; + +pub fn to_implementation( + ident: Ident, + generics: Generics, + container_attrs: NamedTypeOptions, + data: DataEnum, +) -> Result<Implementation, Vec<syn::Error>> { + let Some(Repr::BareUnion { untagged }) = container_attrs.repr else { + unreachable!() + }; + + let mut errors = Vec::new(); + let mut variant_exprs = Vec::new(); + + // Used for checking that the resulting schema is usefull + let mut have_null = false; + let mut names = HashSet::new(); + let mut tuple_sizes = HashSet::new(); Review Comment: ```suggestion let mut tuple_sizes = BTreeSet::new(); ``` To make the order deterministic. Otherwise the UI tests may fail due to different order. ########## avro_derive/src/enums/record_tag_content.rs: ########## @@ -0,0 +1,161 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +use crate::attributes::{NamedTypeOptions, Repr, VariantOptions}; +use crate::case::RenameRule; +use crate::enums::variant_to_schema_expr; +use crate::implementation::Implementation; +use crate::utils::{aliases, json_value_expr, name_expr, preserve_optional, rename_ident}; +use proc_macro2::{Ident, Span}; +use quote::quote; +use std::collections::HashSet; +use syn::spanned::Spanned; +use syn::{DataEnum, Generics}; + +pub fn to_implementation( + input_span: Span, + ident: Ident, + generics: Generics, + container_attrs: NamedTypeOptions, + data: DataEnum, +) -> Result<Implementation, Vec<syn::Error>> { + let Some(Repr::RecordTagContent { tag, content }) = container_attrs.repr else { + unreachable!() + }; + let mut errors = Vec::new(); + let mut symbols = Vec::new(); + let mut variant_exprs = Vec::new(); + + for variant in data.variants { + let variant_attrs = VariantOptions::new(&variant.attrs, variant.span())?; + + if variant_attrs.skip { + continue; + } + + let name = rename_ident( + &variant.ident, + variant_attrs.rename.clone(), + container_attrs.rename_all, + RenameRule::apply_to_variant, + ); + + match variant_to_schema_expr( + variant, + variant_attrs, + container_attrs.rename_all, + container_attrs.rename_all_fields, + true, + true, + |_| Ok(()), + ) { + Ok(expr) => variant_exprs.push(expr), + Err(errs) => errors.extend(errs), + } + + symbols.push(name); Review Comment: Should this be moved to the `Ok(expr)` arm above (line 66) ? ########## avro_derive/src/enums/record_tag_content.rs: ########## @@ -0,0 +1,161 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +use crate::attributes::{NamedTypeOptions, Repr, VariantOptions}; +use crate::case::RenameRule; +use crate::enums::variant_to_schema_expr; +use crate::implementation::Implementation; +use crate::utils::{aliases, json_value_expr, name_expr, preserve_optional, rename_ident}; +use proc_macro2::{Ident, Span}; +use quote::quote; +use std::collections::HashSet; +use syn::spanned::Spanned; +use syn::{DataEnum, Generics}; + +pub fn to_implementation( + input_span: Span, + ident: Ident, + generics: Generics, + container_attrs: NamedTypeOptions, + data: DataEnum, +) -> Result<Implementation, Vec<syn::Error>> { + let Some(Repr::RecordTagContent { tag, content }) = container_attrs.repr else { + unreachable!() + }; + let mut errors = Vec::new(); + let mut symbols = Vec::new(); + let mut variant_exprs = Vec::new(); + + for variant in data.variants { + let variant_attrs = VariantOptions::new(&variant.attrs, variant.span())?; Review Comment: The `?` breaks the error accumulation. Is this intentional ? ########## avro_derive/src/lib.rs: ########## @@ -84,3 +84,37 @@ fn derive_avro_schema(input: DeriveInput) -> Result<Implementation, Vec<syn::Err )]), } } + +// #[cfg(test)] Review Comment: remove ?! ########## avro_derive/tests/enum.rs: ########## @@ -0,0 +1,721 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +use apache_avro::reader::datum::GenericDatumReader; +use apache_avro::writer::datum::GenericDatumWriter; +use apache_avro::{AvroSchema, Schema}; +use pretty_assertions::assert_eq; +use serde::{Deserialize, Serialize, de::DeserializeOwned}; + +/// Takes in a type that implements the right combination of traits and runs it through a Serde Cycle and asserts the result is the same +#[expect( + clippy::needless_pass_by_value, + reason = "Significantly complicates the trait bounds" +)] +#[track_caller] +fn serde_assert<T>(obj: T) +where + T: std::fmt::Debug + Serialize + DeserializeOwned + AvroSchema + PartialEq, +{ + assert_eq!(obj, serde(&obj)); +} + +#[track_caller] +fn serde<T>(obj: &T) -> T +where + T: Serialize + DeserializeOwned + AvroSchema, +{ + de(&ser(obj)) +} + +#[track_caller] +fn ser<T>(obj: &T) -> Vec<u8> +where + T: Serialize + AvroSchema, +{ + let schema = T::get_schema(); + GenericDatumWriter::builder(&schema) + .build() + .unwrap() + .write_ser_to_vec(&obj) + .unwrap() +} + +#[track_caller] +fn de<T>(mut encoded: &[u8]) -> T +where + T: DeserializeOwned + AvroSchema, +{ + assert!(!encoded.is_empty()); + let schema = T::get_schema(); + GenericDatumReader::builder(&schema) + .build() + .unwrap() + .read_deser(&mut encoded) + .unwrap() +} + +#[test] +fn avro_rs_561_bare_union_untagged_empty_tuple_variant() { + #[derive(Debug, Eq, PartialEq, AvroSchema, Serialize, Deserialize)] + #[avro(repr = "bare_union")] + #[serde(untagged)] + enum C { + A(), + } + + serde_assert(C::A()); +} + +#[test] +fn avro_rs_561_bare_union_empty_tuple_variant() { + #[derive(Debug, Eq, PartialEq, AvroSchema, Serialize, Deserialize)] + #[avro(repr = "bare_union")] Review Comment: The documentation for BareUnion says that `#[serde(untagged)]` is required. It seems there is no check for this requirement. Or the documentation is out of date. ########## avro_derive/src/enums/mod.rs: ########## @@ -37,23 +45,223 @@ pub fn to_implementation( "AvroSchema: `#[serde(transparent)]` is only supported on structs", )]); } - if data - .variants - .iter() - .filter(|v| { - // Filter skipped variants, if the attributes fail to parse we'll also filter them out - // `plain` will throw proper errors for that. - VariantOptions::new(&v.attrs, v.span()) - .map(|o| !o.skip) - .unwrap_or(false) - }) - .all(|v| Fields::Unit == v.fields) - { - plain::to_implementation(input_span, ident, generics, container_attrs, data) - } else { - Err(vec![syn::Error::new( + + match &container_attrs.repr { + None => { + if data + .variants + .iter() + .filter(|v| { + // Filter skipped variants, if the attributes fail to parse we'll also filter them out + // `plain` will throw proper errors for that. + VariantOptions::new(&v.attrs, v.span()) + .map(|o| !o.skip) + .unwrap_or(false) + }) + .all(|v| Fields::Unit == v.fields) + { + plain::to_implementation(input_span, ident, generics, container_attrs, data) + } else { + union_of_records::to_implementation(ident, generics, container_attrs, data) + } + } + Some(Repr::Enum) => { + plain::to_implementation(input_span, ident, generics, container_attrs, data) + } + Some(Repr::BareUnion { .. }) => { + bare_union::to_implementation(ident, generics, container_attrs, data) + } + Some(Repr::UnionOfRecords) => { + union_of_records::to_implementation(ident, generics, container_attrs, data) + } + Some(Repr::RecordTagContent { .. }) => record_tag_content::to_implementation( input_span, - "AvroSchema: derive does not work for enums with non unit structs", - )]) + ident, + generics, + container_attrs, + data, + ), + Some(Repr::RecordInternallyTagged { .. }) => { + record_internally_tagged::to_implementation(ident, generics, container_attrs, data) + } + } +} + +fn newtype_extra_attribute_checks( + options: FieldOptions, + span: Span, +) -> Result<FieldOptions, Vec<syn::Error>> { + let mut errors = Vec::new(); + if options.doc.is_some() { + errors.push(syn::Error::new( + span, + r#"AvroSchema: `#[avro(doc = "..")]` only works on newtype variants when the enum uses `#[avro(repr = "union_of_records")`"# + )); + } + if !matches!(options.default, FieldDefault::Trait) { + errors.push(syn::Error::new( + span, + r#"AvroSchema: `#[avro(default = ..)]` only works on newtype variants when the enum uses `#[avro(repr = "union_of_records")`"# + )); + } + if !options.alias.is_empty() { + errors.push(syn::Error::new( + span, + r#"AvroSchema: `#[avro(alias = "..")]` only works on newtype variants when the enum uses `#[avro(repr = "union_of_records")`"# + )); } + if options.rename.is_some() { + errors.push(syn::Error::new( + span, + r#"AvroSchema: `#[avro(alias = "..")]` only works on newtype variants when the enum uses `#[avro(repr = "union_of_records")`"# + )); + } + if options.flatten { + errors.push(syn::Error::new( + span, + r#"AvroSchema: `#[avro(flatten)]` only works on newtype variants when the enum uses `#[avro(repr = "union_of_records")`"#, + )); + } + + if !errors.is_empty() { + return Err(errors); + } + + Ok(options) +} + +fn variant_to_schema_expr( + variant: Variant, + variant_attrs: VariantOptions, + rename_all: RenameRule, + rename_all_fields: RenameRule, + transparent_newtype: bool, + unit_is_null: bool, + mut check_fn: impl FnMut(FieldInfo) -> Result<(), String>, +) -> Result<TokenStream, Vec<syn::Error>> { + let only_skip_rename_and_alias_can_be_set = + variant_attrs.only_skip_rename_and_alias_can_be_set(); + match variant_attrs.with { + With::Serde(path) => { + Ok(quote! { #path::get_schema_in_ctxt(named_schemas, enclosing_namespace) }) + } + With::Expr(Expr::Closure(closure)) => { + if closure.inputs.is_empty() { + Ok(quote! { (#closure)() }) + } else { + Err(vec![syn::Error::new( + variant.span(), + "Expected closure with 0 parameters", + )]) + } + } + With::Expr(Expr::Path(path)) => Ok(quote! { #path(named_schemas, enclosing_namespace) }), + With::Expr(_expr) => Err(vec![syn::Error::new( + variant.span(), + "Invalid expression, expected a function or a closure", + )]), + With::Trait => { + let name = rename_ident( + &variant.ident, + variant_attrs.rename, + rename_all, + RenameRule::apply_to_variant, + ); + + let variant_span = variant.span(); + match variant.fields { + Fields::Named(fields) => { + check_fn(FieldInfo { + variant: FieldVariants::Named(fields.named.len()), + schema_type: SchemaType::Named(&name), + }) + .map_err(|m| vec![syn::Error::new(variant_span, m)])?; + named_fields_to_schema( + &name, + fields, + variant_attrs.rename_all.or(rename_all_fields), + variant_attrs.doc, + &variant_attrs.aliases, + ) + } + Fields::Unnamed(mut fields) if transparent_newtype && fields.unnamed.len() == 1 => { + check_fn(FieldInfo { + variant: FieldVariants::Unnamed(1), + schema_type: SchemaType::Transparent, + }) + .map_err(|m| vec![syn::Error::new(variant_span, m)])?; + let pair = fields.unnamed.pop().expect("There is one field"); + let field = pair.into_value(); + let field_attributes = FieldOptions::new(&field.attrs, field.span())?; Review Comment: ```suggestion let field_attributes = FieldOptions::new(&field.attrs, field.span()) + .and_then(|o| newtype_extra_attribute_checks(o, field.span()))?; ``` ########## avro_derive/tests/enum.rs: ########## @@ -0,0 +1,721 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +use apache_avro::reader::datum::GenericDatumReader; +use apache_avro::writer::datum::GenericDatumWriter; +use apache_avro::{AvroSchema, Schema}; +use pretty_assertions::assert_eq; +use serde::{Deserialize, Serialize, de::DeserializeOwned}; + +/// Takes in a type that implements the right combination of traits and runs it through a Serde Cycle and asserts the result is the same +#[expect( + clippy::needless_pass_by_value, + reason = "Significantly complicates the trait bounds" +)] +#[track_caller] +fn serde_assert<T>(obj: T) +where + T: std::fmt::Debug + Serialize + DeserializeOwned + AvroSchema + PartialEq, +{ + assert_eq!(obj, serde(&obj)); +} + +#[track_caller] +fn serde<T>(obj: &T) -> T +where + T: Serialize + DeserializeOwned + AvroSchema, +{ + de(&ser(obj)) +} + +#[track_caller] +fn ser<T>(obj: &T) -> Vec<u8> +where + T: Serialize + AvroSchema, +{ + let schema = T::get_schema(); + GenericDatumWriter::builder(&schema) + .build() + .unwrap() + .write_ser_to_vec(&obj) + .unwrap() +} + +#[track_caller] +fn de<T>(mut encoded: &[u8]) -> T +where + T: DeserializeOwned + AvroSchema, +{ + assert!(!encoded.is_empty()); + let schema = T::get_schema(); + GenericDatumReader::builder(&schema) + .build() + .unwrap() + .read_deser(&mut encoded) + .unwrap() +} + +#[test] +fn avro_rs_561_bare_union_untagged_empty_tuple_variant() { + #[derive(Debug, Eq, PartialEq, AvroSchema, Serialize, Deserialize)] + #[avro(repr = "bare_union")] + #[serde(untagged)] + enum C { + A(), + } + + serde_assert(C::A()); +} + +#[test] +fn avro_rs_561_bare_union_empty_tuple_variant() { + #[derive(Debug, Eq, PartialEq, AvroSchema, Serialize, Deserialize)] + #[avro(repr = "bare_union")] + enum C { + A(), + B {}, + } + + serde_assert(C::A()); + serde_assert(C::B {}); +} + +#[test] +fn avro_rs_569_enum_repr_default() { + #[derive(AvroSchema, Debug, Serialize, Deserialize, Clone, PartialEq)] + enum Foo { + A, + B, + #[serde(rename = "D")] + C, + } + + let schema = Schema::parse_str( + r#"{ + "type": "enum", + "name": "Foo", + "symbols": ["A", "B", "D"] + }"#, + ) + .unwrap(); + + assert_eq!(Foo::get_schema(), schema); + serde_assert(Foo::A); + serde_assert(Foo::B); + serde_assert(Foo::C); +} + +#[test] +fn avro_rs_569_enum_repr_enum() { + #[derive(AvroSchema, Debug, Serialize, Deserialize, Clone, PartialEq)] + #[avro(repr = "enum")] + enum Foo { + A, + B, + #[serde(rename = "D")] + C, + } + + let schema = Schema::parse_str( + r#"{ + "type": "enum", + "name": "Foo", + "symbols": ["A", "B", "D"] + }"#, + ) + .unwrap(); + + assert_eq!(Foo::get_schema(), schema); + serde_assert(Foo::A); + serde_assert(Foo::B); + serde_assert(Foo::C); +} + +#[test] +fn avro_rs_569_enum_repr_record_tag_content_plain() { + #[derive(AvroSchema, Debug, Serialize, Deserialize, Clone, PartialEq)] + #[avro(repr = "record_tag_content")] + #[serde(tag = "type", content = "value")] + enum Foo { + A, + B, + #[serde(rename = "D")] + C, + } + + let schema = Schema::parse_str( + r#"{ + "type": "record", + "name": "Foo", + "fields": [ + { + "name": "type", + "type": { + "type": "enum", + "name": "type", + "symbols": ["A", "B", "D"] + } + }, + { + "name": "value", + "type": [ + "null" + ] + } + ] + }"#, + ) + .unwrap(); + + assert_eq!(Foo::get_schema(), schema); + serde_assert(Foo::A); + serde_assert(Foo::B); + serde_assert(Foo::C); +} + +#[test] +fn avro_rs_569_enum_repr_record_tag_content_tuple() { + #[derive(AvroSchema, Debug, Serialize, Deserialize, Clone, PartialEq)] + #[avro(repr = "record_tag_content")] + #[serde(tag = "type", content = "value")] + enum Foo { + A, + Alt(), + B(String), + #[serde(rename = "D")] + C( + String, + #[serde(rename = "is_it_true", alias = "is_it_false")] bool, + ), + } + + let schema = Schema::parse_str( + r#"{ + "type": "record", + "name": "Foo", + "fields": [ + { + "name": "type", + "type": { + "type": "enum", + "name": "type", + "symbols": ["A", "Alt", "B", "D"] + } + }, + { + "name": "value", + "type": [ + "null", + { + "type": "record", + "name": "Alt", + "fields": [], + "org.apache.avro.rust.tuple": true + }, + "string", + { + "type": "record", + "name": "D", + "fields": [ + { "name": "field_0", "type": "string" }, + { "name": "is_it_true", "aliases": ["is_it_false"], "type": "boolean" } + ], + "org.apache.avro.rust.tuple": true + } + ] + } + ] + }"#, + ) + .unwrap(); + + assert_eq!(Foo::get_schema(), schema); + serde_assert(Foo::A); Review Comment: ```suggestion serde_assert(Foo::A); serde_assert(Foo::Alt()); ``` ########## avro_derive/src/enums/plain.rs: ########## Review Comment: ```suggestion let renamed = rename_ident( &variant.ident, variant_attrs.rename.clone(), container_attrs.rename_all, RenameRule::apply_to_variant, ); ``` ########## avro_derive/tests/expanded/avro_rs_569_tuple_struct.expanded.rs: ########## @@ -0,0 +1,101 @@ +use apache_avro::AvroSchema; +struct B(i32, String); +#[automatically_derived] +impl ::apache_avro::AvroSchemaComponent for B { + fn get_schema_in_ctxt( + named_schemas: &mut ::std::collections::HashSet<::apache_avro::schema::Name>, + enclosing_namespace: ::apache_avro::schema::NamespaceRef, + ) -> ::apache_avro::schema::Schema { + let name = ::apache_avro::schema::Name::new_with_enclosing_namespace( + "B", + enclosing_namespace, + ) + .expect("Unable to parse `B` as a Name"); + if named_schemas.contains(&name) { + ::apache_avro::schema::Schema::Ref { + name, + } + } else { + let enclosing_namespace = name.namespace(); + named_schemas.insert(name.clone()); + ::apache_avro::schema::Schema::Record( + ::apache_avro::schema::RecordSchema::builder() + .aliases(::std::option::Option::None) + .maybe_doc(::std::option::Option::None) + .fields( + ::alloc::boxed::box_assume_init_into_vec_unsafe( + ::alloc::intrinsics::write_box_via_move( + ::alloc::boxed::Box::new_uninit(), + [ + ::apache_avro::schema::RecordField { + name: "field_0".to_string(), + doc: ::std::option::Option::None, + default: <i32 as ::apache_avro::AvroSchemaComponent>::field_default(), + aliases: ::alloc::vec::Vec::new(), + schema: <i32 as ::apache_avro::AvroSchemaComponent>::get_schema_in_ctxt( + named_schemas, + enclosing_namespace, + ), + custom_attributes: ::std::collections::BTreeMap::new(), + }, + ::apache_avro::schema::RecordField { + name: "field_1".to_string(), + doc: ::std::option::Option::None, + default: <String as ::apache_avro::AvroSchemaComponent>::field_default(), + aliases: ::alloc::vec::Vec::new(), + schema: <String as ::apache_avro::AvroSchemaComponent>::get_schema_in_ctxt( + named_schemas, + enclosing_namespace, + ), + custom_attributes: ::std::collections::BTreeMap::new(), + }, + ], + ), + ), + ) Review Comment: ```suggestion ) .attributes( [( "org.apache.avro.rust.tuple".to_string(), ::serde_json::value::Value::Bool(true), )] .into(), ) ``` ########## avro_derive/tests/ui/avro_rs_561_transparent_enum.rs: ########## @@ -0,0 +1,26 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +use apache_avro::AvroSchema; + +#[derive(AvroSchema)] +#[serde(transparent)] +enum D { + A(A) Review Comment: What is the inner `A` here ? ```suggestion A(i32) ``` ? ########## avro_derive/tests/expanded/mod.rs: ########## @@ -27,3 +27,9 @@ mod avro_rs_501_basic; mod avro_rs_501_namespace; mod avro_rs_501_reference; mod avro_rs_501_struct_with_optional; +mod avro_rs_569_non_basic_enum; +mod avro_rs_569_rename_all_fields; +mod avro_rs_569_tag_content_enum; +mod avro_rs_569_tag_enum; +mod avro_rs_569_tag_struct; Review Comment: ```suggestion mod avro_rs_569_tag_struct; mod avro_rs_569_tuple_struct; ``` ########## avro_derive/tests/ui/avro_rs_569_untagged_enum_multiple_null.stderr: ########## @@ -0,0 +1,5 @@ +error: AvroSchema: Two variants resolve to Schema::Null, this is not supported for `#[avro(repr = "bare_union")] Review Comment: ```suggestion error: AvroSchema: Two variants resolve to Schema::Null, this is not supported for `#[avro(repr = "bare_union")]` ``` ########## avro_derive/src/enums/bare_union.rs: ########## @@ -0,0 +1,107 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +use crate::attributes::{NamedTypeOptions, Repr, VariantOptions}; +use crate::enums::{FieldVariants, SchemaType, variant_to_schema_expr}; +use crate::implementation::Implementation; +use crate::utils::json_value_expr; +use proc_macro2::Ident; +use quote::quote; +use std::collections::HashSet; +use syn::spanned::Spanned; +use syn::{DataEnum, Generics}; + +pub fn to_implementation( + ident: Ident, + generics: Generics, + container_attrs: NamedTypeOptions, + data: DataEnum, +) -> Result<Implementation, Vec<syn::Error>> { + let Some(Repr::BareUnion { untagged }) = container_attrs.repr else { + unreachable!() + }; + + let mut errors = Vec::new(); + let mut variant_exprs = Vec::new(); + + // Used for checking that the resulting schema is usefull + let mut have_null = false; + let mut names = HashSet::new(); + let mut tuple_sizes = HashSet::new(); + + for variant in data.variants { + let variant_span = variant.span(); + let variant_attrs = match VariantOptions::new(&variant.attrs, variant_span) { + Ok(attrs) => attrs, + Err(errs) => { + errors.extend(errs); + continue; + } + }; + + if variant_attrs.skip { + continue; + } + match variant_to_schema_expr( + variant, + variant_attrs, + container_attrs.rename_all, + container_attrs.rename_all_fields, + true, + true, + |info| { + match (info.schema_type, info.variant) { + (_, FieldVariants::Named(0)) if untagged => { + Err("AvroSchema: Empty struct variants are not allowed for `#[serde(untagged)]`".to_string()) + } + (_, FieldVariants::Unnamed(len)) if untagged && len >= 2 && !tuple_sizes.insert(len) => { Review Comment: Why `len >= 2` ? If you have an enum with two variants: `A(u32)` and `B(u32)` then both of them will serialize to Avro Int schemas. ########## avro_derive/src/enums/mod.rs: ########## @@ -37,23 +45,223 @@ pub fn to_implementation( "AvroSchema: `#[serde(transparent)]` is only supported on structs", )]); } - if data - .variants - .iter() - .filter(|v| { - // Filter skipped variants, if the attributes fail to parse we'll also filter them out - // `plain` will throw proper errors for that. - VariantOptions::new(&v.attrs, v.span()) - .map(|o| !o.skip) - .unwrap_or(false) - }) - .all(|v| Fields::Unit == v.fields) - { - plain::to_implementation(input_span, ident, generics, container_attrs, data) - } else { - Err(vec![syn::Error::new( + + match &container_attrs.repr { + None => { + if data + .variants + .iter() + .filter(|v| { + // Filter skipped variants, if the attributes fail to parse we'll also filter them out + // `plain` will throw proper errors for that. + VariantOptions::new(&v.attrs, v.span()) + .map(|o| !o.skip) + .unwrap_or(false) + }) + .all(|v| Fields::Unit == v.fields) + { + plain::to_implementation(input_span, ident, generics, container_attrs, data) + } else { + union_of_records::to_implementation(ident, generics, container_attrs, data) + } + } + Some(Repr::Enum) => { + plain::to_implementation(input_span, ident, generics, container_attrs, data) + } + Some(Repr::BareUnion { .. }) => { + bare_union::to_implementation(ident, generics, container_attrs, data) + } + Some(Repr::UnionOfRecords) => { + union_of_records::to_implementation(ident, generics, container_attrs, data) + } + Some(Repr::RecordTagContent { .. }) => record_tag_content::to_implementation( input_span, - "AvroSchema: derive does not work for enums with non unit structs", - )]) + ident, + generics, + container_attrs, + data, + ), + Some(Repr::RecordInternallyTagged { .. }) => { + record_internally_tagged::to_implementation(ident, generics, container_attrs, data) + } + } +} + +fn newtype_extra_attribute_checks( + options: FieldOptions, + span: Span, +) -> Result<FieldOptions, Vec<syn::Error>> { + let mut errors = Vec::new(); + if options.doc.is_some() { + errors.push(syn::Error::new( + span, + r#"AvroSchema: `#[avro(doc = "..")]` only works on newtype variants when the enum uses `#[avro(repr = "union_of_records")`"# + )); + } + if !matches!(options.default, FieldDefault::Trait) { + errors.push(syn::Error::new( + span, + r#"AvroSchema: `#[avro(default = ..)]` only works on newtype variants when the enum uses `#[avro(repr = "union_of_records")`"# + )); + } + if !options.alias.is_empty() { + errors.push(syn::Error::new( + span, + r#"AvroSchema: `#[avro(alias = "..")]` only works on newtype variants when the enum uses `#[avro(repr = "union_of_records")`"# + )); } + if options.rename.is_some() { + errors.push(syn::Error::new( + span, + r#"AvroSchema: `#[avro(alias = "..")]` only works on newtype variants when the enum uses `#[avro(repr = "union_of_records")`"# Review Comment: ```suggestion r#"AvroSchema: `#[avro(rename = "..")]` only works on newtype variants when the enum uses `#[avro(repr = "union_of_records")`"# ``` ########## avro_derive/src/enums/bare_union.rs: ########## @@ -0,0 +1,107 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +use crate::attributes::{NamedTypeOptions, Repr, VariantOptions}; +use crate::enums::{FieldVariants, SchemaType, variant_to_schema_expr}; +use crate::implementation::Implementation; +use crate::utils::json_value_expr; +use proc_macro2::Ident; +use quote::quote; +use std::collections::HashSet; +use syn::spanned::Spanned; +use syn::{DataEnum, Generics}; + +pub fn to_implementation( + ident: Ident, + generics: Generics, + container_attrs: NamedTypeOptions, + data: DataEnum, +) -> Result<Implementation, Vec<syn::Error>> { + let Some(Repr::BareUnion { untagged }) = container_attrs.repr else { + unreachable!() + }; + + let mut errors = Vec::new(); + let mut variant_exprs = Vec::new(); + + // Used for checking that the resulting schema is usefull + let mut have_null = false; + let mut names = HashSet::new(); + let mut tuple_sizes = HashSet::new(); + + for variant in data.variants { + let variant_span = variant.span(); + let variant_attrs = match VariantOptions::new(&variant.attrs, variant_span) { + Ok(attrs) => attrs, + Err(errs) => { + errors.extend(errs); + continue; + } + }; + + if variant_attrs.skip { + continue; + } + match variant_to_schema_expr( + variant, + variant_attrs, + container_attrs.rename_all, + container_attrs.rename_all_fields, + true, + true, + |info| { + match (info.schema_type, info.variant) { + (_, FieldVariants::Named(0)) if untagged => { + Err("AvroSchema: Empty struct variants are not allowed for `#[serde(untagged)]`".to_string()) + } + (_, FieldVariants::Unnamed(len)) if untagged && len >= 2 && !tuple_sizes.insert(len) => { + Err(format!("AvroSchema: Duplicate tuple sizes detected which is incompatible with `#[serde(untagged)]`: new: {len}, already seen: {tuple_sizes:?}")) + } + (SchemaType::Null, _) if have_null => Err(r#"AvroSchema: Two variants resolve to Schema::Null, this is not supported for `#[avro(repr = "bare_union")]"#.to_string()), Review Comment: ```suggestion (SchemaType::Null, _) if have_null => Err(r#"AvroSchema: Two variants resolve to Schema::Null, this is not supported for `#[avro(repr = "bare_union")]`"#.to_string()), ``` -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
