Fix alembic multi schema migrations#671
Merged
sfc-gh-jcieslak merged 17 commits intoApr 28, 2026
Merged
Conversation
|
|
||
| # Unreleased Notes | ||
|
|
||
| - Fix foreign key reflection returning wrong `referred_schema` for same-schema FKs in non-default schemas, which caused Alembic to generate spurious migrations ([#610](https://github.com/snowflakedb/snowflake-sqlalchemy/issues/610), SNOW-2313675). |
Collaborator
There was a problem hiding this comment.
No source fix in this PR; fix is already in main from #656 — PR description is misleading
Collaborator
There was a problem hiding this comment.
DROP SCHEMA without CASCADE in two tests
test_get_foreign_keys_multi_schema:
# finally block:
metadata.drop_all(engine_testaccount) # drops tables
conn.execute(text(f"DROP SCHEMA IF EXISTS {schema1_name}")) # no CASCADE!
conn.execute(text(f"DROP SCHEMA IF EXISTS {schema2_name}")) # no CASCADE!If drop_all fails partway, the subsequent DROP SCHEMA will fail (schema not empty), leaving both schemas behind.
test_alembic_autogenerate_multi_schema_fk has the same problem — tables are created in schema1/schema2 but schemas are dropped without CASCADE.
test_reflection_same_schema_fk does use CASCADE:
conn.execute(text(f"DROP SCHEMA IF EXISTS {schema_name} CASCADE"))Use CASCADE or ensure tables are explicitly dropped first in a nested try/finally.
Collaborator
There was a problem hiding this comment.
get_multi_foreign_keys (SA 2.x bulk path) not tested
test_get_foreign_keys_multi_schema calls inspector.get_foreign_keys(...) which in SA 2.x typically goes through the single-table path (get_foreign_keys). The get_multi_foreign_keys bulk path (used by MetaData.reflect()) is a different code path and not directly exercised by these tests. The alembic integration test does test MetaData.reflect() behaviour indirectly, but a targeted unit test for get_multi_foreign_keys would be more precise.
| assert foreign_keys_default[0]["referred_schema"] is None | ||
|
|
||
| finally: | ||
| metadata.drop_all(engine_testaccount) |
Collaborator
There was a problem hiding this comment.
Two separate connections are used concurrently. This is harmless since Snowflake DDL is transaction-independent, but the pattern is unusual and could confuse readers.
The typical pattern in this test suite is to manage everything through one connection. Not a blocker, but worth tidying.
sfc-gh-mraba
left a comment
Collaborator
There was a problem hiding this comment.
referred_schema=None for cross-schema FK to default schema may cause Alembic mismatch
snowdialect.py lines 716–720 (_get_table_foreign_keys) and 753–757 (_get_schema_foreign_keys):
same_schemas = {
self.normalize_name(schema), # the schema being reflected
self.default_schema_name, # ← this line
current_schema,
}
default_schema_name is included so that FKs pointing at the default schema return referred_schema=None (no qualifier needed). However, when reflecting a non-default
schema (schema2) that has a FK to the default schema (public), this produces referred_schema=None even though the source and target schemas differ.
Consequence for Alembic autogenerate: if the application's MetaData defines the FK with an explicit schema —
ForeignKey(f"{default_schema}.categories.id", name="fk_to_default")
— Alembic will see referred_schema="public" in metadata vs. referred_schema=None from reflection and generate a spurious remove_fk / add_fk migration.
The new test asserts the current None behavior:
assert foreign_keys_default[0]["referred_schema"] is None
but there is no Alembic autogenerate round-trip test that covers this exact case (non-default-schema table → default-schema table), so the potential mismatch is
untested. Consider either:
1. Adding an Alembic autogenerate test for this scenario (FK from schema2 to the default schema, where metadata defines the FK with an explicit schema), or
2. Documenting intentionally that referred_schema=None is the expected value for FK targets in the default schema, so consumers know to define their ForeignKey(...)
without a schema qualifier in that case.9b5f0e0 to
47ff612
Compare
Changes after review Adjust after review Adjust after review Changes after review Fix after review Fix tests Fix Fix tests
47ff612 to
e7f627f
Compare
6105568 to
cd4c558
Compare
Fix AWS metadata latency in Alembic multi-schema FK test On AWS, SHOW SCHEMAS has its own server-side metadata cache with higher propagation latency than Azure/GCP. The existing SHOW TABLES priming did not cover this, so Alembic's compare_metadata could not find the newly created schemas and generated no add_column diff ops. Add an explicit SHOW SCHEMAS priming call before SHOW TABLES to ensure the new schemas are visible when compare_metadata calls get_schema_names().
aded919 to
c41429b
Compare
1. Added poll_until import from tests.conftest — same pattern used in test_index_reflection.py for the identical AWS metadata propagation issue 2. Removed the manual SHOW SCHEMAS/SHOW TABLES priming — it didn't work because Alembic's compare_metadata issues its own SHOW SCHEMAS internally, ignoring the primed results 3. Wrapped compare_metadata in poll_until — each poll creates a fresh MigrationContext so Alembic re-queries metadata. Returns the diff only when it contains the expected add_column for "status". Timeout of 30s with 2s intervals gives AWS plenty of time to propagate 4. The assertions after the poll remain unchanged — they still validate no spurious add_table or FK churn On Azure/GCP this will pass on the first poll attempt (no delay). On AWS it will retry until the metadata propagates, matching the existing pattern you established. SNOW-2313675: use poll_until to allow cloud propagation ep2 The hypothesis: the original test's single-connection session holds stale SHOW results. Fresh connections should bypass this. SNOW-2313675: use poll_until to allow cloud propagation ep3 The hypothesis: the original test's single-connection session holds stale SHOW results. Fresh connections should bypass this. SNOW-2313675: use poll_until to allow cloud propagation ep4 The hypothesis: the original test's single-connection session holds stale SHOW results. Fresh connections should bypass this. SNOW-2313675: use poll_until to allow cloud propagation ep5 The hypothesis: the original test's single-connection session holds stale SHOW results. Fresh connections should bypass this. SNOW-2313675: replace SHOW with query The hypothesis: the original test's single-connection session holds stale SHOW results. Fresh connections should bypass this. SNOW-2313675: restore workflows The hypothesis: the original test's single-connection session holds stale SHOW results. Fresh connections should bypass this.
0b5e461 to
730d42a
Compare
sfc-gh-mraba
approved these changes
Apr 28, 2026
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 subscribe to this conversation on GitHub.
Already have an account?
Sign in.
2 participants
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.
Please answer these questions before submitting your pull requests. Thanks!
What GitHub issue is this PR addressing? Make sure that there is an accompanying issue to your PR.
Fixes SNOW-2313675: Alembic trying to create same migrations when using multi schema migrations #610
Fill out the following pre-review checklist:
Please describe how your code solves the related issue.
Address a bug in foreign key reflection related to Alembic migrations