aminghadersohi commented on code in PR #45681:
URL: https://github.com/apache/superset/pull/45681#discussion_r4397071696
##########
superset/models/helpers.py:
##########
@@ -4104,6 +4110,286 @@ 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_filter_column(
+ self,
+ name: str,
+ columns_by_name: dict[str, "TableColumn"],
+ ) -> Optional["TableColumn"]:
+ """Resolve real names before the first matching verbose filter
label."""
+ col = columns_by_name.get(name)
+ if col is not None:
+ return col
+ return next((col for col in self.columns if col.verbose_name == name),
None)
+
+ def _get_adhoc_column_rls_expression(
+ self,
+ col: AdhocColumn,
+ rendered_sql: str,
+ template_processor: BaseTemplateProcessor,
+ ) -> str | None:
+ """Query-backed adhoc columns execute their rendered SQL literally."""
+ return rendered_sql
+
+ def _get_query_sql_expressions( # noqa: C901
+ self,
+ query_obj: QueryObjectDict,
+ template_processor: BaseTemplateProcessor,
+ granularity: str | None,
+ removed_filters: list[str],
+ ) -> list[str]:
+ """Render SQL-bearing chart fields without building a chart or its
prequeries.
+
+ Render each source expression once, matching execution. An adhoc column
+ that resolves to saved metadata also renders that column's expression.
+ """
+ columns_by_name = {col.column_name: col for col in self.columns}
+ columns = {name: col.expression for name, col in
columns_by_name.items()}
+ metrics = {metric.metric_name: metric.expression for metric in
self.metrics}
+ selected_columns = query_obj.get("columns") or []
+ selected_metrics = query_obj.get("metrics") or []
+ adhoc_columns_by_label = {
+ col.get("label"): col
+ for col in selected_columns
+ if utils.is_adhoc_column(col)
+ }
+ metric_labels = {utils.get_metric_name(metric) for metric in
selected_metrics}
+ expressions: list[str] = []
+
+ def add_expression(expression: str) -> None:
+ """Render source SQL, preserving literal Jinja in its output."""
+ expressions.append(template_processor.process_template(expression))
+
+ 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
+ # Saved columns without expressions are quoted identifiers.
+ # Unknown selection strings have a separate SQL fallback below.
+ if expression := references.get(value):
+ add_expression(expression)
+ elif isinstance(value, dict):
+ if sql := value.get("sqlExpression"):
+ sql = template_processor.process_template(sql)
+ if not metric:
+ sql = self._get_adhoc_column_rls_expression(
+ cast(AdhocColumn, value), sql, template_processor
+ )
+ if sql:
+ expressions.append(sql)
+ add(value.get("column"))
+ add(value.get("column_name"))
+
+ def add_orderby(value: Any) -> None:
+ """ORDER BY prefers selected labels before stored metric/column
names."""
+ if isinstance(value, str):
+ if value in metric_labels:
+ # Selected metrics have already been collected.
+ return
+ if value in adhoc_columns_by_label:
+ add(adhoc_columns_by_label[value])
+ return
+ add(
+ value,
+ metric=utils.is_adhoc_metric(value)
+ or (isinstance(value, str) and value in metrics),
+ )
+
+ def add_filter(value: Any) -> None:
+ """Filters accept verbose column names before stored metric
names."""
+ # Execution renders adhoc/metric SQL before checking removal, but
+ # delays saved-column SQL until after that check.
+ if not isinstance(value, str):
+ add(value)
+ elif value == utils.DTTM_ALIAS and query_obj.get("is_timeseries",
True):
+ if value not in removed_filters and granularity not in
removed_filters:
+ add(granularity)
+ elif (col := self._get_filter_column(value, columns_by_name)) is
not None:
+ if value not in removed_filters and col.expression:
+ add_expression(col.expression)
+ elif value in metrics:
+ add(value, metric=True)
+ elif value in adhoc_columns_by_label:
+ add(adhoc_columns_by_label[value])
+
+ effective_columns = query_obj.get("groupby") or [
+ col for col in selected_columns if col != utils.DTTM_ALIAS
+ ]
+ # ORDER BY metrics can enable grouping even without selected metrics.
+ need_groupby = bool(
+ query_obj.get("metrics") is not None
+ or query_obj.get("groupby")
+ or any(
+ utils.is_adhoc_metric(cast(Metric, value))
+ or (
+ isinstance(value, str)
+ and (
+ value in metric_labels
+ or (value in metrics and value not in
adhoc_columns_by_label)
+ )
+ )
+ for value, _ascending in query_obj.get("orderby") or []
+ )
+ )
+ for value in effective_columns:
+ if isinstance(value, str) and value not in columns:
+ # GROUP BY executes unknown strings as SQL. Detail selections
+ # quote them before rendering, unlike saved bare column names.
+ add_expression(
+ value if need_groupby else
self.database.quote_identifier(value)
+ )
+ else:
+ add(value)
+ for metric_value in selected_metrics:
+ add(metric_value, metric=True)
+ for orderby_value, _ascending in query_obj.get("orderby") or []:
+ add_orderby(orderby_value)
+ # FROM templates remove filters before execution renders time/filter
columns.
+ # Render with the same processor so its removal state reaches this
collector.
+ if self.sql:
+ self.get_rendered_sql(template_processor)
Review Comment:
My previous commits (90d1e8a992f2dc98d2652cf9ad53b4d757fbe5f3,
1a121cb71cf0dc0c7154dbc96b8220f78e38a9f9,
0c4fa24d190053c1b7f104a76cfe71f5eebb76ed) avoided rendering on the cache-key
path by checking whether a template contained only known parameters. That check
had a bypass. A template that joins a string parameter with a list or dict
parameter (`{{ p }}{{ q }}{{ r }}`, `{{ p ~ q ~ r }}`) was neither rendered nor
viewer-scoped, while execution rendered it into a table subquery with each
viewer's RLS, so viewers with different RLS shared a key. The checks also still
rendered warehouse macros for any expression that did contain a table subquery.
Fixed in 33579b7e917cde2c947b23c20147d52279d18fad: cache-key collection no
longer decides from the template's syntax whether a render is needed. Each
expression is rendered with the context execution uses, through a new
`BaseTemplateProcessor.render_for_discovery` (superset/jinja_context.py:1253).
Template parameters and other plain values are real, so composed outputs render
exactly. `filter_values`, `get_filters`, `url_param` and `cache_key_wrapper`
also run, because they only read the request. Everything else is replaced by a
stub: warehouse partition macros, `dataset`, `metric`, `get_time_filter`,
`current_user_*`, `JINJA_CONTEXT_ADDONS` callables, unknown filters, tests and
globals, and the non-deterministic `random` and `lipsum`. Using a stub stops
the render without running it, and the key is scoped to the viewer. Filters and
tests that receive a stub (`is mapping`, `is sameas`, `is callable`,
`|default`) count as using it. The engine macro namespace (`presto`/`trino`)
pers
ists between execution renders, so it is stubbed whole. A processor that
overrides `process_template`, or an environment with extensions or `finalize`,
is opaque.
The rendered SQL goes through the same parser and RLS collector as before.
Every table subquery that execution injects RLS into therefore adds that
viewer's predicates, and no macro runs before the cache lookup, including in
expressions that contain table subqueries. The dataset's FROM is rendered the
same way, only to learn filter removals. A FROM render that stops early records
fewer removals, which only adds expressions. When the series limit runs as a
prequery (engines without joins or subqueries), the prequery is collected with
its own context (row limit, columns). docs/admin_docs/security/security.mdx now
describes this; the sentence saying templates are "rendered once" was wrong.
Tests (all pass at 33579b7e917cde2c947b23c20147d52279d18fad; counts from
running the new test file against the previous head
0c4fa24d190053c1b7f104a76cfe71f5eebb76ed in a scratch worktree):
- `test_composed_template_parameter_subquery_cache_keys_are_user_scoped`:
163 cases. String parameters joined with list or dict parameters through `~`,
`+`, separate outputs, filters, and `set`/`if`/`for` blocks. Sinks: adhoc
column, SQL metric, WHERE, HAVING, ORDER BY, saved column, saved metric,
saved-column filter, and the padded `expressionType: SQL` reference to a saved
column. Two viewers with different RLS on SQLite and DuckDB. It asserts
different rows, different keys and each viewer's predicate. At the previous
head 146 fail, 78 of them with identical keys.
- `test_engine_namespace_state_across_renders_is_viewer_scoped` and
`test_series_prequery_context_cache_keys_are_user_scoped`: both shared keys at
the previous head.
- `test_cache_key_never_runs_partition_macro_for_subqueries`: partition
macro inside a subquery, or in FROM with a subquery filter/time column, with
discovery succeeding and failing. The macro is never called. 24 of 24 fail at
the previous head.
- `test_render_for_discovery_stubs_macros`,
`test_render_for_discovery_matches_execution_for_data`,
`test_render_for_discovery_runs_request_filter_readers`,
`test_render_for_discovery_custom_processors` in
tests/unit_tests/jinja_context_test.py.
- The verifier's probes also pass at
33579b7e917cde2c947b23c20147d52279d18fad. Adversarial two-viewer probe: 813
passed, 0 failed (49 failed at the previous head). Partition-macro goal probe:
23 passed. Older probe: 40 passed; its 6 BigQuery rows fail on this SQLite
environment, as they did at the previous head.
- Suites: virtual_dataset_connections_test.py and jinja_context_test.py,
1221 passed. tests/unit_tests/sql, 3126 passed at 100% coverage.
tests/unit_tests/models, connectors and jinja_context_test.py, 3285 passed; the
one failure and one collection error are missing local modules
(`sqlalchemy_bigquery`, `parameterized`) and fail identically at the previous
head.
- Changed expectations: `get_time_filter` is now stubbed because it reads
the clock, so a removal it makes in FROM is not seen and the removed column is
still collected (over-inclusion, never sharing).
`test_cache_key_skips_removed_*` and
`test_removed_time_filter_series_limit_cache_matches_execution` now run with
both `get_filters` (removal seen) and `get_time_filter` (removal not seen).
Siblings checked:
- `superset/models/helpers.py:4143` (`_get_query_sql_expressions`, every
sink; the allow-list prefilter `_get_rls_expression_sources` /
`_may_have_rls_subquery` / `record_unrendered_template` is removed) — fixed in
33579b7e917cde2c947b23c20147d52279d18fad
- `superset/connectors/sqla/models.py:2572` (SqlaTable
`_get_adhoc_column_rls_expression`: saved-column expression and
quoted-reference fallback) — fixed in 33579b7e917cde2c947b23c20147d52279d18fad
- `superset/models/helpers.py:4134` (Query
`_get_adhoc_column_rls_expression`) — fixed in
33579b7e917cde2c947b23c20147d52279d18fad
- `superset/models/helpers.py:4286` (FROM render for filter removals,
discovery only) — fixed in 33579b7e917cde2c947b23c20147d52279d18fad
- `superset/models/helpers.py:4356` (series-limit prequery collected with
its own context) — fixed in 33579b7e917cde2c947b23c20147d52279d18fad
- `superset/models/helpers.py:4375` (`_get_query_rls_cache_keys`, viewer
scope for opaque templates) — fixed in 33579b7e917cde2c947b23c20147d52279d18fad
- `superset/connectors/sqla/models.py:3187` (SqlaTable
`get_extra_cache_keys`) — fixed in 33579b7e917cde2c947b23c20147d52279d18fad
- `superset/models/sql_lab.py:395` (Query `get_extra_cache_keys`) — fixed in
33579b7e917cde2c947b23c20147d52279d18fad
- `superset/connectors/sqla/models.py:3025` (`has_extra_cache_key_calls`) —
not affected: master's ExtraCache path, unchanged by this PR; it runs
`get_sqla_query` (and any macro) only when ExtraCache macros such as
`current_user_*` or `url_param` appear.
- `superset/common/query_object.py:561` (`_sanitize_filters`) — not
affected: master's `QueryObject.validate()` renders WHERE/HAVING before
`query_cache_key` (superset/common/query_context_processor.py:274), unchanged
by this PR; listed under Remaining.
Remaining:
- When ALLOW_ADHOC_SUBQUERY is on, an expression that uses a stubbed macro
gets a per-viewer key, which means fewer cache hits for those charts.
- A macro in WHERE/HAVING extras still runs before the lookup through
master's `QueryObject._sanitize_filters`, and through
`has_extra_cache_key_calls` when ExtraCache macros are present. This PR does
not change either path.
I have left the thread open for review.
##########
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:
My previous commit 0c4fa24d190053c1b7f104a76cfe71f5eebb76ed still shared
keys for an adhoc reference to a saved column (`expressionType: SQL`, padded
name) when the saved column's expression joined a string template parameter
with a list or dict parameter into `(FROM physical)`. The known-parameter
allow-list neither rendered that expression nor scoped the key to the viewer.
The same gap covered adhoc columns, SQL and saved metrics, saved columns and
saved-column filters.
Fixed in 33579b7e917cde2c947b23c20147d52279d18fad: cache-key collection no
longer decides from the template's syntax whether a render is needed. Each
expression is rendered with the context execution uses, through a new
`BaseTemplateProcessor.render_for_discovery` (superset/jinja_context.py:1253).
Template parameters and other plain values are real, so composed outputs render
exactly. `filter_values`, `get_filters`, `url_param` and `cache_key_wrapper`
also run, because they only read the request. Everything else is replaced by a
stub: warehouse partition macros, `dataset`, `metric`, `get_time_filter`,
`current_user_*`, `JINJA_CONTEXT_ADDONS` callables, unknown filters, tests and
globals, and the non-deterministic `random` and `lipsum`. Using a stub stops
the render without running it, and the key is scoped to the viewer. Filters and
tests that receive a stub (`is mapping`, `is sameas`, `is callable`,
`|default`) count as using it. The engine macro namespace (`presto`/`trino`)
pers
ists between execution renders, so it is stubbed whole. A processor that
overrides `process_template`, or an environment with extensions or `finalize`,
is opaque.
The rendered SQL goes through the same parser and RLS collector as before.
Every table subquery that execution injects RLS into therefore adds that
viewer's predicates, and no macro runs before the cache lookup, including in
expressions that contain table subqueries. The dataset's FROM is rendered the
same way, only to learn filter removals. A FROM render that stops early records
fewer removals, which only adds expressions. When the series limit runs as a
prequery (engines without joins or subqueries), the prequery is collected with
its own context (row limit, columns). docs/admin_docs/security/security.mdx now
describes this; the sentence saying templates are "rendered once" was wrong.
Tests (all pass at 33579b7e917cde2c947b23c20147d52279d18fad; counts from
running the new test file against the previous head
0c4fa24d190053c1b7f104a76cfe71f5eebb76ed in a scratch worktree):
- `test_composed_template_parameter_subquery_cache_keys_are_user_scoped`:
163 cases. String parameters joined with list or dict parameters through `~`,
`+`, separate outputs, filters, and `set`/`if`/`for` blocks. Sinks: adhoc
column, SQL metric, WHERE, HAVING, ORDER BY, saved column, saved metric,
saved-column filter, and the padded `expressionType: SQL` reference to a saved
column. Two viewers with different RLS on SQLite and DuckDB. It asserts
different rows, different keys and each viewer's predicate. At the previous
head 146 fail, 78 of them with identical keys.
- `test_engine_namespace_state_across_renders_is_viewer_scoped` and
`test_series_prequery_context_cache_keys_are_user_scoped`: both shared keys at
the previous head.
- `test_cache_key_never_runs_partition_macro_for_subqueries`: partition
macro inside a subquery, or in FROM with a subquery filter/time column, with
discovery succeeding and failing. The macro is never called. 24 of 24 fail at
the previous head.
- `test_render_for_discovery_stubs_macros`,
`test_render_for_discovery_matches_execution_for_data`,
`test_render_for_discovery_runs_request_filter_readers`,
`test_render_for_discovery_custom_processors` in
tests/unit_tests/jinja_context_test.py.
- The verifier's probes also pass at
33579b7e917cde2c947b23c20147d52279d18fad. Adversarial two-viewer probe: 813
passed, 0 failed (49 failed at the previous head). Partition-macro goal probe:
23 passed. Older probe: 40 passed; its 6 BigQuery rows fail on this SQLite
environment, as they did at the previous head.
- Suites: virtual_dataset_connections_test.py and jinja_context_test.py,
1221 passed. tests/unit_tests/sql, 3126 passed at 100% coverage.
tests/unit_tests/models, connectors and jinja_context_test.py, 3285 passed; the
one failure and one collection error are missing local modules
(`sqlalchemy_bigquery`, `parameterized`) and fail identically at the previous
head.
- Changed expectations: `get_time_filter` is now stubbed because it reads
the clock, so a removal it makes in FROM is not seen and the removed column is
still collected (over-inclusion, never sharing).
`test_cache_key_skips_removed_*` and
`test_removed_time_filter_series_limit_cache_matches_execution` now run with
both `get_filters` (removal seen) and `get_time_filter` (removal not seen).
Siblings checked:
- `superset/models/helpers.py:4143` (`_get_query_sql_expressions`, every
sink; the allow-list prefilter `_get_rls_expression_sources` /
`_may_have_rls_subquery` / `record_unrendered_template` is removed) — fixed in
33579b7e917cde2c947b23c20147d52279d18fad
- `superset/connectors/sqla/models.py:2572` (SqlaTable
`_get_adhoc_column_rls_expression`: saved-column expression and
quoted-reference fallback) — fixed in 33579b7e917cde2c947b23c20147d52279d18fad
- `superset/models/helpers.py:4134` (Query
`_get_adhoc_column_rls_expression`) — fixed in
33579b7e917cde2c947b23c20147d52279d18fad
- `superset/models/helpers.py:4286` (FROM render for filter removals,
discovery only) — fixed in 33579b7e917cde2c947b23c20147d52279d18fad
- `superset/models/helpers.py:4356` (series-limit prequery collected with
its own context) — fixed in 33579b7e917cde2c947b23c20147d52279d18fad
- `superset/models/helpers.py:4375` (`_get_query_rls_cache_keys`, viewer
scope for opaque templates) — fixed in 33579b7e917cde2c947b23c20147d52279d18fad
- `superset/connectors/sqla/models.py:3187` (SqlaTable
`get_extra_cache_keys`) — fixed in 33579b7e917cde2c947b23c20147d52279d18fad
- `superset/models/sql_lab.py:395` (Query `get_extra_cache_keys`) — fixed in
33579b7e917cde2c947b23c20147d52279d18fad
- `superset/connectors/sqla/models.py:3025` (`has_extra_cache_key_calls`) —
not affected: master's ExtraCache path, unchanged by this PR; it runs
`get_sqla_query` (and any macro) only when ExtraCache macros such as
`current_user_*` or `url_param` appear.
- `superset/common/query_object.py:561` (`_sanitize_filters`) — not
affected: master's `QueryObject.validate()` renders WHERE/HAVING before
`query_cache_key` (superset/common/query_context_processor.py:274), unchanged
by this PR; listed under Remaining.
Remaining:
- When ALLOW_ADHOC_SUBQUERY is on, an expression that uses a stubbed macro
gets a per-viewer key, which means fewer cache hits for those charts.
- A macro in WHERE/HAVING extras still runs before the lookup through
master's `QueryObject._sanitize_filters`, and through
`has_extra_cache_key_calls` when ExtraCache macros are present. This PR does
not change either path.
I have left the thread open for review.
##########
superset/models/helpers.py:
##########
@@ -3994,6 +4019,201 @@ 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_filter_column(
+ self,
+ name: str,
+ columns_by_name: dict[str, "TableColumn"],
+ ) -> Optional["TableColumn"]:
+ """Resolve real names before the first matching verbose filter
label."""
+ col = columns_by_name.get(name)
+ if col is not None:
+ return col
+ return next((col for col in self.columns if col.verbose_name == name),
None)
+
+ def _get_query_sql_expressions( # noqa: C901
+ self,
+ query_obj: QueryObjectDict,
+ template_processor: BaseTemplateProcessor,
+ granularity: str | None,
+ ) -> list[str]:
+ """Find SQL-bearing chart fields without building a chart or its
prequeries."""
+ columns_by_name = {col.column_name: col for col in self.columns}
+ columns = {name: col.expression for name, col in
columns_by_name.items()}
+ metrics = {metric.metric_name: metric.expression for metric in
self.metrics}
+ selected_columns = query_obj.get("columns") or []
+ selected_metrics = query_obj.get("metrics") or []
+ adhoc_columns_by_label = {
+ col.get("label"): col
+ for col in selected_columns
+ if utils.is_adhoc_column(col)
+ }
+ metric_labels = {utils.get_metric_name(metric) for metric in
selected_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 metric else columns.get(sql.strip()) or sql
+ )
+ add(value.get("column"))
+ add(value.get("column_name"))
+
+ def add_orderby(value: Any) -> None:
+ """ORDER BY prefers selected labels before stored metric/column
names."""
+ if isinstance(value, str):
+ if value in metric_labels:
+ # Selected metrics have already been collected.
+ return
+ if value in adhoc_columns_by_label:
+ add(adhoc_columns_by_label[value])
+ return
+ add(
+ value,
+ metric=utils.is_adhoc_metric(value)
+ or (isinstance(value, str) and value in metrics),
+ )
+
+ def add_filter(value: Any) -> None:
+ """Filters accept verbose column names before stored metric
names."""
+ if not isinstance(value, str):
+ add(value)
+ elif value == utils.DTTM_ALIAS and query_obj.get("is_timeseries",
True):
+ add(granularity)
+ elif (col := self._get_filter_column(value, columns_by_name)) is
not None:
+ expressions.append(col.expression or value)
+ elif value in metrics:
+ add(value, metric=True)
+ elif value in adhoc_columns_by_label:
+ add(adhoc_columns_by_label[value])
+
+ for value in query_obj.get("groupby") or selected_columns:
+ add(value)
+ for metric_value in selected_metrics:
+ add(metric_value, metric=True)
+ for orderby_value, _ascending in query_obj.get("orderby") or []:
+ add_orderby(orderby_value)
+ for flt in query_obj.get("filter") or []:
+ add_filter(flt.get("col"))
+ 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(granularity)
+ if (
+ granularity
+ and self.always_filter_main_dttm
+ and self.main_dttm_col in self.dttm_cols
+ and self.main_dttm_col != granularity
+ ):
+ add(self.main_dttm_col)
+ extras = query_obj.get("extras") or {}
+ expressions.extend(
+ extras[key] for key in ("where", "having") if extras.get(key)
+ )
+ if (
+ query_obj.get("apply_fetch_values_predicate")
+ and self.fetch_values_predicate
+ ):
+ expressions.append(self.fetch_values_predicate)
+ return expressions
+
+ def _get_query_rls_cache_keys(
+ self,
+ query_obj: QueryObjectDict,
+ default_schema: str | None = None,
+ resolve_catalog: Callable[[], str | None] | None = None,
+ ) -> list[str]:
+ """Collect expression-only RLS rules without building or executing SQL.
+
+ Only subquery expressions need table discovery. Reuse the
+ datasource SQL's resolved schema, or resolve it once for physical
datasets.
+ Do not exclude the dataset's own id: an independent expression
subquery is
+ not constrained by the outer query's RLS.
+ """
+ from superset.utils.rls import collect_rls_predicates_for_sql
+
+ if not query_obj or not is_feature_enabled("ALLOW_ADHOC_SUBQUERY"):
+ return []
+ resolve_catalog = resolve_catalog or
cache(self.database.get_default_catalog)
+ extras = query_obj.get("extras") or {}
+ granularity = query_obj.get("granularity")
+ if granularity is not None and granularity not in self.dttm_cols:
+ granularity = self.main_dttm_col
+ template_kwargs = {
+ key: query_obj.get(key)
+ for key in (
+ "columns",
+ "groupby",
+ "metrics",
+ "row_limit",
+ "row_offset",
+ "filter",
+ )
+ }
+ template_kwargs.update(
+ time_column=granularity,
+ time_grain=extras.get("time_grain_sqla"),
+ table_columns=[col.column_name for col in self.columns],
+ )
+ template_kwargs.update(self.template_params_dict)
+ template_processor = self.get_template_processor(**template_kwargs)
+ predicates: set[str] = set()
+ for expression in self._get_query_sql_expressions(
+ query_obj, template_processor, granularity
+ ):
+ expression = template_processor.process_template(expression)
Review Comment:
My previous commit 0c4fa24d190053c1b7f104a76cfe71f5eebb76ed rendered adhoc
SQL once, but only when a static check predicted a table subquery. That check
missed templates that join a string parameter with a list or dict parameter, so
such an adhoc metric was not rendered at all, and its RLS never reached the
key. This commit keeps the single render and makes it unconditional: the source
is rendered once through `render_for_discovery`, and its output is parsed,
never rendered again (`test_adhoc_metric_literal_template_cache_request`
asserts one render of the source and one parse of `SELECT MAX('{{ 1 / 0 }}')`).
Fixed in 33579b7e917cde2c947b23c20147d52279d18fad: cache-key collection no
longer decides from the template's syntax whether a render is needed. Each
expression is rendered with the context execution uses, through a new
`BaseTemplateProcessor.render_for_discovery` (superset/jinja_context.py:1253).
Template parameters and other plain values are real, so composed outputs render
exactly. `filter_values`, `get_filters`, `url_param` and `cache_key_wrapper`
also run, because they only read the request. Everything else is replaced by a
stub: warehouse partition macros, `dataset`, `metric`, `get_time_filter`,
`current_user_*`, `JINJA_CONTEXT_ADDONS` callables, unknown filters, tests and
globals, and the non-deterministic `random` and `lipsum`. Using a stub stops
the render without running it, and the key is scoped to the viewer. Filters and
tests that receive a stub (`is mapping`, `is sameas`, `is callable`,
`|default`) count as using it. The engine macro namespace (`presto`/`trino`)
pers
ists between execution renders, so it is stubbed whole. A processor that
overrides `process_template`, or an environment with extensions or `finalize`,
is opaque.
The rendered SQL goes through the same parser and RLS collector as before.
Every table subquery that execution injects RLS into therefore adds that
viewer's predicates, and no macro runs before the cache lookup, including in
expressions that contain table subqueries. The dataset's FROM is rendered the
same way, only to learn filter removals. A FROM render that stops early records
fewer removals, which only adds expressions. When the series limit runs as a
prequery (engines without joins or subqueries), the prequery is collected with
its own context (row limit, columns). docs/admin_docs/security/security.mdx now
describes this; the sentence saying templates are "rendered once" was wrong.
Tests (all pass at 33579b7e917cde2c947b23c20147d52279d18fad; counts from
running the new test file against the previous head
0c4fa24d190053c1b7f104a76cfe71f5eebb76ed in a scratch worktree):
- `test_composed_template_parameter_subquery_cache_keys_are_user_scoped`:
163 cases. String parameters joined with list or dict parameters through `~`,
`+`, separate outputs, filters, and `set`/`if`/`for` blocks. Sinks: adhoc
column, SQL metric, WHERE, HAVING, ORDER BY, saved column, saved metric,
saved-column filter, and the padded `expressionType: SQL` reference to a saved
column. Two viewers with different RLS on SQLite and DuckDB. It asserts
different rows, different keys and each viewer's predicate. At the previous
head 146 fail, 78 of them with identical keys.
- `test_engine_namespace_state_across_renders_is_viewer_scoped` and
`test_series_prequery_context_cache_keys_are_user_scoped`: both shared keys at
the previous head.
- `test_cache_key_never_runs_partition_macro_for_subqueries`: partition
macro inside a subquery, or in FROM with a subquery filter/time column, with
discovery succeeding and failing. The macro is never called. 24 of 24 fail at
the previous head.
- `test_render_for_discovery_stubs_macros`,
`test_render_for_discovery_matches_execution_for_data`,
`test_render_for_discovery_runs_request_filter_readers`,
`test_render_for_discovery_custom_processors` in
tests/unit_tests/jinja_context_test.py.
- The verifier's probes also pass at
33579b7e917cde2c947b23c20147d52279d18fad. Adversarial two-viewer probe: 813
passed, 0 failed (49 failed at the previous head). Partition-macro goal probe:
23 passed. Older probe: 40 passed; its 6 BigQuery rows fail on this SQLite
environment, as they did at the previous head.
- Suites: virtual_dataset_connections_test.py and jinja_context_test.py,
1221 passed. tests/unit_tests/sql, 3126 passed at 100% coverage.
tests/unit_tests/models, connectors and jinja_context_test.py, 3285 passed; the
one failure and one collection error are missing local modules
(`sqlalchemy_bigquery`, `parameterized`) and fail identically at the previous
head.
- Changed expectations: `get_time_filter` is now stubbed because it reads
the clock, so a removal it makes in FROM is not seen and the removed column is
still collected (over-inclusion, never sharing).
`test_cache_key_skips_removed_*` and
`test_removed_time_filter_series_limit_cache_matches_execution` now run with
both `get_filters` (removal seen) and `get_time_filter` (removal not seen).
Siblings checked:
- `superset/models/helpers.py:4143` (`_get_query_sql_expressions`, every
sink; the allow-list prefilter `_get_rls_expression_sources` /
`_may_have_rls_subquery` / `record_unrendered_template` is removed) — fixed in
33579b7e917cde2c947b23c20147d52279d18fad
- `superset/connectors/sqla/models.py:2572` (SqlaTable
`_get_adhoc_column_rls_expression`: saved-column expression and
quoted-reference fallback) — fixed in 33579b7e917cde2c947b23c20147d52279d18fad
- `superset/models/helpers.py:4134` (Query
`_get_adhoc_column_rls_expression`) — fixed in
33579b7e917cde2c947b23c20147d52279d18fad
- `superset/models/helpers.py:4286` (FROM render for filter removals,
discovery only) — fixed in 33579b7e917cde2c947b23c20147d52279d18fad
- `superset/models/helpers.py:4356` (series-limit prequery collected with
its own context) — fixed in 33579b7e917cde2c947b23c20147d52279d18fad
- `superset/models/helpers.py:4375` (`_get_query_rls_cache_keys`, viewer
scope for opaque templates) — fixed in 33579b7e917cde2c947b23c20147d52279d18fad
- `superset/connectors/sqla/models.py:3187` (SqlaTable
`get_extra_cache_keys`) — fixed in 33579b7e917cde2c947b23c20147d52279d18fad
- `superset/models/sql_lab.py:395` (Query `get_extra_cache_keys`) — fixed in
33579b7e917cde2c947b23c20147d52279d18fad
- `superset/connectors/sqla/models.py:3025` (`has_extra_cache_key_calls`) —
not affected: master's ExtraCache path, unchanged by this PR; it runs
`get_sqla_query` (and any macro) only when ExtraCache macros such as
`current_user_*` or `url_param` appear.
- `superset/common/query_object.py:561` (`_sanitize_filters`) — not
affected: master's `QueryObject.validate()` renders WHERE/HAVING before
`query_cache_key` (superset/common/query_context_processor.py:274), unchanged
by this PR; listed under Remaining.
Remaining:
- When ALLOW_ADHOC_SUBQUERY is on, an expression that uses a stubbed macro
gets a per-viewer key, which means fewer cache hits for those charts.
- A macro in WHERE/HAVING extras still runs before the lookup through
master's `QueryObject._sanitize_filters`, and through
`has_extra_cache_key_calls` when ExtraCache macros are present. This PR does
not change either path.
I have left the thread open for review.
--
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]
