Skip to content

460: fix: Enum and Fixed would put default in custom_attributes #47

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
martin-augment wants to merge 1 commit into
base: main
Choose a base branch
from
Open

460: fix: Enum and Fixed would put default in custom_attributes #47

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
73 changes: 68 additions & 5 deletions avro/src/schema/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -294,19 +294,23 @@ impl FixedSchema {
S: Serializer,
{
map.serialize_entry("type", "fixed")?;
if let Some(ref n) = self.name.namespace {
if let Some(n) = self.name.namespace.as_ref() {
map.serialize_entry("namespace", n)?;
}
map.serialize_entry("name", &self.name.name)?;
if let Some(ref docstr) = self.doc {
if let Some(docstr) = self.doc.as_ref() {
map.serialize_entry("doc", docstr)?;
}
map.serialize_entry("size", &self.size)?;

if let Some(ref aliases) = self.aliases {
if let Some(aliases) = self.aliases.as_ref() {
map.serialize_entry("aliases", aliases)?;
}

if let Some(default) = self.default.as_ref() {
map.serialize_entry("default", default)?;
}

for attr in &self.attributes {
map.serialize_entry(attr.0, attr.1)?;
}
Expand Down Expand Up @@ -783,7 +787,7 @@ impl Serialize for Schema {
doc,
fields,
attributes,
..
lookup: _lookup,
}) => {
let mut map = serializer.serialize_map(None)?;
map.serialize_entry("type", "record")?;
Expand All @@ -808,7 +812,8 @@ impl Serialize for Schema {
symbols,
aliases,
attributes,
..
default,
doc,
}) => {
let mut map = serializer.serialize_map(None)?;
map.serialize_entry("type", "enum")?;
Expand All @@ -821,6 +826,12 @@ impl Serialize for Schema {
if let Some(aliases) = aliases {
map.serialize_entry("aliases", aliases)?;
}
if let Some(default) = default {
map.serialize_entry("default", default)?;
}
if let Some(doc) = doc {
map.serialize_entry("doc", doc)?;
}
for attr in attributes {
map.serialize_entry(attr.0, attr.1)?;
}
Expand Down Expand Up @@ -5147,6 +5158,58 @@ mod tests {
error,
Details::ParsePrimitiveSimilar("bool".to_string(), "boolean").to_string()
);

Ok(())
}

#[test]
fn avro_rs_460_fixed_default_in_custom_attributes() -> TestResult {
let schema = Schema::parse_str(
r#"{
"name": "fixed_with_default",
"type": "fixed",
"size": 1,
"default": "\u0000",
"doc": "a docstring"
}"#,
)?;

assert_eq!(schema.custom_attributes().unwrap(), &BTreeMap::new());

let json = serde_json::to_string(&schema)?;
let schema2 = Schema::parse_str(&json)?;

let Schema::Fixed(fixed) = schema2 else {
panic!("Expected Schema::Fixed, got {schema2:?}");
};
assert!(fixed.default.is_some());

Ok(())
}

#[test]
fn avro_rs_460_enum_default_in_custom_attributes() -> TestResult {
let schema = Schema::parse_str(
r#"{
"name": "fixed_with_default",
"type": "enum",
"symbols": ["A", "B", "C"],
"default": "A",
"doc": "a docstring"
}"#,
)?;

assert_eq!(schema.custom_attributes().unwrap(), &BTreeMap::new());

let json = serde_json::to_string(&schema)?;
let schema2 = Schema::parse_str(&json)?;

let Schema::Enum(enum_schema) = schema2 else {
panic!("Expected Schema::Fixed, got {schema2:?}");
Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist bot Feb 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The panic message here seems to be copy-pasted from the avro_rs_460_fixed_default_in_custom_attributes test. It should refer to Schema::Enum instead of Schema::Fixed to be accurate for this test case.

Suggested change
panic!("Expected Schema::Fixed, got {schema2:?}");
panic!("Expected Schema::Enum, got {schema2:?}");

Copy link
Copy Markdown
Owner Author

@martin-augment martin-augment Feb 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

value:useful; category:bug; feedback: The Gemini AI is correct! The panic message is copied from the unit test for Fixed schema. It should be fixed to say Enum instead!

Comment on lines +5207 to +5208
Copy link
Copy Markdown

@coderabbitai coderabbitai bot Feb 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Copy-paste error in panic message.

The panic message says "Expected Schema::Fixed" but this test is checking for Schema::Enum.

Proposed fix 
-            panic!("Expected Schema::Fixed, got {schema2:?}");
+            panic!("Expected Schema::Enum, got {schema2:?}");
📝 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.

Suggested change
let Schema::Enum(enum_schema) = schema2 else {
panic!("Expected Schema::Fixed, got {schema2:?}");
let Schema::Enum(enum_schema) = schema2 else {
panic!("Expected Schema::Enum, got {schema2:?}");
🤖 Prompt for AI Agents 
In `@avro/src/schema/mod.rs` around lines 5207 - 5208, The panic message is a
copy-paste error: when matching let Schema::Enum(enum_schema) = schema2 the
panic should state "Expected Schema::Enum" (and include schema2 debug) instead
of "Expected Schema::Fixed"; update the panic in that else branch to
panic!("Expected Schema::Enum, got {schema2:?}") so it accurately reflects the
pattern being matched (reference symbols: Schema::Enum, schema2, enum_schema).

Copy link
Copy Markdown
Owner Author

@martin-augment martin-augment Feb 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

value:useful; category:bug; feedback: The CodeRabbit AI is correct! The panic message is copied from the unit test for Fixed schema. It should be fixed to say Enum instead!

Copy link
Copy Markdown

@augmentcode augmentcode bot Feb 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This panic message says Schema::Fixed but the pattern being matched is Schema::Enum, so failures here will be misleading.

Severity: low

Fix This in Augment

🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.

Copy link
Copy Markdown
Owner Author

@martin-augment martin-augment Feb 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

value:useful; category:bug; feedback: The Augment AI is correct! The panic message is copied from the unit test for Fixed schema. It should be fixed to say Enum instead!

};
assert!(enum_schema.default.is_some());
assert!(enum_schema.doc.is_some());

Ok(())
}
}
8 changes: 4 additions & 4 deletions avro/src/schema/parser.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -690,7 +690,7 @@ impl Parser {
doc: complex.doc(),
symbols,
default,
attributes: self.get_custom_attributes(complex, vec!["symbols"]),
attributes: self.get_custom_attributes(complex, vec!["symbols", "default"]),
});

self.register_parsed_schema(&fully_qualified_name, &schema, &aliases);
Expand Down Expand Up @@ -792,8 +792,8 @@ impl Parser {
_ => None,
});

if default.is_some() {
let len = default.clone().unwrap().len();
if let Some(default) = &default {
let len = default.len();
if len != size as usize {
return Err(Details::FixedDefaultLenSizeMismatch(len, size).into());
}
Expand All @@ -809,7 +809,7 @@ impl Parser {
doc,
size: size as usize,
default,
attributes: self.get_custom_attributes(complex, vec!["size"]),
attributes: self.get_custom_attributes(complex, vec!["size", "default"]),
Copy link
Copy Markdown

@augmentcode augmentcode bot Feb 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since "default" is now excluded from custom_attributes, a non-string fixed "default" currently gets silently dropped (parsed as None) instead of being reported. Consider erroring when "default" exists but isn’t a JSON string so invalid schemas don’t parse successfully.

Severity: medium

Fix This in Augment

🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.

Copy link
Copy Markdown
Owner Author

@martin-augment martin-augment Feb 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

value:good-but-wont-fix; category:bug; feedback: The Augment AI says that the value should be a String, but https://avro.apache.org/docs/1.12.0/specification/#fixed does not mention "default" field for Fixed schema at all. For Array schema the default field has JSON array type - https://avro.apache.org/docs/1.12.0/specification/#arrays. The same could be used for Fixed too if Fixed could have a default.

});

self.register_parsed_schema(&fully_qualified_name, &schema, &aliases);
Expand Down