mikebridge commented on PR #39977: URL: https://github.com/apache/superset/pull/39977#issuecomment-4433340887
@richardfogaca thanks for the second pass — addressed in `543dff20c9` for items 1 and 4; replying inline for items 2 and 3. **Item 1 (importers cleanup) — fixed.** You're right that the raw Core `DELETE` was trading correctness for perf. `find_existing_for_import` now uses `db.session.delete()` so `ObjectUpdater.after_delete` and `SqlaTable.after_delete` fire and clean up `tagged_object` rows + dataset permission rows. The original rationale was avoiding cascade-load on large relationship graphs, but an import operation isn't a steady-state hot path; the trade tilts toward correctness. Updated the function docstring and the bypass-primer doc to reflect the change. **Item 4 (hardcoded id PK) — fixed.** `_get_deleted_at_map` now resolves the PK via `self.datamodel.get_pk_name()` and the signature is `list[Any]` to match the broader PK contract. Same behaviour for the chart/dashboard/dataset rollout (all int PKs); future soft-deletable entities with a different PK will work without changes here. **Item 2 (per-query bypass scope) — accepted YAGNI, comment added in follow-up.** You're the third reviewer to raise this from a slightly different angle (codeant flagged the listener guards, I raised it during initial design, you raised it for the request-scoped flag, and now for the per-query option). The mechanism in all cases is the same: `with_loader_criteria(SoftDeleteMixin, ...)` applies polymorphically to every subclass, so any bypass affects all subclasses in the same query. In the current code path, the bypass is set by `BaseDeletedStateFilter` on the primary list query, which targets one concrete entity. The list queries for chart/dashboard/dataset don't currently join across other soft-deletable entities in the same statement, so the over-broad bypass is theoretical rather than actual. Per-entity scoping is a real future-proofing option (e.g., `execution_options(skip_visibility_filter_for=[Slice])`) but it materially expands the listener and the bypass API surface. I'd prefer to defer it until a use case actually surfaces. Happy to add a code comment in `BaseDeletedStateFilter.apply` flagging the contract: ```python # The execution-option bypass is per-statement but not per-class — it # skips the soft-delete listener for every SoftDeleteMixin subclass in # the statement, not just self.model. Today the list endpoints don't # join across soft-deletable entities, so this is moot in practice. # If a future list query starts joining (e.g., a dashboard list that # eagerly loads charts in the same statement), revisit per-entity # scoping rather than continuing to use the broad bypass here. ``` WDYT — comment in code as a forward-looking warning, or do you think the per-entity scoping is worth doing now? **Item 3 (visibility auth gap) — SIP-208 question rather than code review.** Legitimate concern. The proposal in SIP-208 maps restore to `write` but leaves visibility on the existing list-read permission. Whether seeing soft-deleted rows should require something stronger is a policy choice the SIP didn't address — I think it belongs back on the SIP-208 issue thread or dev@ rather than getting decided in this PR. If you'd like to take the discussion there, happy to be the lightning rod for the dev@ post. Praise on the try/finally — noted, thank you. That ordering took a couple of iterations to land on. -- 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]
