Address serviceName on field.federation object

With value types, it doesn't make sense for a since service to own the fields on the type. Update the types and implementation to reflect this. This change surfaced a potential bug, so I added a couple tests to ensure things work and resolve as expected when @provides directives are used on fields belonging to a value type.
apollographql · Aug 27, 2019 · 053d1a3 · 053d1a3
1 parent a019e43
commit 053d1a3
Show file tree

Hide file tree

Showing 8 changed files with 273 additions and 31 deletions.
diff --git a/packages/apollo-federation/src/composition/__tests__/compose.test.ts b/packages/apollo-federation/src/composition/__tests__/compose.test.ts
@@ -55,8 +55,8 @@ describe('composeServices', () => {
     const product = schema.getType('Product') as GraphQLObjectType;
     const user = schema.getType('User') as GraphQLObjectType;
 
-    expect(product.federation.owningService).toEqual('serviceA');
-    expect(user.federation.owningService).toEqual('serviceB');
+    expect(product.federation.serviceName).toEqual('serviceA');
+    expect(user.federation.serviceName).toEqual('serviceB');
   });
 
   describe('basic type extensions', () => {
@@ -94,7 +94,7 @@ describe('composeServices', () => {
 
       const product = schema.getType('Product') as GraphQLObjectType;
 
-      expect(product.federation.owningService).toEqual('serviceA');
+      expect(product.federation.serviceName).toEqual('serviceA');
       expect(product.getFields()['price'].federation.serviceName).toEqual(
         'serviceB',
       );
@@ -133,7 +133,7 @@ describe('composeServices', () => {
 
       const product = schema.getType('Product') as GraphQLObjectType;
 
-      expect(product.federation.owningService).toEqual('serviceB');
+      expect(product.federation.serviceName).toEqual('serviceB');
       expect(product.getFields()['price'].federation.serviceName).toEqual(
         'serviceA',
       );
@@ -187,7 +187,7 @@ describe('composeServices', () => {
 
       const product = schema.getType('Product') as GraphQLObjectType;
 
-      expect(product.federation.owningService).toEqual('serviceB');
+      expect(product.federation.serviceName).toEqual('serviceB');
       expect(product.getFields()['price'].federation.serviceName).toEqual(
         'serviceA',
       );
@@ -248,7 +248,7 @@ describe('composeServices', () => {
                         }
                   `);
 
-      expect(product.federation.owningService).toEqual('serviceB');
+      expect(product.federation.serviceName).toEqual('serviceB');
       expect(product.getFields()['price'].federation.serviceName).toEqual(
         'serviceC',
       );
@@ -574,7 +574,7 @@ describe('composeServices', () => {
 
       const product = schema.getType('Product') as GraphQLObjectType;
 
-      expect(product.federation.owningService).toEqual('serviceA');
+      expect(product.federation.serviceName).toEqual('serviceA');
       expect(product.getFields()['id'].federation.serviceName).toEqual(
         'serviceB',
       );
@@ -614,7 +614,7 @@ describe('composeServices', () => {
 
       const query = schema.getQueryType();
 
-      expect(query.federation.owningService).toBeUndefined();
+      expect(query.federation.serviceName).toBeUndefined();
     });
 
     it('treats root Query type definition as an extension, not base definitions', () => {
@@ -655,7 +655,7 @@ describe('composeServices', () => {
 
       const query = schema.getType('Query') as GraphQLObjectType;
 
-      expect(query.federation.owningService).toBeUndefined();
+      expect(query.federation.serviceName).toBeUndefined();
     });
 
     it('allows extension of the Mutation type with no base type definition', () => {
@@ -846,7 +846,7 @@ describe('composeServices', () => {
         expect(product.getFields()['price'].federation.serviceName).toEqual(
           'serviceB',
         );
-        expect(product.federation.owningService).toEqual('serviceA');
+        expect(product.federation.serviceName).toEqual('serviceA');
       });
     });
 
@@ -950,11 +950,12 @@ describe('composeServices', () => {
 
         const review = schema.getType('Review') as GraphQLObjectType;
         expect(review.getFields()['product'].federation).toMatchInlineSnapshot(`
-                              Object {
-                                "provides": sku,
-                                "serviceName": "serviceA",
-                              }
-                        `);
+          Object {
+            "belongsToValueType": false,
+            "provides": sku,
+            "serviceName": "serviceA",
+          }
+        `);
       });
 
       it('adds @provides information to fields using a nested field set', () => {
@@ -1030,11 +1031,56 @@ describe('composeServices', () => {
         const review = schema.getType('Review') as GraphQLObjectType;
         expect(review.getFields()['products'].federation)
           .toMatchInlineSnapshot(`
-                              Object {
-                                "provides": sku,
-                                "serviceName": "serviceA",
-                              }
-                          `);
+          Object {
+            "belongsToValueType": false,
+            "provides": sku,
+            "serviceName": "serviceA",
+          }
+        `);
+      });
+
+      it('adds correct @provides information to fields on value types', () => {
+        const serviceA = {
+          typeDefs: gql`
+            extend type Query {
+              valueType: ValueType
+            }
+
+            type ValueType {
+              id: ID!
+              user: User! @provides(fields: "id name")
+            }
+
+            type User @key(fields: "id") {
+              id: ID!
+              name: String!
+            }
+          `,
+          name: 'serviceA',
+        };
+
+        const serviceB = {
+          typeDefs: gql`
+            type ValueType {
+              id: ID!
+              user: User! @provides(fields: "id name")
+            }
+
+            extend type User @key(fields: "id") {
+              id: ID! @external
+              name: String! @external
+            }
+          `,
+          name: 'serviceB',
+        };
+
+        const { schema, errors } = composeServices([serviceA, serviceB]);
+        expect(errors).toHaveLength(0);
+
+        const valueType = schema.getType('ValueType') as GraphQLObjectType;
+        const userField = valueType.getFields()['user'].federation;
+        expect(userField.belongsToValueType).toBe(true);
+        expect(userField.serviceName).toBe(null);
       });
     });
 

diff --git a/packages/apollo-federation/src/composition/compose.ts b/packages/apollo-federation/src/composition/compose.ts
@@ -341,13 +341,16 @@ export function addFederationMetadataToSchemaNodes({
 
     // Extend each type in the GraphQLSchema with the serviceName that owns it
     // and the key directives that belong to it
+    const isValueType = valueTypes.has(typeName);
+    const serviceName = isValueType ? null : owningService;
+
     namedType.federation = {
       ...namedType.federation,
-      owningService,
+      serviceName,
+      isValueType,
       ...(keyDirectivesMap[typeName] && {
         keys: keyDirectivesMap[typeName],
       }),
-      isValueType: valueTypes.has(typeName),
     };
 
     // For object types, add metadata for all the @provides directives from its fields
@@ -365,10 +368,11 @@ export function addFederationMetadataToSchemaNodes({
         ) {
           field.federation = {
             ...field.federation,
-            serviceName: owningService,
+            serviceName,
             provides: parseSelections(
               providesDirective.arguments[0].value.value,
             ),
+            belongsToValueType: isValueType,
           };
         }
       }

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 {
-  owningService?: string;
+  serviceName?: string | null;
   keys?: ServiceNameToKeyDirectivesMap;
   externals?: {
     [serviceName: string]: ExternalFieldDefinition[];
@@ -30,6 +30,7 @@ export interface FederationField {
   serviceName?: ServiceName;
   requires?: ReadonlyArray<SelectionNode>;
   provides?: ReadonlyArray<SelectionNode>;
+  belongsToValueType?: boolean;
 }
 
 export interface ServiceDefinition {

diff --git a/packages/apollo-federation/src/composition/validate/postComposition/externalMissingOnBase.ts b/packages/apollo-federation/src/composition/validate/postComposition/externalMissingOnBase.ts
@@ -32,7 +32,7 @@ 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.owningService})`,
+                  `marked @external but ${externalFieldName} is not defined on the base service of ${typeName} (${namedType.federation.serviceName})`,
               ),
             );
             continue;
@@ -48,7 +48,7 @@ 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.owningService})`,
+                  `marked @external but ${externalFieldName} was defined in ${matchingBaseField.federation.serviceName}, not in the service that owns ${typeName} (${namedType.federation.serviceName})`,
               ),
             );
           }

diff --git a/packages/apollo-federation/src/composition/validate/postComposition/externalTypeMismatch.ts b/packages/apollo-federation/src/composition/validate/postComposition/externalTypeMismatch.ts
@@ -56,7 +56,7 @@ 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.owningService} (\`${matchingBaseField.type}\`)`,
+                  `Type \`${externalFieldType.name}\` does not match the type of the original field in ${namedType.federation.serviceName} (\`${matchingBaseField.type}\`)`,
               ),
             );
           }

diff --git a/packages/apollo-federation/src/composition/validate/postComposition/providesNotOnEntity.ts b/packages/apollo-federation/src/composition/validate/postComposition/providesNotOnEntity.ts
@@ -28,7 +28,12 @@ export const providesNotOnEntity = (schema: GraphQLSchema) => {
       // the only case where serviceName wouldn't exist is on a base type, and in that case,
       // the `provides` metadata should never get added to begin with. This should be caught in
       // composition work. This kind of error should be validated _before_ composition.
-      if (!serviceName && field.federation && field.federation.provides)
+      if (
+        !serviceName &&
+        field.federation &&
+        field.federation.provides &&
+        !field.federation.belongsToValueType
+      )
         throw Error(
           'Internal Consistency Error: field with provides information does not have service name.',
         );