Kriskras99 commented on code in PR #394: URL: https://github.com/apache/avro-rs/pull/394#discussion_r2685203145
########## avro/tests/avro_schema_component.rs: ########## @@ -0,0 +1,42 @@ +// 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. + +#[test] +fn avro_rs_394_avro_schema_component_without_derive_feature() { + use apache_avro::{AvroSchemaComponent, Schema}; + use std::collections::HashMap; + + // Verify basic types work without derive feature + let schema = i32::get_schema_in_ctxt(&mut HashMap::default(), &None); + assert!(matches!(schema, Schema::Int)); +} + +#[test] +fn avro_rs_394_avro_schema_component_nested_options() { + use apache_avro::{AvroSchemaComponent, Schema}; + use std::collections::HashMap; + + type VeryOptional = Option<Option<Option<Option<Option<i32>>>>>; + + let schema = VeryOptional::get_schema_in_ctxt(&mut HashMap::default(), &None); + match schema { + Schema::Union(union_schema) => { + assert_eq!(union_schema.variants(), &[Schema::Null, Schema::Int]); + } + _ => panic!("Unexpected schema type: {schema}"), + }; +} Review Comment: ```suggestion #[test] #[should_panic(expected = "Option<T> must produce a valid (non-nested) union")] fn avro_rs_394_avro_schema_component_nested_options() { use apache_avro::{AvroSchemaComponent, Schema}; use std::collections::HashMap; type VeryOptional = Option<Option<Option<Option<Option<i32>>>>>; let _schema = VeryOptional::get_schema_in_ctxt(&mut HashMap::default(), &None); } ``` ########## avro/src/schema.rs: ########## @@ -2568,215 +2568,195 @@ fn field_ordering_position(field: &str) -> Option<usize> { /// Trait for types that serve as an Avro data model. Derive implementation available /// through `derive` feature. Do not implement directly! -/// Implement `apache_avro::schema::derive::AvroSchemaComponent` to get this trait +/// Implement [AvroSchemaComponent] to get this trait /// through a blanket implementation. pub trait AvroSchema { fn get_schema() -> Schema; } -#[cfg(feature = "derive")] -pub mod derive { - use super::*; - use std::borrow::Cow; +/// Trait for types that serve as fully defined components inside an Avro data model. Derive +/// implementation available through `derive` feature. This is what is implemented by +/// the `derive(AvroSchema)` macro. +/// +/// # Implementation guide +/// +/// ### Simple implementation +/// To construct a non named simple schema, it is possible to ignore the input argument making the +/// general form implementation look like +/// ```ignore +/// impl AvroSchemaComponent for AType { +/// fn get_schema_in_ctxt(_: &mut Names, _: &Namespace) -> Schema { +/// Schema::? +/// } +///} +/// ``` +/// ### Passthrough implementation +/// +/// To construct a schema for a Type that acts as in "inner" type, such as for smart pointers, simply +/// pass through the arguments to the inner type +/// ```ignore +/// impl AvroSchemaComponent for PassthroughType { +/// fn get_schema_in_ctxt(named_schemas: &mut Names, enclosing_namespace: &Namespace) -> Schema { +/// InnerType::get_schema_in_ctxt(named_schemas, enclosing_namespace) +/// } +///} +/// ``` +///### Complex implementation +/// To implement this for Named schema there is a general form needed to avoid creating invalid +/// schemas or infinite loops. +/// ```ignore +/// impl AvroSchemaComponent for ComplexType { +/// fn get_schema_in_ctxt(named_schemas: &mut Names, enclosing_namespace: &Namespace) -> Schema { +/// // Create the fully qualified name for your type given the enclosing namespace +/// let name = apache_avro::schema::Name::new("MyName") +/// .expect("Unable to parse schema name") +/// .fully_qualified_name(enclosing_namespace); +/// let enclosing_namespace = &name.namespace; +/// // Check, if your name is already defined, and if so, return a ref to that name +/// if named_schemas.contains_key(&name) { +/// apache_avro::schema::Schema::Ref{name: name.clone()} +/// } else { +/// named_schemas.insert(name.clone(), apache_avro::schema::Schema::Ref{name: name.clone()}); +/// // YOUR SCHEMA DEFINITION HERE with the name equivalent to "MyName". +/// // For non-simple sub types delegate to their implementation of AvroSchemaComponent +/// } +/// } +///} +/// ``` +pub trait AvroSchemaComponent { + fn get_schema_in_ctxt(named_schemas: &mut Names, enclosing_namespace: &Namespace) -> Schema; +} - /// Trait for types that serve as fully defined components inside an Avro data model. Derive - /// implementation available through `derive` feature. This is what is implemented by - /// the `derive(AvroSchema)` macro. - /// - /// # Implementation guide - /// - ///### Simple implementation - /// To construct a non named simple schema, it is possible to ignore the input argument making the - /// general form implementation look like - /// ```ignore - /// impl AvroSchemaComponent for AType { - /// fn get_schema_in_ctxt(_: &mut Names, _: &Namespace) -> Schema { - /// Schema::? - /// } - ///} - /// ``` - /// ### Passthrough implementation - /// To construct a schema for a Type that acts as in "inner" type, such as for smart pointers, simply - /// pass through the arguments to the inner type - /// ```ignore - /// impl AvroSchemaComponent for PassthroughType { - /// fn get_schema_in_ctxt(named_schemas: &mut Names, enclosing_namespace: &Namespace) -> Schema { - /// InnerType::get_schema_in_ctxt(names, enclosing_namespace) - /// } - ///} - /// ``` - ///### Complex implementation - /// To implement this for Named schema there is a general form needed to avoid creating invalid - /// schemas or infinite loops. - /// ```ignore - /// impl AvroSchemaComponent for ComplexType { - /// fn get_schema_in_ctxt(named_schemas: &mut Names, enclosing_namespace: &Namespace) -> Schema { - /// // Create the fully qualified name for your type given the enclosing namespace - /// let name = apache_avro::schema::Name::new("MyName") - /// .expect("Unable to parse schema name") - /// .fully_qualified_name(enclosing_namespace); - /// let enclosing_namespace = &name.namespace; - /// // Check, if your name is already defined, and if so, return a ref to that name - /// if named_schemas.contains_key(&name) { - /// apache_avro::schema::Schema::Ref{name: name.clone()} - /// } else { - /// named_schemas.insert(name.clone(), apache_avro::schema::Schema::Ref{name: name.clone()}); - /// // YOUR SCHEMA DEFINITION HERE with the name equivalent to "MyName". - /// // For non-simple sub types delegate to their implementation of AvroSchemaComponent - /// } - /// } - ///} - /// ``` - pub trait AvroSchemaComponent { - fn get_schema_in_ctxt(named_schemas: &mut Names, enclosing_namespace: &Namespace) - -> Schema; +impl<T> AvroSchema for T +where + T: AvroSchemaComponent, +{ + fn get_schema() -> Schema { + T::get_schema_in_ctxt(&mut HashMap::default(), &None) } +} - impl<T> AvroSchema for T - where - T: AvroSchemaComponent, - { - fn get_schema() -> Schema { - T::get_schema_in_ctxt(&mut HashMap::default(), &None) +macro_rules! impl_schema ( + ($type:ty, $variant_constructor:expr) => ( + impl AvroSchemaComponent for $type { + fn get_schema_in_ctxt(_: &mut Names, _: &Namespace) -> Schema { + $variant_constructor + } } + ); +); + +impl_schema!(bool, Schema::Boolean); +impl_schema!(i8, Schema::Int); +impl_schema!(i16, Schema::Int); +impl_schema!(i32, Schema::Int); +impl_schema!(i64, Schema::Long); +impl_schema!(u8, Schema::Int); +impl_schema!(u16, Schema::Int); +impl_schema!(u32, Schema::Long); +impl_schema!(f32, Schema::Float); +impl_schema!(f64, Schema::Double); +impl_schema!(String, Schema::String); +impl_schema!(uuid::Uuid, Schema::Uuid(UuidSchema::String)); + +impl<T> AvroSchemaComponent for Vec<T> +where + T: AvroSchemaComponent, +{ + fn get_schema_in_ctxt(named_schemas: &mut Names, enclosing_namespace: &Namespace) -> Schema { + Schema::array(T::get_schema_in_ctxt(named_schemas, enclosing_namespace)) } +} + +impl<T> AvroSchemaComponent for Option<T> +where + T: AvroSchemaComponent, +{ + fn get_schema_in_ctxt(named_schemas: &mut Names, enclosing_namespace: &Namespace) -> Schema { + let inner_schema = T::get_schema_in_ctxt(named_schemas, enclosing_namespace); - macro_rules! impl_schema ( - ($type:ty, $variant_constructor:expr) => ( - impl AvroSchemaComponent for $type { - fn get_schema_in_ctxt(_: &mut Names, _: &Namespace) -> Schema { - $variant_constructor + let mut variants = vec![Schema::Null]; + match inner_schema { + Schema::Union(union_schema) => { + // Flatten and avoid repeating `null`. + for schema in union_schema.schemas { + if !matches!(schema, Schema::Null) { + variants.push(schema); + } } } - ); - ); - - impl_schema!(bool, Schema::Boolean); - impl_schema!(i8, Schema::Int); - impl_schema!(i16, Schema::Int); - impl_schema!(i32, Schema::Int); - impl_schema!(i64, Schema::Long); - impl_schema!(u8, Schema::Int); - impl_schema!(u16, Schema::Int); - impl_schema!(u32, Schema::Long); - impl_schema!(f32, Schema::Float); - impl_schema!(f64, Schema::Double); - impl_schema!(String, Schema::String); - impl_schema!(uuid::Uuid, Schema::Uuid(UuidSchema::String)); - - impl<T> AvroSchemaComponent for Vec<T> - where - T: AvroSchemaComponent, - { - fn get_schema_in_ctxt( - named_schemas: &mut Names, - enclosing_namespace: &Namespace, - ) -> Schema { - Schema::array(T::get_schema_in_ctxt(named_schemas, enclosing_namespace)) + Schema::Null => { /* keep just null */ } + other => variants.push(other), Review Comment: ```suggestion let variants = vec![Schema::Null, inner_schema]; // TODO: Fallback to a different enum representation if T::get_schema_in_ctxt() is a union ``` This transformation is wrong. `Some(Some(Some(None)))` will now deserialize as `None`. I think it would be better to fallback to one of the enum representations I described in https://github.com/apache/avro-rs/issues/365#issuecomment-3730627801, but that should be done in the PR implementing proper enum support. I'll start working on that. ########## avro/src/schema.rs: ########## @@ -2568,215 +2568,195 @@ fn field_ordering_position(field: &str) -> Option<usize> { /// Trait for types that serve as an Avro data model. Derive implementation available /// through `derive` feature. Do not implement directly! -/// Implement `apache_avro::schema::derive::AvroSchemaComponent` to get this trait +/// Implement [AvroSchemaComponent] to get this trait Review Comment: ```suggestion /// Implement [`AvroSchemaComponent`] to get this trait ``` -- 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]
