fix(composition): Don't remove scalar properties in buildSubgraphSchema() when providing resolvers (#3285)

When a `GraphQLScalarType` is provided in resolvers to `buildSubgraphSchema()`, its properties overwrite the existing one in the schema created from `typeDefs`. However, this also includes non-provided/`undefined` properties, which probably isn't what users expect. This PR updates resolver addition so that `undefined` properties are skipped; if users want to unset/clear scalar properties, they should use an explicit `null` for those properties.

Author: sachindshinde

.changeset/smooth-terms-decide.md

@@ -0,0 +1,5 @@
+---
+"@apollo/subgraph": patch
+---
+
+When a `GraphQLScalarType` resolver is provided to `buildSubgraphSchema()`, omitted configuration options in the `GraphQLScalarType` no longer cause the corresponding properties in the GraphQL document/AST to be cleared. To explicitly clear these properties, use `null` for the configuration option instead. 

subgraph-js/src/__tests__/buildSubgraphSchema.test.ts

@@ -6,10 +6,12 @@ import {
   execute,
   type DefinitionNode,
   OperationTypeNode,
+  GraphQLScalarType,
   GraphQLUnionType,
   printType,
 } from 'graphql';
 import { buildSubgraphSchema } from '../buildSubgraphSchema';
+import { printSubgraphSchema } from '../printSubgraphSchema';
 import { typeSerializer } from 'apollo-federation-integration-testsuite';
 import { errorCauses } from '@apollo/federation-internals';
 
@@ -964,10 +966,12 @@ describe('buildSubgraphSchema', () => {
 
     const validateTag = async (
       header: string,
+      printedHeader: string,
       directiveDefinitions: string,
       typeDefinitions: string,
+      withResolvers: boolean,
     ) => {
-      const schema = buildSubgraphSchema(gql`
+      const typeDefs = gql`
         ${header}
         type User @key(fields: "email") @tag(name: "tagOnType") {
           email: String @tag(name: "tagOnField")
@@ -978,19 +982,15 @@ describe('buildSubgraphSchema', () => {
         }
 
         union UserButAUnion @tag(name: "tagOnUnion") = User
-      `);
 
-      const { data, errors } = await graphql({ schema, source: query });
-      expect(errors).toBeUndefined();
-      expect((data?._service as any).sdl).toMatchString(
-        (header.length === 0
-          ? ''
-          : `
-        ${header.trim()}
-        `) +
-          `
-        ${directiveDefinitions.trim()}
+        scalar CustomScalar @tag(name: "tagOnCustomScalar")
 
+        enum Enum @tag(name: "tagOnEnum") {
+          ENUM_VALUE @tag(name: "tagOnEnumValue")
+        }
+      `;
+
+      const printedTypeDefsWithoutHeader = `
         type User
           @key(fields: "email")
           @tag(name: "tagOnType")
@@ -1008,15 +1008,72 @@ describe('buildSubgraphSchema', () => {
           @tag(name: "tagOnUnion")
          = User
 
+        scalar CustomScalar
+          @tag(name: "tagOnCustomScalar")
+
+        enum Enum
+          @tag(name: "tagOnEnum")
+        {
+          ENUM_VALUE @tag(name: "tagOnEnumValue")
+        }
+      `;
+
+      const schema = withResolvers
+        ? buildSubgraphSchema({
+            typeDefs,
+            resolvers: {
+              User: {
+                email: () => 1,
+              },
+              CustomScalar: new GraphQLScalarType({
+                name: 'CustomScalar',
+                serialize: () => 1,
+                parseValue: () => 1,
+                parseLiteral: () => 1,
+              }),
+              Enum: {
+                ENUM_VALUE: 1,
+              },
+            },
+          })
+        : buildSubgraphSchema(typeDefs);
+
+      // Check the sdl field's schema.
+      const { data, errors } = await graphql({ schema, source: query });
+      expect(errors).toBeUndefined();
+      expect((data?._service as any).sdl).toMatchString(
+        (header.length === 0
+          ? ''
+          : `
+        ${header.trim()}
+        `) +
+          `
+        ${directiveDefinitions.trim()}
+
+        ${printedTypeDefsWithoutHeader.trim()}
+
         ${typeDefinitions.trim()}
       `,
       );
+
+      // Check the printSubgraphSchema's schema.
+      expect(printSubgraphSchema(schema)).toMatchString(
+        (printedHeader.length === 0
+          ? ''
+          : `
+        ${printedHeader.trim()}
+        `) +
+          `
+        ${printedTypeDefsWithoutHeader.trim()}
+      `,
+      );
     };
 
-    it.each([
+    const testCases = [
       {
         name: 'fed1',
         header: '',
+        printedHeader: '',
         directiveDefinitions: `
         directive @key(fields: _FieldSet!, resolvable: Boolean = true) repeatable on OBJECT | INTERFACE
 
@@ -1053,6 +1110,10 @@ describe('buildSubgraphSchema', () => {
         extend schema
           @link(url: "https://specs.apollo.dev/link/v1.0")
           @link(url: "https://specs.apollo.dev/federation/v2.0", import: ["@key", "@tag"])
+      `,
+        printedHeader: `
+        extend schema
+          @link(url: "https://specs.apollo.dev/federation/v2.0", import: ["@key", "@tag"])
       `,
         directiveDefinitions: `
         directive @link(url: String, as: String, for: link__Purpose, import: [link__Import]) repeatable on SCHEMA
@@ -1106,10 +1167,41 @@ describe('buildSubgraphSchema', () => {
         }
       `,
       },
-    ])(
+    ];
+
+    it.each(testCases)(
       'adds it for $name schema',
-      async ({ header, directiveDefinitions, typesDefinitions }) => {
-        await validateTag(header, directiveDefinitions, typesDefinitions);
+      async ({
+        header,
+        printedHeader,
+        directiveDefinitions,
+        typesDefinitions,
+      }) => {
+        await validateTag(
+          header,
+          printedHeader,
+          directiveDefinitions,
+          typesDefinitions,
+          false,
+        );
+      },
+    );
+
+    it.each(testCases)(
+      'adds it for $name schema with resolvers',
+      async ({
+        header,
+        printedHeader,
+        directiveDefinitions,
+        typesDefinitions,
+      }) => {
+        await validateTag(
+          header,
+          printedHeader,
+          directiveDefinitions,
+          typesDefinitions,
+          true,
+        );
       },
     );
   });

subgraph-js/src/schema-helper/buildSchemaFromSDL.ts

@@ -133,7 +133,16 @@ export function addResolversToSchema(
 
     if (isScalarType(type)) {
       for (const fn in fieldConfigs) {
-        (type as any)[fn] = (fieldConfigs as any)[fn];
+        const fnValue = (fieldConfigs as any)[fn];
+        // When users provide a `GraphQLScalarType` for resolvers, they often
+        // omit several config options (effectively providing `undefined`), but
+        // they probably don't mean for this to unset values in the existing
+        // `GraphQLScalarType` (e.g., clearing the AST nodes or description).
+        // So we explicitly ignore `undefined` values here; if users do want to
+        // unset existing values, they can use `null` instead.
+        if (fnValue !== undefined) {
+          (type as any)[fn] = fnValue;
+        }
       }
     }