Error out if differing interface field implementations may lead to la…

…ter issue

This patch detect the cases that would lead to the runtime issue
described on apollographql#1257 and reject composition. That this, this is not a fix
for apollographql#1257 but it ensures that, until we get to fix that issue properly,
we at least error out earlier and with a more helpful error.

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 : '<schema>',
           astNodes
         ));

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 |  |

gateway-js/CHANGELOG.md

@@ -4,7 +4,7 @@
 
 ## vNEXT
 
-- _Nothing yet! Stay tuned._
+- Reject mismatching types for interface field implementation if some of those implementations are `@external`, since this can lead to invalid subgraph queries at runtime [PR #1318](https://github.com/apollographql/federation/pull/1318). This limitation should be lifted in the future once the root cause (the invalid runtime queries) is fixed by issue [#1257](https://github.com/apollographql/federation/issues/1257).
 
 ## v2.0.0-alpha.4
 

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!".'],
+    ]);
+})

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,
 };
 

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,52 @@ function isFieldSatisfyingInterface(field: FieldDefinition<ObjectType | Interfac
   return field.parent.interfaces().some(itf => itf.field(field.name));
 }
 
+/**
+ * Register errors when, for an interface field, some of the implementations of that field are @external
+ * _and_ not all of those field implementation have the same type (which otherwise allowed because field
+ * implementation types can be a subtype of the interface field they implement).
+ * This is done because if that is the case, federation may later generate invalid query plans (see details
+ * on https://github.com/apollographql/federation/issues/1257).
+ * This "limitation" will be removed when we stop generating invalid query plans for it.
+ */
+function validateInterfaceRuntimeImplementationFieldsTypes(
+  itf: InterfaceType,
+  externalTester: ExternalTester, 
+  errorCollector: GraphQLError[],
+): void {
+  const runtimeTypes = itf.possibleRuntimeTypes();
+  for (const field of itf.fields()) {
+    const withExternalOrRequires: FieldDefinition<ObjectType>[] = [];
+    const typeToImplems: MultiMap<string, FieldDefinition<ObjectType>> = 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<CompositeType>): string => `"${f.coordinate}"`;
+
+function formatFieldsToReturnType([type, implems]: [string, FieldDefinition<ObjectType>[]]) {
+  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 +553,10 @@ export class FederationBuiltIns extends BuiltIns {
       }
     }
 
+    for (const itf of schema.types<InterfaceType>('InterfaceType')) {
+      validateInterfaceRuntimeImplementationFieldsTypes(itf, externalTester, errors);
+    }
+
     return errors;
   }
 

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;
+}