The new Encoder.encodeValue and Decoder.decode methods in spring 5.2 do not provide access to the Context

[philsttr]

Previously, in spring 5.1, org.springframework.core.codec.Encoder and Decoder implementations could take advantage of the reactor subscriber Context, since all methods returned a Mono or Flux.

In spring 5.2, the following synchronous methods were added (as part of #22782):

• in Encoder... DataBuffer encodeValue(T value, DataBufferFactory bufferFactory, ResolvableType valueType, MimeType mimeType, Map<String, Object> hints)
• in Decoder... T decode(DataBuffer buffer, ResolvableType targetType, MimeType mimeType, Map<String, Object> hints)

These new methods are called instead of the older methods in various places. E.g. EncoderHttpMessageWriter.write calls encoder.encodeValue in spring 5.2, where it previously called encoder.encode in spring 5.1. See also this comment.

These new methods do not provide access to the subscriber Context (since they don't return a Mono/Flux). And there is also no way to force all callers to go back to the previous behavior of calling the old methods which do provide access to the Context.

Therefore, previous Encoder/Decoder implementations that utilized the subscriber Context are now broken in spring 5.2.

I'd like for the subscriber Context to be available in all of the encode*/decode* methods in an Encoder/Decoder.

Perhaps the Context could be added as a hint? Or another default method added that provides the Context as an additional argument?

[rstoyanchev]

This is for custom Encoder and Decoder implementations I assume?

A custom Encoder would be wrapped with EncoderHttpMessageWriter so some solution there (like adding the context to the hints) could be sufficient. This could be property driven, e.g. true by default and set to false by built-in encoders which don't need it.

A custom Decoder shouldn't require any change, as far as I can tell, since DecoderHttpMessageReader decodes to Flux or Mono, the new decode method is entirely optional and used internally mostly in built-in implementations.

What do you think?

[philsttr]

Yes, this is for a custom Encoder/Decoder.

As a temporary workaround (until this issue is addressed), I was thinking about extending EncoderHttpMessageWriter to make it work like it did in spring 5.1 if the inputStream was a Mono (i.e. call encode rather than encodeValue).

I haven't actually looked to see all the places where the new methods are used. If it is limited to EncoderHttpMessageWriter, then I guess one of two fixes could be made there:

1. have something that made EncoderHttpMessageWriter always call encode, rather than encodeValue (i.e. revert to spring 5.1 behavior), OR
2. have something that made EncoderHttpMessageWriter pass the context as a hint to encodeValue

The more I think about it, however, the more I think that having the context as a hint seems hacky.

And neither of these implementations handle all the possible locations that the new synchronous methods could be used. So, this would limit any Encoder that uses the context to only be able to be used from the newly patched EncoderHttpMessageWriter. I.e. it couldn't be used in any other location that called encodeValue that doesn't put the context in a hint.

What if the new synchronous encodeValue and decode methods were revisited, to instead take/return Mono? This would allow implementations to access the context normally (via Mono.subscriberContext()). And it also seems more inline with the fact that the Encoder/Decoder are reactive Encoder/Decoders. i.e. It seems odd for a synchronous encode/decode method to exist in reactive Encoder/Decoders.

e.g.:

• In Encoder... default Mono<DataBuffer> encodeValue(Mono<T> value, DataBufferFactory bufferFactory, ResolvableType valueType, @Nullable MimeType mimeType, @Nullable Map<String, Object> hints)
• In Decoder... default Mono<T> decode(Mono<DataBuffer> buffer, ResolvableType targetType, @Nullable MimeType mimeType, @Nullable Map<String, Object> hints)

I know this would mean immediately deprecating the newly added synchronous methods, and adding another default reactive method in their place. But, it does seem "cleaner" to have only reactive encode/decode methods in Encoder/Decoder, and to allow implementations to access the context "normally". Also, the default implementation of the new methods might be able to delegate to the other existing methods, rather than throw an exception. (I haven't completely thought that through though)

[rstoyanchev]

At the core, there is a split between use cases with byte streams like HTTP and use cases with discrete messages like RSocket.

For the message-oriented use cases, we are always processing discrete payloads that don't need wrapping to begin with but it's the Mono return value that proved a bigger challenge. I did experiment and it complicates the RSocket implementation quite a bit, especially where we call repeatedly APIs that expect a concrete value (e.g. multiple metadata entries). It also made the code unnecessarily nested throughout.

Taking a step back, by comparison it would be quite straight forward to restore the previous behavior of EncoderHttpMessageWriter and always invoke the Flux encode, and still apply special processing for Mono like setting the content-length. The same is true for all HTTP usage. Remember the contract in HTTP is HttpMessageReader and HttpMessageWriter, which guarantee treatment as an async stream. SseHttpMessageWriter is the only other place in HTTP that needs to be reverted and where an Encoder may be invoked directly.

In light of this, I'd like to refine the Encoder and Decoder docs to indicate the methods with a synchronous value are for use with messaging where the processing is naturally organized around discrete values, and that otherwise the Flux encode methods should be used consistently.

I do understand this imposes a limitation on the use of reactive context within encoders and decoders but in a messaging scenario this contract is more naturally suited, and when the need arises we'll provide a way to extract and pass additional context much like we pass hints to encoders/decoders today with HTTP. Those hints can come from anywhere, including the Reactor Context.

What do you think?

[philsttr]

Ok. That will definitely solve my immediate use case (being able to use the context within encoders used for http)

I'm still a little concerned that these encoders will not be able to be used as-is in other places where encodeValue is called. But I can live with that for now.

Also, for future consideration... I would prefer if access to the context was provided to the encoder, rather than having to pluck things out of the context in order to provide them as hints to the encoder. We have various cross-cutting libraries that provide apis for dealing with the context, rather than exposing what they put into the context directly to app code.

[rstoyanchev]

We have various cross-cutting libraries that provide apis for dealing with the context

What does this API look like? A representative snippet would suffice, just to get a sense and be able to reason more concretely about it.

[philsttr]

The cross-cutting libraries that work with the context generally provide two types of APIs:

1. The primary (preferred) APIs are reactive methods that return Mono/Flux, which just retrieve the context via Mono.subsriberContext() internally, and do something with it. These APIs are used within the application's reactive methods. And these are the methods being used in the encoder/decoder currently. In this case, the application never references the context, or anything inside of it.
2. The secondary APIs are synchronous methods that take a context as an argument. These can be used, for example, within a .doOnEach handler in an application. In this case, the application does have a reference to the context, but it still doesn't directly reference anything inside of it. These synchronous methods could be used by the encodeValue implementation if the context was provided as a hint.

The key point is that, in both cases, everything within the context is encapsulated within the library, and is never referenced directly by the application.

[rstoyanchev]

I have restored the status quo for HTTP processing for now, which should address this regression.