mikebridge commented on PR #39977:
URL: https://github.com/apache/superset/pull/39977#issuecomment-4454315239
@richardfogaca thanks for the careful pass. All four addressed in
`5115202a3f` (infrastructure) plus mechanical entity-branch updates. Per-item:
**Item 1 — `find_existing_for_import` side-effect split — fixed.**
You're right that the destructive cleanup happening as a side effect of
"find" is the wrong shape — today's flow is safe only because `can_write` is
checked once at the import command's top level, but a future overwrite-path
permission check would have a quiet bypass via the soft-delete path.
`find_existing_for_import` is now a pure lookup that returns the row as-is
(live OR soft-deleted) or `None`. A new
`clear_soft_deleted_for_import(existing)` does the explicit hard-delete + ORM
flush. Each entity's v1 importer (sc-106189/90/91) now calls them in the order
you suggested: lookup → permission check on the existing row → only then
`clear_soft_deleted_for_import` if the match was soft-deleted. The destructive
op is at the call site, audit-able, and gated by the same ownership check as
the live-overwrite path.
**Item 2 — `commands/restore.py:65-69` `skip_base_filter=True` — fixed.**
Agreed it had no business being there. `BaseRestoreCommand.validate` now
passes only `skip_visibility_filter=True`. The entity's RBAC `base_filter`
stays in effect, matching how `find_by_ids` is called on the existing delete
paths. The `DashboardFilter`/`ChartFilter`/`DatasetFilter` are pure RBAC — they
don't filter on `deleted_at`, so leaving them on is the correct scope.
Ownership is still verified by `raise_for_ownership` immediately after, so the
rejection codes line up with delete's behavior.
Test renamed and tightened:
`test_validate_calls_dao_with_visibility_bypass_only` asserts the DAO call no
longer includes `skip_base_filter`.
**Item 3 — `run()` transaction wrapping contract — fixed (refactored, not
just documented).**
You're right that "subclass overrides `run()` ONLY to add `@transaction`"
was fragile, especially across multiple entity rollouts. The base
`BaseRestoreCommand` now owns the transactional wrapper directly:
```python
class BaseRestoreCommand(BaseCommand, Generic[T]):
dao: ClassVar[Any]
not_found_exc: ClassVar[type[Exception]]
forbidden_exc: ClassVar[type[Exception]]
restore_failed_exc: ClassVar[type[Exception]] # new
def run(self) -> None:
@transaction(on_error=partial(on_error,
reraise=self.restore_failed_exc))
def _perform() -> None:
model = self.validate()
model.restore()
_perform()
```
The wrapper is built at call time so `on_error` can reference the
per-subclass `restore_failed_exc` ClassVar. Concrete entity restore commands
collapse to four ClassVar assignments with no methods — strictly declarative.
Less to remember, no contract to silently skip.
New test `test_run_translates_sqlalchemy_errors_via_restore_failed_exc` pins
the wrapping: a `SQLAlchemyError` raised inside the unit of work is translated
to the subclass's `restore_failed_exc`, so a refactor that drops the wrapper or
points it at the wrong type fails CI.
**Item 4 — `SoftDeleteMixin` docstring nit — fixed.**
`BaseDAO.delete()` now routes through the mixin to `soft_delete()` for
`SoftDeleteMixin` models; `BaseDAO.hard_delete()` is the permanent-deletion
path. Docstring updated to point at `hard_delete()` for the permanent case.
**Re: the `expunge_all()` praise** — thanks. `Query.get()` short-circuiting
through the identity map can make tests look correct while the listener never
fires; flushing the map made the empirical proof airtight. Glad it reads as
such.
---
Entity branches (sc-106189/90/91) all rebased onto the new infra tip and
pushed. CI should re-run on all four PRs.
Let me know if anything else surfaces.
--
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]
