Adds a scheduled Celery task that prunes version history older than
``SUPERSET_VERSION_HISTORY_RETENTION_DAYS`` (default 30; settable
via env var; ``0`` disables retention entirely).
**Task** — ``superset.tasks.version_history_retention.prune_old_versions``:
1. Computes ``cutoff = utcnow() - timedelta(days=N)``.
2. Selects ``version_transaction.id`` rows with ``issued_at <
cutoff`` and filters out any tx whose parent shadow includes a
live row (``end_transaction_id IS NULL``). The live row is the
only preservation rule — closed historical rows including the
baseline (``operation_type=0``) age out. Per-entity minimum-history
floor is an open question tracked in ``future-work.md``.
3. Deletes rows owned by surviving txs in each parent shadow
table (``dashboards_version`` / ``slices_version`` /
``tables_version``).
4. Deletes child-shadow rows for the same transactions
(``table_columns_version`` / ``sql_metrics_version`` /
``dashboard_slices_version``).
5. Drops the surviving ``version_transaction`` rows. The
``version_changes`` rows cascade via the FK from the previous
commit.
Idempotent and safely retried on partial failure.
**Schedule** — ``superset/config.py`` adds the task to the default
``CeleryConfig.beat_schedule`` (nightly at 03:00). Operators who
override ``CeleryConfig`` in their ``superset_config.py`` need to
merge this entry — see UPDATING.md.
Also adds ``"expose_headers": ["ETag"]`` to the default
``CORS_OPTIONS`` so cross-origin browser clients can read the
``ETag`` header introduced in the next commit. (Co-located here
because both touch ``superset/config.py``; the ETag mechanism
itself ships in the next commit.)
**Auto-discovery** — ``superset/tasks/celery_app.py`` adds
``version_history_retention`` to its late-imports so Celery's
auto-discovery picks up the task.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Exposes the version surface as three new endpoints per entity type
(chart, dashboard, dataset), each carrying the standard Superset
decorator stack (``@protect()``, ``@safe``, ``@statsd_metrics``,
``@event_logger.log_this_with_context``) so they appear in FAB's
``action_log`` alongside other audited operations.
| Method | Path | Purpose |
|---|---|---|
| GET | ``/api/v1/{resource}/<uuid>/versions/`` | List version history (oldest-first; per entry: ``version_uuid``, ``version_number``, ``transaction_id``, ``operation_type``, ``issued_at``, ``changed_by``, ``changes`` array) |
| GET | ``/api/v1/{resource}/<uuid>/versions/<version_uuid>/`` | Read-only snapshot of the entity at the requested version (scalar fields plus ``columns`` / ``metrics`` for datasets) |
| POST | ``/api/v1/{resource}/<uuid>/versions/<version_uuid>/restore`` | Replay the snapshot onto the live entity via Continuum's ``Reverter`` (non-destructive — produces a new version row stamping the restoring user via the standard save path) |
``<version_uuid>`` is a deterministic ``UUIDv5(entity_uuid,
transaction_id)`` so it's stable across replicas and retention
pruning. Authorisation reuses the resource's existing ``can_write``
permission; workspace admins can list / restore any entity.
**Restore commands** — ``superset/commands/{chart,dashboard,dataset}/
restore_version.py`` wrap ``VersionDAO.restore_version`` in the
standard ``@transaction()`` boundary. The command resolves the
``Reverter`` once per related collection (split-revert pattern, with
``flush + expire`` between calls) so a multi-relation restore
doesn't trip Continuum's autoflush race that would otherwise mark
half the collection as ``state.deleted=True`` mid-revert.
**Save responses** — ``PUT /api/v1/{resource}/<pk>`` is updated to
include ``old_version`` / ``new_version`` (0-based numbers),
``old_transaction_id`` / ``new_transaction_id`` (stable across
pruning), and ``old_version_uuid`` / ``new_version_uuid`` body
fields so callers can correlate a save with its resulting version
row. The ``ETag`` response header in the next commit is built on
top of this, but the body fields stay — they predate the header
and remain useful for clients that don't read response headers.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Adds a structured per-field change log alongside the foundational
shadow tables. Each save flush emits zero or more ``version_changes``
rows describing what changed relative to the previous version, with
shape ``[{kind, path, from_value, to_value, sequence}]`` keyed to
``version_transaction.id`` (FR-016 .. FR-021).
**Schema** — ``version_changes`` table, FK to ``version_transaction``
with ``ON DELETE CASCADE`` so retention drops dependent records
without explicit cleanup. Composite unique index on
``(transaction_id, entity_kind, entity_id, sequence)`` so the
listener can write monotonically and downstream readers see a
deterministic order.
**Diff engine** (``superset/versioning/diff.py``) — pure-function
diffing of pre-/post-state pairs:
- ``diff_scalar_fields`` for ordinary columns; emits one record per
changed field with JSON-safe ``from_value`` / ``to_value``.
- ``diff_json_field`` for ``json_metadata`` and ``params``, walking
the parsed structure and emitting per-sub-key records. Honours
an ``exclude_keys`` set
(``_DASHBOARD_JSON_METADATA_AUDIT_KEYS``: ``chart_configuration``,
``global_chart_configuration``, ``map_label_colors``,
``show_chart_timestamps``, ``color_namespace``;
``_CHART_PARAMS_AUDIT_KEYS``) so frontend-stamped sub-keys that
mutate on every save don't dominate the change log (FR-022).
- ``diff_dashboard_layout`` walks ``position_json`` structurally
and emits ``[verb, kind, id]`` records (verbs ``add``, ``remove``,
``move``, ``edit``; kinds from a ``CHART``/``ROW``/``COLUMN``/etc.
→ english map) so a UI can render "Added chart 'Foo'" without
re-parsing JSON. ``HEADER_ID`` is suppressed because it duplicates
the ``dashboard_title`` scalar record.
- ``fold_dashboard_layout_with_chart_changes`` deduplicates layout
records against M2M / chart-membership records by UUID so an
add-and-attach doesn't appear twice.
- ``_values_equivalent`` treats ``None`` and ``""`` as equal; this
matches the save path's habit of normalising nullable strings to
the empty string.
**Listener** — ``superset/versioning/changes.py`` registers a
``before_flush`` listener that captures pre-state for each dirty
entity and an ``after_flush`` listener that runs the diff engine
against the post-state and writes ``version_changes`` rows under
the resolved ``transaction_id``. Tracks processed transaction ids
on ``session.info`` so re-firings within a single transaction
(autoflush triggered by mid-commit queries) don't double-insert and
trip the unique constraint. Reads child rows via raw SELECT against
``table_columns`` / ``sql_metrics`` rather than ``dataset.columns``
because the live collection is stale during the restore path's raw
DELETE+INSERT cycle.
**Endpoint surface** — ``VersionDAO.list_change_records_batch``
batches the lookup across multiple transactions with a single
``WHERE transaction_id IN (...)`` query so the version-list
endpoint avoids N+1 round-trips. ``list_versions`` / ``get_version``
return entries with a populated ``changes`` array (empty for
``operation_type=0`` baseline rows).
**Tests** — ``test_diff.py`` covers the diff engine shape (39
unit cases across scalar, JSON, layout, child-collection, and
fold paths). ``change_records_tests.py`` exercises the listener
end-to-end with realistic save flows. ``perf_validation_tests.py``
is the T044 harness for SC-002/3/4 (list endpoint p95 < 1s,
restore < 3s, save overhead < 50ms).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Adds SQLAlchemy-Continuum as a dependency and wires it as the
canonical capture mechanism for chart, dashboard, and dataset edits.
**Schema** — three Alembic migrations, leaving the chain at one
foundation revision plus one child-shadow revision:
- ``version_transaction`` (renamed from Continuum's default
``transaction``; SQL-reserved-word workaround) carries the per-save
``user_id`` / ``issued_at`` and is the join target for all shadow
rows. Auto-incrementing PK; user_id has no FK so import / Celery /
CLI saves can write rows without an active Flask user.
- Parent shadow tables for the three entity types:
``dashboards_version``, ``slices_version``, ``tables_version``.
- Child shadow tables for dataset children + dashboard M2M:
``table_columns_version``, ``sql_metrics_version``,
``dashboard_slices_version`` (composite PK on the M2M shadow,
matching the live ``dashboard_slices`` reshape from
sc-105349-composite-association-pks).
**Models** — ``Dashboard``, ``Slice``, ``SqlaTable`` (and dataset
children ``TableColumn`` / ``SqlMetric``) gain ``__versioned__``
class attributes. The exclude lists carry both M2M relationships
(``owners``, ``roles``, ``dashboards``) and the ``AuditMixin``
columns (``changed_on`` / ``created_on`` / ``changed_by_fk`` /
``created_by_fk`` plus ``last_saved_at`` / ``last_saved_by_fk``
on ``Slice``) so auto-bumped audit fields cannot trigger a
version row on their own (FR-025).
**Plugins** — ``superset/versioning/factory.py`` ships three
Continuum plugins:
- ``VersionTransactionFactory`` renames the transaction table and
appends the unconditional ``user_id`` column.
- ``VersioningFlaskPlugin`` sources the acting user from Superset's
``g.user`` rather than ``flask_login.current_user`` (Superset's
JWT auth populates ``g.user`` but leaves ``current_user``
anonymous on API routes).
- ``SkipUnmodifiedPlugin`` filters Continuum's UPDATE operations,
marking content-equivalent re-saves as ``processed=True`` so they
don't mint no-op shadow rows (FR-026; see follow-up commits for
the test). Lives in this commit because it shares the file with
the other plugins.
**Save-path glue** — a ``before_flush`` baseline listener
(``superset/versioning/baseline.py``) inserts an ``operation_type=0``
shadow row the first time a pre-existing entity is saved, including
the slice-baseline-under-dashboard pattern that gives the dashboard
M2M shadow a row to join against. ``UpdateDashboardCommand`` wraps
its body in ``no_autoflush`` so ``process_tab_diff`` /
``process_native_filter_diff`` don't fire intermediate flushes that
would mint extra version rows. ``DatasetDAO.update_columns`` is
rewritten as a natural-key upsert keyed on ``column_name`` so child
edits flow through ORM events Continuum sees.
**DAO** — ``superset/daos/version.py`` exposes the read API used by
the version endpoints in the next commits:
``current_version_number`` (0-based index, unstable under retention
pruning), ``current_live_transaction_id`` (stable across pruning),
``current_live_version_uuid`` (deterministic UUIDv5), plus
``list_versions`` / ``get_version`` / ``restore_version`` and a
batch ``list_change_records_batch`` for N+1 avoidance.
**Initialization** — ``superset/initialization/__init__.py`` wires
``init_versioning()`` after ``make_versioned()`` runs and the
versioned mappers are configured. Registers the baseline listener
plus the change-record listener (the latter's body lives in the
next commit but the registration site is here because it shares
the init function).
**Tests** — version-capture and version-list integration tests for
each entity type, plus a ``VersionDAO`` unit test suite. Retention
test uses a backdated ``issued_at`` so it can drive
``_prune_old_versions_impl`` synchronously.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Three improvements from @aminghadersohi's review on
apache/superset#39859:
1. **`fk["name"]` unguarded in ``_downgrade_mysql_table`` re-add loop**
The drop loop gates on ``if fk_name := fk.get("name"):`` but the
re-add loop accessed ``fk["name"]`` unconditionally in an f-string.
MySQL/InnoDB always assigns FK names, so this branch was defensive,
but the asymmetry was confusing. Symmetrized via ``continue`` at the
top of the re-add loop.
2. **``ondelete`` whitelist before raw-SQL interpolation**
The value comes from MySQL's ``information_schema`` (not user
input), but interpolating a reflected string into raw SQL without a
guard left a "what if an unexpected value appears" footgun. Added
``_VALID_ONDELETE_ACTIONS`` (the four SQL-standard actions) and a
``RuntimeError`` when an unexpected value is reflected.
3. **Direct ALTER on PostgreSQL for tables with pre-existing UNIQUE**
``recreate="always"`` is dialect-agnostic — on PostgreSQL it
triggers ``CREATE TABLE AS SELECT → DROP → RENAME`` holding
``ACCESS EXCLUSIVE`` for the full table-copy duration. For a
multi-million-row ``dashboard_slices``, that lock window can be
noticeable. The reflected UNIQUE constraint has a stable name on
PostgreSQL (default ``<table>_<cols>_key`` convention), so dropping
it directly and then running structural change as direct ALTER
avoids the copy entirely.
The reflected UNIQUE name is wrapped in a new
``_drop_redundant_unique_by_name()`` helper. Postgres takes the
direct path; MySQL keeps ``recreate="always"`` because InnoDB binds
FKs to the UNIQUE's underlying index for back-reference (``DROP
CONSTRAINT`` on the UNIQUE there raises ``ERROR 1553``); SQLite
keeps ``recreate="always"`` because unnamed UNIQUEs reflect with
``name=None`` and can't be dropped by name.
Verified end-to-end: downgrade-then-upgrade against MySQL with
~12M total junction rows seeded completes in ~1m 41s (within the
range of the prior measurements).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Justin Park (@justinpark) reported on apache/superset#39859:
MySQLdb.OperationalError: (1832, "Cannot change column 'dashboard_id':
used in a foreign key constraint 'fk_dashboard_roles_dashboard_id_dashboards'")
Root cause: ``batch_op.alter_column(fk1, nullable=False)`` for the six
non-UNIQUE association tables emits ``ALTER COLUMN`` on a column that
participates in an FK constraint. MySQL 8 rejects this with ERROR 1832
when the table has data — even when the change is just ``NULL`` →
``NOT NULL`` and the column is already part of a freshly-added
composite primary key (which InnoDB has just made implicitly NOT NULL
anyway). The error fires on populated tables only; CI's ``test-mysql``
shard runs against empty tables and so didn't catch this, while a
real production-shaped install does.
The ``alter_column`` was only ever needed for SQLite, where composite
``PRIMARY KEY`` does not promote constituent columns to ``NOT NULL``
(a long-standing SQLite quirk — only ``INTEGER PRIMARY KEY`` does).
PostgreSQL and MySQL implicitly promote PK columns to ``NOT NULL`` as
part of ``ADD PRIMARY KEY``, so the explicit step is unnecessary on
both — and on MySQL it's actively broken on populated tables.
Fix: extract the ``alter_column`` pair into a helper
``_enforce_not_null_for_sqlite()`` that no-ops on Postgres and MySQL.
Both branches of the per-table upgrade (the ``recreate="always"`` path
for the two UNIQUE-bearing tables, and the direct-ALTER path for the
other six) now call the helper instead of inlining the
``alter_column``.
Verified end-to-end: downgrade-then-upgrade against MySQL with
~12M total junction rows (10M dashboard_slices + 1M each
slice_user/dashboard_user + 100K dashboard_roles) completes in
1m 39s with no ERROR 1832. The 44 in-memory SQLite tests still pass.
Considered Justin's alternative (drop FKs on MySQL across all eight
tables, unifying the two branches) but rejected as more invasive —
it would require capturing FK metadata and explicitly re-creating
the FKs for the six non-recreate tables, since they don't go through
the ``copy_from`` path that re-creates FKs automatically. The
SQLite-only approach is more targeted: it removes the operation that
MySQL rejects rather than working around the rejection.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Extends the stress-test seed script with an optional duplicate-row
injection step, used to measure the empirical cost of the migration's
``_dedupe_by_min_id`` phase.
Usage: after running the normal seed at a given scale, add
``--dirty-duplicates-pct 5`` (or any non-zero value) to inject that
percentage of duplicate ``(fk1, fk2)`` rows into each non-UNIQUE
junction (slice_user, dashboard_user, dashboard_roles —
dashboard_slices is skipped because its UNIQUE constraint, present
both pre- and post-migration, rejects duplicates).
Pre-condition: requires the DB to be at the pre-migration revision
(33d7e0e21daa). The post-migration composite PK rejects duplicates,
so attempting to inject on the upgraded schema errors out.
Empirical result on MySQL @ 10M dashboard_slices + ~2.1M other
junction rows + 105K injected duplicates (5% on the 3 non-UNIQUE
tables):
Upgrade time: 1m 36s vs clean baseline 1m 37s
→ dedupe cost is within measurement noise; the table-scan that
the migration already performs dominates whether or not
duplicates exist.
This empirically confirms what the cost-model predicted: the
``_dedupe_by_min_id`` GROUP BY scan is the dominant cost of that
phase, and the actual per-duplicate DELETE is negligible.
NULL-FK injection deliberately skipped — would require altering the
six non-UNIQUE FK columns from NOT NULL back to nullable (the
migration's downgrade keeps them NOT NULL by design), which adds
per-backend ALTER complexity for a code path that's structurally
identical in cost shape (DELETE WHERE col IS NULL is the same scan
shape as the dedupe scan).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Add ``scripts/seed_junction_load.py``, a backend-agnostic script that
bulk-inserts synthetic parent rows (dashboards, slices, users, roles,
tables, dbs) and many-to-many junction rows for the four largest
association tables targeted by the composite-PK migration:
``dashboard_slices``, ``slice_user``, ``dashboard_user``,
``dashboard_roles``.
Designed for measuring migration runtime at varying scales — run with
a series of size flags (100K / 1M / 5M / 10M for the target table)
and time the migration at each scale to verify the predicted
``O(N log N)`` extrapolation against real numbers.
Properties:
- **Reproducible**: deterministic cross-product walk through parent IDs
produces a stable pair sequence; re-running is replayable.
- **Idempotent**: re-running with the same target is a no-op; with a
higher target, only new rows are added.
- **Backend-agnostic**: connects via Superset's standard ``DATABASE_*``
env vars (or ``SUPERSET__SQLALCHEMY_DATABASE_URI``). Branches on
dialect for ``BINARY(16)`` vs ``UUID`` vs TEXT/BLOB UUID columns.
- **Batched**: bulk INSERT 10K rows per statement.
- **Per-phase timing**: logs elapsed wall time for the parents phase,
the junctions phase as a whole, and per junction-table.
- **Avoidance set**: loads existing junction pairs into a Python set
so re-runs on top of pre-existing data don't collide on the
uniqueness constraint.
Usage (inside the Superset container):
docker exec superset-superset-1 \\
/app/.venv/bin/python /app/scripts/seed_junction_load.py \\
--dashboard-slices 1000000
Defaults target a "large multi-team install" shape: 1M
``dashboard_slices``, 100K each ``slice_user`` / ``dashboard_user``,
10K ``dashboard_roles``. Override per-table via flags.
Tested locally on MySQL (the user's current eval stack):
- 200/100/100/50 row mini-run produced expected counts.
- Re-running at the same target is a no-op (idempotent).
- ``--dry-run`` plans without writing.
Junction tables not yet covered (``sqlatable_user``, ``rls_filter_*``,
``report_schedule_user``) are typically small in production and
require additional parent seeding (RLS filters, report schedules)
that wasn't worth the scope here. Adding them is straightforward by
extending ``JUNCTIONS`` and writing the corresponding parent seeder.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Fix two follow-on issues reported when starting the dev stack with
docker-compose-mysql.yml:
1. ``superset-init`` step 4 (load-examples) fails with
``MySQLdb.OperationalError: (2002, "Can't connect to server on 'db'")``
because the analytics-examples DB connection inherits ``EXAMPLES_PORT=5432``
(Postgres port) from ``docker/.env``. The override flipped
``DATABASE_DIALECT`` to ``mysql+mysqldb`` but left the EXAMPLES_*
group on Postgres defaults, so the URI became
``mysql+mysqldb://examples:examples@db:5432/examples`` — MySQL
container has no listener on 5432.
Fix: add ``EXAMPLES_HOST/PORT/DB/USER/PASSWORD`` and a complete
``SUPERSET__SQLALCHEMY_EXAMPLES_URI`` to the ``mysql-env`` anchor.
2. The Postgres init scripts under
``docker/docker-entrypoint-initdb.d/`` (``cypress-init.sh``,
``examples-init.sh``) get mounted on the MySQL container too —
compose merges volume lists. They invoke ``psql`` which doesn't
exist in the MySQL image, abort with ``psql: command not found``,
and prevent the ``examples`` DB from being created.
Fix: add a MySQL-specific init script
``docker/mysql-init/examples-init.sql`` that creates the
``examples`` database and user, and mount it at
``/docker-entrypoint-initdb.d`` in the override. Compose's
later-takes-precedence rule on duplicate volume targets displaces
the Postgres init dir, so the MySQL container only sees the
MySQL-compatible script.
(Used a plain duplicate-target mount rather than the ``!override``
tag because pre-commit's ``check-yaml`` doesn't recognize Compose's
custom YAML tags.)
Recovery for an existing failed MySQL stack: ``docker compose -f
docker-compose.yml -f docker-compose-mysql.yml down``, then
``docker volume rm superset_db_home_mysql`` (so the new init script
runs on the next fresh boot), then ``up`` again.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Adds ``docker-compose-mysql.yml``, a compose-override file that swaps
the default Postgres metadata DB for MySQL 8 with one extra ``-f``
flag:
docker compose -f docker-compose.yml -f docker-compose-mysql.yml up
Useful for evaluating dialect-specific behaviour (e.g., the runtime
cost of DDL migrations on a deployment whose production metadata DB
is MySQL — the question raised by review feedback on this PR).
Mirrors the connection settings used by CI's ``test-mysql`` shard:
``mysql+mysqldb`` dialect, charset ``utf8mb4`` with binary_prefix.
Host port defaults to 13306 (configurable via ``DATABASE_PORT_MYSQL``)
to avoid colliding with a native MySQL install on 3306.
A separate volume (``db_home_mysql``) keeps MySQL data isolated from
the Postgres ``db_home`` volume, so switching between the two with
``-f`` flag toggles doesn't corrupt either side.
The Postgres-specific init scripts under
``docker/docker-entrypoint-initdb.d/`` are not mounted on the MySQL
service (they are postgres-only). Examples / cypress fixtures still
load via ``superset-init``'s post-startup steps, which run
``superset load-examples`` against whichever metadata DB is in use.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Mirror of the PostgreSQL diagnostic queries added in 11148779ed,
adapted for MySQL/InnoDB. One important difference: InnoDB rebuilds
the clustered index on every PK change, so all eight tables undergo
a full table rebuild on MySQL — not just the two that go through
the explicit ``recreate="always"`` path. The lock-window estimate
query is updated to cover all eight rather than just two, and the
"migration_path" column makes the rebuild expectation explicit
("direct ALTER (still rebuilds InnoDB clustered index)").
Other notes:
- ``information_schema.TABLES.TABLE_ROWS`` is an InnoDB estimate,
analogous to PostgreSQL's ``reltuples``; documented inline.
- ``KEY_COLUMN_USAGE`` carries both sides of the FK in a single
row on MySQL, so the external-FK pre-flight check is simpler
than the PostgreSQL version (no joins between three views).
- The aggregated dedupe query is portable standard SQL; included
verbatim for copy-paste convenience.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Add a "Sizing the maintenance window on PostgreSQL" sub-section to the
operator runbook. The simple per-table COUNT/duplicate/NULL queries
that were already there are dialect-portable but only count rows;
operators on PostgreSQL with large deployments need to characterize
the migration's runtime cost before scheduling it.
Adds four diagnostic queries:
- Per-table size, row count (from pg_class.reltuples), and which
migration path each table will take (recreate-rewrite vs direct
ALTER). Sizes the work concretely.
- Aggregated duplicate-row roll-up: dup_groups + total rows_dropped
per table. Replaces eight separate per-table queries with one
consolidated result for audit/dump-before-apply decisions.
- External-FK pre-flight check (the same one the migration runs at
upgrade time and aborts on). Lets operators surface any blocking
external reference ahead of the maintenance window. Should be
empty on a stock install.
- Lock-window estimate for the two full-rewrite tables, using
pg_relation_size and a conservative 100 MB/s rewrite throughput
assumption. The other six use direct ALTER and are dominated by
composite-index build time (seconds for low-millions-of-rows
tables).
Prompted by reviewer feedback on apache/superset#39859 from a large
deployment asking how to size the maintenance window. The original
pre-flight queries are kept for cross-dialect operators (MySQL,
SQLite) since the new queries use PostgreSQL-specific catalog views.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
CI cypress + playwright shards were red with:
ERROR [flask_migrate] Error: Multiple head revisions are present
for given argument 'head'
The recent rebase onto master pulled in
``33d7e0e21daa_add_semantic_layers_and_views.py`` (from PR #37815,
"semantic layer extension"), which had been authored against
``ce6bd21901ab`` as its parent — the same parent our migration
referenced. After the rebase both migrations point at
``ce6bd21901ab``, producing two heads and breaking ``flask db
upgrade head`` for any downstream consumer (CI's Cypress / Playwright
shards spin up a real Superset instance via ``superset db upgrade``,
which is why those shards failed first; the integration shards run
against a precomputed schema and didn't surface this).
Fix: chain our migration after the semantic-layer migration by
pointing ``down_revision`` at ``33d7e0e21daa``. The chain is now
linear:
... → ce6bd21901ab → 33d7e0e21daa (semantic layers)
→ 2bee73611e32 (composite PK, this PR)
Verified with ``superset db heads`` (returns single head
``2bee73611e32``) and the local migration test suite (44 passed,
1 skipped).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Found by running fresh-install + round-trip against a real SQLite DB:
6 of the 8 affected tables had FK columns that were originally
declared nullable. PostgreSQL and MySQL implicitly promote the
constituent columns of an ``ALTER TABLE ... ADD PRIMARY KEY`` to
``NOT NULL``; SQLite does not (it's a long-standing SQLite quirk —
only ``INTEGER PRIMARY KEY`` enforces NOT NULL on a composite-PK
column). Result: a fresh SQLite install would accept
``INSERT INTO dashboard_slices (NULL, 5)`` despite both columns
being part of the composite PK.
Our integration tests previously masked this: the test fixture seeds
columns with ``nullable=False``, so the post-upgrade NOT NULL
assertion passed regardless of whether the migration enforced it.
Fix: add explicit ``batch_op.alter_column(fk, nullable=False)`` for
both FK columns inside the per-table batch_alter_table block. On
PostgreSQL and MySQL this is a no-op (PK already implies NOT NULL);
on SQLite it adds the missing NOT NULL declaration so a fresh
install matches the data-model.md "After" contract.
Verified end-to-end:
- Postgres + MySQL: column shape unchanged (still NOT NULL)
- SQLite fresh install + round-trip: all 8 tables now have NOT NULL
on FK columns, ``INSERT (NULL, 5)`` correctly rejected with
IntegrityError on dashboard_slices, dashboard_user, sqlatable_user
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Two MySQL-only failures in the downgrade path, found by running the
full migration history against a fresh MySQL 8 container:
1. ``MySQLdb.OperationalError: (1553, "Cannot drop index 'PRIMARY':
needed in a foreign key constraint")``. InnoDB uses the composite
PK index to back the FK on the leftmost column. The downgrade
tried to drop the composite PK before dropping the FKs, orphaning
the FK's backing index. PostgreSQL and SQLite create separate
indexes for FK columns and don't trip on this.
2. ``Field 'id' doesn't have a default value`` on subsequent INSERT.
``sa.Identity(always=False)`` only emits ``AUTO_INCREMENT`` on
MySQL when the column is created with ``primary_key=True`` — our
portable path adds the column first then creates the PK separately,
so MySQL leaves the column without auto-generation. Existing rows
would all collide on id=0; future inserts fail because no default.
Postgres' ``GENERATED BY DEFAULT AS IDENTITY`` and SQLite's
``INTEGER PRIMARY KEY`` rowid alias don't have this gap.
Fix: extract ``_downgrade_mysql_table()`` that emits the canonical
MySQL idiom — drop FKs, then a single ALTER combining
``DROP PRIMARY KEY, ADD COLUMN id INT NOT NULL AUTO_INCREMENT,
ADD PRIMARY KEY (id)`` (which backfills existing rows with sequential
ids and preserves AUTO_INCREMENT), restore the redundant UNIQUE on
the 2 tables that originally had it, and re-add the FKs with their
original names. Postgres and SQLite keep the existing portable
``batch_alter_table`` path.
Raw SQL is unavoidable for the combined-ALTER form; per the
constitution it's allowed for dialect-specific DDL with no SQLA
equivalent, with triple-quoted strings for legibility.
Verified end-to-end: upgrade → downgrade → upgrade against a fresh
MySQL 8 container with INSERT-without-id sanity check showing the
restored ``id`` column auto-increments correctly.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
CI test-mysql failed with:
MySQLdb.OperationalError: (1826, "Duplicate foreign key constraint
name 'fk_dashboard_slices_slice_id_slices'")
Root cause: MySQL scopes foreign-key constraint names per-database,
not per-table (PostgreSQL and SQLite scope per-table). The
``batch_alter_table(... recreate="always", copy_from=...)`` path
used for ``dashboard_slices`` and ``report_schedule_user`` builds
``_alembic_tmp_<table>`` carrying the original FK names from
``copy_from`` while the original table still holds those names — MySQL
rejects the temp-table creation with ERROR 1826.
Fix: on MySQL only, drop the original FK constraints by name before
the ``batch_alter_table`` runs. The ``copy_from`` re-creates them on
the rebuilt table with their original names, so the post-migration
shape is unchanged. On PostgreSQL and SQLite the original code path
still runs unchanged.
Local SQLite tests (44 passed, 1 skipped) still pass; CI will validate
on MySQL.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Address Beto's review comments on apache/superset#39859: replace
``sa.text(f"...")`` SQL construction in the three pre-flight helpers
(``_delete_null_fk_rows``, ``_dedupe_by_min_id``, ``_assert_no_duplicates``)
with SQLAlchemy core constructs (``sa.delete``, ``sa.select``,
``sa.func``, ``.subquery()``, ``.notin_()``).
A small ``_table_clause()`` helper builds a lightweight ``TableClause``
exposing the columns the queries reference; the three helpers consume
it. Removes all ``# noqa: S608`` comments — they are no longer needed
because there is no string-interpolated SQL.
Verified the compiled SQL is identical on Postgres, MySQL, and SQLite,
including the MySQL ERROR 1093 workaround (the inner aggregation is
wrapped in a derived table via ``.subquery()``, producing
``... NOT IN (SELECT keep_id FROM (SELECT min(id) ...) AS keep_min)``).
Also drops the redundant ``f`` prefix on the two non-interpolating
lines of the ``_check_no_external_fks_to_id`` error message.
44 migration tests still pass.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Four operator-experience improvements from the second review pass:
1. ``TABLES_WITH_NULLABLE_FKS`` is now explicitly documented as an
informational set that is not consulted at runtime; the comment
explains the previous ``dashboard_roles`` omission was the bug
that motivated the always-run cleanup.
2. ``_delete_null_fk_rows`` docstring updated to match the
"always run" semantics (was still claiming "called only on tables
in TABLES_WITH_NULLABLE_FKS").
3. ``_check_no_external_fks_to_id`` now documents its scope
limitation: ``Inspector.get_table_names()`` returns the default
schema only, so cross-schema FKs in non-standard multi-schema
PostgreSQL deployments would not be caught. The single-schema
case (Superset's documented deployment) is fully covered.
4. ``_dedupe_by_min_id`` now logs a sample of up to 10 discarded
``(fk1, fk2, id)`` tuples at WARN before deletion, so operators
can audit which rows the ``MIN(id)`` policy drops. The keep-
original policy is correct in practice but discards later
re-grants on ownership tables; the sample makes that visible.
5. ``UPDATING.md`` documents the upgrade/downgrade primary-key
name divergence (``pk_<table>`` vs ``<table>_pkey``) so
operators using schema-comparison tools don't mistake it for
migration drift.
No schema or runtime-behaviour changes. All 44 migration tests pass.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Two cleanups from PR review:
1. ``dashboard_roles.dashboard_id`` was created nullable in revision
e11ccdd12658 but was missing from ``TABLES_WITH_NULLABLE_FKS``. A
production database with a stray NULL ``dashboard_id`` row would have
failed the PK-add with a cryptic constraint violation. Fix by running
the NULL-FK cleanup on every affected table — it is a no-op DELETE on
tables whose FK columns are already NOT NULL, and it eliminates the
risk of further drift in the hardcoded set. ``dashboard_roles`` is
added to the documentation set; the runtime now does not consult it.
2. The unit-test parent-table name for ``rls_filter_roles`` and
``rls_filter_tables`` was ``rls_filter`` (does not exist) instead of
the real parent ``row_level_security_filters``. Test passes either
way (the in-memory FK is self-consistent), but the parameter is now
accurate.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Replace synthetic id INTEGER PRIMARY KEY with composite PRIMARY KEY (fk1, fk2)
on the eight pure-junction tables: dashboard_roles, dashboard_slices,
dashboard_user, report_schedule_user, rls_filter_roles, rls_filter_tables,
slice_user, sqlatable_user. The redundant UNIQUE(fk1, fk2) on dashboard_slices
and report_schedule_user is dropped (subsumed by the new PK).
Migration handles dialect quirks: copy_from for tables with pre-existing
UNIQUE (so SQLite's anonymous-constraint reflection doesn't matter), wrapped-
subquery dedupe for MySQL (ERROR 1093), sa.Identity(always=False) on downgrade
to backfill the restored id column without NOT NULL violations, and distinct
PK names per direction (pk_<table> on upgrade, <table>_pkey on downgrade) to
avoid round-trip index-name collisions on Postgres.
ORM Table() definitions updated to match. UPDATING.md entry added with
operator runbook (BI-tool impact, pre-flight inventory queries, dedupe-row-
loss notice, pg_dump workaround, FK-NOT-NULL downgrade asymmetry note).
Tests: 8 schema-shape assertions (post-upgrade), 8 duplicate-rejection unit
tests, 8 distinct-pair sanity tests, 1 round-trip + idempotency test
(in-memory SQLite via Alembic MigrationContext).
Continuum-restore verification against the new shape is out of scope for this
PR; it is the responsibility of the versioning epic (sc-103156).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
