codeant-ai-for-open-source[bot] commented on code in PR #41076:
URL: https://github.com/apache/superset/pull/41076#discussion_r3500135902
##########
superset/dashboards/api.py:
##########
@@ -828,17 +887,32 @@ def put(self, pk: int) -> Response:
# This validates custom Schema with custom validations
except ValidationError as error:
return self.response_400(message=error.messages)
+
+ # Live version identifiers before the update (empty + query-free when
+ # ``ENABLE_VERSIONING_CAPTURE`` is off).
+ old_info = current_entity_version_info(Dashboard, pk)
+
try:
changed_model = UpdateDashboardCommand(pk, item).run()
last_modified_time = changed_model.changed_on.replace(
microsecond=0
).timestamp()
+ new_info = current_entity_version_info(
+ Dashboard, changed_model.id, changed_model.uuid
+ )
Review Comment:
**Suggestion:** Add an explicit type annotation for this newly introduced
local variable to comply with the mandatory type-hint rule for relevant
variables. [custom_rule]
**Severity Level:** Minor โ ๏ธ
<details>
<summary><b>Why it matters? ๐ค </b></summary>
This newly added local variable is also unannotated even though it is
assigned from a known helper return value. That is a real omission under the
type-hint requirement.
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=55fd0d3edca94a1ebf3bf65a517f70d1&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=55fd0d3edca94a1ebf3bf65a517f70d1&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent ๐ค </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/dashboards/api.py
**Line:** 900:902
**Comment:**
*Custom Rule: Add an explicit type annotation for this newly introduced
local variable to comply with the mandatory type-hint rule for relevant
variables.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41076&comment_hash=a365287eaade1d1c29c5d8f460de8340ab35560fc212a65ca804970307c98f07&reaction=like'>๐</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41076&comment_hash=a365287eaade1d1c29c5d8f460de8340ab35560fc212a65ca804970307c98f07&reaction=dislike'>๐</a>
##########
superset/migrations/versions/2026-05-01_23-36_2bee73611e32_composite_pk_association_tables.py:
##########
@@ -0,0 +1,580 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+"""composite_pk_association_tables
+
+Replace the unused synthetic ``id INTEGER PRIMARY KEY`` on eight many-to-many
+association tables with a composite primary key on the two FK columns. Drops
+the now-redundant ``UniqueConstraint(fk1, fk2)`` on the two tables that
+already carry one. Pre-flight: deletes rows with NULL FK values (six tables
+allow them today) and any duplicate ``(fk1, fk2)`` rows.
+
+Motivated by SQLAlchemy-Continuum issue #129 (M2M restore against junction
+tables with surrogate PKs); also closes the data-integrity hole where six
+of the eight tables lacked DB-level uniqueness.
+
+Revision ID: 2bee73611e32
+Revises: 78a40c08b4be
+Create Date: 2026-05-01 23:36:34.050058
+
+"""
+
+import logging
+from typing import NamedTuple
+
+import sqlalchemy as sa
+from alembic import op
+from alembic.operations.base import BatchOperations
+from sqlalchemy import inspect
+from sqlalchemy.engine import Connection
+
+# revision identifiers, used by Alembic.
+revision = "2bee73611e32"
+down_revision = "78a40c08b4be"
Review Comment:
**Suggestion:** Add explicit type annotations for the Alembic revision
metadata variables to satisfy the type-hint requirement for annotatable
module-level variables. [custom_rule]
**Severity Level:** Minor โ ๏ธ
<details>
<summary><b>Why it matters? ๐ค </b></summary>
These module-level variables are plain Python assignments and can be
annotated, so they fit the type-hint rule for annotatable variables. The file
already uses type hints elsewhere, but these Alembic metadata variables omit
them.
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=c9eb0fc247444f80b6e717321f5b968d&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=c9eb0fc247444f80b6e717321f5b968d&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent ๐ค </b></summary>
```mdx
This is a comment left during a code review.
**Path:**
superset/migrations/versions/2026-05-01_23-36_2bee73611e32_composite_pk_association_tables.py
**Line:** 45:46
**Comment:**
*Custom Rule: Add explicit type annotations for the Alembic revision
metadata variables to satisfy the type-hint requirement for annotatable
module-level variables.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41076&comment_hash=3884dc7e703913a5741979baf634f529d41d14792b35e32d6c351552e8150680&reaction=like'>๐</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41076&comment_hash=3884dc7e703913a5741979baf634f529d41d14792b35e32d6c351552e8150680&reaction=dislike'>๐</a>
##########
superset/dashboards/api.py:
##########
@@ -828,17 +887,32 @@ def put(self, pk: int) -> Response:
# This validates custom Schema with custom validations
except ValidationError as error:
return self.response_400(message=error.messages)
+
+ # Live version identifiers before the update (empty + query-free when
+ # ``ENABLE_VERSIONING_CAPTURE`` is off).
+ old_info = current_entity_version_info(Dashboard, pk)
Review Comment:
**Suggestion:** Add an explicit variable type annotation for this new local
value (for example, the shared version-info dataclass type) to satisfy the
type-hint requirement. [custom_rule]
**Severity Level:** Minor โ ๏ธ
<details>
<summary><b>Why it matters? ๐ค </b></summary>
The new local variable is introduced without any type annotation, and its
return type is known/annotatable. This matches the python type-hint rule for
relevant variables.
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=01aa8ba3281e43a98f2bf8814e2d1dbf&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=01aa8ba3281e43a98f2bf8814e2d1dbf&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent ๐ค </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/dashboards/api.py
**Line:** 893:893
**Comment:**
*Custom Rule: Add an explicit variable type annotation for this new
local value (for example, the shared version-info dataclass type) to satisfy
the type-hint requirement.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41076&comment_hash=379cbc5e2b26d3c8daa461664e09b6bb30b0c5d18e4ef28dd72681ceab9af52a&reaction=like'>๐</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41076&comment_hash=379cbc5e2b26d3c8daa461664e09b6bb30b0c5d18e4ef28dd72681ceab9af52a&reaction=dislike'>๐</a>
##########
superset/migrations/versions/2026-05-28_19-50_56cd24c07170_add_versioning_tables.py:
##########
@@ -0,0 +1,562 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+"""add_versioning_tables
+
+Creates the full schema backing entity versioning in a single
+migration:
+
+1. ``version_transaction`` โ audit log keyed by Continuum's per-flush
+ transaction id (plus a Postgres-specific id sequence).
+2. **Parent shadow tables** mirroring each versioned entity's columns:
+ ``dashboards_version`` / ``slices_version`` / ``tables_version``.
+3. ``version_changes`` โ field-level diff log keyed to a
+ ``(transaction, entity)`` pair; each row describes one atomic change
+ (one field or one child-collection element) that occurred during a
+ save.
+4. **Child shadow tables** for the collections Continuum auto-registers
+ when ``__versioned__`` is applied to ``TableColumn`` / ``SqlMetric``
+ and the ``slices`` exclude is removed from
+ ``Dashboard.__versioned__``: ``table_columns_version`` /
+ ``sql_metrics_version`` / ``dashboard_slices_version``.
+
+All shadow tables follow the validity-strategy shape (mirrored columns
++ ``transaction_id`` / ``end_transaction_id`` / ``operation_type``
+bookkeeping with FKs to ``version_transaction.id``). The current
+version row has ``end_transaction_id = NULL``.
+
+This migration replaces three iterative migrations from the spike phase
+(``56cd24c07170``, ``e1f3c5a7b9d0``, ``f7a2b3c4d5e6``) that captured the
+same schema in three steps as the feature was developed. Compacting
+gives downstream operators one migration to apply / reverse and one
+review surface. The ``revision`` hash is reused from the original first
+migration so anyone still tracking the chain by that hash lands on the
+same logical change set.
+
+Generated by hand because the current Continuum + Alembic-autogenerate
+interaction trips on the renamed ``transaction`` -> ``version_transaction``
+table key (``KeyError`` lookups in ``table_key_to_table``). Column
+inventories were sourced from the live model ``__table__`` definitions
+and ``version_class(...).__table__`` / Continuum association metadata.
+
+Primary key choice. Both ``version_transaction.id`` and
+``version_changes.id`` are ``BigInteger`` autoincrement โ a deliberate
+carveout from the project's UUID-PK convention for new models (see
+``CLAUDE.md`` ยง"UUID Migration"). ``version_transaction`` is keyed
+externally by SQLAlchemy-Continuum via
+``nextval('version_transaction_id_seq')`` on every INSERT; matching
+that contract is required for ``versioning_manager`` to function.
+``version_changes`` follows the same shape because the user-facing
+identity is the ``(transaction_id, entity_kind, entity_id, sequence)``
+composite unique key, not the row id; the API surfaces a deterministic
+UUIDv5 ``version_uuid`` derived from ``entity.uuid`` and
+``transaction_id`` for stable external references.
+
+Revision ID: 56cd24c07170
+Revises: 2bee73611e32
+Create Date: 2026-05-28 19:50:00.000000
+
+"""
+
+from __future__ import annotations
+
+import sqlalchemy as sa
+from alembic import op
+from sqlalchemy_utils import UUIDType
+
+from superset.utils.core import MediumText
+
+revision = "56cd24c07170"
+# Stacked on the composite-PK association-tables change (2bee73611e32) so the
+# Continuum shadow tables this migration creates can mirror the
+# composite-PK shape of the live association tables. If that change
+# is removed from the stack, this should be reverted to "ce6bd21901ab".
+down_revision = "2bee73611e32"
+
+
+def upgrade() -> None:
+ bind = op.get_bind()
+
+ # ------------------------------------------------------------------
+ # version_transaction
+ #
+ # Audit log for each versioning event. Continuum emits
+ # ``nextval('version_transaction_id_seq')`` on every INSERT, so the
+ # sequence must exist before the table on Postgres. SQLite/MySQL
+ # ignore the explicit CREATE SEQUENCE (they auto-increment natively).
+ # ------------------------------------------------------------------
+ if bind.dialect.name == "postgresql":
+ op.execute("CREATE SEQUENCE IF NOT EXISTS version_transaction_id_seq")
+
+ op.create_table(
Review Comment:
**Suggestion:** Use the migration helper from
`superset.migrations.shared.utils` for table creation instead of calling
Alembic operations directly. [custom_rule]
**Severity Level:** Minor โ ๏ธ
<details>
<summary><b>Why it matters? ๐ค </b></summary>
The shared migration helpers include a create_table wrapper in
superset.migrations.shared.utils, so using bare op.create_table in a migration
is exactly the kind of raw operation this rule flags.
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=8f6aca01d82e4c51a8e270355e794ad1&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=8f6aca01d82e4c51a8e270355e794ad1&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent ๐ค </b></summary>
```mdx
This is a comment left during a code review.
**Path:**
superset/migrations/versions/2026-05-28_19-50_56cd24c07170_add_versioning_tables.py
**Line:** 104:104
**Comment:**
*Custom Rule: Use the migration helper from
`superset.migrations.shared.utils` for table creation instead of calling
Alembic operations directly.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41076&comment_hash=25163d79cf0ecfb53c481d6854e15ed368a0f6d3fb04410912b50b6a63d55e6f&reaction=like'>๐</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41076&comment_hash=25163d79cf0ecfb53c481d6854e15ed368a0f6d3fb04410912b50b6a63d55e6f&reaction=dislike'>๐</a>
##########
superset/migrations/versions/2026-05-01_23-36_2bee73611e32_composite_pk_association_tables.py:
##########
@@ -0,0 +1,580 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+"""composite_pk_association_tables
+
+Replace the unused synthetic ``id INTEGER PRIMARY KEY`` on eight many-to-many
+association tables with a composite primary key on the two FK columns. Drops
+the now-redundant ``UniqueConstraint(fk1, fk2)`` on the two tables that
+already carry one. Pre-flight: deletes rows with NULL FK values (six tables
+allow them today) and any duplicate ``(fk1, fk2)`` rows.
+
+Motivated by SQLAlchemy-Continuum issue #129 (M2M restore against junction
+tables with surrogate PKs); also closes the data-integrity hole where six
+of the eight tables lacked DB-level uniqueness.
+
+Revision ID: 2bee73611e32
+Revises: 78a40c08b4be
+Create Date: 2026-05-01 23:36:34.050058
+
+"""
+
+import logging
+from typing import NamedTuple
+
+import sqlalchemy as sa
+from alembic import op
+from alembic.operations.base import BatchOperations
+from sqlalchemy import inspect
+from sqlalchemy.engine import Connection
+
+# revision identifiers, used by Alembic.
+revision = "2bee73611e32"
+down_revision = "78a40c08b4be"
+
+logger = logging.getLogger("alembic.env")
+
+
+class AssociationTable(NamedTuple):
+ """A junction table being converted from surrogate-id PK to composite-FK
PK."""
+
+ name: str
+ fk1: str
+ fk2: str
+
+
+# Order is alphabetical by table name; deterministic for review and bisection.
+AFFECTED_TABLES: list[AssociationTable] = [
+ AssociationTable("dashboard_roles", "dashboard_id", "role_id"),
+ AssociationTable("dashboard_slices", "dashboard_id", "slice_id"),
+ AssociationTable("dashboard_user", "user_id", "dashboard_id"),
+ AssociationTable("report_schedule_user", "user_id", "report_schedule_id"),
+ AssociationTable("rls_filter_roles", "role_id", "rls_filter_id"),
+ AssociationTable("rls_filter_tables", "table_id", "rls_filter_id"),
+ AssociationTable("slice_user", "user_id", "slice_id"),
+ AssociationTable("sqlatable_user", "user_id", "table_id"),
+]
+
+# These two tables already declare ``UniqueConstraint(fk1, fk2)`` in the model;
+# the composite PK subsumes it, so the migration drops the redundant
constraint.
+TABLES_WITH_PRE_EXISTING_UNIQUE: set[str] = {
+ "dashboard_slices",
+ "report_schedule_user",
+}
+
+# Documentation set: tables whose FK columns are nullable in their original
+# create_table migrations (``dashboard_roles.dashboard_id`` from revision
+# e11ccdd12658 is the most recent addition). ``report_schedule_user`` is the
+# only affected table created with both FK columns ``NOT NULL`` and is
+# intentionally absent here. This set is no longer consulted at runtime โ the
+# upgrade now runs the NULL-FK cleanup on every affected table because the
+# DELETE is a cheap no-op when the columns are already NOT NULL, and that
+# eliminates the risk of bugs from this set going stale (the
+# ``dashboard_roles`` omission caught in PR review was exactly that bug).
+TABLES_WITH_NULLABLE_FKS: set[str] = {
+ "dashboard_roles",
+ "dashboard_slices",
+ "dashboard_user",
+ "rls_filter_roles",
+ "rls_filter_tables",
+ "slice_user",
+ "sqlatable_user",
+}
+
+
+def _check_no_external_fks_to_id(conn: Connection) -> None:
+ """Raise ``RuntimeError`` if any foreign key in the database references one
+ of the eight junction-table ``id`` columns. Uses SQLAlchemy's ``Inspector``
+ for dialect-agnostic introspection across PostgreSQL, MySQL, and SQLite.
+
+ Scope limitation: ``Inspector.get_table_names()`` returns tables in the
+ connection's default schema only. On PostgreSQL deployments where Superset
+ metadata lives in a non-default schema, or on multi-schema deployments
+ that allow cross-schema FKs, an external FK in another schema would not
+ be detected. This is acceptable for the standard single-schema
+ deployment that Superset documents; operators with multi-schema
+ metadata should run the equivalent inventory query against
+ ``information_schema.referential_constraints`` themselves before
+ applying.
+ """
+ affected = {t.name for t in AFFECTED_TABLES}
+ insp = inspect(conn)
+ for table_name in insp.get_table_names():
+ if table_name in affected:
+ continue
+ for fk in insp.get_foreign_keys(table_name):
+ if fk["referred_table"] in affected and "id" in
fk["referred_columns"]:
+ raise RuntimeError(
+ f"Cannot drop synthetic id from {fk['referred_table']}: "
+ f"external FK {fk.get('name', '<unnamed>')} on
{table_name} "
+ f"references
{fk['referred_table']}({fk['referred_columns']}). "
+ "Drop or migrate the referencing FK before applying this "
+ "migration."
+ )
+
+
+def _table_clause(t: AssociationTable) -> sa.sql.expression.TableClause:
+ """Build a lightweight SQLAlchemy ``TableClause`` for ``t`` exposing the
+ columns the helper queries reference (``id``, ``fk1``, ``fk2``). Used so
+ that the dedupe / cleanup / assert SQL can be expressed via SQLAlchemy
+ core constructs rather than via string interpolation."""
+ return sa.table(t.name, sa.column("id"), sa.column(t.fk1),
sa.column(t.fk2))
+
+
+def _delete_null_fk_rows(conn: Connection, t: AssociationTable) -> int:
+ """Delete rows where ``t.fk1`` or ``t.fk2`` is NULL on ``t.name``.
+
+ Returns the deletion count. Required because primary-key columns must be
+ NOT NULL; the PK-add downstream would fail with a cryptic constraint
+ violation if any NULL-FK rows survived. Run unconditionally on every
+ affected table โ see ``TABLES_WITH_NULLABLE_FKS`` above for the rationale.
+ """
+ tbl = _table_clause(t)
+ stmt = sa.delete(tbl).where(sa.or_(tbl.c[t.fk1].is_(None),
tbl.c[t.fk2].is_(None)))
+ result = conn.execute(stmt)
+ n = result.rowcount or 0
+ if n:
+ logger.warning(
+ "Deleted %d row(s) with NULL FK from %s before composite-PK
promotion",
+ n,
+ t.name,
+ )
+ return n
+
+
+def _dedupe_by_min_id(conn: Connection, t: AssociationTable) -> int:
+ """Delete duplicate ``(t.fk1, t.fk2)`` rows from ``t.name`` keeping
``MIN(id)``.
+
+ Returns the deletion count. The ``NOT IN`` argument is wrapped in an
+ extra ``SELECT keep_id FROM (...) AS s`` derived table because MySQL
+ rejects ``DELETE FROM t WHERE id NOT IN (SELECT MIN(id) FROM t GROUP BY
+ ...)`` with ERROR 1093 unless the inner SELECT is materialized through
+ a derived table. SQLAlchemy's ``.subquery()`` produces that wrap.
+
+ Logs a sample (up to 10) of the discarded ``(fk1, fk2, id)`` tuples at
+ WARN before deletion, so operators can audit which rows are dropped โ
+ the "keep ``MIN(id)``" policy preserves the original row, which is
+ correct in practice but discards any later, semantically-identical
+ re-grants.
+ """
+ tbl = _table_clause(t)
+
+ keep_min = (
+ sa.select(sa.func.min(tbl.c.id).label("keep_id"))
+ .group_by(tbl.c[t.fk1], tbl.c[t.fk2])
+ .subquery("keep_min")
+ )
+ keep_ids = sa.select(keep_min.c.keep_id)
+ discarded = tbl.c.id.notin_(keep_ids)
+
+ sample_stmt = (
+ sa.select(tbl.c[t.fk1], tbl.c[t.fk2],
tbl.c.id).where(discarded).limit(10)
+ )
+ sample = list(conn.execute(sample_stmt))
+
+ delete_stmt = sa.delete(tbl).where(discarded)
+ result = conn.execute(delete_stmt)
+ n = result.rowcount or 0
+ if n:
+ logger.warning(
+ "Deduped %d duplicate row(s) from %s; sample of discarded "
+ "(%s, %s, id) tuples (up to 10): %s",
+ n,
+ t.name,
+ t.fk1,
+ t.fk2,
+ sample,
+ )
+ return n
+
+
+def _assert_no_duplicates(conn: Connection, t: AssociationTable) -> None:
+ """Raise ``RuntimeError`` if any ``(t.fk1, t.fk2)`` duplicate group
remains.
+
+ Called after ``_dedupe_by_min_id`` to surface silent dialect-dependent
+ dedupe failures (e.g., a MySQL syntax issue) as an actionable error
+ before the PK-add fires with a less-helpful constraint-violation message.
+ """
+ tbl = _table_clause(t)
+ duplicate_groups = (
+ sa.select(sa.literal(1))
+ .select_from(tbl)
+ .group_by(tbl.c[t.fk1], tbl.c[t.fk2])
+ .having(sa.func.count() > 1)
+ .subquery("duplicate_groups")
+ )
+ count_stmt = sa.select(sa.func.count()).select_from(duplicate_groups)
+ if remaining := conn.scalar(count_stmt) or 0:
+ raise RuntimeError(
+ f"Dedupe failed for {t.name}: {remaining} duplicate "
+ f"({t.fk1}, {t.fk2}) groups remain after _dedupe_by_min_id. "
+ f"Check the dedupe SQL for dialect {conn.dialect.name}."
+ )
+
+
+def _build_pre_upgrade_table(
+ insp: sa.engine.reflection.Inspector,
+ t: AssociationTable,
+ fks: list[dict] | None = None,
+) -> sa.Table:
+ """Build a ``Table`` object representing the pre-upgrade schema of ``t``,
+ explicitly *without* any redundant ``UniqueConstraint(t.fk1, t.fk2)``.
+ Used as ``copy_from`` to ``batch_alter_table`` so the rebuilt table
+ omits the unnamed UNIQUE constraint deterministically across dialects
+ (SQLite reflects unnamed UNIQUEs with ``name=None``, defeating the
+ standard ``batch_op.drop_constraint(name)`` path).
+
+ Reflects column types and FK targets (with original FK constraint names
+ preserved) from the live database; only the redundant UNIQUE is omitted.
+
+ *fks* lets a caller pass a pre-captured ``get_foreign_keys`` result.
+ The MySQL upgrade path drops the live FK constraints before building
+ this table, so re-reflecting here would only see them via the
+ Inspector's per-instance ``info_cache`` โ an implementation detail,
+ not a contract. Passing the pre-drop list makes the dependency
+ explicit instead of relying on reflection caching.
+ """
+ md = sa.MetaData()
+ if fks is None:
+ fks = insp.get_foreign_keys(t.name)
+ fks_for_col: dict[str, list[dict]] = {}
+ for fk in fks:
+ for col_name in fk["constrained_columns"]:
+ fks_for_col.setdefault(col_name, []).append(fk)
+
+ cols: list[sa.Column] = []
+ for c in insp.get_columns(t.name):
+ col_kwargs = {"nullable": c.get("nullable", True)}
+ if c["name"] == "id":
+ col_kwargs["primary_key"] = True
+ col_kwargs["autoincrement"] = True
+ fk_args = []
+ for fk in fks_for_col.get(c["name"], []):
+ idx = fk["constrained_columns"].index(c["name"])
+ target = f"{fk['referred_table']}.{fk['referred_columns'][idx]}"
+ options = {}
+ if fk.get("options", {}).get("ondelete"):
+ options["ondelete"] = fk["options"]["ondelete"]
+ if fk.get("name"):
+ options["name"] = fk["name"]
+ fk_args.append(sa.ForeignKey(target, **options))
+ cols.append(sa.Column(c["name"], c["type"], *fk_args, **col_kwargs))
+ return sa.Table(t.name, md, *cols)
+
+
+def _drop_redundant_unique_by_name(
+ conn: Connection, insp: sa.engine.reflection.Inspector, t: AssociationTable
+) -> None:
+ """Drop the redundant ``UNIQUE(fk1, fk2)`` constraint by its reflected
+ name on PostgreSQL / MySQL.
+
+ The two tables in ``TABLES_WITH_PRE_EXISTING_UNIQUE`` carry a UNIQUE
+ constraint that the composite primary key subsumes. PostgreSQL and
+ MySQL both auto-name UNIQUE constraints (``<table>_<cols>_key`` on
+ Postgres, ``<table>_<col>_<n>`` or the explicit ``uq_*`` we may have
+ given it on MySQL), so they're reflectable by name. SQLite is
+ handled separately via ``recreate="always"`` + ``copy_from`` because
+ it reflects unnamed UNIQUEs with ``name=None``.
+
+ No-op if no matching UNIQUE is found (defensive โ re-runs after a
+ partial application should not error).
+ """
+ for uc in insp.get_unique_constraints(t.name):
+ if set(uc.get("column_names", [])) == {t.fk1, t.fk2} and
uc.get("name"):
+ op.drop_constraint(uc["name"], t.name, type_="unique")
+ return
+
+
+# MySQL ON DELETE actions that the downgrade re-create loop is allowed
+# to interpolate into raw SQL. The reflected value comes from MySQL's
+# information_schema (so not user input), but a whitelist eliminates
+# the "what if an unexpected value appears" question entirely. The
+# four entries are the SQL-standard set; SET DEFAULT is intentionally
+# excluded because InnoDB silently downgrades it to NO ACTION.
+_VALID_ONDELETE_ACTIONS: frozenset[str] = frozenset(
+ {"CASCADE", "SET NULL", "RESTRICT", "NO ACTION"}
+)
+
+
+def _enforce_not_null_for_sqlite(
+ batch_op: BatchOperations, t: AssociationTable, conn: Connection
+) -> None:
+ """Force ``NOT NULL`` on the FK columns post-PK-promotion on SQLite only.
+
+ SQLite has a long-standing quirk: composite ``PRIMARY KEY`` does not
+ promote constituent columns to ``NOT NULL`` (only ``INTEGER PRIMARY KEY``
+ does). PostgreSQL and MySQL implicitly promote the PK columns to
+ ``NOT NULL`` when the constraint is added, making the explicit
+ ``alter_column`` redundant there.
+
+ Skipping the ``alter_column`` on MySQL is also functionally required:
+ MySQL 8 rejects ``ALTER COLUMN`` on a column that participates in a
+ foreign key constraint with ``ERROR 1832 (HY000): Cannot change column
+ 'X': used in a foreign key constraint 'Y'`` whenever the table has
+ data โ even when the only change is ``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 but not on empty ones, which is why CI's
+ ``test-mysql`` shard (fresh schema) didn't catch this and a real
+ production-shaped install does.
+
+ Only SQLite still needs the explicit step, and SQLite has no FK
+ enforcement objection.
+ """
+ if conn.dialect.name == "sqlite":
+ batch_op.alter_column(t.fk1, existing_type=sa.Integer, nullable=False)
+ batch_op.alter_column(t.fk2, existing_type=sa.Integer, nullable=False)
+
+
+def upgrade() -> None:
+ conn = op.get_bind()
+ _check_no_external_fks_to_id(conn)
+ insp = inspect(conn)
+
+ for t in AFFECTED_TABLES:
+ # Resumability guard: on MySQL every DDL statement auto-commits, so
+ # a failure at table N of 8 leaves tables 1..N-1 already converted
+ # while ``alembic_version`` is still un-stamped. Without this guard
+ # a re-run would fail at table 1 (``drop_column("id")`` on a table
+ # that no longer has ``id``), and ``downgrade`` can't run either
+ # (the revision was never stamped) โ recovery would need manual
+ # surgery. A converted table is identified by the absent ``id``
+ # column; skipping it makes re-running the upgrade safe on every
+ # dialect (Postgres/SQLite wrap the migration in a transaction, so
+ # the guard is simply never hit there).
+ if "id" not in {c["name"] for c in insp.get_columns(t.name)}:
+ logger.info(
+ "%s: already converted (no surrogate id column); skipping",
+ t.name,
+ )
+ continue
+
+ # Run NULL-FK cleanup unconditionally: it is a no-op DELETE on tables
+ # whose FK columns are already NOT NULL (cheap), and skipping it on a
+ # table whose FK was nullable would leave the PK-add to fail with a
+ # cryptic constraint violation. Cf. ``TABLES_WITH_NULLABLE_FKS`` above
+ # for documentation of which tables are known to have nullable FKs.
+ _delete_null_fk_rows(conn, t)
+ _dedupe_by_min_id(conn, t)
+ _assert_no_duplicates(conn, t)
+
+ # Two tables (``dashboard_slices``, ``report_schedule_user``)
+ # carry a redundant ``UNIQUE(fk1, fk2)`` that the composite PK
+ # subsumes. Three dialect-specific paths:
+ #
+ # * **PostgreSQL** โ the UNIQUE constraint has a stable
+ # reflected name (Postgres default convention), so we
+ # ``DROP CONSTRAINT`` by name and then run the structural
+ # change as direct ALTER. This avoids the full-table copy
+ # that ``recreate="always"`` would trigger
+ # (``CREATE TABLE AS SELECT โ DROP โ RENAME``), holding
+ # ``ACCESS EXCLUSIVE`` only for the (much shorter) PK
+ # index build instead of the full copy duration.
+ #
+ # * **MySQL** โ InnoDB binds the FK constraints to the
+ # redundant UNIQUE's underlying index for back-reference,
+ # so a direct ``DROP CONSTRAINT`` of the UNIQUE raises
+ # ``ERROR 1553``. Use ``recreate="always"`` to rebuild the
+ # table without the UNIQUE; drop the FKs first to dodge
+ # the ``ERROR 1826`` (duplicate FK constraint name) that
+ # the temp-table phase would otherwise provoke. The FKs
+ # are re-created automatically as part of ``copy_from``.
+ #
+ # * **SQLite** โ unnamed UNIQUE constraints reflect with
+ # ``name=None`` and can't be dropped by name. Use
+ # ``recreate="always"`` + ``copy_from`` (omits UNIQUE).
+ # SQLite always rebuilds for PK changes anyway, so the
+ # recreate isn't extra cost there.
+ if t.name in TABLES_WITH_PRE_EXISTING_UNIQUE:
+ if conn.dialect.name == "postgresql":
+ _drop_redundant_unique_by_name(conn, insp, t)
+ with op.batch_alter_table(t.name) as batch_op:
+ batch_op.drop_column("id")
+ batch_op.create_primary_key(f"pk_{t.name}", [t.fk1, t.fk2])
+ _enforce_not_null_for_sqlite(batch_op, t, conn)
+ else:
+ # Capture the FK list BEFORE dropping: the copy_from table
+ # below must embed these constraints, and re-reflecting
+ # after the drop only works via the Inspector's
+ # per-instance info_cache (see _build_pre_upgrade_table).
+ pre_drop_fks = insp.get_foreign_keys(t.name)
+ if conn.dialect.name == "mysql":
+ for fk in pre_drop_fks:
+ if fk_name := fk.get("name"):
+ op.drop_constraint(fk_name, t.name,
type_="foreignkey")
Review Comment:
**Suggestion:** Use the foreign-key helper utilities from
`superset.migrations.shared.utils` instead of manually dropping constraints
with low-level Alembic operations. [custom_rule]
**Severity Level:** Minor โ ๏ธ
<details>
<summary><b>Why it matters? ๐ค </b></summary>
The migration shared utilities include `drop_fks_for_table`, which performs
dialect-aware FK dropping. This code manually drops foreign keys with Alembic
operations instead of using the shared helper, so it matches the custom
migration-utils rule.
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=a0bb4b1f296e48eb8d0d45be74789996&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=a0bb4b1f296e48eb8d0d45be74789996&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent ๐ค </b></summary>
```mdx
This is a comment left during a code review.
**Path:**
superset/migrations/versions/2026-05-01_23-36_2bee73611e32_composite_pk_association_tables.py
**Line:** 415:417
**Comment:**
*Custom Rule: Use the foreign-key helper utilities from
`superset.migrations.shared.utils` instead of manually dropping constraints
with low-level Alembic operations.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41076&comment_hash=737f9f7b664a26c8a75ba417d9591bffb3581a1d66ca1331ad03ee7cb560bb1e&reaction=like'>๐</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41076&comment_hash=737f9f7b664a26c8a75ba417d9591bffb3581a1d66ca1331ad03ee7cb560bb1e&reaction=dislike'>๐</a>
##########
superset/migrations/versions/2026-05-01_23-36_2bee73611e32_composite_pk_association_tables.py:
##########
@@ -0,0 +1,580 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+"""composite_pk_association_tables
+
+Replace the unused synthetic ``id INTEGER PRIMARY KEY`` on eight many-to-many
+association tables with a composite primary key on the two FK columns. Drops
+the now-redundant ``UniqueConstraint(fk1, fk2)`` on the two tables that
+already carry one. Pre-flight: deletes rows with NULL FK values (six tables
+allow them today) and any duplicate ``(fk1, fk2)`` rows.
+
+Motivated by SQLAlchemy-Continuum issue #129 (M2M restore against junction
+tables with surrogate PKs); also closes the data-integrity hole where six
+of the eight tables lacked DB-level uniqueness.
+
+Revision ID: 2bee73611e32
+Revises: 78a40c08b4be
+Create Date: 2026-05-01 23:36:34.050058
+
+"""
+
+import logging
+from typing import NamedTuple
+
+import sqlalchemy as sa
+from alembic import op
+from alembic.operations.base import BatchOperations
+from sqlalchemy import inspect
+from sqlalchemy.engine import Connection
+
+# revision identifiers, used by Alembic.
+revision = "2bee73611e32"
+down_revision = "78a40c08b4be"
+
+logger = logging.getLogger("alembic.env")
+
+
+class AssociationTable(NamedTuple):
+ """A junction table being converted from surrogate-id PK to composite-FK
PK."""
+
+ name: str
+ fk1: str
+ fk2: str
+
+
+# Order is alphabetical by table name; deterministic for review and bisection.
+AFFECTED_TABLES: list[AssociationTable] = [
+ AssociationTable("dashboard_roles", "dashboard_id", "role_id"),
+ AssociationTable("dashboard_slices", "dashboard_id", "slice_id"),
+ AssociationTable("dashboard_user", "user_id", "dashboard_id"),
+ AssociationTable("report_schedule_user", "user_id", "report_schedule_id"),
+ AssociationTable("rls_filter_roles", "role_id", "rls_filter_id"),
+ AssociationTable("rls_filter_tables", "table_id", "rls_filter_id"),
+ AssociationTable("slice_user", "user_id", "slice_id"),
+ AssociationTable("sqlatable_user", "user_id", "table_id"),
+]
+
+# These two tables already declare ``UniqueConstraint(fk1, fk2)`` in the model;
+# the composite PK subsumes it, so the migration drops the redundant
constraint.
+TABLES_WITH_PRE_EXISTING_UNIQUE: set[str] = {
+ "dashboard_slices",
+ "report_schedule_user",
+}
+
+# Documentation set: tables whose FK columns are nullable in their original
+# create_table migrations (``dashboard_roles.dashboard_id`` from revision
+# e11ccdd12658 is the most recent addition). ``report_schedule_user`` is the
+# only affected table created with both FK columns ``NOT NULL`` and is
+# intentionally absent here. This set is no longer consulted at runtime โ the
+# upgrade now runs the NULL-FK cleanup on every affected table because the
+# DELETE is a cheap no-op when the columns are already NOT NULL, and that
+# eliminates the risk of bugs from this set going stale (the
+# ``dashboard_roles`` omission caught in PR review was exactly that bug).
+TABLES_WITH_NULLABLE_FKS: set[str] = {
+ "dashboard_roles",
+ "dashboard_slices",
+ "dashboard_user",
+ "rls_filter_roles",
+ "rls_filter_tables",
+ "slice_user",
+ "sqlatable_user",
+}
+
+
+def _check_no_external_fks_to_id(conn: Connection) -> None:
+ """Raise ``RuntimeError`` if any foreign key in the database references one
+ of the eight junction-table ``id`` columns. Uses SQLAlchemy's ``Inspector``
+ for dialect-agnostic introspection across PostgreSQL, MySQL, and SQLite.
+
+ Scope limitation: ``Inspector.get_table_names()`` returns tables in the
+ connection's default schema only. On PostgreSQL deployments where Superset
+ metadata lives in a non-default schema, or on multi-schema deployments
+ that allow cross-schema FKs, an external FK in another schema would not
+ be detected. This is acceptable for the standard single-schema
+ deployment that Superset documents; operators with multi-schema
+ metadata should run the equivalent inventory query against
+ ``information_schema.referential_constraints`` themselves before
+ applying.
+ """
+ affected = {t.name for t in AFFECTED_TABLES}
+ insp = inspect(conn)
+ for table_name in insp.get_table_names():
+ if table_name in affected:
+ continue
+ for fk in insp.get_foreign_keys(table_name):
+ if fk["referred_table"] in affected and "id" in
fk["referred_columns"]:
+ raise RuntimeError(
+ f"Cannot drop synthetic id from {fk['referred_table']}: "
+ f"external FK {fk.get('name', '<unnamed>')} on
{table_name} "
+ f"references
{fk['referred_table']}({fk['referred_columns']}). "
+ "Drop or migrate the referencing FK before applying this "
+ "migration."
+ )
+
+
+def _table_clause(t: AssociationTable) -> sa.sql.expression.TableClause:
+ """Build a lightweight SQLAlchemy ``TableClause`` for ``t`` exposing the
+ columns the helper queries reference (``id``, ``fk1``, ``fk2``). Used so
+ that the dedupe / cleanup / assert SQL can be expressed via SQLAlchemy
+ core constructs rather than via string interpolation."""
+ return sa.table(t.name, sa.column("id"), sa.column(t.fk1),
sa.column(t.fk2))
+
+
+def _delete_null_fk_rows(conn: Connection, t: AssociationTable) -> int:
+ """Delete rows where ``t.fk1`` or ``t.fk2`` is NULL on ``t.name``.
+
+ Returns the deletion count. Required because primary-key columns must be
+ NOT NULL; the PK-add downstream would fail with a cryptic constraint
+ violation if any NULL-FK rows survived. Run unconditionally on every
+ affected table โ see ``TABLES_WITH_NULLABLE_FKS`` above for the rationale.
+ """
+ tbl = _table_clause(t)
+ stmt = sa.delete(tbl).where(sa.or_(tbl.c[t.fk1].is_(None),
tbl.c[t.fk2].is_(None)))
+ result = conn.execute(stmt)
+ n = result.rowcount or 0
+ if n:
+ logger.warning(
+ "Deleted %d row(s) with NULL FK from %s before composite-PK
promotion",
+ n,
+ t.name,
+ )
+ return n
+
+
+def _dedupe_by_min_id(conn: Connection, t: AssociationTable) -> int:
+ """Delete duplicate ``(t.fk1, t.fk2)`` rows from ``t.name`` keeping
``MIN(id)``.
+
+ Returns the deletion count. The ``NOT IN`` argument is wrapped in an
+ extra ``SELECT keep_id FROM (...) AS s`` derived table because MySQL
+ rejects ``DELETE FROM t WHERE id NOT IN (SELECT MIN(id) FROM t GROUP BY
+ ...)`` with ERROR 1093 unless the inner SELECT is materialized through
+ a derived table. SQLAlchemy's ``.subquery()`` produces that wrap.
+
+ Logs a sample (up to 10) of the discarded ``(fk1, fk2, id)`` tuples at
+ WARN before deletion, so operators can audit which rows are dropped โ
+ the "keep ``MIN(id)``" policy preserves the original row, which is
+ correct in practice but discards any later, semantically-identical
+ re-grants.
+ """
+ tbl = _table_clause(t)
+
+ keep_min = (
+ sa.select(sa.func.min(tbl.c.id).label("keep_id"))
+ .group_by(tbl.c[t.fk1], tbl.c[t.fk2])
+ .subquery("keep_min")
+ )
+ keep_ids = sa.select(keep_min.c.keep_id)
+ discarded = tbl.c.id.notin_(keep_ids)
+
+ sample_stmt = (
+ sa.select(tbl.c[t.fk1], tbl.c[t.fk2],
tbl.c.id).where(discarded).limit(10)
+ )
+ sample = list(conn.execute(sample_stmt))
+
+ delete_stmt = sa.delete(tbl).where(discarded)
+ result = conn.execute(delete_stmt)
+ n = result.rowcount or 0
+ if n:
+ logger.warning(
+ "Deduped %d duplicate row(s) from %s; sample of discarded "
+ "(%s, %s, id) tuples (up to 10): %s",
+ n,
+ t.name,
+ t.fk1,
+ t.fk2,
+ sample,
+ )
+ return n
+
+
+def _assert_no_duplicates(conn: Connection, t: AssociationTable) -> None:
+ """Raise ``RuntimeError`` if any ``(t.fk1, t.fk2)`` duplicate group
remains.
+
+ Called after ``_dedupe_by_min_id`` to surface silent dialect-dependent
+ dedupe failures (e.g., a MySQL syntax issue) as an actionable error
+ before the PK-add fires with a less-helpful constraint-violation message.
+ """
+ tbl = _table_clause(t)
+ duplicate_groups = (
+ sa.select(sa.literal(1))
+ .select_from(tbl)
+ .group_by(tbl.c[t.fk1], tbl.c[t.fk2])
+ .having(sa.func.count() > 1)
+ .subquery("duplicate_groups")
+ )
+ count_stmt = sa.select(sa.func.count()).select_from(duplicate_groups)
+ if remaining := conn.scalar(count_stmt) or 0:
+ raise RuntimeError(
+ f"Dedupe failed for {t.name}: {remaining} duplicate "
+ f"({t.fk1}, {t.fk2}) groups remain after _dedupe_by_min_id. "
+ f"Check the dedupe SQL for dialect {conn.dialect.name}."
+ )
+
+
+def _build_pre_upgrade_table(
+ insp: sa.engine.reflection.Inspector,
+ t: AssociationTable,
+ fks: list[dict] | None = None,
+) -> sa.Table:
+ """Build a ``Table`` object representing the pre-upgrade schema of ``t``,
+ explicitly *without* any redundant ``UniqueConstraint(t.fk1, t.fk2)``.
+ Used as ``copy_from`` to ``batch_alter_table`` so the rebuilt table
+ omits the unnamed UNIQUE constraint deterministically across dialects
+ (SQLite reflects unnamed UNIQUEs with ``name=None``, defeating the
+ standard ``batch_op.drop_constraint(name)`` path).
+
+ Reflects column types and FK targets (with original FK constraint names
+ preserved) from the live database; only the redundant UNIQUE is omitted.
+
+ *fks* lets a caller pass a pre-captured ``get_foreign_keys`` result.
+ The MySQL upgrade path drops the live FK constraints before building
+ this table, so re-reflecting here would only see them via the
+ Inspector's per-instance ``info_cache`` โ an implementation detail,
+ not a contract. Passing the pre-drop list makes the dependency
+ explicit instead of relying on reflection caching.
+ """
+ md = sa.MetaData()
+ if fks is None:
+ fks = insp.get_foreign_keys(t.name)
+ fks_for_col: dict[str, list[dict]] = {}
+ for fk in fks:
+ for col_name in fk["constrained_columns"]:
+ fks_for_col.setdefault(col_name, []).append(fk)
+
+ cols: list[sa.Column] = []
+ for c in insp.get_columns(t.name):
+ col_kwargs = {"nullable": c.get("nullable", True)}
+ if c["name"] == "id":
+ col_kwargs["primary_key"] = True
+ col_kwargs["autoincrement"] = True
+ fk_args = []
+ for fk in fks_for_col.get(c["name"], []):
+ idx = fk["constrained_columns"].index(c["name"])
+ target = f"{fk['referred_table']}.{fk['referred_columns'][idx]}"
+ options = {}
+ if fk.get("options", {}).get("ondelete"):
+ options["ondelete"] = fk["options"]["ondelete"]
+ if fk.get("name"):
+ options["name"] = fk["name"]
+ fk_args.append(sa.ForeignKey(target, **options))
+ cols.append(sa.Column(c["name"], c["type"], *fk_args, **col_kwargs))
+ return sa.Table(t.name, md, *cols)
+
+
+def _drop_redundant_unique_by_name(
+ conn: Connection, insp: sa.engine.reflection.Inspector, t: AssociationTable
+) -> None:
+ """Drop the redundant ``UNIQUE(fk1, fk2)`` constraint by its reflected
+ name on PostgreSQL / MySQL.
+
+ The two tables in ``TABLES_WITH_PRE_EXISTING_UNIQUE`` carry a UNIQUE
+ constraint that the composite primary key subsumes. PostgreSQL and
+ MySQL both auto-name UNIQUE constraints (``<table>_<cols>_key`` on
+ Postgres, ``<table>_<col>_<n>`` or the explicit ``uq_*`` we may have
+ given it on MySQL), so they're reflectable by name. SQLite is
+ handled separately via ``recreate="always"`` + ``copy_from`` because
+ it reflects unnamed UNIQUEs with ``name=None``.
+
+ No-op if no matching UNIQUE is found (defensive โ re-runs after a
+ partial application should not error).
+ """
+ for uc in insp.get_unique_constraints(t.name):
+ if set(uc.get("column_names", [])) == {t.fk1, t.fk2} and
uc.get("name"):
+ op.drop_constraint(uc["name"], t.name, type_="unique")
+ return
+
+
+# MySQL ON DELETE actions that the downgrade re-create loop is allowed
+# to interpolate into raw SQL. The reflected value comes from MySQL's
+# information_schema (so not user input), but a whitelist eliminates
+# the "what if an unexpected value appears" question entirely. The
+# four entries are the SQL-standard set; SET DEFAULT is intentionally
+# excluded because InnoDB silently downgrades it to NO ACTION.
+_VALID_ONDELETE_ACTIONS: frozenset[str] = frozenset(
+ {"CASCADE", "SET NULL", "RESTRICT", "NO ACTION"}
+)
+
+
+def _enforce_not_null_for_sqlite(
+ batch_op: BatchOperations, t: AssociationTable, conn: Connection
+) -> None:
+ """Force ``NOT NULL`` on the FK columns post-PK-promotion on SQLite only.
+
+ SQLite has a long-standing quirk: composite ``PRIMARY KEY`` does not
+ promote constituent columns to ``NOT NULL`` (only ``INTEGER PRIMARY KEY``
+ does). PostgreSQL and MySQL implicitly promote the PK columns to
+ ``NOT NULL`` when the constraint is added, making the explicit
+ ``alter_column`` redundant there.
+
+ Skipping the ``alter_column`` on MySQL is also functionally required:
+ MySQL 8 rejects ``ALTER COLUMN`` on a column that participates in a
+ foreign key constraint with ``ERROR 1832 (HY000): Cannot change column
+ 'X': used in a foreign key constraint 'Y'`` whenever the table has
+ data โ even when the only change is ``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 but not on empty ones, which is why CI's
+ ``test-mysql`` shard (fresh schema) didn't catch this and a real
+ production-shaped install does.
+
+ Only SQLite still needs the explicit step, and SQLite has no FK
+ enforcement objection.
+ """
+ if conn.dialect.name == "sqlite":
+ batch_op.alter_column(t.fk1, existing_type=sa.Integer, nullable=False)
+ batch_op.alter_column(t.fk2, existing_type=sa.Integer, nullable=False)
+
+
+def upgrade() -> None:
+ conn = op.get_bind()
+ _check_no_external_fks_to_id(conn)
+ insp = inspect(conn)
+
+ for t in AFFECTED_TABLES:
+ # Resumability guard: on MySQL every DDL statement auto-commits, so
+ # a failure at table N of 8 leaves tables 1..N-1 already converted
+ # while ``alembic_version`` is still un-stamped. Without this guard
+ # a re-run would fail at table 1 (``drop_column("id")`` on a table
+ # that no longer has ``id``), and ``downgrade`` can't run either
+ # (the revision was never stamped) โ recovery would need manual
+ # surgery. A converted table is identified by the absent ``id``
+ # column; skipping it makes re-running the upgrade safe on every
+ # dialect (Postgres/SQLite wrap the migration in a transaction, so
+ # the guard is simply never hit there).
+ if "id" not in {c["name"] for c in insp.get_columns(t.name)}:
+ logger.info(
+ "%s: already converted (no surrogate id column); skipping",
+ t.name,
+ )
+ continue
+
+ # Run NULL-FK cleanup unconditionally: it is a no-op DELETE on tables
+ # whose FK columns are already NOT NULL (cheap), and skipping it on a
+ # table whose FK was nullable would leave the PK-add to fail with a
+ # cryptic constraint violation. Cf. ``TABLES_WITH_NULLABLE_FKS`` above
+ # for documentation of which tables are known to have nullable FKs.
+ _delete_null_fk_rows(conn, t)
+ _dedupe_by_min_id(conn, t)
+ _assert_no_duplicates(conn, t)
+
+ # Two tables (``dashboard_slices``, ``report_schedule_user``)
+ # carry a redundant ``UNIQUE(fk1, fk2)`` that the composite PK
+ # subsumes. Three dialect-specific paths:
+ #
+ # * **PostgreSQL** โ the UNIQUE constraint has a stable
+ # reflected name (Postgres default convention), so we
+ # ``DROP CONSTRAINT`` by name and then run the structural
+ # change as direct ALTER. This avoids the full-table copy
+ # that ``recreate="always"`` would trigger
+ # (``CREATE TABLE AS SELECT โ DROP โ RENAME``), holding
+ # ``ACCESS EXCLUSIVE`` only for the (much shorter) PK
+ # index build instead of the full copy duration.
+ #
+ # * **MySQL** โ InnoDB binds the FK constraints to the
+ # redundant UNIQUE's underlying index for back-reference,
+ # so a direct ``DROP CONSTRAINT`` of the UNIQUE raises
+ # ``ERROR 1553``. Use ``recreate="always"`` to rebuild the
+ # table without the UNIQUE; drop the FKs first to dodge
+ # the ``ERROR 1826`` (duplicate FK constraint name) that
+ # the temp-table phase would otherwise provoke. The FKs
+ # are re-created automatically as part of ``copy_from``.
+ #
+ # * **SQLite** โ unnamed UNIQUE constraints reflect with
+ # ``name=None`` and can't be dropped by name. Use
+ # ``recreate="always"`` + ``copy_from`` (omits UNIQUE).
+ # SQLite always rebuilds for PK changes anyway, so the
+ # recreate isn't extra cost there.
+ if t.name in TABLES_WITH_PRE_EXISTING_UNIQUE:
+ if conn.dialect.name == "postgresql":
+ _drop_redundant_unique_by_name(conn, insp, t)
+ with op.batch_alter_table(t.name) as batch_op:
+ batch_op.drop_column("id")
+ batch_op.create_primary_key(f"pk_{t.name}", [t.fk1, t.fk2])
+ _enforce_not_null_for_sqlite(batch_op, t, conn)
+ else:
+ # Capture the FK list BEFORE dropping: the copy_from table
+ # below must embed these constraints, and re-reflecting
+ # after the drop only works via the Inspector's
+ # per-instance info_cache (see _build_pre_upgrade_table).
+ pre_drop_fks = insp.get_foreign_keys(t.name)
+ if conn.dialect.name == "mysql":
+ for fk in pre_drop_fks:
+ if fk_name := fk.get("name"):
+ op.drop_constraint(fk_name, t.name,
type_="foreignkey")
+ with op.batch_alter_table(
+ t.name,
+ recreate="always",
+ copy_from=_build_pre_upgrade_table(insp, t,
fks=pre_drop_fks),
+ ) as batch_op:
+ batch_op.drop_column("id")
+ batch_op.create_primary_key(f"pk_{t.name}", [t.fk1, t.fk2])
+ _enforce_not_null_for_sqlite(batch_op, t, conn)
+ else:
+ with op.batch_alter_table(t.name) as batch_op:
+ batch_op.drop_column("id")
+ batch_op.create_primary_key(f"pk_{t.name}", [t.fk1, t.fk2])
+ _enforce_not_null_for_sqlite(batch_op, t, conn)
+
+
+def downgrade() -> None:
+ # Inverse order: undo upgrade transformations from last-applied to
+ # first-applied. Within each table, drop the composite PK, restore the
+ # surrogate ``id`` column, and re-add the original ``UNIQUE`` constraint
+ # on the two tables that previously carried one.
+ #
+ # Note: FK columns remain NOT NULL after downgrade (intentional asymmetry
+ # โ see UPDATING.md). Restoring the original nullable state would require
+ # an explicit ``alter_column`` per FK per table for no operator value;
+ # junction-table NULL FKs were always meaningless under ``secondary=``
+ # semantics.
+ # The downgrade names the restored PK ``<table>_pkey`` (matching Postgres'
+ # default constraint-naming convention, which was the original constraint
+ # name before this migration ran) so a downgrade-then-upgrade round-trip
+ # doesn't collide on the upgrade's ``pk_<table>`` name.
+ #
+ # Adding a NOT NULL ``id`` column to a table with existing rows requires
+ # a default that fires on the existing rows. ``sa.Identity()`` (Postgres
+ # 10+ / MySQL 8+) and ``sa.Sequence`` (with explicit nextval) both
+ # backfill existing rows during ALTER TABLE; bare ``autoincrement=True``
+ # does not. ``Identity`` is the modern portable choice.
+ conn = op.get_bind()
+ insp = inspect(conn)
+ is_mysql = conn.dialect.name == "mysql"
+ for t in reversed(AFFECTED_TABLES):
+ if is_mysql:
+ _downgrade_mysql_table(insp, t)
+ else:
+ with op.batch_alter_table(t.name) as batch_op:
+ batch_op.drop_constraint(f"pk_{t.name}", type_="primary")
+ batch_op.add_column(
+ sa.Column(
+ "id",
+ sa.Integer,
+ sa.Identity(always=False),
+ nullable=False,
+ )
+ )
+ batch_op.create_primary_key(f"{t.name}_pkey", ["id"])
+ if t.name in TABLES_WITH_PRE_EXISTING_UNIQUE:
+ batch_op.create_unique_constraint(
+ f"uq_{t.name}_{t.fk1}_{t.fk2}", [t.fk1, t.fk2]
+ )
+
+
+def _downgrade_mysql_table(
+ insp: sa.engine.reflection.Inspector, t: AssociationTable
+) -> None:
+ """MySQL-specific downgrade for one table.
+
+ Two MySQL quirks force a dialect-specific path here:
+
+ 1. **ERROR 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. Dropping the PK before the FKs orphans
+ that backing index. PostgreSQL and SQLite create separate indexes
+ for FK columns and don't need this dance. We drop the FKs first
+ and re-add them after the structural change.
+
+ 2. **``Identity(always=False)`` on a non-PK column add does not emit
+ ``AUTO_INCREMENT`` on MySQL.** SQLAlchemy 1.4 only emits
+ ``AUTO_INCREMENT`` when the column has both ``Identity()`` and
+ ``primary_key=True`` at create time. Our portable path adds the
+ column first, then creates the PK separately โ which works on
+ Postgres (the column gets ``GENERATED BY DEFAULT AS IDENTITY``)
+ and SQLite (``INTEGER PRIMARY KEY`` becomes a rowid alias) but
+ leaves MySQL without auto-generation, so existing rows can't be
+ backfilled and future ``INSERT`` statements fail with
+ ``Field 'id' doesn't have a default value``. The combined
+ ``DROP PRIMARY KEY, ADD COLUMN AUTO_INCREMENT, ADD PRIMARY KEY``
+ in a single ALTER statement is the canonical MySQL idiom: MySQL
+ backfills existing rows with sequential values and the column
+ remains auto-incrementing for future inserts.
+
+ Raw SQL is unavoidable here โ there is no SQLAlchemy core equivalent
+ for the combined-ALTER form, and the constitution allows raw SQL for
+ dialect-specific DDL with no programmatic equivalent (preferring
+ triple-quoted strings for legibility).
+
+ Belt-and-braces guard: ``t.name`` is interpolated as a backtick-quoted
+ identifier in the ALTER statements below. The value comes from
+ ``AFFECTED_TABLES`` (a module-level literal), so SQL injection is
+ structurally precluded. The explicit ``allowed`` check here makes
+ that invariant load-bearing rather than implicit, so a future
+ refactor that loosens the call-site can't slip past review.
+ """
+ allowed = {a.name for a in AFFECTED_TABLES}
+ if t.name not in allowed:
+ raise RuntimeError(
+ f"Refusing to ALTER unknown table {t.name!r}: "
+ f"only AFFECTED_TABLES entries may flow through this path."
+ )
+
+ fks = insp.get_foreign_keys(t.name)
+
+ for fk in fks:
+ if fk_name := fk.get("name"):
+ op.execute(f"ALTER TABLE `{t.name}` DROP FOREIGN KEY `{fk_name}`")
Review Comment:
**Suggestion:** Replace raw SQL foreign-key drops with the shared migration
FK helper utilities to keep migration behavior aligned with
database-compatibility abstractions. [custom_rule]
**Severity Level:** Minor โ ๏ธ
<details>
<summary><b>Why it matters? ๐ค </b></summary>
The shared migration utilities provide dialect-aware foreign-key dropping
helpers, but this MySQL path uses raw SQL `ALTER TABLE ... DROP FOREIGN KEY`
instead. Because the helper exists, the code fits the rule that prefers shared
compatibility abstractions over manual raw DDL.
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=c2f5cdcc725f416dabc483daf682be1d&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=c2f5cdcc725f416dabc483daf682be1d&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent ๐ค </b></summary>
```mdx
This is a comment left during a code review.
**Path:**
superset/migrations/versions/2026-05-01_23-36_2bee73611e32_composite_pk_association_tables.py
**Line:** 528:530
**Comment:**
*Custom Rule: Replace raw SQL foreign-key drops with the shared
migration FK helper utilities to keep migration behavior aligned with
database-compatibility abstractions.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41076&comment_hash=f97404a9add27705395fa6bd52212d02bf3e071a95ca88f46ae81a057a20bb83&reaction=like'>๐</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41076&comment_hash=f97404a9add27705395fa6bd52212d02bf3e071a95ca88f46ae81a057a20bb83&reaction=dislike'>๐</a>
##########
superset/migrations/versions/2026-05-28_19-50_56cd24c07170_add_versioning_tables.py:
##########
@@ -0,0 +1,562 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+"""add_versioning_tables
+
+Creates the full schema backing entity versioning in a single
+migration:
+
+1. ``version_transaction`` โ audit log keyed by Continuum's per-flush
+ transaction id (plus a Postgres-specific id sequence).
+2. **Parent shadow tables** mirroring each versioned entity's columns:
+ ``dashboards_version`` / ``slices_version`` / ``tables_version``.
+3. ``version_changes`` โ field-level diff log keyed to a
+ ``(transaction, entity)`` pair; each row describes one atomic change
+ (one field or one child-collection element) that occurred during a
+ save.
+4. **Child shadow tables** for the collections Continuum auto-registers
+ when ``__versioned__`` is applied to ``TableColumn`` / ``SqlMetric``
+ and the ``slices`` exclude is removed from
+ ``Dashboard.__versioned__``: ``table_columns_version`` /
+ ``sql_metrics_version`` / ``dashboard_slices_version``.
+
+All shadow tables follow the validity-strategy shape (mirrored columns
++ ``transaction_id`` / ``end_transaction_id`` / ``operation_type``
+bookkeeping with FKs to ``version_transaction.id``). The current
+version row has ``end_transaction_id = NULL``.
+
+This migration replaces three iterative migrations from the spike phase
+(``56cd24c07170``, ``e1f3c5a7b9d0``, ``f7a2b3c4d5e6``) that captured the
+same schema in three steps as the feature was developed. Compacting
+gives downstream operators one migration to apply / reverse and one
+review surface. The ``revision`` hash is reused from the original first
+migration so anyone still tracking the chain by that hash lands on the
+same logical change set.
+
+Generated by hand because the current Continuum + Alembic-autogenerate
+interaction trips on the renamed ``transaction`` -> ``version_transaction``
+table key (``KeyError`` lookups in ``table_key_to_table``). Column
+inventories were sourced from the live model ``__table__`` definitions
+and ``version_class(...).__table__`` / Continuum association metadata.
+
+Primary key choice. Both ``version_transaction.id`` and
+``version_changes.id`` are ``BigInteger`` autoincrement โ a deliberate
+carveout from the project's UUID-PK convention for new models (see
+``CLAUDE.md`` ยง"UUID Migration"). ``version_transaction`` is keyed
+externally by SQLAlchemy-Continuum via
+``nextval('version_transaction_id_seq')`` on every INSERT; matching
+that contract is required for ``versioning_manager`` to function.
+``version_changes`` follows the same shape because the user-facing
+identity is the ``(transaction_id, entity_kind, entity_id, sequence)``
+composite unique key, not the row id; the API surfaces a deterministic
+UUIDv5 ``version_uuid`` derived from ``entity.uuid`` and
+``transaction_id`` for stable external references.
+
+Revision ID: 56cd24c07170
+Revises: 2bee73611e32
+Create Date: 2026-05-28 19:50:00.000000
+
+"""
+
+from __future__ import annotations
+
+import sqlalchemy as sa
+from alembic import op
+from sqlalchemy_utils import UUIDType
+
+from superset.utils.core import MediumText
+
+revision = "56cd24c07170"
+# Stacked on the composite-PK association-tables change (2bee73611e32) so the
+# Continuum shadow tables this migration creates can mirror the
+# composite-PK shape of the live association tables. If that change
+# is removed from the stack, this should be reverted to "ce6bd21901ab".
+down_revision = "2bee73611e32"
+
+
+def upgrade() -> None:
+ bind = op.get_bind()
+
+ # ------------------------------------------------------------------
+ # version_transaction
+ #
+ # Audit log for each versioning event. Continuum emits
+ # ``nextval('version_transaction_id_seq')`` on every INSERT, so the
+ # sequence must exist before the table on Postgres. SQLite/MySQL
+ # ignore the explicit CREATE SEQUENCE (they auto-increment natively).
+ # ------------------------------------------------------------------
+ if bind.dialect.name == "postgresql":
+ op.execute("CREATE SEQUENCE IF NOT EXISTS version_transaction_id_seq")
+
+ op.create_table(
+ "version_transaction",
+ sa.Column(
+ "id",
+ sa.BigInteger(),
+ sa.Sequence("version_transaction_id_seq"),
+ primary_key=True,
+ autoincrement=True,
+ nullable=False,
+ ),
+ sa.Column("issued_at", sa.DateTime(), nullable=True),
+ sa.Column("remote_addr", sa.String(50), nullable=True),
+ sa.Column("user_id", sa.Integer(), nullable=True),
+ # ``action_kind`` carries the high-level avenue that produced
+ # this transaction (``restore`` / ``import`` / ``clone``).
+ # ``NULL`` is the default "ordinary save" โ most rows leave
+ # this empty. Commands set
+ # ``session.info["_versioning_action_kind"]`` before commit;
+ # the change-record listener stamps the value here. Parallel
+ # to ``version_changes.entity_kind`` and ``version_changes.kind``
+ # โ the schema's third ``*_kind`` column, at transaction scope.
+ sa.Column("action_kind", sa.String(32), nullable=True),
+ )
+
+ if bind.dialect.name == "postgresql":
+ op.execute(
+ "ALTER SEQUENCE version_transaction_id_seq OWNED BY
version_transaction.id"
+ )
+
+ # ------------------------------------------------------------------
+ # dashboards_version
+ # ------------------------------------------------------------------
+ op.create_table(
+ "dashboards_version",
+ sa.Column("uuid", UUIDType(binary=True), nullable=True),
+ sa.Column("id", sa.Integer(), nullable=False),
+ sa.Column("dashboard_title", sa.String(500), nullable=True),
+ # ``MediumText()`` mirrors the live column type โ on MySQL plain
+ # ``TEXT`` caps at 64 KB, which large dashboards exceed; an
+ # oversized live write would then fail the shadow INSERT under
+ # ``STRICT_TRANS_TABLES`` (or silently truncate without it) and
+ # corrupt the history. Postgres ``TEXT`` is unbounded and SQLite
+ # ignores the length annotation so this is MySQL-driven.
+ sa.Column("position_json", MediumText(), nullable=True),
+ sa.Column("description", sa.Text(), nullable=True),
+ sa.Column("css", MediumText(), nullable=True),
+ sa.Column("theme_id", sa.Integer(), nullable=True),
+ sa.Column("certified_by", sa.Text(), nullable=True),
+ sa.Column("certification_details", sa.Text(), nullable=True),
+ sa.Column("json_metadata", MediumText(), nullable=True),
+ sa.Column("slug", sa.String(255), nullable=True),
+ sa.Column("published", sa.Boolean(), nullable=True),
+ sa.Column("is_managed_externally", sa.Boolean(), nullable=True),
+ sa.Column("external_url", sa.Text(), nullable=True),
+ sa.Column("transaction_id", sa.BigInteger(), nullable=False),
+ sa.Column("end_transaction_id", sa.BigInteger(), nullable=True),
+ sa.Column("operation_type", sa.SmallInteger(), nullable=False),
+ sa.PrimaryKeyConstraint("id", "transaction_id"),
+ sa.ForeignKeyConstraint(
+ ["transaction_id"],
+ ["version_transaction.id"],
+ name="fk_dashboards_version_transaction_id",
+ ),
+ sa.ForeignKeyConstraint(
+ ["end_transaction_id"],
+ ["version_transaction.id"],
+ name="fk_dashboards_version_end_transaction_id",
+ ),
+ )
+ op.create_index(
Review Comment:
**Suggestion:** Use the shared index helper from
`superset.migrations.shared.utils` instead of direct `op.create_index` calls
for compatibility-aware migrations. [custom_rule]
**Severity Level:** Minor โ ๏ธ
<details>
<summary><b>Why it matters? ๐ค </b></summary>
superset.migrations.shared.utils provides a create_index helper, so the
migration is using raw Alembic index creation instead of the
compatibility-aware utility required by the rule.
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=c1985729a2d24e81826ce4ec132b9829&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=c1985729a2d24e81826ce4ec132b9829&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent ๐ค </b></summary>
```mdx
This is a comment left during a code review.
**Path:**
superset/migrations/versions/2026-05-28_19-50_56cd24c07170_add_versioning_tables.py
**Line:** 173:173
**Comment:**
*Custom Rule: Use the shared index helper from
`superset.migrations.shared.utils` instead of direct `op.create_index` calls
for compatibility-aware migrations.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41076&comment_hash=9f1f98896f68324030809d1c6fb03189c5db5f3d83a37af06bb01f3eb121ac25&reaction=like'>๐</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41076&comment_hash=9f1f98896f68324030809d1c6fb03189c5db5f3d83a37af06bb01f3eb121ac25&reaction=dislike'>๐</a>
##########
superset/migrations/versions/2026-05-28_19-50_56cd24c07170_add_versioning_tables.py:
##########
@@ -0,0 +1,562 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+"""add_versioning_tables
+
+Creates the full schema backing entity versioning in a single
+migration:
+
+1. ``version_transaction`` โ audit log keyed by Continuum's per-flush
+ transaction id (plus a Postgres-specific id sequence).
+2. **Parent shadow tables** mirroring each versioned entity's columns:
+ ``dashboards_version`` / ``slices_version`` / ``tables_version``.
+3. ``version_changes`` โ field-level diff log keyed to a
+ ``(transaction, entity)`` pair; each row describes one atomic change
+ (one field or one child-collection element) that occurred during a
+ save.
+4. **Child shadow tables** for the collections Continuum auto-registers
+ when ``__versioned__`` is applied to ``TableColumn`` / ``SqlMetric``
+ and the ``slices`` exclude is removed from
+ ``Dashboard.__versioned__``: ``table_columns_version`` /
+ ``sql_metrics_version`` / ``dashboard_slices_version``.
+
+All shadow tables follow the validity-strategy shape (mirrored columns
++ ``transaction_id`` / ``end_transaction_id`` / ``operation_type``
+bookkeeping with FKs to ``version_transaction.id``). The current
+version row has ``end_transaction_id = NULL``.
+
+This migration replaces three iterative migrations from the spike phase
+(``56cd24c07170``, ``e1f3c5a7b9d0``, ``f7a2b3c4d5e6``) that captured the
+same schema in three steps as the feature was developed. Compacting
+gives downstream operators one migration to apply / reverse and one
+review surface. The ``revision`` hash is reused from the original first
+migration so anyone still tracking the chain by that hash lands on the
+same logical change set.
+
+Generated by hand because the current Continuum + Alembic-autogenerate
+interaction trips on the renamed ``transaction`` -> ``version_transaction``
+table key (``KeyError`` lookups in ``table_key_to_table``). Column
+inventories were sourced from the live model ``__table__`` definitions
+and ``version_class(...).__table__`` / Continuum association metadata.
+
+Primary key choice. Both ``version_transaction.id`` and
+``version_changes.id`` are ``BigInteger`` autoincrement โ a deliberate
+carveout from the project's UUID-PK convention for new models (see
+``CLAUDE.md`` ยง"UUID Migration"). ``version_transaction`` is keyed
+externally by SQLAlchemy-Continuum via
+``nextval('version_transaction_id_seq')`` on every INSERT; matching
+that contract is required for ``versioning_manager`` to function.
+``version_changes`` follows the same shape because the user-facing
+identity is the ``(transaction_id, entity_kind, entity_id, sequence)``
+composite unique key, not the row id; the API surfaces a deterministic
+UUIDv5 ``version_uuid`` derived from ``entity.uuid`` and
+``transaction_id`` for stable external references.
+
+Revision ID: 56cd24c07170
+Revises: 2bee73611e32
+Create Date: 2026-05-28 19:50:00.000000
+
+"""
+
+from __future__ import annotations
+
+import sqlalchemy as sa
+from alembic import op
+from sqlalchemy_utils import UUIDType
+
+from superset.utils.core import MediumText
+
+revision = "56cd24c07170"
+# Stacked on the composite-PK association-tables change (2bee73611e32) so the
+# Continuum shadow tables this migration creates can mirror the
+# composite-PK shape of the live association tables. If that change
+# is removed from the stack, this should be reverted to "ce6bd21901ab".
+down_revision = "2bee73611e32"
+
+
+def upgrade() -> None:
+ bind = op.get_bind()
+
+ # ------------------------------------------------------------------
+ # version_transaction
+ #
+ # Audit log for each versioning event. Continuum emits
+ # ``nextval('version_transaction_id_seq')`` on every INSERT, so the
+ # sequence must exist before the table on Postgres. SQLite/MySQL
+ # ignore the explicit CREATE SEQUENCE (they auto-increment natively).
+ # ------------------------------------------------------------------
+ if bind.dialect.name == "postgresql":
+ op.execute("CREATE SEQUENCE IF NOT EXISTS version_transaction_id_seq")
+
+ op.create_table(
+ "version_transaction",
+ sa.Column(
+ "id",
+ sa.BigInteger(),
+ sa.Sequence("version_transaction_id_seq"),
+ primary_key=True,
+ autoincrement=True,
+ nullable=False,
+ ),
+ sa.Column("issued_at", sa.DateTime(), nullable=True),
+ sa.Column("remote_addr", sa.String(50), nullable=True),
+ sa.Column("user_id", sa.Integer(), nullable=True),
+ # ``action_kind`` carries the high-level avenue that produced
+ # this transaction (``restore`` / ``import`` / ``clone``).
+ # ``NULL`` is the default "ordinary save" โ most rows leave
+ # this empty. Commands set
+ # ``session.info["_versioning_action_kind"]`` before commit;
+ # the change-record listener stamps the value here. Parallel
+ # to ``version_changes.entity_kind`` and ``version_changes.kind``
+ # โ the schema's third ``*_kind`` column, at transaction scope.
+ sa.Column("action_kind", sa.String(32), nullable=True),
+ )
+
+ if bind.dialect.name == "postgresql":
+ op.execute(
+ "ALTER SEQUENCE version_transaction_id_seq OWNED BY
version_transaction.id"
+ )
+
+ # ------------------------------------------------------------------
+ # dashboards_version
+ # ------------------------------------------------------------------
+ op.create_table(
+ "dashboards_version",
+ sa.Column("uuid", UUIDType(binary=True), nullable=True),
+ sa.Column("id", sa.Integer(), nullable=False),
+ sa.Column("dashboard_title", sa.String(500), nullable=True),
+ # ``MediumText()`` mirrors the live column type โ on MySQL plain
+ # ``TEXT`` caps at 64 KB, which large dashboards exceed; an
+ # oversized live write would then fail the shadow INSERT under
+ # ``STRICT_TRANS_TABLES`` (or silently truncate without it) and
+ # corrupt the history. Postgres ``TEXT`` is unbounded and SQLite
+ # ignores the length annotation so this is MySQL-driven.
+ sa.Column("position_json", MediumText(), nullable=True),
+ sa.Column("description", sa.Text(), nullable=True),
+ sa.Column("css", MediumText(), nullable=True),
+ sa.Column("theme_id", sa.Integer(), nullable=True),
+ sa.Column("certified_by", sa.Text(), nullable=True),
+ sa.Column("certification_details", sa.Text(), nullable=True),
+ sa.Column("json_metadata", MediumText(), nullable=True),
+ sa.Column("slug", sa.String(255), nullable=True),
+ sa.Column("published", sa.Boolean(), nullable=True),
+ sa.Column("is_managed_externally", sa.Boolean(), nullable=True),
+ sa.Column("external_url", sa.Text(), nullable=True),
+ sa.Column("transaction_id", sa.BigInteger(), nullable=False),
+ sa.Column("end_transaction_id", sa.BigInteger(), nullable=True),
+ sa.Column("operation_type", sa.SmallInteger(), nullable=False),
+ sa.PrimaryKeyConstraint("id", "transaction_id"),
+ sa.ForeignKeyConstraint(
+ ["transaction_id"],
+ ["version_transaction.id"],
+ name="fk_dashboards_version_transaction_id",
+ ),
+ sa.ForeignKeyConstraint(
+ ["end_transaction_id"],
+ ["version_transaction.id"],
+ name="fk_dashboards_version_end_transaction_id",
+ ),
+ )
+ op.create_index(
+ "ix_dashboards_version_end_transaction_id",
+ "dashboards_version",
+ ["end_transaction_id"],
+ )
+ op.create_index(
+ "ix_dashboards_version_operation_type",
+ "dashboards_version",
+ ["operation_type"],
+ )
+ op.create_index(
+ "ix_dashboards_version_transaction_id",
+ "dashboards_version",
+ ["transaction_id"],
+ )
+
+ # ------------------------------------------------------------------
+ # slices_version (Charts)
+ # ------------------------------------------------------------------
+ op.create_table(
+ "slices_version",
+ sa.Column("uuid", UUIDType(binary=True), nullable=True),
+ sa.Column("id", sa.Integer(), nullable=False),
+ sa.Column("slice_name", sa.String(250), nullable=True),
+ sa.Column("datasource_id", sa.Integer(), nullable=True),
+ sa.Column("datasource_type", sa.String(200), nullable=True),
+ sa.Column("datasource_name", sa.String(2000), nullable=True),
+ sa.Column("viz_type", sa.String(250), nullable=True),
+ sa.Column("params", MediumText(), nullable=True),
+ sa.Column("description", sa.Text(), nullable=True),
+ sa.Column("cache_timeout", sa.Integer(), nullable=True),
+ sa.Column("certified_by", sa.Text(), nullable=True),
+ sa.Column("certification_details", sa.Text(), nullable=True),
+ sa.Column("is_managed_externally", sa.Boolean(), nullable=True),
+ sa.Column("external_url", sa.Text(), nullable=True),
+ sa.Column("transaction_id", sa.BigInteger(), nullable=False),
+ sa.Column("end_transaction_id", sa.BigInteger(), nullable=True),
+ sa.Column("operation_type", sa.SmallInteger(), nullable=False),
+ sa.PrimaryKeyConstraint("id", "transaction_id"),
+ sa.ForeignKeyConstraint(
+ ["transaction_id"],
+ ["version_transaction.id"],
+ name="fk_slices_version_transaction_id",
+ ),
+ sa.ForeignKeyConstraint(
+ ["end_transaction_id"],
+ ["version_transaction.id"],
+ name="fk_slices_version_end_transaction_id",
+ ),
+ )
+ op.create_index(
+ "ix_slices_version_end_transaction_id",
+ "slices_version",
+ ["end_transaction_id"],
+ )
+ op.create_index(
+ "ix_slices_version_operation_type",
+ "slices_version",
+ ["operation_type"],
+ )
+ op.create_index(
+ "ix_slices_version_transaction_id",
+ "slices_version",
+ ["transaction_id"],
+ )
+
+ # ------------------------------------------------------------------
+ # tables_version (SqlaTable / Datasets)
+ # ------------------------------------------------------------------
+ op.create_table(
+ "tables_version",
+ sa.Column("uuid", UUIDType(binary=True), nullable=True),
+ sa.Column("id", sa.Integer(), nullable=False),
+ sa.Column("description", sa.Text(), nullable=True),
+ sa.Column("default_endpoint", sa.Text(), nullable=True),
+ sa.Column("is_featured", sa.Boolean(), nullable=True),
+ sa.Column("filter_select_enabled", sa.Boolean(), nullable=True),
+ sa.Column("offset", sa.Integer(), nullable=True),
+ sa.Column("cache_timeout", sa.Integer(), nullable=True),
+ sa.Column("params", sa.String(1000), nullable=True),
+ sa.Column("is_managed_externally", sa.Boolean(), nullable=True),
+ sa.Column("external_url", sa.Text(), nullable=True),
+ sa.Column("table_name", sa.String(250), nullable=True),
+ sa.Column("main_dttm_col", sa.String(250), nullable=True),
+ sa.Column("currency_code_column", sa.String(250), nullable=True),
+ sa.Column("database_id", sa.Integer(), nullable=True),
+ sa.Column("fetch_values_predicate", sa.Text(), nullable=True),
+ sa.Column("schema", sa.String(255), nullable=True),
+ sa.Column("catalog", sa.String(256), nullable=True),
+ sa.Column("sql", MediumText(), nullable=True),
+ sa.Column("is_sqllab_view", sa.Boolean(), nullable=True),
+ sa.Column("template_params", sa.Text(), nullable=True),
+ sa.Column("extra", sa.Text(), nullable=True),
+ sa.Column("normalize_columns", sa.Boolean(), nullable=True),
+ sa.Column("always_filter_main_dttm", sa.Boolean(), nullable=True),
+ sa.Column("folders", sa.JSON(), nullable=True),
+ sa.Column("transaction_id", sa.BigInteger(), nullable=False),
+ sa.Column("end_transaction_id", sa.BigInteger(), nullable=True),
+ sa.Column("operation_type", sa.SmallInteger(), nullable=False),
+ sa.PrimaryKeyConstraint("id", "transaction_id"),
+ sa.ForeignKeyConstraint(
+ ["transaction_id"],
+ ["version_transaction.id"],
+ name="fk_tables_version_transaction_id",
+ ),
+ sa.ForeignKeyConstraint(
+ ["end_transaction_id"],
+ ["version_transaction.id"],
+ name="fk_tables_version_end_transaction_id",
+ ),
+ )
+ op.create_index(
+ "ix_tables_version_end_transaction_id",
+ "tables_version",
+ ["end_transaction_id"],
+ )
+ op.create_index(
+ "ix_tables_version_operation_type",
+ "tables_version",
+ ["operation_type"],
+ )
+ op.create_index(
+ "ix_tables_version_transaction_id",
+ "tables_version",
+ ["transaction_id"],
+ )
+
+ # ------------------------------------------------------------------
+ # version_changes
+ #
+ # Field-level diff log keyed to a (transaction, entity) pair. Each
+ # row describes one atomic change (one field or one child-collection
+ # element) that occurred to one entity during a save.
+ #
+ # ``(entity_kind, entity_id)`` is a polymorphic reference: depending
+ # on ``entity_kind`` (``"chart"`` / ``"dashboard"`` / ``"dataset"``)
+ # the ``entity_id`` is the integer PK on ``slices`` / ``dashboards`` /
+ # ``tables`` respectively. SQL has no native polymorphic FK, so the
+ # constraint is intentionally omitted โ cleanup relies on the
+ # ``CASCADE`` from ``version_transaction.id`` plus command-layer
+ # ordering for entity deletes (the command that hard-deletes the
+ # entity runs inside the same transaction that prunes its history).
+ # A bare ``DELETE FROM <entity_table> WHERE id = X`` outside that
+ # transactional boundary leaves orphan ``version_changes`` rows
+ # whose ``entity_id`` references a vanished row โ the read-side
+ # tombstone-state lookup handles this gracefully.
+ # ------------------------------------------------------------------
+ op.create_table(
+ "version_changes",
+ sa.Column(
+ "id",
+ sa.BigInteger(),
+ primary_key=True,
+ autoincrement=True,
+ nullable=False,
+ ),
+ sa.Column(
+ "transaction_id",
+ sa.BigInteger(),
+ sa.ForeignKey("version_transaction.id", ondelete="CASCADE"),
+ nullable=False,
+ ),
+ sa.Column(
+ "entity_kind",
+ sa.String(length=32),
+ nullable=False,
+ ),
+ sa.Column(
+ "entity_id",
+ sa.Integer(),
+ nullable=False,
+ ),
+ sa.Column(
+ # Integer, not SmallInteger: per-entity sequence within one
+ # transaction is assigned by unbounded enumerate(); a
+ # pathological diff (e.g. a giant position_json rewrite) could
+ # overflow SmallInteger's 32767 on Postgres/MySQL.
+ "sequence",
+ sa.Integer(),
+ nullable=False,
+ ),
+ sa.Column(
+ "kind",
+ sa.String(length=32),
+ nullable=False,
+ ),
+ # ``operation`` is the per-record verb: ``add`` / ``remove`` /
+ # ``move`` / ``edit``. ``move`` only fires for layout records;
+ # the other three apply across every emit site. Made explicit
+ # so consumers don't have to infer the verb from ``from_value``
+ # / ``to_value`` null-tests or from ``path[0]`` for layout records.
+ sa.Column(
+ "operation",
+ sa.String(length=16),
+ nullable=False,
+ ),
+ sa.Column("path", sa.JSON(), nullable=False),
+ sa.Column("from_value", sa.JSON(), nullable=True),
+ sa.Column("to_value", sa.JSON(), nullable=True),
+ sa.UniqueConstraint(
+ "transaction_id",
+ "entity_kind",
+ "entity_id",
+ "sequence",
+ name="uq_version_changes_tx_entity_sequence",
+ ),
+ )
+ op.create_index(
+ "ix_version_changes_kind",
+ "version_changes",
+ ["kind"],
+ )
+ op.create_index(
+ "ix_version_changes_entity",
+ "version_changes",
+ ["entity_kind", "entity_id"],
+ )
+
+ # ------------------------------------------------------------------
+ # table_columns_version
+ # ------------------------------------------------------------------
+ op.create_table(
+ "table_columns_version",
+ sa.Column("uuid", UUIDType(binary=True), nullable=True),
+ sa.Column("id", sa.Integer(), nullable=False),
+ sa.Column("column_name", sa.String(255), nullable=True),
+ sa.Column("verbose_name", sa.String(1024), nullable=True),
+ sa.Column("is_active", sa.Boolean(), nullable=True),
+ sa.Column("type", sa.Text(), nullable=True),
+ sa.Column("advanced_data_type", sa.String(255), nullable=True),
+ sa.Column("groupby", sa.Boolean(), nullable=True),
+ sa.Column("filterable", sa.Boolean(), nullable=True),
+ sa.Column("description", MediumText(), nullable=True),
+ sa.Column("table_id", sa.Integer(), nullable=True),
+ sa.Column("is_dttm", sa.Boolean(), nullable=True),
+ sa.Column("expression", MediumText(), nullable=True),
+ sa.Column("python_date_format", sa.String(255), nullable=True),
+ sa.Column("datetime_format", sa.String(100), nullable=True),
+ sa.Column("extra", sa.Text(), nullable=True),
+ sa.Column("transaction_id", sa.BigInteger(), nullable=False),
+ sa.Column("end_transaction_id", sa.BigInteger(), nullable=True),
+ sa.Column("operation_type", sa.SmallInteger(), nullable=False),
+ sa.PrimaryKeyConstraint("id", "transaction_id"),
+ sa.ForeignKeyConstraint(
+ ["transaction_id"],
+ ["version_transaction.id"],
+ name="fk_table_columns_version_transaction_id",
+ ),
+ sa.ForeignKeyConstraint(
+ ["end_transaction_id"],
+ ["version_transaction.id"],
+ name="fk_table_columns_version_end_transaction_id",
+ ),
+ )
+ op.create_index(
+ "ix_table_columns_version_end_transaction_id",
+ "table_columns_version",
+ ["end_transaction_id"],
+ )
+ op.create_index(
+ "ix_table_columns_version_operation_type",
+ "table_columns_version",
+ ["operation_type"],
+ )
+ op.create_index(
+ "ix_table_columns_version_transaction_id",
+ "table_columns_version",
+ ["transaction_id"],
+ )
+
+ # ------------------------------------------------------------------
+ # sql_metrics_version
+ # ------------------------------------------------------------------
+ op.create_table(
+ "sql_metrics_version",
+ sa.Column("uuid", UUIDType(binary=True), nullable=True),
+ sa.Column("id", sa.Integer(), nullable=False),
+ sa.Column("metric_name", sa.String(255), nullable=True),
+ sa.Column("verbose_name", sa.String(1024), nullable=True),
+ sa.Column("metric_type", sa.String(32), nullable=True),
+ sa.Column("description", MediumText(), nullable=True),
+ sa.Column("d3format", sa.String(128), nullable=True),
+ sa.Column("currency", sa.JSON(), nullable=True),
+ sa.Column("warning_text", sa.Text(), nullable=True),
+ sa.Column("table_id", sa.Integer(), nullable=True),
+ sa.Column("expression", MediumText(), nullable=True),
+ sa.Column("extra", sa.Text(), nullable=True),
+ sa.Column("transaction_id", sa.BigInteger(), nullable=False),
+ sa.Column("end_transaction_id", sa.BigInteger(), nullable=True),
+ sa.Column("operation_type", sa.SmallInteger(), nullable=False),
+ sa.PrimaryKeyConstraint("id", "transaction_id"),
+ sa.ForeignKeyConstraint(
+ ["transaction_id"],
+ ["version_transaction.id"],
+ name="fk_sql_metrics_version_transaction_id",
+ ),
+ sa.ForeignKeyConstraint(
+ ["end_transaction_id"],
+ ["version_transaction.id"],
+ name="fk_sql_metrics_version_end_transaction_id",
+ ),
+ )
+ op.create_index(
+ "ix_sql_metrics_version_end_transaction_id",
+ "sql_metrics_version",
+ ["end_transaction_id"],
+ )
+ op.create_index(
+ "ix_sql_metrics_version_operation_type",
+ "sql_metrics_version",
+ ["operation_type"],
+ )
+ op.create_index(
+ "ix_sql_metrics_version_transaction_id",
+ "sql_metrics_version",
+ ["transaction_id"],
+ )
+
+ # ------------------------------------------------------------------
+ # dashboard_slices_version (M2M association)
+ #
+ # The live ``dashboard_slices`` table is reshaped to a
+ # composite PK on ``(dashboard_id, slice_id)`` โ no surrogate ``id``.
+ # Continuum auto-mirrors the live columns into the shadow Table at
+ # ``make_versioned()`` time, so the shadow's SQLAlchemy metadata
+ # also has no ``id``. The DB shadow PK is the natural composite key
+ # plus Continuum's bookkeeping (``transaction_id``, ``operation_type``);
+ # ``operation_type`` is included because a single transaction can in
+ # principle produce both INSERT and DELETE shadows for the same
+ # ``(dashboard_id, slice_id)`` pair (slice removed and re-added in
+ # one save).
+ #
+ # If that reshape is removed from the stack, the live table reverts to
+ # carrying its surrogate ``id`` and this migration would need to
+ # match.
+ # ------------------------------------------------------------------
+ op.create_table(
+ "dashboard_slices_version",
+ sa.Column("dashboard_id", sa.Integer(), nullable=False),
+ sa.Column("slice_id", sa.Integer(), nullable=False),
+ sa.Column("transaction_id", sa.BigInteger(), nullable=False),
+ sa.Column("end_transaction_id", sa.BigInteger(), nullable=True),
+ sa.Column("operation_type", sa.SmallInteger(), nullable=False),
+ sa.PrimaryKeyConstraint(
+ "dashboard_id", "slice_id", "transaction_id", "operation_type"
+ ),
+ sa.ForeignKeyConstraint(
+ ["transaction_id"],
+ ["version_transaction.id"],
+ name="fk_dashboard_slices_version_transaction_id",
+ ),
+ sa.ForeignKeyConstraint(
+ ["end_transaction_id"],
+ ["version_transaction.id"],
+ name="fk_dashboard_slices_version_end_transaction_id",
+ ),
+ )
+ op.create_index(
+ "ix_dashboard_slices_version_end_transaction_id",
+ "dashboard_slices_version",
+ ["end_transaction_id"],
+ )
+ op.create_index(
+ "ix_dashboard_slices_version_operation_type",
+ "dashboard_slices_version",
+ ["operation_type"],
+ )
+ op.create_index(
+ "ix_dashboard_slices_version_transaction_id",
+ "dashboard_slices_version",
+ ["transaction_id"],
+ )
+
+
+def downgrade() -> None:
+ # Drop in reverse dependency order: children with FKs to
+ # ``version_transaction`` drop first; ``version_transaction`` and its
+ # sequence drop last.
+ op.drop_table("dashboard_slices_version")
Review Comment:
**Suggestion:** Use the shared table-drop helper from
`superset.migrations.shared.utils` instead of direct `op.drop_table`
operations. [custom_rule]
**Severity Level:** Minor โ ๏ธ
<details>
<summary><b>Why it matters? ๐ค </b></summary>
The shared utilities include a drop_table helper, and this downgrade uses
direct op.drop_table calls instead of the database-safe wrapper, matching the
migration rule violation.
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=bc16b980bf1e4020a2680aae7cca9289&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=bc16b980bf1e4020a2680aae7cca9289&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent ๐ค </b></summary>
```mdx
This is a comment left during a code review.
**Path:**
superset/migrations/versions/2026-05-28_19-50_56cd24c07170_add_versioning_tables.py
**Line:** 551:551
**Comment:**
*Custom Rule: Use the shared table-drop helper from
`superset.migrations.shared.utils` instead of direct `op.drop_table` operations.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41076&comment_hash=876063d4a6275bc31ee40cf6d74fce0cf9006151c598d1e15e188cf639109a55&reaction=like'>๐</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41076&comment_hash=876063d4a6275bc31ee40cf6d74fce0cf9006151c598d1e15e188cf639109a55&reaction=dislike'>๐</a>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
