Feature/LocaleFilter - #81
Merged
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review infoConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds a new LocaleFilter that derives a request-scoped locale from the Accept-Language header (defaults to "en-US"), stores it in a ThreadLocal for the request, exposes a static accessor, and includes unit tests for header parsing and null/blank/case scenarios. Changes
Sequence DiagramsequenceDiagram
participant Client
participant LocaleFilter
participant ThreadLocal
participant FilterChain
participant Backend
Client->>LocaleFilter: doFilter(request, response, chain)
activate LocaleFilter
LocaleFilter->>LocaleFilter: resolveLocale(request)
alt Accept-Language header present
LocaleFilter->>ThreadLocal: set(parsed_locale)
else missing/blank/malformed
LocaleFilter->>ThreadLocal: set("en-US")
end
LocaleFilter->>FilterChain: chain.doFilter(request, response)
activate FilterChain
FilterChain->>Backend: process request
Backend-->>FilterChain: response
deactivate FilterChain
LocaleFilter->>ThreadLocal: remove()
LocaleFilter-->>Client: return response
deactivate LocaleFilter
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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
🧹 Nitpick comments (2)
src/main/java/org/example/filter/LocaleFilter.java (1)
62-64: Redundantnullcheck onheaders.
HttpRequest.getHeaders()always returns a non-null map (Collections.emptyMap()when the constructor receivednull), soheaders == nullcan never be true. Theheaders.isEmpty()guard is sufficient.♻️ Proposed simplification
- if (headers == null || headers.isEmpty()) { + if (headers.isEmpty()) { return DEFAULT_LOCALE; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/filter/LocaleFilter.java` around lines 62 - 64, The null check on headers in LocaleFilter is redundant because HttpRequest.getHeaders() always returns a non-null Map; update the guard in the method that uses headers (look for the headers variable in LocaleFilter, likely in a getLocale or doFilter method) to remove the `headers == null` check and only check `headers.isEmpty()` before returning DEFAULT_LOCALE, keeping the rest of the logic unchanged.src/test/java/org/example/filter/LocaleFilterTest.java (1)
14-29: Missing test for a first-entry quality weight — would catch theparts[0].trim()bug.There is no test where the first locale tag carries a
;q=suffix (e.g.,"sv-SE;q=0.9,en-US"). Adding one would directly exercise — and currently expose — the stripping bug flagged inLocaleFilter.resolveLocale.✅ Suggested additional test
`@Test` void shouldStripQualityWeightFromFirstLocale() { Map<String, String> headers = new HashMap<>(); headers.put("Accept-Language", "sv-SE;q=0.9,en-US"); HttpRequest request = new HttpRequest("GET", "/", "HTTP/1.1", headers, null); HttpResponseBuilder response = new HttpResponseBuilder(); LocaleFilter filter = new LocaleFilter(); filter.doFilter(request, response, (req, res) -> { assertEquals("sv-SE", LocaleFilter.getCurrentLocale()); }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/org/example/filter/LocaleFilterTest.java` around lines 14 - 29, Add a test that verifies LocaleFilter correctly strips a leading quality weight from the first Accept-Language entry: create a test named shouldStripQualityWeightFromFirstLocale that builds headers with "Accept-Language" = "sv-SE;q=0.9,en-US", constructs HttpRequest and HttpResponseBuilder, instantiates LocaleFilter, calls filter.doFilter(request, response, (req, res) -> { assertEquals("sv-SE", LocaleFilter.getCurrentLocale()); }), and ensure the assertion checks the locale inside the filter invocation to catch the parts[0].trim() bug in LocaleFilter.resolveLocale.
🤖 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/LocaleFilter.java`:
- Around line 80-85: In LocaleFilter (method handling the acceptLanguage string)
remove the dead check for parts.length == 0 and instead parse the first token to
strip any quality weight; after splitting acceptLanguage by ',' take parts[0],
trim it, then split that token on ';' (or strip everything after the first ';' /
match up to the first ';' with a regex) and return the cleaned locale tag; keep
returning DEFAULT_LOCALE when the acceptLanguage is blank/null or the cleaned
token is empty.
In `@src/test/java/org/example/filter/LocaleFilterTest.java`:
- Around line 85-95: The test name is incorrect because HttpRequest normalizes a
null headers argument to an empty map, so the test exercises the empty-headers
path rather than a null-headers guard; update the test method name in
LocaleFilterTest from shouldUseDefaultWhenHeadersAreNull to
shouldUseDefaultWhenHeadersAreEmpty and keep the body as-is (it constructs new
HttpRequest(..., null, null) and asserts LocaleFilter.getCurrentLocale() inside
the filter.doFilter callback) so the name accurately describes the scenario
being tested.
---
Nitpick comments:
In `@src/main/java/org/example/filter/LocaleFilter.java`:
- Around line 62-64: The null check on headers in LocaleFilter is redundant
because HttpRequest.getHeaders() always returns a non-null Map; update the guard
in the method that uses headers (look for the headers variable in LocaleFilter,
likely in a getLocale or doFilter method) to remove the `headers == null` check
and only check `headers.isEmpty()` before returning DEFAULT_LOCALE, keeping the
rest of the logic unchanged.
In `@src/test/java/org/example/filter/LocaleFilterTest.java`:
- Around line 14-29: Add a test that verifies LocaleFilter correctly strips a
leading quality weight from the first Accept-Language entry: create a test named
shouldStripQualityWeightFromFirstLocale that builds headers with
"Accept-Language" = "sv-SE;q=0.9,en-US", constructs HttpRequest and
HttpResponseBuilder, instantiates LocaleFilter, calls filter.doFilter(request,
response, (req, res) -> { assertEquals("sv-SE",
LocaleFilter.getCurrentLocale()); }), and ensure the assertion checks the locale
inside the filter invocation to catch the parts[0].trim() bug in
LocaleFilter.resolveLocale.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/main/java/org/example/filter/LocaleFilter.javasrc/test/java/org/example/filter/LocaleFilterTest.java
AntonAhlqvist
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/main/java/org/example/filter/LocaleFilter.java (1)
72-80: Consider simplifying the case-insensitive header lookup.
HttpRequest.getHeaders()returns a plainMap(fromMap.copyOf), so the manual iteration is functionally necessary for case-insensitive matching. However, you could tighten this with a stream one-liner or, better, push a case-insensitivegetHeader(String name)method ontoHttpRequestitself so every filter/handler doesn't have to re-implement this pattern.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/filter/LocaleFilter.java` around lines 72 - 80, The manual loop in LocaleFilter that searches headers for "Accept-Language" should be replaced with a case-insensitive lookup: either add a helper method HttpRequest.getHeader(String name) that performs case-insensitive matching over HttpRequest.getHeaders() and returns the first matching value, then call that from LocaleFilter to set acceptLanguage, or replace the loop in LocaleFilter with a concise stream-based one-liner that finds an entry whose key equalsIgnoreCase("Accept-Language") and maps to its value; update references to acceptLanguage and the LocaleFilter header lookup to use the new helper or stream one-liner so the code is clearer and reusable.
🤖 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/LocaleFilter.java`:
- Around line 86-92: The current parsing in LocaleFilter (using acceptLanguage,
parts and parts[0].split(";")[0].trim()) can yield an empty string for malformed
headers; after extracting and trimming the language token from parts[0], add a
final emptiness guard that returns DEFAULT_LOCALE when the token is empty (i.e.,
check the result of parts[0].split(";")[0].trim() and return DEFAULT_LOCALE if
it is blank) so the method never returns an empty locale string.
---
Nitpick comments:
In `@src/main/java/org/example/filter/LocaleFilter.java`:
- Around line 72-80: The manual loop in LocaleFilter that searches headers for
"Accept-Language" should be replaced with a case-insensitive lookup: either add
a helper method HttpRequest.getHeader(String name) that performs
case-insensitive matching over HttpRequest.getHeaders() and returns the first
matching value, then call that from LocaleFilter to set acceptLanguage, or
replace the loop in LocaleFilter with a concise stream-based one-liner that
finds an entry whose key equalsIgnoreCase("Accept-Language") and maps to its
value; update references to acceptLanguage and the LocaleFilter header lookup to
use the new helper or stream one-liner so the code is clearer and reusable.
Boppler12
approved these changes
Feb 24, 2026
Boppler12
pushed a commit
that referenced
this pull request
Feb 25, 2026
* Re-commit LocaleFilter + tests to clean branch for PR * Update LocaleFilter to handle quality weights and improve javadoc * Fix: rename test method to reflect actual headers scenario * Fix: ensure resolveLocale never returns empty string; strip quality weights
This was referenced Feb 27, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Tests