Add JSpecify Nullability checks to SI #10165
Closed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
My Nullability fix has been pushed for AMQP module. Thank you! |
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
* Update formating errors from previous commits * Remove ignore SuppressWarning flags for methods that have been updated to be null safe * Make sure added private methods in tests are static * Update tests so they will succeed after the addition of nullification * Remove Disable from tests * Add appropriate beans, BeanFactories to tests that now fail because of nullify * Remove Mock and use createTestApplicationContext * Apply fixes recommended by Artem * Create environment such that the tests have the beans necessary to attain their test target * Update the TestApplicationContainerAware so that it can handle multiple context refreshes Since gradle can run multiple tests across multiple threads the static context can be refreshed multiple times. Thus we need to add code to handle the potential exception * You will note that are few classes where mocking is used instead of the components of TestApplicationContextAware. This is because test relied on mocking for additional setup of the test or checks This PR participates in the nullification issue: spring-projects#10083
* Rename CONTEXT to TEST_INTEGRATION_CONTEXT for increased clarity of purpose. * Remove Duplicate bean registrations from tests that are already handled by TestUtils.createTestApplicationContext * Replace @BeforAll and AfterAll prep and cleanup with the TestApplicationContextAware interface in PayloadAndHeaderMappingTests.java * Set BeanFactory for ThreadAffinityClientConnectionFactory.java * Add setBeanFactory method to JmsInboundGateway.java so that users can set its beanfactory as sell as for its endpoint * Added javadocs to the TestApplicationContextAware interface. * Make sure that ExpressionEvaluatingCorrelationStrategy.getCorrelationKey returns non null * Rebased
cppwfs
force-pushed
the
SI-NULLIFY-CORE-TMP-4
branch
from
18a0068 to
b143dfd
Compare
artembilan
reviewed
Jun 26, 2025
...core/src/main/java/org/springframework/integration/aggregator/CorrelatingMessageBarrier.java
Outdated
Show resolved
Hide resolved
|
PR has been rebased against main and looks good! |
artembilan
requested changes
Jun 26, 2025
...java/org/springframework/integration/aggregator/ExpressionEvaluatingCorrelationStrategy.java
Outdated
Show resolved
Hide resolved
...ava/org/springframework/integration/aggregator/ExpressionEvaluatingMessageListProcessor.java
Outdated
Show resolved
Hide resolved
...ion-core/src/main/java/org/springframework/integration/context/IntegrationObjectSupport.java
Outdated
Show resolved
Hide resolved
…able Remove SupressWarnings from IntegrationObjectSupport.afterPropertiesSet
…lability on the onInit
artembilan
requested changes
Jun 27, 2025
| this.releaseStrategy = releaseStrategy; | ||
| } | ||
|
|
||
| @SuppressWarnings("NullAway") |
There was a problem hiding this comment.
Instead of this those properties has to be marked as @SuppressWarnings("NullAway.Init").
The point is that bean cannot be in the state where those properties are null.
Therefore the check in the onInit() and that's it.
There was a problem hiding this comment.
The SuppressWarnings were applied to the handleMessageInternal and receive methods after moving the null checks to the onInit, because JSpecify isn't smart enough to see the optimization as a viable way to check for nullability, thus it flags it during compilation time.. i.e checking at onInit time and the null protection offered on the setters are valid methods to prevent a NPE, it does's see that.
There was a problem hiding this comment.
Correct. Then instead of @Nullable on those properties, use @SuppressWarnings("NullAway.Init").
There was a problem hiding this comment.
In this case "NullAway.Init" does not suppress the failure for these methods.
There was a problem hiding this comment.
Good.
Let me double check locally on merge!
I think I had something similar in other places.
Your PR is too big and already good enough to move on.
Thanks
|
Merged after rebase and squash as: a27c74c. Some clean up will be done in the separate commit. Thanks again! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is a refresh of the the PR #10097. The 10097 was closed due its age and some restructuring.
This PR picks up with requested changes from the last review (so you will see the original commits). The last commit contains the changes that were requested in the last review of 10097 as well as a rebase to the latest main.