gkneighb commented on PR #41472:
URL: https://github.com/apache/superset/pull/41472#issuecomment-4931297831
Response to the change request — pushed `0439d6b2b2` (master merged,
hardening applied), but the two headline findings need factual correction:
**1. `_routes_to_soft_delete()` does not always return `False` — the models
do inherit `SoftDeleteMixin` on master.**
Current master (`superset/models/slice.py:67`,
`superset/models/dashboard.py:126`):
```python
class Slice(CoreChart, SoftDeleteMixin, AuditMixinNullable,
ImportExportMixin): ...
class Dashboard(CoreDashboard, SoftDeleteMixin, AuditMixinNullable,
ImportExportMixin): ...
```
The mixin was added by #40128 / #40129 (merged), and this branch has had it
since the 2026-07-06 master merge. The quoted class definitions without the
mixin are from pre-soft-delete code. Empirical check:
`test_delete_chart_soft_delete_reports_restorable` patches only
`is_feature_enabled` and asserts `soft_deleted is True` — it passes (locally
and in CI), which is only possible if the `issubclass` check is truthy.
On replacing the predicate with a bare `is_feature_enabled("SOFT_DELETE")`:
I'd argue against — `BaseDAO.delete` routes on **both** conditions
(`issubclass(model, SoftDeleteMixin) and is_feature_enabled(...)`), so
mirroring it exactly is what keeps the response truthful. A flag-only check
would report `soft_deleted=true` for any model the DAO actually hard-deletes
(e.g. if the mixin were ever conditionally absent), which is the failure mode
this field exists to prevent.
**2. Undelete tools exist — PR #41842.**
`restore_chart` / `restore_dashboard` are open as #41842 (full
implementations, not stubs): they delegate to
`Restore{Chart,Dashboard}Command`, accept ID-or-UUID, and handle not-found /
not-in-trash / forbidden with the same `permission_denied` convention as this
PR. Named `restore_*` to match the REST verb (`POST /{uuid}/restore`); happy to
rename to `undelete_*` for consistency with #40956 if that's the preferred
vocabulary — say the word and I'll align both.
**Also in `0439d6b2b2`** (the legitimate findings from the open threads):
failed delete attempts are now recorded in the audit log (try/except moved
inside `log_context`), `SQLAlchemyError` text no longer reaches the client
(generic message; full exception in server logs), identifier lookup failures
return a structured `LookupFailed` response, and boolean identifiers are
rejected (`identifier: true` would otherwise coerce to ID 1). 17 unit tests, 5
new.
--
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]
