You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@superset.apache.org by as...@apache.org on 2026/09/21 15:59:34 UTC
(superset) 01/01: initial work
This is an automated email from the ASF dual-hosted git repository.
alexandrusoare pushed a commit to branch alexandrusoare/fix/pending-rollback-chart-data-impersonation
in repository https://gitbox.apache.org/repos/asf/superset.git
commit 47aa423898cfba5d7d48f79982d5ff8f07ad44bb
Author: alexandrusoare <al...@gmail.com>
AuthorDate: Mon Sep 21 18:59:00 2026 +0300
initial work
---
superset/common/query_context_processor.py | 10 +++++
superset/common/utils/query_cache_manager.py | 14 +++++++
superset/jinja_context.py | 7 +++-
superset/models/core.py | 3 +-
superset/models/helpers.py | 7 ++++
superset/utils/database.py | 56 ++++++++++++++++++++++++++++
superset/utils/rls.py | 6 +++
7 files changed, 101 insertions(+), 2 deletions(-)
diff --git a/superset/common/query_context_processor.py b/superset/common/query_context_processor.py
index 2c27fb9fb06..042b1fc678d 100644
--- a/superset/common/query_context_processor.py
+++ b/superset/common/query_context_processor.py
@@ -28,6 +28,7 @@ from flask import current_app
from flask_babel import gettext as _
from pandas.api.types import infer_dtype
+from superset import db
from superset.common.chart_data import ChartDataResultFormat
from superset.common.chart_data_timing import (
QueryAcquisitionResult,
@@ -204,6 +205,11 @@ class QueryContextProcessor:
self._force_marker_key(nonce, cache_key)
)
except Exception: # noqa: BLE001 pylint: disable=broad-except
+ # Under `SupersetMetastoreCache` this read is a metadata-DB SELECT,
+ # so roll back before degrading to "absent": this runs before any
+ # engine is built, and leaving db.session in "pending rollback"
+ # state would fail the recompute we are about to force.
+ db.session.rollback() # pylint: disable=consider-using-transaction
logger.warning("Force-nonce marker read failed; forcing recompute")
return True
return marker is None
@@ -241,6 +247,10 @@ class QueryContextProcessor:
"Force-nonce marker write reported failure; may recompute once more"
)
except Exception: # noqa: BLE001 pylint: disable=broad-except
+ # Same reasoning as the marker read: a swallowed metadata-DB write
+ # failure must not leave db.session poisoned for the rest of this
+ # request.
+ db.session.rollback() # pylint: disable=consider-using-transaction
logger.warning("Force-nonce marker write failed; may recompute once more")
def get_df_payload_result(
diff --git a/superset/common/utils/query_cache_manager.py b/superset/common/utils/query_cache_manager.py
index 1a7178a7d4f..ae2c1fdf00d 100644
--- a/superset/common/utils/query_cache_manager.py
+++ b/superset/common/utils/query_cache_manager.py
@@ -24,6 +24,7 @@ from flask import current_app, g, has_request_context
from flask_caching import Cache
from pandas import DataFrame
+from superset import db
from superset.common.db_query_status import QueryStatus
from superset.constants import CacheRegion
from superset.exceptions import CacheLoadError
@@ -161,6 +162,12 @@ class QueryCacheManager:
region=region,
)
except Exception as ex: # pylint: disable=broad-except
+ # `self.set` writes through the cache backend, which is a
+ # metadata-DB write under `SupersetMetastoreCache`. Roll back so a
+ # failed write doesn't poison the rest of this request — the caller
+ # carries on after this (e.g. event logging, or a second query
+ # object in the same `queries` loop).
+ db.session.rollback() # pylint: disable=consider-using-transaction
logger.exception(ex)
if not self.error_message:
self.error_message = str(ex)
@@ -188,6 +195,13 @@ class QueryCacheManager:
# A cache backend outage (e.g. Redis connection/timeout errors)
# should not surface as an error to the caller: treat it the
# same as a cache miss and fall through to querying live data.
+ #
+ # With `SupersetMetastoreCache` this read is a metadata-DB SELECT,
+ # so a caught DB error can leave db.session in "pending rollback"
+ # state. Falling through to querying live data is exactly the path
+ # that would then fail on an unrelated query, attributing the fault
+ # to whatever ran after rather than to this lookup.
+ db.session.rollback() # pylint: disable=consider-using-transaction
logger.warning("Error reading cache: %s", error_msg_from_exception(ex))
cache_value = None
diff --git a/superset/jinja_context.py b/superset/jinja_context.py
index c78d78a98a5..a47ce500d38 100644
--- a/superset/jinja_context.py
+++ b/superset/jinja_context.py
@@ -34,7 +34,7 @@ from sqlalchemy.engine.interfaces import Dialect
from sqlalchemy.sql.expression import bindparam
from sqlalchemy.types import String
-from superset import security_manager
+from superset import db, security_manager
from superset.commands.dataset.exceptions import DatasetNotFoundError
from superset.common.utils.time_range_utils import get_since_until_from_time_range
from superset.constants import LRU_CACHE_MAX_SIZE, NO_TIME_RANGE
@@ -295,6 +295,11 @@ class ExtraCache:
self.cache_key_wrapper(json.dumps(user_roles))
return user_roles
except Exception: # pylint: disable=broad-except
+ # `get_user_roles()` lazy-loads roles from db.session, so a caught
+ # DB error can leave it in "pending rollback" state. This runs
+ # during SQL templating, upstream of the engine build that would
+ # otherwise inherit the failed transaction.
+ db.session.rollback() # pylint: disable=consider-using-transaction
return None
def current_user_rls_rules(self) -> list[str] | None:
diff --git a/superset/models/core.py b/superset/models/core.py
index 5712a306f6a..24ebfd5c735 100755
--- a/superset/models/core.py
+++ b/superset/models/core.py
@@ -87,6 +87,7 @@ from superset.superset_typing import (
from superset.utils import cache as cache_util, core as utils, json
from superset.utils.backports import StrEnum
from superset.utils.core import get_query_source_from_request, get_username
+from superset.utils.database import find_user_for_impersonation
from superset.utils.oauth2 import (
check_for_oauth2,
get_oauth2_access_token,
@@ -663,7 +664,7 @@ class Database(CoreDatabase, AuditMixinNullable, ImportExportMixin): # pylint:
effective_username = self.get_effective_user(sqlalchemy_url)
if effective_username and is_feature_enabled("IMPERSONATE_WITH_EMAIL_PREFIX"):
- user = security_manager.find_user(username=effective_username)
+ user = find_user_for_impersonation(effective_username)
if user and user.email:
effective_username = user.email.split("@")[0]
diff --git a/superset/models/helpers.py b/superset/models/helpers.py
index fe1746440ac..fc434ed3d77 100644
--- a/superset/models/helpers.py
+++ b/superset/models/helpers.py
@@ -3791,6 +3791,13 @@ class ExploreMixin: # pylint: disable=too-many-public-methods
for table in statement.tables
)
except Exception: # pylint: disable=broad-except
+ # This retry queries db.session again and rebuilds an
+ # engine, so it can re-poison the session the outer handler
+ # just rolled back. Roll back again: failing closed below
+ # raises QueryObjectValidationError, which callers catch
+ # and carry on from, and the continue-path keeps running
+ # this query outright.
+ db.session.rollback() # pylint: disable=consider-using-transaction
rls_required = True
if rls_required:
raise QueryObjectValidationError(
diff --git a/superset/utils/database.py b/superset/utils/database.py
index b0711fd71dd..be9668982f5 100644
--- a/superset/utils/database.py
+++ b/superset/utils/database.py
@@ -20,11 +20,15 @@ import logging
from typing import Any, TYPE_CHECKING
from flask import current_app as app
+from flask_babel import gettext as __
+from sqlalchemy.exc import SQLAlchemyError
from sqlalchemy.sql import compiler
from superset.constants import EXAMPLES_DB_UUID
if TYPE_CHECKING:
+ from flask_appbuilder.security.sqla.models import User
+
from superset.connectors.sqla.models import Database
logging.getLogger("MARKDOWN").setLevel(logging.INFO)
@@ -123,6 +127,58 @@ def warm_and_release_connection(instance: Any, *relationships: str) -> None:
session.expire_on_commit = True
+def find_user_for_impersonation(username: str) -> User | None:
+ """
+ Resolve the login backing an impersonated database session.
+
+ ``find_user`` is a metadata-DB read, so it inherits any failed transaction
+ left behind earlier in the request and reports ``PendingRollbackError``
+ instead of the original fault — blaming this lookup for an unrelated
+ failure. Roll back and retry once so a poisoned session doesn't cost us the
+ lookup.
+
+ The resolved value becomes the identity the analytic database connects as,
+ so a lookup that still fails must not degrade to the un-resolved login:
+ that would silently query as a different principal than the one being
+ impersonated. Raise instead.
+
+ :param username: the Superset login to resolve
+ :return: the matching user, or ``None`` if no such login exists
+ :raises SupersetErrorException: if the lookup fails even after a rollback
+ """
+ # pylint: disable=import-outside-toplevel
+ from superset import db
+ from superset.errors import ErrorLevel, SupersetError, SupersetErrorType
+ from superset.exceptions import SupersetErrorException
+ from superset.extensions import security_manager
+
+ try:
+ return security_manager.find_user(username=username)
+ except SQLAlchemyError:
+ logger.warning(
+ "Impersonation lookup for %s failed on a broken transaction; "
+ "rolling back and retrying once.",
+ username,
+ exc_info=True,
+ )
+ db.session.rollback() # pylint: disable=consider-using-transaction
+
+ try:
+ return security_manager.find_user(username=username)
+ except SQLAlchemyError as ex:
+ raise SupersetErrorException(
+ SupersetError(
+ message=__(
+ "Could not resolve the user to impersonate on the database "
+ "connection. The query was not run, because running it "
+ "would have connected as the wrong user."
+ ),
+ error_type=SupersetErrorType.GENERIC_DB_ENGINE_ERROR,
+ level=ErrorLevel.ERROR,
+ )
+ ) from ex
+
+
def apply_mariadb_ddl_fix() -> None:
"""
Fix MariaDB "NO CYCLE" syntax issue - MariaDB uses "NOCYCLE" (no space).
diff --git a/superset/utils/rls.py b/superset/utils/rls.py
index 3de35db1636..8977385e9e7 100644
--- a/superset/utils/rls.py
+++ b/superset/utils/rls.py
@@ -283,6 +283,12 @@ def collect_rls_predicates_for_sql(
}
)
except Exception:
+ # The block above is not only SQL parsing: `get_predicates_for_table`
+ # queries `db.session` and `get_default_catalog()` builds an engine, so
+ # a caught DB error can leave db.session in "pending rollback" state,
+ # which would poison unrelated queries later in this request.
+ db.session.rollback() # pylint: disable=consider-using-transaction
+
# If we can't parse the SQL, we can't tell which (if any) RLS
# predicates would apply, so we can't contribute a meaningful cache
# key component. Returning an empty list here would make every