tests: stop using undocumented __resolveObject AS feature #1658
Conversation
This file contains 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
Apollo Server 2.2.1 introduced an experimental feature named __resolveObject. It was like an extra-powerful version of __resolveReference (which did not yet exist) which also allowed you to return references within a single subgraph and they'd be magically converted into full objects. It was never documented and was described multiple times as experimental. Its implementation is entirely inside the Apollo Server schema instrumentation, which is largely designed for enabling willResolveField plugins (eg, usage reporting). In fact, as the developer of the main feature in AS 3.6.0 (apollographql/apollo-server#5963) I added an optimization to not call schema instrumentation at all if there are no willResolveField plugin hooks... which means that this feature is broken in that case! We are likely to remove this feature in AS4. To get ready for that, stop using it in tests here. This involves changing the existing ones to `__resolveReference` and also adding some direct "object resolution" to resolvers that return objects within the "owning" service. This is an improvement anyway, so that our fixtures use the documented `__resolveReference` feature instead of a different obscure feature.
✅ Deploy Preview for apollo-federation-docs ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
|
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. |
glasser
added a commit
to apollographql/apollo-server
that referenced
this pull request
Mar 29, 2022
This removes the dependency on the deprecated
`@apollographql/apollo-tooling` package by removing support for two
undocumented features that it provides.
The `modules` constructor option was the beginning of an attempt to
provide guidance about how to structure one's schema into "modules".
While there were bigger goals for it, in practice all it allowed you to
do is specify your schema as an array of typeDefs/resolvers objects. It
used a different schema-making function that is similar to but
technically not the same as the `makeExecutableSchema` from
graphql-tools. `makeExecutableSchema` lets you specify both `typeDefs`
and `resolvers` these days, so you should be able to replace any use of
`modules` with
new ApolloServer({
typeDefs: modules.map({ typeDefs } => typeDefs,
resolvers: modules.map({ resolvers } => resolvers,
})
Or of course, you can use the `@apollographql/apollo-tooling`
`buildServiceDefinition` yourself and pass it to the `schema`
constructor option. The basic idea here is that `schema` is the main
option for specifying a (non-gateway) schema and that we provide
`typeDefs`/`resolvers` as syntactic sugar for the common case, but we
don't need to provide two different kinds of syntactic sugar for
specifying typeDefs and resolvers in slightly different data structures
which use surprisingly different code to implement it.
The other undocumented feature implemented by
`@apollographql/apollo-tooling` was the ability to put a
`__resolveObject` pseudo-resolver on a type. This was a predecessor to
the subgraph `__resolveReference` method which offered a superset of its
functionality. Whereas `__resolveReference` is specific to the
`Query._entities` field, `__resolveObject` would run everywhere: it
would run on that field's return values but also on any other field's
return value. Interestingly it was implemented inside the "schema
instrumentation" code which also implements the willResolveField plugin
hook. In v3.6.0 we stopped running schema instrumentation if you aren't
using a willResolveField hook (though admittedly the on-by-default cache
control plugin uses that hook) which would break this feature!
We don't know of any usage of this feature other than federation tests
(which we're updating in
apollographql/federation#1658) so for now we
will delete it. This would better be done in graphql-js anyway, or
perhaps as part of makeExecutableSchema.
|
@trevor-scheer btw I didn't make a PR for it but it would be good for you to review apollographql/apollo-server@1081080 on apollo-server version-4 as well! |
benweatherman
approved these changes
Mar 30, 2022
trevor-scheer
added a commit
that referenced
this pull request
Apr 22, 2022
Also including relevant work from #1658 to remove uses of __resolveObject
trevor-scheer
added a commit
that referenced
this pull request
Apr 22, 2022
Includes relevant work from #1658 to remove uses of `__resolveObject`
glasser
added a commit
that referenced
this pull request
Jul 22, 2022
…hQLDataSource I'm not exactly sure what the purpose of LocalGraphQLDataSource is, other than that it's a convenient thing to use in this repository's test suite. It is exported from `@apollo/gateway` but does not appear to be documented. It currently uses a function stolen from the heart of Apollo Server. We are working on eliminating the dependency of Gateway on Apollo Server (apollographql/apollo-server#6719, apollographql/apollo-server#6057) so this dependency on something that's not even part of the supported API seems worth looking into. `enablePluginsForSchemaResolvers` does two entirely unrelated things. One is the thing that its name says: it tweaks the schema so that it's possible to instrument execution on a per-field basis; this is used to implement Apollo Server's `willResolveField` plugin hook. The other thing it does is entirely undocumented: it enables an obscure and undocumented `__resolveObject` feature which we are already planning to remove in Apollo Server 4. As mentioned, this class seems to really just be a test helper for this repository, and in fact, the tests did use `__resolveObject` until a few months ago in #1658. So one might guess that the point of this line is just to enable `__resolveObject` in tests and that it can be entirely removed. And in fact, ff45cb7 shows that there used to be a comment here saying exactly that! So this commit removes the use of an undocumented internal function to enable an even less documented feature. The tests pass, so that seems good. Perhaps one could object to this as not fully backwards compatible, because perhaps there are users out there who are using LocalGraphQLDataSource directly and... expect willResolveField plugins to work? But that's not really something you can make work without the rest of Apollo Server anyway. I guess maybe they could actually be expecting `__resolveObject` to work? But at that point we're talking about theoretical people trying to combine two undocumented features (`LocalGraphQLDataSource` and `__resolveObject`). If we discover that those people exist, we can encourage them to just add this line back into their own code themselves. Surely, people who enjoy the thrill of using two undocumented features will be even happier using three undocumented features!
glasser
added a commit
that referenced
this pull request
Jul 22, 2022
…hQLDataSource (#2007) I'm not exactly sure what the purpose of LocalGraphQLDataSource is, other than that it's a convenient thing to use in this repository's test suite. It is exported from `@apollo/gateway` but does not appear to be documented. It currently uses a function stolen from the heart of Apollo Server. We are working on eliminating the dependency of Gateway on Apollo Server (apollographql/apollo-server#6719, apollographql/apollo-server#6057) so this dependency on something that's not even part of the supported API seems worth looking into. `enablePluginsForSchemaResolvers` does two entirely unrelated things. One is the thing that its name says: it tweaks the schema so that it's possible to instrument execution on a per-field basis; this is used to implement Apollo Server's `willResolveField` plugin hook. The other thing it does is entirely undocumented: it enables an obscure and undocumented `__resolveObject` feature which we are already planning to remove in Apollo Server 4. As mentioned, this class seems to really just be a test helper for this repository, and in fact, the tests did use `__resolveObject` until a few months ago in #1658. So one might guess that the point of this line is just to enable `__resolveObject` in tests and that it can be entirely removed. And in fact, ff45cb7 shows that there used to be a comment here saying exactly that! So this commit removes the use of an undocumented internal function to enable an even less documented feature inside an undocumented class. The tests pass, so that seems good. Perhaps one could object to this as not fully backwards compatible, because perhaps there are users out there who are using LocalGraphQLDataSource directly and... expect willResolveField plugins to work? But that's not really something you can make work without the rest of Apollo Server anyway. I guess maybe they could actually be expecting `__resolveObject` to work? But at that point we're talking about theoretical people trying to combine two undocumented features (`LocalGraphQLDataSource` and `__resolveObject`). If we discover that those people exist, we can encourage them to just add this line back into their own code themselves. Surely, people who enjoy the thrill of using two undocumented features will be even happier using three undocumented features!
glasser
added a commit
that referenced
this pull request
Jul 22, 2022
…hQLDataSource (#2007) I'm not exactly sure what the purpose of LocalGraphQLDataSource is, other than that it's a convenient thing to use in this repository's test suite. It is exported from `@apollo/gateway` but does not appear to be documented. It currently uses a function stolen from the heart of Apollo Server. We are working on eliminating the dependency of Gateway on Apollo Server (apollographql/apollo-server#6719, apollographql/apollo-server#6057) so this dependency on something that's not even part of the supported API seems worth looking into. `enablePluginsForSchemaResolvers` does two entirely unrelated things. One is the thing that its name says: it tweaks the schema so that it's possible to instrument execution on a per-field basis; this is used to implement Apollo Server's `willResolveField` plugin hook. The other thing it does is entirely undocumented: it enables an obscure and undocumented `__resolveObject` feature which we are already planning to remove in Apollo Server 4. As mentioned, this class seems to really just be a test helper for this repository, and in fact, the tests did use `__resolveObject` until a few months ago in #1658. So one might guess that the point of this line is just to enable `__resolveObject` in tests and that it can be entirely removed. And in fact, ff45cb7 shows that there used to be a comment here saying exactly that! So this commit removes the use of an undocumented internal function to enable an even less documented feature inside an undocumented class. The tests pass, so that seems good. Perhaps one could object to this as not fully backwards compatible, because perhaps there are users out there who are using LocalGraphQLDataSource directly and... expect willResolveField plugins to work? But that's not really something you can make work without the rest of Apollo Server anyway. I guess maybe they could actually be expecting `__resolveObject` to work? But at that point we're talking about theoretical people trying to combine two undocumented features (`LocalGraphQLDataSource` and `__resolveObject`). If we discover that those people exist, we can encourage them to just add this line back into their own code themselves. Surely, people who enjoy the thrill of using two undocumented features will be even happier using three undocumented features!
glasser
added a commit
that referenced
this pull request
Jul 22, 2022
…hQLDataSource (#2007) (#2008) I'm not exactly sure what the purpose of LocalGraphQLDataSource is, other than that it's a convenient thing to use in this repository's test suite. It is exported from `@apollo/gateway` but does not appear to be documented. It currently uses a function stolen from the heart of Apollo Server. We are working on eliminating the dependency of Gateway on Apollo Server (apollographql/apollo-server#6719, apollographql/apollo-server#6057) so this dependency on something that's not even part of the supported API seems worth looking into. `enablePluginsForSchemaResolvers` does two entirely unrelated things. One is the thing that its name says: it tweaks the schema so that it's possible to instrument execution on a per-field basis; this is used to implement Apollo Server's `willResolveField` plugin hook. The other thing it does is entirely undocumented: it enables an obscure and undocumented `__resolveObject` feature which we are already planning to remove in Apollo Server 4. As mentioned, this class seems to really just be a test helper for this repository, and in fact, the tests did use `__resolveObject` until a few months ago in #1658. So one might guess that the point of this line is just to enable `__resolveObject` in tests and that it can be entirely removed. And in fact, ff45cb7 shows that there used to be a comment here saying exactly that! So this commit removes the use of an undocumented internal function to enable an even less documented feature inside an undocumented class. The tests pass, so that seems good. Perhaps one could object to this as not fully backwards compatible, because perhaps there are users out there who are using LocalGraphQLDataSource directly and... expect willResolveField plugins to work? But that's not really something you can make work without the rest of Apollo Server anyway. I guess maybe they could actually be expecting `__resolveObject` to work? But at that point we're talking about theoretical people trying to combine two undocumented features (`LocalGraphQLDataSource` and `__resolveObject`). If we discover that those people exist, we can encourage them to just add this line back into their own code themselves. Surely, people who enjoy the thrill of using two undocumented features will be even happier using three undocumented features!
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.
Apollo Server 2.2.1 introduced an experimental feature named
__resolveObject. It was like an extra-powerful version of
__resolveReference (which did not yet exist) which also allowed you to
return references within a single subgraph and they'd be magically
converted into full objects. It was never documented and was described
multiple times as experimental.
Its implementation is entirely inside the Apollo Server schema
instrumentation, which is largely designed for enabling willResolveField
plugins (eg, usage reporting). In fact, as the developer of the main
feature in AS 3.6.0
(apollographql/apollo-server#5963) I added an
optimization to not call schema instrumentation at all if there are no
willResolveField plugin hooks... which means that this feature is broken
in that case!
We are likely to remove this feature in AS4. To get ready for that, stop
using it in tests here. This involves changing the existing ones to
__resolveReferenceand also adding some direct "object resolution" toresolvers that return objects within the "owning" service.
This is an improvement anyway, so that our fixtures use the documented
__resolveReferencefeature instead of a different obscure feature.