allow unions to declare implementation of interfaces

[yaacovCR]

Cf. #373 (comment)

# generic types
interface Node {
  id: ID!
}

interface Connection {
  pageInfo: PageInfo!
  edges: [Edge]
}

interface Edge {
  cursor: String
  node: Node
}

type PageInfo {
  hasPreviousPage: Boolean
  hasNextPage: Boolean
  startCursor: String
  endCursor: String
}

# schema-specific types
interface Pet {
  id: ID!
  name: String
}

type Cat implements Pet & Node {
  id: ID!
  name: String
}

type Dog implements Pet & Node {
  id: ID!
  name: String
}

union HousePet implements Pet & Node = Cat | Dog

# house-pet-specific types
type HousePetEdge implements Edge {
  cursor: String
  node: HousePet  # <<< This is what is unlocked by this PR
}

type HousePetConnection implements Connection {
  pageInfo: PageInfo!
  edges: [HousePetEdge]
}

# query
type Query {
  housePets: HousePetConnection
}

[netlify]

✅ Deploy Preview for graphql-spec-draft ready!

Name	Link
🔨 Latest commit	dfb9222
🔍 Latest deploy log	https://app.netlify.com/sites/graphql-spec-draft/deploys/628bf8ceeb9e8d0008161744
😎 Deploy Preview	https://deploy-preview-939--graphql-spec-draft.netlify.app/draft
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

---

To edit notification comments on pull requests, go to your Netlify site settings.

[yaacovCR]

see: #518

[AnthonyMDev]

I am really excited for this to become part of the spec. Thanks so much for getting the work done here! This looks great so far. Interested to see what the working group has to say about it!

[yaacovCR]

@AnthonyMDev thanks for the comments! I think you are right on. I also have some changes coming in to the validation edits to reflect how the PR came together.

namely, when validating interface use with unions, we can check simply whether member types explicitly declare that they implement those interfaces and stipulate that the interface implementation for the member types has been validated

[yaacovCR]

Comments from WG:

Unions don't implement interfaces, their member types do. This keyword may be helpful to create a contract by which the union declares that its member types all implement an interface so that queries can be reliably written to rely on that fact. Otherwise, new member types could be added that don't implement the interface and it would be unfortunate if adding a new member type would break existing queries in that way.

Explorations:

1. How do other type systems handle a constraint of this kind?
2. Might we consider using a keyword other than "implements" to indicate this constraint?
3. Should queries be allowed to select fields directly on the union now that the union definitely implements the field?

There were some interesting views on the third point.

Some exploration on the third point within the reference implementation yielded that queries of this type would execute anyway without a problem, but fail validation because of the FieldsOnCorrectTypeRule.

[yaacovCR]

In terms of the first point, I imagine we could think along the lines of a new intersection type that would operate on Union and Interfaces types. It would contain only the object types in the unions but only if they implement all of the interfaces.

Aside from preferred syntax concerns, what would the practical difference be?

I can't think of one (yet). Feedback welcome!

[yaacovCR]

@leebyron does that feel like the correct precedent from other type systems?

[benjie]

Specific keyword aside, imagine you have the schema:

interface Foo {
  foo: Int
}
interface Bar {
  bar: Int
}
type A implements Foo & Bar {
  foo: Int
  bar: Int
  a: Int
}
type B implements Foo & Bar {
  foo: Int
  bar: Int
  b: Int
}
type C implements Foo & Bar {
  foo: Int
  bar: Int
  c: Int
}
union AOrB implements Foo & Bar = A | B

type Query {
  q: AOrB
}

I think we're all agreed during the WG call that the following query should be valid:

{
  q {
    foo
    bar
    ... on A { a }
    ... on B { b }
  }
}

For completeness, I think the following query should not be valid:

{
  q {
    foo
    bar
    ... on A { a }
    ... on B { b }
    ... on C { c }
  }
}

But the following should be valid:

{
  q {
    foo
    bar
    ... on A { a }
    ... on B { b }
    ... on Foo {
      ... on C { c }
    }
  }
}

I think to implement this we need:

1. the syntax changes to GraphQL to allow for this (already done!)
2. the schema validation changes to allow for this (already done!)
3. the request validation changes to allow for this (not yet done)

[yaacovCR]

I think the latter is also done now but I may have missed a case, will have to double check, if you can point to more areas to update, of course that would be great.

[yaacovCR]

But for completeness, I think the question is whether you should be able to do ... on AorB

[rivantsov]

@benjie
I do not quite get it, your sample, the last query with Foo.
The situation is impossible, we never find object C there. And I think deep enough and sophisticated query analysis can find this out (if implementor chooses to code it), and therefore it can declare that the query is NOT valid.
So what I am saying, we should not insist that this query 'should be valid'

[benjie]

@rivantsov It should be valid because it is currently valid on the underlying union (ignoring the new implements keyword) and making the two behave differently just because one has interfaces defined seems a) undesirable due to inconsistency, and b) actively harmful to schema evolution.

[yaacovCR]

In terms of the first point, I imagine we could think along the lines of a new intersection type that would operate on Union and Interfaces types. It would contain only the object types in the unions but only if they implement all of the interfaces.

Aside from preferred syntax concerns, what would the practical difference be?

I can't think of one (yet). Feedback welcome!

See graphql/graphql-js#3550 which introduces a new GraphQLIntersectionType!

The practical difference is that intersections are more powerful, especially if we follow this up with the expansion of Unions to include abstract types such as other unions, intersections, interfaces...

[yaacovCR]

spec PR: #941
discussion @ graphql/graphql-wg#944
implementation PR: graphql/graphql-js#3550

The main practical difference after further reflection seems to be how to handle adding a new type to the union that does not implement the interface. Using intersections, it's not a problem. Using unions that implement interfaces, you are kind of stuck.

[rivantsov]

The main practical difference after further reflection seems to be how to handle adding a new type to the union that does not implement the interface. Using intersections, it's not a problem. Using unions that implement interfaces, you are kind of stuck.

I don't get it - 'you are kind of stuck'. What is the problem of adding a type to union, when type does not implement interface? - this is a clear fault. That's a violation of the promise (union implements interface means all concrete types implement it), so it is an error, at init time should be rejected by schema validation code. That's the whole point! I do not see it as a problem.
Just like when you add a field to interface, and not to implementing type - it would and should be rejected

[michaelstaib]

@rivantsov It's about flexibility. We the intersection type you can combine types more flexibly and schema evolvability.

@yaacovCR I think we should draw up some real use cases that show the problem we are trying to solve. Maybe we even can have a look at a couple of approaches to solving these use cases in an RFC document like we did for input unions.

[yaacovCR]

Please see discussion linked above. It has the real world use case with unions implementing node linked above. I think if we think of a third solution it makes sense to have a separate RFC doc, but right now we are just comparing the two.

We could always add an additional option to avoid the use of unions in favor of interfaces, but that does not seem truly on point to me. Seeing as unions exist, they could exist as fields on types that implement an interface...

[yaacovCR]

Decision Record We decided at the last WG that if unions implement interfaces, then they have directly querable fields. This PR and the implementation PR have been updated with the changes for introspection, validation.

[yaacovCR]

Note that we currently avoid direct validation that the implementations declared by the union do not conflict, i.e., the following will yield a validation error for the empty SomeUnion error but not trigger an independent validation error for the union:

union SomeUnion implements SomeInterface & SomeClashingInterface

interface SomeInterface {
  commonField: String
}

interface SomeClashingInterface {
  commonField: ID
}

Once an object is placed in the union, it won't be able to fulfill both interfaces without an error. That works for me, but I could anticipate feedback that we should have independent validation, so if that's the view, I could try to implement that prior to the next WG.

[yaacovCR]

Any independent rule would have to note that the following is valid:

union SomeUnion implements SomeInterface & AnotherInterface

interface SomeInterface {
  commonField: String
}

interface AnotherInterface {
  commonField: String!
}

[yaacovCR]

My first pass of the algorithm for implementing the independent validation rule would be something like:

1. let fieldDefsMap be a map of field names to lists of field definitions
2. for each interface declared as implemented by the union
   1. for each field defined by the interface
      1. add the definition for field to the list of field definitions for fieldName within fieldDefsMap
3. for each entry in fieldNameMap
   1. let fieldDefinitions be the list of field definitions for field with name fieldName
   2. let initialFieldListDepth be the list depth of initialFieldDef of the first field definition
   3. if the list depth of any of the remaining field definition differs from initialFieldListDepth, report an error (and continue to the next field?)
   4. let initialNamedType be the underlying named type for the first field definition in fieldDefinitions
   5. let the set remainingPossibleSubTypes be populated by initialNamedType in addition to the result of executing the getImplementations algorithm on initialNamedType`
   6. for each remaining namedType in namedTypes
      1. let the set possibleSubTypes be populated by namedType in addition to the result of executing the getImplementations algorithm on namedType`
      2. for each type in remainingPossibleSubTypes, remove the type from remainingPossibleSubTypes if it is not within `possibleSubtypes
      3. break if remainingPossibleSubTypes is empty
   7. if remainingPossibleSubTypes is empty, report an error

[yaacovCR]

Two additional thoughts:

1. I realized that objects and interfaces could also have a separate rule of this kind, but I don't believe they do, so I will pass.
2. While working on the implementation for automatically giving "fields" to unions that implement interfaces, I've come across an edge case that confirms (for me) that this is not a good idea. TLDR I think the breaking change surface area should be as narrow as possible, so I don't think we should allow automatic assignment of fields to union that implement interfaces. But opinions may differ! Let's discuss the below edge case and its implications at the next WG.

Consider the following initial types:

interface Pet {
  name: String
  friends: [Pet]
}

type Cat implements Pet {
  name: String
  friends: [Pet]
}

type Dog implements Pet {
  name: String
  friends: [Pet]
}

union CatOrDog implements Pet = Cat | Dog

query {
  catOrDogs: [CatOrDog]
}

The friends field automatically queriable on the union itself would presumably be of type Pet, so the following would be possible:

{
  catOrDogs: {
    name
    friends {
      name
    }
  }
}

But what happens if we add another interface to the schema:

interface Named {
  name: String
}

interface Friendly {
  friends: [Friendly]
  greeting: String 
}

extend interface Pet implements Named & Friendly {
  greeting: String
}
extend type Cat implements Named & Friendly {
  greeting: String
}
extend type Dog implements Named & Friendly {
  greeting: String
}
extend type CatOrDog implements Named & Friendly

And it all works! Because the type signature of CatOrDog is essentially equivalent to the following pseudo-interface:

union CatOrDog implements Pet & Named & Friendly = Cat | Dog : {
  name: String
  friends: [Pet]
}

But what if we add an interface with an overlapping field name with a slightly different type?

interface FriendlyWithoutGreeting {
  friends: [FriendlyWithoutGreeting]
}

extend interface Friendly implements FriendlyWithoutGreeting
extend interface Pet implements FriendlyWithoutGreeting
extend type Cat implements FriendlyWithoutGreeting
extend type Dog implements FriendlyWithoutGreeting
extend type CatOrDog implements FriendlyWithoutGreeting

But what is the field type for friends when querying directly on CatOrDog is it of Friendly or FriendlyWithoutGreeting. Well, it must be of FriendlyWithoutGreeting, because that's narrower. Which means adding interface FriendlyWithoutGreeting to CatOrDog means that you can no longer query greeting directly on CatOrDog. Adding an interface to an object or interface is NOT a breaking change, but adding one to a union, because of the "automatic" assignment of fields, IS a breaking change.

I think the breaking change surface area should be as narrow as possible, so I don't think we should allow automatic assignment of fields to union that implement interfaces. But opinions may differ! Let's discuss at the next WG

[rivantsov]

@yaacovCR - I do not understand anything. In my understanding:
as soon as two same-name fields but of different types (your 'friendly') from different interfaces collide in a single type (object or interface) - it is an error, invalid schema, everything stops. So in your examples, I see the schema is invalid, before we even get to union.
"Adding an interface to an object or interface is NOT a breaking change, but adding one to a union, because of the "automatic" assignment of fields, IS a breaking change." - I do not understand it at all. An idiot schema designer is a Breaking Change maker, not GraphQL spec or 'abstract schema'. Do not try to stop idiots thru spec, you won't.

[yaacovCR]

The same name fields are of different but overlapping types, so I do not think they collide except when trying to automatically assign fields to the Union.

Similar to the now defunct intersection idea, "automatic" things in graphql cause problems.

[benjie]

@yaacovCR Sorry, I got a little lost trying to follow your argument, please can you write just the final full schema (i.e. without the use of the extend keyword) that you envision that causes this contradiction/breaking change for the union but not other parts of the schema?

If you have union U implements I1 & I2 = A | B then, by your definition, every type in U implements the interfaces I1 and I2. Therefore you can query those fields on any A or B in U by the current GraphQL spec - it does not matter if A and B also implement other interfaces, I1 and I2 must still be honoured. And since you can query them on each individual type in the union, there should be no confusion/conflict querying those fields on U directly?

[yaacovCR]

Well, it must be of FriendlyWithoutGreeting, because that's narrower.

I guess this is where I messed up, it's actually of type "Friendly & FriendlyWithoutGreeting," so everything should be ok?

I guess where I'm getting confused is defining an algorithm for getFields() on unions.
Objects and interfaces specifically define field that explicitly satisfy all of the interfaces that those objects and interfaces implement. But for unions, there could be a number of possible types that satisfy all of those interfaces. Does there need to be a specific algorithm determined by the spec for how getFields() works?

Well, where is getFields() used on unions within the implementation? Pretty much only in the validate FieldsOnCorrectType` rule, so exploration of complex edge cases there might be helpful.

It's possibly that sharper minds than mine might be required for this. :(

[benjie]

If Cat implements both FriendlyWithoutGreeting and Friendly (as in your example) then it is valid to query ... on Cat { greeting friends { greeting } }

Well, it must be of FriendlyWithoutGreeting, because that's narrower.

I don't believe that statement is correct.

In your final schema you have:

union CatOrDog implements Pet & Named & Friendly & FriendlyWithoutGreeting = Cat | Dog

In the end CatOrDog conforms to the final Pet interface, without the use of extend keyword and without "implicit" fields (note that GraphQL does not allow implicit fields on interfaces - you must state them explicitly):

interface Pet implements Named & Friendly & FriendlyWithoutGreeting {
  name: String
  friends: [Friendly]
  greeting: String 
}

I see no contradiction between CatOrDog and Pet at this point. There is no breaking change.

[yaacovCR]

Thank you so much @rivantsov @benjie for your feedback.

I have tried to throw additional edge cases at this, and have failed to create a breaking change problem, although there is still an implementation difficulty I am hitting in terms of how to calculate the introspection results:

For the following schema (and note that the important part is the union toward the bottom):

      interface SomeInterface {
        id: ID
        children: [SomeInterface]
        someField: String
      }

      interface AnotherInterface {
        id: ID
        children: [AnotherInterface]
        anotherField: String
      }

      interface CommonInterface implements SomeInterface & AnotherInterface {
        id: ID
        children: [CommonInterface]
        someField: String
        anotherField: String
      } 

      type SomeType implements CommonInterface & SomeInterface & AnotherInterface {
        id: ID
        children: [CommonInterface]
        someField: String
        anotherField: String
        someUniqueField: String
      }
      
      type AnotherType implements CommonInterface & SomeInterface & AnotherInterface {
        id: ID
        children: [CommonInterface]
        someField: String
        anotherField: String
        anotherUniqueFIeld: String
      }
      
      # The below is the important part
      union SomeUnion implements CommonInterface & SomeInterface & AnotherInterface = SomeType | AnotherType
      
      type Query {
        rootField: [SomeUnion]
      }

The fields directly available on the union, because of CommonInterface are of shape follows:

{
  rootField {
    id
    children {
      id
      children {
        ...
      }
      someField
      anotherField
    }
    someField
    anotherField
  }
}

meaning, the available result shape of the union is equivalent to the following interface that defines no new fields:

interface InterfaceEquivalentForUnion implements CommonInterface & SomeInterface & AnotherInterface {
  id: ID
  children: CommonInterface
  someField: String
  anotherField: String
}

If Unions have fields, then introspection must return those fields, i.e. the fields of the equivalent interface. So how can the implementation determine that algorithmically? Some static analysis can determine that CommonInterface is the controlling interface, perhaps by looking at the set of interfaces, and/or using the object member types as hints. But it does not seem to me to be trivial to write....

Options:

(1) Anonymous interfaces

One way might be to introduce the idea of an "anonymous interface". We can then rewrite the equivalent interface as follows, by following a simple algorithm where we collect all fields from the interfaces that the union implements, and where there is an overlap, introduce the intersection, as follows

interface FieldsForUnionInterspectionResult {
  id: ID
  children: CommonInterface & SomeInterface & AnotherInterface
  someField: String
  anotherField: String
}

If there was an anonymous interface type allowed, then introspection could just return the above directly....

(2) Static analysis?

Perhaps the implementation could generate the anonymous interface above, and then search for a type that fulfills that interface, which does and I guess must (?) exist, and in this case, of course, is CommonInterface itself, as it is controlling.

How can we get from CommonInterface & SomeInterface & AnotherInterface to CommonInterface? In this case, pretty easily, as parent interfaces are implied transitively by child interfaces, and only included within the definition for the schema reader. Once we subtract parent interfaces, CommonInterface is the only interface left.

A more general algorithm might be that if any two types on this list are subtypes of one another, pick the subtype?

I am not sure how to show/prove that these algorithms work.

(3) Not (yet?) allow fields to be queried directly on unions

Meaning, my primary use case was to enable unions fields to fulfill an interface field, avoiding a wrapping fragment could be a separate feature that we could work on later.

[benjie]

Question: does the introspection have to give fields to the union? I can certainly see the value in doing so, but it also seems an amount of redundant data since the interfaces already include those fields and the union is not adding any additional information to them - in particular the union does not declare those fields in SDL, so adding them to introspection would break the symmetry between introspection and SDL. Note this is not the same situation as interfaces and a type declaring the same fields, because in that case the type and interface fields may differ subtly so long as they're compatible; for a union implementing interfaces the fields are from the interfaces itself - the union doesn't really "have" fields.

I think the topic of adding fields to the introspection for unions is independent from the topic of resolving those fields during execution validation, though I can certainly see how adding the former would make the latter easier. I personally don't think that unions should have fields in introspection or SDL even with this "constraint" added to the schema. (They should, however, be queryable directly on the union selection set - like __typename is.)

[yaacovCR]

Thanks for that awesome point!

I guess how we want introspection to work is a good discussion for the working group. I thought giving unions fields would be across the board, but I see your point!

By the way, spec says that for object types, fields is the "available" fields, while for interfaces it is the fields "required" by the interface. So there is already some ambiguity there, tending perhaps in your direction, if could have used "available" for both?

As an aside, having the fields actually defined on the Union helps with validation, not execution. getFieldDef in execute is only passed the concrete type.

Even if we decide that we don't want to return fields for unions during introspection, I am clearly having trouble with the implementation for validation that needs these pseudo fields. Please anyone feel free to reach out with any tips, or post to implementation PR

[martinbonnin]

Linking the ImplicitInheritance RFC here as it seems (loosely at least?) related to this issue?

If:

1. GraphQL allowed for implicit inheritance
2. Introspection only returns each type "own" fields
3. Every tool is modified so that they can infer what fields are available on any given type based on their implemented interfaces (the algorithm you mentioned above)

Then sounds like that would solve automatically the problem of unions implementing interfaces and not declaring any fields?

Of course that's a huge change so not sure how doable that is (certainly not too much) but feels like something that's somewhat related

[yaacovCR]

Very relevant. Basically, I did not solve this part:

If a type inherits multiple interfaces that declare a field with different but compatible types, the resulting type is not immediately clear.

[AnthonyMDev]

What is the status on this @yaacovCR? I know there were a few unresolved questions left. I haven't seen any action on this in a while though. Are you still working on getting this ready to be approved? Are there blockers, that need to be unblocked? Or has this taken a back seat for now?

I'd really love to see us move forward on getting this into the working draft of the spec, but I understand the time and effort needed for that is significant. I'm not in a huge rush for this to happen ASAP, but I would be disappointed to see it never completed. If there is anything that I can do to help move this forward, please let me know!

[yaacovCR]

What is the status on this @yaacovCR? I know there were a few unresolved questions left.

The basic unresolved question is whether and how unions that are constrained so that their member types all implement an interface will allow querying of those interface fields directly on the union. This is straightforward in my canonical case of a union where all members implement the Node interface and so an id field should be queriable on the union, but is not as straightforward in all cases. In particular, if a union is constrained by multiple interfaces that have fields with non-identical, but compatible types, the types of the fields are ambiguous. (This concern also affects implicit interface field inheritance.)

I haven't seen any action on this in a while though.

Yes, I kind of hit a wall. We could try just not allowing that additional client-facing feature and just introducing the constraint. From what I recall, you were actually more interested in that client feature. I am not sure why that is so helpful to you => I wonder if what you are really looking for is for unions to be allowed to include interfaces, which seems like a more useful feature from the aspect that I recall you discussing, which is the annoyance of adding new types to unions. It seems like if interfaces could be members of unions, it may be somewhat common to add new implementations of interfaces that are already in the union, rather than a new union. Although maybe you could reiterate here your more direct use case.

Are you still working on getting this ready to be approved?

Kind of! I moved on to exploring other areas of bolstering subtyping in GraphQL. You can follow my stumbling efforts in a few riveting portions of recent GraphQL WG meetings. Inspired by your prompt here, I have summarized my research/exploration here: https://github.com/graphql/graphql-wg/blob/main/rfcs/ExpandingSubtyping.md -- please feel free to suggest changes/additions to that RFC if you are so inclined, especially in terms of motivations.

Are there blockers, that need to be unblocked? Or has this taken a back seat for now?

I touched on the main blocker for unions declaring implementation of interfaces above. The blockers for unions including other abstract types was the WG's general inclination that the constraint should be listed on the child union, while my contention that we should allow abstract types to be actual members of the union; i.e. a more major change to GraphQL than the simple addition of a constraint. We also discussed whether there would be any problems with (too much?) recursiveness during schema construction; on further reflection, this seems to be a larger problem with unions that contain unions rather than unions that contain interfaces; the latter seems less problematic in terms of recursiveness, as well as more useful!

I'd really love to see us move forward on getting this into the working draft of the spec, but I understand the time and effort needed for that is significant. I'm not in a huge rush for this to happen ASAP, but I would be disappointed to see it never completed. If there is anything that I can do to help move this forward, please let me know!

I don't think I made enough progress since last meeting to really discuss this in depth at tomorrow's meeting, but I can point to the new RFC and ask for feedback. Your feedback, especially your use cases, would definitely be appreciated.