diff --git a/composition-js/src/merging/merge.ts b/composition-js/src/merging/merge.ts index 1d6b78022..8db11f371 100644 --- a/composition-js/src/merging/merge.ts +++ b/composition-js/src/merging/merge.ts @@ -54,6 +54,7 @@ import { sourceASTs, ErrorCodeDefinition, ERRORS, + joinStrings, } from "@apollo/federation-internals"; import { ASTNode, GraphQLError, DirectiveLocation } from "graphql"; import { @@ -127,21 +128,6 @@ export function mergeSubgraphs(subgraphs: Subgraphs, options: CompositionOptions return new Merger(subgraphs, { ...defaultCompositionOptions, ...options }).merge(); } -function join(toJoin: string[], sep: string = ', ', firstSep?: string, lastSep: string = ' and ') { - if (toJoin.length == 0) { - return ''; - } - const first = toJoin[0]; - if (toJoin.length == 1) { - return first; - } - const last = toJoin[toJoin.length - 1]; - if (toJoin.length == 2) { - return first + (firstSep ? firstSep : lastSep) + last; - } - return first + (firstSep ? firstSep : sep) + toJoin.slice(1, toJoin.length - 1) + lastSep + last; -} - function printHumanReadableList(names: string[], prefixSingle?: string, prefixPlural?: string): string { assert(names.length > 0, 'Should not have been called with no names'); if (names.length == 1) { @@ -158,7 +144,7 @@ function printHumanReadableList(names: string[], prefixSingle?: string, prefixPl ? prefixPlural + ' ' : (prefixSingle ? prefixSingle + ' ' : ''); if (toDisplay.length === names.length) { - return prefix + join(toDisplay); + return prefix + joinStrings(toDisplay); } else { return prefix + toDisplay.join(', ') + ', ...'; } @@ -447,7 +433,7 @@ class Merger { (elt, names) => `${elt} in ${names}`, (distribution, nodes) => { this.errors.push(code.err({ - message: message + join(distribution, ' and ', ' but '), + message: message + joinStrings(distribution, ' and ', ' but '), nodes })); }, @@ -474,7 +460,7 @@ class Merger { otherElementsPrinter, (distribution, nodes) => { this.errors.push(code.err({ - message: message + distribution[0] + join(distribution.slice(1), ' and '), + message: message + distribution[0] + joinStrings(distribution.slice(1), ' and '), nodes })); }, @@ -504,7 +490,7 @@ class Merger { (distribution, astNodes) => { this.hints.push(new CompositionHint( hintId, - message + distribution[0] + join(distribution.slice(1), ' and ') + (noEndOfMessageDot ? '' : '.'), + message + distribution[0] + joinStrings(distribution.slice(1), ' and ') + (noEndOfMessageDot ? '' : '.'), supergraphElement instanceof NamedSchemaElement ? supergraphElement.coordinate : '', astNodes )); diff --git a/docs/source/errors.md b/docs/source/errors.md index f7e834b25..823060553 100644 --- a/docs/source/errors.md +++ b/docs/source/errors.md @@ -29,6 +29,7 @@ The following errors may be raised by composition: | `FIELD_ARGUMENT_TYPE_MISMATCH` | An argument (of a field/directive) has a type that is incompatible with that of other declarations of that same argument in other subgraphs. | 2.0.0 | Replaces: `VALUE_TYPE_INPUT_VALUE_MISMATCH` | | `FIELD_TYPE_MISMATCH` | A field has a type that is incompatible with other declarations of that field in other subgraphs. | 2.0.0 | Replaces: `VALUE_TYPE_FIELD_TYPE_MISMATCH` | | `INPUT_FIELD_DEFAULT_MISMATCH` | An input field has a default value that is incompatible with other declarations of that field in other subgraphs. | 2.0.0 | | +| `INTERFACE_FIELD_IMPLEM_TYPE_MISMATCH` | For an interface field, some of its concrete implementations have @external or @requires and there is difference in those implementations return type (which is currently not supported; see https://github.com/apollographql/federation/issues/1257) | 2.0.0 | | | `INTERFACE_FIELD_NO_IMPLEM` | After subgraph merging, an implemenation is missing a field of one of the interface it implements (which can happen for valid subgraphs). | 2.0.0 | | | `INVALID_GRAPHQL` | A schema is invalid GraphQL: it violates one of the rule of the specification. | 2.0.0 | | | `INVALID_SUBGRAPH_NAME` | A subgraph name is invalid (subgraph names cannot be a single underscore ("_")). | 2.0.0 | | diff --git a/internals-js/src/__tests__/subgraphValidation.test.ts b/internals-js/src/__tests__/subgraphValidation.test.ts index b4f958fd3..4c91af1f3 100644 --- a/internals-js/src/__tests__/subgraphValidation.test.ts +++ b/internals-js/src/__tests__/subgraphValidation.test.ts @@ -450,3 +450,31 @@ describe('root types', () => { }); }); + +it('validates all implementations of interface field have same type if any has @external', () => { + const subgraph = gql` + type Query { + is: [I!]! + } + + interface I { + f: Int + } + + type T1 implements I { + f: Int + } + + type T2 implements I { + f: Int! + } + + type T3 implements I { + id: ID! + f: Int @external + } + `; + expect(buildForErrors(subgraph)).toStrictEqual([ + ['INTERFACE_FIELD_IMPLEM_TYPE_MISMATCH', '[S] Some of the runtime implementations of interface field "I.f" are marked @external or have a @require ("T3.f") so all the implementations should use the same type (a current limitation of federation; see https://github.com/apollographql/federation/issues/1257), but "T1.f" and "T3.f" have type "Int" while "T2.f" has type "Int!".'], + ]); +}) diff --git a/internals-js/src/error.ts b/internals-js/src/error.ts index a3b516ab5..884a6a9eb 100644 --- a/internals-js/src/error.ts +++ b/internals-js/src/error.ts @@ -264,6 +264,11 @@ const EXTERNAL_MISSING_ON_BASE = makeCodeDefinition( { addedIn: FED1_CODE }, ); +const INTERFACE_FIELD_IMPLEM_TYPE_MISMATCH = makeCodeDefinition( + 'INTERFACE_FIELD_IMPLEM_TYPE_MISMATCH', + 'For an interface field, some of its concrete implementations have @external or @requires and there is difference in those implementations return type (which is currently not supported; see https://github.com/apollographql/federation/issues/1257)' +); + const SATISFIABILITY_ERROR = makeCodeDefinition( 'SATISFIABILITY_ERROR', 'Subgraphs can be merged, but the resulting supergraph API would have queries that cannot be satisfied by those subgraphs.', @@ -315,6 +320,7 @@ export const ERRORS = { ARGUMENT_DEFAULT_MISMATCH, EXTENSION_WITH_NO_BASE, EXTERNAL_MISSING_ON_BASE, + INTERFACE_FIELD_IMPLEM_TYPE_MISMATCH, SATISFIABILITY_ERROR, }; diff --git a/internals-js/src/federation.ts b/internals-js/src/federation.ts index 667f0e7fe..2fdaee1ad 100644 --- a/internals-js/src/federation.ts +++ b/internals-js/src/federation.ts @@ -26,7 +26,7 @@ import { InputFieldDefinition, isCompositeType } from "./definitions"; -import { assert, OrderedMap } from "./utils"; +import { assert, joinStrings, MultiMap, OrderedMap } from "./utils"; import { SDLValidationRule } from "graphql/validation/ValidationContext"; import { specifiedSDLRules } from "graphql/validation/specifiedRules"; import { @@ -49,9 +49,9 @@ import { tagLocations, TAG_VERSIONS } from "./tagSpec"; import { errorCodeDef, ErrorCodeDefinition, + ERROR_CATEGORIES, ERRORS, } from "./error"; -import { ERROR_CATEGORIES } from "."; export const entityTypeName = '_Entity'; export const serviceTypeName = '_Service'; @@ -311,6 +311,44 @@ function isFieldSatisfyingInterface(field: FieldDefinition itf.field(field.name)); } +function validateInterfaceRuntimeImplementationFieldsTypes( + itf: InterfaceType, + externalTester: ExternalTester, + errorCollector: GraphQLError[], +): void { + const runtimeTypes = itf.possibleRuntimeTypes(); + for (const field of itf.fields()) { + const withExternalOrRequires: FieldDefinition[] = []; + const typeToImplems: MultiMap> = new MultiMap(); + const nodes: ASTNode[] = []; + for (const type of runtimeTypes) { + const implemField = type.field(field.name); + if (!implemField) continue; + if (implemField.sourceAST) { + nodes.push(implemField.sourceAST); + } + if (externalTester.isExternal(implemField) || implemField.hasAppliedDirective(requiresDirectiveName)) { + withExternalOrRequires.push(implemField); + } + const returnType = implemField.type!; + typeToImplems.add(returnType.toString(), implemField); + } + if (withExternalOrRequires.length > 0 && typeToImplems.size > 1) { + const typeToImplemsArray = [...typeToImplems.entries()]; + errorCollector.push(ERRORS.INTERFACE_FIELD_IMPLEM_TYPE_MISMATCH.err({ + message: `Some of the runtime implementations of interface field "${field.coordinate}" are marked @external or have a @require (${withExternalOrRequires.map(printFieldCoordinate)}) so all the implementations should use the same type (a current limitation of federation; see https://github.com/apollographql/federation/issues/1257), but ${formatFieldsToReturnType(typeToImplemsArray[0])} while ${joinStrings(typeToImplemsArray.slice(1).map(formatFieldsToReturnType), ' and ')}.`, + nodes + })); + } + } +} + +const printFieldCoordinate = (f: FieldDefinition): string => `"${f.coordinate}"`; + +function formatFieldsToReturnType([type, implems]: [string, FieldDefinition[]]) { + return `${joinStrings(implems.map(printFieldCoordinate))} ${implems.length == 1 ? 'has' : 'have'} type "${type}"`; +} + export class FederationBuiltIns extends BuiltIns { addBuiltInTypes(schema: Schema) { super.addBuiltInTypes(schema); @@ -507,6 +545,10 @@ export class FederationBuiltIns extends BuiltIns { } } + for (const itf of schema.types('InterfaceType')) { + validateInterfaceRuntimeImplementationFieldsTypes(itf, externalTester, errors); + } + return errors; } diff --git a/internals-js/src/utils.ts b/internals-js/src/utils.ts index 74d47bfb7..376eaf66f 100644 --- a/internals-js/src/utils.ts +++ b/internals-js/src/utils.ts @@ -269,3 +269,25 @@ export function validateStringContainsBoolean(str?: string) : boolean | undefine return undefined; } } + +/** + * Joins an array of string, much like `Array.prototype.join`, but with the ability to use a specific different + * separator for the first and/or last occurence. + * + * The goal is to make reading flow slightly better. For instance, if you have a list of subgraphs `s = ["A", "B", "C"]`, + * then `"subgraphs " + joinString(s)` will yield "subgraphs A, B and C". + */ +export function joinStrings(toJoin: string[], sep: string = ', ', firstSep?: string, lastSep: string = ' and ') { + if (toJoin.length == 0) { + return ''; + } + const first = toJoin[0]; + if (toJoin.length == 1) { + return first; + } + const last = toJoin[toJoin.length - 1]; + if (toJoin.length == 2) { + return first + (firstSep ? firstSep : lastSep) + last; + } + return first + (firstSep ? firstSep : sep) + toJoin.slice(1, toJoin.length - 1) + lastSep + last; +}