kaxil commented on code in PR #76688:
URL: https://github.com/apache/airflow/pull/76688#discussion_r4504571715
##########
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:
Pushed a142da4 with a two-way check instead: the scan is off when an allowed
type is a write node or a base of one. My `denied` suggestion would have broken
`(exp.Select, exp.Merge)` and `(exp.Select, exp.Alter)`, since sqlglot nests
`Insert`/`Update` inside MERGE and `Drop` inside `ALTER ... DROP COLUMN`. So
`(exp.Select, exp.Insert)` still lets a DELETE CTE through, same as 1.1.0.
--
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]
