PR#194: SQLAlchemy: add naming conventions - fedora-qa/blockerbugs

fedora-qa / blockerbugs

#194 SQLAlchemy: add naming conventions

Merged 3 years ago by kparal. Opened 3 years ago by kparal.

feature/db_naming_conventions into develop

SQLAlchemy: add naming conventions

Kamil Páral • 3 years ago

ad2a64c

alembic/versions/bc58ec7c7d31_add_db_naming_conventions.py

file added

+102

		`@@ -0,0 +1,102 @@`
		`+ """Add DB naming conventions`
		`+`
		`+ Revision ID: bc58ec7c7d31`
		`+ Revises: d7b6117a119a`
		`+ Create Date: 2021-07-22 18:21:27.437519`
		`+`
		`+ """`
		`+ from alembic import op`
		`+`
		`+ # revision identifiers, used by Alembic.`
		`+ revision = 'bc58ec7c7d31'`
		`+ down_revision = 'd7b6117a119a'`
		`+`
		`+ # constraint renames {table_name: [(from, to)]}`
		`+ renames = {`
		`+ 'bug': [`
		`+ ('bug_pkey', 'pk_bug'),`
		`+ ('bug_milestone_id_fkey', 'fk_bug_milestone_id_milestone')`
		`+ ],`
		`+ 'build': [`
		`+ ('build_pkey', 'pk_build'),`
		`+ ],`
		`+ 'criterion': [`
		`+ ('criterion_pkey', 'pk_criterion'),`
		`+ ('criterion_milestone_id_fkey', 'fk_criterion_milestone_id_milestone'),`
		`+ ],`
		`+ 'milestone': [`
		`+ ('milestone_pkey', 'pk_milestone'),`
		`+ ('milestone_release_id_fkey', 'fk_milestone_release_id_release'),`
		`+ ('milestone_succeeds_id_fkey', 'fk_milestone_succeeds_id_milestone'),`
		`+ ],`
		`+ 'release': [`
		`+ ('release_pkey', 'pk_release'),`
		`+ ],`
		`+ 'update': [`
		`+ ('update_pkey', 'pk_update'),`
		`+ ('update_release_id_fkey', 'fk_update_release_id_release'),`
		`+ ],`
		`+ 'update_fixes': [`
		`+ ('update_fixes_bug_id_fkey', 'fk_update_fixes_bug_id_bug'),`
		`+ ('update_fixes_update_id_fkey', 'fk_update_fixes_update_id_update'),`
		`+ ],`
		`+ 'update_milestones': [`
		`+ ('update_milestones_milestone_id_fkey', 'fk_update_milestones_milestone_id_milestone'),`
		`+ ('update_milestones_update_id_fkey', 'fk_update_milestones_update_id_update'),`
		`+ ],`
		`+ 'user_info': [`
		`+ ('user_info_pkey', 'pk_user_info'),`
		`+ ],`
		`+ }`
		`+`
		`+`
		`+ def upgrade():`
		`+ # rename all constraints which are not autodetected by Alembic`
		`+ for table, renamelist in renames.items():`
		`+ for from_, to in renamelist:`
		`+ op.execute(f'ALTER TABLE "{table}" RENAME CONSTRAINT {from_} TO {to}')`
		`+`
		`+ # ### commands auto generated by Alembic - please adjust! ###`
		`+ op.drop_constraint('build_nvr_key', 'build', type_='unique')`
		`+ op.drop_constraint('build_pkg_ver_rel', 'build', type_='unique')`
		`+ op.create_unique_constraint(op.f('uq_build_nvr'), 'build', ['nvr'])`
		`+ op.create_unique_constraint('uq_build_pkg_ver_rel', 'build', ['pkg_name', 'version', 'release'])`
		`+ op.drop_constraint('milestone_blocker_tracker_key', 'milestone', type_='unique')`
		`+ op.drop_constraint('milestone_fe_tracker_key', 'milestone', type_='unique')`
		`+ op.drop_constraint('milestone_name_key', 'milestone', type_='unique')`
		`+ op.create_unique_constraint(op.f('uq_milestone_blocker_tracker'), 'milestone', ['blocker_tracker'])`
		`+ op.create_unique_constraint(op.f('uq_milestone_fe_tracker'), 'milestone', ['fe_tracker'])`
		`+ op.create_unique_constraint(op.f('uq_milestone_name'), 'milestone', ['name'])`
		`+ op.drop_constraint('user_info_api_key_hash_key', 'user_info', type_='unique')`
		`+ op.drop_constraint('user_info_bz_user_key', 'user_info', type_='unique')`
		`+ op.drop_constraint('user_info_fas_login_key', 'user_info', type_='unique')`
		`+ op.create_unique_constraint(op.f('uq_user_info_api_key_hash'), 'user_info', ['api_key_hash'])`
		`+ op.create_unique_constraint(op.f('uq_user_info_bz_user'), 'user_info', ['bz_user'])`
		`+ op.create_unique_constraint(op.f('uq_user_info_fas_login'), 'user_info', ['fas_login'])`
		`+ # ### end Alembic commands ###`
		`+`
		`+`
		`+ def downgrade():`
		`+ # ### commands auto generated by Alembic - please adjust! ###`
		`+ op.drop_constraint(op.f('uq_user_info_fas_login'), 'user_info', type_='unique')`
		`+ op.drop_constraint(op.f('uq_user_info_bz_user'), 'user_info', type_='unique')`
		`+ op.drop_constraint(op.f('uq_user_info_api_key_hash'), 'user_info', type_='unique')`
		`+ op.create_unique_constraint('user_info_fas_login_key', 'user_info', ['fas_login'])`
		`+ op.create_unique_constraint('user_info_bz_user_key', 'user_info', ['bz_user'])`
		`+ op.create_unique_constraint('user_info_api_key_hash_key', 'user_info', ['api_key_hash'])`
		`+ op.drop_constraint(op.f('uq_milestone_name'), 'milestone', type_='unique')`
		`+ op.drop_constraint(op.f('uq_milestone_fe_tracker'), 'milestone', type_='unique')`
		`+ op.drop_constraint(op.f('uq_milestone_blocker_tracker'), 'milestone', type_='unique')`
		`+ op.create_unique_constraint('milestone_name_key', 'milestone', ['name'])`
		`+ op.create_unique_constraint('milestone_fe_tracker_key', 'milestone', ['fe_tracker'])`
		`+ op.create_unique_constraint('milestone_blocker_tracker_key', 'milestone', ['blocker_tracker'])`
		`+ op.drop_constraint('uq_build_pkg_ver_rel', 'build', type_='unique')`
		`+ op.drop_constraint(op.f('uq_build_nvr'), 'build', type_='unique')`
		`+ op.create_unique_constraint('build_pkg_ver_rel', 'build', ['pkg_name', 'version', 'release'])`
		`+ op.create_unique_constraint('build_nvr_key', 'build', ['nvr'])`
		`+ # ### end Alembic commands ###`
		`+`
		`+ # rename all constraints which are not autodetected by Alembic`
		`+ for table, renamelist in renames.items():`
		`+ for from_, to in renamelist:`
		`+ op.execute(f'ALTER TABLE "{table}" RENAME CONSTRAINT {to} TO {from_}')`

blockerbugs/__init__.py

file modified

+11 -1

		`@@ -6,6 +6,7 @@`
		`from flask import Flask, render_template`
		`from flask_sqlalchemy import SQLAlchemy`
		`from flask_sqlalchemy.model import DefaultMeta`
		`+ import sqlalchemy`

		`from . import config`

		`@@ -105,7 +106,16 @@`
		`# database`
		`if app.config['SHOW_DB_URI']:`
		`app.logger.debug('using DBURI: %s' % app.config['SQLALCHEMY_DATABASE_URI'])`
		`- db: SQLAlchemy = SQLAlchemy(app)`
		`+ metadata = sqlalchemy.MetaData(`
		`+ naming_convention={`
		`+ 'pk': 'pk_%(table_name)s',`
		`+ 'fk': 'fk_%(table_name)s_%(column_0_name)s_%(referred_table_name)s',`
		`+ 'ix': 'ix_%(column_0_label)s',`
		`+ 'uq': 'uq_%(table_name)s_%(column_0_name)s',`
		`+ 'ck': 'ck_%(table_name)s_%(constraint_name)s',`
		`+ }`
		`+ )`
		`+ db: SQLAlchemy = SQLAlchemy(app=app, metadata=metadata)`
		`# https://stackoverflow.com/questions/63542818/mypy-and-inheriting-from-a-class-that-is-an-attribute-on-an-instance`
		`BaseModel: DefaultMeta = db.Model`

blockerbugs/models/build.py

file modified

+1 -1

		`@@ -36,7 +36,7 @@`
		`epoch = db.Column(db.Integer, nullable=True)`

		`__table_args__ = (UniqueConstraint('pkg_name', 'version', 'release',`
		`- name='build_pkg_ver_rel'), )`
		`+ name='uq_build_pkg_ver_rel'), )`

		`def __unicode__(self):`
		`return 'build: %s' % (self.nvr)`

kparal commented 3 years ago

Without naming conventions, column constraints created with e.g. unique=True
are not named, and therefore future edits of those constraints (or e.g. database
downgrades) fail, unless you figure out the constraint name used in your
particular database and add it into each new Alembic script.

A better approach is to define a naming convention. SQLAlchemy then names all
constraints according to this convention, and Alembic scripts work
out-of-the-box even for constraint changes and downgrades. You can read more
about it at:
https://docs.sqlalchemy.org/en/14/core/constraints.html#configuring-constraint-naming-conventions

This patch adds a default naming convention (as suggested by SQLAlchemy docs),
and adds an Alembic script that performs a one-time rename of all affected
constraints. Unique constraints were detected by Alembic's autogeneration
feature, primary and foreign keys were renamed manually.

kparal commented 3 years ago

This is a one-time somewhat painful rename, so that we can have painless DB schema upgrades in the future. I'd really appreciate if somebody with with a good DB knowledge (yes, I'm looking at you, @jskladan :smirk: ) could tell me if this PR is sane. Thanks!

zuul commented 3 years ago

Build succeeded.

blockerbugs-pytest : SUCCESS in 1m 51s

jskladan commented 3 years ago

I mean, I guess that's fine, but wouldn't it make more sense to set the naming conventions in such a way, that the current scheme (aka pgsql's ?) is codified instead of forcing a migration for all the indexes?

kparal commented 3 years ago

That didn't occur to me. I chose the naming scheme recommended by SQLAlchemy, because that's what most people seem to be using (when searching on the internets). So it seemed like a good upstream default and the constraint renames seemed to be a simple-enough change (?).

I would need a production database dump to be fully sure I get the final names right. But even then, I'm not certain if I can reverse-engineer its naming scheme properly. There are many different variables which can be used in the scheme, and some of them might yield the same value in this particular case, but they might differ from a native Postgres naming in some future cases. This is all very new to me and I'm very unsure about what I'm doing, honestly.

Is there some important benefit in trying to mimic the Postgres naming? When you say "forcing a migration for all the indexes", do you mean primary/foreign keys, or actual indexes (named ix_* in the new scheme)? I haven't found any indexes in my local database when I inspected it with pgadmin3. Do you have performance concerns regarding the migration, or something else?

jskladan commented 3 years ago

LGTM, as discussed in person

kparal commented 3 years ago

Commit 0a5b2d6 fixes this pull-request

Pull-Request has been merged by kparal

3 years ago