search

apollographql / federation

🌐  Build and scale a single data graph across multiple services with Apollo's federation gateway.
https://apollographql.com/docs/federation/
Other
668 stars 254 forks source link

Fragment variable definitions erased in subgraph queries #3119

Closed clenfest closed 3 months ago

clenfest commented 3 months ago

Fixes #3112

Before creating an Operation, we call collectUsedVariables, which will only pull values from the selection set and not from fragments. This isn't great, because a variable used in a fragment won't be collected, and it doesn't make sense to collect variables from fragments because it's before they are optimized and many will be unused.

The inelegant solution I came up with is to pass in available variables in calls to optimize or generateQueryFragments for an operation where we can add back in the unused variables. This should be ok, because we are guaranteed that exactly one of them will get called by toPlanNode. Pretty sure there won't be too much overhead added because we'll only call this once per subgraph fetch.

changeset-bot[bot] commented 3 months ago

🦋 Changeset detected

Latest commit: c99cfcc904c6b89aae1ac1787222f6f786b4e610

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 7 packages | Name | Type | | --------------------------------------- | ----- | | @apollo/query-planner | Patch | | @apollo/federation-internals | Patch | | @apollo/gateway | Patch | | @apollo/composition | Patch | | @apollo/query-graphs | Patch | | @apollo/subgraph | Patch | | apollo-federation-integration-testsuite | Patch |

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

netlify[bot] commented 3 months ago

Deploy Preview for apollo-federation-docs canceled.

Name Link
Latest commit 6d0a5d0fbda3ca71a166dd6f86f960c663efbc2c
Latest deploy log https://app.netlify.com/sites/apollo-federation-docs/deploys/66be106b5d821b0008d7ce88
codesandbox-ci[bot] commented 3 months ago

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

duckki commented 3 months ago

Rust QP went a different way by not using fragments with unexpected variables in them, instead of including added variables in the query (@goto-bus-stop). I hope Rust QP and JS QP go the same direction at least for now.

See this commit: https://github.com/apollographql/router/pull/5730/commits/8c8340d684e15969eecccadf23f9a2cae566b684

goto-bus-stop commented 3 months ago

Yeah. I didn't realise this was the same case. Optimizing can introduce a new variable reference, but only inside a dead branch (otherwise, the variable reference would already be in the expanded selection set). So there's three correct ways to go:

  1. Removing dead branches when reusing a fragment
  2. Ignoring fragments that introduce new variable dependencies
  3. Counting variable references after optimizing

My preference was 2), because 1) is hard and 3) is adding a "dead" variable to the subgraph query. That said, a fix is a fix, and this is not common enough that it will be a big problem for mismatch testing with Rust, I think.

duckki commented 3 months ago

I don't have a strong opinion on which way we want to fix this in JS. I don't want to hold you back especially since this case is not common.