fix(sqllab): Invalid schema fetch by deprecated value

apache · Feb 2, 2023 · a6b91d4 · a6b91d4
1 parent 56d4bd0
commit a6b91d4
Show file tree

Hide file tree

Showing 8 changed files with 463 additions and 153 deletions.
diff --git a/superset-frontend/src/SqlLab/components/SqlEditorLeftBar/SqlEditorLeftBar.test.jsx b/superset-frontend/src/SqlLab/components/SqlEditorLeftBar/SqlEditorLeftBar.test.jsx
@@ -41,15 +41,24 @@ const middlewares = [thunk];
 const mockStore = configureStore(middlewares);
 const store = mockStore(initialState);
 
-fetchMock.get('glob:*/api/v1/database/*/schemas/?*', { result: [] });
-fetchMock.get('glob:*/api/v1/database/*/tables/*', {
-  count: 1,
-  result: [
-    {
-      label: 'ab_user',
-      value: 'ab_user',
-    },
-  ],
+beforeEach(() => {
+  fetchMock.get('glob:*/api/v1/database/*/schemas/?*', {
+    result: ['main', 'new_schema'],
+  });
+  fetchMock.get('glob:*/api/v1/database/**', { result: [] });
+  fetchMock.get('glob:*/api/v1/database/*/tables/*', {
+    count: 1,
+    result: [
+      {
+        label: 'ab_user',
+        value: 'ab_user',
+      },
+    ],
+  });
+});
+
+afterEach(() => {
+  fetchMock.restore();
 });
 
 const renderAndWait = (props, store) =>
@@ -110,8 +119,9 @@ test('should toggle the table when the header is clicked', async () => {
   userEvent.click(header);
 
   await waitFor(() => {
-    expect(store.getActions()).toHaveLength(4);
-    expect(store.getActions()[3].type).toEqual('COLLAPSE_TABLE');
+    expect(store.getActions()[store.getActions().length - 1].type).toEqual(
+      'COLLAPSE_TABLE',
+    );
   });
 });
 
@@ -129,14 +139,55 @@ test('When changing database the table list must be updated', async () => {
         database_name: 'new_db',
         backend: 'postgresql',
       }}
-      queryEditor={{ ...mockedProps.queryEditor, schema: 'new_schema' }}
+      queryEditorId={defaultQueryEditor.id}
       tables={[{ ...mockedProps.tables[0], dbId: 2, name: 'new_table' }]}
     />,
     {
       useRedux: true,
-      initialState,
+      store: mockStore({
+        ...initialState,
+        sqlLab: {
+          ...initialState.sqlLab,
+          unsavedQueryEditor: {
+            id: defaultQueryEditor.id,
+            schema: 'new_schema',
+          },
+        },
+      }),
     },
   );
   expect(await screen.findByText(/new_db/i)).toBeInTheDocument();
   expect(await screen.findByText(/new_table/i)).toBeInTheDocument();
 });
+
+test('ignore schema api when current schema is deprecated', async () => {
+  const invalidSchemaName = 'None';
+  const { rerender } = await renderAndWait(
+    mockedProps,
+    mockStore({
+      ...initialState,
+      sqlLab: {
+        ...initialState.sqlLab,
+        unsavedQueryEditor: {
+          id: defaultQueryEditor.id,
+          schema: invalidSchemaName,
+        },
+      },
+    }),
+  );
+
+  expect(await screen.findByText(/Database/i)).toBeInTheDocument();
+  expect(screen.queryByText(/None/i)).toBeInTheDocument();
+  expect(fetchMock.calls()).not.toContainEqual(
+    expect.arrayContaining([
+      expect.stringContaining(
+        `/tables/${mockedProps.database.id}/${invalidSchemaName}/`,
+      ),
+    ]),
+  );
+  rerender();
+  // Deselect the deprecated schema selection
+  await waitFor(() =>
+    expect(screen.queryByText(/None/i)).not.toBeInTheDocument(),
+  );
+});
diff --git a/superset-frontend/src/components/DatabaseSelector/index.tsx b/superset-frontend/src/components/DatabaseSelector/index.tsx
@@ -24,10 +24,7 @@ import Label from 'src/components/Label';
 import { FormLabel } from 'src/components/Form';
 import RefreshLabel from 'src/components/RefreshLabel';
 import { useToasts } from 'src/components/MessageToasts/withToasts';
-import {
-  getClientErrorMessage,
-  getClientErrorObject,
-} from 'src/utils/getClientErrorObject';
+import { useSchemas, SchemaOption } from 'src/hooks/apiResources';
 
 const DatabaseSelectorWrapper = styled.div`
   ${({ theme }) => `
@@ -86,8 +83,6 @@ export type DatabaseObject = {
   backend: string;
 };
 
-type SchemaValue = { label: string; value: string };
-
 export interface DatabaseSelectorProps {
   db?: DatabaseObject | null;
   emptyState?: ReactNode;
@@ -119,6 +114,8 @@ const SelectLabel = ({
   </LabelStyle>
 );
 
+const EMPTY_SCHEMA_OPTIONS: SchemaOption[] = [];
+
 export default function DatabaseSelector({
   db,
   formMode = false,
@@ -134,13 +131,10 @@ export default function DatabaseSelector({
   schema,
   sqlLabMode = false,
 }: DatabaseSelectorProps) {
-  const [loadingSchemas, setLoadingSchemas] = useState(false);
-  const [schemaOptions, setSchemaOptions] = useState<SchemaValue[]>([]);
   const [currentDb, setCurrentDb] = useState<DatabaseValue | undefined>();
-  const [currentSchema, setCurrentSchema] = useState<SchemaValue | undefined>(
-    schema ? { label: schema, value: schema } : undefined,
+  const [currentSchema, setCurrentSchema] = useState<SchemaOption | undefined>(
+    schema ? { label: schema, value: schema, title: schema } : undefined,
   );
-  const [refresh, setRefresh] = useState(0);
   const { addSuccessToast } = useToasts();
 
   const loadDatabases = useMemo(
@@ -221,48 +215,37 @@ export default function DatabaseSelector({
     );
   }, [db]);
 
-  function changeSchema(schema: SchemaValue) {
+  function changeSchema(schema: SchemaOption | undefined) {
     setCurrentSchema(schema);
     if (onSchemaChange) {
-      onSchemaChange(schema.value);
+      onSchemaChange(schema?.value);
     }
   }
 
-  useEffect(() => {
-    if (currentDb) {
-      setLoadingSchemas(true);
-      const queryParams = rison.encode({ force: refresh > 0 });
-      const endpoint = `/api/v1/database/${currentDb.value}/schemas/?q=${queryParams}`;
+  const {
+    data,
+    isFetching: loadingSchemas,
+    isFetched,
+    refetch,
+  } = useSchemas({
+    dbId: currentDb?.value,
+    onSuccess: data => {
+      onSchemasLoad?.(data);
 
-      // TODO: Would be nice to add pagination in a follow-up. Needs endpoint changes.
-      SupersetClient.get({ endpoint })
-        .then(({ json }) => {
-          const options = json.result.map((s: string) => ({
-            value: s,
-            label: s,
-            title: s,
-          }));
-          if (onSchemasLoad) {
-            onSchemasLoad(options);
-          }
-          setSchemaOptions(options);
-          setLoadingSchemas(false);
-          if (options.length === 1) changeSchema(options[0]);
-          if (refresh > 0) addSuccessToast(t('List refreshed'));
-        })
-        .catch(err => {
-          setLoadingSchemas(false);
-          getClientErrorObject(err).then(clientError => {
-            handleError(
-              getClientErrorMessage(
-                t('There was an error loading the schemas'),
-                clientError,
-              ),
-            );
-          });
-        });
-    }
-  }, [currentDb, onSchemasLoad, refresh]);
+      if (data.length === 1) {
+        changeSchema(data[0]);
+      } else if (!data.find(schemaOption => schema === schemaOption.value)) {
+        changeSchema(undefined);
+      }
+
+      if (isFetched) {
+        addSuccessToast('List refreshed');
+      }
+    },
+    onError: () => handleError(t('There was an error loading the schemas')),
+  });
+
+  const schemaOptions = data || EMPTY_SCHEMA_OPTIONS;
 
   function changeDataBase(
     value: { label: string; value: number },
@@ -309,7 +292,7 @@ export default function DatabaseSelector({
   function renderSchemaSelect() {
     const refreshIcon = !readOnly && (
       <RefreshLabel
-        onClick={() => setRefresh(refresh + 1)}
+        onClick={() => refetch()}
         tooltipContent={t('Force refresh schema list')}
       />
     );
@@ -323,7 +306,7 @@ export default function DatabaseSelector({
         name="select-schema"
         notFoundContent={t('No compatible schema found')}
         placeholder={t('Select schema or type schema name')}
-        onChange={item => changeSchema(item as SchemaValue)}
+        onChange={item => changeSchema(item as SchemaOption)}
         options={schemaOptions}
         showSearch
         value={currentSchema}

diff --git a/superset-frontend/src/components/TableSelector/index.tsx b/superset-frontend/src/components/TableSelector/index.tsx
@@ -275,26 +275,6 @@ const TableSelector: FunctionComponent<TableSelectorProps> = ({
     internalTableChange(value);
   };
 
-  function renderDatabaseSelector() {
-    return (
-      <DatabaseSelector
-        db={database}
-        emptyState={emptyState}
-        formMode={formMode}
-        getDbList={getDbList}
-        handleError={handleError}
-        onDbChange={readOnly ? undefined : internalDbChange}
-        onEmptyResults={onEmptyResults}
-        onSchemaChange={readOnly ? undefined : internalSchemaChange}
-        onSchemasLoad={onSchemasLoad}
-        schema={currentSchema}
-        sqlLabMode={sqlLabMode}
-        isDatabaseSelectEnabled={isDatabaseSelectEnabled && !readOnly}
-        readOnly={readOnly}
-      />
-    );
-  }
-
   const handleFilterOption = useMemo(
     () => (search: string, option: TableOption) => {
       const searchValue = search.trim().toLowerCase();
@@ -346,7 +326,21 @@ const TableSelector: FunctionComponent<TableSelectorProps> = ({
 
   return (
     <TableSelectorWrapper>
-      {renderDatabaseSelector()}
+      <DatabaseSelector
+        db={database}
+        emptyState={emptyState}
+        formMode={formMode}
+        getDbList={getDbList}
+        handleError={handleError}
+        onDbChange={readOnly ? undefined : internalDbChange}
+        onEmptyResults={onEmptyResults}
+        onSchemaChange={readOnly ? undefined : internalSchemaChange}
+        onSchemasLoad={onSchemasLoad}
+        schema={currentSchema}
+        sqlLabMode={sqlLabMode}
+        isDatabaseSelectEnabled={isDatabaseSelectEnabled && !readOnly}
+        readOnly={readOnly}
+      />
       {sqlLabMode && !formMode && <div className="divider" />}
       {renderTableSelect()}
     </TableSelectorWrapper>

diff --git a/superset-frontend/src/hooks/apiResources/index.ts b/superset-frontend/src/hooks/apiResources/index.ts
@@ -29,3 +29,4 @@ export {
 export * from './charts';
 export * from './dashboards';
 export * from './tables';
+export * from './schemas';