Support decoding empty DataBuffers for Decoders that support it

[spheenik]

Some decoders support decoding a proper payload from empty DataBuffers. One good example is the ProtobufDecoder.

This patch adds a method canDecodeEmptyDataBuffer() to the Decoder interface, allowing the Decoder to signal that it is able to do that.

In this case, the PayloadMethodArgumentResolver should pass the empty DataBuffer to the Decoder, and in all other cases behave like before.

[spheenik]

In response to the following issue: #29898

[simonbasle]

this sounds reasonable. Can you please at least add some small test coverage. notably there's a ProtobufDecoderTest that could have a testDecodeEmptyBuffer method for this purpose

[spheenik]

I updated the remaining Decoders that are capable to decode empty buffers to also return true on canDecodeEmptyDataBuffers().
I also added tests for the decode() and the decodeToMono() scenarios, and made sure all the marked Decoders support it properly.

[rstoyanchev]

Thanks for raising this issue, and submitting a PR.

A Decoder has 3 kinds of methods, decoding to Flux, Mono, or a value. The Flux and Mono take a Publisher of buffers each of which could be a chunk of data, and not necessarily a complete message (e.g. parsing HTTP request body). In that case, empty data buffers (chunks) are okay, and does not mean the aggregated message in the end will be empty too. Moreover, since Flux and Mono methods aggregate chunks internally, they also deal with empty messages, perhaps producing a value, or completing empty, or with an error.

It's only the decode to a value method which takes a single DataBuffer and where we can apply a check before calling it. For Flux and Mono, we can do the same only when each DataBuffer is a complete message, but at that point you're processing each DataBuffer individually anyway, and might as well call the decode to value method, which is what we do for RSocket.

This is why it's quite confusing to call the method canDecodeEmptyDataBuffer. It would make more sense to call it canDecodeEmptyMessage and document very clearly that it's referring to an empty complete message and not a chunk of data, which is what each DataBuffer could be, so it really depends on the context in which the Decoder is called.

The exact semantics should also be made clear. I'm not too sure does true mean the Decoder will produce a non-null value, or that it will produce null without raising an error, or in the case of Flux and Mono that translates to producing a value, or filtering empty values, or completing with an error?

[spheenik]

Thank you for the detailed explanation of the toMono-Case, and your other remarks. Seems I definitively had a misunderstanding what it is used for.

As a consequence, I renamed the method to canDecodeEmptyMessage, as suggested. I also improved on it's documentation, stating that, if the decoder returns true here, the contract will be that it will never return null when decoding an empty Message. I did not mention the error case though, which of course can happen.

I was wondering whether or not to leave the test in the toMono case in, and decided to do so, and improved on it by giving it a Flux of two empty DataBuffers as an input, to make sure a supporting decoder gives only one non-null result back.

[simonbasle]

I'm concerned about the potential unforeseen side-effects of canDecodeEmptyMessage() == true for a lot of low-level decoders (String, Resource, buffers...). I would reduce the scope and keep most of these decoders with the default value of false, and just add support for the ProtobufDecoder for now. @rstoyanchev what do you think?

[spheenik]

@simonbasle I was also thinking about this, and I can certainly see why you are worried. I came to the conclusion that as long as the decoder makes sure to always return something non null, it should be fine. All the decode- and decodeToMono-Scenarios of the ByteArray-style decoders act in this way anyway, not even looking at the flag.
For the StringDecoder, it even includes a proper fix, instead of the workaround in gh-26344.
Of course, this is only with my limited knowledge, so I am eagerly awaiting @rstoyanchev 's insight.

[spheenik]

I just rechecked, the only place in the whole framework code where canDecodeEmptyMessage() is even used is in the PayloadMethodArgumentResolver in spring-messaging. Everything else is acting like before.

[spheenik]

It's been a while, nothing happened. To make sure, this doesn't get lost, question: Is there anything more I can do?

[snicoll]

@spheenik thank you for following up. We need to take the time to review it one more time to answer your questions.