bito-code-review[bot] commented on code in PR #41549: URL: https://github.com/apache/superset/pull/41549#discussion_r3574600039
########## superset/tasks/deletion_retention.py: ########## @@ -0,0 +1,258 @@ +# 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. +"""Celery beat task: purge soft-deleted entities past the retention window. + +The deletion-domain analog of ``version_history.prune_old_versions``: where +that ages out version rows while keeping the live entity, this removes +entities that are already soft-deleted. For each +``SoftDeleteMixin`` model it selects rows whose ``deleted_at`` is older than +the per-workspace window and runs the shared cascade per entity, in bounded +id-ordered batches. Convergent, not strictly idempotent: a re-run with the +same clock and data removes nothing, but rows that have since crossed the +cutoff are purged on a later run. +""" + +from __future__ import annotations + +import logging +from collections.abc import Iterator +from datetime import datetime, timedelta +from typing import Any, cast + +import sqlalchemy as sa +from flask import current_app + +from superset import db +from superset.commands.deletion_retention import audit +from superset.commands.deletion_retention.purge_cascade import ( + cascade_hard_delete, + CascadeResult, + dashboard_slice_count, + entity_uuid, + suppress_purge_association_versions, +) +from superset.commands.deletion_retention.window import resolve_retention_window +from superset.extensions import celery_app, feature_flag_manager, stats_logger_manager +from superset.models.helpers import ( + skip_visibility_filter, + SoftDeleteMixin, +) + +logger: logging.Logger = logging.getLogger(__name__) + +_METRIC_PREFIX: str = "deletion_retention" +# Below SQLite's historical 999 bind-variable limit; well under PostgreSQL and +# MySQL limits. There is no existing SQLITE_MAX_VARIABLE_NUMBER symbol to reuse. +_PURGE_DELETE_CHUNK: int = 500 +_BATCH: int = _PURGE_DELETE_CHUNK + + +def _soft_delete_models() -> list[type[SoftDeleteMixin]]: + """The registered ``SoftDeleteMixin`` subclasses (dashboards, charts, + datasets), in a stable order.""" + return list(SoftDeleteMixin._registered_subclasses) # noqa: SLF001 + + +def _model_table(model: type[SoftDeleteMixin]) -> sa.Table: + """Return SQLAlchemy table metadata for a registered soft-delete model.""" + return cast(sa.Table, cast(Any, model).__table__) + + +def _model_table_name(model: type[SoftDeleteMixin]) -> str: + """Return the table name for a registered soft-delete model.""" + return str(cast(Any, model).__tablename__) + + +def _iter_eligible_ids( + model: type[SoftDeleteMixin], cutoff: datetime, batch: int +) -> Iterator[list[int]]: + """Yield id-ordered batches of eligible row ids — ``deleted_at IS NOT NULL + AND deleted_at < cutoff`` — querying with the visibility-filter bypass so + soft-deleted rows are visible. Windowed by an ``id`` watermark so memory + and lock-hold stay bounded on a large first run.""" + table = _model_table(model) + after_id = 0 + while True: + with skip_visibility_filter(db.session, model): + ids = [ + row[0] + for row in db.session.execute( + sa.select(table.c.id) + .where(table.c.deleted_at.is_not(None)) + .where(table.c.deleted_at < cutoff) + .where(table.c.id > after_id) + .order_by(table.c.id) + .limit(batch) + ) + ] + if not ids: + return + yield ids + if len(ids) < batch: + return + after_id = ids[-1] + + +def _purge_impl(window_days: int, dry_run: bool) -> dict[str, Any]: + """Run one purge pass across all soft-delete models.""" + if window_days <= 0: + logger.info("deletion_retention: window is 0 (disabled); skipping") + stats_logger_manager.instance.incr(f"{_METRIC_PREFIX}.skipped") + return {"skipped": 1} + + cutoff = datetime.now() - timedelta(days=window_days) + audit.reconcile_pending() + purged: dict[str, int] = {} + would_purge: dict[str, int] = {} + failures = 0 + blocked = 0 + + for model in _soft_delete_models(): + entity_type = _model_table_name(model) + purged_n, would_n, failed_n, blocked_n = _purge_model(model, cutoff, dry_run) + if would_n: + would_purge[entity_type] = would_n + if purged_n: + purged[entity_type] = purged_n + failures += failed_n + blocked += blocked_n + + if dry_run: + for entity_type, count in would_purge.items(): + stats_logger_manager.instance.gauge( + f"{_METRIC_PREFIX}.would_purge.{entity_type}", count + ) + logger.info("deletion_retention: DRY RUN would_purge=%s", would_purge) + return {"dry_run": 1, "would_purge": would_purge} + + for entity_type, count in purged.items(): + stats_logger_manager.instance.gauge( + f"{_METRIC_PREFIX}.purged.{entity_type}", count + ) + if failures: + stats_logger_manager.instance.incr(f"{_METRIC_PREFIX}.cascade_failures") + if blocked: + stats_logger_manager.instance.gauge( + f"{_METRIC_PREFIX}.blocked_by_reference", blocked + ) + stats = { + "purged": purged, + "cascade_failures": failures, + "blocked_by_reference": blocked, + } + logger.info("deletion_retention: %s", stats) + return stats + + +def _purge_model( + model: type[SoftDeleteMixin], cutoff: datetime, dry_run: bool +) -> tuple[int, int, int, int]: + """Process one model's eligible rows. Returns ``(purged, would_purge, + failures, blocked)``. A single entity's blocked/failed cascade never aborts + the batch.""" + entity_type = _model_table_name(model) + purged = would = failures = blocked = 0 + for id_batch in _iter_eligible_ids(model, cutoff, _BATCH): + if dry_run: + would += len(id_batch) + continue + for entity_id in id_batch: + try: + result = _purge_one(model, entity_id, cutoff) + if result is not None and result.purged: + purged += 1 + elif result is not None and result.blocked_reason is not None: + blocked += 1 + except Exception: # pylint: disable=broad-except Review Comment: <div> <div id="suggestion"> <div id="issue"><b>Move try-except outside loop</b></div> <div id="fix"> `try`-`except` inside loop causes performance overhead. Consider refactoring to batch handling. </div> </div> <small><i>Code Review Run #faba71</i></small> </div> --- Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>) - [ ] Yes, avoid them ########## tests/unit_tests/tasks/test_deletion_retention.py: ########## @@ -0,0 +1,193 @@ +# 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. +"""Unit tests for deletion-retention configuration and window resolution. + +The shared value overrides config, an unset value falls back to config, ``0`` +is preserved as the disable value, and malformed shared values use the fallback. +""" + +import runpy +from datetime import datetime, timedelta +from pathlib import Path +from typing import Any +from unittest.mock import MagicMock, patch + +import pytest +from flask.config import Config + + [email protected] +def app_config(app_context: None) -> Config: + from flask import current_app + + current_app.config["SUPERSET_SOFT_DELETE_RETENTION_DAYS"] = 30 + return current_app.config + + +def _resolve() -> int: + from superset.commands.deletion_retention.window import resolve_retention_window + + return resolve_retention_window() + + +def test_unset_falls_back_to_config(app_config: Config) -> None: + with patch( + "superset.commands.deletion_retention.window.get_shared_value", + return_value=None, + ): + assert _resolve() == 30 + + +def test_shared_value_overrides_config(app_config: Config) -> None: + with patch( + "superset.commands.deletion_retention.window.get_shared_value", + return_value=7, + ): + assert _resolve() == 7 + + +def test_zero_shared_value_is_preserved_not_coerced(app_config: Config) -> None: + # `0` is a meaningful "disable"; it must survive (never `or`-coerced to 30). + with patch( + "superset.commands.deletion_retention.window.get_shared_value", + return_value=0, + ): + assert _resolve() == 0 + + +def test_malformed_shared_value_falls_back(app_config: Config) -> None: + for bad in ("oops", -3, True, 1.5): + with patch( + "superset.commands.deletion_retention.window.get_shared_value", + return_value=bad, + ): + assert _resolve() == 30 + + [email protected]("configured", ["oops", -3, True, None]) +def test_malformed_config_value_falls_back( + app_config: Config, configured: object +) -> None: + app_config["SUPERSET_SOFT_DELETE_RETENTION_DAYS"] = configured + with patch( + "superset.commands.deletion_retention.window.get_shared_value", + return_value=None, + ): + assert _resolve() == 30 + + +def test_window_zero_disables_the_task(app_context: None) -> None: + # A zero window short-circuits the purge entirely. + import superset.tasks.deletion_retention as mod + + with patch.object(mod, "_soft_delete_models") as models: + result = mod._purge_impl(0, dry_run=False) + assert result == {"skipped": 1} + models.assert_not_called() + + +def test_clock_uses_now_not_utcnow() -> None: + import superset.tasks.deletion_retention as mod + from superset.models.slice import Slice + + now = datetime(2026, 7, 13, 12, 0) + with ( + patch.object(mod, "datetime") as clock, + patch.object(mod, "_soft_delete_models", return_value=[Slice]), + patch.object(mod, "_purge_model", return_value=(0, 0, 0, 0)) as purge, + patch.object(mod.audit, "reconcile_pending"), + ): + clock.now.return_value = now + mod._purge_impl(30, dry_run=False) + + purge.assert_called_once_with(Slice, now - timedelta(days=30), False) + + +def test_default_config_is_safe() -> None: + from superset import config + + assert config.SUPERSET_SOFT_DELETE_RETENTION_DAYS == 30 + assert config.SUPERSET_SOFT_DELETE_PURGE_DRY_RUN is True + + +def test_default_celery_config_registers_daily_purge() -> None: + from superset import config + + assert "superset.tasks.deletion_retention" in config.CeleryConfig.imports + entry: dict[str, Any] = config.CeleryConfig.beat_schedule[ + "deletion_retention.purge_soft_deleted" + ] + assert entry["task"] == "deletion_retention.purge_soft_deleted" + assert entry["schedule"].minute == {0} + assert entry["schedule"].hour == {0} + + +def test_docker_celery_config_registers_daily_purge() -> None: + config_path = Path(__file__).parents[3] / "docker/pythonpath_dev/superset_config.py" + with patch("flask_caching.backends.filesystemcache.FileSystemCache"): + docker_config: dict[str, Any] = runpy.run_path(str(config_path)) + celery_config: type[Any] = docker_config["CeleryConfig"] + + assert "superset.tasks.deletion_retention" in celery_config.imports + entry: dict[str, Any] = celery_config.beat_schedule[ + "deletion_retention.purge_soft_deleted" + ] + assert entry["task"] == "deletion_retention.purge_soft_deleted" + assert entry["schedule"].minute == {0} + assert entry["schedule"].hour == {0} + + +def test_purge_suppression_is_session_scoped() -> None: + from superset.commands.deletion_retention.purge_cascade import ( + suppress_purge_association_versions, + ) + + existing = object() + purge_statement = object() + unit_of_work = MagicMock() + unit_of_work.pending_statements = [existing] + manager = MagicMock() + manager.options = {"versioning": True, "native_versioning": False} + manager.unit_of_work.return_value = unit_of_work + session = MagicMock() + + with patch("sqlalchemy_continuum.versioning_manager", manager): + with suppress_purge_association_versions(session): + assert manager.options["versioning"] is True + unit_of_work.pending_statements.append(purge_statement) + + assert unit_of_work.pending_statements == [existing] + manager.unit_of_work.assert_called_once_with(session) + + +def test_purge_model_counts_only_committed_deletions(app_context: None) -> None: + import superset.tasks.deletion_retention as mod + from superset.commands.deletion_retention.purge_cascade import CascadeResult + from superset.models.slice import Slice + + lost_race: CascadeResult = CascadeResult( + purged=False, entity_type="chart", entity_uuid="lost-race" + ) + with ( + patch.object(mod, "_iter_eligible_ids", return_value=[[1]]), + patch.object(mod, "_purge_one", return_value=lost_race), + ): + result: tuple[int, int, int, int] = mod._purge_model( + Slice, datetime.now(), dry_run=False + ) Review Comment: <div> <div id="suggestion"> <div id="issue"><b>datetime.now without timezone</b></div> <div id="fix"> Using `datetime.now()` without timezone info (DTZ005). Use timezone-aware datetime for consistency with other parts of the codebase. </div> <details> <summary> <b>Code suggestion</b> </summary> <blockquote>Check the AI-generated fix before applying</blockquote> <div id="code"> ````suggestion patch.object(mod, "_purge_one", return_value=lost_race), ): result: tuple[int, int, int, int] = mod._purge_model( Slice, datetime.now(tz=timezone.utc), dry_run=False ) ```` </div> </details> </div> <small><i>Code Review Run #faba71</i></small> </div> --- Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>) - [ ] Yes, avoid them ########## tests/integration_tests/deletion_retention/purge_tests.py: ########## @@ -0,0 +1,447 @@ +# 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 the time-based soft-delete purge. + +Exercises ``superset.tasks.deletion_retention`` against a real database: +the cascade (M:N joins, owned children, datasource permission, version +shadows), preservation of surviving entities, dry-run, the explicit-delete +guarantee under FK enforcement OFF, and the version-tables-absent no-op. +""" + +from __future__ import annotations + +from datetime import datetime, timedelta +from typing import Any +from unittest.mock import MagicMock, patch + +import sqlalchemy as sa +from sqlalchemy.sql.dml import Delete + +from superset import db, security_manager +from superset.commands.deletion_retention import audit +from superset.commands.deletion_retention.purge_cascade import cascade_hard_delete +from superset.connectors.sqla.models import ( + RLSFilterTables, + RowLevelSecurityFilter, + SqlaTable, +) +from superset.models.dashboard import Dashboard +from superset.models.slice import Slice +from superset.models.user_attributes import UserAttribute +from superset.reports.models import ReportSchedule +from superset.tags.models import ObjectType, Tag, TaggedObject +from superset.tasks.deletion_retention import _purge_impl + +from ._base import DeletionRetentionTestBase + + +def _purge(window: int = 30, dry_run: bool = False) -> dict[str, Any]: + return _purge_impl(window, dry_run) + + +class TestSoftDeletePurge(DeletionRetentionTestBase): + def test_aged_out_purged_in_window_and_active_preserved(self) -> None: + """Rows past the window are purged; in-window + soft-deleted and active rows are preserved.""" + aged = self.make_chart("aged") + recent = self.make_chart("recent") + active = self.make_chart("active") + aged_id, recent_id, active_id = aged.id, recent.id, active.id + self.soft_delete(aged, days_ago=90) + self.soft_delete(recent, days_ago=5) + + result = _purge(window=30) + + assert result["purged"].get("slices") == 1, result + assert not self.exists(Slice, aged_id) + assert self.exists(Slice, recent_id) + assert self.exists(Slice, active_id) + + def test_window_zero_disables(self) -> None: + """Window 0 disables the time-based purge.""" + aged = self.make_chart("aged") + aged_id = aged.id + self.soft_delete(aged, days_ago=90) + + assert _purge(window=0) == {"skipped": 1} + assert self.exists(Slice, aged_id) + + def test_dry_run_removes_nothing(self) -> None: + """Dry-run reports would_purge but deletes nothing.""" + aged = self.make_chart("aged") + aged_id = aged.id + self.soft_delete(aged, days_ago=90) + + result = _purge(window=30, dry_run=True) + + assert result["would_purge"].get("slices") == 1, result + assert self.exists(Slice, aged_id) + + def test_purging_chart_unlinks_live_dashboard_but_keeps_it(self) -> None: + """Purging a chart removes its dashboard_slices + rows — including one on a *live* dashboard — but the dashboard and + other charts survive.""" + chart = self.make_chart("c") + other = self.make_chart("other") + chart_id, other_id = chart.id, other.id + dash = self.make_dashboard("live", slices=[chart, other]) + dash_id = dash.id + self.soft_delete(chart, days_ago=90) + + _purge(window=30) + + assert not self.exists(Slice, chart_id) + assert self.exists(Slice, other_id) + assert self.exists(Dashboard, dash_id) + remaining = self.count( + "SELECT count(*) FROM dashboard_slices WHERE slice_id = :i", + {"i": chart_id}, + ) + assert remaining == 0 + # the live dashboard keeps its link to the surviving chart + assert ( + self.count( + "SELECT count(*) FROM dashboard_slices WHERE dashboard_id = :d", + {"d": dash_id}, + ) + == 1 + ) + + def test_purging_dashboard_preserves_its_charts(self) -> None: + """Purging a dashboard does not remove the + independently-owned charts it referenced.""" + chart = self.make_chart("kept") + chart_id = chart.id + dash = self.make_dashboard("doomed", slices=[chart]) + dash_id = dash.id + self.soft_delete(dash, days_ago=90) + + _purge(window=30) + + assert not self.exists(Dashboard, dash_id) + assert self.exists(Slice, chart_id) + assert ( + self.count( + "SELECT count(*) FROM dashboard_slices WHERE dashboard_id = :d", + {"d": dash_id}, + ) + == 0 + ) + + def test_purging_dataset_removes_children_and_permission(self) -> None: + """A dataset's owned columns, metrics, and datasource + permission are removed with it.""" + dataset = self.make_dataset("withchildren", with_children=True) + ds_id = dataset.id + vm_name = security_manager.get_dataset_perm( + dataset.id, dataset.table_name, dataset.database.database_name + ) + assert security_manager.find_permission_view_menu( + "datasource_access", vm_name + ), "fixture should have created the datasource PVM" + self.soft_delete(dataset, days_ago=90) + + _purge(window=30) + + assert not self.exists(SqlaTable, ds_id) + assert ( + self.count( + "SELECT count(*) FROM table_columns WHERE table_id = :i", {"i": ds_id} + ) + == 0 + ) + assert ( + self.count( + "SELECT count(*) FROM sql_metrics WHERE table_id = :i", {"i": ds_id} + ) + == 0 + ) + assert not security_manager.find_permission_view_menu( + "datasource_access", vm_name + ) + + def test_restore_race_does_not_remove_dataset_permission(self) -> None: + """A zero-row conditional parent delete leaves its permission intact.""" + dataset = self.make_dataset("restore_race") + vm_name = security_manager.get_dataset_perm( + dataset.id, dataset.table_name, dataset.database.database_name + ) + self.soft_delete(dataset, days_ago=90) + session = db.session() + execute = session.execute + + def lose_parent_delete(statement: Any, *args: Any, **kwargs: Any) -> Any: + if isinstance(statement, Delete) and statement.table.name == "tables": + return MagicMock(rowcount=0) + return execute(statement, *args, **kwargs) + + with patch.object(session, "execute", side_effect=lose_parent_delete): + result = cascade_hard_delete( + session, + dataset, + enforce_window=True, + cutoff=datetime.now() - timedelta(days=30), Review Comment: <div> <div id="suggestion"> <div id="issue"><b>Missing timezone in datetime.now() call</b></div> <div id="fix"> The `datetime.now()` call does not include a timezone argument (`tz`), which can lead to timezone-related bugs. Replace with `datetime.now(tz=timezone.utc)` or use `utcnow()`. </div> </div> <small><i>Code Review Run #faba71</i></small> </div> --- Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>) - [ ] Yes, avoid them -- 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]
