RFC: inputUnion type

[tgriesser]

A proposal in response to #207 and based on my comment on this issue.

Motivation:

Currently, input types are restricted to a single definition / type signature. When you wish to allow for variable mutation inputs for a single type you'll normally end up down one of two paths.

1. Creating one mutation per signature:

input EditPostTitleInput {
  id: Int!
  title: String!  
}
input EditPostBodyInput {
  id: Int!
  body: String!
}
type Mutation {
  editPostTitle(input: EditPostTitleInput!): Post
  editPostBody(input: EditPostBodyInput!): Post
}

2. Loosening up the required fields on signatures and relying on application logic to handle intended uses.

input EditPostInput {
  id: Int!
  title: String
  body: String
}
type Mutation {
  editPost(input: EditPostInput!): Post
}

This proposal aims to offer a third option, inputUnion

input EditPostTitleInput {
  id: Int!
  title: String!  
}
input EditPostBodyInput {
  id: Int!
  body: String!
}


inputUnion EditPostInput = EditPostTitleInput | EditPostBodyInput

type Mutation {
  editPost(input: EditPostInput!): Post
}

mutation {
   editPost(input: {__inputname: "EditPostTitleInput", id: 1, title: "RFC"}) {
      id
   }
}

With the current specification it isn't practical to repurpose GraphQLUnionType for inputs, as these deal with type rather than input. Therefore, this PR proposes a new type, inputUnion.

From @leebyron in #207:

I know at FB we've stayed away from this sort of input flexibility as it leads to more complex and easy to mess up server side implementations and in some cases can lead to ambiguities where it's not possible to determine which type in the input you meant, but I definitely understand why this could be valuable.

This PR attempts to allow the flexibility of input signatures, while also eliminating ambiguity by requiring that the input type is specified when fulfilling the inputUnion via __inputname.

Details

• The inputUnion type mirrors the union type, it is restricted to members being other input types.
• inputUnion are also valid as fields on an input type.
• Any time an inputUnion is fulfilled, it must be fulfilled by a single member of the inputUnion, as specified by the __inputname arg.

Benefits

• More specificity in crafting input types with required fields
• Composing multiple input types into complex type signatures
• More possibility for strongly typed inputs without the need for endless endpoints

Drawbacks

• Requiring __inputname is a bit ugly, but it eliminates ambiguity.

Unresolved questions:

• Is the use of __inputname a valid option based on spec (__ is reserved for introspection, not sure if we can mirror this for execution)
• Does __inputname make sense as the name for this?
• Should the inputUnion only allow for union of input (as it is in this PR) or should we allow for scalar / enum? (checked and this isn't permitted in a normal union so it makes sense it wouldn't be supported here)

[nodkz]

Great work 👍

[IvanGoncharov]

@tgriesser You did a great job with this PR 👍
But I think such change affecting the whole GraphQL community and not only users of graphql-js. So I would suggest creating PR against the spec and follow the standard flow.

[tgriesser]

@IvanGoncharov thanks! Thanks for pointing those out. I complete agree, I will work on a formal PR against the spec to go with this PR.

[danielstclair]

Looks great. Ran into this issue before and truly happy to see this.

[hasyee]

What's about this PR? :)

[jonaskello]

We need this too, please merge :-).

[facebook-github-bot]

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need the corporate CLA signed.

If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks!

[facebook-github-bot]

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

[lblazecki]

any news about this PR?

[IvanGoncharov]

@lblazecki It was discussed on the last working group:
https://github.com/graphql/graphql-wg/blob/master/notes/2018-06-14.md#discuss-proposed-rfc-for-inputunion-type-related-graphql-js-pr-tg-10m
TL;DR; Proposal in its current form was rejected because of __inputname semantic. At the same time, everyone agrees that GraphQL missing such functionality so alternative solutions should be explored.

[raderio]

Proposal in its current form was rejected because of __inputname semantic

Can you please be more detailed about this?

[tgriesser]

@raderio see ongoing discussion in graphql/graphql-spec#395

[acao]

@tgriesser let me know if theres any way we can help as the spec discussion rounds out.

[acao]

I've been working on getting this working in graphiql, at this point focusing mostly on the graphql-language-services monorepo and getting inputUnion tests passing there. Finally have all the dependencies linked and wired up so that tests are failing the way they should be and I can iterate on implementing the language support 🎉

Then I can wire up graphiql to this PR and to a working language services fork, et voila!

I think our only possible option for an experimental shipment of graphiql that supports inputUnion at this point is a dist that we're able to build by linking all these forks together manually. Maybe we can even stick it on a CDN somewhere.

The npm/yarn ecosystem doesn't handle using git branches for package management well, so there's not really a way we can get a standalone working branch of graphiql with inputUnion. For example, graphql-language-services depends on graphql-config and graphql-import (and thus @types/graphql) need updates to be able to aggregate input unions in order for graphql-language-services to have working tests.

In fact, I made this diagram to help myself get some tests running.

Feature completion for the official underlying graphql ecosystem will necessitate PRs to all of these repositories, and requisite updates by downstream consumers of graphql-codemirror and graphql-language-services.

So, graphiql is a good scope completion point because it's the official tool and it ships with most of the runtimes?

P.S. I have some ideas and have done some experiments... I'm sure this has been discussed for a while... but imagine a world where a new language feature could be executed in one PR to the spec, and one reference PR to the entire ecosystem... with one type system... (this is a discussion for elsewhere, possibly an issue on the gls issue queue? or graphqljs?)

[acao]

As for this PR, ASTDefinitionBuilder._makeSchemaDef is missing INPUT_UNION_TYPE_DEFINITION and an _makeInputUnionDef method, as well as a few other things, I'll open a fork with a PR for this branch!

GraphQL Asia in Bengalaru

So - if folks would like to see this working in graphiql at GraphQL Asia, which I won't be attending sadly 💔 , I should have it working by tomorrow 🚀 . Was just in lovely Karnataka last year, I'll be back soon!

Hoping I can write a script tomorrow to re-create this whole stack, requisite manual linking and all, so folks can run the prototype locally at GraphQL Asia? is just a makefile/ shell script ok? If there's demand, I dont mind making sure it works in windows.

What's next

Just getting the last of the validation working in codemirror-graphql, but the rest of the stack down is working on custom branches.

About to wire up graphiql branch with all the local packages! haha. this is fun. Learning the whole ecosystem, so any potential further iterations on this particular spec shouldn't be that difficult for me to support. The default keyword is currently my only missing ingredient in tests and in language-services.

I'll just push my forked branches of each of the repos without opening PRs just yet, and the script will clone from my forks of:

• graphql/graphql-js
• graphql/graphql-language-services (monorepo)
• graphql/codemirror-graphql
• graphql/graphiql
• DefinitelyTyped/DefinitelyTyped
• prisma/graphql-config
• prisma/graphql-import

[acao]

Here's the working branches so far for graphiql, I suspect only maybe a tweak to graphql/codemirror-graphql
 left as described above:

• DefinitelyTyped/DefinitelyTyped@master...acao:graphql-inputUnion
• ardatan/graphql-import@master...acao:inputUnion - tests passing
• tgriesser/graphql-js@inputUnion...acao:inputUnion - the aforementioned tiny fork of this PR, tests passing
• [EXPERIMENTAL] - support inputUnion graphql-language-service#351 - tests passing with inputUnion in .graphql files! only one failure left, where remote executable schemas arent working with server? an issue with graphql-config?
• graphql/codemirror-graphql@master...acao:inputUnion/ - almost all tests passing now, with inputUnion in test fixtures as well. probably related to the issue with the schema def not generating below
• [RFC] inputUnion (WIP) graphiql#782

[acao]

Welp, once I got everything wired up just right, your excellent PR @tgriesser had everything ready to go! (this is from the basic graphiql test schema, the resolver just writes the query variables to string)

All the validation cases worked fine manually, where __inputname was missing or invalid, it gave helpful instructions.

Only issue left is that for some reason the introspection result has no possibleTypes, though the parser result has types, and thus the client schema fails to generate:

(for some reason it swallows the more descriptive exception here)

[acao]

fyi, as of a couple months ago, @tgriesser's PR already worked standalone for schema execution from GraphQLSchema - in the above case using the graphiql test server using graphql-express. My last commit just adds one last bit for full AST support. The last needed working feature is schema introspection, which is needed along with the above graphql-language, codemirror-graphql changes for graphiql or any graphql client to work.

it seems that possible in type/introspection.js we need to add possibleTypes to __InputValue type for this proposal, because a field's arguments will now potentially have possibleTypes from an introspection standpoint? After adding some tests, this seems to be what breaks buildClientSchema. But I can't figure out where when building the schema for the introspection query it's missing out on the types -> possibleTypes mapping?

I've been working on some additional tests for this PR that are helping me to get to the bottom of this!

[acao]

ok mostly got it! just a few more tweaks to this branch and the introspection query works. the above issue turned out to be just a missing condition for isInputUnion in isAbstractType which prevented the introspection resolvers from returning the possibleTypes. Downstream, autocomplete is working, but things are looking good from this repo.

[acao]

Almost done merging in the upstream master branch to this branch, and refactoring around the latest patterns and test improvements. Has been a great opportunity to learn how this library has evolved over the last year or so! Note in the last commit above, there was some regression with util.inspecting tokens in tests which seems to be fixed with the latest code

[asprouse]

@acao Is this the fork that was demo'd yesterday?

[acao]

@asprouse yes I demoed with this, alongside the graphiql fork, and the other forks I listed above.

This branch should be sufficient on its own for a server implementation with, say, apollo-server-core. It should work with any GUI client or cURL, though the GUI clients might throw an error at first.

If you'd like to be able to render the schema for a GUI client, you can use it alongside the graphiql fork. It should build the client schema and render the schema explorer, but without the graphiql fork the schema explorer won't work at all, I'm guessing. Autocomplete and hightlighting also will not work yet, as there is still some work to do on the codemirror-graphql fork, and underlying language-service packages as well.

Possibly this week I will roll a pre-packaged demo of inputUnion

[acao]

https://github.com/acao/apollo-graphql-input-union-example

here's a quick demo!

[asprouse]

@acao Checking this out now!

[acao]

As promised, here's a general utility script I created tonight that handles building all the linkages for you, if anyone wants to iterate on another fork of their own for the various proposals for inputUnion all the way up to graphiql:

https://www.npmjs.com/package/yarn-compose

the readme demonstrates a config for re-creating the setup I described above, using the same branches that are linked from above in this PR. this way, you can switch out for other forks more quickly.

[leebenson]

I'm curious, a year and a half later, if there's still motivation to get this merged?

This solves a very real need for us, and I know a great many other users have been very vocal about the lack of input unions. This seems like it would offer an obvious win.

Could we get a little context as to why this is hanging around so long without getting merged? Thanks!

[IvanGoncharov]

@leebenson graphql-js is reference implementation so we can't merge stuff before it's accepted into GraphQL Specification. There are multiple competing approaches to how we should implement this feature so we started working on Specification RFC: https://github.com/graphql/graphql-spec/blob/master/rfcs/InputUnion.md

[leebenson]

Understood, I realise this is tracking the spec. To be clear, I was querying the state of input unions in their entirety, with this PR representing (to my mind) the fullest concrete implementation.

I've followed the numerous disparate threads on the topic, and conversation for the most part seems to have stalled (with the notable exception of https://github.com/graphql/graphql-spec/blob/master/rfcs/InputUnion.md 4 days ago-- which is encouraging!).

I'm curious what the bottleneck is.

[IvanGoncharov]

@leebenson There are number of alternative proposals with its pros and cons. In the long run, we can't based language design on what PRs are ready to merge we should need to evaluate all alternative proposals.

[leebenson]

Fair point, understood, thanks. I guess I'm mostly just curious why it's taken 1.5+ years and what the current state is after all this time. And more importantly, if there's anything non-core contributors can be doing to help speed this up in some way.

[IvanGoncharov]

@leebenson Community can help by providing well-defined use cases for this feature:
https://github.com/graphql/graphql-spec/blob/master/rfcs/InputUnion.md#use-cases
We need those to evaluate all proposal against them to see what solution are best suited and also fit GraphQL design goals.
If you or any other community member have time to work on it, just open a draft PR against RFC with your use cases to get early feedback.

[acao]

i haven’t had time to prepare an example, with GraphQL LSP work and all, however I had what i think would be a great idea for an example. imagine a mutation field that expects a GeoJSON feature collection as an argument, which would necessitate polymorphism to accept the various types of GeoJSON features, each with their own input object definition. Real world use case would be, say a tool where you can draw geojson features, or a remote data collection tool using GPS with an option to bulk upload/sync findings once the user has access to the network. the other fun aspect is an a freeform property inside each feature i think called properties

[IvanGoncharov]

@acao Thanks for idea 👍 One important note is that use cases preferably should be extracted from real-life I think we already have a lot of them in different issues across graphql-spec repo.

[leebenson]

One important note is that use cases preferably should be extracted from real-life I think we already have a lot of them in different issues across graphql-spec repo.

Many of the examples I've seen are from real-life. Certainly for us, the use-case isn't speculative. Even those that are, are analogous to real-life scenarios. I don't think an organization should necessarily spend time/money/resources on coding up to the point of discovering this obvious flaw before it's taken seriously. The attributes that render this limiting are the same regardless!

The lack of concrete details in our case, is just because our particular business domain doesn't yet allow for public sharing specific blocks of code, so it's easier to talk in broad strokes. The interim TL;DR is that having input types match the flexibility of output types makes for a more balanced API for our users. This is true of a great many domains, since we're only talking about parity between inputs and outputs. Is there a particular reason why Facebook has elected in its design to demote inputs to second class types?

In short (and nothing that hasn't already been discussed by others offering more concrete examples, so this isn't anything new):

Instead of:

doSomethingWithA(input: TypeA!);
doSomethingWithB(input: TypeB!);
doSomethingWithC(input: TypeC!);

(which is verbose and results in a ton of repeated resolver code)

Or:

doSomethingGeneral(inputA: TypeA, inputB: TypeB, inputC: TypeC)

(which is unsafe, since there's no way to mark the resolver as requiring exactly one input, which means deferring to brittle validation logic)

We can just have:

doSomething(input: TypeA! | TypeB! | TypeC!)

(which is type safe even before hitting validation logic)

Again, I'm a little surprised a year and a half later, there's still a request for comments, when the comments appear to be perfectly exhaustive and already cover a great number of obvious scenarios with no useful workarounds except to write tedious blocks of extra resolver code.

Isn't it obvious that the lack of input unions causes verbosity, repeated code and demotes inputs as second-class versus outputs? What more does Facebook expect to see to promote this as a serious spec consideration? Who, how and when will such use-cases be tabled for discussion and a roadmap be given to either promote them to the spec or reject, so users like us that have gone all-in on GraphQL, don't spend another couple of years working around something that may trivially be implemented or not?

Genuine questions -- seems to have dragged on for ages for no good reason and I'm not quite sure what's left to discuss or what kind of use-cases we're realistically hoping to see that haven't already been covered?

[IvanGoncharov]

One important note is that use cases preferably should be extracted from real-life I think we already have a lot of them in different issues across graphql-spec repo.

Many of the examples I've seen are from real-life. Certainly for us, the use-case isn't speculative. Even those that are, are analogous to real-life scenarios. I don't think an organization should necessarily spend time/money/resources on coding up to the point of discovering this obvious flaw before it's taken seriously. The attributes that render this limiting are the same regardless!

@leebenson Idea behind this comment was that we shouldn't invent new examples and it's better to use ones that already reported in the graphql-spec repo. By real-life, I mean that developer/company NEEDS this feature for a particular case and is willing to formulate that. For example, your comment is a perfectly fine example of why you don't want to create a bunch of separate mutations.

Again, I'm a little surprised a year and a half later, there's still a request for comments, when the comments appear to be perfectly exhaustive and already cover a great number of obvious scenarios with no useful workarounds except to write tedious blocks of extra resolver code.

There is so much separate discussion on GitHub so it's hard to keep track of it this is the exact idea behind RFC document to have everything in one place. On GraphQL WG we discussed this proposal and alternative proposals multiple times (~4-5 in last 2 years) and everyone agrees that it should be addressed, but there was no consensus on what should be added into spec. So the only reasonable approach is to start working formal RFC document and as I noted before the next step is to extract all reported use cases from GitHub issues: https://github.com/graphql/graphql-spec/blob/master/rfcs/InputUnion.md#use-cases

[IvanGoncharov]

Closing since since WG reached consensus on "one of" proposal as a way forward, see graphql/graphql-spec#825