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

Improve configuration API of ClientCodecConfigurer.CustomCodecs #24124

Closed
odrotbohm opened this issue Dec 3, 2019 · 8 comments
Closed

Improve configuration API of ClientCodecConfigurer.CustomCodecs #24124

odrotbohm opened this issue Dec 3, 2019 · 8 comments
Assignees
Labels
in: web Issues in web modules (web, webmvc, webflux, websocket) status: backported An issue that has been backported to maintenance branches type: enhancement A general enhancement
Milestone

Comments

@odrotbohm
Copy link
Member

The 5.2.2 release includes new API to allow custom decoders to be registered to use the default configuration for decoders. There are a few things that could be improved about that.

  1. The method one has to work with is named withDefaultConfig(…) and takes a Consumer. Wither methods usually return a new instance of the object being called on taking the argument into account. This one here simply provides a configuration abstraction I now have to use to configure my own decoder.
  2. DefaultCodecConfig's maxInMemorySize() returns a nullable Integer. setMaxInMemorySize(…) takes an int. Looks like everyone trying to use those methods will now have to repeat the same null check before copying the value over.
  3. In general I wonder how the current programming model reacts to additional configuration exposed by DefaultCodecConfig. As it's user code that is responsible to copy properties from left to right, new options will not automatically propagated unless the developer finds out about that new property and adds the necessary line of copying.

Have you though about a CustomCodecs.decoderWithDefaultConfig(…) that register the given Decoder applying all applicable defaults? That especially would free developers from having to play catch up with new options added and also resolve the null checks needed for every client.

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged or decided on label Dec 3, 2019
@rstoyanchev
Copy link
Contributor

There is no one contract for codecs to implement for setting these properties and it would be awkward to introduce one, and we'd likely need multiple since some properties apply to some codecs only.

That said what we could do is to add applyDefaultConfig(Object codec) where we check if the given codec is of the same type as any of the default ones, and if so apply all applicable settings to it. This would be useful when you want to opt into every default setting, present and future.

In addition we could add an overloaded applyDefaultConfig(Consumer<DefaultCodecConfig>) to replace the current withDefaultConfig() which would be deprecated. This would be useful for selective application of some default settings only.

How does that sound?

@rstoyanchev rstoyanchev changed the title Improve configuration API of custom codecs on WebClient Improve configuration API of ClientCodecConfigurer.CustomCodecs Dec 3, 2019
@rstoyanchev rstoyanchev added in: web Issues in web modules (web, webmvc, webflux, websocket) type: enhancement A general enhancement status: waiting-for-feedback We need additional information before we can continue labels Dec 3, 2019
@odrotbohm
Copy link
Member Author

odrotbohm commented Dec 3, 2019

That said what we could do is to add applyDefaultConfig(Object codec) where we check if the given codec is of the same type as any of the default ones, and if so apply all applicable settings to it. This would be useful when you want to opt into every default setting, present and future.

That'll cover the particular usecase that we have in Spring HATEOAS and sounds helpful in general. Would that also add the given decoder to the CustomCodecs instance or would you expect me to call …decoder(…) explicitly?

In addition we could add an overloaded applyDefaultConfig(Consumer) to replace the current withDefaultConfig() which would be deprecated. This would be useful for selective application of some default settings only.

Renaming is appreciated. I have to admit I got a bit lost in all the methods that take a callback as you end up declaring a callback that then calls a method taking the next callback. While this can be explored, it's not easy to keep a mental model of what's applied when and the resulting code looks verbose and is not easy to understand I think. See this example:

// Given a custom decoder instance
AbstractJackson2Decoder decoder = …;

// I currently have to write this to get the default applied
….exchangeStrategies(strategies -> strategies.codecs(codecs -> {
  codecs.customCodecs().withDefaultCodecConfig(config -> {
    Integer maxInMemorySize = config.maxInMemorySize();

    if (maxInMemorySize != null) {
      decoder.setMaxInMemorySize(maxInMemorySize);
    }
  });
});

That's quite verbose I think.

@spring-projects-issues spring-projects-issues added status: feedback-provided Feedback has been provided and removed status: waiting-for-feedback We need additional information before we can continue labels Dec 3, 2019
@rstoyanchev rstoyanchev removed status: feedback-provided Feedback has been provided status: waiting-for-triage An issue we've not yet triaged or decided on labels Dec 4, 2019
@rstoyanchev rstoyanchev added this to the 5.2.3 milestone Dec 4, 2019
@rstoyanchev rstoyanchev self-assigned this Dec 4, 2019
@rstoyanchev
Copy link
Contributor

I suppose we could make it a register-and-apply but I'd rather keep the 4 existing registration methods and then have two applyDefaultConfig variants next to that. The apply method that takes a codec can be a vararg too, so that means at most one extra line in the configuration.

@odrotbohm
Copy link
Member Author

Thanks, Rossen. Happy to rewrite that bit of code in case I have overseen anything that could make this shorter. At least to me the presence of the Consumer overloads create the impression of the need to lazily evaluate those reconfigurations which made me shy away from the other overloads as I didn't want to prematurely trigger any of this. I guess quite a bit of that impression can be resolved in the Javadoc of the methods even.

@rstoyanchev
Copy link
Contributor

rstoyanchev commented Dec 4, 2019

Sorry, it sounded like the renaming + the new apply method would be sufficient. It should now be:

WebClient.builder()
	.exchangeStrategies(strategies ->
		strategies.codecs(codecs -> {
			Jackson2JsonDecoder decoder = new Jackson2JsonDecoder();
			Jackson2JsonEncoder encoder = new Jackson2JsonEncoder();
			codecs.customCodecs().decoder(decoder);
			codecs.customCodecs().encoder(encoder);
			codecs.customCodecs().applyDefaultConfig(decoder, encoder);
		})
	)
	.build();

I don't quite follow the comment about prematurely triggering anything. If you could elaborate I can make sure it's properly reflected in the Javadoc, or is it more than that? I can do a pass soon as well so we can be looking something more concrete.

@odrotbohm
Copy link
Member Author

odrotbohm commented Dec 4, 2019

Whenever I see an overload taking a Consumer, it creates the impression that the work implemented in it should rather be done later. If that's the case I assume there is a reason for not doing the work right now. The Javadoc on …exchangeStrategies(…) mentions the possibility of multiple parties trying to configure it as reason for choosing one method over the other. While that might be important from am implementation point of view, it's not too helpful for the client code implementor as it basically assumes knowledge that she doesn't have ("Will someone else try to configure this? How am I supposed to know?"). That at least leads me to always favor the Consumer variant, which might not be needed.

With applyDefaultConfig(…) apparently not registering the decoder instances, what would be a good use case to call this method without also calling decoder(…)?

That said, it still takes (me) quite a bit of getting used to to these verb-less methods that do something to the underlying instance as they don't communicate what's happening: "customCodecs().decoder(…)? Yeah, what about them/it?". Same for ….exchangeStrategies(…). I assume that this is driven by the builder style of configuration but without the term "builder" actually appearing somewhere explicitly that's not really obvious. Starting from ClientCodecConfigurer that info is not even explicit in the type names. I guess renaming strategies to builder helps on the top level but then again, that's user choice. Maybe it's even the deep nesting (webClientBuilder -> exchangeStrategies -> codecs -> customCodecs -> decoder/encode) that creates the challenges here.

Something along the lines of:

WebClient.builder()
  .exchangeStrategies(builder -> builder.registerWithDefaultConfig(decoder))
  .build();

would feel more use case oriented rather than exposing all the internal structure of the configuration and letting the user dig down the abstractions step by step.

@rstoyanchev
Copy link
Contributor

rstoyanchev commented Dec 4, 2019

Whenever I see an overload taking a Consumer, it creates the impression that the work implemented in it should rather be done later. If that's the case I assume there is a reason for not doing the work right now.

Not really. In this case it's simply to let you apply changes to a common ExchangeStrategies.Bulider instance, rather than creating that instance from scratch. This is a very common pattern used widely in WebMvcConfigurer and many others where configuration is meant to be shared.

it basically assumes knowledge that she doesn't have ("Will someone else try to configure this? How am I supposed to know?").

Maybe we need to be more explicit about this but you shouldn't have to guess. Always use the Consumer variant unless you want to set the starting point, and typically that's reserved for Boot itself. To be a good citizen everyone else uses the Consumer.

With applyDefaultConfig(…) apparently not registering the decoder instances, what would be a good use case to call this method without also calling decoder(…)?

My concern is that registerWithDefaultConfig(Object) looks quite odd next to the existing methods. It would make more sense then to have one register(Object) and two registerWithDefaultConfig and deprecate the existing 5 methods. I didn't want to go there. On second thought maybe it's worth the trouble so that existing usages of those methods we'll have to now choose between register and registerWithDefaultConfig, an option that wasn't available before.

For the nesting I agree it's unfortunate to go 3 levels but CodecConfigurer is much to intricate to make it practical to expose shortcuts for it at the level of ExchangeStrategies.Builder. Rather I see the possibility of a codecs(Consumer<CodecConfigurer>) shortcut at the WebClient.Builder level.

WebClient.builder()
	.codecs(codecs -> {
		Jackson2JsonDecoder decoder = new Jackson2JsonDecoder();
		Jackson2JsonEncoder encoder = new Jackson2JsonEncoder();
		codecs.customCodecs().registerWithDefaultConfig(decoder, encoder);
	})
	.build();

@odrotbohm
Copy link
Member Author

That looks nice, much better than before. Thanks!

@rstoyanchev rstoyanchev added the for: backport-to-5.1.x Marks an issue as a candidate for backport to 5.1.x label Dec 12, 2019
@spring-projects-issues spring-projects-issues added status: backported An issue that has been backported to maintenance branches and removed for: backport-to-5.1.x Marks an issue as a candidate for backport to 5.1.x labels Dec 12, 2019
rstoyanchev added a commit that referenced this issue Dec 12, 2019
The new register methods replace the now deprecated
encoder, decoder, reader, and writer methods, and also offer a choice
to opt into default properties such maxInMemorySize, if configured.

See gh-24124
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) status: backported An issue that has been backported to maintenance branches type: enhancement A general enhancement
Projects
None yet
Development

No branches or pull requests

3 participants