aminghadersohi commented on code in PR #45681:
URL: https://github.com/apache/superset/pull/45681#discussion_r4398022988
##########
superset/jinja_context.py:
##########
@@ -1115,6 +1252,51 @@ def process_template(self, sql: str, **kwargs: Any) ->
str:
raise UndefinedTemplateFunctionException(str(ex)) from ex
raise
+ def render_for_discovery(self, sql: str) -> str:
+ """Render as ``process_template`` would, without running any macro.
+
+ Template parameters and other plain values are real, as are the
+ request-only filter readers (``filter_values``, ``get_filters``,
+ ``url_param``). Every other context callable, such as warehouse
+ partition macros, ``dataset``, ``metric`` or the ``current_user_*``
+ functions, and non-deterministic built-ins (``random``, ``lipsum``)
+ are stubbed: using one raises ``OpaqueTemplateError`` rather than
+ running it. Output is therefore either execution's exact SQL or an
+ error, never a guess. Unknown filters, tests, extensions and processors
+ that override ``process_template`` are opaque for the same reason.
+ """
+ if type(self).process_template is not
BaseTemplateProcessor.process_template:
+ raise OpaqueTemplateError()
+ if self.env.extensions or self.env.finalize is not None:
+ raise OpaqueTemplateError()
+ env = self.env.overlay()
+ env.globals = {
+ name: value
+ if _DISCOVERY_GLOBALS.get(name) is value
+ else _discovery_value(value)
+ for name, value in self.env.globals.items()
+ }
+ env.filters = {
+ name: _reject_opaque_arguments(value)
+ if _DISCOVERY_FILTERS.get(name) is value
+ or (name == "where_in" and isinstance(value, WhereInMacro))
+ or (name == "to_datetime" and value is to_datetime)
Review Comment:
Valid, thanks. Fixed in 0715ff3cd9d08162583bf368d14ffe08ceb733e3.
`render_for_discovery` now wraps the custom filters it allows
(`to_datetime`, `where_in`) so a result that is not plain data is returned as
an `_OpaqueValue` (`_plain_result` in `superset/jinja_context.py`). A
`to_datetime` result is a `datetime`, so any use of it in discovery (`.now()`,
`.today()`, `.utcnow()`, `.date().today()`, `.min.now()`, comparison, printing)
raises `OpaqueTemplateError` and the expression gets the per-viewer key.
`where_in` returns a string and is unchanged. The trade-off is that templates
using `to_datetime` in a subquery expression no longer share cache entries
across viewers.
Regression tests (each fails at 5d0b51d71b307b139098ecc616466df12f8b31a8,
passes at 0715ff3cd9d08162583bf368d14ffe08ceb733e3):
-
`tests/unit_tests/jinja_context_test.py::test_render_for_discovery_stubs_to_datetime_results`
(7 cases, including your template; at the old head every case renders instead
of raising)
-
`tests/unit_tests/connectors/sqla/virtual_dataset_connections_test.py::test_to_datetime_clock_read_is_viewer_scoped[SqlaTable|Query]`:
the `to_datetime` result's `now()` returns a different year when the key is
computed than at execution. Two viewers with different RLS on `physical` get
rows `[[1], [2]]`. At the old head both keys are `[]`, the same key. After the
fix each key is that viewer's fallback marker.
- `test_render_for_discovery_matches_execution_for_data` now checks only the
empty-input `to_datetime` result (`None`, plain), which still renders exactly.
Siblings checked:
- superset/jinja_context.py:1301 — fixed in
0715ff3cd9d08162583bf368d14ffe08ceb733e3: `where_in` goes through the same
`_plain_result` wrapper; it returns `str`, so it renders as before.
- superset/jinja_context.py:1030 — not affected: the default Jinja filters
(`random` already stubbed) are pure functions of their arguments. Discovery
only hands them plain data, Jinja's own objects or stubs, and
`_reject_opaque_arguments` rejects stubs, so no clock-bearing value can reach
them.
- superset/jinja_context.py:1026 — not affected: the allowed globals
(`range`, `dict`, `cycler`, `joiner`, `namespace`; `lipsum` already stubbed)
are deterministic.
- superset/jinja_context.py:1307 — not affected: the default tests return
booleans.
- superset/jinja_context.py:807 — not affected: the request-only readers
kept live (`filter_values`, `get_filters`, `url_param`, `cache_key_wrapper`)
are `safe_proxy` partials, which raise on any return type outside
`ALLOWED_TYPES` and JSON round-trip collections, so they cannot return a
`datetime`.
- superset/jinja_context.py:1081 — not affected: any other context value
that is not plain data, such as a `datetime` or `context_addons` entry, is
already replaced by a stub.
Suites: jinja_context_test 253 passed; virtual_dataset_connections_test 1022
passed; tests/unit_tests/sql 3123 passed at 100% coverage; models, utils,
connectors, common, security, sql_lab passed.
##########
superset/models/helpers.py:
##########
@@ -4104,6 +4110,363 @@ 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,
+ render: Callable[[str], str | None],
+ ) -> 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],
+ opaque_templates: list[str],
+ prequery_objs: list[QueryObjectDict],
+ ) -> list[str]:
+ """Render SQL-bearing chart fields without building a chart or its
prequeries.
+
+ Render each source expression once, matching execution, but through
+ ``render_for_discovery`` so no macro runs before the cache lookup. An
+ expression that needs a macro is recorded in ``opaque_templates``. An
+ adhoc column that resolves to saved metadata also renders that column's
+ expression. A series-limit prequery is appended to ``prequery_objs``.
+ """
+ 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 render(expression: str) -> str | None:
+ """Render like execution without running macros; None when
opaque."""
+ # Only FROM's removals may skip filter columns: execution renders
+ # FROM first, so they are a subset of its own and collection can
+ # only over-include. Removals made by a chart expression are
+ # discarded for the same reason.
+ removed = list(removed_filters)
+ try:
+ return template_processor.render_for_discovery(expression)
+ except Exception: # pylint: disable=broad-except # noqa: BLE001
+ # Opaque (a stubbed macro was used) or invalid: either way the
+ # SQL execution produces is unknown here.
+ opaque_templates.append(expression)
+ return None
+ finally:
+ removed_filters[:] = removed
+
+ def add_expression(expression: str) -> None:
+ """Render source SQL, preserving literal Jinja in its output."""
+ if (rendered := render(expression)) is not None:
+ expressions.append(rendered)
+
+ 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")) and (
+ sql := render(sql)
+ ) is not None:
+ if not metric:
+ sql = self._get_adhoc_column_rls_expression(
+ cast(AdhocColumn, value), sql, render
+ )
+ 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. Discovery runs the request-only filter readers for real, so
+ # their removals reach this collector. A macro aborts the render early,
+ # recording fewer removals, which only over-includes expressions.
+ if self.sql:
+ try:
+
template_processor.render_for_discovery(self.sql.strip("\t\r\n; "))
+ except Exception: # pylint: disable=broad-except # noqa: BLE001,
S110
+ pass
+ collect_time_column = query_obj.get("is_timeseries", True) or (
+ granularity not in removed_filters
+ and utils.DTTM_ALIAS not in removed_filters
+ )
+ if collect_time_column:
+ 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
+ and self.main_dttm_col not in removed_filters
+ ):
+ add(self.main_dttm_col)
+ for flt in query_obj.get("filter") or []:
+ if all(flt.get(key) for key in ("col", "op")):
+ add_filter(flt["col"])
+ extras = query_obj.get("extras") or {}
+ for key in ("where", "having"):
+ if extras.get(key):
+ add_expression(extras[key])
+ if (
+ query_obj.get("apply_fetch_values_predicate")
+ and self.fetch_values_predicate
+ ):
+ add_expression(self.fetch_values_predicate)
+ series_limit = query_obj.get("series_limit") or (
+ query_obj.get("timeseries_limit")
+ if query_obj.get("is_timeseries", True)
+ else 0
+ )
+ series_columns = {
+ self.db_engine_spec.make_label_compatible(name)
+ for name in utils.get_column_names(query_obj.get("series_columns"))
+ }
+ has_series_groupby = bool(
+ need_groupby
+ and effective_columns
+ and (
+ (query_obj.get("is_timeseries", True) and not series_columns)
+ or series_columns.intersection(
+ self.db_engine_spec.make_label_compatible(name)
+ for name in utils.get_column_names(effective_columns)
+ )
+ )
+ )
+ if series_limit and has_series_groupby:
+ # The join planner builds its own time filter without checking
+ # removal. Prequeries rebuild a normal query and honor removal.
+ if (
+ not collect_time_column
+ and self.db_engine_spec.allows_joins
+ and self.db_engine_spec.allows_subqueries
+ and not self.db_engine_spec.time_groupby_inline
+ ):
+ add(granularity)
+ series_limit_metric = query_obj.get("series_limit_metric") or
query_obj.get(
+ "timeseries_limit_metric"
+ )
+ add(series_limit_metric, metric=True)
+ if not (
+ self.db_engine_spec.allows_joins
+ and self.db_engine_spec.allows_subqueries
+ ):
+ # Execution runs a prequery for the top series, which renders
+ # these templates again with its own context (row limit,
+ # columns), so collect it as execution builds it.
+ prequery_objs.append(
+ {
+ "is_timeseries": False,
+ "row_limit": series_limit,
+ "metrics": selected_metrics,
+ "granularity": granularity,
+ "groupby": query_obj.get("groupby") or [],
+ "filter": query_obj.get("filter") or [],
+ # The series metric is rendered by the main query.
+ "orderby": []
+ if series_limit_metric
+ else query_obj.get("orderby") or [],
+ "extras": extras,
+ # Grouped execution selects ``groupby or columns``
+ # without the timestamp pseudo column.
+ "columns":
get_non_base_axis_columns(effective_columns),
+ "order_desc": True,
+ }
+ )
+ 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 (
+ _get_cache_identity,
+ 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)
+ removed_filters: list[str] = []
+ template_kwargs.update(removed_filters=removed_filters,
applied_filters=[])
+ template_processor = self.get_template_processor(**template_kwargs)
+ predicates: set[str] = set()
+ opaque_templates: list[str] = []
+ prequery_objs: list[QueryObjectDict] = []
+ for expression in self._get_query_sql_expressions(
+ query_obj,
+ template_processor,
+ granularity,
+ removed_filters,
+ opaque_templates,
+ prequery_objs,
+ ):
+ sql = f"SELECT {expression}"
+ try:
+ parsed = SQLStatement(sql, self.db_engine_spec.engine)
+ except SupersetParseError:
+ # The collector supplies a per-user failure key for
unparseable SQL.
+ pass
+ else:
+ if not parsed.has_subquery():
+ continue
+ if default_schema is None:
+ default_schema = self._get_rls_schema()
+ # A query root lets scope traversal find tables in scalar
expressions
+ # and select-list fragments such as DISTINCT (SELECT ...).
+ predicates.update(
+ collect_rls_predicates_for_sql(
Review Comment:
Valid, thanks. Fixed in 0715ff3cd9d08162583bf368d14ffe08ceb733e3.
`apply_rls` and the cache-key collector now compute their predicates with
one shared helper, `get_rls_predicates` in `superset/utils/rls.py`. It returns
the rules for the statement's table reads plus, when execution would use
different ones, the rules for reads inside an uncorrelated sub-query.
`collect_rls_predicates_for_sql` gained `include_global_guest_rls` (default
`True`, as on `apply_rls`) and keys on both sets, tagging the sub-query set
`"rls-subquery"` because execution leaves the same table's other reads
unfiltered. Callers pass what their execution path passes:
- adhoc expression sub-queries (`_get_query_rls_cache_keys`): `True`,
matching `validate_adhoc_subquery` → `apply_rls` default
- dataset SQL (SqlaTable and Query `get_extra_cache_keys`): `False`,
matching `get_from_clause`. The sub-query reads in the dataset SQL, which
execution gives the global guest rules (and the dataset's own RLS), are now in
the key too. That was the same gap on that path.
Regression test (fails at 5d0b51d71b307b139098ecc616466df12f8b31a8, passes
at 0715ff3cd9d08162583bf368d14ffe08ceb733e3):
-
`tests/unit_tests/connectors/sqla/virtual_dataset_connections_test.py::test_global_guest_rls_on_subquery_tables_is_keyed[adhoc_expression|dataset_sql-SqlaTable|Query]`:
guest A has a global rule `col1 = 1`, guest B has the same clause scoped to
the chart's dataset. The rows returned differ for both cases. At the old head
both guests get `(['col1 = 1'], [])` (outer key, extra keys), the same key.
After the fix A's extra keys carry the `physical` rule.
Siblings checked:
- superset/models/helpers.py:4453 — fixed in
0715ff3cd9d08162583bf368d14ffe08ceb733e3 (adhoc expression contribution,
`include_global_guest_rls=True`)
- superset/connectors/sqla/models.py:3234 — fixed in
0715ff3cd9d08162583bf368d14ffe08ceb733e3 (dataset SQL sub-query reads, through
the shared helper)
- superset/models/sql_lab.py:423 — fixed in
0715ff3cd9d08162583bf368d14ffe08ceb733e3 (same, for Query)
- superset/models/helpers.py:656 — not affected: execution side (`apply_rls`
defaults), which the adhoc key mirrors.
- superset/models/helpers.py:4507 — not affected: execution side of the
dataset SQL (`include_global_guest_rls=False` plus sub-query rules), which the
dataset SQL key mirrors through the shared helper. For a Query it excludes
`exclude_dataset_id=self.id` while the key excludes nothing, so the key can
only carry more rules than execution applies.
- superset/models/helpers.py:4541 — not affected: the fail-closed
`rls_required` check after an RLS error is a validation path and contributes no
key.
- superset/security/manager.py:6116 — not affected: the outer
`get_rls_cache_key` already carries the datasource's guest clauses. Inner-table
scope is carried by the extra keys above.
- superset/common/query_context_processor.py:460 — not affected: the chart
key combines `get_rls_cache_key` with `get_extra_cache_keys`, both covered
above.
- superset/common/query_context_processor.py:529 — not affected: annotation
layers use the same pair.
- superset/sql_lab.py:553 — not affected: SQL Lab execution with `apply_rls`
defaults. Its results are not looked up by these chart keys.
- superset/sql/execution/executor.py:1160 — not affected: SQL Lab execution
with `apply_rls` defaults. Its results are not looked up by these chart keys.
- superset/commands/sql_lab/estimate.py:162 — not affected: SQL Lab
execution with `apply_rls` defaults. Its results are not looked up by these
chart keys.
- superset/mcp_service/dashboard_scope_sql.py:197 — not affected: applies
dashboard filter constraints, not RLS, and has no shared cache key.
- superset/semantic_layers/cache_host.py:80 — not affected: semantic views
have no SQL sub-queries, and guests with rules are denied by
`raise_for_unsupported_guest_rls`.
Remaining: superset/datasource/api.py:102 — the filter column-values cache
(`_column_values_cache_key`) keys only on the outer `get_rls_cache_key`, not on
the RLS of tables inside a virtual dataset's SQL. That predates this PR (same
code on master), is not on the chart cache path, and is not changed here.
Suites: virtual_dataset_connections_test 1022 passed; jinja_context_test 253
passed; tests/unit_tests/sql 3123 passed at 100% coverage; models, utils (incl.
rls_test), connectors, common, security, sql_lab passed.
--
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]
