planner: reserve a variable name for dependent step node IDs

[cideM]

This is a bug fix for something I discovered while working on other features. Definitely something I'd want to upstream.

EDIT: I created a PR upstream as well nautilus#238

Summary

Fix bug: when a query spanning two services declared a variable named $id and also used it as a field argument in the second service, every dependent node(id:) step overwrote the client's value with the ID of the object being resolved. The client's $id now keeps its own value.

The ID variable of dependent step queries is now named _gateway_node_id instead of id.

Any client query that declares a variable _gateway_node_id is rejected.

Longer Explanation

I believe that there is a bug hiding in the way dependent steps are planned and executed.

Let's take the query below:

query ($id: ID!, $category: String!) {
  allUsers {
    favoriteCatPhoto(category: $category, owner: $id) {
      URL
    }
  }
}

favoriteCatPhoto is owned by a second service. The idea behind the request is that owner: $id acts like a filter, so we only get cat photos where the owner is set to one specific ID (keep that in mind).

At runtime, the above query results in one query to the service that owns allUsers and from that we generate multiple, dependent steps, one for each user. Those dependent steps are sent to the second service in the form of node(id: ...) queries.

Those dependent step queries obviously need a user ID. What the code currently does is it checks if the query, as sent by the client, already has an id variable. If so, this variable is re-used.

// if the original query didn't have an id arg we need to add one
if variables.ForName("id") == nil {
	operation.VariableDefinitions = append(operation.VariableDefinitions, &ast.VariableDefinition{
		Variable: "id",
		Type:     ast.NonNullNamedType("ID", &ast.Position{}),
	})
}

And on the execution side, the node ID is written into the variables under that same name for every dependent step:

// execute.go, executeOneStep
// save the id as a variable to the query
variables["id"] = pointData.ID

In this case, this means that the existing id variable will be populated with each user's ID.

In the end, we generate the following query:

query ($category: String!, $id: ID!) {
  node(id: $id) {
    ... on User {
      favoriteCatPhoto(category: $category, owner: $id) {
        URL
      }
    }
  }
}

Both node(id: $id) and owner: $id now read from the same variable, and the executor sets it to u1, then u2, then u3, one dependent step per user.

The problem here is that the $id variable is now ambiguous as to what it does: originally it was meant to be a single user ID, for the filtering, but now it also carries the meaning of "each user's ID". As a result, the client gets back the wrong result.

There's a second error mode: if the client declares $id: String!, the reused declaration keeps the client's String! type, so the remote rejects node(id: $id) against node(id: ID!).

My proposed fix is to use a variable name that's unlikely to conflict, like _gateway_node_id (TestPlanQuery_clientIDVariableSurvivesNodeStep.). Also add a check that the client query does not already use that variable (TestPlanQuery_rejectsReservedNodeIDVariable).

[coveralls]

Coverage Report for CI Build 33768000178

Coverage increased (+0.07%) to 91.352%

Details

• Coverage increased (+0.07%) from the base build.
• Patch coverage: 1 uncovered change across 1 file (17 of 18 lines covered, 94.44%).
• No coverage regressions found.

Uncovered Changes

File	Changed	Covered	%
plan.go	17	16	94.12%
Total (2 files)	18	17	94.44%

Coverage Regressions

No coverage regressions found.

---

Coverage Stats

Relevant Lines:	3018
Covered Lines:	2757
Line Coverage:	91.35%
Coverage Strength:	395.38 hits per line

---

💛 - Coveralls