fix SNS publish_batch when MessageStructure is JSON #8487
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
bentsku
added
aws:sns
baermat
approved these changes
Jun 15, 2023
| if "default" not in message: | ||
| raise InvalidParameterException( | ||
| "Invalid parameter: Message Structure - No default entry in JSON message body" | ||
| ) | ||
| entry["Message"] = message # noqa |
There was a problem hiding this comment.
Out of curiosity, why do we need the #noqa here?
There was a problem hiding this comment.
Because the type of Message should be str, but I'm already decoding the JSON string so we don't need to downstream, as it needs to be decoded "synchronously" for validation, and once again asynchronously in the publisher when we need to access the data. I could also remove the # noqa and just let the type warning, it might be safer 😅
There was a problem hiding this comment.
I will leave that up to you, thanks for the clarification :)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR fix #8486, the issue is pretty thorough in explaining what the issue is.
Basically, I forgot to add the logic I've implemented in
publishfor thepublish_batch, that would already load the message into a dict if the message structure wasjson. This is now fixed and validated via 2 AWS validated and snapshot tests.I've also moved around some validations in order to be in line with AWS in how and what they validate in what order.