]> git.ipfire.org Git - thirdparty/sqlalchemy/sqlalchemy.git/commitdiff
set identifier length for MySQL constraints to 64
authorMike Bayer <mike_mp@zzzcomputing.com>
Sat, 30 Jan 2021 21:57:50 +0000 (16:57 -0500)
committerMike Bayer <mike_mp@zzzcomputing.com>
Sun, 31 Jan 2021 04:12:31 +0000 (23:12 -0500)
The rule to limit index names to 64 also applies to all
DDL names, such as those coming from naming conventions.
Add another limiting variable for constraint names and
create test cases against all constraint types.

Additionally, codified in the test suite MySQL's lack of
support for naming of a FOREIGN KEY constraint after
the name was given, which apparently assigns the name to an
associated KEY but not the constraint itself, until MySQL 8
and MariaDB 10.5 which appear to have resolved the
behavior.  However it's not clear how Alembic hasn't had
issues reported with this so far.

Fixed long-lived bug in MySQL dialect where the maximum identifier length
of 255 was too long for names of all types of constraints, not just
indexes, all of which have a size limit of 64. As metadata naming
conventions can create too-long names in this area, apply the limit to the
identifier generator within the DDL compiler.

Fixes: #5898
Change-Id: I79549474845dc29922275cf13321c07598dcea08

doc/build/changelog/unreleased_13/5898.rst [new file with mode: 0644]
lib/sqlalchemy/dialects/mysql/base.py
lib/sqlalchemy/engine/default.py
lib/sqlalchemy/sql/compiler.py
lib/sqlalchemy/testing/exclusions.py
lib/sqlalchemy/testing/requirements.py
lib/sqlalchemy/testing/suite/test_ddl.py
test/dialect/mysql/test_reflection.py
test/requirements.py

diff --git a/doc/build/changelog/unreleased_13/5898.rst b/doc/build/changelog/unreleased_13/5898.rst
new file mode 100644 (file)
index 0000000..a0758da
--- /dev/null
@@ -0,0 +1,9 @@
+.. change::
+    :tags: bug, mysql
+    :tickets: 5898
+
+    Fixed long-lived bug in MySQL dialect where the maximum identifier length
+    of 255 was too long for names of all types of constraints, not just
+    indexes, all of which have a size limit of 64. As metadata naming
+    conventions can create too-long names in this area, apply the limit to the
+    identifier generator within the DDL compiler.
index c350316133f9cd62e2152d164e0d924373f369bb..063f750faf988aa7c759e020054cf98ccd3bf5ef 100644 (file)
@@ -2513,6 +2513,7 @@ class MySQLDialect(default.DefaultDialect):
     # identifiers are 64, however aliases can be 255...
     max_identifier_length = 255
     max_index_name_length = 64
+    max_constraint_name_length = 64
 
     supports_native_enum = True
 
index e5a6384d24074c83d5ce79229edd8725129f1eb0..7fddf2814acac8353162f1ffc3feb57f3b67e75b 100644 (file)
@@ -127,12 +127,11 @@ class DefaultDialect(interfaces.Dialect):
 
     isolation_level = None
 
-    # length at which to truncate
-    # the name of an index.
-    # Usually None to indicate
-    # 'use max_identifier_length'.
-    # thanks to MySQL, sigh
+    # sub-categories of max_identifier_length.
+    # currently these accommodate for MySQL which allows alias names
+    # of 255 but DDL names only of 64.
     max_index_name_length = None
+    max_constraint_name_length = None
 
     supports_sane_rowcount = True
     supports_sane_multi_rowcount = True
index 353de2c480a4bfca99c5b68b37763662e57b01fa..3925b251d7f6127ae8d5f89f4ac4a6f82b560b58 100644 (file)
@@ -4793,13 +4793,19 @@ class IdentifierPreparer(object):
             name = constraint.name
 
         if isinstance(name, elements._truncated_label):
+            # calculate these at format time so that ad-hoc changes
+            # to dialect.max_identifier_length etc. can be reflected
+            # as IdentifierPreparer is long lived
             if constraint.__visit_name__ == "index":
                 max_ = (
                     self.dialect.max_index_name_length
                     or self.dialect.max_identifier_length
                 )
             else:
-                max_ = self.dialect.max_identifier_length
+                max_ = (
+                    self.dialect.max_constraint_name_length
+                    or self.dialect.max_identifier_length
+                )
             if len(name) > max_:
                 name = name[0 : max_ - 8] + "_" + util.md5_hex(name)[-4:]
         else:
index b20e174427f1e3c3669bcbffa353cd2779be7e18..6502487f0fe7789fbcf9face6fb5e5b34b4288fd 100644 (file)
@@ -40,6 +40,13 @@ class compound(object):
     def __add__(self, other):
         return self.add(other)
 
+    def as_skips(self):
+        rule = compound()
+        rule.skips.update(self.skips)
+        rule.skips.update(self.fails)
+        rule.tags.update(self.tags)
+        return rule
+
     def add(self, *others):
         copy = compound()
         copy.fails.update(self.fails)
index e5dda7d3e402ef71cf9428c2cbd58be4c50465a2..f5286c85d4bf1bf2df87c0e5ae1ede11b31885e6 100644 (file)
@@ -459,6 +459,18 @@ class SuiteRequirements(Requirements):
         foreign keys"""
         return exclusions.closed()
 
+    @property
+    def foreign_key_constraint_name_reflection(self):
+        """Target supports refleciton of FOREIGN KEY constraints and
+        will return the name of the constraint that was used in the
+        "CONSTRANT <name> FOREIGN KEY" DDL.
+
+        MySQL prior to version 8 and MariaDB prior to version 10.5
+        don't support this.
+
+        """
+        return exclusions.closed()
+
     @property
     def implicit_default_schema(self):
         """target system has a strong concept of 'default' schema that can
index c3cf854e447c95b3e7d76c3a36da2b3d9c6848f2..b3fee551e0114e69b8d353ded2c50c067a5cc1d3 100644 (file)
@@ -1,3 +1,6 @@
+import random
+
+from . import testing
 from .. import config
 from .. import fixtures
 from .. import util
@@ -5,13 +8,16 @@ from ..assertions import eq_
 from ..assertions import is_false
 from ..assertions import is_true
 from ..config import requirements
+from ..schema import Table
+from ... import CheckConstraint
 from ... import Column
+from ... import ForeignKeyConstraint
 from ... import Index
 from ... import inspect
 from ... import Integer
 from ... import schema
 from ... import String
-from ... import Table
+from ... import UniqueConstraint
 
 
 class TableDDLTest(fixtures.TestBase):
@@ -182,4 +188,194 @@ class FutureTableDDLTest(fixtures.FutureEngineMixin, TableDDLTest):
     pass
 
 
-__all__ = ("TableDDLTest", "FutureTableDDLTest")
+class LongNameBlowoutTest(fixtures.TestBase):
+    """test the creation of a variety of DDL structures and ensure
+    label length limits pass on backends
+
+    """
+
+    __backend__ = True
+
+    def fk(self, metadata, connection):
+        convention = {
+            "fk": "foreign_key_%(table_name)s_"
+            "%(column_0_N_name)s_"
+            "%(referred_table_name)s_"
+            + (
+                "_".join(
+                    "".join(random.choice("abcdef") for j in range(20))
+                    for i in range(10)
+                )
+            ),
+        }
+        metadata.naming_convention = convention
+
+        Table(
+            "a_things_with_stuff",
+            metadata,
+            Column("id_long_column_name", Integer, primary_key=True),
+            test_needs_fk=True,
+        )
+
+        cons = ForeignKeyConstraint(
+            ["aid"], ["a_things_with_stuff.id_long_column_name"]
+        )
+        Table(
+            "b_related_things_of_value",
+            metadata,
+            Column(
+                "aid",
+            ),
+            cons,
+            test_needs_fk=True,
+        )
+        actual_name = cons.name
+
+        metadata.create_all(connection)
+
+        if testing.requires.foreign_key_constraint_name_reflection.enabled:
+            insp = inspect(connection)
+            fks = insp.get_foreign_keys("b_related_things_of_value")
+            reflected_name = fks[0]["name"]
+
+            return actual_name, reflected_name
+        else:
+            return actual_name, None
+
+    def pk(self, metadata, connection):
+        convention = {
+            "pk": "primary_key_%(table_name)s_"
+            "%(column_0_N_name)s"
+            + (
+                "_".join(
+                    "".join(random.choice("abcdef") for j in range(30))
+                    for i in range(10)
+                )
+            ),
+        }
+        metadata.naming_convention = convention
+
+        a = Table(
+            "a_things_with_stuff",
+            metadata,
+            Column("id_long_column_name", Integer, primary_key=True),
+            Column("id_another_long_name", Integer, primary_key=True),
+        )
+        cons = a.primary_key
+        actual_name = cons.name
+
+        metadata.create_all(connection)
+        insp = inspect(connection)
+        pk = insp.get_pk_constraint("a_things_with_stuff")
+        reflected_name = pk["name"]
+        return actual_name, reflected_name
+
+    def ix(self, metadata, connection):
+        convention = {
+            "ix": "index_%(table_name)s_"
+            "%(column_0_N_name)s"
+            + (
+                "_".join(
+                    "".join(random.choice("abcdef") for j in range(30))
+                    for i in range(10)
+                )
+            ),
+        }
+        metadata.naming_convention = convention
+
+        a = Table(
+            "a_things_with_stuff",
+            metadata,
+            Column("id_long_column_name", Integer, primary_key=True),
+            Column("id_another_long_name", Integer),
+        )
+        cons = Index(None, a.c.id_long_column_name, a.c.id_another_long_name)
+        actual_name = cons.name
+
+        metadata.create_all(connection)
+        insp = inspect(connection)
+        ix = insp.get_indexes("a_things_with_stuff")
+        reflected_name = ix[0]["name"]
+        return actual_name, reflected_name
+
+    def uq(self, metadata, connection):
+        convention = {
+            "uq": "unique_constraint_%(table_name)s_"
+            "%(column_0_N_name)s"
+            + (
+                "_".join(
+                    "".join(random.choice("abcdef") for j in range(30))
+                    for i in range(10)
+                )
+            ),
+        }
+        metadata.naming_convention = convention
+
+        cons = UniqueConstraint("id_long_column_name", "id_another_long_name")
+        Table(
+            "a_things_with_stuff",
+            metadata,
+            Column("id_long_column_name", Integer, primary_key=True),
+            Column("id_another_long_name", Integer),
+            cons,
+        )
+        actual_name = cons.name
+
+        metadata.create_all(connection)
+        insp = inspect(connection)
+        uq = insp.get_unique_constraints("a_things_with_stuff")
+        reflected_name = uq[0]["name"]
+        return actual_name, reflected_name
+
+    def ck(self, metadata, connection):
+        convention = {
+            "ck": "check_constraint_%(table_name)s"
+            + (
+                "_".join(
+                    "".join(random.choice("abcdef") for j in range(30))
+                    for i in range(10)
+                )
+            ),
+        }
+        metadata.naming_convention = convention
+
+        cons = CheckConstraint("some_long_column_name > 5")
+        Table(
+            "a_things_with_stuff",
+            metadata,
+            Column("id_long_column_name", Integer, primary_key=True),
+            Column("some_long_column_name", Integer),
+            cons,
+        )
+        actual_name = cons.name
+
+        metadata.create_all(connection)
+        insp = inspect(connection)
+        ck = insp.get_check_constraints("a_things_with_stuff")
+        reflected_name = ck[0]["name"]
+        return actual_name, reflected_name
+
+    @testing.combinations(
+        ("fk",),
+        ("pk",),
+        ("ix",),
+        ("ck", testing.requires.check_constraint_reflection.as_skips()),
+        ("uq", testing.requires.unique_constraint_reflection.as_skips()),
+        argnames="type_",
+    )
+    def test_long_convention_name(self, type_, metadata, connection):
+        actual_name, reflected_name = getattr(self, type_)(
+            metadata, connection
+        )
+
+        assert len(actual_name) > 255
+
+        if reflected_name is not None:
+            overlap = actual_name[0 : len(reflected_name)]
+            if len(overlap) < len(actual_name):
+                eq_(overlap[0:-5], reflected_name[0 : len(overlap) - 5])
+            else:
+                eq_(overlap, reflected_name)
+
+
+__all__ = ("TableDDLTest", "FutureTableDDLTest", "LongNameBlowoutTest")
index a4f074cab46a2be47c39a03f383b29b81c94e28d..5b785a7a411867a3c7900e1dacb1946c76742ae2 100644 (file)
@@ -10,6 +10,7 @@ from sqlalchemy import DefaultClause
 from sqlalchemy import event
 from sqlalchemy import exc
 from sqlalchemy import ForeignKey
+from sqlalchemy import ForeignKeyConstraint
 from sqlalchemy import Index
 from sqlalchemy import inspect
 from sqlalchemy import Integer
@@ -1013,6 +1014,52 @@ class ReflectionTest(fixtures.TestBase, AssertsCompiledSQL):
                 ],
             )
 
+    def test_get_foreign_key_name_w_foreign_key_in_name(
+        self, metadata, connection
+    ):
+        Table(
+            "a",
+            metadata,
+            Column("id", Integer, primary_key=True),
+            mysql_engine="InnoDB",
+        )
+
+        cons = ForeignKeyConstraint(
+            ["aid"], ["a.id"], name="foreign_key_thing_with_stuff"
+        )
+        Table(
+            "b",
+            metadata,
+            Column("id", Integer, primary_key=True),
+            Column(
+                "aid",
+            ),
+            cons,
+            mysql_engine="InnoDB",
+        )
+        actual_name = cons.name
+
+        metadata.create_all(connection)
+
+        if testing.requires.foreign_key_constraint_name_reflection.enabled:
+            expected_name = actual_name
+        else:
+            expected_name = "b_ibfk_1"
+
+        eq_(
+            inspect(connection).get_foreign_keys("b"),
+            [
+                {
+                    "name": expected_name,
+                    "constrained_columns": ["aid"],
+                    "referred_schema": None,
+                    "referred_table": "a",
+                    "referred_columns": ["id"],
+                    "options": {},
+                }
+            ],
+        )
+
     @testing.requires.mysql_fully_case_sensitive
     def test_case_sensitive_reflection_dual_case_references(
         self, metadata, connection
index 1a4c5f646d066bd8c17775bf314b5705e577e3ea..bfa8d8cf62451c9f60356d622a56fef42005d287 100644 (file)
@@ -77,6 +77,14 @@ class DefaultRequirements(SuiteRequirements):
 
         return skip_if(no_support("sqlite", "not supported by database"))
 
+    @property
+    def foreign_key_constraint_name_reflection(self):
+        return fails_if(
+            lambda config: against(config, ["mysql", "mariadb"])
+            and not self._mysql_80(config)
+            and not self._mariadb_105(config)
+        )
+
     @property
     def table_ddl_if_exists(self):
         """target platform supports IF NOT EXISTS / IF EXISTS for tables."""
@@ -1546,9 +1554,16 @@ class DefaultRequirements(SuiteRequirements):
 
     def _mariadb_102(self, config):
         return (
-            against(config, "mysql")
+            against(config, ["mysql", "mariadb"])
+            and config.db.dialect._is_mariadb
+            and config.db.dialect._mariadb_normalized_version_info >= (10, 2)
+        )
+
+    def _mariadb_105(self, config):
+        return (
+            against(config, ["mysql", "mariadb"])
             and config.db.dialect._is_mariadb
-            and config.db.dialect._mariadb_normalized_version_info > (10, 2)
+            and config.db.dialect._mariadb_normalized_version_info >= (10, 5)
         )
 
     def _mysql_and_check_constraints_exist(self, config):