codeant-ai-for-open-source[bot] commented on code in PR #40130:
URL: https://github.com/apache/superset/pull/40130#discussion_r3510006852
##########
superset/datasets/api.py:
##########
@@ -59,7 +62,11 @@
from superset.daos.dashboard import DashboardDAO
from superset.daos.dataset import DatasetDAO
from superset.databases.filters import DatabaseFilter
Review Comment:
**Suggestion:** This API documentation states deletion is always a soft
delete, but the actual delete path is feature-flagged and can still hard-delete
when `SOFT_DELETE` is disabled, so the description is factually incorrect.
Update the docs to reflect the conditional behavior (or remove the
unconditional claim) to avoid a contract mismatch for clients/operators.
[comment mismatch]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Docs misrepresent destructive delete as recoverable soft delete.
- ⚠️ Operators may assume restores work and lose datasets.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Ensure the `SOFT_DELETE` feature flag is disabled in configuration so
`is_feature_enabled("SOFT_DELETE")` returns `False`. The flag gate is
checked in
`_should_attach_soft_delete_criteria` in `superset/models/helpers.py:23-32`
and in
`BaseDAO.delete` in `superset/daos/base.py:18-29, 520-29, where
`is_feature_enabled("SOFT_DELETE")` controls whether soft or hard delete is
used.
2. Call the dataset deletion endpoint `DELETE /api/v1/dataset/<pk>`, which
is implemented
by `DatasetRestApi.delete` in `superset/datasets/api.py:47-92`. That
method’s OpenAPI
block documents: `summary: Delete a dataset (soft delete; recoverable via
restore)` (line
64) and the docstring explains that the dataset is “soft-deleted” and
“recoverable via
POST /api/v1/dataset/<uuid>/restore”.
3. The `delete` method invokes `DeleteDatasetCommand([pk]).run()`
(superset/commands/dataset/delete.py:41-45). `DeleteDatasetCommand.run`
validates the
models via `DatasetDAO.find_by_ids(self._model_ids)` and then calls
`DatasetDAO.delete(self._models)`, which uses the inherited `BaseDAO.delete`
implementation in `superset/daos/base.py:6-29, 520-29`.
4. In `BaseDAO.delete`, because `cls.model_cls` is `SqlaTable` (bound by
`DatasetDAO` at
superset/daos/dataset.py:52-59) and `issubclass(cls.model_cls,
SoftDeleteMixin)` is true
but `is_feature_enabled("SOFT_DELETE")` is false, the conditional at
base.py:22-29 falls
into the `else` branch and calls `cls.hard_delete(items)`. `hard_delete`
(base.py:247-259)
loops over items and calls `db.session.delete(item)` for each dataset row,
permanently
removing them from the database instead of setting `deleted_at` for soft
delete.
5. After this hard deletion, a client following the documented contract and
calling `POST
/api/v1/dataset/<uuid>/restore` (implemented by `DatasetRestApi.restore` in
`superset/datasets/api.py:511-553`) receives a 404 (`DatasetNotFoundError` at
restore.py:552-555) because the dataset row no longer exists. This
demonstrates that, when
`SOFT_DELETE` is disabled, the runtime behavior is hard delete while the API
documentation
on line 64 still claims a soft delete “recoverable via restore”, creating a
factual
mismatch between docs and behavior.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=b1aeb7fff60e42eba43aae19879a5074&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=b1aeb7fff60e42eba43aae19879a5074&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/datasets/api.py
**Line:** 64:64
**Comment:**
*Comment Mismatch: This API documentation states deletion is always a
soft delete, but the actual delete path is feature-flagged and can still
hard-delete when `SOFT_DELETE` is disabled, so the description is factually
incorrect. Update the docs to reflect the conditional behavior (or remove the
unconditional claim) to avoid a contract mismatch for clients/operators.
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%2F40130&comment_hash=8f955cfb0dc2c29bd89bd159b6cee881a5012d30c0a703067cfff637f3f411a1&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40130&comment_hash=8f955cfb0dc2c29bd89bd159b6cee881a5012d30c0a703067cfff637f3f411a1&reaction=dislike'>👎</a>
##########
superset/connectors/sqla/models.py:
##########
@@ -1303,6 +1304,7 @@ def data(self) -> dict[str, Any]:
class SqlaTable(
CoreDataset,
+ SoftDeleteMixin,
BaseDatasource,
ExploreMixin,
): # pylint: disable=too-many-public-methods
Review Comment:
**Suggestion:** `SoftDeleteMixin` is placed after `CoreDataset` in the
base-class list, which can prevent `SoftDeleteMixin.__init_subclass__()` from
running in Python's MRO unless earlier bases cooperatively call `super()`. If
registration is skipped, `SqlaTable` won’t be added to the soft-delete subclass
registry, so the global visibility listener won’t auto-filter `deleted_at` rows
and soft-deleted datasets can still appear in normal queries. Make
`SoftDeleteMixin` the first base (or otherwise guarantee its
`__init_subclass__` runs) so dataset soft-delete filtering is actually
enforced. [logic error]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
- ❌ Soft-deleted datasets still appear in dataset list API.
- ⚠️ Deleted datasets continue to surface in related-object lookups.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Soft-delete a dataset via `DELETE /api/v1/dataset/<pk>`, which is
implemented by
`DatasetRestApi.delete` in `superset/datasets/api.py:47-92` and calls
`DeleteDatasetCommand([pk]).run()`
(superset/commands/dataset/delete.py:41-45), ultimately
routing to `DatasetDAO.delete()` which delegates to `BaseDAO.delete()`
(superset/daos/base.py:6-29, 520-29). With `SOFT_DELETE` enabled,
`BaseDAO.delete` detects
`SqlaTable` as a `SoftDeleteMixin` subclass and calls `soft_delete()`,
setting
`deleted_at` on the dataset row.
2. Global soft-delete visibility is enforced by `_add_soft_delete_filter` in
`superset/models/helpers.py:52-123`, which is registered as a
`do_orm_execute` listener
and, when `is_feature_enabled("SOFT_DELETE")` is true
(`_should_attach_soft_delete_criteria` in helpers.py:2-32), iterates
`_all_soft_delete_subclasses()` (helpers.py:35-49) to attach
`with_loader_criteria(cls,
lambda c: c.deleted_at.is_(None), include_aliases=True)` for each registered
`SoftDeleteMixin` subclass.
3. The subclass registry used by `_all_soft_delete_subclasses()` is
populated exclusively
via `SoftDeleteMixin.__init_subclass__` in
`superset/models/helpers.py:194-203`, which
appends each concrete subclass to `SoftDeleteMixin._registered_subclasses`
when
`__init_subclass__` runs. For `SqlaTable`, declared as `class
SqlaTable(CoreDataset,
SoftDeleteMixin, BaseDatasource, ExploreMixin)` in
`superset/connectors/sqla/models.py:106-111`, Python resolves
`__init_subclass__` via the
MRO: if the leftmost external base `CoreDataset` (imported from
`superset_core.common.models.Dataset` at models.py:71) defines its own
`__init_subclass__`
and does not call `super().__init_subclass__`,
`SoftDeleteMixin.__init_subclass__` is
never invoked, leaving `SqlaTable` absent from `_registered_subclasses`.
4. Once `SOFT_DELETE` is enabled, repeat a dataset listing call such as `GET
/api/v1/dataset/` (handled by `DatasetRestApi` in
`superset/datasets/api.py`, which uses
`SQLAInterface(SqlaTable)` and the global ORM session). Because
`_add_soft_delete_filter`
only filters classes present in `SoftDeleteMixin._registered_subclasses`, a
missing
`SqlaTable` entry means no `deleted_at IS NULL` criterion is attached for
this model, and
soft-deleted dataset rows (with `deleted_at` set by `soft_delete()`)
continue to appear in
normal queries and list results despite having been “deleted”. This
reproduction path
depends on the non-cooperative `__init_subclass__` implementation in
`CoreDataset`, which
is defined in the external `superset_core.common.models` module and
therefore not visible
in this repo, but the infrastructure code here shows that such an
implementation would
bypass registration for any `SoftDeleteMixin` placed after it in the base
list.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=3aa4c1ad7e274410814ebf666ebd5b05&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=3aa4c1ad7e274410814ebf666ebd5b05&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/connectors/sqla/models.py
**Line:** 1305:1310
**Comment:**
*Logic Error: `SoftDeleteMixin` is placed after `CoreDataset` in the
base-class list, which can prevent `SoftDeleteMixin.__init_subclass__()` from
running in Python's MRO unless earlier bases cooperatively call `super()`. If
registration is skipped, `SqlaTable` won’t be added to the soft-delete subclass
registry, so the global visibility listener won’t auto-filter `deleted_at` rows
and soft-deleted datasets can still appear in normal queries. Make
`SoftDeleteMixin` the first base (or otherwise guarantee its
`__init_subclass__` runs) so dataset soft-delete filtering is actually enforced.
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%2F40130&comment_hash=597a234fa2b3ea0b6a99483ae543aa2221ef831452f86003716b39f6e88be3a3&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40130&comment_hash=597a234fa2b3ea0b6a99483ae543aa2221ef831452f86003716b39f6e88be3a3&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]
