diff --git a/plugins/printedparts/migrations/versions/0003_gagelabtag.py b/plugins/printedparts/migrations/versions/0003_gagelabtag.py index 3f011156..8271ff74 100644 --- a/plugins/printedparts/migrations/versions/0003_gagelabtag.py +++ b/plugins/printedparts/migrations/versions/0003_gagelabtag.py @@ -3,6 +3,15 @@ The gage lab issues WJRP asset numbers for printed parts; the internal itemcode stays auto-generated, and this optional unique tag carries the 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 import sqlalchemy as sa @@ -15,13 +24,31 @@ branch_labels = 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(): - op.add_column('printeditems', - sa.Column('gagelabtag', sa.String(length=50), nullable=True)) - op.create_index('ix_printeditems_gagelabtag', 'printeditems', - ['gagelabtag'], unique=True) + 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', + 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', + ['gagelabtag'], unique=True) def downgrade(): - op.drop_index('ix_printeditems_gagelabtag', table_name='printeditems') - op.drop_column('printeditems', 'gagelabtag') + 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') + columns = {column['name'] for column in inspector.get_columns('printeditems')} + if 'gagelabtag' in columns: + op.drop_column('printeditems', 'gagelabtag') diff --git a/tests/test_plugin_migrations.py b/tests/test_plugin_migrations.py index 5c58e74c..aaafed1d 100644 --- a/tests/test_plugin_migrations.py +++ b/tests/test_plugin_migrations.py @@ -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 -# is the ONLY divergence possible between schema and stamp, so upgrade-all must -# handle it end to end. A longer chain can also meet an ALTER whose column is -# already present, which no amount of guarding a create_table fixes - see the -# test below for why that is a different problem. -SINGLE_BASELINE_PLUGINS = tuple( - plugin for plugin in POST_CUTOVER_PLUGINS - if len(list((PLUGINS_DIR / plugin / 'migrations' / 'versions').glob('0*.py'))) == 1 -) +# EVERY post-cutover chain, not just the single-baseline ones. This was once +# restricted to plugins whose whole chain is one 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. While this was narrowed, printedparts 0003 sat +# unguarded for a month and would fail upgrade-all on any create-all-built +# 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): """A baseline must stamp, not fail, when its table is already there.