97 implement null handling with jspecify#120
Conversation
- @NullMarked to src/main/java/org/example - Checked all files in example for handling null return values.
- Checked all files for handling null return values
- Checked all files for handling null return values
- Checked all files for handling null return values
- Checked all files for handling null return values
- Checked all files for handling null return values
- Checked all files for handling null return values
- Checked ConfigLoaderTest for null handling
- Checked all tests for null handling - Marked body in HttpRequest parameters as nullable to satisfy tests in CompressionFilterTest. - Marked ip parameters for normalizeIp, addBlockedIp and addAllowedIp to be Nullable since they work as intended but warnings need to be suppressed. -Added @nullable to request parameter for doFilter in LocaleFilter since it is handled.
- Checked all tests for null handling - Marked filename parameter in detectMimeType as Nullable since it's handled.
- Checked all tests for null handling - Marked "in" parameter for setReader in HttpParser as nullable since it is handled.
- Checked ConfigurableFilterPipelineTest for null handling - Marked the routePatterns parameter in the FilterRegistration record in FilterRegistration.java since it is safely null handled.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review infoConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds JSpecify nullability: introduces the jspecify dependency, applies Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the 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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/main/java/org/example/httpparser/HttpParser.java (1)
18-22:⚠️ Potential issue | 🔴 CriticalNull-pointer exception when
inis null.The parameter
inis marked@Nullable, indicating null is a valid input, but line 20 passesindirectly toInputStreamReaderwithout a null check. Ifinis null andthis.readeris also null, this will throw aNullPointerException.Either remove
@Nullableif null is not intended to be accepted, or add null handling:Option 1: Remove `@Nullable` if null input is not valid
- public void setReader(`@Nullable` InputStream in) { + public void setReader(InputStream in) { if (this.reader == null) { this.reader = new BufferedReader(new InputStreamReader(in, StandardCharsets.UTF_8)); } }Option 2: Add null handling if null input should be accepted
public void setReader(`@Nullable` InputStream in) { - if (this.reader == null) { + if (this.reader == null && in != null) { this.reader = new BufferedReader(new InputStreamReader(in, StandardCharsets.UTF_8)); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/httpparser/HttpParser.java` around lines 18 - 22, The setReader method accepts a `@Nullable` InputStream but passes it directly to new InputStreamReader, causing an NPE; update setReader(InputStream in) to handle null: either remove the `@Nullable` annotation if null is invalid (and optionally throw IllegalArgumentException when in is null) or add an explicit null check inside setReader (e.g., if (in == null) { this.reader = null; return; }) before creating new BufferedReader(new InputStreamReader(in, StandardCharsets.UTF_8)); ensure you reference the setReader method, the reader field, and the InputStreamReader creation when making the change.src/main/java/org/example/filter/IpFilter.java (1)
96-107:⚠️ Potential issue | 🟡 MinorReject blank IP values in list mutators.
Line 100 and Line 107 can insert empty strings (
" "→"") into allow/block sets. Treat blank IPs as invalid input.Proposed fix
public void addBlockedIp(`@Nullable` String ip) { - if (ip == null) { - throw new IllegalArgumentException("IP address cannot be null"); + if (ip == null || ip.isBlank()) { + throw new IllegalArgumentException("IP address cannot be null or blank"); } - blockedIps.add(normalizeIp(ip)); + blockedIps.add(ip.trim()); } @@ public void addAllowedIp(`@Nullable` String ip) { - if (ip == null) { - throw new IllegalArgumentException("IP address cannot be null"); + if (ip == null || ip.isBlank()) { + throw new IllegalArgumentException("IP address cannot be null or blank"); } - allowedIps.add(normalizeIp(ip)); + allowedIps.add(ip.trim()); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/filter/IpFilter.java` around lines 96 - 107, addBlockedIp and addAllowedIp currently accept strings that are blank after trimming and insert empty values into blockedIps/allowedIps; update both methods (addBlockedIp and addAllowedIp) to treat blank input as invalid by trimming the input (or using String.isBlank()) and throwing an IllegalArgumentException when the trimmed value is empty before calling normalizeIp and adding to the set, so only non-empty normalized IPs are inserted.src/main/java/org/example/httpparser/HttpRequest.java (1)
27-44:⚠️ Potential issue | 🟠 MajorAlign
bodyfield/getter nullability with constructor contract.The constructor at line 27 accepts
@Nullable String body, but the field at line 20 and getter at line 43 are typed as non-nullableString. This mismatch allowsnullto be stored and returned while the type system claims otherwise. The call site inRequestTimeOutFilter.java:99invokesgetBody()without null-checking, assuming a non-null return.Proposed fix
- private final String body; + private final `@Nullable` String body; @@ - public String getBody() { + public `@Nullable` String getBody() { return body; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/httpparser/HttpRequest.java` around lines 27 - 44, The HttpRequest constructor accepts a nullable body but the class field and getter are declared non-nullable; update the class to reflect the nullable contract: change the field (body) and its getter (getBody()) to return/hold `@Nullable` String (or Optional<String>) consistent with the constructor, and update callers such as RequestTimeOutFilter (e.g., where getBody() is used) to null-check or handle Optional accordingly; ensure headers remain copied as before and preserve immutability when adjusting types in class HttpRequest and usages.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/java/org/example/filter/IpFilter.java`:
- Around line 88-90: Change the method signature of normalizeIp to declare a
nullable return type by annotating the return with `@Nullable` (i.e., add
`@Nullable` to the method return) so it matches the implementation and Javadoc;
update the import if needed (org.jetbrains.annotations.Nullable or the project's
`@Nullable`) and then scan callers of normalizeIp to handle or propagate the
possible null result where applicable (e.g., add null checks or Optional
wrapping) so the codebase respects the nullable contract.
---
Outside diff comments:
In `@src/main/java/org/example/filter/IpFilter.java`:
- Around line 96-107: addBlockedIp and addAllowedIp currently accept strings
that are blank after trimming and insert empty values into
blockedIps/allowedIps; update both methods (addBlockedIp and addAllowedIp) to
treat blank input as invalid by trimming the input (or using String.isBlank())
and throwing an IllegalArgumentException when the trimmed value is empty before
calling normalizeIp and adding to the set, so only non-empty normalized IPs are
inserted.
In `@src/main/java/org/example/httpparser/HttpParser.java`:
- Around line 18-22: The setReader method accepts a `@Nullable` InputStream but
passes it directly to new InputStreamReader, causing an NPE; update
setReader(InputStream in) to handle null: either remove the `@Nullable` annotation
if null is invalid (and optionally throw IllegalArgumentException when in is
null) or add an explicit null check inside setReader (e.g., if (in == null) {
this.reader = null; return; }) before creating new BufferedReader(new
InputStreamReader(in, StandardCharsets.UTF_8)); ensure you reference the
setReader method, the reader field, and the InputStreamReader creation when
making the change.
In `@src/main/java/org/example/httpparser/HttpRequest.java`:
- Around line 27-44: The HttpRequest constructor accepts a nullable body but the
class field and getter are declared non-nullable; update the class to reflect
the nullable contract: change the field (body) and its getter (getBody()) to
return/hold `@Nullable` String (or Optional<String>) consistent with the
constructor, and update callers such as RequestTimeOutFilter (e.g., where
getBody() is used) to null-check or handle Optional accordingly; ensure headers
remain copied as before and preserve immutability when adjusting types in class
HttpRequest and usages.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (22)
pom.xmlsrc/main/java/org/example/App.javasrc/main/java/org/example/config/package-info.javasrc/main/java/org/example/filter/CompressionFilter.javasrc/main/java/org/example/filter/IpFilter.javasrc/main/java/org/example/filter/LocaleFilter.javasrc/main/java/org/example/filter/LocaleFilterWithCookie.javasrc/main/java/org/example/filter/package-info.javasrc/main/java/org/example/http/MimeTypeDetector.javasrc/main/java/org/example/http/package-info.javasrc/main/java/org/example/httpparser/HttpParser.javasrc/main/java/org/example/httpparser/HttpRequest.javasrc/main/java/org/example/httpparser/package-info.javasrc/main/java/org/example/package-info.javasrc/main/java/org/example/server/FilterRegistration.javasrc/main/java/org/example/server/package-info.javasrc/test/java/org/example/config/package-info.javasrc/test/java/org/example/filter/package-info.javasrc/test/java/org/example/http/package-info.javasrc/test/java/org/example/httpparser/package-info.javasrc/test/java/org/example/package-info.javasrc/test/java/org/example/server/package-info.java
…kings and null handling.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/java/org/example/httpparser/HttpParser.java`:
- Around line 18-20: The setter setReader currently silently ignores a null
InputStream and leaves the field reader unset, causing later NPEs in
parseHttp()/parseRequest(); update setReader(InputStream in) to fail fast by
validating the in parameter and throwing an appropriate runtime exception (e.g.,
IllegalArgumentException or NullPointerException) when in is null, instead of
silently returning, so callers get immediate feedback and reader is never left
unexpectedly null; keep the existing behavior of wrapping non-null in with new
BufferedReader(new InputStreamReader(..., StandardCharsets.UTF_8))) and document
the non-null requirement in the method contract for setReader and usages in
parseHttp and parseRequest.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/main/java/org/example/filter/IpFilter.javasrc/main/java/org/example/httpparser/HttpParser.javasrc/main/java/org/example/httpparser/HttpRequest.java
…nge to SetReader in HttpParser
|
Jag kommer att kolla på setReader i HttpParser och det relaterade nullException testet efter jag sovit. |
…solveLocale request parameter since resolveLocale does the actual null handling. - Removed testParserThrowErrorWhenNull test since it is made redundant by jspecify null marking.
# Conflicts: # pom.xml # src/main/java/org/example/filter/CompressionFilter.java # src/main/java/org/example/filter/LocaleFilter.java
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/java/org/example/filter/CompressionFilter.java (1)
213-216:⚠️ Potential issue | 🔴 CriticalBuild blocker:
Encodersymbol is unresolved inbrotliCompress.Line 215 calls
Encoder.compress(data)but the encoder import is missing. Add the missing brotli4j encoder import.🔧 Proposed fix
import com.aayushatharva.brotli4j.Brotli4jLoader; +import com.aayushatharva.brotli4j.encoder.Encoder; import org.example.http.HttpResponseBuilder; import org.example.httpparser.HttpRequest; import org.jspecify.annotations.Nullable;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/filter/CompressionFilter.java` around lines 213 - 216, In CompressionFilter.brotliCompress add the missing brotli4j encoder import and use the Encoder symbol from that library; specifically import com.aayushatharva.brotli4j.encoder.Encoder at the top of the file so the call to Encoder.compress(data) in brotliCompress resolves correctly within the CompressionFilter class.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/main/java/org/example/filter/CompressionFilter.java`:
- Around line 213-216: In CompressionFilter.brotliCompress add the missing
brotli4j encoder import and use the Encoder symbol from that library;
specifically import com.aayushatharva.brotli4j.encoder.Encoder at the top of the
file so the call to Encoder.compress(data) in brotliCompress resolves correctly
within the CompressionFilter class.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
pom.xmlsrc/main/java/org/example/filter/CompressionFilter.javasrc/main/java/org/example/filter/LocaleFilter.java
🚧 Files skipped from review as they are similar to previous changes (1)
- pom.xml
|
Du bör göra en pull request för bara pom.xml istället för med resterande kod. |
- Tests and compilation run without issue on my local setup now.
Har du kollat in dessa? Bra att kolla om det skall tillåta null till att börja med. |
Null-pointer exception when in is null: Reject blank IP values in list mutators: Align body field/getter nullability with constructor contract: |
|
Kom på nu att jag bara implementerade "Align body field/getter nullability with constructor contract" med fixen som gavs av CodeRabbit |
Bra, var den tredje outdated eller är det att den är märkt som non nullable fast kan returnera null? |
Xeutos
left a comment
Xeutos
left a comment
There was a problem hiding this comment.
Ser bra ut i helheten!
Värt att tänka på att det är bättre att göra en pull request för pom.xml separat det minskar mängden merge konflikter.
alicewersen-rgb
left a comment
alicewersen-rgb
left a comment
There was a problem hiding this comment.
Clean and easy to follow implementation. I like that null handling is explicit and that the IP values are normalized before being stored. The code is readable and consistent with the rest of the project!
3 participants
Summary by CodeRabbit
Chores
Refactor
Tests