diff --git a/CHANGELOG.md b/CHANGELOG.md index bbccfd1e..ebad69b4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -105,6 +105,20 @@ ADR-007 and ADR-002. the wrong code. The page index now travels with the page. - The back label's dotted preview outline outspecified the print reset and drew a dashed rule down the edge of every second card. +- **`flask plugin upgrade-all` failed a plugin whose tables already existed** + ("table securesendsecrets already exists" on the tools plugin). Every + post-cutover baseline emitted a plain `op.create_table`, which is correct only + while a table's existence and its plugin's Alembic stamp cannot disagree. + They can: anything that builds tables from the models stamps nothing, and a + run that creates a table then fails before its stamp commits leaves the same + split, where re-running is the obvious response. Creates and indexes across + all nine table-creating plugin migrations now skip what is already there. + Indexes are matched on their COLUMNS rather than their name, because + `db.create_all()` names them by SQLAlchemy's convention and a migration by + this repo's, so a name-keyed check would silently add a duplicate index + instead of skipping. Note the remaining limit: this makes a baseline + idempotent, not a whole chain. A longer chain can still meet an `ALTER` whose + column is already present, which no guard on `create_table` addresses. - **Saving a machine that has a controlling PC answered "this relationship already exists" and refused the save.** Reported against a map position change, but the map was incidental: the form re-POSTed the controlling-PC diff --git a/plugins/backups/migrations/versions/0001_backups_baseline.py b/plugins/backups/migrations/versions/0001_backups_baseline.py index 885a3ca9..5af4fb47 100644 --- a/plugins/backups/migrations/versions/0001_backups_baseline.py +++ b/plugins/backups/migrations/versions/0001_backups_baseline.py @@ -12,6 +12,11 @@ time (NoReferencedTableError). Same shape autogenerate produces. Tables inherit the connection's default charset, matching how the core chain creates its own. """ from alembic import op + +from shopdb.plugins.alembic_template import ( + create_index_if_missing, + create_table_if_missing, +) import sqlalchemy as sa @@ -23,7 +28,7 @@ depends_on = None def upgrade(): - op.create_table( + create_table_if_missing( 'backuprevisions', sa.Column('backuprevisionid', sa.Integer(), nullable=False), sa.Column('assetid', sa.Integer(), nullable=False), @@ -42,15 +47,15 @@ def upgrade(): ondelete='CASCADE'), sa.PrimaryKeyConstraint('backuprevisionid'), ) - op.create_index('ixbackuprevisionsassetid', 'backuprevisions', ['assetid']) - op.create_index('ixbackuprevisionsbackupkind', 'backuprevisions', - ['backupkind']) - op.create_index('ixbackuprevisionscontenthash', 'backuprevisions', - ['contenthash']) + create_index_if_missing('ixbackuprevisionsassetid', 'backuprevisions', ['assetid']) + create_index_if_missing('ixbackuprevisionsbackupkind', 'backuprevisions', + ['backupkind']) + create_index_if_missing('ixbackuprevisionscontenthash', 'backuprevisions', + ['contenthash']) # The dedup read path is "latest revision for this asset+kind", so the # composite index is the one that actually gets used on every collector post. - op.create_index('ixbackuprevisionsassetkind', 'backuprevisions', - ['assetid', 'backupkind']) + create_index_if_missing('ixbackuprevisionsassetkind', 'backuprevisions', + ['assetid', 'backupkind']) def downgrade(): diff --git a/plugins/geenforce/migrations/versions/0001_geenforce_baseline.py b/plugins/geenforce/migrations/versions/0001_geenforce_baseline.py index 2c9831f5..e232aacb 100644 --- a/plugins/geenforce/migrations/versions/0001_geenforce_baseline.py +++ b/plugins/geenforce/migrations/versions/0001_geenforce_baseline.py @@ -8,6 +8,11 @@ tables are created parent-first. computertypeid / measuringtooltypeid / publishedby are SOFT references (plain columns, no FK) to optional plugins/core. """ from alembic import op + +from shopdb.plugins.alembic_template import ( + create_index_if_missing, + create_table_if_missing, +) import sqlalchemy as sa @@ -19,7 +24,7 @@ depends_on = None def upgrade(): - op.create_table( + create_table_if_missing( 'manifestscopes', sa.Column('scopeid', sa.Integer(), nullable=False), sa.Column('scopename', sa.String(length=64), nullable=False), @@ -37,7 +42,7 @@ def upgrade(): sa.PrimaryKeyConstraint('scopeid'), sa.UniqueConstraint('scopename', 'phase', name='uq_scope_name_phase'), ) - op.create_table( + create_table_if_missing( 'manifestentries', sa.Column('entryid', sa.Integer(), nullable=False), sa.Column('scopeid', sa.Integer(), nullable=False), @@ -80,10 +85,10 @@ def upgrade(): sa.PrimaryKeyConstraint('entryid'), sa.UniqueConstraint('scopeid', 'name', name='uq_entry_scope_name'), ) - op.create_index('idx_entry_scope', 'manifestentries', ['scopeid']) - op.create_index('idx_entry_scope_order', 'manifestentries', - ['scopeid', 'sortorder']) - op.create_table( + create_index_if_missing('idx_entry_scope', 'manifestentries', ['scopeid']) + create_index_if_missing('idx_entry_scope_order', 'manifestentries', + ['scopeid', 'sortorder']) + create_table_if_missing( 'manifestentrypctypes', sa.Column('entrypctypeid', sa.Integer(), nullable=False), sa.Column('entryid', sa.Integer(), nullable=False), @@ -93,8 +98,8 @@ def upgrade(): ondelete='CASCADE'), sa.PrimaryKeyConstraint('entrypctypeid'), ) - op.create_index('idx_pctype_entry', 'manifestentrypctypes', ['entryid']) - op.create_table( + create_index_if_missing('idx_pctype_entry', 'manifestentrypctypes', ['entryid']) + create_table_if_missing( 'manifestentryhostnames', sa.Column('entryhostnameid', sa.Integer(), nullable=False), sa.Column('entryid', sa.Integer(), nullable=False), @@ -104,8 +109,8 @@ def upgrade(): ondelete='CASCADE'), sa.PrimaryKeyConstraint('entryhostnameid'), ) - op.create_index('idx_hostname_entry', 'manifestentryhostnames', ['entryid']) - op.create_table( + create_index_if_missing('idx_hostname_entry', 'manifestentryhostnames', ['entryid']) + create_table_if_missing( 'manifestentrymachinenumbers', sa.Column('entrymachinenumberid', sa.Integer(), nullable=False), sa.Column('entryid', sa.Integer(), nullable=False), @@ -115,9 +120,9 @@ def upgrade(): ondelete='CASCADE'), sa.PrimaryKeyConstraint('entrymachinenumberid'), ) - op.create_index('idx_machinenumber_entry', 'manifestentrymachinenumbers', - ['entryid']) - op.create_table( + create_index_if_missing('idx_machinenumber_entry', 'manifestentrymachinenumbers', + ['entryid']) + create_table_if_missing( 'manifestinusechecks', sa.Column('inusecheckid', sa.Integer(), nullable=False), sa.Column('entryid', sa.Integer(), nullable=False), @@ -127,7 +132,7 @@ def upgrade(): sa.PrimaryKeyConstraint('inusecheckid'), sa.UniqueConstraint('entryid'), ) - op.create_table( + create_table_if_missing( 'manifestinusecheckprocesses', sa.Column('inusecheckprocessid', sa.Integer(), nullable=False), sa.Column('inusecheckid', sa.Integer(), nullable=False), @@ -140,9 +145,9 @@ def upgrade(): ondelete='CASCADE'), sa.PrimaryKeyConstraint('inusecheckprocessid'), ) - op.create_index('idx_process_check', 'manifestinusecheckprocesses', - ['inusecheckid']) - op.create_table( + create_index_if_missing('idx_process_check', 'manifestinusecheckprocesses', + ['inusecheckid']) + create_table_if_missing( 'manifestpublishedversions', sa.Column('publishedversionid', sa.Integer(), nullable=False), sa.Column('scopeid', sa.Integer(), nullable=False), @@ -158,9 +163,9 @@ def upgrade(): sa.UniqueConstraint('scopeid', 'versionnumber', name='uq_published_scope_version'), ) - op.create_index('idx_published_scope', 'manifestpublishedversions', - ['scopeid']) - op.create_table( + create_index_if_missing('idx_published_scope', 'manifestpublishedversions', + ['scopeid']) + create_table_if_missing( 'manifestpayloads', sa.Column('payloadid', sa.Integer(), nullable=False), sa.Column('entryid', sa.Integer(), nullable=False), @@ -173,8 +178,8 @@ def upgrade(): ondelete='CASCADE'), sa.PrimaryKeyConstraint('payloadid'), ) - op.create_index('idx_payload_entry', 'manifestpayloads', ['entryid']) - op.create_table( + create_index_if_missing('idx_payload_entry', 'manifestpayloads', ['entryid']) + create_table_if_missing( 'manifestenforcementreports', sa.Column('reportid', sa.Integer(), nullable=False), sa.Column('hostname', sa.String(length=100), nullable=False), @@ -192,13 +197,13 @@ def upgrade(): sa.Column('iscurrent', sa.Boolean(), nullable=False), sa.PrimaryKeyConstraint('reportid'), ) - op.create_index('idx_report_host', 'manifestenforcementreports', - ['hostname']) - op.create_index('idx_report_current', 'manifestenforcementreports', - ['iscurrent']) - op.create_index('idx_report_host_scope', 'manifestenforcementreports', - ['hostname', 'scopename', 'phase']) - op.create_table( + create_index_if_missing('idx_report_host', 'manifestenforcementreports', + ['hostname']) + create_index_if_missing('idx_report_current', 'manifestenforcementreports', + ['iscurrent']) + create_index_if_missing('idx_report_host_scope', 'manifestenforcementreports', + ['hostname', 'scopename', 'phase']) + create_table_if_missing( 'manifestenforcementresults', sa.Column('resultid', sa.Integer(), nullable=False), sa.Column('reportid', sa.Integer(), nullable=False), @@ -212,9 +217,9 @@ def upgrade(): ondelete='CASCADE'), sa.PrimaryKeyConstraint('resultid'), ) - op.create_index('idx_result_report', 'manifestenforcementresults', - ['reportid']) - op.create_table( + create_index_if_missing('idx_result_report', 'manifestenforcementresults', + ['reportid']) + create_table_if_missing( 'pctypealiases', sa.Column('aliasid', sa.Integer(), nullable=False), sa.Column('aliasgroup', sa.Integer(), nullable=False), @@ -223,7 +228,7 @@ def upgrade(): sa.UniqueConstraint('aliasgroup', 'aliasname', name='uq_alias_group_name'), ) - op.create_index('idx_alias_group', 'pctypealiases', ['aliasgroup']) + create_index_if_missing('idx_alias_group', 'pctypealiases', ['aliasgroup']) def downgrade(): diff --git a/plugins/geenforce/migrations/versions/0002_manifest_blobs.py b/plugins/geenforce/migrations/versions/0002_manifest_blobs.py index d196a2ce..31081846 100644 --- a/plugins/geenforce/migrations/versions/0002_manifest_blobs.py +++ b/plugins/geenforce/migrations/versions/0002_manifest_blobs.py @@ -10,6 +10,11 @@ Create Date: 2026-07-21 """ from alembic import op + +from shopdb.plugins.alembic_template import ( + create_index_if_missing, + create_table_if_missing, +) import sqlalchemy as sa @@ -24,7 +29,7 @@ def upgrade(): insp = sa.inspect(bind) if 'manifestblobs' in insp.get_table_names(): return - op.create_table( + create_table_if_missing( 'manifestblobs', sa.Column('sha256', sa.String(length=64), nullable=False), sa.Column('filename', sa.String(length=255), nullable=False), diff --git a/plugins/measuringtools/migrations/versions/0001_measuringtools_baseline.py b/plugins/measuringtools/migrations/versions/0001_measuringtools_baseline.py index 8b102464..15722d64 100644 --- a/plugins/measuringtools/migrations/versions/0001_measuringtools_baseline.py +++ b/plugins/measuringtools/migrations/versions/0001_measuringtools_baseline.py @@ -17,6 +17,11 @@ connection's default charset, so on a utf8mb4 database they are utf8mb4, matchin how the core chain creates its tables. """ from alembic import op + +from shopdb.plugins.alembic_template import ( + create_index_if_missing, + create_table_if_missing, +) import sqlalchemy as sa @@ -28,7 +33,7 @@ depends_on = None def upgrade(): - op.create_table( + create_table_if_missing( 'measuringtooltypes', sa.Column('measuringtooltypeid', sa.Integer(), nullable=False), sa.Column('name', sa.String(length=100), nullable=False), @@ -40,7 +45,7 @@ def upgrade(): sa.PrimaryKeyConstraint('measuringtooltypeid'), sa.UniqueConstraint('name'), ) - op.create_table( + create_table_if_missing( 'measuringtools', sa.Column('measuringtoolid', sa.Integer(), nullable=False), sa.Column('assetid', sa.Integer(), nullable=False), @@ -59,8 +64,8 @@ def upgrade(): sa.PrimaryKeyConstraint('measuringtoolid'), sa.UniqueConstraint('assetid'), ) - op.create_index('idx_measuringtool_type', 'measuringtools', - ['measuringtooltypeid']) + create_index_if_missing('idx_measuringtool_type', 'measuringtools', + ['measuringtooltypeid']) def downgrade(): diff --git a/plugins/printedparts/migrations/versions/0001_printedparts_baseline.py b/plugins/printedparts/migrations/versions/0001_printedparts_baseline.py index f0bb3484..3d16b508 100644 --- a/plugins/printedparts/migrations/versions/0001_printedparts_baseline.py +++ b/plugins/printedparts/migrations/versions/0001_printedparts_baseline.py @@ -11,6 +11,11 @@ ops are written out explicitly anyway to match the measuringtools exemplar and keep the baseline reviewable. """ from alembic import op + +from shopdb.plugins.alembic_template import ( + create_index_if_missing, + create_table_if_missing, +) import sqlalchemy as sa @@ -22,7 +27,7 @@ depends_on = None def upgrade(): - op.create_table( + create_table_if_missing( 'printeditems', sa.Column('printeditemid', sa.Integer(), nullable=False), sa.Column('itemcode', sa.String(length=20), nullable=True), @@ -39,9 +44,9 @@ def upgrade(): sa.PrimaryKeyConstraint('printeditemid'), sa.UniqueConstraint('itemcode'), ) - op.create_index('ix_printeditems_itemcode', 'printeditems', ['itemcode']) + create_index_if_missing('ix_printeditems_itemcode', 'printeditems', ['itemcode']) - op.create_table( + create_table_if_missing( 'printeditemtransactions', sa.Column('transactionid', sa.Integer(), nullable=False), sa.Column('printeditemid', sa.Integer(), nullable=False), @@ -58,12 +63,12 @@ def upgrade(): ondelete='CASCADE'), sa.PrimaryKeyConstraint('transactionid'), ) - op.create_index('ix_printeditemtransactions_printeditemid', - 'printeditemtransactions', ['printeditemid']) - op.create_index('ix_printeditemtransactions_employeesso', - 'printeditemtransactions', ['employeesso']) - op.create_index('ix_printeditemtransactions_transactiondate', - 'printeditemtransactions', ['transactiondate']) + create_index_if_missing('ix_printeditemtransactions_printeditemid', + 'printeditemtransactions', ['printeditemid']) + create_index_if_missing('ix_printeditemtransactions_employeesso', + 'printeditemtransactions', ['employeesso']) + create_index_if_missing('ix_printeditemtransactions_transactiondate', + 'printeditemtransactions', ['transactiondate']) def downgrade(): diff --git a/plugins/printedparts/migrations/versions/0002_printeditemfiles.py b/plugins/printedparts/migrations/versions/0002_printeditemfiles.py index d0b2c9a9..0cfd8553 100644 --- a/plugins/printedparts/migrations/versions/0002_printeditemfiles.py +++ b/plugins/printedparts/migrations/versions/0002_printeditemfiles.py @@ -5,6 +5,11 @@ the ADR-008 payoff: the plugin evolves its own schema without touching the core chain. Applied by `flask plugin upgrade-all`. """ from alembic import op + +from shopdb.plugins.alembic_template import ( + create_index_if_missing, + create_table_if_missing, +) import sqlalchemy as sa @@ -16,7 +21,7 @@ depends_on = None def upgrade(): - op.create_table( + create_table_if_missing( 'printeditemfiles', sa.Column('fileid', sa.Integer(), nullable=False), sa.Column('printeditemid', sa.Integer(), nullable=False), @@ -35,8 +40,8 @@ def upgrade(): sa.PrimaryKeyConstraint('fileid'), sa.UniqueConstraint('storedfilename'), ) - op.create_index('ix_printeditemfiles_printeditemid', - 'printeditemfiles', ['printeditemid']) + create_index_if_missing('ix_printeditemfiles_printeditemid', + 'printeditemfiles', ['printeditemid']) def downgrade(): diff --git a/plugins/printers/migrations/versions/0002_printersupplyalerts.py b/plugins/printers/migrations/versions/0002_printersupplyalerts.py index e563e2ca..fe909c18 100644 --- a/plugins/printers/migrations/versions/0002_printersupplyalerts.py +++ b/plugins/printers/migrations/versions/0002_printersupplyalerts.py @@ -4,6 +4,11 @@ Stores the last tier alerted per (printer, supply) so the poller fires once on a downward crossing and re-arms after a refill. Idempotent create. """ from alembic import op + +from shopdb.plugins.alembic_template import ( + create_index_if_missing, + create_table_if_missing, +) import sqlalchemy as sa @@ -19,7 +24,7 @@ def upgrade(): inspector = sa.inspect(bind) if 'printersupplyalerts' in inspector.get_table_names(): return - op.create_table( + create_table_if_missing( 'printersupplyalerts', sa.Column('printersupplyalertid', sa.Integer(), primary_key=True), sa.Column('printerid', sa.Integer(), nullable=False), @@ -35,8 +40,8 @@ def upgrade(): sa.UniqueConstraint('printerid', 'supplykey', name='uq_printersupplyalert_printer_key'), ) - op.create_index('ix_printersupplyalerts_printerid', - 'printersupplyalerts', ['printerid']) + create_index_if_missing('ix_printersupplyalerts_printerid', + 'printersupplyalerts', ['printerid']) def downgrade(): diff --git a/plugins/printers/migrations/versions/0005_observedqueues.py b/plugins/printers/migrations/versions/0005_observedqueues.py index 8eeea6cd..0a92c9a9 100644 --- a/plugins/printers/migrations/versions/0005_observedqueues.py +++ b/plugins/printers/migrations/versions/0005_observedqueues.py @@ -21,6 +21,11 @@ Revision ID: printers0005observedqueues Revises: printers0004drivervendor """ from alembic import op + +from shopdb.plugins.alembic_template import ( + create_index_if_missing, + create_table_if_missing, +) import sqlalchemy as sa @@ -36,7 +41,7 @@ def upgrade(): inspector = sa.inspect(bind) if 'printerobservedqueues' in inspector.get_table_names(): return - op.create_table( + create_table_if_missing( 'printerobservedqueues', sa.Column('printerobservedqueueid', sa.Integer(), nullable=False), # Nullable and no index of its own beyond the explicit one below: an @@ -67,10 +72,10 @@ def upgrade(): ) # Port address is the primary match key from an observed queue back to a # printer asset, so every comparison read hits it. - op.create_index('idx_printerobservedqueues_portaddress', - 'printerobservedqueues', ['portaddress']) - op.create_index('idx_printerobservedqueues_assetid', - 'printerobservedqueues', ['assetid']) + create_index_if_missing('idx_printerobservedqueues_portaddress', + 'printerobservedqueues', ['portaddress']) + create_index_if_missing('idx_printerobservedqueues_assetid', + 'printerobservedqueues', ['assetid']) def downgrade(): diff --git a/plugins/tools/migrations/versions/0001_securesend_baseline.py b/plugins/tools/migrations/versions/0001_securesend_baseline.py index ebdc7dae..2ead58df 100644 --- a/plugins/tools/migrations/versions/0001_securesend_baseline.py +++ b/plugins/tools/migrations/versions/0001_securesend_baseline.py @@ -12,6 +12,11 @@ plugin's tables, so the foreign key to the core users table cannot resolve at CreateTable-compile time. """ from alembic import op + +from shopdb.plugins.alembic_template import ( + create_index_if_missing, + create_table_if_missing, +) import sqlalchemy as sa @@ -23,7 +28,7 @@ depends_on = None def upgrade(): - op.create_table( + create_table_if_missing( 'securesendsecrets', sa.Column('secretid', sa.Integer(), nullable=False), # 128 bits of randomness as hex. With no login on the read side this is @@ -47,13 +52,13 @@ def upgrade(): ) # Unique, because the token is looked up on every read and two rows sharing # one would make a link ambiguous. - op.create_index('ixsecuresendsecretstoken', 'securesendsecrets', - ['token'], unique=True) + create_index_if_missing('ixsecuresendsecretstoken', 'securesendsecrets', + ['token'], unique=True) # Expiry sweeps scan on this. - op.create_index('ixsecuresendsecretsexpiresat', 'securesendsecrets', - ['expiresat']) - op.create_index('ixsecuresendsecretscreatedby', 'securesendsecrets', - ['createdbyuserid']) + create_index_if_missing('ixsecuresendsecretsexpiresat', 'securesendsecrets', + ['expiresat']) + create_index_if_missing('ixsecuresendsecretscreatedby', 'securesendsecrets', + ['createdbyuserid']) def downgrade(): diff --git a/shopdb/plugins/alembic_template.py b/shopdb/plugins/alembic_template.py index fa91fa24..70e272fe 100644 --- a/shopdb/plugins/alembic_template.py +++ b/shopdb/plugins/alembic_template.py @@ -104,7 +104,12 @@ def create_plugin_tables(plugin_name: str): this is safe on an existing database that has the table from the pre-cutover core baseline as well as on a fresh install (ADR-014 Phase 2). - Called from each plugin's 0001 baseline.py upgrade(). + NOT actually called from anywhere: every post-cutover baseline emits + explicit Alembic ops instead, because this helper's per-plugin MetaData + cannot resolve a foreign key to a CORE table at CreateTable-compile time. + Kept because it is still the reference for what idempotent means here, and + because create_table_if_missing below is the same rule in a form the + baselines can use. See that function for what went wrong without it. """ from alembic import op from sqlalchemy import inspect @@ -120,6 +125,67 @@ def create_plugin_tables(plugin_name: str): op.execute(str(CreateTable(table).compile(dialect=bind.dialect))) +def create_table_if_missing(table_name: str, *columns, **kwargs) -> bool: + """op.create_table, skipped when the table is already there. + + WHY EVERY POST-CUTOVER BASELINE NEEDS THIS. A baseline is the sole creator + of its plugin's tables, so it was written as a plain op.create_table - which + is correct only if the table's existence and the plugin's Alembic stamp can + never disagree. They can, and routinely do: + + * `flask db-utils create-all` runs db.create_all(), which builds EVERY + table registered on the metadata - plugin tables included - and stamps + NOTHING. `flask plugin upgrade-all` then runs the baseline against + tables that already exist. + * A run that creates the table and then fails before its stamp commits + leaves the same split, and re-running is the obvious thing to try. + + Reported as `flask plugin upgrade-all` failing the tools plugin with + "table securesendsecrets already exists". Nothing caught it because the + migration test drops the post-cutover tables db.create_all() made before + running upgrade-all, which arranges the one state where an unguarded create + works and never the state a real database is in. + + Returns True when it created the table, so a caller can skip the indexes + that belong to it. + """ + from alembic import op + from sqlalchemy import inspect + + if inspect(op.get_bind()).has_table(table_name): + logger.info('%s already exists, skipping create', table_name) + return False + op.create_table(table_name, *columns, **kwargs) + return True + + +def create_index_if_missing(index_name: str, table_name: str, columns, **kwargs): + """op.create_index, skipped when an equivalent index already exists. + + Matched on the COLUMN LIST rather than the name, because the two creators + do not agree on names. db.create_all() takes them from SQLAlchemy's default + convention (`ix_securesendsecrets_token`) while a baseline spells them the + way this repo's naming rules require (`ixsecuresendsecretstoken`). Matching + on the name would call the index missing and add a second one over the same + column - MySQL permits that, with only a warning, so the duplicate would + accumulate silently on every affected site. + """ + from alembic import op + from sqlalchemy import inspect + + wanted = [str(column) for column in columns] + inspector = inspect(op.get_bind()) + if not inspector.has_table(table_name): + op.create_index(index_name, table_name, columns, **kwargs) + return + for existing in inspector.get_indexes(table_name): + if list(existing.get('column_names') or []) == wanted: + logger.info('index over %s(%s) already exists as %s, skipping', + table_name, ', '.join(wanted), existing.get('name')) + return + op.create_index(index_name, table_name, columns, **kwargs) + + def drop_plugin_tables(plugin_name: str): """Mirror of create_plugin_tables for downgrade(). Drops in reverse FK order.""" diff --git a/tests/test_plugin_migrations.py b/tests/test_plugin_migrations.py index 54654416..e4ef3976 100644 --- a/tests/test_plugin_migrations.py +++ b/tests/test_plugin_migrations.py @@ -276,3 +276,89 @@ def test_upgrade_all_on_fresh_db_is_clean_and_idempotent(tmp_path, monkeypatch): (plugin_manager._app, plugin_manager._db, plugin_manager.registry, plugin_manager.loader, plugin_manager.migration_manager, plugin_manager._registered_prefixes) = saved + + + +# 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 +) + + +@pytest.mark.parametrize('plugin', SINGLE_BASELINE_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. + + Reported: `flask plugin upgrade-all` failed the tools plugin with "table + securesendsecrets already exists". Every post-cutover baseline was a plain + op.create_table, correct only while a table's existence and its plugin's + Alembic stamp can never disagree. They can - anything that builds tables + from the models (db.create_all) stamps nothing, and a run that creates a + table then fails before its stamp commits leaves the same split. + + The other upgrade-all test DROPS these tables before running, so it only + ever covered the state where they are absent. This is the other state. + """ + from sqlalchemy import inspect, text + from shopdb.config import TestingConfig + from shopdb import create_app + from shopdb.extensions import db + from shopdb.plugins import plugin_manager + + monkeypatch.setattr(TestingConfig, 'SQLALCHEMY_DATABASE_URI', + f'sqlite:///{tmp_path / "preexisting.db"}') + + saved = (plugin_manager._app, plugin_manager._db, plugin_manager.registry, + plugin_manager.loader, plugin_manager.migration_manager, + plugin_manager._registered_prefixes) + try: + app = create_app('testing') + with app.app_context(): + db.create_all() + + # Leave THIS plugin's tables in place - that is the whole point - + # and drop the other post-cutover ones so their own (longer) chains + # run the way the sibling test already covers. + insp0 = inspect(db.engine) + for other in POST_CUTOVER_PLUGINS: + if other == plugin: + continue + for tablename in PLUGIN_TABLE_OWNERS[other]: + if insp0.has_table(tablename): + db.session.execute(text(f'DROP TABLE {tablename}')) + db.session.commit() + + for tablename in PLUGIN_TABLE_OWNERS[plugin]: + assert inspect(db.engine).has_table(tablename), ( + f'{tablename} should have been built by create_all; ' + f'this test is meaningless without it') + + first = app.extensions['plugin_manager'].upgrade_all_plugins() + assert first[plugin] == 'ok', first[plugin] + + second = app.extensions['plugin_manager'].upgrade_all_plugins() + assert second[plugin] == 'ok', second[plugin] + + insp = inspect(db.engine) + row = db.session.execute( + text(f'SELECT version_num FROM alembic_version_{plugin}') + ).fetchone() + assert row and row[0] == EXPECTED_HEAD_REVISION[plugin] + + # No second index over the same columns: create_all names indexes by + # SQLAlchemy's convention and the baseline by this repo's, so a + # name-keyed guard would have added a duplicate rather than skipped. + for tablename in PLUGIN_TABLE_OWNERS[plugin]: + seen = [tuple(ix.get('column_names') or ()) + for ix in insp.get_indexes(tablename)] + assert len(seen) == len(set(seen)), ( + f'{tablename} has two indexes over the same columns: {seen}') + finally: + (plugin_manager._app, plugin_manager._db, plugin_manager.registry, + plugin_manager.loader, plugin_manager.migration_manager, + plugin_manager._registered_prefixes) = saved