No longer able to catch unexpected errors after removal of UnforgivingExecutionContext

[alexhafner]

• What is the current behavior?
  Since the introduction of Add UnforgivingExecutionContext #1255 by @AlecRosenbaum it was possible to raise unexpected errors to the backend processing, as examplained in Error handling #902 (comment). This is critical so you get a decent traceback in the log and no error message for each record in the result set, ie

{
  "errors": [
    {
      "message": "An unknown error occurred.",
      "locations": [
        {
          "line": 5,
          "column": 5
        }
      ],
      "path": [
        "testObject",
        0,
        "testField"
      ]
    },
    {
      "message": "An unknown error occurred.",
      "locations": [
        {
          "line": 5,
          "column": 5
        }
      ],
      "path": [
        "testObject",
        1,
        "testField"
      ]
    }
]
}

Changes in graphql-core started to let the UnforgivingExecutionContext tests fail, as recorded by @mweinelt in #1346 and noted by @weilu #1255 (comment) with a suggestion for a potential fix.

#1357 by @codebyaryan brought in welcome changes for query validation, but also removed UnforgivingExecutionContext in #1357 (comment)

• If the current behavior is a bug, please provide the steps to reproduce and if possible a minimal demo of the problem via
  a github repo, https://repl.it or similar.

If we bring back the Test case for UnforgivingExecutionContext without specifying UnforgivingExecutionContext per this configuration alexhafner@0aaa3fc, we get those tests failing because unexpected errors are no longer being raised up: https://gist.github.com/alexhafner/5215ef726b356eef6571efb969f6a3e7

• What is the expected behavior?

Be able to stop processing a graphql operation when an unexpected error occurs, return a top-level error message to the GraphQL user, and get a full stack trace in the backend to be able to act on the error message further.

For example:

If we bring back UnforgivingExecutionContext, and create a similar solution to the one in #1255 (comment), the tests succeed again as shown here: https://gist.github.com/alexhafner/a78f493f1b869df0ba112b4acb5da5e9. The approach in alexhafner@f765537 raises the original errors for non-GraphQL Errors, and handles GraphQL Errors unchanged by using super() on handle_field_error().

other solutions are welcome

• What is the motivation / use case for changing the behavior?

catching unexpected issues is system and security critical, and a full stack trace is needed.

• Please tell us about your environment:

  • Version: graphene master with graphql-core==3.1.6, graphql-relay==3.1.0 graphql-server==3.0.0b4
  • Platform: OS X

• Other information (e.g. detailed explanation, stacktraces, related issues, suggestions how to fix, links for us to have context, eg. stackoverflow)
  N/A

[aryaniyaps]

This is my mistake, I apologize. Because the test cases relevant to the execution context failed (errors weren't swallowed), I assumed that we no longer needed it, because of graphql-core changes.

We should definitely bring the execution context back.

[alexhafner]

Thanks for the quick feedback @codebyaryan . I've created #1369 as a starting point on how we could handle that.

[jkimbo]

Just to chime in here: the UnforgivingExecutionContext could contravene the spec: https://spec.graphql.org/June2018/#sec-Errors-and-Non-Nullability

Thats not to say that there aren't reasons why you would want it (in tests for example) but it might break assumptions that a client makes if it was used in a server.

The way we've approached this in Strawberry is to log all errors using the default logger by default and add a hook to allow you to customise error reporting: https://strawberry.rocks/docs/types/schema#handling-execution-errors

We're also investigating adding a MaskErrors extension that would allow you to hide particular extensions from being reported to the client: strawberry-graphql/strawberry#1155

[AlecRosenbaum]

@jkimbo Here's our use case for what it's worth:

Basically, there should never be anything within errors except for maybe a uuid that links it to a log message. The "expected" class of errors (i.e. anything with a unique message on a frontend) should all be expressed within the schema on fields that return unions (XPayload = X | XError). Otherwise a significant portion of the schema is effectively omitted from graphql type support (since errors has no type support AFAIK). For unexpected and unhandled errors, we need a hook to log it to a logger + send traceback info to rollbar/sentry. Then it needs to be completely squashed so nothing sensitive is leaked in a user-facing error message.

With both the original change and the proposed reimplementation, field resolvers can catch exceptions and explicitly reraise a GraphQLError to get the nullability behavior described in the spec.

At least in our schema, most fields are required so a cascading null can make the request pretty useless. Uncaught exceptions in web workers typically return a http 500 status code, which I'd expect a client to be able to handle about as well as a mostly empty response due to cascading nulls. Reraising the original exception allows us to reuse other error handling/reporting code already present for non-graphql requests, and also makes it easy to track/alert on error rates (based on http status codes) across the application.

[jkimbo]

@AlecRosenbaum thats makes a lot of sense. I think there might be a way to solve your use case withougt having to implement a custom execution context though. In Strawberry this would be done by implementing an extension (example code here: strawberry-graphql/strawberry#1221 (comment)) but since Graphene doesn't have an equivalent you would have to override the execute method on the schema. Something like this:

import logging
import graphene
from graphql import GraphQLError
from graphql.error import format_error

logger = logging.getLogger("graphene.execution")

class VisibleError(Exception):
	# Raise this if you want the error message to be returned in the response
	pass

class CustomSchema(Schema):
	def execute(self, *args, **kwargs):
		result = super().execute(*args, **kwargs)
		if result.errors:
			processed_errors = []
			for error in result.errors:
				logger.error(error, exc_info=error.original_error, stack_info=True)
				# Explicitly log to Sentry/Rollbar here if needed

				if error.original_error and not isinstance(error.original_error, (VisibleError, GraphQLError)):
					processed_errors.append(
						GraphQLError(
							message="Unexpected error.",
							nodes=error.nodes,
							source=error.source,
							positions=error.positions,
							path=error.path,
							original_error=None
						)
					)
				else:
					processed_errors.append(error)

			result.errors = processed_errors

		return result

See it in action here: https://replit.com/@jkimbo/ShadowyInexperiencedSeptagon#main.py

Admittedly this doesn't solve the issue around returning a 500 HTTP status code but that could be solved by checking if there are errors and no data in the view and changing the status code there. Or actually raising in the above execute function.