codeant-ai-for-open-source[bot] commented on code in PR #41076:
URL: https://github.com/apache/superset/pull/41076#discussion_r3500164244
##########
superset/initialization/__init__.py:
##########
@@ -619,6 +619,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 docstring says this method registers baseline and
retention listeners, but the implementation registers baseline and
change-record listeners and explicitly states retention is no longer
synchronous here. This mismatch is misleading for operators and maintainers;
update the docstring to describe the actual listeners registered. [docstring
mismatch]
<details>
<summary><b>Severity Level:</b> Minor ๐งน</summary>
```mdx
- โ ๏ธ Docstring misleads about retention listener registration behavior.
- โ ๏ธ Could confuse debugging of version retention handling.
```
</details>
<details>
<summary><b>Steps of Reproduction โ
</b></summary>
```mdx
1. Open `SupersetAppInitializer.init_versioning` in
`/superset/initialization/__init__.py:113-133`; the method docstring at line
693 states
`"""Register SQLAlchemy-Continuum baseline and retention listeners.`
indicating it should
register baseline and retention listeners.
2. Scroll down within the same method: at lines 162-165 the code imports
`register_baseline_listener` and `VERSIONED_MODELS` from
`superset.versioning.baseline`,
and at lines 179-181 it imports `register_change_record_listener` from
`superset.versioning.changes`, but there is no import or reference to any
retention
listener.
3. At the end of `init_versioning`, the implementation at lines 210-211 calls
`register_baseline_listener()` and `register_change_record_listener()` only,
and
immediately below, lines 213-216 explicitly document that "Retention pruning
runs
out-of-band as a scheduled Celery beat task" and that the previous
synchronous retention
listener was retired, confirming that retention listeners are no longer
registered here.
4. Because the docstring still claims this method registers "baseline and
retention
listeners" while the implementation only registers baseline and
change-record listeners
and explicitly moves retention out-of-band, operators or maintainers reading
the docstring
can be misled about where retention is handled, even though runtime behavior
is correct.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=9f90c827bde44cb9bffe18ba208e20d3&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=9f90c827bde44cb9bffe18ba208e20d3&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:** 693:693
**Comment:**
*Docstring Mismatch: The docstring says this method registers baseline
and retention listeners, but the implementation registers baseline and
change-record listeners and explicitly states retention is no longer
synchronous here. This mismatch is misleading for operators and maintainers;
update the docstring to describe the actual listeners registered.
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=be7336d9d3c101687e0d5def56b7a88c0279137dc818acdb34bd7d1b0978eebc&reaction=like'>๐</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41076&comment_hash=be7336d9d3c101687e0d5def56b7a88c0279137dc818acdb34bd7d1b0978eebc&reaction=dislike'>๐</a>
##########
superset/initialization/__init__.py:
##########
@@ -619,6 +619,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
Review Comment:
**Suggestion:** The OFF path detaches Continuum's mapper/session/engine
write listeners, but the ON path only flips
`versioning_manager.options["versioning"]` back to `True` and never re-attaches
those listeners. In a process that initializes with capture OFF and later ON
(multi-app/test reentrancy), version rows will silently stop being written even
though capture is reported as enabled. Re-register the Continuum listeners when
enabling capture (or avoid physical listener removal and rely on the option
gate only). [incomplete implementation]
<details>
<summary><b>Severity Level:</b> Major โ ๏ธ</summary>
```mdx
- โ Version history not recorded after OFF-then-ON initialization.
- โ ๏ธ Activity view endpoints miss changes after config toggle.
- โ ๏ธ Multi-app tests see inconsistent version capture behavior.
```
</details>
<details>
<summary><b>Steps of Reproduction โ
</b></summary>
```mdx
1. In a fresh Python process, import `superset.extensions`, which constructs
`db` and
calls `make_versioned()` in `/superset/extensions/__init__.py:29-53`,
attaching
SQLAlchemy-Continuum's mapper/session/engine write listeners for versioning
to the global
`versioning_manager`.
2. Instantiate a `SupersetAppInitializer` whose
`config["ENABLE_VERSIONING_CAPTURE"]` is
`False` and call `init_app_in_ctx()` in
`/superset/initialization/__init__.py:218-246`,
which calls `self.init_versioning()`; the False branch at lines 713-143 logs
a warning,
then calls `_remove_continuum_write_listeners()` at lines 43-88, which
invokes
`versioning_manager.remove_operations_tracking`, `remove_session_tracking`,
and several
`sa.event.remove(...)` calls to detach Continuum's write listeners and sets
`versioning_manager.options["versioning"] = False`.
3. In the same Python process (multi-app or test reentrancy), instantiate a
second
`SupersetAppInitializer` with `config["ENABLE_VERSIONING_CAPTURE"]` set to
`True` and call
`init_app_in_ctx()` again; this time `init_versioning()` at lines 145-155
only imports
`versioning_manager` and sets `versioning_manager.options["versioning"] =
True`, then
registers the baseline and change-record listeners via
`register_baseline_listener()` and
`register_change_record_listener()` at lines 210-211, but nowhere in the
codebase
(verified via Grep for `make_versioned` and `remove_operations_tracking`)
are Continuum's
mapper/session/engine listeners re-attached after
`_remove_continuum_write_listeners()`
has removed them.
4. Perform any write to a versioned model, e.g. update a dataset through the
dataset
update flow in `/superset/datasets/api.py:509-513` (which depends on
versioning via
`current_entity_version_info`) or similar dashboard/chart update flows in
`/superset/dashboards/api.py:892-894` and `/superset/charts/api.py:501-503`;
because
Continuum's write listeners are still detached despite
`ENABLE_VERSIONING_CAPTURE` being
True, no new shadow rows or `version_transaction` rows are written, so
change-record
capture in `/superset/versioning/changes/listener.py:29-37` and activity
queries in
`/superset/versioning/activity/queries.py:27-40` silently see missing
version history even
though capture is reported as enabled.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=6e4044d2cb4245d9a2a5a47052a725cf&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=6e4044d2cb4245d9a2a5a47052a725cf&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:** 713:733
**Comment:**
*Incomplete Implementation: The OFF path detaches Continuum's
mapper/session/engine write listeners, but the ON path only flips
`versioning_manager.options["versioning"]` back to `True` and never re-attaches
those listeners. In a process that initializes with capture OFF and later ON
(multi-app/test reentrancy), version rows will silently stop being written even
though capture is reported as enabled. Re-register the Continuum listeners when
enabling capture (or avoid physical listener removal and rely on the option
gate only).
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=3855e41e4d7341b6326cdb5f471d7bf1d13c7583f477e136d51ca25598192f43&reaction=like'>๐</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41076&comment_hash=3855e41e4d7341b6326cdb5f471d7bf1d13c7583f477e136d51ca25598192f43&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]
