mikebridge commented on PR #41550:
URL: https://github.com/apache/superset/pull/41550#issuecomment-5131183521
## Fix-only review: `0afce20ffe..e8f5b96dd8`
Reviewed the fixes with Python, Preset, Superset committer, SQLAlchemy,
React,
and Tidy First.
### HIGH
1. **Archived-state race remains open at the locked delete** —
`superset/commands/purge.py:89`
`require_archived=True` affects an unlocked resolution, but the subsequent
locked claim and final deletion do not require `deleted_at IS NOT NULL`. A
restore between resolution and locking can still result in permanent
deletion
of a live object. Carry the archived predicate into the locked claim and
conditional delete.
Sources: Python, Preset, SQLAlchemy, Tidy First.
2. **Authorization remains stale across the audit transaction boundary** —
`superset/commands/purge.py:89`
Editorship is validated before rollback, audit commit, and re-resolution,
but
not on the locked entity immediately before deletion. Reauthorize within
the
destructive transaction.
Sources: Python, Preset, SQLAlchemy, Superset committer, Tidy First.
3. **New Redux selector uses prohibited `any`** —
`superset-frontend/src/pages/ArchivedList/index.tsx:411`
`useSelector<any, ...>` violates the repository's prohibition on new `any`
types. Use the established `RootState` and a precise roles result type.
Sources: React, Preset, Superset committer.
### MEDIUM
1. **Restore-race test is a false positive** —
`tests/integration_tests/charts/soft_delete_tests.py:792`
The test patches `validate()` to return a chart, but `run()` reads
`self._model`. Because the mock never populates `_model`, the test
returns 404
before reaching `ForcePurgeCommand`. Test the actual resolution-to-lock
interval.
Sources: Python, Tidy First.
2. **Raw SQLAlchemy exception text reaches API callers** —
`superset/commands/purge.py:100`
The 422 response may expose SQL, parameters, constraints, or database
diagnostics. Log the original exception and return stable user-facing
text.
Source: Preset.
3. **Permission-aware type selection is incomplete at the route boundary** —
`superset/views/archived_assets.py:42`
The UI selects the first readable asset type, but the server route still
requires Chart read permission. Dashboard-only or dataset-only users
cannot
reach the component despite the new frontend behavior and tests.
Source: Superset committer.
4. **Added Python assignments lack required explicit annotations** —
`superset/commands/purge.py:42`
Added production constants/locals and numerous test variables lack the
explicit annotations required by the repository instructions. Added
function
signatures were otherwise annotated.
Source: Python.
### Prior findings confirmed resolved
- Cross-model UUID purge/authorization flaw
- Successful response for blocked or no-op purge
- Purge APIs surviving feature-flag disablement
- Restore double-fire and keyboard-focus issues
- Missing permanent-delete documentation
- Permission filtering within the React component
- Links to pages that cannot display archived objects
**Summary:** 0 BLOCKER, 3 HIGH, 4 MEDIUM, 0 LOW. The fix set still needs
changes
before merge.
--
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]
