Implement independent @omit read

[ab-pm]

From Discord:

@benjie: @omit read was never fully implemented, which is why it requires you omit everything right now. @ab-pm: so what is missing then? @benjie: The ability to @omit read without @omit create/etc e.g. if you're allowed to create a field but you can't see the result. It had a whole bunch of things cascading off of it that would have broken so I just banned those combos allowing me to implement it properly later in a non-breaking way, but I've never got around to it (and not many people have needed it). @ab-pm: I could actually use that feature: I want to @omit read a column but still have it be considered by forward/backward relations. @benjie: YES! Exactly this! I want this so we can make it so we can just use global IDs everywhere and the underlying primary keys are omitted. But there are complications.

Sounds like this would be a really useful feature, but might require a redesign of the CRUDFOAMX table. Also currently many plugins check for @omit read on anything they use, so it might be a breaking change to have more fine-grained checks.

@benjie Could you please discuss your design decisions on this, and possible plans going forward?

[benjie]

Basically this array needs emptying:

https://github.com/graphile/graphile-engine/blob/eff7bbb8633f2ac5fa7f8c0b94479cbefb9233a4/packages/graphile-build-pg/src/omit.js#L37

And then, everywhere that it breaks assumptions in the schema needs fixing. I haven't had time to fully evaluate what those places are, and you're right that there's a conflation between omitting a column and omitting a relation that I regret - we cannot fix that before v5 alas - but we could enable create without read in v4 if we're careful to make sure the schema remains valid.

I've said all along that @omit is not to be used for permissions, so we don't need to worry about "leaking" values - e.g. if you @omit person_id on the posts table, you could still get the person's id from { post { person { id } } } but IMO that's not an issue.

One goal of this would be to enable dealing entirely in node IDs and never seeing the underlying primary key/foreign key values.

Another goal might be for things like a post "pincode" (required to view the post) where you can set it but cannot read it back. Of course, the complexity here is that you could determine the value of pincode by sorting by it and doing a binary search, or similar. But again, @omit isn't for permissions, so perhaps this isn't super important.

Another issue is what does this mean for functions. If you @omit read on an entire table, but return that table from a Custom Query or Computed Column function, should the function field be excluded?

Lots of questions I didn't find the right answers to when I was thinking about this before, so I punted. :)

[ab-pm]

Thanks! I think I'm mostly concerned about https://github.com/graphile/graphile-engine/blob/eff7bbb8633f2ac5fa7f8c0b94479cbefb9233a4/packages/graphile-build-pg/src/plugins/PgBackwardRelationPlugin.js#L99-L104 and https://github.com/graphile/graphile-engine/blob/eff7bbb8633f2ac5fa7f8c0b94479cbefb9233a4/packages/graphile-build-pg/src/plugins/PgForwardRelationPlugin.js#L104-L109

which don't allow me to use relations on omitted columns - and I would be totally fine to omit all the CRUD permissions on them, that's not an issue for me.

If you @omit read on an entire table, but [use the type somewhere else]

Isn't that already a problem currently?

[benjie]

We cannot (and will not) change the above code without a major version bump. We can augment it with alternatives, but @omit read cannot change what it does there.

Isn't that already a problem currently?

Probably. I'm a bit frazzled rn! Been doing about 3 times more open source than I've budget for, working ridiculous hours to boot.

[ab-pm]

Looking forward to v5 :-) Didn't expect this to be changed easily, but imo it's something we need to take into account when redesigning @omit read. What shorter-term alternatives can you think of, something with @includes?

[benjie]

The reason we don't have @includes is because that would fix the list of things we could @omit. We're still adding more things to the @omit list (and thinking of more we could add) so if we used @includes with our current set and then made more granular @omit rules then people's schemas would get broken :disappointed:

I haven't thought of an alternative currently; @omit output could work on a column, perhaps, without messing with the existing @omit read rules. We'd want output to replace read in v5 though, so it'd need to be well thought out. Then we can deprecate read and replace it.

[ab-pm]

OK I don't know how @include is supposed to work, I imagined it could be used together with @omit, overpowering each other (at least on different stages, like @omiting a whole table but @includeing a few selected columns etc.) But yeah it wouldn't be a good idea to have inconsistencies between omit and include about what read means.

How about a legacyOmitRead setting that defaults to true (in v4)? But I like output as well. For the "new" read I would suggest it only prevents exposing columns as graphql fields. An @omit read on the table would omit all columns. It should no longer take effect on constraints (both unique and relations) or make the table non-referrable.

[benjie]

I think v5 needs a different implementation of @omit (e.g. @exclude or similar) and then a layer that translates @omit into @exclude. I think @exclude should be a lot more granular and well thought out than @omit was, and if done well then @include can be added without causing unexpected behaviours. We're still adding new @omit distinctions, still learning what it should apply to.

[benjie]

This is enabled by the behavior system in V5. @behavior -select is likely the thing you want.