openstack
/
keystone
Code Issues Proposed changes

sql: Fix incorrect constraints 

This is our first test of the autogeneration tooling integrated in
change I17c9ff9508c5e2bd9521c18973af093d7550ab5a. To generate this, we
simply removed all but one of the "skipped" constraints defined in the
'env.py' file and then ran the following command within a virtualenv:

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

The resulting schema migrations then needed some manual tweaks to remove
"please adjust!" comments (don't worry, the commands were correct).

Change-Id: Ie1be3df78189f4165079a43d0a9050fcece6989b
Signed-off-by: Stephen Finucane <sfinucan@redhat.com>
This commit is contained in:
Stephen Finucane 2022-08-02 12:23:33 +01:00
parent 1bcf8cee0d
commit 56c47d0a39
7 changed files with 159 additions and 59 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

27
keystone/common/sql/migrations/env.py
View File

@ -50,31 +50,8 @@ def include_object(object, name, type_, reflected, compare_to):
)
BORKED_UNIQUE_CONSTRAINTS = (
# removed constraints
# duplicate constraints on primary key columns
('project_tag', ['project_id', 'name']),
(
'trust',
[
'trustor_user_id',
'trustee_user_id',
'project_id',
'impersonation',
'expires_at',
],
),
# added constraints
('access_rule', ['external_id']),
(
'trust',
[
'trustor_user_id',
'trustee_user_id',
'project_id',
'impersonation',
'expires_at',
'expires_at_int',
],
),
)
BORKED_FK_CONSTRAINTS = (
@ -122,7 +99,7 @@ def include_object(object, name, type_, reflected, compare_to):
# affected item. However, we only want to skip the actual things we know
# about untl we have enough time to fix them. These issues are listed in
# keystone.tests.unit.common.sql.test_upgrades.KeystoneModelsMigrationsSync
# However, this isn't a bug issues since the test is more specific and will
# However, this isn't an issue since the test is more specific and will
# catch other issues and anyone making changes to the columns and hoping to
# autogenerate them would need to fix the latent issue first anyway.
if type_ == 'column':

2
keystone/common/sql/migrations/versions/CONTRACT_HEAD
View File

@ -1 +1 @@
e25ffa003242
99de3849d860

2
keystone/common/sql/migrations/versions/EXPAND_HEAD
View File

@ -1 +1 @@
29e87d24a316
b4f8b3f584e0

36
keystone/common/sql/migrations/versions/bobcat/contract/99de3849d860_fix_incorrect_constraints.py Normal file
View File

@ -0,0 +1,36 @@
# 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.
"""Fix incorrect constraints.
Revision ID: 99de3849d860
Revises: e25ffa003242
Create Date: 2022-08-02 12:23:25.525035
"""
from alembic import op
# revision identifiers, used by Alembic.
revision = '99de3849d860'
down_revision = 'e25ffa003242'
branch_labels = None
depends_on = None
def upgrade():
with op.batch_alter_table('access_rule', schema=None) as batch_op:
batch_op.drop_constraint('access_rule_external_id_key', type_='unique')
with op.batch_alter_table('trust', schema=None) as batch_op:
batch_op.drop_constraint(
'duplicate_trust_constraint_expanded', type_='unique'
)

40
keystone/common/sql/migrations/versions/bobcat/expand/b4f8b3f584e0_fix_incorrect_constraints.py Normal file
View File

@ -0,0 +1,40 @@
# 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.
"""Fix incorrect constraints.
Revision ID: b4f8b3f584e0
Revises: 29e87d24a316
Create Date: 2022-08-02 12:23:25.520570
"""
from alembic import op
# revision identifiers, used by Alembic.
revision = 'b4f8b3f584e0'
down_revision = '29e87d24a316'
branch_labels = None
depends_on = None
def upgrade():
with op.batch_alter_table('trust', schema=None) as batch_op:
batch_op.create_unique_constraint(
'duplicate_trust_constraint',
[
'trustor_user_id',
'trustee_user_id',
'project_id',
'impersonation',
'expires_at',
],
)

35
keystone/tests/unit/common/sql/test_upgrades.py
View File

@ -147,44 +147,15 @@ 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']),
(
'trust',
[
'trustor_user_id',
'trustee_user_id',
'project_id',
'impersonation',
'expires_at',
],
),
):
continue # skip
# FIXME(stephenfin): These have a different name on PostgreSQL.
# Resolve by renaming the constraint on the models.
if element[0] == 'remove_constraint':
if (
element[1].table.name,
[x.name for x in element[1].columns],
) in (
('access_rule', ['external_id']),
(
'trust',
[
'trustor_user_id',
'trustee_user_id',
'project_id',
'impersonation',
'expires_at',
'expires_at_int',
],
),
):
continue # skip

76
keystone/tests/unit/test_sql_banned_operations.py
View File

@ -18,6 +18,7 @@ import fixtures
from oslo_db.sqlalchemy import enginefacade
from oslo_db.sqlalchemy import test_fixtures
from oslo_log import log as logging
import sqlalchemy
from keystone.common import sql
from keystone.common.sql import upgrades
@ -155,6 +156,12 @@ class KeystoneMigrationsWalk(
for branch_label in revision.branch_labels:
banned_ops.extend(self.BANNED_OPS[branch_label])
# SQLite migrations are running in batch mode, which mean we recreate a
# table in all migrations. As such, we can't really blacklist things so
# don't even try.
if self.FIXTURE.DRIVER == 'sqlite':
banned_ops = []
with BannedDBSchemaOperations(banned_ops, version):
alembic_api.upgrade(self.config, version)
@ -178,6 +185,75 @@ class KeystoneMigrationsWalk(
"""This is a no-op migration."""
pass
# 2023.2 bobcat
_99de3849d860_removed_constraints = {
'access_rule': 'access_rule_external_id_key',
'trust': 'duplicate_trust_constraint_expanded',
}
def _pre_upgrade_99de3849d860(self, connection):
inspector = sqlalchemy.inspect(connection)
for table, constraint in (
self._99de3849d860_removed_constraints.items()
):
constraints = [
x['name'] for x in
inspector.get_unique_constraints(table)
]
self.assertIn(constraint, constraints)
def _check_99de3849d860(self, connection):
inspector = sqlalchemy.inspect(connection)
for table, constraint in (
self._99de3849d860_removed_constraints.items()
):
constraints = [
x['name'] for x in
inspector.get_unique_constraints(table)
]
self.assertNotIn(constraint, constraints)
def _pre_upgrade_b4f8b3f584e0(self, connection):
inspector = sqlalchemy.inspect(connection)
constraints = inspector.get_unique_constraints('trust')
self.assertNotIn(
'duplicate_trust_constraint',
{x['name'] for x in constraints},
)
self.assertNotIn(
[
'trustor_user_id',
'trustee_user_id',
'project_id',
'impersonation',
'expires_at',
],
{x['column_names'] for x in constraints},
)
def _check_b4f8b3f584e0(self, connection):
inspector = sqlalchemy.inspect(connection)
constraints = inspector.get_unique_constraints('trust')
self.assertIn(
'duplicate_trust_constraint',
{x['name'] for x in constraints},
)
constraint = [
x for x in constraints if x['name'] ==
'duplicate_trust_constraint'
][0]
self.assertEqual(
[
'trustor_user_id',
'trustee_user_id',
'project_id',
'impersonation',
'expires_at',
],
constraint['column_names'],
)
def test_single_base_revision(self):
"""Ensure we only have a single base revision.