Fix Issues in DefaultKafkaProducerFactory#updateConfigs()

[NathanQingyangXu]

There are many defects in the original DefaultKafkaProducerFactory#updateConfigs() as below:

#1 public void updateConfigs(Map<String, Object> updates) {
#2		updates.entrySet().forEach(entry -> {
#3			if (entry.getKey().equals(ProducerConfig.TRANSACTIONAL_ID_CONFIG)) {
#4				Assert.isTrue(entry.getValue() instanceof String, () -> "'" + ProducerConfig.TRANSACTIONAL_ID_CONFIG
#5						+ "' must be a String, not a " + entry.getClass().getName());
#6				Assert.isTrue(this.transactionIdPrefix != null
#7								? entry.getValue() != null
#8								: entry.getValue() == null,
#9						"Cannot change transactional capability");
#10				this.transactionIdPrefix = (String) entry.getValue();
#11			}
#12			else if (entry.getKey().equals(ProducerConfig.CLIENT_ID_CONFIG)) {
#13				Assert.isTrue(entry.getValue() instanceof String, () -> "'" + ProducerConfig.CLIENT_ID_CONFIG
#14						+ "' must be a String, not a " + entry.getClass().getName());
#15				this.clientIdPrefix = (String) entry.getValue();
#16			}
#17			else {
#18				this.configs.put(entry.getKey(), entry.getValue());
#19			}
#20		});
#21	}

It has following defects:

1. given the input is a Map, we can't rule out the possibility that both key and value could be null (e.g. in default HashMap); when key is null, Line # 2 will thrown NPE; when value is null; Line # 18 will throw exception for ConcurrentHashMap disallows null key or value (this.configs is based on this impl class);
2. from either Line # 10, it implies null value for ProducerConfig.TRANSACTIONAL_ID_CONFIG is allowed; but when entry.getValue() returns null, the entry.getValue() instanceof String in Line # 4 should have return false without reaching later statements (e.g. Line # 6);
3. obviously in both Line # 5 and Line # 14, the intention was "' must be a String, not a " + entry.getValue().getClass().getName()); entry.getClass() would always return Map.Entry.

[garyrussell]

Thanks; but it is not clear why you found it necessary for all those format changes.

[NathanQingyangXu]

Without those format changes, the gradle checkstyleMain task would fail. A surprise to me for I hated the manual labour, naturally. Any way to bypass it?

…

On Wed, Nov 15, 2023, 10:45 a.m. Gary Russell ***@***.***> wrote: ***@***.**** requested changes on this pull request. Thanks; but it is not clear why you found it necessary for all those format changes. ------------------------------ In spring-kafka/src/main/java/org/springframework/kafka/core/DefaultKafkaProducerFactory.java <#2897 (comment)> : > @@ -165,6 +165,7 @@ public class DefaultKafkaProducerFactory<K, V> extends KafkaResourceFactory /** * Construct a factory with the provided configuration. + * We never have blank lines in method javadocs; they are allowed in class java docs. — Reply to this email directly, view it on GitHub <#2897 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AB6UYATSQLCEV7LZCXV3DDDYETPTBAVCNFSM6AAAAAA7LYR4IGVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMYTOMZSGMZTIMZXGA> . You are receiving this because you authored the thread.Message ID: ***@***.***>

[artembilan]

How about this ./gradlew check -x test?
Probably that checkstyleMain doesn't see our src/checkstyle/checkstyle.xml config.

[NathanQingyangXu]

How about this ./gradlew check -x test? Probably that checkstyleMain doesn't see our src/checkstyle/checkstyle.xml config.

thanks. It works. Lesson learned.

[artembilan]

Doesn't this mean that we are going to exit from a loop on a first key as null?
Therefore we just don't examine all the entries in the map.

[NathanQingyangXu]

No, it simply finishes the BiCinsumer's processing. Also, our unit testing case protects us.

…

On Thu, Nov 16, 2023, 9:41 a.m. Artem Bilan ***@***.***> wrote: ***@***.**** requested changes on this pull request. ------------------------------ In spring-kafka/src/main/java/org/springframework/kafka/core/DefaultKafkaProducerFactory.java <#2897 (comment)> : > @@ -626,23 +626,31 @@ public boolean removePostProcessor(ProducerPostProcessor<K, V> postProcessor) { @OverRide public void updateConfigs(Map<String, Object> updates) { - updates.entrySet().forEach(entry -> { - if (entry.getKey().equals(ProducerConfig.TRANSACTIONAL_ID_CONFIG)) { - Assert.isTrue(entry.getValue() instanceof String, () -> "'" + ProducerConfig.TRANSACTIONAL_ID_CONFIG - + "' must be a String, not a " + entry.getClass().getName()); - Assert.isTrue(this.transactionIdPrefix != null - ? entry.getValue() != null - : entry.getValue() == null, - "Cannot change transactional capability"); - this.transactionIdPrefix = (String) entry.getValue(); + updates.forEach((key, value) -> { + if (key == null) { + return; Doesn't this mean that we are going to exit from a loop on a first key as null? Therefore we just don't examine all the entries in the map. — Reply to this email directly, view it on GitHub <#2897 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AB6UYAUY2TN5UI3X3AFYYJLYEYQZ3AVCNFSM6AAAAAA7LYR4IGVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMYTOMZUGU3TIOJSHA> . You are receiving this because you authored the thread.Message ID: ***@***.***>