Fix duplicated entities api calls with better cache check (#33898)

* Fix duplicated api call with better cache check

* Fix issues with e2e

* Fix e2e tests

* Simplify solution

* Add entities loader fetch requests queue

* Refactor waiting for loaded state to await for promise

* Fix e2e test

* Fix unit tests

e2e/test/scenarios/admin/settings/localization.cy.spec.js

@@ -23,6 +23,7 @@ describe("scenarios > admin > localization", () => {
     // filter: created before June 1st, 2022
     // summarize: Count by CreatedAt: Week
 
+    cy.intercept("POST", "/api/card/*/query").as("cardQuery");
     cy.createQuestion({
       name: "Orders created before June 1st 2022",
       query: {
@@ -39,6 +40,8 @@ describe("scenarios > admin > localization", () => {
     // eslint-disable-next-line no-unscoped-text-selectors -- deprecated usage
     cy.findByText("Orders created before June 1st 2022").click();
 
+    cy.wait("@cardQuery");
+
     cy.log("Assert the dates on the X axis");
     // it's hard and tricky to invoke hover in Cypress, especially in our graphs
     // that's why we have to assert on the x-axis, instead of a popover that shows on a dot hover

e2e/test/scenarios/models/model-indexes.cy.spec.js

@@ -214,8 +214,7 @@ describe("scenarios > model indexes", () => {
       cy.findByText("Doohickey");
     });
 
-    // for some reason we hit this endpoint twice on initial load
-    expectCardQueries(2);
+    expectCardQueries(1);
 
     cy.get("body").type("{esc}");
 
@@ -232,7 +231,7 @@ describe("scenarios > model indexes", () => {
       cy.findByText("Upton, Kovacek and Halvorson");
     });
 
-    expectCardQueries(2);
+    expectCardQueries(1);
   });
 });
 

e2e/test/scenarios/models/reproductions/31905-duplicated-api-request.cy.spec.js

+import { restore } from "e2e/support/helpers";
+import { SAMPLE_DATABASE } from "e2e/support/cypress_sample_database";
+
+const { ORDERS_ID } = SAMPLE_DATABASE;
+
+describe("issue 31905", () => {
+  beforeEach(() => {
+    restore();
+    cy.signInAsAdmin();
+
+    cy.intercept("GET", "/api/card/*").as("card");
+
+    cy.createQuestion(
+      {
+        name: "Orders Model",
+        dataset: true,
+        query: { "source-table": ORDERS_ID, limit: 2 },
+      },
+      { visitQuestion: true },
+    );
+  });
+
+  it("should not send more than one same api requests to load a model (metabase#31905)", () => {
+    cy.get("@card.all").should("have.length", 1);
+  });
+});

e2e/test/scenarios/sharing/alert/email-alert.cy.spec.js

@@ -13,7 +13,7 @@ const { PEOPLE_ID } = SAMPLE_DATABASE;
 describe("scenarios > alert > email_alert", { tags: "@external" }, () => {
   beforeEach(() => {
     cy.intercept("POST", "/api/alert").as("savedAlert");
-    cy.intercept("GET", "/api/card/*").as("card");
+    cy.intercept("POST", "/api/card").as("saveCard");
 
     restore();
     cy.signInAsAdmin();
@@ -71,7 +71,7 @@ describe("scenarios > alert > email_alert", { tags: "@external" }, () => {
       cy.findByRole("button", { name: "Save" }).click();
     });
 
-    cy.wait("@card");
+    cy.wait("@saveCard");
 
     modal().within(() => {
       cy.findByRole("button", { name: "Set up an alert" }).click();

frontend/src/metabase/lib/card.js

@@ -26,10 +26,24 @@ export function startNewCard(type, databaseId, tableId) {
 // TODO: move to redux
 export async function loadCard(cardId, { dispatch, getState }) {
   try {
-    await dispatch(Questions.actions.fetch({ id: cardId }, { reload: true }));
+    await dispatch(
+      Questions.actions.fetch(
+        { id: cardId },
+        {
+          properties: [
+            "id",
+            "dataset_query",
+            "display",
+            "visualization_settings",
+          ], // complies with Card interface
+        },
+      ),
+    );
+
     const question = Questions.selectors.getObject(getState(), {
       entityId: cardId,
     });
+
     return question?.card();
   } catch (error) {
     console.error("error loading card", error);

frontend/src/metabase/lib/redux/utils.js

@@ -10,7 +10,9 @@ import {
   setRequestLoaded,
   setRequestError,
   setRequestUnloaded,
+  setRequestPromise,
 } from "metabase/redux/requests";
+import { delay } from "metabase/lib/promise";
 
 // convenience
 export { combineReducers, compose } from "@reduxjs/toolkit";
@@ -79,7 +81,11 @@ export const fetchData = async ({
     const requestState = getIn(getState(), ["requests", ...statePath]);
     if (!requestState || requestState.error || reload) {
       dispatch(setRequestLoading(statePath, queryKey));
-      const data = await getData();
+
+      const queryPromise = getData();
+      dispatch(setRequestPromise(statePath, queryKey, queryPromise));
+
+      const data = await queryPromise;
 
       // NOTE Atte Keinänen 8/23/17:
       // Dispatch `setRequestLoaded` after clearing the call stack because we want to the actual data to be updated
@@ -114,7 +120,11 @@ export const updateData = async ({
   const statePath = requestStatePath.concat(["update"]);
   try {
     dispatch(setRequestLoading(statePath, queryKey));
-    const data = await putData();
+
+    const queryPromise = putData();
+    dispatch(setRequestPromise(statePath, queryKey, queryPromise));
+
+    const data = await queryPromise;
     dispatch(setRequestLoaded(statePath, queryKey));
 
     (dependentRequestStatePaths || []).forEach(statePath =>
@@ -230,7 +240,10 @@ export function withRequestState(getRequestStatePath, getQueryKey) {
       try {
         dispatch(setRequestLoading(statePath, queryKey));
 
-        const result = await thunkCreator(...args)(dispatch, getState);
+        const queryPromise = thunkCreator(...args)(dispatch, getState);
+        dispatch(setRequestPromise(statePath, queryKey, queryPromise));
+
+        const result = await queryPromise;
 
         // Dispatch `setRequestLoaded` after clearing the call stack because
         // we want to the actual data to be updated before we notify
@@ -270,45 +283,64 @@ function withCachedData(
   // thunk decorator:
   return thunkCreator =>
     // thunk creator:
-    (...args) =>
-    // thunk:
-    (dispatch, getState) => {
-      const options = args[args.length - 1] || {};
-      const { useCachedForbiddenError, reload, properties } = options;
-
-      const existingStatePath = getExistingStatePath(...args);
-      const requestStatePath = ["requests", ...getRequestStatePath(...args)];
-      const newQueryKey = getQueryKey && getQueryKey(...args);
-      const existingData = getIn(getState(), existingStatePath);
-      const { loading, loaded, queryKey, error } =
-        getIn(getState(), requestStatePath) || {};
-
-      // Avoid requesting data with permanently forbidded access
-      if (useCachedForbiddenError && error?.status === 403) {
-        throw error;
-      }
+    (...args) => {
+      async function thunk(dispatch, getState) {
+        const options = args[args.length - 1] || {};
+        const { useCachedForbiddenError, reload, properties } = options;
+
+        const existingStatePath = getExistingStatePath(...args);
+        const requestStatePath = ["requests", ...getRequestStatePath(...args)];
+        const newQueryKey = getQueryKey && getQueryKey(...args);
+        const existingData = getIn(getState(), existingStatePath);
+        const { loading, loaded, queryKey, error } =
+          getIn(getState(), requestStatePath) || {};
+
+        // Avoid requesting data with permanently forbidden access
+        if (useCachedForbiddenError && error?.status === 403) {
+          throw error;
+        }
+
+        const hasRequestedProperties =
+          properties &&
+          existingData &&
+          _.all(properties, p => existingData[p] !== undefined);
+
+        // return existing data if
+        if (
+          // we don't want to reload
+          // the check is a workaround for EntityListLoader passing reload function to children
+          reload !== true &&
+          // reload if the query used to load an entity has changed even if it's already loaded
+          newQueryKey === queryKey
+        ) {
+          // and we have a non-error request state or have a list of properties that all exist on the object
+          if (loaded || hasRequestedProperties) {
+            return existingData;
+          } else if (loading) {
+            const queryPromise = getIn(
+              getState(),
+              requestStatePath,
+            )?.queryPromise;
+
+            if (queryPromise) {
+              // wait for current loading request to be resolved
+              await queryPromise;
+
+              // need to wait for next tick to allow loaded request data to be processed and avoid loops
+              await delay(10);
+
+              // retry this function after waited request gets resolved
+              return thunk(dispatch, getState);
+            }
+
+            return existingData;
+          }
+        }
 
-      const hasRequestedProperties =
-        properties &&
-        existingData &&
-        _.all(properties, p => existingData[p] !== undefined);
-
-      // return existing data if
-      if (
-        // we don't want to reload
-        // the check is a workaround for EntityListLoader passing reload function to children
-        reload !== true &&
-        // reload if the query used to load an entity has changed even if it's already loaded
-        newQueryKey === queryKey &&
-        // and we have a an non-error request state or have a list of properties that all exist on the object
-        (loading || loaded || hasRequestedProperties)
-      ) {
-        // TODO: if requestState is LOADING can we wait for the other reques
-        // to complete and return that result instead?
-        return existingData;
-      } else {
         return thunkCreator(...args)(dispatch, getState);
       }
+
+      return thunk;
     };
 }
 

frontend/src/metabase/lib/redux/utils.unit.spec.js

@@ -50,7 +50,25 @@ describe("Metadata", () => {
       const argsDefault = getDefaultArgs({});
       const data = await fetchData(argsDefault);
       await delay(10);
-      expect(argsDefault.dispatch.mock.calls.length).toEqual(2);
+
+      expect(argsDefault.dispatch).toHaveBeenCalledTimes(3);
+
+      expect(argsDefault.dispatch).toHaveBeenCalledWith(
+        expect.objectContaining({
+          type: "metabase/requests/SET_REQUEST_LOADING",
+        }),
+      );
+      expect(argsDefault.dispatch).toHaveBeenCalledWith(
+        expect.objectContaining({
+          type: "metabase/requests/SET_REQUEST_PROMISE",
+        }),
+      );
+      expect(argsDefault.dispatch).toHaveBeenCalledWith(
+        expect.objectContaining({
+          type: "metabase/requests/SET_REQUEST_LOADED",
+        }),
+      );
+
       expect(data).toEqual(args.newData);
     });
 
@@ -59,14 +77,14 @@ describe("Metadata", () => {
         requestState: args.requestStateLoading,
       });
       const dataLoading = await fetchData(argsLoading);
-      expect(argsLoading.dispatch.mock.calls.length).toEqual(0);
+      expect(argsLoading.dispatch).toHaveBeenCalledTimes(0);
       expect(dataLoading).toEqual(args.existingData);
 
       const argsLoaded = getDefaultArgs({
         requestState: args.requestStateLoaded,
       });
       const dataLoaded = await fetchData(argsLoaded);
-      expect(argsLoaded.dispatch.mock.calls.length).toEqual(0);
+      expect(argsLoaded.dispatch).toHaveBeenCalledTimes(0);
       expect(dataLoaded).toEqual(args.existingData);
     });
 
@@ -76,7 +94,25 @@ describe("Metadata", () => {
       });
       const dataError = await fetchData(argsError);
       await delay(10);
-      expect(argsError.dispatch.mock.calls.length).toEqual(2);
+
+      expect(argsError.dispatch).toHaveBeenCalledTimes(3);
+
+      expect(argsError.dispatch).toHaveBeenCalledWith(
+        expect.objectContaining({
+          type: "metabase/requests/SET_REQUEST_LOADING",
+        }),
+      );
+      expect(argsError.dispatch).toHaveBeenCalledWith(
+        expect.objectContaining({
+          type: "metabase/requests/SET_REQUEST_PROMISE",
+        }),
+      );
+      expect(argsError.dispatch).toHaveBeenCalledWith(
+        expect.objectContaining({
+          type: "metabase/requests/SET_REQUEST_LOADED",
+        }),
+      );
+
       expect(dataError).toEqual(args.newData);
     });
 
@@ -92,7 +128,7 @@ describe("Metadata", () => {
         const dataFail = await fetchData(argsFail).catch(error =>
           console.log(error),
         );
-        expect(argsFail.dispatch.mock.calls.length).toEqual(2);
+        expect(argsFail.dispatch).toHaveBeenCalledTimes(2);
         expect(dataFail).toEqual(args.existingData);
       } catch (error) {
         return;
@@ -104,28 +140,28 @@ describe("Metadata", () => {
     it("should return new data regardless of previous request state", async () => {
       const argsDefault = getDefaultArgs({});
       const data = await updateData(argsDefault);
-      expect(argsDefault.dispatch.mock.calls.length).toEqual(2);
+      expect(argsDefault.dispatch).toHaveBeenCalledTimes(3);
       expect(data).toEqual(args.newData);
 
       const argsLoading = getDefaultArgs({
         requestState: args.requestStateLoading,
       });
       const dataLoading = await updateData(argsLoading);
-      expect(argsLoading.dispatch.mock.calls.length).toEqual(2);
+      expect(argsLoading.dispatch).toHaveBeenCalledTimes(3);
       expect(dataLoading).toEqual(args.newData);
 
       const argsLoaded = getDefaultArgs({
         requestState: args.requestStateLoaded,
       });
       const dataLoaded = await updateData(argsLoaded);
-      expect(argsLoaded.dispatch.mock.calls.length).toEqual(2);
+      expect(argsLoaded.dispatch).toHaveBeenCalledTimes(3);
       expect(dataLoaded).toEqual(args.newData);
 
       const argsError = getDefaultArgs({
         requestState: args.requestStateError,
       });
       const dataError = await updateData(argsError);
-      expect(argsError.dispatch.mock.calls.length).toEqual(2);
+      expect(argsError.dispatch).toHaveBeenCalledTimes(3);
       expect(dataError).toEqual(args.newData);
     });
 
@@ -137,7 +173,7 @@ describe("Metadata", () => {
       });
       const data = await updateData(argsFail);
       await delay(10);
-      expect(argsFail.dispatch.mock.calls.length).toEqual(2);
+      expect(argsFail.dispatch).toHaveBeenCalledTimes(2);
       expect(data).toEqual(args.existingData);
     });
   });

frontend/src/metabase/lib/utils.unit.spec.ts

@@ -3,7 +3,7 @@ import {
   compareVersions,
   isEmpty,
   isJWT,
-} from "metabase/lib/utils";
+} from "./utils";
 
 describe("utils", () => {
   describe("versionToNumericComponents", () => {

frontend/src/metabase/nav/containers/MainNavbar/MainNavbar.tsx

@@ -7,12 +7,12 @@ import type { LocationDescriptor } from "history";
 import * as Urls from "metabase/lib/urls";
 
 import { closeNavbar, openNavbar } from "metabase/redux/app";
-import Questions from "metabase/entities/questions";
 
 import { getDashboard } from "metabase/dashboard/selectors";
 
 import type { Dashboard } from "metabase-types/api";
 import type { State } from "metabase-types/store";
+import { useQuestionQuery } from "metabase/common/hooks";
 import type Question from "metabase-lib/Question";
 
 import MainNavbarContainer from "./MainNavbarContainer";
@@ -34,6 +34,7 @@ interface EntityLoaderProps {
 
 interface StateProps {
   dashboard?: Dashboard;
+  questionId?: number;
 }
 
 interface DispatchProps extends MainNavbarDispatchProps {
@@ -45,12 +46,14 @@ type Props = MainNavbarOwnProps &
   StateProps &
   DispatchProps;
 
-function mapStateToProps(state: State) {
+function mapStateToProps(state: State, props: MainNavbarOwnProps) {
   return {
     // Can't use dashboard entity loader instead.
     // The dashboard page uses DashboardsApi.get directly,
     // so we can't re-use data between these components.
     dashboard: getDashboard(state),
+
+    questionId: maybeGetQuestionId(state, props),
   };
 }
 
@@ -64,13 +67,17 @@ function MainNavbar({
   isOpen,
   location,
   params,
-  question,
+  questionId,
   dashboard,
   openNavbar,
   closeNavbar,
   onChangeLocation,
   ...props
 }: Props) {
+  const { data: question } = useQuestionQuery({
+    id: questionId,
+  });
+
   useEffect(() => {
     function handleSidebarKeyboardShortcut(e: KeyboardEvent) {
       if (e.key === "." && (e.ctrlKey || e.metaKey)) {
@@ -132,11 +139,6 @@ function maybeGetQuestionId(
 }
 
 // eslint-disable-next-line import/no-default-export -- deprecated usage
-export default _.compose(
-  connect(mapStateToProps, mapDispatchToProps),
-  Questions.load({
-    id: maybeGetQuestionId,
-    loadingAndErrorWrapper: false,
-    entityAlias: "question",
-  }),
-)(MainNavbar);
+export default _.compose(connect(mapStateToProps, mapDispatchToProps))(
+  MainNavbar,
+);

frontend/src/metabase/redux/requests.js

@@ -3,7 +3,18 @@ import { updateIn, assoc, getIn } from "icepick";
 
 export const setRequestLoading = createAction(
   "metabase/requests/SET_REQUEST_LOADING",
-  (statePath, queryKey) => ({ statePath, queryKey }),
+  (statePath, queryKey) => ({
+    statePath,
+    queryKey,
+  }),
+);
+export const setRequestPromise = createAction(
+  "metabase/requests/SET_REQUEST_PROMISE",
+  (statePath, queryKey, queryPromise) => ({
+    statePath,
+    queryKey,
+    queryPromise,
+  }),
 );
 export const setRequestLoaded = createAction(
   "metabase/requests/SET_REQUEST_LOADED",
@@ -29,14 +40,22 @@ const initialRequestState = {
 const requestStateReducer = handleActions(
   {
     [setRequestLoading]: {
-      next: (state, { payload: { queryKey } }) => ({
+      next: (state, { payload: { queryKey, queryPromise } }) => ({
         ...state,
         queryKey,
+        queryPromise,
         loading: true,
         loaded: false,
         error: null,
       }),
     },
+    [setRequestPromise]: {
+      next: (state, { payload: { queryKey, queryPromise } }) => ({
+        ...state,
+        queryKey,
+        queryPromise,
+      }),
+    },
     [setRequestLoaded]: {
       next: (state, { payload: { queryKey } }) => ({
         ...state,
@@ -61,6 +80,7 @@ const requestStateReducer = handleActions(
         ...state,
         loaded: false,
         error: null,
+        queryPromise: null,
       }),
     },
   },