feat: make HttpResponse mutable and implement NotFoundPlugin default - #39
Conversation
- Updated HttpResponse to be mutable to allow filters and plugins to modify responses. - Implemented NotFoundPlugin as a default fallback for the Pipeline. - Added null safety check in Pipeline.setPlugin. - Added unit tests for Pipeline default behavior and NotFoundPlugin.
📝 WalkthroughWalkthroughPipeline now rejects null plugins and exposes its filters and current plugin; HttpResponse was made mutable with setters for status, headers, and body; a NotFoundPlugin returning 404 was added; unit tests for Pipeline and NotFoundPlugin were added and one test assertion was tightened. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant Pipeline as Pipeline
participant Plugin as Plugin (NotFoundPlugin)
participant Response as HttpResponse
Client->>Pipeline: send(HttpRequest)
Pipeline->>Plugin: handle(HttpRequest, HttpResponse)
Plugin-->>Response: setStatusCode(404)
Plugin-->>Response: setStatusText("Not Found")
Plugin-->>Response: setBody("404 - Resource Not Found")
Plugin-->>Pipeline: return
Pipeline-->>Client: return(HttpResponse)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Suggested reviewers
Poem
🚥 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 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: 2
🤖 Fix all issues with AI agents
In `@src/main/java/org/juv25d/http/HttpResponse.java`:
- Around line 5-9: Run the Spotless auto-formatter to fix the Javadoc formatting
violations for the HttpResponse class: execute mvn spotless:apply, then commit
the changes; specifically reformat the Javadoc block above the HttpResponse
class so it meets Spotless rules (wrap/align lines and ensure proper Javadoc
comment structure for the class comment that mentions mutability and
filters/plugins).
In `@src/test/java/org/juv25d/PipelineTest.java`:
- Around line 1-9: Run the Spotless formatter to fix import/spacing issues in
PipelineTest.java: execute mvn spotless:apply, then review and commit the
updated imports and spacing in the file (the import block at the top of
PipelineTest and the class declaration for PipelineTest) so that ordering,
unused imports, and whitespace comply with the Spotless rules; ensure imports
like org.juv25d.filter.FilterChain, org.juv25d.http.HttpRequest,
org.juv25d.http.HttpResponse, org.juv25d.plugin.Plugin, and
org.junit.jupiter.api.Test are correctly ordered/used and there are no extra
blank lines.
🧹 Nitpick comments (2)
src/main/java/org/juv25d/http/HttpResponse.java (1)
47-53:headers()exposes the internal map directly whilesetHeaderalso exists — consider the intended contract.
headers()returns the mutable backing map, allowing callers to bypasssetHeader. This is fine if intentional, but if you later need to intercept header mutations (e.g., validation, case-normalization), you'd need to change this. Worth documenting the intent.src/main/java/org/juv25d/Pipeline.java (1)
20-25: Null guard is appropriate; consider the same foraddFilter.
setPluginnow rejects null, butaddFilter(Line 17) doesn't guard against null filters. If a null filter is added to the list, it would cause an NPE later increateChain()or during filtering. Consider adding the same guard for consistency.Proposed fix
public void addFilter(Filter filter) { + if (filter == null) { + throw new IllegalArgumentException("Filter cannot be null"); + } filters.add(filter); }
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/main/java/org/juv25d/http/HttpResponse.java`:
- Around line 43-45: In HttpResponse::setStatusText, add a null guard to prevent
assigning a null statusText (e.g., throw IllegalArgumentException or default to
an empty/non-null string) so downstream code that writes the HTTP status line
cannot receive null; update setStatusText(String statusText) to validate the
parameter and follow the same defensive behavior used in other setters of
HttpResponse.
- Around line 24-29: The HttpResponse constructor does not guard against a null
headers parameter causing a NullPointerException at new
LinkedHashMap<>(headers); update the constructor (HttpResponse(int statusCode,
String statusText, Map<String, String> headers, byte[] body)) to defensive-copy
headers the same way body is handled—e.g., if headers is null, use an empty map,
otherwise new LinkedHashMap<>(headers)—so this.headers always gets a valid
LinkedHashMap copy.
🧹 Nitpick comments (1)
src/main/java/org/juv25d/http/HttpResponse.java (1)
47-49:headers()exposes the internal mutable map — inconsistent withbody()defensive copying.
body()returns a clone to protect internal state, butheaders()hands out the backingLinkedHashMapdirectly. Callers can bypasssetHeaderand mutate headers in uncontrolled ways (e.g., removing entries, clearing the map). If this is intentional for pipeline convenience, consider documenting it; otherwise, returningCollections.unmodifiableMap(headers)and routing all mutations throughsetHeaderwould be more consistent.
fmazmz
left a comment
fmazmz
left a comment
There was a problem hiding this comment.
nice!
Some suggestions from my end to keep the architecture consistent and each class have one responsibility.
|
@fmazmz pushed an updated version from your reviews |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/main/java/org/juv25d/http/HttpResponse.java`:
- Around line 45-47: The setHeader method currently allows null header names and
values and should validate inputs like setStatusText does; update
setHeader(String name, String value) to check that name is not null and not
empty (trimmed) and that value is not null, throwing an IllegalArgumentException
with a clear message if validation fails, then only call headers.put(name,
value) when the checks pass (use the existing headers field).
🧹 Nitpick comments (2)
src/main/java/org/juv25d/http/HttpResponse.java (2)
49-51: Redundant null check inbody()getter.After construction and
setBody,this.bodyis guaranteed non-null (both paths assign either a clone ornew byte[0]). Thebody != nullternary in the getter is dead code.public byte[] body() { - return body != null ? body.clone() : new byte[0]; + return body.clone(); }
41-43:headers()exposes the mutable internal map directly.Any caller can add, remove, or clear headers without going through
setHeader. If this is intentional for pipeline flexibility, consider documenting it. If not, returningCollections.unmodifiableMap(headers)would enforce mutation only throughsetHeader.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/main/java/org/juv25d/Pipeline.java`:
- Around line 38-40: getFilters() currently returns the internal mutable field
filters which lets callers mutate the Pipeline state directly; change
getFilters() to return an unmodifiable snapshot (e.g., use List.copyOf(filters)
or Collections.unmodifiableList(filters)) so callers cannot add/remove/clear
filters and preserve the existing defensive copy behavior used by createChain();
keep addFilter() as the sole mutating API.
In `@src/test/java/org/juv25d/PipelineTest.java`:
- Around line 8-24: Add unit tests for Pipeline.getFilters() (e.g., assert empty
list by default and that added filters are returned) and add a guard in
Pipeline.createChain() to validate a plugin has been set: if plugin is null,
throw an IllegalStateException (or IllegalArgumentException) rather than passing
null into FilterChainImpl. Specifically, update tests to include assertions
around getFilters() and a new test that calling createChain() before setPlugin()
results in the chosen exception, and modify Pipeline.createChain() to check the
plugin field before constructing FilterChainImpl so doFilter() will never call
plugin.handle() on null.
addee1
left a comment
addee1
left a comment
There was a problem hiding this comment.
Looks nice!
Making HttpResponse mutable makes sense with the pipeline setup, since filters and plugins need to be able to modify the response. The NotFoundPlugin is also a good addition as a default fallback, and the null check in Pipeline.setPlugin is a solid safety improvement.
Tests look good and cover the expected behavior.
Well Played😉
5 participants
Closes #38
Summary by CodeRabbit
New Features
Improvements
Tests