graphql-spec icon indicating copy to clipboard operation
graphql-spec copied to clipboard

Interface conditions fragments may not be used across spreads, discouraging reusability

Open duckki opened this issue 1 year ago • 2 comments

(This idea was inspired by https://github.com/apollographql/federation/issues/2952 .)

Motivating example

Schema:

type A {
    f(arg:String!): String!
}

interface I {
    id: ID!
    f(arg:String!): String!
}

type B implements I {
    id: ID!
    f(arg:String!): String!
}

type C implements I {
    id: ID!
    f(arg:String!): String!
}

union U = A | B | C

type Query {
    get: U!
}

Accepted query:

query Ok {
    get {
        ...on A {
            f(arg: "a")
        }
        ...on B {
            f(arg: "b") # no problem
        }
        ...on C {
            f(arg: "b") # no problem
        }
    }
}

Rejected query:

# A fragment that works on any types implementing I.
fragment genericFragment on I {
    f(arg: "b")
}

query ValidationRejected {
    get {
        ...on A {
            f(arg: "a")
        }
        ...on B {
            ...genericFragment # rejected
        }
        ...on C {
            ...genericFragment # rejected
        }
    }
}

This query is rejected by GraphQL.js's validator, complaining that f(arg: "a") and f(arg: "b") are conflicting.

This is disappointing because it discourages using interface-conditioned fragments like genericFragment above under a more derived type context (like ...on B).

Potential improvement in FieldsInSetCanMerge(set)

The current FieldsInSetCanMerge rule only looks at the (immediate) parent type of fields when checking if they can merge.

  • If the parent types of {fieldA} and {fieldB} are equal or if either is not an Object Type: ...

I propose to change FieldsInSetCanMerge rule to check:

  • If the context types of fieldA and fieldB has non-empty intersection:

where

  • The context type is defined as the most derived type deduced for the current context. This is new, but it won't be difficult to implement in validators.
  • The intersection of two types A and B means the intersection of GetPossibleTypes(A) and GetPossibleTypes(B) as in the Fragment Spread Is Possible section of the spec.

duckki avatar Mar 18 '24 21:03 duckki

One thing to consider here is if we lift this restriction then adding implements I to type A would be a breaking change, since a previously valid query:

query ValidationRejected {
    get {
        ...on A {
            f(arg: "a")
        }
        ...on I {
            f(arg: "b")
        }
    }
}

would now be invalid.

(Not saying this is a blocker, just one thing to keep in mind when considering trade-offs.)

benjie avatar Mar 19 '24 13:03 benjie

Arguably, this is a weird corner case, where union and interface types are mixed on the same type.

https://github.com/graphql/graphql-spec/issues/367 seems remotely related, but a different rule ("Fragment Spread Is Possible").

duckki avatar Mar 19 '24 22:03 duckki