aminghadersohi commented on code in PR #45681:
URL: https://github.com/apache/superset/pull/45681#discussion_r4379775544
##########
superset/models/helpers.py:
##########
@@ -3994,6 +3995,22 @@ 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 unqualified RLS table names against the schema used at
execution.
+
+ Only dynamic-schema engines apply the datasource's schema to the
connection.
+ Other engines resolve it from connection parameters or the inspector,
using
+ the same query-aware resolver as SQL Lab authorization.
+ """
+ if self.db_engine_spec.supports_dynamic_schema:
+ return self.schema or
self.database.get_default_schema(self.catalog) or ""
+ return (
+ self.database.resolve_query_default_schema(
+ self.sql, self.schema, self.catalog, self.template_params_dict
+ )
+ or ""
Review Comment:
I reproduced this: an undiscoverable default dropped the configured physical
table's RLS predicate from both execution and the cache-key contribution.
Fixed in 5c707e415e9db47ddaa89e40503fbe656c882feb. `_get_rls_schema()` still
prefers a discovered connection schema on non-dynamic engines, but falls back
to `self.schema` when discovery returns no schema. That same value scopes
injection, the fail-closed retry, and both datasource cache-key paths.
Regression tests in
`tests/unit_tests/connectors/sqla/virtual_dataset_connections_test.py`:
- `test_virtual_dataset_rls_preserves_unresolved_schema` checks the executed
result is `[1]`, not `[1, 2]`.
- `test_virtual_dataset_rls_cache_preserves_unresolved_schema` checks
contributions `(col1 = 1)` and `(col1 = 2)` for different rules.
- `test_virtual_dataset_rls_retry_preserves_unresolved_schema` checks
injection failure cannot continue without the predicate.
All 12 combinations failed on ef1127d0c33ead3fbea76c7c5292de45fbe6b699 and
pass after this change: SqlaTable/Query, SQLite/Druid engine specs with schema
discovery returning None. Execution and metadata lookup use SQLite; this is not
a live Druid-driver test. The original 32 connection-count cases still pass,
asserting 0 connections on hits and 1 on misses for dynamic or statically known
connection schemas.
Siblings checked:
- superset/models/helpers.py:3998 — fixed in
5c707e415e9db47ddaa89e40503fbe656c882feb
- superset/models/helpers.py:4037 — fixed in
5c707e415e9db47ddaa89e40503fbe656c882feb
- superset/models/helpers.py:4079 — fixed in
5c707e415e9db47ddaa89e40503fbe656c882feb
- superset/connectors/sqla/models.py:2894 — fixed in
5c707e415e9db47ddaa89e40503fbe656c882feb
- superset/models/sql_lab.py:413 — fixed in
5c707e415e9db47ddaa89e40503fbe656c882feb
- superset/common/query_context_processor.py:461 — fixed in
5c707e415e9db47ddaa89e40503fbe656c882feb
- superset/utils/rls.py:234 — not affected: predicate lookup consumes the
qualified table; no independent schema selection.
- superset/models/helpers.py:2099 — not affected: denylist resolution is
separate from RLS qualification and retains the runtime resolver.
- superset/sql/execution/executor.py:818 — not affected: SQL Lab RLS already
retains opts.schema when discovery returns None.
- superset/commands/sql_lab/estimate.py:141 — not affected: SQL Lab estimate
already retains its explicit schema when discovery returns None.
- superset/security/manager.py:5270 — not affected: separate query
authorization, not a consumer of the virtual-dataset RLS helper.
- superset/semantic_layers/models.py:885 — not affected: SemanticView does
not inherit ExploreMixin or use this virtual-dataset SQL/RLS path.
##########
superset/models/helpers.py:
##########
@@ -3994,6 +3995,22 @@ 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 unqualified RLS table names against the schema used at
execution.
+
+ Only dynamic-schema engines apply the datasource's schema to the
connection.
+ Other engines resolve it from connection parameters or the inspector,
using
+ the same query-aware resolver as SQL Lab authorization.
+ """
+ if self.db_engine_spec.supports_dynamic_schema:
+ return self.schema or
self.database.get_default_schema(self.catalog) or ""
+ return (
+ self.database.resolve_query_default_schema(
+ self.sql, self.schema, self.catalog, self.template_params_dict
+ )
+ or ""
Review Comment:
I reproduced this: an undiscoverable default dropped the configured physical
table's RLS predicate from both execution and the cache-key contribution.
Fixed in 5c707e415e9db47ddaa89e40503fbe656c882feb. `_get_rls_schema()` still
prefers a discovered connection schema on non-dynamic engines, but falls back
to `self.schema` when discovery returns no schema. That same value scopes
injection, the fail-closed retry, and both datasource cache-key paths.
Regression tests in
`tests/unit_tests/connectors/sqla/virtual_dataset_connections_test.py`:
- `test_virtual_dataset_rls_preserves_unresolved_schema` checks the executed
result is `[1]`, not `[1, 2]`.
- `test_virtual_dataset_rls_cache_preserves_unresolved_schema` checks
contributions `(col1 = 1)` and `(col1 = 2)` for different rules.
- `test_virtual_dataset_rls_retry_preserves_unresolved_schema` checks
injection failure cannot continue without the predicate.
All 12 combinations failed on ef1127d0c33ead3fbea76c7c5292de45fbe6b699 and
pass after this change: SqlaTable/Query, SQLite/Druid engine specs with schema
discovery returning None. Execution and metadata lookup use SQLite; this is not
a live Druid-driver test. The original 32 connection-count cases still pass,
asserting 0 connections on hits and 1 on misses for dynamic or statically known
connection schemas.
Siblings checked:
- superset/models/helpers.py:3998 — fixed in
5c707e415e9db47ddaa89e40503fbe656c882feb
- superset/models/helpers.py:4037 — fixed in
5c707e415e9db47ddaa89e40503fbe656c882feb
- superset/models/helpers.py:4079 — fixed in
5c707e415e9db47ddaa89e40503fbe656c882feb
- superset/connectors/sqla/models.py:2894 — fixed in
5c707e415e9db47ddaa89e40503fbe656c882feb
- superset/models/sql_lab.py:413 — fixed in
5c707e415e9db47ddaa89e40503fbe656c882feb
- superset/common/query_context_processor.py:461 — fixed in
5c707e415e9db47ddaa89e40503fbe656c882feb
- superset/utils/rls.py:234 — not affected: predicate lookup consumes the
qualified table; no independent schema selection.
- superset/models/helpers.py:2099 — not affected: denylist resolution is
separate from RLS qualification and retains the runtime resolver.
- superset/sql/execution/executor.py:818 — not affected: SQL Lab RLS already
retains opts.schema when discovery returns None.
- superset/commands/sql_lab/estimate.py:141 — not affected: SQL Lab estimate
already retains its explicit schema when discovery returns None.
- superset/security/manager.py:5270 — not affected: separate query
authorization, not a consumer of the virtual-dataset RLS helper.
- superset/semantic_layers/models.py:885 — not affected: SemanticView does
not inherit ExploreMixin or use this virtual-dataset SQL/RLS path.
##########
superset/models/helpers.py:
##########
@@ -3994,6 +3995,22 @@ 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 unqualified RLS table names against the schema used at
execution.
+
+ Only dynamic-schema engines apply the datasource's schema to the
connection.
+ Other engines resolve it from connection parameters or the inspector,
using
+ the same query-aware resolver as SQL Lab authorization.
+ """
+ if self.db_engine_spec.supports_dynamic_schema:
+ return self.schema or
self.database.get_default_schema(self.catalog) or ""
+ return (
+ self.database.resolve_query_default_schema(
+ self.sql, self.schema, self.catalog, self.template_params_dict
Review Comment:
I reproduced the TypeError for pathless TDengine and Couchbase URIs in both
virtual-dataset entry points.
Fixed in 5c707e415e9db47ddaa89e40503fbe656c882feb, at the engine-extractor
level rather than by catching errors in the resolver. A missing or empty URI
database returns None, allowing the existing inspector fallback to run.
Discovery/security failures still propagate; schema decoding is unchanged when
a database is configured.
Regression tests:
-
`tests/unit_tests/connectors/sqla/virtual_dataset_connections_test.py::test_virtual_dataset_without_uri_database`:
all 8 TDengine/Couchbase × SqlaTable/Query × FROM/cache-key cases fail on
ef1127d0c33ead3fbea76c7c5292de45fbe6b699 and pass after this change.
-
`tests/unit_tests/db_engine_specs/test_schema_parameters.py::test_schema_extractor_without_database`:
missing/empty database cases; 14 fail before the change.
- `test_schema_resolution_without_database` and
`test_schema_resolution_preserves_discovery_errors`: 8 failures each before the
change, passing after it; these pin the shared query-aware fallback and error
propagation.
- `test_schema_extractor_decodes_database` preserves percent-decoding for
all eight affected implementations.
I searched every implementation with `rg -n -A30 'def
get_schema_from_engine_params' superset/db_engine_specs` (15 definitions).
Besides TDengine/Couchbase, MySQL, SingleStore, Hive, and Drill had the same
unquote(None) pattern; Snowflake and Presto had equivalent unguarded
strip(None) failures. All eight have regression coverage. These are
extractor-contract tests, not claims that every engine supports executing a
connection without a configured database.
The existing 32 connection-count cases also pass: dynamic/statically known
schemas still need 0 connections on cache hits and 1 on misses.
Inherited extractors use the guarded MySQL, Hive or Presto implementation;
Aurora includes both MySQL variants. Shared query-aware consumers receive the
guarded extractor result; no resolver exception suppression was introduced.
Siblings checked:
- superset/db_engine_specs/tdengine.py:64 — fixed in
5c707e415e9db47ddaa89e40503fbe656c882feb
- superset/db_engine_specs/couchbase.py:281 — fixed in
5c707e415e9db47ddaa89e40503fbe656c882feb
- superset/db_engine_specs/mysql.py:655 — fixed in
5c707e415e9db47ddaa89e40503fbe656c882feb
- superset/db_engine_specs/singlestore.py:544 — fixed in
5c707e415e9db47ddaa89e40503fbe656c882feb
- superset/db_engine_specs/hive.py:366 — fixed in
5c707e415e9db47ddaa89e40503fbe656c882feb
- superset/db_engine_specs/drill.py:150 — fixed in
5c707e415e9db47ddaa89e40503fbe656c882feb
- superset/db_engine_specs/snowflake.py:447 — fixed in
5c707e415e9db47ddaa89e40503fbe656c882feb
- superset/db_engine_specs/presto.py:380 — fixed in
5c707e415e9db47ddaa89e40503fbe656c882feb
- superset/db_engine_specs/base.py:1217 — not affected: returns None without
dereferencing the URI database.
- superset/db_engine_specs/athena.py:180 — not affected: returns the
nullable database directly.
- superset/db_engine_specs/bigquery.py:893 — not affected: already guards
the URI database.
- superset/db_engine_specs/doris.py:457 — not affected: already guards the
URI database before splitting/decoding.
- superset/db_engine_specs/mongodb.py:149 — not affected: uses nullable
connect-argument/URI fallbacks.
- superset/db_engine_specs/postgres.py:713 — not affected: reads
connect_args search_path, not the URI database.
- superset/db_engine_specs/starrocks.py:308 — not affected: already guards
the URI database before splitting/decoding.
- superset/db_engine_specs/mariadb.py:21 — fixed in
5c707e415e9db47ddaa89e40503fbe656c882feb
- superset/db_engine_specs/aurora.py:22 — fixed in
5c707e415e9db47ddaa89e40503fbe656c882feb
- superset/db_engine_specs/oceanbase.py:77 — fixed in
5c707e415e9db47ddaa89e40503fbe656c882feb
- superset/db_engine_specs/spark.py:44 — fixed in
5c707e415e9db47ddaa89e40503fbe656c882feb
- superset/db_engine_specs/databricks.py:1162 — fixed in
5c707e415e9db47ddaa89e40503fbe656c882feb
- superset/db_engine_specs/trino.py:73 — fixed in
5c707e415e9db47ddaa89e40503fbe656c882feb
- superset/db_engine_specs/base.py:1286 — fixed in
5c707e415e9db47ddaa89e40503fbe656c882feb
- superset/models/core.py:911 — fixed in
5c707e415e9db47ddaa89e40503fbe656c882feb
- superset/models/helpers.py:4009 — fixed in
5c707e415e9db47ddaa89e40503fbe656c882feb
- superset/connectors/sqla/models.py:2894 — fixed in
5c707e415e9db47ddaa89e40503fbe656c882feb
- superset/models/sql_lab.py:413 — fixed in
5c707e415e9db47ddaa89e40503fbe656c882feb
- superset/sql/execution/executor.py:1118 — fixed in
5c707e415e9db47ddaa89e40503fbe656c882feb
- superset/commands/sql_lab/estimate.py:136 — fixed in
5c707e415e9db47ddaa89e40503fbe656c882feb
- superset/security/manager.py:5270 — fixed in
5c707e415e9db47ddaa89e40503fbe656c882feb
- superset/mcp_service/dashboard_scope_sql.py:240 — fixed in
5c707e415e9db47ddaa89e40503fbe656c882feb
--
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]
