Support for Spring's @Value annotation#1505
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #1505 +/- ##
============================================
+ Coverage 88.45% 88.47% +0.02%
- Complexity 2823 2837 +14
============================================
Files 100 101 +1
Lines 9446 9460 +14
Branches 1893 1895 +2
============================================
+ Hits 8355 8370 +15
+ Misses 530 529 -1
Partials 561 561 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (5)
WalkthroughThis PR adds support for Spring framework's Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@nullaway/src/main/java/com/uber/nullaway/NullAway.java`:
- Around line 2645-2648: The current logic calls
config.isExcludedFieldAnnotation(annotationName) first which short-circuits and
prevents SpringUtils.isInjectedByValueAnnotation(fieldSymbol, annotationName)
from being evaluated for org.springframework.beans.factory.annotation.Value;
change the precedence so SpringUtils.isInjectedByValueAnnotation is evaluated
first and if it returns true return true, otherwise fall back to
config.isExcludedFieldAnnotation(annotationName) to decide; update the block
that currently calls config.isExcludedFieldAnnotation(...) then
SpringUtils.isInjectedByValueAnnotation(...) to call
SpringUtils.isInjectedByValueAnnotation(...) first (or explicitly allow the
Value annotation through) so `@Value`("#{null}") is handled by the Spring-specific
check.
In `@nullaway/src/main/java/com/uber/nullaway/SpringUtils.java`:
- Around line 47-48: Remove the local `@Nullable` annotation on the variable
declaration for annotationValue; change "@Nullable String annotationValue =
NullabilityUtil.getAnnotationValue(fieldSymbol, VALUE_ANNOT, true);" to just
"String annotationValue = NullabilityUtil.getAnnotationValue(fieldSymbol,
VALUE_ANNOT, true);" and also remove any now-unused Nullable import. Keep the
call to NullabilityUtil.getAnnotationValue and references to fieldSymbol and
VALUE_ANNOT unchanged.
- Around line 34-35: VALUE_NULL_SPEL_PATTERN in SpringUtils.java currently
matches "null" inside SpEL even when it's inside quoted string literals; update
the pattern so it skips over single-quoted and double-quoted substrings when
scanning inside "#{...}" (i.e., match a repeated sequence of either
non-quote/non-brace characters OR a single-quoted string OR a double-quoted
string, and look for a word-boundary "null" only in those non-quoted parts),
replace the existing Pattern.compile usage that defines VALUE_NULL_SPEL_PATTERN
with this refined pattern, and add a unit test that verifies `@Value`("#{'null'}")
(and similarly quoted "null") is not flagged as nullable while an unquoted null
literal is flagged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 7af2b912-a73b-443c-a8f2-972b3ddacd0c
📒 Files selected for processing (4)
nullaway/src/main/java/com/uber/nullaway/NullAway.javanullaway/src/main/java/com/uber/nullaway/NullabilityUtil.javanullaway/src/main/java/com/uber/nullaway/SpringUtils.javanullaway/src/test/java/com/uber/nullaway/FrameworkTests.java
| if (config.isExcludedFieldAnnotation(annotationName)) { | ||
| return true; | ||
| } | ||
| return SpringUtils.isInjectedByValueAnnotation(fieldSymbol, annotationName); |
There was a problem hiding this comment.
Don’t let ExcludedFieldAnnotations short-circuit the @Value analysis.
Lines 2645-2648 return early for any excluded field annotation, so a config that includes org.springframework.beans.factory.annotation.Value will never reach the new Spring-specific null-SpEL check. In that setup, @Value("#{null}") is still treated as always initialized and the false negative this PR is trying to fix remains.
Suggested precedence change
.anyMatch(
anno -> {
String annotationName = anno.getAnnotationType().toString();
+ if (annotationName.equals(SpringUtils.VALUE_ANNOT)) {
+ return SpringUtils.isInjectedByValueAnnotation(fieldSymbol, annotationName);
+ }
if (config.isExcludedFieldAnnotation(annotationName)) {
return true;
}
- return SpringUtils.isInjectedByValueAnnotation(fieldSymbol, annotationName);
+ return false;
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (config.isExcludedFieldAnnotation(annotationName)) { | |
| return true; | |
| } | |
| return SpringUtils.isInjectedByValueAnnotation(fieldSymbol, annotationName); | |
| String annotationName = anno.getAnnotationType().toString(); | |
| if (annotationName.equals(SpringUtils.VALUE_ANNOT)) { | |
| return SpringUtils.isInjectedByValueAnnotation(fieldSymbol, annotationName); | |
| } | |
| if (config.isExcludedFieldAnnotation(annotationName)) { | |
| return true; | |
| } | |
| return false; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@nullaway/src/main/java/com/uber/nullaway/NullAway.java` around lines 2645 -
2648, The current logic calls config.isExcludedFieldAnnotation(annotationName)
first which short-circuits and prevents
SpringUtils.isInjectedByValueAnnotation(fieldSymbol, annotationName) from being
evaluated for org.springframework.beans.factory.annotation.Value; change the
precedence so SpringUtils.isInjectedByValueAnnotation is evaluated first and if
it returns true return true, otherwise fall back to
config.isExcludedFieldAnnotation(annotationName) to decide; update the block
that currently calls config.isExcludedFieldAnnotation(...) then
SpringUtils.isInjectedByValueAnnotation(...) to call
SpringUtils.isInjectedByValueAnnotation(...) first (or explicitly allow the
Value annotation through) so `@Value`("#{null}") is handled by the Spring-specific
check.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
nullaway/src/main/java/com/uber/nullaway/NullAway.java (1)
2645-2649:⚠️ Potential issue | 🟠 MajorHandle
@Valuebefore excluded-annotation short-circuit.At Line 2645, excluded annotations return early before the Spring
@Valuenull-expression check runs. Iforg.springframework.beans.factory.annotation.Valueis configured as an excluded field annotation,@Value("#{null}")is treated as initialized and the null-producing case is missed.Suggested precedence fix
return NullabilityUtil.getAllAnnotations(fieldSymbol, config) .anyMatch( anno -> { String annotationName = anno.getAnnotationType().toString(); + if (annotationName.equals(SpringUtils.VALUE_ANNOT)) { + return SpringUtils.isInjectedByValueAnnotation(anno); + } if (config.isExcludedFieldAnnotation(annotationName)) { return true; } - return SpringUtils.isInjectedByValueAnnotation(anno); + return false; });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@nullaway/src/main/java/com/uber/nullaway/NullAway.java` around lines 2645 - 2649, The exclusion check short-circuits before recognizing Spring `@Value` null-expressions: change the logic in the predicate so SpringUtils.isInjectedByValueAnnotation(anno) is evaluated before or in conjunction with config.isExcludedFieldAnnotation(annotationName) (e.g., call isInjectedByValueAnnotation first and if true return its result, otherwise apply isExcludedFieldAnnotation) so that `@Value`("#{null}") is not treated as initialized when the annotation is configured as excluded; update the predicate surrounding config.isExcludedFieldAnnotation and SpringUtils.isInjectedByValueAnnotation to enforce this precedence.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@nullaway/src/main/java/com/uber/nullaway/NullAway.java`:
- Around line 2645-2649: The exclusion check short-circuits before recognizing
Spring `@Value` null-expressions: change the logic in the predicate so
SpringUtils.isInjectedByValueAnnotation(anno) is evaluated before or in
conjunction with config.isExcludedFieldAnnotation(annotationName) (e.g., call
isInjectedByValueAnnotation first and if true return its result, otherwise apply
isExcludedFieldAnnotation) so that `@Value`("#{null}") is not treated as
initialized when the annotation is configured as excluded; update the predicate
surrounding config.isExcludedFieldAnnotation and
SpringUtils.isInjectedByValueAnnotation to enforce this precedence.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: b33608b7-9445-4099-a499-03662d62d978
📒 Files selected for processing (5)
AGENTS.mdnullaway/src/main/java/com/uber/nullaway/NullAway.javanullaway/src/main/java/com/uber/nullaway/NullabilityUtil.javanullaway/src/main/java/com/uber/nullaway/SpringUtils.javanullaway/src/test/java/com/uber/nullaway/FrameworkTests.java
lazaroclapp
left a comment
lazaroclapp
left a comment
There was a problem hiding this comment.
I mean, code itself LGTM, but I do want to question a bit why we don't want this in a handler. I think overall the evidence is that handlers are fine performance-wise, so the tradeoff is slightly more code for a cleaner separation of concerns.
I feel support for specific frameworks beyond JSpecify and common flavors of @Nullable annotations should almost always go in either library models or handlers. It can be a general SpringHandler, in case later we need more spring specific functionality.
You call in the end, happy to stamp this if you want, code seems fine :)
|
@lazaroclapp you convinced me :-) I moved the logic to a new |
lazaroclapp
left a comment
lazaroclapp
left a comment
There was a problem hiding this comment.
LGTM! Appreciate it being a handler
| } | ||
|
|
||
| @Override | ||
| public boolean shouldSkipFieldInitializationCheck(AnnotationMirror annotationMirror) { |
There was a problem hiding this comment.
Surprised we didn't have something like this before, but at a glance, we don't, because we always supported the list of initialization skipping annotations "natively" in core + arguments. This could now be used to extend library models to add initialization suppressing annotations 🤔 (This is a nice to have, though, not an urgent task, and much less a blocker for this PR, just musing)
There was a problem hiding this comment.
Yeah, in all cases thus far, just having the fully-qualified annotation name was enough, so doing it as a CLI argument worked fine. This case was different, since we don't just care about the name of the annotation, but also the contents of its value attribute. I think I'd like to see more motivation before adding general support in library models for this
2 participants
Fixes #1483
We add some specialized logic to treat Spring's
@Valueas an external initializer annotation, but only when its SpEL expression does not containnull(which would indicate the field could be initialized to null). Rather than adding a whole extraHandlerfor this, we bake in a very small amount of Spring-specific logic. Also do some refactorings and cleanup inNullabilityUtilwhile editing that code.Summary by CodeRabbit
New Features
@Valueannotation in null-safety analysis, allowing proper handling of Spring-managed fields.Documentation
Tests