Skip to content

fix(migrations): actually drop _customer_location_uc (list == set no-op) - #42642

Draft
mikebridge wants to merge 1 commit into
apache:masterfrom
mikebridge:sc-112173-fix-noop-uc-drop
Draft

fix(migrations): actually drop _customer_location_uc (list == set no-op)#42642
mikebridge wants to merge 1 commit into
apache:masterfrom
mikebridge:sc-112173-fix-noop-uc-drop

Conversation

@mikebridge

Copy link
Copy Markdown
Contributor

SUMMARY

Migration df3d7e2eb9a4 (2024) intended to drop the legacy 3-column unique constraint _customer_location_uc (database_id, schema, table_name) from tables, but passed a list to generic_find_uq_constraint_name, whose body compares columns == set(uq["column_names"]). list == set is always False in Python, so the constraint was never found and never dropped — a silent no-op.

Consequences on databases migrated through it:

  • The NULL-leaky 3-column constraint (no catalog leg) is still present, while schemas built from model metadata (create_all, e.g. CI) carry the model's intended 4-column constraint (database_id, catalog, schema, table_name) instead — so CI cannot reproduce production failure shapes.
  • A row keeps occupying (database_id, schema, table_name) across catalogs: creating the same table under a different catalog passes every catalog-aware app-level check and then hits an opaque IntegrityError only on migrated databases.

Two-part fix:

  1. generic_find_uq_constraint_name now coerces its columns argument to a set (parameter widened to Collection[str]), removing the foot-gun for all callers. Existing set-passing callers are unaffected.
  2. A take-2 migration (16755d4ca4ae) re-attempts the drop with exact set matching. It is a harmless no-op where the constraint is already absent, and the model's 4-column constraint can never match a 3-column set comparison, so it is not at risk. Downgrade restores the legacy constraint best-effort (rows written after the drop may legitimately collide across catalogs), mirroring the tolerant try/except posture of the constraint's original creator.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

N/A — schema-only change.

TESTING INSTRUCTIONS

  1. pytest tests/unit_tests/utils/test_core.py -k generic_find_uq — 3 tests: the list-argument regression pin (fails against the pre-fix helper), the set-argument happy path, and the exact-match guarantee (a 3-column lookup never matches the model's 4-column constraint).
  2. Migration verified on SQLite and PostgreSQL: fresh full-chain superset db upgrade (clean), then superset db downgrade (constraint recreated: _customer_location_uc (database_id, schema, table_name)), then superset db upgrade (constraint dropped), with uq_tables_uuid untouched throughout. On a database that still carries the legacy constraint, the upgrade drops it; on one that never had it, the lookup finds nothing and the migration no-ops.

ADDITIONAL INFORMATION

  • Has associated issue:
  • Required feature flags:
  • Changes UI
  • Includes DB Migration (follow approval process in SIP-59)
    • Migration is atomic, supports rollback & is backwards-compatible
    • Confirm DB migration upgrade and downgrade tested
    • Runtime estimates and downtime expectations provided: single ALTER TABLE ... DROP CONSTRAINT on tables (or a no-op lookup); sub-second on PostgreSQL/MySQL, table-rebuild via batch mode on SQLite. No data movement, no downtime expected.
  • Introduces new feature or API
  • Removes existing feature or API

🤖 Generated with Claude Code

Migration df3d7e2eb9a4 intended to drop the legacy 3-column unique
constraint _customer_location_uc (database_id, schema, table_name) from
the tables table, but passed a list to generic_find_uq_constraint_name,
whose body compares columns == set(uq['column_names']). list == set is
always False in Python, so the constraint was never found and never
dropped — a silent no-op.

Databases migrated through it therefore still carry the constraint,
which leaks across catalogs (no catalog leg) and diverges from schemas
built from model metadata, where the model's 4-column constraint exists
instead. A row can occupy (database_id, schema, table_name) across
catalogs, passing every catalog-aware app-level check and then hitting
an opaque IntegrityError only on migrated databases.

Fix in two parts:
- generic_find_uq_constraint_name coerces its columns argument to a
  set, removing the foot-gun for all callers (existing set-passing
  callers unaffected).
- A take-2 migration re-attempts the drop with exact set matching.
  No-op where the constraint is already absent; the model's 4-column
  constraint can never match a 3-column set comparison. Downgrade
  restores the legacy constraint best-effort, mirroring the tolerant
  posture of its original creator.

Verified on SQLite and PostgreSQL: fresh full-chain upgrade, then
downgrade (constraint recreated) and re-upgrade (constraint dropped),
with uq_tables_uuid untouched throughout.

Fixes sc-112173.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@github-actions github-actions Bot added the risk:db-migration PRs that require a DB migration label Jul 31, 2026
@codecov

codecov Bot commented Jul 31, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 33.33333% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 65.44%. Comparing base (7d2b184) to head (01ee065).

Files with missing lines Patch % Lines
superset/utils/core.py 33.33% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master   #42642      +/-   ##
==========================================
- Coverage   65.44%   65.44%   -0.01%     
==========================================
  Files        2810     2810              
  Lines      159362   159363       +1     
  Branches    36372    36372              
==========================================
- Hits       104301   104298       -3     
- Misses      53019    53022       +3     
- Partials     2042     2043       +1     
Flag Coverage Δ
hive 38.09% <33.33%> (-0.01%) ⬇️
mysql 57.82% <33.33%> (-0.01%) ⬇️
postgres 57.86% <33.33%> (-0.01%) ⬇️
presto 39.98% <33.33%> (-0.01%) ⬇️
python 59.25% <33.33%> (-0.01%) ⬇️
sqlite 57.49% <33.33%> (-0.01%) ⬇️
unit 100.00% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

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

Labels

risk:db-migration PRs that require a DB migration size/L

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant