Repository navigation
fix(chart-data): roll back db.session after swallowed metadata-DB errors - #44653
alexandrusoare merged 9 commits into
Conversation
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #44653 +/- ##
==========================================
- Coverage 81.22% 81.14% -0.08%
==========================================
Files 2968 2963 -5
Lines 179399 179020 -379
Branches 41601 41203 -398
==========================================
- Hits 145719 145271 -448
- Misses 30970 31027 +57
- Partials 2710 2722 +12
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:
|
…andrusoare/fix/pending-rollback-chart-data-impersonation
…andrusoare/fix/pending-rollback-chart-data-impersonation
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
EnxDev
left a comment
EnxDev
left a comment
There was a problem hiding this comment.
The diagnosis holds up, and consolidating the four impersonation lookups into one is a good cleanup.
Left a few notes inline. The main ones: the cache rollbacks might sit better inside SupersetMetastoreCache, the retry catches more than it needs to, and the five rollback sites have no tests yet (codecov patch is at 29%).
One small thing on the description: the old find_user call had no try either, so a failing lookup already raised before this. The "raises instead of falling back" change is really a different exception type plus the retry, which is good news since nothing breaking needs to go in UPDATING.md.
| # substituted the email prefix into ``username``, so looking it up | ||
| # here as if it were still the login finds nothing whenever the two | ||
| # differ, silently leaving the subject unset. | ||
| if email := database.get_impersonation_email(): |
There was a problem hiding this comment.
This fixes the prefix bug, but the updated test mocks get_impersonation_email on a MagicMock, so nothing would catch it coming back.
Worth adding a test with a real Database, IMPERSONATE_WITH_EMAIL_PREFIX on, login alice and email alice.doe@example.org, asserting subject is the full email?
![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 #7d49e8
Actionable Suggestions - 3
-
superset/models/helpers.py - 1
- Conditional rollback poisons session · Line 3820-3821
-
tests/unit_tests/extensions/metastore_cache_test.py - 2
- Missing reraise assertion · Line 57-57
- Missing reraise assertion · Line 73-73
Additional Suggestions - 6
-
tests/unit_tests/models/core_test.py - 1
-
Test setup duplication · Line 851-920The three new tests repeat identical setup: `Database(database_name="my_db", sqlalchemy_uri="trino://", impersonate_user=True)` and the `get_username` patch (lines 867-871, 893-897, 913-917). A shared helper would remove the duplication and keep the email-prefix cases in sync.
-
-
superset/utils/database.py - 1
-
Inline imports lack cycle comment · Line 155-159These four function-level imports carry only a `pylint: disable` marker. BITO rule 12745 allows inline imports solely to break circular dependencies and requires an explanatory comment naming the cycle. None is present. If `superset.models.core` -> `superset.utils.database` (models/core.py:91) is the cycle, document it; otherwise hoist the imports to module top.
-
-
tests/unit_tests/utils/rls_test.py - 1
-
Weak assertion in parse-failure block · Line 261-266The second block discards `collect_rls_predicates_for_sql`'s return value and asserts only the rollback side effect, so this block alone would not catch a changed parse-failure fallback (e.g. returning an empty list) under a mocked `db`. Per rule 9460, assert the computed value too: capture the result and compare it to the user-42 sentinel, mirroring the first block.
-
-
superset/utils/rls.py - 1
-
Rollback may discard pending writes · Line 338-339The rollback on any `SQLAlchemyError` mirrors the session-poisoning concern in `find_user_for_impersonation`, but that helper deliberately narrows rollback to `PendingRollbackError` because a blanket rollback can silently discard the caller's uncommitted session writes (superset/utils/database.py:140). If `collect_rls_predicates_for_sql` ever runs while `db.session` holds pending writes, an unrelated IntegrityError here would discard them. Worth confirming the read-only assumption. ([CWE-460](https://cwe.mitre.org/data/definitions/460.html))
-
-
tests/unit_tests/extensions/metastore_cache_test.py - 1
-
Untyped mock variable · Line 52-52Per the repo rule requiring explicit annotations for mock variables in test files, `session = mocker.patch("superset.extensions.metastore_cache.db").session` (also lines 68 and 84) is untyped; mypy infers `Any` here, so a typo like `session.rollbck` would not be caught. Annotate as `session: MagicMock` with `from unittest.mock import MagicMock`.
-
-
superset/models/helpers.py - 1
-
Blind exception catch in RLS retry · Line 3813-3813Catching a blind `Exception` at line 3813 is flagged by BLE001. Consider catching `SQLAlchemyError` explicitly and adding a separate `except Exception` for the fail-closed path, or add `# noqa: BLE001` if the broad catch is intentional.
-
Review Details
-
Files reviewed - 19 · Commit Range:
47aa423..43c79b2- superset/common/utils/query_cache_manager.py
- superset/db_engine_specs/gsheets.py
- superset/db_engine_specs/snowflake.py
- superset/db_engine_specs/starrocks.py
- superset/extensions/metastore_cache.py
- superset/jinja_context.py
- superset/models/core.py
- superset/models/helpers.py
- superset/translations/messages.pot
- superset/utils/database.py
- superset/utils/rls.py
- tests/unit_tests/db_engine_specs/test_gsheets.py
- tests/unit_tests/db_engine_specs/test_starrocks.py
- tests/unit_tests/extensions/metastore_cache_test.py
- tests/unit_tests/jinja_context_test.py
- tests/unit_tests/models/core_test.py
- tests/unit_tests/models/helpers_rls_rollback_test.py
- tests/unit_tests/utils/rls_test.py
- tests/unit_tests/utils/test_database.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
EnxDev
left a comment
EnxDev
left a comment
There was a problem hiding this comment.
Second pass, on top of my earlier review.
Everything from last time is in: the rollback lives in SupersetMetastoreCache.get/set now, the retry only catches PendingRollbackError, the rls/jinja/helpers sites only roll back on a DB error, and every site has a test that asserts the rollback. The gsheets test goes through a real Database and would fail on the old lookup. CI is green. LGTM from my side.
One small gap left inline on Snowflake. The description still says a failed lookup "now raises instead of falling back", which the old code never did. Might be worth a tweak so nobody goes hunting for an UPDATING.md entry.
Code Review Agent Run #da509dActionable 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 |
|
Bito Automatic Review Skipped – PR Already Merged |
Native run of scripts/translations/babel_update.sh after the second backport pass. Picks up the parameterized annotation-chart query-context message (#44584) and the impersonation-lookup error (#44653), whose master-side messages.pot hunks conflicted and were resolved to 7.0's side, and propagates every newly backported msgid into the .po catalogs.
SUMMARY
Chart-data requests could fail with
PendingRollbackErrorfromsecurity_manager.find_user()inDatabase._get_sqla_engine().That lookup isn't the cause. Earlier steps in the same request catch a DB error, treat it as non-fatal, and carry on without rolling back — leaving
db.sessionin a failed transaction. Every later statement on that session then fails withPendingRollbackError, so whoever reads it next takes the blame. Guarding the lookup alone would just move the error to the next reader.Fix — repair the session where it breaks:
SupersetMetastoreCache.set/getroll back and re-raise, matching whataddandcompare_and_setalready do. This covers every metastore-cache caller rather than individual call sites, and leaves a Redis-backed cache untouched.utils/rls.py,models/helpers.pyandjinja_context.pyroll back on a swallowed DB error. This completes fix(rls): roll back db.session after RLS failure in get_from_clause #43883, which fixed the same bug in a sibling function and missed these.Also: the impersonation email lookup existed in four copies (core plus StarRocks, Snowflake, gsheets), each able to fail the same way. Consolidated into
find_user_for_impersonation()behind twoDatabasemethods, so enginespecs no longer do their own lookup. It rolls back and retries once on
PendingRollbackError; any otherSQLAlchemyErrordescribes the lookup's own failure and propagates untouched.Two behaviour changes worth flagging:
SupersetErrorExceptioninstead of the rawPendingRollbackError, and retries once first. It already raised before this PR, so there's nothing forUPDATING.md. It doesn't fall back to the un-resolved login (what the ticket proposed) — that value is the identity the analytic DB connects as, so falling back could query as the wrong principal.subjectwas silently left unset whenever login and prefix differed.BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
TESTING INSTRUCTIONS
ADDITIONAL INFORMATION