EnxDev commented on code in PR #44650:
URL: https://github.com/apache/superset/pull/44650#discussion_r4105270156
##########
superset/sql/parse.py:
##########
@@ -2794,6 +2821,68 @@ def resolve(scope: Scope, seen: frozenset[int]) ->
list[exp.Table]:
)
+def _find_subquery_scopes(scopes: list[Scope]) -> set[int]:
+ """
+ Find the scopes whose rows only reach a statement through a sub-query.
+
+ That is every uncorrelated ``SUBQUERY`` scope (a scalar, ``IN`` or
``EXISTS``
+ sub-query), every scope nested inside one, and every CTE one of them reads
from,
+ including the scopes nested inside that CTE. A CTE read both from a
sub-query and
+ from the statement's ``FROM`` counts as a sub-query, so its reads get the
stricter
+ rules. A correlated sub-query is keyed to the rows of an enclosing query,
like a
+ join, and the body of a ``LATERAL`` or ``CROSS APPLY`` feeds the output
like a
+ join, so both are left out.
+
+ :param scopes: The scopes of the statement, as returned by
``traverse_scope``
+ :returns: The ``id`` of each scope found
+ """
+ found: set[int] = set()
+ pending = [
+ scope
+ for scope in scopes
+ if scope.scope_type == ScopeType.SUBQUERY
+ and not (scope.parent and scope.parent.scope_type == ScopeType.UDTF)
+ and not _is_correlated(scope)
+ ]
+ while pending:
+ scope = pending.pop()
+ if id(scope) in found:
+ continue
+ found.add(id(scope))
+ pending.extend(child for child in scopes if child.parent is scope)
+ pending.extend(
+ source
+ for source in scope.sources.values()
+ if isinstance(source, Scope) and source.scope_type == ScopeType.CTE
+ )
+ return found
+
+
+def _is_correlated(scope: Scope) -> bool:
+ """
+ Does a sub-query reference a table of an enclosing query?
+
+ Only a column qualified with an enclosing table's name or alias counts,
when the
+ sub-query has no table of its own under that name. An unqualified column
can't
+ be told apart from one of the sub-query's own, so it is treated as local,
which
+ errs toward the sub-query getting the stricter rules. (``Scope``'s own
+ ``is_correlated_subquery`` treats every unqualified column as external.)
+
+ :param scope: A ``SUBQUERY`` scope
+ :returns: True if the sub-query is correlated
+ """
+ enclosing: set[str] = set()
+ parent = scope.parent
+ while parent:
+ enclosing.update(parent.sources)
+ parent = parent.parent
+ return any(
+ column.table in enclosing and column.table not in scope.sources
+ for column in scope.columns
Review Comment:
Good catch. `Scope.columns` pulls in the external columns of nested
sub-queries, so something like `(SELECT COUNT(*) FROM b WHERE EXISTS (SELECT 1
FROM c WHERE c.aid = a.id))` counted as correlated and `b` lost the rules, even
though nothing keys `b` to `a`. cc760a9b2d makes `_is_correlated` only look at
the sub-query's own columns (via `walk_in_scope`), and adds
`uncorrelated-wrapping-correlated` to `test_rls_subquery_predicates`. The inner
correlated sub-query still gets the rules because it sits inside an
uncorrelated one, which errs on the strict side.
##########
superset/sql/parse.py:
##########
@@ -2794,6 +2821,68 @@ def resolve(scope: Scope, seen: frozenset[int]) ->
list[exp.Table]:
)
+def _find_subquery_scopes(scopes: list[Scope]) -> set[int]:
+ """
+ Find the scopes whose rows only reach a statement through a sub-query.
+
+ That is every uncorrelated ``SUBQUERY`` scope (a scalar, ``IN`` or
``EXISTS``
+ sub-query), every scope nested inside one, and every CTE one of them reads
from,
+ including the scopes nested inside that CTE. A CTE read both from a
sub-query and
+ from the statement's ``FROM`` counts as a sub-query, so its reads get the
stricter
+ rules. A correlated sub-query is keyed to the rows of an enclosing query,
like a
+ join, and the body of a ``LATERAL`` or ``CROSS APPLY`` feeds the output
like a
+ join, so both are left out.
+
+ :param scopes: The scopes of the statement, as returned by
``traverse_scope``
+ :returns: The ``id`` of each scope found
+ """
+ found: set[int] = set()
+ pending = [
+ scope
+ for scope in scopes
+ if scope.scope_type == ScopeType.SUBQUERY
+ and not (scope.parent and scope.parent.scope_type == ScopeType.UDTF)
+ and not _is_correlated(scope)
Review Comment:
It's intended, but you're right that "like a join" was the wrong reason, and
I've reworded the docstring in cc760a9b2d. The outer filter doesn't narrow `n`
there. The reason to skip is the case from msyavuz's thread: a correlated
sub-query is almost always a lookup, lookup tables often don't have the rule's
column, and injecting the rule fails the whole query at execution on
AS_PREDICATE engines. A join over a second multi-tenant table has the same gap
unless its tenant column is the one the outer filter hits, so UPDATING.md calls
out both, with the workaround of giving the rule a `dataset`. If you'd rather
fail closed here I'm open to it, but it brings back that breakage.
##########
superset/utils/rls.py:
##########
@@ -83,24 +86,40 @@ def apply_rls(
# collect all RLS predicates for all tables in the query
default_catalog = database.get_default_catalog()
- predicates: dict[Table, list[Any]] = {}
- for table in parsed_statement.tables:
- table = table.qualify(catalog=catalog, schema=schema)
- predicates[table] = [
- parsed_statement.parse_predicate(predicate)
- for predicate in get_predicates_for_table(
- table,
- database,
- default_catalog,
- exclude_dataset_id=exclude_dataset_id,
- include_global_guest_rls=include_global_guest_rls,
- )
- if predicate
- ]
-
- has_predicates = any(predicates.values())
- parsed_statement.apply_rls(catalog, schema, predicates, method)
- return has_predicates
+
+ def collect_predicates(include_global: bool) -> dict[Table, list[Any]]:
+ predicates: dict[Table, list[Any]] = {}
+ for table in parsed_statement.tables:
+ table = table.qualify(catalog=catalog, schema=schema)
+ predicates[table] = [
+ parsed_statement.parse_predicate(predicate)
+ for predicate in get_predicates_for_table(
+ table,
+ database,
+ default_catalog,
+ exclude_dataset_id=exclude_dataset_id,
Review Comment:
Yes, same argument, so sub-query reads don't exclude it anymore
(cc760a9b2d). The sub-query lookup in `apply_rls` passes
`exclude_dataset_id=None`, and the fail-closed check in `get_from_clause` does
the same when the statement has a sub-query. The dataset's own RLS isn't
guest-only, so this applies to every user. To keep non-guests from paying for a
second lookup on every virtual dataset with a sub-query, it only runs when the
dataset has RLS for the current user (`_dataset_has_rls`). Covered by
`test_virtual_dataset_own_rls_applied_to_subquery`.
--
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]
