Add library model for Apache Commons CollectionUtils.isNotEmpty, Amazon CollectionUtils.IsNullOrEmpty, and a couple Amazon StringUtils methods #1242
Conversation
|
|
WalkthroughAdded 19 library-model method entries into two internal maps to mark various utility methods as null/empty guards; added four positive tests exercising those guards; added the AWS SDK utils test dependency and wired it into the nullaway test configuration. Changes
Sequence Diagram(s)sequenceDiagram
participant Test as Test code
participant Analyzer as NullAway Analyzer
participant Models as LibraryModelsHandler
Test->>Analyzer: Analyze guarded dereference (call to utility e.g., isEmpty/isNullOrEmpty/isBlank)
Analyzer->>Models: Query if invoked method implies non-null for parameter 0
Models-->>Analyzer: Yes (method present in FAIL_IF_NULL_PARAMETERS / NULL_IMPLIES_TRUE_PARAMETERS)
Analyzer-->>Test: Mark subsequent dereference as safe
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
nullaway/src/main/java/com/uber/nullaway/handlers/LibraryModelsHandler.java (1)
716-730: Optional: broaden coverage to common AWS/string and map variants.If in scope, consider adding these to reduce false positives across more codebases:
- com.amazonaws.util.StringUtils.isNullOrEmpty(String)
- com.amazonaws.util.CollectionUtils.isNullOrEmpty(java.util.Map)
Proposed additions near this block:
.put( methodRef( "com.amazonaws.util.CollectionUtils", "isNullOrEmpty(java.util.Collection<?>)"), 0) + .put( + methodRef( + "com.amazonaws.util.CollectionUtils", + "isNullOrEmpty(java.util.Map<?,?>)"), + 0) + .put( + methodRef( + "com.amazonaws.util.StringUtils", + "isNullOrEmpty(java.lang.String)"), + 0)Please verify these signatures against your dependency versions before adding.
nullaway/src/test/java/com/uber/nullaway/FrameworkTests.java (1)
1071-1129: Add a negative test to cover the then-branch behavior.For completeness/regression protection, add a case that asserts an error when dereferencing within the then-branch:
- if (CollectionUtils.isEmpty(s)) { s.get(0); } // expect diagnostic
This ensures the cast applies only to the else-branch for null-implies-true models.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
nullaway/src/main/java/com/uber/nullaway/handlers/LibraryModelsHandler.java(1 hunks)nullaway/src/test/java/com/uber/nullaway/FrameworkTests.java(1 hunks)
🔇 Additional comments (5)
nullaway/src/main/java/com/uber/nullaway/handlers/LibraryModelsHandler.java (1)
716-730: Models correctly extend null-implies-true coverage for common collection helpers.Signatures and package names look correct and consistent with existing entries. This will enable the expected else-branch non-null cast for guarded collections.
nullaway/src/test/java/com/uber/nullaway/FrameworkTests.java (4)
1071-1089: LGTM: positive test for Apache Commons Collections (v3) isEmpty guard.Pattern matches null-guard via early return; exercises else-branch non-null inference.
1091-1109: LGTM: positive test for Apache Commons Collections4 isEmpty guard.Covers the generics variant; consistent with library model entry.
1111-1129: LGTM: positive test for AWS CollectionUtils.isNullOrEmpty(Collection<?>) guard.Complements the new model entry and typical usage patterns.
1071-1129: Verify external CollectionUtils stubs are availableI couldn’t locate any stub implementations for the imported CollectionUtils classes in the repository. Please confirm that the following stub files exist in your test stub directory (and are on the compilation classpath):
org/apache/commons/collections/CollectionUtils.javawith
public static boolean isEmpty(Collection<?>)org/apache/commons/collections4/CollectionUtils.javawith
public static <T> boolean isEmpty(Collection<T>)com/amazonaws/util/CollectionUtils.javawith
public static <T> boolean isNullOrEmpty(Collection<T>)Without these, the tests in
FrameworkTests.java(lines 1071–1129) will fail to compile.
f153f1b to
32b5d63
Compare
|
Thanks for the contribution @rmacqueen! Does the suggestion below from the AI bot make any sense? If it does maybe we could handle it in this PR? Otherwise the change looks good.
|
…mazon CollectionUtils.IsNullOrEmpty
|
@msridhar Sure that does make sense! Will add update my PR to add those |
msridhar
left a comment
There was a problem hiding this comment.
LGTM! If you run ./gradlew spotlessApply that should fix the formatting errors that CI is complaining about. Also, if you push all commits under the rmacqueen GitHub account that should fix the CLA check.
a370abd to
dacfb73
Compare
|
@rmacqueen looks like a test is failing. Could you take a look? |
|
@rmacqueen I think the issue with the failing test is that the fully-qualified class name for the Amazon class is Also, if you could not force push that would be great, as it loses the commit history. We will squash commits before merging. |
|
Looks like there are 1.x and 2.x versions of the Amazon utilities, and the package naming changed from |
|
@msridhar Thanks for pointing that out. I’ll update the library models class to use software.amazon.awssdk.utils.CollectionUtils so the test passes. Also noted on avoiding force pushes; I’ll keep the commit history intact going forward. |
|
Let’s just support 2.x going forward. I’ll update the library models to only include software.amazon.* |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #1242 +/- ##
============================================
+ Coverage 88.48% 88.50% +0.02%
Complexity 2373 2373
============================================
Files 90 90
Lines 7787 7802 +15
Branches 1555 1555
============================================
+ Hits 6890 6905 +15
Misses 452 452
Partials 445 445 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Thanks again for the contribution @rmacqueen!! I tweaked the |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
nullaway/src/main/java/com/uber/nullaway/handlers/LibraryModelsHandler.java (1)
726-731: Add the Map<?> variant for AWS CollectionUtils.isNullOrEmpty for completenessAWS SDK v2 also provides isNullOrEmpty(Map). Modeling it here will extend the same guard semantics to map checks.
Apply this diff within the same NULL_IMPLIES_TRUE_PARAMETERS builder:
.put( methodRef( "software.amazon.awssdk.utils.CollectionUtils", "isNullOrEmpty(java.util.Collection<?>)"), 0) + .put( + methodRef( + "software.amazon.awssdk.utils.CollectionUtils", + "isNullOrEmpty(java.util.Map<?,?>)"), + 0)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
nullaway/src/main/java/com/uber/nullaway/handlers/LibraryModelsHandler.java(1 hunks)nullaway/src/test/java/com/uber/nullaway/FrameworkTests.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- nullaway/src/test/java/com/uber/nullaway/FrameworkTests.java
🔇 Additional comments (2)
nullaway/src/main/java/com/uber/nullaway/handlers/LibraryModelsHandler.java (2)
716-738: Correct: model these helpers under NULL_IMPLIES_TRUEThese entries correctly capture “null implies true” semantics for the Apache Commons and AWS SDK v2 helpers. This will tighten flow analysis (else-branch implies NONNULL) without over-constraining arguments.
732-738: FAIL_IF_NULL_PARAMETERS block is clean of AWS/Apache helpersVerified the
FAIL_IF_NULL_PARAMETERSbuilder in LibraryModelsHandler.java and found no references to
software.amazon.awssdk.utils.{StringUtils,CollectionUtils}or
org.apache.commons.collections.CollectionUtils. No further changes needed.
|
@msridhar Many thanks! Ah and I see you updated the author again too. Sorry about that! Really appreciate you helping get this PR in! |
Summary
This PR extends DefaultLibraryModels to recognize additional null-check helper methods from common libraries, ensuring NullAway treats them as guarding against null values.
Added support for:
Each method is now mapped so that NullAway understands they perform null checks on their first argument.
Added corresponding unit tests in FrameworkTests to validate behavior for Apache Commons Collections, Apache Commons Collections4, and AWS CollectionUtils.
Impact
With these changes, NullAway will correctly recognize these helper methods as null guards, reducing false positives when developers use them to validate collections or objects before dereferencing.
Summary by CodeRabbit
New Features
Tests
Chores