Skip to content

Fix for Value::String to Value::Fixed resolution ignoring the size from the schema. #542

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
KitFieldhouse wants to merge 6 commits into
base: main
Choose a base branch
from
+118 −2
Open

Fix for Value::String to Value::Fixed resolution ignoring the size from the schema. #542

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
78d08a9
Fix: String to Fixed resolution will obey the schema's size constrain…
KitFieldhouse May 4, 2026
56e19e4
No need to resolve twice
martin-g May 5, 2026
4372fac
Improve test method names
martin-g May 5, 2026
b8d7826
fmt
martin-g May 5, 2026
d652b11
Merge branch 'main' into fixedResolution
martin-g May 5, 2026
55b9518
Restore idempotence in test
martin-g May 5, 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
118 changes: 117 additions & 1 deletion avro/src/types.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1029,7 +1029,15 @@ impl Value {
Err(Details::CompareFixedSizes { size, n }.into())
}
}
Value::String(s) => Ok(Value::Fixed(s.len(), s.into_bytes())),
Value::String(s) => {
if s.len() > size {
Err(Details::CompareFixedSizes { size, n: s.len() }.into())
} else {
let mut bytes = s.into_bytes();
bytes.resize(size, 0);
Copy link
Copy Markdown
Member

@martin-g martin-g May 5, 2026

Choose a reason for hiding this comment

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

I am not sure the padding applied here is the correct thing to do.
There is no such padding for Bytes->Fixed below. An exact match of the length with the size is required.
I think the same should be done here too. Otherwise the reader of the fixed data will have do handle the padding somehow.

Copy link
Copy Markdown
Author

@KitFieldhouse KitFieldhouse May 5, 2026

Choose a reason for hiding this comment

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

Sounds good, I agree with that logic.

Ok(Value::Fixed(size, bytes))
}
}
Value::Bytes(s) => {
if s.len() == size {
Ok(Value::Fixed(size, s))
Expand Down Expand Up @@ -3494,4 +3502,112 @@ Field with name '"b"' is not a member of the map items"#,
"JSON number 18446744073709551615 could not be converted into an Avro value as it's too large"
);
}

#[test]
fn avro_rs_542_test_bigger_string_to_smaller_fixed_resolution() -> TestResult {
let schema = Schema::parse_str(
r#"
{
"name": "test",
"type": "record",
"fields": [{
"name": "test_field",
"type": {
"name": "myFixed",
"type": "fixed",
"size": 2
}
}]
}"#,
)?;

let long_string = "This string is too long and won't fit!!".to_string();
let value = Value::Record(vec![(
"test_field".to_string(),
Value::String(long_string.clone()),
)]);

assert_eq!(
value.resolve(&schema)
.expect_err("Expected resolution to fail since string is too large for the given fixed schema size")
.into_details()
.to_string(),
Details::CompareFixedSizes { size: 2, n: long_string.len() }.to_string()
);

Ok(())
}

#[test]
fn avro_rs_542_test_smaller_string_to_bigger_fixed_resolution() -> TestResult {
let schema = Schema::parse_str(
r#"
{
"name": "test",
"type": "record",
"fields": [{
"name": "test_field",
"type": {
"name": "myFixed",
"type": "fixed",
"size": 6
}
}]
}"#,
)?;

let mut value = Value::Record(vec![(
"test_field".to_string(),
Value::String("abc".to_string()),
)]);

value = value.resolve(&schema)?;

assert_eq!(
value,
Value::Record(vec![(
"test_field".to_string(),
Value::Fixed(6, vec![97, 98, 99, 0, 0, 0])
)])
);

Ok(())
}

#[test]
fn avro_rs_542_test_smaller_string_to_bigger_fixed_resolution_idempotence() -> TestResult {
let schema = Schema::parse_str(
r#"
{
"name": "test",
"type": "record",
"fields": [{
"name": "test_field",
"type": {
"name": "myFixed",
"type": "fixed",
"size": 6
}
}]
}"#,
)?;

let mut value = Value::Record(vec![(
"test_field".to_string(),
Value::String("abc".to_string()),
)]);

value = value.resolve(&schema)?;
Comment thread
martin-g marked this conversation as resolved.
value = value.resolve(&schema)?;

assert_eq!(
value,
Value::Record(vec![(
"test_field".to_string(),
Value::Fixed(6, vec![97, 98, 99, 0, 0, 0])
)])
);

Ok(())
}
}
2 changes: 1 addition & 1 deletion avro/tests/io.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -127,7 +127,7 @@ fn default_value_examples() -> &'static Vec<(&'static str, &'static str, Value)>
(
r#"{"type": "fixed", "name": "F", "size": 2}"#,
r#""a""#,
Value::Fixed(1, vec![97]),
Value::Fixed(2, vec![97, 0]),
), // ASCII 'a' => one byte
Copy link
Copy Markdown
Member

@martin-g martin-g May 5, 2026

Choose a reason for hiding this comment

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

The comment is outdated here but IMO the padding should be removed. See above.

(
r#"{"type": "fixed", "name": "F", "size": 2}"#,
Expand Down