Initial clean up and refactor

* Clean up artifacts of previous impl * Begin refactor by preserving serviceNames on types rather than keeping only the latest
apollographql · Aug 22, 2019 · f6bc588 · f6bc588
1 parent 0398069
commit f6bc588
Show file tree

Hide file tree

Showing 8 changed files with 79 additions and 66 deletions.
diff --git a/packages/apollo-federation/src/composition/__tests__/compose.test.ts b/packages/apollo-federation/src/composition/__tests__/compose.test.ts
@@ -1,4 +1,4 @@
-import { GraphQLObjectType, GraphQLEnumType } from 'graphql';
+import { GraphQLObjectType } from 'graphql';
 import gql from 'graphql-tag';
 import { composeServices } from '../compose';
 import {
@@ -7,6 +7,7 @@ import {
   selectionSetSerializer,
 } from '../../snapshotSerializers';
 import { normalizeTypeDefs } from '../normalize';
+import { getOwningService } from '../utils';
 
 expect.addSnapshotSerializer(astSerializer);
 expect.addSnapshotSerializer(typeSerializer);
@@ -55,8 +56,8 @@ describe('composeServices', () => {
     const product = schema.getType('Product') as GraphQLObjectType;
     const user = schema.getType('User') as GraphQLObjectType;
 
-    expect(product.federation.serviceName).toEqual('serviceA');
-    expect(user.federation.serviceName).toEqual('serviceB');
+    expect(getOwningService(product)).toEqual('serviceA');
+    expect(getOwningService(user)).toEqual('serviceB');
   });
 
   describe('basic type extensions', () => {
@@ -94,7 +95,7 @@ describe('composeServices', () => {
 
       const product = schema.getType('Product') as GraphQLObjectType;
 
-      expect(product.federation.serviceName).toEqual('serviceA');
+      expect(getOwningService(product)).toEqual('serviceA');
       expect(product.getFields()['price'].federation.serviceName).toEqual(
         'serviceB',
       );
@@ -133,7 +134,7 @@ describe('composeServices', () => {
 
       const product = schema.getType('Product') as GraphQLObjectType;
 
-      expect(product.federation.serviceName).toEqual('serviceB');
+      expect(getOwningService(product)).toEqual('serviceB');
       expect(product.getFields()['price'].federation.serviceName).toEqual(
         'serviceA',
       );
@@ -187,7 +188,7 @@ describe('composeServices', () => {
 
       const product = schema.getType('Product') as GraphQLObjectType;
 
-      expect(product.federation.serviceName).toEqual('serviceB');
+      expect(getOwningService(product)).toEqual('serviceB');
       expect(product.getFields()['price'].federation.serviceName).toEqual(
         'serviceA',
       );
@@ -248,7 +249,7 @@ describe('composeServices', () => {
                         }
                   `);
 
-      expect(product.federation.serviceName).toEqual('serviceB');
+      expect(getOwningService(product)).toEqual('serviceB');
       expect(product.getFields()['price'].federation.serviceName).toEqual(
         'serviceC',
       );
@@ -574,7 +575,7 @@ describe('composeServices', () => {
 
       const product = schema.getType('Product') as GraphQLObjectType;
 
-      expect(product.federation.serviceName).toEqual('serviceA');
+      expect(getOwningService(product)).toEqual('serviceA');
       expect(product.getFields()['id'].federation.serviceName).toEqual(
         'serviceB',
       );
@@ -614,7 +615,7 @@ describe('composeServices', () => {
 
       const query = schema.getQueryType();
 
-      expect(query.federation.serviceName).toBeUndefined();
+      expect(getOwningService(query)).toBeUndefined();
     });
 
     it('treats root Query type definition as an extension, not base definitions', () => {
@@ -655,7 +656,7 @@ describe('composeServices', () => {
 
       const query = schema.getType('Query') as GraphQLObjectType;
 
-      expect(query.federation.serviceName).toBeUndefined();
+      expect(getOwningService(query)).toBeUndefined();
     });
 
     it('allows extension of the Mutation type with no base type definition', () => {
@@ -846,7 +847,7 @@ describe('composeServices', () => {
         expect(product.getFields()['price'].federation.serviceName).toEqual(
           'serviceB',
         );
-        expect(product.federation.serviceName).toEqual('serviceA');
+        expect(getOwningService(product)).toEqual('serviceA');
       });
     });
 

diff --git a/packages/apollo-federation/src/composition/compose.ts b/packages/apollo-federation/src/composition/compose.ts
@@ -27,7 +27,6 @@ import {
   parseSelections,
   mapFieldNamesToServiceName,
   stripExternalFieldsFromTypeDefs,
-  diffTypeNodes,
 } from './utils';
 import {
   ServiceDefinition,
@@ -80,7 +79,7 @@ interface ExtensionsMap {
  */
 interface TypeToServiceMap {
   [typeName: string]: {
-    serviceName?: ServiceName;
+    serviceNames?: ServiceName[];
     extensionFieldsToOwningServiceMap: { [fieldName: string]: string };
   };
 }
@@ -158,10 +157,13 @@ export function buildMapsFromServiceList(serviceList: ServiceDefinition[]) {
          * 2. It was extended by a service before declared
          */
         if (typeToServiceMap[typeName]) {
-          typeToServiceMap[typeName].serviceName = serviceName;
+          typeToServiceMap[typeName].serviceNames = [
+            ...(typeToServiceMap[typeName].serviceNames || []),
+            serviceName,
+          ];
         } else {
           typeToServiceMap[typeName] = {
-            serviceName,
+            serviceNames: [serviceName],
             extensionFieldsToOwningServiceMap: Object.create(null),
           };
         }
@@ -314,7 +316,7 @@ export function addFederationMetadataToSchemaNodes({
 }) {
   for (const [
     typeName,
-    { serviceName: baseServiceName, extensionFieldsToOwningServiceMap },
+    { serviceNames, extensionFieldsToOwningServiceMap },
   ] of Object.entries(typeToServiceMap)) {
     const namedType = schema.getType(typeName) as GraphQLNamedType;
     if (!namedType) continue;
@@ -323,7 +325,7 @@ export function addFederationMetadataToSchemaNodes({
     // and the key directives that belong to it
     namedType.federation = {
       ...namedType.federation,
-      serviceName: baseServiceName,
+      serviceNames,
       ...(keyDirectivesMap[typeName] && {
         keys: keyDirectivesMap[typeName],
       }),
@@ -344,7 +346,7 @@ export function addFederationMetadataToSchemaNodes({
         ) {
           field.federation = {
             ...field.federation,
-            serviceName: baseServiceName,
+            serviceName: serviceNames && serviceNames[serviceNames.length - 1],
             provides: parseSelections(
               providesDirective.arguments[0].value.value,
             ),
@@ -420,38 +422,6 @@ export function composeServices(services: ServiceDefinition[]) {
     keyDirectivesMap,
   } = buildMapsFromServiceList(services);
 
-  for (const [typeName, definitions] of Object.entries(definitionsMap)) {
-    const typeIsValueType =
-      definitions.length > 1 &&
-      definitions.every((definition, index) => {
-        if (findDirectivesOnTypeOrField(definition, 'key').length > 0) {
-          return false;
-        }
-        if (index === 0) {
-          return true;
-        }
-
-        const { name, kind, fields, unionTypes } = diffTypeNodes(
-          definition,
-          definitions[index - 1],
-        );
-
-        return (
-          name.length === 0 &&
-          kind.length === 0 &&
-          Object.keys(fields).length === 0 &&
-          Object.keys(unionTypes).length === 0
-        );
-      });
-
-    if (typeIsValueType) {
-      definitionsMap[typeName] = [
-        { ...definitionsMap[typeName][0], serviceName: null },
-      ];
-      typeToServiceMap[typeName].serviceName = null;
-    }
-  }
-
   let { schema, errors } = buildSchemaFromDefinitionsAndExtensions({
     definitionsMap,
     extensionsMap,

diff --git a/packages/apollo-federation/src/composition/types.ts b/packages/apollo-federation/src/composition/types.ts
@@ -18,7 +18,7 @@ export interface ServiceNameToKeyDirectivesMap {
 }
 
 export interface FederationType {
-  serviceName?: ServiceName;
+  serviceNames?: ServiceName[];
   keys?: ServiceNameToKeyDirectivesMap;
   externals?: {
     [serviceName: string]: ExternalFieldDefinition[];

diff --git a/packages/apollo-federation/src/composition/utils.ts b/packages/apollo-federation/src/composition/utils.ts
@@ -181,6 +181,12 @@ export function errorWithCode(
   );
 }
 
+export function getOwningService(type: GraphQLNamedType) {
+  return type.federation && type.federation.serviceNames
+    ? type.federation.serviceNames[type.federation.serviceNames.length - 1]
+    : undefined;
+}
+
 export function findTypesContainingFieldWithReturnType(
   schema: GraphQLSchema,
   node: GraphQLField<any, any>,

diff --git a/packages/apollo-federation/src/composition/validate/postComposition/externalMissingOnBase.ts b/packages/apollo-federation/src/composition/validate/postComposition/externalMissingOnBase.ts
@@ -1,7 +1,11 @@
 import 'apollo-server-env';
 import { GraphQLSchema, isObjectType, GraphQLError } from 'graphql';
 
-import { logServiceAndType, errorWithCode } from '../../utils';
+import {
+  logServiceAndType,
+  errorWithCode,
+  getOwningService,
+} from '../../utils';
 
 /**
  * All fields marked with @external must exist on the base type
@@ -33,7 +37,9 @@ export const externalMissingOnBase = (schema: GraphQLSchema) => {
               errorWithCode(
                 'EXTERNAL_MISSING_ON_BASE',
                 logServiceAndType(serviceName, typeName, externalFieldName) +
-                  `marked @external but ${externalFieldName} is not defined on the base service of ${typeName} (${namedType.federation.serviceName})`,
+                  `marked @external but ${externalFieldName} is not defined on the base service of ${typeName} (${getOwningService(
+                    namedType,
+                  )})`,
               ),
             );
             continue;
@@ -49,7 +55,11 @@ export const externalMissingOnBase = (schema: GraphQLSchema) => {
               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})`,
+                  `marked @external but ${externalFieldName} was defined in ${
+                    matchingBaseField.federation.serviceName
+                  }, not in the service that owns ${typeName} (${getOwningService(
+                    namedType,
+                  )})`,
               ),
             );
           }

diff --git a/packages/apollo-federation/src/composition/validate/postComposition/externalTypeMismatch.ts b/packages/apollo-federation/src/composition/validate/postComposition/externalTypeMismatch.ts
@@ -5,7 +5,11 @@ import {
   isEqualType,
   GraphQLError,
 } from 'graphql';
-import { logServiceAndType, errorWithCode } from '../../utils';
+import {
+  logServiceAndType,
+  errorWithCode,
+  getOwningService,
+} from '../../utils';
 
 /**
  * All fields marked with @external must match the type definition of the base service.
@@ -56,7 +60,11 @@ export const externalTypeMismatch = (schema: GraphQLSchema) => {
               errorWithCode(
                 'EXTERNAL_TYPE_MISMATCH',
                 logServiceAndType(serviceName, typeName, externalFieldName) +
-                  `Type \`${externalFieldType.name}\` does not match the type of the original field in ${namedType.federation.serviceName} (\`${matchingBaseField.type}\`)`,
+                  `Type \`${
+                    externalFieldType.name
+                  }\` does not match the type of the original field in ${getOwningService(
+                    namedType,
+                  )} (\`${matchingBaseField.type}\`)`,
               ),
             );
           }

diff --git a/packages/apollo-gateway/src/__tests__/integration/value-types.test.ts b/packages/apollo-gateway/src/__tests__/integration/value-types.test.ts
@@ -35,21 +35,21 @@ describe('value types', () => {
       }
     `;
 
-    const { data, errors, queryPlan } = await execute(
+    const { data, errors } = await execute(
       [accounts, books, inventory, product, reviews],
       {
         query,
       },
     );
 
     expect(errors).toBeUndefined();
-    expect(data.topProducts[0].upc).toEqual('1');
-    expect(data.topProducts[0].metadata[0]).toEqual({
+    expect(data!.topProducts[0].upc).toEqual('1');
+    expect(data!.topProducts[0].metadata[0]).toEqual({
       key: 'Condition',
       value: 'excellent',
     });
-    expect(data.topProducts[4].upc).toEqual('0136291554');
-    expect(data.topProducts[4].metadata[0]).toEqual({
+    expect(data!.topProducts[4].upc).toEqual('0136291554');
+    expect(data!.topProducts[4].metadata[0]).toEqual({
       key: 'Condition',
       value: 'used',
     });

diff --git a/packages/apollo-gateway/src/buildQueryPlan.ts b/packages/apollo-gateway/src/buildQueryPlan.ts
@@ -253,12 +253,23 @@ function splitSubfields(
   splitFields(context, path, fields, field => {
     const { parentType, fieldNode, fieldDef } = field;
 
-    const owningService =
-      context.getOwningService(parentType, fieldDef) || parentGroup.serviceName;
+    const baseService = context.getBaseService(parentType);
 
-    const baseService =
-      context.getBaseService(parentType) || parentGroup.serviceName;
+    if (!baseService) {
+      throw new GraphQLError(
+        `Couldn't find base service for type "${parentType.name}"`,
+        fieldNode,
+      );
+    }
 
+    const owningService = context.getOwningService(parentType, fieldDef);
+
+    if (!owningService) {
+      throw new GraphQLError(
+        `Couldn't find owning service for field "${parentType.name}.${fieldDef.name}"`,
+        fieldNode,
+      );
+    }
     // Is the field defined on the base service?
     if (owningService === baseService) {
       // Can we fetch the field from the parent group?
@@ -735,7 +746,14 @@ export class QueryPlanningContext {
   }
 
   getBaseService(parentType: GraphQLObjectType): string | null {
-    return (parentType.federation && parentType.federation.serviceName) || null;
+    return (
+      (parentType.federation &&
+        parentType.federation.serviceNames &&
+        parentType.federation.serviceNames[
+          parentType.federation.serviceNames.length - 1
+        ]) ||
+      null
+    );
   }
 
   getOwningService(