fix: update connector spec to allow re-entry #3310
Merged
+546
−215
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
✅ Docs preview has no changesThe preview was not built because there were no changes. Build ID: 03861b899de3664e080f80c8 |
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. |
dariuszkuc
force-pushed
the
dk/connector_spec_fix
branch
from
db13590 to
9b7d228
Compare
lennyburdette
approved these changes
Oct 1, 2025
sachindshinde
requested changes
Oct 6, 2025
There was a problem hiding this comment.
There's various changes we need to do to support spec input object types in the general case. For now though, we can do a one-off hack for connectors.
It's complicated to explain/implement, so I've filed a PR on top of this PR's branch at #3311. That PR also addresses all the comments I've made below. Once that PR merges into this one, it should be good to go.
| return errors; | ||
| } | ||
| assert(isInputObjectType(existing), 'Should be an input object type'); | ||
| for (const { name: field_name, type, defaultValue } of expectedFields) { |
There was a problem hiding this comment.
This loop over input fields resembles createObjectTypeSpecification(), but it should resemble ensureSameArguments() more, as input fields are really more like arguments. (There's some further restrictions I've made in my PR specifically, it's in the code comments.)
| const createdType = schema.addType(new InputObjectType(actualName, asBuiltIn)); | ||
| for (const { name, type, defaultValue } of expectedFields) { | ||
| const newField = createdType.addField(name, type); | ||
| if (defaultValue) { |
There was a problem hiding this comment.
defaultValue is just a POJO in this case, so we can't rely on its JS truthiness; we can just unequivocally set newField.defaultValue = defaultValue instead.
| # added in v0.2 | ||
| path: JSONSelection | ||
| query: JSONSelection |
There was a problem hiding this comment.
This is a bug in the original code's comment, but it should be queryParams.
| # added in v0.2 | ||
| path: JSONSelection | ||
| query: JSONSelection |
There was a problem hiding this comment.
This is a bug in the original code's comment, but it should be queryParams.
| return [ | ||
| { | ||
| name: 'baseURL', | ||
| type: schema.stringType() |
There was a problem hiding this comment.
The field is baseURL: String!, so this needs a non-null wrapper.
| defaultValue: false | ||
| }, | ||
| { | ||
| name: 'errors', |
There was a problem hiding this comment.
nit: The errors argument is ordered between batch and selection in the original code.
| { | ||
| name: 'selection', | ||
| type: (schema, feature) => | ||
| lookupFeatureTypeInSchema<ScalarType>(JSON_SELECTION, 'ScalarType', schema, feature) |
There was a problem hiding this comment.
The argument is selection: JSONSelection!, so this needs a non-null wrapper.
| } | ||
| directive @source( | ||
| name: String! | ||
| http: SourceHttp! |
There was a problem hiding this comment.
This type should be SourceHTTP! instead of SourceHttp!.
sachindshinde
added a commit
that referenced
this pull request
Oct 6, 2025
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.
sachindshinde
approved these changes
Oct 6, 2025
Updates connector spec to follow the same patterns as other federation spec blueprints (i.e. register types/directives in the constructor and use default logic for adding them to the schema that checks whether they need to be added or not).
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.
dariuszkuc
force-pushed
the
dk/connector_spec_fix
branch
from
3df4612 to
f406377
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Updates connector spec to follow the same patterns as other federation spec blueprints (i.e. register types/directives in the constructor and use default logic for adding them to the schema that checks whether they need to be added or not).