printedparts 0003 was an unguarded ALTER, and no test could see it
Unlike its twenty siblings it ran a bare add_column plus create_index, so `flask plugin upgrade-all` failed with "duplicate column name: gagelabtag" on any database whose tables were built by create_all and stamped nowhere - and a chain stops at its first error, so 0004 never applied either. The index is guarded by COLUMN SET rather than by name: create_all names indexes by SQLAlchemy's convention and the migration by this repo's, so a name-keyed guard would have added a second index over the same column instead of skipping. The test that should have caught it was restricted to plugins whose whole chain is a single baseline, on the reasoning that a longer chain meets a second problem - an ALTER whose column is already present - that guarding a create_table does not fix. True, but the conclusion was wrong: the answer is to guard the ALTERs too, not to stop testing the chains that have them. It now covers every post-cutover chain, and was confirmed to fail against the unguarded migration before the guard went in.
This commit is contained in:
@@ -3,6 +3,15 @@
|
|||||||
The gage lab issues WJRP asset numbers for printed parts; the internal
|
The gage lab issues WJRP asset numbers for printed parts; the internal
|
||||||
itemcode stays auto-generated, and this optional unique tag carries the
|
itemcode stays auto-generated, and this optional unique tag carries the
|
||||||
lab's number. The kiosk resolves scans/typed digits against both.
|
lab's number. The kiosk resolves scans/typed digits against both.
|
||||||
|
|
||||||
|
Guarded/idempotent: skips when the table is absent (plugin disabled) or the
|
||||||
|
column already exists (e.g. a test DB built by db.create_all() from the model).
|
||||||
|
Unguarded, this failed `flask plugin upgrade-all` on any create-all-built
|
||||||
|
database and took 0004 down with it, since a chain stops at its first error.
|
||||||
|
|
||||||
|
The index is matched by COLUMN SET, not by name: create_all names indexes by
|
||||||
|
SQLAlchemy's convention and this migration by the repo's, so a name-keyed guard
|
||||||
|
would add a second index over the same column rather than skip.
|
||||||
"""
|
"""
|
||||||
from alembic import op
|
from alembic import op
|
||||||
import sqlalchemy as sa
|
import sqlalchemy as sa
|
||||||
@@ -15,13 +24,31 @@ branch_labels = None
|
|||||||
depends_on = None
|
depends_on = None
|
||||||
|
|
||||||
|
|
||||||
|
def _indexed(inspector):
|
||||||
|
return any(tuple(ix.get('column_names') or ()) == ('gagelabtag',)
|
||||||
|
for ix in inspector.get_indexes('printeditems'))
|
||||||
|
|
||||||
|
|
||||||
def upgrade():
|
def upgrade():
|
||||||
|
inspector = sa.inspect(op.get_bind())
|
||||||
|
if 'printeditems' not in inspector.get_table_names():
|
||||||
|
return
|
||||||
|
columns = {column['name'] for column in inspector.get_columns('printeditems')}
|
||||||
|
if 'gagelabtag' not in columns:
|
||||||
op.add_column('printeditems',
|
op.add_column('printeditems',
|
||||||
sa.Column('gagelabtag', sa.String(length=50), nullable=True))
|
sa.Column('gagelabtag', sa.String(length=50), nullable=True))
|
||||||
|
inspector = sa.inspect(op.get_bind())
|
||||||
|
if not _indexed(inspector):
|
||||||
op.create_index('ix_printeditems_gagelabtag', 'printeditems',
|
op.create_index('ix_printeditems_gagelabtag', 'printeditems',
|
||||||
['gagelabtag'], unique=True)
|
['gagelabtag'], unique=True)
|
||||||
|
|
||||||
|
|
||||||
def downgrade():
|
def downgrade():
|
||||||
|
inspector = sa.inspect(op.get_bind())
|
||||||
|
if 'printeditems' not in inspector.get_table_names():
|
||||||
|
return
|
||||||
|
if _indexed(inspector):
|
||||||
op.drop_index('ix_printeditems_gagelabtag', table_name='printeditems')
|
op.drop_index('ix_printeditems_gagelabtag', table_name='printeditems')
|
||||||
|
columns = {column['name'] for column in inspector.get_columns('printeditems')}
|
||||||
|
if 'gagelabtag' in columns:
|
||||||
op.drop_column('printeditems', 'gagelabtag')
|
op.drop_column('printeditems', 'gagelabtag')
|
||||||
|
|||||||
@@ -282,18 +282,21 @@ def test_upgrade_all_on_fresh_db_is_clean_and_idempotent(tmp_path, monkeypatch):
|
|||||||
|
|
||||||
|
|
||||||
|
|
||||||
# Plugins whose whole chain is a single baseline. Their tables existing already
|
# EVERY post-cutover chain, not just the single-baseline ones. This was once
|
||||||
# is the ONLY divergence possible between schema and stamp, so upgrade-all must
|
# restricted to plugins whose whole chain is one baseline, on the reasoning that
|
||||||
# handle it end to end. A longer chain can also meet an ALTER whose column is
|
# a longer chain meets a second problem - an ALTER whose column is already
|
||||||
# already present, which no amount of guarding a create_table fixes - see the
|
# present - that guarding a create_table does not fix. True, but the conclusion
|
||||||
# test below for why that is a different problem.
|
# was wrong: the answer is to guard the ALTERs too, not to stop testing the
|
||||||
SINGLE_BASELINE_PLUGINS = tuple(
|
# chains that have them. While this was narrowed, printedparts 0003 sat
|
||||||
plugin for plugin in POST_CUTOVER_PLUGINS
|
# unguarded for a month and would fail upgrade-all on any create-all-built
|
||||||
if len(list((PLUGINS_DIR / plugin / 'migrations' / 'versions').glob('0*.py'))) == 1
|
# database, taking 0004 down with it, with no test able to see it.
|
||||||
)
|
#
|
||||||
|
# The invariant is now the whole one: a post-cutover chain must run to its head
|
||||||
|
# against a database whose tables were built from the models and stamped
|
||||||
|
# nowhere, and must do it twice.
|
||||||
|
|
||||||
|
|
||||||
@pytest.mark.parametrize('plugin', SINGLE_BASELINE_PLUGINS)
|
@pytest.mark.parametrize('plugin', POST_CUTOVER_PLUGINS)
|
||||||
def test_baseline_skips_tables_that_already_exist(plugin, tmp_path, monkeypatch):
|
def test_baseline_skips_tables_that_already_exist(plugin, tmp_path, monkeypatch):
|
||||||
"""A baseline must stamp, not fail, when its table is already there.
|
"""A baseline must stamp, not fail, when its table is already there.
|
||||||
|
|
||||||
|
|||||||
Reference in New Issue
Block a user