martin-g commented on code in PR #479:
URL: https://github.com/apache/avro-rs/pull/479#discussion_r2839272322
##########
avro/src/schema/mod.rs:
##########
@@ -3815,20 +3799,10 @@ mod tests {
]
}
"#;
- let expected = Details::GetDefaultRecordField(
- "f1".to_string(),
- "ns.record1".to_string(),
-
r#"{"name":"ns.record2","type":"record","fields":[{"name":"f1_1","type":"int"}]}"#
- .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!(
+ Schema::parse_str(schema_str).unwrap_err().to_string(),
+ "`default`'s value type of field `f1` in `ns.record1` must be a
`{\"name\":\"ns.record2\",\"type\":\"record\",\"fields\":[{\"name\":\"f1_1\",\"type\":\"int\"}]}`.
Got: String(\"invalid\")"
Review Comment:
nit: Let's use raw string to make a bit more readable. `r#"..."#`
Same for the other similar assertions.
##########
avro/src/error.rs:
##########
@@ -405,6 +405,9 @@ pub enum Details {
#[error("No `type` in complex type")]
GetComplexTypeField,
+ #[error("No `type` in in record field")]
Review Comment:
```suggestion
#[error("No `type` in record field")]
```
##########
avro/src/schema/record/field.rs:
##########
Review Comment:
This needs reworking too.
`logicalType`, `symbols`, `size`, `items` and `values` cannot be at that
level anymore.
##########
avro/src/schema/record/field.rs:
##########
@@ -81,12 +80,14 @@ impl RecordField {
validate_record_field_name(&name)?;
- // TODO: "type" = "<record name>"
- let schema = parser.parse_complex(
- field,
- &enclosing_record.namespace,
- RecordSchemaParseLocation::FromField,
- )?;
+ let ty = field.get("type").ok_or(Details::GetRecordFieldTypeField)?;
+ let schema = parser.parse(ty, &enclosing_record.namespace)?;
+
+ if let Some(logical_type) = field.get("logicalType") {
+ warn!(
+ "Ignored the {enclosing_record}.logicalType property
(`{logical_type}`). It should probably be nested instide the `type` for the
field"
Review Comment:
```suggestion
"Ignored the {enclosing_record}.logicalType property
(`{logical_type}`). It should probably be nested inside the `type` for the
field"
```
##########
avro/src/schema/mod.rs:
##########
@@ -5438,4 +5401,26 @@ mod tests {
Ok(())
}
+
+ #[test]
+ fn avro_rs_476_enum_cannot_be_directly_in_field() -> TestResult {
+ let schema_str = r#"{
+ "type": "record",
+ "name": "ExampleEnum",
+ "namespace": "com.schema",
+ "fields": [
+ {
+ "name": "wrong_enum",
+ "type": "enum",
+ "symbols": ["INSERT", "UPDATE"]
+ }
+ ]
+ }"#;
+ let result = Schema::parse_str(schema_str).unwrap_err();
+ assert_eq!(
+ result.to_string(),
+ "Invalid schema: There is no type called 'enum', if you meant to
define a non-primitive schema, it should be defined inside `type` attribute.
Please review the specification"
Review Comment:
```suggestion
"Invalid schema: There is no type called 'enum', if you meant to
define a non-primitive schema, it should be defined inside `type` attribute.
Please consult with the specification"
```
##########
avro/src/schema/record/field.rs:
##########
@@ -162,10 +163,14 @@ impl RecordField {
.is_ok();
if !resolved {
+ let schemata =
names.values().cloned().collect::<Vec<_>>();
return Err(Details::GetDefaultRecordField(
field_name.to_string(),
record_name.to_string(),
- field_schema.canonical_form(),
+ field_schema
+ .independent_canonical_form(&schemata)
+ .unwrap_or("Unknown".to_string()),
Review Comment:
How about falling back to the canonical form instead of this "Unknown" ?
--
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]
