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

Align codec configuration in ExchangeStrategies.Builder and WebFluxConfigurer [SPR-15682] #20241

Closed
spring-issuemaster opened this issue Jun 19, 2017 · 7 comments

Comments

Projects
None yet
2 participants
@spring-issuemaster
Copy link
Collaborator

commented Jun 19, 2017

Brian Clozel opened SPR-15682 and commented

While working on a Spring Boot issue regarding codecs configuration, I've noticed that the reactive server and client codec configurations don't provide the same API.

On the server side, we're dealing with a ServerCodecConfigurer:

WebFluxConfigurer.configureHttpMessageCodecs(ServerCodecConfigurer configurer)

The client configuration is dealt with ExchangeStrategies, which declares:

ExchangeStrategies.defaultCodecs(Consumer<ClientCodecConfigurer.ClientDefaultCodecs> consumer);

ExchangeStrategies.customCodecs(Consumer<CodecConfigurer.CustomCodecs> consumer);

Those methods provide extension points that aren't aligned with the server side.

To better align those configurations and allow code reuse in the infrastructure setup, I'd like to add the following (and remove the existing ones?):

// both ClientCodecConfigurer and ServerCodecConfigurer extend CodecConfigurer
ExchangeStrategies.codecs(Consumer<ClientCodecConfigurer> consumer)

Affects: 5.0 RC2

Reference URL: spring-projects/spring-boot#9166

Referenced from: commits 52148a1

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented Jun 19, 2017

Rossen Stoyanchev commented

ExchangeStrategies I think is aligned with HandlerStrategie (i.e. functional client and server packages) rather than with WebFluxConfigurer. That said now that the latter is the common way to configure WebFlux.fn it does look like an inconsistency between client and server configuration.

Also having had some time after these change I must say I am not fond of the way defaultCodecs and customCodecs hide ClientCodecConfigurer. The types names they expose directly aren't meant to be used directly but rather in support of the fluent builder API. They're long and the config looks more complex having two things for what is conceptually one common category.

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented Jun 19, 2017

Arjen Poutsma commented

To understand why I did this, it's better to look at HandlerStrategies instead of ExchangeStrategies: they follow the same pattern.

HandlerStrategies consists of more than codecs; it also contains exception handlers, locale resolver, etc. For some of these strategies, it also has defaults. If we would expose ServerCodecConfigurer, possibly through a consumer, we would have two separate methods to use defaults: one for codecs, and one for everything except the codecs. Needless to say, this would get quite confusing.

While ExchangeStrategies currently does not contain any strategies other than codecs, I decided to use the same technique there for consistency reasons. Moreover, this leaves the possibility of adding additional, non-codec strategies later.

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented Jun 19, 2017

Arjen Poutsma commented

Also having had some time after these change I must say I am not fond of the way defaultCodecs and customCodecs hide ClientCodecConfigurer. The types names they expose directly aren't meant to be used directly but rather in support of the fluent builder API. They're long and the config looks more complex having two things for what is conceptually one common category.

I am not saying I like it either, but most users will probably use lambdas to consume defaultCodecs and customCodecs, so they won't see the types. Alternatively, the only way I can see Exchange- and HandlerStrategies exposing CodecConfigurer is by getting rid of that registerDefaults instance method. We could turn it into a static method, for instance, of move it to another type.

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented Jun 19, 2017

Brian Clozel commented

I've pushed a first commit in 52148a10, but I haven't done anything related to the registerDefaults methods.

Is there something missing here? Could you provide a bit more guidance so I can make the relevant changes and add more tests in that area?

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented Jun 19, 2017

Rossen Stoyanchev commented

Looks right to me.

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented Jun 20, 2017

Arjen Poutsma commented

To me as well.

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented Jun 20, 2017

Brian Clozel commented

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.