mikebridge commented on PR #42469: URL: https://github.com/apache/superset/pull/42469#issuecomment-5117544261
Second review round (codeant-ai, 5 threads) triaged — replies in each thread. No code change needed on this PR; `c65d09f94e` still stands. **1 real, filed as a follow-up:** the restore path has no row lock or optimistic version check, so a concurrent edit committed between `validate()` and `revert()` is overwritten silently. Not merge-blocking — overwriting current state is what restore does, and the clobbered edit is itself captured as a version, so it's recoverable. The fix (optimistic transaction-id re-check → 409) wants a matching affordance in the version-history UI, so it belongs in its own change. **4 rejected, evidence in each thread:** - Two soft-delete findings (`find_active_by_uuid`, and the dashboard membership query reattaching trashed charts) — both already handled by the global `do_orm_execute` listener that attaches `deleted_at IS NULL` to standard ORM queries. Neither query bypasses it. The membership case already reports trashed members via `skipped_slice_ids`, which is the exact contract the finding asks for. - No-op restores creating no version row is deliberate: `SkipUnmodifiedPlugin` exists specifically to suppress them, with the rationale and the orphaned-transaction handling documented. - The capture-gate init-vs-request-time gap is documented verbatim in `capture_enabled()`'s docstring, mitigation included. It assumes runtime mutation of `ENABLE_VERSIONING_CAPTURE`, which isn't a supported operation. Worth noting for anyone weighing the "Critical 🚨" labels: all three of those rejected as incorrect were labelled Critical, and in each case the behavior the tool asked for is what the code already does — via a global listener or a dedicated plugin rather than a local filter. Static analysis doesn't see the `do_orm_execute` hook. Still green, still approved, and still looking for a committer to merge — it's the last gate on #41551. -- 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]
