Skip to content

Smart comment @nonNull does not work for computed columns #1605

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

Closed
bajtos opened this issue Apr 5, 2022 · 2 comments
Closed

Smart comment @nonNull does not work for computed columns #1605

bajtos opened this issue Apr 5, 2022 · 2 comments

Comments

@bajtos
Copy link

bajtos commented Apr 5, 2022

Summary

I have a computed column that I would like to mark as non-null using the smart comment feature.

Based on the discussion in #906, the solution is to use the plugin @graphile-contrib/pg-non-null.

However, that plugin seems to be mostly abandoned. Quoting from graphile-contrib/pg-non-null#2 (comment):

Hey there, this plugin is a bit obsolete. The @notNull smart comment is available in PostGraphile as a built-in. This plugin is mostly useful for its PgNonNullRelationsPlugin part.

Unfortunately, the built-in support for @nonNull does not seem to work for computed columns :(

Steps to reproduce

Using a slightly modified example from https://github.com/graphile-contrib/pg-non-null#usage

CREATE TABLE public.customers (
  id         serial PRIMARY KEY,
  first_name text NOT NULL,
  last_name  text
);

CREATE FUNCTION public.customers_full_name("user" public.customers) RETURNS text AS
$$
  SELECT "user".first_name || ' ' || "user".last_name
$$
LANGUAGE SQL STABLE;
COMMENT ON FUNCTION public.customers_full_name(public.customers) IS E'@nonNull';
$ postgraphile -c postgres://YOUR_CONNECTION_STRING --export-schema-graphql schema.graphql

Expected results

The generated GraphQL schema marks the field Customers.fullName as non-nullable.

type Customer implements Node {
  """
  A globally unique identifier. Can be used in various places throughout the system to identify this single value.
  """
  nodeId: ID!
  id: Int!
  firstName: String!
  lastName: String
  fullName: String!
}

Actual results

The generated GraphQL schema marks the field Customers.fullName as nullable.

type Customer implements Node {
  """
  A globally unique identifier. Can be used in various places throughout the system to identify this single value.
  """
  nodeId: ID!
  id: Int!
  firstName: String!
  lastName: String
  fullName: String
}

Additional context

To get the desired behaviour, I have to configure the @graphile-contrib/pg-non-null plugin.

$ postgraphile -c postgres://YOUR_CONNECTION_STRING --export-schema-graphql schema.graphql --append-plugins "@graphile-contrib/pg-non-null"

Possible Solution

It would be awesome if Postgraphile supported this feature out of the box so that I don't have to install that plugin.

The docs at https://www.graphile.org/postgraphile/why-nullable/#what-about-computed-fields say:

I'd be happy to accept a Pull Request that adds functionality marking a function as non-nullable via a smart comment (e.g. COMMENT ON FUNCTION foo_func(foo) IS E'@nonNull';) - do raise an issue if this is of interest to you.

I am happy to contribute to this feature if you could provide me with a few pointers on where to start looking in the codebase and where/how to add tests.

@bajtos bajtos added the 🐛 bug label Apr 5, 2022
@benjie
Copy link
Member

benjie commented Apr 5, 2022

At the moment we’re prioritising getting V5 over the finish line; in the mean time consider using https://www.graphile.org/postgraphile/make-change-nullability-plugin/ instead to achieve this goal 👍

@benjie benjie added the 🔁 revisit-in-v5 Not happy with this behaviour, but changing it is a breaking change. label Apr 5, 2022
@benjie benjie added 🔮 fixed-in-v5 and removed 🔁 revisit-in-v5 Not happy with this behaviour, but changing it is a breaking change. labels Sep 29, 2023
@benjie
Copy link
Member

benjie commented Sep 29, 2023

This works fine in V5, but the smart tag is @notNull (to match the database language) rather than non-null which would match GraphQL.

@benjie benjie closed this as completed Sep 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants