You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "justinpark (via GitHub)" <gi...@apache.org> on 2023/06/28 05:40:18 UTC
[GitHub] [superset] justinpark opened a new pull request, #24539: feat(sqllab): non-blocking persistence mode
justinpark opened a new pull request, #24539:
URL: https://github.com/apache/superset/pull/24539
### SUMMARY
Following up [SIP-93](https://github.com/apache/superset/issues/21385), this commit implements the first step of non-blocking persistence mode for the current working sqllab editor activities.
As described in [SIP-93](https://github.com/apache/superset/issues/21385), the existing PERSISTENCE logic causes a user interaction delay due to the network (or server side) lagging. For example, user had to wait the server response in order to update the tab name or query limit count. (see the before screenshot)
Therefore, this commit aims to improve current tab activities by replacing the following blocking logics with a non-blocking way by cumulating the changes and syncing them with the latest updates asynchronously:
```
- querySuccess
- queryFailed
- toggleLeftBar
- queryEditorSetDb
- queryEditorSetSchema
- queryEditorSetAutorun
- queryEditorSetTitle
- queryEditorSetSql
- queryEditorSetQueryLimit
- queryEditorSetTemplateParams
```
#### Mechanism for syncing unsaved changes
There can be unsaved changes since it is syncing in a non-blocking way and asynchronously. To address this issue, unsaved changes will be stored in local storage (using localStorage in persistence mode) until they are successfully saved on the server. This hybrid approach ensures that user data is not lost and that any unsaved changes are retrieved, even in the event of an unexpected interruption.
#### Resolve conflicts
When a user opens multiple windows, they may be overwritten by the inactive editor tab state. To prevent this unexpected overwrite, the latest update time will be compared with the local cache state, and the out-of-updated state will be discarded. This ensures that users can work on multiple windows without losing their progress or having their work overwritten.
#### Following up
Creating new tabs and removing tabs are still blocking way in this commit but will be migrated into the non-blocking way in future PRs. Additionally, the existing migrateQueryEditorFromLocalStorage logic will be consolidated or replaced by the AutoSync with mutation APIs in the upcoming PRs. These changes will further improve the user experience by making the entire process non-blocking and asynchronous.
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
After:
https://github.com/apache/superset/assets/1392866/3d5b2817-add5-41c8-9071-890e82e81615
Before:
https://github.com/apache/superset/assets/1392866/7efa5a29-b7e0-4698-bb39-f4295693588d
### TESTING INSTRUCTIONS
Set SQLLAB_BACKEND_PERSISTENCE true and play with the SQL Lab editor and keep hard refresh to verify the persisted states.
### ADDITIONAL INFORMATION
- [x] Has associated issue: [SIP-93](https://github.com/apache/superset/issues/21385)
- [x] Required feature flags: `SQLLAB_BACKEND_PERSISTENCE`
- [ ] Changes UI
- [ ] Includes DB Migration (follow approval process in [SIP-59](https://github.com/apache/superset/issues/13351))
- [ ] Migration is atomic, supports rollback & is backwards-compatible
- [ ] Confirm DB migration upgrade and downgrade tested
- [ ] Runtime estimates and downtime expectations provided
- [x] Introduces new feature or API
- [ ] Removes existing feature or API
--
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
[GitHub] [superset] justinpark commented on pull request #24539: feat(sqllab): non-blocking persistence mode
Posted by "justinpark (via GitHub)" <gi...@apache.org>.
justinpark commented on PR #24539:
URL: https://github.com/apache/superset/pull/24539#issuecomment-1635050349
@sadpandajoe Thanks for testing. The missing columns on autocomplete has been fixed from #24611 and I also rebased this branch to include the fix.
I also fixed the bug that is related to the tab switching with a new query editor. Could you try the test cases again?
--
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
[GitHub] [superset] sadpandajoe commented on pull request #24539: feat(sqllab): non-blocking persistence mode
Posted by "sadpandajoe (via GitHub)" <gi...@apache.org>.
sadpandajoe commented on PR #24539:
URL: https://github.com/apache/superset/pull/24539#issuecomment-1633199048
@justinpark is anything cached? I noticed that for columns it didn't show up the first time and then for another schema it works
--
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
[GitHub] [superset] eschutho commented on pull request #24539: feat(sqllab): non-blocking persistence mode
Posted by "eschutho (via GitHub)" <gi...@apache.org>.
eschutho commented on PR #24539:
URL: https://github.com/apache/superset/pull/24539#issuecomment-1665126286
> > does ephemeral logins work for either of you @michael-s-molina / @justinpark? I haven't been testing since I can't log in.
>
> > @sadpandajoe same problem. @michael-s-molina have you faced same problem?
>
> Ephemeral environments are not working with `TALISMAN_ENABLED = True`. @eschutho was working on a [fix](https://github.com/apache/superset/pull/24774) but it's not merged yet. Given that `TALISMAN_ENABLED` is not a feature flag, we can't disable it so we'll need to pull this PR locally to test it.
The talisman Pr is ready for review now.
--
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
[GitHub] [superset] michael-s-molina commented on a diff in pull request #24539: feat(sqllab): non-blocking persistence mode
Posted by "michael-s-molina (via GitHub)" <gi...@apache.org>.
michael-s-molina commented on code in PR #24539:
URL: https://github.com/apache/superset/pull/24539#discussion_r1260318616
##########
superset-frontend/src/SqlLab/App.jsx:
##########
@@ -56,6 +60,37 @@ const sqlLabPersistStateConfig = {
slicer: paths => state => {
const subset = {};
paths.forEach(path => {
+ if (path === 'sqlLab.unsavedQueryEditor') {
+ const {
+ queryEditors,
+ editorTabLastUpdatedAt,
+ unsavedQueryEditor,
+ tables,
+ queries,
+ tabHistory,
+ } = state.sqlLab;
+ const unsavedQueryEditors = filterUnsavedQueryEditorList(
+ queryEditors,
+ unsavedQueryEditor,
+ editorTabLastUpdatedAt,
+ );
+ const hasFinishedMigrationFromLocalStorage =
+ unsavedQueryEditors.every(({ inLocalStorage }) => !inLocalStorage);
+ if (unsavedQueryEditors.length > 0) {
Review Comment:
```suggestion
if (unsavedQueryEditors.length > 0) {
const hasFinishedMigrationFromLocalStorage =
unsavedQueryEditors.every(
({ inLocalStorage }) => !inLocalStorage,
);
```
##########
superset-frontend/src/SqlLab/App.jsx:
##########
@@ -56,6 +60,37 @@ const sqlLabPersistStateConfig = {
slicer: paths => state => {
const subset = {};
paths.forEach(path => {
+ if (path === 'sqlLab.unsavedQueryEditor') {
Review Comment:
Could you extract a constant with `'sqlLab.unsavedQueryEditor'`?
##########
superset-frontend/src/SqlLab/actions/sqlLab.js:
##########
@@ -1037,58 +885,18 @@ export function queryEditorSetAndSaveSql(targetQueryEditor, sql) {
}
export function queryEditorSetQueryLimit(queryEditor, queryLimit) {
- return function (dispatch) {
- const sync = isFeatureEnabled(FeatureFlag.SQLLAB_BACKEND_PERSISTENCE)
- ? SupersetClient.put({
- endpoint: encodeURI(`/tabstateview/${queryEditor.id}`),
- postPayload: { query_limit: queryLimit },
- })
- : Promise.resolve();
-
- return sync
- .then(() =>
- dispatch({
- type: QUERY_EDITOR_SET_QUERY_LIMIT,
- queryEditor,
- queryLimit,
- }),
- )
- .catch(() =>
- dispatch(
- addDangerToast(
- t(
- 'An error occurred while setting the tab name. Please contact your administrator.',
- ),
- ),
- ),
- );
+ return {
+ type: QUERY_EDITOR_SET_QUERY_LIMIT,
+ queryEditor,
+ queryLimit,
};
}
export function queryEditorSetTemplateParams(queryEditor, templateParams) {
- return function (dispatch) {
- dispatch({
- type: QUERY_EDITOR_SET_TEMPLATE_PARAMS,
- queryEditor,
- templateParams,
- });
- const sync = isFeatureEnabled(FeatureFlag.SQLLAB_BACKEND_PERSISTENCE)
- ? SupersetClient.put({
- endpoint: encodeURI(`/tabstateview/${queryEditor.id}`),
- postPayload: { template_params: templateParams },
- })
- : Promise.resolve();
-
- return sync.catch(() =>
- dispatch(
- addDangerToast(
- t(
- 'An error occurred while setting the tab template parameters. ' +
- 'Please contact your administrator.',
- ),
- ),
- ),
- );
+ return {
Review Comment:
Nice cleanup! A lot of duplicated logic in the old actions!
##########
superset-frontend/src/SqlLab/components/EditorAutoSync/index.tsx:
##########
@@ -0,0 +1,120 @@
+/**
+ * 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, t } from '@superset-ui/core';
+import {
+ SqlLabRootState,
+ QueryEditor,
+ UnsavedQueryEditor,
+} from 'src/SqlLab/types';
+import { isFeatureEnabled } from 'src/featureFlags';
+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,
+ lastSavedDateTime: number,
+) {
+ return (
+ queryEditor.inLocalStorage ||
+ (queryEditor.updatedAt && queryEditor.updatedAt > lastSavedDateTime)
+ );
+}
+
+export function filterUnsavedQueryEditorList(
+ queryEditors: QueryEditor[],
+ unsavedQueryEditor: UnsavedQueryEditor,
+ lastSavedDateTime: number,
+) {
+ return queryEditors
+ .map(queryEditor => ({
+ ...queryEditor,
+ ...(unsavedQueryEditor.id === queryEditor.id && unsavedQueryEditor),
+ }))
+ .filter(queryEditor => hasUnsavedChanges(queryEditor, lastSavedDateTime));
+}
+
+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 lastSavedDateTimeRef = 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,
+ lastSavedDateTimeRef.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)) {
+ lastSavedDateTimeRef.current = Date.now();
+ dispatch(setEditorTabLastUpdate(lastSavedDateTimeRef.current));
+ }
+ });
+ }, [debouncedUnsavedQueryEditor, dispatch, queryEditors, updateSqlEditor]);
+
+ useEffect(() => {
+ if (isError) {
+ dispatch(
+ addDangerToast(
+ t(
+ 'An error occurred while storing your query in the backend. ' +
Review Comment:
```suggestion
'An error occurred while saving your editor state. ' +
```
##########
superset-frontend/src/SqlLab/components/EditorAutoSync/EditorAutoSync.test.tsx:
##########
@@ -0,0 +1,178 @@
+/**
+ * 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 featureFlags from 'src/featureFlags';
+import { FeatureFlag } 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 with api when unsaved has made since lastest updated', async () => {
+ const updateEditorTabState = `glob:*/tabstateview/${defaultQueryEditor.id}`;
+ fetchMock.put(updateEditorTabState, 200);
+ const isFeatureEnabledMock = jest
+ .spyOn(featureFlags, 'isFeatureEnabled')
+ .mockImplementation(
+ featureFlag => featureFlag === 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 already synced', async () => {
+ const updateEditorTabState = `glob:*/tabstateview/${defaultQueryEditor.id}`;
+ fetchMock.put(updateEditorTabState, 200);
+ const isFeatureEnabledMock = jest
+ .spyOn(featureFlags, 'isFeatureEnabled')
+ .mockImplementation(
+ featureFlag => featureFlag === 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 api update failed', async () => {
+ const updateEditorTabState = `glob:*/tabstateview/${defaultQueryEditor.id}`;
+ fetchMock.put(updateEditorTabState, {
+ throws: new Error('errorMessage'),
+ });
+ const isFeatureEnabledMock = jest
+ .spyOn(featureFlags, 'isFeatureEnabled')
+ .mockImplementation(
+ featureFlag => featureFlag === 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 storing your query in the backend. ' +
+ 'Please contact your administrator if this problem persists.',
+ );
+ expect(errorToast).toBeTruthy();
+ isFeatureEnabledMock.mockClear();
+ fetchMock.restore();
+});
+
+test('skip syncing the unsaved editor tab state with api when SQLLAB_BACKEND_PERSISTENCE is off', async () => {
Review Comment:
```suggestion
test('skip syncing the unsaved editor tab state when SQLLAB_BACKEND_PERSISTENCE is off', async () => {
```
##########
superset-frontend/src/SqlLab/components/EditorAutoSync/EditorAutoSync.test.tsx:
##########
@@ -0,0 +1,178 @@
+/**
+ * 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 featureFlags from 'src/featureFlags';
+import { FeatureFlag } 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 with api when unsaved has made since lastest updated', async () => {
Review Comment:
```suggestion
test('sync the unsaved editor tab state when there are new changes since the last update', async () => {
```
##########
superset-frontend/src/SqlLab/components/EditorAutoSync/index.tsx:
##########
@@ -0,0 +1,120 @@
+/**
+ * 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, t } from '@superset-ui/core';
+import {
+ SqlLabRootState,
+ QueryEditor,
+ UnsavedQueryEditor,
+} from 'src/SqlLab/types';
+import { isFeatureEnabled } from 'src/featureFlags';
+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,
+ lastSavedDateTime: number,
Review Comment:
Maybe rename all occurrences of `lastSavedDateTime` to `lastSavedTimestamp`?
##########
superset-frontend/src/SqlLab/components/EditorAutoSync/EditorAutoSync.test.tsx:
##########
@@ -0,0 +1,178 @@
+/**
+ * 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 featureFlags from 'src/featureFlags';
+import { FeatureFlag } 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 with api when unsaved has made since lastest updated', async () => {
+ const updateEditorTabState = `glob:*/tabstateview/${defaultQueryEditor.id}`;
+ fetchMock.put(updateEditorTabState, 200);
+ const isFeatureEnabledMock = jest
+ .spyOn(featureFlags, 'isFeatureEnabled')
+ .mockImplementation(
+ featureFlag => featureFlag === 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 already synced', async () => {
+ const updateEditorTabState = `glob:*/tabstateview/${defaultQueryEditor.id}`;
+ fetchMock.put(updateEditorTabState, 200);
+ const isFeatureEnabledMock = jest
+ .spyOn(featureFlags, 'isFeatureEnabled')
+ .mockImplementation(
+ featureFlag => featureFlag === 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 api update failed', async () => {
Review Comment:
```suggestion
test('renders an error toast when the sync fails', async () => {
```
##########
superset-frontend/src/SqlLab/components/EditorAutoSync/EditorAutoSync.test.tsx:
##########
@@ -0,0 +1,178 @@
+/**
+ * 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 featureFlags from 'src/featureFlags';
+import { FeatureFlag } 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 with api when unsaved has made since lastest updated', async () => {
+ const updateEditorTabState = `glob:*/tabstateview/${defaultQueryEditor.id}`;
+ fetchMock.put(updateEditorTabState, 200);
+ const isFeatureEnabledMock = jest
+ .spyOn(featureFlags, 'isFeatureEnabled')
+ .mockImplementation(
+ featureFlag => featureFlag === 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 already synced', async () => {
Review Comment:
```suggestion
test('skip syncing the unsaved editor tab state when the updates are already synced', async () => {
```
--
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
[GitHub] [superset] sadpandajoe commented on pull request #24539: feat(sqllab): non-blocking persistence mode
Posted by "sadpandajoe (via GitHub)" <gi...@apache.org>.
sadpandajoe commented on PR #24539:
URL: https://github.com/apache/superset/pull/24539#issuecomment-1638491140
@justinpark @michael-s-molina do we need to spin up a new ephemeral for the changes?
--
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
[GitHub] [superset] justinpark commented on pull request #24539: feat(sqllab): non-blocking persistence mode
Posted by "justinpark (via GitHub)" <gi...@apache.org>.
justinpark commented on PR #24539:
URL: https://github.com/apache/superset/pull/24539#issuecomment-1638566284
/testenv up FEATURE_SQLLAB_BACKEND_PERSISTENCE=true
--
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
[GitHub] [superset] github-actions[bot] commented on pull request #24539: feat(sqllab): non-blocking persistence mode
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #24539:
URL: https://github.com/apache/superset/pull/24539#issuecomment-1638611385
@michael-s-molina Ephemeral environment spinning up at http://18.236.148.224:8080. Credentials are `admin`/`admin`. Please allow several minutes for bootstrapping and startup.
--
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
Re: [PR] feat(sqllab): non-blocking persistence mode [superset]
Posted by "justinpark (via GitHub)" <gi...@apache.org>.
justinpark commented on code in PR #24539:
URL: https://github.com/apache/superset/pull/24539#discussion_r1393386030
##########
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:
agree (I also thought same as like this). ~~Let me change this to the console warning (or logger for monitoring purpose)~~ will go with logger since it can monitor using flask error logger.
--
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
Re: [PR] feat(sqllab): non-blocking persistence mode [superset]
Posted by "justinpark (via GitHub)" <gi...@apache.org>.
justinpark merged PR #24539:
URL: https://github.com/apache/superset/pull/24539
--
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
[GitHub] [superset] michael-s-molina commented on pull request #24539: feat(sqllab): non-blocking persistence mode
Posted by "michael-s-molina (via GitHub)" <gi...@apache.org>.
michael-s-molina commented on PR #24539:
URL: https://github.com/apache/superset/pull/24539#issuecomment-1633043085
/testenv up FEATURE_SQLLAB_BACKEND_PERSISTENCE=true
--
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
[GitHub] [superset] michael-s-molina commented on pull request #24539: feat(sqllab): non-blocking persistence mode
Posted by "michael-s-molina (via GitHub)" <gi...@apache.org>.
michael-s-molina commented on PR #24539:
URL: https://github.com/apache/superset/pull/24539#issuecomment-1638604334
/testenv up FEATURE_SQLLAB_BACKEND_PERSISTENCE=true
--
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
[GitHub] [superset] justinpark commented on pull request #24539: feat(sqllab): non-blocking persistence mode
Posted by "justinpark (via GitHub)" <gi...@apache.org>.
justinpark commented on PR #24539:
URL: https://github.com/apache/superset/pull/24539#issuecomment-1654163433
@sadpandajoe same problem. @michael-s-molina have you faced same problem?
--
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
[GitHub] [superset] justinpark commented on pull request #24539: feat(sqllab): non-blocking persistence mode
Posted by "justinpark (via GitHub)" <gi...@apache.org>.
justinpark commented on PR #24539:
URL: https://github.com/apache/superset/pull/24539#issuecomment-1665967151
> does ephemeral logins work for either of you michael-s-molina / justinpark? I haven't been testing since I can't log in.
@sadpandajoe could you help to test in the following env?
https://github.com/apache/superset/pull/24885#issuecomment-1665565874
--
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
[GitHub] [superset] sadpandajoe commented on pull request #24539: feat(sqllab): non-blocking persistence mode
Posted by "sadpandajoe (via GitHub)" <gi...@apache.org>.
sadpandajoe commented on PR #24539:
URL: https://github.com/apache/superset/pull/24539#issuecomment-1653986943
does ephemeral logins work for either of you @michael-s-molina / @justinpark? I haven't been testing since I can't log in.
--
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
[GitHub] [superset] michael-s-molina commented on pull request #24539: feat(sqllab): non-blocking persistence mode
Posted by "michael-s-molina (via GitHub)" <gi...@apache.org>.
michael-s-molina commented on PR #24539:
URL: https://github.com/apache/superset/pull/24539#issuecomment-1654331703
> does ephemeral logins work for either of you @michael-s-molina / @justinpark? I haven't been testing since I can't log in.
> @sadpandajoe same problem. @michael-s-molina have you faced same problem?
Ephemeral environments are not working with `TALISMAN_ENABLED = True`. @eschutho was working on a [fix](https://github.com/apache/superset/pull/24774) but it's not merged yet. Given that `TALISMAN_ENABLED` is not a feature flag, we can't disable it so we'll need to pull this PR locally to test it.
--
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
[GitHub] [superset] sadpandajoe commented on pull request #24539: feat(sqllab): non-blocking persistence mode
Posted by "sadpandajoe (via GitHub)" <gi...@apache.org>.
sadpandajoe commented on PR #24539:
URL: https://github.com/apache/superset/pull/24539#issuecomment-1633036105
> @sadpandajoe @jinghua-qa could you help test the feature?
@michael-s-molina sure, if you want to spin up the ephemeral for me. I've noticed some issues already where results aren't always showing up so i want to make sure it's not an issue between OSS and Preset
--
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
[GitHub] [superset] github-actions[bot] commented on pull request #24539: feat(sqllab): non-blocking persistence mode
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #24539:
URL: https://github.com/apache/superset/pull/24539#issuecomment-1633039548
@michael-s-molina Ephemeral environment spinning up at http://54.213.6.20:8080. Credentials are `admin`/`admin`. Please allow several minutes for bootstrapping and startup.
--
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
[GitHub] [superset] sadpandajoe commented on pull request #24539: feat(sqllab): non-blocking persistence mode
Posted by "sadpandajoe (via GitHub)" <gi...@apache.org>.
sadpandajoe commented on PR #24539:
URL: https://github.com/apache/superset/pull/24539#issuecomment-1633318693
Results are not always shown with sql lab query. This is pretty hard to reproduce but also seem to be hitting a performance limit
* Create 10 sql lab tabs
* Run 10 different queries
* Go to any other page
* Go back to sql lab
* Click on each tab and rerun the query
Around tab 6 i'm starting to see performance issues with that tab loading. You can also see at the end of the video that although there is data, it won't return any results.
https://github.com/apache/superset/assets/47334392/5106c06c-4bd4-4b02-9c48-c34f7529f811
--
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
[GitHub] [superset] codecov[bot] commented on pull request #24539: feat(sqllab): non-blocking persistence mode
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #24539:
URL: https://github.com/apache/superset/pull/24539#issuecomment-1636149682
## [Codecov](https://app.codecov.io/gh/apache/superset/pull/24539?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
> Merging [#24539](https://app.codecov.io/gh/apache/superset/pull/24539?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (cbc49cb) into [master](https://app.codecov.io/gh/apache/superset/commit/1473d9705569d45a3fd6b962e5530d45d43cecc5?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (1473d97) will **decrease** coverage by `0.06%`.
> The diff coverage is `87.28%`.
> :exclamation: Current head cbc49cb differs from pull request most recent head a901a45. Consider uploading reports for the commit a901a45 to get more accurate results
```diff
@@ Coverage Diff @@
## master #24539 +/- ##
==========================================
- Coverage 69.03% 68.97% -0.06%
==========================================
Files 1908 1903 -5
Lines 74197 74034 -163
Branches 8186 8190 +4
==========================================
- Hits 51219 51068 -151
+ Misses 20857 20845 -12
Partials 2121 2121
```
| Flag | Coverage Δ | |
|---|---|---|
| javascript | `55.73% <80.64%> (+0.01%)` | :arrow_up: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://app.codecov.io/gh/apache/superset/pull/24539?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [superset-frontend/src/SqlLab/App.jsx](https://app.codecov.io/gh/apache/superset/pull/24539?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9BcHAuanN4) | `0.00% <0.00%> (ø)` | |
| [...rontend/src/SqlLab/components/QueryTable/index.tsx](https://app.codecov.io/gh/apache/superset/pull/24539?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9jb21wb25lbnRzL1F1ZXJ5VGFibGUvaW5kZXgudHN4) | `63.79% <0.00%> (ø)` | |
| [superset-frontend/src/SqlLab/types.ts](https://app.codecov.io/gh/apache/superset/pull/24539?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi90eXBlcy50cw==) | `57.14% <ø> (ø)` | |
| [superset/reports/commands/exceptions.py](https://app.codecov.io/gh/apache/superset/pull/24539?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvcmVwb3J0cy9jb21tYW5kcy9leGNlcHRpb25zLnB5) | `98.33% <ø> (-0.03%)` | :arrow_down: |
| [superset/views/base.py](https://app.codecov.io/gh/apache/superset/pull/24539?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvdmlld3MvYmFzZS5weQ==) | `73.33% <ø> (ø)` | |
| [superset/views/database/forms.py](https://app.codecov.io/gh/apache/superset/pull/24539?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvdmlld3MvZGF0YWJhc2UvZm9ybXMucHk=) | `93.67% <ø> (ø)` | |
| [.../plugin-chart-pivot-table/src/plugin/buildQuery.ts](https://app.codecov.io/gh/apache/superset/pull/24539?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQtZnJvbnRlbmQvcGx1Z2lucy9wbHVnaW4tY2hhcnQtcGl2b3QtdGFibGUvc3JjL3BsdWdpbi9idWlsZFF1ZXJ5LnRz) | `73.33% <50.00%> (+1.90%)` | :arrow_up: |
| [superset/annotation\_layers/api.py](https://app.codecov.io/gh/apache/superset/pull/24539?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvYW5ub3RhdGlvbl9sYXllcnMvYXBpLnB5) | `86.20% <50.00%> (-0.12%)` | :arrow_down: |
| [superset/css\_templates/commands/delete.py](https://app.codecov.io/gh/apache/superset/pull/24539?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvY3NzX3RlbXBsYXRlcy9jb21tYW5kcy9kZWxldGUucHk=) | `87.50% <50.00%> (ø)` | |
| [superset/queries/saved\_queries/commands/delete.py](https://app.codecov.io/gh/apache/superset/pull/24539?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvcXVlcmllcy9zYXZlZF9xdWVyaWVzL2NvbW1hbmRzL2RlbGV0ZS5weQ==) | `87.50% <50.00%> (ø)` | |
| ... and [35 more](https://app.codecov.io/gh/apache/superset/pull/24539?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | |
... and [2 files with indirect coverage changes](https://app.codecov.io/gh/apache/superset/pull/24539/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
:mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
--
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
[GitHub] [superset] michael-s-molina commented on pull request #24539: feat(sqllab): non-blocking persistence mode
Posted by "michael-s-molina (via GitHub)" <gi...@apache.org>.
michael-s-molina commented on PR #24539:
URL: https://github.com/apache/superset/pull/24539#issuecomment-1631581423
@sadpandajoe @jinghua-qa could you help test the feature?
--
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
[GitHub] [superset] sadpandajoe commented on pull request #24539: feat(sqllab): non-blocking persistence mode
Posted by "sadpandajoe (via GitHub)" <gi...@apache.org>.
sadpandajoe commented on PR #24539:
URL: https://github.com/apache/superset/pull/24539#issuecomment-1633146053
@justinpark looks like column auto-complete isn't working in this branch
https://github.com/apache/superset/assets/47334392/661d92f0-eede-4bb1-a10b-0749a31f8328
--
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
[GitHub] [superset] github-actions[bot] commented on pull request #24539: feat(sqllab): non-blocking persistence mode
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #24539:
URL: https://github.com/apache/superset/pull/24539#issuecomment-1638566744
@justinpark Ephemeral environment creation is currently limited to committers.
--
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
[GitHub] [superset] justinpark commented on pull request #24539: feat(sqllab): non-blocking persistence mode
Posted by "justinpark (via GitHub)" <gi...@apache.org>.
justinpark commented on PR #24539:
URL: https://github.com/apache/superset/pull/24539#issuecomment-1638603215
@michael-s-molina could you help to boot up testenv?
--
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
[GitHub] [superset] michael-s-molina commented on pull request #24539: feat(sqllab): non-blocking persistence mode
Posted by "michael-s-molina (via GitHub)" <gi...@apache.org>.
michael-s-molina commented on PR #24539:
URL: https://github.com/apache/superset/pull/24539#issuecomment-1633037152
/testenv up
--
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
[GitHub] [superset] github-actions[bot] commented on pull request #24539: feat(sqllab): non-blocking persistence mode
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #24539:
URL: https://github.com/apache/superset/pull/24539#issuecomment-1633048087
@michael-s-molina Ephemeral environment spinning up at http://54.203.45.163:8080. Credentials are `admin`/`admin`. Please allow several minutes for bootstrapping and startup.
--
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
[GitHub] [superset] sadpandajoe commented on pull request #24539: feat(sqllab): non-blocking persistence mode
Posted by "sadpandajoe (via GitHub)" <gi...@apache.org>.
sadpandajoe commented on PR #24539:
URL: https://github.com/apache/superset/pull/24539#issuecomment-1633173836
Table schemas get removed when switching between tabs
https://github.com/apache/superset/assets/47334392/29150c3f-564b-4ae2-8257-2a6354832cdb
--
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
[GitHub] [superset] sadpandajoe commented on pull request #24539: feat(sqllab): non-blocking persistence mode
Posted by "sadpandajoe (via GitHub)" <gi...@apache.org>.
sadpandajoe commented on PR #24539:
URL: https://github.com/apache/superset/pull/24539#issuecomment-1633178104
User is not able to remove schemas without an error warning
* Select a schema in the left dropdown
* Click the remove button
What happens?
There is an error message to contact the administrator
--
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
[GitHub] [superset] sadpandajoe commented on pull request #24539: feat(sqllab): non-blocking persistence mode
Posted by "sadpandajoe (via GitHub)" <gi...@apache.org>.
sadpandajoe commented on PR #24539:
URL: https://github.com/apache/superset/pull/24539#issuecomment-1670313042
> > does ephemeral logins work for either of you michael-s-molina / justinpark? I haven't been testing since I can't log in.
>
> @sadpandajoe could you help to test in the following env?
>
> [#24885 (comment)](https://github.com/apache/superset/pull/24885#issuecomment-1665565874)
@justinpark went through and looked at the PR commented above and things seem fine to me.
--
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
Re: [PR] feat(sqllab): non-blocking persistence mode [superset]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #24539:
URL: https://github.com/apache/superset/pull/24539#issuecomment-1819653952
Ephemeral environment shutdown and build artifacts deleted.
--
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
Re: [PR] feat(sqllab): non-blocking persistence mode [superset]
Posted by "justinpark (via GitHub)" <gi...@apache.org>.
justinpark commented on PR #24539:
URL: https://github.com/apache/superset/pull/24539#issuecomment-1786060184
@michael-s-molina @eschutho could you review this PR again?
--
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
Re: [PR] feat(sqllab): non-blocking persistence mode [superset]
Posted by "michael-s-molina (via GitHub)" <gi...@apache.org>.
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
Re: [PR] feat(sqllab): non-blocking persistence mode [superset]
Posted by "justinpark (via GitHub)" <gi...@apache.org>.
justinpark commented on code in PR #24539:
URL: https://github.com/apache/superset/pull/24539#discussion_r1393386030
##########
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:
agree (I also thought same as like this). Let me change this to the console warning (or logger for monitoring purpose)
--
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