Data in handle method is @Nullable

[kkocel]

Motivation - Kotlin uses jsr305 annotations to deduct non-null / nullable types. This PR marks data as explicitly nullable (which happen in code).

Without this it's impossible to pass null value as data parameter in Kotlin.

[artembilan]

Please, fix Checkstyle issue:

Error: eckstyle] [ERROR] /home/runner/work/spring-kafka/spring-kafka/spring-kafka/src/main/java/org/springframework/kafka/listener/BatchLoggingErrorHandler.java:26:1: Extra separation in import group before 'org.springframework.lang.Nullable' [ImportOrder]

You can check it locally with Gradle check task.

Thanks

[kkocel]

@artembilan Hi, fixed right away. Sorry for inconvienience.

[artembilan]

I'm OK with this change: doesn't look like it breaks something.
Although I have doubts if we should propagate something reasonable in those default methods instead of null.

Let's see what @garyrussell thinks on the matter!

[garyrussell]

@kkocel Under what circumstances are you seeing it being called with null values?

Even with consumer exceptions (before we receive any ConsumerRecords), the container calls it with empty objects.

spring-kafka/spring-kafka/src/main/java/org/springframework/kafka/listener/KafkaMessageListenerContainer.java

Lines 1597 to 1605 in e624891

	try {
	if (!this.isBatchListener && this.errorHandler != null) {
	this.errorHandler.handle(e, Collections.emptyList(), this.consumer,
	KafkaMessageListenerContainer.this.thisOrParentContainer);
	}
	else if (this.isBatchListener && this.batchErrorHandler != null) {
	this.batchErrorHandler.handle(e, new ConsumerRecords<K, V>(Collections.emptyMap()), this.consumer,
	KafkaMessageListenerContainer.this.thisOrParentContainer);
	}

This is misleading...

spring-kafka/spring-kafka/src/main/java/org/springframework/kafka/listener/ErrorHandler.java

Lines 39 to 41 in e624891

	default void handle(Exception thrownException, List<ConsumerRecord<?, ?>> records, Consumer<?, ?> consumer,
	MessageListenerContainer container) {
	handle(thrownException, null); // NOSONAR

All error handlers provided by the framework implement this method so it is never called.

[delor]

If you implement ErrorHandler in Kotlin like so

import org.apache.kafka.clients.consumer.ConsumerRecord
import org.springframework.kafka.listener.ErrorHandler
import java.lang.Exception

class MyErrorHandler : ErrorHandler {
    override fun handle(thrownException: Exception, data: ConsumerRecord<*, *>) {
        TODO("Not yet implemented")
    }
}

parameter data can be null b/c of @NonNullApi. Then org.springframework.kafka.listener.ErrorHandler#handle will pass null to it and you will get exception in runtime b/c data parameter in MyErrorHandler can't be null.

[kkocel]

@garyrussell having proper annotations is crucial for having proper types in Kotlin, as you can run into a problem @delor stated. More info here: https://www.youtube.com/watch?v=DvqXdnzhlSc&t=622s

[artembilan]

Thank you for the patience, @kkocel, and info!

May you share with us a stack trace for this issue to have a whole picture?
We definitely can live with your change since it is not a breaking change for Java at all.
At the same time it makes Kotlin happy.

Nevertheless I still think that we may revise those default method impls, but that already going to some breaking change in the API. Therefore next framework version.

[garyrussell]

crucial for having proper types in Kotlin,

I fully understand that.

I believe we should deprecate that method; users should always implement ContainerAwareErrorHandler or ContainerAwareBatchErrorHandler since the container always calls the method with 3 parameters.

Making it @Nullable seems wrong to me since it will never be called with a null value by the container.

[delor]

Sorry, can't provide a stacktrace as we have moved on with different approach and can't recreate it.

[garyrussell]

I have tried several different ways to fix this in a non-breaking way without success, so I think I will go ahead and merge this; thanks for the contribution.

I have re-opened this issue #615 so we can rework this error handler hierarchy in a breaking way in 3.0.