diff --git a/.changeset/weak-clouds-argue.md b/.changeset/weak-clouds-argue.md new file mode 100644 index 000000000..9eb3e905a --- /dev/null +++ b/.changeset/weak-clouds-argue.md @@ -0,0 +1,7 @@ +--- +"@apollo/gateway": patch +"@apollo/query-planner": patch +--- + +Fix issue where the query planner was incorrectly not querying `__typename` in a subgraph fetch when `@interfaceObject` is involved + diff --git a/gateway-js/src/__tests__/buildQueryPlan.test.ts b/gateway-js/src/__tests__/buildQueryPlan.test.ts index 2be1df949..1d9beee61 100644 --- a/gateway-js/src/__tests__/buildQueryPlan.test.ts +++ b/gateway-js/src/__tests__/buildQueryPlan.test.ts @@ -881,6 +881,7 @@ describe('buildQueryPlan', () => { __typename ... on Image { ... on NamedObject @include(if: $b) { + __typename name } } diff --git a/gateway-js/src/__tests__/executeQueryPlan.test.ts b/gateway-js/src/__tests__/executeQueryPlan.test.ts index df33a8c10..fefe62890 100644 --- a/gateway-js/src/__tests__/executeQueryPlan.test.ts +++ b/gateway-js/src/__tests__/executeQueryPlan.test.ts @@ -4300,6 +4300,287 @@ describe('executeQueryPlan', () => { } `); }); + + test('handles querying fields of an implementation type coming from an @interfaceObject subgraph', async () => { + const products = [ + { + id: "1", + title: "Jane Eyre", + price: 12.99, + author: "Charlotte Bronte", + ISBN: "9780743273565", + }, + { + id: "2", + title: "Good Will Hunting", + price: 14.99, + director: "Gus Van Sant", + duration: 126, + }, + ]; + + const s1 = { + name: 'products', + typeDefs: gql` + extend schema + @link(url: "https://specs.apollo.dev/federation/v2.3", import: ["@key"]) + + type Query { + products: [Product!]! + } + + interface Product @key(fields: "id") { + id: ID! + description: String + price: Float + } + + type Book implements Product @key(fields: "id") { + id: ID! + description: String + price: Float + pages: Int + ISBN: String! + } + + type Movie implements Product @key(fields: "id") { + id: ID! + description: String + price: Float + duration: Int + } + `, + resolvers: { + Product: { + __resolveType(product: any) { + if (product.author) { + return "Book"; + } else if (product.director) { + return "Movie"; + } else { + return null; + } + }, + __resolveReference(reference: any) { + return products.find((obj) => obj.id === reference.id); + }, + }, + } + } + + const s2 = { + name: 'reviews', + typeDefs: gql` + extend schema + @link(url: "https://specs.apollo.dev/federation/v2.3", import: ["@key", "@interfaceObject"]) + + type Query { + allReviewedProducts: [Product!]! + } + + type Product @key(fields: "id") @interfaceObject { + id: ID! + reviews: [Review!]! + } + + type Review { + author: String + text: String + rating: Int + } + `, + resolvers: { + Query: { + allReviewedProducts: () => products, + } + } + } + + const { serviceMap, schema, queryPlanner} = getFederatedTestingSchema([ s1, s2 ]); + + let operation = parseOp(` + { + allReviewedProducts { + ... on Book { + ISBN + } + } + } + `, schema); + + let queryPlan = buildPlan(operation, queryPlanner); + // We're going check again with almost the query but requesting the `id` field. And the + // plan should be exactly the same since `id` gets queried here anyway as a by-product already. + const expectedPlan = ` + QueryPlan { + Sequence { + Fetch(service: "reviews") { + { + allReviewedProducts { + __typename + id + } + } + }, + Flatten(path: "allReviewedProducts.@") { + Fetch(service: "products") { + { + ... on Product { + __typename + id + } + } => + { + ... on Product { + __typename + ... on Book { + ISBN + } + } + } + }, + }, + }, + } + `; + expect(queryPlan).toMatchInlineSnapshot(expectedPlan); + + let response = await executePlan(queryPlan, operation, undefined, schema, serviceMap); + // Note that the 2nd product is a Movie, so we should get an empty object + expect(response.data).toMatchInlineSnapshot(` + Object { + "allReviewedProducts": Array [ + Object { + "ISBN": "9780743273565", + }, + Object {}, + ], + } + `); + + operation = parseOp(` + { + allReviewedProducts { + ... on Book { + id + ISBN + } + } + } + `, schema); + + queryPlan = buildPlan(operation, queryPlanner); + // As said above, we should get the same plan as the previous time. + expect(queryPlan).toMatchInlineSnapshot(expectedPlan); + + response = await executePlan(queryPlan, operation, undefined, schema, serviceMap); + // But now we should have the "id" of the book (and still nothing for the movie). + expect(response.data).toMatchInlineSnapshot(` + Object { + "allReviewedProducts": Array [ + Object { + "ISBN": "9780743273565", + "id": "1", + }, + Object {}, + ], + } + `); + + // Now with __typename just for the book + operation = parseOp(` + { + allReviewedProducts { + ... on Book { + __typename + ISBN + } + } + } + `, schema); + + queryPlan = buildPlan(operation, queryPlanner); + // The plan is almost the exact same as the previous one, but in this case we do end up asking for __typename + // within `... on Book` on the 2nd fetch. Which is not really necessary since we already have the __typename + // above, and we could optimise it, but unclear it's even worth the effort. + expect(queryPlan).toMatchInlineSnapshot(` + QueryPlan { + Sequence { + Fetch(service: "reviews") { + { + allReviewedProducts { + __typename + id + } + } + }, + Flatten(path: "allReviewedProducts.@") { + Fetch(service: "products") { + { + ... on Product { + __typename + id + } + } => + { + ... on Product { + __typename + ... on Book { + __typename + ISBN + } + } + } + }, + }, + }, + } + `); + + response = await executePlan(queryPlan, operation, undefined, schema, serviceMap); + expect(response.data).toMatchInlineSnapshot(` + Object { + "allReviewedProducts": Array [ + Object { + "ISBN": "9780743273565", + "__typename": "Book", + }, + Object {}, + ], + } + `); + + // And lastly with __typename but for all products + operation = parseOp(` + { + allReviewedProducts { + __typename + ... on Book { + ISBN + } + } + } + `, schema); + + queryPlan = buildPlan(operation, queryPlanner); + // As said above, we should get the same plan as the previous time. + expect(queryPlan).toMatchInlineSnapshot(expectedPlan); + + response = await executePlan(queryPlan, operation, undefined, schema, serviceMap); + expect(response.data).toMatchInlineSnapshot(` + Object { + "allReviewedProducts": Array [ + Object { + "ISBN": "9780743273565", + "__typename": "Book", + }, + Object { + "__typename": "Movie", + }, + ], + } + `); + }); }); describe('fields with conflicting types needing aliasing', () => { diff --git a/query-planner-js/src/__tests__/buildPlan.interfaceObject.test.ts b/query-planner-js/src/__tests__/buildPlan.interfaceObject.test.ts index 2b7475483..ff6125541 100644 --- a/query-planner-js/src/__tests__/buildPlan.interfaceObject.test.ts +++ b/query-planner-js/src/__tests__/buildPlan.interfaceObject.test.ts @@ -122,6 +122,7 @@ describe('basic @key on interface/@interfaceObject handling', () => { } => { ... on I { + __typename x } } @@ -347,6 +348,7 @@ describe('basic @key on interface/@interfaceObject handling', () => { } => { ... on I { + __typename ... on A { x } @@ -651,6 +653,7 @@ it('handles @interfaceObject in nested entity', () => { } => { ... on I { + __typename a } } diff --git a/query-planner-js/src/__tests__/buildPlan.test.ts b/query-planner-js/src/__tests__/buildPlan.test.ts index d05674537..2e17a39b5 100644 --- a/query-planner-js/src/__tests__/buildPlan.test.ts +++ b/query-planner-js/src/__tests__/buildPlan.test.ts @@ -4797,6 +4797,7 @@ describe('merged abstract types handling', () => { u { __typename ... on I { + __typename v } } @@ -5094,6 +5095,7 @@ describe('merged abstract types handling', () => { i1 { __typename ... on I2 { + __typename v } } diff --git a/query-planner-js/src/buildPlan.ts b/query-planner-js/src/buildPlan.ts index 8ba7f80ac..80401c60d 100644 --- a/query-planner-js/src/buildPlan.ts +++ b/query-planner-js/src/buildPlan.ts @@ -57,6 +57,8 @@ import { isInterfaceType, isNonNullType, Type, + FragmentSelection, + InterfaceType, } from "@apollo/federation-internals"; import { advanceSimultaneousPathsWithOperation, @@ -919,6 +921,86 @@ class FetchGroup { } } + // If a group is such that everything is fetches is already included in the inputs, then + // this group does useless fetches. + isUseless(): boolean { + if (!this.inputs || this.mustPreserveSelection) { + return false; + } + + // For groups that fetches from an @interfaceObject, we can sometimes have something like + // { ... on Book { id } } => { ... on Product { id } } + // where `Book` is an implementation of interface `Product`. + // And that is because while only "books" are concerned by this fetch, the `Book` type is unknown + // of the queried subgraph (in that example, it defines `Product` as an @interfaceObject) and + // so we have to "cast" into `Product` instead of `Book`. + // But the fetch above _is_ useless, it does only fetch its inputs, and we wouldn't catch this + // if we do a raw inclusion check of `selection` into `inputs` + // + // We only care about this problem at the top-level of the selections however, so we does that + // top-level check manually (instead of just calling `this.inputs.contains(this.selection)`) + // but fallback on `contains` for anything deeper. + + const conditionInSupergraphIfInterfaceObject = (selection: Selection): InterfaceType | undefined => { + if (selection.kind === 'FragmentSelection') { + const condition = selection.element().typeCondition; + if (condition && isObjectType(condition)) { + const conditionInSupergraph = this.dependencyGraph.supergraphSchema.type(condition.name); + // Note that we're checking the true supergraph, not the API schema, so even @inaccessible types will be found. + assert(conditionInSupergraph, () => `Type ${condition.name} should exists in the supergraph`) + if (isInterfaceType(conditionInSupergraph)) { + return conditionInSupergraph; + } + } + } + return undefined; + } + + const inputSelections = this.inputs.selections(); + // Checks that every selection is contained in the input selections. + return this.selection.selections().every((selection) => { + const conditionInSupergraph = conditionInSupergraphIfInterfaceObject(selection); + if (!conditionInSupergraph) { + // We're not in the @interfaceObject case described above. We just check that an input selection contains the + // one we check. + return inputSelections.some((input) => input.contains(selection)); + } + + const implemTypeNames = conditionInSupergraph.possibleRuntimeTypes().map((t) => t.name); + // Find all the input selections that selects object for this interface, that is selection on + // either the interface directly or on one of it's implementation type (we keep both kind separate). + const interfaceInputSelections: FragmentSelection[] = []; + const implementationInputSelections: FragmentSelection[] = []; + for (const inputSelection of inputSelections) { + // We know that fetch inputs are wrapped in fragments whose condition is an entity type: + // that's how we build them and we couldn't select inputs correctly otherwise. + assert(inputSelection.kind === 'FragmentSelection', () => `Unexpecting input selection ${inputSelection} on ${this}`); + const inputCondition = inputSelection.element().typeCondition; + assert(inputCondition, () => `Unexpecting input selection ${inputSelection} on ${this} (missing condition)`); + if (inputCondition.name == conditionInSupergraph.name) { + interfaceInputSelections.push(inputSelection); + } else if (implemTypeNames.includes(inputCondition.name)) { + implementationInputSelections.push(inputSelection); + } + } + + const subSelectionSet = selection.selectionSet; + // we're only here if `conditionInSupergraphIfInterfaceObject` returned something, we imply that selection is a fragment + // selection and so has a sub-selectionSet. + assert(subSelectionSet, () => `Should not be here for ${selection}`); + + // If there is some selections on the interface, then the selection needs to be contained in those. + // Otherwise, if there is implementation selections, it must be contained in _each_ of them (we + // shouldn't have the case where there is neither interface nor implementation selections, but + // we just return false if that's the case as a "safe" default). + if (interfaceInputSelections.length > 0) { + return interfaceInputSelections.some((input) => input.selectionSet.contains(subSelectionSet)); + } + return implementationInputSelections.length > 0 + && implementationInputSelections.every((input) => input.selectionSet.contains(subSelectionSet)); + }); + } + /** * Merges a child of `this` group into it. * @@ -1868,9 +1950,7 @@ class FetchDependencyGraph { this.removeUselessGroups(child); } - // If a group is such that everything is fetches is already included in the inputs, then - // this group does useless fetches and can be removed. - if (group.inputs && !group.mustPreserveSelection && group.inputs.contains(group.selection)) { + if (group.isUseless()) { // In general, removing a group is a bit tricky because we need to deal with the fact // that the group can have multiple parents and children and no break the "path in parent" // in all those cases. To keep thing relatively easily, we only handle the following @@ -3632,6 +3712,10 @@ function addTypenameFieldForAbstractTypes(selectionSet: SelectionSet) { addTypenameFieldForAbstractTypes(selection.selectionSet); } } else { + const conditionType = selection.element().typeCondition; + if (conditionType && isAbstractType(conditionType)) { + selection.selectionSet!.add(new FieldSelection(new Field(conditionType.typenameField()!))); + } addTypenameFieldForAbstractTypes(selection.selectionSet); } }