Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(federation): Create new @key validations #4498

Merged
merged 9 commits into from
Aug 25, 2020
3 changes: 3 additions & 0 deletions docs/source/federation/errors.md
Original file line number Diff line number Diff line change
Expand Up @@ -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 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`

Expand Down
1 change: 1 addition & 0 deletions packages/apollo-federation/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,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__: Composition - aggregate interfaces for types and interfaces in composed schema [PR #4497](https://github.com/apollographql/apollo-server/pull/4497)
- __FIX__: Create new `@key` validations to prevent invalid compositions [PR #4498](https://github.com/apollographql/apollo-server/pull/4498)

## v0.19.1

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
Original file line number Diff line number Diff line change
@@ -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 originating 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 originating type.",
}
`);
});

it('requires extending services to use a @key specified by the originating 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 originating type. Available @key directives for this type are:
@key(fields: \\"sku upc\\")",
abernix marked this conversation as resolved.
Show resolved Hide resolved
}
`);
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ export {
executableDirectivesInAllServices,
} from './executableDirectivesInAllServices';
export { executableDirectivesIdentical } from './executableDirectivesIdentical';
export { keysMatchBaseService } from './keysMatchBaseService';

export type PostCompositionValidator = ({
schema,
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,86 @@
import { isObjectType, GraphQLError, SelectionNode } from 'graphql';
import {
logServiceAndType,
errorWithCode,
getFederationMetadata,
} from '../../utils';
import { PostCompositionValidator } from '.';
import { printWithReducedWhitespace } from '../../../service';

/**
* 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 originating 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 originating type.`,
),
);
continue;
}

const availableKeys = keys[serviceName].map(printFieldSet);
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
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;
}

// 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(
errorWithCode(
'KEY_NOT_SPECIFIED',
logServiceAndType(extendingService, parentTypeName) +
`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')}`,
),
);
return;
}
});
}
}
}

return errors;
};

function printFieldSet(selections: readonly SelectionNode[]): string {
return selections.map(printWithReducedWhitespace).join(' ');
}