-
Notifications
You must be signed in to change notification settings - Fork 269
Implement one-off support for input objects in the connect spec #3311
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Implement one-off support for input objects in the connect spec #3311
Conversation
|
✅ Docs preview readyThe preview is ready to be viewed. View the preview File Changes 0 new, 2 changed, 0 removed
Build ID: 81d96df8ffd90a0b0d48cfef URL: https://www.apollographql.com/docs/deploy-preview/81d96df8ffd90a0b0d48cfef |
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. |
0ef0e70
to
ae240a6
Compare
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.
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). NOTE: Support for handling input objects in the spec is severely limited and only handles `@connect` spec. For additional details on limitations see #3311. <!-- FED-809 --> --------- Co-authored-by: Sachin D. Shinde <[email protected]>
This PR was opened by the [Changesets release](https://github.com/changesets/action) GitHub action. When you're ready to do a release, you can merge this and the packages will be published to npm automatically. If you're not ready to do a release yet, that's fine, whenever you add more changesets to main, this PR will be updated. # Releases ## @apollo/[email protected] ### Patch Changes - Updated dependencies \[[`8c7a2cd655ad3060e9f5c3b106cfbdb59251701c`](8c7a2cd)]: - @apollo/[email protected] - @apollo/[email protected] ## @apollo/[email protected] ### Patch Changes - Updated dependencies \[[`4faa114215200daf7ad7518be8e50071fcde783c`](4faa114), [`8c7a2cd655ad3060e9f5c3b106cfbdb59251701c`](8c7a2cd)]: - @apollo/[email protected] - @apollo/[email protected] - @apollo/[email protected] ## @apollo/[email protected] ### Patch Changes - Update connector spec to allow re-entry ([#3312](#3312)) 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). NOTE: Support for handling input objects in the spec is severely limited and only handles `@connect` spec. For additional details on limitations see #3311. ## @apollo/[email protected] ### Patch Changes - Updated dependencies \[[`8c7a2cd655ad3060e9f5c3b106cfbdb59251701c`](8c7a2cd)]: - @apollo/[email protected] ## @apollo/[email protected] ### Patch Changes - Fix bug in query planning where a subgraph jump for `@requires` can sometimes try to fetch `@key` fields from a subgraph that doesn't have them. This bug would previously cause query planning to error with a message that looks like "Cannot add selection of field `T.id` to selection set of parent type `T`". ([#3307](#3307)) - Updated dependencies \[[`8c7a2cd655ad3060e9f5c3b106cfbdb59251701c`](8c7a2cd)]: - @apollo/[email protected] - @apollo/[email protected] ## @apollo/[email protected] ### Patch Changes - 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. ([#3285](#3285)) ([#3285](#3285)) - Updated dependencies \[[`8c7a2cd655ad3060e9f5c3b106cfbdb59251701c`](8c7a2cd)]: - @apollo/[email protected] ## [email protected] Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
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:
Schema
) uses JS singleton objects for schema elements.This means that schema building has to proceed in a two-phase process.
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 thefederation
andconnect
specs. These missing definitions/extensions are added (or if already added, they're checked) by thecheckOrAdd()
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:checkOrAdd()
for a type/directive spec to fill in the missing definitions.checkOrAdd()
for a type/directive specification, in bundling everything together, requires the types/directives it references to be fully built.This has several implications:
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.checkOrAdd()
calls, otherwise the checking part fails.checkOrAdd()
(after processing@link
s).In order to support connectors usage of input object types in the short-term, we must do a few things.
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: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.