Skip to content
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.

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

Empty message body fix #1889

Merged
merged 7 commits into from Feb 7, 2023
Merged

Empty message body fix #1889

merged 7 commits into from Feb 7, 2023

Conversation

jpalac
Copy link
Contributor

@jpalac jpalac commented Feb 2, 2023

Fix #1249

@danielmarbach danielmarbach added this to the 6.1.0 milestone Feb 2, 2023
@danielmarbach
Copy link
Contributor

Rebased and added some minor tweaks. Would it be possible to add one or two acceptance tests?

@jpalac
Copy link
Contributor Author

jpalac commented Feb 7, 2023

Unfortunately found that Amazon SQS requires the MessageBody to have at least 1 character, so I have reverted back to using the "empty message" magic :-(

This will also make the decision document irrelevant.

@danielmarbach
Copy link
Contributor

What if we did send {} instead? This would at least make some sense because the wrapper is json. We could byte encode this once and then always send that

@jpalac
Copy link
Contributor Author

jpalac commented Feb 7, 2023

What if we did send {} instead? This would at least make some sense because the wrapper is json. We could byte encode this once and then always send that

Currently with the empty message, we don't base64 encode it, and then check for that string to also not decode it. If we get {}, couldn't that be a real value that is sent through by the user? In that case if they have encoding on, then we should be encoding it.

@danielmarbach
Copy link
Contributor

I think the same argument could be applied to "empty message". So in essence, it boils down to saying there is a representation of an empty message that is not base64 encoded. That is essentially what the transport did, and I think we are now discussing what the better representation of an empty message is.

@jpalac
Copy link
Contributor Author

jpalac commented Feb 7, 2023

Yes and we talked about it and thought that since we are already using "empty message" then keeping it this way is the path of least resistance in terms of wire compatibility.

@danielmarbach danielmarbach merged commit 69b8ca6 into master Feb 7, 2023
@danielmarbach danielmarbach deleted the EmptyMessageBodyFix branch February 7, 2023 07:54
@jpalac jpalac added the Bug label Mar 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Empty message bodies cause deserialization exception
4 participants