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

Add decodeDataBuffer to Decoder and encodeValue to Encoder #22782

Closed
rstoyanchev opened this issue Apr 10, 2019 · 10 comments
Closed

Add decodeDataBuffer to Decoder and encodeValue to Encoder #22782

rstoyanchev opened this issue Apr 10, 2019 · 10 comments
Assignees
Labels
in: web Issues in web modules (web, webmvc, webflux, websocket) type: enhancement A general enhancement
Milestone

Comments

@rstoyanchev
Copy link
Contributor

rstoyanchev commented Apr 10, 2019

In some protocols (HTTP, TCP) the stream of bytes needs to be parsed to be decoded and likewise chunks of encoded content can written out. In other protocols (e.g. RSocket, WebSocket) input and output streams are already split into discrete messages so that each DataBuffer can be decoded and is encoded in full.

Currently Encoder and Decoder contracts are good the former but unnecessarily cumbersome requiring to wrapping in a Mono and joining with DataBufferUtils. Even in WebFlux there are plenty of cases where we've run into this (multipart, form data, etc). We should add decodeDataBuffer to Decoder and encodeValue in Encoder.

Sub-classes of AbstractDataBufferDecoder already have a protected decodeDataBuffer method that does this (on joined buffers). I suspect Jackson and Jaxb2 implementations of decodeToMono could also skip the asynchronous parsing for decoding to Mono and extend this class, see #22783. Likewise Encoder implementations either have methods for encoding one value, or could benefit from one.

@rstoyanchev rstoyanchev added in: web Issues in web modules (web, webmvc, webflux, websocket) type: enhancement A general enhancement labels Apr 10, 2019
@rstoyanchev rstoyanchev added this to the 5.2 M2 milestone Apr 10, 2019
@rstoyanchev rstoyanchev self-assigned this Apr 10, 2019
rstoyanchev added a commit that referenced this issue Apr 11, 2019
@tonydamage
Copy link

This change breaks API compatibility of Encoder, Decoders writen against previous Spring versions.
The use of default methods which throws UnsupportedOperationException is not good pattern, since
it hides the need to implement encodeValue on update of Spring Framework, and moves error from compile time to runtime (which is usually bad).
Looking at implementation this method is not optional, but required if working with Monos.
Better way to handle this change is any of these:

  • default implementation provides backwards compatibility
  • user (HttpEncoder) is able to detect if method is not implemented
  • make method not default - this will break build of encoders which do not implement it, also raises better exception in runtime if wired against encoder which does not implement it.

@rstoyanchev
Copy link
Contributor Author

rstoyanchev commented Dec 6, 2019

Looking at implementation this method is not optional, but required if working with Monos.

For Decoder there is decodeToMono and this new method is entirely optional. It is used in the new spring-messaging reactive infrastructure, and for spring-web it result primarily in internal refactoring. I don't expect that should be able to cause you any trouble.

For Encoder is where I'm guessing you have been impacted? It was initially also meant to be entirely optional but a later optimization was made in EncoderHttpMessagWriter for better handling of single values. My apologies for that. In retrospect that change in EncoderHttpMessagWriter could have been made in backwards compatible manner but it's a bit late for that now.

@seantsb
Copy link

seantsb commented Aug 7, 2020

This caused some massive headaches on our end. Breaking changes like that should be noted in the release notes--can we update those to reflect that?

@seantsb
Copy link

seantsb commented Aug 7, 2020

I guess the right place to make a note would be the wiki page -- I don't think I have edit access but maybe someone can make that note for 5.2?

@rstoyanchev
Copy link
Contributor Author

@seantsb are you referring to the Encoder#encodeValue method and the fact that it must be implemented in order to work with ServerSentEventHttpMessageWriter, or if not could you provide more details?

@seantsb
Copy link

seantsb commented Aug 13, 2020

yep exactly; and more specifically that it will break existing implementations when doing the minor version upgrade to 5.2

@rstoyanchev
Copy link
Contributor Author

Done.

@seantsb
Copy link

seantsb commented Aug 13, 2020

should it be under 5.2 instead?

@rstoyanchev
Copy link
Contributor Author

Yes it should be, thank you.

@seantsb
Copy link

seantsb commented Aug 14, 2020

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: web Issues in web modules (web, webmvc, webflux, websocket) type: enhancement A general enhancement
Projects
None yet
Development

No branches or pull requests

4 participants
@tonydamage @rstoyanchev @seantsb and others