Conversation
Summary of ChangesHello @Lucius1274, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces crucial internationalization support for WebSocket consumers, specifically within the selective transfer feature. By dynamically detecting and activating the user's browser language based on the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
📝 WalkthroughWalkthroughAdds per-user language activation to the SelectiveTransferConsumer by parsing the HTTP Accept-Language header on connect, storing the resolved language, and re-applying that language via translation override during form rendering, error handling, and response sending. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client (browser)
participant Consumer as SelectiveTransferConsumer
participant DjangoI18n as django.utils.translation
participant Renderer as Template/Form Renderer
Client->>Consumer: WebSocket connect (includes Accept-Language header)
Consumer->>DjangoI18n: _activate_user_language(header) -> get_supported_language_variant
DjangoI18n-->>Consumer: resolved user_language
Consumer-->>Consumer: store user_language on instance
Consumer->>Renderer: request to render form/response
Consumer->>DjangoI18n: override(user_language) context
DjangoI18n-->>Renderer: rendering in user_language
Renderer-->>Consumer: rendered payload
Consumer-->>Client: send rendered 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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request correctly activates user-specific language settings in the websocket consumer to address date formatting issues, using the Accept-Language header. However, it introduces a Path Traversal vulnerability due to unvalidated language codes being passed to Django's translation system. Furthermore, the Accept-Language header parsing needs to be more robust to handle quality factors, and the direct use of django.utils.translation.activate in thread pools is not thread-safe, potentially causing language setting leaks. It is recommended to validate language codes against supported variants and use django.utils.translation.override for thread-safe language activation.
adit/selective_transfer/consumers.py
Outdated
| def _activate_user_language(self) -> str: | ||
| headers = dict(self.scope.get("headers", [])) | ||
| accept_language = headers.get(b"accept-language", b"").decode("utf-8") | ||
|
|
||
| # Parse Accept-Language header to get preferred language | ||
| preferred_lang = None | ||
| if accept_language: | ||
| langs = [lang.split(";")[0].strip().lower() for lang in accept_language.split(",")] | ||
| # Default to first language | ||
| preferred_lang = langs[0].split("-")[0] if "-" in langs[0] else langs[0] | ||
|
|
||
| if preferred_lang: | ||
| activate(preferred_lang) | ||
| return preferred_lang | ||
| else: | ||
| return get_language() |
There was a problem hiding this comment.
The _activate_user_language method is vulnerable to a Path Traversal attack. It extracts a language code from the Accept-Language header and passes it directly to django.utils.translation.activate() without validation. This allows an attacker to potentially force the application to load translation files from arbitrary file system locations. To mitigate this, the extracted language code must be validated against supported languages using django.utils.translation.get_supported_language_variant before activation. Additionally, while the provided suggestion addresses the security vulnerability, the parsing of the Accept-Language header could be made more robust to correctly handle quality factors (e.g., en-US,en;q=0.9), which is not fully covered by the current implementation and could lead to incorrect language selection.
def _activate_user_language(self) -> str:
headers = dict(self.scope.get("headers", []))
accept_language = headers.get(b"accept-language", b"").decode("utf-8")
# Parse Accept-Language header to get preferred language
preferred_lang = None
if accept_language:
langs = [lang.split(";")[0].strip().lower() for lang in accept_language.split(",")]
# Default to first language
preferred_lang = langs[0].split("-")[0] if "-" in langs[0] else langs[0]
if preferred_lang:
from django.utils.translation import get_supported_language_variant
try:
supported_lang = get_supported_language_variant(preferred_lang)
activate(supported_lang)
return supported_lang
except LookupError:
pass
return get_language()
adit/selective_transfer/consumers.py
Outdated
| if hasattr(self, "user_language"): | ||
| activate(self.user_language) |
There was a problem hiding this comment.
Using activate() modifies the language for the current thread. Since this code runs in a thread from a thread pool (database_sync_to_async), and threads are reused, this can lead to subtle bugs where a subsequent operation on the same thread unexpectedly runs with the wrong language setting.
It's safer to use django.utils.translation.override as a context manager. It ensures the language is changed only within its scope and is automatically restored. You could wrap the function's body in a with statement:
from django.utils.translation import override
with override(getattr(self, "user_language", None)):
rendered_form: str = render_crispy_form(form)
rendered_error_message: str = render_error_message(message)
return rendered_form + rendered_error_message
adit/selective_transfer/consumers.py
Outdated
| if hasattr(self, "user_language"): | ||
| activate(self.user_language) |
There was a problem hiding this comment.
Using activate() here can have side effects in a threaded environment because threads from the pool are reused. To avoid language settings leaking between different operations, it's better to scope the language change.
Please use django.utils.translation.override as a context manager, which handles restoring the original language automatically.
from django.utils.translation import override
with override(getattr(self, "user_language", None)):
# ... rest of the function body
adit/selective_transfer/consumers.py
Outdated
| if hasattr(self, "user_language"): | ||
| activate(self.user_language) |
There was a problem hiding this comment.
As with the other rendering methods, using activate() here is not thread-safe without restoring the original language. To prevent potential issues with thread reuse, please use the django.utils.translation.override context manager.
from django.utils.translation import override
with override(getattr(self, "user_language", None)):
# ... rest of the function body
Code Review: Activated user-specific browser languageThank you for this fix to address the date format localization issue in selective transfer! ✅ Strengths
🔍 Code Quality Issues1. Parsing Logic Could Be More Robust (lines 74-89)The current Accept-Language parsing is simplistic and doesn't handle quality values (q-values). The header Accept-Language: en-US,en;q=0.9,de;q=0.8 should prefer en-US over de, but current code just takes first lang. Suggestion: Use Django's built-in parse_accept_lang_header from django.utils.translation which respects quality values and matches exactly how Django's LocaleMiddleware works. 2. Defensive Checks Unnecessary (lines 184, 293, 344)Since user_language is set in connect() which always runs before these methods, the hasattr check is redundant. Either remove the check (cleaner) or add a comment explaining why it's needed. 🐛 Potential Issues3. Language Code Validation MissingThe code doesn't validate if the extracted language code is actually supported by Django. If a user sends Accept-Language: xx-ZZ (unsupported language), activate may silently fail or fall back to default. 4. Inconsistent Re-activationYou're re-activating in three rendering methods but not in check_permission() (line 173) which renders error messages. Worth verifying the execution order. 🧪 Testing ConcernsCritical: No tests were added for this functionality. You should add:
🎯 Security & Performance
📝 Recommendations SummaryMust fix:
Should fix: Nice to have: Overall: The fix addresses the reported issue correctly, but needs better parsing logic and test coverage before merging. Happy to review again after updates! Generated by Claude Code |
![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 `@adit/selective_transfer/consumers.py`:
- Around line 183-186: The method _build_form_error_response currently calls
activate(self.user_language) which mutates thread-local state; instead import
override from django.utils.translation and wrap the language activation in a
context manager so the language is restored after rendering (use
override(self.user_language) when self.user_language is set or
contextlib.nullcontext() when not). Apply the same pattern to the other two
methods that call activate (referenced in the diff at the other call sites) so
each of _build_form_error_response, and the other two methods use a with
override(...) / nullcontext(): block around render_crispy_form(form) to prevent
ThreadPoolExecutor worker threads from leaking language state.
- Around line 74-90: The _activate_user_language method currently drops region
subtags and ignores q-values; update it to parse the Accept-Language header with
q-value weights, sort candidates by q descending, and for each candidate use
django.utils.translation.get_supported_language_variant() (checking against
settings.LANGUAGES) to pick the best matching language while preserving region
variants (e.g., en-GB); then activate that language but ensure restoration to
avoid leakage by using django.utils.translation.override(...) as a context
manager around the consumer work that relies on the language or, if not
possible, capture the previous language with
django.utils.translation.get_language(), call activate(...) and always restore
it in a finally block.
| def _activate_user_language(self) -> str: | ||
| headers = dict(self.scope.get("headers", [])) | ||
| accept_language = headers.get(b"accept-language", b"").decode("utf-8") | ||
|
|
||
| # Parse Accept-Language header to get preferred language | ||
| preferred_lang = None | ||
| if accept_language: | ||
| langs = [lang.split(";")[0].strip().lower() for lang in accept_language.split(",")] | ||
| # Default to first language | ||
| preferred_lang = langs[0].split("-")[0] if "-" in langs[0] else langs[0] | ||
|
|
||
| if preferred_lang: | ||
| activate(preferred_lang) | ||
| return preferred_lang | ||
| else: | ||
| return get_language() | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's examine the consumers.py file to understand the full context
cd /repo && wc -l adit/selective_transfer/consumers.pyRepository: openradx/adit
Length of output: 112
🏁 Script executed:
# Read the relevant sections of the consumers.py file
cd /repo && sed -n '1,120p' adit/selective_transfer/consumers.py | cat -nRepository: openradx/adit
Length of output: 112
🏁 Script executed:
# Check if there are any threadpool or async patterns in this consumer
cd /repo && grep -n "thread\|async\|sync_to_async\|ThreadPool" adit/selective_transfer/consumers.py | head -20Repository: openradx/adit
Length of output: 112
🏁 Script executed:
# Check Django version and available utilities
cd /repo && grep -r "get_supported_language_variant\|django.utils.translation" adit/selective_transfer/ --include="*.py"Repository: openradx/adit
Length of output: 112
🏁 Script executed:
# Look for how activate() is currently used in the codebase
cd /repo && grep -n "activate(" adit/selective_transfer/consumers.pyRepository: openradx/adit
Length of output: 112
🏁 Script executed:
# Check if override() is already used elsewhere in the codebase for language handling
cd /repo && grep -r "override(" . --include="*.py" | grep -i language | head -10Repository: openradx/adit
Length of output: 112
🌐 Web query:
Django get_supported_language_variant function availability and Accept-Language header parsing with q-values best practices
💡 Result:
get_supported_language_variant() availability (Django)
- Public API:
django.utils.translation.get_supported_language_variant(lang_code, strict=False). - Introduced in Django 2.1 (“New in Django 2.1”). [1]
- Current behavior (Django 4.2/5.x): returns the best supported match from
settings.LANGUAGES, including fallback to generic (es-ar→es) and (whenstrict=False) sometimes to a more specific variant if that’s all you support. [2] - Security-related length limit: language code parsing is capped (500 chars); docs note a behavior change in Django 4.2.15. [2] (This was released as part of security/bugfix updates; see also the 5.0.7 release notes for the DoS mitigation context.) [3]
Accept-Language parsing with q-values: best practices in Django
Prefer Django’s built-ins (don’t parse manually)
- Use
LocaleMiddleware(normal Django i18n stack) or calltranslation.get_language_from_request(request)directly. It:- Negotiates against
settings.LANGUAGESonly. [4] - Parses
Accept-Languageinto (language, q) pairs ordered by descendingq. [5] - Treats
*as a wildcard and stops considering less-specific matches once encountered. [5]
- Negotiates against
Normalize language tags the “Django way”
- Django’s Accept-Language parser lowercases the header value before processing. So ensure your supported language codes in
LANGUAGESare in Django’s typical lowercase form (e.g.,pt-br,en-us). [5]
Don’t treat Accept-Language as the user’s actual preference
Accept-Languageindicates what the client accepts, often shaped by browser defaults, device UI language, travel, and privacy protections—not necessarily the user’s explicit site preference. (So store an explicit user choice via cookie/session/profile when available, and useAccept-Languageas a fallback.) [6]
Avoid using Django’s private parser directly (unless you accept the risk)
django.utils.translation.trans_real.parse_accept_lang_header()is not a documented public API; Django core discussions explicitly flag it as “undocumented, private”. [7]- If you truly need a standalone parser (outside Django’s negotiation logic), prefer a dedicated library rather than reaching into Django internals (e.g.
acceptlang, which is derived from Django’s logic). [8]
Sources:
[1] Django 2.1 utils docs (get_supported_language_variant “New in Django 2.1”) (docs.djangoproject.com)
[2] Django 5.1 utils docs (current behavior + “Changed in Django 4.2.15”) (docs.djangoproject.com)
[3] Django 5.0.7 release notes (DoS mitigation context for get_supported_language_variant) (fossies.org)
[4] Django utils docs (get_language_from_request behavior) (docs.djangoproject.com)
[5] Django trans_real.py (lowercasing, q-value sorting, length truncation) (fossies.org)
[6] MDN Accept-Language reference (q-values, wildcard, privacy notes) (developer.mozilla.org)
[7] Django ticket discussion noting parse_accept_lang_header() is private/undocumented (groups.google.com)
[8] acceptlang PyPI (standalone parser based on Django logic) (pypi.org)
Citations:
- 1: https://docs.djangoproject.com/en/2.1/ref/utils/?utm_source=openai
- 2: https://docs.djangoproject.com/en/5.1/ref/utils/?utm_source=openai
- 3: https://fossies.org/linux/django/docs/releases/5.0.7.txt?utm_source=openai
- 4: https://docs.djangoproject.com/en/4.2/ref/utils/?utm_source=openai
- 5: https://fossies.org/linux/django/django/utils/translation/trans_real.py?utm_source=openai
- 6: https://developer.mozilla.org/en-US/docs/Web/HTTP/Reference/Headers/Accept-Language?utm_source=openai
- 7: https://groups.google.com/g/django-updates/c/H2aDoV6quyM?utm_source=openai
- 8: https://pypi.org/project/acceptlang/?utm_source=openai
🌐 Web query:
Django Channels WebSocket consumer language activation threadpool async_to_sync language leakage
💡 Result:
In Django/Channels, language selection is effectively “context-local”, and it can leak in WebSocket consumers when you activate a language but don’t reliably restore it—especially when your code runs in reused threads (threadpools).
Why leakage happens
django.utils.translation.activate()is documented as per-thread, and Django explicitly warns you to switch back (or usetranslation.override()) because the change affects other code running in the same thread. [1]- Channels runs SyncConsumers in a threadpool (so threads get reused across connections/events). Channels even recommends SyncConsumers when you touch the ORM/sync Django APIs—i.e., the common case—meaning thread reuse is normal. [2]
asgiref’s sync/async adapters preserve threadlocals/contextvars across the boundary andsync_to_async(thread_sensitive=True)(the default) runs calls in a shared, thread-sensitive executor (a single-thread executor by default), increasing the chance that “thread-local” state (like active language) is observed by later work on the same thread if you didn’t restore it. [3][4]
What to do (practical fixes)
- Never leave a consumer/method with an activated language you set. Wrap language changes:
- Prefer
with translation.override(lang): ...so it restores automatically. [1]
- Prefer
- If you activate manually, restore in
finally:(or calltranslation.deactivate()when appropriate), matching Django’s guidance. [1] - If your leakage is happening inside sync work called from async (or vice-versa), be aware that
thread_sensitive=Truetends to reuse the same thread; isolating execution contexts can help:- Use
ThreadSensitiveContext()to scope a separate thread-sensitive executor for a logical unit of work (e.g., per-connection / per-message handling), rather than sharing the process-wide one. [4] - Alternatively, for non-Django-sensitive sync code,
sync_to_async(..., thread_sensitive=False)forces it off the shared thread-sensitive lane (but don’t do this for ORM access unless you know what you’re doing). [3]
- Use
If you paste a minimal consumer snippet showing where you call translation.activate() (and whether you’re using SyncConsumer, AsyncConsumer, database_sync_to_async, async_to_sync, etc.), I can point out the exact leak path and the smallest safe change.
Sources: Django i18n/translation docs (thread-based activation + override) [1]; Channels consumer guidance (SyncConsumer runs in a thread) [2]; Django async docs on async_to_sync / sync_to_async and preserved threadlocals/contextvars + thread_sensitive behavior [3]; asgiref.sync source for thread-sensitive executor/ThreadSensitiveContext [4].
[1] (docs.djangoproject.com)
[2] (dokk.org)
[3] (docs.djangoproject.com)
[4] (docs.djangoproject.com)
Citations:
- 1: https://docs.djangoproject.com/en/3.1/topics/i18n/translation/?utm_source=openai
- 2: https://dokk.org/documentation/django-channels/3.0.3/topics/consumers/?utm_source=openai
- 3: https://docs.djangoproject.com/en/dev/topics/async/?utm_source=openai
- 4: https://docs.djangoproject.com/en/5.0/_modules/asgiref/sync/?utm_source=openai
🌐 Web query:
Django activate() vs override() context manager thread-local language storage scope
💡 Result:
-
translation.activate(lang)sets Django’s current language for the current execution context and leaves it set until something else changes it (or you calldeactivate()/deactivate_all()). Django documents this as “for the current thread”. In practice, it’s stored in Django’s “active translations” holder. [1][2] -
translation.override(lang, deactivate=False)is just a safer scoping wrapper aroundactivate():- on enter: remembers the previous language and activates
lang(or activatesNullTranslationsiflang is None) - on exit: restores the previous language (or optionally calls
deactivate()ifdeactivate=True)
This is exactly why it’s preferred for temporary switches (e.g., rendering an email in a specific locale). [1][2]
- on enter: remembers the previous language and activates
-
Where the language is stored (scope):
- Django’s translation internals keep the “active” translation in
_active, which is anasgiref.local.Local()instance (not a plainthreading.local). [3] asgiref.local.Localis designed to behave like thread-local in sync code and task-/context-local in async code, and it also proxies values acrosssync_to_async/async_to_syncboundaries (unless you opt into stricter behavior). [4]
So the effective scope is “current thread / current async task context”, not a single global value.
- Django’s translation internals keep the “active” translation in
Rule of thumb: use activate() when you’re intentionally setting the language for the rest of the current request/work unit; use override() when you need a temporary, exception-safe language switch with automatic restoration. [1][2]
Sources: Django utilities translation docs for activate/override and per-thread behavior [1][2]; Django translation internals showing _active = Local() [3]; asgiref docs describing Local semantics across threads/async contexts [4].
[1] https://docs.djangoproject.com/en/5.2/ref/utils/ (translation override, activate)
[2] https://docs.djangoproject.com/en/4.2/topics/i18n/translation/ (per-thread activation notes)
[3] https://fossies.org/linux/django/django/utils/translation/trans_real.py (_active = Local())
[4] https://pypi.org/project/asgiref/ (“Threadlocal replacement” / Local behavior)
🌐 Web query:
Django get_language_from_request WebSocket Channels consumer ASGI alternative
💡 Result:
django.utils.translation.get_language_from_request() wants a Django HttpRequest, but a Channels WebSocket consumer gets an ASGI scope (self.scope) instead, where headers/cookies/session live. In practice you do “language negotiation” yourself from scope, then translation.activate(...). Channels explicitly documents self.scope["headers"] for HTTP/WebSocket and that middleware can add more attributes to scope. [1]
Minimal “ASGI/Channels alternative” (no HttpRequest)
1) Ensure cookies/session are available in scope
Wrap your websocket app with SessionMiddlewareStack (or AuthMiddlewareStack, which includes sessions). Channels documents that SessionMiddlewareStack populates scope["session"] for WebSockets. [2]
2) Pick a language from (session/cookie/Accept-Language), then activate
Django’s locale selection order is: session (when enabled) → cookie (LANGUAGE_COOKIE_NAME, default django_language) → Accept-Language → fallback LANGUAGE_CODE. [3]
Example (Async consumer):
from channels.generic.websocket import AsyncWebsocketConsumer
from django.conf import settings
from django.utils import translation
from django.utils.translation import LANGUAGE_SESSION_KEY
def _get_header(scope, name: bytes) -> str | None:
for k, v in scope.get("headers", []):
if k.lower() == name:
return v.decode("latin-1")
return None
def _parse_cookie(header: str | None) -> dict[str, str]:
if not header:
return {}
out = {}
for part in header.split(";"):
if "=" in part:
k, v = part.split("=", 1)
out[k.strip()] = v.strip()
return out
class MyConsumer(AsyncWebsocketConsumer):
async def connect(self):
# 1) session (if you use SessionMiddlewareStack/AuthMiddlewareStack)
lang = None
session = self.scope.get("session")
if session:
lang = session.get(LANGUAGE_SESSION_KEY)
# 2) cookie (django_language by default)
if not lang:
cookies = _parse_cookie(_get_header(self.scope, b"cookie"))
lang = cookies.get(getattr(settings, "LANGUAGE_COOKIE_NAME", "django_language"))
# 3) Accept-Language (basic: take first tag; you can implement q= weights if you want)
if not lang:
accept = _get_header(self.scope, b"accept-language")
if accept:
lang = accept.split(",")[0].strip()
# 4) fallback
if not lang:
lang = settings.LANGUAGE_CODE
translation.activate(lang)
self.scope["LANGUAGE_CODE"] = lang # optional: make it available like request.LANGUAGE_CODE
await self.accept()Notes:
- WebSocket scopes include raw headers like
accept-language. [4] - If you change language inside a WebSocket and want it persisted, Channels notes WebSockets won’t auto-save sessions; you must save explicitly (
scope["session"].save()/asave()). [2]
If you really want to keep calling get_language_from_request()
Create a tiny adapter that builds a minimal HttpRequest-like object (META, COOKIES, optional session) from scope and pass it to get_language_from_request(). This is more work than just negotiating from scope, but it’s doable.
Sources: Channels “scope” in consumers [1], Channels sessions middleware / scope["session"] [2], Django LocaleMiddleware language selection order (cookie / Accept-Language / fallback) [3], example of accept-language present in WebSocket scope["headers"] [4].
[1] (channels.readthedocs.io)
[2] (channels.readthedocs.io)
[3] (docs.djangoproject.com)
[4] (coffeebytes.dev)
Citations:
- 1: https://channels.readthedocs.io/en/stable/topics/consumers.html?utm_source=openai
- 2: https://channels.readthedocs.io/en/stable/topics/sessions.html?utm_source=openai
- 3: https://docs.djangoproject.com/en/4.2/topics/i18n/translation/?utm_source=openai
- 4: https://coffeebytes.dev/en/django/django-channels-consumers-environments-and-events/?utm_source=openai
Parse Accept-Language header according to q-values and preserve language region variants.
Current parsing strips region subtags (e.g., en-GB → en) and ignores quality weights, which can still produce incorrect date formatting and defeats the header's negotiation semantics. Use get_supported_language_variant() with q-value sorting to normalize against settings.LANGUAGES while respecting browser preference weights.
Additionally, calling activate() without restoration in a Channels consumer risks language leakage across WebSocket messages and connections because Channels runs SyncConsumers in thread-reused threadpools and activate() is thread-local. Either:
- Wrap the language change in
translation.override()context manager (auto-restores), or - Restore the previous language in a
finallyblock after callingactivate().
🤖 Prompt for AI Agents
In `@adit/selective_transfer/consumers.py` around lines 74 - 90, The
_activate_user_language method currently drops region subtags and ignores
q-values; update it to parse the Accept-Language header with q-value weights,
sort candidates by q descending, and for each candidate use
django.utils.translation.get_supported_language_variant() (checking against
settings.LANGUAGES) to pick the best matching language while preserving region
variants (e.g., en-GB); then activate that language but ensure restoration to
avoid leakage by using django.utils.translation.override(...) as a context
manager around the consumer work that relies on the language or, if not
possible, capture the previous language with
django.utils.translation.get_language(), call activate(...) and always restore
it in a finally block.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request correctly implements user-specific language activation for websockets by reading the Accept-Language header. The language is then activated before rendering templates, which fixes issues with date formatting. My feedback focuses on improving maintainability by reducing code duplication. The core logic of the feature is sound.
adit/selective_transfer/consumers.py
Outdated
| if hasattr(self, "user_language"): | ||
| activate(self.user_language) |
There was a problem hiding this comment.
This language activation logic is repeated in send_query_response and build_transfer_response. This creates code duplication which can make future maintenance harder.
To improve this, you could use Django's built-in override context manager from django.utils.translation. It's designed for temporarily changing the active language.
Here's how you could refactor this method (and the others):
from django.utils.translation import override
...
@database_sync_to_async
def _build_form_error_response(self, form: SelectiveTransferJobForm, message: str) -> str:
with override(getattr(self, "user_language", None)):
rendered_form: str = render_crispy_form(form)
rendered_error_message: str = render_error_message(message)
return rendered_form + rendered_error_messageApplying this pattern to all three locations would make the code cleaner and more maintainable.
Code ReviewThank you for this contribution! The PR addresses a legitimate issue with date formatting in WebSocket contexts. Here is my detailed feedback: Strengths
Issues and Concerns1. Thread Safety Risk (CRITICAL)Lines 74-89, 184-185, 293-294, 344-345 The current implementation has a critical thread safety issue. Django activate() function modifies thread-local state, but this code uses ThreadPoolExecutor (line 70) to run methods like send_query_response() in worker threads. Problem: When activate(self.user_language) is called in send_query_response() (line 294), it sets the language for that worker thread, but the thread may be reused for other users requests, causing language bleed between users. Recommended Fix: Use Django translation.override() context manager instead. This ensures the language is scoped to the specific operation and automatically restored afterward. Apply the same pattern to _build_form_error_response() and build_transfer_response(). 2. Fragile hasattr() ChecksLines 184, 293, 344 The defensive hasattr() checks are unnecessary since user_language is always set in connect() (line 67). These checks make the code harder to reason about. Recommendation: Remove the hasattr() checks and add a type annotation at class level instead. 3. Language Parsing OversimplificationLines 74-89 The Accept-Language parsing is overly simplistic and ignores quality values (q-factors) that indicate preference strength. Issues:
Recommended Fix: Use Django built-in get_supported_language_variant() which leverages RFC 2616-compliant parsing. 4. Missing Test CoverageIssue: No tests added for this functionality. Given that this fixes a user-facing bug affecting a critical user flow, tests are essential. Recommendation: Add tests to verify:
5. Code Style ComplianceLines 81-83 Google Python Style Guide prefers clarity over brevity. Break down the complex list comprehension into multiple steps for readability. Summary
Actionable Next Steps
Please let me know if you have questions about any of these recommendations! |
|
Code Review: This PR adds language detection from browser Accept-Language headers to fix date formatting. Strengths: Good test coverage, clean implementation. Main issue: Language parsing ignores q-values (quality parameters). If user sends fr;q=0.5,en;q=0.9,de the code selects fr even though de has highest priority. Recommendation: Use django.utils.translation.get_language_from_request() for proper q-value handling. Also consider adding integration tests for actual date formatting and type hints for user_language attribute. Overall: Approve with minor changes required. |
|
Detailed Review Points:
|
|
Suggested Fix for Language Priority: Instead of manually parsing Accept-Language, consider using Django's built-in function: from django.utils.translation import get_language_from_request
def _activate_user_language(self) -> str:
class FakeRequest:
def __init__(self, headers_dict):
accept_lang = headers_dict.get(b'accept-language', b'').decode('utf-8')
self.META = {'HTTP_ACCEPT_LANGUAGE': accept_lang}
headers = dict(self.scope.get('headers', []))
fake_req = FakeRequest(headers)
return get_language_from_request(fake_req, check_path=False)This properly handles q-values, language variants, and fallback to LANGUAGE_CODE automatically. Security Note: No security concerns identified. Input sanitization is proper, Django's translation system handles exceptions well, and language selection is limited to supported variants. |
2 participants
Custom activation of the user-specific browser locale in the websocket. Fixes wrong date format in selective transfer
Summary by CodeRabbit