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.

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

fix: No joinBy dimension found in cube #1574

Merged
merged 1 commit into from
Jun 7, 2024

Conversation

bprusinowski
Copy link
Collaborator

@bprusinowski bprusinowski commented Jun 7, 2024

Fixes #1475

I am not 100% happy with the solution, but after spending more than 2h researching what might be the underlying problem, I didn't manage to solve it in another way.

I think there is some issue with caching behavior, as ChartDataWrapper that loads the data does not immediately switch to a new query, even though the variables were updated.

The current flow is:

  • AddDatasetDialog fetches the new data via executeDataCubesComponentsQuery, which should populate the client-side cache,
  • after fetching is done, it dispatches DATASET_ADD action that updates the configurator state, which then propagates the changes downstream,
  • ChartDataWrapper receives updated data, but for some reason does not immediately trigger new query, which leads to joinBy dimension not found error (as ChartConfig has already been updated).

I discovered that we had a mismatch with variables keys order when it comes to ChartDataWrapper and AddDatasetDialog (locale should not be the first, but third one) that would be one candidate for not hitting the cache.

I also tried with setting keepPreviousData to false, but none of these approaches worked; that's why I introduced another, non-optional fix to prevent crashing the application.

@ptbrowne do you have an idea on how to fix this in other way, maybe I missed something?

PS. I also tried to change caching key behavior in hook utils file to only rely on options.variables and not options, but I don't think this is the reason, as this only caches query in the useQuery hook, not in the urql client (but anyway, this also didn't work).

@bprusinowski bprusinowski requested a review from ptbrowne as a code owner June 7, 2024 10:14
Copy link

vercel bot commented Jun 7, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
visualization-tool ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jun 7, 2024 1:33pm

...joinBy: string -> string[]
@ptbrowne
Copy link
Collaborator

ptbrowne commented Jun 7, 2024

In the past, I think I had this error and solve it by completely remounting a component, using key=, would it make sense here ?

In the page linked here, Thomas shows a copy of the chart, so the flow would not involve AddDatasetDialog here right ?

@ptbrowne
Copy link
Collaborator

ptbrowne commented Jun 7, 2024

Concerning the variables order, I do not think this is the problem as urql internally normalize the variables through the operation document.

When reading a query from the cache, there's a call to normalize variables.

https://github.com/urql-graphql/urql/blob/118d74b238007264cacfb91fc12de74370d5766e/exchanges/graphcache/src/operations/query.ts#L81

When normalizing, the client re-orders the keys according to what's defined in the
"Document" of the query. The Document contains ordered variables definitions.

Below, part of the DataCubeObservationsDocument

> console.log(JSON.stringify(DataCubeObservationsDocument.definitions))
[
  {
    "kind": "OperationDefinition",
    "operation": "query",
    "name": {
      "kind": "Name",
      "value": "DataCubeObservations"
    },
    "variableDefinitions": [
      {
        "kind": "VariableDefinition",
        "variable": {
          "kind": "Variable",
          "name": {
            "kind": "Name",
            "value": "sourceType"
          }
        },
        "type": {
          "kind": "NonNullType",
          "type": {
            "kind": "NamedType",
            "name": {
              "kind": "Name",
              "value": "String"
            }
          }
        },
        "directives": []
      },
      {
        "kind": "VariableDefinition",
        "variable": {
          "kind": "Variable",
          "name": {
            "kind": "Name",
            "value": "sourceUrl"
          }
        },
        "type": {
          "kind": "NonNullType",
          "type": {
            "kind": "NamedType",
            "name": {
              "kind": "Name",
              "value": "String"
            }
          }
        },

@bprusinowski
Copy link
Collaborator Author

Nice, I didn't know about the normalization :)

I think this problem only appears when we transition from old state to new state, and for a brief moment have a mismatch, I am not sure if it would happen for already merged cubes 🤔 The problem that Thomas mentioned is solved by the new migration (this chart is using string as joinBy, while it should be an array of strings).

I tried to add such key to ChartDataWrapper's root element (chartConfig.cubes.map(d => d.iri).join(",")), but the error is not solved by this :/ Did you mean to add it in some other place?

@bprusinowski bprusinowski force-pushed the fix/no-join-by-found-in-cube branch from 1177fa3 to 17721f9 Compare June 7, 2024 13:23
@bprusinowski
Copy link
Collaborator Author

One of the original problems of this PR was solved by #1577

@bprusinowski bprusinowski merged commit be29e56 into main Jun 7, 2024
5 of 6 checks passed
@bprusinowski bprusinowski deleted the fix/no-join-by-found-in-cube branch June 7, 2024 13:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

✅ join by not found in cube
2 participants