Align function signature/return type for compose and composeAndValidate

Author: trevor-scheer

federation-js/CHANGELOG.md

@@ -5,7 +5,7 @@
 > The changes noted within this `vNEXT` section have not been released yet.  New PRs and commits which introduce changes should include an entry in this `vNEXT` section as part of their development.  When a release is being prepared, a new header will be (manually) created below and the appropriate changes within that release will be moved into the new section.
 
 - Export `GraphQLSchemaModule` type. [PR #293](https://github.com/apollographql/federation/pull/293)
-- __BREAKING__: Remove `ComposedGraphQLSchema` type as it's no longer needed. This is breaking because it was part of the public API, though we strongly believe nobody was or should have had any need for this type. [PR #278](https://github.com/apollographql/federation/pull/278)
+- __BREAKING__: Remove `ComposedGraphQLSchema` type as it's no longer needed. This is breaking because it was part of the public API, though we strongly believe nobody was or should have had any need for this type. Update `composeAndValidate` function signature for better typing, and align the `compose` function signature with that of `composeAndValidate` [PR #278](https://github.com/apollographql/federation/pull/278)
 ## v0.20.7
 
 - Fix check for value types when having fields and arguments with the same name [PR #280](https://github.com/apollographql/federation/pull/280)

federation-js/src/composition/__tests__/compose.test.ts

@@ -12,12 +12,8 @@ import {
   typeSerializer,
   graphqlErrorSerializer,
 } from '../../snapshotSerializers';
-import {
-  compositionHasErrors,
-  CompositionResult,
-  CompositionSuccess,
-  CompositionFailure,
-} from '@apollo/federation';
+import { assertCompositionFailure, assertCompositionSuccess } from '../utils';
+import { compositionHasErrors } from '../compose';
 
 expect.addSnapshotSerializer(astSerializer);
 expect.addSnapshotSerializer(typeSerializer);
@@ -105,30 +101,6 @@ function permutateList<T>(inputArr: T[]) {
   return result;
 }
 
-// This assertion function should be used for the sake of convenient type refinement.
-// It should not be depended on for causing a test to fail. If an error is thrown
-// from here, its use should be reconsidered.
-export function assertCompositionSuccess(
-  compositionResult: CompositionResult,
-  message?: string,
-): asserts compositionResult is CompositionSuccess {
-  if (compositionHasErrors(compositionResult)) {
-    throw new Error(message || 'Unexpected test failure');
-  }
-}
-
-// This assertion function should be used for the sake of convenient type refinement.
-// It should not be depended on for causing a test to fail. If an error is thrown
-// from here, its use should be reconsidered.
-export function assertCompositionFailure(
-  compositionResult: CompositionResult,
-  message?: string,
-): asserts compositionResult is CompositionFailure {
-  if (!compositionHasErrors(compositionResult)) {
-    throw new Error(message || 'Unexpected test failure');
-  }
-}
-
 it('composes and validates all (24) permutations without error', () => {
   permutateList([
     inventoryService,

federation-js/src/composition/__tests__/composeAndValidate.test.ts

@@ -45,6 +45,7 @@ import {
 } from './types';
 import { validateSDL } from 'graphql/validation/validate';
 import { compositionRules } from './rules';
+import { printComposedSdl } from '../service/printComposedSdl';
 
 const EmptyQueryDefinition = {
   kind: Kind.OBJECT_TYPE_DEFINITION,
@@ -120,6 +121,28 @@ export interface KeyDirectivesMap {
  */
 type ValueTypes = Set<string>;
 
+export type CompositionResult = CompositionFailure | CompositionSuccess;
+
+// Yes, it's a bit awkward that we still return a schema when errors occur.
+// This is old behavior that I'm choosing not to modify for now.
+export interface CompositionFailure {
+  /** @deprecated Use composedSdl instead */
+  schema: GraphQLSchema;
+  errors: GraphQLError[];
+}
+
+export interface CompositionSuccess {
+  /** @deprecated Use composedSdl instead */
+  schema: GraphQLSchema;
+  composedSdl: string;
+}
+
+export function compositionHasErrors(
+  compositionResult: CompositionResult,
+): compositionResult is CompositionFailure {
+  return 'errors' in compositionResult;
+}
+
 /**
  * Loop over each service and process its typeDefs (`definitions`)
  * - build up typeToServiceMap
@@ -589,7 +612,7 @@ export function addFederationMetadataToSchemaNodes({
   }
 }
 
-export function composeServices(services: ServiceDefinition[]) {
+export function composeServices(services: ServiceDefinition[]): CompositionResult {
   const {
     typeToServiceMap,
     typeDefinitionsMap,
@@ -644,10 +667,12 @@ export function composeServices(services: ServiceDefinition[]) {
     directiveDefinitionsMap,
   });
 
-  /**
-   * At the end, we're left with a full GraphQLSchema that _also_ has `serviceName` fields for every type,
-   * and every field that was extended. Fields that were _not_ extended (added on the base type by the owner),
-   * there is no `serviceName`, and we should refer to the type's `serviceName`
-   */
-  return { schema, errors };
+  if (errors.length > 0) {
+    return { schema, errors };
+  } else {
+    return {
+      schema,
+      composedSdl: printComposedSdl(schema, services),
+    };
+  }
 }

federation-js/src/composition/compose.ts

@@ -1,35 +1,15 @@
-import { composeServices } from './compose';
+import {
+  composeServices,
+  compositionHasErrors,
+  CompositionResult,
+} from './compose';
 import {
   validateComposedSchema,
   validateServicesBeforeComposition,
   validateServicesBeforeNormalization,
 } from './validate';
 import { ServiceDefinition } from './types';
 import { normalizeTypeDefs } from './normalize';
-import { printComposedSdl } from '../service/printComposedSdl';
-import { GraphQLError, GraphQLSchema } from 'graphql';
-
-export type CompositionResult = CompositionFailure | CompositionSuccess;
-
-// Yes, it's a bit awkward that we still return a schema when errors occur.
-// This is old behavior that I'm choosing not to modify for now.
-export interface CompositionFailure {
-  /** @deprecated Use composedSdl instead */
-  schema: GraphQLSchema;
-  errors: GraphQLError[];
-}
-
-export interface CompositionSuccess {
-  /** @deprecated Use composedSdl instead */
-  schema: GraphQLSchema;
-  composedSdl: string;
-}
-
-export function compositionHasErrors(
-  compositionResult: CompositionResult,
-): compositionResult is CompositionFailure {
-  return 'errors' in compositionResult;
-}
 
 export function composeAndValidate(
   serviceList: ServiceDefinition[],
@@ -46,7 +26,10 @@ export function composeAndValidate(
 
   // generate a schema and any errors or warnings
   const compositionResult = composeServices(normalizedServiceList);
-  errors.push(...compositionResult.errors);
+
+  if (compositionHasErrors(compositionResult)) {
+    errors.push(...compositionResult.errors);
+  }
 
   // validate the composed schema based on service information
   errors.push(
@@ -61,10 +44,7 @@ export function composeAndValidate(
       schema: compositionResult.schema,
       errors,
     };
+  } else {
+    return compositionResult;
   }
-
-  return {
-    schema: compositionResult.schema,
-    composedSdl: printComposedSdl(compositionResult.schema, serviceList),
-  };
 }

federation-js/src/composition/composeAndValidate.ts

@@ -43,6 +43,12 @@ import {
   FederationField,
 } from './types';
 import federationDirectives from '../directives';
+import {
+  CompositionFailure,
+  compositionHasErrors,
+  CompositionResult,
+  CompositionSuccess,
+} from './compose';
 
 export function isStringValueNode(node: any): node is StringValueNode {
   return node.kind === Kind.STRING;
@@ -614,3 +620,27 @@ export function getFederationMetadata(obj: any) {
   else if (isDirective(obj)) return obj.extensions?.federation as FederationDirective | undefined;
   else return obj.extensions?.federation as FederationField | undefined;
 }
+
+// This assertion function should be used for the sake of convenient type refinement.
+// It should not be depended on for causing a test to fail. If an error is thrown
+// from here, its use should be reconsidered.
+export function assertCompositionSuccess(
+  compositionResult: CompositionResult,
+  message?: string,
+): asserts compositionResult is CompositionSuccess {
+  if (compositionHasErrors(compositionResult)) {
+    throw new Error(message || 'Unexpected test failure');
+  }
+}
+
+// This assertion function should be used for the sake of convenient type refinement.
+// It should not be depended on for causing a test to fail. If an error is thrown
+// from here, its use should be reconsidered.
+export function assertCompositionFailure(
+  compositionResult: CompositionResult,
+  message?: string,
+): asserts compositionResult is CompositionFailure {
+  if (!compositionHasErrors(compositionResult)) {
+    throw new Error(message || 'Unexpected test failure');
+  }
+}

federation-js/src/composition/utils.ts

@@ -2,6 +2,7 @@ import gql from 'graphql-tag';
 import { composeServices } from '../../../compose';
 import { keyFieldsMissingOnBase as validateKeyFieldsMissingOnBase } from '../';
 import { graphqlErrorSerializer } from '../../../../snapshotSerializers';
+import { assertCompositionSuccess } from '../../../utils';
 
 expect.addSnapshotSerializer(graphqlErrorSerializer);
 
@@ -36,9 +37,9 @@ describe('keyFieldsMissingOnBase', () => {
     };
 
     const serviceList = [serviceA, serviceB];
-    const { schema, errors } = composeServices(serviceList);
-    expect(errors).toHaveLength(0);
-
+    const compositionResult = composeServices(serviceList);
+    assertCompositionSuccess(compositionResult);
+    const { schema } = compositionResult;
     const warnings = validateKeyFieldsMissingOnBase({ schema, serviceList });
     expect(warnings).toHaveLength(0);
   });
@@ -66,8 +67,9 @@ describe('keyFieldsMissingOnBase', () => {
     };
 
     const serviceList = [serviceA, serviceB];
-    const { schema, errors } = composeServices(serviceList);
-    expect(errors).toHaveLength(0);
+    const compositionResult = composeServices(serviceList);
+    assertCompositionSuccess(compositionResult);
+    const { schema } = compositionResult;
 
     const warnings = validateKeyFieldsMissingOnBase({ schema, serviceList });
     expect(warnings).toMatchInlineSnapshot(`
@@ -104,8 +106,9 @@ describe('keyFieldsMissingOnBase', () => {
     };
 
     const serviceList = [serviceA, serviceB];
-    const { schema, errors } = composeServices(serviceList);
-    expect(errors).toHaveLength(0);
+    const compositionResult = composeServices(serviceList);
+    assertCompositionSuccess(compositionResult);
+    const { schema } = compositionResult;
 
     const warnings = validateKeyFieldsMissingOnBase({ schema, serviceList });
     expect(warnings).toMatchInlineSnapshot();

federation-js/src/composition/validate/postComposition/__tests__/keyFieldsMissingOnBase.test.ts

@@ -2,6 +2,7 @@ import gql from 'graphql-tag';
 import { composeServices } from '../../../compose';
 import { keyFieldsSelectInvalidType as validateKeyFieldsSelectInvalidType } from '../';
 import { graphqlErrorSerializer } from '../../../../snapshotSerializers';
+import { assertCompositionSuccess } from '../../../utils';
 
 expect.addSnapshotSerializer(graphqlErrorSerializer);
 
@@ -36,8 +37,9 @@ describe('keyFieldsSelectInvalidType', () => {
     };
 
     const serviceList = [serviceA, serviceB];
-    const { schema, errors } = composeServices(serviceList);
-    expect(errors).toHaveLength(0);
+    const compositionResult = composeServices(serviceList);
+    assertCompositionSuccess(compositionResult);
+    const { schema } = compositionResult;
 
     const warnings = validateKeyFieldsSelectInvalidType({
       schema,
@@ -72,8 +74,9 @@ describe('keyFieldsSelectInvalidType', () => {
     };
 
     const serviceList = [serviceA, serviceB];
-    const { schema, errors } = composeServices(serviceList);
-    expect(errors).toHaveLength(0);
+    const compositionResult = composeServices(serviceList);
+    assertCompositionSuccess(compositionResult);
+    const { schema } = compositionResult;
 
     const warnings = validateKeyFieldsSelectInvalidType({
       schema,
@@ -113,8 +116,9 @@ describe('keyFieldsSelectInvalidType', () => {
     };
 
     const serviceList = [serviceA, serviceB];
-    const { schema, errors } = composeServices(serviceList);
-    expect(errors).toHaveLength(0);
+    const compositionResult = composeServices(serviceList);
+    assertCompositionSuccess(compositionResult);
+    const { schema } = compositionResult;
 
     const warnings = validateKeyFieldsSelectInvalidType({
       schema,

federation-js/src/composition/validate/postComposition/__tests__/keyFieldsSelectInvalidType.test.ts

@@ -2,6 +2,7 @@ import gql from 'graphql-tag';
 import { composeServices } from '../../../compose';
 import { keysMatchBaseService as validateKeysMatchBaseService } from '../';
 import { graphqlErrorSerializer } from '../../../../snapshotSerializers';
+import { assertCompositionSuccess } from '../../../utils';
 
 expect.addSnapshotSerializer(graphqlErrorSerializer);
 
@@ -28,8 +29,9 @@ describe('keysMatchBaseService', () => {
     };
 
     const serviceList = [serviceA, serviceB];
-    const { schema, errors } = composeServices(serviceList);
-    expect(errors).toHaveLength(0);
+    const compositionResult = composeServices(serviceList);
+    assertCompositionSuccess(compositionResult);
+    const { schema } = compositionResult;
 
     const validationErrors = validateKeysMatchBaseService({
       schema,
@@ -60,8 +62,9 @@ describe('keysMatchBaseService', () => {
     };
 
     const serviceList = [serviceA, serviceB];
-    const { schema, errors } = composeServices(serviceList);
-    expect(errors).toHaveLength(0);
+    const compositionResult = composeServices(serviceList);
+    assertCompositionSuccess(compositionResult);
+    const { schema } = compositionResult;
 
     const validationErrors = validateKeysMatchBaseService({
       schema,
@@ -98,8 +101,9 @@ describe('keysMatchBaseService', () => {
     };
 
     const serviceList = [serviceA, serviceB];
-    const { schema, errors } = composeServices(serviceList);
-    expect(errors).toHaveLength(0);
+    const compositionResult = composeServices(serviceList);
+    assertCompositionSuccess(compositionResult);
+    const { schema } = compositionResult;
 
     const validationErrors = validateKeysMatchBaseService({
       schema,

federation-js/src/composition/validate/postComposition/__tests__/keysMatchBaseService.test.ts

@@ -2,6 +2,7 @@ import gql from 'graphql-tag';
 import { composeServices } from '../../../compose';
 import { providesFieldsMissingExternal as validateProdivesFieldsMissingExternal } from '../';
 import { graphqlErrorSerializer } from '../../../../snapshotSerializers';
+import { assertCompositionSuccess } from '../../../utils';
 
 expect.addSnapshotSerializer(graphqlErrorSerializer);
 
@@ -51,8 +52,9 @@ describe('providesFieldsMissingExternal', () => {
     };
 
     const serviceList = [serviceA, serviceB, serviceC];
-    const { schema, errors } = composeServices(serviceList);
-    expect(errors).toEqual([]);
+    const compositionResult = composeServices(serviceList);
+    assertCompositionSuccess(compositionResult);
+    const { schema } = compositionResult;
     const warnings = validateProdivesFieldsMissingExternal({
       schema,
       serviceList,
@@ -88,8 +90,9 @@ describe('providesFieldsMissingExternal', () => {
     };
 
     const serviceList = [serviceA, serviceB];
-    const { schema, errors } = composeServices(serviceList);
-    expect(errors).toEqual([]);
+    const compositionResult = composeServices(serviceList);
+    assertCompositionSuccess(compositionResult);
+    const { schema } = compositionResult;
     const warnings = validateProdivesFieldsMissingExternal({
       schema,
       serviceList,