From a24e1d8593e938ac64e971bad301183ca12e88a8 Mon Sep 17 00:00:00 2001 From: Trevor Scheer Date: Thu, 20 Aug 2020 17:23:39 -0700 Subject: [PATCH 1/7] fix(federation): Create new @key validations to prevent invalid compositions A few cases of invalid @key directive arrangements are currently possible with our current validation suite. This commit adds 3 new validations to ensure that users are informed of these invalid states and block composition appropriately. 1. Owning type must specify a @key 2. Extending types can't specify multiple @keys 3. Extending types must use a key specified by the owning type Additionally: These invalid arrangements can result in runtime errors when printing CSDL, nor is it really valid to return an "attempted" print of CSDL, so the result of `composedSdl` is now `undefined` in the case that there are composition errors. --- .../src/composition/composeAndValidate.ts | 6 +- .../__tests__/keysMatchBaseService.test.ts | 117 ++++++++++++++++++ .../postComposition/externalUnused.ts | 8 ++ .../validate/postComposition/index.ts | 1 + .../postComposition/keysMatchBaseService.ts | 83 +++++++++++++ 5 files changed, 214 insertions(+), 1 deletion(-) create mode 100644 packages/apollo-federation/src/composition/validate/postComposition/__tests__/keysMatchBaseService.test.ts create mode 100644 packages/apollo-federation/src/composition/validate/postComposition/keysMatchBaseService.ts diff --git a/packages/apollo-federation/src/composition/composeAndValidate.ts b/packages/apollo-federation/src/composition/composeAndValidate.ts index 0683092dba5..d53229bf81b 100644 --- a/packages/apollo-federation/src/composition/composeAndValidate.ts +++ b/packages/apollo-federation/src/composition/composeAndValidate.ts @@ -31,7 +31,11 @@ export function composeAndValidate(serviceList: ServiceDefinition[]) { }), ); - const composedSdl = printComposedSdl(compositionResult.schema, serviceList); + // We shouldn't try to print the SDL if there were errors during composition + const composedSdl = + errors.length === 0 + ? printComposedSdl(compositionResult.schema, serviceList) + : undefined; // TODO remove the warnings array once no longer used by clients return { diff --git a/packages/apollo-federation/src/composition/validate/postComposition/__tests__/keysMatchBaseService.test.ts b/packages/apollo-federation/src/composition/validate/postComposition/__tests__/keysMatchBaseService.test.ts new file mode 100644 index 00000000000..e8adeb60305 --- /dev/null +++ b/packages/apollo-federation/src/composition/validate/postComposition/__tests__/keysMatchBaseService.test.ts @@ -0,0 +1,117 @@ +import gql from 'graphql-tag'; +import { composeServices } from '../../../compose'; +import { keysMatchBaseService as validateKeysMatchBaseService } from '../'; +import { graphqlErrorSerializer } from '../../../../snapshotSerializers'; + +expect.addSnapshotSerializer(graphqlErrorSerializer); + +describe('keysMatchBaseService', () => { + it('returns no errors with proper @key usage', () => { + const serviceA = { + typeDefs: gql` + type Product @key(fields: "sku") { + sku: String! + upc: String! + } + `, + name: 'serviceA', + }; + + const serviceB = { + typeDefs: gql` + extend type Product @key(fields: "sku") { + sku: String! @external + price: Int! + } + `, + name: 'serviceB', + }; + + const serviceList = [serviceA, serviceB]; + const { schema, errors } = composeServices(serviceList); + expect(errors).toHaveLength(0); + + const validationErrors = validateKeysMatchBaseService({ + schema, + serviceList, + }); + expect(validationErrors).toHaveLength(0); + }); + + it('requires a @key to be specified on the owning type', () => { + const serviceA = { + typeDefs: gql` + type Product { + sku: String! + upc: String! + } + `, + name: 'serviceA', + }; + + const serviceB = { + typeDefs: gql` + extend type Product @key(fields: "sku") { + sku: String! @external + price: Int! + } + `, + name: 'serviceB', + }; + + const serviceList = [serviceA, serviceB]; + const { schema, errors } = composeServices(serviceList); + expect(errors).toHaveLength(0); + + const validationErrors = validateKeysMatchBaseService({ + schema, + serviceList, + }); + expect(validationErrors).toHaveLength(1); + expect(validationErrors[0]).toMatchInlineSnapshot(` + Object { + "code": "KEY_MISSING_ON_BASE", + "message": "[serviceA] Product -> appears to be an entity but no @key directives are specified on the owning type.", + } + `); + }); + + it('requires extending services to use a @key specified by the owning type', () => { + const serviceA = { + typeDefs: gql` + type Product @key(fields: "sku upc") { + sku: String! + upc: String! + } + `, + name: 'serviceA', + }; + + const serviceB = { + typeDefs: gql` + extend type Product @key(fields: "sku") { + sku: String! @external + price: Int! + } + `, + name: 'serviceB', + }; + + const serviceList = [serviceA, serviceB]; + const { schema, errors } = composeServices(serviceList); + expect(errors).toHaveLength(0); + + const validationErrors = validateKeysMatchBaseService({ + schema, + serviceList, + }); + expect(validationErrors).toHaveLength(1); + expect(validationErrors[0]).toMatchInlineSnapshot(` + Object { + "code": "KEY_NOT_SPECIFIED", + "message": "[serviceB] Product -> extends from serviceA but specifies an invalid @key directive. Valid @key directives are specified by the owning type. Available @key directives for this type are: + @key(fields: \\"sku upc\\")", + } + `); + }); +}); diff --git a/packages/apollo-federation/src/composition/validate/postComposition/externalUnused.ts b/packages/apollo-federation/src/composition/validate/postComposition/externalUnused.ts index d58b3317f02..2c671ef211a 100644 --- a/packages/apollo-federation/src/composition/validate/postComposition/externalUnused.ts +++ b/packages/apollo-federation/src/composition/validate/postComposition/externalUnused.ts @@ -25,6 +25,14 @@ export const externalUnused: PostCompositionValidator = ({ schema }) => { // If externals is populated, we need to look at each one and confirm // it is used const typeFederationMetadata = getFederationMetadata(parentType); + + // Escape a validation case that's falling through incorrectly. This case + // is handled by `keysMatchBaseService`. + if (typeFederationMetadata) { + const {serviceName, keys} = typeFederationMetadata; + if (serviceName && keys && !keys[serviceName]) continue; + } + if (typeFederationMetadata?.externals) { // loop over every service that has extensions with @external for (const [serviceName, externalFieldsForService] of Object.entries( diff --git a/packages/apollo-federation/src/composition/validate/postComposition/index.ts b/packages/apollo-federation/src/composition/validate/postComposition/index.ts index b184177183d..6e7a35189db 100644 --- a/packages/apollo-federation/src/composition/validate/postComposition/index.ts +++ b/packages/apollo-federation/src/composition/validate/postComposition/index.ts @@ -17,6 +17,7 @@ export { executableDirectivesInAllServices, } from './executableDirectivesInAllServices'; export { executableDirectivesIdentical } from './executableDirectivesIdentical'; +export { keysMatchBaseService } from './keysMatchBaseService'; export type PostCompositionValidator = ({ schema, diff --git a/packages/apollo-federation/src/composition/validate/postComposition/keysMatchBaseService.ts b/packages/apollo-federation/src/composition/validate/postComposition/keysMatchBaseService.ts new file mode 100644 index 00000000000..c377e73c264 --- /dev/null +++ b/packages/apollo-federation/src/composition/validate/postComposition/keysMatchBaseService.ts @@ -0,0 +1,83 @@ +import { isObjectType, GraphQLError, SelectionNode } from 'graphql'; +import { + logServiceAndType, + errorWithCode, + getFederationMetadata, +} from '../../utils'; +import { PostCompositionValidator } from '.'; +import { printWithReducedWhitespace } from '../../../service'; + +/** + * 1. KEY_MISSING_ON_BASE - Owning types must specify at least 1 @key directive + * 2. MULTIPLE_KEYS_ON_EXTENSION - Extending services may not use more than 1 @key directive + * 3. KEY_NOT_SPECIFIED - Extending services must use a valid @key specified by the owning type + */ +export const keysMatchBaseService: PostCompositionValidator = function ({ + schema, +}) { + const errors: GraphQLError[] = []; + const types = schema.getTypeMap(); + for (const [parentTypeName, parentType] of Object.entries(types)) { + // Only object types have fields + if (!isObjectType(parentType)) continue; + + const typeFederationMetadata = getFederationMetadata(parentType); + + if (typeFederationMetadata) { + const { serviceName, keys } = typeFederationMetadata; + + if (serviceName && keys) { + if (!keys[serviceName]) { + errors.push( + errorWithCode( + 'KEY_MISSING_ON_BASE', + logServiceAndType(serviceName, parentTypeName) + + `appears to be an entity but no @key directives are specified on the owning type.`, + ), + ); + continue; + } + + const availableKeys = keys[serviceName].map(printFieldSet); + + // No need to validate that the owning service matches its specified keys + Object.entries(keys) + .filter(([service]) => service !== serviceName) + .forEach(([extendingService, keyFields]) => { + // Extensions can't specify more than one key + if (keyFields.length > 1) { + errors.push( + errorWithCode( + 'MULTIPLE_KEYS_ON_EXTENSION', + logServiceAndType(extendingService, parentTypeName) + + `is extended from service ${serviceName} but specifies multiple @key directives. Extensions may only specify one @key.`, + ), + ); + return; + } + + const extensionKey = printFieldSet(keyFields[0]); + if (!availableKeys.includes(extensionKey)) { + errors.push( + errorWithCode( + 'KEY_NOT_SPECIFIED', + logServiceAndType(extendingService, parentTypeName) + + `extends from ${serviceName} but specifies an invalid @key directive. Valid @key directives are specified by the owning type. Available @key directives for this type are:\n` + + `\t${availableKeys + .map((fieldSet) => `@key(fields: "${fieldSet}")`) + .join('\n\t')}`, + ), + ); + return; + } + }); + } + } + } + + return errors; +}; + +function printFieldSet(selections: readonly SelectionNode[]): string { + return selections.map(printWithReducedWhitespace).join(' '); +} From c8d4a5220edb739602ba4f09a913de841acc6df0 Mon Sep 17 00:00:00 2001 From: Trevor Scheer Date: Thu, 20 Aug 2020 17:27:29 -0700 Subject: [PATCH 2/7] Add documentation for new error codes --- docs/source/federation/errors.md | 3 +++ 1 file changed, 3 insertions(+) diff --git a/docs/source/federation/errors.md b/docs/source/federation/errors.md index 46eea0c79d0..285485c6eb4 100644 --- a/docs/source/federation/errors.md +++ b/docs/source/federation/errors.md @@ -24,6 +24,9 @@ If Apollo Gateway encounters an error, composition fails. This document lists co | `KEY_FIELDS_SELECT_INVALID_TYPE` | The `fields` argument of an entity's `@key` includes at least one root field that results in a list, interface, or union type. Root fields of these types cannot be part of a `@key`. | | `KEY_FIELDS_MISSING_ON_BASE` | The `fields` argument of an entity's `@key` includes at least one field that's also defined in another service. Each field of an entity should be defined in exactly one service. | | `KEY_FIELDS_MISSING_EXTERNAL` | An implementing service is attempting to `extend` another service's entity, but its `@key` includes at least one field that is not marked as `@external`. | +| `KEY_MISSING_ON_BASE` | An implementing service is attempting to `extend` another service's entity, but the owning service hasn't specified a `@key` with which to extend it by. | +| `MULTIPLE_KEYS_ON_EXTENSION` | An implementing service is attempting to `extend` another service's entity, but it's specified multiple `@key` directives. Extending services may only use one of the `@key` directives specified by the owning service. | +| `KEY_NOT_SPECIFIED` | An implementing service is attempting to `extend` another service's entity, but the `@key` it's specified is invalid. Valid `@key`s are specified by the owning service. | ## `@external` From 04987f02e2977f293e1d34795148e31d2b419c3f Mon Sep 17 00:00:00 2001 From: Trevor Scheer Date: Thu, 20 Aug 2020 17:37:36 -0700 Subject: [PATCH 3/7] Update changelog --- 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 3adc90dea38..377196557f6 100644 --- a/packages/apollo-federation/CHANGELOG.md +++ b/packages/apollo-federation/CHANGELOG.md @@ -6,6 +6,7 @@ - __FIX__: CSDL complex `@key`s shouldn't result in an unparseable document [PR #4490](https://github.com/apollographql/apollo-server/pull/4490) - __FIX__: Value type validations - restrict unions, scalars, enums [PR #4496](https://github.com/apollographql/apollo-server/pull/4496) +- __FIX__: Create new @key validations to prevent invalid compositions [PR #4498](https://github.com/apollographql/apollo-server/pull/4498) ## v0.19.1 From 211715488dc9dede15ce22ecd6bd081da0d3727c Mon Sep 17 00:00:00 2001 From: Trevor Scheer Date: Fri, 21 Aug 2020 13:18:44 -0700 Subject: [PATCH 4/7] Address documentation feedback --- docs/source/federation/errors.md | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/docs/source/federation/errors.md b/docs/source/federation/errors.md index 285485c6eb4..d5a73e117e8 100644 --- a/docs/source/federation/errors.md +++ b/docs/source/federation/errors.md @@ -24,9 +24,9 @@ If Apollo Gateway encounters an error, composition fails. This document lists co | `KEY_FIELDS_SELECT_INVALID_TYPE` | The `fields` argument of an entity's `@key` includes at least one root field that results in a list, interface, or union type. Root fields of these types cannot be part of a `@key`. | | `KEY_FIELDS_MISSING_ON_BASE` | The `fields` argument of an entity's `@key` includes at least one field that's also defined in another service. Each field of an entity should be defined in exactly one service. | | `KEY_FIELDS_MISSING_EXTERNAL` | An implementing service is attempting to `extend` another service's entity, but its `@key` includes at least one field that is not marked as `@external`. | -| `KEY_MISSING_ON_BASE` | An implementing service is attempting to `extend` another service's entity, but the owning service hasn't specified a `@key` with which to extend it by. | -| `MULTIPLE_KEYS_ON_EXTENSION` | An implementing service is attempting to `extend` another service's entity, but it's specified multiple `@key` directives. Extending services may only use one of the `@key` directives specified by the owning service. | -| `KEY_NOT_SPECIFIED` | An implementing service is attempting to `extend` another service's entity, but the `@key` it's specified is invalid. Valid `@key`s are specified by the owning service. | +| `KEY_MISSING_ON_BASE` | An implementing service is attempting to `extend` another service's entity, but the originating service hasn't defined a `@key` for the entity. | +| `MULTIPLE_KEYS_ON_EXTENSION` | An implementing service is attempting to `extend` another service's entity, but it's specified multiple `@key` directives. Extending services can only use one of the `@key`s specified by the originating service. | +| `KEY_NOT_SPECIFIED` | An implementing service is attempting to `extend` another service's entity, but it is using a `@key` that is not specified in the originating service. Valid `@key`s are specified by the owning service. | ## `@external` From 92b564167e28b3fed95d004ed3ed963034bb641c Mon Sep 17 00:00:00 2001 From: Trevor Scheer Date: Fri, 21 Aug 2020 13:22:41 -0700 Subject: [PATCH 5/7] Update language to match feedback from @StephenBarlow --- .../__tests__/keysMatchBaseService.test.ts | 8 ++++---- .../validate/postComposition/keysMatchBaseService.ts | 8 ++++---- 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/packages/apollo-federation/src/composition/validate/postComposition/__tests__/keysMatchBaseService.test.ts b/packages/apollo-federation/src/composition/validate/postComposition/__tests__/keysMatchBaseService.test.ts index e8adeb60305..5c2854f64e3 100644 --- a/packages/apollo-federation/src/composition/validate/postComposition/__tests__/keysMatchBaseService.test.ts +++ b/packages/apollo-federation/src/composition/validate/postComposition/__tests__/keysMatchBaseService.test.ts @@ -38,7 +38,7 @@ describe('keysMatchBaseService', () => { expect(validationErrors).toHaveLength(0); }); - it('requires a @key to be specified on the owning type', () => { + it('requires a @key to be specified on the originating type', () => { const serviceA = { typeDefs: gql` type Product { @@ -71,12 +71,12 @@ describe('keysMatchBaseService', () => { expect(validationErrors[0]).toMatchInlineSnapshot(` Object { "code": "KEY_MISSING_ON_BASE", - "message": "[serviceA] Product -> appears to be an entity but no @key directives are specified on the owning type.", + "message": "[serviceA] Product -> appears to be an entity but no @key directives are specified on the originating type.", } `); }); - it('requires extending services to use a @key specified by the owning type', () => { + it('requires extending services to use a @key specified by the originating type', () => { const serviceA = { typeDefs: gql` type Product @key(fields: "sku upc") { @@ -109,7 +109,7 @@ describe('keysMatchBaseService', () => { expect(validationErrors[0]).toMatchInlineSnapshot(` Object { "code": "KEY_NOT_SPECIFIED", - "message": "[serviceB] Product -> extends from serviceA but specifies an invalid @key directive. Valid @key directives are specified by the owning type. Available @key directives for this type are: + "message": "[serviceB] Product -> extends from serviceA but specifies an invalid @key directive. Valid @key directives are specified by the originating type. Available @key directives for this type are: @key(fields: \\"sku upc\\")", } `); diff --git a/packages/apollo-federation/src/composition/validate/postComposition/keysMatchBaseService.ts b/packages/apollo-federation/src/composition/validate/postComposition/keysMatchBaseService.ts index c377e73c264..b605c9df8e1 100644 --- a/packages/apollo-federation/src/composition/validate/postComposition/keysMatchBaseService.ts +++ b/packages/apollo-federation/src/composition/validate/postComposition/keysMatchBaseService.ts @@ -8,9 +8,9 @@ import { PostCompositionValidator } from '.'; import { printWithReducedWhitespace } from '../../../service'; /** - * 1. KEY_MISSING_ON_BASE - Owning types must specify at least 1 @key directive + * 1. KEY_MISSING_ON_BASE - Originating types must specify at least 1 @key directive * 2. MULTIPLE_KEYS_ON_EXTENSION - Extending services may not use more than 1 @key directive - * 3. KEY_NOT_SPECIFIED - Extending services must use a valid @key specified by the owning type + * 3. KEY_NOT_SPECIFIED - Extending services must use a valid @key specified by the originating type */ export const keysMatchBaseService: PostCompositionValidator = function ({ schema, @@ -32,7 +32,7 @@ export const keysMatchBaseService: PostCompositionValidator = function ({ errorWithCode( 'KEY_MISSING_ON_BASE', logServiceAndType(serviceName, parentTypeName) + - `appears to be an entity but no @key directives are specified on the owning type.`, + `appears to be an entity but no @key directives are specified on the originating type.`, ), ); continue; @@ -62,7 +62,7 @@ export const keysMatchBaseService: PostCompositionValidator = function ({ errorWithCode( 'KEY_NOT_SPECIFIED', logServiceAndType(extendingService, parentTypeName) + - `extends from ${serviceName} but specifies an invalid @key directive. Valid @key directives are specified by the owning type. Available @key directives for this type are:\n` + + `extends from ${serviceName} but specifies an invalid @key directive. Valid @key directives are specified by the originating type. Available @key directives for this type are:\n` + `\t${availableKeys .map((fieldSet) => `@key(fields: "${fieldSet}")`) .join('\n\t')}`, From d53d3cb160187725da5a7ddfcb429028761204e9 Mon Sep 17 00:00:00 2001 From: Trevor Scheer Date: Fri, 21 Aug 2020 13:27:00 -0700 Subject: [PATCH 6/7] Update packages/apollo-federation/CHANGELOG.md Co-authored-by: Jesse Rosenberger --- packages/apollo-federation/CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/apollo-federation/CHANGELOG.md b/packages/apollo-federation/CHANGELOG.md index 377196557f6..42f4b2b4998 100644 --- a/packages/apollo-federation/CHANGELOG.md +++ b/packages/apollo-federation/CHANGELOG.md @@ -6,7 +6,7 @@ - __FIX__: CSDL complex `@key`s shouldn't result in an unparseable document [PR #4490](https://github.com/apollographql/apollo-server/pull/4490) - __FIX__: Value type validations - restrict unions, scalars, enums [PR #4496](https://github.com/apollographql/apollo-server/pull/4496) -- __FIX__: Create new @key validations to prevent invalid compositions [PR #4498](https://github.com/apollographql/apollo-server/pull/4498) +- __FIX__: Create new `@key` validations to prevent invalid compositions [PR #4498](https://github.com/apollographql/apollo-server/pull/4498) ## v0.19.1 From 6b05c082d36e06a2d37bb1db5adf0e8ca9195158 Mon Sep 17 00:00:00 2001 From: Trevor Scheer Date: Tue, 25 Aug 2020 11:40:24 -0700 Subject: [PATCH 7/7] Add comment about KEY_NOT_SPECIFIED validation --- .../validate/postComposition/keysMatchBaseService.ts | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/packages/apollo-federation/src/composition/validate/postComposition/keysMatchBaseService.ts b/packages/apollo-federation/src/composition/validate/postComposition/keysMatchBaseService.ts index b605c9df8e1..62e4cc57592 100644 --- a/packages/apollo-federation/src/composition/validate/postComposition/keysMatchBaseService.ts +++ b/packages/apollo-federation/src/composition/validate/postComposition/keysMatchBaseService.ts @@ -39,9 +39,8 @@ export const keysMatchBaseService: PostCompositionValidator = function ({ } const availableKeys = keys[serviceName].map(printFieldSet); - - // No need to validate that the owning service matches its specified keys Object.entries(keys) + // No need to validate that the owning service matches its specified keys .filter(([service]) => service !== serviceName) .forEach(([extendingService, keyFields]) => { // Extensions can't specify more than one key @@ -56,6 +55,10 @@ export const keysMatchBaseService: PostCompositionValidator = function ({ return; } + // This isn't representative of an invalid graph, but it is an existing + // limitation of the query planner that we want to validate against for now. + // In the future, `@key`s just need to be "reachable" through a number of + // services which can link one key to another via "joins". const extensionKey = printFieldSet(keyFields[0]); if (!availableKeys.includes(extensionKey)) { errors.push(