sadpandajoe commented on code in PR #45681:
URL: https://github.com/apache/superset/pull/45681#discussion_r4378796490
##########
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:
This now runs on every virtual-dataset `get_from_clause` and
`get_extra_cache_keys` for non-dynamic engines, and it reaches
`get_schema_from_engine_params`. TDengine's (and Couchbase's) implementation is
`parse.unquote(sqlalchemy_uri.database)`, which raises `TypeError` when the URL
has no database path, e.g. the documented `taosws://root:password@host:6041`. A
virtual dataset with fully qualified SQL such as `SELECT ts, value FROM
metrics.readings` therefore fails before the cache lookup, where it worked
before. Should the resolver tolerate this, or should the engine extractors
handle a missing URL database?
##########
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:
On static-schema engines whose spec has no default-schema discovery (e.g.
Druid via pydruid, where the inspector default is `None`),
`resolve_query_default_schema` returns `None` and this `or ""` makes the RLS
lookup use an empty schema. A virtual dataset with `schema="druid"` and `SELECT
* FROM events` then no longer matches the physical `druid.events` dataset's RLS
rule, so the query and its cache key run without the tenant predicate, where
previously `self.schema` qualified the table. Should an unresolved schema fall
back to `self.schema` (or fail closed) rather than `""`?
--
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]
