move graphql error formatting to a function that can be overridden

[jaemk]

No description provided.

[erewok]

A general question first and then a comment specific to this PR. Do you have a motivating example for this case?

In this code, if we wanted to swap out the error formatter, it seems like a more common suggestion would be to add a kwarg with a default value to the constructor on the class:

class GraphQLApp:
    def __init__(
        self,
        schema: "graphene.Schema",
        executor: typing.Any = None,
        executor_class: type = None,
        graphiql: bool = True,
        error_formatter= format_graphql_error
    ) -> None:
   ...
   self.error_formatter = error_formatter

And then to use that attribute to format errors if any are present. This would be more typically Pythonic and more obvious to readers of the code than defining a method that later gets clobbered. Indeed, it seems like this a pattern that graphene uses?

I also wonder if this doesn't introduce a potential bug. If format_graphql_error ended up being None, a case that looks like it would be virtually impossible to reach in master, then we'd be calling None(error). In allowing the error formatter to be switched, this problem could emerge more easily and would need to be handled (probably in the constructor)?

[jaemk]

motivating example for this case

Error handling between our backend/frontend is done primarily through "error keys" indicating known error cases which the frontend can handle gracefully. These are propagated up through the backend using a custom BaseError class that supports an error_key attribute, error location stacktraces, and error chains. The default error formatter simply strs the error which is good enough for human eyes, but not for programmatic consumption. I'm currently overriding the error formatting so we can do custom error serialization and stacktrace suppression.

The current overriding looks like this (requires patching)

# custom error base class
class BaseError(Exception):
    ....

def format_error(error: Exception) -> dict:
    """
    Format graphql errors into error data that the frontend can consume.
    The most important thing here is the "error_key" field that is set on all BaseErrors.
    Stacktraces are suppressed since the frontend doesn't need to see that much detail.
    """
    # makes sure stuff is or is wrapped in a BaseError
    ... snip ...
    error_data = base_error.to_dict(with_stacktrace=False)
    return error_data


def create_graphql_app(schema: Schema, app_name: str) -> GraphQL:
    # Patch graphene's error formatting function that the starlette GraphQL base class uses.
    from starlette import graphql as starlette_graphql

    setattr(starlette_graphql, "format_graphql_error", format_error)
    .... snip ....
    # returning a subclass of GraphQLApp
    return GraphQL(
        schema=schema, executor_class=AsyncioExecutor, graphiql=True, app_name=app_name
    )

---

it seems like a more common suggestion would be to add a kwarg

I don't have a strong opinion on how the overriding happens, just that it's possible without having to directly patch a graphene import. I can change it to be an optional kwarg if that's the desired way. My reason for breaking it out to a method was that I'm already overriding the majority of the GraphQLApp methods in a subclass so I can support OPTIONS requests, change the context type to be a typed pydantic class, add error logging of individual resolution errors, and add prometheus metrics around the actual execution. I can open additional PRs to support some of this stuff also if there's interest.

class GraphQL(GraphQLApp):
    """
    GraphQLApp is provided by starlette.

    This does almost everything we want, except the context
    isn't configurable and options aren't supported.
    Patching `handle_graphql` and `execute` lets us allow OPTIONS
    requests to pass through and also change the `context`
    parameter passed to the underlying executor.
    """

    def __init__(self, *args: Any, app_name: str, **kwargs: Any):
        self.app_name = app_name
        self.metrics = GraphQLMetrics.get(app_name)
        super().__init__(*args, **kwargs)

    async def handle_graphql(self, request: Request) -> Response:
        # OPTIONS is ok, just pass through so cors headers can be added
        if request.method == "OPTIONS":
            return Response()
        return await super().handle_graphql(request)

    async def execute(  # type: ignore
        self, query, variables=None, context=None, operation_name=None
    ):
        try:
            # context is a dict with request and background keys, we want
            # it to be a typed thing with attributes instead
            request = context["request"]
            ctx = GraphQLContext(
                request=request,
                background=context["background"],
                app_context=request.state.context,
            )
            log = log_bind(logger, ctx=ctx.app_context)
        except Exception:
            logger.exception("Error constructing graphql context")
            raise

        try:
            op = ctx.app_context.graphql_op or "unknown"
            self.metrics.request_counter.labels(op=op).inc()

            # now call with our new special context
            timer = TimeSampler(monotonic=True)
            res = await super().execute(
                query, variables=variables, context=ctx, operation_name=operation_name
            )
            ms = timer.sample_from_last(
                "graphql_time",
                histogram=self.metrics.request_time_ms_histogram.labels(op=op),
            )
            log.debug(f"graphql execution took: {ms}ms", samples=timer.samples)

            if res.errors:
                status_code = "-1"  # unknown status code
                # for any resolution errors, log out their full stack trace
                for err in res.errors:
                    stack = getattr(err, "stack", None)
                    if stack is None:
                        tb_s = f"no traceback for error: {err}"
                    else:
                        tb = traceback.format_exception(type(err), err, stack)
                        tb_s = "\n".join(tb)

                    error_data = format_error(err)
                    status_code = str(
                        error_data.get("response_status_code") or status_code
                    )
                    log.error(f"graphql resolver error: {tb_s}", **error_data)
                self.metrics.error_counter.labels(op=op, status_code=status_code).inc()
            return res
        except Exception:
            log.exception("graphql execution error")
            raise

[jaemk]

Added an optional kwarg to the constructor to override the default

[JayH5]

Thank you for creating this pull request. We have decided to deprecate GraphQL support within Starlette itself so I am going to close this pull request. See #619.