diff --git a/internals-js/src/operations.ts b/internals-js/src/operations.ts index 836a2ae2a..71532577c 100644 --- a/internals-js/src/operations.ts +++ b/internals-js/src/operations.ts @@ -1390,28 +1390,6 @@ 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 are a few sub-cases where that subset is such that we basically need or - // want to consider to ignore the fragment for that subgraph, and that is when: - // 1. the subset that apply is actually empty. The fragment wouldn't be valid in this case anyway. - // 2. the subset is a single leaf field: in that case, using the one field directly is just shorter than using - // the fragment, so we consider the fragment don't really apply to that subgraph. Technically, using the - // fragment could still be of value if the fragment name is a lot smaller than the one field name, but it's - // enough of a niche case that we ignore it. Note in particular that one sub-case of this rule that is likely - // to be common is when the subset ends up being just `__typename`: this would basically mean the fragment - // don't really apply to the subgraph, and that this will ensure this is the case. - private selectionSetIsWorthUsing(selectionSet: SelectionSet): boolean { - const selections = selectionSet.selections(); - if (selections.length === 0) { - return false; - } - if (selections.length === 1) { - const s = selections[0]; - return !(s.kind === 'FieldSelection' && s.element.isLeafField()); - } - return true; - } - rebaseOn(schema: Schema): NamedFragments | undefined { return this.mapInDependencyOrder((fragment, newFragments) => { const rebasedType = schema.type(fragment.selectionSet.parentType.name); @@ -1423,7 +1401,7 @@ export class NamedFragments { // Rebasing can leave some inefficiencies in some case (particularly when a spread has to be "expanded", see `FragmentSpreadSelection.rebaseOn`), // so we do a top-level normalization to keep things clean. rebasedSelection = rebasedSelection.normalize({ parentType: rebasedType }); - return this.selectionSetIsWorthUsing(rebasedSelection) + return rebasedSelection.isWorthUsing() ? new NamedFragmentDefinition(schema, fragment.name, rebasedType).setSelectionSet(rebasedSelection) : undefined; }); @@ -1556,7 +1534,8 @@ export class SelectionSet { seenSelections: Map = new Map(), ): [SelectionSet, NamedFragments] { const minimizedSelectionSet = this.lazyMap((selection) => { - if (selection.kind === 'FragmentSelection' && selection.element.typeCondition && selection.element.appliedDirectives.length === 0 && selection.selectionSet) { + if (selection.kind === 'FragmentSelection' && selection.element.typeCondition && selection.element.appliedDirectives.length === 0 + && selection.selectionSet && selection.selectionSet.isWorthUsing() ) { // No proper hash code, so we use a unique enough number that's cheap to // compute and handle collisions as necessary. const mockHashCode = `on${selection.element.typeCondition}` + selection.selectionSet.selections().length; @@ -1596,7 +1575,7 @@ export class SelectionSet { } return selection; }); - + return [minimizedSelectionSet, namedFragments]; } @@ -2142,6 +2121,34 @@ export class SelectionSet { : selectionsToString; } } + + // `isWorthUsing` method is used to determine whether we want to factor out + // given selection set into a named fragment so it can be reused across the query. + // Currently, it is used in these cases: + // 1) to reuse existing named fragments in subgraph queries (when reuseQueryFragments is on) + // 2) to factor selection sets into named fragments (when generateQueryFragments is on). + // + // 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 are a few sub-cases where that subset is such that we basically need or + // want to consider to ignore the fragment for that subgraph, and that is when: + // 1. the subset that apply is actually empty. The fragment wouldn't be valid in this case anyway. + // 2. the subset is a single leaf field: in that case, using the one field directly is just shorter than using + // the fragment, so we consider the fragment don't really apply to that subgraph. Technically, using the + // fragment could still be of value if the fragment name is a lot smaller than the one field name, but it's + // enough of a niche case that we ignore it. Note in particular that one sub-case of this rule that is likely + // to be common is when the subset ends up being just `__typename`: this would basically mean the fragment + // don't really apply to the subgraph, and that this will ensure this is the case. + isWorthUsing(): boolean { + const selections = this.selections(); + if (selections.length === 0) { + return false; + } + if (selections.length === 1) { + const s = selections[0]; + return !(s.kind === 'FieldSelection' && s.element.isLeafField()); + } + return true; + } } type PathBasedUpdate = { path: OperationPath, selections?: Selection | SelectionSet | readonly Selection[] }; diff --git a/query-planner-js/src/__tests__/buildPlan.test.ts b/query-planner-js/src/__tests__/buildPlan.test.ts index 04bdbaec7..35fad5afd 100644 --- a/query-planner-js/src/__tests__/buildPlan.test.ts +++ b/query-planner-js/src/__tests__/buildPlan.test.ts @@ -5088,6 +5088,7 @@ describe('Fragment autogeneration', () => { const plan = queryPlanner.buildQueryPlan(operation); + // Note: `... on B {}` won't be replaced, since it has only one field. expect(plan).toMatchInlineSnapshot(` QueryPlan { Fetch(service: "Subgraph1") { @@ -5095,7 +5096,9 @@ describe('Fragment autogeneration', () => { t { __typename ..._generated_onA2_0 - ..._generated_onB1_0 + ... on B { + z + } } } @@ -5103,10 +5106,6 @@ describe('Fragment autogeneration', () => { x y } - - fragment _generated_onB1_0 on B { - z - } }, } `); @@ -5127,15 +5126,13 @@ describe('Fragment autogeneration', () => { t { ... on A { x + y } ... on B { z } } } - ... on B { - z - } } } `, @@ -5143,6 +5140,7 @@ describe('Fragment autogeneration', () => { const plan = queryPlanner.buildQueryPlan(operation); + // Note: `... on B {}` won't be replaced, since it has only one field. expect(plan).toMatchInlineSnapshot(` QueryPlan { Fetch(service: "Subgraph1") { @@ -5150,25 +5148,69 @@ describe('Fragment autogeneration', () => { t { __typename ..._generated_onA3_0 - ..._generated_onB1_0 } } - fragment _generated_onA1_0 on A { + fragment _generated_onA2_0 on A { x - } - - fragment _generated_onB1_0 on B { - z + y } fragment _generated_onA3_0 on A { x y + t { + __typename + ..._generated_onA2_0 + ... on B { + z + } + } + } + }, + } + `); + }); + + it('handles fragments with one non-leaf field', () => { + const [api, queryPlanner] = composeAndCreatePlannerWithOptions([subgraph], { + generateQueryFragments: true, + }); + const operation = operationFromDocument( + api, + gql` + query { + t { + ... on A { + t { + ... on B { + z + } + } + } + } + } + `, + ); + + const plan = queryPlanner.buildQueryPlan(operation); + + expect(plan).toMatchInlineSnapshot(` + QueryPlan { + Fetch(service: "Subgraph1") { + { t { __typename ..._generated_onA1_0 - ..._generated_onB1_0 + } + } + + fragment _generated_onA1_0 on A { + t { + __typename + ... on B { + z + } } } },