Skip to content

Commit 3df4612

Browse files
sachindshindesachindshinde
authored
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.
1 parent 7306739 commit 3df4612

File tree

4 files changed

+312
-166
lines changed

4 files changed

+312
-166
lines changed

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

Lines changed: 123 additions & 91 deletions
Original file line numberDiff line numberDiff line change
@@ -1,123 +1,155 @@
11
import { composeServices } from "../compose";
2-
import { printSchema } from "@apollo/federation-internals";
2+
import { buildSubgraph, printSchema } from "@apollo/federation-internals";
33
import { parse } from "graphql/index";
44

55
describe("connect spec and join__directive", () => {
6-
it("composes", () => {
7-
const subgraphs = [
8-
{
9-
name: "with-connectors",
10-
typeDefs: parse(`
11-
extend schema
12-
@link(
13-
url: "https://specs.apollo.dev/federation/v2.10"
14-
import: ["@key"]
15-
)
16-
@link(
17-
url: "https://specs.apollo.dev/connect/v0.1"
18-
import: ["@connect", "@source"]
19-
)
20-
@source(name: "v1", http: { baseURL: "http://v1" })
6+
const subgraphSdl = `
7+
extend schema
8+
@link(
9+
url: "https://specs.apollo.dev/federation/v2.10"
10+
import: ["@key"]
11+
)
12+
@link(
13+
url: "https://specs.apollo.dev/connect/v0.1"
14+
import: ["@connect", "@source"]
15+
)
16+
@source(name: "v1", http: { baseURL: "http://v1" })
17+
18+
type Query {
19+
resources: [Resource!]!
20+
@connect(source: "v1", http: { GET: "/resources" }, selection: "")
21+
}
2122
22-
type Query {
23-
resources: [Resource!]!
24-
@connect(source: "v1", http: { GET: "/resources" }, selection: "")
25-
}
23+
type Resource @key(fields: "id") {
24+
id: ID!
25+
name: String!
26+
}
27+
`;
28+
29+
const expectedSupergraphSdl = `
30+
"schema
31+
@link(url: \\"https://specs.apollo.dev/link/v1.0\\")
32+
@link(url: \\"https://specs.apollo.dev/join/v0.5\\", for: EXECUTION)
33+
@link(url: \\"https://specs.apollo.dev/connect/v0.2\\", for: EXECUTION)
34+
@join__directive(graphs: [WITH_CONNECTORS], name: \\"link\\", args: {url: \\"https://specs.apollo.dev/connect/v0.1\\", import: [\\"@connect\\", \\"@source\\"]})
35+
@join__directive(graphs: [WITH_CONNECTORS], name: \\"source\\", args: {name: \\"v1\\", http: {baseURL: \\"http://v1\\"}})
36+
{
37+
query: Query
38+
}
2639
27-
type Resource @key(fields: "id") {
28-
id: ID!
29-
name: String!
30-
}
31-
`),
32-
},
33-
];
40+
directive @link(url: String, as: String, for: link__Purpose, import: [link__Import]) repeatable on SCHEMA
3441
35-
const result = composeServices(subgraphs);
36-
expect(result.errors ?? []).toEqual([]);
37-
const printed = printSchema(result.schema!);
38-
expect(printed).toMatchInlineSnapshot(`
39-
"schema
40-
@link(url: \\"https://specs.apollo.dev/link/v1.0\\")
41-
@link(url: \\"https://specs.apollo.dev/join/v0.5\\", for: EXECUTION)
42-
@link(url: \\"https://specs.apollo.dev/connect/v0.2\\", for: EXECUTION)
43-
@join__directive(graphs: [WITH_CONNECTORS], name: \\"link\\", args: {url: \\"https://specs.apollo.dev/connect/v0.1\\", import: [\\"@connect\\", \\"@source\\"]})
44-
@join__directive(graphs: [WITH_CONNECTORS], name: \\"source\\", args: {name: \\"v1\\", http: {baseURL: \\"http://v1\\"}})
45-
{
46-
query: Query
47-
}
42+
directive @join__graph(name: String!, url: String!) on ENUM_VALUE
4843
49-
directive @link(url: String, as: String, for: link__Purpose, import: [link__Import]) repeatable on SCHEMA
44+
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
5045
51-
directive @join__graph(name: String!, url: String!) on ENUM_VALUE
46+
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
5247
53-
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
48+
directive @join__implements(graph: join__Graph!, interface: String!) repeatable on OBJECT | INTERFACE
5449
55-
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
50+
directive @join__unionMember(graph: join__Graph!, member: String!) repeatable on UNION
5651
57-
directive @join__implements(graph: join__Graph!, interface: String!) repeatable on OBJECT | INTERFACE
52+
directive @join__enumValue(graph: join__Graph!) repeatable on ENUM_VALUE
5853
59-
directive @join__unionMember(graph: join__Graph!, member: String!) repeatable on UNION
54+
directive @join__directive(graphs: [join__Graph!], name: String!, args: join__DirectiveArguments) repeatable on SCHEMA | OBJECT | INTERFACE | FIELD_DEFINITION
6055
61-
directive @join__enumValue(graph: join__Graph!) repeatable on ENUM_VALUE
56+
enum link__Purpose {
57+
\\"\\"\\"
58+
\`SECURITY\` features provide metadata necessary to securely resolve fields.
59+
\\"\\"\\"
60+
SECURITY
6261
63-
directive @join__directive(graphs: [join__Graph!], name: String!, args: join__DirectiveArguments) repeatable on SCHEMA | OBJECT | INTERFACE | FIELD_DEFINITION
62+
\\"\\"\\"
63+
\`EXECUTION\` features provide metadata necessary for operation execution.
64+
\\"\\"\\"
65+
EXECUTION
66+
}
6467
65-
enum link__Purpose {
66-
\\"\\"\\"
67-
\`SECURITY\` features provide metadata necessary to securely resolve fields.
68-
\\"\\"\\"
69-
SECURITY
68+
scalar link__Import
7069
71-
\\"\\"\\"
72-
\`EXECUTION\` features provide metadata necessary for operation execution.
73-
\\"\\"\\"
74-
EXECUTION
75-
}
70+
enum join__Graph {
71+
WITH_CONNECTORS @join__graph(name: \\"with-connectors\\", url: \\"\\")
72+
}
7673
77-
scalar link__Import
74+
scalar join__FieldSet
7875
79-
enum join__Graph {
80-
WITH_CONNECTORS @join__graph(name: \\"with-connectors\\", url: \\"\\")
81-
}
76+
scalar join__DirectiveArguments
8277
83-
scalar join__FieldSet
78+
scalar join__FieldValue
8479
85-
scalar join__DirectiveArguments
80+
input join__ContextArgument {
81+
name: String!
82+
type: String!
83+
context: String!
84+
selection: join__FieldValue!
85+
}
8686
87-
scalar join__FieldValue
87+
type Query
88+
@join__type(graph: WITH_CONNECTORS)
89+
{
90+
resources: [Resource!]! @join__directive(graphs: [WITH_CONNECTORS], name: \\"connect\\", args: {source: \\"v1\\", http: {GET: \\"/resources\\"}, selection: \\"\\"})
91+
}
8892
89-
input join__ContextArgument {
90-
name: String!
91-
type: String!
92-
context: String!
93-
selection: join__FieldValue!
94-
}
93+
type Resource
94+
@join__type(graph: WITH_CONNECTORS, key: \\"id\\")
95+
{
96+
id: ID!
97+
name: String!
98+
}"
99+
`;
100+
101+
const expectedApiSdl = `
102+
"type Query {
103+
resources: [Resource!]!
104+
}
95105
96-
type Query
97-
@join__type(graph: WITH_CONNECTORS)
98-
{
99-
resources: [Resource!]! @join__directive(graphs: [WITH_CONNECTORS], name: \\"connect\\", args: {source: \\"v1\\", http: {GET: \\"/resources\\"}, selection: \\"\\"})
100-
}
106+
type Resource {
107+
id: ID!
108+
name: String!
109+
}"
110+
`;
101111

102-
type Resource
103-
@join__type(graph: WITH_CONNECTORS, key: \\"id\\")
112+
it("composes", () => {
113+
const subgraphs = [
104114
{
105-
id: ID!
106-
name: String!
107-
}"
108-
`);
115+
name: "with-connectors",
116+
typeDefs: parse(subgraphSdl),
117+
},
118+
];
119+
120+
const result = composeServices(subgraphs);
121+
expect(result.errors ?? []).toEqual([]);
122+
const printed = printSchema(result.schema!);
123+
expect(printed).toMatchInlineSnapshot(expectedSupergraphSdl);
109124

110125
if (result.schema) {
111-
expect(printSchema(result.schema.toAPISchema())).toMatchInlineSnapshot(`
112-
"type Query {
113-
resources: [Resource!]!
114-
}
126+
expect(printSchema(result.schema.toAPISchema()))
127+
.toMatchInlineSnapshot(expectedApiSdl);
128+
}
129+
});
115130

116-
type Resource {
117-
id: ID!
118-
name: String!
119-
}"
120-
`);
131+
it("composes with completed definitions", () => {
132+
const completedSubgraphSdl = printSchema(buildSubgraph(
133+
"with-connectors",
134+
"",
135+
subgraphSdl,
136+
).schema);
137+
138+
const subgraphs = [
139+
{
140+
name: "with-connectors",
141+
typeDefs: parse(completedSubgraphSdl),
142+
},
143+
];
144+
145+
const result = composeServices(subgraphs);
146+
expect(result.errors ?? []).toEqual([]);
147+
const printed = printSchema(result.schema!);
148+
expect(printed).toMatchInlineSnapshot(expectedSupergraphSdl);
149+
150+
if (result.schema) {
151+
expect(printSchema(result.schema.toAPISchema()))
152+
.toMatchInlineSnapshot(expectedApiSdl);
121153
}
122154
});
123155

‎internals-js/src/buildSchema.ts‎

Lines changed: 51 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -56,6 +56,9 @@ import {
5656
} from "./definitions";
5757
import { ERRORS, errorCauses, withModifiedErrorNodes } from "./error";
5858
import { introspectionTypeNames } from "./introspection";
59+
import { coreFeatureDefinitionIfKnown } from "./knownCoreFeatures";
60+
import { connectIdentity } from "./specs/connectSpec";
61+
5962

6063
function buildValue(value?: ValueNode): any {
6164
return value ? valueFromASTUntyped(value) : undefined;
@@ -143,6 +146,48 @@ export function buildSchemaFromAST(
143146
buildSchemaDefinitionInner(schemaExtension, schema.schemaDefinition, errors, schema.schemaDefinition.newExtension());
144147
}
145148

149+
// The following block of code is a one-off to support input objects in the
150+
// connect spec. It will be non-maintainable/bug-prone to do this again, and
151+
// has various limitations/unsupported edge cases already.
152+
//
153+
// There's work to be done to support input objects more generally; please see
154+
// https://github.com/apollographql/federation/pull/3311 for more information.
155+
const connectFeature = schema.coreFeatures?.getByIdentity(connectIdentity);
156+
const handledConnectTypeNames = new Set<string>();
157+
if (connectFeature) {
158+
const connectFeatureDefinition =
159+
coreFeatureDefinitionIfKnown(connectFeature.url);
160+
if (connectFeatureDefinition) {
161+
const connectTypeNamesInSchema = new Set(
162+
connectFeatureDefinition.typeSpecs()
163+
.map(({ name }) => connectFeature.typeNameInSchema(name))
164+
);
165+
for (const typeNode of typeDefinitions) {
166+
if (connectTypeNamesInSchema.has(typeNode.name.value)
167+
&& typeNode.kind === 'InputObjectTypeDefinition'
168+
) {
169+
handledConnectTypeNames.add(typeNode.name.value)
170+
} else {
171+
continue;
172+
}
173+
buildNamedTypeInner(typeNode, schema.type(typeNode.name.value)!, schema.blueprint, errors);
174+
}
175+
for (const typeExtensionNode of typeExtensions) {
176+
if (connectTypeNamesInSchema.has(typeExtensionNode.name.value)
177+
&& typeExtensionNode.kind === 'InputObjectTypeExtension'
178+
) {
179+
handledConnectTypeNames.add(typeExtensionNode.name.value)
180+
} else {
181+
continue;
182+
}
183+
const toExtend = schema.type(typeExtensionNode.name.value)!;
184+
const extension = toExtend.newExtension();
185+
extension.sourceAST = typeExtensionNode;
186+
buildNamedTypeInner(typeExtensionNode, toExtend, schema.blueprint, errors, extension);
187+
}
188+
}
189+
}
190+
146191
// The following is a no-op for "standard" schema, but for federation subgraphs, this is where we handle the auto-addition
147192
// of imported federation directive definitions. That is why we have avoid looking at directive applications within
148193
// directive definition earlier: if one of those application was of an imported federation directive, the definition
@@ -155,9 +200,15 @@ export function buildSchemaFromAST(
155200
}
156201

157202
for (const typeNode of typeDefinitions) {
203+
if (handledConnectTypeNames.has(typeNode.name.value)) {
204+
continue;
205+
}
158206
buildNamedTypeInner(typeNode, schema.type(typeNode.name.value)!, schema.blueprint, errors);
159207
}
160208
for (const typeExtensionNode of typeExtensions) {
209+
if (handledConnectTypeNames.has(typeExtensionNode.name.value)) {
210+
continue;
211+
}
161212
const toExtend = schema.type(typeExtensionNode.name.value)!;
162213
const extension = toExtend.newExtension();
163214
extension.sourceAST = typeExtensionNode;

0 commit comments

Comments
 (0)