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

CommonDelegatingErrorHandler does not check delegates compatibility correctly #3050

Closed
antonin-arquey opened this issue Feb 20, 2024 · 1 comment · Fixed by #3051
Closed

Comments

@antonin-arquey
Copy link
Contributor

antonin-arquey commented Feb 20, 2024

In what version(s) of Spring for Apache Kafka are you seeing this issue?

3.1.0

Describe the bug

When adding a new delegate to a CommonDelegatingErrorHandler, the check for compatibility with ackAfterHandle and seeksAfterHandling is not performed.

The following method does not check the delegatesToCheck but uses the current this.delegates.values() which does not yet contains the new delegates:

@SuppressWarnings("deprecation")
private void checkDelegatesAndUpdateClassifier(Map<Class<? extends Throwable>,
			CommonErrorHandler> delegatesToCheck) {

        boolean ackAfterHandle = this.defaultErrorHandler.isAckAfterHandle();
        boolean seeksAfterHandling = this.defaultErrorHandler.seeksAfterHandling();
        this.delegates.values().forEach(handler -> {
	        Assert.isTrue(ackAfterHandle == handler.isAckAfterHandle(),
			        "All delegates must return the same value when calling 'isAckAfterHandle()'");
	        Assert.isTrue(seeksAfterHandling == handler.seeksAfterHandling(),
			        "All delegates must return the same value when calling 'seeksAfterHandling()'");
        });
        updateClassifier(delegatesToCheck);
}

To Reproduce

Create a CommonDelegatingErrorHandler and add an incompatible CommonErrorHandler, no exception will be thrown.

Expected behavior

The new delegates compatibility should be validated.

antonin-arquey added a commit to antonin-arquey/spring-kafka that referenced this issue Feb 20, 2024
…ibility validation

Fixes: spring-projects#3050

Correct CommonDelegatingErrorHandler validation for delegates compatibility.

Add documentation stating that delegates must be compatible with default error handler.
antonin-arquey added a commit to antonin-arquey/spring-kafka that referenced this issue Feb 20, 2024
…ibility validation

Fixes: spring-projects#3050

Correct CommonDelegatingErrorHandler validation for delegates compatibility.

Add documentation stating that delegates must be compatible with default error handler.
antonin-arquey added a commit to antonin-arquey/spring-kafka that referenced this issue Feb 20, 2024
Fixes: spring-projects#3050

Correct CommonDelegatingErrorHandler validation for delegates compatibility.

Add documentation stating that delegates must be compatible with default error handler.
@antonin-arquey
Copy link
Contributor Author

opened a small PR to fix the compatibility check when adding a delegate

@sobychacko sobychacko added this to the 3.2.0-M2 milestone Feb 21, 2024
antonin-arquey added a commit to antonin-arquey/spring-kafka that referenced this issue Feb 21, 2024
Fixes: spring-projects#3050

Correct CommonDelegatingErrorHandler validation for delegates compatibility.

Add documentation stating that delegates must be compatible with default error handler.
sobychacko pushed a commit that referenced this issue Feb 22, 2024
Fixes: #3050

* Correct CommonDelegatingErrorHandler validation for delegates compatibility.
* Add documentation stating that delegates must be compatible with the default error handler.

**Auto-cherry-pick to `3.1.x`**
spring-builds pushed a commit that referenced this issue Feb 22, 2024
Fixes: #3050

* Correct CommonDelegatingErrorHandler validation for delegates compatibility.
* Add documentation stating that delegates must be compatible with the default error handler.

(cherry picked from commit c383b13)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants