martin-g commented on code in PR #448:
URL: https://github.com/apache/avro-rs/pull/448#discussion_r2744818027
##########
avro_derive/src/lib.rs:
##########
@@ -146,15 +164,13 @@ fn get_struct_schema_def(
} else if field_attrs.flatten {
// Inline the fields of the child record at runtime, as we
don't have access to
// the schema here.
- let flatten_ty = &field.ty;
+ let get_record_fields =
+ get_field_get_record_fields_expr(&field,
field_attrs.with)?;
record_field_exprs.push(quote! {
- if let
::apache_avro::schema::Schema::Record(::apache_avro::schema::RecordSchema {
fields, .. }) = #flatten_ty::get_schema() {
- for mut field in fields {
- field.position = schema_fields.len();
- schema_fields.push(field)
- }
+ if let Some(flattened_fields) = #get_record_fields {
+ schema_fields.extend(flattened_fields)
} else {
- panic!("Can only flatten RecordSchema, got {:?}",
#flatten_ty::get_schema())
+ panic!("#field does not have any fields to flatten
to")
Review Comment:
```suggestion
panic!("{} does not have any fields to flatten
to", stringify!(#field))
```
##########
avro/src/serde/derive.rs:
##########
@@ -177,50 +329,9 @@ where
UnionSchema::new(variants).expect("Option<T> must produce a valid
(non-nested) union"),
)
}
-}
-
-impl<T> AvroSchemaComponent for Map<String, T>
Review Comment:
This impl is lost.
I don't remember any user asking for it. I am not sure whether it is really
needed or not.
##########
avro/src/serde/derive.rs:
##########
@@ -82,7 +83,134 @@ pub trait AvroSchema {
///}
/// ```
pub trait AvroSchemaComponent {
+ /// Get the schema for this component
fn get_schema_in_ctxt(named_schemas: &mut Names, enclosing_namespace:
&Namespace) -> Schema;
+
+ /// Get the fields of this schema if it is a record.
+ ///
+ /// This returns `None` if the schema is not a record.
+ ///
+ /// The default implementation has to do a lot of extra work, so it is
strongly recommended to
+ /// implement this function when manually implementing this trait.
+ fn get_record_fields_in_ctxt(
+ named_schemas: &mut Names,
+ enclosing_namespace: &Namespace,
+ ) -> Option<Vec<RecordField>> {
+ get_record_fields_in_ctxt(named_schemas, enclosing_namespace,
Self::get_schema_in_ctxt)
+ }
+}
+
+/// Get the record fields from `schema_fn` without polluting `named_schemas`
or causing duplicate names
+///
+/// This is public so the derive macro can use it for `#[avro(with = ||)]` and
`#[avro(with = path)]`
+pub fn get_record_fields_in_ctxt(
+ named_schemas: &mut Names,
+ enclosing_namespace: &Namespace,
+ schema_fn: fn(named_schemas: &mut Names, enclosing_namespace: &Namespace)
-> Schema,
+) -> Option<Vec<RecordField>> {
+ let mut record = match schema_fn(named_schemas, enclosing_namespace) {
+ Schema::Record(record) => record,
+ Schema::Ref { name } => {
+ // This schema already exists in `named_schemas` so temporarily
remove it so we can
+ // get the actual schema.
+ let temp = named_schemas
+ .remove(&name)
+ .expect("Name should exist in `named_schemas` otherwise Ref is
invalid");
Review Comment:
```suggestion
.expect("Name '{name}' should exist in `named_schemas`
otherwise Ref is invalid");
```
and maybe also the keys of `named_schemas`
##########
avro_derive/src/lib.rs:
##########
@@ -146,15 +164,13 @@ fn get_struct_schema_def(
} else if field_attrs.flatten {
// Inline the fields of the child record at runtime, as we
don't have access to
// the schema here.
- let flatten_ty = &field.ty;
+ let get_record_fields =
+ get_field_get_record_fields_expr(&field,
field_attrs.with)?;
record_field_exprs.push(quote! {
- if let
::apache_avro::schema::Schema::Record(::apache_avro::schema::RecordSchema {
fields, .. }) = #flatten_ty::get_schema() {
- for mut field in fields {
- field.position = schema_fields.len();
- schema_fields.push(field)
- }
+ if let Some(flattened_fields) = #get_record_fields {
+ schema_fields.extend(flattened_fields)
Review Comment:
Here the `RecordField::position` will have to be re-calculated for the
flattened fields.
https://github.com/apache/avro-rs/blob/5a553beac6b1858e6f97d110318216cf6ecdb4c6/avro/src/schema/record/field.rs#L55
##########
avro/tests/get_record_fields.rs:
##########
@@ -0,0 +1,82 @@
+// 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::{
+ Schema,
+ serde::{AvroSchemaComponent, get_record_fields_in_ctxt},
+};
+use std::collections::HashMap;
+
+use apache_avro_test_helper::TestResult;
+
+#[test]
+fn avro_rs_448_default_get_record_fields_no_recursion() -> TestResult {
+ #[derive(apache_avro_derive::AvroSchema)]
+ struct Foo {
+ _a: i32,
+ _b: String,
+ }
+
+ let mut named_schemas = HashMap::new();
+ let fields =
+ get_record_fields_in_ctxt(&mut named_schemas, &None,
Foo::get_schema_in_ctxt).unwrap();
+
+ assert_eq!(fields.len(), 2);
+ assert!(named_schemas.is_empty(), "Name shouldn't have been added");
Review Comment:
```suggestion
assert!(named_schemas.is_empty(), "Name shouldn't have been added:
{named_schemas:?}");
```
for easier debugging.
also do the same below
##########
avro/tests/get_record_fields.rs:
##########
@@ -0,0 +1,82 @@
+// 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::{
+ Schema,
+ serde::{AvroSchemaComponent, get_record_fields_in_ctxt},
+};
+use std::collections::HashMap;
+
+use apache_avro_test_helper::TestResult;
+
+#[test]
+fn avro_rs_448_default_get_record_fields_no_recursion() -> TestResult {
+ #[derive(apache_avro_derive::AvroSchema)]
+ struct Foo {
+ _a: i32,
+ _b: String,
+ }
+
+ let mut named_schemas = HashMap::new();
+ let fields =
+ get_record_fields_in_ctxt(&mut named_schemas, &None,
Foo::get_schema_in_ctxt).unwrap();
+
+ assert_eq!(fields.len(), 2);
+ assert!(named_schemas.is_empty(), "Name shouldn't have been added");
+
+ // Insert Foo into named_schemas
+ let Schema::Record(_) = Foo::get_schema_in_ctxt(&mut named_schemas, &None)
else {
Review Comment:
let's use a `match`, so we could print the non-record schema in the panic
message
##########
avro_derive/src/lib.rs:
##########
@@ -302,6 +328,41 @@ fn get_field_schema_expr(field: &Field, with: With) ->
Result<TokenStream, Vec<s
}
}
+fn get_field_get_record_fields_expr(
+ field: &Field,
+ with: With,
+) -> Result<TokenStream, Vec<syn::Error>> {
+ match with {
+ With::Trait => Ok(type_to_get_record_fields_expr(&field.ty)?),
+ With::Serde(path) => {
+ Ok(quote! { #path::get_record_fields_in_ctxt(named_schemas,
enclosing_namespace) })
Review Comment:
What happens if there is no such function in the module ?
I guess the compilation fails.
Maybe we should add no-op impls (i.e. returning None) for the `with` impls
like:
https://github.com/apache/avro-rs/blob/5a553beac6b1858e6f97d110318216cf6ecdb4c6/avro/src/serde/with.rs#L144
--
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]
