kaxil commented on code in PR #74487:
URL: https://github.com/apache/airflow/pull/74487#discussion_r4234615943
##########
providers/common/ai/tests/unit/common/ai/utils/test_sql_validation.py:
##########
@@ -614,3 +617,113 @@ def test_flags_dynamic_sql_as_unverifiable(self, sql,
dialect):
scan = collect_table_references(parse_sql(sql, dialect=dialect))
assert scan.tables == []
assert scan.unverifiable_sources
+
+
+# sqlglot has no upper bound in pyproject.toml, so an upstream rename, split
or removal of an
+# ``exp`` class would otherwise make the read-only check silently stop
recognising a write.
+# The tests below turn that drift into a CI failure; if one fails after a
sqlglot bump, review
+# ``_DATA_MODIFYING_NODES`` / ``DEFAULT_ALLOWED_TYPES`` in
``utils/sql_validation.py``.
+_DIALECTS = ["postgres", "mysql", "snowflake", "bigquery", "sqlite"]
+
+_WRITE_STATEMENTS = {
+ "insert": "INSERT INTO t (a) VALUES (1)",
+ "update": "UPDATE t SET a = 1",
+ "delete": "DELETE FROM t",
+ "merge": "MERGE INTO t USING s ON t.id = s.id WHEN MATCHED THEN UPDATE SET
a = 1",
+ "drop": "DROP TABLE t",
+ "truncate": "TRUNCATE TABLE t",
+ "create": "CREATE TABLE t (id INT)",
+ "create_as_select": "CREATE TABLE t AS SELECT 1",
+ "alter": "ALTER TABLE t ADD COLUMN c INT",
+ "copy": "COPY t FROM '/tmp/x'",
+ "grant": "GRANT SELECT ON t TO u",
+ "cte_delete": "WITH d AS (DELETE FROM t RETURNING *) SELECT * FROM d",
+ "cte_insert": "WITH d AS (INSERT INTO t VALUES (1) RETURNING *) SELECT *
FROM d",
+ "select_into": "SELECT * INTO n FROM t",
+}
+
+_READ_STATEMENTS = {
+ "select": "SELECT a FROM t",
+ "cte_select": "WITH c AS (SELECT 1 AS a) SELECT * FROM c",
+ "union_all": "SELECT 1 UNION ALL SELECT 2",
+}
+
+
+class TestSqlglotDriftTripwires:
+ """Fail loudly if a sqlglot upgrade silently changes what the read-only
check recognises."""
+
+ @pytest.mark.parametrize(
+ "node_type",
+ [*DEFAULT_ALLOWED_TYPES, *READ_ONLY_METADATA_TYPES,
*_DATA_MODIFYING_NODES],
+ ids=lambda t: t.__name__,
+ )
+ def test_referenced_node_is_still_a_public_expression_class(self,
node_type):
+ assert isinstance(node_type, type)
+ assert issubclass(node_type, exp.Expression), (
Review Comment:
I'm not sure this one can fail for the drift the header comment lists. If
sqlglot removes a class, `sql_validation.py` raises `AttributeError` while
building its tuples at import, so collection breaks before this test runs.
Renames that keep an alias still pass the `getattr(...) is node_type` check,
and after a split `node_type` is the same object it was.
The case where it does fire is a false alarm. Production types these tuples
as `type[exp.Expr]`, and in sqlglot 30 `Expression` is a subclass of `Expr`. If
a node class moved to subclass `Expr` directly, this assert breaks even though
validation is fine. I'd drop the test (and "removal" from the comment on L622),
or check `issubclass(node_type, exp.Expr)` instead.
##########
providers/common/ai/tests/unit/common/ai/utils/test_sql_validation.py:
##########
@@ -614,3 +617,113 @@ def test_flags_dynamic_sql_as_unverifiable(self, sql,
dialect):
scan = collect_table_references(parse_sql(sql, dialect=dialect))
assert scan.tables == []
assert scan.unverifiable_sources
+
+
+# sqlglot has no upper bound in pyproject.toml, so an upstream rename, split
or removal of an
+# ``exp`` class would otherwise make the read-only check silently stop
recognising a write.
+# The tests below turn that drift into a CI failure; if one fails after a
sqlglot bump, review
+# ``_DATA_MODIFYING_NODES`` / ``DEFAULT_ALLOWED_TYPES`` in
``utils/sql_validation.py``.
+_DIALECTS = ["postgres", "mysql", "snowflake", "bigquery", "sqlite"]
+
+_WRITE_STATEMENTS = {
+ "insert": "INSERT INTO t (a) VALUES (1)",
+ "update": "UPDATE t SET a = 1",
+ "delete": "DELETE FROM t",
+ "merge": "MERGE INTO t USING s ON t.id = s.id WHEN MATCHED THEN UPDATE SET
a = 1",
+ "drop": "DROP TABLE t",
+ "truncate": "TRUNCATE TABLE t",
+ "create": "CREATE TABLE t (id INT)",
+ "create_as_select": "CREATE TABLE t AS SELECT 1",
+ "alter": "ALTER TABLE t ADD COLUMN c INT",
+ "copy": "COPY t FROM '/tmp/x'",
+ "grant": "GRANT SELECT ON t TO u",
+ "cte_delete": "WITH d AS (DELETE FROM t RETURNING *) SELECT * FROM d",
+ "cte_insert": "WITH d AS (INSERT INTO t VALUES (1) RETURNING *) SELECT *
FROM d",
+ "select_into": "SELECT * INTO n FROM t",
+}
+
+_READ_STATEMENTS = {
+ "select": "SELECT a FROM t",
+ "cte_select": "WITH c AS (SELECT 1 AS a) SELECT * FROM c",
+ "union_all": "SELECT 1 UNION ALL SELECT 2",
+}
+
+
+class TestSqlglotDriftTripwires:
+ """Fail loudly if a sqlglot upgrade silently changes what the read-only
check recognises."""
+
+ @pytest.mark.parametrize(
+ "node_type",
+ [*DEFAULT_ALLOWED_TYPES, *READ_ONLY_METADATA_TYPES,
*_DATA_MODIFYING_NODES],
+ ids=lambda t: t.__name__,
+ )
+ def test_referenced_node_is_still_a_public_expression_class(self,
node_type):
+ assert isinstance(node_type, type)
+ assert issubclass(node_type, exp.Expression), (
+ f"{node_type!r} is no longer an exp.Expression subclass; review
utils/sql_validation.py"
+ )
+ assert getattr(exp, node_type.__name__, None) is node_type, (
+ f"exp.{node_type.__name__} no longer resolves to the class
sql_validation.py references; "
+ "review utils/sql_validation.py"
+ )
+
+ def test_allowed_types_never_overlap_data_modifying_nodes(self):
+ """A write type slipping into the allow-list would bypass the
statement-type check."""
+ for allowed in DEFAULT_ALLOWED_TYPES:
+ for denied in _DATA_MODIFYING_NODES:
+ assert not issubclass(allowed, denied), f"{allowed.__name__}
is a kind of {denied.__name__}"
+ assert not issubclass(denied, allowed), f"{denied.__name__} is
a kind of {allowed.__name__}"
+
+ @pytest.mark.parametrize(
+ ("sql", "node_type"),
+ [
+ (_WRITE_STATEMENTS["insert"], exp.Insert),
+ (_WRITE_STATEMENTS["update"], exp.Update),
+ (_WRITE_STATEMENTS["delete"], exp.Delete),
+ (_WRITE_STATEMENTS["merge"], exp.Merge),
+ (_WRITE_STATEMENTS["drop"], exp.Drop),
+ (_WRITE_STATEMENTS["truncate"], exp.TruncateTable),
+ (_WRITE_STATEMENTS["create"], exp.Create),
+ (_WRITE_STATEMENTS["alter"], exp.Alter),
+ (_WRITE_STATEMENTS["select_into"], exp.Into),
+ ],
+ ids=["insert", "update", "delete", "merge", "drop", "truncate",
"create", "alter", "select_into"],
+ )
+ def test_write_statement_parses_to_a_denied_node(self, sql, node_type):
+ """sqlglot must keep producing the node class the deny-list names for
each write."""
+ parsed = parse_sql(sql, dialect="postgres")
+ found = [n for stmt in parsed for n in stmt.walk() if isinstance(n,
node_type)]
+ assert found, f"{sql!r} no longer parses to {node_type.__name__};
review _DATA_MODIFYING_NODES"
+ assert issubclass(node_type, _DATA_MODIFYING_NODES)
+
+ @pytest.mark.parametrize("sql", list(_READ_STATEMENTS.values()),
ids=list(_READ_STATEMENTS))
+ def test_read_statement_parses_to_an_allowed_node(self, sql):
+ assert isinstance(parse_sql(sql, dialect="postgres")[0],
DEFAULT_ALLOWED_TYPES)
Review Comment:
`validate_sql` does this same `isinstance(stmt, DEFAULT_ALLOWED_TYPES)`
check, so `test_read_statements_accepted_in_every_dialect` already covers the
postgres rows. This one can go.
##########
providers/common/ai/tests/unit/common/ai/utils/test_sql_validation.py:
##########
@@ -614,3 +617,113 @@ def test_flags_dynamic_sql_as_unverifiable(self, sql,
dialect):
scan = collect_table_references(parse_sql(sql, dialect=dialect))
assert scan.tables == []
assert scan.unverifiable_sources
+
+
+# sqlglot has no upper bound in pyproject.toml, so an upstream rename, split
or removal of an
+# ``exp`` class would otherwise make the read-only check silently stop
recognising a write.
+# The tests below turn that drift into a CI failure; if one fails after a
sqlglot bump, review
+# ``_DATA_MODIFYING_NODES`` / ``DEFAULT_ALLOWED_TYPES`` in
``utils/sql_validation.py``.
+_DIALECTS = ["postgres", "mysql", "snowflake", "bigquery", "sqlite"]
+
+_WRITE_STATEMENTS = {
+ "insert": "INSERT INTO t (a) VALUES (1)",
+ "update": "UPDATE t SET a = 1",
+ "delete": "DELETE FROM t",
+ "merge": "MERGE INTO t USING s ON t.id = s.id WHEN MATCHED THEN UPDATE SET
a = 1",
+ "drop": "DROP TABLE t",
+ "truncate": "TRUNCATE TABLE t",
+ "create": "CREATE TABLE t (id INT)",
+ "create_as_select": "CREATE TABLE t AS SELECT 1",
+ "alter": "ALTER TABLE t ADD COLUMN c INT",
+ "copy": "COPY t FROM '/tmp/x'",
+ "grant": "GRANT SELECT ON t TO u",
+ "cte_delete": "WITH d AS (DELETE FROM t RETURNING *) SELECT * FROM d",
+ "cte_insert": "WITH d AS (INSERT INTO t VALUES (1) RETURNING *) SELECT *
FROM d",
+ "select_into": "SELECT * INTO n FROM t",
+}
+
+_READ_STATEMENTS = {
+ "select": "SELECT a FROM t",
+ "cte_select": "WITH c AS (SELECT 1 AS a) SELECT * FROM c",
+ "union_all": "SELECT 1 UNION ALL SELECT 2",
+}
+
+
+class TestSqlglotDriftTripwires:
+ """Fail loudly if a sqlglot upgrade silently changes what the read-only
check recognises."""
+
+ @pytest.mark.parametrize(
+ "node_type",
+ [*DEFAULT_ALLOWED_TYPES, *READ_ONLY_METADATA_TYPES,
*_DATA_MODIFYING_NODES],
+ ids=lambda t: t.__name__,
+ )
+ def test_referenced_node_is_still_a_public_expression_class(self,
node_type):
+ assert isinstance(node_type, type)
+ assert issubclass(node_type, exp.Expression), (
+ f"{node_type!r} is no longer an exp.Expression subclass; review
utils/sql_validation.py"
+ )
+ assert getattr(exp, node_type.__name__, None) is node_type, (
+ f"exp.{node_type.__name__} no longer resolves to the class
sql_validation.py references; "
+ "review utils/sql_validation.py"
+ )
+
+ def test_allowed_types_never_overlap_data_modifying_nodes(self):
+ """A write type slipping into the allow-list would bypass the
statement-type check."""
+ for allowed in DEFAULT_ALLOWED_TYPES:
+ for denied in _DATA_MODIFYING_NODES:
+ assert not issubclass(allowed, denied), f"{allowed.__name__}
is a kind of {denied.__name__}"
+ assert not issubclass(denied, allowed), f"{denied.__name__} is
a kind of {allowed.__name__}"
+
+ @pytest.mark.parametrize(
+ ("sql", "node_type"),
+ [
+ (_WRITE_STATEMENTS["insert"], exp.Insert),
+ (_WRITE_STATEMENTS["update"], exp.Update),
+ (_WRITE_STATEMENTS["delete"], exp.Delete),
+ (_WRITE_STATEMENTS["merge"], exp.Merge),
+ (_WRITE_STATEMENTS["drop"], exp.Drop),
+ (_WRITE_STATEMENTS["truncate"], exp.TruncateTable),
+ (_WRITE_STATEMENTS["create"], exp.Create),
+ (_WRITE_STATEMENTS["alter"], exp.Alter),
+ (_WRITE_STATEMENTS["select_into"], exp.Into),
+ ],
+ ids=["insert", "update", "delete", "merge", "drop", "truncate",
"create", "alter", "select_into"],
+ )
+ def test_write_statement_parses_to_a_denied_node(self, sql, node_type):
+ """sqlglot must keep producing the node class the deny-list names for
each write."""
+ parsed = parse_sql(sql, dialect="postgres")
+ found = [n for stmt in parsed for n in stmt.walk() if isinstance(n,
node_type)]
+ assert found, f"{sql!r} no longer parses to {node_type.__name__};
review _DATA_MODIFYING_NODES"
+ assert issubclass(node_type, _DATA_MODIFYING_NODES)
+
+ @pytest.mark.parametrize("sql", list(_READ_STATEMENTS.values()),
ids=list(_READ_STATEMENTS))
+ def test_read_statement_parses_to_an_allowed_node(self, sql):
+ assert isinstance(parse_sql(sql, dialect="postgres")[0],
DEFAULT_ALLOWED_TYPES)
+
+ @pytest.mark.parametrize("dialect", _DIALECTS)
+ @pytest.mark.parametrize("sql", list(_WRITE_STATEMENTS.values()),
ids=list(_WRITE_STATEMENTS))
+ def test_write_and_ddl_statements_rejected_in_every_dialect(self, sql,
dialect):
+ with pytest.raises(SQLSafetyError):
+ validate_sql(sql, dialect=dialect)
+
+ @pytest.mark.parametrize("dialect", _DIALECTS)
+ @pytest.mark.parametrize("sql", list(_READ_STATEMENTS.values()),
ids=list(_READ_STATEMENTS))
+ def test_read_statements_accepted_in_every_dialect(self, sql, dialect):
+ assert validate_sql(sql, dialect=dialect)
+
+ @pytest.mark.parametrize("name", ["nextval", "pg_terminate_backend",
"dblink_exec"])
+ def test_side_effect_function_is_anonymous_and_rejected(self, name):
Review Comment:
`validate_sql("SELECT nextval('x')", dialect="postgres")` and `SELECT
pg_terminate_backend(1)` are both accepted today, with metadata mode on or off.
These calls are only flagged by `collect_table_references`, and the toolset
only runs that when `allowed_tables` is set (`_enforce_allowed_tables` returns
early otherwise). So "rejected", in a class framed as guarding the read-only
check, reads as if read-only mode blocks a backend kill.
Could the name say something like `..._flagged_when_allowed_tables_is_set`,
with an `assert validate_sql(sql, dialect="postgres")` so the real behaviour is
pinned? `dblink_exec` is already covered on L508, and
`test_allowed_functions_is_the_only_way_through` below repeats
`test_allowed_functions_permits_named_function` on L561.
##########
providers/common/ai/tests/unit/common/ai/utils/test_sql_validation.py:
##########
@@ -614,3 +617,113 @@ def test_flags_dynamic_sql_as_unverifiable(self, sql,
dialect):
scan = collect_table_references(parse_sql(sql, dialect=dialect))
assert scan.tables == []
assert scan.unverifiable_sources
+
+
+# sqlglot has no upper bound in pyproject.toml, so an upstream rename, split
or removal of an
+# ``exp`` class would otherwise make the read-only check silently stop
recognising a write.
+# The tests below turn that drift into a CI failure; if one fails after a
sqlglot bump, review
+# ``_DATA_MODIFYING_NODES`` / ``DEFAULT_ALLOWED_TYPES`` in
``utils/sql_validation.py``.
+_DIALECTS = ["postgres", "mysql", "snowflake", "bigquery", "sqlite"]
+
+_WRITE_STATEMENTS = {
+ "insert": "INSERT INTO t (a) VALUES (1)",
+ "update": "UPDATE t SET a = 1",
+ "delete": "DELETE FROM t",
+ "merge": "MERGE INTO t USING s ON t.id = s.id WHEN MATCHED THEN UPDATE SET
a = 1",
+ "drop": "DROP TABLE t",
+ "truncate": "TRUNCATE TABLE t",
+ "create": "CREATE TABLE t (id INT)",
+ "create_as_select": "CREATE TABLE t AS SELECT 1",
+ "alter": "ALTER TABLE t ADD COLUMN c INT",
+ "copy": "COPY t FROM '/tmp/x'",
+ "grant": "GRANT SELECT ON t TO u",
+ "cte_delete": "WITH d AS (DELETE FROM t RETURNING *) SELECT * FROM d",
+ "cte_insert": "WITH d AS (INSERT INTO t VALUES (1) RETURNING *) SELECT *
FROM d",
+ "select_into": "SELECT * INTO n FROM t",
+}
+
+_READ_STATEMENTS = {
+ "select": "SELECT a FROM t",
+ "cte_select": "WITH c AS (SELECT 1 AS a) SELECT * FROM c",
+ "union_all": "SELECT 1 UNION ALL SELECT 2",
+}
+
+
+class TestSqlglotDriftTripwires:
+ """Fail loudly if a sqlglot upgrade silently changes what the read-only
check recognises."""
+
+ @pytest.mark.parametrize(
+ "node_type",
+ [*DEFAULT_ALLOWED_TYPES, *READ_ONLY_METADATA_TYPES,
*_DATA_MODIFYING_NODES],
+ ids=lambda t: t.__name__,
+ )
+ def test_referenced_node_is_still_a_public_expression_class(self,
node_type):
+ assert isinstance(node_type, type)
+ assert issubclass(node_type, exp.Expression), (
+ f"{node_type!r} is no longer an exp.Expression subclass; review
utils/sql_validation.py"
+ )
+ assert getattr(exp, node_type.__name__, None) is node_type, (
+ f"exp.{node_type.__name__} no longer resolves to the class
sql_validation.py references; "
+ "review utils/sql_validation.py"
+ )
+
+ def test_allowed_types_never_overlap_data_modifying_nodes(self):
+ """A write type slipping into the allow-list would bypass the
statement-type check."""
+ for allowed in DEFAULT_ALLOWED_TYPES:
+ for denied in _DATA_MODIFYING_NODES:
+ assert not issubclass(allowed, denied), f"{allowed.__name__}
is a kind of {denied.__name__}"
+ assert not issubclass(denied, allowed), f"{denied.__name__} is
a kind of {allowed.__name__}"
+
+ @pytest.mark.parametrize(
+ ("sql", "node_type"),
+ [
+ (_WRITE_STATEMENTS["insert"], exp.Insert),
+ (_WRITE_STATEMENTS["update"], exp.Update),
+ (_WRITE_STATEMENTS["delete"], exp.Delete),
+ (_WRITE_STATEMENTS["merge"], exp.Merge),
+ (_WRITE_STATEMENTS["drop"], exp.Drop),
+ (_WRITE_STATEMENTS["truncate"], exp.TruncateTable),
+ (_WRITE_STATEMENTS["create"], exp.Create),
+ (_WRITE_STATEMENTS["alter"], exp.Alter),
+ (_WRITE_STATEMENTS["select_into"], exp.Into),
+ ],
+ ids=["insert", "update", "delete", "merge", "drop", "truncate",
"create", "alter", "select_into"],
+ )
+ def test_write_statement_parses_to_a_denied_node(self, sql, node_type):
+ """sqlglot must keep producing the node class the deny-list names for
each write."""
+ parsed = parse_sql(sql, dialect="postgres")
+ found = [n for stmt in parsed for n in stmt.walk() if isinstance(n,
node_type)]
+ assert found, f"{sql!r} no longer parses to {node_type.__name__};
review _DATA_MODIFYING_NODES"
+ assert issubclass(node_type, _DATA_MODIFYING_NODES)
+
+ @pytest.mark.parametrize("sql", list(_READ_STATEMENTS.values()),
ids=list(_READ_STATEMENTS))
+ def test_read_statement_parses_to_an_allowed_node(self, sql):
+ assert isinstance(parse_sql(sql, dialect="postgres")[0],
DEFAULT_ALLOWED_TYPES)
+
+ @pytest.mark.parametrize("dialect", _DIALECTS)
+ @pytest.mark.parametrize("sql", list(_WRITE_STATEMENTS.values()),
ids=list(_WRITE_STATEMENTS))
+ def test_write_and_ddl_statements_rejected_in_every_dialect(self, sql,
dialect):
+ with pytest.raises(SQLSafetyError):
+ validate_sql(sql, dialect=dialect)
Review Comment:
Most of this matrix can't catch deny-list drift. 11 of the 14 statements are
rejected by the top-level type check ("Statement type 'Insert' is not
allowed"), which fails closed, so a sqlglot rename can't get them through. Only
`cte_delete`, `cte_insert` and `select_into` reach `_DATA_MODIFYING_NODES`. The
DDL entries in that tuple exist for `allow_read_only_metadata=True`, which is
what both `SQLToolset` paths pass (`toolsets/sql.py` L522 and L556), and the
matrix never sets it.
Dropping `exp.Alter` from the deny-list leaves `DESCRIBE ALTER TABLE t ADD
COLUMN c INT` accepted in metadata mode, and the only test that notices is the
membership assert on L697. Dropping `exp.Command` fails nothing in this file.
Could you parametrize `allow_read_only_metadata` over both values, add
DESCRIBE/EXPLAIN-wrapped writes (`DESCRIBE ALTER ...`, `EXPLAIN DELETE FROM t`
on mysql), and pin `match=` per guard so a parse error or the wrong guard
doesn't count as a pass?
##########
providers/common/ai/tests/unit/common/ai/utils/test_sql_validation.py:
##########
@@ -614,3 +617,113 @@ def test_flags_dynamic_sql_as_unverifiable(self, sql,
dialect):
scan = collect_table_references(parse_sql(sql, dialect=dialect))
assert scan.tables == []
assert scan.unverifiable_sources
+
+
+# sqlglot has no upper bound in pyproject.toml, so an upstream rename, split
or removal of an
+# ``exp`` class would otherwise make the read-only check silently stop
recognising a write.
+# The tests below turn that drift into a CI failure; if one fails after a
sqlglot bump, review
+# ``_DATA_MODIFYING_NODES`` / ``DEFAULT_ALLOWED_TYPES`` in
``utils/sql_validation.py``.
+_DIALECTS = ["postgres", "mysql", "snowflake", "bigquery", "sqlite"]
+
+_WRITE_STATEMENTS = {
+ "insert": "INSERT INTO t (a) VALUES (1)",
+ "update": "UPDATE t SET a = 1",
+ "delete": "DELETE FROM t",
+ "merge": "MERGE INTO t USING s ON t.id = s.id WHEN MATCHED THEN UPDATE SET
a = 1",
+ "drop": "DROP TABLE t",
+ "truncate": "TRUNCATE TABLE t",
+ "create": "CREATE TABLE t (id INT)",
+ "create_as_select": "CREATE TABLE t AS SELECT 1",
+ "alter": "ALTER TABLE t ADD COLUMN c INT",
+ "copy": "COPY t FROM '/tmp/x'",
+ "grant": "GRANT SELECT ON t TO u",
+ "cte_delete": "WITH d AS (DELETE FROM t RETURNING *) SELECT * FROM d",
+ "cte_insert": "WITH d AS (INSERT INTO t VALUES (1) RETURNING *) SELECT *
FROM d",
+ "select_into": "SELECT * INTO n FROM t",
+}
+
+_READ_STATEMENTS = {
+ "select": "SELECT a FROM t",
+ "cte_select": "WITH c AS (SELECT 1 AS a) SELECT * FROM c",
+ "union_all": "SELECT 1 UNION ALL SELECT 2",
+}
+
+
+class TestSqlglotDriftTripwires:
+ """Fail loudly if a sqlglot upgrade silently changes what the read-only
check recognises."""
+
+ @pytest.mark.parametrize(
+ "node_type",
+ [*DEFAULT_ALLOWED_TYPES, *READ_ONLY_METADATA_TYPES,
*_DATA_MODIFYING_NODES],
+ ids=lambda t: t.__name__,
+ )
+ def test_referenced_node_is_still_a_public_expression_class(self,
node_type):
+ assert isinstance(node_type, type)
+ assert issubclass(node_type, exp.Expression), (
+ f"{node_type!r} is no longer an exp.Expression subclass; review
utils/sql_validation.py"
+ )
+ assert getattr(exp, node_type.__name__, None) is node_type, (
+ f"exp.{node_type.__name__} no longer resolves to the class
sql_validation.py references; "
+ "review utils/sql_validation.py"
+ )
+
+ def test_allowed_types_never_overlap_data_modifying_nodes(self):
+ """A write type slipping into the allow-list would bypass the
statement-type check."""
+ for allowed in DEFAULT_ALLOWED_TYPES:
+ for denied in _DATA_MODIFYING_NODES:
+ assert not issubclass(allowed, denied), f"{allowed.__name__}
is a kind of {denied.__name__}"
+ assert not issubclass(denied, allowed), f"{denied.__name__} is
a kind of {allowed.__name__}"
+
+ @pytest.mark.parametrize(
+ ("sql", "node_type"),
+ [
+ (_WRITE_STATEMENTS["insert"], exp.Insert),
+ (_WRITE_STATEMENTS["update"], exp.Update),
+ (_WRITE_STATEMENTS["delete"], exp.Delete),
+ (_WRITE_STATEMENTS["merge"], exp.Merge),
+ (_WRITE_STATEMENTS["drop"], exp.Drop),
+ (_WRITE_STATEMENTS["truncate"], exp.TruncateTable),
+ (_WRITE_STATEMENTS["create"], exp.Create),
+ (_WRITE_STATEMENTS["alter"], exp.Alter),
+ (_WRITE_STATEMENTS["select_into"], exp.Into),
+ ],
+ ids=["insert", "update", "delete", "merge", "drop", "truncate",
"create", "alter", "select_into"],
Review Comment:
The keys are written twice here, once to index `_WRITE_STATEMENTS` and again
by position in `ids`, and `create_as_select`, `cte_delete` and `cte_insert` get
no node-type check. One `{key: node_type}` dict with `ids=list(...)` would keep
them in step.
##########
providers/common/ai/tests/unit/common/ai/utils/test_sql_validation.py:
##########
@@ -20,6 +20,9 @@
from sqlglot import exp
from airflow.providers.common.ai.utils.sql_validation import (
+ _DATA_MODIFYING_NODES,
+ DEFAULT_ALLOWED_TYPES,
Review Comment:
With `DEFAULT_ALLOWED_TYPES` imported at module level now, the local import
inside `test_deep_scan_runs_with_explicit_default_types` (L217) is redundant.
Worth dropping it in this PR.
--
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]
