From 67ba13abd46102c85e9d6f98c0bf348e81dd6b5d Mon Sep 17 00:00:00 2001 From: vitramir Date: Mon, 2 Sep 2019 22:37:04 +0300 Subject: [PATCH 1/5] Allow to use @provide with interfaces --- .../src/composition/compose.ts | 4 +- .../src/composition/utils.ts | 3 +- .../postComposition/externalUnused.ts | 54 +++++++++++-------- .../postComposition/providesNotOnEntity.ts | 3 +- 4 files changed, 38 insertions(+), 26 deletions(-) diff --git a/packages/apollo-federation/src/composition/compose.ts b/packages/apollo-federation/src/composition/compose.ts index 3cb1f3e10c0..424a4117941 100644 --- a/packages/apollo-federation/src/composition/compose.ts +++ b/packages/apollo-federation/src/composition/compose.ts @@ -131,7 +131,9 @@ export function buildMapsFromServiceList(serviceList: ServiceDefinition[]) { for (let definition of typeDefsWithoutExternalFields.definitions) { if ( definition.kind === Kind.OBJECT_TYPE_DEFINITION || - definition.kind === Kind.OBJECT_TYPE_EXTENSION + definition.kind === Kind.OBJECT_TYPE_EXTENSION || + definition.kind === Kind.INTERFACE_TYPE_DEFINITION || + definition.kind === Kind.INTERFACE_TYPE_EXTENSION ) { const typeName = definition.name.value; diff --git a/packages/apollo-federation/src/composition/utils.ts b/packages/apollo-federation/src/composition/utils.ts index bf4204abd2f..201e15680ae 100644 --- a/packages/apollo-federation/src/composition/utils.ts +++ b/packages/apollo-federation/src/composition/utils.ts @@ -16,6 +16,7 @@ import { GraphQLError, GraphQLSchema, isObjectType, + isInterfaceType, GraphQLObjectType, getNamedType, GraphQLField, @@ -219,7 +220,7 @@ export function findFieldsThatReturnType({ schema: GraphQLSchema; typeToFind: GraphQLNamedType; }): GraphQLField[] { - if (!isObjectType(typeToFind)) return []; + if (!isObjectType(typeToFind) && !isInterfaceType(typeToFind)) return []; const fieldsThatReturnType: GraphQLField[] = []; const types = schema.getTypeMap(); diff --git a/packages/apollo-federation/src/composition/validate/postComposition/externalUnused.ts b/packages/apollo-federation/src/composition/validate/postComposition/externalUnused.ts index 1b058dc664f..e51014caa9a 100644 --- a/packages/apollo-federation/src/composition/validate/postComposition/externalUnused.ts +++ b/packages/apollo-federation/src/composition/validate/postComposition/externalUnused.ts @@ -62,29 +62,37 @@ export const externalUnused = (schema: GraphQLSchema) => { reviews: [Review] } */ - const hasMatchingProvidesOnAnotherType = findFieldsThatReturnType({ - schema, - typeToFind: parentType, - }).some(field => - findDirectivesOnTypeOrField(field.astNode, 'provides').some( - directive => { - if (!directive.arguments) return false; - const selections = - isStringValueNode(directive.arguments[0].value) && - parseSelections(directive.arguments[0].value.value); - // find the selections which are fields with names matching - // our external field name - return ( - selections && - selections.some( - selection => - selection.kind === Kind.FIELD && - selection.name.value === externalFieldName, - ) - ); - }, - ), - ); + const hasMatchingProvidesOnAnotherType = [ + parentType, + ...parentType.getInterfaces(), + ] + .map(searchType => + findFieldsThatReturnType({ + schema, + typeToFind: searchType, + }), + ) + .flat() + .some(field => + findDirectivesOnTypeOrField(field.astNode, 'provides').some( + directive => { + if (!directive.arguments) return false; + const selections = + isStringValueNode(directive.arguments[0].value) && + parseSelections(directive.arguments[0].value.value); + // find the selections which are fields with names matching + // our external field name + return ( + selections && + selections.some( + selection => + selection.kind === Kind.FIELD && + selection.name.value === externalFieldName, + ) + ); + }, + ), + ); if (hasMatchingProvidesOnAnotherType) continue; diff --git a/packages/apollo-federation/src/composition/validate/postComposition/providesNotOnEntity.ts b/packages/apollo-federation/src/composition/validate/postComposition/providesNotOnEntity.ts index a4e0d1519d5..9d7f5b28b82 100644 --- a/packages/apollo-federation/src/composition/validate/postComposition/providesNotOnEntity.ts +++ b/packages/apollo-federation/src/composition/validate/postComposition/providesNotOnEntity.ts @@ -1,6 +1,7 @@ import { GraphQLSchema, isObjectType, + isInterfaceType, GraphQLError, isListType, isNonNullType, @@ -47,7 +48,7 @@ export const providesNotOnEntity = (schema: GraphQLSchema) => { // field has a @provides directive on it if (field.federation && field.federation.provides) { - if (!isObjectType(baseType)) { + if (!isObjectType(baseType) && !isInterfaceType(baseType)) { errors.push( errorWithCode( 'PROVIDES_NOT_ON_ENTITY', From d4689b986d4f9465e652949d0077197e2b2c13e0 Mon Sep 17 00:00:00 2001 From: vitramir Date: Fri, 13 Sep 2019 14:16:24 +0300 Subject: [PATCH 2/5] Add test --- .../src/composition/__tests__/compose.test.ts | 46 +++++++++++++++++++ 1 file changed, 46 insertions(+) diff --git a/packages/apollo-federation/src/composition/__tests__/compose.test.ts b/packages/apollo-federation/src/composition/__tests__/compose.test.ts index 1728843e7b9..9c90124c505 100644 --- a/packages/apollo-federation/src/composition/__tests__/compose.test.ts +++ b/packages/apollo-federation/src/composition/__tests__/compose.test.ts @@ -1082,6 +1082,52 @@ describe('composeServices', () => { expect(userField.belongsToValueType).toBe(true); expect(userField.serviceName).toBe(null); }); + + it('adds @provides information for interface types', () => { + const serviceA = { + typeDefs: gql` + interface Product @key(fields: "id") { + id: ID + color: String + } + + type Book implements Product @key(fields: "id") { + id: ID + color: String + pages: Int + } + + type Furniture implements Product @key(fields: "id") { + id: ID + color: String + manufacturer: String + } + `, + name: 'serviceA', + }; + + const serviceB = { + typeDefs: gql` + type Query { + topProduct: Product @provides(fields: "color") + } + `, + name: 'serviceB', + }; + + const { schema, errors } = composeServices([serviceA, serviceB]); + expect(errors).toHaveLength(0); + + const review = schema.getType('Query') as GraphQLObjectType; + expect(review.getFields()['topProduct'].federation) + .toMatchInlineSnapshot(` + Object { + "belongsToValueType": false, + "provides": color, + "serviceName": "serviceB", + } + `); + }); }); describe('@key directive', () => { From 2c23340a484f2363007bf4b4a15f6cc1c115cb79 Mon Sep 17 00:00:00 2001 From: vitramir Date: Fri, 13 Sep 2019 14:22:57 +0300 Subject: [PATCH 3/5] Add changelog record --- packages/apollo-federation/CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/packages/apollo-federation/CHANGELOG.md b/packages/apollo-federation/CHANGELOG.md index 148ced12623..a866aea273e 100644 --- a/packages/apollo-federation/CHANGELOG.md +++ b/packages/apollo-federation/CHANGELOG.md @@ -5,6 +5,7 @@ > The changes noted within this `vNEXT` section have not been released yet. New PRs and commits which introduce changes should include an entry in this `vNEXT` section as part of their development. When a release is being prepared, a new header will be (manually) created below and the the appropriate changes within that release will be moved into the new section. - Nothing yet! Stay tuned! +- Allow to use provides directive with interface types [#3244](https://github.com/apollographql/apollo-server/pull/3244) ### v0.9.1 From 1ab7efafe79e269232b29a17733a0be8f2527552 Mon Sep 17 00:00:00 2001 From: vitramir Date: Wed, 15 Apr 2020 18:33:24 +0300 Subject: [PATCH 4/5] (gateway): add integration tests for provides directive on interfaces --- .../__tests__/integration/provides.test.ts | 32 +++++++++++++++++++ 1 file changed, 32 insertions(+) diff --git a/packages/apollo-gateway/src/__tests__/integration/provides.test.ts b/packages/apollo-gateway/src/__tests__/integration/provides.test.ts index 3370a3d7036..e13cec8abc6 100644 --- a/packages/apollo-gateway/src/__tests__/integration/provides.test.ts +++ b/packages/apollo-gateway/src/__tests__/integration/provides.test.ts @@ -39,6 +39,38 @@ it('does not have to go to another service when field is given', async () => { expect(queryPlan).toCallService('reviews'); }); +it('does not have to go to another service when field is given for interface', async () => { + const query = gql` + query GetReviewers { + topReviews(first: 3) { + product { + manufacturer + } + } + } + `; + + const { data,errors, queryPlan } = await execute( + [accounts, books, inventory, product, reviews], + { + query, + }, + ); + + expect(data).toEqual({ + topReviews: [ + { product: { manufacturer: 'factoryA' } }, + { product: { manufacturer: 'factoryB' } }, + { product: { manufacturer: null } }, + ], + }); + + expect(errors).toBeUndefined() + expect(queryPlan).not.toCallService('product'); + expect(queryPlan).not.toCallService('book'); + expect(queryPlan).toCallService('reviews'); +}); + it('does not load fields provided even when going to other service', async () => { const username = jest.fn(); const localAccounts = overrideResolversInService(accounts, { From a6347aaa19d081afbcede839adaa4e175a9d52ca Mon Sep 17 00:00:00 2001 From: vitramir Date: Wed, 15 Apr 2020 18:35:52 +0300 Subject: [PATCH 5/5] Remove EXTERNAL_MISSING_ON_BASE error --- .../__tests__/externalMissingOnBase.test.ts | 2 +- .../postComposition/externalMissingOnBase.ts | 24 +++++++++---------- packages/apollo-gateway/src/FieldSet.ts | 7 +++++- .../__tests__/__fixtures__/schemas/product.ts | 3 +++ .../__tests__/__fixtures__/schemas/reviews.ts | 10 ++++---- packages/apollo-gateway/src/buildQueryPlan.ts | 5 +++- 6 files changed, 32 insertions(+), 19 deletions(-) diff --git a/packages/apollo-federation/src/composition/validate/postComposition/__tests__/externalMissingOnBase.test.ts b/packages/apollo-federation/src/composition/validate/postComposition/__tests__/externalMissingOnBase.test.ts index 5f9f086bbca..4be1798ddb9 100644 --- a/packages/apollo-federation/src/composition/validate/postComposition/__tests__/externalMissingOnBase.test.ts +++ b/packages/apollo-federation/src/composition/validate/postComposition/__tests__/externalMissingOnBase.test.ts @@ -6,7 +6,7 @@ import { graphqlErrorSerializer } from '../../../../snapshotSerializers'; expect.addSnapshotSerializer(graphqlErrorSerializer); describe('externalMissingOnBase', () => { - it('warns when an @external field does not have a matching field on the base type', () => { + it.skip('warns when an @external field does not have a matching field on the base type', () => { const serviceA = { typeDefs: gql` type Product @key(fields: "sku") { diff --git a/packages/apollo-federation/src/composition/validate/postComposition/externalMissingOnBase.ts b/packages/apollo-federation/src/composition/validate/postComposition/externalMissingOnBase.ts index 0c406da1d49..0a0f59aa06d 100644 --- a/packages/apollo-federation/src/composition/validate/postComposition/externalMissingOnBase.ts +++ b/packages/apollo-federation/src/composition/validate/postComposition/externalMissingOnBase.ts @@ -41,18 +41,18 @@ export const externalMissingOnBase: PostCompositionValidator = ({ schema }) => { // if the field has a serviceName, then it wasn't defined by the // service that owns the type - if ( - matchingBaseField.federation && - matchingBaseField.federation.serviceName - ) { - errors.push( - errorWithCode( - 'EXTERNAL_MISSING_ON_BASE', - logServiceAndType(serviceName, typeName, externalFieldName) + - `marked @external but ${externalFieldName} was defined in ${matchingBaseField.federation.serviceName}, not in the service that owns ${typeName} (${namedType.federation.serviceName})`, - ), - ); - } + // if ( + // matchingBaseField.federation && + // matchingBaseField.federation.serviceName + // ) { + // errors.push( + // errorWithCode( + // 'EXTERNAL_MISSING_ON_BASE', + // logServiceAndType(serviceName, typeName, externalFieldName) + + // `marked @external but ${externalFieldName} was defined in ${matchingBaseField.federation.serviceName}, not in the service that owns ${typeName} (${namedType.federation.serviceName})`, + // ), + // ); + // } } } } diff --git a/packages/apollo-gateway/src/FieldSet.ts b/packages/apollo-gateway/src/FieldSet.ts index 02cf29a2ff5..a4da058467e 100644 --- a/packages/apollo-gateway/src/FieldSet.ts +++ b/packages/apollo-gateway/src/FieldSet.ts @@ -8,6 +8,7 @@ import { SelectionNode, SelectionSetNode, GraphQLObjectType, + isObjectType, } from 'graphql'; import { getResponseName } from './utilities/graphql'; @@ -41,7 +42,11 @@ export function printFields(fields?: FieldSet) { export function matchesField(field: Field) { // TODO: Compare parent type and arguments return (otherField: Field) => { - return field.fieldDef.name === otherField.fieldDef.name; + return ( + field.fieldDef.name === otherField.fieldDef.name && + isObjectType(field.scope.parentType) && + otherField.scope.possibleTypes.includes(field.scope.parentType) + ); }; } diff --git a/packages/apollo-gateway/src/__tests__/__fixtures__/schemas/product.ts b/packages/apollo-gateway/src/__tests__/__fixtures__/schemas/product.ts index e1de28eb495..052d4165b1e 100644 --- a/packages/apollo-gateway/src/__tests__/__fixtures__/schemas/product.ts +++ b/packages/apollo-gateway/src/__tests__/__fixtures__/schemas/product.ts @@ -29,6 +29,7 @@ export const typeDefs = gql` name: String price: String details: ProductDetails + manufacturer: String } interface ProductDetails { @@ -53,6 +54,7 @@ export const typeDefs = gql` brand: Brand metadata: [MetadataOrError] details: ProductDetailsFurniture + manufacturer: String } extend type Book implements Product @key(fields: "isbn") { @@ -64,6 +66,7 @@ export const typeDefs = gql` name(delimeter: String = " "): String @requires(fields: "title year") price: String details: ProductDetailsBook + manufacturer: String } interface Vehicle { diff --git a/packages/apollo-gateway/src/__tests__/__fixtures__/schemas/reviews.ts b/packages/apollo-gateway/src/__tests__/__fixtures__/schemas/reviews.ts index 69b8f58ef90..ee5d925c186 100644 --- a/packages/apollo-gateway/src/__tests__/__fixtures__/schemas/reviews.ts +++ b/packages/apollo-gateway/src/__tests__/__fixtures__/schemas/reviews.ts @@ -14,7 +14,7 @@ export const typeDefs = gql` id: ID! body(format: Boolean = false): String author: User @provides(fields: "username") - product: Product + product: Product @provides(fields: "manufacturer") metadata: [MetadataOrError] } @@ -36,16 +36,18 @@ export const typeDefs = gql` goodAddress: Boolean @requires(fields: "metadata { address }") } - extend interface Product { + extend interface Product @key(fields:"upc") { reviews: [Review] } extend type Furniture implements Product @key(fields: "upc") { + manufacturer: String @external upc: String! @external reviews: [Review] } extend type Book implements Product @key(fields: "isbn") { + manufacturer: String @external isbn: String! @external reviews: [Review] similarBooks: [Book]! @external @@ -98,14 +100,14 @@ const reviews = [ { id: '1', authorID: '1', - product: { __typename: 'Furniture', upc: '1' }, + product: { __typename: 'Furniture', upc: '1', manufacturer:'factoryA' }, body: 'Love it!', metadata: [{ code: 418, message: "I'm a teapot" }], }, { id: '2', authorID: '1', - product: { __typename: 'Furniture', upc: '2' }, + product: { __typename: 'Furniture', upc: '2', manufacturer:'factoryB' }, body: 'Too expensive.', }, { diff --git a/packages/apollo-gateway/src/buildQueryPlan.ts b/packages/apollo-gateway/src/buildQueryPlan.ts index f6264408585..65759b9cf4f 100644 --- a/packages/apollo-gateway/src/buildQueryPlan.ts +++ b/packages/apollo-gateway/src/buildQueryPlan.ts @@ -331,7 +331,10 @@ function splitSubfields( parentGroup.providedFields.some(matchesField(requiredField)), ) ) { - if (owningService === parentGroup.serviceName) { + if ( + owningService === parentGroup.serviceName || + parentGroup.providedFields.some(matchesField(field)) + ) { return parentGroup; } else { return parentGroup.dependentGroupForService(