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 explosion issue with Fed1 supergraph/Fed2 query planner #1988

Closed
lennyburdette opened this issue Jul 16, 2022 · 1 comment · Fixed by #1994
Closed

Interface explosion issue with Fed1 supergraph/Fed2 query planner #1988

lennyburdette opened this issue Jul 16, 2022 · 1 comment · Fixed by #1994
Assignees

Comments

@lennyburdette
Copy link
Contributor

Reproduction: https://stackblitz.com/edit/basic-federation-xfmlkb?file=fed1-supergraph.graphql,README.md

The customer is trying to upgrade to Gateway 2.0 with their existing fed 1 supergraph. The query planner is improperly exploding the interface to include conditional fragments for all implementations of the interface. (In the original schemas, there are dozens of possible types so the query plan is enormous.)

QueryPlan {
  Fetch(service: "customer-name") {
    {
      profile(uuid: $profile_id) {
        suggestions(sequential: false) {
          nodes {
            suggestedObject {
              __typename
              ... on NewsfeedCard_FeaturedComment {
                id
              }
              ... on Profile {
                id
              }
              ... on Profiles_Suggestion {
                id
              }
            }
          }
        }
      }
    }
  },
}

This causes a runtime error because the subgraph doesn't include all the implementations. I included a second subgraph to show that runtime failure, but the query planner is generating unnecessary conditional fragments even without the second subgraph.

Everything works with a Fed 2 supergraph — the query plan includes no conditional fragments.

pcmanus pushed a commit to pcmanus/federation that referenced this issue Jul 19, 2022
We usually avoid type-exploding the fetching of interface fields when we
can avoid to, but we were avoiding this optimization where the input
was a fed1-generated supergraph (essentially always type-exploding all
interfaces).

The initial reason for that avoidance was a combination of 2 things:
1. going too fast over this, I though this was what the fed1 query
   planning was doing, which is actually not 100% accurate.
2. fed1-generated supergraph have incomplete information when it
  goes to interface (really, all non-entity types), in that it
  does not record when those interfaces/values types are actually
  defined. This lead to known issues (in both fed1 and
  fed2-with-fed1-supergraph) and I took this as additional motivation
  to avoid relying on interface as much as possible (which type
  explosion kind of achieve).

However, regarding the 1st point, fed1 happens to avoid type explosion
when all the implementations of an interface are a value type, so not
doing so is a sort of regression. And while we could mimick that fed1
behaviour, upon further reflexion, the rule we use for fed2 for this
(which only type-explode specific fields where one of the implementation
is either external or has as `@requires`) is actually fine with fed1
supergraphs. More precisely, the issues due to the incompletness of
fed1 supergraph could be a problem with that rule _if_ an interface
had different fieds defined in different subgraphs, but that's actually
not allowed by fed1.

Fixes apollographql#1988.
@pcmanus pcmanus self-assigned this Jul 19, 2022
@pcmanus pcmanus linked a pull request Jul 19, 2022 that will close this issue
@pcmanus
Copy link
Contributor

pcmanus commented Jul 19, 2022

So yes, fed2 with fed1 supergraphs does type-explosion of interfaces in some cases where the fed1 query planner wasn't and that's an oversight (which gets in the way of upgrades).

That is, fed1 always type-explode an interface if any of the implementation is an entity type, but doesn't if all said implementations are value types (the case in this example). But current fed2 with fed1 supergraph currently always type-explode instead.

Anyway, while we can mimick that fed1 behaviour in fed2-with-fed1-supergraphs, attached a PR that instead just remove the special we were making for fed1 supergraph, thus making it behave like fed2 with fed2 supergraphs. Because upon reflexion, I believe that's actually fine, and the fed2 rule (which consider type-explosion separately for each field, and only force type-explosion if ) is a bit "better" (it type-explode less often, a good thing) than the fed1 one.

The query planner is improperly exploding the interface to include conditional fragments for all implementations of the interface.

For the sake of knowledge dissemination, I'll point out that while we can avoid exploding into types here (and this does provides a way out of this particular example), the fact that when it explodes, it (incorrectly) explodes into all implementations is not something we can really fix. Because if you look at the supergraph (which is the only input of the query planner), there is actually no information about where (in which subgraphs) the interface and implementation types are defined.

This is why fed2 "upgraded" supergraphs (to include that kind of information) and it does lead to cases where both fed1 and fed2-with-fed1-supergraphs query planners are generating invalid plans (that fail at runtime). For instance, in the example of this issue, if you simply add a @key to the NewsfeedCard_FeaturedComment type in the 2nd subgraph (without changing the 1st one at all), you'll see that the plan generated by fed1 does type explosion and is broken. Obviously, since fed1 fails on such cases in the first place, users won't have relied on such example in fed1 and it's not an issue for upgrades (just a limitation of fed1 that is fixed by upgrading to fed2 and recomposing into a fed2 supergraph), but just wanted to point out that there is known limitations due to fed1 supergraph having incomplete information.

benweatherman pushed a commit that referenced this issue Jul 19, 2022
We usually avoid type-exploding the fetching of interface fields when we
can avoid to, but we were avoiding this optimization where the input
was a fed1-generated supergraph (essentially always type-exploding all
interfaces).

The initial reason for that avoidance was a combination of 2 things:
1. going too fast over this, I though this was what the fed1 query
   planning was doing, which is actually not 100% accurate.
2. fed1-generated supergraph have incomplete information when it
  goes to interface (really, all non-entity types), in that it
  does not record when those interfaces/values types are actually
  defined. This lead to known issues (in both fed1 and
  fed2-with-fed1-supergraph) and I took this as additional motivation
  to avoid relying on interface as much as possible (which type
  explosion kind of achieve).

However, regarding the 1st point, fed1 happens to avoid type explosion
when all the implementations of an interface are a value type, so not
doing so is a sort of regression. And while we could mimick that fed1
behaviour, upon further reflexion, the rule we use for fed2 for this
(which only type-explode specific fields where one of the implementation
is either external or has as `@requires`) is actually fine with fed1
supergraphs. More precisely, the issues due to the incompletness of
fed1 supergraph could be a problem with that rule _if_ an interface
had different fieds defined in different subgraphs, but that's actually
not allowed by fed1.

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

2 participants