aminghadersohi commented on code in PR #43848:
URL: https://github.com/apache/superset/pull/43848#discussion_r3970972114
##########
superset/charts/filters.py:
##########
@@ -150,16 +155,40 @@ def _apply_viewers(self, query: Query) -> Query:
filters.append(Slice.id.in_(viewer_query))
# (C) No-viewer fallback: charts with no viewers → dataset-based access
+ layer_grant_clause = semantic_layer_grant_clause()
chart_has_viewers = Slice.viewers.any()
table_alias = aliased(SqlaTable)
no_viewer_query = (
db.session.query(Slice.id)
- .join(table_alias, Slice.datasource_id == table_alias.id)
- .join(models.Database, table_alias.database_id ==
models.Database.id)
+ # Type-aware datasource joins (mirroring DashboardAccessFilter):
+ # the SqlaTable join is constrained to table-backed charts (an
+ # unconstrained id join can bind a semantic-view chart to an
+ # unrelated table sharing its numeric id) and kept outer so
+ # charts on other datasource types survive into the access
+ # filter — their access matches through the perm columns
+ # denormalized onto Slice by ``set_related_perm``. A chart whose
+ # datasource row is hard-deleted can still match its stale
+ # denormalized perm here; the object gate stays authoritative
+ # and denies (accepted edge, shared with the dashboard filter).
Review Comment:
`Slice.perm` can't match after an ORM delete: `dataset_after_delete` drops
the `datasource_access` PVM and `hard_delete` fires it per row. The reachable
residual is `schema_perm`/`catalog_perm`, which survive deletion — worth
narrowing this comment and pinning it with a test.
##########
superset/models/dashboard.py:
##########
@@ -272,6 +275,31 @@ def get_url(id_: int, slug: str | None = None) -> str:
def datasources(self) -> set[BaseDatasource]:
return {slc.datasource for slc in self.slices if slc.datasource}
+ def has_member_datasource(self, datasource: BaseDatasource | Explorable)
-> bool:
+ """Type-aware membership: does a member chart reference this
datasource?
+
+ A *member datasource* is the ``(datasource_type, datasource_id)``
+ pair a member chart references. Comparing the pair — never the bare
+ numeric id — makes the test immune to id collisions across
+ datasource types, and it needs no datasource resolution (zero
+ queries). ``datasources`` above stays table-shaped for its
+ export/thumbnail/dataset-payload consumers and must not be used for
+ membership checks: it silently omits every non-table datasource.
+ """
+ # ``type`` is duck-typed on purpose: drill entry points hand this
+ # method loosely-typed datasources, and an object with no type or no
+ # id is simply no member — fail closed. Ids are compared as-is: an
+ # explorable with a string id never matches the integer
+ # ``datasource_id`` column, which is likewise fail closed.
+ candidate_type = getattr(datasource, "type", None)
+ candidate_id = datasource.id
Review Comment:
Docstring says an object with no id is "simply no member — fail closed", but
direct attribute access raises `AttributeError` instead. `type` above already
uses `getattr`; make `id` symmetric.
```suggestion
candidate_id = getattr(datasource, "id", None)
```
--
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]
