GH-10083: Apply Nullability into dsl package
#10291
Conversation
Related to: spring-projects#10083 * Fix all the reported issues and IDE suggestions * Also, improve Nullability in the `TailAdapterSpec` and couple DSL Specs in JDBC module according to a new Nullability rules in the core `dsl` package
| @@ -31,9 +33,9 @@ | |||
| */ | |||
| public class QueueChannelSpec extends MessageChannelSpec<QueueChannelSpec, QueueChannel> { | |||
|
|
|||
| protected Queue<Message<?>> queue; // NOSONAR | |||
| protected @Nullable Queue<Message<?>> queue; // NOSONAR | |||
There was a problem hiding this comment.
Curious. Why do we need to keep the //NOSONAR?
There was a problem hiding this comment.
Because this property is set from extension classes.
Yes, we may remove it since does not look like anything currently in our CI/CD complains 😄
| @@ -47,7 +50,9 @@ public MessageProducerSpec(@Nullable P producer) { | |||
| */ | |||
| @Override | |||
| public S id(@Nullable String id) { | |||
| this.target.setBeanName(id); | |||
| if (id != null) { | |||
| this.target.setBeanName(id); | |||
There was a problem hiding this comment.
Just curious, why are we not throwing an error when they tryin to set the id to null, as it pertains to the bean name?
There was a problem hiding this comment.
Yeah... The logic is a bit convoluted and indeed there is a path when this method is called with null.
That is still not a problem because if an id is null, the real bean name is going to be generated in the IntegrationFlowBeanPostProcessor on this.target bean registration.
There was a problem hiding this comment.
That makes sense. So a bean name does not have to match the id in that case.
There was a problem hiding this comment.
It does match if id is provided.
There was a problem hiding this comment.
Could there be a case where:
mySpec.id("myID");
//beanName is now "myId"
//id is now "myId"
mySpec.id(null); //user calls id again and sets it to null
// beanName is "myId"
// id is null
Is this an issue?
There was a problem hiding this comment.
True. This is a problem.
Let me play with that if you are fine having more files to review in this already big PR 😄
For example:
public static AmqpMessageChannelSpec<?, ?> channel(ConnectionFactory connectionFactory) {
return channel(null, connectionFactory);
}
public static AmqpMessageChannelSpec<?, ?> channel(@Nullable String id, ConnectionFactory connectionFactory) {
return new AmqpMessageChannelSpec<>(connectionFactory)
.id(id);
}
So, the first method would not call the second just to have a nice delegation and code flow.
This way an id in the second method would not be null.
There was a problem hiding this comment.
Code review and coffee are a beautiful thing. 😆
There was a problem hiding this comment.
Turned out that wasn't too bad.
Just only much more classes to review yet 😄
There was a problem hiding this comment.
WOOHOO! Thanks for looking into this and resolving it!
There was a problem hiding this comment.
Thank you! For reviewing this, spotting concern and raising your doubts.
I’m glad that I decided to not save you from boring code pushing the original change directly upstream 🤗
* Fix all the affected classes to deal with not-null or ensure they provide not-null
|
LGTM Thanks for the hard work! |
Related to: #10083
TailAdapterSpecand couple DSL Specs in JDBC module according to a new Nullability rules in the coredslpackage