Add @omit single smart comment

[tinymarsracing]

This would be useful for cleaning up your schema. I think an omit for queries by unique indexes (that are generated in addition to byId) is also not available.

---

BENJIE'S NOTES:

I've renamed the issue; we're currently proposing an @omit single smart comment which would:

• on a table foos: omit the single root fields foo(nodeId: ID) and fooByUniqueKey(uniqueKey: ...)
• on a unique constraint: omit the root field fooByUniqueKey(uniqueKey: ...)
• on a foreign key constraint: omit the "forward" relationship (it would not omit the backward relationship, even if it's a 1-to-1 relationship, to do so you'd use @omit many on the constraint)

[benjie]

The subtleties here can be handled by placing the @omit on the constraints. An omit on the table directly would apply to all constraints. Effectively we want to prevent these constraints/tables being added to the root of the GraphQL schema (i.e. to the Query object).

At the moment we use CRUDFOAMX as the omit options. Since we can't use R for "root" again, I think Q for Query might make sense. Any other suggestions?

[tinymarsracing]

I realize now that a part of this was already documented. But I'm confused now. So, I guess we have a couple of queries we could potentially omit from root:

• allThings --> "@omit all" on table
• thingByPk --> "@omit" on constraint pk
• thingByAnyOtherUniqueConstraint --> "@omit" on that constraint
• thing (= thingByNodeId) --> ???

If we want to save some time:

• all root queries --> "@omit" on table

Okay, and now I think I get it. Currently, "@omit" also omits mutations etc. But we might want to keep them. But I guess in the CRUDFOAMX, I would expect that "@omit read" would do exactly that (omit all root queries). When I tried that however, it threw an error. And then "@omit" would be for omitting the whole CRUDFOAMX. Also, in the table, I would expect it to say: "R | read | omit root queries" and not "omit completely" which is not "@omit read", but "@omit"? I hope you can follow me. :)

[benjie]

The thing is, @omit read to me means remove any way of reading that value - not just at the root, but also anywhere else. Omitting from the root is different.

For example, imagine a many-to-many table situation:

create table a (
  id serial primary key
);
create table b (
  id serial primary key
);
create table a_b (
  a_id int references a,
  b_id int references b,
  primary key(a,b)
);

If you did @omit read on a_b then not only would it drop the root allABs and aBByAIdAndBId accessors, but also the B.aBsByBId and the A.aBsByAId fields which would prevent traversal of the relationship.

On the other hand, omitting from the Query type only (@omit query) would only drop the root-level fields, but would still allow the relation to be traversed deeper into the graph.

And yes, @omit-ing the constraints will also remove (some of) these fields; but that would also remove the various mutations that you may wish to keep; for example createAB may want to exist, even though you don't have a root field to query that relation directly.

Currently you cannot @omit read without also omitting related permissions, because I couldn't reconcile being able to create something with not being able to see the result. This may be lifted in future, hence why it throws an error currently to allow for a future non-breaking change in behaviour.

[tinymarsracing]

Thanks, that answer cleared many things up. I guess the whole GraphQL "mutations also require reads to return the fields" system has some implications.

So that's why you say "omit completely" for "read". But why not just leave out read then? Assuming I don't miss anything, read requires also omitting everything else, thereby making a working read equivalent to a full @omit. Then in additional to the full @omit we would have QCUDFOAMX (query, create, update, delete...).

Considering we already have @omit all which would be a more specific @omit query and then adding that the constraint omits have unintended consequences, I would propose to also add @omit one which would remove any thingByUniqueConstraint (incl. PK). Or if you think people more likely want to split these, you could do @omit byPk and @omit byUnique. So then we have:

*QcudfoaPUmx (* is @omit, new additions to the table are capitalized). This looks like a password! :laughing:

[benjie]

I'm reserving @omit read for future use, and R is read in CRUD.

@omit one is a good idea; how about instead: @omit single which'd give us CRUDXFOAMS

[benjie]

So to omit root you'd do @omit all,single; but it's not clear what that means for the "node" fetcher, since there's also the {node(nodeId: "...") {...}} interface that can still get the relevant record. Then again; I've said all along that these are not permissions features, they're just for helping to tidy your schema.

[tinymarsracing]

Yeah, that's why I wanted to get rid of the node queries. Because there's always this general one you mentioned. But I feel like we don't need smart comments for that. Would be more useful as a general option I think.

I think your solution with @omit all,single is nice. It's minimal and still covers most use cases (except for very specific ones).

I get that you want to stick to a CRUD+ interface, but come on, GraphQL is the future and GraphQL is QMS! :grin: But seriously, I figured you wanted to reserve it in case the strict rules change at some point, but for me the way it is shown in the table in the docs was difficult to understand. So you could just change it there, but leave the actual smart comment unchanged.

[ab-pm]

thing (= thingByNodeId) --> ???

I think that @omit all on the table should remove all the root queries - not just the allThings list/connection, but also the thingByPrimaryKey and thingByNodeId and thingByUniqueKey. To customise, allow @omit all on the individual columns and/or constraints.

[ab-pm]

Bikeshedding: should it be single and all, or one and many?

Maybe even have both, use @omit one to omit the forward relation, and @omit single to omit the row-by-unique fields?

[benjie]

To be clear the all relates to the allFoos property at the root only; we cannot change that as it would be a breaking change.

[ab-pm]

Yes, I don't want to change those. I'm suggesting:

• all: Query.allFoos, Query.allBars
• single: Query.fooById(id), Query.barById(id), and in the future maybe Bar.fooBy…(…)
• many: Bar.foos
• one: Foo.bar

[shellscape]

Given the length of time that this has been open, and the length of time that v5 has been in preparation, is there any chance we could get this into v4?

[benjie]

In V5 the behavior system is how you'd accomplish this. Something like comment on table foo is E'@behavior -query:resource:single';