martin-g commented on code in PR #467:
URL: https://github.com/apache/avro-rs/pull/467#discussion_r2813298810
##########
avro/src/error.rs:
##########
@@ -431,9 +431,21 @@ pub enum Details {
#[error("Duplicate enum symbol {0}")]
EnumSymbolDuplicate(String),
- #[error("Default value for enum must be a string! Got: {0}")]
+ #[error("Default value for an enum must be a string! Got: {0}")]
EnumDefaultWrongType(serde_json::Value),
+ #[error("Default value for an array must be an array! Got: {0}")]
+ ArrayDefaultWrongType(serde_json::Value),
+
+ #[error("Default value for an array must be an array of {0}! Found:
{1:?}")]
+ ArrayDefaultWrongInnerType(Schema, Value),
+
+ #[error("Default value for a map must be a object! Got: {0}")]
+ MapDefaultWrongType(serde_json::Value),
+
+ #[error("Default value for a map must be a object with (String, {0})!
Found: (String, {1:?})")]
Review Comment:
```suggestion
#[error("Default value for a map must be an object with (String, {0})!
Found: (String, {1:?})")]
```
##########
avro/src/error.rs:
##########
@@ -431,9 +431,21 @@ pub enum Details {
#[error("Duplicate enum symbol {0}")]
EnumSymbolDuplicate(String),
- #[error("Default value for enum must be a string! Got: {0}")]
+ #[error("Default value for an enum must be a string! Got: {0}")]
EnumDefaultWrongType(serde_json::Value),
+ #[error("Default value for an array must be an array! Got: {0}")]
+ ArrayDefaultWrongType(serde_json::Value),
+
+ #[error("Default value for an array must be an array of {0}! Found:
{1:?}")]
+ ArrayDefaultWrongInnerType(Schema, Value),
+
+ #[error("Default value for a map must be a object! Got: {0}")]
Review Comment:
```suggestion
#[error("Default value for a map must be an object! Got: {0}")]
```
##########
avro/src/schema/mod.rs:
##########
@@ -5171,4 +5209,180 @@ mod tests {
Ok(())
}
+
+ #[test]
+ fn avro_rs_467_array_default() -> TestResult {
+ let schema = Schema::parse_str(
+ r#"{
+ "type": "array",
+ "items": "string",
+ "default": []
+ }"#,
+ )?;
+
+ let Schema::Array(array) = schema else {
+ panic!("Expected Schema::Array, got {schema:?}");
+ };
+
+ assert_eq!(array.attributes, BTreeMap::new());
+ assert_eq!(array.default, Some(Vec::new()));
+
+ let json = serde_json::to_string(&Schema::Array(array))?;
+ json.contains(r#""default":[]"#);
+
+ Ok(())
+ }
+
+ #[test]
+ fn avro_rs_467_array_default_with_actual_values() -> TestResult {
+ let schema = Schema::parse_str(
+ r#"{
+ "type": "array",
+ "items": "string",
+ "default": ["foo", "bar"]
+ }"#,
+ )?;
+
+ let Schema::Array(array) = schema else {
+ panic!("Expected Schema::Array, got {schema:?}");
+ };
+
+ assert_eq!(array.attributes, BTreeMap::new());
+ assert_eq!(
+ array.default,
+ Some(vec![
+ Value::String("foo".into()),
+ Value::String("bar".into())
+ ])
+ );
+
+ let json = serde_json::to_string(&Schema::Array(array))?;
+ json.contains(r#""default":["foo","bar"]"#);
+
+ Ok(())
+ }
+
+ #[test]
+ fn avro_rs_467_array_default_with_invalid_values() -> TestResult {
+ let err = Schema::parse_str(
+ r#"{
+ "type": "array",
+ "items": "string",
+ "default": [false, true]
+ }"#,
+ )
+ .unwrap_err();
+
+ assert_eq!(
+ err.to_string(),
+ "Default value for an array must be an array of String! Found:
Boolean(false)"
+ );
+
+ Ok(())
+ }
+
+ #[test]
+ fn avro_rs_467_array_default_with_mixed_values() -> TestResult {
+ let err = Schema::parse_str(
+ r#"{
+ "type": "array",
+ "items": "string",
+ "default": ["foo", true]
+ }"#,
+ )
+ .unwrap_err();
+
+ assert_eq!(
+ err.to_string(),
+ "Default value for an array must be an array of String! Found:
Boolean(true)"
+ );
+
+ Ok(())
+ }
+
+ #[test]
+ fn avro_rs_467_map_default() -> TestResult {
+ let schema = Schema::parse_str(
+ r#"{
+ "type": "map",
+ "values": "string",
+ "default": {}
+ }"#,
+ )?;
+
+ let Schema::Map(map) = schema else {
+ panic!("Expected Schema::Map, got {schema:?}");
+ };
+
+ assert_eq!(map.attributes, BTreeMap::new());
+ assert_eq!(map.default, Some(HashMap::new()));
+
+ let json = serde_json::to_string(&Schema::Map(map))?;
+ json.contains(r#""default":{}"#);
+
+ Ok(())
+ }
+
+ #[test]
+ fn avro_rs_467_map_default_with_actual_values() -> TestResult {
+ let schema = Schema::parse_str(
+ r#"{
+ "type": "map",
+ "values": "string",
+ "default": {"foo": "bar"}
+ }"#,
+ )?;
+
+ let Schema::Map(map) = schema else {
+ panic!("Expected Schema::Map, got {schema:?}");
+ };
+
+ let mut hashmap = HashMap::new();
+ hashmap.insert("foo".to_string(), Value::String("bar".into()));
+ assert_eq!(map.attributes, BTreeMap::new());
+ assert_eq!(map.default, Some(hashmap));
+
+ let json = serde_json::to_string(&Schema::Map(map))?;
+ json.contains(r#""default":{"foo":"bar"}"#);
+
+ Ok(())
+ }
+
+ #[test]
+ fn avro_rs_467_map_default_with_invalid_values() -> TestResult {
+ let err = Schema::parse_str(
+ r#"{
+ "type": "map",
+ "values": "string",
+ "default": {"foo": true}
+ }"#,
+ )
+ .unwrap_err();
+
+ assert_eq!(
+ err.to_string(),
+ "Default value for a map must be a object with (String, String)!
Found: (String, Boolean(true))"
Review Comment:
```suggestion
"Default value for a map must be an object with (String,
String)! Found: (String, Boolean(true))"
```
##########
avro/src/schema/mod.rs:
##########
@@ -5171,4 +5209,180 @@ mod tests {
Ok(())
}
+
+ #[test]
+ fn avro_rs_467_array_default() -> TestResult {
+ let schema = Schema::parse_str(
+ r#"{
+ "type": "array",
+ "items": "string",
+ "default": []
+ }"#,
+ )?;
+
+ let Schema::Array(array) = schema else {
+ panic!("Expected Schema::Array, got {schema:?}");
+ };
+
+ assert_eq!(array.attributes, BTreeMap::new());
+ assert_eq!(array.default, Some(Vec::new()));
+
+ let json = serde_json::to_string(&Schema::Array(array))?;
+ json.contains(r#""default":[]"#);
+
+ Ok(())
+ }
+
+ #[test]
+ fn avro_rs_467_array_default_with_actual_values() -> TestResult {
+ let schema = Schema::parse_str(
+ r#"{
+ "type": "array",
+ "items": "string",
+ "default": ["foo", "bar"]
+ }"#,
+ )?;
+
+ let Schema::Array(array) = schema else {
+ panic!("Expected Schema::Array, got {schema:?}");
+ };
+
+ assert_eq!(array.attributes, BTreeMap::new());
+ assert_eq!(
+ array.default,
+ Some(vec![
+ Value::String("foo".into()),
+ Value::String("bar".into())
+ ])
+ );
+
+ let json = serde_json::to_string(&Schema::Array(array))?;
+ json.contains(r#""default":["foo","bar"]"#);
+
+ Ok(())
+ }
+
+ #[test]
+ fn avro_rs_467_array_default_with_invalid_values() -> TestResult {
+ let err = Schema::parse_str(
+ r#"{
+ "type": "array",
+ "items": "string",
+ "default": [false, true]
+ }"#,
+ )
+ .unwrap_err();
+
+ assert_eq!(
+ err.to_string(),
+ "Default value for an array must be an array of String! Found:
Boolean(false)"
+ );
+
+ Ok(())
+ }
+
+ #[test]
+ fn avro_rs_467_array_default_with_mixed_values() -> TestResult {
+ let err = Schema::parse_str(
+ r#"{
+ "type": "array",
+ "items": "string",
+ "default": ["foo", true]
+ }"#,
+ )
+ .unwrap_err();
+
+ assert_eq!(
+ err.to_string(),
+ "Default value for an array must be an array of String! Found:
Boolean(true)"
+ );
+
+ Ok(())
+ }
+
+ #[test]
+ fn avro_rs_467_map_default() -> TestResult {
+ let schema = Schema::parse_str(
+ r#"{
+ "type": "map",
+ "values": "string",
+ "default": {}
+ }"#,
+ )?;
+
+ let Schema::Map(map) = schema else {
+ panic!("Expected Schema::Map, got {schema:?}");
+ };
+
+ assert_eq!(map.attributes, BTreeMap::new());
+ assert_eq!(map.default, Some(HashMap::new()));
+
+ let json = serde_json::to_string(&Schema::Map(map))?;
+ json.contains(r#""default":{}"#);
+
+ Ok(())
+ }
+
+ #[test]
+ fn avro_rs_467_map_default_with_actual_values() -> TestResult {
+ let schema = Schema::parse_str(
+ r#"{
+ "type": "map",
+ "values": "string",
+ "default": {"foo": "bar"}
+ }"#,
+ )?;
+
+ let Schema::Map(map) = schema else {
+ panic!("Expected Schema::Map, got {schema:?}");
+ };
+
+ let mut hashmap = HashMap::new();
+ hashmap.insert("foo".to_string(), Value::String("bar".into()));
+ assert_eq!(map.attributes, BTreeMap::new());
+ assert_eq!(map.default, Some(hashmap));
+
+ let json = serde_json::to_string(&Schema::Map(map))?;
+ json.contains(r#""default":{"foo":"bar"}"#);
+
+ Ok(())
+ }
+
+ #[test]
+ fn avro_rs_467_map_default_with_invalid_values() -> TestResult {
+ let err = Schema::parse_str(
+ r#"{
+ "type": "map",
+ "values": "string",
+ "default": {"foo": true}
+ }"#,
+ )
+ .unwrap_err();
+
+ assert_eq!(
+ err.to_string(),
+ "Default value for a map must be a object with (String, String)!
Found: (String, Boolean(true))"
+ );
+
+ Ok(())
+ }
+
+ #[test]
+ fn avro_rs_467_map_default_with_mixed_values() -> TestResult {
+ let err = Schema::parse_str(
+ r#"{
+ "type": "map",
+ "values": "string",
+ "default": {"foo": "bar", "spam": true}
+ }"#,
+ )
+ .unwrap_err();
+
+ assert_eq!(
+ err.to_string(),
+ "Default value for a map must be a object with (String, String)!
Found: (String, Boolean(true))"
Review Comment:
```suggestion
"Default value for a map must be an object with (String,
String)! Found: (String, Boolean(true))"
```
##########
avro/src/schema/mod.rs:
##########
@@ -5171,4 +5209,180 @@ mod tests {
Ok(())
}
+
+ #[test]
+ fn avro_rs_467_array_default() -> TestResult {
+ let schema = Schema::parse_str(
+ r#"{
+ "type": "array",
+ "items": "string",
+ "default": []
+ }"#,
+ )?;
+
+ let Schema::Array(array) = schema else {
+ panic!("Expected Schema::Array, got {schema:?}");
+ };
+
+ assert_eq!(array.attributes, BTreeMap::new());
+ assert_eq!(array.default, Some(Vec::new()));
+
+ let json = serde_json::to_string(&Schema::Array(array))?;
+ json.contains(r#""default":[]"#);
Review Comment:
```suggestion
assert!(json.contains(r#""default":[]"#));
```
##########
avro/src/schema/parser.rs:
##########
@@ -722,16 +737,33 @@ impl Parser {
complex: &Map<String, Value>,
enclosing_namespace: &Namespace,
) -> AvroResult<Schema> {
- complex
+ let types = complex
.get("values")
.ok_or_else(|| Details::GetMapValuesField.into())
- .and_then(|items| self.parse(items, enclosing_namespace))
- .map(|items| {
- Schema::map_with_attributes(
- items,
- self.get_custom_attributes(complex, vec!["values"]),
- )
- })
+ .and_then(|types| self.parse(types, enclosing_namespace))?;
+
+ let default = if let Some(default) = complex.get("default").cloned() {
+ if let Value::Object(_) = default {
+ let crate::types::Value::Map(map) =
crate::types::Value::from(default) else {
Review Comment:
Not caused by this PR:
This could be exploited to trigger the panic at
https://github.com/apache/avro-rs/blob/af56163614c4b45fb0d8cfaec997ed07a75e34ec/avro/src/types.rs#L286
I think we should get rid of this panic!() and log an error/warning + return
`JsonValue::Double(f64::MAX)` (a best effort).
##########
avro/src/schema/parser.rs:
##########
@@ -704,16 +704,31 @@ impl Parser {
complex: &Map<String, Value>,
enclosing_namespace: &Namespace,
) -> AvroResult<Schema> {
- complex
+ let items = complex
.get("items")
.ok_or_else(|| Details::GetArrayItemsField.into())
- .and_then(|items| self.parse(items, enclosing_namespace))
- .map(|items| {
- Schema::array_with_attributes(
- items,
- self.get_custom_attributes(complex, vec!["items"]),
- )
- })
+ .and_then(|items| self.parse(items, enclosing_namespace))?;
+ let default = if let Some(default) = complex.get("default").cloned() {
+ if let Value::Array(_) = default {
+ let crate::types::Value::Array(array) =
crate::types::Value::from(default) else {
+ unreachable!("JsonValue::Array can only become a
Value::Array")
+ };
+ // Check that the default type matches the schema type
+ if let Some(value) = array.iter().find(|v|
!v.validate(&items)) {
Review Comment:
If the `items` contain Schema::Ref then you will need to pass the
`schemata`, i.e. use `Value::validate_internal()`
##########
avro/src/schema/parser.rs:
##########
@@ -722,16 +737,33 @@ impl Parser {
complex: &Map<String, Value>,
enclosing_namespace: &Namespace,
) -> AvroResult<Schema> {
- complex
+ let types = complex
.get("values")
.ok_or_else(|| Details::GetMapValuesField.into())
- .and_then(|items| self.parse(items, enclosing_namespace))
- .map(|items| {
- Schema::map_with_attributes(
- items,
- self.get_custom_attributes(complex, vec!["values"]),
- )
- })
+ .and_then(|types| self.parse(types, enclosing_namespace))?;
+
+ let default = if let Some(default) = complex.get("default").cloned() {
+ if let Value::Object(_) = default {
+ let crate::types::Value::Map(map) =
crate::types::Value::from(default) else {
+ unreachable!("JsonValue::Object can only become a
Value::Map")
+ };
+ // Check that the default type matches the schema type
+ if let Some(value) = map.values().find(|v|
!v.validate(&types)) {
Review Comment:
If the `values` contain Schema::Ref then you will need to pass the
`schemata`, i.e. use `Value::validate_internal()`
##########
avro/src/schema/mod.rs:
##########
@@ -3952,19 +3982,17 @@ mod tests {
]
}
"#;
- let expected = Details::GetDefaultRecordField(
- "f1".to_string(),
- "ns.record1".to_string(),
- r#"{"type":"map","values":"string"}"#.to_string(),
- )
- .to_string();
+
let result = Schema::parse_str(schema_str);
assert!(result.is_err());
let err = result
.map_err(|e| e.to_string())
.err()
.unwrap_or_else(|| "unexpected".to_string());
- assert_eq!(expected, err);
+ assert_eq!(
+ r#"Default value for a map must be a object! Got: "invalid""#,
Review Comment:
```suggestion
r#"Default value for a map must be an object! Got: "invalid""#,
```
##########
avro/src/schema/mod.rs:
##########
@@ -5171,4 +5209,180 @@ mod tests {
Ok(())
}
+
+ #[test]
+ fn avro_rs_467_array_default() -> TestResult {
+ let schema = Schema::parse_str(
+ r#"{
+ "type": "array",
+ "items": "string",
+ "default": []
+ }"#,
+ )?;
+
+ let Schema::Array(array) = schema else {
+ panic!("Expected Schema::Array, got {schema:?}");
+ };
+
+ assert_eq!(array.attributes, BTreeMap::new());
+ assert_eq!(array.default, Some(Vec::new()));
+
+ let json = serde_json::to_string(&Schema::Array(array))?;
+ json.contains(r#""default":[]"#);
+
+ Ok(())
+ }
+
+ #[test]
+ fn avro_rs_467_array_default_with_actual_values() -> TestResult {
+ let schema = Schema::parse_str(
+ r#"{
+ "type": "array",
+ "items": "string",
+ "default": ["foo", "bar"]
+ }"#,
+ )?;
+
+ let Schema::Array(array) = schema else {
+ panic!("Expected Schema::Array, got {schema:?}");
+ };
+
+ assert_eq!(array.attributes, BTreeMap::new());
+ assert_eq!(
+ array.default,
+ Some(vec![
+ Value::String("foo".into()),
+ Value::String("bar".into())
+ ])
+ );
+
+ let json = serde_json::to_string(&Schema::Array(array))?;
+ json.contains(r#""default":["foo","bar"]"#);
+
+ Ok(())
+ }
+
+ #[test]
+ fn avro_rs_467_array_default_with_invalid_values() -> TestResult {
+ let err = Schema::parse_str(
+ r#"{
+ "type": "array",
+ "items": "string",
+ "default": [false, true]
+ }"#,
+ )
+ .unwrap_err();
+
+ assert_eq!(
+ err.to_string(),
+ "Default value for an array must be an array of String! Found:
Boolean(false)"
+ );
+
+ Ok(())
+ }
+
+ #[test]
+ fn avro_rs_467_array_default_with_mixed_values() -> TestResult {
+ let err = Schema::parse_str(
+ r#"{
+ "type": "array",
+ "items": "string",
+ "default": ["foo", true]
+ }"#,
+ )
+ .unwrap_err();
+
+ assert_eq!(
+ err.to_string(),
+ "Default value for an array must be an array of String! Found:
Boolean(true)"
+ );
+
+ Ok(())
+ }
+
+ #[test]
+ fn avro_rs_467_map_default() -> TestResult {
+ let schema = Schema::parse_str(
+ r#"{
+ "type": "map",
+ "values": "string",
+ "default": {}
+ }"#,
+ )?;
+
+ let Schema::Map(map) = schema else {
+ panic!("Expected Schema::Map, got {schema:?}");
+ };
+
+ assert_eq!(map.attributes, BTreeMap::new());
+ assert_eq!(map.default, Some(HashMap::new()));
+
+ let json = serde_json::to_string(&Schema::Map(map))?;
+ json.contains(r#""default":{}"#);
+
+ Ok(())
+ }
+
+ #[test]
+ fn avro_rs_467_map_default_with_actual_values() -> TestResult {
+ let schema = Schema::parse_str(
+ r#"{
+ "type": "map",
+ "values": "string",
+ "default": {"foo": "bar"}
+ }"#,
+ )?;
+
+ let Schema::Map(map) = schema else {
+ panic!("Expected Schema::Map, got {schema:?}");
+ };
+
+ let mut hashmap = HashMap::new();
+ hashmap.insert("foo".to_string(), Value::String("bar".into()));
+ assert_eq!(map.attributes, BTreeMap::new());
+ assert_eq!(map.default, Some(hashmap));
+
+ let json = serde_json::to_string(&Schema::Map(map))?;
+ json.contains(r#""default":{"foo":"bar"}"#);
Review Comment:
```suggestion
assert!(json.contains(r#""default":{"foo":"bar"}"#));
```
##########
avro/src/schema/mod.rs:
##########
@@ -5171,4 +5209,180 @@ mod tests {
Ok(())
}
+
+ #[test]
+ fn avro_rs_467_array_default() -> TestResult {
+ let schema = Schema::parse_str(
+ r#"{
+ "type": "array",
+ "items": "string",
+ "default": []
+ }"#,
+ )?;
+
+ let Schema::Array(array) = schema else {
+ panic!("Expected Schema::Array, got {schema:?}");
+ };
+
+ assert_eq!(array.attributes, BTreeMap::new());
+ assert_eq!(array.default, Some(Vec::new()));
+
+ let json = serde_json::to_string(&Schema::Array(array))?;
+ json.contains(r#""default":[]"#);
+
+ Ok(())
+ }
+
+ #[test]
+ fn avro_rs_467_array_default_with_actual_values() -> TestResult {
+ let schema = Schema::parse_str(
+ r#"{
+ "type": "array",
+ "items": "string",
+ "default": ["foo", "bar"]
+ }"#,
+ )?;
+
+ let Schema::Array(array) = schema else {
+ panic!("Expected Schema::Array, got {schema:?}");
+ };
+
+ assert_eq!(array.attributes, BTreeMap::new());
+ assert_eq!(
+ array.default,
+ Some(vec![
+ Value::String("foo".into()),
+ Value::String("bar".into())
+ ])
+ );
+
+ let json = serde_json::to_string(&Schema::Array(array))?;
+ json.contains(r#""default":["foo","bar"]"#);
Review Comment:
```suggestion
assert!(json.contains(r#""default":["foo","bar"]"#));
```
##########
avro/src/schema/mod.rs:
##########
@@ -5171,4 +5209,180 @@ mod tests {
Ok(())
}
+
+ #[test]
+ fn avro_rs_467_array_default() -> TestResult {
+ let schema = Schema::parse_str(
+ r#"{
+ "type": "array",
+ "items": "string",
+ "default": []
+ }"#,
+ )?;
+
+ let Schema::Array(array) = schema else {
+ panic!("Expected Schema::Array, got {schema:?}");
+ };
+
+ assert_eq!(array.attributes, BTreeMap::new());
+ assert_eq!(array.default, Some(Vec::new()));
+
+ let json = serde_json::to_string(&Schema::Array(array))?;
+ json.contains(r#""default":[]"#);
+
+ Ok(())
+ }
+
+ #[test]
+ fn avro_rs_467_array_default_with_actual_values() -> TestResult {
+ let schema = Schema::parse_str(
+ r#"{
+ "type": "array",
+ "items": "string",
+ "default": ["foo", "bar"]
+ }"#,
+ )?;
+
+ let Schema::Array(array) = schema else {
+ panic!("Expected Schema::Array, got {schema:?}");
+ };
+
+ assert_eq!(array.attributes, BTreeMap::new());
+ assert_eq!(
+ array.default,
+ Some(vec![
+ Value::String("foo".into()),
+ Value::String("bar".into())
+ ])
+ );
+
+ let json = serde_json::to_string(&Schema::Array(array))?;
+ json.contains(r#""default":["foo","bar"]"#);
+
+ Ok(())
+ }
+
+ #[test]
+ fn avro_rs_467_array_default_with_invalid_values() -> TestResult {
+ let err = Schema::parse_str(
+ r#"{
+ "type": "array",
+ "items": "string",
+ "default": [false, true]
+ }"#,
+ )
+ .unwrap_err();
+
+ assert_eq!(
+ err.to_string(),
+ "Default value for an array must be an array of String! Found:
Boolean(false)"
+ );
+
+ Ok(())
+ }
+
+ #[test]
+ fn avro_rs_467_array_default_with_mixed_values() -> TestResult {
+ let err = Schema::parse_str(
+ r#"{
+ "type": "array",
+ "items": "string",
+ "default": ["foo", true]
+ }"#,
+ )
+ .unwrap_err();
+
+ assert_eq!(
+ err.to_string(),
+ "Default value for an array must be an array of String! Found:
Boolean(true)"
+ );
+
+ Ok(())
+ }
+
+ #[test]
+ fn avro_rs_467_map_default() -> TestResult {
+ let schema = Schema::parse_str(
+ r#"{
+ "type": "map",
+ "values": "string",
+ "default": {}
+ }"#,
+ )?;
+
+ let Schema::Map(map) = schema else {
+ panic!("Expected Schema::Map, got {schema:?}");
+ };
+
+ assert_eq!(map.attributes, BTreeMap::new());
+ assert_eq!(map.default, Some(HashMap::new()));
+
+ let json = serde_json::to_string(&Schema::Map(map))?;
+ json.contains(r#""default":{}"#);
Review Comment:
```suggestion
assert!(json.contains(r#""default":{}"#));
```
--
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]
