Skip to content
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

Improve invalid accept header error message #7493

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
7 changes: 7 additions & 0 deletions .changeset/gentle-shirts-relate.md
@@ -0,0 +1,7 @@
---
'@apollo/server': patch
---

Improve bad 'accept' header error message

It's not obvious that users can bypass content-type negotiation within Apollo Server if they want to use a content-type that isn't exactly one of the two we prescribe. This improves the error message so that users understand how to skip AS's negotiation step if they choose to use a custom content-type.
87 changes: 78 additions & 9 deletions packages/server/src/__tests__/ApolloServer.test.ts
@@ -1,22 +1,22 @@
import { ApolloServer, HeaderMap } from '..';
import type { ApolloServerOptions } from '..';
import type { GatewayInterface } from '@apollo/server-gateway-interface';
import { makeExecutableSchema } from '@graphql-tools/schema';
import { describe, expect, it, jest } from '@jest/globals';
import assert from 'assert';
import {
FormattedExecutionResult,
GraphQLError,
GraphQLSchema,
parse,
TypedQueryDocumentNode,
parse,
} from 'graphql';
import gql from 'graphql-tag';
import type { ApolloServerOptions } from '..';
import { ApolloServer, HeaderMap } from '..';
import type { ApolloServerPlugin, BaseContext } from '../externalTypes';
import type { GraphQLResponseBody } from '../externalTypes/graphql';
import { ApolloServerPluginCacheControlDisabled } from '../plugin/disabled/index.js';
import { ApolloServerPluginUsageReporting } from '../plugin/usageReporting/index.js';
import { makeExecutableSchema } from '@graphql-tools/schema';
import { mockLogger } from './mockLogger.js';
import gql from 'graphql-tag';
import type { GatewayInterface } from '@apollo/server-gateway-interface';
import { jest, describe, it, expect } from '@jest/globals';
import type { GraphQLResponseBody } from '../externalTypes/graphql';
import assert from 'assert';

const typeDefs = gql`
type Query {
Expand Down Expand Up @@ -687,3 +687,72 @@ it('TypedQueryDocumentNode', async () => {
// that.
}
});

describe('content-type negotiation', () => {
it('responds with a BadRequest error with custom `accept` header', async () => {
const server = new ApolloServer({
typeDefs,
resolvers,
});
await server.start();

const { body } = await server.executeHTTPGraphQLRequest({
httpGraphQLRequest: {
body: { query: '{ hello }' },
headers: new HeaderMap([
['accept', 'application/json;v=1'],
['content-type', 'application/json'],
]),
method: 'POST',
search: '',
},
context: async () => ({ foo: 'bla' }),
});
assert(body.kind === 'complete');
const result = JSON.parse(body.string);
expect(result.errors).toMatchInlineSnapshot(`
[
{
"extensions": {
"code": "BAD_REQUEST",
},
"message": "An 'accept' header was provided for this request which does not accept application/json; charset=utf-8 or application/graphql-response+json; charset=utf-8. If you'd like to use a custom content-type and bypass content-type negotiation altogether, set the \`content-type\` response header in a plugin.",
},
]
`);
await server.stop();
});

it('permits a custom `accept` header when the `content-type` response header is set', async () => {
const server = new ApolloServer({
typeDefs,
resolvers,
plugins: [
{
async requestDidStart({ response }) {
response.http?.headers.set('content-type', 'application/json;v=1');
},
},
],
});
await server.start();

const { body } = await server.executeHTTPGraphQLRequest({
httpGraphQLRequest: {
body: { query: '{ hello }' },
headers: new HeaderMap([
['accept', 'application/json;v=1'],
['content-type', 'application/json'],
]),
method: 'POST',
search: '',
},
context: async () => ({ foo: 'bla' }),
});
assert(body.kind === 'complete');
const result = JSON.parse(body.string);
expect(result.errors).toBeUndefined();
expect(result.data?.hello).toBe('world');
await server.stop();
});
});
4 changes: 3 additions & 1 deletion packages/server/src/runHttpQuery.ts
Expand Up @@ -248,7 +248,9 @@ export async function runHttpQuery<TContext extends BaseContext>({
if (contentType === null) {
throw new BadRequestError(
`An 'accept' header was provided for this request which does not accept ` +
`${MEDIA_TYPES.APPLICATION_JSON} or ${MEDIA_TYPES.APPLICATION_GRAPHQL_RESPONSE_JSON}`,
`${MEDIA_TYPES.APPLICATION_JSON} or ${MEDIA_TYPES.APPLICATION_GRAPHQL_RESPONSE_JSON}. ` +
`If you'd like to use a custom content-type and bypass content-type ` +
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm. I like the idea of making this more discoverable. On the other hand, this is an error that is presented to clients over HTTP based on problematic client request format, not problematic server setup. Telling clients to write plugins seems wrong? Perhaps this could be a "dev mode only" thing, though I don't know if we really have dev mode any more.

`negotiation altogether, set the \`content-type\` response header in a plugin.`,
// Use 406 Not Accepted
{ extensions: { http: { status: 406 } } },
);
Expand Down