Relax the validation of field selection on mutually exclusive types

[oprypkhantc]

Hey.

Currently, per spec, paragraph 5.3.2, two fields of the same name cannot be selected on two different, mutually exclusive types, if field types or arguments differ. E.g. given this schema:

union CatOrDog = Cat | Dog

type Query {
  findPet: CatOrDog!
  findDog: Dog!
}

interface Pet {
  name: String!
}

type Dog implements Pet {
  name: String!
  nickname: String
  barkVolume: Int
  anotherCommonlyNamedField: String
}

type Cat implements Pet {
  name: String!
  nickname: String
  meowVolume: Int
  anotherCommonlyNamedField: String!
}

this will result in an error (Example #125 in the spec):

fragment conflictingDifferingResponses on Pet {
  ... on Dog {
    someValue: nickname
  }
  ... on Cat {
    someValue: meowVolume
  }
}

and so will this:

query {
  findPet {
    __typename
    ... on Dog {
      name
      anotherCommonlyNamedField
    }
    ... on Cat {
      name
      anotherCommonlyNamedField
    }
  }
}

due to anotherCommonlyNamedField field having different return types in Dog and Cat types.

---

It forces usage of aliases when objects are obtained through a union type. E.g. if client's uses a type, based on structure of a GraphQL type Dog, one they can obtain from calling findDog, they would no longer be able to have the same structure if they need to switch to findPet.

For example, let's say a client uses this query:

{
  findDog {
    name
    nickname
    barkVolume
    anotherCommonlyNamedField
  }
}

and they have a type like so, used in multiple places in the project:

type Dog = {
  name: string
  nickname: string | null
  barkVolume: number | null
  anotherCommonlyNamedField: string | null
}

and then Cat and findPet are introduced into the schema, the client needs to support both Cat and Dog, they'd need to switch to the new findPet field:

{
  findPet {
    __typename
    ... on Dog {
      name
      nickname
      barkVolume
      anotherCommonlyNamedField
    }
    ... on Cat {
      name
      nickname
      meowVolume
      anotherCommonlyNamedField
    }
  }
}

which, unexpectedly, doesn't work. Instead, they have to introduce aliases:

{
  findPet {
    __typename
    ... on Dog {
      name
      nickname
      barkVolume
      anotherCommonlyNamedFieldNullable: anotherCommonlyNamedField
    }
    ... on Cat {
      name
      nickname
      meowVolume
      anotherCommonlyNamedFieldNonNullable: anotherCommonlyNamedField
    }
  }
}

which breaks the original Dog interface the client had. Now, they either have to manually convert the type to a different structure (rename anotherCommonlyNamedFieldNullable and anotherCommonlyNamedFieldNonNullable back into anotherCommonlyNamedField), or change it across the entire client code.

This is suboptimal, as it forces the client to do extra work. In either situation, the client must check for the typename through __typename - because only dogs have barkVolume, and only cats have meowVolume. If this check is done either way, why would different types of a shared-name field matter?

In other words, if the client already has to differentiate between different, unrelated, mutually exclusive types, that may have entirely different number of fields and an entirely different structure, why do only fields that intersect between the two have to have a shared type? If response structure is expected to be the same, regardless of the actual type of an object, then shouldn't it also enforce that a Dog must have a meowVolume, and a Cat must have a barkVolume?

Reported elsewhere

This has been reported as a bug/problem in implementations of GraphQL:

• JS: Querying same field w/ and w/o attributes for different union types results in error graphql-js#53
• PHP: OverlappingFieldsCanBeMerged throws an error when querying same field w/ and w/o attributes for different union types webonyx/graphql-php#1500
• Java: Unions too strict on field type checking? graphql-java/graphql-java#1281
• C#: Non-mergeable fields error when using UnionTypes ChilliCream/graphql-platform#2240
• ... probably others

The issues were closed as the spec dictates the specific behaviour a GraphQL server should follow.

Proposal

The proposal is to skip conflict checking for mutually exclusive types altogether. In terms of graphql-js implementation, a patch would look like so:

https://github.com/graphql/graphql-js/blob/v16.11.0/src/validation/rules/OverlappingFieldsCanBeMergedRule.ts#L606

-  if (!areMutuallyExclusive) {
+  if (areMutuallyExclusive} {
+    return null;
+  }

An alternative proposal is to do the same, but only if __typename field was requested.

[benjie]

I think one of the main reasons was that GraphQL aims to be maximally compatible; imagine a type system that doesn't support TypeScript's union or discriminated types (for example, older versions of Java). A query such as

query FindPet {
  findPet {
    __typename
    ... on Dog {
      name
      nickname
      barkVolume
    }
    ... on Cat {
      name
      nickname
      meowVolume
    }
  }
}

might be typed as (TypeScript syntax, but imagine this done in another language):

type FindPetQuery = {
  findPet?: {
    __typename: string;
    name?: string;
    nickname?: Maybe<string>;
    barkVolume?: Maybe<number>;
    meowVolume?: Maybe<number>;
  }
}

(Note: I've made all fields optional other than __typename because if another type, Fish, was added to the union, no fields would be selected on that type.)

Adding anotherCommonlyNamedField could be added via anotherCommonlyNamedField?: Maybe<string>... But as you mention, this would not line up with the expectations of Cat: "non-nullable if present".

This may be less of a concern now than it was 10+ years ago, and is worth revisiting if it solves a significant problem.

Related issues:

• union does not allow sub-union or covariance #711
• Improve support for covariance in interfaces with field selection merging #804

[oprypkhantc]

might be typed as (TypeScript syntax, but imagine this done in another language):

I see, but a language like Java would also use a proper deserialization, which usually allows deserializing polymorphic types either out-of-the-box or with a plugin. Three of the most popular serialization libraries support it:

• GSON (1st party): https://github.com/google/gson/blob/dd2fe59c0d3390b2ad3dd365ed6938a5c15844cb/extras/src/test/java/com/google/gson/typeadapters/RuntimeTypeAdapterFactoryTest.java#L31
• Jackson (1st party): https://github.com/FasterXML/jackson-annotations#handling-polymorphic-types
• Moshi (3rd party): https://github.com/onenowy/moshi-polymorphic-adapter

I understand that you only mentioned Java as an example, but I believe such argument can be also made against other languages that seemingly wouldn't support such unions. Typing a union as a combination/merge of it's types is a pretty bad idea from type safety perspective, so GraphQL enforcing such an idea with a validation rule seems off - given how much GraphQL is based around type safety :)

GraphQL enforcing this in of itself wouldn't be a problem for me, but such validation also makes it much harder to work with unions as one would expect with modern languages (typing a union as multiple separate types).

is worth revisiting if it solves a significant problem.

I'm very much an outsider and have very limited experience with GraphQL. Does the proposed solution sound reasonable to you at all? Does requiring __typename sound reasonable? I can draft an RFC if it does.

[martinbonnin]

+1 to relaxing that validation.

As someone with a Java/Kotlin background, this is something that got me puzled for a while. I think most codegen out there have the capability to generate interfaces and/or unions?

Would be interesting to know what graphql-code-generator is doing. @eddeee888, do you know if relaxing that limitation would be an issue?

[oprypkhantc]

Well, at least the latest version of graphql-codegen generates normal unions in TypeScript:

export type UnionUserLead = Lead | User;

[eddeee888]

Thanks for tagging me @martinbonnin !

do you know if relaxing that limitation would be an issue?

It would be no issue for the base TypeScript document plugins because __typename is used as discriminator for the union.

For example, if Cat.name is nullable and Dog.name is non-nullable, then the generated type would look something like this:

export type TestQuery = {
  __typename?: "Query";
  findPet:
    | { __typename: "Cat"; name?: string | null }
    | { __typename: "Dog"; name: string };
};