graphql / graphql-spec

GraphQL is a query language and execution engine tied to any backend service.
https://spec.graphql.org
14.31k stars 1.13k forks source link

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

Open duckki opened 8 months ago

duckki commented 8 months ago

(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](https://spec.graphql.org/draft/#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:

where

benjie commented 8 months ago

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.)

duckki commented 8 months ago

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").