Copilot commented on code in PR #39357:
URL: https://github.com/apache/superset/pull/39357#discussion_r3190502185
##########
tests/unit_tests/views/test_bootstrap_auth.py:
##########
@@ -133,3 +133,78 @@ def test_recaptcha_shown_for_non_federated_auth(
payload = _get_bootstrap()
assert payload["conf"]["RECAPTCHA_PUBLIC_KEY"] == "test-key"
+
+
+# --- _load_language_pack -------------------------------------------------
+
+
[email protected]("language", [None, "", "en"])
+def test_load_language_pack_returns_none_for_english(language: str | None) ->
None:
+ """English (and falsy values) should never load a pack."""
+ assert _load_language_pack(language) is None
+
+
+def test_load_language_pack_returns_none_when_file_missing() -> None:
+ """Locales without a messages.json file fall through to the async path."""
+ with patch("superset.views.base.os.path.isfile", return_value=False):
+ assert _load_language_pack("xx_NOPE") is None
+
+
+def test_load_language_pack_returns_dict_when_file_present() -> None:
+ """A valid Jed JSON file is parsed and returned as a dict."""
+ fake_pack = {"domain": "superset", "locale_data": {"superset": {}}}
+ mock_open = patch(
+ "builtins.open",
+ new=MagicMock(),
+ )
+ with (
+ patch("superset.views.base.os.path.isfile", return_value=True),
+ patch(
+ "superset.views.base.json.loads",
+ return_value=fake_pack,
+ ) as mock_loads,
+ mock_open as mock_file,
+ ):
+ mock_file.return_value.__enter__.return_value.read.return_value = "{}"
+ result = _load_language_pack("pt_BR")
+
+ assert result == fake_pack
+ mock_loads.assert_called_once()
+
+
+def test_load_language_pack_returns_none_on_parse_error() -> None:
+ """Malformed JSON is swallowed; caller falls back to the async path."""
+ mock_open = patch("builtins.open", new=MagicMock())
+ with (
+ patch("superset.views.base.os.path.isfile", return_value=True),
+ patch(
+ "superset.views.base.json.loads",
+ side_effect=ValueError("bad json"),
+ ),
+ mock_open as mock_file,
+ ):
+ mock_file.return_value.__enter__.return_value.read.return_value =
"garbage"
+ assert _load_language_pack("pt_BR") is None
+
+
+def test_bootstrap_includes_language_pack_when_locale_set(
+ app_context: None,
+) -> None:
+ """The bootstrap payload carries the loaded pack under
common.language_pack."""
+ fake_pack = {"domain": "superset", "locale_data": {"superset": {}}}
+ with patch(
+ "superset.views.base._load_language_pack",
+ return_value=fake_pack,
+ ) as mock_load:
+ payload = _get_bootstrap()
+
+ assert payload["language_pack"] == fake_pack
+ mock_load.assert_called_once()
Review Comment:
This test is prone to flakiness because `_get_bootstrap()` calls a
`@memoize`d `cached_common_bootstrap_data` with fixed arguments (`user_id=1`,
`locale=None`). If any earlier test already populated the memoized value,
`_load_language_pack` won’t be invoked and the patch/assertions can fail.
Consider either clearing the memoized cache in the test setup, or calling
`cached_common_bootstrap_data` with a unique `(user_id, locale)` tuple (and
ideally a non-English `locale`, e.g. `'pt_BR'`) so the behavior under test
actually matches the docstring.
##########
superset/views/base.py:
##########
@@ -524,11 +552,20 @@ def cached_common_bootstrap_data( # pylint:
disable=unused-argument
)
frontend_config["AUTH_PROVIDERS"] = saml_providers
+ # Inject the Jed language pack into the bootstrap payload so the
+ # frontend can configure the translator synchronously, before any
+ # code-split chunk evaluates a module-level `const X = t('...')`.
+ # Without this, strings captured in module-scope constants render in
+ # English even when the locale is set and the async language_pack
+ # endpoint returns a valid pack (upstream issue #35330).
+ language_pack = _load_language_pack(language)
+
bootstrap_data = {
"application_root": app.config["APPLICATION_ROOT"],
"static_assets_prefix": app.config["STATIC_ASSETS_PREFIX"],
"conf": frontend_config,
"locale": language,
+ "language_pack": language_pack,
Review Comment:
Injecting the full Jed pack into `cached_common_bootstrap_data` means the
memoized cache entry now includes a potentially large blob per `(user_id,
locale)` key. This can significantly increase cache memory/Redis usage and
churn (especially with many users/locales) since the language pack itself is
locale-scoped, not user-scoped. A more scalable approach is to keep
`cached_common_bootstrap_data` small and inject `language_pack` outside the
memoized function (or cache `_load_language_pack(language)` separately keyed
only by `language`, then insert it into the response without storing it inside
the per-user memoized payload).
##########
superset/views/base.py:
##########
@@ -436,6 +436,34 @@ def get_default_spinner_svg() -> str | None:
return None
+def _load_language_pack(language: str | None) -> dict[str, Any] | None:
+ """Read the Jed language pack for ``language`` off disk.
+
+ Returns ``None`` for English, when the JSON file is missing, or when
+ it can't be parsed. The result is injected into the bootstrap payload
+ so the frontend can configure the translator synchronously before any
+ code-split chunk evaluates a module-level ``const X = t('...')``
+ (see upstream issue #35330).
+ """
+ if not language or language == "en":
+ return None
+ pack_path = os.path.join(
+ os.path.dirname(__file__),
+ "..",
+ "translations",
+ language,
+ "LC_MESSAGES",
+ "messages.json",
+ )
+ if not os.path.isfile(pack_path):
+ return None
+ try:
+ with open(pack_path, encoding="utf-8") as fp:
+ return cast(dict[str, Any], json.loads(fp.read()))
+ except (OSError, ValueError):
+ return None
+
+
@cache_manager.cache.memoize(timeout=60)
def cached_common_bootstrap_data( # pylint: disable=unused-argument
user_id: int | None, locale: str | None
Review Comment:
Injecting the full Jed pack into `cached_common_bootstrap_data` means the
memoized cache entry now includes a potentially large blob per `(user_id,
locale)` key. This can significantly increase cache memory/Redis usage and
churn (especially with many users/locales) since the language pack itself is
locale-scoped, not user-scoped. A more scalable approach is to keep
`cached_common_bootstrap_data` small and inject `language_pack` outside the
memoized function (or cache `_load_language_pack(language)` separately keyed
only by `language`, then insert it into the response without storing it inside
the per-user memoized payload).
--
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]
