Improve configuration API of ClientCodecConfigurer.CustomCodecs

[odrotbohm]

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.

[rstoyanchev]

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?

[odrotbohm]

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.

[rstoyanchev]

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]

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.