Enhance error handling and expression in the GraphQL schema

[lifeiscontent]

Hey there,

I was wondering how I could use postgraphql with relay that has a bit of app state as well?

it looks like what facebook does with modern is build a graphql endpoint that just doesn't do anything to the database, but then the cache picks it up, so how would this work within postgraphql?

[benjie]

@lifeiscontent You can extend the GraphQL schema to do that using the v4 alpha; install with npm install postgraphql@next and build a plugin for it. There's limited documentation currently; I'm working on writing more docs over the next few days.

https://github.com/postgraphql/graphql-build/tree/master/packages/graphql-build

Let us know how you get on!

[lifeiscontent]

@benjie if I worked on an apollo client for the example postgraph app would you help me fill in the blanks where needed?

[benjie]

Sure 👍

[lifeiscontent]

@benjie I've started the project, feel free to track my work here: https://github.com/lifeiscontent/postgraphql-forum-example-apollo-client

also, if you think I should move this into the postgraphql examples folder, I can do that as well, but I figured this might be a good example to have as a separate project. Let me know your thoughts 👍

[benjie]

I'm currently planning to move the examples and documentation out of this repository; so I definitely support it being it's own repo 👍

[lifeiscontent]

@benjie is there a reason why the authenticate method in the forum example returns this?

mutation authenicate($input: AuthenticateInput!) {
  authenticate(input: $input) {
    jwtToken {
      role
      personId
    }
  }
}

I would've expected a token string and not a role and personId, should I update the sql in the forum example?

[benjie]

The reason for this is that PostGraphQL receives this composite type and then signs it to transform it into a JWT token itself - we do not post the JWT secret into the database for the database to perform the signing. See jwtPgTypeIdentifier here:

https://github.com/postgraphql/postgraphql/blob/607f6629986735139e2b77d5a9b1143846c4f691/docs/library.md

[lifeiscontent]

@benjie is there a cli option for that?

[lifeiscontent]

ah, found it. --token that's confusing

[lifeiscontent]

@benjie I implemented login, and authorizations today. Will continue to work on it tomorrow.

[benjie]

Awesome 👏

[lifeiscontent]

@benjie how would you create a post as the currentPerson? I'm wondering if I should hit the graphql endpoint and store the result of the currentPerson query in the apollo cache, and then just ask for currentPerson each time, but that seems inefficient, any suggestions?

[benjie]

That's what I tend to do. An alternative is to do it through a BEFORE INSERT trigger where it sets the user from the current_setting('jwt.claims.user_id') or similar; this requires setting the column to be nullable though (although you can add a constraint that requires it not to be null to counteract this... but it's messy). This is definitely something we'd like to improve.

[lifeiscontent]

@benjie alright, post creation is working now. This is where I need your help: https://github.com/lifeiscontent/postgraphql-forum-example-apollo-client/blob/master/src/containers/CreatePostFormContainer.js

on line 14, you'll notice the errors: [] state, I'm wondering how to take the forum example to produce nice errors for the user to know from both the client and the server they did something wrong. E.g. didn't fill in a required field. Also, for the Topic that's a enum within the server, is there a way to say hey postgraph, what are my options? Or how would you achieve this?

[benjie]

Regarding the errors this is kind of an open issue. There's been some work on improving this with #513 but I think it's still quite manual - having to extract what went wrong from the error itself. I'm not actually sure how best to handle this.

Regarding the topic ENUM, you could use GraphQL introspection to do this:

query IntrospectEnums {
  __schema {
    types {
      kind
      name
      description
      enumValues {
        name
        description
        isDeprecated
      }
    }
  }
}

I'm afraid you'll have to filter it client side, there doesn't seem to be support for only getting the enum types.

const myEnumDetails = data.___schema.types.find(
  ({kind, name}) => kind === 'ENUM' && name === 'MyEnumName'
);

[lifeiscontent]

@benjie ah, okay great. I guess I'll wait till the merge. Looking forward to it 👍

[lifeiscontent]

@benjie when will the extended errors be published? should I use @next or latest?

[benjie]

I'll probably publish to @latest on Tuesday as 3.4.0; if I can merge it to @next at the same time I'll also release that.

[lifeiscontent]

@benjie okay, sounds good. I got the introspection enum on the post working 👍 If you have a moment, I'd love for you to take a look at the code and see if I'm doing anything dumb.

https://github.com/lifeiscontent/postgraphql-forum-example-apollo-client/pull/1/files

just so you can have an idea of where I want to take this, I'd like to provide an example to the community that would be considered a production ready application using postgraphql.

[benjie]

LGTM; few comments:

• Add the SQL schema to the project (and customise it if you like); for example you perform email validation in redux but there's little point doing that unless it's backed up by e.g. a domain in the schema that enforces the exact same structure
• For things like titlecase it may be better to use a library (such as change-case) which will be faster and more memory efficient
• Good use of compose 👍
• Getting the jwtToken from localStorage is expensive (localStorage can be slow); instead you should fetch it from redux (have the redux reducers initial state read it from localStorage)
• Should typeof window.__REDUX_DEVTOOLS_EXTENSION__ !== 'undefined' be === 'function' instead? Or is it a "callable object"?
• isLocalhost would be good to support *.local and *.dev (and, if you fancy, *.meh domains from my mehserve)

[lifeiscontent]

• add SQL Schema
• use change-case
• get jwtToken from redux
• change __REDUX_DEVTOOLS_EXTENSION__ check to check for function instead of undefined.
• isLocalhost

if you can answer these for me, I don't mind documenting the readme with your answers if you want.

so for the unfinished todos, can you explain a bit more?

for the SQL Schema, how do I go about adding that to the project/change it?

for the jwtToken, the current process is, check localStorage, if it exists, setup the middleware for apollo, setup the apollo client, and setup the redux store, once the store is setup dispatch the SIGN_IN action and tell the store the user is signed in.

if I store the token in redux first, how do you suggest I tell apollo to use the middleware/call the redux store if it's not setup?

I'm not sure what isLocalhost is, this is my first time hearing about it, do you have any reading material on where I can figure out how to set that up?

[benjie]

for the SQL Schema, how do I go about adding that to the project/change it?

It's up to you if you feel migrations are inside or outside the scope of this project. Personally I'd err on the side of "outside" and just put the schema in db/schema.sql verbatim. If you update your local DB then just re-dump it into this file to update it.

if I store the token in redux first, how do you suggest I tell apollo to use the middleware/call the redux store if it's not setup?

I'm not sure exactly; I would just make sure Apollo's network layer has access to the redux store so it can pull data out of it and then the network layer would just say "if JWT then authenticate otherwise don't". You could then set up the store and once that's set up issue the SIGN_IN based on the contents of localStorage if you like? It'd take a while for me to follow through exactly how your redux system is set up and this is meant to be a demo of PostGraphQL rather than redux, so the simpler the better. (Personally I'd avoid redux and just use React/setState to keep it simple.)

I'm not sure what isLocalhost is

It's from the service worker create-react-app created for you, it's just a boolean that determines if window.location.host looks like localhost or not (i.e. is it development or not).

[lifeiscontent]

@benjie okay, so currently I'm using the proxy option within create-react-app to proxy to the postgraphql server, is there a better way to connect to postgraph? because using this option, it's making it difficult to connect to .meh,.local,.dev domains.

You can see my current changes here: https://github.com/lifeiscontent/postgraphql-forum-example-apollo-client/pull/1/commits/96d4965c6289535bf61986620199dcfa3a320d89

[benjie]

It's much better to use CORS than to proxy - the JWT is your source of security so using CORS on a separate server helps solve security issues such as CSRF since the CSRF attacks would not contain the JWT token (since it's not in cookies) and thus cannot do anything harmful.

[benjie]

Though having glanced at your code now you're not doing anything funky, so your proxying should be fine security-wise.

I found with create-react-app that the proxy option didn't work very well with PostGraphQL, so instead I flipped it on its head and ran an express server which handled the routes I wanted (/graphql, /graphiql, /auth/*, etc) and then proxied the remainder into create-react-app (rather than the other way around - CRA has some annoying rules for it's proxying (such as looking for text/html requests) which meant I couldn't get it working proxying graphiql).

[lifeiscontent]

@benjie so do you think I should enable cors? It says in the CLI docs that we should use a proxy if possible. https://github.com/postgraphql/postgraphql/blob/master/docs/cli.md

The only problem with the proxy in CRA is it doesn't work for *.dev, etc domains only localhost.

[lifeiscontent]

@benjie also in terms of migrations, I'm still trying to understand how to build a good workflow between postgraphql and graphql schemas. Is there a way to go from graphql schema to -> postgraphql? or would you just write the raw sql schema and have postgraphql create the graphql representation?

I think there are parts of graphql that I'm not completely understanding to bridge the gap in my knowledge.

Would really appreciate it if you could shed some light here as again, it'll benefit the community for learning purposes and easier onboarding for potential postgraph users.

[benjie]

so do you think I should enable cors? It says in the CLI docs that we should use a proxy if possible. /docs/cli.md@master

Personally I would build an express server for PostGraphQL and then proxy through to the create-react-app code; this then means that on production you just switch out the "proxy through to create-react-app" with "serve the static folder containing built assets".

Is there a way to go from graphql schema to -> postgraphql? or would you just write the raw sql schema and have postgraphql create the graphql representation?

The core feature of PostGraphQL is that it builds a GraphQL API based on your PostgreSQL schema, so I would just worry about the PostgreSQL schema and let PostGraphQL handle the rest.

Have you had a look through the docs I'm building for Graphile (the core of PostGraphQL v4)? https://www.graphile.org/ To extend the schema you can pass additional graphile-build plugins to PostGraphQL via the PostGraphQL library options (there's no documentation on this yet and you have to use it as a library rather than CLI, but here's the option names to use: https://github.com/graphile/graphile-build/blob/2c3156b4d0b359b735a180e6558b0e0142cf3d38/packages/postgraphile-core/src/index.js#L30-L32 ).

[lifeiscontent]

@benjie I guess I'm still a bit confused on what exactly the purpose of graphile is, but I'm interested in knowing more.

Also, I just implemented a update_password function for the forum_example project.

can you give feedback on the SQL? I'm not sure if this is something you'd expect and apparently GraphQL wants to return the field as boolean which should probably be called status, but I'm not as savvy with SQL as I probably should be.

create function forum_example.update_password(
  email text,
  previous_password text,
  password text,
  password_confirmation text
) returns boolean as $$
declare
  account forum_example_private.person_account;
begin
  select a.* into account
  from forum_example_private.person_account as a
  where a.email = $1;

  if account.password_hash = crypt(previous_password, account.password_hash) and password = password_confirmation then
    UPDATE forum_example_private.person_account set password_hash = crypt(password, gen_salt('bf')) where forum_example_private.person_account.email = $1;
    return true;
  else
    return false;
  end if;
end;
$$ language plpgsql strict security definer;

comment on function forum_example.update_password(text, text, text, text) is 'Updates the password of a person.';

[benjie]

I'm still a bit confused on what exactly the purpose of graphile is, but I'm interested in knowing more.

It's basically a way of extending a GraphQL API using plugins; the entire of the GraphQL schema in PostGraphQL v4 is generated through a number of plugins: some do introspection, some add connection types, some add table types, some add the relevant columns to those types, some add the condition filter, etc etc.

can you give feedback on the SQL?

I'm not sure if this is something you'd expect and apparently GraphQL wants to return the field as boolean which should probably be called status, but I'm not as savvy with SQL as I probably should be.

The update_password function is marked as returns boolean so GraphQL will make the result a boolean; a common thing to do would be to return user and then return the updated user - that way the client could re-query e.g. user.passwordLastUpdatedAt. But boolean is fine too. You don't currently have any control over the name of the boolean property that's returned from GraphQL (though you could over-ride it with a plugin!) because PostGraphQL doesn't know how to get its name so it just uses a default one.

I don't think the mutation needs password_confirmation - to me that's a client-side concern.

if account.password_hash doesn't explicitly cover there having been no account, which should probably raise exception? (Technically it's handled because of how nulls work in Postgres, but it's not the easiest to read.)

Other than that, looks good 👍

[benjie]

We don't currently support anonymous types:

https://github.com/postgraphql/postgraphql/blob/master/TROUBLESHOOTING.md#stored-procedure--function-not-appearing

Create a named type instead and then return that.

[lifeiscontent]

@benjie ah okay, thanks.

so I'm almost finished wrapping up the "Edit Account" feature, but I just ran into another issue that I'm not sure how to handle. In Postgraphql the updatePerson method expects a nodeId and not the Id of the actual person, how would I get this information? Or is this a bug?

[benjie]

Use updatePersonById instead, if you prefer. nodeId is available on all records (it's called id if you use --classic-ids, which is required to comply with the relay global objects spec) so you can pull that down and send it up again. Personally I never use the nodeId stuff since I moved to Apollo rather than Relay.

[lifeiscontent]

@benjie so I just looked at the extendedErrors option, and it still seems a bit simplistic for a production app, unless I'm not understanding how I can use these errors to then map to my forms to let users know what they didn't fill in correctly.

Would you recommend using graphile instead of postgraphql? or would you want to help me power through these problems so postgraphql is a more robust so it can handle real world use cases?

I'm currently confused in terms of which direction I should go, eventually, after I'm done building this project I plan to use what I learn from you and potentially the postgraphql community to have postgraph power my company's own app. I just need to understand what the current limitations are and how to work around them in a reasonable manner.

[benjie]

PostGraphQL v4 is mostly just a wrapper around graphile; so you'll still be using PostGraphQL. I definitely want to make PostGraphQL the best it can be; what do you need from the errors to be more useful for your project? An example of what it gives currently vs what you would like would be really useful - then we can try and figure out how to get that data from PostgreSQL into your app via PostGraphQL. This is just the first punt at --extended-errors, we wrote it in a way that should mean we can extend it with more information without requiring a breaking change.

[benjie]

If it turns out we can't get the error information you need from PostgreSQL directly then that's where an app-specific plugin may come in (e.g. first layer of validation in Node.js rather than the DB?).

[lifeiscontent]

@benjie man I love how enthusiastic you are about this stuff, it's really inspiring. I'll put together some error messages when I wake up tomorrow and explain the use cases I'm thinking of 👍

Thanks again for all the hard work

[lifeiscontent]

@benjie Hi again, sorry about being M.I.A. for the last 2 days. I'm back in, working on the app. first question I'm curious about, if I wanted to be able to return the authenticated user with the authenticate method in the example forum app, how would I go about doing something like that? or is this even possible currently?

at the very least, the data requirements of a typical app is the token, and the id of the current user.

create function forum_example.authenticate(
  email text,
  password text
) returns forum_example.jwt_token as $$
declare
  account forum_example_private.person_account;
begin
  select a.* into account
  from forum_example_private.person_account as a
  where a.email = $1;

  if account.password_hash = crypt(password, account.password_hash) then
    return ('forum_example_person', account.person_id)::forum_example.jwt_token;
  else
    return null;
  end if;
end;

[benjie]

So I'd do it like this:

mutation Authenticate($email: String, $password: String) {
  authenticate(email: $email, password: $password) {
    jwtToken
    query {
      currentUser {
        id
        name
      }
    }
  }
}

(Assuming you have a currentUser method.)

The caveat with this is that your authenticate method will need to select set_config('jwt.claims.user_id', account.person_id, true); or similar otherwise that won't be carried through to the rest of the query.

[lifeiscontent]

@benjie when I update the method like so, I get an error.

create function forum_example.authenticate(
  email text,
  password text
) returns forum_example.jwt_token as $$
declare
  account forum_example_private.person_account;
begin
  select a.* into account
  from forum_example_private.person_account as a
  where a.email = $1;

  if account.password_hash = crypt(password, account.password_hash) then
    select set_config('jwt.claims.person_id', account.person_id, true);
    return ('forum_example_person', account.person_id)::forum_example.jwt_token;
  else
    return null;
  end if;
end;
$$ language plpgsql strict security definer;

[lifeiscontent]

@benjie sorry, this questions probably pretty basic, I just figure you've already dealt with a lot of these problems.

[lifeiscontent]

got it.

create function forum_example.authenticate(
  email text,
  password text
) returns forum_example.jwt_token as $$
declare
  account forum_example_private.person_account;
begin
  select a.* into account
  from forum_example_private.person_account as a
  where a.email = $1;

  if account.password_hash = crypt(password, account.password_hash) then
    perform set_config('jwt.claims.person_id', account.person_id::text, true);
    return ('forum_example_person', account.person_id)::forum_example.jwt_token;
  else
    return null;
  end if;
end;
$$ language plpgsql strict security definer;

[benjie]

Oh yeah, sorry I forgot you have to change select for perform if you're not using the result.

For future issues, when reporting a bug "I get an error" is not useful - tell us what the error is 🙂

[lifeiscontent]

@benjie I've been looking through the docs for apollo, so I'm trying to optimize the app as far as I can without bugging you, and then when I'm ready, hopefully I'll have some solid feedback for you 😄

[lifeiscontent]

@benjie just fixed all the mutations to update the cache properly, next is error messages! Would love another review when you have time. https://github.com/lifeiscontent/postgraphql-forum-example-apollo-client/pull/1/files

[lifeiscontent]

@benjie so here's what I've been thinking about in terms of an interface for errors.

you should be able to query it within mutations/queries.

for example:

query AllPostsQuery {
  allPosts {
    nodes {
      title
    }
    errors {
      field
      type
    }
  }
}

where errors is.

type Errors = {
  field: String
  type: ErrorType!
}

errors.field will return null if there is an error that isn't related to a field.

I don't suggest having a message field so that users can agnostically create the message on the client using I18n.

and ErrorType being a value from an enum depending on the type of error such as field is required, email is unique, range errors such as min and max for dates/int/string length, etc.

let me know what you think about this, I can also create a seperate issue if you want for this topic in case others have opinions on the matter, but I think this would be sufficient for rendering the proper messages to the client so a user can know how to proceed.

[lifeiscontent]

@benji any updates here? If there is something I can help with, I wouldn't mind contributing, but I'm still very new to the codebase so guidance would be helpful if you don't have the time to focus on this. 👍

[benjie]

Sorry for the delay, I do this work in my spare time and wanted to make sure I had a solid understanding of the best practices before replying.

I had a read through graphql/graphql-js#560 (also graphql/graphql.github.io#204) and a number of other resources on the web and the consensus seems to be that sometimes modelling the errors as part of the data of the query (as you've done above) can be the best way 👍

How do you think we would implement this? I'd love to see some examples - input data, executed SQL, error thrown/caught, how that is output. I'm not sure how this would work with queries as you have suggested, but for mutations if it's validation errors / unique constraint conflicts I can imagine it working.

Rather than having a generic Errors type as you have above; I think I'd use union types or interfaces, so you can do:

mutation {
  createPost(input: {post: {slug: "/about", title: "About} }) {
    post {
      id
      slug
      title
    }
    errors {
      __typename
      message
      ... on ConflictError {
        fields
        conflictingRecord {
          id
          ... on Post {
            author
          }
        }
      }
      ... on ValidationError {
        field
        expected
        actual
      }
    }
  }
}

I hear you regarding not having message for errors; but if the system is updated and a new error added it would be better for clients to display a default error message (in the wrong language) than it would be to not display an error message. At least with a default error message they can stick it into Google Translate.

This definitely sounds like quite an undertaking - please do open a new issue about it.

[lifeiscontent]

@benjie sorry for the silence here, we shifted gears, now we're back on the grind for using postgraphile. I was wondering if there has been any movement in error handling since our last talk.

[benjie]

Afraid not; however there is a powerful plugin interface now, so expanding the schema with error features is feasible.

[lifeiscontent]

@benjie any plans to do some documentation on plugins? I'd love to jump in but I feel the docs are a bit bare so I'm not sure where to start.

On another note, how much would you charge to implement something like this?

[benjie]

Better docs for graphile-build (and some helpers to make it easier!) are on my todo list, but PostGraphile itself is my focus right now so it'll be a while before I get around to it. I'm working on trying to get more income so I can spend more time working on PostGraphile / graphile-build.

Could you supply a detailed spec of what you're after? Also have you seen @pyramation's work in #723 ?

[benjie]

graphile-utils exists now which makes it a bit easier to add certain types of plugins (e.g. ones that add fields/types):

https://www.graphile.org/postgraphile/extending/#the-easy-way-graphile-utils

I'm sticking with using GraphQL errors, but do note that we now expose the handleErrors function for you to override, so you can add whatever detail you like to the error (or even replace it completely) - this should make error reporting a lot more flexible.

I'm going to close this issue for now, but please feel free to keep discussing below. Reading back over this was pretty eye-opening and very valuable feedback - thank you for taking the time to engage with me! 👍