EnxDev commented on code in PR #44650:
URL: https://github.com/apache/superset/pull/44650#discussion_r4106028008
##########
superset/sql/parse.py:
##########
@@ -2794,6 +2822,75 @@ 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. The body of a ``LATERAL`` or ``CROSS APPLY`` feeds the output like
a join,
+ so it is left out. A correlated sub-query is left out too: it is typically
a
+ lookup keyed to the enclosing rows, often over a table without the rule's
+ columns, which the rules would break the same way they would break a join.
The
+ outer query doesn't scope such a sub-query's tables either (UPDATING.md).
+
+ :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.)
+
+ Only the sub-query's own columns count, not those of a sub-query nested in
it
+ (which ``Scope.columns`` includes): a nested correlated sub-query doesn't
key the
+ wrapping sub-query's tables to the enclosing rows.
+
+ :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(
+ isinstance(node, exp.Column)
+ and node.table in enclosing
+ and node.table not in scope.sources
Review Comment:
Fixed in cc3ed1a8ad. `_is_correlated` now takes names from
`selected_sources` (the sub-query's own `FROM` and joins) instead of `sources`,
so `base` counts as the outer query's and your example no longer gets `b.org_id
= 1`. Covered by `correlated-to-outer-cte` in `test_rls_subquery_predicates`.
##########
superset/sql/parse.py:
##########
@@ -2794,6 +2822,75 @@ 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. The body of a ``LATERAL`` or ``CROSS APPLY`` feeds the output like
a join,
+ so it is left out. A correlated sub-query is left out too: it is typically
a
+ lookup keyed to the enclosing rows, often over a table without the rule's
+ columns, which the rules would break the same way they would break a join.
The
+ outer query doesn't scope such a sub-query's tables either (UPDATING.md).
+
+ :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.)
+
+ Only the sub-query's own columns count, not those of a sub-query nested in
it
+ (which ``Scope.columns`` includes): a nested correlated sub-query doesn't
key the
+ wrapping sub-query's tables to the enclosing rows.
+
+ :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(
+ isinstance(node, exp.Column)
+ and node.table in enclosing
Review Comment:
Yes, fixed in cc3ed1a8ad. Names are compared ignoring case on every dialect,
so `B` counts as the sub-query's own table and `secret` gets the rule. On a
case-sensitive engine, a qualifier that only matches when case is ignored
either points at one of the sub-query's own tables (stricter rules) or at no
table at all (the engine rejects the query), so it can't fail open. Covered by
`alias-shadowing-outer-table-case-insensitive`.
##########
superset/sql/parse.py:
##########
@@ -2794,6 +2822,75 @@ 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. The body of a ``LATERAL`` or ``CROSS APPLY`` feeds the output like
a join,
+ so it is left out. A correlated sub-query is left out too: it is typically
a
+ lookup keyed to the enclosing rows, often over a table without the rule's
+ columns, which the rules would break the same way they would break a join.
The
+ outer query doesn't scope such a sub-query's tables either (UPDATING.md).
+
+ :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()
Review Comment:
Fixed in cc3ed1a8ad. `_find_subquery_scopes` now only follows the CTEs in
the sub-query's own `FROM` and joins (`selected_sources`), so `l` keeps the
outer rules and `lookup` no longer gets `org_id = 1`. Covered by
`cte-joined-beside-unrelated-subquery`.
##########
superset/models/helpers.py:
##########
@@ -3799,8 +3799,13 @@ def get_from_clause(
),
self.database,
self.database.get_default_catalog(),
- exclude_dataset_id=self_id,
- include_global_guest_rls=False,
+ # at least as strict as apply_rls(), which injects
+ # this dataset's own RLS and the global guest rules
+ # into the inner SQL's sub-queries
+ exclude_dataset_id=(
+ None if statement.has_subquery() else self_id
Review Comment:
Added in cc3ed1a8ad. The fixture had no `id`, so `self_id` was already
`None` there and the assertion alone wouldn't have caught a regression. Both
tests now set `id = 99`: the sub-query one asserts `exclude_dataset_id is
None`, and `test_get_from_clause_excludes_global_guest_rls` asserts it is `99`.
Reverting the line to `self_id` fails the test.
--
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]
