aminghadersohi commented on code in PR #45681:
URL: https://github.com/apache/superset/pull/45681#discussion_r4385228868
##########
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 fixed the remaining bare-column-name gap in
efe9841047e1bebe7ccedd152621a4336ed4d9be. My earlier statement that
helpers.py:4075 was not affected was incorrect. The collector renders stored
expressions, not bare names, including filter-column references.
Regression:
`tests/unit_tests/connectors/sqla/virtual_dataset_connections_test.py::test_bare_column_names_are_not_templates_in_cache_collection`
covers `a{#b`, `a{%b`, and `{{ 1/0 }}` in columns, groupby, filters, orderby,
and SIMPLE metric columns on both SqlaTable and Query. All 30 cases execute
successfully, then fail during cache collection with the original
ef4a77e10ace36e546c51e740a16f9db1342926e source; all 30 pass with this commit,
asserting the underlying RLS predicate and no template render of the name.
Docker results: 595 focused tests, 1,739 connector/model tests, and 6 RLS
utility tests passed, including the prior literal-template, user-scoped
single-render, and stored-expression controls. Staged-file pre-commit and
Docker Ruff passed. Branch-wide changed-file hooks reproduce an existing Ruff
S704 on `superset/models/sql_lab.py:613`; the same expression also fails on
origin/master. No unrelated lint change is included.
Siblings checked:
- superset/models/helpers.py:4076 — fixed in
efe9841047e1bebe7ccedd152621a4336ed4d9be
- superset/models/helpers.py:4113 — fixed in
efe9841047e1bebe7ccedd152621a4336ed4d9be
- superset/models/helpers.py:4080 — not affected: adhoc SQL renders once;
the prior single-render regressions pass.
- superset/models/helpers.py:4152 — not affected: WHERE/HAVING are SQL
expressions, rendered once.
- superset/models/helpers.py:4157 — not affected: the fetch predicate is
SQL, rendered once; stored-expression controls pass.
- superset/models/helpers.py:4201 — not affected: consumes rendered
expressions without another template pass.
- superset/models/helpers.py:2141 — not affected: execution renders adhoc
metric SQL, not bare metadata names.
- superset/models/helpers.py:2175 — not affected: execution renders SQL
clauses, not bare metadata names.
- superset/models/helpers.py:3966 — not affected: renders datasource SQL,
not column references.
- superset/models/helpers.py:5030 — not affected: expression-validation
rendering, not cache-key name collection.
- superset/models/helpers.py:5115 — not affected: execution-only timestamp
helper; the collector does not call it.
- superset/models/helpers.py:5137 — not affected: calculated-column
rendering is guarded by a stored expression.
- superset/connectors/sqla/models.py:1170 — not affected: renders RLS SQL
clauses, not names.
- superset/connectors/sqla/models.py:1182 — not affected: renders guest RLS
SQL clauses, not names.
- superset/connectors/sqla/models.py:1507 — not affected: renders only
stored calculated-column expressions.
- superset/connectors/sqla/models.py:1598 — not affected: renders only
stored timestamp expressions.
- superset/connectors/sqla/models.py:1752 — not affected: renders stored
metric SQL, not metric names.
- superset/connectors/sqla/models.py:2207 — not affected: renders the SQL
fetch predicate.
- superset/connectors/sqla/models.py:2380 — not affected: renders adhoc SQL
for metadata lookup once.
- superset/connectors/sqla/models.py:2871 — not affected: delegates
expression collection to the shared helper; both datasource wrappers are
covered.
- superset/models/sql_lab.py:395 — not affected: delegates expression
collection to the shared helper; Query regressions pass.
- superset/utils/rls.py:309 — not affected: discovers tables/predicates
without rendering column names.
--
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]
