codeant-ai-for-open-source[bot] commented on code in PR #40129: URL: https://github.com/apache/superset/pull/40129#discussion_r3500177838
########## superset/models/slice.py: ########## @@ -67,7 +71,7 @@ class Slice( # pylint: disable=too-many-public-methods - CoreChart, AuditMixinNullable, ImportExportMixin + CoreChart, SoftDeleteMixin, AuditMixinNullable, ImportExportMixin Review Comment: **Suggestion:** Adding `SoftDeleteMixin` here immediately changes all chart deletes to soft-delete through `BaseDAO.delete()` with no feature-flag guard in this branch, so environments expecting hard-delete semantics will silently get different behavior. Gate this rollout behind the `SOFT_DELETE` toggle in the delete path (or defer this inheritance until that guard is present) to avoid shipping unguarded behavior changes. [logic error] <details> <summary><b>Severity Level:</b> Critical 🚨</summary> ```mdx - ❌ Chart DELETE endpoint always soft-deletes charts without gating. - ⚠️ Hard-delete expectations broken; rows remain in slices table. - ⚠️ Soft-deleted charts accumulate, increasing database storage usage. ``` </details> <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Observe that the `Slice` ORM model now inherits `SoftDeleteMixin` in `superset/models/slice.py:73-75`, specifically the base-class line `CoreChart, SoftDeleteMixin, AuditMixinNullable, ImportExportMixin` at line 74 (from the PR hunk and Grep output). 2. Follow the chart delete API entrypoint: `DELETE /api/v1/chart/<pk>` is handled by `delete(self, pk: int)` in `superset/charts/api.py:483-501`, which (per the body at lines ~520-522 in the same file) calls `DeleteChartCommand([pk]).run()`. 3. In `superset/commands/chart/delete.py:16-21`, `DeleteChartCommand.run()` calls `ChartDAO.delete(self._models)`. `ChartDAO` is defined as `class ChartDAO(BaseDAO[Slice]):` in `superset/daos/chart.py:45`, so `BaseDAO`’s generic machinery (`__init_subclass__` in `superset/daos/base.py:45-48`) sets `ChartDAO.model_cls` to `Slice`. 4. In `superset/daos/base.py:523-532`, `BaseDAO.delete()` checks `if cls.model_cls is not None and issubclass(cls.model_cls, SoftDeleteMixin):` (lines 524-539) and, for such models, calls `cls.soft_delete(items)`. Because `Slice` now subclasses `SoftDeleteMixin`, `ChartDAO.delete()` always routes to `BaseDAO.soft_delete()` (lines 493-502), which iterates items and calls `item.soft_delete()`. `SoftDeleteMixin.soft_delete()` in `superset/models/helpers.py:743-748` sets `self.deleted_at = datetime.now()` without removing the row. There is no feature-flag or `SOFT_DELETE` toggle anywhere in the repo (confirmed by `Grep` returning no matches for `SOFT_DELETE` under `/workspace/superset`). As a result, every call to the chart DELETE API now performs a soft delete instead of the previous hard delete, unconditionally changing behaviour for all environments. ``` </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=520b72cb0bd1408aa6e04787963b0154&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=520b72cb0bd1408aa6e04787963b0154&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/models/slice.py **Line:** 74:74 **Comment:** *Logic Error: Adding `SoftDeleteMixin` here immediately changes all chart deletes to soft-delete through `BaseDAO.delete()` with no feature-flag guard in this branch, so environments expecting hard-delete semantics will silently get different behavior. Gate this rollout behind the `SOFT_DELETE` toggle in the delete path (or defer this inheritance until that guard is present) to avoid shipping unguarded behavior changes. 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%2F40129&comment_hash=e2596a7c48e57da59d1c61c635f07fccf4a35e5500f302df078aa743ca786809&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40129&comment_hash=e2596a7c48e57da59d1c61c635f07fccf4a35e5500f302df078aa743ca786809&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]
