You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "michael-s-molina (via GitHub)" <gi...@apache.org> on 2023/11/14 13:47:05 UTC
Re: [PR] feat(sqllab): non-blocking persistence mode [superset]
michael-s-molina commented on code in PR #24539:
URL: https://github.com/apache/superset/pull/24539#discussion_r1392553992
##########
superset-frontend/src/SqlLab/components/EditorAutoSync/EditorAutoSync.test.tsx:
##########
@@ -0,0 +1,181 @@
+/**
+ * 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.
+ */
+/**
+ * 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 React from 'react';
+import fetchMock from 'fetch-mock';
+import * as uiCore from '@superset-ui/core';
+import { render, act } from 'spec/helpers/testing-library';
+import ToastContainer from 'src/components/MessageToasts/ToastContainer';
+import { initialState, defaultQueryEditor } from 'src/SqlLab/fixtures';
+import EditorAutoSync from '.';
+
+const editorTabLastUpdatedAt = Date.now();
+const unsavedSqlLabState = {
+ ...initialState.sqlLab,
+ unsavedQueryEditor: {
+ id: defaultQueryEditor.id,
+ name: 'updated tab name',
+ updatedAt: editorTabLastUpdatedAt + 100,
+ },
+ editorTabLastUpdatedAt,
+};
+beforeAll(() => {
+ jest.useFakeTimers();
+});
+
+afterAll(() => {
+ jest.useRealTimers();
+});
+
+test('sync the unsaved editor tab state when there are new changes since the last update', async () => {
+ const updateEditorTabState = `glob:*/tabstateview/${defaultQueryEditor.id}`;
+ fetchMock.put(updateEditorTabState, 200);
+ const isFeatureEnabledMock = jest
+ .spyOn(uiCore, 'isFeatureEnabled')
+ .mockImplementation(
+ featureFlag =>
+ featureFlag === uiCore.FeatureFlag.SQLLAB_BACKEND_PERSISTENCE,
+ );
+ expect(fetchMock.calls(updateEditorTabState)).toHaveLength(0);
+ render(<EditorAutoSync />, {
+ useRedux: true,
+ initialState: {
+ ...initialState,
+ sqlLab: unsavedSqlLabState,
+ },
+ });
+ await act(async () => {
Review Comment:
```suggestion
await waitFor(() => jest.runAllTimers());
```
##########
superset-frontend/src/SqlLab/components/EditorAutoSync/index.tsx:
##########
@@ -0,0 +1,119 @@
+/**
+ * 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 React, { useRef, useEffect } from 'react';
+import { useDispatch, useSelector } from 'react-redux';
+import { FeatureFlag, isFeatureEnabled, t } from '@superset-ui/core';
+import {
+ SqlLabRootState,
+ QueryEditor,
+ UnsavedQueryEditor,
+} from 'src/SqlLab/types';
+import { useUpdateSqlEditorTabMutation } from 'src/hooks/apiResources/sqlEditorTabs';
+import { useDebounceValue } from 'src/hooks/useDebounceValue';
+import {
+ addDangerToast,
+ setEditorTabLastUpdate,
+} from 'src/SqlLab/actions/sqlLab';
+
+const INTERVAL = 5000;
+
+function hasUnsavedChanges(
+ queryEditor: QueryEditor,
+ lastSavedTimestamp: number,
+) {
+ return (
+ queryEditor.inLocalStorage ||
+ (queryEditor.updatedAt && queryEditor.updatedAt > lastSavedTimestamp)
+ );
+}
+
+export function filterUnsavedQueryEditorList(
+ queryEditors: QueryEditor[],
+ unsavedQueryEditor: UnsavedQueryEditor,
+ lastSavedTimestamp: number,
+) {
+ return queryEditors
+ .map(queryEditor => ({
+ ...queryEditor,
+ ...(unsavedQueryEditor.id === queryEditor.id && unsavedQueryEditor),
+ }))
+ .filter(queryEditor => hasUnsavedChanges(queryEditor, lastSavedTimestamp));
+}
+
+const EditorAutoSync: React.FC = () => {
+ const queryEditors = useSelector<SqlLabRootState, QueryEditor[]>(
+ state => state.sqlLab.queryEditors,
+ );
+ const unsavedQueryEditor = useSelector<SqlLabRootState, UnsavedQueryEditor>(
+ state => state.sqlLab.unsavedQueryEditor,
+ );
+ const editorTabLastUpdatedAt = useSelector<SqlLabRootState, number>(
+ state => state.sqlLab.editorTabLastUpdatedAt,
+ );
+ const dispatch = useDispatch();
+ const lastSavedTimestampRef = useRef<number>(editorTabLastUpdatedAt);
+ const [updateSqlEditor, { isError }] = useUpdateSqlEditorTabMutation();
+
+ const debouncedUnsavedQueryEditor = useDebounceValue(
+ unsavedQueryEditor,
+ INTERVAL,
+ );
+
+ useEffect(() => {
+ if (!isFeatureEnabled(FeatureFlag.SQLLAB_BACKEND_PERSISTENCE)) {
+ return;
+ }
+ const unsaved = filterUnsavedQueryEditorList(
+ queryEditors,
+ debouncedUnsavedQueryEditor,
+ lastSavedTimestampRef.current,
+ );
+
+ Promise.all(
+ unsaved
+ // TODO: Migrate migrateQueryEditorFromLocalStorage
+ // in TabbedSqlEditors logic by addSqlEditor mutation later
+ .filter(({ inLocalStorage }) => !inLocalStorage)
+ .map(queryEditor => updateSqlEditor({ queryEditor })),
+ ).then(resolvers => {
+ if (!resolvers.some(result => 'error' in result)) {
+ lastSavedTimestampRef.current = Date.now();
+ dispatch(setEditorTabLastUpdate(lastSavedTimestampRef.current));
+ }
+ });
+ }, [debouncedUnsavedQueryEditor, dispatch, queryEditors, updateSqlEditor]);
+
+ useEffect(() => {
+ if (isError) {
Review Comment:
Should this be a danger toast or just a console warning given that we're still able to save the state to local storage?
##########
superset-frontend/src/SqlLab/components/EditorAutoSync/EditorAutoSync.test.tsx:
##########
@@ -0,0 +1,181 @@
+/**
+ * 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.
+ */
+/**
+ * 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 React from 'react';
+import fetchMock from 'fetch-mock';
+import * as uiCore from '@superset-ui/core';
+import { render, act } from 'spec/helpers/testing-library';
+import ToastContainer from 'src/components/MessageToasts/ToastContainer';
+import { initialState, defaultQueryEditor } from 'src/SqlLab/fixtures';
+import EditorAutoSync from '.';
+
+const editorTabLastUpdatedAt = Date.now();
+const unsavedSqlLabState = {
+ ...initialState.sqlLab,
+ unsavedQueryEditor: {
+ id: defaultQueryEditor.id,
+ name: 'updated tab name',
+ updatedAt: editorTabLastUpdatedAt + 100,
+ },
+ editorTabLastUpdatedAt,
+};
+beforeAll(() => {
+ jest.useFakeTimers();
+});
+
+afterAll(() => {
+ jest.useRealTimers();
+});
+
+test('sync the unsaved editor tab state when there are new changes since the last update', async () => {
+ const updateEditorTabState = `glob:*/tabstateview/${defaultQueryEditor.id}`;
+ fetchMock.put(updateEditorTabState, 200);
+ const isFeatureEnabledMock = jest
+ .spyOn(uiCore, 'isFeatureEnabled')
+ .mockImplementation(
+ featureFlag =>
+ featureFlag === uiCore.FeatureFlag.SQLLAB_BACKEND_PERSISTENCE,
+ );
+ expect(fetchMock.calls(updateEditorTabState)).toHaveLength(0);
+ render(<EditorAutoSync />, {
+ useRedux: true,
+ initialState: {
+ ...initialState,
+ sqlLab: unsavedSqlLabState,
+ },
+ });
+ await act(async () => {
+ jest.runAllTimers();
+ });
+ expect(fetchMock.calls(updateEditorTabState)).toHaveLength(1);
+ isFeatureEnabledMock.mockClear();
+ fetchMock.restore();
+});
+
+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);
+ const isFeatureEnabledMock = jest
+ .spyOn(uiCore, 'isFeatureEnabled')
+ .mockImplementation(
+ featureFlag =>
+ featureFlag === uiCore.FeatureFlag.SQLLAB_BACKEND_PERSISTENCE,
+ );
+ expect(fetchMock.calls(updateEditorTabState)).toHaveLength(0);
+ render(<EditorAutoSync />, {
+ useRedux: true,
+ initialState: {
+ ...initialState,
+ sqlLab: {
+ ...initialState.sqlLab,
+ unsavedQueryEditor: {
+ id: defaultQueryEditor.id,
+ name: 'updated tab name',
+ updatedAt: editorTabLastUpdatedAt - 100,
+ },
+ editorTabLastUpdatedAt,
+ },
+ },
+ });
+ await act(async () => {
+ jest.runAllTimers();
+ });
+ expect(fetchMock.calls(updateEditorTabState)).toHaveLength(0);
+ isFeatureEnabledMock.mockClear();
+ fetchMock.restore();
+});
+
+test('renders an error toast when the sync failed', async () => {
+ const updateEditorTabState = `glob:*/tabstateview/${defaultQueryEditor.id}`;
+ fetchMock.put(updateEditorTabState, {
+ throws: new Error('errorMessage'),
+ });
+ const isFeatureEnabledMock = jest
+ .spyOn(uiCore, 'isFeatureEnabled')
+ .mockImplementation(
+ featureFlag =>
+ featureFlag === uiCore.FeatureFlag.SQLLAB_BACKEND_PERSISTENCE,
+ );
+ expect(fetchMock.calls(updateEditorTabState)).toHaveLength(0);
+ const { findByText } = render(
+ <>
+ <EditorAutoSync />
+ <ToastContainer />
+ </>,
+ {
+ useRedux: true,
+ initialState: {
+ ...initialState,
+ sqlLab: unsavedSqlLabState,
+ },
+ },
+ );
+ await act(async () => {
+ jest.runAllTimers();
+ });
+ const errorToast = await findByText(
+ 'An error occurred while saving your editor state. ' +
+ 'Please contact your administrator if this problem persists.',
+ );
+ expect(errorToast).toBeTruthy();
+ isFeatureEnabledMock.mockClear();
+ fetchMock.restore();
+});
+
+test('skip syncing the unsaved editor tab stat when SQLLAB_BACKEND_PERSISTENCE is off', async () => {
+ const updateEditorTabState = `glob:*/tabstateview/${defaultQueryEditor.id}`;
+ fetchMock.put(updateEditorTabState, 200);
+ const isFeatureEnabledMock = jest
+ .spyOn(uiCore, 'isFeatureEnabled')
+ .mockImplementation(
+ featureFlag =>
+ featureFlag !== uiCore.FeatureFlag.SQLLAB_BACKEND_PERSISTENCE,
+ );
+ expect(fetchMock.calls(updateEditorTabState)).toHaveLength(0);
+ render(<EditorAutoSync />, {
+ useRedux: true,
+ initialState: {
+ ...initialState,
+ sqlLab: unsavedSqlLabState,
+ },
+ });
+ await act(async () => {
+ jest.runAllTimers();
+ });
Review Comment:
```suggestion
await waitFor(() => jest.runAllTimers());
```
##########
superset-frontend/src/hooks/apiResources/sqlEditorTabs.ts:
##########
@@ -0,0 +1,69 @@
+/**
+ * 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 { pickBy } from 'lodash';
+import { QueryEditor } from 'src/SqlLab/types';
+import { api, JsonResponse } from './queryApi';
+
+export type EditorMutationParams = {
+ queryEditor: QueryEditor;
+ extra?: Record<string, any>;
+};
+
+const sqlEditorApi = api.injectEndpoints({
+ endpoints: builder => ({
+ updateSqlEditorTab: builder.mutation<JsonResponse, EditorMutationParams>({
+ query: ({
+ queryEditor: {
Review Comment:
Given that the query editor state is saved as a JSON object, it would be a good idea to add a version field to the object. If we have schema changes in the future, we'll be able to create transformers between the different versions and preserve compatibility. This is one of the main problems we face today with other JSON objects.
##########
superset-frontend/src/SqlLab/components/EditorAutoSync/EditorAutoSync.test.tsx:
##########
@@ -0,0 +1,181 @@
+/**
+ * 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.
+ */
+/**
+ * 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 React from 'react';
+import fetchMock from 'fetch-mock';
+import * as uiCore from '@superset-ui/core';
+import { render, act } from 'spec/helpers/testing-library';
Review Comment:
```suggestion
import { render, waitFor } from 'spec/helpers/testing-library';
```
##########
superset-frontend/src/SqlLab/reducers/getInitialState.test.ts:
##########
@@ -245,4 +245,109 @@ describe('getInitialState', () => {
);
});
});
+
+ describe('restore unsaved changes for PERSISTENCE mode', () => {
+ const lastUpdatedTime = Date.now();
+ const expectedValue = 'updated editor value';
+ beforeEach(() => {
+ localStorage.setItem(
+ 'redux',
+ JSON.stringify({
+ sqlLab: {
+ queryEditors: [
+ {
+ // restore cached value since updates are after server update time
+ id: '1',
+ name: expectedValue,
+ updatedAt: lastUpdatedTime + 100,
+ },
+ {
+ // out of update since last updated time is before server update time
+ id: '2',
+ name: expectedValue,
+ updatedAt: lastUpdatedTime - 100,
+ },
+ {
+ // out of update since no updatedAt
Review Comment:
```suggestion
// no update required given that there's no updatedAt
```
##########
superset-frontend/src/SqlLab/components/EditorAutoSync/EditorAutoSync.test.tsx:
##########
@@ -0,0 +1,181 @@
+/**
+ * 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.
+ */
+/**
+ * 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 React from 'react';
+import fetchMock from 'fetch-mock';
+import * as uiCore from '@superset-ui/core';
+import { render, act } from 'spec/helpers/testing-library';
+import ToastContainer from 'src/components/MessageToasts/ToastContainer';
+import { initialState, defaultQueryEditor } from 'src/SqlLab/fixtures';
+import EditorAutoSync from '.';
+
+const editorTabLastUpdatedAt = Date.now();
+const unsavedSqlLabState = {
+ ...initialState.sqlLab,
+ unsavedQueryEditor: {
+ id: defaultQueryEditor.id,
+ name: 'updated tab name',
+ updatedAt: editorTabLastUpdatedAt + 100,
+ },
+ editorTabLastUpdatedAt,
+};
+beforeAll(() => {
+ jest.useFakeTimers();
+});
+
+afterAll(() => {
+ jest.useRealTimers();
+});
+
+test('sync the unsaved editor tab state when there are new changes since the last update', async () => {
+ const updateEditorTabState = `glob:*/tabstateview/${defaultQueryEditor.id}`;
+ fetchMock.put(updateEditorTabState, 200);
+ const isFeatureEnabledMock = jest
+ .spyOn(uiCore, 'isFeatureEnabled')
+ .mockImplementation(
+ featureFlag =>
+ featureFlag === uiCore.FeatureFlag.SQLLAB_BACKEND_PERSISTENCE,
+ );
+ expect(fetchMock.calls(updateEditorTabState)).toHaveLength(0);
+ render(<EditorAutoSync />, {
+ useRedux: true,
+ initialState: {
+ ...initialState,
+ sqlLab: unsavedSqlLabState,
+ },
+ });
+ await act(async () => {
+ jest.runAllTimers();
+ });
+ expect(fetchMock.calls(updateEditorTabState)).toHaveLength(1);
+ isFeatureEnabledMock.mockClear();
+ fetchMock.restore();
+});
+
+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);
+ const isFeatureEnabledMock = jest
+ .spyOn(uiCore, 'isFeatureEnabled')
+ .mockImplementation(
+ featureFlag =>
+ featureFlag === uiCore.FeatureFlag.SQLLAB_BACKEND_PERSISTENCE,
+ );
+ expect(fetchMock.calls(updateEditorTabState)).toHaveLength(0);
+ render(<EditorAutoSync />, {
+ useRedux: true,
+ initialState: {
+ ...initialState,
+ sqlLab: {
+ ...initialState.sqlLab,
+ unsavedQueryEditor: {
+ id: defaultQueryEditor.id,
+ name: 'updated tab name',
+ updatedAt: editorTabLastUpdatedAt - 100,
+ },
+ editorTabLastUpdatedAt,
+ },
+ },
+ });
+ await act(async () => {
+ jest.runAllTimers();
+ });
Review Comment:
```suggestion
await waitFor(() => jest.runAllTimers());
```
##########
superset-frontend/src/SqlLab/components/EditorAutoSync/EditorAutoSync.test.tsx:
##########
@@ -0,0 +1,181 @@
+/**
+ * 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.
+ */
+/**
+ * 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 React from 'react';
+import fetchMock from 'fetch-mock';
+import * as uiCore from '@superset-ui/core';
+import { render, act } from 'spec/helpers/testing-library';
+import ToastContainer from 'src/components/MessageToasts/ToastContainer';
+import { initialState, defaultQueryEditor } from 'src/SqlLab/fixtures';
+import EditorAutoSync from '.';
+
+const editorTabLastUpdatedAt = Date.now();
+const unsavedSqlLabState = {
+ ...initialState.sqlLab,
+ unsavedQueryEditor: {
+ id: defaultQueryEditor.id,
+ name: 'updated tab name',
+ updatedAt: editorTabLastUpdatedAt + 100,
+ },
+ editorTabLastUpdatedAt,
+};
+beforeAll(() => {
+ jest.useFakeTimers();
+});
+
+afterAll(() => {
+ jest.useRealTimers();
+});
+
+test('sync the unsaved editor tab state when there are new changes since the last update', async () => {
+ const updateEditorTabState = `glob:*/tabstateview/${defaultQueryEditor.id}`;
+ fetchMock.put(updateEditorTabState, 200);
+ const isFeatureEnabledMock = jest
+ .spyOn(uiCore, 'isFeatureEnabled')
+ .mockImplementation(
+ featureFlag =>
+ featureFlag === uiCore.FeatureFlag.SQLLAB_BACKEND_PERSISTENCE,
+ );
+ expect(fetchMock.calls(updateEditorTabState)).toHaveLength(0);
+ render(<EditorAutoSync />, {
+ useRedux: true,
+ initialState: {
+ ...initialState,
+ sqlLab: unsavedSqlLabState,
+ },
+ });
+ await act(async () => {
+ jest.runAllTimers();
+ });
+ expect(fetchMock.calls(updateEditorTabState)).toHaveLength(1);
+ isFeatureEnabledMock.mockClear();
+ fetchMock.restore();
+});
+
+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);
+ const isFeatureEnabledMock = jest
+ .spyOn(uiCore, 'isFeatureEnabled')
+ .mockImplementation(
+ featureFlag =>
+ featureFlag === uiCore.FeatureFlag.SQLLAB_BACKEND_PERSISTENCE,
+ );
+ expect(fetchMock.calls(updateEditorTabState)).toHaveLength(0);
+ render(<EditorAutoSync />, {
+ useRedux: true,
+ initialState: {
+ ...initialState,
+ sqlLab: {
+ ...initialState.sqlLab,
+ unsavedQueryEditor: {
+ id: defaultQueryEditor.id,
+ name: 'updated tab name',
+ updatedAt: editorTabLastUpdatedAt - 100,
+ },
+ editorTabLastUpdatedAt,
+ },
+ },
+ });
+ await act(async () => {
+ jest.runAllTimers();
+ });
+ expect(fetchMock.calls(updateEditorTabState)).toHaveLength(0);
+ isFeatureEnabledMock.mockClear();
+ fetchMock.restore();
+});
+
+test('renders an error toast when the sync failed', async () => {
+ const updateEditorTabState = `glob:*/tabstateview/${defaultQueryEditor.id}`;
+ fetchMock.put(updateEditorTabState, {
+ throws: new Error('errorMessage'),
+ });
+ const isFeatureEnabledMock = jest
+ .spyOn(uiCore, 'isFeatureEnabled')
+ .mockImplementation(
+ featureFlag =>
+ featureFlag === uiCore.FeatureFlag.SQLLAB_BACKEND_PERSISTENCE,
+ );
+ expect(fetchMock.calls(updateEditorTabState)).toHaveLength(0);
+ const { findByText } = render(
+ <>
+ <EditorAutoSync />
+ <ToastContainer />
+ </>,
+ {
+ useRedux: true,
+ initialState: {
+ ...initialState,
+ sqlLab: unsavedSqlLabState,
+ },
+ },
+ );
+ await act(async () => {
+ jest.runAllTimers();
+ });
Review Comment:
```suggestion
await waitFor(() => jest.runAllTimers());
```
##########
superset-frontend/src/SqlLab/middlewares/persistSqlLabStateEnhancer.js:
##########
@@ -80,6 +118,8 @@ const sqlLabPersistStateConfig = {
};
export const persistSqlLabStateEnhancer = persistState(
- sqlLabPersistStateConfig.paths,
+ isFeatureEnabled(FeatureFlag.SQLLAB_BACKEND_PERSISTENCE)
+ ? [PERSISTENCE_LOCAL_STORAGE_PATH]
+ : sqlLabPersistStateConfig.paths,
Review Comment:
```suggestion
sqlLabPersistStateConfig.paths,
```
##########
superset-frontend/src/SqlLab/reducers/getInitialState.test.ts:
##########
@@ -245,4 +245,109 @@ describe('getInitialState', () => {
);
});
});
+
+ describe('restore unsaved changes for PERSISTENCE mode', () => {
+ const lastUpdatedTime = Date.now();
+ const expectedValue = 'updated editor value';
+ beforeEach(() => {
+ localStorage.setItem(
+ 'redux',
+ JSON.stringify({
+ sqlLab: {
+ queryEditors: [
+ {
+ // restore cached value since updates are after server update time
+ id: '1',
+ name: expectedValue,
+ updatedAt: lastUpdatedTime + 100,
+ },
+ {
+ // out of update since last updated time is before server update time
Review Comment:
```suggestion
// no update required given that last updated time comes before server update time
```
##########
superset-frontend/src/SqlLab/components/EditorAutoSync/index.tsx:
##########
@@ -0,0 +1,119 @@
+/**
+ * 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 React, { useRef, useEffect } from 'react';
+import { useDispatch, useSelector } from 'react-redux';
+import { FeatureFlag, isFeatureEnabled, t } from '@superset-ui/core';
+import {
+ SqlLabRootState,
+ QueryEditor,
+ UnsavedQueryEditor,
+} from 'src/SqlLab/types';
+import { useUpdateSqlEditorTabMutation } from 'src/hooks/apiResources/sqlEditorTabs';
+import { useDebounceValue } from 'src/hooks/useDebounceValue';
+import {
+ addDangerToast,
+ setEditorTabLastUpdate,
+} from 'src/SqlLab/actions/sqlLab';
+
+const INTERVAL = 5000;
+
+function hasUnsavedChanges(
+ queryEditor: QueryEditor,
+ lastSavedTimestamp: number,
+) {
+ return (
+ queryEditor.inLocalStorage ||
+ (queryEditor.updatedAt && queryEditor.updatedAt > lastSavedTimestamp)
+ );
+}
+
+export function filterUnsavedQueryEditorList(
+ queryEditors: QueryEditor[],
+ unsavedQueryEditor: UnsavedQueryEditor,
+ lastSavedTimestamp: number,
+) {
+ return queryEditors
+ .map(queryEditor => ({
+ ...queryEditor,
+ ...(unsavedQueryEditor.id === queryEditor.id && unsavedQueryEditor),
+ }))
+ .filter(queryEditor => hasUnsavedChanges(queryEditor, lastSavedTimestamp));
+}
+
+const EditorAutoSync: React.FC = () => {
+ const queryEditors = useSelector<SqlLabRootState, QueryEditor[]>(
+ state => state.sqlLab.queryEditors,
+ );
+ const unsavedQueryEditor = useSelector<SqlLabRootState, UnsavedQueryEditor>(
+ state => state.sqlLab.unsavedQueryEditor,
+ );
+ const editorTabLastUpdatedAt = useSelector<SqlLabRootState, number>(
+ state => state.sqlLab.editorTabLastUpdatedAt,
+ );
+ const dispatch = useDispatch();
+ const lastSavedTimestampRef = useRef<number>(editorTabLastUpdatedAt);
+ const [updateSqlEditor, { isError }] = useUpdateSqlEditorTabMutation();
+
+ const debouncedUnsavedQueryEditor = useDebounceValue(
+ unsavedQueryEditor,
+ INTERVAL,
+ );
+
+ useEffect(() => {
+ if (!isFeatureEnabled(FeatureFlag.SQLLAB_BACKEND_PERSISTENCE)) {
+ return;
+ }
Review Comment:
This check is already done by its parent as it should given that this component does nothing without the feature flag.
```suggestion
```
##########
superset-frontend/src/SqlLab/middlewares/persistSqlLabStateEnhancer.js:
##########
@@ -32,12 +35,47 @@ const CLEAR_ENTITY_HELPERS_MAP = {
unsavedQueryEditor: qe => clearQueryEditors([qe])[0],
};
+const PERSISTENCE_LOCAL_STORAGE_PATH = 'sqlLab.unsavedQueryEditor';
+
const sqlLabPersistStateConfig = {
paths: ['sqlLab'],
config: {
slicer: paths => state => {
const subset = {};
paths.forEach(path => {
+ if (path === PERSISTENCE_LOCAL_STORAGE_PATH) {
Review Comment:
```suggestion
if (isFeatureEnabled(FeatureFlag.SQLLAB_BACKEND_PERSISTENCE)) {
```
##########
superset-frontend/src/SqlLab/components/EditorAutoSync/index.tsx:
##########
@@ -0,0 +1,119 @@
+/**
+ * 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 React, { useRef, useEffect } from 'react';
+import { useDispatch, useSelector } from 'react-redux';
+import { FeatureFlag, isFeatureEnabled, t } from '@superset-ui/core';
+import {
+ SqlLabRootState,
+ QueryEditor,
+ UnsavedQueryEditor,
+} from 'src/SqlLab/types';
+import { useUpdateSqlEditorTabMutation } from 'src/hooks/apiResources/sqlEditorTabs';
+import { useDebounceValue } from 'src/hooks/useDebounceValue';
+import {
+ addDangerToast,
+ setEditorTabLastUpdate,
+} from 'src/SqlLab/actions/sqlLab';
+
+const INTERVAL = 5000;
Review Comment:
Maybe rename it to `SYNC_INTERVAL` to make it more descriptive?
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org