Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Duplicate requests to /api/card from Query Builder #31905

Closed
iethree opened this issue Jun 27, 2023 · 2 comments · Fixed by #33898
Closed

Duplicate requests to /api/card from Query Builder #31905

iethree opened this issue Jun 27, 2023 · 2 comments · Fixed by #33898
Assignees
@deniskaber
Labels
.Frontend .Performance Priority:P3 Cosmetic bugs, minor bugs with a clear workaround Querying/GUI Query builder catch-all, including simple mode Type:Bug Product defects
Milestone
0.48

Comments

@iethree
Copy link
Contributor

iethree commented Jun 27, 2023 •
edited
Loading

Describe the bug

Regression between 45 and 46.

The query builder makes 2 identical request to GET /api/card/{id} on load.

Screen Shot 2023-06-27 at 9 58 23 AM

To Reproduce

  • Make any question or model
  • open the network inspector
  • refresh the page
  • see 2 of the same GET request

Expected behavior

1 request to GET /api/card/{id}

Information about your Metabase installation

{
  "browser-info": {
    "language": "en-US",
    "platform": "MacIntel",
    "userAgent": "Mozilla/5.0 (Macintosh; Intel Mac OS X 10_15_7) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/114.0.0.0 Safari/537.36",
    "vendor": "Google Inc."
  },
  "system-info": {
    "file.encoding": "UTF-8",
    "java.runtime.name": "OpenJDK Runtime Environment",
    "java.runtime.version": "18.0.1+0",
    "java.vendor": "Homebrew",
    "java.vendor.url": "https://github.com/Homebrew/homebrew-core/issues",
    "java.version": "18.0.1",
    "java.vm.name": "OpenJDK 64-Bit Server VM",
    "java.vm.version": "18.0.1+0",
    "os.name": "Mac OS X",
    "os.version": "12.4",
    "user.language": "en",
    "user.timezone": "America/Denver"
  },
  "metabase-info": {
    "databases": [
      "h2",
      "postgres"
    ],
    "hosting-env": "unknown",
    "application-database": "h2",
    "application-database-details": {
      "database": {
        "name": "H2",
        "version": "2.1.212 (2022-04-09)"
      },
      "jdbc-driver": {
        "name": "H2 JDBC Driver",
        "version": "2.1.212 (2022-04-09)"
      }
    },
    "run-mode": "prod",
    "version": {
      "date": "2023-06-14",
      "tag": "v0.46.5",
      "branch": "release-x.46.x",
      "hash": "272e3e6"
    },
    "settings": {
      "report-timezone": null
    }
  }
}

Severity

not user visible, an under-the-hood performance issue

Additional context

No response
@iethree iethree added Type:Bug Product defects Priority:P3 Cosmetic bugs, minor bugs with a clear workaround .Performance Querying/GUI Query builder catch-all, including simple mode labels Jun 27, 2023
@WiNloSt WiNloSt self-assigned this Sep 7, 2023
@WiNloSt
Copy link
Member

WiNloSt commented Sep 7, 2023

The first call is a little hard to track where it originates from but the second request is from here.

metabase/frontend/src/metabase/query_builder/actions/core/initializeQB.ts

Lines 107 to 112 in e7363d9

async function fetchAndPrepareSavedQuestionCards(
cardId: number,
dispatch: Dispatch,
getState: GetState,
) {
const card = await loadCard(cardId, { dispatch, getState });

In loadCards we specify reload: true

metabase/frontend/src/metabase/lib/card.js

Line 29 in e7363d9

await dispatch(Questions.actions.fetch({ id: cardId }, { reload: true }));

It'd be quite complicated to make sure this second request doesn't fire.

I could think of one of the possible fix:

  1. when specifying reload: true it won't always reload and will use the cache from the past x milliseconds, so in case we call reload: true in rapid succession, we will only call the API once.

I however don't think this is worth pursuing since there would be no visible change to users and the fix would be quite complicated + need more discussion on the exact number we want to delay before actually firing a new request. I don't think it's worth all the effort at the end of the day.

@WiNloSt WiNloSt removed their assignment Sep 7, 2023
@deniskaber deniskaber self-assigned this Sep 13, 2023
@deniskaber deniskaber mentioned this issue Sep 14, 2023
Merged
1 task
@deniskaber
Copy link
Contributor

The first call is a little hard to track where it originates from but the second request is from here.

The first call comes from MainNavbar component. We need it to select an active collection in the left menu.
As @WiNloSt mentioned, the second call has reload: true. Removing it can break some other use cases we have - like this test, for instance:

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

Line 174 in 83ae490

it("should not reload the model for record in the same model", () => {

There we navigate to home page, then search some stuff in search box. Search api call loads metadata about a model, however this data does not contain the full entity data needed to display a model. Because of that we cannot just remove reload flag.

So, it seems, we should make this cache smarter to understand in which cases we have partial data in state.

The logic for the first api request has been added here - https://github.com/metabase/metabase/pull/28057/files#diff-a6742c995fe25a651bfe9c2bf58296681d8440cc022c9569d338150364fdfb88R135 , it is a deliberate change.

Also, in my first naive attempt to fix the issue, @ranquild mentioned that we should not create implicit logical dependecy between the left nav menu and main content area - #33898 (review).

In the end, we cannot just remove either of api calls, and the second one has reload: true to cope with some other use cases. I think we can add some logic to do reload only when current cached entity is not complete.

@deniskaber deniskaber added this to the 0.48 milestone Oct 4, 2023
@deniskaber deniskaber mentioned this issue Oct 13, 2023
Merged
1 task
@deploysentinel deploysentinel bot mentioned this issue Nov 9, 2023
Merged
This was referenced Jan 11, 2024
Merged
Merged
Merged
Merged
Merged
Closed
Merged
Merged
Merged
Merged
Merged
Closed
Merged
This was referenced Feb 5, 2024
Merged
Merged
Merged
Merged
Merged
Merged
Merged
Merged
Merged
Merged
Merged
Merged
Merged
Merged
Merged
Merged
Merged
Merged
Merged
Merged
Merged
Merged
Open
Merged
Merged
Merged
Merged
Merged
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@deniskaber deniskaber

Labels
.Frontend .Performance Priority:P3 Cosmetic bugs, minor bugs with a clear workaround Querying/GUI Query builder catch-all, including simple mode Type:Bug Product defects
Projects
None yet
Milestone
0.48
Development

Successfully merging a pull request may close this issue.

Use question from state for navbar instead of api call metabase/metabase
4 participants
@WiNloSt @deniskaber @ranquild @iethree