aminghadersohi commented on code in PR #45681:
URL: https://github.com/apache/superset/pull/45681#discussion_r4385092041
##########
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:
I corrected the double render in ef4a77e10ace36e546c51e740a16f9db1342926e.
`_get_query_sql_expressions` returns rendered SQL, and
`_get_query_rls_cache_keys` parses it without another template pass. Adhoc
columns that resolve to saved metadata still render the saved column's separate
source expression, matching execution.
Regression tests in
`tests/unit_tests/connectors/sqla/virtual_dataset_connections_test.py`:
- `test_adhoc_metric_literal_template_cache_request`: raw literal `{{ x }}`,
a parameter expanding to `{{ 1 / 0 }}`, and query-backed raw division text. It
verifies real SQL execution, one collection render, the exact SQL parsed for
collection, and successful chart payload acquisition through the cache lookup.
- `test_single_render_subquery_cache_keys_are_user_scoped`: the
comment-wrapped conditional/subquery across columns, GROUP BY, metrics, ORDER
BY, filters and series-limit metrics, for both SqlaTable and Query. It verifies
executed RLS, exact collected predicates and different full cache keys for two
users; WHERE/HAVING are controls.
- `test_stored_expression_cache_collection_renders_once`: controls for saved
columns/metrics, templated saved-column references, saved filters and fetch
predicates.
With the incoming-head helper from dc56dcc7f9d2a66b952e2c552be45331ca92d39b
restored in Docker and the final tests retained: **16 failed, 9 passed**. With
this commit: **25 passed** within the **299 passing** affected-file tests. The
original-helper SHA256 matched `git show` before that run. Full
`tests/unit_tests/connectors` and `tests/unit_tests/models`, plus the RLS and
query-context-processor test files, pass in Docker (**1,804 passed**);
staged-file pre-commit, including ruff and mypy, exits 0.
Siblings checked:
- superset/models/helpers.py:4078 — fixed in
ef4a77e10ace36e546c51e740a16f9db1342926e
- superset/models/helpers.py:4198 — fixed in
ef4a77e10ace36e546c51e740a16f9db1342926e
- superset/models/helpers.py:4116 — fixed in
ef4a77e10ace36e546c51e740a16f9db1342926e
- superset/models/helpers.py:4118 — fixed in
ef4a77e10ace36e546c51e740a16f9db1342926e
- superset/models/helpers.py:4088 — fixed in
ef4a77e10ace36e546c51e740a16f9db1342926e
- superset/models/helpers.py:4103 — fixed in
ef4a77e10ace36e546c51e740a16f9db1342926e
- superset/models/helpers.py:4129 — fixed in
ef4a77e10ace36e546c51e740a16f9db1342926e
- superset/connectors/sqla/models.py:2912 — fixed in
ef4a77e10ace36e546c51e740a16f9db1342926e
- superset/models/sql_lab.py:425 — fixed in
ef4a77e10ace36e546c51e740a16f9db1342926e
- superset/models/helpers.py:4069 — not affected: unrendered stored SQL,
WHERE/HAVING and fetch predicates enter this helper once; it does not consume
already-rendered adhoc output.
- superset/models/helpers.py:4075 — not affected: saved-name references
supply a separate stored expression, rendered once; covered by
stored-expression controls.
- superset/models/helpers.py:4081 — not affected: rendering an adhoc
metadata reference and rendering the matched saved expression are two different
source expressions, as in execution; covered by the templated-reference control.
- superset/models/helpers.py:4138 — not affected: temporal-column references
supply stored source SQL, not rendered adhoc output; existing
effective-time-column and sibling-field tests pass.
- superset/models/helpers.py:4147 — not affected: WHERE/HAVING were not
rendered by add; the four conditional-subquery controls pass before and after.
- superset/models/helpers.py:4154 — not affected: the fetch predicate is raw
source SQL; the stored fetch control passes before and after.
- superset/models/helpers.py:2141 — not affected: metric execution renders
source once, then passes render_template=False to the validation stage.
- superset/models/helpers.py:2175 — not affected: generic execution
processes source SQL, not output returned by the cache collector.
- superset/models/helpers.py:3966 — not affected: virtual-dataset SQL
rendering is a separate input, not adhoc output from add.
- superset/models/helpers.py:5027 — not affected: expression-validation
endpoint renders its own source once and does not call add.
- superset/models/helpers.py:5112 — not affected: query-backed timestamp
execution renders the original column source, not collector output.
- superset/models/helpers.py:5134 — not affected: query-backed saved-column
execution renders its original stored expression, not collector output.
- superset/connectors/sqla/models.py:1170 — not affected: dataset RLS-clause
rendering, not adhoc SQL returned by add.
- superset/connectors/sqla/models.py:1182 — not affected: guest RLS-clause
rendering, not adhoc SQL returned by add.
- superset/connectors/sqla/models.py:1507 — not affected: saved-column
execution renders its stored source once.
- superset/connectors/sqla/models.py:1598 — not affected: saved timestamp
execution renders its stored source once.
- superset/connectors/sqla/models.py:1752 — not affected: saved-metric
execution renders its stored source once.
- superset/connectors/sqla/models.py:2207 — not affected: fetch-predicate
execution renders its original source once.
- superset/connectors/sqla/models.py:2380 — not affected: adhoc-column
metadata lookup renders the original source; fallback execution separately
renders that same original source, not the lookup's rendered output.
- superset/connectors/sqla/models.py:2900 — not affected: dataset-SQL
predicate collection is separate from expression collection and does not
consume add output.
- superset/models/sql_lab.py:417 — not affected: query-SQL predicate
collection is separate from expression collection and does not consume add
output.
--
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]
