fix: Review feedback

---------
Co-authored-by: Martin Grigorov <martin-g@users.noreply.github.com>

Author: 2 people

avro/src/documentation/avro_data_model_to_serde.rs

@@ -21,7 +21,7 @@
 //! how the different data models are mapped. When mapping from Rust types to an Avro schema,
 //! see [the documentation for the reverse](super::serde_data_model_to_avro).
 //!
-//! Only the the mapping as defined here is supported. Any other behavior might change in a minor version.
+//! Only the mapping as defined here is supported. Any other behavior might change in a minor version.
 //!
 //! ## Primitive Types
 //! - `null`: `()`
@@ -38,11 +38,11 @@
 //! - `records`: A struct with the same name and fields or a tuple with the same fields.
 //!     - Extra fields can be added to the struct if they are marked with `#[serde(skip)]`
 //! - `enums`: A enum with the same name and unit variants for every symbol
-//!     - The index of the symbol most match the index of the variant
+//!     - The index of the symbol must match the index of the variant
 //! - `arrays`: [`Vec`] (or any type that uses [`Serialize::serialize_seq`](serde::Serialize), [`Deserialize::deserialize_seq`](serde::Deserialize))
 //!     - `[T; N]` is (de)serialized as a tuple by Serde, to (de)serialize them as an `array` use [`apache_avro::serde::array`]
 //! - `maps`: [`HashMap<String, _>`](std::collections::HashMap) (or any type that uses [`Serialize::serialize_map`](serde::Serialize), [`Deserialize::deserialize_map`](serde::Deserialize))
-//! - `unions`: A enum with a variant for each variant
+//! - `unions`: An enum with a variant for each union variant
 //!     - The index of the union variant must match the enum variant
 //!     - A `null` can be a unit variant or a newtype variant with a unit type
 //!     - All other variants must be newtype variants, struct variants, or tuple variants.

avro/src/documentation/serde_data_model_to_avro.rs

@@ -19,7 +19,7 @@
 //!
 //! When manually mapping Rust types to an Avro schema, or the reverse, it is important to understand
 //! how the different data models are mapped. When mapping from an Avro schema to Rust types,
-//! see [the documentation for the reverse](super::serde_data_model_to_avro).
+//! see [the documentation for the reverse](super::avro_data_model_to_serde).
 //!
 //! Only the schemas generated by the [`AvroSchema`] derive and the mapping as defined here are
 //! supported. Any other behavior might change in a minor version.
@@ -47,7 +47,7 @@
 //! - **newtype variant** => See [Enums](##Enums)
 //! - **seq** => [`Schema::Array`]
 //! - **tuple**
-//!     - For tuples with one element, the schema will be the schema the only element
+//!     - For tuples with one element, the schema will be the schema of the only element
 //!     - For tuples with more than one element, the schema will be a [`Schema::Record`] with as many fields as there are elements.
 //!       The schema must have the attribute `org.apache.avro.rust.tuple` with the value set to `true`.
 //!     - **Note:** Serde (de)serializes arrays (`[T; N]`) as tuples. To (de)serialize an array as a
@@ -91,7 +91,7 @@
 //! `content` tag and use the [`Schema::Union`] schema.
 //!
 //! ### Untagged
-//! This enum representation is ued by Serde if the attribute `#[serde(untagged)]` is used. It maps
+//! This enum representation is used by Serde if the attribute `#[serde(untagged)]` is used. It maps
 //! to a [`Schema::Union`] with the following schemas:
 //!   - **unit_variant** => [`Schema::Null`].
 //!   - **newtype_variant** => The schema of the inner type.

avro/src/error.rs

@@ -157,7 +157,9 @@ pub enum Details {
     #[error("Union index {index} out of bounds: {num_variants}")]
     GetUnionVariant { index: i64, num_variants: usize },
 
-    #[error("Enum symbol index out of bounds: {num_variants}")]
+    #[error(
+        "Enum symbol index out of bounds: got {index} but there are only {num_variants} variants"
+    )]
     EnumSymbolIndex { index: usize, num_variants: usize },
 
     #[error("Enum symbol not found {0}")]
@@ -553,7 +555,7 @@ pub enum Details {
     #[error("Decoded integer out of range for i32: {1}: {0}")]
     ZagI32(#[source] std::num::TryFromIntError, i64),
 
-    #[error("unable to read block")]
+    #[error("Did not read any bytes, block is corrupt")]
     ReadBlock,
 
     #[error("Failed to serialize value into Avro value: {0}")]
@@ -661,6 +663,14 @@ pub enum Details {
         "The implementation of `SchemaNameValidator` is incorrect! It returned an out-of-bounds index or provided a regex that did not capture a group named `name`"
     )]
     InvalidSchemaNameValidatorImplementation,
+
+    #[error(
+        "Not all tuple fields were serialized, expected to serialize element at position {position} of a {total_elements}-tuple but `SerializeTuple::end()` was called"
+    )]
+    SerializeTupleMissingElements {
+        position: usize,
+        total_elements: usize,
+    },
 }
 
 #[derive(thiserror::Error, PartialEq)]

avro/src/reader/block.rs

@@ -230,7 +230,8 @@ impl<'r, R: Read> Block<'r, R> {
         let b_original = block_bytes.len();
 
         let item = if read_schema.is_some() {
-            todo!("Schema aware deserialisation does not resolve schemas yet");
+            // TODO: Implement SchemaAwareResolvingDeserializer
+            panic!("Schema aware deserialisation does not resolve schemas yet");
         } else {
             let config = Config {
                 names: &self.names_refs,

avro/src/reader/datum.rs

@@ -146,7 +146,8 @@ impl<'s> GenericDatumReader<'s> {
         // `reader` is `impl Read` instead of a generic on the function like T so it's easier to
         // specify the type wanted (`read_deser<String>` vs `read_deser<String, _>`)
         if let Some((_, _)) = &self.reader {
-            todo!("Schema aware deserialisation does not resolve schemas yet");
+            // TODO: Implement SchemaAwareResolvingDeserializer
+            panic!("Schema aware deserialisation does not resolve schemas yet");
         } else {
             T::deserialize(SchemaAwareDeserializer::new(
                 reader,

avro/src/reader/mod.rs

@@ -69,6 +69,10 @@ impl<'a, R: Read> Reader<'a, R> {
     #[builder(finish_fn = build)]
     pub fn builder(
         #[builder(start_fn)] reader: R,
+        /// The schema the written data needs to be adapted to.
+        ///
+        /// This is currently not compatible with [`ReaderDeser`] and will panic during reading if the
+        /// writer schema is not the same as the reader schema.
         reader_schema: Option<&'a Schema>,
         schemata: Option<Vec<&'a Schema>>,
         #[builder(default = is_human_readable())] human_readable: bool,
@@ -127,6 +131,12 @@ impl<'a, R: Read> Reader<'a, R> {
     }
 
     fn read_next_deser<T: DeserializeOwned>(&mut self) -> AvroResult<Option<T>> {
+        // TODO: Implement SchemaAwareResolvingDeserializer
+        assert!(
+            !self.should_resolve_schema,
+            "Schema aware deserialisation does not resolve schemas yet"
+        );
+
         self.block.read_next_deser(self.reader_schema)
     }
 }

avro/src/schema/mod.rs

@@ -841,6 +841,7 @@ impl Schema {
             | Schema::Ref { name } => {
                 let name: String = name
                     .to_string()
+                    .replace('_', "__")
                     .chars()
                     .map(|c| if c.is_ascii_alphanumeric() { c } else { '_' })
                     .collect();
@@ -5182,4 +5183,29 @@ mod tests {
 
         Ok(())
     }
+
+    #[test]
+    fn avro_rs_512_unique_normalized_name_must_be_unique() -> TestResult {
+        let one = Schema::Ref {
+            name: "a.b.c".parse()?,
+        };
+        let two = Schema::Ref {
+            name: "a_b_c".parse()?,
+        };
+        let three = Schema::Ref {
+            name: "a__b__c".parse()?,
+        };
+        let four = Schema::Ref {
+            name: "a._b._c".parse()?,
+        };
+
+        assert_ne!(one, two);
+        assert_ne!(one, three);
+        assert_ne!(one, four);
+        assert_ne!(two, three);
+        assert_ne!(two, four);
+        assert_ne!(three, four);
+
+        Ok(())
+    }
 }

avro/src/serde/deser_schema/map.rs

@@ -60,7 +60,7 @@ impl<'s, 'r, R: Read, S: Borrow<Schema>> MapDeserializer<'s, 'r, R, S> {
             let _bytes = zag_i64(reader)?;
         }
         if remaining == 0 {
-            // If the block size is zero the array is finished
+            // If the block size is zero the map is finished
             Ok(None)
         } else {
             Ok(Some(remaining.unsigned_abs()))