feat: catch server-default, CHECK-constraint and enum drift the diff test misses - #38
Merged
Merged
Conversation
…test misses `compare_metadata()` skips server defaults unless asked, compares CHECK constraints only through a plugin Alembic 1.19.2 made opt-in, and never compares enum members. `migration_diff_compare_server_default` passes the first through to Alembic; `test_check_constraints_match` and `test_enum_values_match` on `MigrationConsistencyMixin` cover the other two on every Alembic the library supports. The lock moves alembic from 1.19.1 to 1.19.2, the version users install today.
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
test_migrations_up_to_dateis only as thorough ascompare_metadata(), andcompare_metadata()does not look at three things: server defaults (unless
compare_server_defaultis in the contextopts), CHECK constraints, and enum members. A suite that trusts "the diff is empty" is trusting it
for a schema that can still have
is_active DEFAULT falsewhere the model saystrue, nochk_orders_amount_positiveat all, and anorder_statustype the application writes'shipped'into.
Three changes, one per gap.
Server defaults —
migration_diff_compare_server_default: ClassVar[bool] = FalseonMigrationConsistencyMixin, passed straight through ascompare_server_defaultin theMigrationContextopts. Off by default, so no existing suite changes its result. I measured whichspellings agree on PostgreSQL 17 rather than guessing: Alembic's PostgreSQL comparison compares the
text first and, when it differs, asks the server whether the two expressions are equal, so
text("true"),sa.true()and"true"all agree with a column defaulted totrue,func.now()agrees with
now()andCURRENT_TIMESTAMP,"0"with0,text("'{}'")with'{}'::jsonb. Whatit reports is a different value, two different volatile functions (
clock_timestamp()againstnow()), and a default present on one side only — including a model with a Python-sidedefault=against a migration with a
server_default. That last one is why it stays opt-in; the table indocs/guide/configuration.mdsays all of this.CHECK constraints —
test_check_constraints_match, comparing by name, per table, in bothdirections. I did not re-derive names from the convention: SQLAlchemy already does that when the
constraint attaches to the table, so
CheckConstraint("amount > 0", name="amount_positive")under achk_%(table_name)s_%(constraint_name)sconvention already carrieschk_orders_amount_positivebythe time we see the metadata, and
rules_from_metadataonly knows prefixes and suffixes, so it couldnever produce a name anyway. The two cases that leaves are deferred names
(
Boolean(create_constraint=True), resolved at DDL time) and names over 63 characters, whichPostgreSQL truncates with a hash. Alembic's
_get_constraint_final_namehandles both and is whatautogenerate already uses for index and unique-constraint names, so the check test resolves names
exactly the way the diff test does. A named constraint the models declare and the database lacks is
always reported; one the database has and the models do not is reported except on a table where the
model has an unnamed check constraint, because that one carries whatever name PostgreSQL gave it and
the "unexpected" side would be a false positive.
Enum members —
test_enum_values_match, every native, namedEnumcolumn, its labels readthrough the inspector's
get_enums()(ordered byenumsortorder) and compared as a list againstEnum.enums, so a value added in the wrong position is reported too. A type in the database that nocolumn uses is not this test's business.
Both tests go on
MigrationConsistencyMixin, not a fourth mixin: they ask the same question astest_migrations_up_to_datewith the same four fixtures,MigrationTestBasewould include a newmixin anyway, and a separate one would only add a name to learn. The five existing tests keep their
names and behaviour, and
migration_diff_ignore_tablesapplies to both new tests. The comparisonsthemselves live in
utils/diff.pyas two sync helpers next tois_ignored_diff_item, so they areunit-testable and usable from a hand-written check.
Rejected along the way: comparing CHECK constraints by SQL text (PostgreSQL rewrites
amount > 0as(amount > (0)::numeric), so it would only ever match by luck); readingpg_enumby hand, as theissue's lab does, when the inspector already returns the labels in order; and turning the
server-default comparison on by default.
One dependency note. The lock moves alembic 1.19.1 → 1.19.2 (3 lines, alembic only), and it is
load-bearing here rather than housekeeping: 1.19.0 shipped name-based CHECK-constraint detection as
an autogenerate plugin that was on by default, and 1.19.2 renamed it to
alembic.ext.checkconstraint_bynameand took it out of the default plugin set. On 1.19.0 and 1.19.1the diff test therefore reports named CHECK constraints as well, which would make
TestDriftCheckConstraintMissingfail on a test it is not about. Pinning CI to the version usersinstall today keeps the new test's subject unambiguous. Nothing in the change depends on it: both
helpers work on the declared floor, verified below.
Type of change
Checklist
tests/integration/test_drift_issue_37.py(a new sample app in fourvariants: clean, wrong server default, missing check, enum without
shipped) and 15 unit tests forthe two helpers in
tests/unit/test_diff.pymake checkpasses locally (ruff+mypy)CHANGELOG.mdupdated under[Unreleased]— n/a, Release Please owns itdocs/guide/quickstart.md(what each test catches, two new failuremodes in Troubleshooting),
docs/guide/configuration.md(the new attribute and the table ofspellings that agree),
docs/guide/advanced.md,docs/index.md,docs/reference/index.md,README.md, anddocs/agents.md(the seven tests, the two helpers, three new rules)Verification
The full gate, on alembic 1.19.2:
The negative control — the same new tests with
alembic_gauntlet/restored to master's version:The lab from the issue,
tests/test_gauntlet.py— the inherited suite, nothing hand-written —against a wheel built from this branch:
drift_check_missingfails three tests rather than one because that variant's revision 0003 createsnothing on upgrade but still drops the constraint on downgrade, so the stairway and full-downgrade
tests hit
constraint "chk_orders_amount_positive" of relation "orders" does not exist. The new testis the one that names the drift itself.
Floor check, because
_get_constraint_final_nameandPGInspector.get_enumsare the two newimports: on a fresh venv with
alembic==1.8.0andsqlalchemy==2.0.0, the declared minimums, bothhelpers import and return the same lines as on 1.19.2 — including the deferred
Boolean(create_constraint=True)name, resolved tock_users_activeon both.Closes #37