refactor(sqllab): migrate share queries via kv by permalink (#29163)

Author: justinpark

RESOURCES/FEATURE_FLAGS.md

@@ -62,6 +62,7 @@ These features flags are **safe for production**. They have been tested and will
 [//]: # "PLEASE KEEP THESE LISTS SORTED ALPHABETICALLY"
 
 ### Flags on the path to feature launch and flag deprecation/removal
+
 - DASHBOARD_VIRTUALIZATION
 - DRILL_BY
 - DISABLE_LEGACY_DATASOURCE_EDITOR

UPDATING.md

@@ -23,6 +23,7 @@ This file documents any backwards-incompatible changes in Superset and
 assists people when migrating to a new version.
 
 ## Next
+
 - [31894](https://github.com/apache/superset/pull/31894) Domain sharding is deprecated in favor of HTTP2. The `SUPERSET_WEBSERVER_DOMAINS` configuration will be removed in the next major version (6.0)
 - [31774](https://github.com/apache/superset/pull/31774): Fixes the spelling of the `USE-ANALAGOUS-COLORS` feature flag. Please update any scripts/configuration item to use the new/corrected `USE-ANALOGOUS-COLORS` flag spelling.
 - [31582](https://github.com/apache/superset/pull/31582) Removed the legacy Area, Bar, Event Flow, Heatmap, Histogram, Line, Sankey, and Sankey Loop charts. They were all automatically migrated to their ECharts counterparts with the exception of the Event Flow and Sankey Loop charts which were removed as they were not actively maintained and not widely used. If you were using the Event Flow or Sankey Loop charts, you will need to find an alternative solution.
@@ -33,8 +34,9 @@ assists people when migrating to a new version.
 - [31262](https://github.com/apache/superset/pull/31262) NOTE: deprecated `pylint` in favor of `ruff` as our only python linter. Only affect development workflows positively (not the release itself). It should cover most important rules, be much faster, but some things linting rules that were enforced before may not be enforce in the exact same way as before.
 - [31173](https://github.com/apache/superset/pull/31173) Modified `fetch_csrf_token` to align with HTTP standards, particularly regarding how cookies are handled. If you encounter any issues related to CSRF functionality, please report them as a new issue and reference this PR for context.
 - [31385](https://github.com/apache/superset/pull/31385) Significant docker refactor, reducing access levels for the `superset` user, streamlining layer building, ...
-- [31503](https://github.com/apache/superset/pull/31503) Deprecating python 3.9.x support, 3.11 is  now the recommended version and 3.10 is still supported over the Superset 5.0 lifecycle.
+- [31503](https://github.com/apache/superset/pull/31503) Deprecating python 3.9.x support, 3.11 is now the recommended version and 3.10 is still supported over the Superset 5.0 lifecycle.
 - [29121](https://github.com/apache/superset/pull/29121) Removed the `css`, `position_json`, and `json_metadata` from the payload of the dashboard list endpoint (`GET api/v1/dashboard`) for performance reasons.
+- [29163](https://github.com/apache/superset/pull/29163) Removed the `SHARE_QUERIES_VIA_KV_STORE` and `KV_STORE` feature flags and changed the way Superset shares SQL Lab queries to use permalinks. The legacy `/kv` API was removed but we still support legacy links in 5.0. In 6.0, only permalinks will be supported.
 
 ### Potential Downtime
 

superset-frontend/packages/superset-ui-core/src/utils/featureFlags.ts

@@ -52,7 +52,6 @@ export enum FeatureFlag {
   HorizontalFilterBar = 'HORIZONTAL_FILTER_BAR',
   ListviewsDefaultCardView = 'LISTVIEWS_DEFAULT_CARD_VIEW',
   ScheduledQueries = 'SCHEDULED_QUERIES',
-  ShareQueriesViaKvStore = 'SHARE_QUERIES_VIA_KV_STORE',
   SqllabBackendPersistence = 'SQLLAB_BACKEND_PERSISTENCE',
   SqlValidatorsByEngine = 'SQL_VALIDATORS_BY_ENGINE',
   SshTunneling = 'SSH_TUNNELING',

superset-frontend/src/SqlLab/actions/sqlLab.js

@@ -1167,9 +1167,31 @@ export function persistEditorHeight(queryEditor, northPercent, southPercent) {
   };
 }
 
+export function popPermalink(key) {
+  return function (dispatch) {
+    return SupersetClient.get({ endpoint: `/api/v1/sqllab/permalink/${key}` })
+      .then(({ json }) =>
+        dispatch(
+          addQueryEditor({
+            name: json.name ? json.name : t('Shared query'),
+            dbId: json.dbId ? parseInt(json.dbId, 10) : null,
+            catalog: json.catalog ? json.catalog : null,
+            schema: json.schema ? json.schema : null,
+            autorun: json.autorun ? json.autorun : false,
+            sql: json.sql ? json.sql : 'SELECT ...',
+            templateParams: json.templateParams,
+          }),
+        ),
+      )
+      .catch(() => dispatch(addDangerToast(ERR_MSG_CANT_LOAD_QUERY)));
+  };
+}
+
 export function popStoredQuery(urlId) {
   return function (dispatch) {
-    return SupersetClient.get({ endpoint: `/kv/${urlId}` })
+    return SupersetClient.get({
+      endpoint: `/api/v1/sqllab/permalink/kv:${urlId}`,
+    })
       .then(({ json }) =>
         dispatch(
           addQueryEditor({

superset-frontend/src/SqlLab/components/ShareSqlLabQuery/ShareSqlLabQuery.test.tsx

@@ -23,10 +23,9 @@ import fetchMock from 'fetch-mock';
 import * as uiCore from '@superset-ui/core';
 import { Provider } from 'react-redux';
 import { supersetTheme, ThemeProvider } from '@superset-ui/core';
-import { render, screen, act } from '@testing-library/react';
+import { render, screen, act, waitFor } from '@testing-library/react';
 import '@testing-library/jest-dom';
 import userEvent from '@testing-library/user-event';
-import * as utils from 'src/utils/common';
 import ShareSqlLabQuery from 'src/SqlLab/components/ShareSqlLabQuery';
 import { initialState } from 'src/SqlLab/fixtures';
 
@@ -92,20 +91,24 @@ const standardProviderWithUnsaved: FC = ({ children }) => (
 );
 
 describe('ShareSqlLabQuery', () => {
-  const storeQueryUrl = 'glob:*/kv/store/';
-  const storeQueryMockId = '123';
+  const storeQueryUrl = 'glob:*/api/v1/sqllab/permalink';
+  const storeQueryMockId = 'ci39c3';
 
   beforeEach(async () => {
-    fetchMock.post(storeQueryUrl, () => ({ id: storeQueryMockId }), {
-      overwriteRoutes: true,
-    });
+    fetchMock.post(
+      storeQueryUrl,
+      () => ({ key: storeQueryMockId, url: `/p/${storeQueryMockId}` }),
+      {
+        overwriteRoutes: true,
+      },
+    );
     fetchMock.resetHistory();
     jest.clearAllMocks();
   });
 
   afterAll(fetchMock.reset);
 
-  describe('via /kv/store', () => {
+  describe('via permalink api', () => {
     beforeAll(() => {
       isFeatureEnabledMock = jest
         .spyOn(uiCore, 'isFeatureEnabled')
@@ -124,11 +127,13 @@ describe('ShareSqlLabQuery', () => {
       });
       const button = screen.getByRole('button');
       const { id, remoteId, ...expected } = mockQueryEditor;
-      const storeQuerySpy = jest.spyOn(utils, 'storeQuery');
       userEvent.click(button);
-      expect(storeQuerySpy.mock.calls).toHaveLength(1);
-      expect(storeQuerySpy).toHaveBeenCalledWith(expected);
-      storeQuerySpy.mockRestore();
+      await waitFor(() =>
+        expect(fetchMock.calls(storeQueryUrl)).toHaveLength(1),
+      );
+      expect(
+        JSON.parse(fetchMock.calls(storeQueryUrl)[0][1]?.body as string),
+      ).toEqual(expected);
     });
 
     it('calls storeQuery() with unsaved changes', async () => {
@@ -139,48 +144,13 @@ describe('ShareSqlLabQuery', () => {
       });
       const button = screen.getByRole('button');
       const { id, ...expected } = unsavedQueryEditor;
-      const storeQuerySpy = jest.spyOn(utils, 'storeQuery');
-      userEvent.click(button);
-      expect(storeQuerySpy.mock.calls).toHaveLength(1);
-      expect(storeQuerySpy).toHaveBeenCalledWith(expected);
-      storeQuerySpy.mockRestore();
-    });
-  });
-
-  describe('via saved query', () => {
-    beforeAll(() => {
-      isFeatureEnabledMock = jest
-        .spyOn(uiCore, 'isFeatureEnabled')
-        .mockImplementation(() => false);
-    });
-
-    afterAll(() => {
-      isFeatureEnabledMock.mockReset();
-    });
-
-    it('does not call storeQuery() with the query when getCopyUrl() is called and feature is not enabled', async () => {
-      await act(async () => {
-        render(<ShareSqlLabQuery {...defaultProps} />, {
-          wrapper: standardProvider,
-        });
-      });
-      const storeQuerySpy = jest.spyOn(utils, 'storeQuery');
-      const button = screen.getByRole('button');
       userEvent.click(button);
-      expect(storeQuerySpy.mock.calls).toHaveLength(0);
-      storeQuerySpy.mockRestore();
-    });
-
-    it('button is disabled and there is a request to save the query', async () => {
-      const updatedProps = {
-        queryEditorId: disabled.id,
-      };
-
-      render(<ShareSqlLabQuery {...updatedProps} />, {
-        wrapper: standardProvider,
-      });
-      const button = await screen.findByRole('button', { name: /copy link/i });
-      expect(button).toBeDisabled();
+      await waitFor(() =>
+        expect(fetchMock.calls(storeQueryUrl)).toHaveLength(1),
+      );
+      expect(
+        JSON.parse(fetchMock.calls(storeQueryUrl)[0][1]?.body as string),
+      ).toEqual(expected);
     });
   });
 });

superset-frontend/src/SqlLab/components/ShareSqlLabQuery/index.tsx

@@ -17,18 +17,16 @@
  * under the License.
  */
 import {
-  FeatureFlag,
   styled,
   t,
   useTheme,
-  isFeatureEnabled,
   getClientErrorObject,
+  SupersetClient,
 } from '@superset-ui/core';
 import Button from 'src/components/Button';
 import Icons from 'src/components/Icons';
 import withToasts from 'src/components/MessageToasts/withToasts';
 import CopyToClipboard from 'src/components/CopyToClipboard';
-import { storeQuery } from 'src/utils/common';
 import useQueryEditor from 'src/SqlLab/hooks/useQueryEditor';
 import { LOG_ACTIONS_SQLLAB_COPY_LINK } from 'src/logger/LogUtils';
 import useLogAction from 'src/logger/useLogAction';
@@ -54,23 +52,21 @@ const ShareSqlLabQuery = ({
 }: ShareSqlLabQueryProps) => {
   const theme = useTheme();
   const logAction = useLogAction({ queryEditorId });
-  const { dbId, name, schema, autorun, sql, remoteId, templateParams } =
-    useQueryEditor(queryEditorId, [
-      'dbId',
-      'name',
-      'schema',
-      'autorun',
-      'sql',
-      'remoteId',
-      'templateParams',
-    ]);
+  const { dbId, name, schema, autorun, sql, templateParams } = useQueryEditor(
+    queryEditorId,
+    ['dbId', 'name', 'schema', 'autorun', 'sql', 'templateParams'],
+  );
 
-  const getCopyUrlForKvStore = (callback: Function) => {
+  const getCopyUrlForPermalink = (callback: Function) => {
     const sharedQuery = { dbId, name, schema, autorun, sql, templateParams };
 
-    return storeQuery(sharedQuery)
-      .then(shortUrl => {
-        callback(shortUrl);
+    return SupersetClient.post({
+      endpoint: '/api/v1/sqllab/permalink',
+      headers: { 'Content-Type': 'application/json' },
+      body: JSON.stringify(sharedQuery),
+    })
+      .then(({ json }) => {
+        callback(json.url);
       })
       .catch(response => {
         getClientErrorObject(response).then(() => {
@@ -79,61 +75,29 @@ const ShareSqlLabQuery = ({
       });
   };
 
-  const getCopyUrlForSavedQuery = (callback: Function) => {
-    let savedQueryToastContent;
-
-    if (remoteId) {
-      savedQueryToastContent = `${
-        window.location.origin + window.location.pathname
-      }?savedQueryId=${remoteId}`;
-      callback(savedQueryToastContent);
-    } else {
-      savedQueryToastContent = t('Please save the query to enable sharing');
-      callback(savedQueryToastContent);
-    }
-  };
   const getCopyUrl = (callback: Function) => {
     logAction(LOG_ACTIONS_SQLLAB_COPY_LINK, {
       shortcut: false,
     });
-    if (isFeatureEnabled(FeatureFlag.ShareQueriesViaKvStore)) {
-      return getCopyUrlForKvStore(callback);
-    }
-    return getCopyUrlForSavedQuery(callback);
+    return getCopyUrlForPermalink(callback);
   };
 
-  const buildButton = (canShare: boolean) => {
-    const tooltip = canShare
-      ? t('Copy query link to your clipboard')
-      : t('Save the query to enable this feature');
+  const buildButton = () => {
+    const tooltip = t('Copy query link to your clipboard');
     return (
-      <Button buttonSize="small" tooltip={tooltip} disabled={!canShare}>
-        <StyledIcon
-          iconColor={
-            canShare ? theme.colors.primary.base : theme.colors.grayscale.base
-          }
-          iconSize="xl"
-        />
+      <Button buttonSize="small" tooltip={tooltip}>
+        <StyledIcon iconColor={theme.colors.primary.base} iconSize="xl" />
         {t('Copy link')}
       </Button>
     );
   };
 
-  const canShare =
-    !!remoteId || isFeatureEnabled(FeatureFlag.ShareQueriesViaKvStore);
-
   return (
-    <>
-      {canShare ? (
-        <CopyToClipboard
-          getText={getCopyUrl}
-          wrapped={false}
-          copyNode={buildButton(canShare)}
-        />
-      ) : (
-        buildButton(canShare)
-      )}
-    </>
+    <CopyToClipboard
+      getText={getCopyUrl}
+      wrapped={false}
+      copyNode={buildButton()}
+    />
   );
 };
 

superset-frontend/src/SqlLab/components/TabbedSqlEditors/TabbedSqlEditors.test.tsx

@@ -28,10 +28,6 @@ import { Store } from 'redux';
 import { RootState } from 'src/views/store';
 import { SET_ACTIVE_QUERY_EDITOR } from 'src/SqlLab/actions/sqlLab';
 
-fetchMock.get('glob:*/api/v1/database/*', {});
-fetchMock.get('glob:*/api/v1/saved_query/*', {});
-fetchMock.get('glob:*/kv/*', {});
-
 jest.mock('src/SqlLab/components/SqlEditor', () => () => (
   <div data-test="mock-sql-editor" />
 ));
@@ -46,11 +42,20 @@ const setup = (overridesStore?: Store, initialState?: RootState) =>
     initialState,
     ...(overridesStore && { store: overridesStore }),
   });
+let pathStub = jest.spyOn(URI.prototype, 'path');
 
 beforeEach(() => {
+  fetchMock.get('glob:*/api/v1/database/*', {});
+  fetchMock.get('glob:*/api/v1/saved_query/*', {});
+  pathStub = jest.spyOn(URI.prototype, 'path').mockReturnValue(`/sqllab/`);
   store.clearActions();
 });
 
+afterEach(() => {
+  fetchMock.reset();
+  pathStub.mockReset();
+});
+
 describe('componentDidMount', () => {
   let uriStub = jest.spyOn(URI.prototype, 'search');
   let replaceState = jest.spyOn(window.history, 'replaceState');
@@ -62,14 +67,47 @@ describe('componentDidMount', () => {
     replaceState.mockReset();
     uriStub.mockReset();
   });
-  test('should handle id', () => {
+  test('should handle id', async () => {
+    const id = 1;
+    fetchMock.get(`glob:*/api/v1/sqllab/permalink/kv:${id}`, {
+      label: 'test permalink',
+      sql: 'SELECT * FROM test_table',
+      dbId: 1,
+    });
     uriStub.mockReturnValue({ id: 1 });
     setup(store);
     expect(replaceState).toHaveBeenCalledWith(
       expect.anything(),
       expect.anything(),
       '/sqllab',
     );
+    await waitFor(() =>
+      expect(
+        fetchMock.calls(`glob:*/api/v1/sqllab/permalink/kv:${id}`),
+      ).toHaveLength(1),
+    );
+    fetchMock.reset();
+  });
+  test('should handle permalink', async () => {
+    const key = '9sadkfl';
+    fetchMock.get(`glob:*/api/v1/sqllab/permalink/${key}`, {
+      label: 'test permalink',
+      sql: 'SELECT * FROM test_table',
+      dbId: 1,
+    });
+    pathStub.mockReturnValue(`/sqllab/p/${key}`);
+    setup(store);
+    await waitFor(() =>
+      expect(
+        fetchMock.calls(`glob:*/api/v1/sqllab/permalink/${key}`),
+      ).toHaveLength(1),
+    );
+    expect(replaceState).toHaveBeenCalledWith(
+      expect.anything(),
+      expect.anything(),
+      '/sqllab',
+    );
+    fetchMock.reset();
   });
   test('should handle savedQueryId', () => {
     uriStub.mockReturnValue({ savedQueryId: 1 });