fix handling of null groupConfiguration#1130
Conversation
Fixes aws#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.
jlizen
left a comment
jlizen
left a comment
There was a problem hiding this comment.
Looks reasonable, just some small doc tweaks
| } | ||
|
|
||
| /// Deserializes any `Default` type, mapping JSON `null` to `T::default()`. | ||
| pub(crate) fn deserialize_nullish<'de, D, T>(deserializer: D) -> Result<T, D::Error> |
There was a problem hiding this comment.
Seems odd that this is the first we've encountered this. Would this helper be useful elsewhere?
There was a problem hiding this comment.
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...)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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), i see no harm in cutting to the helper. No pressure, though.
| #[serde(deserialize_with = "deserialize_lambda_map")] | ||
| #[serde(default)] | ||
| pub user_attributes: HashMap<String, String>, | ||
| #[serde(default, deserialize_with = "deserialize_nullish")] |
There was a problem hiding this comment.
Can we give this field a doc comment mentioning the "null-ish as default" semantics?
| #[serde(deserialize_with = "deserialize_lambda_map")] | ||
| #[serde(default)] | ||
| pub user_attributes: HashMap<String, String>, | ||
| #[serde(default, deserialize_with = "deserialize_nullish")] |
There was a problem hiding this comment.
Can we give this field a doc comment mentioning the "null-ish as default" semantics?
There was a problem hiding this comment.
will do (and the other occurrence)
2 participants
Fixes #1129 .
📬 Issue #, if available: 1129
✍️ Description of changes: I added the "nullish" serde function and test emulating similar treatment for nullish boolean and map fields. this addresses the problem with the groupConfiguration field discussed in the issue report.
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.
🔏 By submitting this pull request
cargo +nightly fmt.cargo clippy --fix.