Copilot commented on code in PR #41780:
URL: https://github.com/apache/superset/pull/41780#discussion_r3679075231
##########
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:
`get_language_pack_version()` logs a warning and returns None when the pack
file is missing, but it doesn’t cache the negative result. In environments
where translations aren’t built (or a locale is configured without a pack),
this will re-stat/read the filesystem and re-log on every request. Cache the
“missing” result to avoid repeated I/O and log spam.
##########
superset/views/base.py:
##########
@@ -611,25 +611,50 @@ def common_bootstrap_payload() -> dict[str, Any]:
# Convert locale to string for proper cache key hashing
locale_str = str(locale) if locale else None
payload = dict(cached_common_bootstrap_data(utils.get_user_id(),
locale_str))
- # Inject the Jed language pack outside the per-user memoize so the cached
- # payload stays small and the pack is shared across users for the same
- # locale. The frontend uses it to configure the translator synchronously,
- # before any code-split chunk evaluates a module-level `const X = t('...')`
- # (upstream issue #35330).
- language = payload.get("locale")
- if language and language != "en":
- # Respect a pack already provided via COMMON_BOOTSTRAP_OVERRIDES_FUNC
- # (the workaround in #35330 does exactly that), otherwise load the
- # shared one. `get_language_pack` returns the empty English pack on a
- # miss, which is the right result (English) when no translation file
- # exists.
- pack = payload.get("language_pack") or get_language_pack(language)
- else:
- pack = None
- payload["language_pack"] = pack
+ # The language pack itself is NOT embedded in the payload: spa.html loads
+ # it through the content-addressed
/language_pack/<lang>/<version>/script.js
+ # tag before the entry bundle, keeping HTML small while still configuring
+ # the translator synchronously (upstream issue #35330). A pack provided via
+ # COMMON_BOOTSTRAP_OVERRIDES_FUNC (the historical workaround) is respected
+ # and takes precedence over the script tag.
+ payload.setdefault("language_pack", None)
return payload
+def get_language_pack_template_context(common: dict[str, Any]) -> dict[str,
Any]:
+ """Template vars controlling how spa.html delivers the language pack.
+
+ ``common`` is the already-built common bootstrap payload for the request
+ (passed in rather than re-derived, so callers that assembled or mocked
+ their own payload stay consistent with what the template sees).
+
+ Three mutually exclusive outcomes:
+ - English (or no locale): no script tag, nothing to stash.
+ - Operator supplied a pack via COMMON_BOOTSTRAP_OVERRIDES_FUNC: no script
+ tag; spa.html stashes the bootstrap pack on window instead.
+ - Otherwise: emit the content-addressed script URL so the browser can
+ cache the pack as immutable and cache-bust on translation changes.
+ """
+ language = common.get("locale")
+ if not language or language == "en":
+ return {"language_pack_src": None, "language_pack_inline": False}
+ if common.get("language_pack"):
+ return {"language_pack_src": None, "language_pack_inline": True}
Review Comment:
`get_language_pack_template_context` uses a truthiness check for
`common.get("language_pack")`, which will treat an empty dict as “not provided”
and emit the script tag instead of honoring the operator override. Since
`common_bootstrap_payload()` always sets the key (defaulting to None), it’s
safer to check for `is not None` to preserve any explicit override value.
--
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]
