Please do NOT incorrectly add @Nullable annotations
Closed this issue · 3 comments
I’m sorry if this comes across as offensive, but I’m somewhat skeptical whether the official Spring Data Redis development team has ever actually used their own library in real-world projects.
If you had, you would have easily noticed that the usage of @Nullable annotations across many Spring Data Redis APIs is deeply problematic.
Take, for example, the API org.springframework.data.redis.core.ValueOperations#increment(K). Its return value is annotated with @Nullable.
However, in reality, this Redis command never returns null.
I understand that the official team might argue: “In Redis pipelining or transactions, this command returns null.”
But please think carefully:
First, any Redis command is used in one of two scenarios:
- Direct usage in Java code:
// code snippet 1
Long value = redisTemplate().opsForValue().increment("redisKey");
// value must be NOT null- Usage within Redis pipelining or transactions:
// code snippet 2
redisTemplate.executePipelined(new SessionCallback<>() {
@Override
public List<Object> execute(RedisOperations operations) throws DataAccessException {
Long value = operations.opsForValue().increment("redisKey");
// value is always null
return null;
}
});In 2nd scenario, every Redis command returns null.
So, who in the real world would actually capture the return value inside a pipeline or transaction and write code like this?
// code snippet 3
redisTemplate.executePipelined(new SessionCallback<>() {
@Override
public List<Object> execute(RedisOperations operations) throws DataAccessException {
Long value = operations.opsForValue().increment("redisKey"); // value always null
if (value == null) {
// do something when value is null
} else if (value > 100) {
// do something when value > 100
} else {
// do something when value <= 100
}
return null;
}
});Since value is always null, the if-else logic code above is meaningless in practice.
In real-world applications, no one captures and uses Redis command return values inside pipelines or transactions.
This is simply not a realistic business scenario.
In the real world, the vast majority of Redis commands are executed in first scenario.
Yet, because of the poorly applied @Nullable annotations, every usage of Redis commands now forces developers to suppress or handle IDE null warnings — adding unnecessary overhead — even though null will never be returned.
It’s highly questionable to annotate all non-null APIs with @Nullable just to accommodate an unrealistic edge case that shouldn’t exist in production.
Secondly, the official team might argue that adding @Nullable helps remind developers to use Redis command return values correctly within pipelines or transactions.
Let’s analyze both possibilities:
- If a developer understands pipelining/transactions correctly, they won’t capture the return value at all — so whether
@Nullableis present or not makes no difference. - If a developer is a beginner unfamiliar with pipelining/transactions, and mistakenly captures the return value inside a pipeline,
seeing the IDE null warning might lead them to instinctively write code like code snippet 3 — addingif (value == null) else ...branches to silence the warning.
While this avoids runtime errors, it introduces serious logical bugs, because theelse if/elsebranches will never execute.
In this case, If the API is not annotated with@Nullable, developers won’t add null-checking code, and any incorrect usage will trigger anNullPointerExceptiondirectly at runtime — which, ironically, is the most effective way to expose the issue sooner.
Moreover, even according to your own reasoning — ValueOperations#increment(K) requires the @Nullable annotation, because it may return null in pipelines or transaction, then why does NOT HashOperations#increment(H, HK, long) receive the same annotation? Shouldn’t nearly all Redis commands be consistently annotated with @Nullable ?
Your usage of the @Nullable annotation is confusing, logically inconsistent, and lacks rigor and scientific grounding — it fails to meet the objective demands of real-world scenarios.
To cope with this poor design, we developers are forced to pay an unnecessary additional cost.
Genuine suggestion: Please do not add @Nullable annotations solely based on extreme edge cases — such as capturing return values inside pipelines or transactions — which hold no practical meaning in the real world.
If an API does not return null under normal (non-pipeline, non-transaction) usage, it should not be annotated with @Nullable.
Thank you for an elaborate report and your analysis about the matter. You've covered technical correctness very well and illustrated the inconvenient ergonomics of our Redis Template API.
There are two additional aspects to be mentioned:
- Using Pipelining and Transactional modes through the same API have causes the issue and so the problem is a design problem causing all sorts of downstream issues. To be complete about your example, Redis can be used with
@Transactionalmethods where most reading commands bypass the transaction entirely and only write commands return without a value. In the context of a@Transactionalmethod (potentially in a deeply nested method invocation) it isn't so simple anymore to figure out that you're actually invoking API in a transactional context. - There's also the Kotlin side. Without proper annotations, Kotlin fails on a techical level as Kotlin inserts
nullguards at (to me) random locations to ensure that nonullvalue leaks into a method invoked from Kotlin.
We've identified the @Nullable sprawl as problem quite a while ago, but until Spring Framework 7 there has not been a way to express unknown nullness. This is a state that only JSpecify can really express in a sane way. With the migration to JSpecify we've annotated most of the problematic API's with @NullUnmarked to selectively apply @NonNull on the parameter level.
Please have a look at the current milestone to cross-check whether the migration to JSpecify alleviates the difficulties you're experiencing currently.
If you would like us to look at this issue, please provide the requested information. If the information is not provided within the next 7 days this issue will be closed.
Closing due to lack of requested feedback. If you would like us to look at this issue, please provide the requested information and we will re-open the issue.