martin-g commented on code in PR #351:
URL: https://github.com/apache/avro-rs/pull/351#discussion_r2597308762
##########
avro/src/ser_schema.rs:
##########
@@ -249,6 +249,9 @@ impl<W: Write> ser::SerializeMap for
SchemaAwareWriteSerializeMap<'_, '_, W> {
pub struct SchemaAwareWriteSerializeStruct<'a, 's, W: Write> {
ser: &'a mut SchemaAwareWriteSerializer<'s, W>,
record_schema: &'s RecordSchema,
+ /// Fields we received in the wrong order
+ field_cache: Vec<(usize, Vec<u8>)>,
+ next_field: usize,
Review Comment:
```suggestion
field_position: usize,
```
I think this will be a better self-documenting name
##########
avro/src/ser_schema.rs:
##########
@@ -268,19 +273,86 @@ impl<'a, 's, W: Write>
SchemaAwareWriteSerializeStruct<'a, 's, W> {
where
T: ?Sized + ser::Serialize,
{
- // If we receive fields in order, write them directly to the main
writer
- let mut value_ser = SchemaAwareWriteSerializer::new(
- &mut *self.ser.writer,
- &field.schema,
- self.ser.names,
- self.ser.enclosing_namespace.clone(),
- );
- self.bytes_written += value.serialize(&mut value_ser)?;
+ if field.position == self.next_field {
+ // If we receive fields in order, write them directly to the main
writer
+ let mut value_ser = SchemaAwareWriteSerializer::new(
+ &mut *self.ser.writer,
+ &field.schema,
+ self.ser.names,
+ self.ser.enclosing_namespace.clone(),
+ );
+ self.bytes_written += value.serialize(&mut value_ser)?;
+ self.next_field += 1;
+ while let Some(index) = self
+ .field_cache
+ .iter()
+ .position(|(pos, _)| pos == &self.next_field)
+ {
+ let (_, bytes) = self.field_cache.remove(index);
+ self.ser
+ .writer
+ .write_all(&bytes)
+ .map_err(Details::WriteBytes)?;
+ self.bytes_written += bytes.len();
+ self.next_field += 1;
+ }
+ } else {
+ // This field is in the wrong order, write it to a temporary
buffer so we can add it at the right time
+ let mut bytes = Vec::new();
+ let mut value_ser = SchemaAwareWriteSerializer::new(
+ &mut bytes,
+ &field.schema,
+ self.ser.names,
+ self.ser.enclosing_namespace.clone(),
+ );
+ value.serialize(&mut value_ser)?;
+ self.field_cache.push((field.position, bytes));
+ }
Ok(())
}
- fn end(self) -> Result<usize, Error> {
+ fn end(mut self) -> Result<usize, Error> {
+ // Write any fields that are `serde(skip)` or `serde(skip_serializing)`
+ while self.next_field != self.record_schema.fields.len() {
+ let field_info = &self.record_schema.fields[self.next_field];
+ if let Some(index) = self
+ .field_cache
+ .iter()
+ .position(|(pos, _)| pos == &self.next_field)
+ {
+ let (_, bytes) = self.field_cache.remove(index);
+ self.ser
+ .writer
+ .write_all(&bytes)
+ .map_err(Details::WriteBytes)?;
+ self.bytes_written += bytes.len();
+ self.next_field += 1;
+ } else if let Some(default) = &field_info.default {
+ self.serialize_next_field(field_info, default)
+ .map_err(|e| Details::SerializeRecordFieldWithSchema {
+ field_name: field_info.name.clone(),
+ record_schema:
Schema::Record(self.record_schema.clone()),
+ error: Box::new(e),
+ })?;
+ } else {
+ return Err(Details::MissingDefaultForSkippedField {
+ field_name: field_info.name.clone(),
+ schema: Schema::Record(self.record_schema.clone()),
+ }
+ .into());
+ }
+ }
+
+ // Check if all fields were written
+ if self.next_field != self.record_schema.fields.len() {
+ let name = self.record_schema.fields[self.next_field].name.clone();
+ return Err(Details::GetField(name).into());
+ }
+ assert!(
+ self.field_cache.is_empty(),
+ "There should be no more unwritten fields at this point"
Review Comment:
Let's print the contents of `self.field_cache` for easier debugging.
##########
avro/src/ser_schema.rs:
##########
@@ -249,6 +249,9 @@ impl<W: Write> ser::SerializeMap for
SchemaAwareWriteSerializeMap<'_, '_, W> {
pub struct SchemaAwareWriteSerializeStruct<'a, 's, W: Write> {
ser: &'a mut SchemaAwareWriteSerializer<'s, W>,
record_schema: &'s RecordSchema,
+ /// Fields we received in the wrong order
+ field_cache: Vec<(usize, Vec<u8>)>,
Review Comment:
Why a Vec ? Wouldn't it be simpler with a `HashMap<usize, Vec<u8>>` ?
##########
avro/src/ser_schema.rs:
##########
@@ -2900,4 +2978,50 @@ mod tests {
string_record.serialize(&mut serializer)?;
Ok(())
}
+
+ #[test]
+ fn different_field_order_serde_vs_schema() -> TestResult {
Review Comment:
```suggestion
fn avro_rs_351_different_field_order_serde_vs_schema() -> TestResult {
```
##########
avro/tests/avro-rs-226.rs:
##########
@@ -128,3 +133,69 @@ fn
avro_rs_226_index_out_of_bounds_with_serde_skip_multiple_fields() -> TestResu
},
)
}
+
+#[test]
+#[should_panic(expected = "Missing default for skipped field 'y' for schema")]
+fn avro_rs_351_no_default_for_serde_skip_serializing_if_should_panic() {
+ #[derive(AvroSchema, Clone, Debug, Deserialize, PartialEq, Serialize)]
+ struct T {
+ x: Option<i8>,
+ #[serde(skip_serializing_if = "Option::is_none")]
+ y: Option<String>,
+ z: Option<i8>,
+ }
+
+ ser_deser::<T>(
+ &T::get_schema(),
+ T {
+ x: None,
+ y: None,
+ z: Some(1),
+ },
+ )
+ .unwrap()
+}
+
+#[test]
+#[should_panic(expected = "Missing default for skipped field 'x' for schema")]
+fn avro_rs_351_no_default_for_serde_skip_should_panic() {
+ #[derive(AvroSchema, Clone, Debug, Deserialize, PartialEq, Serialize)]
+ struct T {
+ #[serde(skip)]
+ x: Option<i8>,
+ y: Option<String>,
+ z: Option<i8>,
+ }
+
+ ser_deser::<T>(
+ &T::get_schema(),
+ T {
+ x: None,
+ y: None,
+ z: Some(1),
+ },
+ )
+ .unwrap()
+}
+
+#[test]
+#[should_panic(expected = "Missing default for skipped field 'z' for schema")]
+fn avro_rs_351_no_default_for_serde_skip_serializing_should_panic() {
+ #[derive(AvroSchema, Clone, Debug, Deserialize, PartialEq, Serialize)]
+ struct T {
+ x: Option<i8>,
+ y: Option<String>,
+ #[serde(skip_serializing)]
+ z: Option<i8>,
+ }
+
+ ser_deser::<T>(
+ &T::get_schema(),
+ T {
+ x: Some(0),
+ y: None,
+ z: None,
+ },
+ )
+ .unwrap()
Review Comment:
Let's use `TestResult` return type and avoid using `.unwrap()`.
##########
avro/tests/avro-rs-226.rs:
##########
@@ -128,3 +133,69 @@ fn
avro_rs_226_index_out_of_bounds_with_serde_skip_multiple_fields() -> TestResu
},
)
}
+
+#[test]
+#[should_panic(expected = "Missing default for skipped field 'y' for schema")]
+fn avro_rs_351_no_default_for_serde_skip_serializing_if_should_panic() {
+ #[derive(AvroSchema, Clone, Debug, Deserialize, PartialEq, Serialize)]
+ struct T {
+ x: Option<i8>,
+ #[serde(skip_serializing_if = "Option::is_none")]
+ y: Option<String>,
+ z: Option<i8>,
+ }
+
+ ser_deser::<T>(
+ &T::get_schema(),
+ T {
+ x: None,
+ y: None,
+ z: Some(1),
+ },
+ )
+ .unwrap()
Review Comment:
Let's use `TestResult` return type and avoid using `.unwrap()`.
##########
avro/src/ser_schema.rs:
##########
@@ -2900,4 +2978,50 @@ mod tests {
string_record.serialize(&mut serializer)?;
Ok(())
}
+
+ #[test]
+ fn different_field_order_serde_vs_schema() -> TestResult {
+ #[derive(Debug, Clone, PartialEq, Serialize, Deserialize)]
+ struct Foo {
+ a: String,
+ b: String,
+ }
+ let schema = Schema::parse_str(
+ r#"
+ {
+ "type":"record",
+ "name":"Foo",
+ "fields": [
+ {
+ "name":"b",
+ "type":"string"
+ },
+ {
+ "name":"a",
+ "type":"string"
+ }
+ ]
+ }
+ "#,
+ )?;
+
+ let mut writer = Writer::new(&schema, Vec::new())?;
+ if let Err(e) = writer.append_ser(Foo {
+ a: "Hello".into(),
+ b: "World".into(),
+ }) {
+ panic!("{e:?}");
Review Comment:
You can use 'append_ser(...)?'. The method returns `TestResult`
##########
avro/src/ser_schema.rs:
##########
@@ -2900,4 +2978,50 @@ mod tests {
string_record.serialize(&mut serializer)?;
Ok(())
}
+
+ #[test]
+ fn different_field_order_serde_vs_schema() -> TestResult {
+ #[derive(Debug, Clone, PartialEq, Serialize, Deserialize)]
+ struct Foo {
+ a: String,
+ b: String,
Review Comment:
Let's make a struct with at least 5 fields. Just to make it a bit more real.
##########
avro/tests/avro-rs-226.rs:
##########
@@ -128,3 +133,69 @@ fn
avro_rs_226_index_out_of_bounds_with_serde_skip_multiple_fields() -> TestResu
},
)
}
+
+#[test]
+#[should_panic(expected = "Missing default for skipped field 'y' for schema")]
+fn avro_rs_351_no_default_for_serde_skip_serializing_if_should_panic() {
+ #[derive(AvroSchema, Clone, Debug, Deserialize, PartialEq, Serialize)]
+ struct T {
+ x: Option<i8>,
+ #[serde(skip_serializing_if = "Option::is_none")]
+ y: Option<String>,
+ z: Option<i8>,
+ }
+
+ ser_deser::<T>(
+ &T::get_schema(),
+ T {
+ x: None,
+ y: None,
+ z: Some(1),
+ },
+ )
+ .unwrap()
+}
+
+#[test]
+#[should_panic(expected = "Missing default for skipped field 'x' for schema")]
+fn avro_rs_351_no_default_for_serde_skip_should_panic() {
+ #[derive(AvroSchema, Clone, Debug, Deserialize, PartialEq, Serialize)]
+ struct T {
+ #[serde(skip)]
+ x: Option<i8>,
+ y: Option<String>,
+ z: Option<i8>,
+ }
+
+ ser_deser::<T>(
+ &T::get_schema(),
+ T {
+ x: None,
+ y: None,
+ z: Some(1),
+ },
+ )
+ .unwrap()
Review Comment:
Let's use `TestResult` return type and avoid using `.unwrap()`.
--
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]
