From cdca6f7fdccaa7834a74b61456d725d017f8bc5c Mon Sep 17 00:00:00 2001 From: Evan Rusackas Date: Wed, 20 May 2026 21:19:19 -0700 Subject: [PATCH] fix(sqllab): keep saved-query list working when Jinja `dataset(id)` references a deleted dataset (#39703) Co-authored-by: Claude Opus 4.7 --- superset/models/sql_lab.py | 14 +++++- tests/unit_tests/models/sql_lab_test.py | 57 +++++++++++++++++++------ 2 files changed, 58 insertions(+), 13 deletions(-) diff --git a/superset/models/sql_lab.py b/superset/models/sql_lab.py index 9f528da266a..a8454ebc88d 100644 --- a/superset/models/sql_lab.py +++ b/superset/models/sql_lab.py @@ -53,7 +53,11 @@ from superset_core.queries.models import ( ) from superset import security_manager -from superset.exceptions import SupersetParseError, SupersetSecurityException +from superset.exceptions import ( + SupersetException, + SupersetParseError, + SupersetSecurityException, +) from superset.explorables.base import TimeGrainDict from superset.jinja_context import BaseTemplateProcessor, get_template_processor from superset.models.helpers import ( @@ -99,6 +103,14 @@ class SqlTablesMixin: # pylint: disable=too-few-public-methods ) except (SupersetSecurityException, SupersetParseError, TemplateError): return [] + except SupersetException as ex: + # Jinja macros such as ``{{ dataset(id) }}`` or ``{{ metric(...) }}`` + # may reference resources that no longer exist (e.g. a deleted + # dataset). Surfacing the failure here would break list endpoints + # that include ``sql_tables`` in their payload, hiding every saved + # query from the user. Treat it as a parse failure instead. + logger.warning("Unable to extract tables from SQL via Jinja: %s", ex) + return [] class Query( diff --git a/tests/unit_tests/models/sql_lab_test.py b/tests/unit_tests/models/sql_lab_test.py index c359e4f630b..1018b58a379 100644 --- a/tests/unit_tests/models/sql_lab_test.py +++ b/tests/unit_tests/models/sql_lab_test.py @@ -21,8 +21,14 @@ from flask_appbuilder import Model from jinja2.exceptions import TemplateError from pytest_mock import MockerFixture +from superset.commands.dataset.exceptions import DatasetNotFoundError from superset.errors import ErrorLevel, SupersetError, SupersetErrorType -from superset.exceptions import SupersetParseError, SupersetSecurityException +from superset.exceptions import ( + SupersetParseError, + SupersetSecurityException, + SupersetTemplateException, +) +from superset.models import sql_lab as sql_lab_module from superset.models.sql_lab import Query, SavedQuery @@ -34,34 +40,61 @@ from superset.models.sql_lab import Query, SavedQuery ], ) @pytest.mark.parametrize( - "exception", + ("exception", "should_warn"), [ - SupersetSecurityException( - SupersetError( - error_type=SupersetErrorType.QUERY_SECURITY_ACCESS_ERROR, - message="", - level=ErrorLevel.ERROR, - ) + # Original silent handler — security/parse/template errors are + # expected during list rendering and produce no log noise. + ( + SupersetSecurityException( + SupersetError( + error_type=SupersetErrorType.QUERY_SECURITY_ACCESS_ERROR, + message="", + level=ErrorLevel.ERROR, + ) + ), + False, ), - SupersetParseError( - sql="INVALID SQL", - message="Invalid SQL syntax", + ( + SupersetParseError( + sql="INVALID SQL", + message="Invalid SQL syntax", + ), + False, ), - TemplateError, + (TemplateError, False), + # ``{{ dataset(id) }}`` referencing a deleted dataset previously + # bubbled up through ``sql_tables`` and broke saved-query list + # endpoints (see issue #32771). The new handler swallows it but + # logs a warning so the underlying breakage is still observable — + # pinned here so a future refactor that collapses the case into + # the silent handler fails this test. + (DatasetNotFoundError("Dataset 1 not found!"), True), + (SupersetTemplateException("Template rendering failed"), True), ], ) def test_sql_tables_mixin_sql_tables_exception( klass: type[Model], exception: Exception, + should_warn: bool, mocker: MockerFixture, ) -> None: mocker.patch( "superset.models.sql_lab.process_jinja_sql", side_effect=exception, ) + warning_spy = mocker.spy(sql_lab_module.logger, "warning") assert klass(sql="SELECT 1", database=MagicMock()).sql_tables == [] + if should_warn: + assert warning_spy.call_count == 1, ( + f"{type(exception).__name__} should hit the warning-logging " + "handler; if this fails, the case was likely collapsed into " + "the silent first-handler clause." + ) + else: + warning_spy.assert_not_called() + @pytest.mark.parametrize( "klass",