Skip to content
This repository has been archived by the owner on Mar 20, 2023. It is now read-only.

A way to post-process the response #36

Closed
mnylen opened this issue Nov 20, 2015 · 11 comments
Closed

A way to post-process the response #36

mnylen opened this issue Nov 20, 2015 · 11 comments

Comments

@mnylen
Copy link

mnylen commented Nov 20, 2015

Would be nice if there was some way of post-processing the response. Some use cases:

  • Log errors
  • Sanitize error messages from any sensitive information
  • Return 400 on validation error; 500 on any other error
  • Add custom response headers based on response body
@mnylen
Copy link
Author

mnylen commented Nov 20, 2015

A nasty hack that I'm currently using to achieve this:

app.use('/graphql', function(req, res, next) {
  let end = res.end

  res.end = function(chunk, encoding) {
    res.end = end

    let contentType = res.get('content-type') || 'text/html'
    if (contentType.indexOf('application/json') === 0) { // don't overwrite graphiql responses
      let response = JSON.parse(chunk)

      if (response.errors) {
        let validationErrors = response.errors
          .filter(({message}) => message.indexOf('Syntax Error') !== -1 || message.indexOf('Cannot query field') !== -1)
          .map(({message}) => ({ message }))

        if (validationErrors.length > 0) {
          res.status(400).json({ errors: validationErrors })
        } else {
          res.status(500).json({ errors: [{ message: "Query could not be executed" }] })
        }
      } else {
        res.end(chunk, encoding)
      }
    } else {
      res.end(chunk, encoding)
    }
  }

  return next()
})

@mnylen
Copy link
Author

mnylen commented Nov 20, 2015

Still, although this hack allows me to sanitize the error messages, it doesn't give me an easy way to access the parsed query and variables from request body / query string.

Ideally I'd like to do something like:

app.use('/graphql', graphqlHTTP({
  schema: schema,
  graphiql: true,
  writeResponse: (query, variables, result, req, res) => {
    if (result.errors) {
      logger.error("Error executing query: " + query + "\nVariables:\n" + JSON.stringify(variables) + "\nErrors:\n" + JSON.stringify(result))
      res.send(500).json(sanitizeErrors(result))
    } else {
      res.send(200).json(result)
    }
  }
})

@leebyron
Copy link
Contributor

Those use cases sound like things we should support, but post-processing the response is not how I would prefer them implemented.

The goal of this package is to create a standardized http interface for GraphQL, which post-processing could easily circumvent.

I'll propose some paths forward:

Log errors

graphql-js could already use a comprehensive logging interface, regardless of if it is accessed via http. I'd like to focus a logging interface on graphql-js itself, and just make that easy to interface via express-graphql.

Sanitize error messages from any sensitive information

Similar to the above. Sanitizing sensitive info in errors seems like an issue regardless of the access mechanism.

Return 400 on validation error; 500 on any other error

This just seems like the right thing to do, let's standardize that.

Add custom response headers based on response body

Could you elaborate on this? What custom headers are you planning to send?

@leebyron
Copy link
Contributor

leebyron commented Dec 1, 2015

Return 400 on validation error; 500 on any other error.

Turns out this was already the current behavior, but there were some other underspecified error codes which should now be correct as of 546191d

@mnylen
Copy link
Author

mnylen commented Dec 2, 2015

Yes, I agree now that post processing probably isn't the best way to approach this. If logging errors and sanitizing them from responses could be baked directly into graphql-js, that would fully solve my use case.

Add custom response headers based on response body

Could you elaborate on this? What custom headers are you planning to send?

Actually, this was just something I grabbed from another issue - seemed like something the post processing could also fix.

@vitosamson
Copy link

Could you elaborate on this? What custom headers are you planning to send?

I have a use case where my GraphQL resolver hits another HTTP service, which returns certain headers for data like pagination. I'd need to forward those headers to the GraphQL client that made the initial request.

Is that currently supported without @mnylen's hack?

@colllin
Copy link

colllin commented Feb 2, 2016

@vitosamson How would you know which field the pagination headers applied to?

@vitosamson
Copy link

you're right - I realized that limitation after I posted the comment. I wound up wrapping paginatable types so that they return both the list and the pagination data, and my HTTP resolvers just grab the headers from the response and add them onto the returned data. A query winds up looking like query { thingList { things {}, pagination }}, which is a bit cumbersome but flexible.

@leebyron
Copy link
Contributor

leebyron commented Feb 3, 2016

@vitosamson - you should check out https://facebook.github.io/relay/docs/graphql-connections.html where there are in depth docs on best practices for pagination.

@mnylen - I think you're right that the best solution here is to allow GraphQL.js the ability to operate on errors to do this sort of thing. Tracking that in graphql/graphql-js#284

That said, there are still some errors that occur within this package, so having the ability to sanitize at the last step is totally reasonable.

leebyron added a commit that referenced this issue Feb 3, 2016
This provides the ability for a custom error function, as requested in #36, so that outgoing errors can be sanitized or expanded.

Adds test cases to illustrate.
leebyron added a commit that referenced this issue Feb 3, 2016
This provides the ability for a custom error function, as requested in #36, so that outgoing errors can be sanitized or expanded.

Adds test cases to illustrate.
@leebyron leebyron closed this as completed Feb 3, 2016
@guikubivan
Copy link

guikubivan commented Nov 3, 2017

Anybody can show an example of how to do this:

Return 400 on validation error; 500 on any other error

I wasn't sure if closing the issue meant it was addressed and if it's possible to do this now.

@plee-nm
Copy link

plee-nm commented Sep 25, 2018

@guikubivan I just proved it out locally not sure if it is a good idea. It doesn't look bad to me but I would love feedback from anyone.

  graphqlHTTP(async (request, response) => ({
    formatError: (error: GraphQLError) => {
      response.status(400);
      ...

A more real life example would do something like:

 import { formatError, GraphQLError } from 'graphql';

  graphqlHTTP(async (request, response) => ({
    formatError: (error: GraphQLError) => {
      const { originalError } = error;
      // add custom error logic
      if(isValidationError(originalError) {
          response.status(400);
      }
      return formatError(error);
  }))

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants