fix(charts): don't clobber the datasource_type-required error

Skip the non-table datasource_type guard entirely when datasource_type
is empty, so the existing "Datasource type is required" message isn't
overwritten by "Datasource type is invalid" for the same field key.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

superset/commands/chart/create.py

@@ -32,11 +32,13 @@ from superset.commands.chart.exceptions import (
     DashboardsForbiddenError,
     DashboardsNotFoundValidationError,
 )
+from superset.commands.exceptions import DatasourceTypeInvalidError
 from superset.commands.utils import get_datasource_by_id, populate_subjects
 from superset.daos.chart import ChartDAO
 from superset.daos.dashboard import DashboardDAO
 from superset.exceptions import SupersetSecurityException
 from superset.utils import json
+from superset.utils.core import DatasourceType
 from superset.utils.decorators import on_error, transaction
 
 logger = logging.getLogger(__name__)
@@ -71,6 +73,16 @@ class CreateChartCommand(CreateMixin, BaseCommand):
 
         # Validate/Populate datasource
         try:
+            # Slice.datasource only ever resolves the ``table`` relationship
+            # (see Slice.datasource in superset/models/slice.py), so a chart
+            # pointed at any other datasource_type would "create"
+            # successfully but could never actually render. Reject those
+            # up front instead of failing later -- either at this lookup
+            # (SavedQuery/Query have no ``.name`` attribute, so accessing it
+            # below raises an unhandled AttributeError) or silently, by
+            # producing a permanently broken chart.
+            if datasource_type != DatasourceType.TABLE:
+                raise DatasourceTypeInvalidError()
             datasource = get_datasource_by_id(datasource_id, datasource_type)
             self._properties["datasource_name"] = datasource.name
             security_manager.raise_for_access(datasource=datasource)

superset/commands/chart/update.py

@@ -35,6 +35,7 @@ from superset.commands.chart.exceptions import (
     DashboardsNotFoundValidationError,
     DatasourceTypeUpdateRequiredValidationError,
 )
+from superset.commands.exceptions import DatasourceTypeInvalidError
 from superset.commands.utils import (
     compute_subjects,
     get_datasource_by_id,
@@ -49,6 +50,7 @@ from superset.models.dashboard import Dashboard
 from superset.models.slice import Slice
 from superset.tags.models import ObjectType
 from superset.utils import json
+from superset.utils.core import DatasourceType
 from superset.utils.decorators import on_error, transaction
 from superset.versioning.changes.normalization import (
     register_matching_normalization_context,
@@ -221,8 +223,22 @@ class UpdateChartCommand(UpdateMixin, BaseCommand):
             exceptions.append(ex)
 
         # Validate/Populate datasource
-        if datasource_id is not None:
+        # An empty datasource_type was already flagged above via
+        # DatasourceTypeUpdateRequiredValidationError; skip this block so
+        # we don't clobber that message with DatasourceTypeInvalidError.
+        if datasource_id is not None and datasource_type:
             try:
+                # Slice.datasource only ever resolves the ``table``
+                # relationship (see Slice.datasource in
+                # superset/models/slice.py), so repointing a chart at any
+                # other datasource_type would "succeed" but leave the chart
+                # permanently unable to render. Reject those up front
+                # instead of failing later -- either at this lookup
+                # (SavedQuery/Query have no ``.name`` attribute, so
+                # accessing it below raises an unhandled AttributeError) or
+                # silently.
+                if datasource_type != DatasourceType.TABLE:
+                    raise DatasourceTypeInvalidError()
                 datasource = get_datasource_by_id(datasource_id, datasource_type)
                 self._properties["datasource_name"] = datasource.name
                 security_manager.raise_for_access(datasource=datasource)

tests/integration_tests/charts/api_tests.py

@@ -35,12 +35,14 @@ from superset.extensions import cache_manager, db, security_manager
 from superset.models.core import Database, FavStar, FavStarClassName
 from superset.models.dashboard import Dashboard
 from superset.models.slice import Slice
+from superset.models.sql_lab import SavedQuery
 from superset.reports.models import ReportSchedule, ReportScheduleType
 from superset.subjects.models import Subject
 from superset.subjects.types import SubjectType
 from superset.tags.models import ObjectType, Tag, TaggedObject, TagType
 from superset.utils import json
 from superset.utils.core import get_example_default_schema
+from superset.utils.database import get_example_database
 from tests.integration_tests.base_api_tests import ApiEditorsTestCaseMixin
 from tests.integration_tests.base_tests import (
     subjects_from_users,
@@ -660,6 +662,46 @@ class TestChartApi(ApiEditorsTestCaseMixin, InsertChartMixin, SupersetTestCase):
         response = json.loads(rv.data.decode("utf-8"))
         assert response == {"message": {"datasource_id": ["Datasource does not exist"]}}
 
+    def test_create_chart_from_saved_query_rejected_cleanly(self):
+        """
+        Chart API: creating a chart with datasource_type="saved_query" must
+        fail with a clean validation error, not the unhandled 500 "Fatal
+        error" reported in apache/superset#29697. Slice.datasource only
+        ever resolves the "table" relationship, so even a chart that
+        "created" successfully with this datasource_type could never
+        actually render -- "saved_query" is a real, existing row here
+        (not a bad ID), reproducing the original report exactly rather
+        than a not-found case.
+        """
+        self.login(ADMIN_USERNAME)
+        example_db = get_example_database()
+        saved_query = SavedQuery(
+            db_id=example_db.id,
+            label="issue-29697-repro",
+            schema=get_example_default_schema(),
+            sql="SELECT 1 AS value",
+        )
+        db.session.add(saved_query)
+        db.session.commit()
+        saved_query_id = saved_query.id
+
+        chart_data = {
+            "slice_name": "issue-29697-repro-chart",
+            "datasource_id": saved_query_id,
+            "datasource_type": "saved_query",
+            "viz_type": "table",
+        }
+        rv = self.post_assert_metric("/api/v1/chart/", chart_data, "post")
+
+        db.session.delete(db.session.query(SavedQuery).get(saved_query_id))
+        db.session.commit()
+
+        assert rv.status_code == 422
+        response = json.loads(rv.data.decode("utf-8"))
+        assert response == {
+            "message": {"datasource_type": ["Datasource type is invalid"]}
+        }
+
     @pytest.mark.usefixtures("load_world_bank_dashboard_with_slices")
     def test_create_chart_validate_user_is_dashboard_editor(self):
         """

tests/unit_tests/commands/chart/create_test.py

@@ -0,0 +1,154 @@
+# 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.
+"""Unit tests for CreateChartCommand.
+
+Regression coverage for apache/superset#29697: POST /api/v1/chart/ with
+datasource_type="saved_query" (or "query") crashes with an unhandled
+AttributeError -- reported to API clients as an opaque 500 "Fatal error" --
+because SavedQuery and Query models have no ``.name`` attribute, and because
+Slice.datasource only ever resolves a ``table``-typed datasource, so even a
+successfully created chart of another type could never actually render.
+"""
+
+import pytest
+from pytest_mock import MockerFixture
+
+from superset.commands.chart.create import CreateChartCommand
+from superset.commands.chart.exceptions import ChartForbiddenError, ChartInvalidError
+from superset.commands.exceptions import DatasourceTypeInvalidError
+from superset.errors import ErrorLevel, SupersetError, SupersetErrorType
+from superset.exceptions import SupersetSecurityException
+
+
+def _base_mocks(mocker: MockerFixture) -> None:
+    mocker.patch(
+        "superset.commands.chart.create.DashboardDAO.find_by_ids", return_value=[]
+    )
+    mocker.patch(
+        "superset.commands.chart.create.populate_subjects",
+        side_effect=lambda properties, exceptions: None,
+    )
+
+
+@pytest.mark.parametrize("datasource_type", ["saved_query", "query"])
+def test_create_chart_rejects_non_table_datasource_type(
+    mocker: MockerFixture, datasource_type: str
+) -> None:
+    """A chart can only ever query a table-backed datasource -- Slice.datasource
+    only ever resolves the ``table`` relationship, so any other type would
+    produce a chart that "creates" successfully but can never render.
+
+    The two types fail differently before this fix, which is exactly why
+    both are covered here:
+    - "saved_query": SavedQuery has no ``.name`` attribute, so validation
+      crashes with an unhandled AttributeError -- surfaced to API clients as
+      an opaque 500 "Fatal error" (apache/superset#29697).
+    - "query": Query *does* define a synthetic ``.name`` property (used for
+      CTAS table naming, not as a real display name), so this one doesn't
+      crash -- it silently "succeeds" and creates a chart with a nonsense
+      name and a datasource that Slice.datasource can never resolve.
+
+    ``get_datasource_by_id`` is mocked with ``spec=`` the real model classes
+    so accessing ``.name`` on the mock behaves exactly like the real ORM
+    objects do if the new guard doesn't stop the code from getting there;
+    ``raise_for_access`` is mocked to a no-op so nothing downstream masks
+    that behavior.
+    """
+    from superset.models.sql_lab import Query, SavedQuery
+
+    _base_mocks(mocker)
+    model_cls = SavedQuery if datasource_type == "saved_query" else Query
+    get_datasource_by_id = mocker.patch(
+        "superset.commands.chart.create.get_datasource_by_id",
+        return_value=mocker.MagicMock(spec=model_cls),
+    )
+    mocker.patch("superset.commands.chart.create.security_manager.raise_for_access")
+
+    with pytest.raises(ChartInvalidError) as exc_info:
+        CreateChartCommand(
+            {
+                "datasource_id": 11,
+                "datasource_type": datasource_type,
+                "slice_name": "some_name",
+                "viz_type": "table",
+            }
+        ).validate()
+
+    assert any(
+        isinstance(ex, DatasourceTypeInvalidError) for ex in exc_info.value._exceptions
+    )
+    # The invalid type must be rejected before ever touching the datasource
+    # lookup, not caught incidentally by some downstream failure.
+    get_datasource_by_id.assert_not_called()
+
+
+def test_create_chart_accepts_table_datasource(mocker: MockerFixture) -> None:
+    """The one supported datasource_type must keep working."""
+    _base_mocks(mocker)
+    datasource = mocker.MagicMock(name="table_datasource")
+    datasource.name = "my_table"
+    mocker.patch(
+        "superset.commands.chart.create.get_datasource_by_id",
+        return_value=datasource,
+    )
+    mocker.patch("superset.commands.chart.create.security_manager.raise_for_access")
+
+    cmd = CreateChartCommand(
+        {
+            "datasource_id": 11,
+            "datasource_type": "table",
+            "slice_name": "some_name",
+            "viz_type": "table",
+        }
+    )
+    cmd.validate()
+
+    assert cmd._properties["datasource_name"] == "my_table"
+
+
+def test_create_chart_datasource_access_denied_still_raises_forbidden(
+    mocker: MockerFixture,
+) -> None:
+    """The invalid-type guard must not shadow the existing access-denied path
+    for a legitimately table-typed datasource the user can't access."""
+    _base_mocks(mocker)
+    datasource = mocker.MagicMock()
+    datasource.name = "my_table"
+    mocker.patch(
+        "superset.commands.chart.create.get_datasource_by_id",
+        return_value=datasource,
+    )
+    mocker.patch(
+        "superset.commands.chart.create.security_manager.raise_for_access",
+        side_effect=SupersetSecurityException(
+            SupersetError(
+                error_type=SupersetErrorType.DATASOURCE_SECURITY_ACCESS_ERROR,
+                message="No access",
+                level=ErrorLevel.ERROR,
+            )
+        ),
+    )
+
+    with pytest.raises(ChartForbiddenError):
+        CreateChartCommand(
+            {
+                "datasource_id": 11,
+                "datasource_type": "table",
+                "slice_name": "some_name",
+                "viz_type": "table",
+            }
+        ).validate()

tests/unit_tests/commands/chart/update_test.py

@@ -17,8 +17,13 @@
 import pytest
 from pytest_mock import MockerFixture
 
-from superset.commands.chart.exceptions import ChartForbiddenError, ChartInvalidError
+from superset.commands.chart.exceptions import (
+    ChartForbiddenError,
+    ChartInvalidError,
+    DatasourceTypeUpdateRequiredValidationError,
+)
 from superset.commands.chart.update import UpdateChartCommand
+from superset.commands.exceptions import DatasourceTypeInvalidError
 from superset.errors import ErrorLevel, SupersetError, SupersetErrorType
 from superset.exceptions import SupersetSecurityException
 from superset.utils import json
@@ -238,3 +243,71 @@ def test_update_chart_query_context_without_datasource_is_allowed(
         1,
         {"query_context": query_context, "query_context_generation": True},
     ).validate()
+
+
+@pytest.mark.parametrize("datasource_type", ["saved_query", "query"])
+def test_update_chart_rejects_repointing_to_non_table_datasource(
+    mocker: MockerFixture, datasource_type: str
+) -> None:
+    """Repointing a chart's datasource_id must be rejected the same way
+    CreateChartCommand rejects it (apache/superset#29697): Slice.datasource
+    only ever resolves the ``table`` relationship, so repointing at a
+    saved_query or query datasource would "succeed" but leave the chart
+    permanently unable to render -- or, for saved_query specifically, crash
+    on SavedQuery's missing ``.name`` attribute before that point is even
+    reached. This is a regular (non-query-context) update, so it goes
+    through editorship + compute_subjects, unlike the query-context-only
+    tests above."""
+    find_by_id = mocker.patch("superset.commands.chart.update.ChartDAO.find_by_id")
+    find_by_id.return_value = mocker.MagicMock(id=1, tags=[], dashboards=[])
+    mocker.patch("superset.commands.chart.update.security_manager.raise_for_editorship")
+    mocker.patch(
+        "superset.commands.chart.update.compute_subjects",
+        side_effect=lambda model, properties, exceptions: None,
+    )
+    get_datasource_by_id = mocker.patch(
+        "superset.commands.chart.update.get_datasource_by_id"
+    )
+
+    with pytest.raises(ChartInvalidError) as exc_info:
+        UpdateChartCommand(
+            1, {"datasource_id": 11, "datasource_type": datasource_type}
+        ).validate()
+
+    assert any(
+        isinstance(ex, DatasourceTypeInvalidError) for ex in exc_info.value._exceptions
+    )
+    get_datasource_by_id.assert_not_called()
+
+
+def test_update_chart_missing_datasource_type_keeps_required_error(
+    mocker: MockerFixture,
+) -> None:
+    """When datasource_id is given without datasource_type, the response
+    must keep reporting DatasourceTypeUpdateRequiredValidationError
+    ("Datasource type is required") rather than having it overwritten by
+    DatasourceTypeInvalidError ("Datasource type is invalid") -- both
+    exceptions key their message under ``datasource_type``, and
+    normalized_messages() only keeps the last one written for a given key."""
+    find_by_id = mocker.patch("superset.commands.chart.update.ChartDAO.find_by_id")
+    find_by_id.return_value = mocker.MagicMock(id=1, tags=[], dashboards=[])
+    mocker.patch("superset.commands.chart.update.security_manager.raise_for_editorship")
+    mocker.patch(
+        "superset.commands.chart.update.compute_subjects",
+        side_effect=lambda model, properties, exceptions: None,
+    )
+    get_datasource_by_id = mocker.patch(
+        "superset.commands.chart.update.get_datasource_by_id"
+    )
+
+    with pytest.raises(ChartInvalidError) as exc_info:
+        UpdateChartCommand(1, {"datasource_id": 11}).validate()
+
+    assert any(
+        isinstance(ex, DatasourceTypeUpdateRequiredValidationError)
+        for ex in exc_info.value._exceptions
+    )
+    assert not any(
+        isinstance(ex, DatasourceTypeInvalidError) for ex in exc_info.value._exceptions
+    )
+    get_datasource_by_id.assert_not_called()