sadpandajoe commented on code in PR #45681:
URL: https://github.com/apache/superset/pull/45681#discussion_r4390830608
##########
superset/connectors/sqla/models.py:
##########
@@ -2886,22 +2887,30 @@ class and any keys added via `ExtraCache`.
filtered_query_obj = {
k: v for k, v in query_obj.items() if k in SQLA_QUERY_KEYS
}
- sqla_query = self.get_sqla_query(**cast(Any, filtered_query_obj))
+ sqla_query = self.get_sqla_query(
+ **cast(Any, filtered_query_obj), execute_prequeries=False
+ )
extra_cache_keys += sqla_query.extra_cache_keys
+ resolve_catalog = cache(self.database.get_default_catalog)
+ default_schema = None
# For virtual datasets, include RLS predicates in the cache key
if self.is_virtual and self.sql:
- default_schema = self.database.get_default_schema(self.catalog)
+ default_schema = self._get_rls_schema()
rls_predicates = collect_rls_predicates_for_sql(
self.sql,
self.database,
self.catalog,
- self.schema or default_schema or "",
+ default_schema,
exclude_dataset_id=self.id,
+ resolve_catalog=resolve_catalog,
)
# Add each predicate as a separate cache key component
extra_cache_keys.extend(rls_predicates)
+ extra_cache_keys.extend(
+ self._get_query_rls_cache_keys(query_obj, default_schema,
resolve_catalog)
Review Comment:
Expression-subquery predicates are merged with the dataset-SQL predicates as
bare strings in an unordered set, so which table each predicate protects is
lost. That can collapse keys that were distinct before this change: with
`ALLOW_ADHOC_SUBQUERY` on, user A has RLS `t1 -> tenant_id = 1` and `t2 ->
tenant_id = 2`, user B has `t1 -> tenant_id = 2` and `t2 -> tenant_id = 1`, and
a virtual dataset `SELECT amount FROM t1` is charted with metric `MAX(amount) +
(SELECT SUM(amount) FROM t2)`. Before, the `t1` predicates alone gave A and B
different keys; now both contribute `{"(tenant_id = 1)", "(tenant_id = 2)"}`,
so whoever warms the cache first serves the other a result computed under the
wrong rules (e.g. 120 vs 210 on the same data). The same merge exists in
`Query.get_extra_cache_keys` (`superset/models/sql_lab.py`). Should the
expression-subquery contribution carry the qualified table with each predicate
(or be kept in a separate, tagged key component) so swapped rule sets stay d
istinguishable?
##########
superset/models/helpers.py:
##########
@@ -529,13 +531,27 @@ def validate_adhoc_subquery(
)
)
+ # Scope traversal needs a query root to find tables inside bare
expressions
+ # such as MAX((SELECT ...)), including Query-backed adhoc columns.
+ if not parsed_statement.is_select() and not
parsed_statement.is_set_operation():
+ parsed_statement = SQLStatement(f"SELECT {sql}", engine)
+ wrapped = True
+
# enforce RLS rules in any relevant tables
+ default_schema = get_rls_schema(database, catalog, default_schema, sql)
rls_applied = apply_rls(database, catalog, default_schema,
parsed_statement)
# Only regenerate the SQL if RLS predicates were actually applied;
# unnecessary round-tripping through sqlglot can alter dialect-specific
# syntax.
- return parsed_statement.format() if rls_applied else sql
+ if not rls_applied:
+ return sql
+ rewritten = parsed_statement.format()
+ if wrapped:
+ return re.split(r"SELECT\s+", rewritten, maxsplit=1,
flags=re.IGNORECASE)[
Review Comment:
The synthetic `SELECT` is stripped by splitting on the first textual
`SELECT`, but when an applicable RLS rule rewrites the statement, sqlglot moves
a leading comment ahead of the synthetic `SELECT`. For a saved expression such
as `/* SELECT reporting value */ (SELECT MAX(col1) FROM physical)` the
rewritten text is `/* SELECT reporting value */ SELECT (SELECT ...)`, the split
matches inside the comment, and the returned expression starts with `reporting
value */ SELECT ...`, which is invalid SQL. The base version returned
`parsed_statement.format()` directly and did not break on this input. Could the
wrapped form be unwrapped from the parsed expression (e.g. take the select list
of the parsed root) rather than by regex on the formatted text?
--
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]
