codeant-ai-for-open-source[bot] commented on code in PR #41075:
URL: https://github.com/apache/superset/pull/41075#discussion_r3574613363
##########
superset/initialization/__init__.py:
##########
@@ -849,10 +857,67 @@ def init_versioning(self) -> None:
register_baseline_listener()
register_change_record_listener()
- # Retention pruning runs out-of-band as a scheduled Celery beat
- # task, shipped as a separate stacked PR. The previous
- # synchronous after_commit listener was retired so retention work
- # doesn't add latency to user saves.
+ # Retention is time-based and runs out-of-band as a Celery beat
+ # task — see ``superset/tasks/version_history_retention.py``
+ # and the ``version_history.prune_old_versions`` entry in
+ # ``CeleryConfig.beat_schedule`` (``superset/config.py``). The
+ # previous synchronous after_commit listener was retired so
+ # retention work doesn't add latency to user saves.
+
+ _RETENTION_TASK_NAME: str = "version_history.prune_old_versions"
+
+ def _warn_if_retention_beat_missing(self) -> None:
+ """WARN at startup when the resolved Celery beat schedule has no
+ ``version_history.prune_old_versions`` entry.
+
+ Operators who redefine ``CeleryConfig`` in ``superset_config.py``
+ — instead of subclassing or merging the default — silently lose
+ the retention task. Capture continues writing rows; the prune
+ never runs; disk grows until paged. The default config carries
+ the entry; this check makes the misconfiguration visible in the
+ deploy log before disk pressure makes it visible at 03:00.
+
+ Handles four shapes of ``CELERY_CONFIG``:
+ * ``None`` — Celery deliberately disabled, no retention either
+ way; return without warning.
+ * a class or module with a ``beat_schedule`` attribute — the
+ default ``CeleryConfig`` shape.
+ * a dict — Celery's documented "config as dict" shape, supported
+ by ``celery_app.config_from_object``.
+ * a dotted import string — also accepted by Celery, but deliberately
+ skipped here because resolving operator code solely for this warning
+ would duplicate Celery loader behavior and could add startup side
+ effects.
+ """
+ celery_config = self.config.get("CELERY_CONFIG")
Review Comment:
**Suggestion:** Add an explicit type annotation for this local configuration
variable to satisfy the type-hint requirement in new code. [custom_rule]
**Severity Level:** Minor 🧹
<details>
<summary><b>Why it matters? ⭐ </b></summary>
This is newly added Python code and the local variable is left untyped even
though the method already uses type annotations elsewhere. Under the stated
rule, this omission is a valid type-hint violation because the variable can be
annotated as an optional Celery config object or a broader union type.
</details>
<details>
<summary><b>Rule source 📖 </b></summary>
.cursor/rules/dev-standard.mdc (line 28)
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=686a067441f1480c822739c045875672&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=686a067441f1480c822739c045875672&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:** 892:892
**Comment:**
*Custom Rule: Add an explicit type annotation for this local
configuration variable to satisfy the type-hint requirement in new code.
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=531125ce624f328206d3c6511c20a9bc1fa110fb9e178eac1cc46ad6684ccc12&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41075&comment_hash=531125ce624f328206d3c6511c20a9bc1fa110fb9e178eac1cc46ad6684ccc12&reaction=dislike'>👎</a>
##########
tests/integration_tests/versioning/retention_prune_tests.py:
##########
@@ -0,0 +1,507 @@
+# 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.
+"""Integration coverage for version-history retention pruning.
+
+Exercises ``superset.tasks.version_history_retention`` end-to-end against
+a real database: that an aged-out transaction's shadow rows are pruned
+while the live row is always preserved, and that the SERIALIZABLE pass
+retries on transient serialization failures and gives up after the cap.
+"""
+
+from __future__ import annotations
+
+from collections.abc import Iterator
+from typing import Any
+
+import pytest
+from sqlalchemy_continuum import version_class
+
+from superset.extensions import db
+from superset.models.dashboard import Dashboard
+from superset.models.slice import Slice
+from superset.versioning.changes.table import version_changes_table
+from tests.integration_tests.base_tests import SupersetTestCase
+from tests.integration_tests.fixtures.birth_names_dashboard import ( # noqa:
F401
+ load_birth_names_dashboard_with_slices,
+ load_birth_names_data,
+)
+
+
+def _get_version_rows(dashboard: Dashboard) -> list[Any]:
+ ver_cls = version_class(Dashboard)
+ return (
+ db.session.query(ver_cls)
+ .filter(ver_cls.id == dashboard.id)
+ .order_by(ver_cls.transaction_id.asc())
+ .all()
+ )
+
+
+def _persist_fixture_state() -> None:
+ """Force the fixture's pending INSERTs to commit in their own transaction.
+
+ The birth_names fixture stages charts and the dashboard via session.add()
+ but does not commit. Without this, the test's first commit batches the
+ INSERTs and UPDATEs into the same Continuum transaction, causing the
+ existing version row to be updated in place instead of a new one being
+ created.
+ """
+ db.session.commit()
+
+
+class TestDashboardVersionRetention(SupersetTestCase):
+ """Retention pruning drops shadow rows older than
+ ``SUPERSET_VERSION_HISTORY_RETENTION_DAYS`` while preserving live rows."""
+
+ @pytest.fixture(autouse=True)
+ def _load_data(
+ self,
+ load_birth_names_dashboard_with_slices: None, # noqa: F811
+ ) -> Iterator[None]:
+ """Establish the capture state this integration class requires.
+
+ Continuum stores its option and SQLAlchemy event listeners globally.
+ Initialization unit tests intentionally exercise the kill-switch that
+ detaches those listeners, so a mixed or reordered pytest invocation can
+ otherwise enter these tests with capture disabled. Reasserting the
+ integration suite's prerequisite here makes each test
order-independent.
+ """
+ import sqlalchemy as sa
+ from sqlalchemy_continuum import versioning_manager
+
+ from superset.initialization import SupersetAppInitializer
+
+ previous_versioning: bool = bool(
+ versioning_manager.options.get("versioning", True)
+ )
+ listeners_were_attached: bool = sa.event.contains(
+ sa.orm.Mapper,
+ "after_insert",
+ versioning_manager.track_inserts,
+ )
+
+ versioning_manager.options["versioning"] = True
+ SupersetAppInitializer._add_continuum_write_listeners()
+ try:
+ yield
+ finally:
+ if listeners_were_attached:
+ SupersetAppInitializer._add_continuum_write_listeners()
+ else:
+ SupersetAppInitializer._remove_continuum_write_listeners()
+ versioning_manager.options["versioning"] = previous_versioning
+
+ def test_retention_prunes_old_rows(self) -> None:
+ """``prune_old_versions`` removes shadow rows whose owning
+ ``version_transaction.issued_at`` is older than the retention
+ window, while preserving every transaction that anchors a live row."""
+ from datetime import datetime, timedelta
+
+ import sqlalchemy as sa
+
+ from superset.extensions import db as _db
+ from superset.tasks.version_history_retention import (
+ _prune_old_versions_impl,
+ )
+
+ _persist_fixture_state()
+ dashboard: Dashboard = (
+ db.session.query(Dashboard)
+ .filter(Dashboard.dashboard_title == "USA Births Names")
+ .first()
+ )
+ assert dashboard is not None
+
+ original_title = dashboard.dashboard_title
+
+ try:
+ # Force a few saves so we have ≥ 2 closed shadow rows plus
+ # a baseline plus the live row.
+ for i in range(3):
+ dashboard.dashboard_title = f"USA Births Names retention test
{i}"
+ db.session.commit()
+
+ rows_before = _get_version_rows(dashboard)
+ assert len(rows_before) >= 3, "Expected at least 3 version rows"
+
+ def _version_changes_count() -> int:
+ return (
+ _db.session.execute(
+ sa.text("SELECT COUNT(*) FROM version_changes")
+ ).scalar()
+ or 0
+ )
+
+ changes_before = _version_changes_count()
+ assert changes_before >= 1, (
+ "expected version_changes rows from the edits above"
+ )
+
+ # Backdate every version_transaction row by 100 days so the
+ # prune sees them as old. Skip baseline+live rows; the prune
+ # itself preserves them.
+ from sqlalchemy_continuum import versioning_manager
+
+ tx_table = versioning_manager.transaction_cls.__table__
+ with _db.engine.begin() as conn:
+ conn.execute(
+ sa.update(tx_table).values(
+ issued_at=datetime.utcnow() - timedelta(days=100)
+ )
+ )
+
+ stats = _prune_old_versions_impl(retention_days=30)
+ assert stats.get("pruned_transactions", 0) >= 1, stats
+
+ # version_changes rows for pruned transactions must be gone too.
+ # The prune deletes version_transaction rows and relies on the
+ # ON DELETE CASCADE FK from version_changes.transaction_id; assert
+ # the cascade actually fired rather than orphaning change records.
+ db.session.expire_all()
+ assert _version_changes_count() < changes_before, (
+ "version_changes rows for pruned transactions were not removed
"
+ f"(before={changes_before}, after={_version_changes_count()})"
+ )
+
+ rows_after = _get_version_rows(dashboard)
+ # Live row must still exist (this is the only preservation rule)
+ live_rows = [r for r in rows_after if r.end_transaction_id is None]
+ assert len(live_rows) >= 1, "Live row must never be pruned"
+ # Some rows should have been pruned. Closed historical rows —
+ # including the synthetic baseline (operation_type=0) — are
+ # subject to retention like everything else.
+ assert len(rows_after) < len(rows_before), (
+ f"Expected fewer rows after prune; before={len(rows_before)} "
+ f"after={len(rows_after)}"
+ )
+
+ finally:
+ dashboard.dashboard_title = original_title
+ db.session.commit()
+
+ def test_retention_preserves_live_child_and_m2m_rows(self) -> None:
+ """Regression for the live-row preservation BLOCKER.
+
+ A parent-only edit leaves the dashboard's chart associations (the
+ M2M ``dashboard_slices_version`` rows) live but anchored at an
+ *older* transaction than the dashboard's current live row. The
+ prune must preserve that older transaction too — if it scans only
+ parent shadows for live rows, it deletes the still-live
+ association and the surviving version silently loses its slices.
+
+ The parent-only ``test_retention_prunes_old_rows`` cannot catch
+ this: it asserts on the dashboard parent shadow alone, which is
+ exactly the blind spot the bug lived in.
+ """
+ from datetime import datetime, timedelta
+
+ import sqlalchemy as sa
+ from sqlalchemy_continuum import version_class, versioning_manager
+
+ from superset.extensions import db as _db
+ from superset.tasks.version_history_retention import
_prune_old_versions_impl
+
+ _persist_fixture_state()
+ dashboard: Dashboard = (
+ db.session.query(Dashboard)
+ .filter(Dashboard.dashboard_title == "USA Births Names")
+ .first()
+ )
+ assert dashboard is not None
+ assert dashboard.slices, "fixture dashboard must have slices for this
test"
+ dashboard_id = dashboard.id
+ original_title = dashboard.dashboard_title
+
+ m2m = version_class(Dashboard).__table__.metadata.tables[
+ "dashboard_slices_version"
+ ]
+
+ def _live_m2m_count() -> int:
+ with _db.engine.begin() as conn:
+ return conn.execute(
+ sa.select(sa.func.count())
+ .select_from(m2m)
+ .where(m2m.c.dashboard_id == dashboard_id)
+ .where(m2m.c.end_transaction_id.is_(None))
+ ).scalar_one()
+
+ try:
+ # Baseline capture synthesizes the live M2M association rows at
+ # the baseline tx. A parent-only edit then opens a newer parent
+ # live row while the association rows stay live at the older tx.
+ dashboard.dashboard_title = "USA Births Names (parent-only edit)"
+ db.session.commit()
+
+ live_before = _live_m2m_count()
+ assert live_before >= 1, (
+ "expected live dashboard_slices_version rows before prune"
+ )
+
+ # Backdate every transaction so the whole chain is older than
+ # the window — including the tx anchoring the live association.
+ tx_table = versioning_manager.transaction_cls.__table__
+ with _db.engine.begin() as conn:
+ conn.execute(
+ sa.update(tx_table).values(
+ issued_at=datetime.utcnow() - timedelta(days=100)
+ )
+ )
+
+ _prune_old_versions_impl(retention_days=30)
+
+ # The live association rows must survive: their anchoring tx is
+ # preserved because they are live, even though it predates the
+ # dashboard's current live parent row.
+ assert _live_m2m_count() == live_before, (
+ "prune deleted live dashboard_slices_version rows — the "
+ "surviving version lost its slices (preservation BLOCKER)"
+ )
+ finally:
+ dashboard.dashboard_title = original_title
+ db.session.commit()
+
+ def test_retention_preserves_multi_flush_transaction(self) -> None:
+ """Pruning preserves #41940's final multi-flush semantic projection."""
+ from datetime import datetime, timedelta, timezone
+
+ import sqlalchemy as sa
+ from sqlalchemy_continuum import versioning_manager
+
+ from superset.tasks.version_history_retention import
_prune_old_versions_impl
+
+ _persist_fixture_state()
+ dashboard: Dashboard = (
+ db.session.query(Dashboard)
+ .filter(Dashboard.dashboard_title == "USA Births Names")
+ .one()
+ )
+ chart: Slice = dashboard.slices[0]
+ dashboard_title = dashboard.dashboard_title
+ chart_name = chart.slice_name
+ tx_table = versioning_manager.transaction_cls.__table__
+ dashboard_version_table = version_class(Dashboard).__table__
+ chart_version_table = version_class(Slice).__table__
+ boundary = db.session.scalar(sa.select(sa.func.max(tx_table.c.id))) or 0
+
+ try:
+ dashboard.dashboard_title = "USA Births Names multi-flush
dashboard"
+ db.session.flush()
+ chart.slice_name = "USA Births Names multi-flush chart"
+ db.session.commit()
+
+ change_rows = db.session.execute(
+ sa.select(
+ version_changes_table.c.transaction_id,
+ version_changes_table.c.entity_kind,
+ )
+ .where(version_changes_table.c.transaction_id > boundary)
+ .where(
+ sa.or_(
+ sa.and_(
+ version_changes_table.c.entity_kind == "dashboard",
+ version_changes_table.c.entity_id == dashboard.id,
+ ),
+ sa.and_(
+ version_changes_table.c.entity_kind == "chart",
+ version_changes_table.c.entity_id == chart.id,
+ ),
+ )
+ )
+ ).all()
+ assert {row.entity_kind for row in change_rows} == {"dashboard",
"chart"}
+ transaction_ids = {row.transaction_id for row in change_rows}
Review Comment:
**Suggestion:** Add a concrete type annotation for this derived transaction
identifier set. [custom_rule]
**Severity Level:** Minor 🧹
<details>
<summary><b>Why it matters? ⭐ </b></summary>
This set is a newly introduced local variable with an inferable type, but it
is left unannotated even though it is used in subsequent assertions, so it fits
the type-hint requirement for relevant variables.
</details>
<details>
<summary><b>Rule source 📖 </b></summary>
.cursor/rules/dev-standard.mdc (line 28)
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=7b710750b38e4286b9c23fcbb34171e2&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=7b710750b38e4286b9c23fcbb34171e2&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:** tests/integration_tests/versioning/retention_prune_tests.py
**Line:** 326:326
**Comment:**
*Custom Rule: Add a concrete type annotation for this derived
transaction identifier set.
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=f2215753fa0aa37f44ff046dfc2a3c46e4f81c14a058cedeaa9f683f752b6a96&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41075&comment_hash=f2215753fa0aa37f44ff046dfc2a3c46e4f81c14a058cedeaa9f683f752b6a96&reaction=dislike'>👎</a>
##########
tests/unit_tests/initialization_test.py:
##########
@@ -518,3 +519,161 @@ def test_trailing_slash_app_root_is_normalized(self):
assert status.startswith("200")
assert captured["PATH_INFO"] == "/welcome/"
assert captured["SCRIPT_NAME"] == "/myapp"
+
+
+class TestRetentionBeatWarning:
+ """Cover ``_warn_if_retention_beat_missing`` — the startup check that
+ surfaces a missing ``version_history.prune_old_versions`` beat entry.
+ (The ``ENABLE_VERSIONING_CAPTURE`` kill-switch branch of
+ ``init_versioning`` is covered by ``TestInitVersioning`` above.)
+
+ Operators who redefine ``CeleryConfig`` instead of subclassing or
+ merging the default silently lose the retention task; this pins that
+ the misconfiguration is logged at startup rather than discovered when
+ disk fills."""
+
+ def _initializer(self, config: dict[str, Any]) -> SupersetAppInitializer:
+ """Build a ``SupersetAppInitializer`` against a minimal mock app
+ whose only meaningful attribute is the config dict;
+ ``_warn_if_retention_beat_missing`` only reads from ``self.config``."""
+ app = MagicMock()
+ app.config = config
+ return SupersetAppInitializer(app)
+
+ @patch("superset.initialization.logger")
+ def test_warn_when_celery_beat_schedule_missing_retention_entry(
+ self, mock_logger: MagicMock
+ ) -> None:
+ """When ``CELERY_CONFIG.beat_schedule`` is present but lacks the
+ ``version_history.prune_old_versions`` entry, the helper emits
+ a WARNING. This guards the silent-failure mode where capture writes
+ rows but the prune never fires."""
+
+ class _PartialCeleryConfig:
+ beat_schedule = {"reports.scheduler": {"task":
"reports.scheduler"}}
Review Comment:
**Suggestion:** Add an explicit type annotation to this class-level schedule
variable to satisfy the type-hint requirement for newly introduced variables.
[custom_rule]
**Severity Level:** Minor 🧹
<details>
<summary><b>Why it matters? ⭐ </b></summary>
This is newly added Python code defining a class-level variable without a
type annotation, which matches the type-hint rule for relevant variables.
</details>
<details>
<summary><b>Rule source 📖 </b></summary>
.cursor/rules/dev-standard.mdc (line 28)
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=4bd02b84b4874031bf0600584dcd2bb5&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=4bd02b84b4874031bf0600584dcd2bb5&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:** tests/unit_tests/initialization_test.py
**Line:** 553:553
**Comment:**
*Custom Rule: Add an explicit type annotation to this class-level
schedule variable to satisfy the type-hint requirement for newly introduced
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%2F41075&comment_hash=40e8fdad383ad2389402284291ec21c69f6fd2527475f3b30c622a3b31e02dd7&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41075&comment_hash=40e8fdad383ad2389402284291ec21c69f6fd2527475f3b30c622a3b31e02dd7&reaction=dislike'>👎</a>
##########
tests/unit_tests/migrations/test_version_transaction_issued_at_index.py:
##########
@@ -0,0 +1,81 @@
+# 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.
+"""Tests for the version-transaction retention index migration."""
+
+from importlib import import_module
+
+import pytest
+from alembic.migration import MigrationContext
+from alembic.operations import Operations
+from sqlalchemy import Column, create_engine, DateTime, inspect, Integer,
MetaData, Table
+from sqlalchemy.engine import Engine
+
+migration = import_module(
+ "superset.migrations.versions."
+ "2026-06-15_16-30_d3b9a1f6c204_version_transaction_issued_at_index"
+)
Review Comment:
**Suggestion:** Add an explicit type annotation to the module-level
`migration` variable so this new code fully complies with the type-hint
requirement. [custom_rule]
**Severity Level:** Minor 🧹
<details>
<summary><b>Why it matters? ⭐ </b></summary>
This new module-level variable is inferred without an explicit type hint,
and it can be annotated (for example as a ModuleType or more specific migration
module type). That matches the Python type-hint requirement for relevant
variables in new code.
</details>
<details>
<summary><b>Rule source 📖 </b></summary>
.cursor/rules/dev-standard.mdc (line 28)
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=c147aec5bf544801a27f9e6815fae9ac&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=c147aec5bf544801a27f9e6815fae9ac&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:**
tests/unit_tests/migrations/test_version_transaction_issued_at_index.py
**Line:** 27:30
**Comment:**
*Custom Rule: Add an explicit type annotation to the module-level
`migration` variable so this new code fully complies with 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%2F41075&comment_hash=ff0854cf287a2295ffb1a863ba2589066c64558c7eac71244b4e605198fd6947&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41075&comment_hash=ff0854cf287a2295ffb1a863ba2589066c64558c7eac71244b4e605198fd6947&reaction=dislike'>👎</a>
##########
tests/integration_tests/versioning/retention_prune_tests.py:
##########
@@ -0,0 +1,507 @@
+# 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.
+"""Integration coverage for version-history retention pruning.
+
+Exercises ``superset.tasks.version_history_retention`` end-to-end against
+a real database: that an aged-out transaction's shadow rows are pruned
+while the live row is always preserved, and that the SERIALIZABLE pass
+retries on transient serialization failures and gives up after the cap.
+"""
+
+from __future__ import annotations
+
+from collections.abc import Iterator
+from typing import Any
+
+import pytest
+from sqlalchemy_continuum import version_class
+
+from superset.extensions import db
+from superset.models.dashboard import Dashboard
+from superset.models.slice import Slice
+from superset.versioning.changes.table import version_changes_table
+from tests.integration_tests.base_tests import SupersetTestCase
+from tests.integration_tests.fixtures.birth_names_dashboard import ( # noqa:
F401
+ load_birth_names_dashboard_with_slices,
+ load_birth_names_data,
+)
+
+
+def _get_version_rows(dashboard: Dashboard) -> list[Any]:
+ ver_cls = version_class(Dashboard)
+ return (
+ db.session.query(ver_cls)
+ .filter(ver_cls.id == dashboard.id)
+ .order_by(ver_cls.transaction_id.asc())
+ .all()
+ )
+
+
+def _persist_fixture_state() -> None:
+ """Force the fixture's pending INSERTs to commit in their own transaction.
+
+ The birth_names fixture stages charts and the dashboard via session.add()
+ but does not commit. Without this, the test's first commit batches the
+ INSERTs and UPDATEs into the same Continuum transaction, causing the
+ existing version row to be updated in place instead of a new one being
+ created.
+ """
+ db.session.commit()
+
+
+class TestDashboardVersionRetention(SupersetTestCase):
+ """Retention pruning drops shadow rows older than
+ ``SUPERSET_VERSION_HISTORY_RETENTION_DAYS`` while preserving live rows."""
+
+ @pytest.fixture(autouse=True)
+ def _load_data(
+ self,
+ load_birth_names_dashboard_with_slices: None, # noqa: F811
+ ) -> Iterator[None]:
+ """Establish the capture state this integration class requires.
+
+ Continuum stores its option and SQLAlchemy event listeners globally.
+ Initialization unit tests intentionally exercise the kill-switch that
+ detaches those listeners, so a mixed or reordered pytest invocation can
+ otherwise enter these tests with capture disabled. Reasserting the
+ integration suite's prerequisite here makes each test
order-independent.
+ """
+ import sqlalchemy as sa
+ from sqlalchemy_continuum import versioning_manager
+
+ from superset.initialization import SupersetAppInitializer
+
+ previous_versioning: bool = bool(
+ versioning_manager.options.get("versioning", True)
+ )
+ listeners_were_attached: bool = sa.event.contains(
+ sa.orm.Mapper,
+ "after_insert",
+ versioning_manager.track_inserts,
+ )
+
+ versioning_manager.options["versioning"] = True
+ SupersetAppInitializer._add_continuum_write_listeners()
+ try:
+ yield
+ finally:
+ if listeners_were_attached:
+ SupersetAppInitializer._add_continuum_write_listeners()
+ else:
+ SupersetAppInitializer._remove_continuum_write_listeners()
+ versioning_manager.options["versioning"] = previous_versioning
+
+ def test_retention_prunes_old_rows(self) -> None:
+ """``prune_old_versions`` removes shadow rows whose owning
+ ``version_transaction.issued_at`` is older than the retention
+ window, while preserving every transaction that anchors a live row."""
+ from datetime import datetime, timedelta
+
+ import sqlalchemy as sa
+
+ from superset.extensions import db as _db
+ from superset.tasks.version_history_retention import (
+ _prune_old_versions_impl,
+ )
+
+ _persist_fixture_state()
+ dashboard: Dashboard = (
+ db.session.query(Dashboard)
+ .filter(Dashboard.dashboard_title == "USA Births Names")
+ .first()
+ )
+ assert dashboard is not None
+
+ original_title = dashboard.dashboard_title
+
+ try:
+ # Force a few saves so we have ≥ 2 closed shadow rows plus
+ # a baseline plus the live row.
+ for i in range(3):
+ dashboard.dashboard_title = f"USA Births Names retention test
{i}"
+ db.session.commit()
+
+ rows_before = _get_version_rows(dashboard)
+ assert len(rows_before) >= 3, "Expected at least 3 version rows"
+
+ def _version_changes_count() -> int:
+ return (
+ _db.session.execute(
+ sa.text("SELECT COUNT(*) FROM version_changes")
+ ).scalar()
+ or 0
+ )
+
+ changes_before = _version_changes_count()
+ assert changes_before >= 1, (
+ "expected version_changes rows from the edits above"
+ )
+
+ # Backdate every version_transaction row by 100 days so the
+ # prune sees them as old. Skip baseline+live rows; the prune
+ # itself preserves them.
+ from sqlalchemy_continuum import versioning_manager
+
+ tx_table = versioning_manager.transaction_cls.__table__
+ with _db.engine.begin() as conn:
+ conn.execute(
+ sa.update(tx_table).values(
+ issued_at=datetime.utcnow() - timedelta(days=100)
+ )
+ )
+
+ stats = _prune_old_versions_impl(retention_days=30)
+ assert stats.get("pruned_transactions", 0) >= 1, stats
+
+ # version_changes rows for pruned transactions must be gone too.
+ # The prune deletes version_transaction rows and relies on the
+ # ON DELETE CASCADE FK from version_changes.transaction_id; assert
+ # the cascade actually fired rather than orphaning change records.
+ db.session.expire_all()
+ assert _version_changes_count() < changes_before, (
+ "version_changes rows for pruned transactions were not removed
"
+ f"(before={changes_before}, after={_version_changes_count()})"
+ )
+
+ rows_after = _get_version_rows(dashboard)
+ # Live row must still exist (this is the only preservation rule)
+ live_rows = [r for r in rows_after if r.end_transaction_id is None]
+ assert len(live_rows) >= 1, "Live row must never be pruned"
+ # Some rows should have been pruned. Closed historical rows —
+ # including the synthetic baseline (operation_type=0) — are
+ # subject to retention like everything else.
+ assert len(rows_after) < len(rows_before), (
+ f"Expected fewer rows after prune; before={len(rows_before)} "
+ f"after={len(rows_after)}"
+ )
+
+ finally:
+ dashboard.dashboard_title = original_title
+ db.session.commit()
+
+ def test_retention_preserves_live_child_and_m2m_rows(self) -> None:
+ """Regression for the live-row preservation BLOCKER.
+
+ A parent-only edit leaves the dashboard's chart associations (the
+ M2M ``dashboard_slices_version`` rows) live but anchored at an
+ *older* transaction than the dashboard's current live row. The
+ prune must preserve that older transaction too — if it scans only
+ parent shadows for live rows, it deletes the still-live
+ association and the surviving version silently loses its slices.
+
+ The parent-only ``test_retention_prunes_old_rows`` cannot catch
+ this: it asserts on the dashboard parent shadow alone, which is
+ exactly the blind spot the bug lived in.
+ """
+ from datetime import datetime, timedelta
+
+ import sqlalchemy as sa
+ from sqlalchemy_continuum import version_class, versioning_manager
+
+ from superset.extensions import db as _db
+ from superset.tasks.version_history_retention import
_prune_old_versions_impl
+
+ _persist_fixture_state()
+ dashboard: Dashboard = (
+ db.session.query(Dashboard)
+ .filter(Dashboard.dashboard_title == "USA Births Names")
+ .first()
+ )
+ assert dashboard is not None
+ assert dashboard.slices, "fixture dashboard must have slices for this
test"
+ dashboard_id = dashboard.id
+ original_title = dashboard.dashboard_title
+
+ m2m = version_class(Dashboard).__table__.metadata.tables[
+ "dashboard_slices_version"
+ ]
+
+ def _live_m2m_count() -> int:
+ with _db.engine.begin() as conn:
+ return conn.execute(
+ sa.select(sa.func.count())
+ .select_from(m2m)
+ .where(m2m.c.dashboard_id == dashboard_id)
+ .where(m2m.c.end_transaction_id.is_(None))
+ ).scalar_one()
+
+ try:
+ # Baseline capture synthesizes the live M2M association rows at
+ # the baseline tx. A parent-only edit then opens a newer parent
+ # live row while the association rows stay live at the older tx.
+ dashboard.dashboard_title = "USA Births Names (parent-only edit)"
+ db.session.commit()
+
+ live_before = _live_m2m_count()
+ assert live_before >= 1, (
+ "expected live dashboard_slices_version rows before prune"
+ )
+
+ # Backdate every transaction so the whole chain is older than
+ # the window — including the tx anchoring the live association.
+ tx_table = versioning_manager.transaction_cls.__table__
+ with _db.engine.begin() as conn:
+ conn.execute(
+ sa.update(tx_table).values(
+ issued_at=datetime.utcnow() - timedelta(days=100)
+ )
+ )
+
+ _prune_old_versions_impl(retention_days=30)
+
+ # The live association rows must survive: their anchoring tx is
+ # preserved because they are live, even though it predates the
+ # dashboard's current live parent row.
+ assert _live_m2m_count() == live_before, (
+ "prune deleted live dashboard_slices_version rows — the "
+ "surviving version lost its slices (preservation BLOCKER)"
+ )
+ finally:
+ dashboard.dashboard_title = original_title
+ db.session.commit()
+
+ def test_retention_preserves_multi_flush_transaction(self) -> None:
+ """Pruning preserves #41940's final multi-flush semantic projection."""
+ from datetime import datetime, timedelta, timezone
+
+ import sqlalchemy as sa
+ from sqlalchemy_continuum import versioning_manager
+
+ from superset.tasks.version_history_retention import
_prune_old_versions_impl
+
+ _persist_fixture_state()
+ dashboard: Dashboard = (
+ db.session.query(Dashboard)
+ .filter(Dashboard.dashboard_title == "USA Births Names")
+ .one()
+ )
+ chart: Slice = dashboard.slices[0]
+ dashboard_title = dashboard.dashboard_title
+ chart_name = chart.slice_name
+ tx_table = versioning_manager.transaction_cls.__table__
+ dashboard_version_table = version_class(Dashboard).__table__
+ chart_version_table = version_class(Slice).__table__
+ boundary = db.session.scalar(sa.select(sa.func.max(tx_table.c.id))) or 0
+
+ try:
+ dashboard.dashboard_title = "USA Births Names multi-flush
dashboard"
+ db.session.flush()
+ chart.slice_name = "USA Births Names multi-flush chart"
+ db.session.commit()
+
+ change_rows = db.session.execute(
+ sa.select(
+ version_changes_table.c.transaction_id,
+ version_changes_table.c.entity_kind,
+ )
+ .where(version_changes_table.c.transaction_id > boundary)
+ .where(
+ sa.or_(
+ sa.and_(
+ version_changes_table.c.entity_kind == "dashboard",
+ version_changes_table.c.entity_id == dashboard.id,
+ ),
+ sa.and_(
+ version_changes_table.c.entity_kind == "chart",
+ version_changes_table.c.entity_id == chart.id,
+ ),
+ )
+ )
+ ).all()
+ assert {row.entity_kind for row in change_rows} == {"dashboard",
"chart"}
+ transaction_ids = {row.transaction_id for row in change_rows}
+ assert len(transaction_ids) == 1
+ transaction_id = transaction_ids.pop()
Review Comment:
**Suggestion:** Annotate this local variable with its expected scalar type
to comply with the rule requiring type hints on relevant variables.
[custom_rule]
**Severity Level:** Minor 🧹
<details>
<summary><b>Why it matters? ⭐ </b></summary>
This scalar local is derived from a set and is used later in multiple
database assertions, so it is a relevant variable that can reasonably be
annotated but currently is not.
</details>
<details>
<summary><b>Rule source 📖 </b></summary>
.cursor/rules/dev-standard.mdc (line 28)
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=aad6cb84df62420f9549a1bcebcf4e0b&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=aad6cb84df62420f9549a1bcebcf4e0b&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:** tests/integration_tests/versioning/retention_prune_tests.py
**Line:** 328:328
**Comment:**
*Custom Rule: Annotate this local variable with its expected scalar
type to comply with the rule requiring type hints on 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%2F41075&comment_hash=13d5ab1a332e091a72247684fad44c5f8bd21799e1abdb97ab99c908103cc38c&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41075&comment_hash=13d5ab1a332e091a72247684fad44c5f8bd21799e1abdb97ab99c908103cc38c&reaction=dislike'>👎</a>
##########
tests/unit_tests/initialization_test.py:
##########
@@ -518,3 +519,161 @@ def test_trailing_slash_app_root_is_normalized(self):
assert status.startswith("200")
assert captured["PATH_INFO"] == "/welcome/"
assert captured["SCRIPT_NAME"] == "/myapp"
+
+
+class TestRetentionBeatWarning:
+ """Cover ``_warn_if_retention_beat_missing`` — the startup check that
+ surfaces a missing ``version_history.prune_old_versions`` beat entry.
+ (The ``ENABLE_VERSIONING_CAPTURE`` kill-switch branch of
+ ``init_versioning`` is covered by ``TestInitVersioning`` above.)
+
+ Operators who redefine ``CeleryConfig`` instead of subclassing or
+ merging the default silently lose the retention task; this pins that
+ the misconfiguration is logged at startup rather than discovered when
+ disk fills."""
+
+ def _initializer(self, config: dict[str, Any]) -> SupersetAppInitializer:
+ """Build a ``SupersetAppInitializer`` against a minimal mock app
+ whose only meaningful attribute is the config dict;
+ ``_warn_if_retention_beat_missing`` only reads from ``self.config``."""
+ app = MagicMock()
+ app.config = config
+ return SupersetAppInitializer(app)
+
+ @patch("superset.initialization.logger")
+ def test_warn_when_celery_beat_schedule_missing_retention_entry(
+ self, mock_logger: MagicMock
+ ) -> None:
+ """When ``CELERY_CONFIG.beat_schedule`` is present but lacks the
+ ``version_history.prune_old_versions`` entry, the helper emits
+ a WARNING. This guards the silent-failure mode where capture writes
+ rows but the prune never fires."""
+
+ class _PartialCeleryConfig:
+ beat_schedule = {"reports.scheduler": {"task":
"reports.scheduler"}}
+
+ initializer = self._initializer({"CELERY_CONFIG":
_PartialCeleryConfig})
+ initializer._warn_if_retention_beat_missing()
+
+ assert any(
+ "version_history.prune_old_versions" in str(call)
+ for call in mock_logger.warning.call_args_list
+ ), (
+ "Expected a WARNING naming the missing retention entry; "
+ f"got {mock_logger.warning.call_args_list}"
+ )
+
+ @patch("superset.initialization.logger")
+ def test_no_warn_when_celery_beat_schedule_includes_retention_entry(
+ self, mock_logger: MagicMock
+ ) -> None:
+ """When the default ``CeleryConfig`` (or any class with the
+ entry) is in play, no warning fires. The happy path."""
+
+ class _CompleteCeleryConfig:
+ beat_schedule = {
+ "version_history.prune_old_versions": {
+ "task": "version_history.prune_old_versions",
+ },
+ }
+
+ initializer = self._initializer({"CELERY_CONFIG":
_CompleteCeleryConfig})
+ initializer._warn_if_retention_beat_missing()
+
+ mock_logger.warning.assert_not_called()
+
+ @patch("superset.initialization.logger")
+ def test_no_warn_when_retention_task_registered_under_other_key(
+ self, mock_logger: MagicMock
+ ) -> None:
+ """The retention task registered under a non-matching schedule key
+ (a valid Celery config) MUST NOT warn: the check matches on each
+ entry's ``task``, not the schedule key."""
+
+ class _RenamedKeyCeleryConfig:
+ beat_schedule = {
+ "prune_versions": {
+ "task": "version_history.prune_old_versions",
+ },
+ }
Review Comment:
**Suggestion:** Annotate this newly added class-level schedule mapping with
an explicit type to comply with required type hints. [custom_rule]
**Severity Level:** Minor 🧹
<details>
<summary><b>Why it matters? ⭐ </b></summary>
This is another newly added class-level variable without a type annotation,
so the type-hint requirement applies.
</details>
<details>
<summary><b>Rule source 📖 </b></summary>
.cursor/rules/dev-standard.mdc (line 28)
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=ec28e9717cf5483883180e642a6fa9a0&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=ec28e9717cf5483883180e642a6fa9a0&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:** tests/unit_tests/initialization_test.py
**Line:** 594:598
**Comment:**
*Custom Rule: Annotate this newly added class-level schedule mapping
with an explicit type to comply with required type hints.
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=d3791a943e2d253ba525861f2bcc9b6375a6b46f7c95915af8510caf903b04f9&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41075&comment_hash=d3791a943e2d253ba525861f2bcc9b6375a6b46f7c95915af8510caf903b04f9&reaction=dislike'>👎</a>
##########
tests/unit_tests/initialization_test.py:
##########
@@ -518,3 +519,161 @@ def test_trailing_slash_app_root_is_normalized(self):
assert status.startswith("200")
assert captured["PATH_INFO"] == "/welcome/"
assert captured["SCRIPT_NAME"] == "/myapp"
+
+
+class TestRetentionBeatWarning:
+ """Cover ``_warn_if_retention_beat_missing`` — the startup check that
+ surfaces a missing ``version_history.prune_old_versions`` beat entry.
+ (The ``ENABLE_VERSIONING_CAPTURE`` kill-switch branch of
+ ``init_versioning`` is covered by ``TestInitVersioning`` above.)
+
+ Operators who redefine ``CeleryConfig`` instead of subclassing or
+ merging the default silently lose the retention task; this pins that
+ the misconfiguration is logged at startup rather than discovered when
+ disk fills."""
+
+ def _initializer(self, config: dict[str, Any]) -> SupersetAppInitializer:
+ """Build a ``SupersetAppInitializer`` against a minimal mock app
+ whose only meaningful attribute is the config dict;
+ ``_warn_if_retention_beat_missing`` only reads from ``self.config``."""
+ app = MagicMock()
+ app.config = config
+ return SupersetAppInitializer(app)
+
+ @patch("superset.initialization.logger")
+ def test_warn_when_celery_beat_schedule_missing_retention_entry(
+ self, mock_logger: MagicMock
+ ) -> None:
+ """When ``CELERY_CONFIG.beat_schedule`` is present but lacks the
+ ``version_history.prune_old_versions`` entry, the helper emits
+ a WARNING. This guards the silent-failure mode where capture writes
+ rows but the prune never fires."""
+
+ class _PartialCeleryConfig:
+ beat_schedule = {"reports.scheduler": {"task":
"reports.scheduler"}}
+
+ initializer = self._initializer({"CELERY_CONFIG":
_PartialCeleryConfig})
+ initializer._warn_if_retention_beat_missing()
+
+ assert any(
+ "version_history.prune_old_versions" in str(call)
+ for call in mock_logger.warning.call_args_list
+ ), (
+ "Expected a WARNING naming the missing retention entry; "
+ f"got {mock_logger.warning.call_args_list}"
+ )
+
+ @patch("superset.initialization.logger")
+ def test_no_warn_when_celery_beat_schedule_includes_retention_entry(
+ self, mock_logger: MagicMock
+ ) -> None:
+ """When the default ``CeleryConfig`` (or any class with the
+ entry) is in play, no warning fires. The happy path."""
+
+ class _CompleteCeleryConfig:
+ beat_schedule = {
+ "version_history.prune_old_versions": {
+ "task": "version_history.prune_old_versions",
+ },
+ }
Review Comment:
**Suggestion:** Add a concrete type annotation for this class-level
`beat_schedule` declaration instead of leaving it inferred. [custom_rule]
**Severity Level:** Minor 🧹
<details>
<summary><b>Why it matters? ⭐ </b></summary>
This added class-level mapping is unannotated, so it violates the rule
requiring type hints for newly introduced relevant variables.
</details>
<details>
<summary><b>Rule source 📖 </b></summary>
.cursor/rules/dev-standard.mdc (line 28)
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=35bc097bd00f451d9bddd1f738d92ac9&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=35bc097bd00f451d9bddd1f738d92ac9&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:** tests/unit_tests/initialization_test.py
**Line:** 574:578
**Comment:**
*Custom Rule: Add a concrete type annotation for this class-level
`beat_schedule` declaration instead of leaving it inferred.
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=1d6952424f5aa0b0780b7b0dd31e094bdf6da8d1110d9ab4d98f82ed12f280d5&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41075&comment_hash=1d6952424f5aa0b0780b7b0dd31e094bdf6da8d1110d9ab4d98f82ed12f280d5&reaction=dislike'>👎</a>
##########
tests/integration_tests/versioning/retention_prune_tests.py:
##########
@@ -0,0 +1,507 @@
+# 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.
+"""Integration coverage for version-history retention pruning.
+
+Exercises ``superset.tasks.version_history_retention`` end-to-end against
+a real database: that an aged-out transaction's shadow rows are pruned
+while the live row is always preserved, and that the SERIALIZABLE pass
+retries on transient serialization failures and gives up after the cap.
+"""
+
+from __future__ import annotations
+
+from collections.abc import Iterator
+from typing import Any
+
+import pytest
+from sqlalchemy_continuum import version_class
+
+from superset.extensions import db
+from superset.models.dashboard import Dashboard
+from superset.models.slice import Slice
+from superset.versioning.changes.table import version_changes_table
+from tests.integration_tests.base_tests import SupersetTestCase
+from tests.integration_tests.fixtures.birth_names_dashboard import ( # noqa:
F401
+ load_birth_names_dashboard_with_slices,
+ load_birth_names_data,
+)
+
+
+def _get_version_rows(dashboard: Dashboard) -> list[Any]:
+ ver_cls = version_class(Dashboard)
+ return (
+ db.session.query(ver_cls)
+ .filter(ver_cls.id == dashboard.id)
+ .order_by(ver_cls.transaction_id.asc())
+ .all()
+ )
+
+
+def _persist_fixture_state() -> None:
+ """Force the fixture's pending INSERTs to commit in their own transaction.
+
+ The birth_names fixture stages charts and the dashboard via session.add()
+ but does not commit. Without this, the test's first commit batches the
+ INSERTs and UPDATEs into the same Continuum transaction, causing the
+ existing version row to be updated in place instead of a new one being
+ created.
+ """
+ db.session.commit()
+
+
+class TestDashboardVersionRetention(SupersetTestCase):
+ """Retention pruning drops shadow rows older than
+ ``SUPERSET_VERSION_HISTORY_RETENTION_DAYS`` while preserving live rows."""
+
+ @pytest.fixture(autouse=True)
+ def _load_data(
+ self,
+ load_birth_names_dashboard_with_slices: None, # noqa: F811
+ ) -> Iterator[None]:
+ """Establish the capture state this integration class requires.
+
+ Continuum stores its option and SQLAlchemy event listeners globally.
+ Initialization unit tests intentionally exercise the kill-switch that
+ detaches those listeners, so a mixed or reordered pytest invocation can
+ otherwise enter these tests with capture disabled. Reasserting the
+ integration suite's prerequisite here makes each test
order-independent.
+ """
+ import sqlalchemy as sa
+ from sqlalchemy_continuum import versioning_manager
+
+ from superset.initialization import SupersetAppInitializer
+
+ previous_versioning: bool = bool(
+ versioning_manager.options.get("versioning", True)
+ )
+ listeners_were_attached: bool = sa.event.contains(
+ sa.orm.Mapper,
+ "after_insert",
+ versioning_manager.track_inserts,
+ )
+
+ versioning_manager.options["versioning"] = True
+ SupersetAppInitializer._add_continuum_write_listeners()
+ try:
+ yield
+ finally:
+ if listeners_were_attached:
+ SupersetAppInitializer._add_continuum_write_listeners()
+ else:
+ SupersetAppInitializer._remove_continuum_write_listeners()
+ versioning_manager.options["versioning"] = previous_versioning
+
+ def test_retention_prunes_old_rows(self) -> None:
+ """``prune_old_versions`` removes shadow rows whose owning
+ ``version_transaction.issued_at`` is older than the retention
+ window, while preserving every transaction that anchors a live row."""
+ from datetime import datetime, timedelta
+
+ import sqlalchemy as sa
+
+ from superset.extensions import db as _db
+ from superset.tasks.version_history_retention import (
+ _prune_old_versions_impl,
+ )
+
+ _persist_fixture_state()
+ dashboard: Dashboard = (
+ db.session.query(Dashboard)
+ .filter(Dashboard.dashboard_title == "USA Births Names")
+ .first()
+ )
+ assert dashboard is not None
+
+ original_title = dashboard.dashboard_title
+
+ try:
+ # Force a few saves so we have ≥ 2 closed shadow rows plus
+ # a baseline plus the live row.
+ for i in range(3):
+ dashboard.dashboard_title = f"USA Births Names retention test
{i}"
+ db.session.commit()
+
+ rows_before = _get_version_rows(dashboard)
+ assert len(rows_before) >= 3, "Expected at least 3 version rows"
+
+ def _version_changes_count() -> int:
+ return (
+ _db.session.execute(
+ sa.text("SELECT COUNT(*) FROM version_changes")
+ ).scalar()
+ or 0
+ )
+
+ changes_before = _version_changes_count()
+ assert changes_before >= 1, (
+ "expected version_changes rows from the edits above"
+ )
+
+ # Backdate every version_transaction row by 100 days so the
+ # prune sees them as old. Skip baseline+live rows; the prune
+ # itself preserves them.
+ from sqlalchemy_continuum import versioning_manager
+
+ tx_table = versioning_manager.transaction_cls.__table__
+ with _db.engine.begin() as conn:
+ conn.execute(
+ sa.update(tx_table).values(
+ issued_at=datetime.utcnow() - timedelta(days=100)
+ )
+ )
+
+ stats = _prune_old_versions_impl(retention_days=30)
+ assert stats.get("pruned_transactions", 0) >= 1, stats
+
+ # version_changes rows for pruned transactions must be gone too.
+ # The prune deletes version_transaction rows and relies on the
+ # ON DELETE CASCADE FK from version_changes.transaction_id; assert
+ # the cascade actually fired rather than orphaning change records.
+ db.session.expire_all()
+ assert _version_changes_count() < changes_before, (
+ "version_changes rows for pruned transactions were not removed
"
+ f"(before={changes_before}, after={_version_changes_count()})"
+ )
+
+ rows_after = _get_version_rows(dashboard)
+ # Live row must still exist (this is the only preservation rule)
+ live_rows = [r for r in rows_after if r.end_transaction_id is None]
+ assert len(live_rows) >= 1, "Live row must never be pruned"
+ # Some rows should have been pruned. Closed historical rows —
+ # including the synthetic baseline (operation_type=0) — are
+ # subject to retention like everything else.
+ assert len(rows_after) < len(rows_before), (
+ f"Expected fewer rows after prune; before={len(rows_before)} "
+ f"after={len(rows_after)}"
+ )
+
+ finally:
+ dashboard.dashboard_title = original_title
+ db.session.commit()
+
+ def test_retention_preserves_live_child_and_m2m_rows(self) -> None:
+ """Regression for the live-row preservation BLOCKER.
+
+ A parent-only edit leaves the dashboard's chart associations (the
+ M2M ``dashboard_slices_version`` rows) live but anchored at an
+ *older* transaction than the dashboard's current live row. The
+ prune must preserve that older transaction too — if it scans only
+ parent shadows for live rows, it deletes the still-live
+ association and the surviving version silently loses its slices.
+
+ The parent-only ``test_retention_prunes_old_rows`` cannot catch
+ this: it asserts on the dashboard parent shadow alone, which is
+ exactly the blind spot the bug lived in.
+ """
+ from datetime import datetime, timedelta
+
+ import sqlalchemy as sa
+ from sqlalchemy_continuum import version_class, versioning_manager
+
+ from superset.extensions import db as _db
+ from superset.tasks.version_history_retention import
_prune_old_versions_impl
+
+ _persist_fixture_state()
+ dashboard: Dashboard = (
+ db.session.query(Dashboard)
+ .filter(Dashboard.dashboard_title == "USA Births Names")
+ .first()
+ )
+ assert dashboard is not None
+ assert dashboard.slices, "fixture dashboard must have slices for this
test"
+ dashboard_id = dashboard.id
+ original_title = dashboard.dashboard_title
+
+ m2m = version_class(Dashboard).__table__.metadata.tables[
+ "dashboard_slices_version"
+ ]
Review Comment:
**Suggestion:** Add an explicit type annotation for this table reference to
satisfy the type-hint requirement for important local variables. [custom_rule]
**Severity Level:** Minor 🧹
<details>
<summary><b>Why it matters? ⭐ </b></summary>
This local table reference is newly introduced without any type annotation,
and it is a relevant variable that can be annotated under the Python type-hint
rule.
</details>
<details>
<summary><b>Rule source 📖 </b></summary>
.cursor/rules/dev-standard.mdc (line 28)
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=31cc596d9c04470d94c21e6d0461376b&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=31cc596d9c04470d94c21e6d0461376b&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:** tests/integration_tests/versioning/retention_prune_tests.py
**Line:** 229:231
**Comment:**
*Custom Rule: Add an explicit type annotation for this table reference
to satisfy the type-hint requirement for important local 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%2F41075&comment_hash=e9ed3225c1255a0bf50d26e6db40e8d2bdfc725d1467b39423dd08f3e709d300&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41075&comment_hash=e9ed3225c1255a0bf50d26e6db40e8d2bdfc725d1467b39423dd08f3e709d300&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]
