sql: Remove duplicate constraints

A primary key is automatically unique, therefore if one or columns is
included in a primary key constraint there is no need to add a separate
unique constraint for these columns. Remove it.

Note that this only affects MySQL. Both SQLite and PostgreSQL appear to
ignore the duplicate unique constraint. As a result, it was necessary to
run auto-generation against MySQL instead of the default SQLite. The
actual command used was similar to what we normally do, however.

  $ python keystone/common/sql/migrations/manage.py revision \
      --autogenerate --message 'Remove duplicate constraints'

As always, the resulting schema migrations then needed some manual
tweaks to remove "please adjust!" comments and unnecessary imports but
they are correct.

Change-Id: I64252086f994901a5ebe05afec37a6afd3a192ee
Signed-off-by: Stephen Finucane <sfinucan@redhat.com>

keystone/common/sql/migrations/env.py

@@ -50,8 +50,6 @@ def include_object(object, name, type_, reflected, compare_to):
     )
 
     BORKED_UNIQUE_CONSTRAINTS = (
-        # duplicate constraints on primary key columns
-        ('project_tag', ['project_id', 'name']),
     )
 
     BORKED_FK_CONSTRAINTS = (

keystone/common/sql/migrations/versions/CONTRACT_HEAD

@@ -1 +1 @@
-99de3849d860
+c88cdce8f248

keystone/common/sql/migrations/versions/bobcat/contract/c88cdce8f248_remove_duplicate_constraints.py

@@ -0,0 +1,80 @@
+# Licensed under the Apache License, Version 2.0 (the "License"); you may
+# not use this file except in compliance with the License. You may obtain
+# a copy of the License at
+#
+#     http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+# WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+# License for the specific language governing permissions and limitations
+# under the License.
+
+"""Remove duplicate constraints.
+
+Revision ID: c88cdce8f248
+Revises: 99de3849d860
+Create Date: 2023-03-15 13:17:44.060715
+"""
+
+from alembic import op
+from sqlalchemy.engine import reflection
+
+
+# revision identifiers, used by Alembic.
+revision = 'c88cdce8f248'
+down_revision = '99de3849d860'
+branch_labels = None
+depends_on = None
+
+
+def upgrade():
+    bind = op.get_bind()
+
+    # This only affects MySQL - PostgreSQL and SQLite were smart enough to
+    # ignore the duplicate constraints
+    if bind.engine.name != 'mysql':
+        return
+
+    # We want to drop a duplicate index on the 'project_tag' table. To drop an
+    # index, we would normally just use drop_index like so:
+    #
+    #   with op.batch_alter_table('project_tag', schema=None) as batch_op:
+    #       batch_op.drop_index(foo)
+    #
+    # However, the index wasn't explicitly named so we're not sure what 'foo'
+    # is. It has two potential names:
+    #
+    # - If it was created by alembic, alembic will have left things up to the
+    #   backend, which on MySQL means the index will have the same name as the
+    #   first column the index covers [1]. Alternatively, ...
+    # - If it was created by sqlalchemy-migrate then it will be called
+    #   '{table_name}_{first_column_name}_key' [2]
+    #
+    # We need to handle both, so we need to first inspect the table to find
+    # which it is.
+    #
+    # Note that unlike MariaDB [3], MySQL [4] doesn't support the 'ALTER TABLE
+    # tbl_name DROP CONSTRAINT IF EXISTS constraint_name' syntax, which would
+    # have allowed us to avoid the inspection. Boo.
+    #
+    # [1] https://dba.stackexchange.com/a/160712
+    # [2] https://opendev.org/x/sqlalchemy-migrate/src/commit/5d1f322542cd8eb42381612765be4ed9ca8105ec/migrate/changeset/constraint.py#L199  # noqa: E501
+    # [3] https://mariadb.com/kb/en/alter-table/
+    # [4] https://dev.mysql.com/doc/refman/8.0/en/alter-table.html
+
+    inspector = reflection.Inspector.from_engine(bind)
+    indexes = inspector.get_indexes('project_tag')
+
+    index_name = None
+    for index in indexes:
+        if index['column_names'] == ['project_id', 'name']:
+            index_name = index['name']
+            break
+    else:
+        # This should never happen *but* we silently ignore it since there's no
+        # need to break user's upgrade flow, even if they've borked something
+        return
+
+    with op.batch_alter_table('project_tag', schema=None) as batch_op:
+        batch_op.drop_index(index_name)

keystone/resource/backends/sql_model.py

@@ -118,7 +118,6 @@ class ProjectTag(sql.ModelBase, sql.ModelDictMixin):
         sql.String(64), sql.ForeignKey('project.id', ondelete='CASCADE'),
         nullable=False, primary_key=True)
     name = sql.Column(sql.Unicode(255), nullable=False, primary_key=True)
-    __table_args__ = (sql.UniqueConstraint('project_id', 'name'),)
 
 
 class ProjectOption(sql.ModelBase):

keystone/tests/unit/common/sql/test_upgrades.py

@@ -147,18 +147,6 @@ class KeystoneModelsMigrationsSync(test_migrations.ModelsMigrationsSync):
                     ):
                         continue  # skip
             else:
-                # FIXME(stephenfin): These unique constraint are unecessary
-                # since the columns are already included in a primary key
-                # constraint. The constraints are ignored in PostgreSQL.
-                if element[0] == 'add_constraint':
-                    if (
-                        element[1].table.name,
-                        [x.name for x in element[1].columns],
-                    ) in (
-                        ('project_tag', ['project_id', 'name']),
-                    ):
-                        continue  # skip
-
                 # FIXME(stephenfin): These indexes are present in the
                 # migrations but not on the equivalent models. Resolve by
                 # updating the models.

keystone/tests/unit/test_sql_banned_operations.py

@@ -254,6 +254,27 @@ class KeystoneMigrationsWalk(
             constraint['column_names'],
         )
 
+    def _pre_upgrade_c88cdce8f248(self, connection):
+        if connection.engine.name != 'mysql':
+            return
+
+        # NOTE(stephenfin): Even though the migration is written to handle
+        # names generated by both alembic and sqlalchemy-migrate, we only check
+        # for the former here since we don't apply sqlalchemy-migrate
+        # migrations anymore
+        inspector = sqlalchemy.inspect(connection)
+        indexes = inspector.get_indexes('project_tag')
+        self.assertIn('project_id', {x['name'] for x in indexes})
+
+    def _check_c88cdce8f248(self, connection):
+        # This migration only applies to MySQL
+        if connection.engine.name != 'mysql':
+            return
+
+        inspector = sqlalchemy.inspect(connection)
+        indexes = inspector.get_indexes('project_tag')
+        self.assertNotIn('project_id', {x['name'] for x in indexes})
+
     def test_single_base_revision(self):
         """Ensure we only have a single base revision.