fix(sqllab): excessive API calls for schemas (apache#29279)

justinpark · Jun 18, 2024 · 4537ab6 · 4537ab6
1 parent 725afc3
commit 4537ab6
Show file tree

Hide file tree

Showing 3 changed files with 59 additions and 100 deletions.
diff --git a/superset-frontend/src/hooks/apiResources/catalogs.ts b/superset-frontend/src/hooks/apiResources/catalogs.ts
@@ -16,7 +16,7 @@
  * specific language governing permissions and limitations
  * under the License.
  */
-import { useCallback, useEffect, useRef } from 'react';
+import { useCallback, useEffect } from 'react';
 import useEffectEvent from 'src/hooks/useEffectEvent';
 import { api, JsonResponse } from './queryApi';
 
@@ -68,7 +68,6 @@ export const {
 export const EMPTY_CATALOGS = [] as CatalogOption[];
 
 export function useCatalogs(options: Params) {
-  const isMountedRef = useRef(false);
   const { dbId, onSuccess, onError } = options || {};
   const [trigger] = useLazyCatalogsQuery();
   const result = useCatalogsQuery(
@@ -78,47 +77,28 @@ export function useCatalogs(options: Params) {
     },
   );
 
-  const handleOnSuccess = useEffectEvent(
-    (data: CatalogOption[], isRefetched: boolean) => {
-      onSuccess?.(data, isRefetched);
-    },
-  );
-
-  const handleOnError = useEffectEvent(() => {
-    onError?.();
-  });
-
-  const refetch = useCallback(() => {
-    if (dbId) {
-      trigger({ dbId, forceRefresh: true }).then(
-        ({ isSuccess, isError, data }) => {
+  const fetchData = useEffectEvent(
+    (dbId: FetchCatalogsQueryParams['dbId'], forceRefresh = false) => {
+      if (dbId && (!result.currentData || forceRefresh)) {
+        trigger({ dbId, forceRefresh }).then(({ isSuccess, isError, data }) => {
           if (isSuccess) {
-            handleOnSuccess(data || EMPTY_CATALOGS, true);
+            onSuccess?.(data || EMPTY_CATALOGS, forceRefresh);
           }
           if (isError) {
-            handleOnError();
+            onError?.();
           }
-        },
-      );
-    }
-  }, [dbId, handleOnError, handleOnSuccess, trigger]);
+        });
+      }
+    },
+  );
+
+  const refetch = useCallback(() => {
+    fetchData(dbId, true);
+  }, [dbId, fetchData]);
 
   useEffect(() => {
-    if (isMountedRef.current) {
-      const { requestId, isSuccess, isError, isFetching, data, originalArgs } =
-        result;
-      if (!originalArgs?.forceRefresh && requestId && !isFetching) {
-        if (isSuccess) {
-          handleOnSuccess(data || EMPTY_CATALOGS, false);
-        }
-        if (isError) {
-          handleOnError();
-        }
-      }
-    } else {
-      isMountedRef.current = true;
-    }
-  }, [result, handleOnSuccess, handleOnError]);
+    fetchData(dbId, false);
+  }, [dbId, fetchData]);
 
   return {
     ...result,

diff --git a/superset-frontend/src/hooks/apiResources/schemas.test.ts b/superset-frontend/src/hooks/apiResources/schemas.test.ts
@@ -88,7 +88,7 @@ describe('useSchemas hook', () => {
         })}`,
       ).length,
     ).toBe(1);
-    expect(onSuccess).toHaveBeenCalledTimes(2);
+    expect(onSuccess).toHaveBeenCalledTimes(1);
     act(() => {
       result.current.refetch();
     });
@@ -100,7 +100,7 @@ describe('useSchemas hook', () => {
         })}`,
       ).length,
     ).toBe(1);
-    expect(onSuccess).toHaveBeenCalledTimes(3);
+    expect(onSuccess).toHaveBeenCalledTimes(2);
     expect(result.current.data).toEqual(expectedResult);
   });
 
@@ -149,28 +149,36 @@ describe('useSchemas hook', () => {
       },
     );
 
-    await waitFor(() => expect(result.current.data).toEqual(expectedResult));
+    await waitFor(() =>
+      expect(result.current.currentData).toEqual(expectedResult),
+    );
     expect(fetchMock.calls(schemaApiRoute).length).toBe(1);
-    expect(onSuccess).toHaveBeenCalledTimes(2);
+    expect(onSuccess).toHaveBeenCalledTimes(1);
 
     rerender({ dbId: 'db2' });
-    await waitFor(() => expect(result.current.data).toEqual(expectedResult2));
+    await waitFor(() =>
+      expect(result.current.currentData).toEqual(expectedResult2),
+    );
     expect(fetchMock.calls(schemaApiRoute).length).toBe(2);
-    expect(onSuccess).toHaveBeenCalledTimes(4);
+    expect(onSuccess).toHaveBeenCalledTimes(2);
 
     rerender({ dbId: expectDbId });
-    await waitFor(() => expect(result.current.data).toEqual(expectedResult));
+    await waitFor(() =>
+      expect(result.current.currentData).toEqual(expectedResult),
+    );
     expect(fetchMock.calls(schemaApiRoute).length).toBe(2);
-    expect(onSuccess).toHaveBeenCalledTimes(5);
+    expect(onSuccess).toHaveBeenCalledTimes(2);
 
     // clean up cache
     act(() => {
       store.dispatch(api.util.invalidateTags(['Schemas']));
     });
 
-    await waitFor(() => expect(fetchMock.calls(schemaApiRoute).length).toBe(3));
+    await waitFor(() => expect(fetchMock.calls(schemaApiRoute).length).toBe(4));
     expect(fetchMock.calls(schemaApiRoute)[2][0]).toContain(expectDbId);
-    await waitFor(() => expect(result.current.data).toEqual(expectedResult));
+    await waitFor(() =>
+      expect(result.current.currentData).toEqual(expectedResult),
+    );
   });
 
   test('returns correct schema list by a catalog', async () => {
@@ -201,7 +209,7 @@ describe('useSchemas hook', () => {
 
     await waitFor(() => expect(fetchMock.calls(schemaApiRoute).length).toBe(1));
     expect(result.current.data).toEqual(expectedResult3);
-    expect(onSuccess).toHaveBeenCalledTimes(2);
+    expect(onSuccess).toHaveBeenCalledTimes(1);
 
     rerender({ dbId, catalog: 'catalog2' });
     await waitFor(() => expect(fetchMock.calls(schemaApiRoute).length).toBe(2));

diff --git a/superset-frontend/src/hooks/apiResources/schemas.ts b/superset-frontend/src/hooks/apiResources/schemas.ts
@@ -16,7 +16,7 @@
  * specific language governing permissions and limitations
  * under the License.
  */
-import { useCallback, useEffect, useRef } from 'react';
+import { useCallback, useEffect } from 'react';
 import useEffectEvent from 'src/hooks/useEffectEvent';
 import { api, JsonResponse } from './queryApi';
 
@@ -72,7 +72,6 @@ export const {
 export const EMPTY_SCHEMAS = [] as SchemaOption[];
 
 export function useSchemas(options: Params) {
-  const isMountedRef = useRef(false);
   const { dbId, catalog, onSuccess, onError } = options || {};
   const [trigger] = useLazySchemasQuery();
   const result = useSchemasQuery(
@@ -82,62 +81,34 @@ export function useSchemas(options: Params) {
     },
   );
 
-  const handleOnSuccess = useEffectEvent(
-    (data: SchemaOption[], isRefetched: boolean) => {
-      onSuccess?.(data, isRefetched);
+  const fetchData = useEffectEvent(
+    (
+      dbId: FetchSchemasQueryParams['dbId'],
+      catalog: FetchSchemasQueryParams['catalog'],
+      forceRefresh = false,
+    ) => {
+      if (dbId && (!result.currentData || forceRefresh)) {
+        trigger({ dbId, catalog, forceRefresh }).then(
+          ({ isSuccess, isError, data }) => {
+            if (isSuccess) {
+              onSuccess?.(data || EMPTY_SCHEMAS, forceRefresh);
+            }
+            if (isError) {
+              onError?.();
+            }
+          },
+        );
+      }
     },
   );
 
-  const handleOnError = useEffectEvent(() => {
-    onError?.();
-  });
-
   useEffect(() => {
-    if (dbId) {
-      trigger({ dbId, catalog, forceRefresh: false }).then(
-        ({ isSuccess, isError, data }) => {
-          if (isSuccess) {
-            handleOnSuccess(data || EMPTY_SCHEMAS, true);
-          }
-          if (isError) {
-            handleOnError();
-          }
-        },
-      );
-    }
-  }, [dbId, catalog, handleOnError, handleOnSuccess, trigger]);
+    fetchData(dbId, catalog, false);
+  }, [dbId, catalog, fetchData]);
 
   const refetch = useCallback(() => {
-    if (dbId) {
-      trigger({ dbId, catalog, forceRefresh: true }).then(
-        ({ isSuccess, isError, data }) => {
-          if (isSuccess) {
-            handleOnSuccess(data || EMPTY_SCHEMAS, true);
-          }
-          if (isError) {
-            handleOnError();
-          }
-        },
-      );
-    }
-  }, [dbId, catalog, handleOnError, handleOnSuccess, trigger]);
-
-  useEffect(() => {
-    if (isMountedRef.current) {
-      const { requestId, isSuccess, isError, isFetching, data, originalArgs } =
-        result;
-      if (!originalArgs?.forceRefresh && requestId && !isFetching) {
-        if (isSuccess) {
-          handleOnSuccess(data || EMPTY_SCHEMAS, false);
-        }
-        if (isError) {
-          handleOnError();
-        }
-      }
-    } else {
-      isMountedRef.current = true;
-    }
-  }, [catalog, result, handleOnSuccess, handleOnError]);
+    fetchData(dbId, catalog, true);
+  }, [dbId, catalog, fetchData]);
 
   return {
     ...result,