mikebridge commented on PR #41551: URL: https://github.com/apache/superset/pull/41551#issuecomment-5141573961
### Correction — withdrawing the `canOverwriteSlice` finding I led both comments above with `canOverwriteSlice` dropping the chart-owner branch, called it blocker-grade, and cited that three of nine independent lenses reached it separately. **That finding is wrong, and I'm withdrawing it.** The change is correct as written. Since I flagged it as must-fix-before-merge, here is the evidence rather than just the retraction. **`slice.owners` does not exist.** [#38831](https://github.com/apache/superset/pull/38831) (`33f0fc93ed`, 2026-07-08) introduced the `Subject` model and deleted the `slice_user` association table together with the `owners` relationship on `Slice`. What `Slice.data` emits today is `editors`, `extra_editors` and `viewers` (`superset/models/slice.py:269-271`) — there is no `owners` key, `SliceSchema` (`superset/explore/schemas.py:178`) declares none, and `superset-frontend/src/types/Chart.ts` carries none. `hydrateExplore` passes the payload through unchanged, and the only post-hydration writer (`SLICE_UPDATED`, from `PUT /api/v1/chart/<pk>`) returns a schema with no `owners` either. **So the branch removed dead code, not a working feature.** [#41352](https://github.com/apache/superset/pull/41352) (`c0e5f5226d`) landed 2026-07-13 — five days *after* the field it reads was deleted. `isCurrentUserOwner()` has always returned `false` in production. It type-checked only because `SaveModal` declares its prop as `slice?: Record<string, any>`, and it passed CI only because its accompanying test fabricated `owners: [{ id: 1 }]` directly into a mock store — the one place in the tree where that field has ever existed. **And the new predicate is strictly more correct than the old one.** The server authorizes overwrite through `UpdateChartCommand.validate()` → `raise_for_editorship` → `is_editor`, which resolves to admin ∪ `editors` ∪ `extra_editors` (`superset/security/manager.py:4910-4938`). Master's client-side gate consulted a nonexistent `owners` field and ignored `extra_editors`; this branch's helper drops the former and adds the latter, so the client gate matches the server for the first time. `can_overwrite` itself is computed entirely client-side in `hydrateExplore.ts:182-187` — the backend never sends it, and it ignores both admin status and `extra_editors`, which is the original defect this extraction set out to fix. **The test concern is already handled**, in `04157272fb`, and more thoroughly than I asked: `SaveModal.test.tsx:361` now sets `editors: []` explicitly, asserts that overwrite is *disabled* for an owner who is not an editor, and carries a comment explaining that the shared fixture would otherwise satisfy the gate by a different route. There is a companion test for the owner-who-is-also-an-editor case. Nothing to do here. **On why this got through nine lenses.** Each one compared the diff against the stated intent of a merged commit and concluded a deliberate fix was being reverted. None checked whether the field that commit referenced still existed. I presented the three-way convergence as the strongest signal in the review; it was three lenses sharing one blind spot, which is worth less than one lens that checks the model. My apologies for the noise on the most prominent item. Two small things that did surface from digging into it, neither blocking and neither this PR's doing: - `"owners"` is still listed in `__versioned__["exclude"]` at `superset/models/slice.py:96`, referring to an attribute that no longer exists — leftover from #38831. - `EXTRA_OWNERS_RESOLVER` (`superset/charts/api.py:377`) is read but never declared in `config.py`, unlike its `EXTRA_EDITORS_RESOLVER` counterpart (`config.py:3086`). @rusackas — worth knowing separately that #41352 is a no-op against current master for the reason above. If there was a real report behind it, it is still open; under the Subject model the fix would be granting editorship rather than reading `owners`. Everything else in the two comments above stands as posted. _This correction was generated by Claude (AI) on behalf of @mikebridge._ -- 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]
