mohawk2
commented
6 years ago First off, as you can tell by the "TODO" in there, I don't think of the code there as finished! So thanks for taking a look and thinking about this.
Overarching principles are that the DBIC plugin is an attempt at making a full-CRUD interface in an idiomatically GraphQL way, so that just individual parts can be picked and chosen from, probably by annotating the "real" types with directives like @allowFullQuery on maybe the type. That would guide the creation of the Query type's fields. Another principle is that the schema must be maximally introspection-friendly, so that reading the Query fields available will give a very clear and accurate picture of what is available in the API. I suppose that makes it a "(modified) db schema as API" concept, which I like as it is very declarative. My goal is to make APIs constructable with just type declarations and some directives.
One nitpick I would draw to your attention: your code depends on "only one" PK, but DBIC supports multiple columns as a PK. I would prefer not to lose that. Naturally, that means we would need to add such a beast to the tests!
The "R" part of CRUD is something I haven't thought about very deeply so far. One consideration will be some combination of real security, and DOS-avoidance: any wide-ranging "allX" type query should be isolated into an individual query, so that it can be removed from the schema wholesale rather than just its interface being tweaked in not-very-visible ways.
Before delving too deeply into this specific path (which by the way is generally already useful!), could I ask you to take a look at making a more "Relay"-ish interface (which dictates the shape of the GraphQL schema somewhat)? You may see on the roadmap that's part of the plan, but so far I've been fighting with OpenAPI :-) (update: the "arbitrary object as input" thing finally works, so after a quick blogpost about Mojo OpenAPI<->GraphQL I'll be looking at refactoring it to make the data transformations clearer and easier for me to understand)
The links I've saved for Relay stuff are:
- https://www.learnrelay.org/
- https://facebook.github.io/relay/docs/tutorial.html
- https://www.howtographql.com/react-relay/1-getting-started/ (more React, obviously)
- https://blog.graph.cool/getting-started-with-relay-modern-46f8de6bd6ec
- https://blog.graph.cool/migrating-to-relay-modern-eb2711e7facd
Another very powerful thing if you feel like having a go would be a from_graphql method: making a DBIC schema (or even SQLT schema, same-same) from a set of GraphQL types.
Not ready for merging (tests will fail), consider a request for comment. This will somewhat change existing behavior and might lead to breaking changes.
The idea is to follow GraphQL convention for using singular vs plural query forms to return single item or a list of items, e.g.
user(id: Int!): Uservsusers(id: [Int!]!): [User]. Not having a way to request single object leads to some inconvenient gymnastics on the client side.Another point of interest is non-null requirement for PK argument in plural queries. Presently the list is strictly required so requesting the whole dataset involves passing in a list of arguments with zero members. I think this is a superficial requirement, making the list optional will be more readable. That would be a small but potentially breaking change, not implemented in this PR.
If the approach is ok I'll follow up based on comments and fix the tests.