From 535bc8e22ecb67eaa17a7a0a61037aac6f602b21 Mon Sep 17 00:00:00 2001 From: Stephen Finucane Date: Wed, 15 Mar 2023 13:12:06 +0000 Subject: [PATCH] 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 --- keystone/common/sql/migrations/env.py | 2 - .../sql/migrations/versions/CONTRACT_HEAD | 2 +- ...8cdce8f248_remove_duplicate_constraints.py | 80 +++++++++++++++++++ keystone/resource/backends/sql_model.py | 1 - .../tests/unit/common/sql/test_upgrades.py | 12 --- .../tests/unit/test_sql_banned_operations.py | 21 +++++ 6 files changed, 102 insertions(+), 16 deletions(-) create mode 100644 keystone/common/sql/migrations/versions/bobcat/contract/c88cdce8f248_remove_duplicate_constraints.py diff --git a/keystone/common/sql/migrations/env.py b/keystone/common/sql/migrations/env.py index fcd9d224fb..4c7b63d76b 100644 --- a/keystone/common/sql/migrations/env.py +++ b/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 = ( diff --git a/keystone/common/sql/migrations/versions/CONTRACT_HEAD b/keystone/common/sql/migrations/versions/CONTRACT_HEAD index 670f0ee770..9839de2f45 100644 --- a/keystone/common/sql/migrations/versions/CONTRACT_HEAD +++ b/keystone/common/sql/migrations/versions/CONTRACT_HEAD @@ -1 +1 @@ -99de3849d860 +c88cdce8f248 diff --git a/keystone/common/sql/migrations/versions/bobcat/contract/c88cdce8f248_remove_duplicate_constraints.py b/keystone/common/sql/migrations/versions/bobcat/contract/c88cdce8f248_remove_duplicate_constraints.py new file mode 100644 index 0000000000..24692f567b --- /dev/null +++ b/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) diff --git a/keystone/resource/backends/sql_model.py b/keystone/resource/backends/sql_model.py index b6b472e8e7..ed90f63b6d 100644 --- a/keystone/resource/backends/sql_model.py +++ b/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): diff --git a/keystone/tests/unit/common/sql/test_upgrades.py b/keystone/tests/unit/common/sql/test_upgrades.py index 637520b804..e56b0dfa0b 100644 --- a/keystone/tests/unit/common/sql/test_upgrades.py +++ b/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. diff --git a/keystone/tests/unit/test_sql_banned_operations.py b/keystone/tests/unit/test_sql_banned_operations.py index 3463daa939..f01e29a45f 100644 --- a/keystone/tests/unit/test_sql_banned_operations.py +++ b/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.