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

Commits on May 23, 2019

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

    **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 committed May 23, 2019
    Configuration menu
    Copy the full SHA
    736ba41 View commit details
    Browse the repository at this point in the history