Use latest Spring Framework 7.1.0-SNAPSHOT - #11351
Conversation
artembilan
left a comment
There was a problem hiding this comment.
Please, repurpose this PR just for SF 7.2.0-SNAPSHOT.
Looks like this is going to take a while until it is in a good shape.
The rest of dependencies should not suffer.
Thanks
bffb7d2 to
b13b232
Compare
|
This change includes those that were in PR #11367. So if this one still needs alot of work then merge 11367 and I'll continue work on this PR. |
db5bc01 to
0ecf55c
Compare
0ecf55c to
102a20f
Compare
artembilan
left a comment
There was a problem hiding this comment.
I think this is something what deserves some docs, too.
Including whats-new.
Thanks
| /** | ||
| * @author Glenn Renfro | ||
| */ | ||
| public class JmsDestinationPollingSourceTests implements TestApplicationContextAware { |
There was a problem hiding this comment.
I think we agreed that public is redundant in tests.
There was a problem hiding this comment.
Not fixed.
I see changes in the test methods, but class declaration could also benefit from not having public modifier 😄
|
I mean. |
|
Also, please, add a comment into this PR with a |
- The latest snapshots have introduced some breaking changes.
Resolve the issues below
- Treat a null result from `MessageConverter.fromMessage()` as a
conversion failure across all inbound JMS paths.
- Throw `MessageConversionException` from `AbstractJmsChannel`,
`JmsDestinationPollingSource`, `ChannelPublishingJmsMessageListener`,
and `JmsOutboundGateway` instead of silently discarding the message.
- Remove the discard branch in `SubscribableJmsChannel` since a null
conversion is no longer possible without an exception.
- Add tests asserting `MessageConversionException` for each of the
four affected components.
- Updated core to use builder for `SpelParserConfiguration` as the
constructor is now deprecated.
102a20f to
afaa17b
Compare
- Update docs to reflect SI's response to framewworks changes to Jms message conversion - Add author tags
afaa17b to
83fc3ad
Compare
| } | ||
|
|
||
| /** | ||
| * Converts {@link jakarta.jms.Message} to a Spring {@link Message}. |
There was a problem hiding this comment.
Method Javadocs must be imperative, like commands.
| } | ||
|
|
||
| /** | ||
| * Processes an incoming JMS message, transforms it into a Spring Integration message, |
There was a problem hiding this comment.
I think this Javadoc was generated by AI.
Please, revise it as it is too much info.
And it is not imperative.
| * @param jmsMessage the incoming {@link jakarta.jms.Message} to process | ||
| * @param session the active JMS {@link jakarta.jms.Session} associated with the listener | ||
| * @throws JMSException if a native JMS error occurs while processing, resolving destinations, or sending replies | ||
| * @throws MessageConversionException if payload extraction produces a {@code null} result when required |
There was a problem hiding this comment.
I don't think this is only case.
The message converter may thrown such an exception for other reason.
We probably should rephrase it somehow saying that in case of null such an exception is thrown, too.
Or what is better wording would it be from English perspective 😄
| @@ -408,6 +423,9 @@ public void onMessage(jakarta.jms.Message jmsMessage, Session session) throws JM | |||
| result = this.messageConverter.fromMessage(jmsMessage); | |||
| this.logger.debug(() -> "converted JMS Message [" + jmsMessage + "] to integration Message payload [" | |||
There was a problem hiding this comment.
And no this debug message is suspicious with null result.
I think we need to revise logic as if..else.
| * Will receive a JMS {@link jakarta.jms.Message} converting and returning it as | ||
| * a Spring Integration {@link Message}. This method will also use the current | ||
| * {@link JmsHeaderMapper} instance to map JMS properties to the MessageHeaders. | ||
| * @throws MessageConversionException if the converter returns {@code null}. |
| source.setBeanFactory(TEST_INTEGRATION_CONTEXT); | ||
| source.afterPropertiesSet(); | ||
|
|
||
| // A JMS message converted to a null payload is a conversion failure, not a message to discard. |
| } | ||
|
|
||
| @Test | ||
| void payloadFromConverterIsUsed() throws Exception { |
There was a problem hiding this comment.
I don't see value in this test.
Or better to say: we do have such a coverage in other tests in the module.
| when(session.createProducer(any())).thenReturn(mock()); | ||
| MessageConsumer consumer = mock(); | ||
| when(session.createConsumer(any())).thenReturn(consumer); | ||
| when(consumer.receive(anyLong())).thenReturn(mock()); |
There was a problem hiding this comment.
See MockSettings instead of all of these, e.g. Answers.RETURNS_MOCKS.
More over I think this is too much just for an exception in the end.
See if StubConnection and friends can help anyhow instead of this mock nightmare.
| See the xref:http.adoc[HTTP Support] chapter for more information about these and other changes. | ||
|
|
||
| [[x7.2-jms-changes]] | ||
| === JMS Support Changes |
There was a problem hiding this comment.
Missed blank line after the title.
| [[x7.2-jms-changes]] | ||
| === JMS Support Changes | ||
| Spring Integration supports Spring Framework's ability to return `null` when converting a JMS `Message` where the payload has not been set. | ||
| This also means that if a Spring `Message` uses the payload of a JMS `Message` that is null, a `MessageConversionException` will be thrown. |
There was a problem hiding this comment.
I think this text needs to be revised.
It is also missing a link to the target chapter.
And I don't see a change in the target chapter.
The latest snapshots have introduced some breaking changes.
Resolve the issues below
MessageConverter.fromMessage()as aconversion failure across all inbound JMS paths.
MessageConversionExceptionfromAbstractJmsChannel,JmsDestinationPollingSource,ChannelPublishingJmsMessageListener,and
JmsOutboundGatewayinstead of silently discarding the message.SubscribableJmsChannelsince a nullconversion is no longer possible without an exception.
MessageConversionExceptionfor each of thefour affected components.
SpelParserConfigurationas theconstructor is now deprecated.