Apply Nullability to spring-integration-websocket module
#10195
Conversation
Related to: spring-projects#10083 Signed-off-by: Jooyoung Pyoung <[email protected]>
| @@ -56,20 +59,22 @@ public class ServerWebSocketContainer extends IntegrationWebSocketContainer | |||
|
|
|||
| private final String[] paths; | |||
|
|
|||
| @SuppressWarnings("NullAway.Init") | |||
There was a problem hiding this comment.
This can be null if respective setter is not called.
Let's have Assert.notNull() before calling .setHandshakeHandler(this.handshakeHandler) in the registerWebSocketHandlers()!
| private HandshakeHandler handshakeHandler; | ||
|
|
||
| private HandshakeInterceptor[] interceptors; | ||
| private HandshakeInterceptor @Nullable [] interceptors; |
There was a problem hiding this comment.
Can we have it to be initialized to empty arrays instead?
Looks like there is a check if (!ObjectUtils.isEmpty(interceptors)) {.
However, I agree that WebSocketHandlerRegistration API has to support nulls, and for the mentioned HandshakeHandler, too.
At the same time the AbstractWebSocketHandlerRegistration does support null:
public WebSocketHandlerRegistration setHandshakeHandler(@Nullable HandshakeHandler handshakeHandler) {
|
|
||
| private String[] origins; | ||
| private String @Nullable [] origins; |
There was a problem hiding this comment.
Same here for an empty array by default.
|
|
||
| private boolean autoStartup = true; | ||
|
|
||
| private int phase = 0; | ||
|
|
||
| @SuppressWarnings("NullAway.Init") | ||
| private TaskScheduler sockJsTaskScheduler; |
There was a problem hiding this comment.
Let's revise this!
It indeed can be null.
Especially its getter.
| @@ -324,6 +327,7 @@ else if (StompCommand.RECEIPT.equals(stompCommand)) { | |||
| } | |||
| else { | |||
| if (this.useBroker) { | |||
| Assert.notNull(this.brokerHandler, "brokerHandler is required"); | |||
There was a problem hiding this comment.
This is redundant.
We do have such a check in the onInit().
This method has to be marked with a @SuppressWarnings("NullAway") // Dataflow analysis limitation instead.
See more info in docs: https://docs.spring.io/spring-framework/reference/7.0/core/null-safety.html#_warnings_suppression
| if (this.useBroker) { | ||
| Assert.notNull(this.brokerHandler, "brokerHandler is required"); |
| @@ -374,7 +379,10 @@ private void produceConnectAckMessage(Message<?> message, SimpMessageHeaderAcces | |||
| } | |||
|
|
|||
| private void produceMessage(Message<?> message, SimpMessageHeaderAccessor headerAccessor) { | |||
| Object payload = this.messageConverter.fromMessage(message, this.payloadType.get()); | |||
| Class<?> targetType = this.payloadType.get(); | |||
| Assert.notNull(targetType, "payloadType must not be null"); | |||
There was a problem hiding this comment.
This is not correct: the value of that AtomicReference is never null.
Actually, I'm thinking if it has to be an AtomicReference at all.
Can you experiment if that could be just plain Class<?> property?
Related to: #10083