-
Notifications
You must be signed in to change notification settings - Fork 53
fix: accept JSON string defaults for decimal fields in union #533
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.
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -846,6 +846,20 @@ impl Value { | |||||
| Ok(Value::Decimal(Decimal::from(bytes))) | ||||||
| } | ||||||
| } | ||||||
| // JSON string defaults per spec §Records: codepoints 0-255 | ||||||
| // map to byte values 0-255. No precision check — defaults | ||||||
| // need only be valid `bytes`, not fit the declared precision. | ||||||
| Value::String(s) => { | ||||||
| let mut bytes = Vec::with_capacity(s.len()); | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
| for c in s.chars() { | ||||||
| let cp = c as u32; | ||||||
| if cp > 0xFF { | ||||||
| return Err(Details::ResolveDecimal(Value::String(s)).into()); | ||||||
| } | ||||||
| bytes.push(cp as u8); | ||||||
| } | ||||||
| Ok(Value::Decimal(Decimal::from(bytes))) | ||||||
| } | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: Let's use the functional style: Value::String(s) => {
let bytes = s.chars()
.map(|c| {
let cp = c as u32;
if cp > 0xFF {
Err(Details::ResolveDecimal(Value::String(s.clone())).into())
} else {
Ok(cp as u8)
}
})
.collect::<Result<Vec<u8>, Error>>()?;
Ok(Value::Decimal(Decimal::from(bytes)))
} |
||||||
| other => Err(Details::ResolveDecimal(other).into()), | ||||||
| } | ||||||
| } | ||||||
|
|
@@ -1776,6 +1790,69 @@ Field with name '"b"' is not a member of the map items"#, | |||||
| Ok(()) | ||||||
| } | ||||||
|
|
||||||
| #[test] | ||||||
| fn resolve_decimal_from_string_default() -> TestResult { | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
| let value = Value::String("\u{0000}".to_string()); | ||||||
| let resolved = value.resolve(&Schema::Decimal(DecimalSchema { | ||||||
| precision: 10, | ||||||
| scale: 4, | ||||||
| inner: InnerDecimalSchema::Bytes, | ||||||
| }))?; | ||||||
| assert_eq!(resolved, Value::Decimal(Decimal::from(vec![0u8]))); | ||||||
|
|
||||||
| let mut all_bytes_str = String::new(); | ||||||
| for b in 0u8..=255u8 { | ||||||
| all_bytes_str.push(char::from_u32(b as u32).unwrap()); | ||||||
| } | ||||||
| let resolved = Value::String(all_bytes_str).resolve(&Schema::Decimal(DecimalSchema { | ||||||
| precision: 10, | ||||||
| scale: 0, | ||||||
| inner: InnerDecimalSchema::Bytes, | ||||||
| }))?; | ||||||
| assert_eq!( | ||||||
| resolved, | ||||||
| Value::Decimal(Decimal::from((0u8..=255u8).collect::<Vec<_>>())) | ||||||
| ); | ||||||
|
|
||||||
| let value = Value::String("\u{0100}".to_string()); | ||||||
| assert!( | ||||||
| value | ||||||
| .resolve(&Schema::Decimal(DecimalSchema { | ||||||
| precision: 10, | ||||||
| scale: 4, | ||||||
| inner: InnerDecimalSchema::Bytes, | ||||||
| })) | ||||||
| .is_err() | ||||||
| ); | ||||||
|
|
||||||
| Ok(()) | ||||||
| } | ||||||
|
|
||||||
| #[test] | ||||||
| fn parse_schema_with_nullable_decimal_string_default() -> TestResult { | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
| let schema_json = r#"{ | ||||||
| "type": "record", | ||||||
| "name": "NullableDecimal", | ||||||
| "fields": [ | ||||||
| { | ||||||
| "name": "amount", | ||||||
| "type": [ | ||||||
| { | ||||||
| "type": "bytes", | ||||||
| "scale": 4, | ||||||
| "precision": 10, | ||||||
| "logicalType": "decimal" | ||||||
| }, | ||||||
| "null" | ||||||
| ], | ||||||
| "default": "\u0000" | ||||||
| } | ||||||
| ] | ||||||
| }"#; | ||||||
| Schema::parse_str(schema_json)?; | ||||||
| Ok(()) | ||||||
| } | ||||||
|
|
||||||
| #[test] | ||||||
| fn resolve_decimal_invalid_scale() { | ||||||
| let value = Value::Decimal(Decimal::from(vec![1, 2])); | ||||||
|
|
||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comment talks about the field's
defaultbut the match arm would be used for any String->Decimal resolve.Most probably we will need to update the error message for
avro-rs/avro/src/error.rs
Line 191 in 5bb5c4b