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

diff --git a/doc/build/changelog/unreleased_20/13105.rst b/doc/build/changelog/unreleased_20/13105.rst
new file mode 100644 (file)
index 0000000..dc5560d
--- /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 7b31b0c0c41c104d552b044e76f710c023cbfe60..05be03c5a65fe8931dcef496efcf42ffe9fa5512 100644 (file)
--- 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 16f29d92f5b0efe7b4368111282188879c480b08..ce8ff27a2d172b7eaef9d19f654b0057b3826082 100644 (file)
--- 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 1366cb7a32e33cd8bc60e129c183c07a2fcab398..5f3aa87a3bb68fb277407506261a5c692263b9d7 100644 (file)
--- 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",