This is an automated email from the ASF dual-hosted git repository.
justinpark pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/superset.git
The following commit(s) were added to refs/heads/master by this push:
new ddc9f06786 refactor(sqllab): nonblocking delete query editor (#29233)
ddc9f06786 is described below
commit ddc9f06786a71dd16d89c0e74c9608b5432b2de9
Author: JUST.in DO IT <[email protected]>
AuthorDate: Fri Jun 14 12:25:55 2024 -0700
refactor(sqllab): nonblocking delete query editor (#29233)
---
superset-frontend/src/SqlLab/actions/sqlLab.js | 28 ++++---------------
.../src/SqlLab/actions/sqlLab.test.js | 10 ++-----
.../EditorAutoSync/EditorAutoSync.test.tsx | 23 ++++++++++++++++
.../src/SqlLab/components/EditorAutoSync/index.tsx | 32 +++++++++++++++++++++-
superset-frontend/src/SqlLab/fixtures.ts | 1 +
.../middlewares/persistSqlLabStateEnhancer.js | 10 ++++++-
.../src/SqlLab/reducers/getInitialState.test.ts | 15 +++++++++-
.../src/SqlLab/reducers/getInitialState.ts | 12 ++++++++
superset-frontend/src/SqlLab/reducers/sqlLab.js | 9 ++++++
.../src/SqlLab/reducers/sqlLab.test.js | 17 ++++++++++++
superset-frontend/src/SqlLab/types.ts | 1 +
.../src/hooks/apiResources/sqlEditorTabs.test.ts | 21 ++++++++++++++
.../src/hooks/apiResources/sqlEditorTabs.ts | 8 ++++++
13 files changed, 155 insertions(+), 32 deletions(-)
diff --git a/superset-frontend/src/SqlLab/actions/sqlLab.js
b/superset-frontend/src/SqlLab/actions/sqlLab.js
index aa81f8429e..18b74d2afc 100644
--- a/superset-frontend/src/SqlLab/actions/sqlLab.js
+++ b/superset-frontend/src/SqlLab/actions/sqlLab.js
@@ -103,6 +103,7 @@ export const CREATE_DATASOURCE_FAILED =
'CREATE_DATASOURCE_FAILED';
export const SET_EDITOR_TAB_LAST_UPDATE = 'SET_EDITOR_TAB_LAST_UPDATE';
export const SET_LAST_UPDATED_ACTIVE_TAB = 'SET_LAST_UPDATED_ACTIVE_TAB';
+export const CLEAR_DESTROYED_QUERY_EDITOR = 'CLEAR_DESTROYED_QUERY_EDITOR';
export const addInfoToast = addInfoToastAction;
export const addSuccessToast = addSuccessToastAction;
@@ -715,29 +716,12 @@ export function toggleLeftBar(queryEditor) {
};
}
-export function removeQueryEditor(queryEditor) {
- return function (dispatch) {
- const sync = isFeatureEnabled(FeatureFlag.SqllabBackendPersistence)
- ? SupersetClient.delete({
- endpoint: encodeURI(`/tabstateview/${queryEditor.id}`),
- })
- : Promise.resolve();
+export function clearDestoryedQueryEditor(queryEditorId) {
+ return { type: CLEAR_DESTROYED_QUERY_EDITOR, queryEditorId };
+}
- return sync
- .then(() => dispatch({ type: REMOVE_QUERY_EDITOR, queryEditor }))
- .catch(({ status }) => {
- if (status !== 404) {
- return dispatch(
- addDangerToast(
- t(
- 'An error occurred while removing tab. Please contact your
administrator.',
- ),
- ),
- );
- }
- return dispatch({ type: REMOVE_QUERY_EDITOR, queryEditor });
- });
- };
+export function removeQueryEditor(queryEditor) {
+ return { type: REMOVE_QUERY_EDITOR, queryEditor };
}
export function removeAllOtherQueryEditors(queryEditor) {
diff --git a/superset-frontend/src/SqlLab/actions/sqlLab.test.js
b/superset-frontend/src/SqlLab/actions/sqlLab.test.js
index db051da5ea..11ab424512 100644
--- a/superset-frontend/src/SqlLab/actions/sqlLab.test.js
+++ b/superset-frontend/src/SqlLab/actions/sqlLab.test.js
@@ -610,7 +610,7 @@ describe('async actions', () => {
describe('removeQueryEditor', () => {
it('updates the tab state in the backend', () => {
- expect.assertions(2);
+ expect.assertions(1);
const store = mockStore({});
const expectedActions = [
@@ -619,12 +619,8 @@ describe('async actions', () => {
queryEditor,
},
];
- return store
- .dispatch(actions.removeQueryEditor(queryEditor))
- .then(() => {
- expect(store.getActions()).toEqual(expectedActions);
- expect(fetchMock.calls(updateTabStateEndpoint)).toHaveLength(1);
- });
+ store.dispatch(actions.removeQueryEditor(queryEditor));
+ expect(store.getActions()).toEqual(expectedActions);
});
});
diff --git
a/superset-frontend/src/SqlLab/components/EditorAutoSync/EditorAutoSync.test.tsx
b/superset-frontend/src/SqlLab/components/EditorAutoSync/EditorAutoSync.test.tsx
index 6ccae610a1..121ca07521 100644
---
a/superset-frontend/src/SqlLab/components/EditorAutoSync/EditorAutoSync.test.tsx
+++
b/superset-frontend/src/SqlLab/components/EditorAutoSync/EditorAutoSync.test.tsx
@@ -146,6 +146,29 @@ test('sync the active editor id when there are updates in
tab history', async ()
expect(fetchMock.calls(updateActiveEditorTabState)).toHaveLength(1);
});
+test('sync the destroyed editor id when there are updates in destroyed
editors', async () => {
+ const removeId = 'removed-tab-id';
+ const deleteEditorState = `glob:*/tabstateview/${removeId}`;
+ fetchMock.delete(deleteEditorState, { id: removeId });
+ expect(fetchMock.calls(deleteEditorState)).toHaveLength(0);
+ render(<EditorAutoSync />, {
+ useRedux: true,
+ initialState: {
+ ...initialState,
+ sqlLab: {
+ ...initialState.sqlLab,
+ destroyedQueryEditors: {
+ [removeId]: 123,
+ },
+ },
+ },
+ });
+ await waitFor(() => jest.advanceTimersByTime(INTERVAL));
+ expect(fetchMock.calls(deleteEditorState)).toHaveLength(1);
+ await waitFor(() => jest.advanceTimersByTime(INTERVAL));
+ expect(fetchMock.calls(deleteEditorState)).toHaveLength(1);
+});
+
test('skip syncing the unsaved editor tab state when the updates are already
synced', async () => {
const updateEditorTabState = `glob:*/tabstateview/${defaultQueryEditor.id}`;
fetchMock.put(updateEditorTabState, 200);
diff --git a/superset-frontend/src/SqlLab/components/EditorAutoSync/index.tsx
b/superset-frontend/src/SqlLab/components/EditorAutoSync/index.tsx
index 5477c4fa13..0b9173d368 100644
--- a/superset-frontend/src/SqlLab/components/EditorAutoSync/index.tsx
+++ b/superset-frontend/src/SqlLab/components/EditorAutoSync/index.tsx
@@ -29,12 +29,14 @@ import {
import {
useUpdateCurrentSqlEditorTabMutation,
useUpdateSqlEditorTabMutation,
+ useDeleteSqlEditorTabMutation,
} from 'src/hooks/apiResources/sqlEditorTabs';
import { useDebounceValue } from 'src/hooks/useDebounceValue';
import {
syncQueryEditor,
setEditorTabLastUpdate,
setLastUpdatedActiveTab,
+ clearDestoryedQueryEditor,
} from 'src/SqlLab/actions/sqlLab';
import useEffectEvent from 'src/hooks/useEffectEvent';
@@ -82,8 +84,13 @@ const EditorAutoSync: FC = () => {
const lastUpdatedActiveTab = useSelector<SqlLabRootState, string>(
({ sqlLab }) => sqlLab.lastUpdatedActiveTab,
);
+ const destroyedQueryEditors = useSelector<
+ SqlLabRootState,
+ Record<string, number>
+ >(({ sqlLab }) => sqlLab.destroyedQueryEditors);
const [updateSqlEditor, { error }] = useUpdateSqlEditorTabMutation();
const [updateCurrentSqlEditor] = useUpdateCurrentSqlEditorTabMutation();
+ const [deleteSqlEditor] = useDeleteSqlEditorTabMutation();
const debouncedUnsavedQueryEditor = useDebounceValue(
unsavedQueryEditor,
@@ -119,6 +126,22 @@ const EditorAutoSync: FC = () => {
}
});
+ const syncDeletedQueryEditor = useEffectEvent(() => {
+ if (Object.keys(destroyedQueryEditors).length > 0) {
+ Object.keys(destroyedQueryEditors).forEach(id => {
+ deleteSqlEditor(id)
+ .then(() => {
+ dispatch(clearDestoryedQueryEditor(id));
+ })
+ .catch(({ status }) => {
+ if (status === 404) {
+ dispatch(clearDestoryedQueryEditor(id));
+ }
+ });
+ });
+ }
+ });
+
useEffect(() => {
let saveTimer: NodeJS.Timeout;
function saveUnsavedQueryEditor() {
@@ -131,11 +154,18 @@ const EditorAutoSync: FC = () => {
}
const syncTimer = setInterval(syncCurrentQueryEditor, INTERVAL);
saveTimer = setTimeout(saveUnsavedQueryEditor, INTERVAL);
+ const clearQueryEditorTimer = setInterval(syncDeletedQueryEditor,
INTERVAL);
return () => {
clearTimeout(saveTimer);
clearInterval(syncTimer);
+ clearInterval(clearQueryEditorTimer);
};
- }, [getUnsavedNewQueryEditor, syncCurrentQueryEditor, dispatch]);
+ }, [
+ getUnsavedNewQueryEditor,
+ syncCurrentQueryEditor,
+ syncDeletedQueryEditor,
+ dispatch,
+ ]);
useEffect(() => {
const unsaved = getUnsavedItems(debouncedUnsavedQueryEditor);
diff --git a/superset-frontend/src/SqlLab/fixtures.ts
b/superset-frontend/src/SqlLab/fixtures.ts
index 54f1c278f8..95d45ba760 100644
--- a/superset-frontend/src/SqlLab/fixtures.ts
+++ b/superset-frontend/src/SqlLab/fixtures.ts
@@ -681,6 +681,7 @@ export const initialState = {
queriesLastUpdate: 0,
activeSouthPaneTab: 'Results',
unsavedQueryEditor: {},
+ destroyedQueryEditors: {},
},
messageToasts: [],
user,
diff --git
a/superset-frontend/src/SqlLab/middlewares/persistSqlLabStateEnhancer.js
b/superset-frontend/src/SqlLab/middlewares/persistSqlLabStateEnhancer.js
index d83e06d316..12c9c34853 100644
--- a/superset-frontend/src/SqlLab/middlewares/persistSqlLabStateEnhancer.js
+++ b/superset-frontend/src/SqlLab/middlewares/persistSqlLabStateEnhancer.js
@@ -50,6 +50,7 @@ const sqlLabPersistStateConfig = {
queries,
tabHistory,
lastUpdatedActiveTab,
+ destroyedQueryEditors,
} = state.sqlLab;
const unsavedQueryEditors = filterUnsavedQueryEditorList(
queryEditors,
@@ -58,7 +59,13 @@ const sqlLabPersistStateConfig = {
);
const hasUnsavedActiveTabState =
tabHistory.slice(-1)[0] !== lastUpdatedActiveTab;
- if (unsavedQueryEditors.length > 0 || hasUnsavedActiveTabState) {
+ const hasUnsavedDeletedQueryEditors =
+ Object.keys(destroyedQueryEditors).length > 0;
+ if (
+ unsavedQueryEditors.length > 0 ||
+ hasUnsavedActiveTabState ||
+ hasUnsavedDeletedQueryEditors
+ ) {
const hasFinishedMigrationFromLocalStorage =
unsavedQueryEditors.every(
({ inLocalStorage }) => !inLocalStorage,
@@ -76,6 +83,7 @@ const sqlLabPersistStateConfig = {
...(hasUnsavedActiveTabState && {
tabHistory,
}),
+ destroyedQueryEditors,
};
}
return;
diff --git a/superset-frontend/src/SqlLab/reducers/getInitialState.test.ts
b/superset-frontend/src/SqlLab/reducers/getInitialState.test.ts
index 1e7745f6d0..f7e5897c41 100644
--- a/superset-frontend/src/SqlLab/reducers/getInitialState.test.ts
+++ b/superset-frontend/src/SqlLab/reducers/getInitialState.test.ts
@@ -274,6 +274,9 @@ describe('getInitialState', () => {
name: expectedValue,
},
],
+ destroyedQueryEditors: {
+ 10: 12345,
+ },
},
}),
);
@@ -291,7 +294,10 @@ describe('getInitialState', () => {
updatedAt: lastUpdatedTime,
},
},
- tab_state_ids: [{ id: 1, label: '' }],
+ tab_state_ids: [
+ { id: 1, label: '' },
+ { id: 10, label: 'removed' },
+ ],
};
expect(
getInitialState(apiDataWithLocalStorage).sqlLab.queryEditors[0],
@@ -301,6 +307,13 @@ describe('getInitialState', () => {
name: expectedValue,
}),
);
+ expect(
+ getInitialState(apiDataWithLocalStorage).sqlLab.queryEditors,
+ ).not.toContainEqual(
+ expect.objectContaining({
+ id: '10',
+ }),
+ );
expect(
getInitialState(apiDataWithLocalStorage).sqlLab.lastUpdatedActiveTab,
).toEqual(apiDataWithTabState.active_tab.id.toString());
diff --git a/superset-frontend/src/SqlLab/reducers/getInitialState.ts
b/superset-frontend/src/SqlLab/reducers/getInitialState.ts
index 6a156bacfd..6052daedbd 100644
--- a/superset-frontend/src/SqlLab/reducers/getInitialState.ts
+++ b/superset-frontend/src/SqlLab/reducers/getInitialState.ts
@@ -146,6 +146,8 @@ export default function getInitialState({
}),
};
+ const destroyedQueryEditors = {};
+
/**
* If the `SQLLAB_BACKEND_PERSISTENCE` feature flag is off, or if the user
* hasn't used SQL Lab after it has been turned on, the state will be stored
@@ -219,6 +221,15 @@ export default function getInitialState({
tabHistory.push(...sqlLab.tabHistory);
}
lastUpdatedActiveTab = tabHistory.slice(tabHistory.length - 1)[0] ||
'';
+
+ if (sqlLab.destroyedQueryEditors) {
+ Object.entries(sqlLab.destroyedQueryEditors).forEach(([id, ts]) => {
+ if (queryEditors[id]) {
+ destroyedQueryEditors[id] = ts;
+ delete queryEditors[id];
+ }
+ });
+ }
}
}
} catch (error) {
@@ -253,6 +264,7 @@ export default function getInitialState({
queryCostEstimates: {},
unsavedQueryEditor,
lastUpdatedActiveTab,
+ destroyedQueryEditors,
},
localStorageUsageInKilobytes: 0,
common,
diff --git a/superset-frontend/src/SqlLab/reducers/sqlLab.js
b/superset-frontend/src/SqlLab/reducers/sqlLab.js
index f290d29f6a..5f0b27e97a 100644
--- a/superset-frontend/src/SqlLab/reducers/sqlLab.js
+++ b/superset-frontend/src/SqlLab/reducers/sqlLab.js
@@ -163,9 +163,18 @@ export default function sqlLabReducer(state = {}, action) {
...(action.queryEditor.id !== state.unsavedQueryEditor.id &&
state.unsavedQueryEditor),
},
+ destroyedQueryEditors: {
+ ...newState.destroyedQueryEditors,
+ [queryEditor.id]: Date.now(),
+ },
};
return newState;
},
+ [actions.CLEAR_DESTROYED_QUERY_EDITOR]() {
+ const destroyedQueryEditors = { ...state.destroyedQueryEditors };
+ delete destroyedQueryEditors[action.queryEditorId];
+ return { ...state, destroyedQueryEditors };
+ },
[actions.REMOVE_QUERY]() {
const newQueries = { ...state.queries };
delete newQueries[action.query.id];
diff --git a/superset-frontend/src/SqlLab/reducers/sqlLab.test.js
b/superset-frontend/src/SqlLab/reducers/sqlLab.test.js
index 0cf6b1d487..c3b603667b 100644
--- a/superset-frontend/src/SqlLab/reducers/sqlLab.test.js
+++ b/superset-frontend/src/SqlLab/reducers/sqlLab.test.js
@@ -267,6 +267,23 @@ describe('sqlLabReducer', () => {
expect(newState.tabHistory).toContain('updatedNewId');
expect(newState.tabHistory).not.toContain(qe.id);
});
+ it('should clear the destroyed query editors', () => {
+ const expectedQEId = '1233289';
+ const action = {
+ type: actions.CLEAR_DESTROYED_QUERY_EDITOR,
+ queryEditorId: expectedQEId,
+ };
+ newState = sqlLabReducer(
+ {
+ ...newState,
+ destroyedQueryEditors: {
+ [expectedQEId]: Date.now(),
+ },
+ },
+ action,
+ );
+ expect(newState.destroyedQueryEditors).toEqual({});
+ });
});
describe('Tables', () => {
let newState;
diff --git a/superset-frontend/src/SqlLab/types.ts
b/superset-frontend/src/SqlLab/types.ts
index 2dee58c223..eb49ca8ac1 100644
--- a/superset-frontend/src/SqlLab/types.ts
+++ b/superset-frontend/src/SqlLab/types.ts
@@ -110,6 +110,7 @@ export type SqlLabRootState = {
queryCostEstimates?: Record<string, QueryCostEstimate>;
editorTabLastUpdatedAt: number;
lastUpdatedActiveTab: string;
+ destroyedQueryEditors: Record<string, number>;
};
localStorageUsageInKilobytes: number;
messageToasts: toastState[];
diff --git a/superset-frontend/src/hooks/apiResources/sqlEditorTabs.test.ts
b/superset-frontend/src/hooks/apiResources/sqlEditorTabs.test.ts
index e2bb7c41e9..42f738f3c9 100644
--- a/superset-frontend/src/hooks/apiResources/sqlEditorTabs.test.ts
+++ b/superset-frontend/src/hooks/apiResources/sqlEditorTabs.test.ts
@@ -25,6 +25,7 @@ import {
import { api } from 'src/hooks/apiResources/queryApi';
import { LatestQueryEditorVersion } from 'src/SqlLab/types';
import {
+ useDeleteSqlEditorTabMutation,
useUpdateCurrentSqlEditorTabMutation,
useUpdateSqlEditorTabMutation,
} from './sqlEditorTabs';
@@ -120,3 +121,23 @@ test('posts activate request with queryEditorId', async ()
=> {
expect(fetchMock.calls(tabStateMutationApiRoute).length).toBe(1),
);
});
+
+test('deletes destoryed query editors', async () => {
+ const tabStateMutationApiRoute =
`glob:*/tabstateview/${expectedQueryEditor.id}`;
+ fetchMock.delete(tabStateMutationApiRoute, 200);
+ const { result, waitFor } = renderHook(
+ () => useDeleteSqlEditorTabMutation(),
+ {
+ wrapper: createWrapper({
+ useRedux: true,
+ store,
+ }),
+ },
+ );
+ act(() => {
+ result.current[0](expectedQueryEditor.id);
+ });
+ await waitFor(() =>
+ expect(fetchMock.calls(tabStateMutationApiRoute).length).toBe(1),
+ );
+});
diff --git a/superset-frontend/src/hooks/apiResources/sqlEditorTabs.ts
b/superset-frontend/src/hooks/apiResources/sqlEditorTabs.ts
index 29fe102235..90f4a057c1 100644
--- a/superset-frontend/src/hooks/apiResources/sqlEditorTabs.ts
+++ b/superset-frontend/src/hooks/apiResources/sqlEditorTabs.ts
@@ -73,10 +73,18 @@ const sqlEditorApi = api.injectEndpoints({
transformResponse: () => queryEditorId,
}),
}),
+ deleteSqlEditorTab: builder.mutation<void, string>({
+ query: queryEditorId => ({
+ method: 'DELETE',
+ endpoint: encodeURI(`/tabstateview/${queryEditorId}`),
+ transformResponse: () => queryEditorId,
+ }),
+ }),
}),
});
export const {
useUpdateSqlEditorTabMutation,
useUpdateCurrentSqlEditorTabMutation,
+ useDeleteSqlEditorTabMutation,
} = sqlEditorApi;
