aminghadersohi commented on code in PR #45681:
URL: https://github.com/apache/superset/pull/45681#discussion_r4397288620
##########
superset/models/helpers.py:
##########
@@ -4104,6 +4111,490 @@ 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_rls_expression_sources( # noqa: C901
+ self,
+ expression: str,
+ template_processor: BaseTemplateProcessor,
+ ) -> set[str]:
+ """Inspect SQL and literal template inputs without executing any
macros.
+
+ Include parameter values and constant outputs so templated subqueries
+ and saved-column references are not mistaken for plain expressions.
+ This is only a prefilter; rendered SQL still undergoes AST discovery.
+ """
+ sources = {expression.strip()}
+ try:
+ template = template_processor.env.parse(expression)
+ except TemplateError:
+ return sources
+ unknown = object()
+
+ def literal(node: nodes.Node) -> Any: # noqa: C901
+ """Read JSON values and concatenate strings, never calls or
filters."""
+ if isinstance(node, nodes.Const):
+ return node.value
+ if isinstance(node, nodes.TemplateData):
+ return node.data
+ if isinstance(node, nodes.Name):
+ return self.template_params_dict.get(node.name, unknown)
+ if isinstance(node, (nodes.Getattr, nodes.Getitem)):
+ value = literal(node.node)
+ key = (
+ node.attr if isinstance(node, nodes.Getattr) else
literal(node.arg)
+ )
+ if isinstance(value, dict) and isinstance(key, (str, int)):
+ return value.get(key, unknown)
+ if isinstance(value, list) and isinstance(key, int):
+ try:
+ return value[key]
+ except IndexError:
+ pass
+ return unknown
+ if isinstance(node, nodes.Add):
+ left, right = literal(node.left), literal(node.right)
+ if isinstance(left, str) and isinstance(right, str):
+ return left + right
+ if isinstance(left, (int, float)) and isinstance(right, (int,
float)):
+ return left + right
+ return unknown
+ parts = node.nodes if isinstance(node, (nodes.Output,
nodes.Concat)) else []
+ values = [literal(part) for part in parts]
+ if parts and all(
+ isinstance(value, (str, int, float, bool, type(None)))
+ for value in values
+ ):
+ return "".join(str(value) for value in values)
+ return unknown
+
+ for output in template.find_all(nodes.Output):
+ if isinstance(value := literal(output), str):
+ sources.add(value.strip())
+ return sources
+
+ def _may_have_rls_subquery(
+ self,
+ expression: str,
+ template_processor: BaseTemplateProcessor,
+ ) -> bool | None:
+ """Detect subqueries without rendering; return None for ambiguous
SQL."""
+ ambiguous = False
+ for source in self._get_rls_expression_sources(expression,
template_processor):
+ # Jinja calls are opaque, not SQL. Inspect the surrounding SQL and
+ # literal template outputs separately, without executing either.
+ source = re.sub(
+ r"\{\{.*?\}\}|\{%.*?%\}|\{#.*?#\}", "NULL", source,
flags=re.DOTALL
+ )
+ # Use the execution parser, not a keyword gate: table subqueries
+ # can start with FROM, including in otherwise SELECT-less SQL.
+ try:
+ script = SQLScript(f"SELECT {source}",
self.db_engine_spec.engine)
+ except SupersetParseError:
+ ambiguous = True
+ else:
+ if any(
+ statement.has_subquery() and statement.tables
+ for statement in script.statements
+ ):
+ return True
+ return None if ambiguous else False
+
+ def _get_query_sql_expressions( # noqa: C901
+ self,
+ query_obj: QueryObjectDict,
+ template_processor: BaseTemplateProcessor,
+ granularity: str | None,
+ removed_filters: list[str],
+ unrendered_templates: list[str],
+ ) -> list[str]:
+ """Render SQL-bearing chart fields without building a chart or its
prequeries.
+
+ Render only possible subqueries, once per source, matching execution.
+ An adhoc column resolving to saved metadata also checks its 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] = []
+ from_template_rendered = False
+
+ def record_unrendered_template(expression: str) -> None:
+ """Keep opaque macro outputs viewer-scoped without executing
them."""
+ if self._may_have_rls_subquery(expression, template_processor) is
None:
+ unrendered_templates.append(expression)
+ return
+ try:
+ template = template_processor.env.parse(expression)
+ except TemplateError:
+ unrendered_templates.append(expression)
+ return
+ string_inputs = any(
+ isinstance(node, nodes.TemplateData)
+ or (isinstance(node, nodes.Const) and isinstance(node.value,
str))
+ or (
+ isinstance(node, nodes.Name)
+ and isinstance(self.template_params_dict.get(node.name),
str)
+ )
+ for node in template.find_all(nodes.Node)
+ )
+ unknown_string_operation = string_inputs and any(
Review Comment:
At 0c4fa24d190053c1b7f104a76cfe71f5eebb76ed this was a real shared-key case.
The prefilter treated `{{ 1 * 2 }}` as inspectable and never rendered it, so it
never looked up the saved column `2`. Execution rendered `2`, resolved the
saved column, and injected each viewer's RLS on its subquery.
Fixed in 33579b7e917cde2c947b23c20147d52279d18fad, with the regression test
added in 5d0b51d71b307b139098ecc616466df12f8b31a8 (head): the redesign removed
the node-type prefilter entirely, so no operator list needs to be complete.
Each adhoc expression is rendered as execution renders it, through
`render_for_discovery`, with template parameters and plain values real and
every macro stubbed. The rendered, stripped output then goes through the same
saved-column lookup as execution (`_get_adhoc_column_rls_expression`), and the
saved expression is rendered and collected for RLS the same way. Any template
that cannot be rendered without running a macro gets the viewer-scoped key.
Test: `test_rendered_saved_column_name_cache_keys_are_user_scoped`, 32
cases. A saved column `2` has expression `(SELECT MAX(col1) FROM physical)` and
is used as an adhoc column or an adhoc filter column (`expressionType: SQL`).
The templates are `{{ 1 * 2 }}`, Sub, Div, FloorDiv, Mod, Pow, Neg, a
conditional expression, a comparison, `if`/`for`/`set` blocks, a macro, list
indexing, dict attribute and a string method. Two viewers with different RLS on
`physical` execute on SQLite. The test asserts different rows, each viewer's
predicate in the key, and different keys. All 32 pass at
5d0b51d71b307b139098ecc616466df12f8b31a8. Run against
0c4fa24d190053c1b7f104a76cfe71f5eebb76ed in a scratch worktree, 28 fail with an
empty key (e.g. `assert [] == ['["rls", null, "main", "physical", "(col1 =
1)"]']`). The 4 that pass there are the `if`-block and macro cases. Also at
5d0b51d71b307b139098ecc616466df12f8b31a8: virtual_dataset_connections_test.py
and jinja_context_test.py, 1263 passed; te
sts/unit_tests/sql, 3126 passed at 100% coverage.
Siblings checked:
- `superset/models/helpers.py:4204` (adhoc `sqlExpression` render feeding
the saved-column lookup, columns and filters) — fixed in
33579b7e917cde2c947b23c20147d52279d18fad
- `superset/connectors/sqla/models.py:2572` (SqlaTable
`_get_adhoc_column_rls_expression`: rendered, stripped name resolved against
saved columns, saved expression rendered for RLS) — fixed in
33579b7e917cde2c947b23c20147d52279d18fad
- `superset/models/helpers.py:4173` (shared `render` used by every sink,
including saved columns, metrics and filters) — fixed in
33579b7e917cde2c947b23c20147d52279d18fad
- `superset/models/helpers.py:4134` (Query
`_get_adhoc_column_rls_expression`) — not affected: Query executes adhoc SQL
literally and has no saved-column expressions to resolve
(`test_query_column_metadata_does_not_expand_saved_expressions`).
- `superset/connectors/sqla/models.py:1026` (`get_column`) — not affected:
the same lookup execution uses; collection calls it on the same rendered,
stripped text.
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. This is
by design and does not share keys.
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]
