-
Notifications
You must be signed in to change notification settings - Fork 1.7k
GH-4097: Fix SmartMessageConverter support in batch listeners #4121
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
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please, take a look into this article: https://cbea.ms/git-commit/.
The point is to say as much as possible about why? in the commit message, rather then what?.
Your comment on the PR and in the issue is a great foundation about what to put into commit message.
Side note: the first commit message becomes PR title and description.
Therefore, no need in extra work editing it.
And in the long run no one would think about PRs and issue. We would just look into commit history to determine what is going on.
This way I believe just with a single comprehensive commit message we shot a lot of birds and save some time from typing and reviewing.
With the current state I have to read everything to understand what is going.
Please, think about this while we are reviewing your contribution.
Thank you!
|
Alright, will do. Moving forward I will keep this in mind and commit
with clear and concise messages.
Thank you for informing me, I apologize for any inconvenience caused
on my end.
Message ID: ***@***.***
… .com>
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for contribution!
I think we need to dedicate more time for reverse engineering to see what is going on and how it can be fixed.
...in/java/org/springframework/kafka/listener/adapter/BatchMessagingMessageListenerAdapter.java
Outdated
Show resolved
Hide resolved
...in/java/org/springframework/kafka/listener/adapter/BatchMessagingMessageListenerAdapter.java
Outdated
Show resolved
Hide resolved
...rc/main/java/org/springframework/kafka/support/converter/BatchMessagingMessageConverter.java
Outdated
Show resolved
Hide resolved
...rc/main/java/org/springframework/kafka/support/converter/BatchMessagingMessageConverter.java
Outdated
Show resolved
Hide resolved
...-kafka/src/test/java/org/springframework/kafka/listener/BatchSmartMessageConverterTests.java
Outdated
Show resolved
Hide resolved
...-kafka/src/test/java/org/springframework/kafka/listener/BatchSmartMessageConverterTests.java
Outdated
Show resolved
Hide resolved
...-kafka/src/test/java/org/springframework/kafka/listener/BatchSmartMessageConverterTests.java
Outdated
Show resolved
Hide resolved
...-kafka/src/test/java/org/springframework/kafka/listener/BatchSmartMessageConverterTests.java
Outdated
Show resolved
Hide resolved
...-kafka/src/test/java/org/springframework/kafka/listener/BatchSmartMessageConverterTests.java
Outdated
Show resolved
Hide resolved
|
Thanks for the review @artembilan! I'll make those formatting fixes.
Regarding the core logic - you're absolutely right to question this. Let me
investigate if MessagingMessageConverter already handles the
SmartMessageConverter internally when we call toMessage().
I'll update the PR with my findings and the necessary changes.
Thank you.
… Message ID: <spring-projects/spring-kafka/pull/4121/review/3369915950@
github.com>
|
a41485a to
5a43d31
Compare
|
Hi @artembilan, I've addressed all your feedback in the latest commit. The changes include:
All tests pass and the fix is ready for review. Thanks for the detailed feedback! |
|
Thank you for an update! One note though: no need to squash commits all the time. |
|
Alright, thank you for letting me know. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Error: eckstyle] [ERROR] /home/runner/work/spring-kafka/spring-kafka/spring-kafka/src/main/java/org/springframework/kafka/listener/adapter/BatchMessagingMessageListenerAdapter.java:146: Line has leading space characters; indentation should be performed with tabs only. [RegexpSinglelineJava]
> Task :spring-kafka:checkstyleMain
Error: eckstyle] [ERROR] /home/runner/work/spring-kafka/spring-kafka/spring-kafka/src/main/java/org/springframework/kafka/listener/adapter/BatchMessagingMessageListenerAdapter.java:161: Line has leading space characters; indentation should be performed with tabs only. [RegexpSinglelineJava]
Error: eckstyle] [ERROR] /home/runner/work/spring-kafka/spring-kafka/spring-kafka/src/main/java/org/springframework/kafka/support/converter/BatchMessagingMessageConverter.java:228: Line has leading space characters; indentation should be performed with tabs only. [RegexpSinglelineJava]
Error: eckstyle] [ERROR] /home/runner/work/spring-kafka/spring-kafka/spring-kafka/src/main/java/org/springframework/kafka/support/converter/BatchMessagingMessageConverter.java:243: Line has leading space characters; indentation should be performed with tabs only. [RegexpSinglelineJava]
Error: eckstyle] [ERROR] /home/runner/work/spring-kafka/spring-kafka/spring-kafka/src/main/java/org/springframework/kafka/support/converter/BatchMessagingMessageConverter.java:244: Line has leading space characters; indentation should be performed with tabs only. [RegexpSinglelineJava]
Please, run gradlew check locally before pushing to PR.
Thanks
BatchMessagingMessageConverter was missing setMessagingConverter() method that exists in MessagingMessageConverter, causing SmartMessageConverter configured via @KafkaListener(contentTypeConverter) to be ignored in batch listeners. This inconsistency between regular and batch listeners leads to ClassCastException when byte[] values aren't converted to the expected String type, breaking the contract that SmartMessageConverter should work the same way regardless of listener type. The fix ensures SmartMessageConverter propagation works consistently by: - Adding setMessagingConverter() to BatchMessagingMessageConverter that delegates to underlying MessagingMessageConverter - Overriding setMessagingConverter() in BatchMessagingMessageListenerAdapter to propagate the converter to batch converter - Maintaining the same SmartMessageConverter behavior between regular and batch listeners Fixes spring-projectsGH-4097 Signed-off-by: Jujuwryy <georgemahfoud2@gmail.com>
|
All done. Builds were successful. Thank you. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please, add your real name to the @author tag of all the affected classes.
...in/java/org/springframework/kafka/listener/adapter/BatchMessagingMessageListenerAdapter.java
Outdated
Show resolved
Hide resolved
...rc/main/java/org/springframework/kafka/support/converter/BatchMessagingMessageConverter.java
Outdated
Show resolved
Hide resolved
...-kafka/src/test/java/org/springframework/kafka/listener/BatchSmartMessageConverterTests.java
Show resolved
Hide resolved
...-kafka/src/test/java/org/springframework/kafka/listener/BatchSmartMessageConverterTests.java
Outdated
Show resolved
Hide resolved
|
Please, don't squash commits. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The annotations in commit messages have to be back-ticked like code snippets to avoid GH user invitation.
You can use -s Git command option to add Singed-of-by automatically into each commit.
…ntation Integration testing revealed the root cause of spring-projectsGH-4097. When Spring processes a `@KafkaListener` with `contentTypeConverter` and `batch="true"`, the framework: 1. Calls `setBatchMessageConverter()` on the adapter 2. This internally calls `setMessageConverter()` which sets `converterSet=true` 3. Spring then tries to apply `contentTypeConverter` by calling `setMessagingConverter()` 4. The parent's validation `Assert.isTrue(!this.converterSet, ...)` blocks this The unit test didn't catch this because it bypassed the adapter and Spring framework integration entirely. Changes: - `BatchMessagingMessageListenerAdapter.setMessagingConverter()`: Override now directly applies `SmartMessageConverter` to batch converter (which propagates to record converter) without calling super, bypassing the validation that doesn't apply to the batch listener workflow - `BatchSmartMessageConverterTests`: Replaced unit test with full integration test using `@SpringJUnitConfig`, `@EmbeddedKafka`, `ConcurrentKafkaListenerContainerFactory`, and `@KafkaListener` to verify the complete framework flow - Added minimal `ByteArrayToStringConverter` (24 lines) for testing as no existing Spring Framework converter provides simple byte[] to String conversion needed for this test scenario All tests pass and checkstyle validation successful. Signed-off-by: Jujuwryy <georgemahfoud2@gmail.com>
|
Apologies for the force push - I needed to amend the commit to add the DCO sign-off and use backticks around annotations to avoid GitHub mentions. The latest commit does include a small change to the "setMessagingConverter" method in the BatchMessagingMessageListenerAdapter.java. Thank you. |
...in/java/org/springframework/kafka/listener/adapter/BatchMessagingMessageListenerAdapter.java
Outdated
Show resolved
Hide resolved
...in/java/org/springframework/kafka/listener/adapter/BatchMessagingMessageListenerAdapter.java
Outdated
Show resolved
Hide resolved
...-kafka/src/test/java/org/springframework/kafka/listener/BatchSmartMessageConverterTests.java
Show resolved
Hide resolved
|
Hello @artembilan, I tested the setMessagingConverter() implementation both with and without calling super.setMessagingConverter(messageConverter). The Issue with Calling Super: "text Root Cause: setBatchMessageConverter() internally calls setMessageConverter(), which sets converterSet = true in the parent class When Spring later processes @KafkaListener(contentTypeConverter = "...") and calls setMessagingConverter(), the parent's validation Assert.isTrue(!this.converterSet) blocks it This creates a timing conflict specific to batch listeners due to their different initialization flow Current Solution: We bypass the parent's restrictive validation We directly propagate the converter to the batch converter The batch converter then properly delegates to the record converter via its own setMessagingConverter() method This approach respects the different initialization sequence required for batch listeners Regarding the Test Converter: I've also incorporated your previous feedback by: Using pattern matching for instanceof where applicable. The current implementation ensures that @KafkaListener(contentTypeConverter = "...") works consistently for both record and batch listeners without triggering the framework's validation conflicts. Kindly let me know if i should re - commit the code. |
- Use pattern matching for instanceof in setMessagingConverter() to avoid explicit casting - Fix constructor parameter indentation to use tabs only (not mixed spaces) - Address checkstyle violations per reviewer feedback These changes improve code readability without affecting functionality. Signed-off-by: Jujuwryy <georgemahfoud2@gmail.com>
...in/java/org/springframework/kafka/listener/adapter/BatchMessagingMessageListenerAdapter.java
Outdated
Show resolved
Hide resolved
...in/java/org/springframework/kafka/listener/adapter/BatchMessagingMessageListenerAdapter.java
Outdated
Show resolved
Hide resolved
...in/java/org/springframework/kafka/listener/adapter/BatchMessagingMessageListenerAdapter.java
Outdated
Show resolved
Hide resolved
...rc/main/java/org/springframework/kafka/support/converter/BatchMessagingMessageConverter.java
Outdated
Show resolved
Hide resolved
- Fix toMessagingMessage() parameter indentation in BatchMessagingMessageListenerAdapter - Fix toMessage() parameter indentation in BatchMessagingMessageConverter - Use single tab indentation consistently per Spring code style Signed-off-by: Jujuwryy <georgemahfoud2@gmail.com>
Fixes #4097
Problem: SmartMessageConverter was ignored in batch listeners, causing ClassCastException
Root Cause: BatchMessagingMessageConverter lacked setMessagingConverter() method
Solution:
Testing: All existing tests pass, new tests verify the fix works
Note: This is my first contribution to Spring Kafka. Happy to make any requested changes!