martin-g commented on code in PR #477:
URL: https://github.com/apache/avro-rs/pull/477#discussion_r2832814140
##########
avro/src/serde/derive.rs:
##########
@@ -562,6 +614,13 @@ where
) -> Option<Vec<RecordField>> {
None
}
+
+ /// If `T` has a field default, this will return an array of with that
default. Otherwise there is no default.
+ fn field_default() -> Option<serde_json::Value> {
+ T::field_default().map(|default| {
+ serde_json::Value::Array(std::array::from_fn::<_, N, _>(|_|
default.clone()).to_vec())
Review Comment:
```suggestion
serde_json::Value::Array(vec![default; N])
```
Does this work ?
Or it needs to call `.clone()` explicitly ?
##########
avro/src/serde/derive.rs:
##########
@@ -562,6 +614,13 @@ where
) -> Option<Vec<RecordField>> {
None
}
+
+ /// If `T` has a field default, this will return an array of with that
default. Otherwise there is no default.
Review Comment:
```suggestion
/// If `T` has a field default, this will return an array of elements
with that default. Otherwise there is no default.
```
##########
avro/src/serde/derive.rs:
##########
@@ -113,11 +121,14 @@ use std::collections::{HashMap, HashSet};
///
/// Set the `doc` attribute of the field. Defaults to the documentation of
the field.
///
-/// - `#[avro(default = "null")]`
+/// - `#[avro(default = "null")]` or `#[avro(default = false)]`
///
-/// Set the `default` attribute of the field.
+/// Control the `default` attribute of the field. When not used, it will
use [`AvroSchemaComponent::field_default`]
+/// to get the default value for a type. This default value can be
overriden by providing a JSON string.
+/// To remove the `default` attribute for a field, set `default` to `false`.
Review Comment:
nit: What if one needs to set the default to `false` explicitly ?
Can we use some marker struct/constant instead ?
##########
avro/src/serde/derive.rs:
##########
@@ -479,6 +515,10 @@ where
macro_rules! impl_schema (
($type:ty, $variant_constructor:expr) => (
+ impl_schema!($type, $variant_constructor, <$type as
Default>::default());
Review Comment:
Is there a reason to use `Default::default()` by default ?
IMO the default should be `None` and only the app developer should
explicitly sets non-None defaults.
##########
avro_derive/src/attributes/mod.rs:
##########
@@ -116,12 +118,29 @@ impl NamedTypeOptions {
let doc = avro.doc.or_else(|| extract_rustdoc(attributes));
+ let default = match avro.default {
+ None => quote! { None },
+ Some(default_value) => {
+ let _: serde_json::Value =
+ serde_json::from_str(&default_value[..]).map_err(|e| {
+ vec![syn::Error::new(
+ ident.span(),
+ format!("Invalid avro default json: \n{e}"),
+ )]
+ })?;
+ quote! {
+
Some(serde_json::from_str(#default_value).expect(format!("Invalid JSON: {:?}",
#default_value).as_str()))
Review Comment:
```suggestion
Some(#json)
```
This does not work ?!
##########
avro/src/serde/derive.rs:
##########
@@ -81,6 +85,10 @@ use std::collections::{HashMap, HashSet};
///
/// Set the `doc` attribute of the schema. Defaults to the documentation of
the type.
///
+/// - `#[avro(default = r#"{"field": 42, "other": "Spam"}"#)]`
Review Comment:
I am not sure this is a good idea.
Using such container level attribute with stringified map looks fragile. If
a field is renamed then its string key here may become obsolete/stale.
The field level attribute looks better to me. More type safe.
##########
avro_derive/src/attributes/mod.rs:
##########
@@ -116,12 +118,29 @@ impl NamedTypeOptions {
let doc = avro.doc.or_else(|| extract_rustdoc(attributes));
+ let default = match avro.default {
+ None => quote! { None },
+ Some(default_value) => {
+ let _: serde_json::Value =
+ serde_json::from_str(&default_value[..]).map_err(|e| {
+ vec![syn::Error::new(
+ ident.span(),
+ format!("Invalid avro default json: \n{e}"),
Review Comment:
```suggestion
format!("Invalid Avro `default` JSON: \n{e}"),
```
##########
avro_derive/src/attributes/mod.rs:
##########
@@ -116,12 +118,29 @@ impl NamedTypeOptions {
let doc = avro.doc.or_else(|| extract_rustdoc(attributes));
+ let default = match avro.default {
+ None => quote! { None },
+ Some(default_value) => {
+ let _: serde_json::Value =
Review Comment:
```suggestion
let json: serde_json::Value =
```
##########
avro_derive/src/attributes/mod.rs:
##########
@@ -116,12 +118,29 @@ impl NamedTypeOptions {
let doc = avro.doc.or_else(|| extract_rustdoc(attributes));
+ let default = match avro.default {
+ None => quote! { None },
+ Some(default_value) => {
+ let _: serde_json::Value =
+ serde_json::from_str(&default_value[..]).map_err(|e| {
+ vec![syn::Error::new(
+ ident.span(),
+ format!("Invalid avro default json: \n{e}"),
+ )]
+ })?;
+ quote! {
+
Some(serde_json::from_str(#default_value).expect(format!("Invalid JSON: {:?}",
#default_value).as_str()))
Review Comment:
Why the second parsing is needed here ?
##########
avro_derive/tests/derive.rs:
##########
@@ -2377,3 +2378,203 @@ fn avro_rs_448_flatten_field_positions() {
.collect::<Vec<_>>();
assert_eq!(positions.as_slice(), &[0, 1, 2, 3][..]);
}
+
+#[test]
+fn avro_rs_476_field_default() {
+ #[derive(AvroSchema)]
+ struct Bar {
+ _field: Box<Bar>,
+ }
+
+ #[derive(AvroSchema)]
+ #[avro(default = r#"{"_field": true}"#)]
+ struct Spam {
+ _field: bool,
+ }
+
+ #[derive(AvroSchema)]
+ struct Foo {
+ _a: bool,
+ _b: i8,
+ _c: i16,
+ _d: i32,
+ _e: i64,
+ _f: u8,
+ _g: u16,
+ _h: u32,
+ _i: f32,
+ _j: f64,
+ _k: String,
+ _l: Box<str>,
+ _m: char,
+ _n: Box<Spam>,
+ _o: Vec<bool>,
+ _p: [u8; 5],
+ _p_alt: [Bar; 5],
+ _q: HashMap<String, String>,
+ _r: Option<f64>,
+ _s: Duration,
+ _t: Uuid,
+ _u: u64,
+ _v: u128,
+ _w: i128,
+ _x: Bar,
+ }
+
+ let schema = Foo::get_schema();
+ assert_eq!(
+ serde_json::to_string(&schema).unwrap(),
+
r#"{"type":"record","name":"Foo","fields":[{"name":"_a","type":"boolean","default":false},{"name":"_b","type":"int","default":0},{"name":"_c","type":"int","default":0},{"name":"_d","type":"int","default":0},{"name":"_e","type":"long","default":0},{"name":"_f","type":"int","default":0},{"name":"_g","type":"int","default":0},{"name":"_h","type":"long","default":0},{"name":"_i","type":"float","default":0.0},{"name":"_j","type":"double","default":0.0},{"name":"_k","type":"string","default":""},{"name":"_l","type":"string","default":""},{"name":"_m","type":"string","default":"\u0000"},{"name":"_n","type":{"type":"record","name":"Spam","fields":[{"name":"_field","type":"boolean","default":false}]},"default":{"_field":true}},{"name":"_o","type":{"type":"array","items":"boolean"},"default":[]},{"name":"_p","type":{"type":"array","items":"int"},"default":[0,0,0,0,0]},{"name":"_p_alt","type":{"type":"array","items":{"type":"record","name":"Bar","fields":[{"name":"_field","type":"Bar"}
]}}},{"name":"_q","type":{"type":"map","values":"string"},"default":{}},{"name":"_r","type":["null","double"],"default":null},{"name":"_s","type":{"type":"fixed","name":"duration","size":12,"logicalType":"duration"},"default":"\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000"},{"name":"_t","type":{"type":"fixed","name":"uuid","size":16,"logicalType":"uuid"},"default":"\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000"},{"name":"_u","type":{"type":"fixed","name":"u64","size":8},"default":"\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000"},{"name":"_v","type":{"type":"fixed","name":"u128","size":16},"default":"\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000"},{"name":"_w","type":{"type":"fixed","name":"i128","size":16},"default":"\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000"},{"name":"_x","type":"Bar"}]}"#
+ );
+}
+
+#[test]
+fn avro_rs_476_field_default_false() {
+ #[derive(AvroSchema)]
+ struct Bar {
+ _field: Box<Bar>,
+ }
+
+ #[derive(AvroSchema)]
+ #[avro(default = r#"{"_field": true}"#)]
+ struct Spam {
+ _field: bool,
+ }
+
+ #[derive(AvroSchema)]
+ struct Foo {
+ #[avro(default = false)]
+ _a: bool,
+ #[avro(default = false)]
+ _b: i8,
+ #[avro(default = false)]
+ _c: i16,
+ #[avro(default = false)]
+ _d: i32,
+ #[avro(default = false)]
+ _e: i64,
+ #[avro(default = false)]
+ _f: u8,
+ #[avro(default = false)]
+ _g: u16,
+ #[avro(default = false)]
+ _h: u32,
+ #[avro(default = false)]
+ _i: f32,
+ #[avro(default = false)]
+ _j: f64,
+ #[avro(default = false)]
+ _k: String,
+ #[avro(default = false)]
+ _l: Box<str>,
+ #[avro(default = false)]
+ _m: char,
+ #[avro(default = false)]
+ _n: Box<Spam>,
+ #[avro(default = false)]
+ _o: Vec<bool>,
+ #[avro(default = false)]
+ _p: [u8; 5],
+ #[avro(default = false)]
+ _p_alt: [Bar; 5],
+ #[avro(default = false)]
+ _q: HashMap<String, String>,
+ #[avro(default = false)]
+ _r: Option<f64>,
+ #[avro(default = false)]
+ _s: Duration,
+ #[avro(default = false)]
+ _t: Uuid,
+ #[avro(default = false)]
+ _u: u64,
+ #[avro(default = false)]
+ _v: u128,
+ #[avro(default = false)]
+ _w: i128,
+ #[avro(default = false)]
+ _x: Bar,
+ }
+
+ let schema = Foo::get_schema();
+ assert_eq!(
+ serde_json::to_string(&schema).unwrap(),
+
r#"{"type":"record","name":"Foo","fields":[{"name":"_a","type":"boolean"},{"name":"_b","type":"int"},{"name":"_c","type":"int"},{"name":"_d","type":"int"},{"name":"_e","type":"long"},{"name":"_f","type":"int"},{"name":"_g","type":"int"},{"name":"_h","type":"long"},{"name":"_i","type":"float"},{"name":"_j","type":"double"},{"name":"_k","type":"string"},{"name":"_l","type":"string"},{"name":"_m","type":"string"},{"name":"_n","type":{"type":"record","name":"Spam","fields":[{"name":"_field","type":"boolean","default":false}]}},{"name":"_o","type":{"type":"array","items":"boolean"}},{"name":"_p","type":{"type":"array","items":"int"}},{"name":"_p_alt","type":{"type":"array","items":{"type":"record","name":"Bar","fields":[{"name":"_field","type":"Bar"}]}}},{"name":"_q","type":{"type":"map","values":"string"}},{"name":"_r","type":["null","double"]},{"name":"_s","type":{"type":"fixed","name":"duration","size":12,"logicalType":"duration"}},{"name":"_t","type":{"type":"fixed","name":"u
uid","size":16,"logicalType":"uuid"}},{"name":"_u","type":{"type":"fixed","name":"u64","size":8}},{"name":"_v","type":{"type":"fixed","name":"u128","size":16}},{"name":"_w","type":{"type":"fixed","name":"i128","size":16}},{"name":"_x","type":"Bar"}]}"#
+ );
+}
+
+#[test]
+fn avro_rs_476_field_default_provided() {
+ #[derive(AvroSchema)]
+ #[avro(default = r#"{"_field": true}"#)]
+ struct Spam {
+ _field: bool,
+ }
+
+ #[derive(AvroSchema)]
+ struct Foo {
+ #[avro(default = "true")]
+ _a: bool,
+ #[avro(default = "42")]
+ _b: i8,
+ #[avro(default = "42")]
+ _c: i16,
+ #[avro(default = "42")]
+ _d: i32,
+ #[avro(default = "42")]
+ _e: i64,
+ #[avro(default = "42")]
+ _f: u8,
+ #[avro(default = "42")]
+ _g: u16,
+ #[avro(default = "42")]
+ _h: u32,
+ #[avro(default = "42.0")]
+ _i: f32,
+ #[avro(default = "42.0")]
+ _j: f64,
+ #[avro(default = r#""String""#)]
+ _k: String,
+ #[avro(default = r#""str""#)]
+ _l: Box<str>,
+ #[avro(default = r#""Z""#)]
+ _m: char,
+ #[avro(default = r#"{"_field": false}"#)]
+ _n: Box<Spam>,
+ #[avro(default = "[true, false, true]")]
+ _o: Vec<bool>,
+ #[avro(default = "[1,2,3,4,5]")]
+ _p: [u8; 5],
+ #[avro(
+ default = r#"[{"_field": true},{"_field": false},{"_field":
true},{"_field": false},{"_field": true}]"#
+ )]
+ _p_alt: [Spam; 5],
+ #[avro(default = r#"{"A": "B"}"#)]
+ _q: HashMap<String, String>,
+ #[avro(default = "42.0")]
Review Comment:
Since this is an Option it will generate a Union schema with Null being the
first variant, so the default must be `null`.
Another way is to detect that the default type is not Null and generate a
Union schema with a proper first variant that matches the default's type. But
this would be more complex.
##########
avro/src/serde/derive.rs:
##########
@@ -113,11 +121,14 @@ use std::collections::{HashMap, HashSet};
///
/// Set the `doc` attribute of the field. Defaults to the documentation of
the field.
///
-/// - `#[avro(default = "null")]`
+/// - `#[avro(default = "null")]` or `#[avro(default = false)]`
///
-/// Set the `default` attribute of the field.
+/// Control the `default` attribute of the field. When not used, it will
use [`AvroSchemaComponent::field_default`]
+/// to get the default value for a type. This default value can be
overriden by providing a JSON string.
Review Comment:
```suggestion
/// to get the default value for a type. This default value can be
overridden by providing a JSON string.
```
##########
avro_derive/src/attributes/mod.rs:
##########
@@ -274,11 +320,11 @@ impl FieldOptions {
}
if ((serde.skip_serializing && !serde.skip_deserializing)
|| serde.skip_serializing_if.is_some())
- && avro.default.is_none()
+ && avro.default == FieldDefault::Disabled
Review Comment:
What happens if `avro.default` is `FieldDefault::Trait` but
`field_default()` returns `None` ?
--
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]
