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

Query Plan produces invalid query for subgraphs when they define a stub for an entity that belongs to a merged union #2307

Closed
mNalon opened this issue Dec 28, 2022 · 6 comments

Comments

@mNalon
Copy link
Contributor

mNalon commented Dec 28, 2022

While federating another part of our graph we got an unexpected behavior for the query plan.

I will try to summarize with an example to ease the points.

Considering subgraph 1 with the following schema:

  type A {
    fieldA: String
  }

  type B @shareable {
    fieldB: String
  }

  type C @key(fields: "fieldC", resolvable: false) {
    fieldC: String!
  } #supposing type C is referenced in other parts of this subgraph

  union Merged = A|B

  type Query {
    unionFromSubgraphOne: Merged
  }

And subgraph 2:

  type B @shareable {
    fieldB: String
  }

  type C @key(fields: "fieldC"){
    fieldC: String
  }

  union Merged = B|C

  type Query {
    unionFromSubgraphTwo: Merged
  }

The composition of a supergraph composed by those two subgraphs works fine.

And when we do the following query through the supergraph:

query {
  unionFromSubgraphTwo {
    ...on A {
      fieldA
    }

    ...on B {
      fieldB
    }

    ...on C {
      fieldC
    }
  }
}

even though subgraph 2 does not declare the type A on the Merged union, this query works correctly (as we are expecting considering the union strategy on the composition).

Although, when we do the following query

query {
  unionFromSubgraphOne {
    ...on A {
      fieldA
    }

    ...on B {
      fieldB
    }

    ...on C {
      fieldC
    }
  }
}

it results in a bad request coming from subgraph 1, saying that “Merged can never be of type C”. What tells us that gateway is calling subgraph 1 with an invalid query.

Reproduced with latest version of rover/router.

Expected behavior: query plan should not be adding … on C on the query sent to subgraph 1.

@pcmanus
Copy link
Contributor

pcmanus commented Jan 23, 2023

Sorry for missing this earlier, but this is the same as #2256 and is due to the supergraph unfortunately not preserving enough informations about unions. It's fixed (by #2288) in the upcoming 2.3 release (which we hope to have out within the next week or 2) but as it requires changing the format of the supergraph slightly, it's hard to backport to earlier release unfortunately.

@pcmanus
Copy link
Contributor

pcmanus commented Jan 24, 2023

Closing as I'm fairly confident that it is fixed by the PR mentioned above, but of course feel free to re-open if you still see issues with this once 2.3 is out.

@pcmanus pcmanus closed this as completed Jan 24, 2023
@tiago154
Copy link

Hello.

I did a test using managed federation 2.3 and I still have the same problem.
I ran the test running the gateway locally, using managed federation and just modifying the host of the subgraph I wanted to test. I used @apollo/gateway@2.4.0.

When I make a query and the requested field is a union, the gateway is sending the request to the subgraph that does not have the declaration of the type in the union. The field should be resolved in another subgraph where the type is declared in the union, but we are getting a location unavailable error.

@mNalon
Copy link
Contributor Author

mNalon commented Apr 10, 2023

Hi, @pcmanus,

sorry for the long break. We only could test the fix right now.

As @tiago154 mentioned, the query planner seemingly still produces invalid queries for this scenario when the managed federation process builds the supergraph. Are you able to reproduce this? Is it worth reopening this issue?

@pcmanus
Copy link
Contributor

pcmanus commented Apr 11, 2023

Hi @tiago154 and @mNalon. Are you saying that you have the same problem with the exact example from the description for this issue? Or is that a slightly different example?

I just tested the example in the description of this issue on current main, and it seems to be working correctly (in the plan for the last query above, the 1st subgraph is not asked for type C). So if you have issues with that very example, something may be wrong with managed federation somehow. If that is case and you can share the supergraph that managed federation gives you for that example, this might help see if that's the issue.

If the problem is with another example or some variation of it, then that modified example would help.

@mNalon
Copy link
Contributor Author

mNalon commented Apr 13, 2023

Thanks for the quick reply @pcmanus. I think I got what was going wrong.

Previously our workaround was adding the type C in the union Merged of subgraph 1. Then to test the fix we removed type C from the union again and run subgraph 1 locally, but still loading in the gateway the supergraph composed by the managed federation before this change on subgraph 1, just pointing the URL for the subgraph 1 to this local version. Since in the previous composition subgraph 1 had type C in the union, the query planner was missending the invalid fragment ...on C to subgraph 1 because the gateway was still thinking type C was in the union defined by this subgraph. Republishing subgraph 1 and recomposing the supergraph everything worked.

sorry for the false alarm!

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

3 participants