From 997fc4bc356ec3105188d57b0af178332a9be1db Mon Sep 17 00:00:00 2001 From: Jesse Rosenberger Date: Thu, 28 May 2020 16:14:02 +0000 Subject: [PATCH] server-testing: Ensure user-provided context objects are cloned. The `apollo-server-testing` package uses an internal Apollo Server method called `executeOperation` (introduced in [#1909]) in order to power its `createTestClient` functionality. This is the testing practice which is documented within [Integration testing] in the Apollo Server documentation. However, it failed to introduce the same context-cloning which [takes place in `runHttpQuery`][Ref 1], prior to arriving at the main request pipeline. Since the context was not cloned, and we had made the expectation in [#3988] that it was a unique context on every single request (which it was, in a non-testing context), the Symbol we use to implement `willResolveField` was already present [on the request pipeline][Ref 2] when running a subsequent test via `createTestClient`! This commit introduces the same cloning that takes place in `buildRequestContext` within `runHttpQuery`, and adds tests to ensure the behavior is preserved. [Fixes #4170]: https://github.com/apollographql/apollo-server/issues/4170 [#1909]: https://github.com/apollographql/apollo-server/pull/1909 [Integration testing]: https://www.apollographql.com/docs/apollo-server/testing/testing/ [Ref 1]: https://git.io/Jfou6 [#3988]: https://github.com/apollographql/apollo-server/pull/3988 [Ref 2]: https://git.io/Jfouy --- CHANGELOG.md | 4 + .../apollo-server-core/src/ApolloServer.ts | 10 ++ .../apollo-server-core/src/runHttpQuery.ts | 3 +- .../src/ApolloServer.ts | 142 ++++++++++++++---- 4 files changed, 131 insertions(+), 28 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 61132a08c98..e0b00c9f548 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,10 @@ The version headers in this history reflect the versions of Apollo Server itself - _Nothing yet! Stay tuned!_ +### v2.14.1 + +- `apollo-server-testing`: Ensure that user-provided context is cloned when using `createTestClient`, per the instructions in the [intergration testing]() section of the Apollo Server documentation. [Issue #4170](https://github.com/apollographql/apollo-server/issues/4170) [PR #TODO](https://github.com/apollographql/apollo-server/pull/TODO) + ### v2.14.0 - `apollo-server-core` / `apollo-server-plugin-base`: Add support for `willResolveField` and corresponding end-handler within `executionDidStart`. This brings the remaining bit of functionality that was previously only available from `graphql-extensions` to the new plugin API. The `graphql-extensions` API (which was never documented) will be deprecated in Apollo Server 3.x. To see the documentation for the request pipeline API, see [its documentation](https://www.apollographql.com/docs/apollo-server/integrations/plugins/). For more details, see the attached PR. [PR #3988](https://github.com/apollographql/apollo-server/pull/3988) diff --git a/packages/apollo-server-core/src/ApolloServer.ts b/packages/apollo-server-core/src/ApolloServer.ts index 18181de0eb9..d9ad6be6e7c 100644 --- a/packages/apollo-server-core/src/ApolloServer.ts +++ b/packages/apollo-server-core/src/ApolloServer.ts @@ -78,6 +78,7 @@ import { CacheControlExtensionOptions, } from 'apollo-cache-control'; import { getEngineApiKey, getEngineGraphVariant } from "apollo-engine-reporting/dist/agent"; +import { cloneObject } from "./runHttpQuery"; const NoIntrospection = (context: ValidationContext) => ({ Field(node: FieldDefinitionNode) { @@ -855,8 +856,17 @@ export class ApolloServerBase { if (typeof options.context === 'function') { options.context = (options.context as () => never)(); + } else if (typeof options.context === 'object') { + // FIXME: We currently shallow clone the context for every request, + // but that's unlikely to be what people want. + // We allow passing in a function for `context` to ApolloServer, + // but this only runs once for a batched request (because this is resolved + // in ApolloServer#graphQLServerOptions, before runHttpQuery is invoked). + // NOTE: THIS IS DUPLICATED IN runHttpQuery.ts' buildRequestContext. + options.context = cloneObject(options.context); } + const requestCtx: GraphQLRequestContext = { logger: this.logger, schema: options.schema, diff --git a/packages/apollo-server-core/src/runHttpQuery.ts b/packages/apollo-server-core/src/runHttpQuery.ts index 90053cc2c28..dd86f349464 100644 --- a/packages/apollo-server-core/src/runHttpQuery.ts +++ b/packages/apollo-server-core/src/runHttpQuery.ts @@ -246,6 +246,7 @@ export async function processHTTPRequest( // We allow passing in a function for `context` to ApolloServer, // but this only runs once for a batched request (because this is resolved // in ApolloServer#graphQLServerOptions, before runHttpQuery is invoked). + // NOTE: THIS IS DUPLICATED IN ApolloServerBase.prototype.executeOperation. const context = cloneObject(options.context); return { // While `logger` is guaranteed by internal Apollo Server usage of @@ -458,6 +459,6 @@ function prettyJSONStringify(value: any) { return JSON.stringify(value) + '\n'; } -function cloneObject(object: T): T { +export function cloneObject(object: T): T { return Object.assign(Object.create(Object.getPrototypeOf(object)), object); } diff --git a/packages/apollo-server-integration-testsuite/src/ApolloServer.ts b/packages/apollo-server-integration-testsuite/src/ApolloServer.ts index 85257054458..e2942795b71 100644 --- a/packages/apollo-server-integration-testsuite/src/ApolloServer.ts +++ b/packages/apollo-server-integration-testsuite/src/ApolloServer.ts @@ -566,11 +566,13 @@ export function testApolloServer( describe('Plugins', () => { let apolloFetch: ApolloFetch; let apolloFetchResponse: ParsedResponse; + let serverInstance: ApolloServerBase; const setupApolloServerAndFetchPairForPlugins = async ( plugins: PluginDefinition[] = [], ) => { - const { url: uri } = await createApolloServer({ + const { url: uri, server } = await createApolloServer({ + context: { customContext: true }, typeDefs: gql` type Query { justAField: String @@ -579,6 +581,8 @@ export function testApolloServer( plugins, }); + serverInstance = server; + apolloFetch = createApolloFetch({ uri }) // Store the response so we can inspect it. .useAfter(({ response }, next) => { @@ -587,6 +591,56 @@ export function testApolloServer( }); }; + // Test for https://github.com/apollographql/apollo-server/issues/4170 + it('works when using executeOperation', async () => { + const encounteredFields = []; + const encounteredContext = []; + await setupApolloServerAndFetchPairForPlugins([ + { + requestDidStart: () => ({ + executionDidStart: () => ({ + willResolveField({ info, context }) { + encounteredFields.push(info.path); + encounteredContext.push(context); + }, + }), + }), + }, + ]); + + // The bug in 4170 (linked above) was occurring because of a failure + // to the context in `executeOperation`, in the same way that occurs + // in `runHttpQuery` prior to entering the request pipeline. That + // resulted in the inability to attach a symbol to the context because + // the symbol already existed on the context. Of course, a context + // is only created after the first invocation, so we'll run this twice + // to encounter the error where it was in the way when we tried to set + // it the second time. While we could have tested for the property + // before assigning to it, that is not the contract we have with the + // context, which should have been copied on `executeOperation` (which + // is meant to be used by testing, currently). + await serverInstance.executeOperation({ + query: '{ justAField }', + }); + await serverInstance.executeOperation({ + query: '{ justAField }', + }); + + expect(encounteredFields).toStrictEqual([ + { key: 'justAField', prev: undefined }, + { key: 'justAField', prev: undefined }, + ]); + + // This bit is just to ensure that nobody removes `context` from the + // `setupApolloServerAndFetchPairForPlugins` thinking it's unimportant. + // When a custom context is not provided, a new one is initialized + // on each request. + expect(encounteredContext).toStrictEqual([ + expect.objectContaining({customContext: true}), + expect.objectContaining({customContext: true}), + ]); + }); + it('returns correct status code for a normal operation', async () => { await setupApolloServerAndFetchPairForPlugins(); @@ -1492,37 +1546,71 @@ export function testApolloServer( expect(spy).toHaveBeenCalledTimes(2); }); - it('clones the context for every request', async () => { - const uniqueContext = { key: 'major' }; - const spy = jest.fn(() => 'hi'); - const typeDefs = gql` - type Query { - hello: String - } - `; - const resolvers = { - Query: { - hello: (_parent: any, _args: any, context: any) => { - expect(context.key).toEqual('major'); - context.key = 'minor'; - return spy(); + describe('context cloning', () => { + it('clones the context for request pipeline requests', async () => { + const uniqueContext = { key: 'major' }; + const spy = jest.fn(() => 'hi'); + const typeDefs = gql` + type Query { + hello: String + } + `; + const resolvers = { + Query: { + hello: (_parent: any, _args: any, context: any) => { + expect(context.key).toEqual('major'); + context.key = 'minor'; + return spy(); + }, }, - }, - }; - const { url: uri } = await createApolloServer({ - typeDefs, - resolvers, - context: uniqueContext, + }; + const { url: uri } = await createApolloServer({ + typeDefs, + resolvers, + context: uniqueContext, + }); + + const apolloFetch = createApolloFetch({ uri }); + + expect(spy).not.toBeCalled(); + + await apolloFetch({ query: '{hello}' }); + expect(spy).toHaveBeenCalledTimes(1); + await apolloFetch({ query: '{hello}' }); + expect(spy).toHaveBeenCalledTimes(2); }); - const apolloFetch = createApolloFetch({ uri }); + // https://github.com/apollographql/apollo-server/issues/4170 + it('for every request with executeOperation', async () => { + const uniqueContext = { key: 'major' }; + const spy = jest.fn(() => 'hi'); + const typeDefs = gql` + type Query { + hello: String + } + `; + const resolvers = { + Query: { + hello: (_parent: any, _args: any, context: any) => { + expect(context.key).toEqual('major'); + context.key = 'minor'; + return spy(); + }, + }, + }; + const { server } = await createApolloServer({ + typeDefs, + resolvers, + context: uniqueContext, + }); - expect(spy).not.toBeCalled(); + expect(spy).not.toBeCalled(); - await apolloFetch({ query: '{hello}' }); - expect(spy).toHaveBeenCalledTimes(1); - await apolloFetch({ query: '{hello}' }); - expect(spy).toHaveBeenCalledTimes(2); + await server.executeOperation({ query: '{hello}' }); + expect(spy).toHaveBeenCalledTimes(1); + await server.executeOperation({ query: '{hello}' }); + expect(spy).toHaveBeenCalledTimes(2); + }); }); describe('as a function', () => {