Implement one-off support for input objects in the connect spec (#3311)

This PR implements one-off support for input objects in the connect
spec. Note that this implementation is hacky, and has various edge cases
where it won't behave as expected. In the short-term, we should avoid
using input objects in specs. In the long-term, there's a proper
strategy to supporting input objects in specs, which is described below.

To give some background:
1. Our schema representation (`Schema`) uses JS singleton objects for
schema elements.
2. Type references in these schema elements (e.g. the type of a field or
argument) will refer directly to these JS singletons.
3. This schema representation also tracks type/directive references in
the referenced schema element.

This means that schema building has to proceed in a two-phase process.
1. The first phase declares the types/directive definitions/extensions
(referred to as "shallow building" of the type/directive).
2. The second phase defines the types/directive definitions/extensions
(referred to as just "building" of the type/directive).

This is necessary because types/directives are allowed to form
referential cycles, so we can't just e.g. topologically sort the
types/directives by referential order and fully build in that order.

A problem arises, however, with subgraph schemas (a.k.a. schemas using
`FederationBlueprint`). In that case, schemas are allowed to omit
definitions/extensions of spec types/directives, for both the
`federation` and `connect` specs. These missing definitions/extensions
are added (or if already added, they're checked) by the `checkOrAdd()`
methods of type/directive specifications.

The `checkOrAdd()` pattern has a design flaw in that it bundles shallow
building, building, and checking into one single function. Specifically:
1. When building a schema, during the first phase, there may be missing
spec types/directives that are not written.
2. This blocks us from the second phase of building types/directives, as
they may depend on these missing types/directives.
3. This results in us calling `checkOrAdd()` for a type/directive spec
to fill in the missing definitions.
4. However, `checkOrAdd()` for a type/directive specification, in
bundling everything together, requires the types/directives it
references to be fully built.

This has several implications:
1. Specs with circular type/directive references just won't work.
2. Even if there's some topological order of references that can be
established, that order is currently not computed or followed. The
current `checkOrAdd()` calling logic just calls it for all registered
type specifications, and then all registered directive specifications.
This order has to be a topological referential order for schemas that
omit all spec type/directive definitions/extensions to build properly,
and that registration can be error-prone to manually specify/maintain.
- Also, this isn't done in a way that's interleaved with the building of
those elements if they already exist; for those elements, they need to
be built prior to the `checkOrAdd()` calls, otherwise the checking part
fails.
4. This has led to a very bespoke fragile pattern in schema building
where we first fully build enums and scalars while skipping directive
applications (since they can't reference other types/directives in that
case), then directive definitions while skipping directive applications,
and then call `checkOrAdd()` (after processing `@link`s).
5. This bespoke fragile pattern only worked before because the
federation spec just has type/directive definitions that only reference
enum/scalar types. And even in that case, if someone e.g. wrote in a
spec directive definition that referenced spec enums/scalars, they must
also write the definitions for those referenced enums/scalars, since
otherwise that directive definition would fail to build.
6. This bespoke fragile pattern does not work for connectors, since
those spec types/directives reference spec input object types, and some
of those spec input object types even reference other spec input object
types.

In order to support connectors usage of input object types in the
short-term, we must do a few things.
1. Verify there are no referential cycles (there aren't thankfully), and
register types/directives in the appropriate topological order.
2. Before calling `checkOrAdd()` for the connect spec, ensure that input
objects are built if they exist in the schema. This is going to have the
same limitation that the federation spec has with directive definitions,
where if someone wrote in a spec input object definition that references
other spec types/directives, they also need to write the definitions for
those referenced types/directives.

This will handle the main cases we wish to support, which are when no
connect spec definitions/extensions are provided and when they're all
provided. That's what this PR does (it also adds a test).

The actual long-term solution involves splitting the `checkOrAdd()`
logic into three separate functions:
1. Shallow building of spec types/directives that aren't in the schema
(accounting for renames as needed).
2. Full building of spec types/directives that aren't in the schema.
3. Checking of spec types/directives to ensure they conform to expected
definitions.

The gist is that we first do some processing on schema
definitions/extensions to bootstrap the `link` spec, then we couple spec
shallow building with the rest of shallow building (phase 1), couple
spec full building with the rest of full building (phase 2), and save
checking for after everything's built. There could be some additional
tracking to avoid doing checking for types/directives that weren't
originally in the schema, but that's not strictly necessary.

This PR also implements various fixes for bugs noticed in
#3310; these will be
pointed out in that PR's review.

Author: sachindshinde

composition-js/src/__tests__/connectors.test.ts

@@ -1,123 +1,155 @@
 import { composeServices } from "../compose";
-import { printSchema } from "@apollo/federation-internals";
+import { buildSubgraph, printSchema } from "@apollo/federation-internals";
 import { parse } from "graphql/index";
 
 describe("connect spec and join__directive", () => {
-  it("composes", () => {
-    const subgraphs = [
-      {
-        name: "with-connectors",
-        typeDefs: parse(`
-                    extend schema
-                    @link(
-                        url: "https://specs.apollo.dev/federation/v2.10"
-                        import: ["@key"]
-                    )
-                    @link(
-                        url: "https://specs.apollo.dev/connect/v0.1"
-                        import: ["@connect", "@source"]
-                    )
-                    @source(name: "v1", http: { baseURL: "http://v1" })
+  const subgraphSdl = `
+      extend schema
+      @link(
+          url: "https://specs.apollo.dev/federation/v2.10"
+          import: ["@key"]
+      )
+      @link(
+          url: "https://specs.apollo.dev/connect/v0.1"
+          import: ["@connect", "@source"]
+      )
+      @source(name: "v1", http: { baseURL: "http://v1" })
+
+      type Query {
+          resources: [Resource!]!
+          @connect(source: "v1", http: { GET: "/resources" }, selection: "")
+      }
 
-                    type Query {
-                        resources: [Resource!]!
-                        @connect(source: "v1", http: { GET: "/resources" }, selection: "")
-                    }
+      type Resource @key(fields: "id") {
+          id: ID!
+          name: String!
+      }
+  `;
+
+  const expectedSupergraphSdl = `
+    "schema
+      @link(url: \\"https://specs.apollo.dev/link/v1.0\\")
+      @link(url: \\"https://specs.apollo.dev/join/v0.5\\", for: EXECUTION)
+      @link(url: \\"https://specs.apollo.dev/connect/v0.2\\", for: EXECUTION)
+      @join__directive(graphs: [WITH_CONNECTORS], name: \\"link\\", args: {url: \\"https://specs.apollo.dev/connect/v0.1\\", import: [\\"@connect\\", \\"@source\\"]})
+      @join__directive(graphs: [WITH_CONNECTORS], name: \\"source\\", args: {name: \\"v1\\", http: {baseURL: \\"http://v1\\"}})
+    {
+      query: Query
+    }
 
-                    type Resource @key(fields: "id") {
-                        id: ID!
-                        name: String!
-                    }
-                `),
-      },
-    ];
+    directive @link(url: String, as: String, for: link__Purpose, import: [link__Import]) repeatable on SCHEMA
 
-    const result = composeServices(subgraphs);
-    expect(result.errors ?? []).toEqual([]);
-    const printed = printSchema(result.schema!);
-    expect(printed).toMatchInlineSnapshot(`
-      "schema
-        @link(url: \\"https://specs.apollo.dev/link/v1.0\\")
-        @link(url: \\"https://specs.apollo.dev/join/v0.5\\", for: EXECUTION)
-        @link(url: \\"https://specs.apollo.dev/connect/v0.2\\", for: EXECUTION)
-        @join__directive(graphs: [WITH_CONNECTORS], name: \\"link\\", args: {url: \\"https://specs.apollo.dev/connect/v0.1\\", import: [\\"@connect\\", \\"@source\\"]})
-        @join__directive(graphs: [WITH_CONNECTORS], name: \\"source\\", args: {name: \\"v1\\", http: {baseURL: \\"http://v1\\"}})
-      {
-        query: Query
-      }
+    directive @join__graph(name: String!, url: String!) on ENUM_VALUE
 
-      directive @link(url: String, as: String, for: link__Purpose, import: [link__Import]) repeatable on SCHEMA
+    directive @join__type(graph: join__Graph!, key: join__FieldSet, extension: Boolean! = false, resolvable: Boolean! = true, isInterfaceObject: Boolean! = false) repeatable on OBJECT | INTERFACE | UNION | ENUM | INPUT_OBJECT | SCALAR
 
-      directive @join__graph(name: String!, url: String!) on ENUM_VALUE
+    directive @join__field(graph: join__Graph, requires: join__FieldSet, provides: join__FieldSet, type: String, external: Boolean, override: String, usedOverridden: Boolean, overrideLabel: String, contextArguments: [join__ContextArgument!]) repeatable on FIELD_DEFINITION | INPUT_FIELD_DEFINITION
 
-      directive @join__type(graph: join__Graph!, key: join__FieldSet, extension: Boolean! = false, resolvable: Boolean! = true, isInterfaceObject: Boolean! = false) repeatable on OBJECT | INTERFACE | UNION | ENUM | INPUT_OBJECT | SCALAR
+    directive @join__implements(graph: join__Graph!, interface: String!) repeatable on OBJECT | INTERFACE
 
-      directive @join__field(graph: join__Graph, requires: join__FieldSet, provides: join__FieldSet, type: String, external: Boolean, override: String, usedOverridden: Boolean, overrideLabel: String, contextArguments: [join__ContextArgument!]) repeatable on FIELD_DEFINITION | INPUT_FIELD_DEFINITION
+    directive @join__unionMember(graph: join__Graph!, member: String!) repeatable on UNION
 
-      directive @join__implements(graph: join__Graph!, interface: String!) repeatable on OBJECT | INTERFACE
+    directive @join__enumValue(graph: join__Graph!) repeatable on ENUM_VALUE
 
-      directive @join__unionMember(graph: join__Graph!, member: String!) repeatable on UNION
+    directive @join__directive(graphs: [join__Graph!], name: String!, args: join__DirectiveArguments) repeatable on SCHEMA | OBJECT | INTERFACE | FIELD_DEFINITION
 
-      directive @join__enumValue(graph: join__Graph!) repeatable on ENUM_VALUE
+    enum link__Purpose {
+      \\"\\"\\"
+      \`SECURITY\` features provide metadata necessary to securely resolve fields.
+      \\"\\"\\"
+      SECURITY
 
-      directive @join__directive(graphs: [join__Graph!], name: String!, args: join__DirectiveArguments) repeatable on SCHEMA | OBJECT | INTERFACE | FIELD_DEFINITION
+      \\"\\"\\"
+      \`EXECUTION\` features provide metadata necessary for operation execution.
+      \\"\\"\\"
+      EXECUTION
+    }
 
-      enum link__Purpose {
-        \\"\\"\\"
-        \`SECURITY\` features provide metadata necessary to securely resolve fields.
-        \\"\\"\\"
-        SECURITY
+    scalar link__Import
 
-        \\"\\"\\"
-        \`EXECUTION\` features provide metadata necessary for operation execution.
-        \\"\\"\\"
-        EXECUTION
-      }
+    enum join__Graph {
+      WITH_CONNECTORS @join__graph(name: \\"with-connectors\\", url: \\"\\")
+    }
 
-      scalar link__Import
+    scalar join__FieldSet
 
-      enum join__Graph {
-        WITH_CONNECTORS @join__graph(name: \\"with-connectors\\", url: \\"\\")
-      }
+    scalar join__DirectiveArguments
 
-      scalar join__FieldSet
+    scalar join__FieldValue
 
-      scalar join__DirectiveArguments
+    input join__ContextArgument {
+      name: String!
+      type: String!
+      context: String!
+      selection: join__FieldValue!
+    }
 
-      scalar join__FieldValue
+    type Query
+      @join__type(graph: WITH_CONNECTORS)
+    {
+      resources: [Resource!]! @join__directive(graphs: [WITH_CONNECTORS], name: \\"connect\\", args: {source: \\"v1\\", http: {GET: \\"/resources\\"}, selection: \\"\\"})
+    }
 
-      input join__ContextArgument {
-        name: String!
-        type: String!
-        context: String!
-        selection: join__FieldValue!
-      }
+    type Resource
+      @join__type(graph: WITH_CONNECTORS, key: \\"id\\")
+    {
+      id: ID!
+      name: String!
+    }"
+  `;
+
+  const expectedApiSdl = `
+    "type Query {
+      resources: [Resource!]!
+    }
 
-      type Query
-        @join__type(graph: WITH_CONNECTORS)
-      {
-        resources: [Resource!]! @join__directive(graphs: [WITH_CONNECTORS], name: \\"connect\\", args: {source: \\"v1\\", http: {GET: \\"/resources\\"}, selection: \\"\\"})
-      }
+    type Resource {
+      id: ID!
+      name: String!
+    }"
+  `;
 
-      type Resource
-        @join__type(graph: WITH_CONNECTORS, key: \\"id\\")
+  it("composes", () => {
+    const subgraphs = [
       {
-        id: ID!
-        name: String!
-      }"
-    `);
+        name: "with-connectors",
+        typeDefs: parse(subgraphSdl),
+      },
+    ];
+
+    const result = composeServices(subgraphs);
+    expect(result.errors ?? []).toEqual([]);
+    const printed = printSchema(result.schema!);
+    expect(printed).toMatchInlineSnapshot(expectedSupergraphSdl);
 
     if (result.schema) {
-      expect(printSchema(result.schema.toAPISchema())).toMatchInlineSnapshot(`
-                      "type Query {
-                        resources: [Resource!]!
-                      }
+      expect(printSchema(result.schema.toAPISchema()))
+        .toMatchInlineSnapshot(expectedApiSdl);
+    }    
+  });
 
-                      type Resource {
-                        id: ID!
-                        name: String!
-                      }"
-                  `);
+  it("composes with completed definitions", () => {
+    const completedSubgraphSdl = printSchema(buildSubgraph(
+      "with-connectors",
+      "",
+      subgraphSdl,
+    ).schema);
+
+    const subgraphs = [
+      {
+        name: "with-connectors",
+        typeDefs: parse(completedSubgraphSdl),
+      },
+    ];
+
+    const result = composeServices(subgraphs);
+    expect(result.errors ?? []).toEqual([]);
+    const printed = printSchema(result.schema!);
+    expect(printed).toMatchInlineSnapshot(expectedSupergraphSdl);
+
+    if (result.schema) {
+      expect(printSchema(result.schema.toAPISchema()))
+        .toMatchInlineSnapshot(expectedApiSdl);
     }
   });
 

internals-js/src/buildSchema.ts

@@ -56,6 +56,9 @@ import {
 } from "./definitions";
 import { ERRORS, errorCauses, withModifiedErrorNodes } from "./error";
 import { introspectionTypeNames } from "./introspection";
+import { coreFeatureDefinitionIfKnown } from "./knownCoreFeatures";
+import { connectIdentity } from "./specs/connectSpec";
+
 
 function buildValue(value?: ValueNode): any {
   return value ? valueFromASTUntyped(value) : undefined;
@@ -143,6 +146,48 @@ export function buildSchemaFromAST(
     buildSchemaDefinitionInner(schemaExtension, schema.schemaDefinition, errors, schema.schemaDefinition.newExtension());
   }
 
+  // The following block of code is a one-off to support input objects in the
+  // connect spec. It will be non-maintainable/bug-prone to do this again, and
+  // has various limitations/unsupported edge cases already.
+  //
+  // There's work to be done to support input objects more generally; please see
+  // https://github.com/apollographql/federation/pull/3311 for more information.
+  const connectFeature = schema.coreFeatures?.getByIdentity(connectIdentity);
+  const handledConnectTypeNames = new Set<string>();
+  if (connectFeature) {
+    const connectFeatureDefinition =
+      coreFeatureDefinitionIfKnown(connectFeature.url);
+    if (connectFeatureDefinition) {
+      const connectTypeNamesInSchema = new Set(
+        connectFeatureDefinition.typeSpecs()
+          .map(({ name }) => connectFeature.typeNameInSchema(name))
+      );
+      for (const typeNode of typeDefinitions) {
+        if (connectTypeNamesInSchema.has(typeNode.name.value)
+          && typeNode.kind === 'InputObjectTypeDefinition'
+        ) {
+          handledConnectTypeNames.add(typeNode.name.value)
+        } else {
+          continue;
+        }
+        buildNamedTypeInner(typeNode, schema.type(typeNode.name.value)!, schema.blueprint, errors);
+      }
+      for (const typeExtensionNode of typeExtensions) {
+        if (connectTypeNamesInSchema.has(typeExtensionNode.name.value)
+          && typeExtensionNode.kind === 'InputObjectTypeExtension'
+        ) {
+          handledConnectTypeNames.add(typeExtensionNode.name.value)
+        } else {
+          continue;
+        }
+        const toExtend = schema.type(typeExtensionNode.name.value)!;
+        const extension = toExtend.newExtension();
+        extension.sourceAST = typeExtensionNode;
+        buildNamedTypeInner(typeExtensionNode, toExtend, schema.blueprint, errors, extension);
+      }
+    }
+  }
+
   // The following is a no-op for "standard" schema, but for federation subgraphs, this is where we handle the auto-addition
   // of imported federation directive definitions. That is why we have avoid looking at directive applications within
   // directive definition earlier: if one of those application was of an imported federation directive, the definition
@@ -155,9 +200,15 @@ export function buildSchemaFromAST(
   }
 
   for (const typeNode of typeDefinitions) {
+    if (handledConnectTypeNames.has(typeNode.name.value)) {
+      continue;
+    }
     buildNamedTypeInner(typeNode, schema.type(typeNode.name.value)!, schema.blueprint, errors);
   }
   for (const typeExtensionNode of typeExtensions) {
+    if (handledConnectTypeNames.has(typeExtensionNode.name.value)) {
+      continue;
+    }
     const toExtend = schema.type(typeExtensionNode.name.value)!;
     const extension = toExtend.newExtension();
     extension.sourceAST = typeExtensionNode;