mikebridge commented on code in PR #41549: URL: https://github.com/apache/superset/pull/41549#discussion_r3683125704
########## superset/tasks/deletion_retention.py: ########## @@ -0,0 +1,278 @@ +# 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" +# Batch window for the eligible-id scan (SELECT ... LIMIT): bounds how many +# entities one iteration holds eligible before purging them one at a time. +_BATCH: int = 500 + + +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} + + # Same clock as SoftDeleteMixin.soft_delete(): deleted_at is stamped + # with naive-local datetime.now() (mirroring changed_on per the + # PR #33693 UTC revert), so the cutoff must be naive-local too — a + # UTC-derived cutoff would shift the retention window by the server's + # timezone offset, purging early west of UTC. If deleted_at ever moves + # to UTC-aware, this must move with it. + cutoff = datetime.now() - timedelta(days=window_days) + audit.reconcile_pending() + purged: dict[str, int] = {} Review Comment: Fixed in \`2cc0d92\`. Reconciliation is a durable write, and a dry run is documented as reporting what *would* happen, so it must not resolve another run's audit attempts as a side effect — an operator sizing up a rollout would otherwise change the very record they are inspecting. \`_purge_impl\` now calls \`_reconcile_unless_dry_run(dry_run)\`, and a test asserts \`reconcile_pending\` is not called on the dry-run path. ########## superset/commands/deletion_retention/force_purge.py: ########## @@ -0,0 +1,151 @@ +# 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. +"""Compliance force-purge of a single entity by UUID. + +Immediate, irreversible removal of one entity regardless of the retention +window or whether it is currently soft-deleted or live. Runs the same cascade +as the time-based task with ``enforce_window=False`` — identical dependent +handling with legacy hard-delete semantics: M:N join rows hard-deleted, +a referencing live chart's loose ``datasource_id`` left dangling (the chart is +never modified). Idempotent: a UUID that resolves to nothing is a no-op. +""" + +from __future__ import annotations + +import logging +from typing import Any, cast + +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, + suppress_purge_association_versions, +) +from superset.models.helpers import skip_visibility_filter, SoftDeleteMixin + +logger: logging.Logger = logging.getLogger(__name__) + + +class ForcePurgeCommand: + """Force-purge the entity identified by *uuid*, if any.""" + + def __init__(self, uuid: str, actor: str = "operator") -> None: + self._uuid: str = uuid + self._actor: str = actor + + def _resolve(self) -> SoftDeleteMixin | None: + """Find the entity across every soft-delete model by UUID, matching + live or soft-deleted rows (visibility-filter bypassed).""" + for model in SoftDeleteMixin._registered_subclasses: # noqa: SLF001 + if not hasattr(model, "uuid"): + continue + with skip_visibility_filter(db.session, model): + entity = ( + db.session.query(model).filter(model.uuid == self._uuid).first() + ) + if entity is not None: + return entity Review Comment: Fixed in \`2cc0d92\`, and completed in \`2cdfe31\`. \`_resolve\` now collects matches across every candidate model and raises \`AmbiguousPurgeTargetError\` when more than one matches, rather than returning the first. Callers that already know the type — the REST route in the stacked PR, where authorization was necessarily checked against one specific entity — pass \`model_cls\` so resolution cannot wander. Follow-up in \`2cdfe31\`: the refusal told operators to "pass the entity type to disambiguate" when the CLI had no such option and no handler for the exception, so they got a raw traceback *after* answering the irreversible confirmation prompt. \`force-purge\` now takes \`--type/-t\`. -- 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]
