Skip to content

445: feat!: Rework SpecificSingleObjectWriter #42

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 2 commits into
base: main
Choose a base branch
from
Open

445: feat!: Rework SpecificSingleObjectWriter #42

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a14885f
feat: Share logic between `ResolvedSchema` and `ResolvedOwnedSchema`
Kriskras99 Jan 27, 2026
3221b97
feat!: Rework `SpecificSingleObjectWriter`
Kriskras99 Jan 27, 2026
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
58 changes: 57 additions & 1 deletion Cargo.lock
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions avro/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,7 @@ digest = { default-features = false, version = "0.10.7", features = ["core-api"]
miniz_oxide = { default-features = false, version = "0.9.0", features = ["with-alloc"] }
log = { workspace = true }
num-bigint = { default-features = false, version = "0.4.6", features = ["std", "serde"] }
ouroboros = { default-features = false, version = "0.18.5", features = ["std"] }
regex-lite = { default-features = false, version = "0.1.8", features = ["std", "string"] }
serde = { workspace = true }
serde_bytes = { workspace = true }
Expand Down
2 changes: 1 addition & 1 deletion avro/examples/specific_single_object.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ fn main() -> Result<(), Box<dyn std::error::Error>> {
b: "foo".to_string(),
};

let mut writer = SpecificSingleObjectWriter::<Test>::with_capacity(1024)?;
let writer = SpecificSingleObjectWriter::<Test>::new()?;
let reader = SpecificSingleObjectReader::<Test>::new()?;

for test in repeat_n(test, 2) {
Expand Down
2 changes: 1 addition & 1 deletion avro/examples/test_interop_single_object_encoding.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@ fn main() -> Result<(), Box<dyn Error>> {

fn test_write(expected: &[u8]) {
let mut encoded: Vec<u8> = Vec::new();
apache_avro::SpecificSingleObjectWriter::<InteropMessage>::with_capacity(1024)
apache_avro::SpecificSingleObjectWriter::<InteropMessage>::new()
.expect("Resolving failed")
.write_value(InteropMessage, &mut encoded)
.expect("Encoding failed");
Expand Down
17 changes: 8 additions & 9 deletions avro/src/reader.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -226,21 +226,20 @@ impl<'r, R: Read> Block<'r, R> {
})
.ok_or(Details::GetAvroSchemaFromMap)?;
if !self.schemata.is_empty() {
let rs = ResolvedSchema::try_from(self.schemata.clone())?;
let names: Names = rs
.get_names()
.iter()
.map(|(name, schema)| (name.clone(), (*schema).clone()))
.collect();
self.writer_schema = Schema::parse_with_names(&json, names)?;
let mut names = HashMap::new();
resolve_names_with_schemata(
self.schemata.iter().copied(),
&mut self.names_refs,
&mut names,
&None,
&HashMap::new(),
)?;
self.names_refs = names.into_iter().map(|(n, s)| (n, s.clone())).collect();
Copy link
Copy Markdown

@augmentcode augmentcode bot Jan 27, 2026

Choose a reason for hiding this comment

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

names_refs is a Names (owned Schema values), but here names is a NamesRef (&Schema values), so (n, s.clone()) looks like it would keep references rather than cloning the underlying Schema (and may not type-check). This matters because decode_internal later relies on names_refs to resolve Schema::Ref during decoding.

Other Locations
  • avro/src/reader.rs:242

Fix This in Augment

🤖 Was this useful? React with 👍 or 👎

Copy link
Copy Markdown
Owner Author

@martin-augment martin-augment Jan 27, 2026

Choose a reason for hiding this comment

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

value:annoying; category:bug; feedback:The Augment AI reviewer is not correct! There is no need to dereference a variable to clone it. Calling the method on the reference will still do the right thing and return an owned clone of that object.

self.writer_schema = Schema::parse_with_names(&json, self.names_refs.clone())?;
} else {
self.writer_schema = Schema::parse(&json)?;
resolve_names(&self.writer_schema, &mut self.names_refs, &None)?;
let mut names = HashMap::new();
resolve_names(&self.writer_schema, &mut names, &None, &HashMap::new())?;
self.names_refs = names.into_iter().map(|(n, s)| (n, s.clone())).collect();
}
Comment on lines 228 to 243
Copy link
Copy Markdown

@coderabbitai coderabbitai bot Jan 27, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

Writer schema names are not merged when schemata is provided.

In the non-empty schemata branch, names_refs is populated only from the provided schemata (Line 229+). Named types defined inside the writer schema itself won’t be added, so decode_internal can fail to resolve Schema::Ref (e.g., recursive records). Consider resolving names from self.writer_schema after parsing and merging them with the known schemata.

🛠️ Suggested merge of writer schema names 
-            let mut names = HashMap::new();
+            let mut known = HashMap::new();
             resolve_names_with_schemata(
                 self.schemata.iter().copied(),
-                &mut names,
+                &mut known,
                 &None,
                 &HashMap::new(),
             )?;
-            self.names_refs = names.into_iter().map(|(n, s)| (n, s.clone())).collect();
-            self.writer_schema = Schema::parse_with_names(&json, self.names_refs.clone())?;
+            let mut names_refs: Names =
+                known.iter().map(|(n, s)| (n.clone(), (*s).clone())).collect();
+            self.writer_schema = Schema::parse_with_names(&json, names_refs.clone())?;
+            let mut writer_names = HashMap::new();
+            resolve_names(&self.writer_schema, &mut writer_names, &None, &known)?;
+            names_refs.extend(writer_names.into_iter().map(|(n, s)| (n, s.clone())));
+            self.names_refs = names_refs;
📝 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
if !self.schemata.is_empty() {
let rs = ResolvedSchema::try_from(self.schemata.clone())?;
let names: Names = rs
.get_names()
.iter()
.map(|(name, schema)| (name.clone(), (*schema).clone()))
.collect();
self.writer_schema = Schema::parse_with_names(&json, names)?;
let mut names = HashMap::new();
resolve_names_with_schemata(
self.schemata.iter().copied(),
&mut self.names_refs,
&mut names,
&None,
&HashMap::new(),
)?;
self.names_refs = names.into_iter().map(|(n, s)| (n, s.clone())).collect();
self.writer_schema = Schema::parse_with_names(&json, self.names_refs.clone())?;
} else {
self.writer_schema = Schema::parse(&json)?;
resolve_names(&self.writer_schema, &mut self.names_refs, &None)?;
let mut names = HashMap::new();
resolve_names(&self.writer_schema, &mut names, &None, &HashMap::new())?;
self.names_refs = names.into_iter().map(|(n, s)| (n, s.clone())).collect();
}
if !self.schemata.is_empty() {
let mut known = HashMap::new();
resolve_names_with_schemata(
self.schemata.iter().copied(),
&mut known,
&None,
&HashMap::new(),
)?;
let mut names_refs: Names =
known.iter().map(|(n, s)| (n.clone(), (*s).clone())).collect();
self.writer_schema = Schema::parse_with_names(&json, names_refs.clone())?;
let mut writer_names = HashMap::new();
resolve_names(&self.writer_schema, &mut writer_names, &None, &known)?;
names_refs.extend(writer_names.into_iter().map(|(n, s)| (n, s.clone())));
self.names_refs = names_refs;
} else {
self.writer_schema = Schema::parse(&json)?;
let mut names = HashMap::new();
resolve_names(&self.writer_schema, &mut names, &None, &HashMap::new())?;
self.names_refs = names.into_iter().map(|(n, s)| (n, s.clone())).collect();
}
🤖 Prompt for AI Agents 
In `@avro/src/reader.rs` around lines 228 - 243, When schemata is non-empty the
code only fills names_refs from the external schemata and skips named types
declared inside the parsed writer_schema; update the if-branch after calling
Schema::parse_with_names(&json, self.names_refs.clone())? to also call
resolve_names(&self.writer_schema, &mut names, &None, &HashMap::new())? (or a
merge variant) and then merge those discovered names into self.names_refs
(ensuring existing schemata entries are preserved or overridden as intended), so
resolve_names_with_schemata, self.writer_schema, names_refs, resolve_names and
Schema::parse_with_names are all involved in the merge step.

Copy link
Copy Markdown
Owner Author

@martin-augment martin-augment Jan 27, 2026

Choose a reason for hiding this comment

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

value:incorrect-but-reasonable; category:bug; feedback:The CodeRabbit AI reviewer is not correct! The names of the parsed schemas are added to an internal data structure of the parser itself, so they are also visible to any schema that is parsed later.

Ok(())
}
Expand Down
Loading