23 define and create filter interface - #46
Conversation
|
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:
📝 WalkthroughWalkthroughAdds a filter-chain subsystem: Changes
Sequence DiagramsequenceDiagram
participant Client
participant Chain as FilterChainImpl
participant FilterA as Filter
participant FilterB as Filter
participant Endpoint
Client->>Chain: doFilter(request, response)
Chain->>FilterA: FilterA.doFilter(request, response, this)
FilterA->>Chain: chain.doFilter(request, response)
Chain->>FilterB: FilterB.doFilter(request, response, this)
FilterB->>Chain: chain.doFilter(request, response)
Chain->>Endpoint: execute final request (TODO)
Endpoint-->>Client: response
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 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)
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 |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/main/java/org/example/filter/FilterChain.java`:
- Line 5: The files FilterChain, Filter, and FilterChainImpl incorrectly import
java.net.http.HttpRequest; replace that import with the project's custom request
class org.example.httpparser.HttpRequest so the interfaces and implementations
(e.g., class FilterChain, interface Filter, class FilterChainImpl) use the
correct type and are compatible across the PR.
In `@src/main/java/org/example/filter/FilterChainImpl.java`:
- Around line 5-6: In FilterChainImpl remove the unused import
java.io.IOException and replace the incorrect java.net.http.HttpRequest import
with the servlet request type used by the project (e.g.,
javax.servlet.http.HttpServletRequest) so the class compiles; update any
references inside FilterChainImpl to use HttpServletRequest (matching the
import) and mirror the same fix applied in FilterChain.java.
🧹 Nitpick comments (3)
src/main/java/org/example/filter/FilterChain.java (1)
7-9:FilterChain.doFiltershould not take aFilterChainparameter.In the standard Servlet Filter pattern,
FilterChain.doFilter(request, response)does not accept achainargument — the chain isthis. Thechainparameter is redundant here and also inconsistent with howFilterChainImpl.doFilteractually works (it ignores thechainargument and usesthisinternally on Line 28 ofFilterChainImpl.java).♻️ Simplify the signature
public interface FilterChain { - void doFilter(HttpRequest request, HttpResponseBuilder response, FilterChain chain); + void doFilter(HttpRequest request, HttpResponseBuilder response); }Then update
Filter.doFiltersimilarly — filters callchain.doFilter(request, response)to continue the chain.src/main/java/org/example/filter/FilterChainImpl.java (2)
20-22: Consider making a defensive copy of the filters list.The constructor stores the list by reference. If the caller mutates the list after constructing the chain, it will corrupt the chain's iteration. Use
List.copyOf(filters)for safety, consistent with the defensive-copy approach used inHttpRequestfor headers.♻️ Proposed fix
public FilterChainImpl(List<Filter> filters) { - this.filters = filters; + this.filters = List.copyOf(filters); }
24-32: Thechainparameter is ignored —thisis used instead.Line 28 passes
thisrather than thechainargument, confirming the parameter is redundant (as noted in theFilterChaininterface comment). Also, the TODO on Line 30 should be tracked — when the chain is exhausted, this is where the actual request handler (servlet/endpoint) should be invoked.Would you like me to open an issue to track the TODO for invoking the terminal request handler when all filters have been processed?
|
Jag har märkt att det finns dubbla implementeringar av FilterChain och HttpRequest både här och i PR "Url redirect filter". Jag undrar vilken av dessa implementeringar som passar bäst för vårt projekt? Om vi inkluderar båda kommer vi att få namnkonflikter och redundant kod i projektet. Det vore bra om vi kunde komma överens om en gemensam struktur innan vi mergar. |
|
Du skulle kunna lägga till minimala setAttribute/getAttribute i HttpRequest för att ge filter möjlighet att spara metadata på requesten på ett enkelt och generellt sätt. Till exempel genom att lägga till fältet: private final Map<String, Object> attributes = new HashMap<>(); tillsammans med metoderna: setAttribute(String key, Object value) och: getAttribute(String key). |
|
@AntonAhlqvist Inte alls en dum ide, den kom med i senaste push nu. Krävs att vi behöver veta vilken ordning filtren kommer att gå, och vilka attribut som kommer skrivas i tidigare filter. |
|
@gvaguirres, jag missade din kommentar igår när jag skrev. Ledsen för det! Har du några tankar kring arkitekturen när det kommer till HttpRequest? Som Eric nämnde finns det ju en risk för att filter skriver över varandras data om vi använder setAttribute/getAttribute. Om vi går den vägen krävs det nog att vi är rätt noga med hur vi namnger och hanterar den information som olika filter lägger till. |
viktorlindell12
left a comment
There was a problem hiding this comment.
Grymt jobbat – tydlig HttpRequest-modell och bra att headers görs immutable med Map.copyOf.
En liten tanke: HTTP-headers är case-insensitive, så det kan vara värt att normalisera header-namn (t.ex. lowercase) eller ha en helper getHeader(name) som gör case-insensitive lookup. I övrigt ser det bra ut.
|
Under föreläsningen kom vi fram till att det är bäst att du återställer koden till hur den såg ut innan mitt förslag (det kan implementeras separat), så att pull-requesten kan färdigställas så snart som möjligt. |
AntonAhlqvist
left a comment
There was a problem hiding this comment.
Toppen Eric!
Det var inte meningen att röra till allt, men nu ser det bra ut igen!
AnnaZiafar
left a comment
There was a problem hiding this comment.
Ser bra ut, tycker att det är redo att mergeas
…est that the server needs and easier to handle by future filters
This reverts commit 0fd490e.
4f4ad08 to
525ea10
Compare
* initial commit, added interfaces Filter and FilterChain * Added HttpRequest class, groups together all information about a request that the server needs and easier to handle by future filters * added interfaces Filter and FilterChain with Java Servlet Filter architecture. * added FilterChainImpl * Corrected imports from JDKS HttpRequest, to projects HttpRequest class * Changed, params for FilterChain * Updated HttpRequest with attributes, * Revert "Updated HttpRequest with attributes," This reverts commit 0fd490e.
Summary by CodeRabbit