codeant-ai-for-open-source[bot] commented on code in PR #41549: URL: https://github.com/apache/superset/pull/41549#discussion_r3573705227
########## superset/commands/deletion_retention/force_purge.py: ########## @@ -0,0 +1,118 @@ +# 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, + suspend_version_capture, +) +from superset.models.helpers import skip_visibility_filter, SoftDeleteMixin + +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 = uuid + self._actor = 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 + return None + + def run(self) -> dict[str, Any]: + """Resolve + purge. Returns a summary; a no-op when nothing matches.""" + audit.reconcile_pending() + entity = self._resolve() + if entity is None: + logger.info("force_purge: no entity for uuid=%s (no-op)", self._uuid) + return {"purged": False, "reason": "not_found", "uuid": self._uuid} + + entity_type = str(cast(Any, type(entity)).__tablename__) + record_id = audit.write_ahead( + trigger=audit.TRIGGER_FORCE, + actor=self._actor, + entity_type=entity_type, + entity_uuid=self._uuid, + removed_dashboard_slices=dashboard_slice_count(db.session, entity), + ) + try: + with suspend_version_capture(): + result: CascadeResult = cascade_hard_delete( + db.session, entity, enforce_window=False + ) + db.session.commit() + except Exception: + db.session.rollback() + audit.fail(record_id) + raise + if result.purged: + audit.confirm( + record_id, + affected_referrers=result.dangling_chart_uuids, + removed_dashboard_slices=result.removed_dashboard_slices, + ) + elif result.blocked_reason is not None: + audit.block(record_id) + else: + audit.fail(record_id) + logger.info( + "force_purge: purged %s uuid=%s (dangling charts=%d, dashboard_slices=%d)", + result.entity_type, + self._uuid, + len(result.dangling_chart_uuids), + result.removed_dashboard_slices, + ) Review Comment: **Suggestion:** This success log line runs even when nothing was actually purged (for example when deletion is blocked), producing false “purged” audit logs and misleading operators. Log conditionally based on the result state (purged/blocked/not-found) so operational logs match real outcomes. [comment mismatch] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ⚠️ Operator logs misreport blocked purges as successful deletions. - ⚠️ Compliance reviews may misread purge history from logs. ``` </details> <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Create a dashboard or chart with associated alerts/reports so `ReportSchedule` rows reference it, matching the guard logic in `superset/commands/deletion_retention/purge_cascade.py:229-249` where `_validate_deletion_allowed()` raises `PurgeBlockedError` if a `ReportSchedule` exists for the entity. 2. Note the UUID of that dashboard or chart (the entity models use `SoftDeleteMixin` and expose a `uuid` attribute as resolved in `ForcePurgeCommand._resolve()` at `superset/commands/deletion_retention/force_purge.py:52-64`), and run the operator CLI `superset deletion_retention force-purge --uuid <uuid>` which is wired in `superset/cli/deletion_retention.py:70-79` and constructs `ForcePurgeCommand(uuid)` at line 81. 3. During `ForcePurgeCommand.run()` in `superset/commands/deletion_retention/force_purge.py:66-88`, the code calls `cascade_hard_delete(db.session, entity, enforce_window=False)` (implementation in `superset/commands/deletion_retention/purge_cascade.py:122-219`); `_validate_deletion_allowed()` at lines 229-249 detects the associated `ReportSchedule` and raises `PurgeBlockedError`, which is caught at lines 199-210, returning `CascadeResult(purged=False, blocked_reason=str(ex), ...)`. 4. Back in `ForcePurgeCommand.run()` at `superset/commands/deletion_retention/force_purge.py:92-101`, the command correctly calls `audit.block(record_id)` when `result.blocked_reason` is not `None`, but then unconditionally executes the `logger.info(...)` block at lines 102-108, logging the message `"force_purge: purged %s uuid=%s ..."` even though `result.purged` is `False`, so operator-facing logs report a successful purge for an entity whose purge was actually blocked. ``` </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=b5925f1fd22b44d3ad2b17db1a891881&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=b5925f1fd22b44d3ad2b17db1a891881&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/commands/deletion_retention/force_purge.py **Line:** 102:108 **Comment:** *Comment Mismatch: This success log line runs even when nothing was actually purged (for example when deletion is blocked), producing false “purged” audit logs and misleading operators. Log conditionally based on the result state (purged/blocked/not-found) so operational logs match real outcomes. 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%2F41549&comment_hash=87efc7592a161c6aa5f95fe913bf399d8a451bd1821acfd72234b8a8044441ba&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41549&comment_hash=87efc7592a161c6aa5f95fe913bf399d8a451bd1821acfd72234b8a8044441ba&reaction=dislike'>👎</a> ########## superset/commands/deletion_retention/window.py: ########## @@ -0,0 +1,63 @@ +# 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. +"""Resolve the soft-delete retention window.""" + +from __future__ import annotations + +import logging + +from flask import current_app + +from superset.key_value.shared_entries import get_shared_value +from superset.key_value.types import SharedKey + +logger = logging.getLogger(__name__) + +_DEFAULT_RETENTION_DAYS = 30 + + +def resolve_retention_window() -> int: + """Return the retention window in days, read live on each call. + + Resolution order: + + 1. The per-workspace value persisted under + ``SharedKey.SOFT_DELETE_RETENTION_DAYS`` (read live; takes + precedence when present). + 2. Otherwise the ``SUPERSET_SOFT_DELETE_RETENTION_DAYS`` config / + environment seed default (itself defaulting to 30). + + ``0`` from either source is a meaningful "disable", so the shared + value is selected with an explicit ``is None`` check — never ``or``, + which would treat ``0`` as unset. A malformed shared value is + rejected (logged) and the fallback is used rather than crashing the + scheduled task. + """ + if (shared := get_shared_value(SharedKey.SOFT_DELETE_RETENTION_DAYS)) is not None: + if isinstance(shared, bool) or not isinstance(shared, int) or shared < 0: + logger.warning( + "deletion_retention: ignoring malformed shared retention value %r; " + "falling back to config", + shared, + ) + else: + return shared + return int( + current_app.config.get( + "SUPERSET_SOFT_DELETE_RETENTION_DAYS", _DEFAULT_RETENTION_DAYS + ) + ) Review Comment: **Suggestion:** The config fallback is cast with a bare `int(...)` and no error handling, so a non-numeric value in `SUPERSET_SOFT_DELETE_RETENTION_DAYS` will raise `ValueError` and break callers like `show_window` (and force the Celery task into repeated error runs). Validate and guard this conversion the same way malformed shared values are handled, then fall back to the default instead of raising. [type error] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ❌ Celery purge_soft_deleted crashes on malformed retention config. - ⚠️ CLI show-window fails, confusing operations and diagnostics. ``` </details> <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Configure the Flask app such that `current_app.config["SUPERSET_SOFT_DELETE_RETENTION_DAYS"]` is a non-numeric value (e.g. the environment variable or config sets `"thirty"`), which is the value read in the fallback at `superset/commands/deletion_retention/window.py:59-62`. 2. Ensure there is no shared override set for `SharedKey.SOFT_DELETE_RETENTION_DAYS` (i.e. do not call the `set_window` CLI, or clear it), so `get_shared_value(SharedKey.SOFT_DELETE_RETENTION_DAYS)` returns `None` and `resolve_retention_window()` at `window.py:33-58` falls through to the config path and executes the `return int(current_app.config.get(...))` statement at lines 59-63. 3. Trigger window resolution via the operator CLI by running `superset deletion_retention show-window`, which is implemented in `superset/cli/deletion_retention.py:59-67` and imports `resolve_retention_window` at line 63, then calls it at line 65 to compute `days`. 4. Alternatively, let the Celery beat task `deletion_retention.purge_soft_deleted` run (entrypoint in `superset/tasks/deletion_retention.py:229-247`), which calls `window_days = resolve_retention_window()` at line 240 outside its own `try` block; in both CLI and task cases, when `resolve_retention_window()` executes `int(current_app.config.get(...))` with a non-numeric string, Python raises `ValueError`, causing `show_window` to crash and the Celery task `purge_soft_deleted` to fail before reaching its internal error handling, resulting in repeated task errors whenever the malformed config is present. ``` </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=e604880821e24d17b62d0d3c4e7effe9&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=e604880821e24d17b62d0d3c4e7effe9&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/commands/deletion_retention/window.py **Line:** 59:63 **Comment:** *Type Error: The config fallback is cast with a bare `int(...)` and no error handling, so a non-numeric value in `SUPERSET_SOFT_DELETE_RETENTION_DAYS` will raise `ValueError` and break callers like `show_window` (and force the Celery task into repeated error runs). Validate and guard this conversion the same way malformed shared values are handled, then fall back to the default instead of raising. 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%2F41549&comment_hash=b21d96059da262b453cb02b6d6c32db5d8b7446a46790f1a0eb9cc76b3923cb8&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41549&comment_hash=b21d96059da262b453cb02b6d6c32db5d8b7446a46790f1a0eb9cc76b3923cb8&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]
