martin-g commented on code in PR #397:
URL: https://github.com/apache/avro-rs/pull/397#discussion_r2681673473
##########
avro/src/schema.rs:
##########
@@ -2765,17 +2767,49 @@ pub mod derive {
name: "duration".to_string(),
namespace: enclosing_namespace.clone(),
};
- named_schemas
- .entry(name.clone())
- .or_insert(Schema::Duration(FixedSchema {
- name,
+ if named_schemas.contains_key(&name) {
+ Schema::Ref { name }
+ } else {
+ let schema = Schema::Duration(FixedSchema {
+ name: name.clone(),
aliases: None,
doc: None,
size: 12,
default: None,
attributes: Default::default(),
- }))
- .clone()
+ });
+ named_schemas.insert(name.clone(), schema.clone());
+ schema
+ }
+ }
+ }
+
+ impl AvroSchemaComponent for uuid::Uuid {
+ /// The schema is [`Schema::Uuid`] with the name `uuid`.
+ ///
+ /// The underlying schema is [`Schema::Fixed`] with a size of 16.
+ fn get_schema_in_ctxt(
+ named_schemas: &mut Names,
+ enclosing_namespace: &Namespace,
+ ) -> Schema {
+ let name = Name {
+ name: "uuid".to_string(),
Review Comment:
I have a bad feeling about the hardcoded names we use here: `"uuid"` and
`"duration"` above.
They may clash with user-defined schemas with the same names ...
I have no better solution than using some name prefix, e.g.
"avro_derived_uuid"
##########
avro_derive/src/attributes/avro.rs:
##########
@@ -137,6 +154,16 @@ pub struct FieldAttributes {
/// [`serde::FieldAttributes::flatten`]:
crate::attributes::serde::FieldAttributes::flatten
#[darling(default)]
pub flatten: bool,
+ /// How to get the schema for this field.
+ ///
+ /// By default uses `<T as AvroSchemaComponent>::get_schema_in_ctxt`.
+ ///
+ /// When it's provided without an argument (`#[avro(with)]`), it will use
the function `get_schema_in_ctxt` defined
+ /// in the same module as the `#[serde(with = "..")]` attribute.
Review Comment:
This is not very clear to me. It sounds like it will use the serde
attribute's module.
##########
avro_derive/src/attributes/mod.rs:
##########
@@ -145,13 +146,50 @@ impl VariantOptions {
}
}
+/// How to get the schema for this field or variant.
+#[derive(Debug, PartialEq, Default)]
+pub enum With {
+ /// Use `<T as AvroSchemaComponent>::get_schema_with_ctxt`.
+ #[default]
+ Trait,
+ /// Use `module::get_schema_with_ctxt` where the module is defined by
Serde's `with` attribute.
+ Serde(Path),
+ /// Call the function in this expression.
+ Expr(Expr),
+}
+
+impl With {
+ fn from_avro_and_serde(
+ avro: &avro::With,
+ serde: &Option<String>,
+ span: Span,
+ ) -> Result<Self, syn::Error> {
+ match &avro {
+ avro::With::Trait => Ok(Self::Trait),
+ avro::With::Serde => {
+ if let Some(serde) = serde {
+ let path = Path::from_string(serde).unwrap();
+ Ok(Self::Serde(path))
+ } else {
+ Err(syn::Error::new(
+ span,
+ "`#[avro(with)]` requires `#[serde(with = \"..\")]` or
provide a function to call `#[avro(width = ..)]`",
Review Comment:
```suggestion
"`#[avro(with)]` requires `#[serde(with = \"..\")]`
or provide a function to call `#[avro(with = ..)]`",
```
##########
avro_derive/src/attributes/avro.rs:
##########
@@ -137,6 +154,16 @@ pub struct FieldAttributes {
/// [`serde::FieldAttributes::flatten`]:
crate::attributes::serde::FieldAttributes::flatten
#[darling(default)]
pub flatten: bool,
+ /// How to get the schema for this field.
Review Comment:
```suggestion
/// How to get the schema for a field.
```
##########
avro_derive/src/attributes/avro.rs:
##########
@@ -97,6 +99,21 @@ impl VariantAttributes {
}
}
+/// How to get the schema for this field.
+#[derive(Debug, FromMeta, PartialEq, Default)]
+#[darling(from_expr = |expr| Ok(With::Expr(expr.clone())))]
+pub enum With {
+ /// Use `<T as AvroSchemaComponent>::get_schema_with_ctxt`.
Review Comment:
```suggestion
/// Use `<T as AvroSchemaComponent>::get_schema_in_ctxt`.
```
##########
avro/src/schema.rs:
##########
Review Comment:
We should add Schema::Duration to
https://github.com/apache/avro-rs/blob/f3dd04f6db7d2604fdf87d8a003dced94d8b66ea/avro/src/schema.rs#L592
and treat it as a named schema. It is a bug from an earlier PR.
##########
avro_derive/src/attributes/avro.rs:
##########
@@ -137,6 +154,16 @@ pub struct FieldAttributes {
/// [`serde::FieldAttributes::flatten`]:
crate::attributes::serde::FieldAttributes::flatten
#[darling(default)]
pub flatten: bool,
+ /// How to get the schema for this field.
+ ///
+ /// By default uses `<T as AvroSchemaComponent>::get_schema_in_ctxt`.
+ ///
+ /// When it's provided without an argument (`#[avro(with)]`), it will use
the function `get_schema_in_ctxt` defined
+ /// in the same module as the `#[serde(with = "..")]` attribute.
+ ///
+ /// When it's provided with an argument (`#[avro(with = ..)]`), it will
use that function.
Review Comment:
Let's try to make the documentation clear enough without assuming the reader
knows what `#[serde(with)]` is and how to use it.
I think it would be better to use:
```suggestion
/// When it's provided with an argument (`#[avro(with = some_fn)]`), it
will use that function.
```
Currently with `..` I have to guess that it is the referenced function, and
my second guess would be how to refer to it:
- as a string without a parenthesis: `with = "some_function"`
- as a string with parenthesis: `with = "some_function()"`
- or as an expression: `with = some_function`
##########
avro_derive/src/attributes/mod.rs:
##########
@@ -145,13 +146,50 @@ impl VariantOptions {
}
}
+/// How to get the schema for this field or variant.
+#[derive(Debug, PartialEq, Default)]
+pub enum With {
+ /// Use `<T as AvroSchemaComponent>::get_schema_with_ctxt`.
Review Comment:
```suggestion
/// Use `<T as AvroSchemaComponent>::get_schema_in_ctxt`.
```
##########
avro_derive/tests/ui/avro_rs_396_with_expr_type.rs:
##########
@@ -0,0 +1,27 @@
+// 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, Schema};
+
+#[derive(AvroSchema)]
+struct Foo {
+ #[avro(with = Schema::Bytes)]
Review Comment:
Let's add one more test with:
```rust
#[avro(with = || Schema::Bytes)]
```
Would inline closure like this work ?!
##########
avro_derive/src/attributes/avro.rs:
##########
@@ -97,6 +99,21 @@ impl VariantAttributes {
}
}
+/// How to get the schema for this field.
Review Comment:
```suggestion
/// How to get the schema for a field.
```
##########
avro_derive/src/attributes/mod.rs:
##########
@@ -145,13 +146,50 @@ impl VariantOptions {
}
}
+/// How to get the schema for this field or variant.
+#[derive(Debug, PartialEq, Default)]
+pub enum With {
+ /// Use `<T as AvroSchemaComponent>::get_schema_with_ctxt`.
+ #[default]
+ Trait,
+ /// Use `module::get_schema_with_ctxt` where the module is defined by
Serde's `with` attribute.
+ Serde(Path),
+ /// Call the function in this expression.
+ Expr(Expr),
+}
+
+impl With {
+ fn from_avro_and_serde(
+ avro: &avro::With,
+ serde: &Option<String>,
+ span: Span,
+ ) -> Result<Self, syn::Error> {
+ match &avro {
+ avro::With::Trait => Ok(Self::Trait),
+ avro::With::Serde => {
+ if let Some(serde) = serde {
+ let path = Path::from_string(serde).unwrap();
Review Comment:
Let's return an Err instead of `.unwrap()`-ing here.
##########
avro_derive/src/attributes/avro.rs:
##########
@@ -97,6 +99,21 @@ impl VariantAttributes {
}
}
+/// How to get the schema for this field.
+#[derive(Debug, FromMeta, PartialEq, Default)]
+#[darling(from_expr = |expr| Ok(With::Expr(expr.clone())))]
+pub enum With {
+ /// Use `<T as AvroSchemaComponent>::get_schema_with_ctxt`.
+ #[default]
+ #[darling(skip)]
+ Trait,
+ /// Use `module::get_schema_with_ctxt` where the module is defined by
Serde's `with` attribute.
Review Comment:
```suggestion
/// Use `module::get_schema_in_ctxt` where the module is defined by
Serde's `with` attribute.
```
##########
avro_derive/src/attributes/mod.rs:
##########
@@ -145,13 +146,50 @@ impl VariantOptions {
}
}
+/// How to get the schema for this field or variant.
+#[derive(Debug, PartialEq, Default)]
+pub enum With {
+ /// Use `<T as AvroSchemaComponent>::get_schema_with_ctxt`.
+ #[default]
+ Trait,
+ /// Use `module::get_schema_with_ctxt` where the module is defined by
Serde's `with` attribute.
Review Comment:
```suggestion
/// Use `module::get_schema_in_ctxt` where the module is defined by
Serde's `with` attribute.
```
--
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]
