aminghadersohi commented on code in PR #45681:
URL: https://github.com/apache/superset/pull/45681#discussion_r4391351233
##########
superset/connectors/sqla/models.py:
##########
@@ -2886,22 +2887,30 @@ class and any keys added via `ExtraCache`.
filtered_query_obj = {
k: v for k, v in query_obj.items() if k in SQLA_QUERY_KEYS
}
- sqla_query = self.get_sqla_query(**cast(Any, filtered_query_obj))
+ sqla_query = self.get_sqla_query(
+ **cast(Any, filtered_query_obj), execute_prequeries=False
+ )
extra_cache_keys += sqla_query.extra_cache_keys
+ resolve_catalog = cache(self.database.get_default_catalog)
+ default_schema = None
# For virtual datasets, include RLS predicates in the cache key
if self.is_virtual and self.sql:
- default_schema = self.database.get_default_schema(self.catalog)
+ default_schema = self._get_rls_schema()
rls_predicates = collect_rls_predicates_for_sql(
self.sql,
self.database,
self.catalog,
- self.schema or default_schema or "",
+ default_schema,
exclude_dataset_id=self.id,
+ resolve_catalog=resolve_catalog,
)
# Add each predicate as a separate cache key component
extra_cache_keys.extend(rls_predicates)
+ extra_cache_keys.extend(
+ self._get_query_rls_cache_keys(query_obj, default_schema,
resolve_catalog)
Review Comment:
Fixed in 30e118ee16464af7d368c189404515d0c51b4c03. The shared collector tags
every predicate with its catalog, schema and table before either datasource
merges cache components. This also covers multiple protected tables inside the
dataset SQL itself.
Regression:
`tests/unit_tests/connectors/sqla/virtual_dataset_connections_test.py::test_swapped_table_rules_have_distinct_cache_keys`
fails in all four cases at efe9841047e1bebe7ccedd152621a4336ed4d9be: execution
produces 210 and 120, but the cache keys compare equal. All four pass with the
fix, covering SqlaTable and Query, both the reported expression-subquery
scenario and the dataset-SQL sibling. Docker validation: 3,279 tests passed
across connectors, models, RLS utilities and SQL parsing/metric normalization;
another 40 schema-resolution tests passed. Changed-file pre-commit passed.
Siblings checked:
- superset/connectors/sqla/models.py:2871 — fixed in
30e118ee16464af7d368c189404515d0c51b4c03
- superset/models/sql_lab.py:395 — fixed in
30e118ee16464af7d368c189404515d0c51b4c03
- superset/models/helpers.py:4141 — fixed in
30e118ee16464af7d368c189404515d0c51b4c03
- superset/utils/rls.py:309 — fixed in
30e118ee16464af7d368c189404515d0c51b4c03
- superset/common/query_context_processor.py:455 — not affected: keeps
datasource extra keys and the outer dataset's RLS in separate components;
consumes the corrected datasource collectors.
- superset/common/query_context_processor.py:529 — not affected: annotation
RLS is mapped by source identity, not merged across expression tables.
- superset/security/manager.py:6116 — not affected: returns rules for one
outer datasource, separately from expression-table contributions.
- superset/datasource/api.py:90 — not affected: this cache material binds
one datasource UID and its own RLS; it does not merge expression-subquery
predicates.
- superset/semantic_layers/cache_host.py:80 — not affected: the identity
includes principal, roles and guest-token scope; no expression-predicate merge.
- superset/semantic_layers/models.py:886 — not affected: contributes
semantic metadata/version discriminators, not RLS predicates.
##########
superset/models/helpers.py:
##########
@@ -529,13 +531,27 @@ def validate_adhoc_subquery(
)
)
+ # Scope traversal needs a query root to find tables inside bare
expressions
+ # such as MAX((SELECT ...)), including Query-backed adhoc columns.
+ if not parsed_statement.is_select() and not
parsed_statement.is_set_operation():
+ parsed_statement = SQLStatement(f"SELECT {sql}", engine)
+ wrapped = True
+
# enforce RLS rules in any relevant tables
+ default_schema = get_rls_schema(database, catalog, default_schema, sql)
rls_applied = apply_rls(database, catalog, default_schema,
parsed_statement)
# Only regenerate the SQL if RLS predicates were actually applied;
# unnecessary round-tripping through sqlglot can alter dialect-specific
# syntax.
- return parsed_statement.format() if rls_applied else sql
+ if not rls_applied:
+ return sql
+ rewritten = parsed_statement.format()
+ if wrapped:
+ return re.split(r"SELECT\s+", rewritten, maxsplit=1,
flags=re.IGNORECASE)[
Review Comment:
Fixed in 30e118ee16464af7d368c189404515d0c51b4c03. Rewritten expressions are
unwrapped from the parsed SELECT projections, including DISTINCT fragments,
rather than searching formatted SQL for SELECT. I also replaced the sibling
keyword splits and made the denylist wrapper recognize a commented SELECT
structurally. Intact synthetic prefixes retain source-preserving removal;
normalized comments use AST extraction without introducing another dialect
rewrite.
Regressions in
`tests/unit_tests/connectors/sqla/virtual_dataset_connections_test.py`:
- `test_comment_keywords_survive_rls_expression_unwrap`: all 16 cases fail
at efe9841047e1bebe7ccedd152621a4336ed4d9be and pass with the fix, including
the exact comment-prefixed saved expression, applicable RLS, the stored
DISTINCT fallback, rendered expressions and SELECT/ORDER BY wrappers on both
datasource types. The test executes the resulting SQL and checks the restricted
result.
- `test_comment_unwrap_preserves_numeric_precision`: all eight cases fail
before and pass with the fix; unwrapping must produce valid SQL without adding
a ROUND/DECIMAL cast.
Docker validation: 3,279 tests passed across connectors, models, RLS
utilities and SQL parsing/metric normalization; another 40 schema-resolution
tests passed. Changed-file pre-commit passed.
Siblings checked:
- superset/models/helpers.py:520 — fixed in
30e118ee16464af7d368c189404515d0c51b4c03
- superset/models/helpers.py:570 — fixed in
30e118ee16464af7d368c189404515d0c51b4c03
- superset/models/helpers.py:638 — fixed in
30e118ee16464af7d368c189404515d0c51b4c03
- superset/models/helpers.py:2216 — fixed in
30e118ee16464af7d368c189404515d0c51b4c03
- superset/models/helpers.py:2263 — fixed in
30e118ee16464af7d368c189404515d0c51b4c03
- superset/models/helpers.py:2280 — fixed in
30e118ee16464af7d368c189404515d0c51b4c03
- superset/models/helpers.py:2301 — fixed in
30e118ee16464af7d368c189404515d0c51b4c03
- superset/models/helpers.py:2318 — fixed in
30e118ee16464af7d368c189404515d0c51b4c03
- superset/sql/parse.py:1801 — fixed in
30e118ee16464af7d368c189404515d0c51b4c03
- superset/connectors/sqla/models.py:1293 — not affected: save-time
validation checks a synthetic SELECT but does not unwrap or return the
rewritten SQL.
- superset/connectors/sqla/models.py:1487 — not affected: stored column and
metric getters delegate to the shared validators without their own prefix
stripping.
- superset/models/helpers.py:5066 — not affected: stored-column conversion
delegates to the shared validators without independent stripping.
- superset/models/helpers.py:4185 — not affected: the cache collector parses
the synthetic SELECT for table discovery and never unwraps it.
- superset/connectors/sqla/models.py:175 — not affected: remaining split
handles qualified identifiers, not SELECT wrappers; URL splitting at line 2115
is unrelated too.
- superset/models/sql_lab.py:254 — not affected: remaining split formats a
timestamp, not SQL.
- superset/models/helpers.py:3260 — not affected: remaining splits parse
time ranges, not SQL wrappers.
--
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]
