rusackas commented on code in PR #41780: URL: https://github.com/apache/superset/pull/41780#discussion_r3526398716
########## tests/unit_tests/translations/utils_test.py: ########## @@ -0,0 +1,71 @@ +# 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. +import hashlib +from collections.abc import Iterator + +import pytest + +from superset.translations import utils as translations_utils +from superset.translations.utils import ( + get_language_pack_filename, + get_language_pack_version, +) + + [email protected](autouse=True) +def _clear_version_cache() -> Iterator[None]: + translations_utils.ALL_LANGUAGE_PACK_VERSIONS.clear() + yield + translations_utils.ALL_LANGUAGE_PACK_VERSIONS.clear() + + +def test_language_pack_filename_resolution() -> None: + assert get_language_pack_filename("fr").endswith("/fr/LC_MESSAGES/messages.json") + assert get_language_pack_filename("en").endswith("/empty_language_pack.json") + assert get_language_pack_filename("").endswith("/empty_language_pack.json") + + +def test_version_is_short_content_hash(tmp_path, monkeypatch) -> None: Review Comment: pytest's `tmp_path`/`monkeypatch` fixtures are injected with well-known types and the `tests.*` mypy override relaxes untyped defs... every test module in the repo leaves these unannotated, so keeping parity here. ########## tests/unit_tests/translations/utils_test.py: ########## @@ -0,0 +1,71 @@ +# 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. +import hashlib +from collections.abc import Iterator + +import pytest + +from superset.translations import utils as translations_utils +from superset.translations.utils import ( + get_language_pack_filename, + get_language_pack_version, +) + + [email protected](autouse=True) +def _clear_version_cache() -> Iterator[None]: + translations_utils.ALL_LANGUAGE_PACK_VERSIONS.clear() + yield + translations_utils.ALL_LANGUAGE_PACK_VERSIONS.clear() + + +def test_language_pack_filename_resolution() -> None: + assert get_language_pack_filename("fr").endswith("/fr/LC_MESSAGES/messages.json") + assert get_language_pack_filename("en").endswith("/empty_language_pack.json") + assert get_language_pack_filename("").endswith("/empty_language_pack.json") + + +def test_version_is_short_content_hash(tmp_path, monkeypatch) -> None: + pack_file = tmp_path / "fr" / "LC_MESSAGES" / "messages.json" + pack_file.parent.mkdir(parents=True) + pack_file.write_bytes(b'{"domain": "superset"}') + monkeypatch.setattr(translations_utils, "DIR", str(tmp_path)) + + version = get_language_pack_version("fr") + + expected = hashlib.sha256(b'{"domain": "superset"}').hexdigest()[:12] + assert version == expected + + +def test_version_is_cached_and_changes_with_content(tmp_path, monkeypatch) -> None: Review Comment: Same as the thread above... standard unannotated pytest fixtures, matching the repo's test convention. ########## tests/unit_tests/translations/utils_test.py: ########## @@ -0,0 +1,71 @@ +# 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. +import hashlib +from collections.abc import Iterator + +import pytest + +from superset.translations import utils as translations_utils +from superset.translations.utils import ( + get_language_pack_filename, + get_language_pack_version, +) + + [email protected](autouse=True) +def _clear_version_cache() -> Iterator[None]: + translations_utils.ALL_LANGUAGE_PACK_VERSIONS.clear() + yield + translations_utils.ALL_LANGUAGE_PACK_VERSIONS.clear() + + +def test_language_pack_filename_resolution() -> None: + assert get_language_pack_filename("fr").endswith("/fr/LC_MESSAGES/messages.json") + assert get_language_pack_filename("en").endswith("/empty_language_pack.json") + assert get_language_pack_filename("").endswith("/empty_language_pack.json") + + +def test_version_is_short_content_hash(tmp_path, monkeypatch) -> None: + pack_file = tmp_path / "fr" / "LC_MESSAGES" / "messages.json" + pack_file.parent.mkdir(parents=True) + pack_file.write_bytes(b'{"domain": "superset"}') + monkeypatch.setattr(translations_utils, "DIR", str(tmp_path)) + + version = get_language_pack_version("fr") + + expected = hashlib.sha256(b'{"domain": "superset"}').hexdigest()[:12] + assert version == expected + + +def test_version_is_cached_and_changes_with_content(tmp_path, monkeypatch) -> None: + pack_file = tmp_path / "fr" / "LC_MESSAGES" / "messages.json" + pack_file.parent.mkdir(parents=True) + pack_file.write_bytes(b"{}") + monkeypatch.setattr(translations_utils, "DIR", str(tmp_path)) + + first = get_language_pack_version("fr") + pack_file.write_bytes(b'{"changed": true}') + # Cached within the process lifetime: same version until cache cleared. + assert get_language_pack_version("fr") == first + + translations_utils.ALL_LANGUAGE_PACK_VERSIONS.clear() + assert get_language_pack_version("fr") != first + + +def test_version_none_when_pack_missing(tmp_path, monkeypatch) -> None: Review Comment: Same as above. ########## 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": {"": {}}}} Review Comment: mypy infers `FAKE_PACK`/`FAKE_VERSION` from their literals, and test modules are covered by the `tests.*` override... annotating constants only here would diverge from the sibling test files. Skipping. ########## 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" Review Comment: Same as the constant thread above. ########## 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) Review Comment: mypy infers `version` as `str | None` from the typed dict's `.get()`, and the pre-existing `get_language_pack` in this same file leaves its equivalent locals (`pack`, `filename`) unannotated... matching the file's convention. mypy passes in pre-commit. ########## superset/translations/utils.py: ########## @@ -37,11 +70,7 @@ def get_language_pack(locale: str) -> Optional[dict[str, Any]]: """ pack = ALL_LANGUAGE_PACKS.get(locale) if not pack: - filename = DIR + f"/{locale}/LC_MESSAGES/messages.json" - if not locale or locale == "en": - # Forcing a dummy, quasy-empty language pack for English since the file - # in the en directory is contains data with empty mappings - filename = DIR + "/empty_language_pack.json" + filename = get_language_pack_filename(locale) Review Comment: Same rationale as `version` above... `filename` is inferred `str` from the annotated helper's return type. ########## superset/views/core.py: ########## @@ -917,6 +918,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") + Review Comment: These locals are all inferred precisely from annotated signatures (`get_language_pack_version -> Optional[str]`, `get_language_pack -> Optional[dict]`, `Response(...)`), and `core.py`'s convention leaves equivalent locals unannotated throughout. mypy passes, so skipping the three sibling threads together. ########## superset/views/core.py: ########## @@ -917,6 +918,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) Review Comment: Covered by the reply on the first core.py thread above. ########## superset/views/core.py: ########## @@ -917,6 +918,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: + return json_error_response( + "Language pack doesn't exist on the server", status=404 + ) + + response = Response( + f"window.__SUPERSET_LANGUAGE_PACK__ = {json.dumps(pack)};", + mimetype="application/javascript; charset=utf-8", + ) Review Comment: Covered by the reply on the first core.py thread above. -- 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]
