From dba04cdd232ab72704df58cff791d52c1c99bc90 Mon Sep 17 00:00:00 2001 From: Morgan Fainberg Date: Mon, 28 Mar 2016 10:50:12 -0700 Subject: [PATCH] Correct `role_name` constraint dropping The `role_name` constraint was not properly dropped in some cases because the unique constraint was not consistently named. In all cases we must search for the constraint expected, not assume the name of the constraint will be consistent (especially from older installs that have been moved forward in releases). This change fixes migration 88, updates 96 (for backport to stable/mitaka) This is being overly cautious, but specifically is to handle the case where someone performed the upgrade and manually fixed the migration resulting in duplicated constraints in the role_table. Note that migration 100 was not backported since it was a placeholder migration. Conflicts: keystone/tests/unit/test_sql_upgrade.py Co-Authored-By: "Matthew Thode" Change-Id: Ie0dc3d2449bace57d3e9323b281a2abd2ad0c983 closes-bug: #1562934 (cherry picked from commit 276e57e2083dcad8cbabf9aa9b3bd06c6079c415) --- .../versions/088_domain_specific_roles.py | 31 ++++++- .../versions/096_drop_role_name_constraint.py | 50 ++++++++++ keystone/tests/unit/test_sql_upgrade.py | 93 +++++++++++++++++++ 3 files changed, 170 insertions(+), 4 deletions(-) create mode 100644 keystone/common/sql/migrate_repo/versions/096_drop_role_name_constraint.py diff --git a/keystone/common/sql/migrate_repo/versions/088_domain_specific_roles.py b/keystone/common/sql/migrate_repo/versions/088_domain_specific_roles.py index 92f2b906dc..8b792dfa75 100644 --- a/keystone/common/sql/migrate_repo/versions/088_domain_specific_roles.py +++ b/keystone/common/sql/migrate_repo/versions/088_domain_specific_roles.py @@ -13,9 +13,10 @@ import migrate import sqlalchemy as sql -_ROLE_NAME_OLD_CONSTRAINT = 'ixu_role_name' + _ROLE_NAME_NEW_CONSTRAINT = 'ixu_role_name_domain_id' _ROLE_TABLE_NAME = 'role' +_ROLE_NAME_COLUMN_NAME = 'name' _DOMAIN_ID_COLUMN_NAME = 'domain_id' _NULL_DOMAIN_ID = '<>' @@ -27,10 +28,32 @@ def upgrade(migrate_engine): role_table = sql.Table(_ROLE_TABLE_NAME, meta, autoload=True) domain_id = sql.Column(_DOMAIN_ID_COLUMN_NAME, sql.String(64), nullable=False, server_default=_NULL_DOMAIN_ID) - role_table.create_column(domain_id) - migrate.UniqueConstraint(role_table.c.name, - name=_ROLE_NAME_OLD_CONSTRAINT).drop() + # NOTE(morganfainberg): the `role_name` unique constraint is not + # guaranteed to be a fixed name, such as 'ixu_role_name`, so we need to + # search for the correct constraint that only affects role_table.c.name + # and drop that constraint. + to_drop = None + if migrate_engine.name == 'mysql': + for c in role_table.indexes: + if (c.unique and len(c.columns) == 1 and + _ROLE_NAME_COLUMN_NAME in c.columns): + to_drop = c + break + else: + for c in role_table.constraints: + if len(c.columns) == 1 and _ROLE_NAME_COLUMN_NAME in c.columns: + to_drop = c + break + + if to_drop is not None: + migrate.UniqueConstraint(role_table.c.name, + name=to_drop.name).drop() + + # perform changes after constraint is dropped. + if 'domain_id' not in role_table.columns: + # Only create the column if it doesn't already exist. + role_table.create_column(domain_id) migrate.UniqueConstraint(role_table.c.name, role_table.c.domain_id, diff --git a/keystone/common/sql/migrate_repo/versions/096_drop_role_name_constraint.py b/keystone/common/sql/migrate_repo/versions/096_drop_role_name_constraint.py new file mode 100644 index 0000000000..0156de2172 --- /dev/null +++ b/keystone/common/sql/migrate_repo/versions/096_drop_role_name_constraint.py @@ -0,0 +1,50 @@ +# 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. + +import migrate +import sqlalchemy as sql + +_ROLE_TABLE_NAME = 'role' +_ROLE_NAME_COLUMN_NAME = 'name' + + +def upgrade(migrate_engine): + meta = sql.MetaData() + meta.bind = migrate_engine + + role_table = sql.Table(_ROLE_TABLE_NAME, meta, autoload=True) + + # NOTE(morganfainberg): the `role_name` unique constraint is not + # guaranteed to be named 'ixu_role_name', so we need to search for the + # correct constraint that only affects role_table.c.name and drop + # that constraint. + # + # This is an idempotent change that reflects the fix to migration + # 88 if the role_name unique constraint was not named consistently and + # someone manually fixed the migrations / db without dropping the + # old constraint. + to_drop = None + if migrate_engine.name == 'mysql': + for c in role_table.indexes: + if (c.unique and len(c.columns) == 1 and + _ROLE_NAME_COLUMN_NAME in c.columns): + to_drop = c + break + else: + for c in role_table.constraints: + if len(c.columns) == 1 and _ROLE_NAME_COLUMN_NAME in c.columns: + to_drop = c + break + + if to_drop is not None: + migrate.UniqueConstraint(role_table.c.name, + name=to_drop.name).drop() diff --git a/keystone/tests/unit/test_sql_upgrade.py b/keystone/tests/unit/test_sql_upgrade.py index 207122ef64..5ca12f66fc 100644 --- a/keystone/tests/unit/test_sql_upgrade.py +++ b/keystone/tests/unit/test_sql_upgrade.py @@ -32,6 +32,7 @@ WARNING:: import json import uuid +import migrate from migrate.versioning import api as versioning_api from migrate.versioning import repository import mock @@ -494,6 +495,11 @@ class SqlUpgradeTests(SqlMigrateBase): table = sqlalchemy.Table(table_name, meta, autoload=True) return index_name in [idx.name for idx in table.indexes] + def does_constraint_exist(self, table_name, constraint_name): + meta = sqlalchemy.MetaData(bind=self.engine) + table = sqlalchemy.Table(table_name, meta, autoload=True) + return constraint_name in [con.name for con in table.constraints] + def test_endpoint_policy_upgrade(self): self.assertTableDoesNotExist('policy_association') self.upgrade(81) @@ -1059,6 +1065,93 @@ class SqlUpgradeTests(SqlMigrateBase): # assert id column is an integer (after) self.assertEqual('INTEGER', str(revocation_event_table.c.id.type)) + def _add_unique_constraint_to_role_name(self, + constraint_name='ixu_role_name'): + meta = sqlalchemy.MetaData() + meta.bind = self.engine + role_table = sqlalchemy.Table('role', meta, autoload=True) + migrate.UniqueConstraint(role_table.c.name, + name=constraint_name).create() + + def _drop_unique_constraint_to_role_name(self, + constraint_name='ixu_role_name'): + role_table = sqlalchemy.Table('role', self.metadata, autoload=True) + migrate.UniqueConstraint(role_table.c.name, + name=constraint_name).drop() + + def test_migration_88_drops_unique_constraint(self): + self.upgrade(87) + if self.engine.name == 'mysql': + self.assertTrue(self.does_index_exist('role', 'ixu_role_name')) + else: + self.assertTrue(self.does_constraint_exist('role', + 'ixu_role_name')) + self.upgrade(88) + if self.engine.name == 'mysql': + self.assertFalse(self.does_index_exist('role', 'ixu_role_name')) + else: + self.assertFalse(self.does_constraint_exist('role', + 'ixu_role_name')) + + def test_migration_88_inconsistent_constraint_name(self): + self.upgrade(87) + self._drop_unique_constraint_to_role_name() + + constraint_name = uuid.uuid4().hex + self._add_unique_constraint_to_role_name( + constraint_name=constraint_name) + + if self.engine.name == 'mysql': + self.assertTrue(self.does_index_exist('role', constraint_name)) + self.assertFalse(self.does_index_exist('role', 'ixu_role_name')) + else: + self.assertTrue(self.does_constraint_exist('role', + constraint_name)) + self.assertFalse(self.does_constraint_exist('role', + 'ixu_role_name')) + + self.upgrade(88) + if self.engine.name == 'mysql': + self.assertFalse(self.does_index_exist('role', constraint_name)) + self.assertFalse(self.does_index_exist('role', 'ixu_role_name')) + else: + self.assertFalse(self.does_constraint_exist('role', + constraint_name)) + self.assertFalse(self.does_constraint_exist('role', + 'ixu_role_name')) + + def test_migration_96(self): + self.upgrade(95) + if self.engine.name == 'mysql': + self.assertFalse(self.does_index_exist('role', 'ixu_role_name')) + else: + self.assertFalse(self.does_constraint_exist('role', + 'ixu_role_name')) + + self.upgrade(96) + if self.engine.name == 'mysql': + self.assertFalse(self.does_index_exist('role', 'ixu_role_name')) + else: + self.assertFalse(self.does_constraint_exist('role', + 'ixu_role_name')) + + def test_migration_96_constraint_exists(self): + self.upgrade(95) + self._add_unique_constraint_to_role_name() + + if self.engine.name == 'mysql': + self.assertTrue(self.does_index_exist('role', 'ixu_role_name')) + else: + self.assertTrue(self.does_constraint_exist('role', + 'ixu_role_name')) + + self.upgrade(96) + if self.engine.name == 'mysql': + self.assertFalse(self.does_index_exist('role', 'ixu_role_name')) + else: + self.assertFalse(self.does_constraint_exist('role', + 'ixu_role_name')) + class VersionTests(SqlMigrateBase):