Union type not being merged correctly

[edwinmo-splunk]

Hi! We're having difficulty with the merger not returning the correct PossibleTypes for a Union type. For an example based on our use case, I have defined the following union in both remote schemas:

type Bar { ... }
type Baz { ... }
union Foo = Bar | Baz

After introspecting each remote schema separately, the github.com/vektah/gqlparser/*ast.Schema.PossibleTypes["Foo"] I get looks like the following for both (which is correct!):

[definition{"Bar"}, definition{"Baz"}]

The two remote schemas are being served by Apollo (node/js) and Graphqlgen (Golang)

However, when running the same introspection query on the Nautilus Gateway (after merging both Remote Schemas), I get the following for github.com/vektah/gqlparser/*ast.Schema.PossibleTypes["Foo"]:

[definition{"Foo"}]

This causes the GraphQL Playground user interface to go into a stack overflow trying to define Foo infinitely.

---

The piece of code that I found interesting is the following:

https://github.com/nautilus/gateway/blob/master/merge.go#L95-L96

This indiscriminately adds itself as a PossibleType, but I didn't see anywhere that would add a union's possible types to itself. I was able to get around this by doing the following:

unionSet := map[string]bool{}

if definition.Kind == ast.Union {
	for _, possibleType := range definition.Types {
		for _, typedef := range types[possibleType] {
			if _, exists := unionSet[typedef.Name]; !exists {
				unionSet[typedef.Name] = true
				result.AddPossibleType(name, typedef)
			}
		}
	}
} else {
	// register the type as an implementer of itself
	result.AddPossibleType(name, definition)
}

I had to use a set to avoid returning the following for a union's PossibleTypes:

[definition{Bar}, definition{Bar}, definition{Baz}, definition{Baz}]

Few things I'd like to point out:

• Introspection of remote schemas is working fine. The resulting *ast.Schemas contain the expected PossibleTypes for the union Foo
• It is only after merging the schemas (https://github.com/nautilus/gateway/blob/master/gateway.go#L145) that the resulting *ast.Schema contains itself as a PossibleType for union Foo.

Any help is appreciated! Awesome job on this library, I hope that it continues growing and improving. Really looking forward to v1 😄

[AlecAivazis]

Hey! I'm sorry to hear you are running into problems.

Just to make sure i'm following, you're running into issues when a union type is spread across service boundaries? If so, #43 already tracks this.

Assuming that's what you mean, the problem is not just that PossibleTypes needs to do the merge. A service will error if we ask for an id of a type it does not recognize so when we add a branch in a query plan, we have to have some way of only firing a step when __typename is some value. It's definitely doable but this extra complexity is why I did not do it when I was initially building this out.

That being said, this work should also set up #66 since they both set up conditional forks in a query plan.

[edwinmo-splunk]

Hey Alec! Thanks for the quick response.

Just to make sure i'm following, you're running into issues when a union type is spread across service boundaries?

Not quite, the union type is identical in both services. I did consider #43 as a related issue, but decided this was different and was expecting this to be supported.

I believe I tested removing one of the services to see what sort of schema the nautilus gateway would report. And sure enough, the introspection query was still returning:

{
    "name": "Foo",
    "possibleTypes": ["Foo"]
}

I'll be able to verify the above tomorrow as I don't have access to my work laptop right now. If I can confirm that having a single remote schema is still reporting the union's possibleType as itself, that's a problem, right?

Thanks for the help!

[edwinmo-splunk]

Hi again 😄

I did the following and was able to reproduce the issue where a union's possibleTypes includes itself (and nothing else):

1. I have only a single remote schema that defines the union type (Foo)
2. When introspecting the nautilus/gateway's schema, the following is returned for the union type in question:

        {
          "description": "",
          "enumValues": [],
          "fields": [],
          "inputFields": [],
          "interfaces": [],
          "kind": "UNION",
          "name": "Foo",
          "possibleTypes": [
            {
              "kind": "UNION",
              "name": "Foo",
              "ofType": null
            }
          ]
        },

3. However, when introspecting the remote schema directly (and this is what I expect nautilus/gateway to report):

        {
          "kind": "UNION",
          "name": "Foo",
          "description": null,
          "fields": null,
          "inputFields": null,
          "interfaces": null,
          "enumValues": null,
          "possibleTypes": [
            {
              "kind": "OBJECT",
              "name": "Baz",
              "ofType": null
            },
            {
              "kind": "OBJECT",
              "name": "Bar",
              "ofType": null
            }
          ]
        },

The impact of this:

• The GraphQL playground is not able to parse the reported schema by nautilus/gateway. It results in a stack overflow trying to define itself infinitely:

Observable.js:59 Uncaught RangeError: Maximum call stack size exceeded
    at E (buildClientSchema.js:196)
    at l (buildClientSchema.js:144)
    at n (buildClientSchema.js:104)
    at t (buildClientSchema.js:93)
    at a (buildClientSchema.js:122)
    at Array.map (<anonymous>)
    at E (buildClientSchema.js:200)
    at l (buildClientSchema.js:144)
    at n (buildClientSchema.js:104)
    at t (buildClientSchema.js:93)

• Mutations/queries depending on this union don't work either:

{
  "data": {},
  "errors": [
    {
      "extensions": {
        "code": ""
      },
      "message": "input:3: Fragment cannot be spread here as objects of type \"Foo\" can never be of type \"Bar\".\n"
    }
  ]
}

[AlecAivazis]

Hey @edwinmo-splunk! I am just now sitting down at my computer for the day if you want to poke at this a bit more. I'll be on the gopher's slack channel 👍

That being said, I think that a type does need to be an implementer of itself, since the following is a valid query:

{
    node(id: "1234") {
        ... on Node {
            ... on Node { # <- this just checks `schema.PossibleTypes` iirc
                id
            }
        }
    }
}

I could be wrong but some memory is telling me that I didn't originally have types implementing themselves but then added it to make a test pass.

[AlecAivazis]

Ah well that does seem pretty conclusive... Alright so as long as we can still support a query like ^ i think you're change makes sense. I hope the non-null values in that response don't cause problems either...

[edwinmo-splunk]

Hey @edwinmo-splunk! I am just now sitting down at my computer for the day if you want to poke at this a bit more. I'll be on the gopher's slack channel 👍

That being said, I think that a type does need to be an implementer of itself, since the following is a valid query:

{
    node(id: "1234") {
        ... on Node {
            ... on Node { # <- this just checks `schema.PossibleTypes` iirc
                id
            }
        }
    }
}

I could be wrong but some memory is telling me that I didn't originally have types implementing themselves but then added it to make a test pass.

Good call on checking that query continues working. Looks like everything's okay:

mutation {
  mutateReturnsFoo(id:"some-id") {
    ... on Foo {
      ... on Foo {
        ... on Bar {
          <fields>
        }
        ... on Baz {
          <fields>
        }
      }
    }
  }
}

The mutation executed successfully.

I hope the non-null values in that response don't cause problems either...

This caught my eye too! This hasn't caused me any issues and has been like this before I made any changes, so I would say it's fine for now. However we might want to be safe than sorry and have them be the zeroth value for each type. My references are gqlgen and apollo's graphql servers.

[AlecAivazis]

Fantastic! Want to open a PR with your changes? I'll be around so we can get it merged pretty quickly

[edwinmo-splunk]

Hey! Sorry, didn't mean to leave you hanging, got caught up in meetings on Friday. I'll put out a PR today/tomorrow. Thanks again for the help, I'll update this issue with that PR when ready. Cheers

[AlecAivazis]

No worries at all! I figured something came up. I’ll keep an eye out for the PR - also if you could include a test that verifies it works appropriately that’s be awesome!

[AlecAivazis]

Closing this since the PR has been merged. Thanks again!