mikebridge commented on code in PR #41551: URL: https://github.com/apache/superset/pull/41551#discussion_r3707406999
########## superset-frontend/src/explore/exploreUtils/canOverwriteSlice.ts: ########## @@ -0,0 +1,71 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +import { + isUserAdmin, + isUserInSubjects, + type SubjectRef, +} from 'src/dashboard/util/permissionUtils'; +import { UserWithPermissionsAndRoles } from 'src/types/bootstrapTypes'; + +interface SliceLike { + editors?: SubjectRef[] | null; + extra_editors?: SubjectRef[] | null; + is_managed_externally?: boolean | null; +} + +/** + * Whether *user* may modify *slice*, in the sense that governs both saving + * over a chart and acting on its version history. + * + * `canOverwrite` is the value the explore store holds, which `hydrateExplore` + * computes purely as "the current user is among the slice's editors". That is + * necessary but not sufficient: a chart with no explicit editors — which + * includes every seeded and example chart — yields false for everyone, + * administrators included. So the store value is treated as one of several + * routes to permission rather than the whole answer. + * + * Externally managed slices are excluded: their source of truth lives outside + * Superset, so overwriting or reverting one would be overwritten again by the + * next sync. + */ +export function canOverwriteSlice({ + slice, + user, + canOverwrite = false, +}: { + slice?: SliceLike | null; + user?: UserWithPermissionsAndRoles; + canOverwrite?: boolean; +}): boolean { + if (!slice || slice.is_managed_externally) { + return false; + } + if (canOverwrite || isUserAdmin(user)) { + return true; + } Review Comment: *Generated by Claude (AI) on behalf of @mikebridge.* Refuting this one with evidence: the `can_write` check was never in the chart contract, so this PR didn't drop it. The shipped predicate on master before this extraction (`SaveModal.tsx`, commit `c0e5f5226d`) was: ```ts can_overwrite || isUserAdmin(user) || canEditSlice || isCurrentUserOwner() ``` — no `findPermission('can_write', ...)` anywhere. `canOverwriteSlice` preserves that exactly, minus the dead `owners` branch (unreachable since #38831 removed `Slice.owners`) and plus `extra_editors`. You're right that this diverges from `canUserEditDashboard`, which does gate on `can_write` — but that asymmetry is pre-existing between the chart and dashboard contracts, not something introduced here, and tightening the chart gate would be a behaviour change for every deployment outside this PR's flag. Worth doing as its own change if the team wants the contracts aligned; I'd rather not smuggle it into a flagged feature PR, which is the same reasoning that got the `extra_editors` fix split out into #42708. Practical impact meanwhile is a UI-only mismatch in an unusual configuration (Admin role without `can_write` on Chart): the action shows and the API rejects it. Fail-safe direction — the server still enforces. ########## superset-frontend/src/features/versionHistory/useVersionActions.tsx: ########## @@ -0,0 +1,323 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +import { ReactElement, useCallback, useEffect, useState } from 'react'; +import { useDispatch, useSelector } from 'react-redux'; +import { t } from '@apache-superset/core/translation'; +import { useToasts } from 'src/components/MessageToasts/withToasts'; +import { getClientErrorObject } from '@superset-ui/core'; +import { + closeOpenedTab, + navigateOpenedTab, + openBlankTab, +} from 'src/utils/navigationUtils'; +import type { VersionedEntityType } from './types'; +import { + createChartFromSnapshot, + createDashboardFromSnapshot, + fetchActivity, + fetchVersionSnapshot, + restoreVersion, +} from './api'; +import { + clearVersionPreview, + selectVersionSessionLog, + versionRestored, + type VersionHistoryRootState, +} from './reducer'; +import { formatVersionMonthDay } from './display'; +import RestoreConfirmModal from './RestoreConfirmModal'; + +/** The version a restore / open-as-new action operates on. */ +export interface VersionActionTarget { + versionUuid: string; + headline: string; + issuedAt: string; +} + +// The in-flight locks live at module scope because the invariant they +// protect is entity-wide, not instance-wide: the preview banner and the +// history panel each mount their own useVersionActions for the same +// entity, so a ref-scoped lock would let the banner's activation slip +// past the panel's and fork a duplicate (or start a second restore). +// State guards alone are no lock at all — two activations in one tick +// both read the pre-update state value. Keys are removed in `finally`, +// so the set is self-cleaning; the per-instance isRestoring/isCreating +// state remains only to drive that instance's spinner. +const inFlightActions = new Set<string>(); + +const restoreLockKey = (entityType: string, uuid: string): string => + `restore:${entityType}:${uuid}`; +const forkLockKey = ( + entityType: string, + uuid: string, + versionUuid: string, +): string => `fork:${entityType}:${uuid}:${versionUuid}`; + +export interface UseVersionActionsResult { + /** Opens the restore confirmation modal for the given version. */ + requestRestore: (target: VersionActionTarget) => void; + /** Forks the given version into a new chart/dashboard in a new tab. */ + openAsNew: (target: VersionActionTarget) => void; + /** True while an openAsNew fork is in flight; disable its triggers. */ + isCreating: boolean; + /** Render this alongside the calling component. */ + restoreModal: ReactElement | null; +} + +/** + * Restore and open-as-new flows shared by the panel kebabs and the + * preview banner. Restore success is broadcast via the redux + * `restoreCount` so page-level hooks can rehydrate and refresh activity. + */ +export function useVersionActions( + entityType: VersionedEntityType, + uuid: string | undefined, +): UseVersionActionsResult { + const dispatch = useDispatch(); + const { addSuccessToast, addInfoToast, addWarningToast, addDangerToast } = + useToasts(); + const [restoreTarget, setRestoreTarget] = + useState<VersionActionTarget | null>(null); + const [isRestoring, setIsRestoring] = useState(false); + const [isCreating, setIsCreating] = useState(false); + + // A restore rehydrates the page from the server, which silently wipes + // in-progress edits and their undo history — the same hazard the preview + // entry gate guards against. The dirty signal is page-specific: dashboards + // track hasUnsavedChanges; explore's signal is the session log, which lists + // exactly the unsaved control changes the panel shows under "Current + // version". Read here rather than passed in, so no call site (panel kebab, + // preview banner) can forget it. + const hasUnsavedChanges = useSelector< + VersionHistoryRootState & { + dashboardState?: { hasUnsavedChanges?: boolean }; + }, + boolean + >(state => + entityType === 'dashboard' + ? !!state.dashboardState?.hasUnsavedChanges + : selectVersionSessionLog(state).length > 0, + ); + + // A pending confirmation names a version of the entity it was opened for. + // If the page's entity changes underneath it (an in-place slice swap), the + // modal must not survive to combine the new uuid with the old version — + // the server would refuse the mismatch, but the user would be shown a + // confusing failure for an action they aimed at something else. + useEffect(() => { + setRestoreTarget(null); + }, [entityType, uuid]); + + const requestRestore = useCallback( + (target: VersionActionTarget) => { + if (hasUnsavedChanges) { + addDangerToast( + t('Save or discard your unsaved changes to restore a version.'), + ); + return; + } + setRestoreTarget(target); + }, + [addDangerToast, hasUnsavedChanges], + ); + + const cancelRestore = useCallback(() => { + setRestoreTarget(null); + }, []); + + // TODO(version-history): backend workaround — remove when the restore + // endpoint reports whether it created a version (e.g. `created: boolean` + // in its response). The endpoint reports success but not whether a new + // version transaction was created (restoring an already-matching state + // is a server-side no-op); probe the newest self transaction to tell the + // two apart in the toast. A save by another user landing between the + // two probes can skew which toast variant shows — accepted, cosmetic. + const latestTransactionId = useCallback(async (): Promise<number | null> => { + if (!uuid) { + return null; + } + try { + const { result } = await fetchActivity(entityType, uuid, { + include: 'self', + page: 0, + pageSize: 1, + }); + return result[0]?.transaction_id ?? null; + } catch { + return null; + } + }, [entityType, uuid]); + + const confirmRestore = useCallback(async () => { + if (!restoreTarget || !uuid) { + return; + } + // Entity-wide, not per-version: two concurrent restores to different + // versions of the same entity would race each other's rehydration. + const lockKey = restoreLockKey(entityType, uuid); + if (inFlightActions.has(lockKey)) { + return; + } + if (hasUnsavedChanges) { + // The request-time gate can be outrun: work turning dirty while the + // confirmation modal sits open (an in-flight edit resolving late) + // would still be wiped by the rehydration. Re-check at the moment of + // mutation. + addDangerToast( + t('Save or discard your unsaved changes to restore a version.'), + ); + setRestoreTarget(null); + return; + } + inFlightActions.add(lockKey); + setIsRestoring(true); + try { + const beforeTransactionId = await latestTransactionId(); + const { message } = await restoreVersion( + entityType, + uuid, + restoreTarget.versionUuid, + ); + const afterTransactionId = await latestTransactionId(); + if ( + beforeTransactionId !== null && + afterTransactionId !== null && + beforeTransactionId === afterTransactionId + ) { + addInfoToast(t('Already at this version')); + } else { + addSuccessToast(t("Restored to '%s' version", restoreTarget.headline)); + } + // A restore can succeed while dropping chart associations the snapshot + // referenced but that no longer exist, and the endpoint says so in its + // message rather than in a status code. Reporting only the success + // would tell the user their dashboard came back whole when it did not. + if (message && message !== 'OK') { + addWarningToast(message); + } + setRestoreTarget(null); + dispatch(clearVersionPreview(uuid)); + dispatch(versionRestored(uuid)); Review Comment: *Generated by Claude (AI) on behalf of @mikebridge.* Refuting: entity-level is the correct scope here, and version-level would be wrong. A restore doesn't just change a version — it rewrites the entity's live state and triggers a full page rehydration (`versionRestored` → `hydrateExplore`/`hydrateDashboard`). If a preview of *any* version of that entity survived the restore, the banner would sit over freshly-rehydrated live content claiming the user is viewing history. That's precisely the class of bug this PR has been closing, so exiting every preview of the restored entity is deliberate, not incidental. The scenario you describe — exit preview, select v2, restore of v1 completes — costs the user a re-selection of v2, which is cheap and self-evident on screen. Leaving v2's preview active over a rehydrating page would be silently wrong instead. The entity check itself was added in `5d9fe736f2` for the case that *is* a bug: a restore of a **different** entity clearing this page's preview. ########## superset-frontend/src/features/versionHistory/useVersionActions.tsx: ########## @@ -0,0 +1,295 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +import { ReactElement, useCallback, useEffect, useRef, useState } from 'react'; +import { useDispatch, useSelector } from 'react-redux'; +import { t } from '@apache-superset/core/translation'; +import { useToasts } from 'src/components/MessageToasts/withToasts'; +import { getClientErrorObject } from '@superset-ui/core'; +import { + closeOpenedTab, + navigateOpenedTab, + openBlankTab, +} from 'src/utils/navigationUtils'; +import type { VersionedEntityType } from './types'; +import { + createChartFromSnapshot, + createDashboardFromSnapshot, + fetchActivity, + fetchVersionSnapshot, + restoreVersion, +} from './api'; +import { + clearVersionPreview, + selectVersionSessionLog, + versionRestored, + type VersionHistoryRootState, +} from './reducer'; +import { formatVersionMonthDay } from './display'; +import RestoreConfirmModal from './RestoreConfirmModal'; + +/** The version a restore / open-as-new action operates on. */ +export interface VersionActionTarget { + versionUuid: string; + headline: string; + issuedAt: string; +} + +export interface UseVersionActionsResult { + /** Opens the restore confirmation modal for the given version. */ + requestRestore: (target: VersionActionTarget) => void; + /** Forks the given version into a new chart/dashboard in a new tab. */ + openAsNew: (target: VersionActionTarget) => void; + /** True while an openAsNew fork is in flight; disable its triggers. */ + isCreating: boolean; + /** Render this alongside the calling component. */ + restoreModal: ReactElement | null; +} + +/** + * Restore and open-as-new flows shared by the panel kebabs and the + * preview banner. Restore success is broadcast via the redux + * `restoreCount` so page-level hooks can rehydrate and refresh activity. + */ +export function useVersionActions( + entityType: VersionedEntityType, + uuid: string | undefined, +): UseVersionActionsResult { + const dispatch = useDispatch(); + const { addSuccessToast, addInfoToast, addWarningToast, addDangerToast } = + useToasts(); + const [restoreTarget, setRestoreTarget] = + useState<VersionActionTarget | null>(null); + const [isRestoring, setIsRestoring] = useState(false); + const [isCreating, setIsCreating] = useState(false); + // The state flags drive rendering; these refs are the actual locks. Two + // activations in one tick both read the pre-update state value, so a + // guard on state alone lets the second through and forks a duplicate. + const restoringRef = useRef(false); + const creatingRef = useRef(false); + + // A restore rehydrates the page from the server, which silently wipes + // in-progress edits and their undo history — the same hazard the preview + // entry gate guards against. The dirty signal is page-specific: dashboards + // track hasUnsavedChanges; explore's signal is the session log, which lists + // exactly the unsaved control changes the panel shows under "Current + // version". Read here rather than passed in, so no call site (panel kebab, + // preview banner) can forget it. + const hasUnsavedChanges = useSelector< + VersionHistoryRootState & { + dashboardState?: { hasUnsavedChanges?: boolean }; + }, + boolean + >(state => + entityType === 'dashboard' + ? !!state.dashboardState?.hasUnsavedChanges + : selectVersionSessionLog(state).length > 0, + ); + + // A pending confirmation names a version of the entity it was opened for. + // If the page's entity changes underneath it (an in-place slice swap), the + // modal must not survive to combine the new uuid with the old version — + // the server would refuse the mismatch, but the user would be shown a + // confusing failure for an action they aimed at something else. + useEffect(() => { + setRestoreTarget(null); + }, [entityType, uuid]); + + const requestRestore = useCallback( + (target: VersionActionTarget) => { + if (hasUnsavedChanges) { + addDangerToast( + t('Save or discard your unsaved changes to restore a version.'), + ); + return; + } + setRestoreTarget(target); + }, + [addDangerToast, hasUnsavedChanges], + ); + + const cancelRestore = useCallback(() => { + setRestoreTarget(null); + }, []); + + // The restore endpoint reports success but not whether a new version + // transaction was created (restoring an already-matching state is a + // server-side no-op); probe the newest self transaction to tell the + // two apart in the toast. A save by another user landing between the + // two probes can skew which toast variant shows — accepted, cosmetic. + const latestTransactionId = useCallback(async (): Promise<number | null> => { + if (!uuid) { + return null; + } + try { + const { result } = await fetchActivity(entityType, uuid, { + include: 'self', + page: 0, + pageSize: 1, + }); + return result[0]?.transaction_id ?? null; + } catch { + return null; + } + }, [entityType, uuid]); + + const confirmRestore = useCallback(async () => { + if (!restoreTarget || !uuid || restoringRef.current) { + return; + } + if (hasUnsavedChanges) { + // The request-time gate can be outrun: work turning dirty while the + // confirmation modal sits open (an in-flight edit resolving late) + // would still be wiped by the rehydration. Re-check at the moment of + // mutation. + addDangerToast( + t('Save or discard your unsaved changes to restore a version.'), + ); + setRestoreTarget(null); + return; + } + restoringRef.current = true; + setIsRestoring(true); + try { + const beforeTransactionId = await latestTransactionId(); + const { message } = await restoreVersion( + entityType, + uuid, + restoreTarget.versionUuid, + ); + const afterTransactionId = await latestTransactionId(); + if ( + beforeTransactionId !== null && + afterTransactionId !== null && + beforeTransactionId === afterTransactionId + ) { + addInfoToast(t('Already at this version')); + } else { + addSuccessToast(t("Restored to '%s' version", restoreTarget.headline)); + } + // A restore can succeed while dropping chart associations the snapshot + // referenced but that no longer exist, and the endpoint says so in its + // message rather than in a status code. Reporting only the success + // would tell the user their dashboard came back whole when it did not. + if (message && message !== 'OK') { + addWarningToast(message); + } + setRestoreTarget(null); + dispatch(clearVersionPreview()); + dispatch(versionRestored(uuid)); Review Comment: *Generated by Claude (AI) on behalf of @mikebridge.* This thread is superseded — the unscoped `clearVersionPreview()` it flagged was fixed in `5d9fe736f2` (required entity uuid, reducer no-ops on mismatch, so all five dispatchers inherit the guard). @rusackas raised the same point independently and it's answered on his thread. -- 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]
