From 6fbdd3602136fe7589238c657f61de60b85c3c54 Mon Sep 17 00:00:00 2001 From: Mike Bayer Date: Tue, 3 Feb 2026 16:11:13 -0500 Subject: [PATCH] parse ON UPDATE / ON DELETE in any order Fixed an issue in the PostgreSQL dialect where foreign key constraint reflection would incorrectly swap or fail to capture ``onupdate`` and ``ondelete`` values when these clauses appeared in a different order than expected in the constraint definition. This issue primarily affected PostgreSQL-compatible databases such as CockroachDB, which may return ``ON DELETE`` before ``ON UPDATE`` in the constraint definition string. The reflection logic now correctly parses both clauses regardless of their ordering. Fixes: #13105 Change-Id: I1331b433f713632e84ae6a34467806e080b8003e --- doc/build/changelog/unreleased_20/13105.rst | 12 + lib/sqlalchemy/dialects/postgresql/base.py | 59 +++- test/dialect/postgresql/test_dialect.py | 93 ------ test/dialect/postgresql/test_reflection.py | 330 ++++++++++++++++++-- 4 files changed, 360 insertions(+), 134 deletions(-) create mode 100644 doc/build/changelog/unreleased_20/13105.rst diff --git a/doc/build/changelog/unreleased_20/13105.rst b/doc/build/changelog/unreleased_20/13105.rst new file mode 100644 index 0000000000..dc5560d149 --- /dev/null +++ b/doc/build/changelog/unreleased_20/13105.rst @@ -0,0 +1,12 @@ +.. change:: + :tags: bug, postgresql + :tickets: 13105 + + Fixed an issue in the PostgreSQL dialect where foreign key constraint + reflection would incorrectly swap or fail to capture ``onupdate`` and + ``ondelete`` values when these clauses appeared in a different order than + expected in the constraint definition. This issue primarily affected + PostgreSQL-compatible databases such as CockroachDB, which may return ``ON + DELETE`` before ``ON UPDATE`` in the constraint definition string. The + reflection logic now correctly parses both clauses regardless of their + ordering. diff --git a/lib/sqlalchemy/dialects/postgresql/base.py b/lib/sqlalchemy/dialects/postgresql/base.py index 7b31b0c0c4..05be03c5a6 100644 --- a/lib/sqlalchemy/dialects/postgresql/base.py +++ b/lib/sqlalchemy/dialects/postgresql/base.py @@ -4826,15 +4826,59 @@ class PGDialect(default.DefaultDialect): r"FOREIGN KEY \((.*?)\) " rf"REFERENCES (?:({qtoken})\.)?({qtoken})\(((?:{qtoken}(?: *, *)?)+)\)" # noqa: E501 r"[\s]?(MATCH (FULL|PARTIAL|SIMPLE)+)?" - r"[\s]?(ON UPDATE " - r"(CASCADE|RESTRICT|NO ACTION|SET NULL|SET DEFAULT)+)?" - r"[\s]?(ON DELETE " + r"[\s]?(?:ON (UPDATE|DELETE) " + r"(CASCADE|RESTRICT|NO ACTION|" + r"SET (?:NULL|DEFAULT)(?:\s\(.+\))?)+)?" + r"[\s]?(?:ON (UPDATE|DELETE) " r"(CASCADE|RESTRICT|NO ACTION|" r"SET (?:NULL|DEFAULT)(?:\s\(.+\))?)+)?" r"[\s]?(DEFERRABLE|NOT DEFERRABLE)?" r"[\s]?(INITIALLY (DEFERRED|IMMEDIATE)+)?" ) + def _parse_fk(self, condef): + FK_REGEX = self._fk_regex_pattern + m = re.search(FK_REGEX, condef).groups() + + ( + constrained_columns, + referred_schema, + referred_table, + referred_columns, + _, + match, + upddelkey1, + upddelval1, + upddelkey2, + upddelval2, + deferrable, + _, + initially, + ) = m + + onupdate = ( + upddelval1 + if upddelkey1 == "UPDATE" + else upddelval2 if upddelkey2 == "UPDATE" else None + ) + ondelete = ( + upddelval1 + if upddelkey1 == "DELETE" + else upddelval2 if upddelkey2 == "DELETE" else None + ) + + return ( + constrained_columns, + referred_schema, + referred_table, + referred_columns, + match, + onupdate, + ondelete, + deferrable, + initially, + ) + def get_multi_foreign_keys( self, connection, @@ -4851,8 +4895,6 @@ class PGDialect(default.DefaultDialect): query = self._foreing_key_query(schema, has_filter_names, scope, kind) result = connection.execute(query, params) - FK_REGEX = self._fk_regex_pattern - fkeys = defaultdict(list) default = ReflectionDefaults.foreign_keys for table_name, conname, condef, conschema, comment in result: @@ -4862,23 +4904,18 @@ class PGDialect(default.DefaultDialect): fkeys[(schema, table_name)] = default() continue table_fks = fkeys[(schema, table_name)] - m = re.search(FK_REGEX, condef).groups() ( constrained_columns, referred_schema, referred_table, referred_columns, - _, match, - _, onupdate, - _, ondelete, deferrable, - _, initially, - ) = m + ) = self._parse_fk(condef) if deferrable is not None: deferrable = True if deferrable == "DEFERRABLE" else False diff --git a/test/dialect/postgresql/test_dialect.py b/test/dialect/postgresql/test_dialect.py index 16f29d92f5..ce8ff27a2d 100644 --- a/test/dialect/postgresql/test_dialect.py +++ b/test/dialect/postgresql/test_dialect.py @@ -3,7 +3,6 @@ import dataclasses import datetime import logging import logging.handlers -import re from sqlalchemy import BigInteger from sqlalchemy import bindparam @@ -60,101 +59,9 @@ from sqlalchemy.testing.assertions import expect_raises from sqlalchemy.testing.assertions import ne_ -def _fk_expected( - constrained_columns, - referred_table, - referred_columns, - referred_schema=None, - match=None, - onupdate=None, - ondelete=None, - deferrable=None, - initially=None, -): - return ( - constrained_columns, - referred_schema, - referred_table, - referred_columns, - f"MATCH {match}" if match else None, - match, - f"ON UPDATE {onupdate}" if onupdate else None, - onupdate, - f"ON DELETE {ondelete}" if ondelete else None, - ondelete, - deferrable, - f"INITIALLY {initially}" if initially else None, - initially, - ) - - class DialectTest(fixtures.TestBase): """python-side dialect tests.""" - @testing.combinations( - ( - "FOREIGN KEY (tid) REFERENCES some_table(id)", - _fk_expected("tid", "some_table", "id"), - ), - ( - 'FOREIGN KEY (tid) REFERENCES "(2)"(id)', - _fk_expected("tid", '"(2)"', "id"), - ), - ( - 'FOREIGN KEY (tid) REFERENCES some_table("(2)")', - _fk_expected("tid", "some_table", '"(2)"'), - ), - ( - 'FOREIGN KEY (tid1, tid2) REFERENCES some_table("(2)", "(3)")', - _fk_expected("tid1, tid2", "some_table", '"(2)", "(3)"'), - ), - ( - "FOREIGN KEY (tid) REFERENCES some_table(id) " - "DEFERRABLE INITIALLY DEFERRED", - _fk_expected( - "tid", - "some_table", - "id", - deferrable="DEFERRABLE", - initially="DEFERRED", - ), - ), - ( - "FOREIGN KEY (tid1, tid2) " - "REFERENCES some_schema.some_table(id1, id2)", - _fk_expected( - "tid1, tid2", - "some_table", - "id1, id2", - referred_schema="some_schema", - ), - ), - ( - "FOREIGN KEY (tid1, tid2) " - "REFERENCES some_schema.some_table(id1, id2) " - "MATCH FULL " - "ON UPDATE CASCADE " - "ON DELETE CASCADE " - "DEFERRABLE INITIALLY DEFERRED", - _fk_expected( - "tid1, tid2", - "some_table", - "id1, id2", - referred_schema="some_schema", - onupdate="CASCADE", - ondelete="CASCADE", - match="FULL", - deferrable="DEFERRABLE", - initially="DEFERRED", - ), - ), - ) - def test_fk_parsing(self, condef, expected): - FK_REGEX = postgresql.dialect()._fk_regex_pattern - groups = re.search(FK_REGEX, condef).groups() - - eq_(groups, expected) - def test_range_constructor(self): """test kwonly arguments in the range constructor, as we had to do dataclasses backwards compat operations""" diff --git a/test/dialect/postgresql/test_reflection.py b/test/dialect/postgresql/test_reflection.py index 1366cb7a32..5f3aa87a3b 100644 --- a/test/dialect/postgresql/test_reflection.py +++ b/test/dialect/postgresql/test_reflection.py @@ -889,6 +889,306 @@ class ArrayReflectionTest(fixtures.TablesTest): assert_is_integer_array(table.c.datasss.type) +class RegexTest(fixtures.TestBase): + + @staticmethod + def _fk_match( + constrained_columns, + referred_table, + referred_columns, + referred_schema=None, + match=None, + onupdate=None, + ondelete=None, + deferrable=None, + initially=None, + ): + return ( + constrained_columns, + referred_schema, + referred_table, + referred_columns, + match, + onupdate, + ondelete, + deferrable, + initially, + ) + + @testing.combinations( + ( + "FOREIGN KEY (tid) REFERENCES some_table(id)", + _fk_match("tid", "some_table", "id"), + ), + ( + 'FOREIGN KEY (tid) REFERENCES "(2)"(id)', + _fk_match("tid", '"(2)"', "id"), + ), + ( + 'FOREIGN KEY (tid) REFERENCES some_table("(2)")', + _fk_match("tid", "some_table", '"(2)"'), + ), + ( + 'FOREIGN KEY (tid1, tid2) REFERENCES some_table("(2)", "(3)")', + _fk_match("tid1, tid2", "some_table", '"(2)", "(3)"'), + ), + ( + "FOREIGN KEY (tid) REFERENCES some_table(id) " + "DEFERRABLE INITIALLY DEFERRED", + _fk_match( + "tid", + "some_table", + "id", + deferrable="DEFERRABLE", + initially="DEFERRED", + ), + ), + ( + "FOREIGN KEY (tid1, tid2) " + "REFERENCES some_schema.some_table(id1, id2)", + _fk_match( + "tid1, tid2", + "some_table", + "id1, id2", + referred_schema="some_schema", + ), + ), + ( + "FOREIGN KEY (tid1, tid2) " + "REFERENCES some_schema.some_table(id1, id2) " + "MATCH FULL " + "ON UPDATE CASCADE " + "ON DELETE CASCADE " + "DEFERRABLE INITIALLY DEFERRED", + _fk_match( + "tid1, tid2", + "some_table", + "id1, id2", + referred_schema="some_schema", + onupdate="CASCADE", + ondelete="CASCADE", + match="FULL", + deferrable="DEFERRABLE", + initially="DEFERRED", + ), + ), + # Test that FK regex correctly parses ON UPDATE/DELETE in any order. + # + # This addresses an issue where CockroachDB may return ON DELETE before + # ON UPDATE in the constraint definition, which was causing the values + # to be swapped or missed entirely. + # + # Relates to issue #13105. + ( + "FOREIGN KEY (col) REFERENCES ref_table(ref_col) " + "ON UPDATE CASCADE ON DELETE RESTRICT", + _fk_match( + "col", + "ref_table", + "ref_col", + onupdate="CASCADE", + ondelete="RESTRICT", + ), + ), + ( + "FOREIGN KEY (col) REFERENCES ref_table(ref_col) " + "ON DELETE RESTRICT ON UPDATE CASCADE", + _fk_match( + "col", + "ref_table", + "ref_col", + onupdate="CASCADE", + ondelete="RESTRICT", + ), + ), + ( + "FOREIGN KEY (tid, fk_id_del_set_default) " + "REFERENCES pktable(tid, id) ON UPDATE CASCADE " + "ON DELETE SET DEFAULT (fk_id_del_set_default)", + _fk_match( + "tid, fk_id_del_set_default", + "pktable", + "tid, id", + onupdate="CASCADE", + ondelete="SET DEFAULT (fk_id_del_set_default)", + ), + ), + ( + "FOREIGN KEY (tid, fk_id_del_set_default) " + "REFERENCES pktable(tid, id) " + "ON DELETE SET DEFAULT (fk_id_del_set_default) " + "ON UPDATE CASCADE", + _fk_match( + "tid, fk_id_del_set_default", + "pktable", + "tid, id", + onupdate="CASCADE", + ondelete="SET DEFAULT (fk_id_del_set_default)", + ), + ), + ( + "FOREIGN KEY (tid, fk_id_del_set_default) " + "REFERENCES pktable(tid, id) " + "ON DELETE SET DEFAULT (fk_id_del_set_default)", + _fk_match( + "tid, fk_id_del_set_default", + "pktable", + "tid, id", + ondelete="SET DEFAULT (fk_id_del_set_default)", + ), + ), + ( + "FOREIGN KEY (col) REFERENCES ref_table(ref_col) " + "ON UPDATE SET NULL ON DELETE SET DEFAULT", + _fk_match( + "col", + "ref_table", + "ref_col", + onupdate="SET NULL", + ondelete="SET DEFAULT", + ), + ), + ( + "FOREIGN KEY (col) REFERENCES ref_table(ref_col) " + "ON DELETE SET DEFAULT ON UPDATE SET NULL", + _fk_match( + "col", + "ref_table", + "ref_col", + onupdate="SET NULL", + ondelete="SET DEFAULT", + ), + ), + ( + "FOREIGN KEY (col) REFERENCES ref_table(ref_col) " + "ON UPDATE NO ACTION ON DELETE CASCADE", + _fk_match( + "col", + "ref_table", + "ref_col", + onupdate="NO ACTION", + ondelete="CASCADE", + ), + ), + ( + "FOREIGN KEY (col) REFERENCES ref_table(ref_col) " + "ON DELETE CASCADE ON UPDATE NO ACTION", + _fk_match( + "col", + "ref_table", + "ref_col", + onupdate="NO ACTION", + ondelete="CASCADE", + ), + ), + ( + "FOREIGN KEY (col) REFERENCES ref_table(ref_col) " + "ON UPDATE CASCADE", + _fk_match( + "col", + "ref_table", + "ref_col", + onupdate="CASCADE", + ), + ), + ( + "FOREIGN KEY (col) REFERENCES ref_table(ref_col) " + "ON DELETE RESTRICT", + _fk_match( + "col", + "ref_table", + "ref_col", + ondelete="RESTRICT", + ), + ), + ( + "FOREIGN KEY (col) REFERENCES ref_table(ref_col) " + "MATCH FULL ON UPDATE RESTRICT ON DELETE CASCADE " + "DEFERRABLE INITIALLY DEFERRED", + _fk_match( + "col", + "ref_table", + "ref_col", + match="FULL", + onupdate="RESTRICT", + ondelete="CASCADE", + deferrable="DEFERRABLE", + initially="DEFERRED", + ), + ), + ( + "FOREIGN KEY (col) REFERENCES ref_table(ref_col) " + "MATCH FULL ON DELETE CASCADE ON UPDATE RESTRICT " + "DEFERRABLE INITIALLY DEFERRED", + _fk_match( + "col", + "ref_table", + "ref_col", + match="FULL", + onupdate="RESTRICT", + ondelete="CASCADE", + deferrable="DEFERRABLE", + initially="DEFERRED", + ), + ), + # Test cases from test_fk_regex_unicode_patterns + # Tests unicode identifier handling + # This specifically tests the improved qtoken regex pattern to + # support PostgreSQL variants such as CockroachDB that deliver + # unquoted unicode identifiers in FK constraint definitions, whereas + # PostgreSQL itself delivers the same identifiers with quotes. + ( + 'FOREIGN KEY ("tid") REFERENCES some_table(id)', + _fk_match('"tid"', "some_table", "id"), + ), + ( + 'FOREIGN KEY (tid) REFERENCES "some_table"(id)', + _fk_match("tid", '"some_table"', "id"), + ), + ( + 'FOREIGN KEY ("測試") REFERENCES unitable1("méil")', + _fk_match('"測試"', "unitable1", '"méil"'), + ), + ( + "FOREIGN KEY (測試) REFERENCES unitable1(méil)", + _fk_match("測試", "unitable1", "méil"), + ), + ( + 'FOREIGN KEY ("col_名前") REFERENCES "table_テーブル"("ref_參考")', + _fk_match('"col_名前"', '"table_テーブル"', '"ref_參考"'), + ), + ( + "FOREIGN KEY (普通_column) REFERENCES 普通_table(普通_ref)", + _fk_match("普通_column", "普通_table", "普通_ref"), + ), + ( + 'FOREIGN KEY ("tid1", tid2) ' + 'REFERENCES some_schema.some_table("id1", id2)', + _fk_match( + '"tid1", tid2', + "some_table", + '"id1", id2', + referred_schema="some_schema", + ), + ), + ( + 'FOREIGN KEY ("測試1", 測試2) ' + 'REFERENCES "schema_スキーマ"."table_表"("ref1_參考", ref2_参考)', + _fk_match( + '"測試1", 測試2', + '"table_表"', + '"ref1_參考", ref2_参考', + referred_schema='"schema_スキーマ"', + ), + ), + ) + def test_fk_parsing(self, condef, expected): + + received = postgresql.dialect()._parse_fk(condef) + + eq_(received, expected) + + class ReflectionTest( ReflectionFixtures, AssertsCompiledSQL, ComparesIndexes, fixtures.TestBase ): @@ -980,36 +1280,6 @@ class ReflectionTest( ) ) - @testing.combinations( - "FOREIGN KEY (tid) REFERENCES some_table(id)", - 'FOREIGN KEY ("tid") REFERENCES some_table(id)', - 'FOREIGN KEY (tid) REFERENCES "some_table"(id)', - 'FOREIGN KEY ("測試") REFERENCES unitable1("méil")', - "FOREIGN KEY (測試) REFERENCES unitable1(méil)", - 'FOREIGN KEY ("col_名前") REFERENCES "table_テーブル"("ref_參考")', - "FOREIGN KEY (普通_column) REFERENCES 普通_table(普通_ref)", - ( - 'FOREIGN KEY ("tid1", tid2) ' - 'REFERENCES some_schema.some_table("id1", id2)' - ), - ( - 'FOREIGN KEY ("測試1", 測試2) ' - 'REFERENCES "schema_スキーマ"."table_表"("ref1_參考", ref2_参考)' - ), - ) - def test_fk_regex_unicode_patterns(self, condef): - """Test that FK regex pattern handles unicode identifiers. - - This specifically tests the improved qtoken regex pattern to - support PostgreSQL variants such as CockroachDB that deliver - unquoted unicode identifiers in FK constraint definitions, whereas - PostgreSQL itself delivers the same identifiers with quotes. - - """ - FK_REGEX = postgresql.dialect()._fk_regex_pattern - match = re.search(FK_REGEX, condef) - self.assert_(match is not None, f"Expected regex to match: {condef}") - def test_reflect_default_over_128_chars(self, metadata, connection): Table( "t", -- 2.47.3