[Bugfix] Add base64 decode on json mashaller #330

kojisaikiAtSony · 2024-10-24T12:25:26Z

We found an issue with the binary handling of MessageAttributes.

It causes the md5 hash validation error with AWS SDK for Java. Surprisingly, AWS SDK for Go does not have the md5 hash validation...

kojisaikiAtSony · 2024-10-24T12:31:28Z

app/models/responses.go

+		// When encoding json, BinaryValue([]byte) are automatically converted to base64.
+		binary, _ := base64.StdEncoding.DecodeString(attr.Value.BinaryValue)
 		m.MessageAttributes[attr.Name] = sqstypes.MessageAttributeValue{
 			DataType:    &attr.Value.DataType,
-			StringValue: &attr.Value.StringValue,
-			BinaryValue: []byte(attr.Value.BinaryValue),
+			StringValue: attr.Value.StringValue,
+			BinaryValue: binary,


Here is the key change of this PR.

encoding/json lib automatically applies base 64 encode if the arg type is slice.
But our internal MessageAttributeValue.BinaryValue is already base 64 encoded string.
So the base64 encode was done "twice".

I need some more information about what's going on here. I don't really mind not encoding this, but I've gotten several complaints about the MessageAttributes in the last week or so and I need to be more careful about it.

Can you explain more about what the issue is that you found, and how this will solve it? Also the below if you know:

When did the []byte get encoded to begin with? If we're doing it internally, let's just avoid doing it at all, and let the marshallers handle it.

It's been a minute since I looked at this, but why did we have to do the override of MarshalJSON period here? I see your comment about the structure, but I can't see why we needed an array to begin with. It seems to me like it comes in as a map in SendMessage why not maintain the map format and not kill ourselves going back and forth? Was this to be compatible with legacy code some how? I know there's all this "convert to old style" stuff, but I sense that's the essence of a lot of these problems.

The error we faced was like below:

com.amazonaws.AmazonClientException: MD5 returned by SQS does not match the calculation on the original request. (MD5 calculated by the message attributes: "565a1514fba6a202b2f3e2de433300cf", MD5 checksum returned: "1dffa6b307b25550e81d62cf3377dacd")

When did the []byte get encoded to begin with?

The base 64 encoding was performed by AWS SDK Client, and goaws preserves the base 64 encoded string received over HTTP for internal processing. So the value was already encoded at here.

It's been a minute since I looked at this, but why did we have to do the override of MarshalJSON period here?

It's unclear whether we can completely get rid of MarshalJSON while supporting both xml and json, but as you say, tackling the below "old style" TODO will help simplify the MessageAttributes processing.
https://github.com/Admiral-Piett/goaws/blob/master/app/utils/utils.go#L95

Got it, thanks for the extra info here. I wonder if we marked the inbound BinaryValue fields as type []byte the Marshaller would decode them for us on the way in, and all this would be obscured from us. I've always been confused about why it's not that type anyway, but as you say, I assume it's because we just carried the value around and we don't care much about what it is.

Did you happen to test that approach? If it's reasonable to implement it might make a lot of this easier to consume in the long term.

Ok, I'm digging into this now a little more thoroughly as a result of this issue - #328. I think I understand what is going on in here now a little more.

I think the overriding of the MarshalJSON method this way ties our hands, and there's a lot of different "ResultMessageAttribute" VS "ResultAttribute" stuff that makes this hard to consume. I'm going to work on tweaking this a little and seeing if I can bring this piece more in line with the other end points. Stand by!

Ok, ok - I take it back. 🙃 . The message attributes are a real mess and our flexibility is all tied up in this pattern, and to fix my issue I'll have to rebuild it all.

If you, want I'll merge this unblock you if you're ready with all this, but I'm going to refactor the Message Attributes stuff now. That will be my next order of business. So after I do that, I'd appreciate it if you could smoke test it to make sure it still works for your team?

Thoughts on that plan?

OK! We'd be happy to apply this change to resolve our block.
And of course, let me help you on the test with your refactoring on MessageAttributes! 💪 We can use our app and AWS SDK for Java.

As far as I look into #328, this PR won't resolve the issue with only itself. But it is related with jackson-ize process with empty map in go 🤔 We may need to add a condition somewhere to prevent map from being made carelessly.

kojisaikiAtSony · 2024-10-24T12:32:10Z

smoke_tests/sns_publish_test.go

+	// check client md5hash and received md5hash
+	hasher := md5.New()
+	addBytesToHash(hasher, []byte("someKey"))
+	addBytesToHash(hasher, []byte(*messageAttribute.DataType))
+	hasher.Write([]byte{2})
+	addBytesToHash(hasher, messageAttribute.BinaryValue)
+	clientHash := hex.EncodeToString(hasher.Sum(nil))
+
+	assert.Equal(t, clientHash, *receiveMessageResponse.Messages[0].MD5OfMessageAttributes)


Added md5 hash assertion.

kojisaikiAtSony · 2024-10-24T12:33:31Z

smoke_tests/sqs_send_message_batch_test.go

-	assert.Equal(t, "YmFzZTY0LWVuY29kZWQtdmFsdWU=", attr1.Value.BinaryValue) // base64 encoded value
+	assert.Equal(t, binaryValueString, attr1.Value.BinaryValue)


Through the AWS SDK we should receive the raw string instead of the base 64 encoded string.

kojisaikiAtSony · 2024-10-24T12:35:58Z

Hi @Admiral-Piett we found another issue. Could you please check this?

By the way, we found that this fix and the previous fix on nil check are all that is needed to be compatible with the latest AWS SDK for Java!

Admiral-Piett

Hey @kojisaikiAtSony - thanks for looking at this but I need some more info. See my response to your comment below.

Also, what's all this extra stuff if all we needed here was to make sure we're not double encoding something. Thanks!

Admiral-Piett · 2024-10-24T20:09:28Z

app/models/responses.go

+		// When encoding json, BinaryValue([]byte) are automatically converted to base64.
+		binary, _ := base64.StdEncoding.DecodeString(attr.Value.BinaryValue)
 		m.MessageAttributes[attr.Name] = sqstypes.MessageAttributeValue{
 			DataType:    &attr.Value.DataType,
-			StringValue: &attr.Value.StringValue,
-			BinaryValue: []byte(attr.Value.BinaryValue),
+			StringValue: attr.Value.StringValue,
+			BinaryValue: binary,


I need some more information about what's going on here. I don't really mind not encoding this, but I've gotten several complaints about the MessageAttributes in the last week or so and I need to be more careful about it.

Can you explain more about what the issue is that you found, and how this will solve it? Also the below if you know:

When did the []byte get encoded to begin with? If we're doing it internally, let's just avoid doing it at all, and let the marshallers handle it.

It's been a minute since I looked at this, but why did we have to do the override of MarshalJSON period here? I see your comment about the structure, but I can't see why we needed an array to begin with. It seems to me like it comes in as a map in SendMessage why not maintain the map format and not kill ourselves going back and forth? Was this to be compatible with legacy code some how? I know there's all this "convert to old style" stuff, but I sense that's the essence of a lot of these problems.

kojisaikiAtSony · 2024-10-25T08:09:04Z

app/models/responses.go

-	StringValue string `xml:"StringValue,omitempty"`
-	BinaryValue string `xml:"BinaryValue,omitempty"`
+	DataType    string  `xml:"DataType,omitempty"`
+	StringValue *string `xml:"StringValue,omitempty"`


Sorry to miss the description of this change.

string can't have "nil" value and it generates "StringValue": "" definitely on JSON serialize.
AWS SDK for Java won't calculate "BinaryValue" if "StringValue" is not null on their md5 hash validation, so the validation will be failed definitely if a message attribute has "BinaryValue".

Admiral-Piett · 2024-10-28T21:37:52Z

If you agree with all the above give us a rebase and we'll merge this? Then I think we can let this be the last thing to go into the 0.5.1 release. I'll do my refactoring after, so we have something to roll back to. Thanks again!

kojisaikiAtSony · 2024-10-29T00:17:10Z

Thank you for your accepting! We'll rebase soon 👍

kojisaikiAtSony · 2024-10-29T07:01:04Z

Hi @Admiral-Piett , we rebased the branch and the tests passed in our local but Github action test looks failed. Could you please check it and merge this if there was no problem?

Admiral-Piett · 2024-10-29T14:26:03Z

@kojisaikiAtSony are you running your tests with a command like below? Make sure you try it with the -race flag.

go test -race ./app/...

kojisaikiAtSony · 2024-10-30T04:44:51Z

@kojisaikiAtSony are you running your tests with a command like below? Make sure you try it with the -race flag.
go test -race ./app/...

@Admiral-Piett
We tried go test -cover -race ./app/... that is the same command with the github action but these were succeeded...

We are using the below environments:

macOS sonoma + go version go1.22.0 darwin/arm64
Ubuntu 20.04.2 + go version go1.23.2 linux/amd64

Do you have any thoughts on the difference with the Github Action? And, how is it on your local?

Admiral-Piett · 2024-10-31T20:24:07Z

@kojisaikiAtSony Sorry, I haven't had time to look into your test failings, but I will try to do that soon. As an alternative, and like I think we talked about, I finished what I think is a pretty good refactor on the MessageAttributes. I made a comment on PR for you to look at here - https://github.com/Admiral-Piett/goaws/pull/334/files#r1825116530, since I think that piece is the chunk that will really solve your issue.

Essentially it gets us out of the business of needing it encoded or decoded and leaves all that to marshaller. It's pretty slick once we get the right type in place, and it just works. I also kind of flipped your MarshalJSON logic, to make the "special" handling for the XML only, hoping I can delete that some day.

Let me know what you think about any or all of it! I'm happy to discuss. I can also build it for you if you want to test it before I merge it (and the model refactoring - they need to go together). Either way is fine with me, but if we don't decide to put it in v0.5.1 it'll definitely be in v0.5.2, so you'll get this sooner or later. :)

kojisaikiAtSony · 2024-11-01T11:18:15Z

@Admiral-Piett I confirmed #334 will be an alternative to this PR, so I will close this PR later.
But I'd like to recreate a different PR that has the tests in this PR that asserts the hash. That would be required to keep the hash behavior since AWS SDK Go does not perform the hash validation.

Admiral-Piett · 2024-11-01T13:56:48Z

@kojisaikiAtSony Sounds good, if I missed some tests by all means let me know what you're thinking, or if you have tests handy I'm happy to include those too. Whatever works for you. Thanks again for working with me on all this!

Admiral-Piett · 2024-11-01T14:12:00Z

@kojisaikiAtSony 0.5.1 is out! Thanks again for all your help!

kojisaikiAtSony · 2024-11-14T14:05:15Z

I will add some tests for hash validation in separate PR.

kojisaikiAtSony commented Oct 24, 2024

View reviewed changes

Admiral-Piett reviewed Oct 24, 2024

View reviewed changes

kojisaikiAtSony commented Oct 25, 2024

View reviewed changes

Admiral-Piett approved these changes Oct 28, 2024

View reviewed changes

kojisaikiAtSony force-pushed the bugfix/added-base64decode-on-receive-message-api branch from 9fd3026 to b7a428e Compare October 29, 2024 06:21

added base64 decode on json mashaller

8eae00f

kojisaikiAtSony force-pushed the bugfix/added-base64decode-on-receive-message-api branch from b7a428e to 8eae00f Compare October 29, 2024 06:50

Admiral-Piett mentioned this pull request Oct 31, 2024

Refactor MessageAttributes #334

Merged

kojisaikiAtSony closed this Nov 14, 2024

kojisaikiAtSony deleted the bugfix/added-base64decode-on-receive-message-api branch November 14, 2024 14:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Bugfix] Add base64 decode on json mashaller #330

[Bugfix] Add base64 decode on json mashaller #330

kojisaikiAtSony commented Oct 24, 2024

kojisaikiAtSony Oct 24, 2024

Admiral-Piett Oct 24, 2024

kojisaikiAtSony Oct 25, 2024 •

edited

Loading

Admiral-Piett Oct 28, 2024

Admiral-Piett Oct 28, 2024

Admiral-Piett Oct 28, 2024

kojisaikiAtSony Oct 29, 2024

kojisaikiAtSony Oct 29, 2024 •

edited

Loading

kojisaikiAtSony Oct 24, 2024

kojisaikiAtSony Oct 24, 2024

kojisaikiAtSony commented Oct 24, 2024

Admiral-Piett left a comment

Admiral-Piett Oct 24, 2024

kojisaikiAtSony Oct 25, 2024

Admiral-Piett commented Oct 28, 2024

kojisaikiAtSony commented Oct 29, 2024

kojisaikiAtSony commented Oct 29, 2024

Admiral-Piett commented Oct 29, 2024

kojisaikiAtSony commented Oct 30, 2024 •

edited

Loading

Admiral-Piett commented Oct 31, 2024

kojisaikiAtSony commented Nov 1, 2024

Admiral-Piett commented Nov 1, 2024

Admiral-Piett commented Nov 1, 2024

kojisaikiAtSony commented Nov 14, 2024

		assert.Equal(t, "YmFzZTY0LWVuY29kZWQtdmFsdWU=", attr1.Value.BinaryValue) // base64 encoded value
		assert.Equal(t, binaryValueString, attr1.Value.BinaryValue)

[Bugfix] Add base64 decode on json mashaller #330

[Bugfix] Add base64 decode on json mashaller #330

Conversation

kojisaikiAtSony commented Oct 24, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kojisaikiAtSony Oct 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kojisaikiAtSony Oct 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kojisaikiAtSony commented Oct 24, 2024

Admiral-Piett left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Admiral-Piett commented Oct 28, 2024

kojisaikiAtSony commented Oct 29, 2024

kojisaikiAtSony commented Oct 29, 2024

Admiral-Piett commented Oct 29, 2024

kojisaikiAtSony commented Oct 30, 2024 • edited Loading

Admiral-Piett commented Oct 31, 2024

kojisaikiAtSony commented Nov 1, 2024

Admiral-Piett commented Nov 1, 2024

Admiral-Piett commented Nov 1, 2024

kojisaikiAtSony commented Nov 14, 2024

kojisaikiAtSony Oct 25, 2024 •

edited

Loading

kojisaikiAtSony Oct 29, 2024 •

edited

Loading

kojisaikiAtSony commented Oct 30, 2024 •

edited

Loading