Skip to content

test(tortoise): enable aerich downgrade coverage for native DROP COLUMN - #600

Open
mripley wants to merge 1 commit into
awslabs:mainfrom
mripley:enable-drop-column-support
Open

mripley wants to merge 1 commit into
awslabs:mainfrom
mripley:enable-drop-column-support

Conversation

@mripley

@mripley mripley commented Aug 28, 2026

Copy link
Copy Markdown

Summary

test_aerich_add_column_migration skipped its downgrade half behind this comment:

# Note: Downgrade not tested here because DSQL doesn't support ALTER TABLE DROP COLUMN.
# See other tests for downgrade testing.

Aurora DSQL has supported ALTER TABLE ... DROP COLUMN since 2026-08-03, so the note is stale — and it was suppressing real coverage: the aerich downgrade path for a column-adding migration was going untested.

Changes

  • Capture the Command returned by migrate_to() so downgrade() can be called on it. The original code discarded the return value, so there was no handle to downgrade with.
  • Downgrade and assert the added column is gone while its sibling survives.

This follows the shape already used by test_aerich_add_model_and_downgrade_last and test_aerich_multi_model_migration — same command = await migrate_to(...) capture, same downgrade(version=-1, delete=False) call, same connection re-fetch and assertion style. Column-level assertions use the existing get_table_columns() helper (as this test already does) rather than the table_exists the model-level siblings use, since this migration adds a column rather than a table. No new helper, fixture, or marker.

Why the emitted SQL is safe on DSQL

aerich's Migrate._remove_field calls ddl.drop_column, and PostgresDDL inherits the base template ALTER TABLE "{table_name}" DROP COLUMN "{column_name}" — no CASCADE, no MySQL-style quoting. aurora_dsql_tortoise/aerich/patch.py does not intercept drops, so it reaches DSQL as a single-statement DDL, which is what DSQL requires. The column here is an ordinary non-key column, so it avoids the one restriction that remains: dropping a primary key column is still unsupported.

Testing

This is an integration test requiring a live DSQL cluster and IAM auth (tests/integration/conftest.py validates CLUSTER_ENDPOINT at import time), so it runs in CI via the dsql-cluster-create workflow rather than locally. Verified offline:

  • pytest --collect-only tests/integration/test_aerich.py26 tests collected, including test_aerich_add_column_migration[psycopg] and [asyncpg]. Collection exercises the real import chain, including the DSQL backend registration.
  • ruff check → all checks passed; ruff format --check → already formatted.

Please run the integration suite on this branch to confirm the downgrade passes against a real cluster.

Related

Part of the DSQL DROP COLUMN rollout:

The adapters themselves already emit native DROP COLUMN as of #588 — this was the last spot in the repo still gated on the old limitation.

test_aerich_add_column_migration skipped its downgrade half behind a comment
saying DSQL does not support ALTER TABLE DROP COLUMN. That has been supported
since 2026-08-03, so the note was stale and the aerich downgrade path was
going untested.

Capture the Command returned by migrate_to() so downgrade() can be called on
it, then assert the column is gone and its sibling survives -- the same shape
as test_aerich_add_model_and_downgrade_last and
test_aerich_multi_model_migration.

aerich's Migrate._remove_field emits the base PostgresDDL template
ALTER TABLE "t" DROP COLUMN "c" as a single-statement DDL, which DSQL accepts;
the tortoise adapter does not intercept it.
@mripley

mripley commented Aug 28, 2026

Copy link
Copy Markdown
Author

Integration test results — run against a live cluster

Ran on a DSQL gamma cluster in us-west-2 (Python 3.12, both backends).

test_aerich.py — 26/26 passed, including the downgrade coverage this PR restores:

tests/integration/test_aerich.py::test_aerich_add_column_migration[psycopg] PASSED
tests/integration/test_aerich.py::test_aerich_add_column_migration[asyncpg] PASSED
...
======================= 26 passed, 26 warnings in 39.97s =======================

The two assertions cannot pass vacuously: assert "name" in columns proves the column list was really fetched, while assert "description" not in columns proves the downgrade's ALTER TABLE ... DROP COLUMN actually took effect.

Full integration suite — 204 passed, 8 failed. All 8 failures are in test_connection.py and are unrelated to this PR, which touches only test_aerich.py.

Those 8 are an artifact of testing against a gamma cluster. They derive the endpoint from cluster-id + region, which produces the production hostname form:

failed to resolve host 'yzt33bewukovxyosrypra5gpki.dsql.us-west-2.on.aws': [Errno -2] Name or service not known

Note the missing -gamma — the real endpoint is ...dsql-gamma.us-west-2.on.aws, so DNS resolution fails before any auth or query happens.

Verified as pre-existing rather than assumed: I checked out unmodified upstream/main (23da334, with this PR's change confirmed absent) and reproduced the identical 2 failed, 2 errors. They should pass in CI, which provisions a standard production-form cluster via dsql-cluster-create.yml.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants