fix: No joinBy dimension found in cube

bprusinowski commented 4 weeks ago

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).

vercel[bot] commented 4 weeks ago

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

ptbrowne commented 4 weeks ago

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 commented 4 weeks ago

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 commented 4 weeks ago

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 commented 4 weeks ago

One of the original problems of this PR was solved by https://github.com/visualize-admin/visualization-tool/pull/1577

visualize-admin / visualization-tool

fix: No joinBy dimension found in cube #1574