sadpandajoe commented on code in PR #44169:
URL: https://github.com/apache/superset/pull/44169#discussion_r4015850650
##########
superset/models/slice.py:
##########
@@ -168,6 +169,15 @@ class Slice( # pylint: disable=too-many-public-methods
remote_side="SqlaTable.id",
lazy="subquery",
)
+ semantic_view = relationship(
Review Comment:
Adding this non-FK relationship makes deletion-retention policy validation
fail for every `Slice`: dependency discovery includes relationships without a
physical FK, but `purge_policy_registry` only declares `Slice.table`, so
`get_purge_policy(Slice)` raises an incomplete-policy error before purging even
a table-backed chart. Should this declare `semantic_view` as `PRESERVE` and add
purge coverage?
##########
superset/commands/chart/create.py:
##########
@@ -81,7 +81,10 @@ def validate(self) -> None:
# (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:
+ if datasource_type not in {
+ DatasourceType.TABLE,
+ DatasourceType.SEMANTIC_VIEW,
Review Comment:
Charts saved through this new type cannot survive their own export/import
flow: the exporter removes `datasource_type` and only emits `dataset_uuid` when
`model.table` is set, while chart import requires `dataset_uuid`. A
semantic-view chart therefore exports YAML without any datasource pointer and
immediately fails re-import; can the portable format carry a semantic-view
reference before this type is accepted?
##########
superset/models/dashboard.py:
##########
@@ -358,7 +358,7 @@ def datasets_trimmed_for_slices(
# Use the eagerly-loaded datasource from any slice in the group
datasource = next(iter(slices)).datasource
- if datasource:
+ if isinstance(datasource, BaseDatasource):
Review Comment:
Grouping these slices by bare `datasource_id` merges the independent
`SqlaTable` and `SemanticView` ID spaces. If both have ID 5,
`next(iter(slices))` can select the semantic-view chart and this guard drops
the entire group, including the table metadata, or it can pass the semantic
slice into the table's `data_for_slices`; can this key by `(datasource_type,
datasource_id)` and cover a mixed dashboard collision?
##########
superset/models/slice.py:
##########
@@ -190,8 +200,10 @@ def __repr__(self) -> str:
return self.slice_name or str(self.id)
@property
- def datasource(self) -> SqlaTable | None:
- return self.table
+ def datasource(self) -> SqlaTable | SemanticView | None:
+ if table := self.table:
+ return table
+ return self.semantic_view
Review Comment:
SECURITY FIX REQUIRED: Returning a `SemanticView` here feeds its integer ID
into `Dashboard.datasources`, while `can_drill_dataset_via_dashboard_access`
compares only bare IDs. A guest for a dashboard containing semantic view 7 can
request `/datasource/samples` with table ID 7 and receive raw rows from the
unrelated `SqlaTable` 7; can this keep table-only authorization callers
type-pinned or compare `(datasource_type, id)`?
##########
superset/charts/filters.py:
##########
@@ -163,7 +175,21 @@ def _apply_viewers(self, query: Query) -> Query:
)
)
)
- filters.append(Slice.id.in_(no_viewer_query))
+ filters.append(Slice.id.in_(no_viewer_table_query))
+
+ # SEMANTIC_VIEW charts have no SqlaTable/Database row to join; access
is
+ # evaluated against the chart's own perm, which set_related_perm keeps
in
+ # sync with the view's ``datasource_access`` perm (no numeric-id join).
+ no_viewer_semantic_view_query = db.session.query(Slice.id).filter(
+ and_(
+ Slice.datasource_type == DatasourceType.SEMANTIC_VIEW,
+ ~chart_has_viewers,
+ Slice.perm.in_(
Review Comment:
Chart visibility now depends on the stored `Slice.perm`, but the
semantic-view and semantic-layer rename hooks update the view menu and
`SemanticView.perm` without updating existing slices, unlike the dataset rename
path. After either rename, users retain current `datasource_access` yet their
saved charts disappear from the list; can this join the current permission or
cascade the rename and cover both cases?
--
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]
