bito-code-review[bot] commented on code in PR #44180: URL: https://github.com/apache/superset/pull/44180#discussion_r4040943088
########## tests/unit_tests/views/test_i18n_constants.py: ########## @@ -0,0 +1,640 @@ +# 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 collections import Counter +from collections.abc import Iterator +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 _iter_eager_gettext_calls( + node: ast.AST, bindings: dict[str, str] +) -> Iterator[ast.Call]: + """Inspect nested values while respecting explicitly deferred expressions.""" + if isinstance(node, ast.Lambda): + # Defaults execute when the lambda is created; its body does not. + default: ast.expr | None + for default in (*node.args.defaults, *node.args.kw_defaults): + if default is not None: + yield from _iter_eager_gettext_calls(default, bindings) + return + if isinstance(node, ast.GeneratorExp): + # Only the outer iterable is evaluated when a generator is created. + yield from _iter_eager_gettext_calls(node.generators[0].iter, bindings) + return + if isinstance(node, ast.Call) and _is_eager_gettext_call(node.func, bindings): + yield node + child: ast.AST + for child in ast.iter_child_nodes(node): + yield from _iter_eager_gettext_calls(child, bindings) Review Comment: <div> <div id="suggestion"> <div id="issue"><b>Duplicate yield for nested calls</b></div> <div id="fix"> In `_iter_eager_gettext_calls`, after yielding a matched `ast.Call` at line 130-131, control falls through to the `ast.iter_child_nodes` loop at line 133, which re-visits the call's arguments. For a nested eager call like `tr(tr("x"))` the inner call is yielded twice, so `_unexpected_eager_calls`'s `Counter` budget (line 543-549) consumes two allowlist slots for one real occurrence and can false-fail the fence. Return after yielding a matched call. </div> </div> <small><i>Code Review Run #cb3a2a</i></small> </div> --- Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>) - [ ] Yes, avoid them -- 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]
