sadpandajoe commented on code in PR #43499:
URL: https://github.com/apache/superset/pull/43499#discussion_r4367032564
##########
superset/commands/chart/utils.py:
##########
@@ -36,18 +38,42 @@
def validate_chart_datasource_type(datasource_type: str) -> None:
"""Accept persistent chart sources with names and type-aware access
policies.
- SQL Lab queries are registered datasource types but are not persistent
- chart sources; accepting all DatasourceDAO registrations would include
them.
+ A raw SQL Lab ``query`` is a registered datasource type but not a
+ persistent chart source, so it's excluded here; accepting all
+ DatasourceDAO registrations would include it. A ``saved_query`` is
+ explicitly persisted and named by the user, so it's accepted alongside
+ ``table``/``semantic_view``.
"""
if (
datasource_type == DatasourceType.SEMANTIC_VIEW
and not is_semantic_layers_enabled()
):
raise ValidationError(SemanticLayersDisabledError.message)
- if datasource_type not in (DatasourceType.TABLE,
DatasourceType.SEMANTIC_VIEW):
+ if datasource_type not in (
+ DatasourceType.TABLE,
+ DatasourceType.SEMANTIC_VIEW,
+ DatasourceType.SAVEDQUERY,
Review Comment:
Allowing `saved_query` here lets chart create/update succeed, but the chart
can't be rendered: `SavedQuery` still has no `columns`, `uid`, or
`get_extra_cache_keys`, so `/api/v1/chart/<id>/data/` goes through
`_apply_granularity` (`datasource.columns`) and fails with an `AttributeError`
(500). Explore's access map also still doesn't know `saved_query`. Is the
render/explore path intended to land in this PR, or should creation stay
rejected until it does?
##########
superset/migrations/versions/2026-10-13_00-00_dfa683969f48_allow_saved_query_chart_datasource.py:
##########
@@ -0,0 +1,83 @@
+# 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.
+"""allow saved_query chart datasource
+
+Revision ID: dfa683969f48
+Revises: d623a0cb6bb0
+Create Date: 2026-10-13 00:00:00.000000
+
+"""
+
+import logging
+
+import sqlalchemy as sa
+from alembic import op
+from sqlalchemy.orm import declarative_base
+
+from superset import db
+
+# revision identifiers, used by Alembic.
+revision = "dfa683969f48"
+down_revision = "d623a0cb6bb0"
+
+Base = declarative_base()
+
+logger = logging.getLogger("alembic.env")
+
+
+class Slice(Base): # type: ignore
+ __tablename__ = "slices"
+
+ id = sa.Column(sa.Integer, primary_key=True)
+ datasource_type = sa.Column(sa.String(200))
+
+
+def upgrade() -> None:
+ # Update chart datasource constraint to allow saved_query, matching
+ # CreateChartCommand/UpdateChartCommand.validate_chart_datasource_type
+ # (apache/superset#29697), which now accepts a saved_query as a valid,
+ # persistent chart datasource alongside table and semantic_view.
+ with op.batch_alter_table("slices") as batch_op:
+ batch_op.drop_constraint("ck_chart_datasource", type_="check")
+ batch_op.create_check_constraint(
+ "ck_chart_datasource",
+ "datasource_type in ('table', 'semantic_view', 'saved_query')",
+ )
+
+
+def downgrade() -> None:
+ # Any chart persisted with datasource_type == saved_query would violate
+ # the stricter constraint being restored below, so reassign those rows
+ # first (mirrors the cleanup in 7e67aecbf3f1_chart_ds_constraint.py).
+ bind = op.get_bind()
+ session = db.Session(bind=bind)
+ for slc in session.query(Slice).filter(Slice.datasource_type ==
"saved_query"):
+ logger.warning(
+ "downgrading slice.id = %s from datasource_type saved_query to
table",
+ slc.id,
+ )
+ slc.datasource_type = "table"
Review Comment:
On downgrade this only flips `datasource_type` to `table` and keeps
`datasource_id`, which is a saved query ID. If saved query 7 backs a chart and
an unrelated dataset 7 exists, the chart silently rebinds to that dataset (and
its perm columns still hold the saved-query strings). Would it be safer to
detach these charts (null the datasource id/perms) or refuse the downgrade
instead?
##########
superset/security/manager.py:
##########
@@ -4682,6 +4687,175 @@ def semantic_view_before_update(
.values(perm=new_perm)
)
+ def saved_query_before_update(
+ self,
+ mapper: Mapper,
+ connection: Connection,
+ target: "SavedQuery",
+ ) -> None:
+ """
+ Keep a saved query's dependent charts' denormalized permission
+ columns in sync when its database, catalog, or schema changes.
+
+ ``SavedQuery.perm``/``catalog_perm``/``schema_perm`` are computed
+ properties, not persisted columns (unlike ``SqlaTable`` and
+ ``SemanticView``, which each get a PVM-rename dance in
+ ``dataset_before_update``/``semantic_view_before_update``), so there
+ is no FAB ViewMenu here to rename -- saved queries never get a
+ ``datasource_access`` PVM of their own. But ``Slice``'s
+ ``before_insert``/``before_update`` listener (``set_related_perm``)
+ copies those computed strings onto the chart at save time, and
+ nothing re-copies them if the saved query moves afterwards: a chart
+ would otherwise keep matching stale schema/catalog grants
+ indefinitely. This is the dependent-chart half of
+ ``semantic_view_before_update``, without the PVM-rename half.
+ """
+ # pylint: disable=import-outside-toplevel
+ from superset.models.sql_lab import SavedQuery
+
+ sq_table = SavedQuery.__table__ # pylint: disable=no-member
+ current = connection.execute(
+ sq_table.select().where(sq_table.c.id == target.id)
+ ).one()
+
+ if (
+ current.db_id == target.db_id
+ and current.catalog == target.catalog
+ and current.schema == target.schema
+ ):
+ return
+
+ if target.database is None:
+ return
+
+ # Skip the perm computation (which may hit the warehouse via
+ # ``get_default_catalog``) when no chart references this saved query.
+ # pylint: disable=import-outside-toplevel
+ from superset.models.slice import Slice
+
+ chart_table = Slice.__table__ # pylint: disable=no-member
+ has_charts = connection.execute(
+ select(chart_table.c.id)
+ .where(
+ chart_table.c.datasource_type == DatasourceType.SAVEDQUERY,
+ chart_table.c.datasource_id == target.id,
+ )
+ .limit(1)
+ ).first()
+ if has_charts is None:
+ return
+
+ self._sync_saved_query_chart_perms(connection, target, target.database)
+
+ def _saved_query_chart_perm_values(
+ self,
+ saved_query: "SavedQuery",
+ database: "Database",
+ ) -> dict[str, Optional[str]]:
+ """
+ Compute the denormalized ``perm``/``catalog_perm``/``schema_perm`` a
+ chart backed by ``saved_query`` should carry, given ``database``.
+
+ ``database`` is passed explicitly rather than read off the saved query
+ because callers run inside a flush: ``SavedQuery.before_update`` may
+ have just reloaded it from a changed ``db_id``, and on a database
+ rename (``database_after_update``) the new ``database_name`` only
+ exists on the in-memory target.
+ """
+ database_name = database.database_name
+ return {
+ "perm":
f"[{database_name}].[{saved_query.name}](id:{saved_query.id})",
+ "catalog_perm": self.get_catalog_perm(database_name,
saved_query.catalog),
+ "schema_perm": self.get_schema_perm(
+ database_name,
+ saved_query.catalog or database.get_default_catalog(),
+ saved_query.schema,
+ ),
+ }
+
+ def _sync_saved_query_chart_perms(
+ self,
+ connection: Connection,
+ saved_query: "SavedQuery",
+ database: "Database",
+ ) -> None:
+ """Re-copy ``saved_query``'s perm strings onto its dependent charts."""
+ # pylint: disable=import-outside-toplevel
+ from superset.models.slice import Slice
+
+ chart_table = Slice.__table__ # pylint: disable=no-member
+ connection.execute(
+ chart_table.update()
+ .where(
+ chart_table.c.datasource_type == DatasourceType.SAVEDQUERY,
+ chart_table.c.datasource_id == saved_query.id,
+ )
+ .values(**self._saved_query_chart_perm_values(saved_query,
database))
+ )
+
+ def _update_saved_query_chart_perms(
+ self,
+ connection: Connection,
+ target: "Database",
+ ) -> None:
+ """
+ Rewrite the denormalized perm columns on every chart backed by one of
+ ``target``'s saved queries after the database is renamed.
+
+ ``_update_vm_datasources_access`` only walks datasets, so saved-query
+ charts would otherwise keep their ``[old_name].[...]`` strings and
+ match a schema or catalog grant on an unrelated connection that later
+ takes the old name.
+ """
+ # pylint: disable=import-outside-toplevel
+ from superset.models.sql_lab import SavedQuery
+
+ saved_queries = (
+ self.session.query(SavedQuery).filter(SavedQuery.db_id ==
target.id).all()
Review Comment:
This loads every saved query on the renamed database and recomputes chart
perms for each one, even when no chart references it (unlike
`saved_query_before_update`, which checks for dependent charts first). For
engines like Databricks without an explicit default catalog,
`get_default_catalog()` can hit the warehouse per saved query, so renaming a
connection with many ordinary SQL Lab saved queries does that many round trips
inside the flush. Could this restrict to saved queries that have charts and
resolve the default catalog once?
##########
tests/unit_tests/commands/chart/test_access_checks.py:
##########
@@ -100,6 +105,161 @@ def
test_create_chart_command_allowed_when_access_passes() -> None:
command.validate() # should not raise
+def test_create_chart_command_supports_saved_query_datasource() -> None:
Review Comment:
The command-level tests force `all_query_access=True` (and the denial test
injects an unrelated access exception), so removing the
`raise_for_saved_query_ownership` call from
`CreateChartCommand`/`UpdateChartCommand` would keep them green. Could you add
a case where `raise_for_access` passes but the caller isn't the creator and
lacks `all_query_access`, asserting `ChartForbiddenError` from both
`validate()` calls (and success for the owner)?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
