fix(context): route KafkaNull payloads through conversion for non-Message types - #1456
Open
adityaanikam wants to merge 1 commit into
Open
fix(context): route KafkaNull payloads through conversion for non-Message types#1456adityaanikam wants to merge 1 commit into
adityaanikam wants to merge 1 commit into
Conversation
…sage types convertInputIfNecessary returned the raw, unconverted Message as soon as it saw a KafkaNull payload, regardless of what the target function declared as its input type. For a function bound to a concrete type such as Consumer<MyType>, that Message was then cast to the declared type, throwing "class GenericMessage cannot be cast to class MyType" -- an error that gives no indication a Kafka tombstone was involved. The shortcut also ran before any MessageConverter or MessageConverterHelper was consulted, so the hook added in spring-cloudgh-1168 could not see these messages at all. Guard the shortcut with isInputTypeMessage(), the same predicate the class already uses elsewhere to test whether the declared input type is itself Message-compatible. A function genuinely declared to accept Message<?> still receives the raw message exactly as before; anything else now follows the normal conversion path. Fixes spring-cloudgh-1448 Signed-off-by: adityaanikam <adityanikam9502@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes gh-1448
Problem
FunctionInvocationWrapper#convertInputIfNecessaryreturns the raw, unconvertedMessageas soon as it sees aKafkaNullpayload:That shortcut runs regardless of what the target function declared as its input type. For a function bound to a concrete type (
Consumer<MyType>), the returnedMessageis then cast to the declared type at invocation, producing:which gives no indication that a Kafka tombstone was involved. The shortcut also returns before
functionInvocationHelper.preProcessInputandconvertInputMessageIfNecessaryare reached, so noMessageConverter— and therefore noMessageConverterHelper— ever sees these messages. The hook added for gh-1168 cannot apply here.Fix
Guard the shortcut with
isInputTypeMessage(), the predicate this class already uses a few dozen lines earlier for the same question:A function genuinely declared to accept
Message<?>still receives the raw message exactly as it does today. Anything else now follows the normal conversion path instead of bypassing it.I did not implement the alternative suggested on the issue (constructing a
MessageConversionExceptionand callingshouldFailIfCantConvertdirectly fromSimpleFunctionRegistry), becausemessageConverterHelpersSupplierandfailConversionIfNecessaryare both private toSmartCompositeMessageConverterandSimpleFunctionRegistryholds its converter as aCompositeMessageConverter. Doing it that way would need new public API; letting the message follow the existing path does not.Scope of the improvement
Worth being precise about what this does and does not change downstream, since it depends on the shape of the declared type:
convertInputMessageIfNecessarycallsfromMessage(Message, Class, Object), which callsfailConversionIfNecessarywhen no converter succeeds — so a registeredMessageConverterHelperis consulted and can raiseMessageConversionException.Consumer<MyType>, it calls the two-argumentfromMessage(Message, Class), which only callsfailConversionIfNecessarywhen a converter throws, not when converters quietly returnnull. So that path still ends in aClassCastException.In the second case the improvement is narrower but real: the payload is now unwrapped before the cast, so the error names the actual cause:
instead of pointing at the wrapping
GenericMessage. The asymmetry between the twofromMessageoverloads' failure semantics looks orthogonal to this issue, so I left it alone rather than widen the change.Testing
Two tests in
SimpleFunctionRegistryTests:testKafkaNullWithConcreteConsumerTypeNoLongerReachesFunctionAsRawMessage— aConsumer<Person>receiving aKafkaNullpayload; asserts the failure namesKafkaNulland notGenericMessage.testKafkaNullWithMessageTypedConsumerStillPassesThroughUnconverted— aConsumer<Message<Object>>still receives the raw message with theKafkaNullpayload intact.SimpleFunctionRegistrydetects the tombstone by comparinggetClass().getName()against the literal class name rather than by type, so the tests use a same-named test class undersrc/test/java/org/springframework/kafka/support/instead of adding a spring-kafka dependency.Verified with a negative control: reverting only the source change (keeping both tests) makes the first test fail with
class org.springframework.messaging.support.GenericMessage cannot be cast to class ...Person, matching the symptom reported on the issue; restoring it brings the suite back to 34 passing, 2 pre-existing skips.