bito-code-review[bot] commented on code in PR #41780:
URL: https://github.com/apache/superset/pull/41780#discussion_r3562698849
##########
tests/unit_tests/views/test_bootstrap_auth.py:
##########
@@ -156,71 +156,104 @@ def test_bootstrap_does_not_crash_without_recaptcha_key(
assert "RECAPTCHA_PUBLIC_KEY" not in payload["conf"]
-# --- language_pack injection --------------------------------------------
+# --- language pack delivery ----------------------------------------------
#
-# The Jed pack is injected by `common_bootstrap_payload` (outside the
-# memoized `cached_common_bootstrap_data`) using the shared
-# `superset.translations.utils.get_language_pack`. Tests here cover the
-# wrapper to confirm the pack lands on the payload for non-English
-# locales and is None for English.
+# The pack is NOT embedded in the bootstrap payload; spa.html loads it via a
+# content-addressed script tag whose URL comes from
+# `get_language_pack_template_context`. A pack supplied through
+# COMMON_BOOTSTRAP_OVERRIDES_FUNC still rides the payload and suppresses the
+# script tag.
-def test_common_bootstrap_payload_includes_language_pack_for_non_english(
+def test_common_bootstrap_payload_does_not_embed_language_pack(
app_context: None,
) -> None:
- """common.language_pack carries the shared utility's pack for non-en."""
- fake_pack = {"domain": "superset", "locale_data": {"superset": {}}}
+ """The payload stays small: no full pack even for non-English locales."""
with (
patch(
"superset.views.base.cached_common_bootstrap_data",
return_value={"locale": "fr"},
),
- patch(
- "superset.views.base.get_language_pack",
- return_value=fake_pack,
- ) as mock_get,
patch("superset.views.base.utils.get_user_id", return_value=1),
patch("superset.views.base.get_locale", return_value="fr"),
):
payload = common_bootstrap_payload()
- assert payload["language_pack"] == fake_pack
- mock_get.assert_called_once_with("fr")
+ assert payload["language_pack"] is None
-def test_common_bootstrap_payload_skips_pack_for_english(
+def test_common_bootstrap_payload_preserves_override_pack(
app_context: None,
) -> None:
- """English short-circuits: pack is None and the utility is not called."""
+ """A pack from COMMON_BOOTSTRAP_OVERRIDES_FUNC is passed through."""
+ fake_pack = {"domain": "superset", "locale_data": {"superset": {}}}
with (
patch(
"superset.views.base.cached_common_bootstrap_data",
- return_value={"locale": "en"},
+ return_value={"locale": "fr", "language_pack": fake_pack},
),
- patch("superset.views.base.get_language_pack") as mock_get,
patch("superset.views.base.utils.get_user_id", return_value=1),
- patch("superset.views.base.get_locale", return_value="en"),
+ patch("superset.views.base.get_locale", return_value="fr"),
):
payload = common_bootstrap_payload()
- assert payload["language_pack"] is None
- mock_get.assert_not_called()
+ assert payload["language_pack"] == fake_pack
def test_common_bootstrap_payload_does_not_mutate_memoized_dict(
app_context: None,
) -> None:
- """Injecting language_pack must not write back into the memoize cache."""
+ """Defaulting language_pack must not write back into the memoize cache."""
cached: dict[str, Any] = {"locale": "fr"}
with (
patch(
"superset.views.base.cached_common_bootstrap_data",
return_value=cached,
),
- patch("superset.views.base.get_language_pack", return_value={"x": 1}),
patch("superset.views.base.utils.get_user_id", return_value=1),
patch("superset.views.base.get_locale", return_value="fr"),
):
common_bootstrap_payload()
assert "language_pack" not in cached
+
+
+def _language_pack_context(locale: str, payload_extra: dict[str, Any]) -> Any:
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Weak return type annotation</b></div>
<div id="fix">
Replace `-> Any` with `-> dict[str, Any]` on `_language_pack_context`. All
three call sites at lines 240, 250, and 258 access
`context["language_pack_src"]` and `context["language_pack_inline"]` via
string-keyed dict indexing, and `get_language_pack_template_context`
(base.py:624) always returns a `dict[str, Any]`. The `Any` annotation erases
this contract and blocks static type checking.
</div>
<details>
<summary>
<b>Code suggestion</b>
</summary>
<blockquote>Check the AI-generated fix before applying</blockquote>
<div id="code">
````suggestion
def _language_pack_context(locale: str, payload_extra: dict[str, Any])
-> dict[str, Any]:
````
</div>
</details>
</div>
<small><i>Code Review Run #3b37bb</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
superset/translations/utils.py:
##########
@@ -24,9 +25,41 @@
# Global caching for JSON language packs
ALL_LANGUAGE_PACKS: dict[str, dict[str, Any]] = {"en": {}}
+# Global caching for language pack content hashes, used to build
+# content-addressed (cache-busting) asset URLs
+ALL_LANGUAGE_PACK_VERSIONS: dict[str, str] = {}
+
DIR = os.path.dirname(os.path.abspath(__file__))
+def get_language_pack_filename(locale: str) -> str:
+ """Resolve the on-disk JSON pack for a locale (empty pack for English)"""
+ if not locale or locale == "en":
+ # Forcing a dummy, quasi-empty language pack for English since the file
+ # in the en directory contains data with empty mappings
+ return DIR + "/empty_language_pack.json"
+ return DIR + f"/{locale}/LC_MESSAGES/messages.json"
+
+
+def get_language_pack_version(locale: str) -> Optional[str]:
+ """Get/cache a short content hash of the language pack file
+
+ The hash is embedded in the pack's asset URL so browsers can cache it
+ as immutable and pick up a fresh copy whenever translations change.
+ Returns None when the pack file cannot be read.
+ """
+ version = ALL_LANGUAGE_PACK_VERSIONS.get(locale)
+ if not version:
+ try:
+ with open(get_language_pack_filename(locale), "rb") as f:
+ version = hashlib.sha256(f.read()).hexdigest()[:12]
+ ALL_LANGUAGE_PACK_VERSIONS[locale] = version
+ except OSError:
+ logger.warning("No language pack file to version for locale %s",
locale)
+ return None
+ return version
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Runtime 404 due to missing files</b></div>
<div id="fix">
get_language_pack_version() reads the same non-existent `messages.json`
(line 54) and will return None for any non-English locale (line 59). The
language_pack_script endpoint in core.py uses this version for cache-control
decisions (line 966): when version is None, it returns 404. So the endpoint
will break for every locale.
</div>
</div>
<small><i>Code Review Run #3b37bb</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
superset/translations/utils.py:
##########
@@ -24,9 +25,41 @@
# Global caching for JSON language packs
ALL_LANGUAGE_PACKS: dict[str, dict[str, Any]] = {"en": {}}
+# Global caching for language pack content hashes, used to build
+# content-addressed (cache-busting) asset URLs
+ALL_LANGUAGE_PACK_VERSIONS: dict[str, str] = {}
+
DIR = os.path.dirname(os.path.abspath(__file__))
+def get_language_pack_filename(locale: str) -> str:
+ """Resolve the on-disk JSON pack for a locale (empty pack for English)"""
+ if not locale or locale == "en":
+ # Forcing a dummy, quasi-empty language pack for English since the file
+ # in the en directory contains data with empty mappings
+ return DIR + "/empty_language_pack.json"
+ return DIR + f"/{locale}/LC_MESSAGES/messages.json"
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Runtime error on missing files</b></div>
<div id="fix">
The function resolves to `{locale}/LC_MESSAGES/messages.json`, but no such
files exist in the repo — only .po files are shipped (e.g.
`fr/LC_MESSAGES/messages.po`). `get_language_pack()` on line 73 calls this and
will hit `OSError` at runtime. Either generate JSON packs from .po during the
build (using polib), or read the .po directly here.
</div>
</div>
<small><i>Code Review Run #3b37bb</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
