codeant-ai-for-open-source[bot] commented on code in PR #41549: URL: https://github.com/apache/superset/pull/41549#discussion_r3658929611
########## superset/cli/deletion_retention.py: ########## @@ -0,0 +1,98 @@ +# 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. +"""Operator CLI for deletion retention. + +``force-purge`` and ``set-window`` are **operator-gated** — they are +protected by deployment/shell access (the ``SECURITY.md`` operator trust +boundary), not Flask-AppBuilder RBAC: a CLI invocation has no ``g.user``, so +there is no ``403`` to enforce. A future REST route would carry real +Admin/workspace-admin RBAC. +""" + +import logging + +import click +from flask.cli import with_appcontext + +logger = logging.getLogger(__name__) + + [email protected]() +def deletion_retention() -> None: + """Manage purge of soft-deleted entities (operator-gated).""" Review Comment: ✅ **Customized review instruction saved!** **Instruction:** > Do not flag Click command groups for missing explicit root-CLI registration when they are defined under superset/cli; superset/cli/main.py auto-discovers and registers them. **Applied to:** - `superset/cli/**` --- 💡 *To manage or update this instruction, visit: [CodeAnt AI Settings](https://app.codeant.ai/org/settings/learnings)* ########## superset/commands/deletion_retention/purge_cascade.py: ########## @@ -0,0 +1,534 @@ +# 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. +"""Shared hard-delete cascade for the purge task and force-purge command. + +A single code path keeps the two surfaces from drifting. Every dependent row +is removed by an explicit ``sa.delete``; +the DB ``ON DELETE CASCADE`` constraints are a backstop only — SQLite does +not enforce FKs unless ``PRAGMA foreign_keys=ON`` and Core bulk DML fires +only DB-level cascades, so relying on cascade silently leaks rows. + +Cascade tiers: + +* **M:N join rows** — hard-deleted for the purged entity, including join + rows owned by *surviving* entities (e.g. a live dashboard's + ``dashboard_slices`` row to a purged chart). The entity on the other side + is never touched except to lose that one relationship row. +* **Owned children** (``delete-orphan``, no independent existence) — a + dataset's columns and metrics, hard-deleted with it. +* **Independently-owned entities** (a dashboard's charts, a chart's dataset) + are **preserved**. A live chart's loose ``datasource_id`` to a purged + dataset is left dangling — legacy hard-delete semantics, no guard. +* **Version history** — the entity's own ``*_version`` shadows and + the ``version_changes`` scoped to them, plus an orphan-sweep of any + ``version_transaction`` left owning zero surviving shadows. Runs + behind a ``has_table`` check so it no-ops when versioning is absent. +""" + +from __future__ import annotations + +import logging +from collections.abc import Iterator +from contextlib import contextmanager +from dataclasses import dataclass, field +from datetime import datetime +from typing import Any + +import sqlalchemy as sa +from sqlalchemy.exc import IntegrityError +from sqlalchemy.orm import Session + +logger: logging.Logger = logging.getLogger(__name__) + + +@contextmanager +def suppress_purge_association_versions(session: Session) -> Iterator[None]: + """Discard only association versions generated by this purge session. + + ``dashboard_slices`` is a Continuum-tracked association table. Its + engine-level listener expects a unit-of-work even for Core deletes and + queues a shadow-table insert for each removed relationship. A purge + removes history rather than creating it, so preserve any pending work + that predates this block and discard only statements queued by the purge. + + This is deliberately session-scoped. Mutating Continuum's process-global + ``options["versioning"]`` would let concurrent requests silently lose + unrelated history while a purge is running. + """ + try: + from sqlalchemy_continuum import versioning_manager + except ImportError: + yield + return + + options = versioning_manager.options + if not (options.get("versioning") or options.get("native_versioning")): + yield + return + + unit_of_work = versioning_manager.unit_of_work(session) + pending_before = len(unit_of_work.pending_statements) + try: + yield + finally: + del unit_of_work.pending_statements[pending_before:] + + +def entity_uuid(entity: Any) -> str | None: + """Return the entity's UUID as a string, or ``None`` if it has none.""" + value = getattr(entity, "uuid", None) + return str(value) if value is not None else None + + +def _version_tables_present(bind: Any) -> bool: + """Whether the version-history tables exist (a testable seam, so the + version cascade no-ops cleanly before versioning is installed).""" + return sa.inspect(bind).has_table("version_transaction") + + +@dataclass +class CascadeResult: + """Outcome of one entity's cascade. + + ``purged`` is False when the conditional entity-row delete matched no + row — the entity was restored between selection and delete or was already + gone. In that case + no dependents are touched. + """ + + purged: bool + entity_type: str + entity_uuid: str | None + dangling_chart_uuids: list[str] = field(default_factory=list) + removed_dashboard_slices: int = 0 + version_rows_removed: int = 0 + blocked_reason: str | None = None + + +class PurgeBlockedError(Exception): + """Raised when ordinary deletion policy forbids purging an entity.""" + + +class PurgeRaceLostError(Exception): + """Raised to roll back dependent cleanup when the entity delete loses.""" + + +def cascade_hard_delete( + session: Session, + entity: Any, + *, + enforce_window: bool, + cutoff: datetime | None = None, +) -> CascadeResult: + """Remove *entity* and everything that depends on it in one transaction. + + The entity row is locked and its eligibility is re-checked before any + dependent state is touched. Ordinary deletion blockers remain + authoritative. All cleanup and the conditional parent delete run inside a + savepoint so a lost race or restrictive foreign key leaves no side effects. + """ + # pylint: disable=import-outside-toplevel + from superset.connectors.sqla.models import SqlaTable + from superset.models.slice import Slice + + if enforce_window and cutoff is None: + raise ValueError("cutoff is required when enforce_window=True") + + model = type(entity) + table = model.__table__ + entity_id = entity.id + uuid = entity_uuid(entity) + entity_type = _USER_FACING_TYPE.get(table.name, table.name) + + dangling_chart_uuids: list[str] = [] + removed_dashboard_slices = 0 + version_rows = 0 + permission_name = _dataset_permission_name(entity) if model is SqlaTable else None + + try: + with session.begin_nested(): + claim = sa.select(table.c.id).where(table.c.id == entity_id) + if enforce_window: + claim = claim.where(table.c.deleted_at.is_not(None)).where( + table.c.deleted_at < cutoff + ) + if session.execute(claim.with_for_update()).scalar_one_or_none() is None: + raise PurgeRaceLostError + + _validate_deletion_allowed(session, model, entity_id) + removed_dashboard_slices = _count_dashboard_slices( + session, model, entity_id + ) + if model is SqlaTable: + dangling_chart_uuids = [ + str(chart_uuid) + for (chart_uuid,) in session.execute( + sa.select(Slice.uuid) + .where(Slice.datasource_id == entity_id) + .where(Slice.datasource_type == "table") + ) + ] + + _delete_m2m_joins(session, model, entity_id) + _delete_owned_children(session, model, entity_id) + version_rows = _delete_version_history(session, entity, entity_id) + + delete_entity = sa.delete(table).where(table.c.id == entity_id) + if enforce_window: + delete_entity = delete_entity.where( + table.c.deleted_at.is_not(None) + ).where(table.c.deleted_at < cutoff) + if session.execute(delete_entity).rowcount == 0: + raise PurgeRaceLostError + + if permission_name is not None: + _cleanup_dataset_permission(session, permission_name, entity_id) + except PurgeRaceLostError: + logger.info( + "deletion_retention: %s id=%s not purged (restored or already gone)", + entity_type, + entity_id, + ) + return CascadeResult(purged=False, entity_type=entity_type, entity_uuid=uuid) + except (PurgeBlockedError, IntegrityError) as ex: + logger.info( + "deletion_retention: %s id=%s blocked by existing deletion rules", + entity_type, + entity_id, + ) + return CascadeResult( + purged=False, + entity_type=entity_type, + entity_uuid=uuid, + blocked_reason=str(ex), + ) Review Comment: ✅ **Customized review instruction saved!** **Instruction:** > In the purge cascade, treat RESTRICT-style IntegrityError foreign-key violations, including those from external or plugin tables, as blocked deletions; preserve the original error text in blocked_reason while allowing non-integrity operational failures to propagate. **Applied to:** - `superset/commands/deletion_retention/purge_cascade.py` --- 💡 *To manage or update this instruction, visit: [CodeAnt AI Settings](https://app.codeant.ai/org/settings/learnings)* ########## tests/integration_tests/deletion_retention/_base.py: ########## @@ -0,0 +1,228 @@ +# 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. +"""Shared base + self-contained builders for deletion-retention tests. + +These tests do not depend on the example datasets — each builds its own +``Database`` + ``SqlaTable`` so they run on a bare (schema-only) test DB. +Everything created is torn down (bypassing the soft-delete visibility +filter) so a leftover soft-deleted row never trips a later test. +""" + +from __future__ import annotations + +from datetime import datetime, timedelta +from typing import Any + +import sqlalchemy as sa + +from superset import db +from superset.connectors.sqla.models import SqlaTable, SqlMetric, TableColumn +from superset.constants import SKIP_VISIBILITY_FILTER_CLASSES +from superset.models.core import Database +from superset.models.dashboard import Dashboard +from superset.models.slice import Slice +from tests.integration_tests.base_tests import SupersetTestCase + +_PREFIX = "retention_it_" + + +def _bypass(model: type[Any]) -> dict[str, Any]: + return {"execution_options": {SKIP_VISIBILITY_FILTER_CLASSES: {model}}} + + +class DeletionRetentionTestBase(SupersetTestCase): + """Builds an isolated database + dataset and cleans up after itself.""" + + def setUp(self) -> None: + super().setUp() + self._cleanup() + self.database: Database = Database( + database_name=f"{_PREFIX}db", sqlalchemy_uri="sqlite://" + ) Review Comment: ✅ **Customized review instruction saved!** **Instruction:** > Do not flag the shared fixed database name as a race condition when these integration tests are intentionally run serially and cleanup is designed to converge on that shared resource. **Applied to:** - `tests/integration_tests/deletion_retention/**` --- 💡 *To manage or update this instruction, visit: [CodeAnt AI Settings](https://app.codeant.ai/org/settings/learnings)* ########## tests/integration_tests/deletion_retention/_base.py: ########## @@ -0,0 +1,228 @@ +# 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. +"""Shared base + self-contained builders for deletion-retention tests. + +These tests do not depend on the example datasets — each builds its own +``Database`` + ``SqlaTable`` so they run on a bare (schema-only) test DB. +Everything created is torn down (bypassing the soft-delete visibility +filter) so a leftover soft-deleted row never trips a later test. +""" + +from __future__ import annotations + +from datetime import datetime, timedelta +from typing import Any + +import sqlalchemy as sa + +from superset import db +from superset.connectors.sqla.models import SqlaTable, SqlMetric, TableColumn +from superset.constants import SKIP_VISIBILITY_FILTER_CLASSES +from superset.models.core import Database +from superset.models.dashboard import Dashboard +from superset.models.slice import Slice +from tests.integration_tests.base_tests import SupersetTestCase + +_PREFIX = "retention_it_" + + +def _bypass(model: type[Any]) -> dict[str, Any]: + return {"execution_options": {SKIP_VISIBILITY_FILTER_CLASSES: {model}}} + + +class DeletionRetentionTestBase(SupersetTestCase): + """Builds an isolated database + dataset and cleans up after itself.""" + + def setUp(self) -> None: + super().setUp() + self._cleanup() + self.database: Database = Database( + database_name=f"{_PREFIX}db", sqlalchemy_uri="sqlite://" + ) + db.session.add(self.database) + db.session.commit() + self.dataset: SqlaTable = self.make_dataset("ds") + + def tearDown(self) -> None: + self._cleanup() + super().tearDown() + + # -- builders ----------------------------------------------------------- + + def make_dataset(self, name: str, with_children: bool = False) -> SqlaTable: + ds = SqlaTable(table_name=f"{_PREFIX}{name}", database=self.database) + db.session.add(ds) + db.session.commit() + if with_children: + db.session.add(TableColumn(column_name=f"{_PREFIX}col", table=ds)) + db.session.add( + SqlMetric( + metric_name=f"{_PREFIX}metric", expression="count(*)", table=ds + ) + ) + db.session.commit() + return ds + + def make_chart(self, name: str, dataset: SqlaTable | None = None) -> Slice: + dataset = dataset or self.dataset + chart = Slice( + slice_name=f"{_PREFIX}{name}", + datasource_type="table", + datasource_id=dataset.id, + viz_type="table", + ) + db.session.add(chart) + db.session.commit() + return chart + + def make_dashboard(self, name: str, slices: list[Slice] | None = None) -> Dashboard: + dash = Dashboard( + dashboard_title=f"{_PREFIX}{name}", + slug=f"{_PREFIX}{name}", + slices=slices or [], + ) + db.session.add(dash) + db.session.commit() + return dash + + def soft_delete(self, entity: Any, days_ago: int) -> None: + """Mark *entity* soft-deleted with a backdated ``deleted_at``.""" + entity.deleted_at = datetime.now() - timedelta(days=days_ago) + db.session.add(entity) + db.session.commit() + + # -- assertions / lookups ---------------------------------------------- + + def exists(self, model: type[Any], entity_id: int) -> bool: + row = ( + db.session.query(model) + .execution_options(**{SKIP_VISIBILITY_FILTER_CLASSES: {model}}) + .filter(model.id == entity_id) + .one_or_none() + ) + return row is not None + + def count(self, sql: str, params: dict[str, Any]) -> int: + return db.session.execute(sa.text(sql), params).scalar() or 0 + + # -- version-history forging ------------------------------------------- + + def forge_version_row(self, model: type[Any], entity_id: int, tx_id: int) -> None: + """Insert a version_transaction + parent shadow + version_changes row + for *entity_id* anchored at *tx_id* (so a purge has history to remove + without needing live capture to be enabled).""" + from superset.versioning.changes import ENTITY_KIND_BY_CLASS_NAME + + shadow = { + Slice: "slices_version", + Dashboard: "dashboards_version", + SqlaTable: "tables_version", + }[model] + kind = ENTITY_KIND_BY_CLASS_NAME[model.__name__] + # The transaction may be shared across entities — insert it once. + exists = db.session.execute( + sa.text("SELECT 1 FROM version_transaction WHERE id = :t"), {"t": tx_id} + ).first() + if not exists: + db.session.execute( + sa.text( + "INSERT INTO version_transaction (id, issued_at) VALUES (:t, :ts)" + ), + {"t": tx_id, "ts": datetime.utcnow()}, + ) + db.session.execute( + sa.text( + f"INSERT INTO {shadow} (id, transaction_id, operation_type) " # noqa: S608 + "VALUES (:i, :t, 0)" + ), + {"i": entity_id, "t": tx_id}, + ) + db.session.execute( + sa.text( + "INSERT INTO version_changes " + "(transaction_id, entity_kind, entity_id, sequence, kind, " + "operation, path) VALUES (:t, :k, :i, 1, 'set', 0, :p)" + ), + {"t": tx_id, "k": kind, "i": entity_id, "p": '["x"]'}, + ) + db.session.commit() + + # -- cleanup ------------------------------------------------------------ + + def _cleanup(self) -> None: + db.session.rollback() + from superset.connectors.sqla.models import RowLevelSecurityFilter + from superset.reports.models import ReportSchedule + + for report in db.session.query(ReportSchedule).filter( + ReportSchedule.name.like(f"{_PREFIX}%") + ): + db.session.delete(report) + for rule in db.session.query(RowLevelSecurityFilter).filter( + RowLevelSecurityFilter.name.like(f"{_PREFIX}%") + ): + db.session.delete(rule) + db.session.commit() + for model in (Dashboard, Slice, SqlaTable): + rows = ( + db.session.query(model) + .execution_options(**{SKIP_VISIBILITY_FILTER_CLASSES: {model}}) + .all() + ) + for row in rows: + name = ( + getattr(row, "slice_name", None) + or getattr(row, "dashboard_title", None) + or getattr(row, "table_name", "") + ) + if str(name).startswith(_PREFIX): Review Comment: ✅ **Customized review instruction saved!** **Instruction:** > In the deletion-retention integration test suite, retain the suite-scoped cleanup prefix for convergence after crashed runs; do not require per-instance prefixes that could leave leaked entities. **Applied to:** - `tests/integration_tests/deletion_retention/**` --- 💡 *To manage or update this instruction, visit: [CodeAnt AI Settings](https://app.codeant.ai/org/settings/learnings)* ########## superset/commands/deletion_retention/force_purge.py: ########## @@ -0,0 +1,147 @@ +# 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 + 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__) + removed_dashboard_slices = dashboard_slice_count(db.session, entity) + # The audit row commits independently. Release the resolving read + # transaction first, then resolve again against post-audit state. + db.session.rollback() # pylint: disable=consider-using-transaction + record_id = audit.write_ahead( + trigger=audit.TRIGGER_FORCE, + actor=self._actor, + entity_type=entity_type, + entity_uuid=self._uuid, + removed_dashboard_slices=removed_dashboard_slices, + ) + entity = self._resolve() Review Comment: ✅ **Customized review instruction saved!** **Instruction:** > For operator-invoked force purges, allow the purge to proceed when the independent audit write fails; treat fail-open behavior as intentional and do not require aborting solely because audit.write_ahead returns None. **Applied to:** - `superset/commands/deletion_retention/**` --- 💡 *To manage or update this instruction, visit: [CodeAnt AI Settings](https://app.codeant.ai/org/settings/learnings)* -- 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]
