martin-g commented on code in PR #351:
URL: https://github.com/apache/avro-rs/pull/351#discussion_r2597681410
##########
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:
I see.
##########
avro/src/ser_schema.rs:
##########
@@ -268,19 +273,79 @@ 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.field_position {
+ // 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.field_position += 1;
+ while let Some(bytes) =
self.field_cache.remove(&self.field_position) {
+ self.ser
+ .writer
+ .write_all(&bytes)
+ .map_err(Details::WriteBytes)?;
+ self.bytes_written += bytes.len();
+ self.field_position += 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)?;
+ if self.field_cache.insert(field.position, bytes).is_some() {
+ return
Err(Details::FieldNameDuplicate(field.name.clone()).into());
+ }
+ }
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.field_position != self.record_schema.fields.len() {
+ let field_info = &self.record_schema.fields[self.field_position];
+ if let Some(bytes) = self.field_cache.remove(&self.field_position)
{
+ self.ser
+ .writer
+ .write_all(&bytes)
+ .map_err(Details::WriteBytes)?;
+ self.bytes_written += bytes.len();
+ self.field_position += 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.field_position != self.record_schema.fields.len() {
Review Comment:
Is this check really needed ?
The `while` above makes sure of it or returns an Err.
##########
avro/src/ser_schema.rs:
##########
@@ -268,19 +273,79 @@ 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.field_position {
+ // 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.field_position += 1;
+ while let Some(bytes) =
self.field_cache.remove(&self.field_position) {
+ self.ser
+ .writer
+ .write_all(&bytes)
+ .map_err(Details::WriteBytes)?;
+ self.bytes_written += bytes.len();
+ self.field_position += 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)?;
+ if self.field_cache.insert(field.position, bytes).is_some() {
+ return
Err(Details::FieldNameDuplicate(field.name.clone()).into());
+ }
+ }
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.field_position != self.record_schema.fields.len() {
+ let field_info = &self.record_schema.fields[self.field_position];
+ if let Some(bytes) = self.field_cache.remove(&self.field_position)
{
+ self.ser
+ .writer
+ .write_all(&bytes)
+ .map_err(Details::WriteBytes)?;
+ self.bytes_written += bytes.len();
+ self.field_position += 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.field_position != self.record_schema.fields.len() {
+ let name =
self.record_schema.fields[self.field_position].name.clone();
+ return Err(Details::GetField(name).into());
+ }
+ assert!(
Review Comment:
I am not sure about that.
A library should try to avoid panicking if possible.
I think `debug_assert!()` or returning an Err is better.
--
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]
