fix handling of null groupConfiguration (#1130)

* fix handling of null groupConfiguration

Fixes #1129 .

I added the "nullish" serde function and test emulating similar treatment for nullish boolean and map fields. i think the new deserialize_nullish function could supersede deserialize_nullish_boolean, deserialize_lambda_map, and possibly deserialize_lambda_dynamodb_item. but i left that out for a potential follow-up PR, to keep the blast radius small.

* consolidate nullish deserializers.

addressed review feedback for #1129 .

all of the implementations for the removed functions were identical, so they are exactly
equivalent to the generic deserialize_nullish. existing unit tests demonstrate
the equivalence. functions are private to the crate and safe to remove.

I added one assertion which demonstrates failing behavior for missing fields (which are *not*
covered by this deserializer). the behavior for that case is the same before and after
the change, it just wasn't tested.

also updated field documents and the helper function to explain the semantics.

* fix documentation and lint workflow errors.

* test cases for events with nullish behavior

Following CR feedback on #1130 .

The selected cases cover booleans, dynamodb items, maps, and the GroupConfiguration data structure, so we have at least one instance of each nullish use case.

I added a utility function for basic serde verification. I retrofitted it to existing tests for the data structures that got new cases, just so the code is consistent for them. There are a very large number of other similar cases that I left alone to keep the review reasonable.

Author: jct-tympanon

lambda-events/src/custom_serde/mod.rs

@@ -3,7 +3,6 @@ use serde::{
     de::{Deserialize, Deserializer, Error as DeError},
     ser::Serializer,
 };
-use std::collections::HashMap;
 
 #[cfg(feature = "codebuild")]
 pub(crate) mod codebuild_time;
@@ -58,47 +57,18 @@ where
     serializer.serialize_str(&base64::engine::general_purpose::STANDARD.encode(value))
 }
 
-/// Deserializes `HashMap<_>`, mapping JSON `null` to an empty map.
-pub(crate) fn deserialize_lambda_map<'de, D, K, V>(deserializer: D) -> Result<HashMap<K, V>, D::Error>
+/// Deserializes any `Default` type, mapping JSON `null` to `T::default()`.
+///
+/// **Note** null-to-empty semantics are usually clear for container types (Map, Vec, etc).
+/// For most other data types, prefer modeling fields as ```Option<T>``` with #[serde(default)]
+/// instead of using this deserializer. Option preserves information about the message
+/// for the application, and default semantics for the target data type may change
+/// over time without warning.
+pub(crate) fn deserialize_nullish<'de, D, T>(deserializer: D) -> Result<T, D::Error>
 where
     D: Deserializer<'de>,
-    K: serde::Deserialize<'de>,
-    K: std::hash::Hash,
-    K: std::cmp::Eq,
-    V: serde::Deserialize<'de>,
+    T: Default + Deserialize<'de>,
 {
-    // https://github.com/serde-rs/serde/issues/1098
-    let opt = Option::deserialize(deserializer)?;
-    Ok(opt.unwrap_or_default())
-}
-
-#[cfg(feature = "dynamodb")]
-/// Deserializes `Item`, mapping JSON `null` to an empty item.
-pub(crate) fn deserialize_lambda_dynamodb_item<'de, D>(deserializer: D) -> Result<serde_dynamo::Item, D::Error>
-where
-    D: Deserializer<'de>,
-{
-    // https://github.com/serde-rs/serde/issues/1098
-    let opt = Option::deserialize(deserializer)?;
-    Ok(opt.unwrap_or_default())
-}
-
-/// Deserializes `HashMap<_>`, mapping JSON `null` to an empty map.
-#[cfg(any(
-    feature = "alb",
-    feature = "apigw",
-    feature = "cloudwatch_events",
-    feature = "code_commit",
-    feature = "cognito",
-    feature = "sns",
-    feature = "vpc_lattice",
-    test
-))]
-pub(crate) fn deserialize_nullish_boolean<'de, D>(deserializer: D) -> Result<bool, D::Error>
-where
-    D: Deserializer<'de>,
-{
-    // https://github.com/serde-rs/serde/issues/1098
     let opt = Option::deserialize(deserializer)?;
     Ok(opt.unwrap_or_default())
 }
@@ -107,7 +77,9 @@ where
 #[allow(deprecated)]
 mod test {
     use super::*;
+
     use serde::{Deserialize, Serialize};
+    use std::collections::HashMap;
 
     #[test]
     fn test_deserialize_base64() {
@@ -141,7 +113,7 @@ mod test {
     fn test_deserialize_map() {
         #[derive(Deserialize)]
         struct Test {
-            #[serde(deserialize_with = "deserialize_lambda_map")]
+            #[serde(deserialize_with = "deserialize_nullish")]
             v: HashMap<String, String>,
         }
         let input = serde_json::json!({
@@ -160,9 +132,9 @@ mod test {
     #[cfg(feature = "dynamodb")]
     #[test]
     fn test_deserialize_lambda_dynamodb_item() {
-        #[derive(Deserialize)]
+        #[derive(Deserialize, Debug)]
         struct Test {
-            #[serde(deserialize_with = "deserialize_lambda_dynamodb_item")]
+            #[serde(deserialize_with = "deserialize_nullish")]
             v: serde_dynamo::Item,
         }
         let input = serde_json::json!({
@@ -176,13 +148,39 @@ mod test {
         });
         let decoded: Test = serde_json::from_value(input).unwrap();
         assert_eq!(serde_dynamo::Item::from(HashMap::new()), decoded.v);
+
+        let input = serde_json::json!({});
+        let failure = serde_json::from_value::<Test>(input);
+        assert!(failure.is_err(), "Missing field should not default: {failure:?}")
+    }
+
+    #[test]
+    fn test_deserialize_nullish() {
+        #[derive(Debug, Default, Deserialize, PartialEq)]
+        struct Inner {
+            x: u32,
+        }
+        #[derive(Deserialize)]
+        struct Test {
+            #[serde(default, deserialize_with = "deserialize_nullish")]
+            v: Inner,
+        }
+
+        let decoded: Test = serde_json::from_str(r#"{"v": null}"#).unwrap();
+        assert_eq!(decoded.v, Inner::default());
+
+        let decoded: Test = serde_json::from_str(r#"{}"#).unwrap();
+        assert_eq!(decoded.v, Inner::default());
+
+        let decoded: Test = serde_json::from_str(r#"{"v": {"x": 42}}"#).unwrap();
+        assert_eq!(decoded.v, Inner { x: 42 });
     }
 
     #[test]
     fn test_deserialize_nullish_boolean() {
         #[derive(Deserialize)]
         struct Test {
-            #[serde(default, deserialize_with = "deserialize_nullish_boolean")]
+            #[serde(default, deserialize_with = "deserialize_nullish")]
             v: bool,
         }
 

lambda-events/src/event/activemq/mod.rs

@@ -5,7 +5,7 @@ use serde::{Deserialize, Serialize};
 use serde_json::Value;
 use std::collections::HashMap;
 
-use crate::custom_serde::deserialize_lambda_map;
+use crate::custom_serde::deserialize_nullish;
 
 #[non_exhaustive]
 #[cfg_attr(feature = "builders", derive(Builder))]
@@ -54,7 +54,7 @@ pub struct ActiveMqMessage {
     pub data: Option<String>,
     pub broker_in_time: i64,
     pub broker_out_time: i64,
-    #[serde(deserialize_with = "deserialize_lambda_map")]
+    #[serde(deserialize_with = "deserialize_nullish")]
     #[serde(default)]
     pub properties: HashMap<String, String>,
     /// Catchall to catch any additional fields that were present but not explicitly defined by this struct.
@@ -87,14 +87,24 @@ pub struct ActiveMqDestination {
 #[cfg(test)]
 mod test {
     use super::*;
+    use crate::fixtures::verify_serde_roundtrip;
 
     #[test]
     #[cfg(feature = "activemq")]
     fn example_activemq_event() {
-        let data = include_bytes!("../../fixtures/example-activemq-event.json");
-        let parsed: ActiveMqEvent = serde_json::from_slice(data).unwrap();
-        let output: String = serde_json::to_string(&parsed).unwrap();
-        let reparsed: ActiveMqEvent = serde_json::from_slice(output.as_bytes()).unwrap();
-        assert_eq!(parsed, reparsed);
+        verify_serde_roundtrip::<ActiveMqEvent>(include_bytes!("../../fixtures/example-activemq-event.json"));
+    }
+
+    #[test]
+    #[cfg(feature = "activemq")]
+    fn example_activemq_event_null_properties() {
+        let event: ActiveMqEvent = verify_serde_roundtrip(include_bytes!(
+            "../../fixtures/example-activemq-event-null-properties.json"
+        ));
+        assert_eq!(
+            0,
+            event.messages[0].properties.len(),
+            "null properties should deserialize to empty map"
+        )
     }
 }

lambda-events/src/event/alb/mod.rs

@@ -1,7 +1,7 @@
 use crate::{
     custom_serde::{
-        deserialize_headers, deserialize_nullish_boolean, http_method, serialize_headers,
-        serialize_multi_value_headers, serialize_query_string_parameters,
+        deserialize_headers, deserialize_nullish, http_method, serialize_headers, serialize_multi_value_headers,
+        serialize_query_string_parameters,
     },
     encodings::Body,
 };
@@ -35,7 +35,7 @@ pub struct AlbTargetGroupRequest {
     #[serde(serialize_with = "serialize_multi_value_headers")]
     pub multi_value_headers: HeaderMap,
     pub request_context: AlbTargetGroupRequestContext,
-    #[serde(default, deserialize_with = "deserialize_nullish_boolean")]
+    #[serde(default, deserialize_with = "deserialize_nullish")]
     pub is_base64_encoded: bool,
     pub body: Option<String>,
     /// Catchall to catch any additional fields that were present but not explicitly defined by this struct.
@@ -101,7 +101,7 @@ pub struct AlbTargetGroupResponse {
     pub multi_value_headers: HeaderMap,
     #[serde(skip_serializing_if = "Option::is_none")]
     pub body: Option<Body>,
-    #[serde(default, deserialize_with = "deserialize_nullish_boolean")]
+    #[serde(default, deserialize_with = "deserialize_nullish")]
     pub is_base64_encoded: bool,
     /// Catchall to catch any additional fields that were present but not explicitly defined by this struct.
     /// Enabled with Cargo feature `catch-all-fields`.