Make nodes array items in Pg Connection non null.

[bradleyayers]

No description provided.

[benjie]

So this has some interesting reasoning; running this:

begin;
create table foo ( id serial primary key, name text not null );
insert into foo(name) values ('First result'), ('Third result');
create function bar() returns setof foo as $$
begin
  return query select * from foo where id = 1;
  return next null;
  return query select * from foo where id = 2;
end;
$$ language plpgsql stable;
select * from bar();
rollback;

Returns this:

 id |     name
----+--------------
  1 | First result
  ¤ | ¤
  2 | Third result
(3 rows)

Time: 42.686 ms

i.e. it is possible to return null from a setof foo function. In v3 functions got their own connection type; however this differed from v2 and caused a lot of issues for me personally as it meant I couldn't use the same fragments for paginating two different connections of the same type which happened to come from different functions (e.g. followers vs followees which are pretty much identical things to paginate otherwise).

Note that this isn't just functions, tables have this issue too, a bit (though in the case of a table all fields must be nullable for the row to be null):

begin;
  create table foo (id text);
  insert into foo (id) values ('First'), (null), ('Third');
  select foo is null from foo;
rollback;

 ?column?
----------
 f
 t
 f
(3 rows)

Time: 0.139 ms

I've gone with the broadest thing so that the most PostgreSQL functions and tables are valid. I'm open to making this not the case with a flag though (and I'm also open to making this flag on by default, because really this is very much an edge case!).

[benjie]

(As an aside, I'm also open to adding a flag that restores v3 compatibility for the UUID and JSON types. Something like --schema-compat=3 maybe?)

[bradleyayers]

Thanks for the detailed explanation! This would be great content for the upgrade guide, and probably the main connections guide too. I can't imagine it's obvious to anyone who only casually uses Postgres.

Regarding Uuid/UUID — I wouldn't worry about a flag. I handled it with some translation middleware that I'll keep in place until I'm confident all my users have updated their clients:

express.use((req, res, next) => {
  if (req.body && typeof req.body.query === "string") {
    req.body.query = req.body.query
      .replace(/: Uuid/g, ": UUID")
      .replace(/: Json/g, ": JSON");
  }
  next();
});

It's very crude, but means I have a graceful upgrade path. A simple flag to keep v3 compatibility would only delay this pain. Perhaps it's useful for others, but I personally vote for that dev investment going elsewhere.

[benjie]

Thanks for the input; I appreciate it!

If you wanted to jot down notes on what you've done in a wiki page (over on the PostGraphQL repo) so that I can pull them in when I write the release/migration guide that'd be super helpful.

[bradleyayers]

Did you consider just filtering out nulls in postgraphile, so that the type can be non null?

[benjie]

Yes, but that would not be a fair representation of that function. The null has to be fairly deliberate!

As stated though, I’d be happy for that behaviour to be controlled by a flag, and for the default to be that it’s non-null, because it’s really unexpected for null to be possible there for most database designs!

[bradleyayers]

After sleeping on this for a few days, I think I'd vote for being controlled by a flag, and defaulting to non-null. A flag is a bit tricky though. Simplest would be global, but it's probably more ideal to be per-table/function?

[benjie]

Yeah; maybe something like pgColumnFilter in #73; but only exposed on the PostGraphQL command line as a single flag --allow-nullable-table or similar. Something like pgAllowNullableTable(table) which returns false by default. Suggestions for better names welcome.