From b96637e1a17e3b1184299f5066dde9ec2798a7c6 Mon Sep 17 00:00:00 2001 From: cproudlock Date: Mon, 24 Aug 2026 09:28:37 -0400 Subject: [PATCH] Let a baseline meet a table that is already there Reported: `flask plugin upgrade-all` failed the tools plugin with "table securesendsecrets already exists". Every post-cutover baseline emitted a plain op.create_table. That is correct only while a table's existence and its plugin's Alembic stamp can never disagree, and they can. Anything that builds tables from the models - db.create_all(), which `flask db-utils create-all` calls - creates them and stamps nothing. A run that creates a table and then fails before its stamp commits leaves the same split, and re-running is the obvious thing to try. In both cases upgrade-all meets existing tables with an unstamped chain and the create is fatal. Nothing caught it because the migration test DROPS the post-cutover tables db.create_all() built before running upgrade-all. That arranges the one state where an unguarded create works, and never the state the report came from. The new test is the same sequence with the drop removed for the plugin under test. create_table_if_missing and create_index_if_missing are in alembic_template.py next to create_plugin_tables, which had the idempotency rule right and turns out to be called by nobody - its docstring claimed every 0001 used it. Fixed the docstring rather than rewiring the baselines onto it, because the reason they hand-write their DDL still stands: the helper's per-plugin MetaData cannot resolve a foreign key to a core table at CreateTable-compile time. INDEXES ARE MATCHED ON COLUMNS, NOT NAME, and that distinction matters. The two creators disagree about names - db.create_all() takes SQLAlchemy's default convention (ix_securesendsecrets_token) and a baseline spells it the way this repo's naming rules require (ixsecuresendsecretstoken). A name-keyed guard would have called the index missing and added a second one over the same column, which MySQL permits with a warning, so every affected site would have quietly grown duplicates. The test asserts no table ends up with two indexes over the same columns. Applied to all nine table-creating plugin migrations, not just the tools baseline that was reported: the four later migrations that create tables have the same exposure, and geenforce, measuringtools, printedparts and backups are the same defect waiting for a different site. WHAT THIS DOES NOT FIX. It makes a baseline idempotent, not a whole chain. A longer chain can still meet an ALTER whose column is already present - printedparts adding gagelabtag to a table create_all built at head is the case that showed up while testing. Guarding every op one at a time is the wrong answer to that; a database built from the models is already AT head and its chains want stamping rather than replaying, and there is no `flask plugin stamp` today. Left alone deliberately rather than half-built, and the new test is scoped to single-baseline plugins so it does not pretend otherwise. --- CHANGELOG.md | 14 +++ .../versions/0001_backups_baseline.py | 21 +++-- .../versions/0001_geenforce_baseline.py | 71 ++++++++------- .../versions/0002_manifest_blobs.py | 7 +- .../versions/0001_measuringtools_baseline.py | 13 ++- .../versions/0001_printedparts_baseline.py | 23 +++-- .../versions/0002_printeditemfiles.py | 11 ++- .../versions/0002_printersupplyalerts.py | 11 ++- .../versions/0005_observedqueues.py | 15 ++-- .../versions/0001_securesend_baseline.py | 19 ++-- shopdb/plugins/alembic_template.py | 68 ++++++++++++++- tests/test_plugin_migrations.py | 86 +++++++++++++++++++ 12 files changed, 285 insertions(+), 74 deletions(-) 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