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, 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.
There was a problem hiding this comment.
I dropped some feedback on the doc page per your suggestion. I included a pointer back to the issue report here for motivation.
| #[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)
addressed review feedback for aws#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.
| pub broker_in_time: i64, | ||
| pub broker_out_time: i64, | ||
| #[serde(deserialize_with = "deserialize_lambda_map")] | ||
| #[serde(deserialize_with = "deserialize_nullish")] |
There was a problem hiding this comment.
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.
|
|
||
| let input = serde_json::json!({}); | ||
| let failure = serde_json::from_value::<Test>(input); | ||
| assert!(failure.is_err(), "Missing field should not default: {failure:?}") |
There was a problem hiding this comment.
none of the existing test cases demonstrated behavior on a missing field, so added that here.
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.