codeant-ai-for-open-source[bot] commented on code in PR #40679: URL: https://github.com/apache/superset/pull/40679#discussion_r3799950843
########## superset/utils/i18n.py: ########## @@ -0,0 +1,270 @@ +# 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. +"""Localization of user-defined asset metadata (chart names, dashboard titles). + +This is distinct from UI-chrome translation (Flask-Babel / gettext), which +covers static strings baked into the application. Here we resolve *data* that +users author -- a chart called "Sales" should be able to display as "Ventes" +for a French viewer -- by delegating to a deployment-provided ``TRANSLATION_HOOK``. + +Superset core intentionally does not store these translations itself; the hook +abstracts where they live (a database table, an external translation service, +a static mapping, ...), keeping core minimal and the feature pluggable. +""" + +from __future__ import annotations + +import logging +from collections.abc import Iterable, Mapping +from typing import Any, Callable + +from flask import current_app as app, g, has_request_context +from flask_babel import get_locale + +from superset.extensions import feature_flag_manager + +logger = logging.getLogger(__name__) + +#: Feature flag gating asset-metadata translation. +FEATURE_FLAG = "ENABLE_I18N_ASSET_TRANSLATIONS" + +#: Attribute on ``flask.g`` holding the per-request resolution memo. +_CACHE_ATTR = "_asset_translation_cache" + +#: Key identifying one resolution: locale + source text + hook context. The +#: context is part of the key because the same string may resolve differently +#: per field (a chart named "Sales" and a dashboard titled "Sales"). +_CacheKey = tuple[str, str, tuple[tuple[str, str], ...]] + + +def is_asset_translation_enabled() -> bool: + """Whether asset-metadata translation should be attempted at all. + + Gated on *both* conditions, mirroring the SIP-161 design: + 1. the ``ENABLE_I18N_ASSET_TRANSLATIONS`` feature flag is on, and + 2. more than one language is configured in ``LANGUAGES``. + + The second condition means single-language deployments (the default) pay + zero cost: ``translate`` short-circuits before resolving the locale or + invoking the hook. + """ + if not feature_flag_manager.is_feature_enabled(FEATURE_FLAG): + return False + return len(app.config.get("LANGUAGES") or {}) > 1 + + +def _target_locale() -> str | None: + """The locale to translate into, or ``None`` when there is nothing to do. + + ``None`` means the active locale is unresolved or already the default the + canonical text is authored in, so the stored text is the correct answer. + """ + locale = get_locale() + if locale is None: + return None + + locale_str = str(locale) + if locale_str == app.config.get("BABEL_DEFAULT_LOCALE", "en"): + return None + + return locale_str + + +def _cache() -> dict[_CacheKey, str | None] | None: + """Per-request resolution memo, or ``None`` outside a request context. + + Scoping to the request is what makes prefetching worthwhile: a batch + resolved up front stays visible to the per-item lookups that follow it + during serialization, while staleness is bounded to a single response. + Background jobs (thumbnails, reports) have no request and simply resolve + directly. + """ + if not has_request_context(): + return None + + cache: dict[_CacheKey, str | None] | None = getattr(g, _CACHE_ATTR, None) + if cache is None: + cache = {} + setattr(g, _CACHE_ATTR, cache) + return cache + + +def _cache_key(locale: str, text: str, context: dict[str, Any]) -> _CacheKey: + return (locale, text, tuple(sorted((k, str(v)) for k, v in context.items()))) + + +def _call_hook( + hook: Callable[..., str | None], + text: str, + locale: str, + context: dict[str, Any], +) -> str | None: + """Invoke the single-text hook, swallowing failures.""" + try: + return hook(text, locale, **context) + except Exception: # pylint: disable=broad-except + # A failing hook must never break asset rendering -- log and fall back. + logger.exception( + "TRANSLATION_HOOK raised while translating %r to %s", text, locale + ) + return None + + +def _call_batch_hook( + hook: Callable[..., Mapping[str, str | None] | None], + texts: list[str], + locale: str, + context: dict[str, Any], +) -> dict[str, str | None]: + """Invoke the batch hook, swallowing failures and bad return types.""" + try: + resolved = hook(list(texts), locale, **context) + except Exception: # pylint: disable=broad-except + logger.exception( + "TRANSLATION_BATCH_HOOK raised while translating %d strings to %s", + len(texts), + locale, + ) + return {} + + if resolved is None: + return {} + + if not isinstance(resolved, Mapping): + logger.warning( + "TRANSLATION_BATCH_HOOK returned %s, expected a mapping of " + "source text to translation; falling back to the canonical text", + type(resolved).__name__, + ) + return {} + + return dict(resolved) + + +def translate(default_text: str | None, **context: object) -> str | None: + """Resolve ``default_text`` for the active locale, or return it unchanged. + + Returns ``default_text`` verbatim when the feature is disabled, the active + locale is the default locale, no hook is configured, or the hook fails or + declines to translate. The original text is *always* a safe fallback so a + missing or broken translation never blanks out a chart or dashboard name. + + Extra ``context`` (e.g. ``model_name``, ``field_name``) is forwarded to the + hook so an implementation can disambiguate identical strings across fields. + + Resolutions are memoized for the request, so repeated strings -- and any + value already fetched by :func:`translate_many` -- cost no extra hook call. + """ + if not default_text or not is_asset_translation_enabled(): + return default_text + + locale_str = _target_locale() + if locale_str is None: + return default_text + + cache = _cache() + key = _cache_key(locale_str, default_text, context) + if cache is not None and key in cache: + return cache[key] or default_text + + hook = app.config.get("TRANSLATION_HOOK") + if hook is None: + # A deployment may configure only the batch hook; route through it so + # it is a complete replacement rather than an add-on. + if app.config.get("TRANSLATION_BATCH_HOOK") is None: + return default_text + return translate_many([default_text], **context).get(default_text, default_text) + + translated = _call_hook(hook, default_text, locale_str, context) + if cache is not None: + cache[key] = translated + + return translated or default_text Review Comment: **Suggestion:** When both hooks are configured, `translate` invokes `TRANSLATION_HOOK` directly instead of routing the lookup through `TRANSLATION_BATCH_HOOK`. This contradicts the documented contract that the batch hook fully replaces the single-value hook and means individual lookups can bypass the configured batch store or return results inconsistent with prefetched values. Prefer the batch hook whenever it is configured. [api mismatch] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ⚠️ Chart display can differ between prefetched and direct lookups. - ⚠️ Dashboard localized titles may bypass the configured batch store. - ⚠️ Recent-activity serialization uses the same single lookup path. ``` </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=9023d14415b9478490b78fa10e758841&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=9023d14415b9478490b78fa10e758841&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) <details> <summary><b>Prompt for AI Agent 🤖 </b></summary> ```mdx This is a comment left during a code review. **Path:** superset/utils/i18n.py **Line:** 184:196 **Comment:** *Api Mismatch: When both hooks are configured, `translate` invokes `TRANSLATION_HOOK` directly instead of routing the lookup through `TRANSLATION_BATCH_HOOK`. This contradicts the documented contract that the batch hook fully replaces the single-value hook and means individual lookups can bypass the configured batch store or return results inconsistent with prefetched values. Prefer the batch hook whenever it is configured. 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%2F40679&comment_hash=d3244aae3cad369cbc13d9e3f479e4afec56a0b52f5fac347c76103cf362790e&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40679&comment_hash=d3244aae3cad369cbc13d9e3f479e4afec56a0b52f5fac347c76103cf362790e&reaction=dislike'>👎</a> ########## examples/asset_metadata_translation/hook.py: ########## @@ -0,0 +1,110 @@ +# 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. +"""Reference translation hooks backed by the ``AssetTranslation`` table. + +NOT part of Superset core -- see this directory's README. Assign +``translation_hook`` to ``TRANSLATION_HOOK`` in ``superset_config.py``, or +``translation_batch_hook`` to ``TRANSLATION_BATCH_HOOK`` to resolve a whole +collection in a single query (preferred for a table-backed store). +""" + +from __future__ import annotations + +import logging +from collections.abc import Sequence + +logger = logging.getLogger(__name__) + + +def translation_hook( + default_text: str, + locale: str, + **kwargs: object, +) -> str | None: + """Look up a stored translation for the active locale. + + Matches on the source text plus the ``model_name``/``field_name`` context + Superset passes, so the same string can be translated differently per field. + Returns ``None`` when there is no match (Superset falls back to the canonical + text). Any failure is swallowed so rendering never breaks on a lookup error. + """ + # Local imports: these are only importable inside the running app context. + from superset import db + + from .model import AssetTranslation + + try: + row = ( + db.session.query(AssetTranslation.translated_text) + .filter( + AssetTranslation.language_code == locale, + AssetTranslation.default_text == default_text, + AssetTranslation.model_name == kwargs.get("model_name", ""), + AssetTranslation.field_name == kwargs.get("field_name", ""), + ) + .first() + ) + except Exception: # pylint: disable=broad-except + logger.exception("asset translation lookup failed for %r", default_text) + return None + + return row[0] if row else None + + +def translation_batch_hook( + default_texts: Sequence[str], + locale: str, + **kwargs: object, +) -> dict[str, str]: + """Look up many stored translations in one query. + + The batch counterpart to :func:`translation_hook`: Superset passes every + string it is about to render for one context -- all of a dashboard's chart + names, say -- so a table-backed store answers with a single ``IN`` query + instead of one per string. Strings with no stored translation are simply + absent from the result; Superset falls back to the canonical text. + """ + # Local imports: these are only importable inside the running app context. + from superset import db + + from .model import AssetTranslation + + if not default_texts: + return {} + + try: + rows = ( + db.session.query( + AssetTranslation.default_text, + AssetTranslation.translated_text, + ) + .filter( + AssetTranslation.language_code == locale, + AssetTranslation.default_text.in_(default_texts), + AssetTranslation.model_name == kwargs.get("model_name", ""), + AssetTranslation.field_name == kwargs.get("field_name", ""), + ) + .all() + ) + except Exception: # pylint: disable=broad-except + logger.exception( + "asset translation batch lookup failed for %d strings", + len(default_texts), + ) + return {} + + return dict(rows) Review Comment: **Suggestion:** The batch query returns SQLAlchemy `Row` objects, but `dict(rows)` does not reliably convert two-column `Row` objects into a source-to-translation mapping under SQLAlchemy 2. This raises a `TypeError` after the query succeeds, so the batch hook fails and core falls back to canonical text. Convert each row through its mapping or explicitly build the dictionary from the two values. [type error] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ❌ Batch-backed asset translations fail after successful database queries. - ⚠️ Dashboard chart names fall back to canonical text. - ⚠️ Recent-activity translations also lose batch results. ``` </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=ff740f87eae845c2bc5bae2b0b30b9e6&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=ff740f87eae845c2bc5bae2b0b30b9e6&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) <details> <summary><b>Prompt for AI Agent 🤖 </b></summary> ```mdx This is a comment left during a code review. **Path:** examples/asset_metadata_translation/hook.py **Line:** 89:110 **Comment:** *Type Error: The batch query returns SQLAlchemy `Row` objects, but `dict(rows)` does not reliably convert two-column `Row` objects into a source-to-translation mapping under SQLAlchemy 2. This raises a `TypeError` after the query succeeds, so the batch hook fails and core falls back to canonical text. Convert each row through its mapping or explicitly build the dictionary from the two values. 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%2F40679&comment_hash=506ec62ea7148a8996d140d84517018c7efdde8bb504e240733e7558a038f7ee&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40679&comment_hash=506ec62ea7148a8996d140d84517018c7efdde8bb504e240733e7558a038f7ee&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]
