codeant-ai-for-open-source[bot] commented on code in PR #41549: URL: https://github.com/apache/superset/pull/41549#discussion_r3655572968
########## 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: **Suggestion:** The fixed database name is shared by every test instance. When these integration tests run in parallel, concurrent setup/cleanup can delete or collide with another test's database, causing nondeterministic failures and cross-test data corruption. Generate a unique database name per test instance or serialize these tests. [race condition] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ❌ Parallel retention tests can fail nondeterministically. - ⚠️ Shared database records can corrupt test isolation. - ⚠️ CI workers may produce order-dependent results. ``` </details> <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Run multiple deletion-retention integration test instances concurrently, for example through the test runner's parallel workers; each instance constructs `DeletionRetentionTestBase` from `tests/integration_tests/deletion_retention/_base.py:47`. 2. Each instance executes `setUp()` at `tests/integration_tests/deletion_retention/_base.py:50`, calls `_cleanup()` at line 51, and then creates a `Database` named `retention_it_db` at lines 53-55. 3. Both workers attempt to insert the same database name into the shared Superset metadata database, or one worker cleans up rows created by the other before its test completes. 4. Observe database uniqueness errors, ambiguous database lookups, or nondeterministic test failures caused by the shared `retention_it_db` record. ``` </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=633c08b2c4e0472d8876b5a4d6b41eb3&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=633c08b2c4e0472d8876b5a4d6b41eb3&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:** tests/integration_tests/deletion_retention/_base.py **Line:** 53:55 **Comment:** *Race Condition: The fixed database name is shared by every test instance. When these integration tests run in parallel, concurrent setup/cleanup can delete or collide with another test's database, causing nondeterministic failures and cross-test data corruption. Generate a unique database name per test instance or serialize these tests. 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=e74a9eb180be5d02a23f992f950641a84396ada41979ae6a172cc582e9feb33f&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41549&comment_hash=e74a9eb180be5d02a23f992f950641a84396ada41979ae6a172cc582e9feb33f&reaction=dislike'>👎</a> ########## 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: **Suggestion:** The cleanup routine removes every entity whose name starts with the global `_PREFIX`, rather than only rows created by the current test. Parallel workers or another test process using this suite can therefore have its entities deleted during setup or teardown, making test outcomes order-dependent. Scope cleanup to identifiers owned by the current test or use a unique prefix per test instance. [race condition] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ❌ Concurrent tests can delete each other's fixtures. - ⚠️ Retention assertions become order-dependent. - ⚠️ Parallel CI results may be nondeterministic. ``` </details> <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Run two deletion-retention test instances concurrently; each test uses `setUp()` and `tearDown()` from `tests/integration_tests/deletion_retention/_base.py:47-62`. 2. During setup or teardown, `_cleanup()` at `tests/integration_tests/deletion_retention/_base.py:166` queries all dashboards, charts, and datasets while bypassing the soft-delete visibility filter at lines 180-185. 3. For every returned entity, the cleanup code derives its name at lines 187-191 and deletes it when the name starts with the global `_PREFIX` at line 192. 4. If one worker has created a `retention_it_*` entity, the other worker's setup or teardown also matches that entity and deletes it at lines 193-198, causing the first test to observe missing fixtures or altered state. ``` </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=381f245370b1409eaa27e9d8c160a3f9&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=381f245370b1409eaa27e9d8c160a3f9&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:** tests/integration_tests/deletion_retention/_base.py **Line:** 192:192 **Comment:** *Race Condition: The cleanup routine removes every entity whose name starts with the global `_PREFIX`, rather than only rows created by the current test. Parallel workers or another test process using this suite can therefore have its entities deleted during setup or teardown, making test outcomes order-dependent. Scope cleanup to identifiers owned by the current test or use a unique prefix per test instance. 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=9eba75fc44e56256991b9f59cb7c72dba4f05d865e2972d05e750e17690cade0&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41549&comment_hash=9eba75fc44e56256991b9f59cb7c72dba4f05d865e2972d05e750e17690cade0&reaction=dislike'>👎</a> ########## 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: **Suggestion:** The new Click group is defined but this file does not register `deletion_retention` with Superset's root CLI command group. Unless another changed file performs that registration, `superset deletion-retention ...` will not be discoverable or executable. Register this group in the application's CLI command registry. [possible bug] <details> <summary><b>Severity Level:</b> Critical 🚨</summary> ```mdx - ❌ Documented deletion-retention CLI commands are unavailable. - ❌ Operators cannot inspect or change retention windows. - ❌ Operators cannot invoke force-purge by UUID. ``` </details> <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Build or install the PR version; the new command group is defined at `superset/cli/deletion_retention.py:34-36`. 2. Run the documented command `superset deletion-retention show-window`; the command entry point resolves through Superset's root CLI registry, not merely through the standalone function definition. 3. No registration of `deletion_retention` is present in `superset/cli/deletion_retention.py`, and the provided changed-file set contains no root-CLI registration. 4. Observe that Click cannot discover the group and reports `No such command 'deletion-retention'`, preventing `set-window`, `show-window`, and `force-purge` from executing. ``` </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=c70fba4d88974a4b91fd57d8a884753e&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=c70fba4d88974a4b91fd57d8a884753e&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/cli/deletion_retention.py **Line:** 34:36 **Comment:** *Possible Bug: The new Click group is defined but this file does not register `deletion_retention` with Superset's root CLI command group. Unless another changed file performs that registration, `superset deletion-retention ...` will not be discoverable or executable. Register this group in the application's CLI command registry. 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=d1cca37922f6e7704c0fccdbd06f6513e6ee6d37f548286c35529b45b22be293&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41549&comment_hash=d1cca37922f6e7704c0fccdbd06f6513e6ee6d37f548286c35529b45b22be293&reaction=dislike'>👎</a> ########## 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).""" + + +@deletion_retention.command() +@with_appcontext [email protected]( + "--days", + "-d", + required=True, + type=int, + help="Retention window in days; 0 disables.", +) +def set_window(days: int) -> None: + """Set the per-workspace retention window (SharedKey, upsert).""" + from superset.key_value.shared_entries import upsert_shared_value + from superset.key_value.types import SharedKey + + if days < 0: + raise click.BadParameter("--days must be >= 0") + upsert_shared_value(SharedKey.SOFT_DELETE_RETENTION_DAYS, days) Review Comment: **Suggestion:** This writes the retention value under one global `SharedKey` without supplying any workspace identifier or workspace-scoped context, so successive invocations overwrite the same value and `set-window` cannot maintain distinct per-workspace overrides as documented. Persist and resolve the value with the workspace scope used by the purge task. [logic error] <details> <summary><b>Severity Level:</b> Critical 🚨</summary> ```mdx - ❌ Multiple workspaces share one retention override. - ⚠️ Purge timing can change after another workspace updates settings. - ❌ Workspace-specific retention policy is not enforceable. ``` </details> <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Invoke `set-window` through `set_window()` at `superset/cli/deletion_retention.py:48` for workspace A; line 55 persists only `SharedKey.SOFT_DELETE_RETENTION_DAYS` and passes no workspace identifier. 2. Invoke the same command for workspace B with a different `--days` value; the second call upserts the same shared key rather than a workspace-qualified key. 3. Call `resolve_retention_window()` from `superset/commands/deletion_retention/window.py:55`; its lookup at line 72 reads that single shared value. 4. Observe that the value returned for the purge pipeline is the most recently stored value, so workspace A and workspace B cannot retain independent overrides. ``` </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=07c93ca501bc43458c3d7c2c074c885b&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=07c93ca501bc43458c3d7c2c074c885b&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/cli/deletion_retention.py **Line:** 55:55 **Comment:** *Logic Error: This writes the retention value under one global `SharedKey` without supplying any workspace identifier or workspace-scoped context, so successive invocations overwrite the same value and `set-window` cannot maintain distinct per-workspace overrides as documented. Persist and resolve the value with the workspace scope used by the purge task. 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=9056103e641141431b1d6df5af52796b9866902db4ac6b831c3a8459f47efdf3&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41549&comment_hash=9056103e641141431b1d6df5af52796b9866902db4ac6b831c3a8459f47efdf3&reaction=dislike'>👎</a> ########## superset/commands/deletion_retention/window.py: ########## @@ -0,0 +1,81 @@ +# 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.Logger = logging.getLogger(__name__) + +_DEFAULT_RETENTION_DAYS: int = 30 + + +def _config_retention_days() -> int: + """Return a validated config fallback without breaking scheduled runs.""" + configured = current_app.config.get( + "SUPERSET_SOFT_DELETE_RETENTION_DAYS", _DEFAULT_RETENTION_DAYS + ) + try: + if isinstance(configured, bool): + raise ValueError + days = int(configured) + if days < 0: + raise ValueError + return days + except (TypeError, ValueError): + logger.warning( + "deletion_retention: ignoring malformed config retention value %r; " + "falling back to %d days", + configured, + _DEFAULT_RETENTION_DAYS, + ) + return _DEFAULT_RETENTION_DAYS + + +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: Review Comment: **Suggestion:** The resolver reads the retention value from a single unscoped shared key, so scheduled execution cannot select a different override for each workspace. This contradicts the documented per-workspace behavior and can cause one workspace's retention setting to control purges for another. [logic error] <details> <summary><b>Severity Level:</b> Critical 🚨</summary> ```mdx - ❌ Scheduled purges cannot apply workspace-specific windows. - ⚠️ One workspace's setting affects other workspaces. - ❌ Retention policy isolation is lost during entity purging. ``` </details> <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Configure different retention values through `set_window()` at `superset/cli/deletion_retention.py:48-55`; both invocations write the same `SharedKey.SOFT_DELETE_RETENTION_DAYS`. 2. Run the retention resolution path, which calls `resolve_retention_window()` at `superset/commands/deletion_retention/window.py:55`. 3. The resolver executes `get_shared_value(SharedKey.SOFT_DELETE_RETENTION_DAYS)` at line 72 without receiving or selecting a workspace context. 4. Observe that every purge execution receives the same shared value at line 80, or the same config fallback, instead of a value associated with the workspace being purged. ``` </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=8c2f36a8358941edb908c896d2edad7a&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=8c2f36a8358941edb908c896d2edad7a&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:** 72:72 **Comment:** *Logic Error: The resolver reads the retention value from a single unscoped shared key, so scheduled execution cannot select a different override for each workspace. This contradicts the documented per-workspace behavior and can cause one workspace's retention setting to control purges for another. 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=fed43c9539aba6c9e77fe33103ca48d865beece67c341d9a75e3749836533eca&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41549&comment_hash=fed43c9539aba6c9e77fe33103ca48d865beece67c341d9a75e3749836533eca&reaction=dislike'>👎</a> ########## superset/commands/deletion_retention/audit.py: ########## @@ -0,0 +1,225 @@ +# 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. +"""Write-ahead purge audit record. + +Every purge — time-based or force — writes an immutable record that +**survives** the entity it names, on a **dedicated session** outside the +purge transaction so it neither entangles with the ``DBEventLogger`` +(which shares ``db.session`` and commits mid-request) nor vanishes if the +purge rolls back. The record is written ``pending`` *before* the purge and +flipped to ``confirmed`` *after* it commits, so a crash leaves at most a +``pending`` row, never a missing one. ``pending`` rows are reconciled on the +next run (the purge is convergent). + +The dedicated ``purge_audit_log`` table is content-free (no name or PII; only +action, actor, UTC time, entity type, UUID, and affected referrers) and is never +removed by the purge cascade. +""" + +from __future__ import annotations + +import logging +from datetime import datetime, timedelta +from typing import Any, cast +from uuid import UUID, uuid4 + +import sqlalchemy as sa +from sqlalchemy import Column, DateTime, Integer, String, Text +from sqlalchemy.orm import Session, sessionmaker +from sqlalchemy_utils import UUIDType + +from superset import db + +logger: logging.Logger = logging.getLogger(__name__) + + +def _dedicated_session() -> Session: + """A fresh session on its own connection, independent of the request / + task ``db.session``. The audit write must commit on its own so it survives + a rolled-back or crashed purge.""" + return sessionmaker(bind=db.engine)() + + +STATUS_PENDING = "pending" +STATUS_CONFIRMED = "confirmed" +STATUS_FAILED = "failed" +STATUS_BLOCKED = "blocked" + +_PENDING_STALE_AFTER = timedelta(hours=1) + +TRIGGER_RETENTION = "retention" +TRIGGER_FORCE = "force" + +ACTOR_SYSTEM = "system" + + +class PurgeAuditLog(db.Model): + """Immutable, content-free record of a purge.""" + + __tablename__ = "purge_audit_log" + + id = Column(UUIDType(binary=True), primary_key=True, default=uuid4) + status = Column(String(16), nullable=False, default=STATUS_PENDING) + trigger = Column(String(16), nullable=False) + actor = Column(String(256), nullable=False) + entity_type = Column(String(64), nullable=False) + entity_uuid = Column(String(36), nullable=True, index=True) + # Comma-joined UUIDs of charts left dangling / dashboards that lost a join + # row (force-purge visibility). Free text, content-free. + affected_referrers = Column(Text, nullable=True) + removed_dashboard_slices = Column(Integer, nullable=False, default=0) + created_on = Column(DateTime, nullable=False) + confirmed_on = Column(DateTime, nullable=True) + + +def write_ahead( + *, + trigger: str, + actor: str, + entity_type: str, + entity_uuid: str | None, + removed_dashboard_slices: int = 0, +) -> UUID | None: + """Insert a ``pending`` audit row on a dedicated session, before the + purge runs. Returns the row id to confirm later, or ``None`` if the audit + write itself fails (which must not block the purge).""" + session = _dedicated_session() + try: + record = PurgeAuditLog( + status=STATUS_PENDING, + trigger=trigger, + actor=actor, + entity_type=entity_type, + entity_uuid=entity_uuid, + removed_dashboard_slices=removed_dashboard_slices, + created_on=datetime.utcnow(), + ) + session.add(record) + session.commit() + return cast(UUID, record.id) + except Exception: # pylint: disable=broad-except + session.rollback() + logger.warning( + "deletion_retention: failed to write pending audit row", exc_info=True + ) + return None Review Comment: **Suggestion:** Returning `None` after an audit write failure allows the purge to continue without any durable write-ahead record, contradicting the guarantee that every purge is auditable. A database outage or unavailable audit table can therefore permanently erase an entity without an audit attempt; fail the purge when the required audit record cannot be written, or provide an equivalent durable retry mechanism. [security] <details> <summary><b>Severity Level:</b> Critical 🚨</summary> ```mdx - ❌ Retention purge can delete entities without audit evidence. - ⚠️ Operators cannot reconcile unaudited permanent deletions. - ⚠️ Audit failures are only logged and do not stop purging. ``` </details> <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Run the scheduled Celery entry point `purge_soft_deleted()` at `superset/tasks/deletion_retention.py:240`; with `SOFT_DELETE` enabled and dry-run disabled, it calls `_purge_impl()` at line 254. 2. `_purge_impl()` selects eligible soft-deleted entities and `_purge_one()` at `superset/tasks/deletion_retention.py:191` calls `audit.write_ahead()` at line 205 before deleting each entity. 3. Make the audit insert fail, for example by running against a database where the `purge_audit_log` migration is unavailable or the dedicated audit connection cannot commit. `write_ahead()` catches the exception at `superset/commands/deletion_retention/audit.py:114`, logs it, and returns `None` at line 119. 4. `_purge_one()` continues with `cascade_hard_delete()` at `superset/tasks/deletion_retention.py:219`, commits the entity deletion at line 220, and later `audit.confirm(None, ...)` becomes a no-op through `finalize()` at `audit.py:124`, leaving no audit attempt for the permanent deletion. ``` </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=8238ee92ee6e4da7a81a2ffc6fd6ca53&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=8238ee92ee6e4da7a81a2ffc6fd6ca53&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/audit.py **Line:** 114:119 **Comment:** *Security: Returning `None` after an audit write failure allows the purge to continue without any durable write-ahead record, contradicting the guarantee that every purge is auditable. A database outage or unavailable audit table can therefore permanently erase an entity without an audit attempt; fail the purge when the required audit record cannot be written, or provide an equivalent durable retry mechanism. 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=767e48884570417cd5121e526aa56359619a709ddfa59ce73ef3d68f8dafc548&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41549&comment_hash=767e48884570417cd5121e526aa56359619a709ddfa59ce73ef3d68f8dafc548&reaction=dislike'>👎</a> ########## superset/commands/deletion_retention/audit.py: ########## @@ -0,0 +1,225 @@ +# 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. +"""Write-ahead purge audit record. + +Every purge — time-based or force — writes an immutable record that +**survives** the entity it names, on a **dedicated session** outside the +purge transaction so it neither entangles with the ``DBEventLogger`` +(which shares ``db.session`` and commits mid-request) nor vanishes if the +purge rolls back. The record is written ``pending`` *before* the purge and +flipped to ``confirmed`` *after* it commits, so a crash leaves at most a +``pending`` row, never a missing one. ``pending`` rows are reconciled on the +next run (the purge is convergent). + +The dedicated ``purge_audit_log`` table is content-free (no name or PII; only +action, actor, UTC time, entity type, UUID, and affected referrers) and is never +removed by the purge cascade. +""" + +from __future__ import annotations + +import logging +from datetime import datetime, timedelta +from typing import Any, cast +from uuid import UUID, uuid4 + +import sqlalchemy as sa +from sqlalchemy import Column, DateTime, Integer, String, Text +from sqlalchemy.orm import Session, sessionmaker +from sqlalchemy_utils import UUIDType + +from superset import db + +logger: logging.Logger = logging.getLogger(__name__) + + +def _dedicated_session() -> Session: + """A fresh session on its own connection, independent of the request / + task ``db.session``. The audit write must commit on its own so it survives + a rolled-back or crashed purge.""" + return sessionmaker(bind=db.engine)() + + +STATUS_PENDING = "pending" +STATUS_CONFIRMED = "confirmed" +STATUS_FAILED = "failed" +STATUS_BLOCKED = "blocked" + +_PENDING_STALE_AFTER = timedelta(hours=1) + +TRIGGER_RETENTION = "retention" +TRIGGER_FORCE = "force" + +ACTOR_SYSTEM = "system" + + +class PurgeAuditLog(db.Model): + """Immutable, content-free record of a purge.""" + + __tablename__ = "purge_audit_log" + + id = Column(UUIDType(binary=True), primary_key=True, default=uuid4) + status = Column(String(16), nullable=False, default=STATUS_PENDING) + trigger = Column(String(16), nullable=False) + actor = Column(String(256), nullable=False) + entity_type = Column(String(64), nullable=False) + entity_uuid = Column(String(36), nullable=True, index=True) + # Comma-joined UUIDs of charts left dangling / dashboards that lost a join + # row (force-purge visibility). Free text, content-free. + affected_referrers = Column(Text, nullable=True) + removed_dashboard_slices = Column(Integer, nullable=False, default=0) + created_on = Column(DateTime, nullable=False) + confirmed_on = Column(DateTime, nullable=True) + + +def write_ahead( + *, + trigger: str, + actor: str, + entity_type: str, + entity_uuid: str | None, + removed_dashboard_slices: int = 0, +) -> UUID | None: + """Insert a ``pending`` audit row on a dedicated session, before the + purge runs. Returns the row id to confirm later, or ``None`` if the audit + write itself fails (which must not block the purge).""" + session = _dedicated_session() + try: + record = PurgeAuditLog( + status=STATUS_PENDING, + trigger=trigger, + actor=actor, + entity_type=entity_type, + entity_uuid=entity_uuid, + removed_dashboard_slices=removed_dashboard_slices, + created_on=datetime.utcnow(), + ) + session.add(record) + session.commit() + return cast(UUID, record.id) + except Exception: # pylint: disable=broad-except + session.rollback() + logger.warning( + "deletion_retention: failed to write pending audit row", exc_info=True + ) + return None + finally: + session.close() + + +def finalize(record_id: UUID | None, status: str, **details: Any) -> None: + """Finalize a pending attempt on the dedicated audit session.""" + if record_id is None: + return + session = _dedicated_session() + try: + record = session.get(PurgeAuditLog, record_id) + if record is None: + return + record.status = status + if status == STATUS_CONFIRMED: + record.confirmed_on = datetime.utcnow() Review Comment: **Suggestion:** `finalize` can overwrite any existing audit status without checking that the row is still `pending`. A delayed confirmation from a previous worker can therefore change a reconciled `failed`, `blocked`, or already `confirmed` record, making the immutable audit history inaccurate. Update only rows currently in the pending state and treat a status transition conflict as a no-op or error. [race condition] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ⚠️ Concurrent workers can produce contradictory purge outcomes. - ⚠️ Audit status no longer reliably reflects transaction history. - ⚠️ Reconciliation results can be overwritten by delayed workers. ``` </details> <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Start a retention purge through `purge_soft_deleted()` at `superset/tasks/deletion_retention.py:240`; `_purge_one()` writes a pending audit row at `superset/tasks/deletion_retention.py:205`. 2. Allow the purge process to stop after the entity transaction commits but before `audit.confirm()` at `superset/tasks/deletion_retention.py:227-232`, leaving the audit row pending. 3. On a later scheduled run, `audit.reconcile_pending()` at `superset/tasks/deletion_retention.py:118` processes the stale row and sets it to `confirmed` or `failed` at `superset/commands/deletion_retention/audit.py:207-214`. 4. If the delayed original worker resumes and calls `finalize()` at `audit.py:124`, the assignment at line 133 changes the already-finalized status without checking for `STATUS_PENDING`, allowing stale work to overwrite the reconciliation result. ``` </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=837868f27ca241a89a4e24d71437bbd9&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=837868f27ca241a89a4e24d71437bbd9&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/audit.py **Line:** 133:135 **Comment:** *Race Condition: `finalize` can overwrite any existing audit status without checking that the row is still `pending`. A delayed confirmation from a previous worker can therefore change a reconciled `failed`, `blocked`, or already `confirmed` record, making the immutable audit history inaccurate. Update only rows currently in the pending state and treat a status transition conflict as a no-op or error. 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=00c44aa57dcb9863f2694de27c89e7112473faf17cc0fb0d653109142101e4f6&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41549&comment_hash=00c44aa57dcb9863f2694de27c89e7112473faf17cc0fb0d653109142101e4f6&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]
