Skip to content

fix handling of null groupConfiguration #1130

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
jct-tympanon wants to merge 3 commits into
base: main
Choose a base branch
from
+168 −166
Open

fix handling of null groupConfiguration #1130

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
d740e6f
fix handling of null groupConfiguration
jct-tympanon Mar 18, 2026
1757858
consolidate nullish deserializers.
jct-tympanon Mar 18, 2026
36d211b
fix documentation and lint workflow errors.
jct-tympanon Mar 18, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
84 changes: 41 additions & 43 deletions lambda-events/src/custom_serde/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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>
Copy link
Collaborator

@jlizen jlizen Mar 18, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems odd that this is the first we've encountered this. Would this helper be useful elsewhere?

Copy link
Collaborator

@jlizen jlizen Mar 18, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I see, this is a hack to work around us not setting the field as Option<T> in the first place. Seems fine. Maybe add that context in the doc comment (prefer using Option<T>, this is for...)

Copy link
Collaborator

@jlizen jlizen Mar 18, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Gotcha, I don't mind scope creeping those in if you feel like it.

Copy link
Author

@jct-tympanon jct-tympanon Mar 18, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wow thanks for the turnaround. i was not expecting a same-day review for a PR.

us not setting the field as Option in the first place [...] Maybe add that context in the doc comment

will do. i looked at the history before starting and i think the gap is really in the cognito doc. they should publish json schema or similar, anything more contractual than just sample JSON. it's not clear that the field should be optional because it's in every provided example.

Would this helper be useful elsewhere?

in short, yes. mentioned in the commit comment, it's nearly identical to several existing functions in the same file and generalizes them (nullish_boolean, lambda_map, and dynamodb_item). i think you could probably remove all of those and just use this one instead.

however that change has a bigger blast radius, and I wasn't in a position to integration test it thoroughly myself. it's not hard though, i can put that in a separate commit on my fork if you want to eyeball it.

Copy link
Collaborator

@jlizen jlizen Mar 18, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For cognito docs, there is a "give feedback" button somewhere on the page. It would be great to give the technical writers with cognito a heads up about the gap.

Either way, we would rather stay defensive against this schema issue and avoid failing the overall deserialization.

Glad to review a PR to migrate others - if we have unit tests guarding that the behavior is unchanged (we should, with fixtures, just make sure we have fixtures exercising the null handling that pass against both approaches), i see no harm in cutting to the helper. No pressure, though.

Copy link
Author

@jct-tympanon jct-tympanon Mar 18, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I dropped some feedback on the doc page per your suggestion. I included a pointer back to the issue report here for motivation.

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())
}
Expand All @@ -107,7 +77,9 @@ where
#[allow(deprecated)]
mod test {
use super::*;

use serde::{Deserialize, Serialize};
use std::collections::HashMap;

#[test]
fn test_deserialize_base64() {
Expand Down Expand Up @@ -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!({
Expand All @@ -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!({
Expand All @@ -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:?}")
Copy link
Author

@jct-tympanon jct-tympanon Mar 18, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

none of the existing test cases demonstrated behavior on a missing field, so added that here.

}

#[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,
}

Expand Down
4 changes: 2 additions & 2 deletions lambda-events/src/event/activemq/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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))]
Expand Down Expand Up @@ -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")]
Copy link
Author

@jct-tympanon jct-tympanon Mar 18, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this one was used a lot. i do think it slightly improves the documentation in the code, since "deserialize_lambda_map" tells you something you already know about the field, whereas "deserialize_nullish" suggests what it's doing.

#[serde(default)]
pub properties: HashMap<String, String>,
/// Catchall to catch any additional fields that were present but not explicitly defined by this struct.
Expand Down
8 changes: 4 additions & 4 deletions lambda-events/src/event/alb/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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,
};
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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`.
Expand Down
45 changes: 22 additions & 23 deletions lambda-events/src/event/apigw/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
use crate::{
custom_serde::{
deserialize_headers, deserialize_lambda_map, deserialize_nullish_boolean, http_method, serialize_headers,
serialize_multi_value_headers,
deserialize_headers, deserialize_nullish, http_method, serialize_headers, serialize_multi_value_headers,
},
encodings::Body,
iam::IamPolicyStatement,
Expand Down Expand Up @@ -39,17 +38,17 @@ pub struct ApiGatewayProxyRequest {
pub query_string_parameters: QueryMap,
#[serde(default, deserialize_with = "query_map::serde::standard::deserialize_empty")]
pub multi_value_query_string_parameters: QueryMap,
#[serde(deserialize_with = "deserialize_lambda_map")]
#[serde(deserialize_with = "deserialize_nullish")]
#[serde(default)]
pub path_parameters: HashMap<String, String>,
#[serde(deserialize_with = "deserialize_lambda_map")]
#[serde(deserialize_with = "deserialize_nullish")]
#[serde(default)]
pub stage_variables: HashMap<String, String>,
#[serde(bound = "")]
pub request_context: ApiGatewayProxyRequestContext,
#[serde(default)]
pub body: Option<String>,
#[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`.
Expand All @@ -76,7 +75,7 @@ pub struct ApiGatewayProxyResponse {
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`.
Expand Down Expand Up @@ -238,12 +237,12 @@ pub struct ApiGatewayV2httpRequest {
#[serde(skip_serializing_if = "QueryMap::is_empty")]
#[serde(serialize_with = "query_map::serde::aws_api_gateway_v2::serialize_query_string_parameters")]
pub query_string_parameters: QueryMap,
#[serde(deserialize_with = "deserialize_lambda_map")]
#[serde(deserialize_with = "deserialize_nullish")]
#[serde(default)]
#[serde(skip_serializing_if = "HashMap::is_empty")]
pub path_parameters: HashMap<String, String>,
pub request_context: ApiGatewayV2httpRequestContext,
#[serde(deserialize_with = "deserialize_lambda_map")]
#[serde(deserialize_with = "deserialize_nullish")]
#[serde(default)]
pub stage_variables: HashMap<String, String>,
#[serde(skip_serializing_if = "Option::is_none")]
Expand Down Expand Up @@ -314,7 +313,7 @@ pub struct ApiGatewayRequestAuthorizer {
rename = "lambda",
default,
skip_serializing_if = "HashMap::is_empty",
deserialize_with = "deserialize_lambda_map"
deserialize_with = "deserialize_nullish"
)]
pub fields: HashMap<String, Value>,
#[serde(skip_serializing_if = "Option::is_none")]
Expand All @@ -335,7 +334,7 @@ pub struct ApiGatewayRequestAuthorizer {
#[derive(Clone, Debug, Default, Deserialize, Eq, PartialEq, Serialize)]
#[serde(rename_all = "camelCase")]
pub struct ApiGatewayRequestAuthorizerJwtDescription {
#[serde(deserialize_with = "deserialize_lambda_map")]
#[serde(deserialize_with = "deserialize_nullish")]
#[serde(default)]
pub claims: HashMap<String, String>,
#[serde(skip_serializing_if = "Option::is_none")]
Expand Down Expand Up @@ -442,7 +441,7 @@ pub struct ApiGatewayV2httpResponse {
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,
pub cookies: Vec<String>,
/// Catchall to catch any additional fields that were present but not explicitly defined by this struct.
Expand Down Expand Up @@ -525,17 +524,17 @@ pub struct ApiGatewayWebsocketProxyRequest {
pub query_string_parameters: QueryMap,
#[serde(default, deserialize_with = "query_map::serde::standard::deserialize_empty")]
pub multi_value_query_string_parameters: QueryMap,
#[serde(deserialize_with = "deserialize_lambda_map")]
#[serde(deserialize_with = "deserialize_nullish")]
#[serde(default)]
pub path_parameters: HashMap<String, String>,
#[serde(deserialize_with = "deserialize_lambda_map")]
#[serde(deserialize_with = "deserialize_nullish")]
#[serde(default)]
pub stage_variables: HashMap<String, String>,
#[serde(bound = "")]
pub request_context: ApiGatewayWebsocketProxyRequestContext,
#[serde(default)]
pub body: Option<String>,
#[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`.
Expand Down Expand Up @@ -815,13 +814,13 @@ pub struct ApiGatewayV2CustomAuthorizerV1Request {
#[serde(deserialize_with = "http_serde::header_map::deserialize", default)]
#[serde(serialize_with = "serialize_headers")]
pub headers: HeaderMap,
#[serde(deserialize_with = "deserialize_lambda_map")]
#[serde(deserialize_with = "deserialize_nullish")]
#[serde(default)]
pub query_string_parameters: HashMap<String, String>,
#[serde(deserialize_with = "deserialize_lambda_map")]
#[serde(deserialize_with = "deserialize_nullish")]
#[serde(default)]
pub path_parameters: HashMap<String, String>,
#[serde(deserialize_with = "deserialize_lambda_map")]
#[serde(deserialize_with = "deserialize_nullish")]
#[serde(default)]
pub stage_variables: HashMap<String, String>,
pub request_context: ApiGatewayV2CustomAuthorizerV1RequestTypeRequestContext,
Expand Down Expand Up @@ -860,14 +859,14 @@ pub struct ApiGatewayV2CustomAuthorizerV2Request {
#[serde(deserialize_with = "http_serde::header_map::deserialize", default)]
#[serde(serialize_with = "serialize_headers")]
pub headers: HeaderMap,
#[serde(deserialize_with = "deserialize_lambda_map")]
#[serde(deserialize_with = "deserialize_nullish")]
#[serde(default)]
pub query_string_parameters: HashMap<String, String>,
pub request_context: ApiGatewayV2httpRequestContext,
#[serde(deserialize_with = "deserialize_lambda_map")]
#[serde(deserialize_with = "deserialize_nullish")]
#[serde(default)]
pub path_parameters: HashMap<String, String>,
#[serde(deserialize_with = "deserialize_lambda_map")]
#[serde(deserialize_with = "deserialize_nullish")]
#[serde(default)]
pub stage_variables: HashMap<String, String>,
/// Catchall to catch any additional fields that were present but not explicitly defined by this struct.
Expand All @@ -890,7 +889,7 @@ pub struct ApiGatewayCustomAuthorizerContext {
pub principal_id: Option<String>,
pub string_key: Option<String>,
pub num_key: Option<i64>,
#[serde(default, deserialize_with = "deserialize_nullish_boolean")]
#[serde(default, deserialize_with = "deserialize_nullish")]
pub bool_key: bool,
/// Catchall to catch any additional fields that were present but not explicitly defined by this struct.
/// Enabled with Cargo feature `catch-all-fields`.
Expand Down Expand Up @@ -993,10 +992,10 @@ pub struct ApiGatewayCustomAuthorizerRequestTypeRequest {
pub query_string_parameters: QueryMap,
#[serde(default, deserialize_with = "query_map::serde::standard::deserialize_empty")]
pub multi_value_query_string_parameters: QueryMap,
#[serde(deserialize_with = "deserialize_lambda_map")]
#[serde(deserialize_with = "deserialize_nullish")]
#[serde(default)]
pub path_parameters: HashMap<String, String>,
#[serde(deserialize_with = "deserialize_lambda_map")]
#[serde(deserialize_with = "deserialize_nullish")]
#[serde(default)]
pub stage_variables: HashMap<String, String>,
pub request_context: ApiGatewayCustomAuthorizerRequestTypeRequestContext,
Expand Down
10 changes: 5 additions & 5 deletions lambda-events/src/event/appsync/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ use serde::{de::DeserializeOwned, Deserialize, Serialize};
use serde_json::Value;
use std::collections::HashMap;

use crate::custom_serde::deserialize_lambda_map;
use crate::custom_serde::deserialize_nullish;

/// Deprecated: `AppSyncResolverTemplate` does not represent resolver events sent by AppSync. Instead directly model your input schema, or use `map[string]string`, `json.RawMessage`,` interface{}`, etc..
#[non_exhaustive]
Expand Down Expand Up @@ -79,7 +79,7 @@ where
pub issuer: Option<String>,
#[serde(default)]
pub username: Option<String>,
#[serde(deserialize_with = "deserialize_lambda_map")]
#[serde(deserialize_with = "deserialize_nullish")]
#[serde(default)]
#[serde(bound = "")]
pub claims: HashMap<String, T1>,
Expand Down Expand Up @@ -139,7 +139,7 @@ where
pub query_string: Option<String>,
#[serde(default)]
pub operation_name: Option<String>,
#[serde(deserialize_with = "deserialize_lambda_map")]
#[serde(deserialize_with = "deserialize_nullish")]
#[serde(default)]
#[serde(bound = "")]
pub variables: HashMap<String, T1>,
Expand All @@ -164,7 +164,7 @@ where
T1: Serialize,
{
pub is_authorized: bool,
#[serde(deserialize_with = "deserialize_lambda_map")]
#[serde(deserialize_with = "deserialize_nullish")]
#[serde(default)]
#[serde(bound = "")]
pub resolver_context: HashMap<String, T1>,
Expand Down Expand Up @@ -229,7 +229,7 @@ where
#[derive(Debug, Default, Clone, Eq, PartialEq, Deserialize, Serialize)]
#[serde(rename_all = "camelCase")]
pub struct AppSyncRequest {
#[serde(deserialize_with = "deserialize_lambda_map")]
#[serde(deserialize_with = "deserialize_nullish")]
#[serde(default)]
#[serde(bound = "")]
pub headers: HashMap<String, Option<String>>,
Expand Down
Loading