hasura / graphql-engine

Blazing fast, instant realtime GraphQL APIs on your DB with fine grained access control, also trigger webhooks on database events.
https://hasura.io
Apache License 2.0
31.18k stars 2.77k forks source link

There can be only one variable named \"$hasura_json_var_1\ #7170

Closed jemink closed 3 years ago

jemink commented 3 years ago

I upgraded my hasura on below version hasura/graphql-engine:v2.0.0-beta.2.cli-migrations-v3

And run below query

mutation updateLocation(
  $id: Int!
  $employees: [EmployeeType]
  $badges: [BadgeType]
) {
  updateLocation(
    id: $id
    employees: $employees
    badges: $badges
  ) {
    affected_row
  }
}

Variable:-

{
"badges": [],
"employees": [],
"id": 8157
}

Then I am getting below error

{
  "data": null,
  "errors": [
    {
      "originalError": {},
      "name": "GraphQLError",
      "positions": [
        96,
        149
      ],
      "source": {
        "body": [
          "mutation ($hasura_json_var_1: [EmployeeType],$hasura_json_var_1: [BadgeType],$id: Int!) { updateLocation(badges: $hasura_json_var_1,id: $id, employees: $hasura_json_var_1) { affected_row  } }"
        ],
        "locationOffset": {
          "line": 1,
          "column": 1
        },
        "name": "GraphQL request"
      },
      "message": "There can be only one variable named \"$hasura_json_var_1\".",
      "nodes": [
        {
          "kind": "Name",
          "value": "hasura_json_var_1",
          "loc": {
            "start": 96,
            "end": 113
          }
        },
        {
          "kind": "Name",
          "value": "hasura_json_var_1",
          "loc": {
            "start": 149,
            "end": 166
          }
        }
      ],
      "stack": [
        "GraphQLError: There can be only one variable named \"$hasura_json_var_1\".",
        "    at Object.VariableDefinition (/usr/app/node_modules/graphql/validation/rules/UniqueVariableNamesRule.js:25:29)",
        "    at Object.enter (/usr/app/node_modules/graphql/language/visitor.js:323:29)",
        "    at Object.enter (/usr/app/node_modules/graphql/utilities/TypeInfo.js:370:25)",
        "    at visit (/usr/app/node_modules/graphql/language/visitor.js:243:26)",
        "    at validate (/usr/app/node_modules/graphql/validation/validate.js:69:24)",
        "    at Object.validateDocument (/usr/app/node_modules/graphql-helix/dist/process-request.js:21:30)",
        "    at /usr/app/node_modules/graphql-helix/dist/process-request.js:58:21",
        "    at Object.processRequest (/usr/app/node_modules/graphql-helix/dist/process-request.js:220:7)",
        "    at /usr/app/node_modules/@graphql-mesh/cli/bin.js:426:47",
        "    at processTicksAndRejections (node:internal/process/task_queues:96:5)"
      ],
      "locations": [
        {
          "line": 1,
          "column": 97
        },
        {
          "line": 1,
          "column": 150
        }
      ]
    },
    {
      "originalError": {},
      "name": "GraphQLError",
      "positions": [
        148,
        817
      ],
      "source": {
        "body": [
          "mutation ($hasura_json_var_1: [EmployeeType],$hasura_json_var_1: [BadgeType],$id: Int!) { updateLocation(badges: $hasura_json_var_1,id: $id,employees: $hasura_json_var_1) { affected_row  } }"
        ],
        "locationOffset": {
          "line": 1,
          "column": 1
        },
        "name": "GraphQL request"
      },
      "message": "Variable \"$hasura_json_var_1\" of type \"[BadgeType]\" used in position expecting type \"[EmployeeType]\".",
      "nodes": [
        {
          "directives": [],
          "kind": "VariableDefinition",
          "variable": {
            "kind": "Variable",
            "name": {
              "kind": "Name",
              "value": "hasura_json_var_1",
              "loc": {
                "start": 149,
                "end": 166
              }
            },
            "loc": {
              "start": 148,
              "end": 166
            }
          },
          "loc": {
            "start": 148,
            "end": 179
          },
          "type": {
            "kind": "ListType",
            "loc": {
              "start": 168,
              "end": 179
            },
            "type": {
              "kind": "NamedType",
              "name": {
                "kind": "Name",
                "value": "BadgeType",
                "loc": {
                  "start": 169,
                  "end": 178
                }
              },
              "loc": {
                "start": 169,
                "end": 178
              }
            }
          }
        },
        {
          "kind": "Variable",
          "name": {
            "kind": "Name",
            "value": "hasura_json_var_1",
            "loc": {
              "start": 818,
              "end": 835
            }
          },
          "loc": {
            "start": 817,
            "end": 835
          }
        }
      ],
      "stack": [
        "GraphQLError: Variable \"$hasura_json_var_1\" of type \"[BadgeType]\" used in position expecting type \"[EmployeeType]\".",
        "    at Object.leave (/usr/app/node_modules/graphql/validation/rules/VariablesInAllowedPositionRule.js:55:35)",
        "    at Object.leave (/usr/app/node_modules/graphql/language/visitor.js:344:29)",
        "    at Object.leave (/usr/app/node_modules/graphql/utilities/TypeInfo.js:390:21)",
        "    at visit (/usr/app/node_modules/graphql/language/visitor.js:243:26)",
        "    at validate (/usr/app/node_modules/graphql/validation/validate.js:69:24)",
        "    at Object.validateDocument (/usr/app/node_modules/graphql-helix/dist/process-request.js:21:30)",
        "    at /usr/app/node_modules/graphql-helix/dist/process-request.js:58:21",
        "    at Object.processRequest (/usr/app/node_modules/graphql-helix/dist/process-request.js:220:7)",
        "    at /usr/app/node_modules/@graphql-mesh/cli/bin.js:426:47",
        "    at processTicksAndRejections (node:internal/process/task_queues:96:5)"
      ],
      "locations": [
        {
          "line": 1,
          "column": 149
        },
        {
          "line": 1,
          "column": 818
        }
      ]
    }
  ]
}

But it is working fine with alpha 10

hasura/graphql-engine:v2.0.0-alpha.10.cli-migrations-v3
btodorce commented 3 years ago

This is causing us a massive issue, since we have multiple mutations that previously worked that are suddenly broken and we "forced" to use the alpha v10 instead of the latest version.

jemink commented 3 years ago

EmployeeType

type EmployeeType {
id: Int
name: String
}

BadgeType

type BadgeType {
icon: String
name: String
}

Also I connected remote schema inside my hasura and created custom resolver. It is working fine on remote schema url

abooij commented 3 years ago

We've had some internal discussions about this today. There seems to be an issue with the way the GraphQL queries that are sent to remotes GraphQL servers are built. In particular, there is a phase that tries to come up with fresh GraphQL variables, so that presets for certain object fields can be incorporated. This is essentially what resolveRemoteVariable here is concerned with.

The philosophy of that RemoteJSONValue case of that code (starting on line 185) is that sometimes, we're kind of forced to come up with a new variable to represent the JSON value that we'd truly like to pass to the remote GraphQL server, and so it generates a fresh name for it, so that we can truly pass that value as JSON, without having to inline anything into the GraphQL query (which is not possible in general). The so-called variable cache keeps track of which JSON value is passed under which variable name.

In this case, the value [] gets passed twice. The code believes it can simply reuse the same variable name, since, from a JSON point of view, it's the same value. This is not quite right, however, as from a GraphQL point of view they are two values of different types - in this case [EmployeeBadge] and [BadgeType], and so we need to pass them separately.


The above explains why the remote GraphQL server chokes on the query it receives. However, for me there is one additional point of confusion: why did Hasura declare the variable hasura_json_var_1 twice? From the log:

          "mutation ($hasura_json_var_1: [EmployeeType],$hasura_json_var_1: [BadgeType],$id: Int!) { updateLocation(badges: $hasura_json_var_1,id: $id, employees: $hasura_json_var_1) { affected_row  } }"

I believe the answer can be found here. At that point, all we've generated for the GraphQL query we're building, is an (augmented) GraphQL selection set. That particular method generates the list of variable declarations. The particular line linked extracts the desired list of variable declarations from it as follows:

  1. it (intuitively speaking*) syntactically browses through that selection set, saving all (augmented) variables being used
  2. it deduplicates those usages into a HashSet,
  3. it converts that set into a list,
  4. and finally, for each element of the list, it converts the (augmented) variable usage into a variable declaration.

(* The reason this is at best an intuition is that in reality such things are represented in graphql-engine by abstract syntax trees.)

The reason that hasura_json_var_1 isn't being deduplicated by the HashSet is that the augmented variables indeed contain type information (in this case something representing [EmployeeType] resp. [BadgeType]) and are hence distinct.


@0x777 suggested that the so-called variable cache should index based on not just JSON values, but also on GraphQL types. I think that's necessary, but I'm not yet fully convinced it's sufficient. The augmented variable usages have a third identifying element, namely a VariableInfo, which indicates whether the variable is required or optional. I believe that, for the purpose of generating correct names, this is not relevant, since at this point if a variable wasn't assigned a value yet then we'd have no way to generate a valid GraphQL query. Additionally, in the current version of the code, when the RemoteJSONValue case of resolveRemoteVariable fires, it always marks the variable as VIRequired.

But the duplication problem above could potentially apply, especially if at some point in the future we would for some reason also want to generate optional variables here. So to avoid declaring a variable twice, I think we should also keep track of whether the variable is required or not.

Anyway, the last paragraph is not super important to get right, I think. If the so-called variable cache simply indexes both by value and by type, this issue should be avoided.

@nicuveo @jkachmar

jemink commented 3 years ago

I can't able to access this page https://github.com/hasura/graphql-engine-mono/blob/6417ffc9b24a1a2c8437f77cd8c3c7e742f9c5fc/server/src-lib/Hasura/GraphQL/Execute/Remote.hs#L78 Also my input is optional so If user doesn't select anything then I am passing empty array that is a expected scenarios

abooij commented 3 years ago

@jemink Thanks, I've updated the links, they should work for you now.

abooij commented 3 years ago

@jemink Ah, and I realize it perhaps wasn't clear from my phrasing, but indeed this is a bug, and we're fixing it. My message above was simply analyzing what's going on internally inside graphql-engine.

jemink commented 3 years ago

Hello @abooij Is there any update regarding this issue ?

abooij commented 3 years ago

@jemink A fix is being developed and will be released ASAP.

jemink commented 3 years ago

Thank you. Please let me know when you released the new version

jkachmar commented 3 years ago

Closed via c322af93f8391346bfaa32943fe1f14291c89251.