Conversation
📝 WalkthroughWalkthroughIntroduces a filter chain pattern for HTTP request processing with configurable redirect rules. Includes FilterChain orchestrator, TerminalHandler interface, RedirectFilter implementation, RedirectRule data class with validation, RedirectRulesLoader utility for pattern compilation supporting wildcards and regex, and comprehensive unit tests. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as HTTP Client
participant FC as FilterChain
participant RF as RedirectFilter
participant TH as TerminalHandler
Client->>FC: doFilter(request, response)
loop While filters remain
FC->>RF: doFilter(request, response, chain)
alt Rule matches
RF->>RF: matches(requestPath)
RF->>response: setStatus(statusCode)
RF->>response: setHeader("Location", targetUrl)
Note over RF: Stop processing
else No match
RF->>FC: chain.doFilter(request, response)
FC->>TH: handle(request, response)
TH->>response: populateResponse
end
end
FC-->>Client: Response with redirect or content
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labels
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 |
![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
🤖 Fix all issues with AI agents
In `@src/main/java/org/example/server/RedirectResponse.java`:
- Around line 3-19: Remove the unused mutable class RedirectResponse: delete the
RedirectResponse class entirely (it is not referenced anywhere; RedirectFilter
sets status and headers directly) and ensure no leftover imports reference
RedirectResponse; follow the PR convention used by RedirectRule (declare types
as final/immutable) by not reintroducing this mutable type—run a build to
confirm no usages remain and adjust any tests or references to use HttpResponse
via RedirectFilter instead.
🧹 Nitpick comments (2)
src/main/java/org/example/server/RedirectRulesLoader.java (1)
34-45:wildcardToRegexproduces unnecessarily bloated patterns by quoting each character individually.
Pattern.quote(String.valueOf(c))wraps every single literal character in\Q…\E. For"/docs/*"this yields\Q/\E\Qd\E\Qo\E\Qc\E\Qs\E\Q/\E.*instead of the simpler\Q/docs/\E.*. Consider accumulating runs of literal characters and quoting them in one call.♻️ Suggested refactor
private static String wildcardToRegex(String wildcard) { StringBuilder sb = new StringBuilder(); - for (int i = 0; i < wildcard.length(); i++) { - char c = wildcard.charAt(i); - if (c == '*') { - sb.append(".*"); - } else { - sb.append(Pattern.quote(String.valueOf(c))); + int start = 0; + for (int i = 0; i < wildcard.length(); i++) { + if (wildcard.charAt(i) == '*') { + if (i > start) { + sb.append(Pattern.quote(wildcard.substring(start, i))); + } + sb.append(".*"); + start = i + 1; } } + if (start < wildcard.length()) { + sb.append(Pattern.quote(wildcard.substring(start))); + } return sb.toString(); }src/main/java/org/example/server/FilterChain.java (1)
15-18: Consider defensively copying thefiltersarray.The constructor stores the caller's array reference directly. If the caller later mutates the array, the chain's behavior becomes unpredictable. A shallow copy is cheap and eliminates this risk.
🛡️ Proposed fix
public FilterChain(HttpFilter[] filters, TerminalHandler terminal) { - this.filters = Objects.requireNonNull(filters, "filters"); + Objects.requireNonNull(filters, "filters"); + this.filters = filters.clone(); this.terminal = Objects.requireNonNull(terminal, "terminal"); }
gitnes94
left a comment
gitnes94
left a comment
There was a problem hiding this comment.
Allt ser bra ut nu, jätteduktigt jobbat!
|
Jag föreslår att vi pausar merge av specifika filter då jag har märkt att vi har dubbla implementeringar av FilterChain och HttpRequest, tills vi har definierat en gemensam arkitektur och paketstruktur. Annars kommer vi att få namnkonflikter och redundant kod eller vad tycker ni? |
|
Låter smart tycker jag! |
|
Ja precis @eeebbaandersson. Jag håller med dig att vänta på @eraiicphu basstrukturen |
|
Wild Eric joins conversation: Vi tänkte på samma sak även när det kom till HttpRequest klass, men ser att du har färre fält (?) Jag speglade klassen HttpParseRequestLina som mergades den klassen tänker jag är den som parsar inkommande HTTP från browsern. Så bara från lite snabb kik så verkar det som att mina klasserna: Ser även att du gjort en HttpResponse. |
|
Vi kan försöka linka ihop så kan vi försöka reda ut och få det att funka tillsammans :D |
|
Jag föreslår att vi kör basstrukturen som @eraiicphu gjort som grund i sin issue #23, då den ändå handlade om att definiera interfacen. Tänker att det blir lättare att ha det satt på plats och sen får vi övriga som skapar separata filter modifiera vår kod lite för att förhålla oss till detta? Som Eric nämnde fanns ju även färdig kod i main för HttpResponse via HttpResponseBuilder. @Ericthilen @gvaguirres @viktorlindell12 @AnnaZiafar @fredrikmohlen @Rickank @apaegs Vad tror ni om detta? :) |
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (4)
src/test/java/org/example/filter/redirect/RedirectFilterTest.java (2)
108-112: Consider asserting the exception message for richer test diagnostics.Currently the test only verifies the exception type. Chaining
.withMessageContaining(...)makes the intent explicit and gives better failure output if the wrongIllegalArgumentExceptionis thrown (e.g., from an unrelated guard).✨ Suggested addition
assertThatThrownBy(() -> new RedirectRule(Pattern.compile("^/x$"), "/y", 307)) - .isInstanceOf(IllegalArgumentException.class); + .isInstanceOf(IllegalArgumentException.class) + .withMessageContaining("307");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/org/example/filter/redirect/RedirectFilterTest.java` around lines 108 - 112, The test redirect_rule_rejects_invalid_status_code in RedirectFilterTest currently only asserts the exception type; update the assertion to also check the exception message to make the failure diagnostic-specific (e.g., chain .withMessageContaining(...) or equivalent) so it verifies the IllegalArgumentException thrown by the RedirectRule constructor (new RedirectRule(Pattern.compile("^/x$"), "/y", 307)) contains the expected message about invalid status codes.
92-98: Consider covering edge cases/docs/and/docsin the wildcard test.The current assertions document that
*is a single-segment wildcard but leave undefined whether/docs/(empty segment) or/docs(missing segment entirely) match. These boundary cases are the most likely sources of future regressions when the pattern-compilation logic is modified.✨ Suggested additions
`@Test` void wildcard_matching_docs_star() { var p = RedirectRulesLoader.compileSourcePattern("/docs/*"); assertThat(p.matcher("/docs/test").matches()).isTrue(); assertThat(p.matcher("/docs/any/path").matches()).isFalse(); assertThat(p.matcher("/doc/test").matches()).isFalse(); + assertThat(p.matcher("/docs/").matches()).isFalse(); // empty segment + assertThat(p.matcher("/docs").matches()).isFalse(); // no segment at all }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/org/example/filter/redirect/RedirectFilterTest.java` around lines 92 - 98, Add assertions to the wildcard_matching_docs_star test to cover the boundary cases: verify that compileSourcePattern("/docs/*") does NOT match "/docs/" (empty segment) and does NOT match "/docs" (missing segment). Update RedirectFilterTest (method wildcard_matching_docs_star) using the existing pattern variable p from RedirectRulesLoader.compileSourcePattern to add assertThat(p.matcher("/docs/").matches()).isFalse() and assertThat(p.matcher("/docs").matches()).isFalse().src/main/java/org/example/filter/redirect/RedirectRulesLoader.java (1)
34-44:[^/]*semantics undocumented; per-character quoting is harder to read than a split-based approachTwo points:
Single-segment wildcard —
*maps to[^/]*, meaning/docs/*matches/docs/foobut not/docs/foo/bar. This is a meaningful, non-obvious constraint (unlike, e.g., Ant-style**). It should be documented in the method's Javadoc so rule authors are not surprised.Per-character
Pattern.quote— wrapping each non-*character in its own\Q…\Eescape is correct but noisy and harder to read. Splitting on*and quoting each literal segment is the conventional approach:♻️ Proposed refactor
- private static String wildcardToRegex(String wildcard) { - StringBuilder sb = new StringBuilder(); - for (int i = 0; i < wildcard.length(); i++) { - char c = wildcard.charAt(i); - if (c == '*') { - sb.append("[^/]*"); - } else { - sb.append(Pattern.quote(String.valueOf(c))); - } - } - return sb.toString(); - } + /** + * Converts a wildcard path to a regex fragment. + * Each {`@code` *} matches any sequence of non-slash characters (single path segment only). + * Use {`@code` regex:} prefix in the source path for cross-segment or more complex patterns. + */ + private static String wildcardToRegex(String wildcard) { + String[] parts = wildcard.split("\\*", -1); + StringBuilder sb = new StringBuilder(); + for (int i = 0; i < parts.length; i++) { + if (i > 0) { + sb.append("[^/]*"); + } + sb.append(Pattern.quote(parts[i])); + } + return sb.toString(); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/filter/redirect/RedirectRulesLoader.java` around lines 34 - 44, Update the wildcardToRegex method: add Javadoc that clearly states a '*' matches any characters except '/' (single path segment semantics so "/docs/*" does not match "/docs/foo/bar"), and refactor the implementation to avoid per-character Pattern.quote by splitting the input on '*' and Pattern.quote-ing each literal segment, then joining them with the literal segment-regex for '*' (i.e., "[^/]*") to preserve current behavior; reference the wildcardToRegex method when making these changes.src/main/java/org/example/filter/redirect/RedirectRule.java (1)
13-15: Consider supporting additional standard redirect codes (303, 307, 308).RFC 7231/9110 defines 303 See Other, 307 Temporary Redirect, and 308 Permanent Redirect as valid redirect responses. Restricting to only 301/302 will silently reject legitimate configurations. This is a future-proofing concern but worth addressing before the API solidifies.
♻️ Proposed fix
- if (statusCode != 301 && statusCode != 302) { - throw new IllegalArgumentException("statusCode must be 301 or 302"); + if (statusCode != 301 && statusCode != 302 && statusCode != 303 + && statusCode != 307 && statusCode != 308) { + throw new IllegalArgumentException("statusCode must be a valid redirect code (301, 302, 303, 307, 308)"); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/filter/redirect/RedirectRule.java` around lines 13 - 15, The constructor/check in RedirectRule currently throws for any statusCode other than 301 or 302; update the validation to allow the full set of standard redirect status codes (301, 302, 303, 307, 308) instead of only 301/302. Modify the conditional that checks statusCode in RedirectRule to accept 303, 307 and 308 (or validate against a small set/enum of allowed codes) and keep the IllegalArgumentException for any non-redirect codes; ensure any unit tests referencing RedirectRule/statusCode are updated accordingly.
🤖 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/redirect/RedirectFilter.java`:
- Line 31: In RedirectFilter (LOG.info(...) line) the user-controlled variable
path is logged raw; remove control characters or encode them before logging to
prevent CRLF log injection. Update the code around the LOG.info(...) call to
create a sanitizedPath (e.g., strip/replace \r and \n and other control chars or
URL-encode the value) and use sanitizedPath in the log message instead of path;
keep rule.getTargetUrl() and rule.getStatusCode() unchanged.
- Around line 27-30: Guard against a null request path before calling
RedirectRule.matches: check the result of request.getPath() and handle null
(e.g., skip processing or treat as empty string) so you never pass null into
rule.matches which calls sourcePattern.matcher(requestPath).matches(); update
the logic around request.getPath() in RedirectFilter (the variable `path`) to
early-return or continue the loop when null, or normalize to a non-null value,
ensuring RedirectRule.matches and its use of sourcePattern.matcher(...) never
receive null.
- Around line 3-6: RedirectFilter currently imports
org.example.httpparser.HttpRequest and org.example.filter.FilterChain which
duplicates existing core types; update RedirectFilter to use the consolidated,
canonical core abstractions (replace imports to the single project definitions
such as org.example.server.HttpRequest and org.example.server.FilterChain or
whichever package your codebase has chosen as canonical), adjust any method
signatures or usages inside RedirectFilter to the canonical types (e.g., the
class name RedirectFilter, method parameters using HttpRequest and FilterChain),
and remove the old conflicting imports so the file consistently references the
single agreed-upon HttpRequest and FilterChain implementations.
In `@src/main/java/org/example/filter/redirect/RedirectRule.java`:
- Line 12: The constructor in RedirectRule currently uses
Objects.requireNonNull(targetUrl, "targetUrl") but doesn't reject empty or blank
strings, which can produce a Location header with no destination; update
RedirectRule to validate targetUrl is not empty/blank (e.g., trim and check
length or use String.isBlank()) and throw an IllegalArgumentException with a
clear message (e.g., "targetUrl must not be blank") when it fails, replacing or
augmenting the existing requireNonNull call so targetUrl is guaranteed non-null
and non-empty for later use.
In `@src/main/java/org/example/filter/redirect/RedirectRulesLoader.java`:
- Around line 18-19: The extracted regex string rawRegex in RedirectRulesLoader
is not trimmed after removing REGEX_PREFIX, so leading spaces (e.g. "regex:
/docs/.*") become part of the pattern and cause non-matching rules; update the
code that sets rawRegex (and the subsequent blank check) to trim() the substring
result (e.g., rawRegex = trimmed.substring(REGEX_PREFIX.length()).trim()) before
calling Pattern.compile and before the blank check, ensuring you still handle
empty/blank values and surface a clear error if the pattern is empty or invalid.
In `@src/test/java/org/example/filter/redirect/RedirectFilterTest.java`:
- Around line 52-67: The test returns_302_redirect currently doesn't assert the
filter stops the chain; add the same AtomicBoolean guard used in
returns_301_redirect_and_stops_pipeline to ensure the FilterChain is not
invoked. Modify the local FilterChain lambda in returns_302_redirect to flip an
AtomicBoolean (e.g., chainCalled.set(true)) if executed, and after
filter.doFilter(...) assert that chainCalled.get() is false in addition to the
existing status/Location assertions; reference RedirectFilter, RedirectRule,
FilterChain and responseAsString when locating the test to change.
---
Nitpick comments:
In `@src/main/java/org/example/filter/redirect/RedirectRule.java`:
- Around line 13-15: The constructor/check in RedirectRule currently throws for
any statusCode other than 301 or 302; update the validation to allow the full
set of standard redirect status codes (301, 302, 303, 307, 308) instead of only
301/302. Modify the conditional that checks statusCode in RedirectRule to accept
303, 307 and 308 (or validate against a small set/enum of allowed codes) and
keep the IllegalArgumentException for any non-redirect codes; ensure any unit
tests referencing RedirectRule/statusCode are updated accordingly.
In `@src/main/java/org/example/filter/redirect/RedirectRulesLoader.java`:
- Around line 34-44: Update the wildcardToRegex method: add Javadoc that clearly
states a '*' matches any characters except '/' (single path segment semantics so
"/docs/*" does not match "/docs/foo/bar"), and refactor the implementation to
avoid per-character Pattern.quote by splitting the input on '*' and
Pattern.quote-ing each literal segment, then joining them with the literal
segment-regex for '*' (i.e., "[^/]*") to preserve current behavior; reference
the wildcardToRegex method when making these changes.
In `@src/test/java/org/example/filter/redirect/RedirectFilterTest.java`:
- Around line 108-112: The test redirect_rule_rejects_invalid_status_code in
RedirectFilterTest currently only asserts the exception type; update the
assertion to also check the exception message to make the failure
diagnostic-specific (e.g., chain .withMessageContaining(...) or equivalent) so
it verifies the IllegalArgumentException thrown by the RedirectRule constructor
(new RedirectRule(Pattern.compile("^/x$"), "/y", 307)) contains the expected
message about invalid status codes.
- Around line 92-98: Add assertions to the wildcard_matching_docs_star test to
cover the boundary cases: verify that compileSourcePattern("/docs/*") does NOT
match "/docs/" (empty segment) and does NOT match "/docs" (missing segment).
Update RedirectFilterTest (method wildcard_matching_docs_star) using the
existing pattern variable p from RedirectRulesLoader.compileSourcePattern to add
assertThat(p.matcher("/docs/").matches()).isFalse() and
assertThat(p.matcher("/docs").matches()).isFalse().
src/main/java/org/example/filter/redirect/RedirectRulesLoader.java
Outdated
Show resolved
Hide resolved
src/test/java/org/example/filter/redirect/RedirectFilterTest.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/main/java/org/example/filter/redirect/RedirectFilter.java (2)
27-27: Null path guard still missing — already flagged in a prior review.If
request.getPath()returnsnull,path.replaceAll(...)(orrule.matches(path)once restored) will throw aNullPointerException. Add a null check before the loop as previously suggested.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/filter/redirect/RedirectFilter.java` at line 27, The code in RedirectFilter uses String path = request.getPath() and then calls path.replaceAll(...) / rule.matches(path) which will NPE if path is null; add a null guard right after obtaining path (e.g., if (path == null) { /* either set path = "" or skip processing/return early depending on desired behavior */ }) so the subsequent loop over rules (and calls to rule.matches(path) or path.replaceAll(...)) never executes with a null path; update RedirectFilter to handle the null case consistently with existing redirect semantics.
3-6: Duplicate type definitions — already flagged in a prior review.This file still imports from
org.example.filter.Filter,org.example.filter.FilterChain, andorg.example.httpparser.HttpRequest, while the canonical project types live underorg.example.server(e.g.,FilterChain.doFilter(HttpRequest, HttpResponse)). These need to be reconciled with the agreed-upon base interfaces before merging.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/filter/redirect/RedirectFilter.java` around lines 3 - 6, The RedirectFilter currently imports incorrect duplicate types (org.example.filter.Filter, org.example.filter.FilterChain, org.example.httpparser.HttpRequest); update imports to use the canonical server interfaces (e.g., org.example.server.Filter, org.example.server.FilterChain, org.example.server.HttpRequest) and adjust any method signatures in RedirectFilter (such as calls to FilterChain.doFilter(HttpRequest, HttpResponse) and the RedirectFilter's implemented Filter.doFilter) to match the canonical interfaces so the class compiles against the agreed base types.
🤖 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/redirect/RedirectFilter.java`:
- Around line 29-36: Restore the missing match condition inside RedirectFilter's
loop: wrap the existing redirect logic (sanitizing path, logging, setting
response status/header and return) in an if (rule.matches(path)) block so the
redirect only runs for matching RedirectRule instances; ensure the braces around
the for (RedirectRule rule : rules) loop and the new if are balanced so
chain.doFilter(...) remains inside the method body and non-matching requests
continue down the filter chain.
---
Duplicate comments:
In `@src/main/java/org/example/filter/redirect/RedirectFilter.java`:
- Line 27: The code in RedirectFilter uses String path = request.getPath() and
then calls path.replaceAll(...) / rule.matches(path) which will NPE if path is
null; add a null guard right after obtaining path (e.g., if (path == null) { /*
either set path = "" or skip processing/return early depending on desired
behavior */ }) so the subsequent loop over rules (and calls to
rule.matches(path) or path.replaceAll(...)) never executes with a null path;
update RedirectFilter to handle the null case consistently with existing
redirect semantics.
- Around line 3-6: The RedirectFilter currently imports incorrect duplicate
types (org.example.filter.Filter, org.example.filter.FilterChain,
org.example.httpparser.HttpRequest); update imports to use the canonical server
interfaces (e.g., org.example.server.Filter, org.example.server.FilterChain,
org.example.server.HttpRequest) and adjust any method signatures in
RedirectFilter (such as calls to FilterChain.doFilter(HttpRequest, HttpResponse)
and the RedirectFilter's implemented Filter.doFilter) to match the canonical
interfaces so the class compiles against the agreed base types.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/main/java/org/example/filter/redirect/RedirectFilter.java (1)
3-6:⚠️ Potential issue | 🟠 MajorUnresolved: conflicting
HttpRequestandFilterChaintype definitions across the codebase.This file still imports
org.example.httpparser.HttpRequestandorg.example.filter.FilterChain, while multiple competing definitions exist in other packages (org.example.server.HttpRequest,org.example.server.FilterChain,org.example.filter.FilterChainImpl). The type consolidation mentioned in the PR comments (waiting for@eraiicphu's base interfaces from Issue#23) has not yet happened; merging this as-is will make that future reconciliation harder.#!/bin/bash # Verify current state of duplicate type definitions echo "=== HttpRequest definitions ===" rg -n "class HttpRequest|interface HttpRequest" -t java -A1 echo "" echo "=== FilterChain definitions ===" rg -n "class FilterChain|interface FilterChain" -t java -A1 echo "" echo "=== Filter interface definitions ===" rg -n "^public interface Filter\b" -t java -A1🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/filter/redirect/RedirectFilter.java` around lines 3 - 6, The file RedirectFilter imports conflicting types (org.example.httpparser.HttpRequest and org.example.filter.FilterChain) which clash with other definitions (org.example.server.HttpRequest, org.example.server.FilterChain, org.example.filter.FilterChainImpl); update RedirectFilter to use the project’s canonical base interfaces (replace the current imports with the agreed server-level interfaces org.example.server.HttpRequest and org.example.server.FilterChain or the canonical Filter interface used across the codebase) and add a short TODO referencing Issue `#23` and `@eraiicphu` so future reconciliation is clear; ensure all references inside class RedirectFilter compile against the chosen HttpRequest and FilterChain symbols and remove any reliance on the duplicate org.example.httpparser or filter-local types.
🧹 Nitpick comments (1)
src/main/java/org/example/filter/redirect/RedirectFilter.java (1)
35-35: Remove debug artifact comment before merge.The inline comment
// <-- DENNA RADEN SKA FINNAS("THIS LINE SHOULD EXIST" in Swedish) is a development-time annotation confirming the restoredifcondition and has no value in production code.🧹 Proposed cleanup
- if (rule.matches(path)) { // <-- DENNA RADEN SKA FINNAS + if (rule.matches(path)) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/filter/redirect/RedirectFilter.java` at line 35, Remove the development-time inline comment next to the if condition in RedirectFilter (the comment after "if (rule.matches(path))"); open the RedirectFilter class and delete the trailing "// <-- DENNA RADEN SKA FINNAS" artifact so the if statement reads cleanly (preserve existing spacing/formatting and keep the condition "if (rule.matches(path))" intact).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/main/java/org/example/filter/redirect/RedirectFilter.java`:
- Around line 3-6: The file RedirectFilter imports conflicting types
(org.example.httpparser.HttpRequest and org.example.filter.FilterChain) which
clash with other definitions (org.example.server.HttpRequest,
org.example.server.FilterChain, org.example.filter.FilterChainImpl); update
RedirectFilter to use the project’s canonical base interfaces (replace the
current imports with the agreed server-level interfaces
org.example.server.HttpRequest and org.example.server.FilterChain or the
canonical Filter interface used across the codebase) and add a short TODO
referencing Issue `#23` and `@eraiicphu` so future reconciliation is clear; ensure
all references inside class RedirectFilter compile against the chosen
HttpRequest and FilterChain symbols and remove any reliance on the duplicate
org.example.httpparser or filter-local types.
---
Nitpick comments:
In `@src/main/java/org/example/filter/redirect/RedirectFilter.java`:
- Line 35: Remove the development-time inline comment next to the if condition
in RedirectFilter (the comment after "if (rule.matches(path))"); open the
RedirectFilter class and delete the trailing "// <-- DENNA RADEN SKA FINNAS"
artifact so the if statement reads cleanly (preserve existing spacing/formatting
and keep the condition "if (rule.matches(path))" intact).
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/main/java/org/example/filter/redirect/RedirectFilter.javasrc/main/java/org/example/filter/redirect/RedirectRule.javasrc/main/java/org/example/filter/redirect/RedirectRulesLoader.javasrc/test/java/org/example/filter/redirect/RedirectFilterTest.java
🚧 Files skipped from review as they are similar to previous changes (3)
- src/main/java/org/example/filter/redirect/RedirectRulesLoader.java
- src/test/java/org/example/filter/redirect/RedirectFilterTest.java
- src/main/java/org/example/filter/redirect/RedirectRule.java
feat: add regex support and enforce 301/302 redirects
Ericthilen
force-pushed
the
URL-Redirect-Filter
branch
from
ebfe583 to
42f23cd
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/main/java/org/example/server/RedirectRule.java (1)
11-18:⚠️ Potential issue | 🟡 MinorReject blank
targetUrlto avoid emptyLocationheaders.
requireNonNullallows""/whitespace, which can produce a redirect with no destination.🛡️ Suggested guard
public RedirectRule(Pattern sourcePattern, String targetUrl, int statusCode) { this.sourcePattern = Objects.requireNonNull(sourcePattern, "sourcePattern"); - this.targetUrl = Objects.requireNonNull(targetUrl, "targetUrl"); + this.targetUrl = Objects.requireNonNull(targetUrl, "targetUrl"); + if (this.targetUrl.isBlank()) { + throw new IllegalArgumentException("targetUrl must not be blank"); + } if (statusCode != 301 && statusCode != 302) { throw new IllegalArgumentException("statusCode must be 301 or 302"); } this.statusCode = statusCode; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/server/RedirectRule.java` around lines 11 - 18, The constructor RedirectRule(Pattern sourcePattern, String targetUrl, int statusCode) currently only checks for null but allows empty or whitespace targetUrl which can produce empty Location headers; after Objects.requireNonNull(targetUrl, "targetUrl") add a validation that rejects blank values (e.g. if targetUrl.trim().isEmpty() throw new IllegalArgumentException("targetUrl must not be blank")) so the constructor enforces a non-null, non-blank targetUrl while preserving the existing statusCode check.
🧹 Nitpick comments (1)
src/main/java/org/example/server/FilterChain.java (1)
5-23: FilterChain statefulness is a design concern, but the current codebase doesn't manifest this issue.The
indexfield being mutable would indeed cause problems if FilterChain instances were reused across requests or shared concurrently. However, the current application code does not instantiate or use FilterChain at all—TcpServer only accepts and closes connections. Tests create fresh instances per test case. While the suggested refactor (making index immutable and creating new chain instances per filter step) is sound defensive design practice, there is no immediate bug to fix in the current codebase.If FilterChain is to be integrated into the actual HTTP request handling pipeline, ensure each request gets a fresh instance, or adopt the immutable pattern shown in the proposed diff to prevent future reuse-related bugs.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/server/FilterChain.java` around lines 5 - 23, FilterChain currently keeps mutable state in the field "index", which can cause reuse/concurrency bugs; change the design so doFilter does not mutate shared state: remove the "index" field and make doFilter accept an index parameter or create a new FilterChain instance with the next index when delegating; update the constructor and any callers to create fresh chains per request (refer to class FilterChain, field index, constructor FilterChain(HttpFilter[] filters, TerminalHandler terminal), and method doFilter(HttpRequest request, HttpResponse response)) so each filter invocation uses an immutable/indexed chain rather than mutating a single instance.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/main/java/org/example/server/RedirectRule.java`:
- Around line 11-18: The constructor RedirectRule(Pattern sourcePattern, String
targetUrl, int statusCode) currently only checks for null but allows empty or
whitespace targetUrl which can produce empty Location headers; after
Objects.requireNonNull(targetUrl, "targetUrl") add a validation that rejects
blank values (e.g. if targetUrl.trim().isEmpty() throw new
IllegalArgumentException("targetUrl must not be blank")) so the constructor
enforces a non-null, non-blank targetUrl while preserving the existing
statusCode check.
---
Nitpick comments:
In `@src/main/java/org/example/server/FilterChain.java`:
- Around line 5-23: FilterChain currently keeps mutable state in the field
"index", which can cause reuse/concurrency bugs; change the design so doFilter
does not mutate shared state: remove the "index" field and make doFilter accept
an index parameter or create a new FilterChain instance with the next index when
delegating; update the constructor and any callers to create fresh chains per
request (refer to class FilterChain, field index, constructor
FilterChain(HttpFilter[] filters, TerminalHandler terminal), and method
doFilter(HttpRequest request, HttpResponse response)) so each filter invocation
uses an immutable/indexed chain rather than mutating a single instance.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
src/main/java/org/example/server/FilterChain.javasrc/main/java/org/example/server/RedirectFilter.javasrc/main/java/org/example/server/RedirectRule.javasrc/main/java/org/example/server/RedirectRulesLoader.javasrc/main/java/org/example/server/TerminalHandler.javasrc/main/resources/.gitkeepsrc/test/java/org/example/server/RedirectFilterTest.javasrc/test/resources/.gitkeep
|
Created new pull request and added redirect and tests #95 |
7 participants
Implements a RedirectFilter that intercepts incoming requests and returns HTTP redirects based on a configurable list of redirect rules.
Requirements covered
Locationheader + status codeTests
How to test
Run RedirectFilterTest.
Issue #33
Co-authored by @viktorlindell12
Summary by CodeRabbit
New Features
Tests