Reuse named fragment in subgraphs even if they only apply partially #2639

pcmanus · 2023-06-23T08:16:15Z

The focus of the code reusing fragment has been thus far on making that if a given named fragment can be used in a given selection set, then the code finds it. But in practice, the named fragments we try to reuse are those of the input query, so defined against the supergraph API schema, while we're trying to reuse them on subgraph fetches, so the fragment may be defined on a type that a subgraph does know, or request fields the subgraph does not define.

Currently, when asking if a given named fragment can be used against a given subgraph, we only keep fragment that fully apply to that subgraph (that is, only if everything the fragment queries exists in the subgraph).

This does limit the usefulness of named fragment reuse however, as one of the point of federation is that each subgraph defines a subset of the full supergraph API.

This commit improves this by instead computing the subset of each named fragment that do apply to a given subgraph, and define the fragment as only that fragment for the subgraph.

Note that it does so, this commit do not try reusing a named fragment against a subgraph if the subset of the fragment on the subgraph comes to only a single leaf field. This avoids spending time trying to reuse a fragment that ends up being just:

fragment MySuperFragment on X {
  __typename
}

or even:

fragment MySuperFragment on X {
 id
}

as those aren't productive (the goal of fragment reuse is to try to make the subgraph fetches smaller, but it is small to just request id, even 10 times, than to request 10 times ...MySuperFragment).

netlify · 2023-06-23T08:16:17Z

👷 Deploy request for apollo-federation-docs pending review.

Visit the deploys page to approve it

Name	Link
🔨 Latest commit	`f4172d4`

changeset-bot · 2023-06-23T08:16:18Z

🦋 Changeset detected

Latest commit: f4172d4

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

The focus of the code reusing fragment has been thus far on making that if a given named fragment can be used in a given selection set, then the code finds it. But in practice, the named fragments we try to reuse are those of the input query, so defined against the supergraph API schema, while we're trying to reuse them on subgraph fetches, so the fragment may be defined on a type that a subgraph does know, or request fields the subgraph does not define. Currently, when asking if a given named fragment can be used against a given subgraph, we only keep fragment that _fully_ apply to that subgraph (that is, only if everything the fragment queries exists in the subgraph). This does limit the usefulness of named fragment reuse however, as one of the point of federation is that each subgraph defines a subset of the full supergraph API. This commit improves this by instead computing the subset of each named fragment that do apply to a given subgraph, and define the fragment as only that fragment for the subgraph. Note that it does so, this commit do not try reusing a named fragment against a subgraph if the subset of the fragment on the subgraph comes to only a single leaf field. This avoids spending time trying to reuse a fragment that ends up being just: ```graphql fragment MySuperFragment on X { __typename } ``` or even: ```graphql fragment MySuperFragment on X { id } ``` as those aren't productive (the goal of fragment reuse is to try to make the subgraph fetches smaller, but it is small to just request `id`, even 10 times, than to request 10 times `...MySuperFragment`).

codesandbox-ci · 2023-06-23T08:18:14Z

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.

dariuszkuc

Looks good. Left few nits related to comments.

dariuszkuc · 2023-06-23T20:35:47Z

internals-js/src/__tests__/operations.test.ts

+      }
+    `);
+
+    // F1 reduces to nothing, and F2 reduces to just __typename so we should keep them.


Suggested change

// F1 reduces to nothing, and F2 reduces to just __typename so we should keep them.

// F1 reduces to nothing, and F2 reduces to just __typename so we shouldn't keep them.

dariuszkuc · 2023-06-23T21:22:32Z

internals-js/src/operations.ts

@@ -1347,21 +1358,39 @@ export class NamedFragments {
    });
  }

+  // When we rebase named fragments on a subgraph schema, only a subset of what the fragment handles may belong
+  // to that particular subgraph. And there is a few sub-cases where that subset is such that we basically need or


Suggested change

// to that particular subgraph. And there is a few sub-cases where that subset is such that we basically need or

// to that particular subgraph. And there are a few sub-cases where that subset is such that we basically need or

dariuszkuc · 2023-06-23T21:28:16Z

internals-js/src/operations.ts

+      // As we check inclusion, we ignore the case where the fragment queries __typename but the subSelection does not.
+      // The rational is that querying `__typename` unecessarily is mostly harmess (it always works and it's super cheap)
+      // so we don't want to not use a fragment just to save querying a `__typename` in a few case. But the underlying
+      // context of why this matter is that the query planner always request __typename for abstract type, and will do
+      // so in fragments too, but we can have a field that _does_ return an abstract type within a fragment, but that
+      // _do not_ end up returning an abstract type when applied in a "more specific" context (think a fragment on
+      // an interface I1 where a inside field returns another interface I2, but applied in the context of a implementation
+      // type of I1 where that particular field returns an implentation of I2 rather than I2 directly; we would have
+      // added __typename to the fragment (because it's all interfaces), but the selection itself, which only deals
+      // with object type, may not have __typename requested; using the fragment might still be a good idea, and
+      // querying __typename needlessly is a very small price to pay for that).


Suggested change

// As we check inclusion, we ignore the case where the fragment queries __typename but the subSelection does not.

// The rational is that querying `__typename` unecessarily is mostly harmess (it always works and it's super cheap)

// so we don't want to not use a fragment just to save querying a `__typename` in a few case. But the underlying

// context of why this matter is that the query planner always request __typename for abstract type, and will do

// so in fragments too, but we can have a field that _does_ return an abstract type within a fragment, but that

// _do not_ end up returning an abstract type when applied in a "more specific" context (think a fragment on

// an interface I1 where a inside field returns another interface I2, but applied in the context of a implementation

// type of I1 where that particular field returns an implentation of I2 rather than I2 directly; we would have

// added __typename to the fragment (because it's all interfaces), but the selection itself, which only deals

// with object type, may not have __typename requested; using the fragment might still be a good idea, and

// querying __typename needlessly is a very small price to pay for that).

// As we check inclusion, we ignore the case where the fragment queries __typename but the subSelection does not.

// The rational is that querying `__typename` unecessarily is mostly harmless (it always works and it's super cheap)

// so we don't want to not use a fragment just to save querying a `__typename` in a few cases. But the underlying

// context of why this matters is that the query planner always requests __typename for abstract type, and will do

// so in fragments too, but we can have a field that _does_ return an abstract type within a fragment, but that

// _does not_ end up returning an abstract type when applied in a "more specific" context (think a fragment on

// an interface I1 where a inside field returns another interface I2, but applied in the context of a implementation

// type of I1 where that particular field returns an implementation of I2 rather than I2 directly; we would have

// added __typename to the fragment (because it's all interfaces), but the selection itself, which only deals

// with an object type, may not have __typename requested; using the fragment might still be a good idea, and

// querying __typename needlessly is a very small price to pay for that).

…tially (#2639)" This reverts commit d60349b.

### Update router bridge and add option for `reuse_query_fragments` ([Issue #3452](#3452)) Federation v2.4.9 enabled a new feature for [query fragment reuse](apollographql/federation#2639) that is causing issues for some users. A new option has been added to he router config file to opt into this feature: ```yaml supergraph: experimental_reuse_query_fragments: true ``` The default is disabled. Fixes #3452  **Checklist** Complete the checklist (and note appropriate exceptions) before a final PR is raised. - [ ] Changes are compatible[^1] - [ ] Documentation[^2] completed - [ ] Performance impact assessed and acceptable - Tests added and passing[^3] - [ ] Unit Tests - [ ] Integration Tests - [ ] Manual Tests **Exceptions** *Note any exceptions here* **Notes** [^1]. It may be appropriate to bring upcoming changes to the attention of other (impacted) groups. Please endeavour to do this before seeking PR approval. The mechanism for doing this will vary considerably, so use your judgement as to how and when to do this. [^2]. Configuration is an important part of many changes. Where applicable please try to document configuration examples. [^3]. Tick whichever testing boxes are applicable. If you are adding Manual Tests: - please document the manual testing (extensively) in the Exceptions. - please raise a separate issue to automate the test and label it (or ask for it to be labeled) as `manual test` --------- Co-authored-by: bryn <[email protected]>

This reverts commit b6be9f9.

This commit reapplies #2639 with an additional fix related to reproducing and resolving #2680. The bug existed in the SelectionSet.contains logic in the final check to provide a ContainsResult. Here the lengths of the compared selection sets are used to determine subset or strictly equal. In the case that the __typename field was ignored up above, the comparison becomes invalid unless we offset the comparison by 1 to account for the non-existent field.

pcmanus requested a review from a team as a code owner June 23, 2023 08:16

pcmanus force-pushed the frag-reuse-followups branch from 9117adb to d9fd30a Compare June 23, 2023 08:16

korinne added the P3 An issue that should be addressed when able. label Jun 23, 2023

korinne assigned dariuszkuc Jun 23, 2023

dariuszkuc approved these changes Jun 23, 2023

View reviewed changes

Fix typos/grammatical errors

f4172d4

pcmanus merged commit d60349b into apollographql:main Jun 27, 2023

This was referenced Jun 27, 2023

release: on branch main #2634

Merged

release: on branch next #2597

Merged

trevor-scheer mentioned this pull request Jul 17, 2023

Revert #2639, an attempt to fix query fragment reuse #2681

Merged

trevor-scheer added a commit that referenced this pull request Jul 17, 2023

Revert "Reuse named fragment in subgraphs even if they only apply par…

c66bbcd

…tially (#2639)" This reverts commit d60349b.

trevor-scheer added a commit that referenced this pull request Jul 17, 2023

Revert #2639, an attempt to fix query fragment reuse (#2681)

b6be9f9

github-actions bot mentioned this pull request Jul 17, 2023

release: on branch main #2682

Merged

This was referenced Jul 18, 2023

Reuse of query fragments issue apollographql/router#3452

Closed

Update router bridge and add option for reuse_query_fragments apollographql/router#3453

Merged

trevor-scheer added a commit that referenced this pull request Jul 18, 2023

Revert "Revert #2639, an attempt to fix query fragment reuse (#2681)"

f41936f

This reverts commit b6be9f9.

trevor-scheer mentioned this pull request Jul 19, 2023

Reapply #2639 (with fix for #2680) #2684

Merged

github-actions bot mentioned this pull request Jul 19, 2023

release: on branch version-2.4 #2686

Merged

trevor-scheer mentioned this pull request Jul 19, 2023

Reapply #2639 (with fix for #2680) #2687

Merged

github-actions bot mentioned this pull request Jul 19, 2023

release: on branch main #2690

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reuse named fragment in subgraphs even if they only apply partially #2639

Reuse named fragment in subgraphs even if they only apply partially #2639

pcmanus commented Jun 23, 2023

netlify bot commented Jun 23, 2023 •

edited

Loading

changeset-bot bot commented Jun 23, 2023 •

edited

Loading

codesandbox-ci bot commented Jun 23, 2023 •

edited

Loading

dariuszkuc left a comment

dariuszkuc Jun 23, 2023

dariuszkuc Jun 23, 2023

dariuszkuc Jun 23, 2023

	// F1 reduces to nothing, and F2 reduces to just __typename so we should keep them.
	// F1 reduces to nothing, and F2 reduces to just __typename so we shouldn't keep them.

	// to that particular subgraph. And there is a few sub-cases where that subset is such that we basically need or
	// to that particular subgraph. And there are a few sub-cases where that subset is such that we basically need or

Reuse named fragment in subgraphs even if they only apply partially #2639

Reuse named fragment in subgraphs even if they only apply partially #2639

Conversation

pcmanus commented Jun 23, 2023

netlify bot commented Jun 23, 2023 • edited Loading

👷 Deploy request for apollo-federation-docs pending review.

changeset-bot bot commented Jun 23, 2023 • edited Loading

🦋 Changeset detected

codesandbox-ci bot commented Jun 23, 2023 • edited Loading

dariuszkuc left a comment

Choose a reason for hiding this comment

dariuszkuc Jun 23, 2023

Choose a reason for hiding this comment

dariuszkuc Jun 23, 2023

Choose a reason for hiding this comment

dariuszkuc Jun 23, 2023

Choose a reason for hiding this comment

netlify bot commented Jun 23, 2023 •

edited

Loading

changeset-bot bot commented Jun 23, 2023 •

edited

Loading

codesandbox-ci bot commented Jun 23, 2023 •

edited

Loading