Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
duckki opened this issue Mar 18, 2024 · 2 comments

Comments

@duckki
Copy link

duckki commented Mar 18, 2024

(This idea was inspired by apollographql/federation#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.
@benjie
Copy link
Member

benjie commented Mar 19, 2024

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
Copy link
Author

duckki commented Mar 19, 2024

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

#367 seems remotely related, but a different rule ("Fragment Spread Is Possible").

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants