bito-code-review[bot] commented on code in PR #43490: URL: https://github.com/apache/superset/pull/43490#discussion_r3855202894
########## superset/migrations/versions/2026-08-24_15-50_a6c21e5b4d93_index_purge_audit_pruning.py: ########## @@ -0,0 +1,46 @@ +# 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. +"""Index purge-audit pruning predicates. + +Revision ID: a6c21e5b4d93 +Revises: 1072de5ed955 +Create Date: 2026-08-24 15:50:00.000000 + +""" + +from superset.migrations.shared.utils import create_index, drop_index + +# revision identifiers, used by Alembic. +revision: str = "a6c21e5b4d93" +down_revision: str = "1072de5ed955" + +_TABLE_NAME: str = "purge_audit_log" +_INDEX_NAME: str = "ix_purge_audit_log_pruning" + + +def upgrade() -> None: + """Add an index matching recurring pruning access patterns.""" + create_index( + _TABLE_NAME, + _INDEX_NAME, + ["status", "entity_type", "entity_uuid", "created_on"], + ) + + +def downgrade() -> None: + """Remove the purge-audit pruning index.""" + drop_index(_TABLE_NAME, _INDEX_NAME) Review Comment: <!-- Bito Reply --> The user's clarification confirms that the migration is indeed covered by the existing test suite, which validates the table, index name, ordered columns, and downgrade symmetry. The confusion likely stemmed from the filename focusing on the subsequent coordination migration, which may have obscured the index coverage. No further action is required regarding the suggestion for additional unit tests. ########## tests/unit_tests/commands/deletion_retention/test_prune_audit.py: ########## @@ -0,0 +1,294 @@ +# 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 the pure parts of purge-audit pruning. + +Query behavior against real rows (streak survivors, batching, protection +invariants) is covered by +``tests/integration_tests/deletion_retention/prune_audit_tests.py``. +""" + +from __future__ import annotations + +from contextlib import contextmanager +from datetime import datetime +from typing import Any, Iterator +from unittest.mock import patch + +import pytest +import sqlalchemy as sa +from flask import current_app +from sqlalchemy.dialects import mysql + +from superset.commands.deletion_retention import prune_audit +from superset.commands.deletion_retention.prune_audit import ( + EVIDENCE_RETENTION_KEY, + OPERATIONAL_RETENTION_KEY, + OPERATIONAL_STATUSES, + PROTECTED_STATUSES, + PruneRunResult, + resolve_evidence_retention_days, + resolve_operational_retention_days, +) +from superset.models.purge_audit_log import ( + ALL_STATUSES, + STATUS_BLOCKED, + STATUS_CONFIRMED, + STATUS_FAILED, + STATUS_PENDING, + STATUS_TARGET_ABSENT, +) + +_METRIC_PREFIX: str = "deletion_retention.prune_purge_audit" + + +@contextmanager +def without_config(key: str) -> Iterator[None]: + """Remove a config key for the duration of the block, then restore it. + + ``patch.dict`` cannot express key *removal*, and the unit-test app + fixture is module-scoped, so a bare ``pop`` would leak to later tests. + """ + with patch.dict(current_app.config): + current_app.config.pop(key, None) + yield + + +def test_retention_categories_partition_every_status() -> None: + """Require every status to belong to a retention category. + + A new status added to the model without a category would silently never + be pruned; this assertion catches that omission. + """ + assert OPERATIONAL_STATUSES == {STATUS_BLOCKED, STATUS_FAILED} + assert PROTECTED_STATUSES == {STATUS_CONFIRMED, STATUS_TARGET_ABSENT} + assert not OPERATIONAL_STATUSES & PROTECTED_STATUSES + assert OPERATIONAL_STATUSES | PROTECTED_STATUSES | {STATUS_PENDING} == ALL_STATUSES + + +def test_only_proof_of_destruction_breaks_a_blockage_streak() -> None: + """Require destruction evidence to break a blockage streak. + + A failed attempt is an infrastructure outcome, and pending is provisional. + Neither proves that the blockage cleared. + """ + assert prune_audit._STREAK_BREAKING_STATUSES == { + STATUS_CONFIRMED, + STATUS_TARGET_ABSENT, + } + assert STATUS_FAILED not in prune_audit._STREAK_BREAKING_STATUSES + assert STATUS_PENDING not in prune_audit._STREAK_BREAKING_STATUSES + + +def test_operational_retention_defaults_to_ninety_days() -> None: + """Use the shipped 90-day operational retention default.""" + assert current_app.config[OPERATIONAL_RETENTION_KEY] == 90 + assert resolve_operational_retention_days().days == 90 + + [email protected]("value", [30, 1, 36500]) +def test_operational_retention_accepts_positive_days(value: int) -> None: + with patch.dict(current_app.config, {OPERATIONAL_RETENTION_KEY: value}): + assert resolve_operational_retention_days() == (value, None) + + [email protected]("value", [0, -5, True, False, "ninety", None, 1.5]) +def test_operational_retention_fails_closed_on_invalid_values(value: Any) -> None: + """Disable a category and identify its invalid configuration key.""" + with patch.dict(current_app.config, {OPERATIONAL_RETENTION_KEY: value}): + window: prune_audit.ResolvedWindow = resolve_operational_retention_days() + assert window.days is None + assert window.invalid_key == OPERATIONAL_RETENTION_KEY + + +def test_missing_operational_key_is_reported_as_invalid_not_assumed() -> None: + """A popped key is operator error, not a silent 90-day assumption.""" + with without_config(OPERATIONAL_RETENTION_KEY): + window: prune_audit.ResolvedWindow = resolve_operational_retention_days() + assert window.days is None + assert window.invalid_key == OPERATIONAL_RETENTION_KEY + + +def test_evidence_retention_defaults_to_off_without_warning() -> None: + """Unset is the documented never-expire default (FR-006), not an error: + disabled, no warning, and no key reported as invalid.""" + with without_config(EVIDENCE_RETENTION_KEY): + with patch.object(prune_audit, "logger") as mock_logger: + window: prune_audit.ResolvedWindow = resolve_evidence_retention_days() + mock_logger.warning.assert_not_called() + assert window == (None, None) + + +def test_evidence_retention_accepts_the_explicit_opt_in() -> None: + with patch.dict(current_app.config, {EVIDENCE_RETENTION_KEY: 3650}): + window: prune_audit.ResolvedWindow = resolve_evidence_retention_days() + assert window.days == 3650 + + [email protected]("value", [0, -1, True, "forever"]) +def test_evidence_retention_fails_closed_on_invalid_values(value: Any) -> None: + with patch.dict(current_app.config, {EVIDENCE_RETENTION_KEY: value}): + with patch.object(prune_audit, "logger") as mock_logger: + window: prune_audit.ResolvedWindow = resolve_evidence_retention_days() + mock_logger.warning.assert_called_once() + assert window.days is None + assert window.invalid_key == EVIDENCE_RETENTION_KEY + + +def test_prune_run_result_totals_and_dict_shape() -> None: + result: PruneRunResult = PruneRunResult( + blocked_duplicates=3, operational_expired=2, evidence_expired=1 + ) + assert result.total_removed == 6 + assert result.as_dict() == { + "removed": { + "blocked_duplicates": 3, + "operational_expired": 2, + "evidence_expired": 1, + }, + "carried_over": False, + "invalid_config_keys": [], + } + + +def test_disabled_task_reports_itself_and_removes_nothing() -> None: + """Report a disabled run without reaching the prune implementation.""" + from superset.tasks import deletion_retention as task_module + + with patch.dict(current_app.config, {"PURGE_AUDIT_PRUNING_ENABLED": False}): + with ( + patch.object(task_module, "stats_logger_manager") as mock_stats, + patch.object(task_module, "logger") as mock_logger, + patch.object(task_module.prune_audit, "run_prune") as mock_run, + ): + outcome: dict[str, Any] = task_module.prune_purge_audit() + assert outcome == {"skipped_disabled": 1} + mock_run.assert_not_called() + mock_stats.instance.incr.assert_called_once_with( + f"{_METRIC_PREFIX}.skipped_disabled" + ) + assert mock_logger.info.called + + [email protected]("value", ["false", "0", 1, None]) +def test_non_boolean_master_switch_fails_closed(value: Any) -> None: + """Never interpret truthy strings or numeric values as deletion opt-in.""" + from superset.tasks import deletion_retention as task_module + + with patch.dict(current_app.config, {"PURGE_AUDIT_PRUNING_ENABLED": value}): + with ( + patch.object(task_module, "stats_logger_manager") as mock_stats, + patch.object(task_module.prune_audit, "run_prune") as mock_run, + ): + outcome: dict[str, Any] = task_module.prune_purge_audit() + + assert outcome == {"skipped_invalid_config": 1} + mock_run.assert_not_called() + mock_stats.instance.incr.assert_called_once_with( + f"{_METRIC_PREFIX}.skipped_invalid_config" + ) + + +def test_failed_run_is_isolated_rolled_back_and_distinguishable() -> None: + """Report and isolate a failed pruning run. + + The task rolls back and returns an error marker that cannot be mistaken + for a successful run that removed nothing. + """ + from superset.tasks import deletion_retention as task_module + + with patch.dict(current_app.config, {"PURGE_AUDIT_PRUNING_ENABLED": True}): + with ( + patch.object(task_module, "stats_logger_manager") as mock_stats, + patch.object(task_module, "db") as mock_db, + patch.object( + task_module.prune_audit, + "run_prune", + side_effect=RuntimeError("boom"), + ), + ): + outcome: dict[str, Any] = task_module.prune_purge_audit() + assert outcome == {"error": 1} + mock_db.session.rollback.assert_called_once() + mock_stats.instance.incr.assert_called_once_with(f"{_METRIC_PREFIX}.failed") + + +def test_successful_run_mirrors_counts_and_carryover_into_metrics() -> None: + """Expose category counts and convergence through metrics.""" + from superset.tasks import deletion_retention as task_module + + fake: PruneRunResult = PruneRunResult( + blocked_duplicates=7, + operational_expired=4, + evidence_expired=0, + carried_over=True, + ) Review Comment: <!-- Bito Reply --> The user's decision to maintain the local test setups is reasonable. Given that the tests exercise distinct task outcomes and their patch contexts make the boundaries explicit, extracting a shared fixture would indeed introduce unnecessary indirection without providing a significant maintenance benefit. The current approach is acceptable as it keeps the test logic self-contained and clear. -- 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]
