sadpandajoe commented on code in PR #43499: URL: https://github.com/apache/superset/pull/43499#discussion_r4451991299
########## 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: f72b6e9c104a +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 = "f72b6e9c104a" Review Comment: Master now has `b8f3a2c91d64` (the Databricks connector URI migration) whose `down_revision` is also `f72b6e9c104a`, so with this parent the tree has two heads and `superset db upgrade` fails with multiple heads once this merges. Could you rebase and re-parent this onto `b8f3a2c91d64`? ########## superset/models/slice.py: ########## @@ -232,6 +234,8 @@ def _display_datasource(self) -> SqlaTable | SemanticView | None: """ if self.datasource_type == utils.DatasourceType.SEMANTIC_VIEW: return self.semantic_view + if self.datasource_type == utils.DatasourceType.SAVEDQUERY: + return self.resolved_datasource Review Comment: `datasource_name_text` and `datasource_url` both reach this branch, and `resolved_datasource` falls through to `DatasourceDAO.get_datasource()` for a saved query, which is a fresh SELECT per call (including the SQL text column) rather than an identity-map hit like `semantic_view`. A chart list page with 100 saved-query charts would add about 200 extra queries during serialization. Would a type-guarded relationship like `semantic_view`, or a batched lookup, work here? ########## superset/security/manager.py: ########## @@ -4888,6 +4893,223 @@ 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", + default_catalog: Optional[str] = None, + ) -> 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. + + ``default_catalog`` lets callers that resolve many saved queries + against one database look the default catalog up once (it can be a + warehouse round trip); when omitted it is resolved lazily, and only + for a saved query whose own ``catalog`` is unset. + """ + database_name = database.database_name + catalog = saved_query.catalog or default_catalog + if catalog is None and not saved_query.catalog: + catalog = database.get_default_catalog() + return { + "perm": f"[{database_name}].[{saved_query.name}](id:{saved_query.id})", + "catalog_perm": self.get_catalog_perm(database_name, catalog), + "schema_perm": self.get_schema_perm( + database_name, catalog, saved_query.schema + ), + } + + def _sync_saved_query_chart_perms( + self, + connection: Connection, + saved_query: "SavedQuery", + database: "Database", + default_catalog: Optional[str] = None, + ) -> 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, Review Comment: The `datasource_type == SAVEDQUERY` predicate on this UPDATE is what keeps a table chart with the same numeric `datasource_id` untouched, but the resync tests only contain saved-query charts (the same-id table chart appears only in the `after_delete` test). If this predicate were dropped, every resync test would still pass while a table chart sharing the id would have its `perm`/`catalog_perm`/`schema_perm` overwritten. Could you add a case with a same-id table chart that changes the saved query's schema/catalog and asserts the table chart's perms are unchanged? -- 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]
