aminghadersohi commented on code in PR #45681:
URL: https://github.com/apache/superset/pull/45681#discussion_r4378423009
##########
superset/connectors/sqla/models.py:
##########
@@ -2891,7 +2891,9 @@ class and any keys added via `ExtraCache`.
# 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.schema or self.database.get_default_schema(
Review Comment:
I reproduced the missing cache-key contribution with
`test_virtual_dataset_rls_cache_uses_connection_schema`, using the real dataset
lookup and a rule on the default-schema SQLite table. Both datasource classes
returned an empty contribution for an ignored dataset schema.
Fixed in ef1127d0c33ead3fbea76c7c5292de45fbe6b699. The virtual-dataset
cache-key path uses the same `_get_rls_schema()` as enforcement and its
fail-closed retry. On non-dynamic engines that resolves the connection schema
rather than preferring the dataset field. The regression checks contributions
`(col1 = 1)` and `(col1 = 2)` for different rules and verifies they differ. Its
two cases failed before the source change and pass after it.
The original connection-count cases also pass for dynamic-schema engines and
for a statically known connection schema. A required inspector lookup on
another non-dynamic engine is retained rather than skipped.
Siblings checked:
- superset/models/helpers.py — fixed in
ef1127d0c33ead3fbea76c7c5292de45fbe6b699
- superset/connectors/sqla/models.py — fixed in
ef1127d0c33ead3fbea76c7c5292de45fbe6b699
- superset/models/sql_lab.py — fixed in
ef1127d0c33ead3fbea76c7c5292de45fbe6b699
- superset/utils/rls.py — not affected: it consumes qualified table names
and retains the required null-schema matching fallback.
- superset/security/manager.py — not affected: SQL Lab authorization already
uses the query-aware resolver and schema-permission enumeration uses stored
physical table schemas.
- superset/db_engine_specs/oceanbase.py — not affected: its reflection
fallback qualifies the metadata SQL with the supplied schema.
##########
superset/models/sql_lab.py:
##########
@@ -410,7 +410,7 @@ def get_extra_cache_keys(self, query_obj: QueryObjectDict)
-> list[Hashable]:
collect_rls_predicates_for_sql,
)
- default_schema = self.database.get_default_schema(self.catalog)
+ default_schema = self.schema or
self.database.get_default_schema(self.catalog)
Review Comment:
I confirmed the same failure for the Query variants of
`test_virtual_dataset_rls_cache_uses_connection_schema` and
`test_virtual_dataset_rls_uses_connection_schema`: the rule was absent from the
cache-key contribution and the executed query returned unfiltered rows.
Fixed in ef1127d0c33ead3fbea76c7c5292de45fbe6b699.
`Query.get_extra_cache_keys()` and inherited FROM-clause construction use the
shared runtime-schema resolver. Dynamic-schema engines retain the schema
shortcut; non-dynamic engines resolve the effective schema through the existing
query-aware logic. Both Query regressions failed before the source change and
pass after it, including the filtered result and differing RLS contributions.
The Query fail-closed retry case also passes.
The original Query connection-count cases remain at 0 connections on cache
hits and 1 on misses where the schema is dynamic or statically known. Live
external-engine testing is not claimed.
Siblings checked:
- superset/models/helpers.py — fixed in
ef1127d0c33ead3fbea76c7c5292de45fbe6b699
- superset/connectors/sqla/models.py — fixed in
ef1127d0c33ead3fbea76c7c5292de45fbe6b699
- superset/models/sql_lab.py — fixed in
ef1127d0c33ead3fbea76c7c5292de45fbe6b699
- superset/utils/rls.py — not affected: it consumes qualified table names
and retains the required null-schema matching fallback.
- superset/security/manager.py — not affected: SQL Lab authorization already
uses the query-aware resolver and schema-permission enumeration uses stored
physical table schemas.
- superset/db_engine_specs/oceanbase.py — not affected: its reflection
fallback qualifies the metadata SQL with the supplied schema.
--
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]
