529: Fixed conversion from byte array to String

[lmeyer4]

Arrays.toString converts the byte array into a String containing the numeric representation of each byte, e.g. [10, 29, 15]. What we actually want is a conversion into the characters, which the bytes represent.

This prevents a correct conversion of the payload from jms messages.

[lmeyer4]

Looks like the build failed because SonarQube wasn't available. What am I supposed to do now?

[svettwer]

Hi!

I'll have a look. It's the first external PR since we introduced sonarcloud. Maybe we missed some configuration.

BR,
Sven

[lmeyer4]

Hi, thank you :-)

[christophd]

I think we have to think about what this change implies to other payloads that are represented as byte array but not resolve to a String value. I think of binary data such as images, pdf and so on.

The new String(byteArray) would give us some character sequence with lots of undefined unicode chars. I think this is why I initially switched to Arrays.toString. I can remember that the character stream before has caused severe errors just by logging the output.

[lmeyer4]

I see. To avoid this I put the conversion directly in JsonMappingValidationCallback. I guess it solves the problem, but it is rather ugly.

[svettwer]

I think we have to think about what this change implies to other payloads that are represented as byte array but not resolve to a String value. I think of binary data such as images, pdf and so on.

The new String(byteArray) would give us some character sequence with lots of undefined unicode chars. I think this is why I initially switched to Arrays.toString. I can remember that the character stream before has caused severe errors just by logging the output.

We should check this. If this is the case, we're missing some tests here to ensure that binary data works.

BR,
Sven

[christophd]

We should check this. If this is the case, we're missing some tests here to ensure that binary data works.

You would have to find the binary data that causes some trouble when print to the console with new String(binaryData)and put it into a unit test.

You simply do not want to print String representation of binary data that is not resolvable to a character stream. Your log will explode with non readable chars. That is why we used Arrays.toString().

[christophd]

Let us look at this from another point of view. We get some binary message on JMS and want to validate the payload to an expected String as we know that the binary data is actually a String representation.

Why not using one of the binaryBase64 message validators or introduce a message processor that takes the binary data and converts to a Spring as suggested with new String(binaryData). The user can choose one to many message processors that get called before message validation takes place.

[svettwer]

Hi!

You simply do not want to print String representation of binary data that is not resolvable to a character stream. Your log will explode with non readable chars. That is why we used Arrays.toString().

But this change is not only about logging. Changes in the TypeConversionUtils would affect more than this. The change you mentioned was introduced in #359 (diff). If the change was required in #359 to fix the issue, a test should have failed if the changes of this PR broke something. So if the changes of this PR break something and no test failed, we're missing a test case.

BR,
Sven

[svettwer]

@lmeyer4 Could please open an issue concerning the change, describing what's wrong with the current implementation from your perspective, including a test case sample?

This would be very helpful for further discussion. =)

BR,
Sven

[christophd]

So if the changes of this PR break something and no test failed, we're missing a test case

Agreed

[lmeyer4]

@svettwer Sure, I'll do that later today or tomorrow

[svettwer]

Hi!

Concerning the broken build. There is currently a bug in the addon for travis from sonarcloud.

• Forum report
• Jira issue

I'll add the mentioned workaround and rebase your PR branch.

BR,
Sven

[lmeyer4]

I created the issue (#529)

[svettwer]

Hi!

I think we should double check the change from #359 concerning the TypeConversionUtils.

The use of TypeConversionUtils.convertIfNecessary is IMHO to convert a certain payload into a given type. In case of byte[] -> String this is not the case currently, as (T) Arrays.toString((byte[]) target); returns a representation of the byte array as String, but not the byte arrays content as String if this makes sense.

Therefore I'd recommend to revert the change of the TypeConversionUtils.convertIfNecessary implementation and ensure, that a binary payload is detected earlier and a String conversion is prevented. The decision whether it is a binary payload or not may be done when content types inference is available.

BR,
Sven