477: feat: Allow types to provide default values#53
477: feat: Allow types to provide default values#53martin-augment wants to merge 1 commit intomainfrom
Conversation
`AvroSchemaComponent` is extended with a function `field_default`
which will be called when deriving a record to set the default value
for a field. The default implementation is to return `None`, which
means no default.
On the derive side, it is now possible to specify a default for a type
using `#[avro(default = "..")]`. It is also possible to disable setting
a default for a field with `#[avro(default = false)]`.
This enables users to use `#[serde(skip_serializing{_if})]` on most fields
without having to provide a default value.
Summary of ChangesHello @martin-augment, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances how default values are handled in Avro schemas generated by the Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
WalkthroughThis pull request introduces comprehensive support for field-level defaults in Avro schema derivation. It adds a new ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
The pull request introduces a significant enhancement to the AvroSchema derive macro by allowing types to provide default values through the AvroSchemaComponent trait. This aligns the Rust implementation more closely with the Avro specification's support for default values in record fields. The implementation is thorough, covering many standard types and providing compile-time validation for JSON default values provided via attributes. The addition of #[avro(default = false)] to disable defaults is also a useful feature for fine-grained control over schema generation.
| None | ||
| } | ||
|
|
||
| /// If `T` has a field default, this will return an array of with that default. Otherwise there is no default. |
There was a problem hiding this comment.
There is a small typo in the doc comment: "of with" should be "with".
| /// If `T` has a field default, this will return an array of with that default. Otherwise there is no default. | |
| /// If `T` has a field default, this will return an array with that default. Otherwise there is no default. |
There was a problem hiding this comment.
value:valid-but-wont-fix; category:bug; feedback: The Gemini AI reviewer is not correct! The grammar would be better if "elements" is added instead of removing the "of": this will return an array of elements with that default
| /// 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()) |
There was a problem hiding this comment.
Using vec![default; N] is a simpler and more idiomatic way to create a vector with N cloned elements than std::array::from_fn followed by to_vec().
| serde_json::Value::Array(std::array::from_fn::<_, N, _>(|_| default.clone()).to_vec()) | |
| serde_json::Value::Array(vec![default; N]) |
There was a problem hiding this comment.
value:good-to-have; category:bug; feedback: The Gemini AI reviewer is correct! Using the vec! initializer will be shorter and more succint.
| serde_json::from_str(&default_value[..]).map_err(|e| { | ||
| vec![syn::Error::new( | ||
| ident.span(), | ||
| format!("Invalid avro default json: \n{e}"), |
There was a problem hiding this comment.
"Avro" and "JSON" should be capitalized in the error message for consistency and correctness.
| format!("Invalid avro default json: \n{e}"), | |
| format!("Invalid Avro default JSON: \n{e}"), |
There was a problem hiding this comment.
value:good-to-have; category:documentation; feedback: The Gemini AI reviewer is correct! The names and abbreviations should be capitalized/upper-cased to emphasise their meaning.
| )] | ||
| })?; | ||
| quote! { | ||
| Some(serde_json::from_str(#default_value).expect(format!("Invalid JSON: {:?}", #default_value).as_str())) |
There was a problem hiding this comment.
Since the JSON string has already been validated at compile time (lines 124-130), using format! and as_str() inside expect is redundant. A simple static message is sufficient.
| Some(serde_json::from_str(#default_value).expect(format!("Invalid JSON: {:?}", #default_value).as_str())) | |
| Some(serde_json::from_str(#default_value).expect("Invalid JSON")) |
There was a problem hiding this comment.
value:good-to-have; category:documentation; feedback: The Gemini AI reviewer is correct! There is no need to parse the JSON twice. The successful result could be assigned and quoted instead. This way there won't be a need to expect() and provide a panic message.
| @@ -199,7 +208,6 @@ fn get_struct_schema_def( | |||
| Some(serde_json::from_str(#default_value).expect(format!("Invalid JSON: {:?}", #default_value).as_str())) | |||
There was a problem hiding this comment.
Since the JSON string has already been validated at compile time (lines 200-206), using format! and as_str() inside expect is redundant. A simple static message is sufficient.
| Some(serde_json::from_str(#default_value).expect(format!("Invalid JSON: {:?}", #default_value).as_str())) | |
| Some(serde_json::from_str(#default_value).expect("Invalid JSON")) |
There was a problem hiding this comment.
value:good-to-have; category:documentation; feedback: The Gemini AI reviewer is correct! There is no need to parse the JSON twice. The successful result could be assigned and quoted instead. This way there won't be a need to expect() and provide a panic message.
PR Review: feat: Allow types to provide default valuesOverall this is a well-structured feature that extends Critical Issues1. Undocumented Breaking ChangeThe most significant concern: before this PR, all derived fields produced For example, existing code like: #[derive(AvroSchema)]
struct MyRecord {
count: i32,
name: String,
}will now produce a schema with 2. Regression in
|
| Category | Issue |
|---|---|
| Breaking | Auto-defaults change schema for all existing derived types |
| Bug | skip_serializing validation regression for types returning None from field_default() |
| Bug | Test exercises invalid Avro schema (non-null default on Option<T>) |
| Quality | format\! in expect() allocates unnecessarily in generated code |
| Quality | Typo: "overriden" -> "overridden" |
| Quality | Misleading doc comment on when to implement field_default() |
| Design | char default is null character, possibly surprising |
The feature is useful and the implementation is mostly clean, but the breaking change aspect needs to be addressed — either by making auto-defaults opt-in (e.g., #[avro(auto_defaults)] at the container level) or by clearly documenting this as a breaking change with a major version bump.
🤖 Augment PR SummarySummary: This PR extends schema derivation to support type-provided default values and makes field defaults more flexible. Changes:
Technical Notes: Default values are represented as 🤖 Was this summary useful? React with 👍 or 👎 |
![augmentcode[bot]](https://avatars.githubusercontent.com/in/1027498?s=60&v=4){kind=small}
| Value::Uuid(Uuid::from_slice(bytes).map_err(Details::ConvertSliceToUuid)?) | ||
| } | ||
| (Value::String(ref string), UuidSchema::Fixed(_)) => { | ||
| let bytes = string.as_bytes(); |
There was a problem hiding this comment.
For UuidSchema::Fixed, using string.as_bytes() treats the JSON default as UTF-8 bytes; this can change both the length and the actual byte values for non-ASCII \u00xx escapes, so fixed/uuid defaults may resolve incorrectly. It may be worth ensuring this follows Avro’s “fixed/bytes default as JSON string of raw bytes” interpretation (1 character → 1 byte).
Severity: medium
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
| let default_value = match field_attrs.default { | ||
| Some(default_value) => { | ||
| FieldDefault::Disabled => quote! { None }, | ||
| FieldDefault::Trait => type_to_field_default_expr(&field.ty)?, |
There was a problem hiding this comment.
FieldDefault::Trait always emits <FieldType as AvroSchemaComponent>::field_default(), even when the field schema is provided via #[avro(with = ...)]; this can break the documented “foreign type” use-case where the field type itself doesn’t implement AvroSchemaComponent. As-is, derives that previously worked with with may now fail to compile unless the field default is explicitly disabled/provided.
Severity: high
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
| #get_record_fields_impl | ||
| } | ||
|
|
||
| fn field_default() -> ::std::option::Option<::serde_json::Value> { |
There was a problem hiding this comment.
The generated impl references ::serde_json::Value directly; consumers deriving AvroSchemaComponent may now need to add an explicit serde_json dependency even if they don’t otherwise use schema-default features. If that’s not intended, it may be worth ensuring the generated path resolves without requiring a direct dependency in downstream crates.
Severity: medium
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
avro_derive/tests/derive.rs (1)
2383-2393:BarandSpamstructs are duplicated across two test functions.Both
avro_rs_476_field_defaultandavro_rs_476_field_default_falsedefine identicalBarandSpamlocally. Moving them to module scope would eliminate the duplication and avoid divergence if either is changed later.♻️ Proposed refactor: hoist shared types to module scope
+/// Recursive type with no auto-derivable default (used by avro_rs_476_* tests). +#[derive(AvroSchema)] +struct AvroRs476Bar { + _field: Box<AvroRs476Bar>, +} + +/// Type with a struct-level default (used by avro_rs_476_* tests). +#[derive(AvroSchema)] +#[avro(default = r#"{"_field": true}"#)] +struct AvroRs476Spam { + _field: bool, +} #[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, - } // use AvroRs476Bar and AvroRs476Spam instead ...Also applies to: 2434-2442
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@avro_derive/tests/derive.rs` around lines 2383 - 2393, The two tests avro_rs_476_field_default and avro_rs_476_field_default_false duplicate the local struct definitions Bar and Spam; hoist Bar and Spam to module scope (declare them once at the top of the test module) and remove the local definitions in both test functions so both tests reuse the same types (ensure you keep the AvroSchema derives and the #[avro(default = ...)] attribute on Spam when moving).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@avro_derive/tests/derive.rs`:
- Around line 2553-2554: The test sets an Avro default on the union field `_r:
Option<f64>` using #[avro(default = "42.0")], which violates Avro union-default
rules because Option<f64> maps to ["null","double"] and defaults must match the
first union branch (null); fix by changing the attribute to #[avro(default =
"null")] for `_r` (or remove the default entirely), or if you intentionally want
an invalid/default-for-second-branch case, keep it but add a clear comment like
`// Invalid per Avro spec` next to `#[avro(default = "42.0")]` to document the
deliberate deviation.
- Around line 1374-1375: Several test structs produce schemas with "default"
entries because the derive uses FieldDefault::Trait by default; update each test
struct used in schema equality assertions—e.g. TestAllSupportedBaseTypes,
TestBasicWithBool, TestSmartPointers, TestNested (or TestInnerStruct),
TestGeneric (and any other fixtures referenced in tests like test_basic_types,
test_inner_struct, test_smart_pointers, avro_3634_test_basic_with_bool,
test_generic_container_1/2)—by adding the attribute #[avro(default = false)] to
every primitive-typed field so the derived schema omits "default" and matches
the hardcoded expected schema fixtures.
---
Nitpick comments:
In `@avro_derive/tests/derive.rs`:
- Around line 2383-2393: The two tests avro_rs_476_field_default and
avro_rs_476_field_default_false duplicate the local struct definitions Bar and
Spam; hoist Bar and Spam to module scope (declare them once at the top of the
test module) and remove the local definitions in both test functions so both
tests reuse the same types (ensure you keep the AvroSchema derives and the
#[avro(default = ...)] attribute on Spam when moving).
| #[avro(default = false)] | ||
| c: f64, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's look at the context around the modified line
head -n 1400 avro_derive/tests/derive.rs | tail -n 100Repository: martin-augment/avro-rs
Length of output: 2648
🏁 Script executed:
# Get the file size to see if we can read it all
wc -l avro_derive/tests/derive.rsRepository: martin-augment/avro-rs
Length of output: 98
🏁 Script executed:
# Search for TestBasicStructWithDefaultValues to understand the test structure
rg -n "TestBasicStructWithDefaultValues" avro_derive/tests/derive.rs -A 20Repository: martin-augment/avro-rs
Length of output: 3528
🏁 Script executed:
# Look for other tests that check f64 fields to see if they have defaults
rg -n "test_basic_types|test_inner_struct|avro_3634_test_basic_with_bool|test_smart_pointers" avro_derive/tests/derive.rs | head -20Repository: martin-augment/avro-rs
Length of output: 411
🏁 Script executed:
# Search for field_default implementations to understand the auto-default mechanism
rg -n "field_default" avro_derive/ --type rustRepository: martin-augment/avro-rs
Length of output: 15750
🏁 Script executed:
# Look at the FieldDefault enum definition to understand the mechanism
rg -n "enum FieldDefault" avro_derive/ -A 5Repository: martin-augment/avro-rs
Length of output: 443
🏁 Script executed:
# Search for how FieldDefault is determined - is it opt-in or automatic?
rg -n "FieldDefault::" avro_derive/src/attributes/mod.rs -B 2 -A 2Repository: martin-augment/avro-rs
Length of output: 308
🏁 Script executed:
# Check the actual test functions mentioned in the search results to understand the feature
sed -n '2383,2450p' avro_derive/tests/derive.rsRepository: martin-augment/avro-rs
Length of output: 3349
🏁 Script executed:
# Also check the other field_default test
sed -n '2432,2510p' avro_derive/tests/derive.rsRepository: martin-augment/avro-rs
Length of output: 3189
🏁 Script executed:
# Let's check the test_basic_types function to see if it has been updated
rg -n "fn test_basic_types" avro_derive/tests/derive.rs -A 200 | head -250Repository: martin-augment/avro-rs
Length of output: 5809
🏁 Script executed:
# Search for schema assertions in test_basic_types
sed -n '263,340p' avro_derive/tests/derive.rsRepository: martin-augment/avro-rs
Length of output: 1823
🏁 Script executed:
# Check test_inner_struct
sed -n '337,420p' avro_derive/tests/derive.rsRepository: martin-augment/avro-rs
Length of output: 2446
🏁 Script executed:
# Look for avro_3634_test_basic_with_bool to see if it's been modified
rg -n "fn avro_3634_test_basic_with_bool" avro_derive/tests/derive.rs -A 100 | head -120Repository: martin-augment/avro-rs
Length of output: 3358
🏁 Script executed:
# Let's count how many tests assert schema equality without defaults
# Look for the pattern where schema strings are compared
rg -n 'get_schema()' avro_derive/tests/derive.rs | wc -lRepository: martin-augment/avro-rs
Length of output: 68
🏁 Script executed:
# See which tests use assert_eq with schema comparisons but likely don't have defaults in their expected strings
rg -n "assert_eq.*get_schema" avro_derive/tests/derive.rs | head -20Repository: martin-augment/avro-rs
Length of output: 1302
🏁 Script executed:
# Let's find the definition of TestAllSupportedBaseTypes to see if it has #[avro(default = false)] attributes
rg -n "struct TestAllSupportedBaseTypes" avro_derive/tests/derive.rs -A 12Repository: martin-augment/avro-rs
Length of output: 294
🏁 Script executed:
# Find TestBasicWithBool definition
rg -n "struct TestBasicWithBool" avro_derive/tests/derive.rs -A 5Repository: martin-augment/avro-rs
Length of output: 171
🏁 Script executed:
# Check TestSmartPointers which is also in the list of tests that assert schema equality
rg -n "struct TestSmartPointers" avro_derive/tests/derive.rs -A 15Repository: martin-augment/avro-rs
Length of output: 440
🏁 Script executed:
# Let's see the full test_smart_pointers function
sed -n '909,985p' avro_derive/tests/derive.rsRepository: martin-augment/avro-rs
Length of output: 1912
🏁 Script executed:
# Let's check if there are any structs with #[avro(default = false)] on multiple fields
rg -n "#\[avro\(default = false\)\]" avro_derive/tests/derive.rs | head -30Repository: martin-augment/avro-rs
Length of output: 1053
🏁 Script executed:
# Check the total count - are there many instances?
rg -c "#\[avro\(default = false\)\]" avro_derive/tests/derive.rsRepository: martin-augment/avro-rs
Length of output: 68
🏁 Script executed:
# Let's specifically check if TestAllSupportedBaseTypes is used in multiple tests
rg -n "TestAllSupportedBaseTypes" avro_derive/tests/derive.rs | head -20Repository: martin-augment/avro-rs
Length of output: 777
🏁 Script executed:
# Now let's understand the actual behavior - check if there's documentation or implementation
# of when field_default() gets called
rg -n "fn field_default" avro/src/ --type rust -A 3 | head -50Repository: martin-augment/avro-rs
Length of output: 2627
🏁 Script executed:
# Check what the actual implementation of field_default is for primitive types
rg -n "impl.*AvroSchemaComponent.*for.*bool\|impl.*AvroSchemaComponent.*for.*i32\|impl.*AvroSchemaComponent.*for.*f64" avro/src/ -A 10 | head -80Repository: martin-augment/avro-rs
Length of output: 48
🏁 Script executed:
# Let's search for field_default implementations for specific primitive types
rg -n "impl AvroSchemaComponent" avro/src/serde/derive.rs | grep -E "bool|i32|f64|String" | head -20Repository: martin-augment/avro-rs
Length of output: 48
🏁 Script executed:
# Search more broadly for the macro that implements field_default
rg -n "fn field_default" avro/src/serde/derive.rsRepository: martin-augment/avro-rs
Length of output: 971
🏁 Script executed:
# Let's look at the actual implementations around line 530 where the macro is defined
sed -n '500,600p' avro/src/serde/derive.rsRepository: martin-augment/avro-rs
Length of output: 3777
🏁 Script executed:
# Check what the avro_schema_impl_for macro does
rg -n "avro_schema_impl_for" avro/src/serde/derive.rs -A 20 | head -100Repository: martin-augment/avro-rs
Length of output: 48
🏁 Script executed:
# Let's verify the exact behavior by looking at the test that DOES work with auto-defaults
sed -n '2383,2440p' avro_derive/tests/derive.rsRepository: martin-augment/avro-rs
Length of output: 3144
🏁 Script executed:
# And compare it with a test that should be failing - test_basic_types
# Let's extract just the schema assertion to be clear
sed -n '313p' avro_derive/tests/derive.rsRepository: martin-augment/avro-rs
Length of output: 130
🏁 Script executed:
# Check if there's any configuration or feature flag that controls auto-defaults
rg -n "cfg\|feature" avro_derive/tests/derive.rs | head -30Repository: martin-augment/avro-rs
Length of output: 48
🏁 Script executed:
# Let's see if TestAllSupportedBaseTypes is defined with any special attributes
sed -n '246,260p' avro_derive/tests/derive.rsRepository: martin-augment/avro-rs
Length of output: 314
🏁 Script executed:
# Let's verify by running a test to see if it passes or fails
# First, check if there are any recent changes to these test functions
git log --oneline -n 20 avro_derive/tests/derive.rsRepository: martin-augment/avro-rs
Length of output: 117
🏁 Script executed:
# Let's look for all structs with primitive types that DON'T have #[avro(default = false)]
# and check what their expected schemas contain
rg -n "assert_eq!\(schema, .*::get_schema\(\)\)" avro_derive/tests/derive.rs | while read line; do
linenum=$(echo "$line" | cut -d: -f1)
testname=$(sed -n "1,${linenum}p" avro_derive/tests/derive.rs | grep "fn test_\|fn avro_" | tail -1)
echo "Line $linenum: $testname"
done | head -30Repository: martin-augment/avro-rs
Length of output: 1660
🏁 Script executed:
# Let's check a specific case: does the expected schema in test_basic_types contain "default"?
sed -n '264,311p' avro_derive/tests/derive.rs | grep -i "default"Repository: martin-augment/avro-rs
Length of output: 48
🏁 Script executed:
# Check test_smart_pointers expected schema for defaults
sed -n '910,935p' avro_derive/tests/derive.rs | grep -i "default"Repository: martin-augment/avro-rs
Length of output: 48
🏁 Script executed:
# Most importantly: check if TestAllSupportedBaseTypes struct definition has any #[avro(default=false)]
sed -n '246,260p' avro_derive/tests/derive.rs | grep "#\[avro"Repository: martin-augment/avro-rs
Length of output: 48
🏁 Script executed:
# Let's verify the concern by checking if those tests would actually fail
# by looking at the actual vs expected - let's construct what the schemas SHOULD contain
# First, let's see what avro_rs_476_field_default() shows us about the expected schema with defaults
python3 << 'PYEOF'
import json
# From avro_rs_476_field_default test, here's what the schema WITH defaults looks like
with_defaults = 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"}]}'
# Verify it's valid JSON and pretty-print to see the pattern
schema_obj = json.loads(with_defaults)
print("Fields with auto-defaults (from avro_rs_476_field_default test):")
for field in schema_obj['fields']:
if 'default' in field:
print(f" {field['name']}: {field['type']} -> default={field['default']}")
else:
print(f" {field['name']}: {field['type']} -> NO DEFAULT")
PYEOFRepository: martin-augment/avro-rs
Length of output: 1412
🏁 Script executed:
# Now let's check: has the feature actually been enabled in the test file?
# Let's look at git diff to see what changed in this PR
git diff HEAD~1 avro_derive/tests/derive.rs | grep -A 5 -B 5 "avro(default = false)" | head -50Repository: martin-augment/avro-rs
Length of output: 252
🏁 Script executed:
# Check if the feature is controlled by a runtime flag or if it's unconditionally on
rg -n "field_default_impl" avro_derive/src/lib.rs -B 3 -A 3Repository: martin-augment/avro-rs
Length of output: 556
🏁 Script executed:
# Let's check git status to see if there are uncommitted changes
git statusRepository: martin-augment/avro-rs
Length of output: 131
🏁 Script executed:
# Check the git log to understand the PR better
git log --oneline -n 10Repository: martin-augment/avro-rs
Length of output: 117
🏁 Script executed:
# Let's look at the actual commit that introduced the field_default feature
git show 13b4e53 --statRepository: martin-augment/avro-rs
Length of output: 11279
🏁 Script executed:
# Now let's see what files were modified in that commit
git show 13b4e53 avro_derive/tests/derive.rs | head -200Repository: martin-augment/avro-rs
Length of output: 6021
🏁 Script executed:
# This is interesting - the entire test file was newly created in this commit
# Let's verify: are test_basic_types and other unchanged tests' expected schemas
# supposed to have defaults or not?
# Check if there are any updates to test_basic_types or test_smart_pointers
# in the full commit by looking at the full file as it exists now
sed -n '247,259p' avro_derive/tests/derive.rsRepository: martin-augment/avro-rs
Length of output: 242
🏁 Script executed:
# Check if the expected schema in test_basic_types includes defaults
sed -n '264,311p' avro_derive/tests/derive.rs | grep -C 2 "default"Repository: martin-augment/avro-rs
Length of output: 48
🏁 Script executed:
# So test_basic_types expects NO defaults. Let's check if TestAllSupportedBaseTypes
# has been defined with #[avro(default = false)] on its fields
sed -n '245,260p' avro_derive/tests/derive.rsRepository: martin-augment/avro-rs
Length of output: 315
🏁 Script executed:
# Now compare to a test that DOES have auto-defaults (avro_rs_476)
# which doesn't use #[avro(default = false)]
sed -n '2405,2440p' avro_derive/tests/derive.rs | grep -E "struct|#\[avro|_[a-z]:"Repository: martin-augment/avro-rs
Length of output: 478
🏁 Script executed:
# Let's verify the core concern: does the new mechanism apply auto-defaults
# based on the FieldDefault enum default value?
rg -n "FieldDefault::" avro_derive/src/lib.rs -B 3 -A 3Repository: martin-augment/avro-rs
Length of output: 608
🏁 Script executed:
# Check how field_default is actually applied in the schema generation
rg -n "field_default\(\)" avro_derive/src/lib.rs -B 2 -A 2Repository: martin-augment/avro-rs
Length of output: 524
🏁 Script executed:
# See the exact context where field_default is called
sed -n '180,220p' avro_derive/src/lib.rsRepository: martin-augment/avro-rs
Length of output: 2316
🏁 Script executed:
# Count tests that assert schema equality and have primitive types but NO #[avro(default = false)]
# These are the ones that will fail
# First, let's list test names that check schemas
rg -n "fn test_|fn avro_" avro_derive/tests/derive.rs | grep -E "^[0-9]+:" | head -50Repository: martin-augment/avro-rs
Length of output: 2518
🏁 Script executed:
# Now let's verify by checking a few key test structs to see if they have #[avro(default = false)]
# Check TestNested
sed -n '330,333p' avro_derive/tests/derive.rs
# Check TestOptional
sed -n '420,423p' avro_derive/tests/derive.rs
# Check TestGeneric
sed -n '453,458p' avro_derive/tests/derive.rsRepository: martin-augment/avro-rs
Length of output: 427
Add #[avro(default = false)] to all test struct fields to match hardcoded expected schemas.
The auto-default feature enables FieldDefault::Trait by default, which calls field_default() on all field types. All primitive types (bool, i8, i16, i32, i64, u8, u16, u32, f32, f64, String, char) return Some(default_value), so derived schemas will automatically include "default" fields. However, the expected schemas in most tests—test_basic_types, test_inner_struct, test_smart_pointers, avro_3634_test_basic_with_bool, test_generic_container_1, test_generic_container_2, and others—do not include defaults. Test structs like TestAllSupportedBaseTypes, TestBasicWithBool, TestSmartPointers, TestNested, and TestGeneric lack #[avro(default = false)] on their primitive-typed fields, causing schema mismatches.
Add #[avro(default = false)] to all fields in test structs that are used in schema equality assertions where the expected schema omits default values. This applies to all structs with primitive fields that lack explicit default annotations in their hardcoded schema fixtures.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@avro_derive/tests/derive.rs` around lines 1374 - 1375, Several test structs
produce schemas with "default" entries because the derive uses
FieldDefault::Trait by default; update each test struct used in schema equality
assertions—e.g. TestAllSupportedBaseTypes, TestBasicWithBool, TestSmartPointers,
TestNested (or TestInnerStruct), TestGeneric (and any other fixtures referenced
in tests like test_basic_types, test_inner_struct, test_smart_pointers,
avro_3634_test_basic_with_bool, test_generic_container_1/2)—by adding the
attribute #[avro(default = false)] to every primitive-typed field so the derived
schema omits "default" and matches the hardcoded expected schema fixtures.
There was a problem hiding this comment.
value:good-to-have; category:bug; feedback: The CodeRabbit AI reviewer is correct! The Pull Request proposes to add "default" field for all generated schemas. When the application developer does not specify a default explicitly then the type's default is used. This is not the right way. It would be better to not add "default" field at all if the user application didn't ask for such. I.e. only when a field is annotated with #[avro(default = ...)] or AvroSchemaComponent::field_default() returns Some then add the default attribute to the schema
| #[avro(default = "42.0")] | ||
| _r: Option<f64>, |
There was a problem hiding this comment.
Spec-violating union default: 42.0 is not a valid default for ["null","double"].
The Avro specification states that "when a default value is specified for a record field whose type is a union, the type of the default value must match the first element of the union. Thus, for unions containing null, the null is usually listed first, since the default value of such unions is typically null."
_r: Option<f64> maps to ["null","double"] (first type: null). The explicit "42.0" default is a double value matching the second type, which is spec-invalid. Default values for union fields correspond to the first schema in the union. While the test happens to pass (it only calls serde_json::to_string, which doesn't validate union default semantics), a schema registry or any strict Avro consumer will reject this schema.
The test should either use #[avro(default = "null")], or demonstrate non-null defaults only on non-union types (which are already well-covered in this test). If the intent is to document that #[avro(default = "..")] deliberately bypasses spec validation, that should at minimum carry a // Invalid per Avro spec comment.
🛡️ Proposed fix
- #[avro(default = "42.0")]
- _r: Option<f64>,
+ // Option<f64> maps to ["null","double"]; spec-valid default must be null
+ #[avro(default = "null")]
+ _r: Option<f64>,📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| #[avro(default = "42.0")] | |
| _r: Option<f64>, | |
| // Option<f64> maps to ["null","double"]; spec-valid default must be null | |
| #[avro(default = "null")] | |
| _r: Option<f64>, |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@avro_derive/tests/derive.rs` around lines 2553 - 2554, The test sets an Avro
default on the union field `_r: Option<f64>` using #[avro(default = "42.0")],
which violates Avro union-default rules because Option<f64> maps to
["null","double"] and defaults must match the first union branch (null); fix by
changing the attribute to #[avro(default = "null")] for `_r` (or remove the
default entirely), or if you intentionally want an
invalid/default-for-second-branch case, keep it but add a clear comment like `//
Invalid per Avro spec` next to `#[avro(default = "42.0")]` to document the
deliberate deviation.
There was a problem hiding this comment.
value:useful; category:bug; feedback: The CodeRabbit AI reviewer is correct! As per Avro specification the type of the default value must match with the first variant in a union schema. Since Option generates a Union schema with first variant Schema::Null and second an appropriate schema for Xyz the set default value here can be only "null" and nothing else.
value:useful; category:bug; feedback: The Claude AI reviewer is correct! As per Avro specification the type of the default value must match with the first variant in a union schema. Since Option generates a Union schema with first variant Schema::Null and second an appropriate schema for Xyz the set default value here can be only "null" and nothing else. |
value:good-to-have; category:bug; feedback: The Claude AI reviewer is correct! The Pull Request proposes to add "default" field for all generated schemas. When the application developer does not specify a default explicitly then the type's default is used. This is not the right way. It would be better to not add "default" field at all if the user application didn't ask for such. I.e. only when a field is annotated with #[avro(default = ...)] or AvroSchemaComponent::field_default() returns Some then add the default attribute to the schema |
value:useful; category:bug; feedback: The Claude AI reviewer is correct! If AvroSchemaComponent is used to derive the schema then the result value of its field_default() should be used to check whether there is a default or not, and thus whether |
2 participants
477: To review by AI