mikebridge commented on PR #41550: URL: https://github.com/apache/superset/pull/41550#issuecomment-5142959309
## Codex capstone review — complete soft-deletion functionality This review was run via **OpenAI Codex** over the full `master...HEAD` soft-deletion diff using these lenses: `preset-review`, `python-review`, `react-review`, `sqlalchemy-review`, `clean-code-review`, `domain-driven-design-review`, and `tidy-first-review`. The SQLAlchemy/Alembic lens returned no additional findings. ### HIGH 1. **`superset/commands/deletion_retention/audit.py:199` — Reconciliation equates target absence with this purge attempt succeeding.** Concurrent purge attempts or an unrelated deletion can cause a crashed attempt to be recorded as confirmed even though it did not perform the purge. Persist an operation/claim token atomically with the purge, or use an `indeterminate` / `target_absent` outcome instead of attributing success. _Source: domain-driven-design-review._ ### MEDIUM 1. **`superset/commands/purge.py:104` — Purge endpoints expose raw database exception details.** `str(ex)` can include SQL, bind parameters, constraint names, and metadata-schema details. Log the original exception and return a stable user-facing deletion failure. _Source: preset-review._ 2. **`superset/views/archived_assets.py:53` — Recently Archived is unreachable for dashboard-only and dataset-only roles.** The page shell requires Chart read permission even though its APIs are independently authorized. Admit users who can read any supported resource while retaining the per-API checks. _Source: preset-review._ 3. **`superset/views/filters.py:235` — Some authorized restore principals cannot discover their objects.** `EXTRA_EDITORS_RESOLVER` and role-derived editors can restore a known object directly but are omitted from Recently Archived enumeration. Implement a discoverable audience mechanism or narrow the documented contract and provide another discovery path. _Source: preset-review._ 4. **`superset-frontend/src/pages/ArchivedList/index.tsx:209` — Row actions are re-enabled before refresh completes.** Restore and purge call `refreshData()` without awaiting it, then clear the in-flight UUID. A stale row can issue a duplicate operation and show a failure after success. Await the refresh (or remove the row locally) and add a deferred-refresh regression test. _Sources: react-review, tidy-first-review._ 5. **`superset-frontend/src/pages/ArchivedList/index.tsx:447` — Selected resource type can become invalid after permissions resolve.** `type` is initialized from `availableTypes` only once, so later role resolution can leave an unavailable selection active. Synchronize or derive the effective selection and add a role-transition test. _Source: clean-code-review._ 6. **`superset/commands/deletion_retention/audit.py:220` — An active long-running purge can be reconciled as failed.** A worker active beyond the stale threshold can still expose its uncommitted entity; reconciliation finalizes the attempt as failed and the eventual worker confirmation cannot replace it. Use a lease/heartbeat or another durable completion marker. _Source: python-review._ 7. **`superset/commands/deletion_retention/force_purge.py:129` — Non-success audit records retain a relationship-removal count.** The write-ahead row records the pre-purge count, but blocked, failed, and lost-race paths leave it intact even though cleanup rolled back. Store it only on confirmation or reset it for non-success outcomes. _Source: python-review._ 8. **`superset/commands/deletion_retention/purge_cascade.py:166` — Soft-delete registration and purge support have separate sources of truth.** A future `SoftDeleteMixin` adopter is automatically processed by retention but may have no explicit dependency-cleanup policy. Require a fail-closed purge-policy registration for every supported model. _Source: clean-code-review._ 9. **`superset/commands/deletion_retention/purge_cascade.py:412` — Chart purge mutates surviving dashboard composition through raw association deletion.** This bypasses dashboard audit/version behavior. Route the membership change through the dashboard application boundary or an explicit post-purge handler. _Source: domain-driven-design-review._ 10. **`superset/models/purge_audit_log.py:49` — The new model uses untyped legacy `Column` declarations.** Move the fields to the repository-supported typed ORM form and annotate table metadata constants. _Source: python-review._ 11. **`tests/integration_tests/charts/version_restore_tests.py:69` — Added test/helper signatures are not fully typed.** Similar omissions occur in other new fixtures and helpers. Annotate added parameters and returns, including pytest fixtures and nested helpers. _Source: python-review._ 12. **Python assignment annotation audit — qualification required.** The Python lens reported 1,323 added assignments without explicit annotations. That raw count should not be treated as 1,323 automatic defects: repository rules require every added assignment to be reviewed against typing rules, but do not unambiguously require annotating every inferable local. Remediation should narrow this to concrete violations such as module constants, attributes, `None` initializations, ambiguous containers, and public signatures. ### LOW 1. **`superset-frontend/src/pages/ArchivedList/index.tsx:461` — The no-readable-types fallback contradicts the filtering intent.** It exposes all three types when none is readable, leading to predictable 403s. Render an unavailable/empty state instead. _Source: tidy-first-review._ 2. **`superset/models/purge_audit_log.py:33` — Purge lifecycle values are stringly typed and unconstrained.** Consider explicit value types plus database constraints for status and trigger invariants. _Source: domain-driven-design-review._ 3. **`superset/charts/api.py:807`, `superset/dashboards/api.py:1469`, `superset/datasets/api.py:1234` — Purge route control flow is duplicated.** Extract the executable workflow while retaining explicit resource bindings and Flask-AppBuilder route registration. _Sources: clean-code-review, tidy-first-review._ ### Verdict **Changes requested:** 1 HIGH, 12 accepted/qualified MEDIUM, and 3 LOW findings. No code was modified as part of this Codex review. -- 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]
