From f642208971b446d65083be8b2eb49b9ba978f0c1 Mon Sep 17 00:00:00 2001
From: Lachlan Deakin <ljdgit@gmail.com>
Date: Thu, 25 Jan 2024 21:13:13 +1100
Subject: [PATCH] More informative `Metadata` deserialisation error message

---
 CHANGELOG.md    |  3 ++-
 src/metadata.rs | 44 +++++++++++++++++++++++++++++---------------
 2 files changed, 31 insertions(+), 16 deletions(-)

diff --git a/CHANGELOG.md b/CHANGELOG.md
index 4f93045d..602a8dfe 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -8,7 +8,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 ## [Unreleased]
 
 ### Added
- - Tests for `ByteRange`, `BytesRepresentation`, `StorePrefix`, `StoreKey`, `ArrayBuilder`, `ArraySubset`, `GroupBuilder`, `Group`, `NodeName`, `NodePath`, `Node`, `AdditionalFields`, `Metadata`, `FillValue`, `Group`
+ - Tests for `ByteRange`, `BytesRepresentation`, `StorePrefix`, `StoreKey`, `ArrayBuilder`, `ArraySubset`, `GroupBuilder`, `Group`, `NodeName`, `NodePath`, `Node`, `AdditionalFields`, `Metadata`, `FillValue`, `Group`, `Metadata`
  - `array_subset::IncompatibleStartEndIndicesError`
  - Add `array::transmute_from_bytes_vec`
  - Re-export public dependencies at the crate root: `bytes`, `bytemuck`, `dyn_clone`, `serde_json`, `ndarray`, `object_store`, and `opendal`
@@ -35,6 +35,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
  - **Breaking**: `InvalidByteRangeError` now holds a `ByteRange` and bytes length and returns a more informative error message
  - **Breaking**: Remove `StorageError::InvalidJSON` and add `StorageError::InvalidMetadata`
    - `InvalidMetadata` additionally holds a `StoreKey` for more informative error messages
+ - More informative `Metadata` deserialisation error message with an invalid configuration
 
 ### Removed
  - **Breaking**: Remove `StorePrefixError::new`, deprecated since `v0.7.3`
diff --git a/src/metadata.rs b/src/metadata.rs
index 92343126..1a9a23be 100644
--- a/src/metadata.rs
+++ b/src/metadata.rs
@@ -90,7 +90,9 @@ impl<'de> serde::Deserialize<'de> for Metadata {
             NameConfiguration(MetadataNameConfiguration),
         }
 
-        let metadata = MetadataIntermediate::deserialize(d)?;
+        let metadata = MetadataIntermediate::deserialize(d).map_err(|_| {
+            serde::de::Error::custom(r#"Expected metadata "<name>" or {"name":"<name>"} or {"name":"<name>","configuration":{}}"#)
+        })?;
         match metadata {
             MetadataIntermediate::Name(name) => Ok(Self {
                 name,
@@ -296,15 +298,31 @@ mod tests {
 
     #[test]
     fn metadata() {
-        assert!(Metadata::try_from(r#""bytes""#).is_ok());
+        let metadata = Metadata::try_from(r#""bytes""#);
+        assert!(metadata.is_ok());
+        assert_eq!(metadata.unwrap().to_string(), r#"bytes"#);
         assert!(Metadata::try_from(r#"{ "name": "bytes" }"#).is_ok());
-        assert!(Metadata::try_from(
-            r#"{ "name": "bytes", "configuration": { "endian": "little" } }"#
-        )
-        .is_ok());
-        assert!(
+        let metadata =
+            Metadata::try_from(r#"{ "name": "bytes", "configuration": { "endian": "little" } }"#);
+        assert!(metadata.is_ok());
+        let metadata = metadata.unwrap();
+        assert_eq!(
+            metadata.to_string(),
+            r#"bytes {"endian": String("little")}"#
+        );
+        assert_eq!(metadata.name(), "bytes");
+        assert!(metadata.configuration().is_some());
+        let configuration = metadata.configuration().unwrap();
+        assert!(configuration.contains_key("endian"));
+        assert_eq!(
+            configuration.get("endian").unwrap().as_str().unwrap(),
+            "little"
+        );
+        assert_eq!(
             Metadata::try_from(r#"{ "name": "bytes", "invalid": { "endian": "little" } }"#)
-                .is_err()
+                .unwrap_err()
+                .to_string(),
+            r#"Expected metadata "<name>" or {"name":"<name>"} or {"name":"<name>","configuration":{}}"#
         );
         let metadata =
             Metadata::try_from(r#"{ "name": "bytes", "configuration": { "endian": "little" } }"#)
@@ -334,13 +352,9 @@ mod tests {
         let additional_fields: AdditionalFields = additional_fields.into();
         let validate = additional_fields.validate();
         assert!(validate.is_err());
-        match validate {
-            Ok(()) => {}
-            Err(err) => {
-                assert_eq!(err.name(), "key");
-                assert_eq!(err.value(), &serde_json::Value::Object(additional_field));
-            }
-        }
+        let err = validate.unwrap_err();
+        assert_eq!(err.name(), "key");
+        assert_eq!(err.value(), &serde_json::Value::Object(additional_field));
     }
 
     #[test]