codeant-ai-for-open-source[bot] commented on code in PR #41780: URL: https://github.com/apache/superset/pull/41780#discussion_r3562108288
########## 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: **Suggestion:** Add a type annotation to this local variable to make the expected value type explicit in the test logic. [custom_rule] **Severity Level:** Minor ๐งน <details> <summary><b>Why it matters? โญ </b></summary> The local variable `template` is assigned a string value and could be annotated as `str`, but no type hint is present. This fits the stated rule for relevant variables that can be annotated. </details> <details> <summary><b>Rule source ๐ </b></summary> .cursor/rules/dev-standard.mdc (line 28) </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=56c44688d50846a29763e5948a9950f0&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=56c44688d50846a29763e5948a9950f0&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) *(Use Cmd/Ctrl + Click for best experience)* <details> <summary><b>Prompt for AI Agent ๐ค </b></summary> ```mdx This is a comment left during a code review. **Path:** tests/unit_tests/views/test_language_pack_script.py **Line:** 105:107 **Comment:** *Custom Rule: Add a type annotation to this local variable to make the expected value type explicit in the test logic. Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise. Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix ``` </details> <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41780&comment_hash=6e88b94fdaef3eee8dabf744bc880d6fba3b0fc2cc6be85e8646bcc0d570cf5c&reaction=like'>๐</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41780&comment_hash=6e88b94fdaef3eee8dabf744bc880d6fba3b0fc2cc6be85e8646bcc0d570cf5c&reaction=dislike'>๐</a> ########## 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: **Suggestion:** Provide an explicit integer type annotation for this index variable to align with the type-hint requirement. [custom_rule] **Severity Level:** Minor ๐งน <details> <summary><b>Why it matters? โญ </b></summary> `tag_start` is a new local integer variable derived from `str.index(...)` and is left unannotated. The custom rule explicitly calls for type hints on relevant variables that can be annotated. </details> <details> <summary><b>Rule source ๐ </b></summary> .cursor/rules/dev-standard.mdc (line 28) </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=dcde2bb110b84f0e9f99b40441c9847e&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=dcde2bb110b84f0e9f99b40441c9847e&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) *(Use Cmd/Ctrl + Click for best experience)* <details> <summary><b>Prompt for AI Agent ๐ค </b></summary> ```mdx This is a comment left during a code review. **Path:** tests/unit_tests/views/test_language_pack_script.py **Line:** 109:109 **Comment:** *Custom Rule: Provide an explicit integer type annotation for this index variable to align with the type-hint requirement. Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise. Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix ``` </details> <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41780&comment_hash=2dc5cf5de7544441de553b28e292aff8efbc7457197eec190f99bf60e9cc4048&reaction=like'>๐</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41780&comment_hash=2dc5cf5de7544441de553b28e292aff8efbc7457197eec190f99bf60e9cc4048&reaction=dislike'>๐</a> ########## 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: **Suggestion:** Add an explicit type annotation for `fake_pack` to satisfy the requirement for annotating relevant newly introduced variables. [custom_rule] **Severity Level:** Minor ๐งน <details> <summary><b>Why it matters? โญ </b></summary> The new variable is a structured dict introduced in modified Python code and can clearly be annotated (for example, as a dict[str, Any]). The rule requires type hints for relevant newly added variables, so this omission is a real violation. </details> <details> <summary><b>Rule source ๐ </b></summary> .cursor/rules/dev-standard.mdc (line 28) </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=f131e809e5214150aad5b5b7c80f01a0&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=f131e809e5214150aad5b5b7c80f01a0&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) *(Use Cmd/Ctrl + Click for best experience)* <details> <summary><b>Prompt for AI Agent ๐ค </b></summary> ```mdx This is a comment left during a code review. **Path:** tests/unit_tests/views/test_bootstrap_auth.py **Line:** 189:189 **Comment:** *Custom Rule: Add an explicit type annotation for `fake_pack` to satisfy the requirement for annotating relevant newly introduced variables. Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise. Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix ``` </details> <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41780&comment_hash=18adf81672350a5f0cf906bc014fe42f810fe91f286d5914029c0ae09b27d44e&reaction=like'>๐</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41780&comment_hash=18adf81672350a5f0cf906bc014fe42f810fe91f286d5914029c0ae09b27d44e&reaction=dislike'>๐</a> -- 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]
