feat(postgraphql) add extendedErrors option #513

zopf · 2017-07-13T19:26:15Z

In response to issue #485. Adds the extendedErrors option to the postgraphql middleware and CLI. Includes documentation update, but no tests since I could not find a good way to fit an error-producing test into the kitchen-sink integration test suite.

Example:

// ---- PostGraphQL setup (for Koa)
[...]
config.options = {
  extendedErrors: ['hint', 'detail', 'errcode']
};
app.use(postgraphql(config.postgresCnx, config.schemaName, config.options));


// ---- SQL function:
CREATE OR REPLACE FUNCTION throw_test_error(throw boolean)
RETURNS boolean LANGUAGE 'plpgsql' STABLE AS $f$
begin
  if throw then
    RAISE EXCEPTION 'test message'
      USING ERRCODE = '12345', HINT = 'test hint', DETAIL = 'test detail';
  else return true;
  end if;
end; $f$;
GRANT EXECUTE ON FUNCTION throw_test_error(boolean) TO PUBLIC;


// ---- test query:
query test {
  throwTestError(throw: true)
}


// ---- test response, with extendedErrors undefined
{
  "errors": [
    {
      "message": "test message",
      "locations": [
        {
          "line": 28,
          "column": 3
        }
      ],
      "path": [
        "throwTestError"
      ]
    }
  ],
  "data": {
    "throwTestError": null
  }
}


// ---- test response, with extendedErrors set to ['hint', 'detail', 'errcode']
{
  "errors": [
    {
      "message": "test message",
      "locations": [
        {
          "line": 28,
          "column": 3
        }
      ],
      "path": [
        "throwTestError"
      ],
      "hint": "test hint",
      "detail": "test detail",
      "errcode": "12345"
    }
  ],
  "data": {
    "throwTestError": null
  }
}

zopf · 2017-07-13T19:32:45Z

Note: this allows the user to expose the hint, detail and code (as errcode) fields on a Postgres error as desired. Additional fields that could be added in future PRs can be seen here: https://github.com/brianc/node-postgres/blob/master/lib/connection.js#L579

benjie

Excellent start, couple minor niggles plus I'd love an integration test for this. To do so:

add your throw_test_error to https://github.com/postgraphql/postgraphql/blob/master/examples/kitchen-sink/schema.sql (put it in schema a unless another one seems more appropriate
add a new gqlSchema with your required options here: https://github.com/postgraphql/postgraphql/blob/e62d7af5c47065a0aba27f0518c02ac795931d89/src/postgraphql/__tests__/postgraphqlIntegrationQueries-test.js#L33-L35
add a new graphql query triggering the error here: https://github.com/postgraphql/postgraphql/tree/e62d7af5c47065a0aba27f0518c02ac795931d89/src/postgraphql/__tests__/fixtures/queries
run the test suite
review the generated snapshots to see if they're what you'd expect for the given options
commit everything including the snapshots

Let me know if you want any further pointers 👍

benjie · 2017-07-14T07:52:24Z

src/graphql/utils/extendedFormatError.ts

+  hint: string,
+  detail: string,
+  code: string,
+}


Please move this into src/postgraphql as it's done as an operation after graphql resolves rather than being part of the graphql resolving itself.

ah, yes, makes sense. done

benjie · 2017-07-14T07:53:42Z

src/graphql/utils/extendedFormatError.ts

+    throw new Error('Received null or undefined error.')
+  }
+  const originalError = error.originalError as GraphQLErrorExtended
+  fields = fields.map(field => field.toLowerCase())


This should be done when the option is passed to postgraphql so it's only executed once in total rather than once for every error

yes that would be more efficient. done

benjie · 2017-07-14T07:57:02Z

src/postgraphql/postgraphql.ts

@@ -21,6 +21,7 @@ type PostGraphQLOptions = {
  jwtPgTypeIdentifier?: string,
  watchPg?: boolean,
  showErrorStack?: boolean,
+  extendedErrors?: Array<string>,


Process it to lower case in this file 👍

(Regarding my comment on

fields = fields.map(field => field.toLowerCase())

)

zopf · 2017-07-14T15:58:47Z

I had indeed tried that same integration test approach before, but encountered errors I did not fully understand, so I backed it out.

For example:

● unique-constraints.graphql

    expect(value).toMatchSnapshot()

    Received value does not match stored snapshot 1.

    - Snapshot
    + Received

    @@ -1,18 +1,16 @@
     Object {
       "data": Object {
    -    "a": Object {
    -      "email": "sara.smith@email.com",
    -      "name": "Sara Smith",
    -    },
    +    "a": null,
         "b": null,
    -    "c": Object {
    -      "personId1": 1,
    -      "personId2": 2,
    -    },
    -    "d": Object {
    -      "personId1": 4,
    -      "personId2": 4,
    -    },
    +    "c": null,
    +    "d": null,
         "e": null,
       },
    +  "errors": Array [
    +    [GraphQLError: current transaction is aborted, commands ignored until end of transaction block],
    +    [GraphQLError: current transaction is aborted, commands ignored until end of transaction block],
    +    [GraphQLError: current transaction is aborted, commands ignored until end of transaction block],
    +    [GraphQLError: current transaction is aborted, commands ignored until end of transaction block],
    +    [GraphQLError: current transaction is aborted, commands ignored until end of transaction block],
    +  ],
     }

I was puzzled because unique-constraints.graphql's function shouldn't be affected at all by my extended-errors.graphql file's execution, wouldn't you think?

But here's the problem: we're executing all the graphql queries in parallel, using the same pg client! Thus, if any of the queries causes an error, it causes the others to fail during that same transaction on that same connection.

I am going to try resolving that by executing the integration test graphql queries in sequence instead.

benjie · 2017-07-14T16:10:37Z

Good idea; withPgClient wraps them in a transaction, but you can use savepoints between them, something like:

    return await withPgClient(async pgClient => {
      // Add data to the client instance we are using.
      await pgClient.query(await kitchenSinkData)
      for (const fileName of queryFileNames) { // <--
        await pgClient.query("savepoint pretest"); // <--
        // Read the query from the file system.
        const query = await new Promise((resolve, reject) => {
          readFile(resolvePath(queriesDir, fileName), 'utf8', (error, data) => {
            if (error) reject(error)
            else resolve(data)
          })
        })
        await pgClient.query("rollback to savepoint pretest"); // <--
        ...

zopf · 2017-07-17T18:16:09Z

It appears that the savepoint doesn't prevent a raised exception from rendering the transaction unusable.

Here's an isolated SQL test case:

begin;
savepoint foo;
select a.throw_test_error(true);
rollback to savepoint foo;
select a.throw_test_error(false);
commit;

yields:

ERROR:  current transaction is aborted, commands ignored until end of transaction block
********** Error **********

ERROR: current transaction is aborted, commands ignored until end of transaction block
SQL state: 25P02

So, it appears the only way to ensure that a query that could raise an exception does not affect other tests is to run it in a separate transaction, which means running it in a separate withPgClient block. The mildly unfortunate implication is that this means we need to set up the kitchen sink data on each client.. but luckily there is not much data, so it only adds a couple seconds to the full test suite's execution time. Will submit a revised commit in a bit for review.

benjie · 2017-07-17T20:55:12Z

@zopf you didn't rollback to savepoint after the second one before the commit?

cat <<HERE | psql postgres
begin;
savepoint foo;
do \$\$ begin raise exception 'Exception thrown from PL/PGSQL'; end; \$\$ language plpgsql;
rollback to savepoint foo;
do \$\$ begin raise exception 'Exception thrown from PL/PGSQL'; end; \$\$ language plpgsql;
rollback to savepoint foo;
select 1;
commit;
HERE

SET
Time: 0.205 ms
BEGIN
Time: 0.059 ms
SAVEPOINT
Time: 0.058 ms
ERROR:  P0001: Exception thrown from PL/PGSQL
CONTEXT:  PL/pgSQL function inline_code_block line 1 at RAISE
LOCATION:  exec_stmt_raise, pl_exec.c:3165
Time: 1.487 ms
ROLLBACK
Time: 0.067 ms
ERROR:  P0001: Exception thrown from PL/PGSQL
CONTEXT:  PL/pgSQL function inline_code_block line 1 at RAISE
LOCATION:  exec_stmt_raise, pl_exec.c:3165
Time: 0.103 ms
ROLLBACK
Time: 0.038 ms
 ?column?
----------
        1
(1 row)

Time: 0.206 ms
COMMIT
Time: 0.048 ms

zopf · 2017-07-17T22:23:14Z

Sorry, should have posted my throw_test_error function! The false argument means that it just returns true. I actually don't need to have it there. Posting full example here for clarity:

DROP FUNCTION IF EXISTS a.throw_test_error(boolean);
CREATE FUNCTION a.throw_test_error(throw boolean)
RETURNS boolean LANGUAGE 'plpgsql' STABLE AS $f$
begin
  if throw then
    RAISE EXCEPTION 'test message'
      USING ERRCODE = '12345', HINT = 'test hint', DETAIL = 'test detail';
  else return true;
  end if;
end; $f$;

begin;
savepoint foo;
select a.throw_test_error(true);
rollback to savepoint foo;
select 1;
commit;

It turns out that I was executing this on a client that maintained the same connection. After the commit, when I ran the script again, it would give me the ERROR: current transaction is aborted, commands ignored until end of transaction block.

If I just run it once on a fresh connection, or if I run it via the command line, it appears to work properly.

After some refactoring, it looks like the savepoint and rollback are working properly with sequential queries with just a single client! I'm adding a test on the HTTP interface as well, since that's where the error reporting and formatting actually happens...

zopf · 2017-07-17T22:56:50Z

Soooo after figuring out how to get all these tests to run in sequence effectively instead of in parallel, I realized that since this is just the schema passing into graphql, the error is never actually getting passed through formatError, since that happens only in the HTTP request handler. Thus, we can't actually test the effect of the extendedErrors option in postgraphqlIntegrationQueries-test.js!

I wrote a couple simple tests in createPostGraphQLHttpRequestHandler-test.js which should be enough to cover the basics here.

benjie · 2017-07-18T06:53:27Z

Oh gosh, I'm sorry I should have realised that we were testing the wrong thing! 😳

benjie · 2017-07-18T07:05:58Z

Just some minor lint issues to fix

zopf · 2017-07-18T14:43:24Z

Ok, lint issues fixed!

benjie

One last tweak and then we're good to merge I think? 👍

(Have you tested with the CLI?)

benjie · 2017-07-18T16:04:13Z

src/postgraphql/cli.ts

@@ -122,6 +123,7 @@ const server = createServer(postgraphql(pgConfig, schemas, {
  pgDefaultRole,
  watchPg,
  showErrorStack,
+  extendedErrors,


You've added the options here, but you've not added them to the list on lines 23-45? Also you may need to coerce to array here by splitting on commas?

Absolutely right. Fixed, tested with CLI, and cleaned up docs

benjie

Sorry for the delay, I missed that you'd updated it!

This looks good to me, the only issue is the toLowerCase because if we later need to access err.camelCaseThing for whatever reason we'll need a breaking API change to do it (a major version bump).

For that reason, I'm going to remove the toLowerCase and merge - an update to the documentation to reflect this would be most welcome.

benjie · 2017-08-04T13:42:48Z

@zopf If you're happy with my changes I'll get this merged and released in 3.4.0 sometime next week 👍

* feat(postgraphql) add extendedErrors option * feat(postgraphql) add extendedErrors option tests * Remove `toLowerCase`, filter empty strings

zopf force-pushed the master branch from 7635f86 to 3a4e39c Compare July 13, 2017 19:28

zopf force-pushed the master branch from 3a4e39c to e29380f Compare July 13, 2017 19:34

benjie requested changes Jul 14, 2017

View reviewed changes

zopf force-pushed the master branch from e29380f to 65104c3 Compare July 17, 2017 22:57

zopf force-pushed the master branch from 65104c3 to 9e12526 Compare July 18, 2017 14:30

benjie requested changes Jul 18, 2017

View reviewed changes

zopf force-pushed the master branch 3 times, most recently from f39b991 to 19d2d58 Compare July 18, 2017 17:38

feat(postgraphql) add extendedErrors option

3bc7497

zopf force-pushed the master branch from 19d2d58 to 1857bf2 Compare July 18, 2017 17:39

feat(postgraphql) add extendedErrors option tests

c5fbbf9

zopf force-pushed the master branch from 1857bf2 to c5fbbf9 Compare July 18, 2017 17:45

benjie mentioned this pull request Aug 4, 2017

Enhance error handling and expression in the GraphQL schema #533

Closed

benjie previously approved these changes Aug 4, 2017

View reviewed changes

Remove toLowerCase, filter empty strings

ee39ce0

benjie dismissed their stale review via ee39ce0 August 4, 2017 13:41

Merge branch 'master' into master

6573d24

benjie approved these changes Aug 4, 2017

View reviewed changes

benjie merged commit a41e5f2 into graphile:master Aug 4, 2017

chadfurman mentioned this pull request Sep 19, 2017

Document the way to return error messages to the UI from the DB graphile/graphile.github.io#18

Open

benjie added a commit that referenced this pull request Sep 16, 2023

More mermaid plan fixes (#513)

81863e4

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(postgraphql) add extendedErrors option #513

feat(postgraphql) add extendedErrors option #513

zopf commented Jul 13, 2017

zopf commented Jul 13, 2017

benjie left a comment

benjie Jul 14, 2017

zopf Jul 14, 2017

benjie Jul 14, 2017

zopf Jul 14, 2017

benjie Jul 14, 2017

zopf Jul 14, 2017

zopf commented Jul 14, 2017

benjie commented Jul 14, 2017

zopf commented Jul 17, 2017

benjie commented Jul 17, 2017

zopf commented Jul 17, 2017

zopf commented Jul 17, 2017

benjie commented Jul 18, 2017

benjie commented Jul 18, 2017

zopf commented Jul 18, 2017

benjie left a comment

benjie Jul 18, 2017

zopf Jul 18, 2017

benjie left a comment

benjie commented Aug 4, 2017

feat(postgraphql) add extendedErrors option #513

feat(postgraphql) add extendedErrors option #513

Conversation

zopf commented Jul 13, 2017

zopf commented Jul 13, 2017

benjie left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zopf commented Jul 14, 2017

benjie commented Jul 14, 2017

zopf commented Jul 17, 2017

benjie commented Jul 17, 2017

zopf commented Jul 17, 2017

zopf commented Jul 17, 2017

benjie commented Jul 18, 2017

benjie commented Jul 18, 2017

zopf commented Jul 18, 2017

benjie left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benjie left a comment

Choose a reason for hiding this comment

benjie commented Aug 4, 2017