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

Allow plugins to modify the HTTP status code under ERROR conditions. #2714

Merged

Conversation

abernix
Copy link
Member

@abernix abernix commented May 23, 2019

Note: This currently only covers error conditions. Read below for details.

This commit aims to provide user-configurable opt-in to mapping GraphQL-specific behavior to HTTP status codes under error conditions, which are not always a 1:1 mapping and often implementation specific.

HTTP status codes — traditionally used for a single resource and meant to represent the success or failure of an entire request — are less natural to GraphQL which supports partial successes and failures within the same response.

For example, some developers might be leveraging client implementations which rely on HTTP status codes rather than checking the errors property in a GraphQL response for an AuthenticationError. These client implementations might necessitate a 401 status code. Or as another example, perhaps there's some monitoring infrastructure in place that doesn't understand the idea of partial successes and failures. (Side-note: Apollo Engine aims to consider these partial failures, and we're continuing to improve our error observability. Feedback very much appreciated.)

I've provided some more thoughts on this matter in my comment on: #2269 (comment)

This implementation falls short of providing the more complete implementation which I aim to provide via a didEnounterErrors life-cycle hook on the new request pipeline, but it's a baby-step forward. It was peculiar, for example, that we couldn't already mostly accomplish this through the willSendResponse life-cycle hook.

Leveraging the willSendResponse life-cycle hook has its limitations though. Specifically, it requires that the implementer leverage the already-formatted errors (i.e. those that are destined for the client in the response), rather than the UN-formatted errors which are more ergonomic to server-code (read: internal friendly).

Details on the didEncounterErrors proposal are roughly discussed in this comment: #1709 (comment)

(tl;dr The didEncounterErrors hook would receive an errors property which is pre-formatError.)

As I opened this commit message with: It's critical to note that this DOES NOT currently provide the ability to override the HTTP status code for "success" conditions, which will continue to return "200 OK" for the time-being. This requires more digging around in various places to get correct, and I'd like to give it a bit more consideration. Errors seem to be the pressing matter right now.

Hopefully the didEncounterErrors hook will come together this week.

**Note:** This currently only covers error conditions.  Read below for details.

This commit aims to provide user-configurable opt-in to mapping
GraphQL-specific behavior to HTTP status codes under error conditions, which
are not always a 1:1 mapping and often implementation specific.

HTTP status codes — traditionally used for a single resource and meant to
represent the success or failure of an entire request — are less natural to
GraphQL which supports partial successes and failures within the same
response.

For example, some developers might be leveraging client implementations
which rely on HTTP status codes rather than checking the `errors` property
in a GraphQL response for an `AuthenticationError`.  These client
implementations might necessitate a 401 status code.  Or as another example,
perhaps there's some monitoring infrastructure in place that doesn't
understand the idea of partial successes and failures. (Side-note: Apollo
Engine aims to consider these partial failures, and we're continuing to
improve our error observability.  Feedback very much appreciated.)

I've provided some more thoughts on this matter in my comment on:
#2269 (comment)

This implementation falls short of providing the more complete
implementation which I aim to provide via a `didEnounterErrors` life-cycle
hook on the new request pipeline, but it's a baby-step forward.  It was
peculiar, for example, that we couldn't already mostly accomplish this
through the `willSendResponse` life-cycle hook.

Leveraging the `willSendResponse` life-cycle hook has its limitations
though.  Specifically, it requires that the implementer leverage the
already-formatted errors (i.e. those that are destined for the client in the
response), rather than the UN-formatted errors which are more ergonomic to
server-code (read: internal friendly).

Details on the `didEncounterErrors` proposal are roughly discussed in this
comment:
#1709 (comment)

(tl;dr The `didEncounterErrors` hook would receive an `errors` property
which is pre-`formatError`.)

As I opened this commit message with: It's critical to note that this DOES
NOT currently provide the ability to override the HTTP status code for
"success" conditions, which will continue to return "200 OK" for the
time-being.  This requires more digging around in various places to get
correct, and I'd like to give it a bit more consideration.  Errors seem to
be the pressing matter right now.

Hopefully the `didEncounterErrors` hook will come together this week.
@abernix abernix added this to the Release 2.6.0 milestone May 23, 2019
@abernix abernix merged commit 163bba7 into release-2.6.0 May 23, 2019
@abernix abernix deleted the abernix/allow-mutable-HTTP-statusCode-for-errors branch May 23, 2019 19:20
abernix added a commit that referenced this pull request May 24, 2019
(As I mentioned at the bottom of
#2714)

This adds a new life-cycle event to the new request pipeline API called
`didEncounterErrors`, which receives `requestContext`'s **unformatted**
`errors`.  (The `requestContext` represents an individual request within
Apollo Server.)  These `errors` give access to potentially different
`errors` than those received within `response.errors`, which are sent to the
client and available on the `willSendResponse` life-cycle hook, since they
are **not** passed through the `formatError` transformation.  This can allow
plugin implementers to take advantage of the actual errors and properties
which may be pruned from the error before transmission to the client.

While most other request pipeline life-cycle events provide a guarantee of
their arguments, this `didEncounterErrors` will have a little bit less
certainty since the errors could have happened in parsing, validation, or
execution, and thus different contracts would have been made (e.g. we may
not have been able to negotiate a `requestContext.document` if we could not
parse the operation).

This still needs tests and I suspect I'll have some additional changes
prior to this becoming official, but it can ship as-is for now and will live
in the Apollo Server 2.6.0 alpha for the time-being.  Use with minimal risk,
but with the caveat that the final API may change.

Also, I'll need to add docs to
#2008.
abernix added a commit that referenced this pull request May 24, 2019
(As I mentioned at the bottom of
#2714)

This adds a new life-cycle event to the new request pipeline API called
`didEncounterErrors`, which receives `requestContext`'s **unformatted**
`errors`.  (The `requestContext` represents an individual request within
Apollo Server.)  These `errors` give access to potentially different
`errors` than those received within `response.errors`, which are sent to the
client and available on the `willSendResponse` life-cycle hook, since they
are **not** passed through the `formatError` transformation.  This can allow
plugin implementers to take advantage of the actual errors and properties
which may be pruned from the error before transmission to the client.

While most other request pipeline life-cycle events provide a guarantee of
their arguments, this `didEncounterErrors` will have a little bit less
certainty since the errors could have happened in parsing, validation, or
execution, and thus different contracts would have been made (e.g. we may
not have been able to negotiate a `requestContext.document` if we could not
parse the operation).

This still needs tests and I suspect I'll have some additional changes
prior to this becoming official, but it can ship as-is for now and will live
in the Apollo Server 2.6.0 alpha for the time-being.  Use with minimal risk,
but with the caveat that the final API may change.

Also, I'll need to add docs to
#2008.
abernix added a commit that referenced this pull request May 27, 2019
abernix added a commit that referenced this pull request May 27, 2019
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 22, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant