rusackas commented on code in PR #41780: URL: https://github.com/apache/superset/pull/41780#discussion_r3564076999
########## tests/unit_tests/views/test_language_pack_script.py: ########## @@ -0,0 +1,115 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. +from pathlib import Path +from typing import Any +from unittest.mock import patch + +FAKE_PACK = {"domain": "superset", "locale_data": {"superset": {"": {}}}} +FAKE_VERSION = "abc123def456" + + +def test_script_served_immutable_when_version_matches(client: Any) -> None: + with ( + patch( + "superset.views.core.get_language_pack_version", + return_value=FAKE_VERSION, + ), + patch("superset.views.core.get_language_pack", return_value=FAKE_PACK), + ): + response = client.get(f"/language_pack/fr/{FAKE_VERSION}/script.js") + + assert response.status_code == 200 + assert response.mimetype == "application/javascript" + body = response.get_data(as_text=True) + assert body.startswith("window.__SUPERSET_LANGUAGE_PACK__ = ") + assert '"domain": "superset"' in body + cache_control = response.headers["Cache-Control"] + assert "immutable" in cache_control + assert "max-age=31536000" in cache_control + assert "public" in cache_control + + +def test_script_not_cacheable_when_version_stale(client: Any) -> None: + """A pre-upgrade HTML page may reference an old version: serve fresh + content, but do not let caches pin it under the stale address.""" + with ( + patch( + "superset.views.core.get_language_pack_version", + return_value=FAKE_VERSION, + ), + patch("superset.views.core.get_language_pack", return_value=FAKE_PACK), + ): + response = client.get("/language_pack/fr/000000000000/script.js") + + assert response.status_code == 200 + assert "no-cache" in response.headers["Cache-Control"] + assert "immutable" not in response.headers["Cache-Control"] + + +def test_script_404_when_pack_missing(client: Any) -> None: + with patch( + "superset.views.core.get_language_pack_version", + return_value=None, + ): + response = client.get(f"/language_pack/xx/{FAKE_VERSION}/script.js") + + assert response.status_code == 404 + + +def test_script_rejects_malformed_lang_and_version(client: Any) -> None: + assert client.get(f"/language_pack/../{FAKE_VERSION}/script.js").status_code in ( + 400, + 404, + ) + assert client.get("/language_pack/fr/not-a-hash!/script.js").status_code == 400 + + +def test_script_serves_only_the_requested_locale(client: Any) -> None: + """The endpoint resolves exactly one pack: the locale in the URL.""" + with ( + patch( + "superset.views.core.get_language_pack_version", + return_value=FAKE_VERSION, + ) as mock_version, + patch( + "superset.views.core.get_language_pack", return_value=FAKE_PACK + ) as mock_pack, + ): + client.get(f"/language_pack/pt_BR/{FAKE_VERSION}/script.js") + + mock_version.assert_called_once_with("pt_BR") + mock_pack.assert_called_once_with("pt_BR") + + +def test_spa_template_loads_pack_before_entry_bundle() -> None: + """Static guard on spa.html: the language pack script tag must precede + the entry bundle and stay a classic script (no async/defer). A deferred + or reordered tag would let code-split chunks evaluate module-level + `t('...')` calls before the translator is configured (issue #35330).""" + import superset + + template = ( + Path(superset.__file__).parent / "templates" / "superset" / "spa.html" + ).read_text() Review Comment: Fixed—annotated `template: str`. ########## tests/unit_tests/views/test_language_pack_script.py: ########## @@ -0,0 +1,115 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. +from pathlib import Path +from typing import Any +from unittest.mock import patch + +FAKE_PACK = {"domain": "superset", "locale_data": {"superset": {"": {}}}} +FAKE_VERSION = "abc123def456" + + +def test_script_served_immutable_when_version_matches(client: Any) -> None: + with ( + patch( + "superset.views.core.get_language_pack_version", + return_value=FAKE_VERSION, + ), + patch("superset.views.core.get_language_pack", return_value=FAKE_PACK), + ): + response = client.get(f"/language_pack/fr/{FAKE_VERSION}/script.js") + + assert response.status_code == 200 + assert response.mimetype == "application/javascript" + body = response.get_data(as_text=True) + assert body.startswith("window.__SUPERSET_LANGUAGE_PACK__ = ") + assert '"domain": "superset"' in body + cache_control = response.headers["Cache-Control"] + assert "immutable" in cache_control + assert "max-age=31536000" in cache_control + assert "public" in cache_control + + +def test_script_not_cacheable_when_version_stale(client: Any) -> None: + """A pre-upgrade HTML page may reference an old version: serve fresh + content, but do not let caches pin it under the stale address.""" + with ( + patch( + "superset.views.core.get_language_pack_version", + return_value=FAKE_VERSION, + ), + patch("superset.views.core.get_language_pack", return_value=FAKE_PACK), + ): + response = client.get("/language_pack/fr/000000000000/script.js") + + assert response.status_code == 200 + assert "no-cache" in response.headers["Cache-Control"] + assert "immutable" not in response.headers["Cache-Control"] + + +def test_script_404_when_pack_missing(client: Any) -> None: + with patch( + "superset.views.core.get_language_pack_version", + return_value=None, + ): + response = client.get(f"/language_pack/xx/{FAKE_VERSION}/script.js") + + assert response.status_code == 404 + + +def test_script_rejects_malformed_lang_and_version(client: Any) -> None: + assert client.get(f"/language_pack/../{FAKE_VERSION}/script.js").status_code in ( + 400, + 404, + ) + assert client.get("/language_pack/fr/not-a-hash!/script.js").status_code == 400 + + +def test_script_serves_only_the_requested_locale(client: Any) -> None: + """The endpoint resolves exactly one pack: the locale in the URL.""" + with ( + patch( + "superset.views.core.get_language_pack_version", + return_value=FAKE_VERSION, + ) as mock_version, + patch( + "superset.views.core.get_language_pack", return_value=FAKE_PACK + ) as mock_pack, + ): + client.get(f"/language_pack/pt_BR/{FAKE_VERSION}/script.js") + + mock_version.assert_called_once_with("pt_BR") + mock_pack.assert_called_once_with("pt_BR") + + +def test_spa_template_loads_pack_before_entry_bundle() -> None: + """Static guard on spa.html: the language pack script tag must precede + the entry bundle and stay a classic script (no async/defer). A deferred + or reordered tag would let code-split chunks evaluate module-level + `t('...')` calls before the translator is configured (issue #35330).""" + import superset + + template = ( + Path(superset.__file__).parent / "templates" / "superset" / "spa.html" + ).read_text() + + tag_start = template.index('<script src="{{ language_pack_src }}"') Review Comment: Fixed—annotated `tag_start: int`. ########## 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": {}}} Review Comment: Fixed—annotated `fake_pack: dict[str, Any]`. ########## superset/views/core.py: ########## @@ -932,6 +933,48 @@ def language_pack(self, lang: str) -> FlaskResponse: "Language pack doesn't exist on the server", status=404 ) + @expose("/language_pack/<lang>/<version>/script.js") + def language_pack_script(self, lang: str, version: str) -> FlaskResponse: + """Serve the language pack as a content-addressed classic script. + + spa.html loads this BEFORE the entry bundle so translations are + configured synchronously (no race with code-split chunks), while the + versioned URL lets browsers cache the pack as immutable and pick up a + fresh copy whenever translations change. + + Deliberately unauthenticated: translation catalogs are static, public + content shipped in the Superset repo, contain no user or tenant data, + and must load for anonymous principals (login page, embedded). + """ + # Only allow expected language formats like "en", "pt_BR", etc. + if not re.match(r"^[a-z]{2,3}(_[A-Z]{2})?$", lang): + abort(400, "Invalid language code") + if not re.match(r"^[0-9a-f]{12}$", version): + abort(400, "Invalid language pack version") + + current_version = get_language_pack_version(lang) + pack = get_language_pack(lang) + if current_version is None or pack is None: Review Comment: Fair point—short-circuited on `current_version is None` so a missing locale skips the second file read and error log. -- 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]
