sadpandajoe commented on code in PR #45681:
URL: https://github.com/apache/superset/pull/45681#discussion_r4381691997
##########
superset/models/helpers.py:
##########
@@ -3994,6 +4019,156 @@ def get_rendered_sql(
def text(self, clause: str) -> TextClause:
return self.db_engine_spec.get_text_clause(clause)
+ def _get_rls_schema(self) -> str:
+ """Resolve RLS tables with the same schema policy as SQL
expressions."""
+ return get_rls_schema(
+ self.database,
+ self.catalog,
+ self.schema,
+ self.sql or "",
+ self.template_params_dict,
+ )
+
+ def _get_query_sql_expressions(
+ self, query_obj: QueryObjectDict, template_processor:
BaseTemplateProcessor
+ ) -> list[str]:
+ """Find SQL-bearing chart fields without building a chart or its
prequeries."""
+ # Real column names take precedence over verbose names in filter
lookup.
+ columns = {col.verbose_name: col.expression for col in self.columns}
+ columns.update({col.column_name: col.expression for col in
self.columns})
+ metrics = {metric.metric_name: metric.expression for metric in
self.metrics}
+ expressions: list[str] = []
+
+ def add(value: Any, metric: bool = False) -> None:
+ """Resolve stored references in the namespace used by the query
sink."""
+ if isinstance(value, str):
+ references = metrics if metric else columns
+ expressions.append(references.get(value) or value)
+ elif isinstance(value, dict):
+ if sql := value.get("sqlExpression"):
+ sql = template_processor.process_template(sql)
+ # Adhoc columns can name a saved calculated column after
+ # rendering. SQL metrics use their SQL literally instead.
+ expressions.append(
+ sql if "expressionType" in value else columns.get(sql)
or sql
Review Comment:
The cache-key collector skips the saved-column lookup whenever
`expressionType` is present, but execution (`adhoc_column_to_sqla`) resolves
the rendered, stripped `sqlExpression` against saved columns regardless of
`expressionType`. The frontend always sends `expressionType: 'SQL'` on adhoc
columns, including every time-series x-axis. So with `ALLOW_ADHOC_SUBQUERY` on,
an adhoc column naming a saved calculated column like `(SELECT ... FROM
protected)` executes with each user's RLS on `protected`, but the collector
sees a bare identifier and contributes no predicates. Two users with different
RLS on `protected` then share a cache key and the second can be served the
first user's result. The same mismatch occurs without `expressionType` when the
name carries surrounding whitespace (`columns.get(sql)` is unstripped). Should
this lookup follow the sink's rule (strip, then resolve saved columns for
non-metric values) instead of keying on `expressionType`?
##########
superset/models/helpers.py:
##########
@@ -3994,6 +4019,156 @@ def get_rendered_sql(
def text(self, clause: str) -> TextClause:
return self.db_engine_spec.get_text_clause(clause)
+ def _get_rls_schema(self) -> str:
+ """Resolve RLS tables with the same schema policy as SQL
expressions."""
+ return get_rls_schema(
+ self.database,
+ self.catalog,
+ self.schema,
+ self.sql or "",
+ self.template_params_dict,
+ )
+
+ def _get_query_sql_expressions(
+ self, query_obj: QueryObjectDict, template_processor:
BaseTemplateProcessor
+ ) -> list[str]:
+ """Find SQL-bearing chart fields without building a chart or its
prequeries."""
+ # Real column names take precedence over verbose names in filter
lookup.
+ columns = {col.verbose_name: col.expression for col in self.columns}
+ columns.update({col.column_name: col.expression for col in
self.columns})
+ metrics = {metric.metric_name: metric.expression for metric in
self.metrics}
+ expressions: list[str] = []
+
+ def add(value: Any, metric: bool = False) -> None:
+ """Resolve stored references in the namespace used by the query
sink."""
+ if isinstance(value, str):
+ references = metrics if metric else columns
+ expressions.append(references.get(value) or value)
+ elif isinstance(value, dict):
+ if sql := value.get("sqlExpression"):
+ sql = template_processor.process_template(sql)
+ # Adhoc columns can name a saved calculated column after
+ # rendering. SQL metrics use their SQL literally instead.
+ expressions.append(
+ sql if "expressionType" in value else columns.get(sql)
or sql
+ )
+ add(value.get("column"))
+ add(value.get("column_name"))
+
+ for value in [
+ *(query_obj.get("columns") or []),
+ *(query_obj.get("groupby") or []),
+ ]:
+ add(value)
+ for metric_value in query_obj.get("metrics") or []:
+ add(metric_value, metric=True)
+ for orderby_value, _ascending in query_obj.get("orderby") or []:
+ add(
+ orderby_value,
+ metric=isinstance(orderby_value, str) and orderby_value in
metrics,
+ )
+ for flt in query_obj.get("filter") or []:
+ filter_column = flt.get("col")
+ add(
+ filter_column,
+ metric=isinstance(filter_column, str) and filter_column not in
columns,
+ )
+ series_limit = query_obj.get("series_limit") or (
+ query_obj.get("timeseries_limit")
+ if query_obj.get("is_timeseries", True)
+ else 0
+ )
+ add(
+ (
+ query_obj.get("series_limit_metric")
+ or query_obj.get("timeseries_limit_metric")
+ )
+ if series_limit
+ else None,
+ metric=True,
+ )
+ add(query_obj.get("granularity"))
+ add(self.main_dttm_col if query_obj.get("granularity") else None)
Review Comment:
`self.main_dttm_col` is evaluated here whenever `granularity` is set. For
SQL Lab `Query` datasources it calls `col.get('is_dttm')` on `TableColumn`
objects (`Query.columns`), which have no `get`, so with `ALLOW_ADHOC_SUBQUERY`
on, Explore-from-SQL-Lab with `granularity: 'ds'` raises `AttributeError`
during cache-key generation, before any cache lookup or execution. Previously a
valid granularity never reached that property. Separately, on `SqlaTable` this
adds the primary datetime column's expression even when execution uses a
different time column (execution only falls back to `main_dttm_col` with
`always_filter_main_dttm`), so an unused column with a failing template or
macro can break cache-key generation. Could this collect only the column
execution would actually use, and avoid `main_dttm_col` for `Query`?
##########
docs/admin_docs/security/security.mdx:
##########
@@ -662,6 +662,13 @@ has important implications when working with virtual
(SQL-based) datasets:
Additionally, RLS filters on the **underlying physical datasets** referenced
by the
virtual dataset's SQL are injected into the inner subquery for each
referenced table.
+When `ALLOW_ADHOC_SUBQUERY` is enabled, subqueries in chart columns, metrics
and filters
+also receive the referenced datasets' RLS filters, and those filters
contribute to the
+chart's cache key. Unqualified table names use the connection's execution
schema on
+engines that cannot change schema per query, not an ignored dataset schema
setting.
Review Comment:
This reads as if the dataset schema is ignored on engines that cannot change
schema per query, but `get_rls_schema` falls back to the configured dataset
schema when neither the URI nor the inspector yields a default schema (e.g.
Druid via pydruid). An operator who clears the dataset schema believing it
irrelevant would drop the fallback used to match physical-dataset RLS rules and
build cache keys. Could the text say the execution schema takes precedence when
it can be discovered, and that the dataset schema remains the lookup fallback
otherwise?
--
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]
