codeant-ai-for-open-source[bot] commented on code in PR #41176:
URL: https://github.com/apache/superset/pull/41176#discussion_r3538023043
##########
superset/dashboards/api.py:
##########
@@ -850,17 +908,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:** `pk` comes from a route declared as `"/<pk>"` (string path
segment), but this new pre-update lookup passes it directly into version
queries before the command-level error handling runs. When versioning capture
is enabled, non-numeric IDs can trigger a DB type-cast error here and return a
500 instead of the expected handled 4xx/422 path. Convert `pk` to int (or use
an `<int:pk>` route) before calling this helper. [api mismatch]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Dashboard update endpoint may 500 on malformed IDs.
- ⚠️ Versioning capture pre-check bypasses normal error mapping.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Enable versioning capture by setting `ENABLE_VERSIONING_CAPTURE=True` in
the Superset
config, so `current_entity_version_info()` actually queries the database (see
`_capture_enabled` in `superset/versioning/api_helpers.py:71-72` and its use
in
`current_entity_version_info` at lines 75-101).
2. Start Superset and issue an authenticated HTTP `PUT` request to the
dashboard update
endpoint with a non-numeric path segment, for example: `PUT
/api/v1/dashboard/foo` with a
valid JSON body, which is routed to `DashboardRestApi.put` defined in
`superset/dashboards/api.py:811-953` and exposed as `@expose("/<pk>",
methods=("PUT",))`
at line 811.
3. Observe that Flask/FAB passes the path segment `"foo"` as the `pk`
argument (the route
uses `<pk>` with no `<int:...>` converter), and before any `try/except` block
`DashboardRestApi.put` invokes `old_info =
current_entity_version_info(Dashboard, pk)` at
`superset/dashboards/api.py:914`.
4. Inside `current_entity_version_info` in
`superset/versioning/api_helpers.py:75-101`,
the code calls
`db.session.scalar(sa.select(model_cls.uuid).where(model_cls.id ==
entity_id))`, so `Dashboard.id == "foo"` is sent to the database as an
integer-column
comparison with a string value; this can raise a database
`DataError`/type-cast error that
is not caught by any surrounding `try/except` in `DashboardRestApi.put`,
resulting in a
500 response instead of the usual mapped 4xx/422 errors from
`UpdateDashboardCommand`.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=b1542aae01a74cb6addcfffed3854d84&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=b1542aae01a74cb6addcfffed3854d84&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:** 914:914
**Comment:**
*Api Mismatch: `pk` comes from a route declared as `"/<pk>"` (string
path segment), but this new pre-update lookup passes it directly into version
queries before the command-level error handling runs. When versioning capture
is enabled, non-numeric IDs can trigger a DB type-cast error here and return a
500 instead of the expected handled 4xx/422 path. Convert `pk` to int (or use
an `<int:pk>` route) before calling this helper.
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%2F41176&comment_hash=23a67abacaec3650b19de7ebc66b4519569df80712d82178ad769f7b1725df08&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41176&comment_hash=23a67abacaec3650b19de7ebc66b4519569df80712d82178ad769f7b1725df08&reaction=dislike'>👎</a>
##########
superset/datasets/api.py:
##########
@@ -464,17 +527,69 @@ 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(SqlaTable, pk)
Review Comment:
**Suggestion:** This pre-update version lookup uses `pk` before any command
wrapper/exception mapping, but the route is `"/<pk>"` (string) rather than an
int-converted path. With capture enabled, a non-numeric ID can fail at the SQL
layer here and bypass normal error handling, producing a 500. Cast/validate
`pk` first or change the route converter to `<int:pk>`. [api mismatch]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Dataset update endpoint may 500 on malformed IDs.
- ⚠️ Versioning pre-lookup bypasses dataset error handling.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Enable versioning capture by setting `ENABLE_VERSIONING_CAPTURE=True` so
version
lookups are active (see `_capture_enabled` and `current_entity_version_info`
in
`superset/versioning/api_helpers.py:71-101`).
2. Start Superset and send an authenticated `PUT` request to the dataset
update endpoint
with a non-numeric primary key, for example `PUT /api/v1/dataset/foo` with a
valid JSON
body; this is wired to `DatasetRestApi.put` at
`superset/datasets/api.py:417-601`, which
is exposed as `@expose("/<pk>", methods=("PUT",))` at line 417 (string route
segment).
3. In `DatasetRestApi.put`, before entering the `try:` block that wraps
`UpdateDatasetCommand`, the code executes `old_info =
current_entity_version_info(SqlaTable, pk)` at
`superset/datasets/api.py:533`, passing the
string `"foo"` as `entity_id`.
4. Within `current_entity_version_info`
(`superset/versioning/api_helpers.py:75-101`), the
helper issues `sa.select(model_cls.uuid).where(model_cls.id == entity_id)`;
comparing
integer `SqlaTable.id` to the string `"foo"` can cause a database type-cast
error
(`DataError`), and because this call happens before the `try/except` around
`UpdateDatasetCommand` (lines 535-600), the exception is not translated into
`DatasetNotFoundError`/`DatasetInvalidError` responses and surfaces as a 500.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=965d462433bf4cf6a181439ced48d96d&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=965d462433bf4cf6a181439ced48d96d&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/datasets/api.py
**Line:** 533:533
**Comment:**
*Api Mismatch: This pre-update version lookup uses `pk` before any
command wrapper/exception mapping, but the route is `"/<pk>"` (string) rather
than an int-converted path. With capture enabled, a non-numeric ID can fail at
the SQL layer here and bypass normal error handling, producing a 500.
Cast/validate `pk` first or change the route converter to `<int:pk>`.
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%2F41176&comment_hash=18b6392c049f9827938bcc27c0a4778f498567379e1c241ff9c288961011aed3&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41176&comment_hash=18b6392c049f9827938bcc27c0a4778f498567379e1c241ff9c288961011aed3&reaction=dislike'>👎</a>
##########
superset/initialization/__init__.py:
##########
@@ -626,6 +626,181 @@ 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
Review Comment:
**Suggestion:** The early return in the detach helper exits before forcing
versioning_manager.options["versioning"] to False, so a process that already
has detached listeners can still keep the master switch True from a prior
enabled init. In that state, previously registered baseline listeners can
continue writing rows even though capture is configured off. Move the
option-off assignment before this return (or remove the return path) so OFF
always enforces the kill-switch. [incorrect condition logic]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
❌ Kill-switch may fail to disable baseline capture fully.
⚠️ Operators cannot reliably stop new version baselines in-process.
⚠️ Confusing divergence between config flag and actual capture.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Inspect `_remove_continuum_write_listeners()` in
`superset/initialization/__init__.py:629-84`. At lines 656-659 it checks
`sa.event.contains(sa.orm.Mapper, "after_insert",
versioning_manager.track_inserts)` and
returns immediately when the mapper listener is already detached, skipping
the later
`versioning_manager.options["versioning"] = False` assignment at line 684.
2. Start a first Superset app with a configuration where
`ENABLE_VERSIONING_CAPTURE=False`. During initialization, `init_versioning()`
(`superset/initialization/__init__.py:699-130`) takes the OFF branch, calls
`_remove_continuum_write_listeners()`, sees the Continuum mapper listener
present, removes
all Continuum write listeners, and sets
`versioning_manager.options["versioning"] =
False`.
3. In the same process, create a second app with
`ENABLE_VERSIONING_CAPTURE=True`.
`init_versioning()`’s ON branch
(`superset/initialization/__init__.py:738-141`) sets
`versioning_manager.options["versioning"] = True` and registers the baseline
and
change-record listeners (`register_baseline_listener` in
`superset/versioning/baseline/listener.py:76-131` and
`register_change_record_listener` in
`superset/versioning/changes/listener.py:122-248`) on the shared
`db.session`.
4. Create a third app in the same process with
`ENABLE_VERSIONING_CAPTURE=False`. This
time `_remove_continuum_write_listeners()` runs with the Continuum mapper
listener already
gone (detached by the first OFF app), so the `sa.event.contains(...)` check
at lines
656-658 returns False and the function exits at line 659 without setting
`versioning_manager.options["versioning"]` back to False. The baseline and
change-record
listeners registered in step 3 remain attached and gate only on
`versioning_manager.options["versioning"]`, so baseline capture continues to
run (see the
guard at `superset/versioning/baseline/listener.py:104-105`) even though
this third app’s
configuration has `ENABLE_VERSIONING_CAPTURE=False`, meaning the kill-switch
does not
fully disable capture in this multi-app sequence.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=49f1d808881d46988f582d6aa059ac60&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=49f1d808881d46988f582d6aa059ac60&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:** 656:659
**Comment:**
*Incorrect Condition Logic: The early return in the detach helper exits
before forcing versioning_manager.options["versioning"] to False, so a process
that already has detached listeners can still keep the master switch True from
a prior enabled init. In that state, previously registered baseline listeners
can continue writing rows even though capture is configured off. Move the
option-off assignment before this return (or remove the return path) so OFF
always enforces the kill-switch.
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%2F41176&comment_hash=7d23643ffb10f5247b7f92dd9fa3e1ad10f9f817f0de15b0e28aa604c5e0a80d&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41176&comment_hash=7d23643ffb10f5247b7f92dd9fa3e1ad10f9f817f0de15b0e28aa604c5e0a80d&reaction=dislike'>👎</a>
##########
superset/initialization/__init__.py:
##########
@@ -626,6 +626,181 @@ 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.
+ """
+ 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
+
+ from sqlalchemy.orm import Session # noqa: F401
+ from sqlalchemy_continuum import version_class
+
+ from superset.connectors.sqla.models import SqlaTable
+ from superset.models.dashboard import Dashboard
+ from superset.models.slice import Slice
+ from superset.versioning.baseline import (
+ register_baseline_listener,
+ VERSIONED_MODELS,
+ )
+
+ # Note: previously this block called ``configure_mappers()`` before
+ # importing the snapshot modules, believing their Table declarations
+ # needed ``version_transaction`` to exist. That's not actually the
+ # case — the snapshot tables reference ``version_transaction.id``
+ # only at the DB level (via the migration); the SQLAlchemy Table
+ # objects here intentionally declare ``transaction_id`` as a plain
+ # ``BigInteger`` without a FK to avoid the resolution dependency.
+ # Removing the global ``configure_mappers()`` avoids eagerly
+ # resolving relationships in other unrelated models (notably
+ # Flask-AppBuilder's AuditMixin on classes like Tag, whose
+ # ``created_by`` primaryjoin only resolves under specific class
+ # registry states in SQLAlchemy 1.4).
+ from superset.versioning.changes import ( # noqa: E402
+ register_change_record_listener,
+ )
+
+ # All versioned models — Dashboard / Slice / SqlaTable plus their
+ # children (TableColumn / SqlMetric) and the dashboard_slices
+ # M2M — go through Continuum's shadow tables. The JSON-snapshot
+ # path that previously backed dataset / dashboard child diffs
+ # has been removed (full-Continuum spike).
+ for model_cls in (Dashboard, Slice, SqlaTable):
+ try:
+ version_class(model_cls) # ensure Continuum wired this model
+ # Dedup guard: VERSIONED_MODELS is module-level state, and
+ # test fixtures initialize multiple Superset apps per
+ # process — without the check each re-init appends
+ # duplicate entries.
+ if model_cls not in VERSIONED_MODELS:
+ VERSIONED_MODELS.append(model_cls)
+ except Exception: # pylint: disable=broad-except
+ # Continuum failed to wire versioning for this model. We
+ # boot in degraded mode rather than failing startup, but a
+ # silent skip would hide that change capture has stopped for
+ # the model — so surface it at WARNING with the traceback.
+ logger.warning(
+ "Versioning is not wired for %s; change capture will be "
+ "skipped for it. This usually means Continuum did not "
+ "register a version class for the model.",
+ model_cls.__name__,
+ exc_info=True,
+ )
+
+ register_baseline_listener()
+ register_change_record_listener()
Review Comment:
**Suggestion:** The ON path does not restore SQLAlchemy-Continuum write
listeners after the OFF path detaches them, so toggling capture back to enabled
in the same process only flips a flag and registers custom listeners but still
leaves core Continuum writes disabled. This causes partial/empty version
capture despite ENABLE_VERSIONING_CAPTURE=True. Re-attach Continuum listeners
(or avoid removing them in OFF mode and rely on the versioning option gate)
before enabling capture. [incomplete implementation]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
❌ Multi-app deployments silently lose version_changes capture when enabled.
⚠️ /versions/ history incomplete despite ENABLE_VERSIONING_CAPTURE=True
configuration.
⚠️ Baseline-only capture breaks expected versioning semantics for entities.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. In `create_app` (`superset/app.py:52-88`), note it instantiates
`SupersetAppInitializer` and calls `init_app()`, which in turn calls
`init_app_in_ctx()`
and then `init_versioning()` (`superset/initialization/__init__.py:804-233`)
inside the
app context.
2. Start one Superset app in a process with a config module setting
`ENABLE_VERSIONING_CAPTURE = False`. During initialization
`init_versioning()`
(`superset/initialization/__init__.py:699-130`) takes the OFF branch, logs
the warning,
and calls `_remove_continuum_write_listeners()`
(`superset/initialization/__init__.py:629-84`), which removes Continuum's
mapper/session/engine listeners and sets
`versioning_manager.options["versioning"] =
False`.
3. In the same Python process, create a second Superset app with a
configuration where
`ENABLE_VERSIONING_CAPTURE = True`. In this app, `init_versioning()`
executes the ON
branch: the code at `superset/initialization/__init__.py:738-141` imports
`versioning_manager` and sets `versioning_manager.options["versioning"] =
True`, then
registers baseline and change-record listeners via
`register_baseline_listener()` and
`register_change_record_listener()`
(`superset/initialization/__init__.py:796-198`), but
never re-attaches the Continuum write listeners that
`_remove_continuum_write_listeners()`
previously detached.
4. Perform a normal write to a versioned entity (for example, a dashboard
update via
`UpdateDashboardCommand` in `superset/commands/dashboard/update.py`, which
writes a
`Dashboard` row defined in `superset/models/dashboard.py:150-40`). The
baseline listener
`capture_baseline()` (`superset/versioning/baseline/listener.py:94-120`)
runs because
`versioning_manager.options["versioning"]` is True, but the change-record
listener’s
`_current_transaction_id()` (`superset/versioning/changes/listener.py:3-14`)
finds no
Continuum transaction because the core listeners were removed, causing
`flush_change_records()` (`superset/versioning/changes/listener.py:157-213`)
to drop the
buffer. Versions and change records are therefore missing or incomplete
despite
`ENABLE_VERSIONING_CAPTURE=True` in this second app.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=a62f3d5164034a1c95d2b4f8ab4ee363&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=a62f3d5164034a1c95d2b4f8ab4ee363&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:** 738:797
**Comment:**
*Incomplete Implementation: The ON path does not restore
SQLAlchemy-Continuum write listeners after the OFF path detaches them, so
toggling capture back to enabled in the same process only flips a flag and
registers custom listeners but still leaves core Continuum writes disabled.
This causes partial/empty version capture despite
ENABLE_VERSIONING_CAPTURE=True. Re-attach Continuum listeners (or avoid
removing them in OFF mode and rely on the versioning option gate) before
enabling capture.
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%2F41176&comment_hash=3d9dd3bb56ff81ffc2ab40e0166c312706d1d83e35e84254a430296149bf163c&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41176&comment_hash=3d9dd3bb56ff81ffc2ab40e0166c312706d1d83e35e84254a430296149bf163c&reaction=dislike'>👎</a>
##########
superset/models/dashboard.py:
##########
@@ -151,6 +151,32 @@ class Dashboard(CoreDashboard, SoftDeleteMixin,
AuditMixinNullable, ImportExport
"""The dashboard object!"""
__tablename__ = "dashboards"
+ # deleted_at exclusion will be added when soft delete is merged.
Review Comment:
**Suggestion:** The comment states that deleted_at exclusion will be added
later, but the exclude list directly below already includes deleted_at. This
mismatch is misleading during future maintenance and can cause incorrect
assumptions during soft-delete rollout work; update the comment to reflect
current behavior. [comment mismatch]
<details>
<summary><b>Severity Level:</b> Minor 🧹</summary>
```mdx
⚠️ Soft-delete rollout work misled by outdated exclusion comment.
⚠️ Comment contradicts implemented __versioned__ deleted_at exclusion
behavior.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Inspect the `Dashboard` model in `superset/models/dashboard.py:150-40`.
At line 154,
immediately under `__tablename__ = "dashboards"`, there is a comment stating
`# deleted_at
exclusion will be added when soft delete is merged.`.
2. Read the `__versioned__` configuration declared just below at lines
169-179. Its
`exclude` list includes `"deleted_at"` along with `"owners"`, `"roles"`, and
audit fields
such as `"changed_on"` and `"created_on"`, showing that the deleted_at
exclusion is
already implemented, not pending.
3. Note that `Dashboard` inherits `SoftDeleteMixin`
(`superset/models/dashboard.py:11`),
and the multi-line comment at lines 26-29 explains that `deleted_at` is
deletion-state
metadata, is tracked by soft delete rather than content versioning, and is
absent from
Continuum’s shadow table, further confirming that the exclusion is active and
intentionally required.
4. This makes the one-line “will be added” comment at line 154 stale and
misleading; a
maintainer reading it could mistakenly believe that deleted_at exclusion is
future work
and adjust soft-delete migrations or versioning behavior under that
incorrect assumption,
even though the code already excludes the field.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=9ca525cf990645f78f5189f593a6fae2&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=9ca525cf990645f78f5189f593a6fae2&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/models/dashboard.py
**Line:** 154:154
**Comment:**
*Comment Mismatch: The comment states that deleted_at exclusion will be
added later, but the exclude list directly below already includes deleted_at.
This mismatch is misleading during future maintenance and can cause incorrect
assumptions during soft-delete rollout work; update the comment to reflect
current behavior.
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%2F41176&comment_hash=77ce9f737a1800b5b53d9bbc5dca2c47bba472ebb8809d667e2713d570c5d4e6&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41176&comment_hash=77ce9f737a1800b5b53d9bbc5dca2c47bba472ebb8809d667e2713d570c5d4e6&reaction=dislike'>👎</a>
##########
superset/initialization/__init__.py:
##########
@@ -626,6 +626,181 @@ 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.
Review Comment:
**Suggestion:** The method docstring says this registers baseline and
retention listeners, but this code registers baseline and change-record
listeners while retention is explicitly out-of-band. Update the docstring to
match actual behavior so operators and maintainers do not infer a nonexistent
retention listener lifecycle from this initializer. [docstring mismatch]
<details>
<summary><b>Severity Level:</b> Minor 🧹</summary>
```mdx
⚠️ Maintainers may incorrectly expect retention listener active here.
⚠️ Misleading initializer docs complicate future retention feature work.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Open `SupersetAppInitializer.init_versioning` in
`superset/initialization/__init__.py:699-203`. The method docstring at line
700 reads
`"""Register SQLAlchemy-Continuum baseline and retention listeners.`
implying that both
baseline and retention listeners are wired here.
2. Read the body of `init_versioning()` in the same file. At lines 149-152
it imports
`register_baseline_listener` and `VERSIONED_MODELS` from
`superset.versioning.baseline`,
and at lines 166-167 it imports `register_change_record_listener` from
`superset.versioning.changes`. The function then calls
`register_baseline_listener()` and
`register_change_record_listener()` at lines 196-198, but does not register
any retention
listener.
3. Inspect the retention comment at the end of `init_versioning()`
(`superset/initialization/__init__.py:199-203`), which states that retention
pruning runs
out-of-band as a scheduled Celery beat task and that the previous
synchronous after_commit
listener was retired, confirming that retention is explicitly not wired in
this
initializer.
4. From these code locations it is clear that the docstring’s mention of
“retention
listeners” no longer matches the implementation, and operators or
maintainers reading the
docstring could incorrectly infer that retention lifecycle hooks are
attached by
`init_versioning()` when in fact they are not.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=ea99a276d24545cc9b8ab2b343a6b600&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=ea99a276d24545cc9b8ab2b343a6b600&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:** 700:700
**Comment:**
*Docstring Mismatch: The method docstring says this registers baseline
and retention listeners, but this code registers baseline and change-record
listeners while retention is explicitly out-of-band. Update the docstring to
match actual behavior so operators and maintainers do not infer a nonexistent
retention listener lifecycle from this initializer.
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%2F41176&comment_hash=0a10f9b665e65ba7695b2ef8db0fdc64e7729bac94983fd746b844f7c56eb1b1&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41176&comment_hash=0a10f9b665e65ba7695b2ef8db0fdc64e7729bac94983fd746b844f7c56eb1b1&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]
