martin-g commented on code in PR #542:
URL: https://github.com/apache/avro-rs/pull/542#discussion_r3186351825
##########
avro/src/types.rs:
##########
@@ -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);
Review Comment:
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.
##########
avro/tests/io.rs:
##########
@@ -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
Review Comment:
The comment is outdated here but IMO the padding should be removed. See
above.
##########
avro/src/types.rs:
##########
@@ -3494,4 +3502,111 @@ 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)?;
Review Comment:
```suggestion
value = value.resolve(&schema)?;
value = value.resolve(&schema)?;
```
it seems this second resolve was intended.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
