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

Example: how to create dedicated email and URL types #109

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Example: how to create dedicated email and URL types #109

wants to merge 2 commits into from

Conversation

singingwolfboy
Copy link
Collaborator

@singingwolfboy singingwolfboy commented Feb 13, 2020

This pull request uses PostgreSQL's domain types feature to define two new types directly in the database: email and URL. It also modifies the existing tables to use these types where appropriate, and creates two new graphile-engine plugins to expose these data types in the GraphQL schema.

The benefit of this change is that GraphQL clients are informed that these values in the schema won't accept just any string -- it must be properly formatted, whether it's an email or a URL. This was already enforced in the database, but because it wasn't exposed in the GraphQL type system, clients may not recognize this requirement until they run into a validation error.

If this pull request is accepted, then we can also use the URL type in #108.

@singingwolfboy singingwolfboy changed the title Create dedicated URL type Create dedicated email and URL types Feb 14, 2020
);

return GraphQLEmailType;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@singingwolfboy It seems to me that this plugin adds the Email type, with a comment, and performs validation on it. However we already perform validation on the type using the domain constraint you added; so I'm not sure if these 149 lines of code add much additional value? If this plugin were removed, would the following be sufficient?

create domain app_public.email as citext check(VALUE ~ '[^@]+@[^@]+\.[^@]+');
comment on domain app_public.email is
  'An address in the electronic mail system. Email addresses such as `John.Smith@example.com` are made up of a local-part, followed by an `@` symbol, followed by a domain.';

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As I mentioned in the description of the pull request, the benefit of making an Email GraphQL scalar is that anyone introspecting the schema knows that the corresponding field won't accept just any string: it must be a valid email address. You're correct that this was already enforced at the database level, but because it wasn't exposed at the schema level, it could cause surprises. Using an Email scalar is a fairly trivial example, but consider that this repo is also meant to be an example of how to do things with PostGraphile: I think that learning how to create your own GraphQL scalar with PostGraphile is very useful!

For a more concrete example of how this could be useful, consider a front-end application that you can point at an arbitrary GraphQL schema, and it can generate a pretty HTML form for doing queries and mutations on that schema. Because it needs to work with arbitrary schemas, it can't have much knowledge about the structure of the schema that it's going to see: as a result, it makes sense to have this application look at the scalars in the schema, and generate form fields based on that. If we expose the User.email field as type String, this application could only generate a standard text field; but if we expose it as type Email, it could style the field differently and provide dynamic validation based on how emails are formatted.

There's a section in the Shopify GraphQL Design Tutorial about naming and scalars, which I think is relevant here. Semantic value can be really useful for GraphQL clients, so I think it makes sense to expose that semantic value as clearly as possible.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I’m cool with the custom scalar and totally understand the reasoning; for some reason I was expecting creating a custom domain to result in a custom scalar being created. Seems maybe I never implemented that https://github.com/graphile/graphile-engine/blob/v4/packages/graphile-build-pg/src/plugins/PgTypesPlugin.js#L844

@benjie
Copy link
Member

benjie commented Feb 26, 2020

I think I'd like to see this as a separate plugin, potentially a built-in option to PostGraphile, rather than adding a significant amount of code to starter to handle it. Ideally just issuing

create domain app_public.email as citext check(VALUE ~ '[^@]+@[^@]+\.[^@]+');
comment on domain app_public.email is
  'An address in the electronic mail system. Email addresses such as `John.Smith@example.com` are made up of a local-part, followed by an `@` symbol, followed by a domain.';

should result in the GraphQL schema containing

"""
An address in the electronic mail system. Email addresses such as 
`John.Smith@example.com` are made up of a local-part, followed by
an `@` symbol, followed by a domain.
"""
scalar Email

@singingwolfboy
Copy link
Collaborator Author

That makes sense to me. Are you OK with leaving this pull request open until that feature exists in PostGraphile? That way, other people using PostGraphile are more likely to find this code and use it if they need it.

@benjie
Copy link
Member

benjie commented Feb 26, 2020

Absolutely; I plan to leave it open 👍

@benjie benjie changed the title Create dedicated email and URL types Example: how to create dedicated email and URL types Mar 25, 2020
@benjie benjie changed the base branch from master to main June 24, 2020 11:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants