graphile
Allow relation objects with non-null key columns to be defined as non-nullable in generated schema [has plugin]
I'm submitting a ...
- [ ] bug report
- [x] feature request
- [ ] question
PostGraphile version: 4.0.0-rc.3
Minimal SQL file that can be loaded into a clean database:
create table current.a (id serial not null primary key);
create table current.b (id serial not null primary key, some_a integer not null references current.a);
insert into current.a values (DEFAULT);
insert into current.b (some_a) values (1);
Steps to reproduce:
- Run Postgraphile in the usual manner.
- Examine generated schema.
Current behavior: B.aBySomeA is specified as nullable.
Expected behavior: B.aBySomeA is specified as non-nullable because the corresponding relation column some_a is not null, just like B.someA.
Thank you again for your hard work, and for the quick fix on the last issue I reported!
Well, in an effort to be more helpful, I did a little digging and see that (1) this issue should probably be in the graphile-build repository (sorry about that), and (2) there is a comment in the affected area which says // Nullable since RLS may forbid fetching. I guess that could make sense in some scenarios, so then I guess I am instead asking for this to be configurable in some way (or for the system to be more intelligent about relations without any policies on the destination table). As the programmer, I can guarantee that some relations will exist since they have no RLS applied to them, and making these non-nullable makes life easier for consumers who would otherwise need to do unnecessary null checks.
Hey, great job on the digging! I really need to get round to writing a “why is it nullable?” article in the docs!
Removing a non-null constraint is a breaking change, so this certainly wouldn’t be done by default to avoid breakage if you implemented RLS later. A plugin to make these relations non-null should be pretty simple though; you just need to replace the “type” attribute with itself but GraphQLNonNull-wrapped. I’m not available to sketch it out right now but ask in the gitter chat and someone may be able to help you.
Removing a non-null constraint is a breaking change, so this certainly wouldn’t be done by default to avoid breakage if you implemented RLS later.
I can understand not wanting to create a default-breaking change, though I guess to me this doesn’t feel any more or less plausible than a programmer altering a column’s NOT NULL flag, and that would cause the same kind of schema change.
So, I wrote a plugin for this. It works for me, but I’m having some trouble fully generalising the additional database introspection so it works for all users.
(1) It’s unclear what the correct way is to run an additional query to retrieve policy data from the server. The introspection plugin uses an internal withPgClient wrapper, which handles the different ways that the connection might be exposed on pgConfig. (Incidentally, since Node 8.x, the withPgClient wrapper is mostly duplicating pg.Pool#query, so if there is a single place to get a fully constructed pg.Pool instance, withPgClient can probably be deleted.)
For the moment I am just using pgConfig directly since I’m running Postgraphile as a library so guarantee that pgConfig is a pg.Pool instance. The alternative seem to be to do an import of that internal module, which seemed worse.
It seems to me like the best thing would be to construct and expose a single pg.Pool instead of pgConfig which abstracts away the connection information from the rest of the system.
(2) It’s unclear to me what is the right thing to do about schema watching. The policy list needs to be updated on a schema change to policies, but this seems to all be internalised to PgIntrospectionPlugin (which doesn’t check for CREATE/DROP POLICY anyway, which is probably a bug since RBAC relies on that?) and SchemaBuilder. Hooks are synchronous so I can’t look things up on 'introspection' or 'build' hooks, which seems like it’d be the easiest way to do this. Since I don’t use this functionality I don’t bother with it right now.
That’s all for now from me.
withPgClient is required for when people don’t use a pgPool; for example they may want to ensure just one client is used and that it is correctly configured before handing to PostGraphile. This is particularly useful in tests.
Just using pgPool is fine, just note that’s what you do in the README 👍
RBAC support only uses GRANT right now; policies don’t affect what tables/columns you get, only the rows, so we didn’t need it. Please feel free to send a PR adding to the watch schema though! 👍
Watching is done with the registerWatcher function, documented here:
https://www.graphile.org/graphile-build/schema-builder/#registerwatcherwatcher-unwatcher
You’re correct that a schema build is (deliberately) synchronous; all async work should be completed separately, stored into your plugin’s closure, then a schema rebuild triggered.
I see your point about nothing else being able to add an event listener to the introspection plugin’s change listener. That’s not something I’d seen a need for before now. I shall give it some thought.