Fix union selection planning regressions from #231

[cideM]

I believe #231 introduced a few regressions in the way union selection sets are planned. We hit the first one in production right after upgrading, and discovered the others along the way.

To be clear: the idea behind #231 is good, and this PR keeps it. So what's the issue then?

Let's take the query below, which is what Apollo Client sends for pretty much any union field, since it adds __typename automatically:

query {
  foo {
    __typename
    ... on Bar {
      bar
    }
    ... on Baz {
      baz
    }
  }
}

1. __typename on a union is rejected

Since #231 the planner refuses this query:

{
  "errors": [
    {
      "extensions": { "code": "GRAPHQL_VALIDATION_FAILED" },
      "message": "unsupported selection type inside union: *ast.Field"
    }
  ]
}

The union branch in extractSelection only accepts inline fragments and fragment spreads. Everything else hits the default case:

for _, subSelection := range selection.SelectionSet {
	switch subSelection := subSelection.(type) {
	case *ast.InlineFragment:
		// ...
	case *ast.FragmentSpread:
		// ...
	default:
		return nil, fmt.Errorf("unsupported selection type inside union: %T", subSelection)
	}
}

__typename is the one field the spec allows directly on a union (3.8 Unions), so any client that adds it breaks on a union field.

Fix: accept __typename as a field on the union and request it from the service that owns the union field. Covered by TestPlanQuery_unionAllowsTypenameField.

2. Member fields are flattened onto the union

Without __typename the query works, but is sent to the wrong service. #231 strips the ... on Bar { } wrapper, plans the inner fields against Bar, and then appends the planned fields under foo (!):

query {
  foo {
    bar
    baz
  }
}

That's not valid GraphQL since the union Foo has no field bar. gqlparser reports it as Cannot query field "bar" on type "Foo".

The id the planner adds for dependent node lookups has the same problem, which is why the existing test asserted foo { id } as the expected query for the union's own service. But the expected query string was itself invalid, so I changed it to foo { ... on Bar { id } }.

Fix: after planning a member type's selections, wrap them back into ... on Member { } before adding them to the union field. Selections that belong to the union itself (__typename) stay unwrapped. Covered by TestPlanQuery_unionSingleLocationKeepsTypeConditions, plus the corrected TestPlanQuery_unionShouldNotBeQueriedOnOtherServices.

3. Named fragment spreads panic

The spread case looks up the fragment by the parent (selection), when it should be subSelection:

case *ast.FragmentSpread:
    //                                                  v-- this should be subSelection, not selection
	fragment := config.step.FragmentDefinitions.ForName(selection.Name)
	subSelectionTypes[fragment.TypeCondition] = // ...

I also believe that we're missing a lookup on the plan. For example, groupSelectionSet first checks the step, then the plan:

			// look up if we already have a definition for this fragment in the step
			defn := config.step.FragmentDefinitions.ForName(selection.Name)

			// if we don't have it
			if defn == nil {
				// look in the operation
				defn = config.plan.FragmentDefinitions.ForName(selection.Name)
				if defn == nil {
					return nil, nil, fmt.Errorf("could not find definition for directive: %s", selection.Name)
				}
			}

Fix: resolve the spread by subSelection.Name, check the step's definitions first and fall back to the operation's, and return an error instead of dereferencing nil if neither has it. Covered by TestPlanQuery_unionAllowsFragmentSpread.

4. Fragments on the union type itself

Fragments don't have to be on a member type. Apollo codebases commonly declare them on the union:

query {
  foo {
    ...fooFields
  }
}

fragment fooFields on Foo {
  __typename
  ... on Bar {
    bar
  }
}

The same goes for an inline fragment without a type condition (... @include(if: $x) { ... }), which also applies to the union. With the spread lookup fixed, these are grouped under the union type and handed to the pre-#231 code path, so they hit both of the problems above at once: the id is placed directly on the union (foo { __typename id }) and ... on Foo is sent to a service that doesn't know Foo.

Fix: when a fragment's type condition is the union itself (or absent), flatten its selections into the same per-type groups, recursively. Member fragments nested inside then get planned against the member type like everything else. Covered by TestPlanQuery_unionAllowsFragmentSpreadOnUnionType and TestPlanQuery_unionAllowsInlineFragmentWithoutTypeCondition.

Misc

The new tests use an assertPlannedQueries helper that collects the query each step would send, keyed by service URL, and compares that against the expected queries. Comparing the outgoing queries directly is what caught bugs 2 and 4, since the plan structure looked fine while the query strings were invalid. The helper normalizes both sides by sorting every selection set, because the planner groups union selections in a map and the order of the output is not stable otherwise.

One thing I have left alone: directives on the fragments inside a union (... on Bar @include(if: $x) { bar }) are dropped when the selection set is flattened into the map. That was already the case after #231 and seemed like a separate issue.

[JohnStarich]

Thanks @cideM, will take a look after I return from vacation in ~1.5 weeks!