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

Expose request Object to formatError #614

Closed
gajus opened this issue Oct 11, 2017 · 14 comments · Fixed by #1547
Closed

Expose request Object to formatError #614

gajus opened this issue Oct 11, 2017 · 14 comments · Fixed by #1547

Comments

@gajus
Copy link

gajus commented Oct 11, 2017

I would like to log the request body when an error occurs. This requires access to the request object.

In general, request object often contains additional details that might be desired to be logged, e.g. user session ID.

@gajus
Copy link
Author

gajus commented Nov 2, 2017

You can have access to the request if your pass options as a function.

You cannot access it in the formatError function. You can use a factory pattern to create a new formatError on every request, though.

@domarmstrong
Copy link
Contributor

#631

@majelbstoat
Copy link
Contributor

majelbstoat commented May 19, 2018

Rebinding the format error function on every request seems very wasteful. Would be great if it took request, or context, or something that exposed this, a second parameter.

(My usecase is stackdriver error reporting, which can make use of requests and formatError is a convenient place to log all graphql errors)

@codyzu
Copy link

codyzu commented Aug 3, 2018

I have the same requirement as @majelbstoat . When logging and reporting errors, I need to get some info from the request in order to create a relation between the errors and the requester. I am using apollo-server-koa and I am unable to find a way to do this. The apollo-server-koa does not accept a function for the options... so even the workaround does not seem possible.

Has anyone made any progress for this or found work around?

@robrichard
Copy link
Contributor

I also have this requirement. The best workaround I found was to extend ApolloServer from apollo-server-express and overwrite the createGraphQLServerOptions method. Unfortunately, you can't use formatError here either since it doesn't get called (see #1439), but you can access the errors in formatResponse.

class MyApolloServer extends ApolloServer {
    async createGraphQLServerOptions(req) {
        return {
            schema,
            rootValue,
            context,
//          formatError: e => handleError(e, req) // doesn't get called
            formatResponse: graphqlResponse => {
                if (graphqlResponse.errors) {
                    errors = graphqlResponse.errors.map(e => handleError(e, req));
                }
                return { ...graphqlResponse, errors };
            },
        };
    }
}

@vinayuttam
Copy link

Is there any update on this? Like others, I also have a requirement to log some information to correlate between events that happen in the request.

@declanelcocks
Copy link

A bit of a blocker for me too, since our current server initialisation utilises graphqlExpress to return the formatError option based on the request object. We use it to handle things such as language so upgrading to v2 is looking quite painful.

@ValterSantosMatos
Copy link

ValterSantosMatos commented Sep 5, 2018

Also blocker for me. As a very dirty workaround I ended up wrapping all resolvers into a try catch so I could access the context in case of an error, just before passing them to the Apollo server...

static addStorageToContext(resolvers) {
    const keys = Object.keys(resolvers);

    // Recursively translates the errors if they are custom errors
    keys.map((key) => {
      if (typeof resolvers[key] === 'function') {
        const originalResolver = resolvers[key];
        resolvers[key] = (_parent, _input, context: IContext, ...args) => {
          return Promise.resolve(
            originalResolver(_parent, _input, context, ...args),
          ).catch((error) => {
            // Log error...

            throw error;
          });
        };
      } else {
        ConfigApollo.addStorageToContext(resolvers[key]);
      }
    });

    return resolvers;
  }

evans pushed a commit that referenced this issue Sep 8, 2018
* Pass the context along to all the extension methods

Addresses #1343 

With this change you should now be able to implement an extension like so:

```javascript
class MyErrorTrackingExtension extends GraphQLExtension {
    willSendResponse(o) {
        const { context, graphqlResponse } = o

        context.trackErrors(graphqlResponse.errors)

        return o
    }
}
```

Edit by @evans :
fixes #1343
fixes #614 as the request object can be taken from context or from requestDidStart
fixes #631 ""

* Remove context from extra extension functions

The context shouldn't be necessary for format, parse, validation, and
execute. Format generally outputs the state of the extension. Parse and
validate don't depend on the context. Execution includes the context
argument as contextValue

* Move change entry under vNext
abernix pushed a commit that referenced this issue Oct 10, 2018
* Pass the context along to all the extension methods

Addresses #1343

With this change you should now be able to implement an extension like so:

```javascript
class MyErrorTrackingExtension extends GraphQLExtension {
    willSendResponse(o) {
        const { context, graphqlResponse } = o

        context.trackErrors(graphqlResponse.errors)

        return o
    }
}
```

Edit by @evans :
fixes #1343
fixes #614 as the request object can be taken from context or from requestDidStart
fixes #631 ""

* Remove context from extra extension functions

The context shouldn't be necessary for format, parse, validation, and
execute. Format generally outputs the state of the extension. Parse and
validate don't depend on the context. Execution includes the context
argument as contextValue

* Move change entry under vNext
@fakership
Copy link

@robrichard I also meet up with the problem, how to resolve it?

@philsch
Copy link

philsch commented Feb 11, 2019

The linked code change above kind of solves the problem, although the context is still not available in the formatError callback. As this was also a repetitive task in my projects, I've created a npm package for it https://github.com/philsch/graphql-error-tracking-extension. I hope it is useful for you, if not you can still use the package code to understand how to use the context extension. You can also read about a real life example at my blog post.

@techwes
Copy link

techwes commented Jul 29, 2019

This is blocking my upgrade to 2.0 as well. In 1.0 I used code like this:

graphqlExpress(request => ({
  schema,
  context: request => ({ request }),
  formatError: request => error => {
    // Log Error to Sentry with extra request context
    // If appropriate, change error message to obscure real error
    // from non-admin users.
  }
}))

But now upgrading to 2.0 I see no way to get the request information to add context to my logs. It seems like philsch's code might be a workaround and I also found this example.

Is implementing a custom graphql-extension going to remain the only supported way to accomplish this? Or is there a chance we can get the graphql context passed as a parameter to the formatError function?

@toverux
Copy link

toverux commented Jan 14, 2020

I'd very much like to get the request context in formatError. I don't want to use GraphQLExtension#willSendResponse as we can only access GraphQLFormattedError objects here.

I have localizable error objects, and I'd like to localize the error message in formatError based on the Accept-Language header. This is cleaner if I can do that from GraphQLError objects (the ones formatError receives), and avoid manipualting raw objects in willSendResponse, that will be clunky and error-prone.

Should I definitely forget about getting the context in formatError and go for the extension, or is there a chance that this gets fixed?

@twelve17
Copy link

twelve17 commented Jun 3, 2020

@evans , may I ask how #1547 has fixed this issue? I do not see an ability for formatError to access the context or request. Did I miss something, perhaps?

@eladchen
Copy link

eladchen commented Jul 10, 2020

For anyone stumbling upon this, here's a gist of how to apply @robrichard proposal
(I know this issue closed, but a google search pointed me here).

https://gist.github.com/eladchen/00251cfd96a3fc4fe0c986d83fd48bc2

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 20, 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 a pull request may close this issue.