This is an automated email from the ASF dual-hosted git repository.
rusackas pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/superset.git
The following commit(s) were added to refs/heads/master by this push:
new 3b355b6a854 fix(chart): allow semantic view datasource when saving
charts (#44169)
3b355b6a854 is described below
commit 3b355b6a854c45bb6a5c023357d1902cfa7584cb
Author: Kunal <[email protected]>
AuthorDate: Thu Sep 24 03:43:12 2026 +0530
fix(chart): allow semantic view datasource when saving charts (#44169)
Co-authored-by: Gabriel Torres Ruiz <[email protected]>
---
superset/models/dashboard.py | 3 +
superset/security/manager.py | 38 +++-
tests/integration_tests/charts/api_tests.py | 109 ++++++++++
tests/unit_tests/charts/test_filters.py | 96 +++++++++
tests/unit_tests/commands/chart/create_test.py | 33 ++-
tests/unit_tests/commands/chart/update_test.py | 41 +++-
tests/unit_tests/dao/test_query_stop_race.py | 68 +++++--
tests/unit_tests/models/dashboard_test.py | 41 +++-
tests/unit_tests/semantic_layers/models_test.py | 225 +++++++++++++++++++++
tests/unit_tests/subjects/test_raise_for_access.py | 122 +++++++++++
10 files changed, 753 insertions(+), 23 deletions(-)
diff --git a/superset/models/dashboard.py b/superset/models/dashboard.py
index c2adb0fdfab..e6a09759e17 100644
--- a/superset/models/dashboard.py
+++ b/superset/models/dashboard.py
@@ -377,6 +377,9 @@ class Dashboard(CoreDashboard, SoftDeleteMixin,
AuditMixinNullable, ImportExport
self,
) -> list[tuple[BaseDatasource | SemanticView, dict[str, Any]]]:
"""Return trimmed chart metadata, keeping datasource types distinct."""
+ # Key by (datasource_type, datasource_id): SqlaTable and SemanticView
+ # have independent auto-increment id spaces, so grouping by the bare
+ # datasource_id would merge unrelated datasources with colliding ids.
slices_by_datasource: dict[tuple[str, int], set[Slice]] =
defaultdict(set)
for slc in self.slices:
diff --git a/superset/security/manager.py b/superset/security/manager.py
index 7d685f33772..18a74250304 100644
--- a/superset/security/manager.py
+++ b/superset/security/manager.py
@@ -4232,7 +4232,9 @@ class SupersetSecurityManager( # pylint:
disable=too-many-public-methods
.values(perm=new_perm)
)
- # Cascade: update view perms that embed the layer name
+ # Cascade: update view perms that embed the layer name, and
+ # dependent charts so their denormalized perm stays in sync with
+ # the views (chart-list access filters match on Slice.perm).
sv_table = SemanticView.__table__ # pylint: disable=no-member
views = connection.execute(
sv_table.select().where(sv_table.c.semantic_layer_uuid ==
target.uuid)
@@ -4254,6 +4256,24 @@ class SupersetSecurityManager( # pylint:
disable=too-many-public-methods
.values(perm=new_view_perm)
)
+ # Update dependent charts so their denormalized perm stays
+ # in sync with the view: chart-list access filters match
+ # on Slice.perm.
+ from superset.models.slice import ( # pylint:
disable=import-outside-toplevel
+ Slice,
+ )
+
+ chart_table = Slice.__table__ # pylint: disable=no-member
+ connection.execute(
+ chart_table.update()
+ .where(
+ chart_table.c.datasource_type
+ == DatasourceType.SEMANTIC_VIEW,
+ chart_table.c.datasource_id == view_row.id,
+ )
+ .values(perm=new_view_perm)
+ )
+
def semantic_layer_after_delete(
self,
mapper: Mapper,
@@ -4356,6 +4376,22 @@ class SupersetSecurityManager( # pylint:
disable=too-many-public-methods
.values(perm=new_perm)
)
+ # Update dependent charts so their denormalized perm stays in sync
+ # with the view: chart-list access filters match on Slice.perm.
+ from superset.models.slice import ( # pylint:
disable=import-outside-toplevel
+ Slice,
+ )
+
+ chart_table = Slice.__table__ # pylint: disable=no-member
+ connection.execute(
+ chart_table.update()
+ .where(
+ chart_table.c.datasource_type ==
DatasourceType.SEMANTIC_VIEW,
+ chart_table.c.datasource_id == target.id,
+ )
+ .values(perm=new_perm)
+ )
+
def semantic_view_after_delete(
self,
mapper: Mapper,
diff --git a/tests/integration_tests/charts/api_tests.py
b/tests/integration_tests/charts/api_tests.py
index 37cfdc7caae..b9501de7ddd 100644
--- a/tests/integration_tests/charts/api_tests.py
+++ b/tests/integration_tests/charts/api_tests.py
@@ -38,6 +38,7 @@ 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.semantic_layers.models import SemanticLayer, SemanticView
from superset.subjects.models import Subject
from superset.subjects.types import SubjectType
from superset.tags.models import ObjectType, Tag, TaggedObject, TagType
@@ -704,6 +705,114 @@ class TestChartApi(ApiEditorsTestCaseMixin,
InsertChartMixin, SupersetTestCase):
db.session.delete(db.session.query(SavedQuery).get(saved_query_id))
db.session.commit()
+ def test_create_chart_from_semantic_view(self):
+ """
+ Chart API: creating a chart with datasource_type="semantic_view" must
+ succeed (apache/superset#44167). Semantic views are first-class
+ resolvable datasources (Slice resolves them through the type-guarded
+ ``semantic_view`` relationship), so the non-table datasource_type
+ guard must explicitly allow them rather than rejecting them the way
+ it rejects saved_query. This reproduces the exact API call shape from
+ the bug report: a real semantic view row, then POST /api/v1/chart/
+ with datasource_type="semantic_view".
+ """
+ self.login(ADMIN_USERNAME)
+ suffix = uuid.uuid4().hex
+ layer = SemanticLayer(
+ uuid=uuid.uuid4(),
+ name=f"issue-44167-layer-{suffix}",
+ type="test",
+ configuration="{}",
+ )
+ view = SemanticView(
+ uuid=uuid.uuid4(),
+ name=f"issue-44167-view-{suffix}",
+ semantic_layer_uuid=layer.uuid,
+ configuration="{}",
+ )
+ db.session.add_all([layer, view])
+ db.session.commit()
+ view_id = view.id
+
+ chart_data = {
+ "slice_name": "issue-44167-repro-chart",
+ "datasource_id": view_id,
+ "datasource_type": "semantic_view",
+ "viz_type": "table",
+ }
+ chart_id = None
+ try:
+ rv = self.post_assert_metric("/api/v1/chart/", chart_data, "post")
+
+ assert rv.status_code == 201
+ data = json.loads(rv.data.decode("utf-8"))
+ chart_id = data.get("id")
+ model = db.session.query(Slice).get(chart_id)
+ assert model.datasource_type == "semantic_view"
+ assert model.datasource_id == view_id
+
+ # The saved chart is now resolvable: its owner (admin) can
+ # retrieve it, and the chart's perm carries the view perm.
+ rv = self.get_assert_metric(f"/api/v1/chart/{chart_id}", "get")
+ assert rv.status_code == 200
+ assert model.perm == view.perm
+
+ gamma = self.get_user("gamma")
+ uri = "api/v1/chart/?q=" + rison.dumps(
+ {
+ "filters": [
+ {
+ "col": "slice_name",
+ "opr": "ct",
+ "value": "issue-44167-repro-chart",
+ }
+ ]
+ }
+ )
+
+ # Drop the admin session before impersonating gamma: logging in as
+ # the temporary user does not replace an already-authenticated
+ # session, so the admin would otherwise leak into these checks.
+ self.logout()
+
+ # Without the view's datasource_access perm, a non-owner cannot
+ # list/open the chart.
+ with self.temporary_user(gamma, login=True):
+ rv = self.client.get(uri, "get_list")
+ assert rv.status_code == 200
+ assert json.loads(rv.data.decode("utf-8"))["count"] == 0
+
+ # all_database_access short-circuits the chart filter just like
+ # all_datasource_access: a user who can access every database sees
+ # every chart, including semantic-view charts that have no database
+ # of their own.
+ perm = ("all_database_access", "all_database_access")
+ with self.temporary_user(gamma, extra_pvms=[perm], login=True):
+ rv = self.client.get(uri, "get_list")
+ assert rv.status_code == 200
+ assert json.loads(rv.data.decode("utf-8"))["count"] == 1
+
+ # With the view's datasource_access perm, a non-owner can
+ # list and retrieve the chart.
+ perm = ("datasource_access", view.perm)
+ with self.temporary_user(gamma, extra_pvms=[perm], login=True):
+ rv = self.client.get(uri, "get_list")
+ assert rv.status_code == 200
+ data = json.loads(rv.data.decode("utf-8"))
+ assert data["count"] == 1
+ rv = self.get_assert_metric(f"/api/v1/chart/{chart_id}", "get")
+ assert rv.status_code == 200
+ finally:
+ if chart_id:
+ model = db.session.query(Slice).get(chart_id)
+ if model:
+ db.session.delete(model)
+ view = db.session.query(SemanticView).get(view_id)
+ if view:
+ db.session.delete(view)
+ db.session.delete(layer)
+ db.session.commit()
+
@pytest.mark.usefixtures("load_world_bank_dashboard_with_slices")
def test_create_chart_validate_user_is_dashboard_editor(self):
"""
diff --git a/tests/unit_tests/charts/test_filters.py
b/tests/unit_tests/charts/test_filters.py
index 112f7fab0a9..7c560cad1e2 100644
--- a/tests/unit_tests/charts/test_filters.py
+++ b/tests/unit_tests/charts/test_filters.py
@@ -179,3 +179,99 @@ def test_chart_filter_guest_no_resources_denied(mocker:
MockerFixture) -> None:
assert filt.apply(query, None) is query
query.filter.assert_called_once() # scoped (to nothing), not role-based
viewers.assert_not_called()
+
+
+def _compile(clause: Any) -> str:
+ return str(
+ clause.statement.compile(
+ create_engine("sqlite://"),
+ compile_kwargs={"literal_binds": True},
+ )
+ )
+
+
+def _no_viewer_fallbacks(mocker: MockerFixture, perms: set[str]) -> Any:
+ """Build the no-viewer fallback clauses for a non-admin user.
+
+ Returns the fully-filtered Query so tests can compile and assert on the
SQL.
+ get_user_id() is mocked to None to keep the editor/viewer subqueries out.
+ """
+ from superset import db
+ from superset.charts.filters import ChartFilter
+ from superset.extensions import security_manager
+ from superset.models.slice import Slice
+
+ mocker.patch("superset.charts.filters.get_user_id", return_value=None)
+ mocker.patch.object(
+ security_manager, "get_accessible_databases", return_value=[1, 2, 3]
+ )
+
+ def _perms(perm_name: str) -> set[str]:
+ return perms if perm_name == "datasource_access" else set()
+
+ mocker.patch.object(security_manager, "user_view_menu_names",
side_effect=_perms)
+
+ filt: ChartFilter = ChartFilter.__new__(ChartFilter)
+ filt.model = Slice
+ return filt._apply_viewers(db.session.query(Slice))
+
+
+def test_chart_filter_no_viewer_semantic_view_matches_by_perm(
+ mocker: MockerFixture,
+) -> None:
+ """A semantic-view chart with no viewers is matched through the chart's own
+ perm (the view's ``datasource_access`` perm), not through a numeric-id join
+ to SqlaTable. The view and its parent layer are joined so the layer's perm
+ can participate in the access check."""
+ compiled = _compile(_no_viewer_fallbacks(mocker,
perms={"[layer].[view](id:1)"}))
+
+ assert "slices.datasource_type = 'semantic_view'" in compiled
+ assert "'[layer].[view](id:1)'" in compiled
+ assert "JOIN semantic_views" in compiled
+ assert "JOIN semantic_layers" in compiled
+ # the table branch is still guarded by datasource_type so semantic-view
+ # foreign ids can never ride along on the table/database join
+ assert "slices.datasource_type = 'table'" in compiled
+ assert "JOIN tables AS tables_1" in compiled
+
+
+def test_chart_filter_no_viewer_semantic_view_matches_layer_perm(
+ mocker: MockerFixture,
+) -> None:
+ """A datasource_access grant on a parent semantic layer matches its views'
+ charts through the layer perm (mirrors SemanticView.raise_for_access)."""
+ layer_perm = "[test_layer](id:11111111-2222-3333-4444-555566667777)"
+ compiled = _compile(_no_viewer_fallbacks(mocker, perms={layer_perm}))
+
+ assert "slices.datasource_type = 'semantic_view'" in compiled
+ assert "JOIN semantic_views" in compiled
+ assert "JOIN semantic_layers" in compiled
+ assert f"'{layer_perm}'" in compiled
+ # the layer perm is ANDed with the semantic-view type guard, never the
+ # table branch
+ assert "slices.datasource_type = 'table'" in compiled
+ assert "JOIN tables AS tables_1" in compiled
+
+
+def test_chart_filter_no_viewer_semantic_view_excluded_without_perm(
+ mocker: MockerFixture,
+) -> None:
+ """Without the view's datasource_access perm, the semantic-view branch
+ carries no matching perm literal."""
+ compiled = _compile(_no_viewer_fallbacks(mocker, perms=set()))
+
+ assert "slices.datasource_type = 'semantic_view'" in compiled
+ assert "'[layer].[view](id:1)'" not in compiled
+ assert "slices.datasource_type = 'table'" in compiled
+
+
+def test_chart_filter_no_viewer_table_matches_by_database_access(
+ mocker: MockerFixture,
+) -> None:
+ """Table charts with no viewers keep the pre-existing database-access join:
+ matched via the databases the user can access even without explicit
perms."""
+ compiled = _compile(_no_viewer_fallbacks(mocker, perms=set()))
+
+ assert "slices.datasource_type = 'table'" in compiled
+ assert "JOIN tables AS tables_1" in compiled
+ assert "dbs.id IN (1, 2, 3)" in compiled
diff --git a/tests/unit_tests/commands/chart/create_test.py
b/tests/unit_tests/commands/chart/create_test.py
index 2585aec44c3..93d8b06c002 100644
--- a/tests/unit_tests/commands/chart/create_test.py
+++ b/tests/unit_tests/commands/chart/create_test.py
@@ -35,6 +35,7 @@ from superset.commands.chart.exceptions import (
from superset.commands.exceptions import DatasourceTypeInvalidError
from superset.errors import ErrorLevel, SupersetError, SupersetErrorType
from superset.exceptions import SupersetSecurityException
+from superset.semantic_layers.models import SemanticView
from superset.utils import json
@@ -48,7 +49,7 @@ def _base_mocks(mocker: MockerFixture) -> None:
)
[email protected]("datasource_type", ["saved_query", "query"])
[email protected]("datasource_type", ["saved_query", "query", "bogus"])
def test_create_chart_rejects_non_table_datasource_type(
mocker: MockerFixture, datasource_type: str
) -> None:
@@ -122,6 +123,36 @@ def test_create_chart_accepts_table_datasource(mocker:
MockerFixture) -> None:
assert cmd._properties["datasource_name"] == "my_table"
+def test_create_chart_accepts_semantic_view_datasource(
+ mocker: MockerFixture,
+) -> None:
+ """A chart backed by a SIP-182 semantic view must be accepted: the view
+ is a first-class resolvable datasource (Slice resolves it through the
+ type-guarded ``semantic_view`` relationship), so the non-table guard
+ must explicitly allow it (apache/superset#44167)."""
+ _base_mocks(mocker)
+ datasource = mocker.MagicMock(spec=SemanticView)
+ datasource.name = "my_semantic_view"
+ get_datasource_by_id = 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": "semantic_view",
+ "slice_name": "some_name",
+ "viz_type": "table",
+ }
+ )
+ cmd.validate()
+
+ assert cmd._properties["datasource_name"] == "my_semantic_view"
+ get_datasource_by_id.assert_called_once_with(11, "semantic_view")
+
+
def test_create_chart_datasource_access_denied_still_raises_forbidden(
mocker: MockerFixture,
) -> None:
diff --git a/tests/unit_tests/commands/chart/update_test.py
b/tests/unit_tests/commands/chart/update_test.py
index a7ddfa941b6..ded20053462 100644
--- a/tests/unit_tests/commands/chart/update_test.py
+++ b/tests/unit_tests/commands/chart/update_test.py
@@ -34,6 +34,7 @@ from superset.commands.exceptions import (
from superset.errors import ErrorLevel, SupersetError, SupersetErrorType
from superset.exceptions import SupersetSecurityException
from superset.models.slice import Slice
+from superset.semantic_layers.models import SemanticView
from superset.utils import json
@@ -406,7 +407,7 @@ def
test_update_chart_query_context_without_datasource_is_allowed(
).validate()
[email protected]("datasource_type", ["saved_query", "query"])
[email protected]("datasource_type", ["saved_query", "query", "bogus"])
def test_update_chart_rejects_repointing_to_non_table_datasource(
mocker: MockerFixture, datasource_type: str
) -> None:
@@ -443,6 +444,42 @@ def
test_update_chart_rejects_repointing_to_non_table_datasource(
get_datasource_by_id.assert_not_called()
+def test_update_chart_accepts_semantic_view_datasource(
+ mocker: MockerFixture,
+) -> None:
+ """Repointing a chart at a SIP-182 semantic view must be accepted: the
+ view is a first-class resolvable datasource (Slice resolves it through
+ the type-guarded ``semantic_view`` relationship), so the non-table guard
+ must explicitly allow it (apache/superset#44167)."""
+ find_by_id =
mocker.patch("superset.commands.chart.update.ChartDAO.find_by_id")
+ find_by_id.return_value = mocker.MagicMock(
+ is_managed_externally=False, 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,
+ )
+ datasource = mocker.MagicMock(spec=SemanticView)
+ datasource.name = "my_semantic_view"
+ get_datasource_by_id = mocker.patch(
+ "superset.commands.chart.update.get_datasource_by_id",
+ return_value=datasource,
+ )
+ raise_for_access = mocker.patch(
+ "superset.commands.chart.update.security_manager.raise_for_access"
+ )
+
+ cmd = UpdateChartCommand(
+ 1, {"datasource_id": 11, "datasource_type": "semantic_view"}
+ )
+ cmd.validate()
+
+ get_datasource_by_id.assert_called_once_with(11, "semantic_view")
+ raise_for_access.assert_called_once_with(datasource=datasource)
+ assert cmd._properties["datasource_name"] == "my_semantic_view"
+
+
def test_update_chart_missing_datasource_type_keeps_required_error(
mocker: MockerFixture,
) -> None:
@@ -478,7 +515,7 @@ def
test_update_chart_missing_datasource_type_keeps_required_error(
get_datasource_by_id.assert_not_called()
[email protected]("datasource_type", ["saved_query", "query"])
[email protected]("datasource_type", ["saved_query", "query", "bogus"])
def test_update_chart_rejects_type_only_non_table_datasource(
mocker: MockerFixture, datasource_type: str
) -> None:
diff --git a/tests/unit_tests/dao/test_query_stop_race.py
b/tests/unit_tests/dao/test_query_stop_race.py
index e719605e9b4..201753facc5 100644
--- a/tests/unit_tests/dao/test_query_stop_race.py
+++ b/tests/unit_tests/dao/test_query_stop_race.py
@@ -75,6 +75,14 @@ import pytest
from pytest_mock import MockerFixture
from sqlalchemy.orm.session import Session
+# Generous thread handshake/join timeouts: CI runs the full unit suite on
+# loaded shards, and both threads here share a single StaticPool sqlite
+# connection, so a momentarily-blocked main thread must not time out the
+# worker's pause. A healthy run finishes these handshakes in well under a
+# second; the timeouts exist only to fail loudly on a genuine deadlock.
+HANDSHAKE_TIMEOUT = 60
+JOIN_TIMEOUT = 120
+
def test_stop_before_worker_starts_marks_stopped_without_dispatch(
app: Any, session: Session
@@ -256,7 +264,9 @@ def
test_stop_during_pre_dispatch_pause_marks_stopped_without_dispatch(
# but well before the DB connection is opened -- squarely inside the
# race window from the RCA.
reached_pause.set()
- assert release_execution.wait(timeout=5), "test deadlocked waiting for
release"
+ assert release_execution.wait(timeout=HANDSHAKE_TIMEOUT), (
+ "test deadlocked waiting for release"
+ )
return real_apply_limit(query, parsed_statement)
mocker.patch("superset.sql_lab.apply_limit",
side_effect=paused_apply_limit)
@@ -269,7 +279,9 @@ def
test_stop_during_pre_dispatch_pause_marks_stopped_without_dispatch(
worker = harness.run_execution(app, query_id, execution_result)
try:
- assert reached_pause.wait(timeout=5), "execution never reached the
pause point"
+ assert reached_pause.wait(timeout=HANDSHAKE_TIMEOUT), (
+ "execution never reached the pause point"
+ )
query = harness.fresh_query(query_id)
assert query.status == QueryStatus.RUNNING
@@ -285,7 +297,7 @@ def
test_stop_during_pre_dispatch_pause_marks_stopped_without_dispatch(
finally:
release_execution.set()
- worker.join(timeout=10)
+ worker.join(timeout=JOIN_TIMEOUT)
assert not worker.is_alive(), "execute_sql_statements did not finish in
time"
# The statement itself was never sent to the database -- this is the
@@ -339,7 +351,9 @@ def
test_stop_after_dispatch_with_no_cancel_support_raises_honestly(
# -- i.e. after QUERY_DISPATCHED_KEY is committed, right as the
# statement is about to be (or already is being) sent to the engine.
reached_pause.set()
- assert release_execution.wait(timeout=5), "test deadlocked waiting for
release"
+ assert release_execution.wait(timeout=HANDSHAKE_TIMEOUT), (
+ "test deadlocked waiting for release"
+ )
return real_execute_query(query, cursor, log_params)
execute_query_spy = mocker.patch(
@@ -350,7 +364,9 @@ def
test_stop_after_dispatch_with_no_cancel_support_raises_honestly(
worker = harness.run_execution(app, query_id, execution_result)
try:
- assert reached_pause.wait(timeout=5), "execution never reached the
pause point"
+ assert reached_pause.wait(timeout=HANDSHAKE_TIMEOUT), (
+ "execution never reached the pause point"
+ )
query = harness.fresh_query(query_id)
assert query.status == QueryStatus.RUNNING
@@ -372,7 +388,7 @@ def
test_stop_after_dispatch_with_no_cancel_support_raises_honestly(
finally:
release_execution.set()
- worker.join(timeout=10)
+ worker.join(timeout=JOIN_TIMEOUT)
assert not worker.is_alive(), "execute_sql_statements did not finish in
time"
# The statement genuinely ran (real execute_query was called through).
@@ -619,7 +635,9 @@ def
test_stop_racing_normal_completion_keeps_payload_and_results_write_consisten
# payload/results-write built", which is what this test is about.
result = real_execute_query(query, cursor, log_params)
reached_pause.set()
- assert release_execution.wait(timeout=5), "test deadlocked waiting for
release"
+ assert release_execution.wait(timeout=HANDSHAKE_TIMEOUT), (
+ "test deadlocked waiting for release"
+ )
return result
mocker.patch("superset.sql_lab.execute_query",
side_effect=paused_execute_query)
@@ -628,7 +646,9 @@ def
test_stop_racing_normal_completion_keeps_payload_and_results_write_consisten
worker = harness.run_execution(app, query_id, execution_result,
store_results=True)
try:
- assert reached_pause.wait(timeout=5), "execution never reached the
pause point"
+ assert reached_pause.wait(timeout=HANDSHAKE_TIMEOUT), (
+ "execution never reached the pause point"
+ )
query = harness.fresh_query(query_id)
assert query.status == QueryStatus.RUNNING
@@ -644,7 +664,7 @@ def
test_stop_racing_normal_completion_keeps_payload_and_results_write_consisten
finally:
release_execution.set()
- worker.join(timeout=10)
+ worker.join(timeout=JOIN_TIMEOUT)
assert not worker.is_alive(), "execute_sql_statements did not finish in
time"
real_cancel_spy.assert_called_once()
@@ -689,7 +709,9 @@ def
test_stop_racing_exception_path_keeps_stopped_not_failed(
def paused_failing_execute_query(query: Any, cursor: Any, log_params: Any)
-> Any:
reached_pause.set()
- assert release_execution.wait(timeout=5), "test deadlocked waiting for
release"
+ assert release_execution.wait(timeout=HANDSHAKE_TIMEOUT), (
+ "test deadlocked waiting for release"
+ )
raise RuntimeError("simulated unrelated failure")
mocker.patch(
@@ -700,7 +722,9 @@ def
test_stop_racing_exception_path_keeps_stopped_not_failed(
worker = harness.run_execution(app, query_id, execution_result)
try:
- assert reached_pause.wait(timeout=5), "execution never reached the
pause point"
+ assert reached_pause.wait(timeout=HANDSHAKE_TIMEOUT), (
+ "execution never reached the pause point"
+ )
QueryDAO.stop_query(client_id)
stopped_query = harness.fresh_query(query_id)
@@ -708,7 +732,7 @@ def
test_stop_racing_exception_path_keeps_stopped_not_failed(
finally:
release_execution.set()
- worker.join(timeout=10)
+ worker.join(timeout=JOIN_TIMEOUT)
assert not worker.is_alive(), "execute_sql_statements did not finish in
time"
final_query = harness.fresh_query(query_id)
@@ -812,7 +836,9 @@ def
test_stop_racing_results_backend_write_failure_keeps_stopped_not_failed(
# statement has already finished executing), then reports failure --
# the exact window the reviewer's repro targets.
reached_pause.set()
- assert release_execution.wait(timeout=5), "test deadlocked waiting for
release"
+ assert release_execution.wait(timeout=HANDSHAKE_TIMEOUT), (
+ "test deadlocked waiting for release"
+ )
return False
results_backend_mock.set.side_effect = paused_failing_set
@@ -842,7 +868,9 @@ def
test_stop_racing_results_backend_write_failure_keeps_stopped_not_failed(
worker.start()
try:
- assert reached_pause.wait(timeout=5), "execution never reached the
pause point"
+ assert reached_pause.wait(timeout=HANDSHAKE_TIMEOUT), (
+ "execution never reached the pause point"
+ )
query = harness.fresh_query(query_id)
assert query.status == QueryStatus.RUNNING
@@ -853,7 +881,7 @@ def
test_stop_racing_results_backend_write_failure_keeps_stopped_not_failed(
finally:
release_execution.set()
- worker.join(timeout=10)
+ worker.join(timeout=JOIN_TIMEOUT)
assert not worker.is_alive(), "execute_sql_statements did not finish in
time"
assert "error" not in execution_error, (
@@ -1049,7 +1077,9 @@ def
test_timed_out_exception_racing_stop_keeps_stopped_not_resurrected(
def paused_timed_out_execute_query(query: Any, cursor: Any, log_params:
Any) -> Any:
reached_pause.set()
- assert release_execution.wait(timeout=5), "test deadlocked waiting for
release"
+ assert release_execution.wait(timeout=HANDSHAKE_TIMEOUT), (
+ "test deadlocked waiting for release"
+ )
# Mirrors execute_query()'s own `except SoftTimeLimitExceeded`
# handler exactly: set status locally (uncommitted), then raise. Also
# dirties an unrelated field the same way real code paths would
@@ -1075,7 +1105,9 @@ def
test_timed_out_exception_racing_stop_keeps_stopped_not_resurrected(
worker = harness.run_execution(app, query_id, execution_result)
try:
- assert reached_pause.wait(timeout=5), "execution never reached the
pause point"
+ assert reached_pause.wait(timeout=HANDSHAKE_TIMEOUT), (
+ "execution never reached the pause point"
+ )
query = harness.fresh_query(query_id)
assert query.status == QueryStatus.RUNNING
@@ -1089,7 +1121,7 @@ def
test_timed_out_exception_racing_stop_keeps_stopped_not_resurrected(
finally:
release_execution.set()
- worker.join(timeout=10)
+ worker.join(timeout=JOIN_TIMEOUT)
assert not worker.is_alive(), "execute_sql_statements did not finish in
time"
final_query = harness.fresh_query(query_id)
diff --git a/tests/unit_tests/models/dashboard_test.py
b/tests/unit_tests/models/dashboard_test.py
index a7934722ec5..40e0836f2fb 100644
--- a/tests/unit_tests/models/dashboard_test.py
+++ b/tests/unit_tests/models/dashboard_test.py
@@ -16,11 +16,12 @@
# under the License.
import logging
-from unittest.mock import patch, PropertyMock
+from unittest.mock import Mock, patch, PropertyMock
import pytest
from flask import current_app
+from superset.connectors.sqla.models import BaseDatasource
from superset.models.dashboard import Dashboard
from superset.utils import json
@@ -698,3 +699,41 @@ def
test_tabs_places_a_node_the_layout_reaches_twice_only_once(
assert "skipping layout node TAB-1, the layout reaches it more than once"
in (
caplog.text
)
+
+
+def test_datasets_trimmed_for_slices_keeps_colliding_ids_separate() -> None:
+ """Keys slices by (datasource_type, datasource_id) to avoid id collisions.
+
+ SqlaTable and SemanticView use independent auto-increment id spaces, so a
+ table chart and a semantic-view chart can carry the same datasource_id.
+ Grouping by the bare id would merge the table chart into the semantic-view
+ group (whose non-table datasource is dropped), losing the table chart's
+ metadata from the dashboard payload.
+ """
+ table_datasource = Mock(spec=BaseDatasource)
+ table_datasource.table_name = "orders"
+ table_datasource.data_for_slices.return_value = {"cols": ["column"]}
+
+ sesh_table_slice = Mock()
+ sesh_table_slice.datasource_id = 1
+ sesh_table_slice.datasource_type = "table"
+ sesh_table_slice.resolved_datasource = table_datasource
+
+ semantic_view_slice = Mock()
+ semantic_view_slice.datasource_id = 1
+ semantic_view_slice.datasource_type = "semantic_view"
+ semantic_view_slice.resolved_datasource = None
+
+ dash = Dashboard()
+ with patch.object(
+ Dashboard,
+ "slices",
+ new_callable=PropertyMock,
+ return_value=[sesh_table_slice, semantic_view_slice],
+ ):
+ result = dash.datasets_trimmed_for_slices()
+
+ # Only the table-backed chart is kept; its datasource must be reported
+ # with the exact slice list (no semantic-view slice leaking into it).
+ assert result == [(table_datasource, {"cols": ["column"]})]
+
table_datasource.data_for_slices.assert_called_once_with([sesh_table_slice])
diff --git a/tests/unit_tests/semantic_layers/models_test.py
b/tests/unit_tests/semantic_layers/models_test.py
index e260476310f..6b1b26469e7 100644
--- a/tests/unit_tests/semantic_layers/models_test.py
+++ b/tests/unit_tests/semantic_layers/models_test.py
@@ -1601,6 +1601,181 @@ def test_semantic_view_before_update_updates_perm(app:
Any) -> None:
db.session.rollback()
+def test_semantic_view_before_update_syncs_dependent_slice_perms(app: Any) ->
None:
+ """Renaming a view also updates dependent charts' denormalized perm.
+
+ The chart-list access filter matches no-viewer semantic-view charts on
+ ``Slice.perm``, so a rename must propagate the new perm to the chart or the
+ chart loses visibility in lists even for entitled users.
+ """
+ from superset import security_manager
+ from superset.charts.filters import ChartFilter
+ from superset.extensions import db
+ from superset.models.slice import Slice
+ from superset.utils.core import DatasourceType
+
+ layer = SemanticLayer()
+ layer.name = "Sync Layer"
+ layer.uuid = uuid.UUID("bbbb1111-2222-3333-4444-555566667777")
+ layer.type = "test"
+
+ view = SemanticView()
+ view.name = "Old Sync View"
+ view.semantic_layer_uuid = layer.uuid
+
+ db.session.add(layer)
+ db.session.add(view)
+ db.session.flush()
+
+ chart = Slice(
+ slice_name="On the view",
+ datasource_type=DatasourceType.SEMANTIC_VIEW,
+ datasource_id=view.id,
+ datasource_name="Old Sync View",
+ viz_type="table",
+ params="{}",
+ )
+ db.session.add(chart)
+ db.session.flush()
+
+ try:
+ assert chart.perm == view.perm
+
+ view.name = "New Sync View"
+ db.session.flush()
+ db.session.expire(chart)
+ db.session.expire(view)
+
+ new_perm = view.perm
+ assert chart.perm == new_perm
+
+ # The chart stays discoverable through the chart-list access filter
+ # (ChartFilter._apply_viewers) that matches by Slice.perm.
+ with (
+ patch("superset.charts.filters.get_user_id", return_value=None),
+ patch.object(
+ security_manager, "user_view_menu_names",
return_value={new_perm}
+ ),
+ patch.object(security_manager, "get_accessible_databases",
return_value=[]),
+ ):
+ filt: ChartFilter = ChartFilter.__new__(ChartFilter)
+ filt.model = Slice
+ visible = filt._apply_viewers(db.session.query(Slice)).all()
+ assert chart.id in {slc.id for slc in visible}
+ finally:
+ db.session.rollback()
+
+
+def test_chart_filter_no_viewer_semantic_view_layer_perm_grants_visibility(
+ app: Any,
+) -> None:
+ """A datasource_access grant on the parent layer makes a no-viewer
+ semantic-view chart discoverable through the chart-list access filter
+ (mirrors SemanticView.raise_for_access)."""
+ from superset import security_manager
+ from superset.charts.filters import ChartFilter
+ from superset.extensions import db
+ from superset.models.slice import Slice
+ from superset.utils.core import DatasourceType
+
+ layer = SemanticLayer()
+ layer.name = "Layer Grant Layer"
+ layer.uuid = uuid.UUID("cccc1111-2222-3333-4444-555566667777")
+ layer.type = "test"
+
+ view = SemanticView()
+ view.name = "Layer Grant View"
+ view.semantic_layer_uuid = layer.uuid
+
+ db.session.add(layer)
+ db.session.add(view)
+ db.session.flush()
+
+ chart = Slice(
+ slice_name="On the layer-granted view",
+ datasource_type=DatasourceType.SEMANTIC_VIEW,
+ datasource_id=view.id,
+ datasource_name="Layer Grant View",
+ viz_type="table",
+ params="{}",
+ )
+ db.session.add(chart)
+ db.session.flush()
+
+ try:
+ # The insert listener stamps the computed perms on flush.
+ layer_perm = layer.perm
+ assert layer_perm
+ assert chart.perm == view.perm
+
+ with (
+ patch("superset.charts.filters.get_user_id", return_value=None),
+ patch.object(
+ security_manager, "user_view_menu_names",
return_value={layer_perm}
+ ),
+ patch.object(security_manager, "get_accessible_databases",
return_value=[]),
+ ):
+ filt: ChartFilter = ChartFilter.__new__(ChartFilter)
+ filt.model = Slice
+ visible = filt._apply_viewers(db.session.query(Slice)).all()
+ assert chart.id in {slc.id for slc in visible}
+ finally:
+ db.session.rollback()
+
+
+def test_chart_filter_no_viewer_semantic_view_unrelated_perm_denies_visibility(
+ app: Any,
+) -> None:
+ """An unrelated datasource_access grant does not expose a no-viewer
+ semantic-view chart through the chart-list access filter."""
+ from superset import security_manager
+ from superset.charts.filters import ChartFilter
+ from superset.extensions import db
+ from superset.models.slice import Slice
+ from superset.utils.core import DatasourceType
+
+ layer = SemanticLayer()
+ layer.name = "Unrelated Perm Layer"
+ layer.uuid = uuid.UUID("dddd1111-2222-3333-4444-555566667777")
+ layer.type = "test"
+
+ view = SemanticView()
+ view.name = "Unrelated Perm View"
+ view.semantic_layer_uuid = layer.uuid
+
+ db.session.add(layer)
+ db.session.add(view)
+ db.session.flush()
+
+ chart = Slice(
+ slice_name="On the unrelated-perm view",
+ datasource_type=DatasourceType.SEMANTIC_VIEW,
+ datasource_id=view.id,
+ datasource_name="Unrelated Perm View",
+ viz_type="table",
+ params="{}",
+ )
+ db.session.add(chart)
+ db.session.flush()
+
+ try:
+ with (
+ patch("superset.charts.filters.get_user_id", return_value=None),
+ patch.object(
+ security_manager,
+ "user_view_menu_names",
+ return_value={"[someone][else](id:98765)"},
+ ),
+ patch.object(security_manager, "get_accessible_databases",
return_value=[]),
+ ):
+ filt: ChartFilter = ChartFilter.__new__(ChartFilter)
+ filt.model = Slice
+ visible = filt._apply_viewers(db.session.query(Slice)).all()
+ assert chart.id not in {slc.id for slc in visible}
+ finally:
+ db.session.rollback()
+
+
def test_semantic_layer_after_delete_calls_security_manager() -> None:
"""Test SemanticLayer.after_delete delegates to security manager."""
from superset import security_manager
@@ -1659,6 +1834,56 @@ def
test_semantic_layer_rename_cascades_to_view_perms(app: Any) -> None:
db.session.rollback()
+def test_semantic_layer_rename_cascades_to_slice_perms(app: Any) -> None:
+ """Renaming a layer updates dependent charts' denormalized perm.
+
+ The chart-list access filter matches no-viewer semantic-view charts on
+ ``Slice.perm`` (mirroring ``set_related_perm``), so a layer rename that
+ rewrites the view perms must also rewrite the perm of charts pinned to
+ those views or the charts lose list visibility for entitled users.
+ """
+ from superset.extensions import db
+ from superset.models.slice import Slice
+ from superset.utils.core import DatasourceType
+
+ layer = SemanticLayer()
+ layer.name = "Old Slice Layer"
+ layer.uuid = uuid.UUID("dddd1111-2222-3333-4444-555566667777")
+ layer.type = "test"
+
+ view = SemanticView()
+ view.name = "Slice View"
+ view.semantic_layer_uuid = layer.uuid
+
+ db.session.add(layer)
+ db.session.add(view)
+ db.session.flush()
+
+ chart = Slice(
+ slice_name="On cascade view",
+ datasource_type=DatasourceType.SEMANTIC_VIEW,
+ datasource_id=view.id,
+ datasource_name="Slice View",
+ viz_type="table",
+ params="{}",
+ )
+ db.session.add(chart)
+ db.session.flush()
+
+ assert chart.perm == view.perm
+
+ try:
+ layer.name = "New Slice Layer"
+ db.session.flush()
+
+ # Cascade update is via raw SQL, so refresh the ORM objects
+ db.session.refresh(view)
+ db.session.refresh(chart)
+ assert chart.perm == f"[New Slice Layer].[Slice View](id:{view.id})"
+ finally:
+ db.session.rollback()
+
+
# =============================================================================
# build_semantic_view_query dual perm tests
# =============================================================================
diff --git a/tests/unit_tests/subjects/test_raise_for_access.py
b/tests/unit_tests/subjects/test_raise_for_access.py
index 7c8666b3aa4..7e1f131a44d 100644
--- a/tests/unit_tests/subjects/test_raise_for_access.py
+++ b/tests/unit_tests/subjects/test_raise_for_access.py
@@ -267,6 +267,128 @@ def
test_raise_for_access_chart_editor_allows(app_context):
sm.raise_for_access(chart=chart)
+def _make_real_slice_chart(*, kind: str):
+ """Build a real Slice whose ``datasource`` resolves through the real
+ ``table`` / ``semantic_view`` relationships (no MagicMock of the
property)."""
+ from superset.connectors.sqla.models import SqlaTable
+ from superset.models.slice import Slice
+ from superset.semantic_layers.models import SemanticView
+ from superset.utils.core import DatasourceType
+
+ chart = Slice()
+ chart.datasource_id = 1
+ chart.viewers = []
+ chart.editors = []
+
+ if kind == DatasourceType.SEMANTIC_VIEW:
+ chart.datasource_type = DatasourceType.SEMANTIC_VIEW
+ view = MagicMock(spec=SemanticView)
+ view.id = 1
+ view.perm = "[layer].[view](id:1)"
+ view.schema_perm = None
+ view.catalog_perm = None
+ chart.semantic_view = view
+ else:
+ chart.datasource_type = DatasourceType.TABLE
+ table = MagicMock(spec=SqlaTable)
+ table.id = 1
+ table.perm = "[db].[table](id:1)"
+ table.schema_perm = None
+ table.catalog_perm = None
+ chart.table = table
+
+ return chart
+
+
+def test_raise_for_access_semantic_view_chart_allows_with_datasource_access(
+ app_context,
+):
+ """A semantic-view chart with no viewers resolves its datasource through
the
+ ``semantic_view`` relationship and is granted when the user holds the
view's
+ ``datasource_access`` perm."""
+ sm = _make_sm()
+ chart = _make_real_slice_chart(kind="semantic_view")
+
+ with (
+ patch.object(sm, "is_admin", return_value=False),
+ patch.object(sm, "is_editor", return_value=False),
+ patch.object(sm, "is_viewer", return_value=False),
+ patch.object(sm, "is_guest_user", return_value=False),
+ patch.object(sm, "can_access", return_value=True),
+ patch("superset.is_feature_enabled", return_value=False),
+ ):
+ assert chart.resolved_datasource is chart.semantic_view
+ sm.raise_for_access(chart=chart)
+
+
+def test_raise_for_access_semantic_view_chart_denied_without_datasource_access(
+ app_context,
+):
+ """A semantic-view chart with no viewers is denied when the user does not
+ hold the view's ``datasource_access`` perm."""
+ sm = _make_sm()
+ chart = _make_real_slice_chart(kind="semantic_view")
+
+ with (
+ patch.object(sm, "is_admin", return_value=False),
+ patch.object(sm, "is_editor", return_value=False),
+ patch.object(sm, "is_viewer", return_value=False),
+ patch.object(sm, "is_guest_user", return_value=False),
+ patch.object(sm, "can_access", return_value=False),
+ patch.object(sm, "can_access_schema", return_value=False),
+ patch.object(sm, "get_chart_access_error_object",
return_value=MagicMock()),
+ patch("superset.is_feature_enabled", return_value=False),
+ ):
+ with pytest.raises(SupersetSecurityException):
+ sm.raise_for_access(chart=chart)
+
+
+def test_raise_for_access_semantic_view_chart_editor_allows(app_context):
+ """An editor of the chart is granted regardless of datasource perms."""
+ sm = _make_sm()
+ chart = _make_real_slice_chart(kind="semantic_view")
+
+ with (
+ patch.object(sm, "is_admin", return_value=False),
+ patch.object(sm, "is_editor", side_effect=lambda r: r is chart),
+ patch.object(sm, "is_guest_user", return_value=False),
+ patch("superset.is_feature_enabled", return_value=False),
+ ):
+ sm.raise_for_access(chart=chart)
+
+
+def test_raise_for_access_semantic_view_chart_admin_allows(app_context):
+ """Admin is granted regardless of datasource perms."""
+ sm = _make_sm()
+ chart = _make_real_slice_chart(kind="semantic_view")
+
+ with (
+ patch.object(sm, "is_admin", return_value=True),
+ patch.object(sm, "is_editor", return_value=False),
+ patch.object(sm, "is_guest_user", return_value=False),
+ patch("superset.is_feature_enabled", return_value=False),
+ ):
+ sm.raise_for_access(chart=chart)
+
+
+def test_raise_for_access_table_chart_resolution_unchanged(app_context):
+ """A table chart still resolves through the ``table`` relationship and is
+ granted via its table perm; the semantic-view path is not consulted."""
+ sm = _make_sm()
+ chart = _make_real_slice_chart(kind="table")
+
+ with (
+ patch.object(sm, "is_admin", return_value=False),
+ patch.object(sm, "is_editor", return_value=False),
+ patch.object(sm, "is_viewer", return_value=False),
+ patch.object(sm, "is_guest_user", return_value=False),
+ patch.object(sm, "can_access", return_value=True),
+ patch("superset.is_feature_enabled", return_value=False),
+ ):
+ assert chart.datasource is chart.table
+ sm.raise_for_access(chart=chart)
+
+
# -- Datasource chart-viewer promiscuous mode tests --
