Fix: expand all possible abstract ids

[gmac]

This fixes a bug with the notoriously tricky hint selections of abstract types.

The problem

Here's a schema...

type Apple @boundary {
  id: ID!
  name: String!
}
type Banana @boundary {
  id: ID!
  name: String!
}
union Fruit = Apple | Banana

type Query {
  fruit: Fruit
}

Then here's a query:

query {
  fruit {
    ...on Apple { name }
  }
}

This query works fine if the endpoint returns an Apple, but it fails if a Banana is returned:

{
  "errors": [
    {
      "message": "boundaryIDFromMap: 'id' or '_id' not found"
    }
  ],
  "data": null
}

Why does a Banana fail? It's because ID hint selections are only being added to user-defined boundary fragments, i.e: Apple. In the above selection, the user hasn't explicitly made a Banana selection, so there's no ID hint added for a banana. That means that when a Banana gets is returned, the executor comes up with a boundary type that returned no ID.

The fix

This updates the query planner to expand abstract types with ID selection hints for all possible boundary types. The above request now comes out looking like this:

query {
  fruit {
    ... on Apple {
      _id: id
    }
    ... on Banana {
      _id: id
    }
    ... on Apple {
      name
    }
  }
}

Now, regardless of what the user has selected, we know that all possible boundary results will come back with the expected ID. GraphQL Tools stitching does something very similar. It's generally better to normalize the request than to make complicated assumptions about the result.

Gross, there's now two ...on Apple fragments, can we consolidate those? Sure, we could using something like selectionSetHasFieldNamed for fragments. However, I'm generally against adding loops for the sake of human cosmetics when GraphQL servers don't care if fragments or fields get duplicated in the selection. But if there's strong resistance to this, I'm happy to prettify it.

Tests

Opening this now for initial review on the implementation... I'm still learning Go, so I'm a bit slow to write code and still have to figure out how to write/run tests.

[nmaquet]

@gmac This is excellent. Thanks a lot for this. I'm happy to write unit tests for this if you'd prefer. Should be a simple case of adding the Apple/Banana example to plan_test.go. As to whether it's OK to have two fragments on the same type in the query, IMHO it's completely fine. The spec allows its and all GraphQL implementations can deal with it (and merging it would be a pain) so I'm happy with this as is.

[lucianjon]

Good catch and thanks for the fix @gmac. I agree with @nmaquet that the approach makes sense, bramble isn't optimised to create human readable queries. It's good to see some of the edge cases around fragments not getting matched being discovered and fixed, we had a similar case with #76.

[gmac]

Thanks @nmaquet & @lucianjon, tests have been added. Bramble is teaching me Go, and I'm really enjoying it! This PR is good to go on my end!

[codecov-commenter]

Codecov Report

Merging #81 (f0483da) into main (dbac320) will increase coverage by 0.26%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main      #81      +/-   ##
==========================================
+ Coverage   66.78%   67.05%   +0.26%     
==========================================
  Files          24       24              
  Lines        2728     2747      +19     
==========================================
+ Hits         1822     1842      +20     
+ Misses        750      749       -1     
  Partials      156      156              

Impacted Files	Coverage Δ
plan.go	81.06% <100.00%> (+1.60%)	⬆️
auth.go	87.42% <0.00%> (+0.62%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update dbac320...f0483da. Read the comment docs.