codeant-ai-for-open-source[bot] commented on code in PR #41075: URL: https://github.com/apache/superset/pull/41075#discussion_r3500180454
########## 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"), +] + Review Comment: **Suggestion:** The non-MySQL downgrade path also runs on SQLite but adds `id` using `sa.Identity`, which is not supported consistently on SQLite in this codebase and can fail or produce non-portable DDL/backfill behavior. Use a SQLite-safe pattern (table rebuild with `INTEGER PRIMARY KEY AUTOINCREMENT` semantics or dialect-conditional logic) instead of `Identity` on SQLite. [api mismatch] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx ❌ SQLite metadata deployments cannot safely downgrade this migration. ⚠️ Downgrade may leave association tables with broken ids. ``` </details> <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. In a deployment using SQLite for Superset metadata, Alembic upgrade moves the database to revision `2bee73611e32` by running `upgrade()` in `superset/migrations/versions/2026-05-01_23-36_2bee73611e32_composite_pk_association_tables.py:342-431`, converting the junction tables to composite primary keys. 2. Later, an operator or developer runs a downgrade (via the standard migration CLI wired through Flask-Migrate and `superset.extensions.migrate`), which invokes `downgrade()` in the same file at lines 34-76 to restore the surrogate `id` primary keys on those association tables. 3. On non-MySQL dialects (PostgreSQL and SQLite), the `downgrade()` loop takes the generic branch at lines 62-76, where for each affected table it calls `batch_op.add_column(sa.Column("id", sa.Integer, sa.Identity(always=False), nullable=False))` (lines 64-70) followed by `batch_op.create_primary_key(f"{t.name}_pkey", ["id"])` (line 72). 4. SQLAlchemy’s `Identity` construct is designed around dialects with native identity/autoincrement syntax (explicitly documented in the comments at lines 50-54 and 93-100), and using it for SQLite can emit non-portable or unsupported DDL during downgrade, leading to migration failures or a non-autoincrementing `id` column on SQLite deployments that attempt to downgrade this revision. ``` </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=701121beb7244be5a9b64e5842e70c59&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=701121beb7244be5a9b64e5842e70c59&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:** 64:70 **Comment:** *Api Mismatch: The non-MySQL downgrade path also runs on SQLite but adds `id` using `sa.Identity`, which is not supported consistently on SQLite in this codebase and can fail or produce non-portable DDL/backfill behavior. Use a SQLite-safe pattern (table rebuild with `INTEGER PRIMARY KEY AUTOINCREMENT` semantics or dialect-conditional logic) instead of `Identity` on SQLite. 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%2F41075&comment_hash=80b19085fa6de1294c999282edd7022279f49e894312648aeb43612297368904&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41075&comment_hash=80b19085fa6de1294c999282edd7022279f49e894312648aeb43612297368904&reaction=dislike'>👎</a> ########## superset/initialization/__init__.py: ########## @@ -619,6 +619,229 @@ def init_extensions(self) -> None: extension.manifest.id, ) + @staticmethod + def _remove_continuum_write_listeners() -> None: + """Detach SQLAlchemy-Continuum's own write listeners. + + ``make_versioned()`` runs unconditionally at import of + ``superset.extensions`` and registers Continuum's mapper, session, + and engine listeners — the ones that write shadow rows and + ``version_transaction`` rows on every flush. Skipping only the + custom baseline/change-record listeners would leave those running, + so with the kill-switch off the shadow tables would silently keep + accumulating, contradicting the documented contract. + + This is deliberately a *targeted subset* of + ``sqlalchemy_continuum.remove_versioning()``: that helper also + calls ``manager.reset()``, which clears ``version_class_map`` — + and ``version_class()`` would then silently return the live model + class, breaking the read-only ``/versions/`` endpoints this flag + promises to keep working. + + Idempotent: guarded on a representative listener so repeated app + initializations in one process (test fixtures) don't raise on + double-removal. + """ + # pylint: disable=import-outside-toplevel + import sqlalchemy as sa + from sqlalchemy_continuum import versioning_manager + + if not sa.event.contains( + sa.orm.Mapper, "after_insert", versioning_manager.track_inserts + ): + return # already detached by a prior init + versioning_manager.remove_operations_tracking(sa.orm.Mapper) + versioning_manager.remove_session_tracking(sa.orm.session.Session) + sa.event.remove( + sa.engine.Engine, + "before_execute", + versioning_manager.track_association_operations, + ) + sa.event.remove( + sa.engine.Engine, "rollback", versioning_manager.clear_connection + ) + sa.event.remove( + sa.engine.Engine, + "set_connection_execution_options", + versioning_manager.track_cloned_connections, + ) + + # Belt-and-suspenders: flip Continuum's master option off as well. + # Every write listener checks ``manager.options['versioning']`` before + # doing work (manager.py / unit_of_work.py), so if a future Continuum + # version registers an additional write listener this detach does not + # know to remove, that listener still no-ops. ``version_class()`` reads + # from ``version_class_map`` and ignores this option, so the read-only + # ``/versions/`` endpoints are unaffected. + versioning_manager.options["versioning"] = False + + # Verify the known write listeners are actually gone. A Continuum + # upgrade that renamed a handler would make the removals above silently + # miss, leaving capture half-on while we report "disabled"; surface + # that rather than booting in a contradictory state. + if sa.event.contains( + sa.orm.Mapper, "after_insert", versioning_manager.track_inserts + ): + logger.warning( + "versioning: Continuum write listeners still attached after " + "detach; capture may not be fully disabled. This usually means " + "the pinned sqlalchemy-continuum version changed how it " + "registers listeners." + ) + + def init_versioning(self) -> None: + """Register SQLAlchemy-Continuum baseline and retention listeners. + + Must be called after all versioned model classes have been imported so + that VERSIONED_MODELS can be populated and configure_mappers() has run. + + ``ENABLE_VERSIONING_CAPTURE`` (ships default ``False``) gates the two + before-flush listener registrations. The flag is operational, not + feature: with it off the infrastructure is inert (no save writes + shadow rows); flipping it on activates capture. The switch also lets + an operator who observes a versioning-induced regression (e.g. a + save-path slowdown attributable to the change-record listener) + disable capture in ``superset_config.py`` and restart workers — a + 30-second recovery instead of revert-and-redeploy. Shadow tables + already created by the migration stay; they just stop accumulating + new rows. + + The fallback here is ``False`` so that any app-factory path that + does not load ``superset.config`` (some test factories, embedded + use) stays inert by default rather than silently enabling capture. + """ + # Beat-schedule check first: the retention task is independent of + # save-path capture and remains useful for ageing-out rows already + # written by prior deploys. An operator hitting the kill-switch in + # anger may also be running a hand-rolled ``CeleryConfig`` that + # silently dropped the prune entry; surfacing both misconfigurations + # at the same restart is the cheap, observability-positive shape. + self._warn_if_retention_beat_missing() + + if not self.config.get("ENABLE_VERSIONING_CAPTURE", False): + logger.warning( + "versioning: ENABLE_VERSIONING_CAPTURE is False; " + "skipping baseline + change-record listener registration " + "and detaching Continuum's write listeners. Save-path " + "capture is disabled; existing shadow tables and " + "/versions/ endpoints continue to work read-only." + ) + self._remove_continuum_write_listeners() + return + + # Symmetric with the OFF branch's ``options['versioning'] = False``: + # re-assert it on here so capture is restored even if a prior app + # init in the same process (multi-app / test reentrancy) flipped the + # process-global Continuum option off. Without this, an OFF app + # initialized before an ON app would leave the option False and the + # baseline listener — which gates on it — would silently write no + # baselines despite capture being "enabled". + from sqlalchemy_continuum import versioning_manager + + versioning_manager.options["versioning"] = True Review Comment: **Suggestion:** The ON path only flips `versioning_manager.options["versioning"]` back to `True`, but it never re-registers Continuum write listeners that were removed by the OFF path. After an OFF init in the same process, turning capture back ON will still not write Continuum shadow rows/transactions, so capture remains partially or fully broken. Re-attach Continuum tracking listeners (or rerun the proper Continuum setup) when re-enabling. [incomplete implementation] <details> <summary><b>Severity Level:</b> Critical 🚨</summary> ```mdx ❌ Re-enabling versioning capture still writes no shadow rows. ❌ /versions endpoints and history views silently stop updating. ⚠️ Multi-app deployments with mixed flags see inconsistent behavior. ``` </details> <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. First Superset app is created in a process via `create_app()` (`superset/app.py:80-12`), with configuration setting `ENABLE_VERSIONING_CAPTURE` to False; `SupersetAppInitializer.init_app()` (`superset/initialization/__init__.py:1080-1159`) enters the OFF branch of `init_versioning()` at lines 162-171. 2. That OFF branch logs a warning and calls `_remove_continuum_write_listeners()` (`superset/initialization/__init__.py:643-132`), which detaches Continuum’s mapper, session, and engine listeners and sets `versioning_manager.options["versioning"] = False` at line 117, globally affecting the process. 3. In the same process, a second Superset app is created with `ENABLE_VERSIONING_CAPTURE = True` (the integration test configuration in `tests/integration_tests/superset_test_config.py:98` shows capture-on is a supported mode); its initializer again calls `init_versioning()` (`superset/initialization/__init__.py:133-239`), but now the ON branch at lines 173-183 only executes `from sqlalchemy_continuum import versioning_manager` and `versioning_manager.options["versioning"] = True` (lines 739-741) without re-attaching the Continuum write listeners previously removed. 4. Subsequent versioned operations such as chart updates via `/api/v1/chart/{id}` (capturing version info in `superset/charts/api.py:498-502` and tested behavior in `tests/integration_tests/versioning/conftest.py:80-86`) and the baseline listener in `superset/versioning/baseline/listener.py:15-26` run with `options["versioning"]` True but with Continuum’s write listeners still detached, so no shadow rows or `version_transaction` records are written even though capture is nominally enabled, breaking the versioning feature until the process is restarted. ``` </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=788a18fa3dd844879257262af3fffc3e&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=788a18fa3dd844879257262af3fffc3e&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/initialization/__init__.py **Line:** 739:741 **Comment:** *Incomplete Implementation: The ON path only flips `versioning_manager.options["versioning"]` back to `True`, but it never re-registers Continuum write listeners that were removed by the OFF path. After an OFF init in the same process, turning capture back ON will still not write Continuum shadow rows/transactions, so capture remains partially or fully broken. Re-attach Continuum tracking listeners (or rerun the proper Continuum setup) when re-enabling. 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%2F41075&comment_hash=65b84dcb5de27d87e71efabd4e86af06877878751485536e47417e5fec520740&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41075&comment_hash=65b84dcb5de27d87e71efabd4e86af06877878751485536e47417e5fec520740&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]
