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

SmartCompositeMessageConverter incorrectly tries next converter when a conversion exception is thrown #1234

Closed
kelseyh opened this issue Feb 4, 2025 · 0 comments
Assignees
Milestone

Comments

@kelseyh
Copy link

kelseyh commented Feb 4, 2025

As a result of #1168 the following fix 6dee668 was created. Unfortunately, this is not ideal, with the current "fix" all the remaining converters are tried before giving up.

Given the MessageConverter#fromMessage contract which states, "If the converter does not support the specified media type or cannot perform the conversion, it should return null". I believe the current "fix" leads to undesired/unexpected behaviour, given the contract one would expect conversion exceptions to be handled by implementations should they wish to be lenient and defer to the next converter.

I think it would be more useful if the MessageConverterHelper#this.failConversionIfNecessary was called in the exception handler block rather than after all the converters have been exhausted. The reason for this being that we do NOT want to try any additional converters as this may lead to unexpected behaviour. We have a custom application/json converter and do NOT wish to try the default jsonMapper based application/json converter when there's a json deserialisation problem as this then also throws a json processing exception, this is wasteful and unwanted.

@olegz olegz self-assigned this Apr 2, 2025
@olegz olegz added this to the 4.3 milestone Apr 2, 2025
olegz added a commit that referenced this issue Apr 2, 2025
I have also added a new method to MessageConverterHelper.houldFailIfCantConvert(Message<?> message, Throwable t) to include Throwable and changed the callback in SmartCompositeMessageConverter to ensure it passes it in case it needs to be taken into account.

Resolves #1234
olegz added a commit that referenced this issue Apr 2, 2025
I have also added a new method to MessageConverterHelper.shouldFailIfCantConvert(Message<?> message, Throwable t) to include Throwable and changed the callback in SmartCompositeMessageConverter to ensure it passes it in case it needs to be taken into account.

Resolves #1234
@olegz olegz closed this as completed in 017db7d Apr 2, 2025
olegz added a commit that referenced this issue Apr 4, 2025
This ensures that if Throwable is not provided it can fal back to already implemented method
olegz added a commit that referenced this issue Apr 4, 2025
This ensures that if Throwable is not provided it can fal back to already implemented method
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants