mikebridge commented on code in PR #41075:
URL: https://github.com/apache/superset/pull/41075#discussion_r3574708958
##########
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:
Addressed in b41582799 with a concrete class-level schedule mapping
annotation.
##########
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:
Addressed in b41582799 with a concrete class-level schedule mapping
annotation.
##########
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:
Addressed in b41582799 with an explicit `ModuleType` annotation; PR-scoped
MyPy passes.
##########
superset/config.py:
##########
@@ -1575,6 +1575,36 @@ def sync_theme_logo_href(
os.environ.get("ENABLE_VERSIONING_CAPTURE", "false")
)
+# Retention window (days) for entity version history. Version rows
+# whose owning ``version_transaction.issued_at`` is older than this
+# value are pruned by the ``version_history.prune_old_versions``
+# Celery beat task (registered below in ``CeleryConfig.beat_schedule``).
+# If any row anchored at a transaction is live
+# (``end_transaction_id IS NULL``), that entire transaction is preserved.
+# Baseline rows (``operation_type=0``) and closed historical rows otherwise
+# age out alongside the rest. Any non-positive value disables pruning.
+# Read from environment variable of the same name.
+_DEFAULT_VERSION_HISTORY_RETENTION_DAYS: int = 30
+
+
+def _parse_version_history_retention_days() -> int:
+ """Parse the retention window without making invalid input fatal."""
+ value = os.environ.get("SUPERSET_VERSION_HISTORY_RETENTION_DAYS")
+ if value is None:
+ return _DEFAULT_VERSION_HISTORY_RETENTION_DAYS
+ try:
+ return int(value)
+ except ValueError:
Review Comment:
Fixed in b41582799. The parser now rejects retention windows above 36,500
days and falls back to the 30-day default, keeping cutoff arithmetic safely
inside the datetime range. Added an oversized-value regression test; 55 related
tests and all PR-scoped pre-commit hooks pass.
--
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]
