mikebridge commented on PR #39977:
URL: https://github.com/apache/superset/pull/39977#issuecomment-4502609584
@aminghadersohi thanks for the post-approval flags — both are worth
addressing. **F-002 fixed in `8b0495aacb`; F-001 deferred to a focused
follow-up PR.**
### F-002 — session bypass cleanup. Fixed.
You're right that relying on session teardown left a within-request
widened-visibility window. The fix uses a variation on your option B/C: the
filter records classes it added in `g._deleted_state_added_classes`
(request-scoped, auto-cleans), and `SoftDeleteApiMixin.pre_get_list` removes
those entries from `session.info[SKIP_VISIBILITY_FILTER_CLASSES]` in a
`try/finally` immediately after augmenting the response.
```python
# BaseDeletedStateFilter._add_session_bypass — track for release
bypass = query.session.info.setdefault(SKIP_VISIBILITY_FILTER_CLASSES, set())
bypass.add(self.model)
added = getattr(g, DELETED_STATE_ADDED_CLASSES, set()) | {self.model}
setattr(g, DELETED_STATE_ADDED_CLASSES, added)
# SoftDeleteApiMixin.pre_get_list — release after augmentation
try:
self._inject_deleted_at(data)
finally:
self._release_session_bypass()
@staticmethod
def _release_session_bypass() -> None:
added = getattr(g, DELETED_STATE_ADDED_CLASSES, set())
if not added:
return
bypass = db.session.info.get(SKIP_VISIBILITY_FILTER_CLASSES, set())
bypass -= added
setattr(g, DELETED_STATE_ADDED_CLASSES, set())
```
Key property: the release is scoped to entries *the filter* added. A
programmatic caller using the `skip_visibility_filter` context manager or a DAO
`skip_visibility_filter=True` call earlier in the same request is unaffected —
those bypasses manage their own lifecycles via try/finally or DAO call scope.
Pinned by `test_mixin_release_does_not_touch_unrelated_bypass_entries`.
Sequence within a list response:
1. `BaseDeletedStateFilter.apply` adds `self.model` to both
`session.info[SKIP_VISIBILITY_FILTER_CLASSES]` and
`g._deleted_state_added_classes`.
2. FAB issues count + inner + outer + relationship loads — all see the
bypass.
3. `pre_get_list` runs *after* the list query: consumes the augmentation
flag, injects `deleted_at`, then releases the filter's bypass.
4. Any code that runs later in the same request (audit hooks,
`after_request` handlers, dependent serialisation) sees the normal filtered
view.
Four new tests:
- `test_filter_records_added_classes_on_g` — pins the tracker
- `test_mixin_releases_bypass_after_inject` — pins the release fires after
`pre_get_list` returns
- `test_mixin_release_does_not_touch_unrelated_bypass_entries` — pins that
programmatic bypasses are preserved
- `test_mixin_release_is_noop_when_filter_was_not_invoked` — pins the no-op
path for requests that didn't use `deleted_state`
44 unit tests pass on the infrastructure branch (was 40).
### F-001 — TOCTOU on restore. Deferred.
You're right that the `validate()` → `model.restore()` → commit window has a
stale-authorization race. The fix isn't a one-liner, though, and the design
choice between the two options has real trade-offs:
- **`SELECT ... FOR UPDATE`** on the re-query needs either a new
`for_update` kwarg on `BaseDAO.find_by_id` (widening a load-bearing DAO method)
or bypassing the DAO for restore (losing the abstraction). Plus
dialect-specific behaviour: SQLite silently no-ops, PostgreSQL/MySQL differ on
timeouts and `nowait` / `skip_locked` policy.
- **Atomic conditional update** — `UPDATE … WHERE uuid = ? AND deleted_at IS
NOT NULL` — closes the window precisely but **bypasses ORM `before_update` /
`after_update` event listeners**, including `AuditMixinNullable`'s `changed_on`
/ `changed_by_fk` stamping. Losing audit attribution on restore is a real cost;
we'd have to re-add stamping manually.
Proper test coverage means an actual concurrent-session test rig (two
sessions, observe lock contention or rowcount=0), not just a unit-level mock —
a different kind of work than what's been in this PR.
So F-001 lands separately, where the locking-vs-atomic-update design can get
focused review and the concurrency tests can be built deliberately. Tracked in
the spec at `sc-103157-soft-deletes/spec.md` under "Follow-up Items from
Review."
---
Infrastructure tip is now `8b0495aacb`. Entity branches rebased: `sc-106189`
→ `9c6d1584a2`, `sc-106190` → `5e26ebc084`, `sc-106191` → `ac38056b41`. CI
re-running across all four.
--
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]
