spring-boot icon indicating copy to clipboard operation
spring-boot copied to clipboard

Remove SslBundles from KafkaProperties as it is no longer used

Open Torres-09 opened this issue 7 months ago • 4 comments

This PR removes the deprecated buildConsumerProperties(SslBundles) method from the KafkaProperties.

The removed test has been restored and updated to reflect the new logic. All tests and builds pass locally.

assertThatExceptionOfType(MutuallyExclusiveConfigurationPropertiesException.class).isThrownBy(properties::buildConsumerProperties);

gh-45722

Torres-09 avatar May 30 '25 08:05 Torres-09

I’d like to ask for guidance regarding the proper direction for this change.

Since buildConsumerProperties(SslBundles) has been removed, I'm deciding between two implementation options for buildConsumerProperties().

Option 1 – Minimal change (currently applied in PR):

public Map<String, Object> buildConsumerProperties() {
    Map<String, Object> properties = buildCommonProperties(null);
    properties.putAll(this.consumer.buildProperties(null));
    return properties;
}

This keeps the current method signature of buildProperties(SslBundles) and simply passes null, minimizing code changes.

Option 2 – Cleanup approach:

public Map<String, Object> buildConsumerProperties() {
    Map<String, Object> properties = buildCommonProperties(null);
    properties.putAll(this.consumer.buildProperties());
    return properties;
}
public static class Consumer {
    private final Ssl ssl = new Ssl();
    ...

    public Map<String, Object> buildProperties() {
        Properties properties = new Properties();
        PropertyMapper map = PropertyMapper.get().alwaysApplyingWhenNonNull();
        map.from(this::getAutoCommitInterval)
            .asInt(Duration::toMillis)
            ...
            .to(properties.in(ConsumerConfig.MAX_POLL_INTERVAL_MS_CONFIG));
        return properties.with(this.ssl, this.security, this.properties, null);
    }
}

This removes the now-unnecessary parameter entirely and results in a cleaner method.

Functionally, both options appear to be equivalent.

I'd love to hear your thoughts. Thank you!

Torres-09 avatar May 30 '25 08:05 Torres-09

Thanks for noticing this, @Torres-09. I think we should go for the second option. SslBundles appears in the signature of several methods in KafkaProperties but is never actually used. It looks like all references to SslBundles can be removed from KafkaProperties. This will require some small changes to its tests and a couple of configuration classes as well.

wilkinsona avatar May 30 '25 09:05 wilkinsona

@wilkinsona

Thank you for the feedback!

I’ll proceed to remove all remaining references to SslBundles from KafkaProperties, as well as update the related classes and tests accordingly. I’ll revise the PR to reflect these changes.

Torres-09 avatar May 30 '25 09:05 Torres-09

@wilkinsona 👋 I accidentally marked this conversation as resolved and forgot to leave a comment — sorry about that.

Thanks for the review. I've restored the four KafkaPropertiesTests and updated them using .isThrownBy(properties::buildConsumerProperties).

All tests pass locally. Let me know if anything else needs to be adjusted!

Torres-09 avatar Jun 05 '25 17:06 Torres-09

Does the same problem not also apply for buildProducerProperties. Should buildProducerProperties(SslBundles sslBundles) not be removed?

wesboe avatar Jun 16 '25 13:06 wesboe

@wesboe the changes proposed here already do that

wilkinsona avatar Jun 17 '25 07:06 wilkinsona

Good stuff @Torres-09, thank you!

Closes by https://github.com/spring-projects/spring-boot/commit/c86415346d1241846ab8d0120e9d3bae50bc9314

snicoll avatar Jun 17 '25 12:06 snicoll