Apply Nullability to JMS module #10194
Conversation
related to: spring-projects#10083 Signed-off-by: Jiandong Ma <[email protected]>
|
|
||
| private JmsHeaderMapper headerMapper = new DefaultJmsHeaderMapper(); | ||
|
|
||
| private BeanFactory beanFactory; | ||
| private @Nullable BeanFactory beanFactory; |
There was a problem hiding this comment.
I think (but not very sure), this class might be used a pure Java Object, without calling the BeanFactoryAware callback. so make BeanFactory nullable here.
because I saw in JmsMessageDrivenEndpoint#onInit, there it directly calls this class this.listener.afterPropertiesSet(); without setting a beanFactory instance.
There was a problem hiding this comment.
This is not correct assumption.
The class is public and it is really a bean-like, Spring-aware component.
So, this has to be @SuppressWarnings("NullAway.Init").
And that JmsMessageDrivenEndpoint#onInit has to propagate its BF if it is mission.
| @@ -156,7 +157,8 @@ public void setBeanFactory(BeanFactory beanFactory) { | |||
|
|
|||
| @Override | |||
| protected void onInit() { | |||
| this.endpoint.setComponentName(getComponentName()); | |||
| String componentName = getComponentName(); | |||
| this.endpoint.setComponentName(componentName == null ? "jms-inbound-gateway" : componentName); | |||
There was a problem hiding this comment.
not sure here, will getComponentName() from gateway always not null ?
| @@ -218,7 +219,8 @@ protected void onInit() { | |||
| this.listenerContainer.setSessionAcknowledgeMode(acknowledgeMode); | |||
| } | |||
| } | |||
| this.listener.setComponentName(getComponentName()); | |||
| String componentName = getComponentName(); | |||
| this.listener.setComponentName(componentName == null ? "jms-message-listener" : componentName); | |||
There was a problem hiding this comment.
not sure as well, will getComponentName() from endpoint always not null ?
should we set parameter nullable in method IntegrationObjectSupport.setComponentName in core module
There was a problem hiding this comment.
I don't think so.
If we haven't set anything, or the component is not a bean, then yes, getComponentName() may be null.
But I don't think we should allow to pass null into setComponentName()
There was a problem hiding this comment.
I think JMS is one of the hardest modules to apply Nullability.
Thank you for doing this @mjd507 !
|
|
||
| private JmsHeaderMapper headerMapper = new DefaultJmsHeaderMapper(); | ||
|
|
||
| private BeanFactory beanFactory; | ||
| private @Nullable BeanFactory beanFactory; |
There was a problem hiding this comment.
This is not correct assumption.
The class is public and it is really a bean-like, Spring-aware component.
So, this has to be @SuppressWarnings("NullAway.Init").
And that JmsMessageDrivenEndpoint#onInit has to propagate its BF if it is mission.
| @@ -174,7 +175,7 @@ public void setShouldTrack(boolean shouldTrack) { | |||
| } | |||
|
|
|||
| @Override | |||
| public String getComponentName() { | |||
| public @Nullable String getComponentName() { | |||
| @@ -156,7 +157,8 @@ public void setBeanFactory(BeanFactory beanFactory) { | |||
|
|
|||
| @Override | |||
| protected void onInit() { | |||
| this.endpoint.setComponentName(getComponentName()); | |||
| String componentName = getComponentName(); | |||
| this.endpoint.setComponentName(componentName == null ? "jms-inbound-gateway" : componentName); | |||
| @@ -218,7 +219,8 @@ protected void onInit() { | |||
| this.listenerContainer.setSessionAcknowledgeMode(acknowledgeMode); | |||
| } | |||
| } | |||
| this.listener.setComponentName(getComponentName()); | |||
| String componentName = getComponentName(); | |||
| this.listener.setComponentName(componentName == null ? "jms-message-listener" : componentName); | |||
There was a problem hiding this comment.
I don't think so.
If we haven't set anything, or the component is not a bean, then yes, getComponentName() may be null.
But I don't think we should allow to pass null into setComponentName()
| Connection connection = createConnection(); // NOSONAR - closed in ConnectionFactoryUtils. | ||
| Session session = null; | ||
| Assert.notNull(this.replyContainer, "'replyContainer' must not be null"); |
There was a problem hiding this comment.
We don't need to do this here.
We have respective if..else in the handleRequestMessage().
This method has to be marked like:
@SuppressWarnings("NullAway") // Dataflow analysis limitation
See more info in docs: https://docs.spring.io/spring-framework/reference/7.0/core/null-safety.html#_warnings_suppression
| int priority) throws JMSException { | ||
|
|
||
| String correlation = null; | ||
| MessageProducer messageProducer = null; | ||
| try { | ||
| messageProducer = session.createProducer(reqDestination); | ||
| correlation = this.gatewayCorrelation + "_" + this.correlationId.incrementAndGet(); | ||
| Assert.notNull(this.correlationKey, "'correlationKey' must not be null"); |
There was a problem hiding this comment.
There is logic like this:
if (isAsync() && this.correlationKey == null) {
logger.warn("'async=true' requires a correlationKey; ignored");
setAsync(false);
}
}
else {
if (isAsync()) {
logger.warn("'async=true' is ignored when a reply container is not being used");
setAsync(false);
}
So, when we reach this method, this.correlationKey is really not null.
Therefore similar @SuppressWarnings for the method as before.
| @@ -1109,7 +1113,7 @@ private Object doSendAndReceiveAsync(Destination reqDestination, jakarta.jms.Mes | |||
| return future; | |||
| } | |||
| else { | |||
| return obtainReplyFromContainer(correlation, replyQueue); | |||
| return obtainReplyFromContainer(correlation, Objects.requireNonNull(replyQueue)); | |||
There was a problem hiding this comment.
Same here: we don't need this assert since when we reach this code block, the replyQueue is not null.
The mentioned @SuppressWarnings on this method will cover us.
| && JmsOutboundGateway.this.replies.size() == 0 && | ||
| JmsOutboundGateway.this.replyContainer.isRunning()) { | ||
| && JmsOutboundGateway.this.replies.isEmpty() && | ||
| Objects.requireNonNull(JmsOutboundGateway.this.replyContainer).isRunning()) { |
There was a problem hiding this comment.
Looks like better to propagate this.replyContainer as this class ctor argument from that handleRequestMessage().
|
|
||
| logger.debug(() -> getComponentName() + ": Stopping idle reply container."); | ||
| JmsOutboundGateway.this.replyContainer.stop(); | ||
| JmsOutboundGateway.this.idleTask.cancel(false); | ||
| Objects.requireNonNull(JmsOutboundGateway.this.idleTask).cancel(false); |
There was a problem hiding this comment.
This is not OK.
We have to extract JmsOutboundGateway.this.idleTask into a local variable.
Guard with if (idleTaskToCancel != null) and so on the rest against this variable.
But yeah... JmsOutboundGateway.this.idleTask = null; is still correct.
| throw new MessageDeliveryException(messageToSend, exceptionMessage, e); | ||
| throw messageToSend != null | ||
| ? new MessageDeliveryException(messageToSend, exceptionMessage, e) | ||
| : new MessageDeliveryException(exceptionMessage); |
There was a problem hiding this comment.
This is not correct.
We are in a MessageDispatchingException case, which can happen only from the this.dispatcher.dispatch(messageToSend);.
Therefore messageToSend is never null here when we reach this place.
So, I suggest to have a @SuppressWarnings("NullAway") as we discussed before.
related to: #10083