sadpandajoe commented on code in PR #43499:
URL: https://github.com/apache/superset/pull/43499#discussion_r4289664223
##########
superset/commands/chart/utils.py:
##########
@@ -33,15 +33,22 @@
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:
Accepting `saved_query` here means these charts get persisted, but
`Slice._display_datasource()` only resolves `table` and `semantic_view`
(`superset/models/slice.py:223-234`) and returns `table` otherwise. For a
saved-query chart that is `None`, so `datasource_name_text` and
`datasource_url` come back null from `/api/v1/chart/` and the chart list and
card show a blank datasource. Should the display resolver learn about saved
queries in this PR, or should the type stay rejected until it does?
##########
tests/unit_tests/models/sql_lab_test.py:
##########
@@ -219,6 +219,117 @@ def test_query_schema_perm_guards_unset_database() ->
None:
assert query.schema_perm == ""
[email protected](
+ (
+ "catalog",
+ "schema",
+ "database_name",
+ "default_catalog",
+ "expected",
+ ),
+ [
+ (None, "s1", "db", None, "[db].[s1]"),
+ ("cat", "s1", "db", None, "[db].[cat].[s1]"),
+ (None, "s1", "db", "defcat", "[db].[defcat].[s1]"),
+ ],
+)
+def test_saved_query_schema_perm(
+ catalog,
+ schema,
+ database_name,
+ default_catalog,
+ expected,
+) -> None:
+ """SavedQuery.schema_perm mirrors Query.schema_perm's default-catalog
+ fallback.
+
+ Regression test: a saved query against a database with a default
+ catalog can leave ``catalog`` unset (same as a raw SQL Lab ``Query``),
+ so this must fall back to ``database.get_default_catalog()`` -- not
+ building a ``[db].[schema]`` permission string that never matches the
+ ``[db].[catalog].[schema]`` grant a schema-authorized user actually
+ holds.
+ """
+ database = MagicMock()
+ database.database_name = database_name
+ database.get_default_catalog.return_value = default_catalog
+ saved_query = SavedQuery(label="q", schema=schema, catalog=catalog)
+ saved_query.database = database
+ with patch.object(
+ sql_lab_module.security_manager,
+ "get_schema_perm",
+ return_value=expected or None,
+ ) as mock_get_schema_perm:
+ result = saved_query.schema_perm
+ assert result == expected
+ effective_catalog = catalog if catalog is not None else default_catalog
+ mock_get_schema_perm.assert_called_once_with(
+ database_name, effective_catalog, schema
+ )
+
+
+def test_saved_query_schema_perm_guards_unset_database() -> None:
+ """schema_perm returns None instead of raising when the database is
+ unset, mirroring Query.schema_perm_guards_unset_database.
+ """
+ saved_query = SavedQuery(label="q", schema="s1", db_id=1)
+ assert saved_query.schema_perm is None
+
+
+def test_saved_query_before_update_syncs_dependent_slice_perms(app: Any) ->
None:
+ """Moving a saved query to a new schema also updates dependent charts'
+ denormalized perm/catalog_perm/schema_perm.
+
+ ``SavedQuery`` has no persisted perm columns of its own and gets no
+ ``datasource_access`` PVM (unlike ``SqlaTable``/``SemanticView``), but
+ ``Slice``'s ``set_related_perm`` listener still copies its computed
+ perm strings onto the chart at save time. Without a ``before_update``
+ listener on ``SavedQuery`` to re-copy them, a chart would keep matching
+ a stale schema/catalog grant after the saved query moves -- the
+ no-viewer chart-list fallback matches on ``Slice.schema_perm``/
+ ``catalog_perm`` (see ``ChartFilter._apply_viewers``).
+ """
+ from superset.extensions import db
+ from superset.models.core import Database
+ from superset.models.slice import Slice
+ from superset.utils.core import DatasourceType
+
+ database = Database(database_name="syncdb", sqlalchemy_uri="sqlite://")
+ saved_query = SavedQuery(label="Sync Query", schema="old_schema",
database=database)
+
+ db.session.add(database)
+ db.session.add(saved_query)
+ db.session.flush()
+
+ chart = Slice(
+ slice_name="On the saved query",
+ datasource_type=DatasourceType.SAVEDQUERY,
+ datasource_id=saved_query.id,
+ datasource_name="Sync Query",
+ viz_type="table",
+ params="{}",
+ )
+ db.session.add(chart)
+ db.session.flush()
+
+ try:
+ assert chart.schema_perm == saved_query.schema_perm
+ old_schema_perm = chart.schema_perm
+
+ saved_query.schema = "new_schema"
Review Comment:
The resync test only changes `schema`, so it still passes if
`target.load_database()` is removed from the `before_update` listener. If a
saved query is moved to a different database, dependent charts would keep the
old database's `perm`/`schema_perm` and nothing here would catch it. Could you
add a case that preloads database A, sets `saved_query.db_id` to database B,
flushes, and asserts the dependent chart's perm strings reference B?
--
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]
