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

Union types are not rebuilt properly from the supergraph, creating potential runtime issues #2256

Closed
pcmanus opened this issue Nov 16, 2022 · 1 comment · Fixed by #2288
Closed
Assignees

Comments

@pcmanus
Copy link
Contributor

pcmanus commented Nov 16, 2022

Union types are merged by composition, but we don't preserve information about which subgraph declares which types of the union in the supergraph (very much an oversight). As the result, the query planner does not know which subgraph declares which type as part of the union, and that can lead to send invalid queries to a subgraph (which are thus rejected at runtime).

Consider the following example:

# subgraph `S1`
type Query {
  u: U
}

union U = A | B

type A @key(fields: "id") {
  id: ID!
  a1: Int
}

type B {
  id: ID!
  b: Int
}

type C  @key(fields: "id") {
  id: ID!
  c1: Int
}
# subgraph `S2` 
type Query {
  otherQuery: U
}

union U = A | C

type A @key(fields: "id") {
  id: ID!
  a2: Int
}

type C @key(fields: "id") {
  id: ID!
  c2: Int
}

The definition of interface U in the supergraph ends up as:

union U = A | B | C

and so the following query is valid (on the supergraph API):

query {
  u {
    ... on C {
      c1
    }
  }
}

However, the query planner currently generate the following fetch:

Fetch(service: "S1") {
  {
    u {
      __typename
     ... on C {
       c1
     }
    }
  }
},

which is not valid GraphQL because for S1, union U does not contains C and so the subgraph would reject the query with an error looking like:

Fragment cannot be spread here as objects of type "U" can never be of type "C".

But the underlying reason this happens is that the composed supergraph does not preserve the information regarding which subgraphs defined which types for union U. As a result, the "view" of the subgraph that the query planner has is that both S1 and S2 declares U as A | B | C, which is incorrect.

@pcmanus
Copy link
Contributor Author

pcmanus commented Nov 16, 2022

Note that fixing this require a bump of the underlying "join spec" used by supergraphs (which is ok, that's why we have versioning in place, but does mean that the supergraph generated by whichever release this goes in won't be readable by older versions).

@pcmanus pcmanus self-assigned this Nov 30, 2022
pcmanus pushed a commit to pcmanus/federation that referenced this issue Dec 14, 2022
While composition allows unions and enums to differ between subgraphs (at
least under certain conditions), the supergraph currently was not
preserving which subgraph defined which union member/enum value, and in
particular the query planner made the (sometimes incorrect) assumption
that unions and enums were defined the same in all the subgraphs in
which they are defined.

As shown in apollographql#2256, this was leading to genuine issues in the case of
unions, where the query planner was generating invalid subgraph queries.
I am not sure this created similar issues for enum values due to a
combination of how the merging rule for enum work and the fact that
values of enum are not types, but it is nonetheless a bad idea to
run the query planner with incorrect information on the subgraph it
generates queries for, and this can get in the way of other toolings
that wants to use the supergraph (for instance, this gets in the
way of apollographql#2250).

To fix this, this patch ensures the supergraph preserve the information
regarding which subgraph defines which union member and enum values in
the supergraph by adding 2 new dedicated "join spec" directives.

Fixes apollographql#2256.
pcmanus pushed a commit that referenced this issue Dec 14, 2022
While composition allows unions and enums to differ between subgraphs (at
least under certain conditions), the supergraph currently was not
preserving which subgraph defined which union member/enum value, and in
particular the query planner made the (sometimes incorrect) assumption
that unions and enums were defined the same in all the subgraphs in
which they are defined.

As shown in #2256, this was leading to genuine issues in the case of
unions, where the query planner was generating invalid subgraph queries.
I am not sure this created similar issues for enum values due to a
combination of how the merging rule for enum work and the fact that
values of enum are not types, but it is nonetheless a bad idea to
run the query planner with incorrect information on the subgraph it
generates queries for, and this can get in the way of other toolings
that wants to use the supergraph (for instance, this gets in the
way of #2250).

To fix this, this patch ensures the supergraph preserve the information
regarding which subgraph defines which union member and enum values in
the supergraph by adding 2 new dedicated "join spec" directives.

Fixes #2256.
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

Successfully merging a pull request may close this issue.

1 participant