fix(postgres): stop a naming convention from qualifying the check constraints twice - #37
Conversation
…straints twice get_event_constraints declared every check constraint with a finished ck_<table>_<rule> name. A MetaData whose ck naming convention interpolates %(constraint_name)s — sqlalchemy-foundation-kit's "%(table_name)s_%(constraint_name)s_check", or the convention SQLAlchemy's documentation recommends — qualifies that name a second time, six of the twelve run past PostgreSQL's 63 bytes, and SQLAlchemy truncates them with a hash: outbox_events_ck_outbox_events_completed_status_consist_a784. The name in the catalog then no longer matches the convention that produced it, which is what alembic-gauntlet's test_naming_conventions checks. The helper now takes the MetaData and does what SQLAlchemy is about to do with the name: when the ck rule interpolates the constraint name it hands over the bare rule (attempts_valid, completed_status_consistency, lock_consistency) and the convention builds the name; otherwise it declares ck_<table>_<rule> as before. The four bases build __table_args__ in a declared_attr that passes cls.metadata, which also gives each concrete model its own Index objects — the shared tuple could only ever be bound to one DeclarativeBase per process. Closes #36
|
Reporter here — verified against the shape that produced the report, and it holds. Our models inherit from two parents, the library base and our own declarative base, and the class InboxEventDB(BaseTable, InboxEventPartitionedDBBase): ...That was the thing I could not tell from the diff: whether Six of six inside 63 bytes, all ending in The One coordination note for whoever picks the release: |
|
Good to know it holds through a second parent — that was the case I could only reason about, not run. The changelog here is generated from the commit subject, so the release notes cannot carry the line; the migration guide is where an upgrade is described, and 46c8193 puts it right under the |
Closes #36.
Problem
get_event_constraintsdeclared every check constraint with a finishedck_<table>_<rule>name. AMetaDatawhosecknaming convention interpolates%(constraint_name)s— sqlalchemy-foundation-kit's"%(table_name)s_%(constraint_name)s_check", or the"ck_%(table_name)s_%(constraint_name)s"SQLAlchemy's documentation recommends — qualifies that name a second time. Six of the twelve then run past PostgreSQL's 63 bytes and SQLAlchemy truncates them with a hash; the shortest offender is on the plain outbox, not the partitioned one:A name like that matches neither the prefix nor the suffix alembic-gauntlet derives from the convention, so
test_naming_conventionsfails on a table the consumer did not write.Design
The helper takes the
MetaDataand does what SQLAlchemy is about to do with the name — the branch insqlalchemy/sql/naming.pyis "rewrite an explicit name only when the rule interpolatesconstraint_name". When it does,get_event_constraintshands over the bare rule (attempts_valid,completed_status_consistency,lock_consistency) and the convention builds the name; when it does not, the name isck_<table>_<rule>exactly as before.ck_outbox_events_p_completed_status_consistency(unchanged)outbox_events_partitioned_completed_status_consistency_checkck_outbox_events_partitioned_completed_status_consistencyUnder SQLAlchemy's convention the non-partitioned tables come out byte for byte as today.
The four bases build
__table_args__in adeclared_attr.directivethat passescls.metadata, andget_event_constraintsgains*, metadata=Nonefor anyone composing__table_args__by hand. Thedeclared_attrfixes a second thing the reproduction ran into first: the tuple was built once at import, itsIndexobjects were attached to the first table that used them, and binding the bases to a secondDeclarativeBasein the same process raisedArgumentError: Index 'idx_outbox_events_pending_fetch' is against table 'outbox_events'. Each concrete model now gets its own.Indexes are left as they are on purpose. Neither convention's
ixrule interpolatesconstraint_name, so SQLAlchemy keeps an explicitly namedIndex, and an index name is schema-scoped in PostgreSQL — a finishedidx_<table>_…is the right name for one. A test pins that they do not move under either convention.Rejected
sqlalchemy.schema.conv()around the current names stops the doubling but leavesck_…in the catalog; gauntlet derives_checkas the allowed suffix from the foundation-kit rule and keepschk_as the allowed prefix, so the same test fails for the same reporter. Bare rules unconditionally would create a constraint literally calledattempts_validfor everyone with a plainDeclarativeBaseand make every database built from the documented DDL differ from a fresh one for no reason. Documenting an exclusion fixes nothing.Compatibility
Nobody without a qualifying
ckrule sees a different name; direct callers of the helper are unchanged unless they passmetadata. The one shape change:OutboxEventDBBase.__table_args__and its three siblings aredeclared_attrs now, so reading the tuple off the abstract class no longer works — that was never documented; the documented way is the helper. A database that already carries truncated names keeps them until renamed; alembic's autogenerate does not compare CHECK constraints unlessalembic.ext.checkconstraint_byname(1.19+) is enabled, sodocs/migrations.mdnow carries the query that finds them and theRENAME CONSTRAINT.Verification
The new integration test binds the bases to a
MetaData(naming_convention=<foundation-kit>, schema=…), runscreate_allagainst PostgreSQL 17 and reads the names back throughinspect(conn).get_check_constraints— the same call gauntlet uses.With
origin/master'sorm.pyswapped in, the new tests fail: every test that binds the bases to a secondDeclarativeBasewithArgumentError: Index 'idx_outbox_events_pending_fetch' is against table 'outbox_events'(the one that runs first in the process passes — which is why the existing suite never hit it), the helper-level ones withTypeError: unexpected keyword argument 'metadata'. The truncation itself, on the old helper attached to a bareTableunder the foundation-kit convention: