#194 SQLAlchemy: add naming conventions
Merged 2 years ago by kparal. Opened 2 years ago by kparal.

@@ -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_}')

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

  

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)

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.

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!

Build succeeded.

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?

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?

LGTM, as discussed in person

Commit 0a5b2d6 fixes this pull-request

Pull-Request has been merged by kparal

2 years ago