mikebridge commented on code in PR #44180: URL: https://github.com/apache/superset/pull/44180#discussion_r4039286256
########## tests/unit_tests/views/test_i18n_constants.py: ########## @@ -0,0 +1,282 @@ +# 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. +"""sc-120397: module-level user-facing constants must be LAZY gettext. + +A module-level constant is evaluated once at import time, outside any +request, so eager ``__()`` freezes it in the default locale for every +user forever. The convention (paired with sc-120052's inverse): eager +``__()`` for strings built inside request handlers; lazy ``_()`` for +module-scope constants, coerced with ``str()`` at the point of use. +""" + +import ast +import pathlib +from unittest.mock import Mock + +import pytest +from flask_babel.speaklater import LazyString +from pytest_mock import MockerFixture + +from superset.errors import SupersetErrorType +from superset.exceptions import CertificateException +from superset.sqllab.query_render import PARAMETER_MISSING_ERR +from superset.views.core import DATASOURCE_MISSING_ERR + + [email protected]("constant", [DATASOURCE_MISSING_ERR, PARAMETER_MISSING_ERR]) +def test_module_constants_are_lazy(constant: object) -> None: + """The constants must be LazyString, not import-time-resolved str.""" + assert isinstance(constant, LazyString) + + +def test_constant_resolves_through_the_live_translation_lookup( + mocker: MockerFixture, +) -> None: + """str(constant) consults the active translation machinery per call. + + Stubbing flask-babel's domain proves every render goes through the + lookup — an eager constant would have been frozen to a plain str + before the stub existed and could never produce the sentinel. Runs on + every backend, unlike a compiled-catalog-dependent locale pin.""" + domain: Mock = mocker.Mock() + domain.gettext.side_effect = lambda s, **kw: f"[[{s}]]" + mocker.patch("flask_babel.get_domain", return_value=domain) + + assert str(DATASOURCE_MISSING_ERR) == ( + "[[The data source seems to have been deleted]]" + ) + + [email protected]("message", ["", "Custom certificate error"]) +def test_certificate_error_translates_default_at_construction( + mocker: MockerFixture, message: str +) -> None: + """Translate default instance messages while preserving explicit error details.""" + translate: Mock = mocker.patch( + "superset.exceptions._", return_value="Translated certificate error" + ) + cause: Exception = ValueError("Invalid PEM") + error: CertificateException = CertificateException( + message, cause, SupersetErrorType.GENERIC_BACKEND_ERROR + ) + expected: str = message or "Translated certificate error" + assert str(error) == expected + assert error.to_dict()["message"] == expected + assert error.exception is cause + assert error.error_type == SupersetErrorType.GENERIC_BACKEND_ERROR + if message: + translate.assert_not_called() + else: + translate.assert_called_once_with("Invalid certificate") + + +def _is_eager_gettext_call(node: ast.expr, bindings: dict[str, str]) -> bool: + """Recognize calls to imported eager Babel functions or module attributes.""" + eager_names: set[str] = {"gettext", "ngettext", "pgettext", "npgettext"} + if isinstance(node, ast.Name): + return bindings.get(node.id) in eager_names + return ( + isinstance(node, ast.Attribute) + and isinstance(node.value, ast.Name) + and bindings.get(node.value.id) == "flask_babel" + and node.attr in eager_names + ) + + +def _record_gettext_import(node: ast.AST, bindings: dict[str, str]) -> None: + """Resolve Babel import aliases to their original function or module names.""" + alias: ast.alias + if isinstance(node, ast.ImportFrom): + if node.module == "flask_babel" and node.level == 0: + for alias in node.names: + bindings[alias.asname or alias.name] = alias.name + elif isinstance(node, ast.Import): + for alias in node.names: + if alias.name == "flask_babel": + bindings[alias.asname or alias.name] = "flask_babel" + + +def _find_eager_gettext_assignments(source: str) -> list[int]: + """Return line numbers of direct eager gettext assignments at import time.""" + offenders: list[int] = [] + + def visit(node: ast.AST, bindings: dict[str, str]) -> None: + """Track imports through executable blocks, excluding function bodies.""" + if isinstance(node, (ast.FunctionDef, ast.AsyncFunctionDef)): + return + if isinstance(node, ast.ClassDef): + bindings = bindings.copy() + _record_gettext_import(node, bindings) + if isinstance(node, (ast.Assign, ast.AnnAssign)): + if isinstance(node.value, ast.Call) and _is_eager_gettext_call( Review Comment: Addressed in bab91169a63ce3786bc7f29920497521dd451a94. The tripwire now visits nested assignment expressions, including keyword metadata, dict/list values and eager comprehensions. It excludes deferred lambda/generator bodies while checking lambda defaults and generator outer iterables. The wider scan finds 137 individual calls across 24 files. These are recorded as explicit existing debt by file + normalized call expression + occurrence budget, rather than exempting whole files or converting unrelated metadata to lazy strings. Controls reject a new message and extra occurrences of an allowed call. Four new classifier cases failed before the fix; the final focused suite passes all 36 tests, and scoped pre-commit checks pass for all five branch-changed files. No production behavior changes in this follow-up. The scanner remains a bounded syntax guard, not an interprocedural evaluator. Fresh CI on this SHA is still pending. -- 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]
