kaxil commented on code in PR #76688:
URL: https://github.com/apache/airflow/pull/76688#discussion_r4504510304
##########
providers/common/ai/src/airflow/providers/common/ai/utils/sql_validation.py:
##########
@@ -433,7 +433,8 @@ def validate_sql(
:param allowed_types: Tuple of sqlglot expression types to permit.
Defaults to ``(Select, Union, Intersect, Except)``. When supplied, the
caller takes full control of the allow-list and
``allow_read_only_metadata``
- is ignored.
+ is ignored. The deep scan for data-modifying nodes still runs unless
one of
+ the supplied types is itself a data-modifying type (e.g.
``exp.Insert``).
Review Comment:
The comment above `_DATA_MODIFYING_NODES` still says custom `allowed_types`
bypass the deep scan entirely, and `docs/stability.rst` still scopes the
nested-write check to the default types. Can you update both to the new rule?
##########
providers/common/ai/src/airflow/providers/common/ai/utils/sql_validation.py:
##########
@@ -445,18 +446,17 @@ def validate_sql(
:raises SQLSafetyError: If the SQL is empty, contains disallowed statement
types,
or has multiple statements when not permitted.
"""
- # A caller-supplied ``allowed_types`` is an explicit opt-out of the curated
- # read-only defaults (and the data-modifying deep scan). Otherwise we use
the
- # read-only defaults, optionally widened with metadata statements, and keep
- # the deep scan on.
if allowed_types is None:
types: tuple[type[exp.Expr], ...] = DEFAULT_ALLOWED_TYPES
if allow_read_only_metadata:
types = types + READ_ONLY_METADATA_TYPES
- run_data_modifying_scan = True
else:
types = allowed_types
- run_data_modifying_scan = types == DEFAULT_ALLOWED_TYPES
+ # Only an allow-list that admits a write opts out of the deep scan.
Comparing to
+ # DEFAULT_ALLOWED_TYPES would also switch it off for a narrower or
reordered
+ # read-only list such as ``(exp.Select,)``, letting ``WITH d AS (DELETE
...)
+ # SELECT ...`` through.
+ run_data_modifying_scan = not any(issubclass(t, _DATA_MODIFYING_NODES) for
t in types)
Review Comment:
This only checks one direction. `(exp.Select, exp.DML)` admits an INSERT at
the `isinstance` check below, but `issubclass(exp.DML, _DATA_MODIFYING_NODES)`
is False, so the scan stays on and rejects that same INSERT as "read-only
mode". That config works on 1.1.0. Could the scan look only for the nodes the
list doesn't admit, e.g. `denied = tuple(n for n in _DATA_MODIFYING_NODES if
not issubclass(n, types))`? That would also stop `(exp.Select, exp.Insert)`
from letting a DELETE CTE through, which passes both before and after this
change.
##########
providers/common/ai/tests/unit/common/ai/utils/test_sql_validation.py:
##########
@@ -220,6 +220,26 @@ def test_deep_scan_runs_with_explicit_default_types(self):
dialect="postgres",
)
+ @pytest.mark.parametrize(
+ "allowed_types",
+ [
+ pytest.param((exp.Select,), id="narrower"),
Review Comment:
Could you add rows where the list admits a write, like `(exp.Select,
exp.Insert)` and `(exp.Select, exp.DML)`, and assert the INSERT passes? Nothing
covers the opt-out side today, so changing `any` to `all` keeps every test
green. `DEFAULT_ALLOWED_TYPES` could also be a param here and replace
`test_deep_scan_runs_with_explicit_default_types`.
--
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]
