From b979062dd7fdb7678d3a439640b400cda13917a5 Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Mon, 31 Aug 2026 00:29:57 -0400 Subject: [PATCH] db: rename the four double-prefixed CHECK constraints (#3275) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Run 5026 got the models-vs-chain diff to 7 lines. Three findings, and one of them reverses an assumption I made in the previous commit. The doubled CHECK names are what the DATABASE has, not what the generator invented. base.py's convention is ck_%(table_name)s_%(constraint_name)s, which — unlike uq/fk/ix — applies even to a constraint that already has a name, so four migrations that passed an already-prefixed name got it prefixed twice: ck_import_settings_ck_import_settings_singleton ck_ml_settings_ck_ml_settings_singleton ck_post_ck_post_translation_override ck_tag_ck_tag_fandom_requires_character The workflow repair added last commit is still correct and still needed — autogenerate really does re-double a name on the round trip — but it was making the MODELS side clean against a chain that is dirty. The comment in ml_settings.py claiming its bare name "matches migration 0003" was simply false; 0003 produces the doubled form. Nothing reads a CHECK constraint by name, so this has never done harm. But it is precisely the development-era residue the collapsed baseline exists to leave behind, and a public schema should not ship it — so 0088 renames the deployed constraints and all six models now declare bare names. RENAME CONSTRAINT is catalog-only: no scan, no rewrite, no revalidation, which is why this is safe on post and tag. Guarded on pg_constraint scoped by conrelid, so it is a no-op on a database built from the models. ix_tag_fandom_id showed as a difference only because chain_ref was pinned to 0a5bbe8, which predates 0088 — the comparison was measuring the models against a chain missing the migration that closes the gap. chain_ref now defaults to blank, meaning "the chain in this ref". Pin it to a commit only after the collapse, when the tree no longer carries the revisions. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_017QHszn9H8VBvx5Ke8x1hvw --- .forgejo/workflows/baseline.yml | 11 +++- .../0088_reconcile_models_with_schema.py | 65 ++++++++++++++++++- backend/app/models/external_link.py | 8 ++- backend/app/models/import_settings.py | 5 ++ backend/app/models/ml_settings.py | 5 +- backend/app/models/post.py | 6 +- backend/app/models/tag.py | 6 +- 7 files changed, 96 insertions(+), 10 deletions(-) diff --git a/.forgejo/workflows/baseline.yml b/.forgejo/workflows/baseline.yml index 4681a35..7b49421 100644 --- a/.forgejo/workflows/baseline.yml +++ b/.forgejo/workflows/baseline.yml @@ -34,9 +34,9 @@ on: workflow_dispatch: inputs: chain_ref: - description: 'Commit/tag that still carries the full 0001..0087 chain' + description: 'Commit/tag carrying the full chain; blank = this ref (use a pinned commit only AFTER the collapse)' type: string - default: '0a5bbe8' + default: '' mode: description: 'chain = compare against this tree''s migrations; models = compare against a schema built from the MODELS' type: string @@ -100,10 +100,15 @@ jobs: - name: Build the schema the OLD chain produces env: CHAIN_REF: ${{ github.event.inputs.chain_ref }} + THIS_SHA: ${{ github.sha }} run: | set -eux docker exec "$PG_CONTAINER" createdb -U fabledcurator fc_chain - git worktree add /tmp/chain "$CHAIN_REF" + # Blank means "the chain in this ref", which is what you want while + # the chain is still intact — comparing the models against a PINNED + # older commit reports every migration written since as a difference. + # Pin it only after the collapse, when the tree no longer has them. + git worktree add /tmp/chain "${CHAIN_REF:-$THIS_SHA}" ls /tmp/chain/alembic/versions/*.py | wc -l cd /tmp/chain DB_NAME=fc_chain alembic upgrade head diff --git a/alembic/versions/0088_reconcile_models_with_schema.py b/alembic/versions/0088_reconcile_models_with_schema.py index a286c6c..30020dc 100644 --- a/alembic/versions/0088_reconcile_models_with_schema.py +++ b/alembic/versions/0088_reconcile_models_with_schema.py @@ -6,8 +6,8 @@ schema disagreed. Almost all of them were the MODEL being wrong — missing migration — and those are fixed in the model files with no DDL at all, because the database already had them. -This migration carries the remainder: the one case where the MODEL was right -and the database was missing something. +This migration carries the remainder — the two places where DDL is actually +needed, because the database is what is wrong. `tag.fandom_id` is declared `index=True` on the model, but no migration ever created that index. Every autogenerate run since would have proposed adding @@ -29,6 +29,29 @@ guarantee built from different objects, which is why the two schemas did not line up. The model now declares the constraint and the plain index separately, so it describes what is actually there. No DDL is needed for it. +Also here: four CHECK constraints whose names carry their table prefix TWICE. + +`base.py`'s naming convention is `ck_%(table_name)s_%(constraint_name)s`, and +unlike the uq/fk/ix entries it applies even to a constraint that already has a +name. Four migrations passed an already-prefixed name, so the convention +prefixed it again: + + ck_import_settings_ck_import_settings_singleton + ck_ml_settings_ck_ml_settings_singleton + ck_post_ck_post_translation_override + ck_tag_ck_tag_fandom_requires_character + +Nothing reads a CHECK constraint by name, so this has never done any harm — +but it is exactly the development-era residue the collapsed baseline exists to +leave behind, and a public schema should not ship it. The models now declare +bare names, which the convention renders into the single-prefix form; this +renames the deployed constraints to match. + +RENAME CONSTRAINT is a catalog-only operation: no table scan, no rewrite, no +validation of existing rows. It takes a brief ACCESS EXCLUSIVE lock and +returns. That is why this is safe to do on `post` and `tag`, which are the two +large tables in the schema. + Revision ID: 0088 Revises: 0087 Create Date: 2026-08-30 @@ -43,6 +66,38 @@ down_revision: Union[str, None] = "0087" branch_labels: Union[str, Sequence[str], None] = None depends_on: Union[str, Sequence[str], None] = None +# (table, doubled name, single-prefix name) +DOUBLED_CHECKS = ( + ("import_settings", "ck_import_settings_ck_import_settings_singleton", + "ck_import_settings_singleton"), + ("ml_settings", "ck_ml_settings_ck_ml_settings_singleton", + "ck_ml_settings_singleton"), + ("post", "ck_post_ck_post_translation_override", + "ck_post_translation_override"), + ("tag", "ck_tag_ck_tag_fandom_requires_character", + "ck_tag_fandom_requires_character"), +) + + +def _rename_check(table: str, old: str, new: str) -> None: + # Guarded on pg_constraint rather than run bare: a database built from the + # models (a fresh install, or the CI integration schema) already has the + # single-prefix name, and this migration must be a no-op there rather than + # an error. Same reasoning as the CREATE INDEX IF NOT EXISTS below. + op.execute( + f""" + DO $$ + BEGIN + IF EXISTS ( + SELECT 1 FROM pg_constraint + WHERE conname = '{old}' AND conrelid = '{table}'::regclass + ) THEN + ALTER TABLE {table} RENAME CONSTRAINT {old} TO {new}; + END IF; + END $$; + """ + ) + def upgrade() -> None: # IF NOT EXISTS because the index is what the model already asks for: any @@ -50,6 +105,12 @@ def upgrade() -> None: # and this migration must be a no-op there rather than an error. op.execute("CREATE INDEX IF NOT EXISTS ix_tag_fandom_id ON tag (fandom_id)") + for table, old, new in DOUBLED_CHECKS: + _rename_check(table, old, new) + def downgrade() -> None: + for table, old, new in DOUBLED_CHECKS: + _rename_check(table, new, old) + op.execute("DROP INDEX IF EXISTS ix_tag_fandom_id") diff --git a/backend/app/models/external_link.py b/backend/app/models/external_link.py index dcf8ee8..b06bf9b 100644 --- a/backend/app/models/external_link.py +++ b/backend/app/models/external_link.py @@ -43,11 +43,15 @@ class ExternalLink(Base): # needs its constraint swapped in the same migration (#3275). CheckConstraint( "host IN ('mega', 'gdrive', 'mediafire', 'dropbox', 'pixeldrain')", - name="ck_external_link_host", + # Bare name: Base.metadata's naming convention prepends + # ck__. Pre-prefixing it here doubles the prefix — see + # alembic 0088, which renames the four constraints that shipped + # that way (#3275). + name="host", ), CheckConstraint( "status IN ('pending', 'downloading', 'downloaded', 'failed', 'skipped', 'dead')", - name="ck_external_link_status", + name="status", ), # One row per (post, url). The full url (incl. #fragment) is the identity # — the same file linked twice in a post collapses to one row. diff --git a/backend/app/models/import_settings.py b/backend/app/models/import_settings.py index 78c0fab..83ae9c2 100644 --- a/backend/app/models/import_settings.py +++ b/backend/app/models/import_settings.py @@ -22,6 +22,11 @@ class ImportSettings(Base): __tablename__ = "import_settings" # Bare constraint name — Base.metadata's naming convention applies the # ck_
_ prefix, producing the final ck_import_settings_singleton. + # Bare name — Base.metadata's naming convention prepends ck_
_, + # producing ck_import_settings_singleton. The chain shipped the DOUBLED + # ck_import_settings_ck_import_settings_singleton, because the migration + # pre-prefixed the name and the convention prefixed it again; alembic + # 0088 renames it to what this line has always produced (#3275). __table_args__ = (CheckConstraint("id = 1", name="singleton"),) id: Mapped[int] = mapped_column(Integer, primary_key=True) diff --git a/backend/app/models/ml_settings.py b/backend/app/models/ml_settings.py index d70449c..4705d1a 100644 --- a/backend/app/models/ml_settings.py +++ b/backend/app/models/ml_settings.py @@ -21,7 +21,10 @@ from .base import Base class MLSettings(Base): __tablename__ = "ml_settings" # Bare name — Base.metadata's naming convention prepends ck_
_, - # producing the final ck_ml_settings_singleton (matches migration 0003). + # producing ck_ml_settings_singleton. The chain shipped the DOUBLED + # ck_ml_settings_ck_ml_settings_singleton, because the migration + # pre-prefixed the name and the convention prefixed it again; alembic + # 0088 renames it to what this line has always produced (#3275). __table_args__ = (CheckConstraint("id = 1", name="singleton"),) id: Mapped[int] = mapped_column(Integer, primary_key=True) diff --git a/backend/app/models/post.py b/backend/app/models/post.py index 1cc15a4..6b4ec33 100644 --- a/backend/app/models/post.py +++ b/backend/app/models/post.py @@ -41,7 +41,11 @@ class Post(Base): UniqueConstraint("source_id", "external_post_id", name="uq_post_source_external_id"), CheckConstraint( "translation_override IN ('auto', 'force', 'original')", - name="ck_post_translation_override", + # Bare name: Base.metadata's naming convention prepends + # ck_
_. Pre-prefixing it here doubles the prefix — see + # alembic 0088, which renames the four constraints that shipped + # that way (#3275). + name="translation_override", ), ) diff --git a/backend/app/models/tag.py b/backend/app/models/tag.py index d3107d0..cc23686 100644 --- a/backend/app/models/tag.py +++ b/backend/app/models/tag.py @@ -83,7 +83,11 @@ class Tag(Base): unique=True), CheckConstraint( "(fandom_id IS NULL) OR (kind = 'character')", - name="ck_tag_fandom_requires_character", + # Bare name: Base.metadata's naming convention prepends + # ck_
_. Pre-prefixing it here doubles the prefix — see + # alembic 0088, which renames the four constraints that shipped + # that way (#3275). + name="fandom_requires_character", ), )