aminghadersohi commented on code in PR #45681:
URL: https://github.com/apache/superset/pull/45681#discussion_r4380159058
##########
superset/models/helpers.py:
##########
@@ -4015,7 +4015,9 @@ def get_from_clause(
# Apply RLS filters to virtual dataset SQL to prevent RLS bypass
# For each table referenced in the virtual dataset, apply its RLS
filters
if parsed_script.statements:
- default_schema = self.database.get_default_schema(self.catalog)
+ default_schema = self.schema or self.database.get_default_schema(
Review Comment:
I closed the remaining adhoc-expression path in
57f71df01abfd0b8fba7445ad28f004e349b188e. Adhoc, stored and rendered expression
validators use the same execution-schema resolver as the virtual dataset FROM
clause. Bare scalar expressions are wrapped for table-scope traversal; stored
select-list fragments retain the RLS rewrite, or fail closed if only a
different dialect can parse it.
Both SqlaTable and Query also collect cache-key predicates from the built
(not executed) chart query when subqueries are enabled. This covers tables
referenced only by expressions, not just tables in the dataset SQL.
Regression tests in
`tests/unit_tests/connectors/sqla/virtual_dataset_connections_test.py`:
- `test_adhoc_subquery_rls_uses_connection_schema`: 14 cases, including
column/arithmetic, metric, WHERE, HAVING, ORDER BY and filter-column subqueries
for both datasource types. Results and exact SQL predicate counts are checked.
- `test_adhoc_subquery_rls_cache_uses_connection_schema`: 14 cases; the base
SQL reads no table, and changing the expression table's rule changes the exact
predicate contribution.
- `test_expression_validation_rls_uses_connection_schema`: 8 cases for
stored, rendered, save-time and select-list-fragment validation.
- `test_stored_subquery_rls_uses_connection_schema`: 8 cases covering stored
columns/metrics, plain/Jinja-rendered, execution/cache keys.
- `test_adhoc_subquery_rls_schema_discovery_fails_closed` and
`test_stored_subquery_rls_dialect_fallback_fails_closed`: 2 cases each.
Before: a Docker scratch copy of production source at
5c707e415e9db47ddaa89e40503fbe656c882feb with these tests, using `pytest -q
tests/unit_tests/connectors/sqla/virtual_dataset_connections_test.py -k
'adhoc_subquery or expression_validation or stored_subquery or
expression_without' -m 'not manual and not llm_compliance'`, returned `48
failed, 2 passed, 104 deselected` (exit 1). The two passes are the
ordinary-expression/no-discovery controls.
After: Docker `pytest -q
tests/unit_tests/connectors/sqla/virtual_dataset_connections_test.py
tests/unit_tests/db_engine_specs/test_schema_parameters.py -m 'not manual and
not llm_compliance'` returned `194 passed` (exit 0), including all 48
regressions. The expanded `test_virtual_dataset_avoids_schema_connections`
matrix passes all 64 cases (32 original plus 32 with adhoc subqueries),
asserting exactly 0 connections on hits and 1 on misses for dynamic or
statically known connection schemas, with ALLOW_ADHOC_SUBQUERY enabled. The
broader two-worker/loadfile Docker run returned `1646 passed` (exit 0). Staged
pre-commit and pre-commit on all 16 branch-changed files both returned exit 0.
Siblings checked:
- superset/models/helpers.py:503 — fixed in
57f71df01abfd0b8fba7445ad28f004e349b188e
- superset/models/helpers.py:556 — fixed in
57f71df01abfd0b8fba7445ad28f004e349b188e
- superset/models/helpers.py:626 — fixed in
57f71df01abfd0b8fba7445ad28f004e349b188e
- superset/models/helpers.py:2134 — fixed in
57f71df01abfd0b8fba7445ad28f004e349b188e
- superset/models/helpers.py:2164 — fixed in
57f71df01abfd0b8fba7445ad28f004e349b188e
- superset/connectors/sqla/models.py:1265 — fixed in
57f71df01abfd0b8fba7445ad28f004e349b188e
- superset/connectors/sqla/models.py:1484 — fixed in
57f71df01abfd0b8fba7445ad28f004e349b188e
- superset/connectors/sqla/models.py:1733 — fixed in
57f71df01abfd0b8fba7445ad28f004e349b188e
- superset/models/sql_lab.py:498 — fixed in
57f71df01abfd0b8fba7445ad28f004e349b188e
- superset/models/helpers.py:4904 — fixed in
57f71df01abfd0b8fba7445ad28f004e349b188e
- superset/models/helpers.py:4031 — fixed in
57f71df01abfd0b8fba7445ad28f004e349b188e
- superset/connectors/sqla/models.py:2870 — fixed in
57f71df01abfd0b8fba7445ad28f004e349b188e
- superset/models/sql_lab.py:393 — fixed in
57f71df01abfd0b8fba7445ad28f004e349b188e
- superset/models/helpers.py:4069 — not affected: FROM injection and its
fail-closed retry still consume the shared resolver; the existing
connection-schema and unresolved-default tests pass.
- superset/models/helpers.py:2101 — not affected: separate query-aware
denylist resolver, not an RLS predicate lookup.
- superset/utils/rls.py:251 — not affected: predicate lookup consumes the
caller-qualified table; no independent schema selection.
- superset/utils/rls.py:308 — not affected: shared collector retains
predicate/error handling; both datasource cache paths supply the resolved
schema.
- superset/sql/execution/executor.py:1160 — not affected: separate SQL Lab
execution-options path, not an adhoc expression using dataset schema.
- superset/sql_lab.py:553 — not affected: legacy SQL Lab execution path, not
an adhoc expression using dataset schema.
- superset/commands/sql_lab/estimate.py:162 — not affected: separate SQL Lab
request-schema path, not an adhoc expression using dataset schema.
- superset/sql/parse.py:2436 — not affected: transformer consumes qualified
predicates; no independent metadata RLS lookup.
- superset/mcp_service/dashboard_scope_sql.py:197 — not affected: direct
transformer for dashboard constraints, not metadata RLS lookup.
- superset/common/query_context_processor.py:461 — not affected: synchronous
and async chart reads call the tested datasource cache-key methods; no separate
expression resolver.
- superset/semantic_layers/models.py:885 — not affected: SemanticView does
not inherit ExploreMixin or use these expression validators.
--
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]
