460: fix: Enum and Fixed would put default in custom_attributes#47
460: fix: Enum and Fixed would put default in custom_attributes#47martin-augment wants to merge 1 commit intomainfrom
default in custom_attributes#47Conversation
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 addresses a bug where 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 modifies Avro schema serialization and parsing logic. FixedSchema serialization now conditionally includes a "default" field when present and uses borrowed references for optional fields. Record and Enum variant serialization changes include "default" and "doc" fields in emitted JSON when provided. The parser extends custom attribute extraction to exclude "default" for Enum and Fixed schemas, treating it separately from custom attributes. Parser changes also adopt borrowed patterns instead of clone-based approaches. Tests are added to verify round-tripping of fixed and enum schemas with defaults and documentation preserved. No public API signatures are altered. ✨ Finishing touches
🧪 Generate unit tests (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. 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 |
PR Review: Fix Enum and Fixed Default in Custom AttributesSummaryThis PR fixes a bug where Code Quality: ✅ GoodStrengths:
Minor observations:
Potential Issues:
|
![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
This pull request correctly fixes an issue where default attributes for Enum and Fixed schemas were being incorrectly handled as custom attributes. The changes in parsing and serialization logic are sound, and the addition of new tests ensures the fix is working as expected. I've found one minor issue in a test case that I've commented on.
| let schema2 = Schema::parse_str(&json)?; | ||
|
|
||
| let Schema::Enum(enum_schema) = schema2 else { | ||
| panic!("Expected Schema::Fixed, got {schema2:?}"); |
There was a problem hiding this comment.
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.
| panic!("Expected Schema::Fixed, got {schema2:?}"); | |
| panic!("Expected Schema::Enum, got {schema2:?}"); |
There was a problem hiding this comment.
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!
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@avro/src/schema/mod.rs`:
- Around line 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).
| let Schema::Enum(enum_schema) = schema2 else { | ||
| panic!("Expected Schema::Fixed, got {schema2:?}"); |
There was a problem hiding this comment.
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.
| 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).
There was a problem hiding this comment.
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!
🤖 Augment PR SummarySummary: Fixes Enum/Fixed schema handling so the Avro-standard Changes:
🤖 Was this summary useful? React with 👍 or 👎 |
![augmentcode[bot]](https://avatars.githubusercontent.com/in/1027498?s=60&v=4){kind=small}
| let schema2 = Schema::parse_str(&json)?; | ||
|
|
||
| let Schema::Enum(enum_schema) = schema2 else { | ||
| panic!("Expected Schema::Fixed, got {schema2:?}"); |
There was a problem hiding this comment.
This panic message says Schema::Fixed but the pattern being matched is Schema::Enum, so failures here will be misleading.
Severity: low
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
There was a problem hiding this comment.
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!
| size: size as usize, | ||
| default, | ||
| attributes: self.get_custom_attributes(complex, vec!["size"]), | ||
| attributes: self.get_custom_attributes(complex, vec!["size", "default"]), |
There was a problem hiding this comment.
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
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
There was a problem hiding this comment.
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.
2 participants
460: To review by AI