Add support for variable-types apart from keyword

[itadventurer]

This is a cross-post of the Issue in the original venia repository as it seems dead: Vincit/venia#36.

Currently the spec describes allows to be a :variable/type to be only a keyword:

(s/def :variable/type keyword?)

This works fine in many cases but is for our use case a serious limitation.

I would propose to add functionality to suport all valid GraphQL input types as described in the GraphQL Spec.

Here are some examples:

I will use following query as an example (and modify it accordingly):

query Foo($Foo:Int){employee{name}}

The corresponding Venia query (as it works now without problems) is:

{:venia/operation {:operation/name "Foo" :operation/type :query}
 :venia/variables
 [{:variable/name "Foo"
   :variable/type :Int}]
 :venia/queries [[:employee [:name]]]}

I will focus now only on the :venia/variables part:

Simple variable

GraphQL:

query Foo($Foo:Int){employee{name}}

Variables:

[{:variable/name "Foo"
   :variable/type :Int}]

Required variable

GraphQL:

query Foo($Foo:Int!){employee{name}}

Variables:

[{:variable/name "Foo"
  :variable/type {:type/type :Int
                  :type/required? true}}]

Another option would be to add a :variable/required? key to the variable map but I personally dislike this option as the required-property is part of the type and not the variable definition.

List

GraphQL:

query Foo($Foo:[Int!]!){employee{name}}

Variables:

[{:variable/name "Foo"
  :variable/type {:type/kind :list ;; :type/kind instead of :type/type!
                  :type.list/items {:type/type :Int
                                    :type/required? true}
                  :type/required? true}}]

I do not particulary like the :type/kind declaration as there is only one valid value (:list) for it but I do not see another nice way to do so. I mislike the idea to add a special :type/type called :List as it is not reserved by the GraphQL specification (as far as I can see) and someone may add a custom :List type to their schema and then this will break.

Final remarks

According to the GraphQL specification this are the only valid options for the variable types.

Implementation

@r0man already implemented a way to add Lists as types here but it misses following patterns (when I read the code correctly): [Int]!, [[Int]] and so on.

I will provide an implemenation (and create a PR) as soon as possible but would be happy for any feedback for this solution approach.

[itadventurer]

I have added a PR to the original venia repository: Vincit/venia#37 If you agree, I will modify it to match this repository and post it here.

[fbielejec]

I have added a PR to the original venia repository: Vincit/venia#37 If you agree, I will modify it to match this repository and post it here.

Sorry for getting back so late, I have missed your comment. If you're still interested please make a PR in this repository and we'll merge it.

[itadventurer]

Currently I am not doing any Clojure so I will not open a PR. But if anybody else wants to do that, feel free to move the PR to that repo.

[vvvvalvalval]

While I agree this issue needs fixing, I would suggest to consider an alternative to what's been done in Vincit/venia#37 : just allow :variable/type to be passed as a raw GraphQL String. Happy to provide a PR for that.

Granted, that leaves more room for invalid queries, but OTOH invalid queries cannot be prevented unless this lib implements a full type checker.

Meanwhile, as GraphQL gets extended, we're at risk of new syntax for GraphQL variable types, forcing us to petition the maintainers of this lib at every turn. Using raw Strings as an escape hatch could go a long way in mitigating that risk.

Admittedly, the downside of raw GraphQL expressions is that they somewhat impede static analysis on this lib's query data structures (though in GraphQL's current spec, parsing a variable type would not be that hard). Furthermore, currently this lib is already prone to such parsing difficulties, by allowing keywords such as :Int! (or (keyword "[Int]") people who dare to use this sort of stuff, even though it's quite fragile).

Of course, what I suggest and what Vincit/venia#37 did are not mutually exclusive strategies.