-
Notifications
You must be signed in to change notification settings - Fork 2k
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
base: main
Are you sure you want to change the base?
Conversation
✅ Deploy Preview for apollo-server-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. Latest deployment of this branch, based on commit 8d104f9:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@@ -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 ` + |
There was a problem hiding this comment.
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.
Agree with @glasser 's hesitance to recommend server impl details to a client. After talking about it a bit, the simplest / not-breaking fix for this would be to document the various The test cases here are still good to have, but I'll revert the change to the error message and create a |
It's not obvious that users can bypass
content-type
negotiation within Apollo Server if they want to use acontent-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 customcontent-type
.