GH-10083: Add Nullability to core support package #10395
Conversation
| Object correlationId = message.getHeaders().getId(); | ||
| AtomicInteger sequenceNumber = new AtomicInteger(1); | ||
|
|
||
| Assert.state(correlationId != null, "Correlation ID must not be null."); |
There was a problem hiding this comment.
This is redundant here.
The createBuilder() can accept it as null.
And then e have a check at the applySequence condition.
| * @see IntegrationMessageHeaderAccessor#isReadOnly(String) | ||
| */ | ||
| @SuppressWarnings("NullAway") // Dataflow analysis limitation | ||
| public B readOnlyHeaders(@Nullable String... readOnlyHeaders) { |
There was a problem hiding this comment.
This @Nullable is at wrong place.
Has to be next to ellipsis.
Therefore private String @Nullable [] readOnlyHeaders;
Then everything else in this class should be fixed well.
| * @since 4.3.2 | ||
| */ | ||
| @SuppressWarnings("NullAway") // Dataflow analysis limitation | ||
| public void setReadOnlyHeaders(@Nullable String... readOnlyHeaders) { |
There was a problem hiding this comment.
Missed to fix properly: setReadOnlyHeaders(String @Nullable ... readOnlyHeaders)
| * @param role the role. | ||
| */ | ||
| public void startLifecyclesInRole(String role) { | ||
| public void startLifecyclesInRole(@Nullable String role) { |
There was a problem hiding this comment.
I think this is wrong.
This is not possible by the logic of this API.
There was a problem hiding this comment.
On line 321 startLifecyclesInRole is used and a nullable value can be passed in. Do we add a Assert.state prior to line 321?
There was a problem hiding this comment.
Let's revise that AbstractLeaderEvent instead!
Remove AbstractLeaderEvent(Object source) ctor as it is out of use.
Looks like the other ctor is never used with null.
| } | ||
| Assert.notNull(headers, error); | ||
|
|
||
| Assert.state(payload != null, () -> "Payload must not be null"); |
There was a problem hiding this comment.
This class is marked for removal.
So, it is OK to not spend any time on it and just mark the whole class with NullAway.
| private final String[] trustedPackages; | ||
|
|
||
| @SuppressWarnings("NullAway") // Dataflow analysis limitation | ||
| AllowListTypeResolverBuilder(String... trustedPackages) { |
There was a problem hiding this comment.
String @Nullable ... instead.
|
|
||
| default <T> T fromJson(Object json, Class<T> valueType) throws IOException { | ||
| @Nullable | ||
| default <T> T fromJson(@Nullable Object json, Class<T> valueType) throws IOException { |
There was a problem hiding this comment.
How that happened that we allow null over here?
Doesn't looks like Jackson accepts such a behavior:
public <T> T readValue(String content, JavaType valueType) throws JacksonException
{
_assertNotNull("content", content);
Please, revise all the places which put a null for this API.
| replyBuilder = (responseBody instanceof Message<?>) | ||
| ? messageBuilderFactory.fromMessage((Message<?>) responseBody) | ||
| : messageBuilderFactory.withPayload(responseBody); // NOSONAR - hasBody() | ||
| Assert.state(responseBody != null, "The response body must not be null"); |
There was a problem hiding this comment.
Not correct. The httpResponse.hasBody() does cover this.
Please, revise. Perhaps in favor of NullAway
...core/src/main/java/org/springframework/integration/support/json/JacksonJsonObjectMapper.java
Outdated
Show resolved
Hide resolved
| } | ||
|
|
||
| protected abstract <T> T fromJson(Object json, J type) throws IOException; | ||
| protected abstract <T> T fromJson(@Nullable Object json, J type) throws IOException; |
There was a problem hiding this comment.
Please, revise these from methods.
No null is allowed.
cppwfs
force-pushed
the
GH-10083-core-support
branch
2 times, most recently
from
5ff7320 to
fc29e23
Compare
| * @since 4.3.2 | ||
| */ | ||
| @SuppressWarnings("NullAway") // Dataflow analysis limitation | ||
| public void setReadOnlyHeaders(@Nullable String... readOnlyHeaders) { |
There was a problem hiding this comment.
Missed to fix properly: setReadOnlyHeaders(String @Nullable ... readOnlyHeaders)
| @Override | ||
| public void onApplicationEvent(AbstractLeaderEvent event) { | ||
| if (event instanceof OnGrantedEvent) { | ||
| Assert.state(event.getRole() != null, "A role is required"); |
There was a problem hiding this comment.
That's not what I meant.
That AbstractLeaderEvent has to be fixed to not accept (and produce) nulls at all.
Thanks
| * @param role the role. | ||
| */ | ||
| public void stopLifecyclesInRole(String role) { | ||
| public void stopLifecyclesInRole(@Nullable String role) { |
There was a problem hiding this comment.
Therefore, same here: no null allowed.
| public class DefaultMessageBuilderFactory implements MessageBuilderFactory { | ||
|
|
||
| private String[] readOnlyHeaders; | ||
| @SuppressWarnings("NullAway.Init") |
There was a problem hiding this comment.
If the property is nullable, then this suppression is redundant.
| private volatile boolean modified; | ||
|
|
||
| private String[] readOnlyHeaders; | ||
| @SuppressWarnings("NullAway.Init") |
| * @see IntegrationMessageHeaderAccessor#isReadOnly(String) | ||
| */ | ||
| public B readOnlyHeaders(@Nullable String... readOnlyHeaders) { | ||
| public B readOnlyHeaders(String... readOnlyHeaders) { |
There was a problem hiding this comment.
Put @Nullable next to ellipsis.
| public <T> MessageBuilder<T> fromMessage(Message<T> message) { | ||
| return MessageBuilder.fromMessage(message) | ||
| .readOnlyHeaders(this.readOnlyHeaders); | ||
| if (this.readOnlyHeaders != null) { |
There was a problem hiding this comment.
When you fix BaseMessageBuilder, then this change has to be reverted.
| public <T> MessageBuilder<T> withPayload(T payload) { | ||
| return MessageBuilder.withPayload(payload) | ||
| .readOnlyHeaders(this.readOnlyHeaders); | ||
| if (this.readOnlyHeaders != null) { |
| if (!ObjectUtils.isEmpty(this.readOnlyHeaders)) { | ||
| for (String readOnly : this.readOnlyHeaders) { | ||
| if (headers.containsKey(readOnly)) { | ||
| if (readOnly != null && headers.containsKey(readOnly)) { |
There was a problem hiding this comment.
This does not reflect reality.
The String @Nullable [] is about the whole array being null.
To reflect that individual items could be null, too, we have to make it like:
@Nullable String @Nullable []
But that is not our case.
Therefore this != null is not true.
| } | ||
|
|
||
| private boolean containsReadOnly(MessageHeaders headers) { | ||
| if (!ObjectUtils.isEmpty(this.readOnlyHeaders)) { |
There was a problem hiding this comment.
Let's change this to != null to remove that @SuppressWarnings("NullAway.Init") on the property.
Related to: spring-projects#10083 * Add Nullability support to support.management package * Add Nullability support to support.management.micrometer package * Add Nullability support to support.management.observation * Add Nullability support to support.json
* Disabled null checks in Deprecated Jackson2 classes Remove Nullable from Jackson2 classes
Remove unnecessary if statements following review
cppwfs
force-pushed
the
GH-10083-core-support
branch
from
febeea9 to
d0f07eb
Compare
2 participants
Related to: #10083