aminghadersohi commented on code in PR #45814:
URL: https://github.com/apache/superset/pull/45814#discussion_r4406889374
##########
superset/connectors/sqla/models.py:
##########
@@ -3239,6 +3227,35 @@ class and any keys added via `ExtraCache`.
return list(set(extra_cache_keys))
+ def get_inner_rls_cache_key(self) -> list[str]:
+ """
+ The table-bound RLS predicates execution injects into the tables read
+ by this dataset's SQL, as cache key components.
+
+ ``security_manager.get_rls_cache_key`` covers only the dataset's own
+ rules, so a cache of a virtual dataset's results must also key on
these,
+ or viewers whose RLS differs only on an inner table share entries.
Empty
+ for a physical dataset. If the predicates cannot be collected, a
+ viewer-scoped marker is returned instead.
+
+ :return: The RLS predicates, one cache key component each
+ """
+ from superset.utils.rls import collect_rls_predicates_for_sql
+
+ if not (self.is_virtual and self.sql):
+ return []
+ # The default schema is looked up only when the dataset has none: the
+ # lookup connects to the database, which serving a cached entry must
+ # not depend on.
+ return collect_rls_predicates_for_sql(
+ self.sql,
+ self.database,
+ self.catalog,
+ self.schema or self.database.get_default_schema(self.catalog) or
"",
Review Comment:
Correction to my reply above: the chart cache key is not fully fixed by this
PR. For a virtual dataset with no schema whose SQL uses a cache-key macro such
as `current_username`, `SqlaTable.get_extra_cache_keys` still raises
`ConnectionError` during a database outage, through `get_sqla_query` ->
`get_from_clause` -> `self._get_rls_schema()` at
`superset/models/helpers.py:4532`, which has no failure guard. Master behaved
the same before this PR, so this is not a regression. My earlier "not affected"
entry for that path was wrong. Remaining: that chart-key path, not changed by
this PR. The annotation key, the virtual-dataset column-values key and the SQL
Lab query column-values key are fixed.
##########
superset/datasource/api.py:
##########
@@ -102,6 +103,13 @@ def _column_values_cache_key(
rls=security_manager.get_rls_cache_key(datasource),
changed_on=str(getattr(datasource, "changed_on", "")),
)
+ # Executing a virtual dataset (or a SQL Lab query) also injects the RLS of
+ # the tables its SQL reads, which ``get_rls_cache_key`` does not cover.
+ # Only added when non-empty, so keys without inner rules keep their bytes.
+ if isinstance(datasource, (SqlaTable, Query)) and (
+ inner_rls := datasource.get_inner_rls_cache_key()
+ ):
Review Comment:
Correction to my reply above: the chart cache key is not fully fixed by this
PR. For a virtual dataset with no schema whose SQL uses a cache-key macro such
as `current_username`, `SqlaTable.get_extra_cache_keys` still raises
`ConnectionError` during a database outage, through `get_sqla_query` ->
`get_from_clause` -> `self._get_rls_schema()` at
`superset/models/helpers.py:4532`, which has no failure guard. Master behaved
the same before this PR, so this is not a regression. My earlier "not affected"
entry for that path was wrong. Remaining: that chart-key path, not changed by
this PR. The annotation key, the virtual-dataset column-values key and the SQL
Lab query column-values key are fixed.
--
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]
