Repository navigation
fix(i18n): make module-level error constants lazy gettext - #44180
Conversation
DATASOURCE_MISSING_ERR (views/core.py) and PARAMETER_MISSING_ERR (sqllab/query_render.py) were built with the eager gettext alias at module scope, so they were resolved exactly once at import time — no request, no locale context — and served in the default locale to every user forever. The mirror image of sc-120052's bug, defining the paired convention: eager __() for strings built inside request handlers, lazy _() for module-scope constants, coerced with str() at the point of use (which also keeps this independent of apache#44090's LazyString coercion — correct in either merge order). USER_MISSING_ERR and views/core.py's duplicate PARAMETER_MISSING_ERR had no consumers at all and are removed. Tests pin the constants as LazyString, render DATASOURCE_MISSING_ERR under fr/en per-request locale (skipped where the fr catalog is not compiled), and add the inverse-convention tripwire: no module-level NAME = __(...) may exist in any module binding __ to eager gettext. sc-120397 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JRLEJS4mUqKBoPjSjviKUW
Committer-round optional fold: the per-locale render pin was skipif'd on a compiled fr catalog that CI never builds, so CI never ran it. Stub flask-babel's get_domain with a sentinel catalog instead: every str() of the constant must consult the live lookup (an eager constant would have been frozen to a plain str before the stub existed), which runs everywhere and stops hard-coding the fr catalog's exact French. sc-120397 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JRLEJS4mUqKBoPjSjviKUW
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #44180 +/- ##
==========================================
- Coverage 80.40% 80.39% -0.01%
==========================================
Files 2932 2932
Lines 174525 174526 +1
Branches 40548 40548
==========================================
- Hits 140319 140317 -2
- Misses 31537 31540 +3
Partials 2669 2669
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
…xception default at construction Review round on the tripwire (Codex astra, python lens) — the substring scan had a false-positive class (`"gettext as __" in source` also matches lazy_gettext) and blind spots (annotated assignments, bare gettext(), other aliases, class-body constants, docstring text). Replaced with an AST scanner: resolves each module's actual Babel import bindings (from-imports incl. aliases, and module-attribute calls like fb.gettext), then inspects Assign and AnnAssign at import-time scopes (module body, class bodies, module-level if/try/with) while skipping function bodies. 20 classifier self-test cases pin every probed corner. The stronger scanner immediately caught a fourth real offender: CertificateException's class-level default message, eager-translated at import (exceptions.py binds `_` to eager gettext — the alias the old scan could not see). The default now translates at construction; explicit messages, causes, and error types are preserved (tested), and the parse_ssl_cert raise site drops its redundant untranslated literal so the translated default is actually reachable. No consumer reads the class attribute or the exception's message (the schema validator emits its own). Also: LazyString annotations at both constant definition sites, and a two-locale test proving per-request locale SELECTION through real Babel machinery (compiled catalog + locale selector), not just deferred lookup. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JRLEJS4mUqKBoPjSjviKUW
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
|
The flagged issue is correct. Constructing To resolve this, you should use Would you like me to fetch all other comments on this PR to validate them and implement fixes for the rest? superset/exceptions.py |
|
Bito Automatic Review Failed - Technical Failure |
fitzee
left a comment
fitzee
left a comment
There was a problem hiding this comment.
Adversarial review of the lazy-i18n-constants change. This is a clean, well-tested i18n fix — the removed constants have no consumers, the AST classifier flags exactly the offender lines, the eager __ imports stay used (no F401), and the two behavioral tests (get_domain stub + real two-locale render) genuinely pin per-request translation. No correctness bugs. Two lower-severity notes below — one on the repo-wide tripwire test's robustness, one a constructive alternative for the CertificateException change. (Automated review; posted by a human reviewer.)
…state the construction-time rationale Review round (fitzee): the package tripwire gains an explicit (empty) allowlist as the opt-out for a future legitimately-eager module, skips unparseable files instead of failing an unrelated i18n test, and its docstring concedes the linter-rule home (a custom pylint checker is the right follow-up; the walk is ~1s meanwhile). CertificateException's __init__ now states why it diverges from the sibling class-attr pattern: construction-time translation resolves to plain str inside the raising request, so no LazyString can leak into to_dict()/JSON bodies. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JRLEJS4mUqKBoPjSjviKUW
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Code Review Agent Run #8f54c9Actionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
aminghadersohi
left a comment
aminghadersohi
left a comment
There was a problem hiding this comment.
Two inlines: the str() coercion the PR is built around is load-bearing but unasserted (dropping it empties the response body), and the new __init__ docstring's "sibling pattern" premise doesn't hold.
| # Check if datasource exists | ||
| if not datasource: | ||
| return json_error_response(DATASOURCE_MISSING_ERR) | ||
| return json_error_response(str(DATASOURCE_MISSING_ERR)) |
There was a problem hiding this comment.
json_error_response dispatches on isinstance(error_details, str) with no else, and LazyString is not a str subclass. Drop this str() and the body becomes {} — message silently gone — yet all 27 new tests still pass. No fence: the fix is a test, not a line here.
There was a problem hiding this comment.
Retracting the consequence half of this: your merge pulled in #44090, which added an elif isinstance(error_details, LazyString) branch, so dropping this str() now yields a byte-identical body (measured). Redundant rather than load-bearing — nothing to change.
| Deliberately diverges from the sibling pattern of a class-level | ||
| ``message`` default: translating at construction resolves to a |
There was a problem hiding this comment.
Parsed all 49 classes in this file: CertificateException was the only subclass carrying a class-level message default (SupersetException.message = "" is the base, not a sibling). No sibling pattern to diverge from — this converges on what the other 47 already do.
| Deliberately diverges from the sibling pattern of a class-level | |
| ``message`` default: translating at construction resolves to a | |
| Drops the class-level ``message`` default that made this the only | |
| such subclass: translating at construction resolves to a |
There was a problem hiding this comment.
Addressed in a2e72b4: the CertificateException docstring describes this subclass’s construction-time translation without claiming a sibling class-level pattern. No behavior change. The i18n suite passed (27 tests), pre-commit passed on all branch-changed files, and an independent Claude review approved the final snapshot. The retracted LazyString concern and accepted lint-rule follow-up remain unchanged.
Code Review Agent Run #298e05Actionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
|
CI refresh: merged Apache master containing the already-merged migration repair #44364 into this branch, published at 5a43e6e. No duplicate migration was added. The fresh PR merge ref includes repair commit 4c77b0b; its migration directory matches that repaired baseline. Local verification: 28 focused tests passed (including the single-migration-head regression), all applicable pre-commit hooks passed against the complete branch-changed file list, and the final source snapshot received independent Python review. Fresh CI has been triggered; this is not an all-green claim. Live database integration remains a remote-CI gate. The older migration failures tested pre-repair merge snapshots and are not evidence about this new head. |
|
Readiness follow-through at 5a43e6e: fresh checks have completed with no failing or pending checks (64 success, 10 skipped, 2 neutral). PostgreSQL tested 858333e, which includes merged migration repair #44364, and passed 2,790 tests. The CertificateException docstring correction is published as described in the inline reply. The LazyString consequence was explicitly retracted by the reviewer; the deferred lint-rule follow-up remains separate. Author work is complete, with reviewer confirmation still outstanding on the docstring thread. No new approval or automatic thread resolution is implied. |
aminghadersohi
left a comment
aminghadersohi
left a comment
There was a problem hiding this comment.
Docstring correction verified accurate against master (CertificateException did carry a class-level message). Three mutants on the lazy constants all fail as intended. One detection-scope gap in the tripwire, inline.
| 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( |
There was a problem hiding this comment.
Only a direct node.value Call is checked, so an eager call nested in a kwarg/dict/list is invisible: 80 import-time sites across 24 files pass today — e.g. db_engine_specs/base.py:3046, metadata={"description": __("Username")}. No fence; widening needs an allowlist call.
There was a problem hiding this comment.
Addressed in bab9116. 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.
![bito-code-review[bot]](https://avatars.githubusercontent.com/in/1061978?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review Agent Run #cb3a2a
Actionable Suggestions - 1
-
tests/unit_tests/views/test_i18n_constants.py - 1
- Duplicate yield for nested calls · Line 130-134
Additional Suggestions - 1
-
tests/unit_tests/views/test_i18n_constants.py - 1
-
Stale allowlist entries undetected · Line 249-538Every allowlist entry is consumed by real eager calls today (verified by re-running the classifier over `superset/`), but nothing fails when an entry goes stale: `_unexpected_eager_calls` only counts calls upward, never budgets downward. Once a message is fixed upstream, its entry becomes dead weight that silently re-exempts a future regression of the same call. Consider asserting all budgets are consumed.
-
Review Details
-
Files reviewed - 1 · Commit Range:
a2e72b4..bab9116- tests/unit_tests/views/test_i18n_constants.py
-
Files skipped - 0
-
Tools
- MyPy (Static Code Analysis) - ✔︎ Successful
- Astral Ruff (Static Code Analysis) - ✔︎ Successful
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful
Bito Usage Guide
Commands
Type the following command in the pull request comment and save the comment.
-
/review- Manually triggers an incremental AI Review. -
/review full- Manually triggers a full AI Review. -
/pause- Pauses automatic reviews on this pull request. -
/resume- Resumes automatic reviews. -
/resolve- Marks all Bito-posted review comments as resolved. -
/abort- Cancels all in-progress reviews.
Refer to the documentation for additional commands.
Configuration
This repository uses Superset You can customize the agent settings here or contact your Bito workspace admin at evan@preset.io.
Documentation & Help
AI Code Review powered by
| 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) |
There was a problem hiding this comment.
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.
Code Review Run #cb3a2a
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
Co-authored-by: Claude Fable 5 <noreply@anthropic.com> Co-authored-by: Mike Bridge <michael.bridge@ext.preset.io> (cherry picked from commit f1dc566)
…ation
`schema` as a constructor parameter on `BaseTemplateProcessor` took a name
away from users. A dataset's `template_params` are passed as keyword
arguments -- `models.helpers`: `template_kwargs.update(self.template_params_dict)`
-- so `{"schema": "sales"}` with `SELECT * FROM {{ schema }}.orders` stopped
rendering and left the reference in place. Verified against master, which
renders it.
Estimation instead builds an unpersisted `Query`, the way the execution path
has one, and passes it: the processor reads the location from it as it always
has, so no keyword is claimed and nothing else moves. `catalog` travels the
same way, from the same source as the schema, because
`presto.latest_partition` has to read partition metadata from the catalog the
query runs in, not the connection's default.
That query also carries the rendered SQL to the authorization check, pinned as
`executed_sql` -- the handle `_validate_rendered_access` uses for the same
reason. `raise_for_access` builds an equivalent query from a raw `sql=`, so
this is parity of mechanism rather than a change in what is authorized, and
the regression test says so rather than claiming more.
`PARAMETER_MISSING_ERR` follows apache#44180 and is lazy, resolved to `str` at each
response, so the suggestion is not frozen in the locale that imported it.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ation
`schema` as a constructor parameter on `BaseTemplateProcessor` took a name
away from users. A dataset's `template_params` are passed as keyword
arguments -- `models.helpers`: `template_kwargs.update(self.template_params_dict)`
-- so `{"schema": "sales"}` with `SELECT * FROM {{ schema }}.orders` stopped
rendering and left the reference in place. Verified against master, which
renders it.
Estimation instead builds an unpersisted `Query`, the way the execution path
has one, and passes it: the processor reads the location from it as it always
has, so no keyword is claimed and nothing else moves. `catalog` travels the
same way, from the same source as the schema, because
`presto.latest_partition` has to read partition metadata from the catalog the
query runs in, not the connection's default.
That query also carries the rendered SQL to the authorization check, pinned as
`executed_sql` -- the handle `_validate_rendered_access` uses for the same
reason. `raise_for_access` builds an equivalent query from a raw `sql=`, so
this is parity of mechanism rather than a change in what is authorized, and
the regression test says so rather than claiming more.
`PARAMETER_MISSING_ERR` follows apache#44180 and is lazy, resolved to `str` at each
response, so the suggestion is not frozen in the locale that imported it.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
SUMMARY
DATASOURCE_MISSING_ERR(views/core.py) andPARAMETER_MISSING_ERR(sqllab/query_render.py) were built with the eager gettext alias at module scope, so each was resolved exactly once at import time — no request, no locale context — and then served in the default locale to every user forever. This is the mirror image of the bug fixed in #44090 (lazy gettext inside request handlers), and together they define the convention:__()for strings built inside request handlers;_()for module-scope constants, coerced withstr()at the point of use inside the request.The point-of-use
str()also makes this PR independent of #44090'sLazyStringcoercion injson_error_response— correct in either merge order.Also removed:
USER_MISSING_ERRandviews/core.py's duplicatePARAMETER_MISSING_ERR, which had no consumers anywhere (dead constants).Review round (Codex, python lens): the package tripwire was rewritten from a text scan to an AST scanner — the substring approach had a false-positive class (
"gettext as __"also matcheslazy_gettext as __) and blind spots (annotated assignments, baregettext(...), other aliases, class-body constants, docstring text). The AST scanner resolves each module's actual Babel import bindings and inspectsAssign/AnnAssignat import-time scopes only. It immediately caught a fourth real offender the old scan could not see:CertificateException's class-level default message (exceptions.pybinds_to eager gettext). The default is now translated at construction — explicit messages, causes, and error types preserved — andparse_ssl_certdrops its redundant untranslated literal so the translated default is reachable. No consumer reads the class attribute or the exception's message (the DB schema validator emits its own).BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
N/A — i18n behavior. Before: a French user got "The data source seems to have been deleted". After: "La source de données semble avoir été effacée".
TESTING INSTRUCTIONS
pytest tests/unit_tests/views/test_i18n_constants.py(27 tests) —LazyString(fails pre-fix);get_domainwith a sentinel catalog and asserts everystr()of the constant consults the live lookup — runs on every backend, no compiled catalog needed;CertificateException; pairs with fix(views): error-response bodies must carry their message #44090's lazy-in-request scan), and 20 classifier self-test cases pin the scanner's corners (lazy aliases not flagged, function bodies skipped, annotated assignments and class bodies caught, docstrings inert, class-local rebinding scoped);CertificateExceptionconstruction is pinned: default message translated exactly once, explicit message bypasses translation, cause and error type preserved.Reverting the production change flips the LazyString pins and the tripwire.
ADDITIONAL INFORMATION
🤖 Generated with Claude Code
https://claude.ai/code/session_01JRLEJS4mUqKBoPjSjviKUW