aminghadersohi commented on code in PR #45681:
URL: https://github.com/apache/superset/pull/45681#discussion_r4381899188
##########
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:
Fixed in 9da337ea2de9464e92c5db61477a82e451cf62e6. I changed the collector
to render and strip adhoc column references before saved-column lookup,
independently of `expressionType`, while keeping SQL metrics literal.
Regression test:
`tests/unit_tests/connectors/sqla/virtual_dataset_connections_test.py::test_adhoc_saved_column_cache_keys_are_user_scoped`
executes with two users and two different RLS rules, then asserts different
full cache keys. Against the incoming head, 15 cases fail with equal keys and 3
existing-behavior controls pass; all 18 pass after the change.
`test_expression_cache_key_ignores_shadowed_references` and
`test_groupby_alias_retains_saved_column_rls` also fail before and pass after
for namespace/label siblings. The complete focused module passes 230 tests.
Siblings checked:
- superset/models/helpers.py:4054 — fixed in
9da337ea2de9464e92c5db61477a82e451cf62e6
- superset/models/helpers.py:4070 — fixed in
9da337ea2de9464e92c5db61477a82e451cf62e6
- superset/models/helpers.py:4085 — fixed in
9da337ea2de9464e92c5db61477a82e451cf62e6
- superset/connectors/sqla/models.py:2402 — not affected: SELECT/GROUP BY
and adhoc filters already share the rendered, stripped saved-column lookup; the
regression exercises these execution paths.
- superset/connectors/sqla/models.py:2286 — not affected: SQL metrics stay
literal and SIMPLE metrics resolve their column namespace;
test_sql_metric_cache_key_does_not_resolve_saved_column and
test_expression_cache_key_sibling_fields cover both.
- superset/models/sql_lab.py:484 — not affected: query-backed columns
execute raw expressions rather than saved calculated columns; the existing
Query expression/cache tests pass.
- superset/common/query_context_processor.py:461 — not affected: chart cache
consumers use the shared datasource collector, with no separate lookup
implementation.
- superset/semantic_layers/models.py:886 — not affected: separate cache
implementation that does not inherit ExploreMixin.
##########
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:
Fixed in 9da337ea2de9464e92c5db61477a82e451cf62e6. I pass the
execution-normalized granularity into expression collection and only collect
the primary time column additionally when `always_filter_main_dttm` uses it. A
valid SQL Lab Query time column never evaluates `main_dttm_col`.
Regression tests:
`tests/unit_tests/connectors/sqla/virtual_dataset_connections_test.py::test_cache_key_uses_only_executed_time_column`
first builds executable SQL, then checks cache keys. Both incoming-head cases
fail (Query: AttributeError; SqlaTable: unused template division-by-zero); both
pass after the change. `test_cache_key_collects_effective_primary_time_column`
also pins invalid-granularity fallback and always-filter-primary RLS
contributions: the unused non-temporal expression case fails before and passes
after; the primary-filter case is a passing control.
Siblings checked:
- superset/models/helpers.py:4120 — fixed in
9da337ea2de9464e92c5db61477a82e451cf62e6
- superset/models/helpers.py:4180 — fixed in
9da337ea2de9464e92c5db61477a82e451cf62e6
- superset/models/helpers.py:5203 — not affected: execution already
normalizes invalid granularity and gates the additional primary filter with
always_filter_main_dttm; the new tests compare against this builder.
- superset/models/sql_lab.py:364 — not affected: valid Query granularity no
longer reaches this property during collection; invalid granularity already
reaches it during execution and is not a valid execution path repaired here.
- superset/connectors/sqla/models.py:2871 — not affected: SqlaTable
delegates to the same collector; test_cache_key_uses_only_executed_time_column
covers its unused primary column.
- superset/models/sql_lab.py:395 — not affected: Query delegates to the same
collector; the Query parameter of the regression covers its TableColumn
metadata.
- superset/common/query_context_processor.py:461 — not affected:
test_cache_hit_schema_discovery_connection_budget retains exactly one discovery
connection and test_cache_hit_series_limit_does_not_execute_prequery retains
zero prequeries.
##########
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:
Fixed in 9da337ea2de9464e92c5db61477a82e451cf62e6. The text says the
execution schema takes precedence when discoverable, and the configured dataset
schema remains the fallback for RLS lookup and cache keys when connection
parameters and the inspector yield no schema.
Regression test:
`tests/unit_tests/connectors/sqla/virtual_dataset_connections_test.py::test_rls_schema_fallback_documented`
checks the empty-discovery fallback and its documentation. It fails on the
incoming wording and passes after this edit. The existing unresolved-schema
execution, retry and cache-key cases also pass.
Siblings checked:
- docs/admin_docs/security/security.mdx:668 — fixed in
9da337ea2de9464e92c5db61477a82e451cf62e6
- superset/utils/rls.py:58 — not affected: runtime already retains the
configured schema after empty discovery; no policy change is needed.
- superset/models/helpers.py:4022 — not affected: get_from_clause and
expression cache collection share this schema resolver, covered by the
unresolved-schema and connection-schema tests.
- superset/connectors/sqla/models.py:2871 — not affected: dataset cache
collection uses the same resolver and retains the fallback.
- superset/models/sql_lab.py:395 — not affected: query-backed cache
collection uses the same resolver and retains the fallback.
--
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]
