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

Errors Lifecycle: user extensions > engine reporting > formatError #1272

Merged
merged 9 commits into from
Jun 29, 2018

Conversation

evans
Copy link
Contributor

@evans evans commented Jun 29, 2018

This PR changes how error are handled to be passed through user defined extensions in willSendResponse, then engine reporting, and finally formatError. This provides rich logging inside of extensions with the ability to mask out errors sent to the client. See the slack message below for more context.

https://apollographql.slack.com/archives/CB0GM8R9U/p1530168037000253

@ghost ghost added the ⛲️ feature New addition or enhancement to existing solutions label Jun 29, 2018
@@ -302,7 +307,11 @@ export async function runHttpQuery(
context = await context();
} catch (e) {
e.message = `Context creation failed: ${e.message}`;
throwHttpGraphQLError(500, [e], optionsObject);
if (e.extensions && e.extensions.code) {
throwHttpGraphQLError(400, [e], optionsObject);
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the thinking here? Why are errors that set a code always 400?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The thinking is that errors with codes are thrown by the user and are generally not internal server errors. I think adding a check for non INTERNAL_SERVER_ERROR makes sense

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For example an authentication error should have a 400 level response

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmmm, I don't think that really holds though. 4xx errors are client errors, but not all errors thrown by user code are client errors. If a backend is down for example, that would be more appropriately modeled as a 5xx error (maybe similar to 503 service unavailable).

I don't think this is super relevant because GraphQL responses don't map cleanly to status codes anyway, but 400 seems like an odd default because it signals Bad Request, and that puts the blame on the client. A client may want to retry 5xx requests for example, but not do this for a 400.

@@ -47,7 +47,9 @@ export class GraphQLExtension<TContext = any> {
executionArgs: ExecutionArgs;
}): EndHandler | void;

public willSendResponse?(o: { graphqlResponse: GraphQLResponse }): void;
public willSendResponse?(o: {
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmmm, why are we passing in and returning { graphqlResponse: GraphQLResponse } instead of a single object? If this is about being able to set HTTP headers, maybe we can find another way to parametrize GraphQLResponse instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's to leave room for future changes. I think it's definitely something we can revisit after 2.0

graphqlResponse: GraphQLResponse;
}): void | { graphqlResponse: GraphQLResponse } {
if (o.graphqlResponse.errors) {
return {
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmmm, I'm wondering if modifying the GraphQLResponse passed in instead of returning a new one wouldn't be clearer as an API for extensions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's totally an option, though I feel that this API is more self documenting. It's still possible to modify the graphqlResponse in-place too

Copy link

@wKovacs64 wKovacs64 left a comment

Choose a reason for hiding this comment

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

formatError is only being called once now and the full error is being passed in for logging/masking while the client sees what is returned from formatError (yay!), but the Engine proxy seems to be receiving the masked/returned version as well.

Edit (for future readers): probably no slick way to solve the Engine proxy situation, but should be somewhat moot once the built-in Engine reporting agent is capable of full response caching.

@evans evans added this to the Release 2.0 milestone Jun 29, 2018
@evans evans merged commit e0f7052 into version-2 Jun 29, 2018
@evans evans deleted the server-2.0/error-handling branch June 29, 2018 17:36
@evans evans mentioned this pull request Jun 29, 2018
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 23, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
⛲️ feature New addition or enhancement to existing solutions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants