Introduce NullMarked to some packages in spring-integration-core module #10097

cppwfs · 2025-06-11T14:42:32Z

This is a first pass at the module to make sure it is being converted properly.

There are 3 commits that comprise this PR. Each commit handles a specific series of package(s) on the model.
This was done to simplify the review process so each package could be reviewed indepently.

Commit 1 - org.springframework.integration.container
Commit 2 - org.springframework.integration.messaging.core
Commit 3 - org.springframework.integration.aggregator & org.springframework.integration.acks

Note

Highlights information that users should take into account, even when skimming. in this commit that there cases where methods are called where the parameters passed in may contain a null.
In those cases the attribute that contains the potential null is tested with an Assert.notNull. This may not be the best approach, let's discuss.

Note

Also note that in FluxAggregatorMessageHandler line 112 @NullUnmarked is used on the the applyWindowOptions method.
This is because the consumer.apply on line 121 can not be null according to JSpecify. But we can't make that assumption for how the user implemented the fuction.
There maybe a better way of handling this besides marking the method as NullUnmarked. Let's discuss.
Similarly in FluxAggregatorMessageHandler the way in which sequenceSizeHeader is utilized it had to be @NullUnmarked. Let's discuss

Related to #10083

artembilan

This is very cool!
Let's see if we can agree on my feedback.
Thanks

artembilan · 2025-06-11T14:51:14Z

...ava/org/springframework/integration/aggregator/AbstractAggregatingMessageGroupProcessor.java

@@ -117,6 +119,7 @@ protected Map<String, Object> aggregateHeaders(MessageGroup group) {
 		return getHeadersFunction().apply(group);
 	}

+	@Nullable


I don't think this is OK.
The payload cannot be null for the message we are going to produce from an aggregator.
If it is potentially null in the specific implementation, let's address there, e.g. with an Assert.state()!

artembilan · 2025-06-11T14:53:44Z

.../main/java/org/springframework/integration/aggregator/AbstractCorrelatingMessageHandler.java

@@ -641,8 +652,10 @@ protected boolean isExpireGroupsUponCompletion() {
 	}

 	private void removeEmptyGroupAfterTimeout(UUID groupId, long timeout) {
+		TaskScheduler taskScheduler = getTaskScheduler();
+		Assert.notNull(taskScheduler, "'taskScheduler' must not be null");


We probably need to address this in the IntegrationObjectSupport.getTaskScheduler().
Make it really non-null and have this Assert.notNull over there in the end of method.

artembilan · 2025-06-11T15:00:55Z

...va/org/springframework/integration/aggregator/ExpressionEvaluatingMessageGroupProcessor.java

@@ -58,6 +60,7 @@ public void setExpectedType(Class<?> expectedType) {
 	 * {@link org.springframework.integration.core.MessagingTemplate} to send downstream.
 	 */
 	@Override
+	@Nullable
 	protected Object aggregatePayloads(MessageGroup group, Map<String, Object> headers) {
 		return this.processor.process(group.getMessages());


Yes, that's what I mean. The SpEL may return null, indeed.
But that is not what we expect here.
Let's assert its result before returning!
Or do you see some use-case where it is deliberately returned as null?

artembilan · 2025-06-11T15:04:20Z

...e/src/main/java/org/springframework/integration/aggregator/FluxAggregatorMessageHandler.java

 	private Predicate<Message<?>> boundaryTrigger;

-	private Function<Message<?>, Integer> windowSizeFunction = FluxAggregatorMessageHandler::sequenceSizeHeader;
+	@Nullable
+	public Function<Message<?>, Integer> windowSizeFunction = FluxAggregatorMessageHandler::sequenceSizeHeader;


This is not correct. The @Nullable on the property type would mean that FluxAggregatorMessageHandler::sequenceSizeHeader lambda is null.
However that is not the case.
Here we are talking about result of message.getHeaders().get(IntegrationMessageHeaderAccessor.SEQUENCE_SIZE, Integer.class).
Therefore it is null for function invocation.
Hence, has to be like this:

private Function<Message<?>, @Nullable Integer> windowSizeFunction = FluxAggregatorMessageHandler::sequenceSizeHeader;

I'm not sure why have you made this property as public, though...

artembilan · 2025-06-11T15:06:00Z

...e/src/main/java/org/springframework/integration/aggregator/FluxAggregatorMessageHandler.java

@@ -99,13 +109,15 @@ private Flux<Message<?>> releaseBy(Flux<Message<?>> groupFlux) {
 				.flatMap((windowFlux) -> windowFlux.transform(this.combineFunction));
 	}

+	@NullUnmarked


When you do Function<Message<?>, @Nullable Integer>, we won't need this anymore.

Even after the change has been applied the following line in the method is getting flagged:
https://github.com/spring-projects/spring-integration/blob/main/spring-integration-core/src/main/java/org/springframework/integration/aggregator/FluxAggregatorMessageHandler.java#L109

This is because the R apply(T t); from the Function.java has not been annotated with @nullable.
Let's discuss to see if their is a smoother resolution than the @NullMarked

artembilan · 2025-06-11T15:07:05Z

...tion-core/src/main/java/org/springframework/integration/aggregator/MessageListProcessor.java

@@ -27,6 +29,7 @@
 @FunctionalInterface
 public interface MessageListProcessor {

+	@Nullable


I think this one has to be non-null as well.

artembilan · 2025-06-11T15:09:23Z

...tion-core/src/main/java/org/springframework/integration/context/IntegrationContextUtils.java

@@ -128,6 +131,7 @@ public static MetadataStore getMetadataStore(BeanFactory beanFactory) {
 	 * @param beanFactory BeanFactory for lookup, must not be null.
 	 * @return The {@link MessageChannel} bean whose name is "errorChannel".
 	 */
+	@Nullable
 	public static MessageChannel getErrorChannel(BeanFactory beanFactory) {
 		return getBeanOfType(beanFactory, ERROR_CHANNEL_BEAN_NAME, MessageChannel.class);


This bean is always there in the Integration infrastructure.
So, let's put an Assert.notNull in the end of this method instead!

Was not addressed

artembilan · 2025-06-11T15:10:12Z

...tion-core/src/main/java/org/springframework/integration/context/IntegrationContextUtils.java

@@ -136,6 +140,7 @@ public static MessageChannel getErrorChannel(BeanFactory beanFactory) {
 	 * @param beanFactory BeanFactory for lookup, must not be null.
 	 * @return The {@link TaskScheduler} bean whose name is "taskScheduler" if available.
 	 */
+	@Nullable
 	public static TaskScheduler getTaskScheduler(BeanFactory beanFactory) {
 		return getBeanOfType(beanFactory, TASK_SCHEDULER_BEAN_NAME, TaskScheduler.class);


Same for TaskScheduler.
See DefaultConfiguringBeanFactoryPostProcessor for those infra beans we populate when @EnableIntegration is in charge.

...e/src/main/java/org/springframework/integration/aggregator/FluxAggregatorMessageHandler.java

artembilan

See my last commit into your PR for the FluxAggregatorMessageHandler fix.
At least that what I think about those nulls.

.../java/org/springframework/integration/syslog/inbound/SyslogReceivingChannelAdapterTests.java

artembilan

This is cool!
We can chat on Monday if you wish.
I also hope you do not update Copyright year manually . We have a Gradle task to do that for us 😉

artembilan · 2025-06-14T00:16:18Z

.../main/java/org/springframework/integration/aggregator/AbstractCorrelatingMessageHandler.java

@@ -929,6 +937,7 @@ protected Collection<Message<?>> completeGroup(Message<?> message, Object correl
 			this.logger.debug(() -> "Completing group with correlationKey [" + correlationKey + "]");

 			result = this.outputProcessor.processMessageGroup(group);
+			Assert.notNull(result, "the group returned a null result");


I think that is processor is responsible for null result, not group. But again: it really can return null. Might be something is off with our logic here?
Not saying to do that right now with such a huge pr: let’s just have a note to come back to this method!
For now, fix, please, this error message .

artembilan · 2025-06-14T00:19:47Z

...ava/org/springframework/integration/aggregator/ExpressionEvaluatingMessageListProcessor.java

@@ -102,7 +105,9 @@ public void setExpectedType(Class<?> expectedType) {
 	 */
 	@Override
 	public Object process(Collection<? extends Message<?>> messages) {
-		return this.evaluateExpression(this.expression, messages, this.expectedType);
+		Object object = this.evaluateExpression(this.expression, messages, this.expectedType);
+		Assert.state(object != null, "Failed to evaluate expression: " + this.expression);


I don’t think it is really correct . The expression has been just evaluated to null, but we don’t expect null here. Let’s say exactly that condition: the expression evaluated to null and therefore this exception .

artembilan · 2025-06-14T00:21:19Z

...va/org/springframework/integration/aggregator/ExpressionEvaluatingMessageGroupProcessor.java

@@ -59,7 +60,9 @@ public void setExpectedType(Class<?> expectedType) {
 	 */
 	@Override
 	protected Object aggregatePayloads(MessageGroup group, Map<String, Object> headers) {
-		return this.processor.process(group.getMessages());
+		Object object = this.processor.process(group.getMessages());
+		Assert.notNull(object, "Result from processor must not be null");


Use state() instead. This really not an illegal arg.

artembilan · 2025-06-14T00:23:30Z

...ain/java/org/springframework/integration/aggregator/MethodInvokingMessageGroupProcessor.java

@@ -87,7 +88,9 @@ public void setBeanFactory(BeanFactory beanFactory) {
 	@Override
 	protected final Object aggregatePayloads(MessageGroup group, Map<String, Object> headers) {
 		final Collection<Message<?>> messagesUpForProcessing = group.getMessages();
-		return this.processor.process(messagesUpForProcessing, headers);
+		Object object = this.processor.process(messagesUpForProcessing, headers);
+		Assert.notNull(object, "Result from processor must not be null");


Same here: state()

artembilan · 2025-06-14T00:24:21Z

...tion-core/src/main/java/org/springframework/integration/context/IntegrationContextUtils.java

@@ -128,6 +131,7 @@ public static MetadataStore getMetadataStore(BeanFactory beanFactory) {
 	 * @param beanFactory BeanFactory for lookup, must not be null.
 	 * @return The {@link MessageChannel} bean whose name is "errorChannel".
 	 */
+	@Nullable
 	public static MessageChannel getErrorChannel(BeanFactory beanFactory) {
 		return getBeanOfType(beanFactory, ERROR_CHANNEL_BEAN_NAME, MessageChannel.class);


Was not addressed

artembilan · 2025-06-14T00:38:32Z

...ip/src/test/java/org/springframework/integration/ip/tcp/TcpReceivingChannelAdapterTests.java

 		noopPublisher(scf);
 		interceptorsGuts(scf);
 		scf.stop();
 	}

+	private AbstractServerConnectionFactory getDefaultServerConnectionFactory() {


static and probably move to the end of class

artembilan · 2025-06-14T00:43:37Z

...a/org/springframework/integration/ip/tcp/connection/CachingClientConnectionFactoryTests.java

@@ -820,5 +822,4 @@ private static AbstractClientConnectionFactory createFactoryWithMockConnection(T
 		when(factory.isActive()).thenReturn(true);
 		return factory;
 	}
-


See if you can use https://github.com/spring-projects/spring-integration/blob/main/src/idea/spring-framework.xml in your IDEA to avoid this kind of change.

Was not addressed, please

artembilan · 2025-06-14T00:44:54Z

.../org/springframework/integration/ip/tcp/connection/FailoverClientConnectionFactoryTests.java

@@ -698,5 +699,18 @@ private static AbstractClientConnectionFactory createFactoryWithMockConnection(T
 		return factory;
 	}

+	private TcpNetServerConnectionFactory getTcpNetServerConnectionFactory(int port) {


Was not addressed

artembilan · 2025-06-14T00:45:23Z

.../org/springframework/integration/ip/tcp/connection/FailoverClientConnectionFactoryTests.java

+		return result;
+	}
+
+	private TcpNetClientConnectionFactory getTcpNetServerConnectionFactory(String host, int port) {


artembilan · 2025-06-14T00:58:05Z

.../java/org/springframework/integration/syslog/inbound/SyslogReceivingChannelAdapterTests.java

+				.thenReturn(taskScheduler);
+		when(beanFactory.containsBean("taskScheduler")).thenReturn(true);
+		return beanFactory;
+	}


Empty line before the last }vin the class.
In other words: every member in the class has to be surrounded with blank lines.
I think the editor config for IDEA I suggested before should do the trick when you reformat files.

This is a first pass at the module to make sure it is being converted properly. Related to spring-projects#10083

…rk.acks packages Note in this commit that there cases where methods are called where the parameters passed in may contain a null. In those cases the attribute that contains the potential null is tested with an `Assert.notNull`. This may not be the best approach, let's discuss. Also note that in FluxAggregatorMessageHandler line 112 `@NullUnmarked` is used on the the applyWindowOptions method. This is because the consumer.apply on line 121 can not be null according to JSpecify. But we can't make that assumption for how the user implemented the function. There maybe a better way of handling this besides marking the method as `NullUnmarked`. Let's discuss. Similarly in FluxAggregatorMessageHandler the way in which sequenceSizeHeader is utilized it had to be `@NullUnmarked`. Let's discuss Make sure all annotations are applied consistently

* This PR is not ready for merge. There are failing tests, but wanted to capture where it is at. * Also still having issue with the .apply() in FluxAggregatorMessageHandler * Make `windowSizeFunction` as `Function<Message<?>, @nullable Integer>` because `sequenceSizeHeader()` may return `null` from message headers * Extract local `subscriptionToDispose` in the `stop()` to satisfy null check context * Use `Objects.requireNonNull(signal.get())` to satisfy `Function.apply()` contract. The `if (signal.hasValue()) {` does the trick for us, but currently that is not visible for that `signal.get()` * Remove `@NullUnmarked` since we have just mitigated all the null problems Updated tests so that they work with nullability changes Update the tests so that they will pass with nullify changes

* Remove Nullunmarked from FluxAggregatorMessageHandler * Add TaskScheuler to tests that are failing because of nullability * In the past the TaskScheduler attribute in the testscould be set to null and the tests would succeed. But because of nullability additions these need to be populated

* Added style to IDE to resolve style issues * Resolved the missed remove from IntegrationContextUtils. * Made sure that private test methods were marked static * Updated Assert.NotNulls to Assert.state and updated their messages * Rebased and updated nullification for changes

artembilan

Sorry, there are still some review feedback.
But this is great to be merged soon enough.

Thanks

artembilan · 2025-06-16T16:59:32Z

.../main/java/org/springframework/integration/aggregator/AbstractCorrelatingMessageHandler.java

@@ -125,8 +125,10 @@ public abstract class AbstractCorrelatingMessageHandler extends AbstractMessageP

 	private boolean releaseStrategySet;

+	@SuppressWarnings("NullAway.Init")


This is not correct on this property.
It is initialized in the getDiscardChannel().
So, it has to be @Nullable and that getter has to be used instead in other places.

artembilan · 2025-06-16T17:01:09Z

...ava/org/springframework/integration/aggregator/ExpressionEvaluatingMessageListProcessor.java

@@ -102,7 +105,9 @@ public void setExpectedType(Class<?> expectedType) {
 	 */
 	@Override
 	public Object process(Collection<? extends Message<?>> messages) {
-		return this.evaluateExpression(this.expression, messages, this.expectedType);
+		Object object = this.evaluateExpression(this.expression, messages, this.expectedType);
+		Assert.state(object != null, "The evaluation of the expression returned a null. Null result is not expected." + this.expression);


Probably you mean: Null result is not expected: " + this.expression ?

artembilan · 2025-06-16T17:02:16Z

...e/src/main/java/org/springframework/integration/aggregator/FluxAggregatorMessageHandler.java

@@ -106,7 +115,8 @@ private Flux<Flux<Message<?>>> applyWindowOptions(Flux<Message<?>> groupFlux) {
 		return groupFlux
 				.switchOnFirst((signal, group) -> {
 					if (signal.hasValue()) {
-						Integer maxSize = this.windowSizeFunction.apply(signal.get());
+						Assert.notNull(this.windowSizeFunction, "'windowSizeFunction' must not be null");


How can that be null, please?

artembilan · 2025-06-16T17:03:02Z

...e/src/main/java/org/springframework/integration/aggregator/FluxAggregatorMessageHandler.java

 		return message.getHeaders().get(IntegrationMessageHeaderAccessor.SEQUENCE_SIZE, Integer.class);
 	}
-


Blank line in the end of class before its last }

artembilan · 2025-06-16T17:04:08Z

...tion-core/src/main/java/org/springframework/integration/aggregator/MessageListProcessor.java

@@ -1,5 +1,5 @@
 /*
- * Copyright 2002-2019 the original author or authors.
+ * Copyright 2002-2025 the original author or authors.


This looks like redundant change for now.
But that's OK.
We will move to present instead soon enough 😄

artembilan · 2025-06-16T17:06:05Z

...tion-core/src/main/java/org/springframework/integration/context/IntegrationContextUtils.java

@@ -156,6 +163,7 @@ public static TaskScheduler getRequiredTaskScheduler(BeanFactory beanFactory) {
 	 * @return the instance of {@link StandardEvaluationContext} bean whose name is
 	 * {@value #INTEGRATION_EVALUATION_CONTEXT_BEAN_NAME}.
 	 */
+	@Nullable


This cannot be null.
Another infrastructure bean always present there by Integration infrastructure.

artembilan · 2025-06-16T17:06:38Z

...tion-core/src/main/java/org/springframework/integration/context/IntegrationContextUtils.java

@@ -166,11 +174,13 @@ public static StandardEvaluationContext getEvaluationContext(BeanFactory beanFac
 	 * {@value #INTEGRATION_SIMPLE_EVALUATION_CONTEXT_BEAN_NAME}.
 	 * @since 4.3.15
 	 */
+	@Nullable


Same here: never null.
Therefore similar to that:

Assert.state(taskScheduler != null, "No such bean '" + INTEGRATION_SIMPLE_EVALUATION_CONTEXT_BEAN_NAME + "'");

And in the previous, too.

artembilan · 2025-06-16T17:07:27Z

...ion-core/src/main/java/org/springframework/integration/context/IntegrationObjectSupport.java

@@ -208,6 +224,7 @@ public final void setPrimaryExpression(Expression expression) {
 	}

 	@Override
+	@SuppressWarnings("NullAway")


Any comments why this is still here, please?

artembilan · 2025-06-16T17:15:19Z

...a/org/springframework/integration/ip/tcp/connection/CachingClientConnectionFactoryTests.java

@@ -820,5 +822,4 @@ private static AbstractClientConnectionFactory createFactoryWithMockConnection(T
 		when(factory.isActive()).thenReturn(true);
 		return factory;
 	}
-


Was not addressed, please

artembilan · 2025-06-16T17:15:42Z

.../org/springframework/integration/ip/tcp/connection/FailoverClientConnectionFactoryTests.java

@@ -698,5 +699,18 @@ private static AbstractClientConnectionFactory createFactoryWithMockConnection(T
 		return factory;
 	}

+	private TcpNetServerConnectionFactory getTcpNetServerConnectionFactory(int port) {


Was not addressed

cppwfs assigned artembilan Jun 11, 2025

artembilan requested changes Jun 11, 2025

View reviewed changes

artembilan added this to the 7.0.0-M1 milestone Jun 11, 2025

artembilan added in: core type: refactoring labels Jun 11, 2025

artembilan requested changes Jun 11, 2025

View reviewed changes

...e/src/main/java/org/springframework/integration/aggregator/FluxAggregatorMessageHandler.java Outdated Show resolved Hide resolved

artembilan requested changes Jun 12, 2025

View reviewed changes

cppwfs force-pushed the SI-NULLIFY-CORE-1 branch 2 times, most recently from a6ee2ac to 750cf77 Compare June 12, 2025 21:33

cppwfs commented Jun 13, 2025

View reviewed changes

.../java/org/springframework/integration/syslog/inbound/SyslogReceivingChannelAdapterTests.java Show resolved Hide resolved

artembilan requested changes Jun 14, 2025

View reviewed changes

cppwfs and others added 8 commits June 16, 2025 07:37

Update SI Core to add Nullability to module

9b2648f

This is a first pass at the module to make sure it is being converted properly. Related to spring-projects#10083

Add nullability to org.springframework.messaging.core package

ed5f14f

Apply code changes from code review

de899a0

Add TaskScheduler for adapter in testNet tests

6081100

cppwfs force-pushed the SI-NULLIFY-CORE-1 branch from 16e7b45 to 7305d7b Compare June 16, 2025 13:07

artembilan requested changes Jun 16, 2025

View reviewed changes

		@@ -125,8 +125,10 @@ public abstract class AbstractCorrelatingMessageHandler extends AbstractMessageP

		private boolean releaseStrategySet;

		@SuppressWarnings("NullAway.Init")

		return message.getHeaders().get(IntegrationMessageHeaderAccessor.SEQUENCE_SIZE, Integer.class);
		}

Introduce NullMarked to some packages in spring-integration-core module #10097

Are you sure you want to change the base?

Introduce NullMarked to some packages in spring-integration-core module #10097

Uh oh!

Conversation

cppwfs commented Jun 11, 2025

Uh oh!

artembilan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cppwfs Jun 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

artembilan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

artembilan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

artembilan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

cppwfs Jun 11, 2025 •

edited

Loading