Skip to content

Commit

Permalink
TODO(AS4) squashing (#6936)
Browse files Browse the repository at this point in the history
Address a number of v4-related TODOS.

Most notably (copied from changesets):
* Update executeOperation second parameter to be an optional options
object which includes an optional `contextValue`.
* `HTTPGraphQLRequest` now uses a specific `HeaderMap` class which we
export instead of allowing a standard `Map`. The `HeaderMap` downcases
all incoming keys, as header names are not case-sensitive.
* Update `validationRules` typing for correctness. This is sort of a
breaking change for TS users in that the types were more permissive than
they should have been. All `validationRules` list items should conform
to the `graphql-js` `ValidationRule` type.
* Add test for batch requests with no elements (this potentially affects
integration testsuite users)

<!--
First, 🌠 thank you 🌠 for taking the time to consider a contribution to
Apollo!

Here are some important details to follow:

* ⏰ Your time is important
To save your precious time, if the contribution you are making will take
more
than an hour, please make sure it has been discussed in an issue first.
          This is especially true for feature requests!
* 💡 Features
Feature requests can be created and discussed within a GitHub Issue. Be
sure to search for existing feature requests (and related issues!) prior
to
opening a new request. If an existing issue covers the need, please
upvote
that issue by using the 👍 emote, rather than opening a new issue.
* 🔌 Integrations
Apollo Server has many web-framework integrations including Express,
Koa,
Hapi and more. When adding a new feature, or fixing a bug, please take a
peak and see if other integrations are also affected. In most cases, the
fix can be applied to the other frameworks as well. Please note that,
since new web-frameworks have a high maintenance cost, pull-requests for
new web-frameworks should be discussed with a project maintainer first.
* 🕷 Bug fixes
These can be created and discussed in this repository. When fixing a
bug,
please _try_ to add a test which verifies the fix. If you cannot, you
should
still submit the PR but we may still ask you (and help you!) to create a
test.
* 📖 Contribution guidelines
Follow
https://github.com/apollographql/apollo-server/blob/main/CONTRIBUTING.md
when submitting a pull request. Make sure existing tests still pass, and
add
          tests for all new behavior.
* ✏️ Explain your pull request
Describe the big picture of your changes here to communicate to what
your
pull request is meant to accomplish. Provide 🔗 links 🔗 to associated
issues!

We hope you will find this to be a positive experience! Open source
contribution can be intimidating and we hope to alleviate that pain as
much as possible. Without following these guidelines, you may be missing
context that can help you succeed with your contribution, which is why
we encourage discussion first. Ultimately, there is no guarantee that we
will be able to merge your pull-request, but by following these
guidelines we can try to avoid disappointment.
-->
  • Loading branch information
trevor-scheer authored Sep 26, 2022
1 parent 054dcbb commit a404bf1
Show file tree
Hide file tree
Showing 30 changed files with 215 additions and 207 deletions.
5 changes: 5 additions & 0 deletions .changeset/big-kiwis-explain.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'@apollo/server': patch
---

Update executeOperation second parameter to be an optional options object which includes an optional `contextValue`.
5 changes: 5 additions & 0 deletions .changeset/healthy-deers-search.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'@apollo/server': patch
---

`HTTPGraphQLRequest` now uses a specific `HeaderMap` class which we export instead of allowing a standard `Map`. The `HeaderMap` downcases all incoming keys, as header names are not case-sensitive.
5 changes: 5 additions & 0 deletions .changeset/late-numbers-pull.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'@apollo/server': patch
---

Update `validationRules` typing for correctness. This is sort of a breaking change for TS users in that the types were more permissive than they should have been. All `validationRules` list items should conform to the `graphql-js` `ValidationRule` type.
5 changes: 5 additions & 0 deletions .changeset/witty-moles-fail.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'@apollo/server-integration-testsuite': patch
---

Add test for batch requests with no elements
4 changes: 3 additions & 1 deletion docs/source/api/apollo-server.mdx
Original file line number Diff line number Diff line change
Expand Up @@ -330,6 +330,8 @@ An object containing configuration options for connecting Apollo Server to [Apol

Controls whether to allow [Batching Queries](../workflow/requests/#batching) in a single HTTP Request. Defaults to `false`. If a request comes in formatted as an array rather than as a single request object, an error will be thrown ( i.e., `Operation batching disabled`) _unless_ batching is enabled.

> Note: In the case of duplicate response headers across separate requests, the later request's header will take precedence on a per-header basis.
</td>
</tr>

Expand Down Expand Up @@ -599,7 +601,7 @@ The `executeOperation` method takes two arguments:

- The first argument is an object describing the GraphQL operation to be executed.
- Supported fields are listed in the table below.
- The second optional argument is used as the operation's [context value](../data/resolvers/#the-context-argument). Note, this argument is only optional if your server _doesn't_ expect a context value (i.e., your server uses the default context because you didn't explicitly provide another one).
- The second argument is the optional options object. This object includes the operation's optional [`contextValue`](../data/resolvers/#the-context-argument). Note, this argument is only optional if your server _doesn't_ expect a context value (i.e., your server uses the default context because you didn't explicitly provide another one).

The `response` object returned from `executeOperation` is a `GraphQLResponse`, which has `body` and `http` fields.

Expand Down
24 changes: 18 additions & 6 deletions docs/source/migration.mdx
Original file line number Diff line number Diff line change
Expand Up @@ -1119,7 +1119,7 @@ So a test for Apollo Server 3 that looks like this:

<MultiCodeBlock>

```ts {13-17}
```ts
const server = new ApolloServer({
typeDefs: "type Query { hello: String!}",
resolvers: {
Expand All @@ -1133,9 +1133,13 @@ const server = new ApolloServer({
const result = await server.executeOperation({
query: 'query helloContext { hello }',
}, {
// A half-hearted attempt at making something vaguely like an express.Request,
// and not bothering to make the express.Response at all.
req: { headers: { name: 'world' } },
// highlight-start
contextValue: {
// A half-hearted attempt at making something vaguely like an express.Request,
// and not bothering to make the express.Response at all.
req: { headers: { name: 'world' } },
}
// highlight-end
});

expect(result.data?.hello).toBe('Hello world!'); // -> true
Expand All @@ -1147,7 +1151,7 @@ looks like this in Apollo Server 4:

<MultiCodeBlock>

```ts {16-18}
```ts
interface MyContext {
name: string;
}
Expand All @@ -1164,7 +1168,11 @@ const server = new ApolloServer<MyContext>({
const { body } = await server.executeOperation({
query: 'query helloContext { hello }',
}, {
name: 'world',
// highlight-start
contextValue: {
name: 'world',
}
// highlight-end
});

// Note the use of Node's assert rather than Jest's expect; if using
Expand Down Expand Up @@ -1676,6 +1684,10 @@ const { url } = await startStandaloneServer(apolloServerInstance, {
});
```

### Improved typing for `validationRules`

In Apollo Server 3, the `validationRules` option was declared as a list of functions that returned `any`. However, at runtime the functions actually needed to return a `graphql-js` `ASTVisitor`. In Apollo Server 4, the TypeScript type ensures that `validationRules` is a list of `graphql-js` `ValidationRule`s. If your `validationRules` has a TypeScript error, you will need to fix one or more of your rules to correctly return `ASTVisitor`s.

### `@apollo/utils.fetcher` replaces `apollo-server-env`

In Apollo Server 3, the `apollo-server-env` package primarily provides TypeScript typings and polyfills for the `fetch` and `URL` APIs.
Expand Down
13 changes: 11 additions & 2 deletions docs/source/testing/testing.mdx
Original file line number Diff line number Diff line change
Expand Up @@ -103,8 +103,17 @@ it('fetches single launch', async () => {
query: GET_LAUNCH,
variables: { id: 1 },
},
// this second argument is used as the context for our operation
{ user: { id: 1, email: 'a@a.a' }, dataSources: { userAPI, launchAPI } }, //highlight-line
{
// highlight-start
contextValue: {
user: { id: 1, email: 'a@a.a' },
dataSources: {
userAPI,
launchAPI,
},
},
// highlight-end
},
);

expect(res).toMatchSnapshot();
Expand Down
2 changes: 2 additions & 0 deletions docs/source/workflow/requests.md
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,8 @@ If you have enabled HTTP batching, you can send a batch of queries in a single `

If you send a batched request, Apollo Server responds with a corresponding array of GraphQL responses.

> Note: In the case of duplicate response headers across separate requests, the later request's header will take precedence on a per-header basis.
## GET requests

Apollo Server also accepts `GET` requests for queries (but not mutations). With a `GET` request, query details (`query`, `operationName`, `variables`) are provided as URL query parameters. The `variables` option is a URL-escaped JSON object.
Expand Down
1 change: 0 additions & 1 deletion packages/integration-testsuite/.npmignore
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
*
node_modules/**/*
!src/**/*
!dist/**/*
!package.json
Expand Down
83 changes: 14 additions & 69 deletions packages/integration-testsuite/src/apolloServerTests.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,10 +13,9 @@ import {
GraphQLString,
GraphQLError,
ValidationContext,
FieldDefinitionNode,
ResponsePath,
DocumentNode,
printSchema,
FieldNode,
} from 'graphql';

// Note that by doing deep imports here we don't need to install React.
Expand All @@ -29,11 +28,12 @@ import {
ApolloFetch,
ParsedResponse,
} from './apolloFetch.js';
import type {
import {
ApolloServerOptions,
ApolloServer,
BaseContext,
ApolloServerPlugin,
HeaderMap,
} from '@apollo/server';
import fetch, { type Headers } from 'node-fetch';

Expand Down Expand Up @@ -209,7 +209,7 @@ export function defineIntegrationTestSuiteApolloServerTests(
describe('validation rules', () => {
it('accepts additional rules', async () => {
const NoTestString = (context: ValidationContext) => ({
Field(node: FieldDefinitionNode) {
Field(node: FieldNode) {
if (node.name.value === 'testString') {
context.reportError(
new GraphQLError('Not allowed to use', { nodes: [node] }),
Expand Down Expand Up @@ -538,12 +538,11 @@ export function defineIntegrationTestSuiteApolloServerTests(
describe('Plugins', () => {
let apolloFetch: ApolloFetch;
let apolloFetchResponse: ParsedResponse;
let serverInstance: ApolloServer<BaseContext>;

const setupApolloServerAndFetchPairForPlugins = async (
plugins: ApolloServerPlugin<BaseContext>[] = [],
) => {
const { server, url } = await createServer(
const { url } = await createServer(
{
typeDefs: gql`
type Query {
Expand All @@ -555,8 +554,6 @@ export function defineIntegrationTestSuiteApolloServerTests(
{ context: async () => ({ customContext: true }) },
);

serverInstance = server;

apolloFetch = createApolloFetch({ uri: url })
// Store the response so we can inspect it.
.useAfter(({ response }, next) => {
Expand All @@ -565,64 +562,6 @@ export function defineIntegrationTestSuiteApolloServerTests(
});
};

// TODO(AS4): Is this test still relevant now that we pass
// the context explicitly to executeOperation?
// Test for https://github.com/apollographql/apollo-server/issues/4170
it('works when using executeOperation', async () => {
const encounteredFields: ResponsePath[] = [];
const encounteredContext: BaseContext[] = [];
await setupApolloServerAndFetchPairForPlugins([
{
requestDidStart: async () => ({
executionDidStart: async () => ({
willResolveField({ info, contextValue }) {
encounteredFields.push(info.path);
encounteredContext.push(contextValue);
},
}),
}),
},
]);

// The bug in 4170 (linked above) was occurring because of a failure
// to clone 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 }',
},
{ customContext: true },
);
await serverInstance.executeOperation(
{
query: '{ justAField }',
},
{ customContext: true },
);

expect(encounteredFields).toStrictEqual([
{ key: 'justAField', prev: undefined, typename: 'Query' },
{ key: 'justAField', prev: undefined, typename: 'Query' },
]);

// 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();

Expand Down Expand Up @@ -1664,9 +1603,15 @@ export function defineIntegrationTestSuiteApolloServerTests(

expect(spy).not.toBeCalled();

await server.executeOperation({ query: '{hello}' }, uniqueContext);
await server.executeOperation(
{ query: '{hello}' },
{ contextValue: uniqueContext },
);
expect(spy).toHaveBeenCalledTimes(1);
await server.executeOperation({ query: '{hello}' }, uniqueContext);
await server.executeOperation(
{ query: '{hello}' },
{ contextValue: uniqueContext },
);
expect(spy).toHaveBeenCalledTimes(2);
});
});
Expand Down Expand Up @@ -1847,7 +1792,7 @@ export function defineIntegrationTestSuiteApolloServerTests(
extensions: {
http: {
status: 404,
headers: new Map([['special', 'hello']]),
headers: new HeaderMap([['special', 'hello']]),
},
},
}),
Expand Down
25 changes: 23 additions & 2 deletions packages/integration-testsuite/src/httpServerTests.ts
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ import type {
GraphQLRequestListener,
PersistedQueryOptions,
} from '@apollo/server';
import { HeaderMap } from '@apollo/server';
import { ApolloServerPluginCacheControl } from '@apollo/server/plugin/cacheControl';
import { ApolloServerPluginCacheControlDisabled } from '@apollo/server/plugin/disabled';
import {
Expand Down Expand Up @@ -202,7 +203,7 @@ const queryType = new GraphQLObjectType({
resolve() {
throw new GraphQLError('error 2', {
extensions: {
http: { headers: new Map([['erroneous', 'indeed']]) },
http: { headers: new HeaderMap([['erroneous', 'indeed']]) },
},
});
},
Expand All @@ -212,7 +213,7 @@ const queryType = new GraphQLObjectType({
resolve() {
throw new GraphQLError('error 3', {
extensions: {
http: { headers: new Map([['felonious', 'nah']]) },
http: { headers: new HeaderMap([['felonious', 'nah']]) },
},
});
},
Expand Down Expand Up @@ -1211,6 +1212,26 @@ export function defineIntegrationTestSuiteHttpServerTests(
});
});

it('returns an error on batch requests with no elements', async () => {
const app = await createApp({ schema, allowBatchedHttpRequests: true });
const req = request(app).post('/').send([]);
return req.then((res) => {
expect(res.status).toEqual(400);
expect(res.body).toMatchInlineSnapshot(`
{
"errors": [
{
"extensions": {
"code": "BAD_REQUEST",
},
"message": "No operations found in request.",
},
],
}
`);
});
});

it('can handle batch requests in parallel', async function () {
const parallels = 100;
const delayPerReq = 40;
Expand Down
Original file line number Diff line number Diff line change
@@ -1,10 +1,11 @@
import type { CacheHint } from '@apollo/cache-control-types';
import type {
import {
ApolloServerPlugin,
BaseContext,
GraphQLRequestContext,
GraphQLRequestListener,
GraphQLResponse,
HeaderMap,
} from '@apollo/server';
import { createHash } from '@apollo/utils.createhash';
import {
Expand Down Expand Up @@ -220,7 +221,7 @@ export default function plugin<TContext extends BaseContext>(
body: { kind: 'single', singleResult: { data: value.data } },
http: {
status: undefined,
headers: new Map(),
headers: new HeaderMap(),
},
};
}
Expand Down
2 changes: 0 additions & 2 deletions packages/server/.npmignore
Original file line number Diff line number Diff line change
@@ -1,8 +1,6 @@
*
# Include the package.json files that give us deep imports.
!**/package.json
# TODO(AS4): glasser isn't this covered by the * above?
node_modules/**/*
!src/**/*
src/**/__tests__/**
!dist/**/*
Expand Down
Loading

0 comments on commit a404bf1

Please sign in to comment.