Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Start Input Union RFC document #584

Merged
merged 4 commits into from
Jun 20, 2019
Merged

Conversation

binaryseed
Copy link
Collaborator

@binaryseed binaryseed commented May 15, 2019

At the last WG meeting we discussed creating a RFC document to further the discussion around Input Union solutions.

This isn't complete by any means, but hopefully we can try to gather all the possible solutions in one place to enable better conversations.

Missing solutions:

  • Order-based structural discrimination
  • ??

Very open to feedback on how to handle this doc!

@binaryseed binaryseed mentioned this pull request May 15, 2019
@IvanGoncharov
Copy link
Member

IvanGoncharov commented May 15, 2019

@binaryseed Thanks for getting the ball rolling 👍
I have a few suggestions to make examples more realistic:

  1. From what I saw in a wild, majority of GraphQL APIs use Input suffix for input type names. Also in your example, we can assume that it should be some way to return submitted metrics so, in reality, it should be SummaryMetricInput, CounterMetricInput and HistogramMetric.
  2. Not sure if it applied to your particular example but in many cases you need separate input type for creating and updating mutations so input type names usually contain verbs like Create, Add, Update, etc.

Also, I suggest adding a more general example so I propose to use a modified example from
@tgriesser original post:

input AddPostInput {
  title: String!
  body: String!
}
input AddImageInput {
  photo: String!
  caption: String
}

inputUnion AddMediaBlockInput = AddPostInput | AddImageInput

input UpdatePostInput {
  title: String
  body: String
}
input UpdateImageInput {
  photo: String
  caption: String
}

inputUnion UpdateMediaBlockInput = UpdatePostInput | UpdateImageInput

type Mutation {
   addContent(content: [AddMediaBlockInput]!): Post
   updateContent(content: [UpdateMediaBlockInput]!): Post   
}

To evaluate DX of type names as discriminator proposal we need realistically looking type names. And even if in the initial version of your GraphQL API you manage to use nice looking names for input types like Post, Image, etc. it will become a problem later when you will need to add either input or output type with conflicting type.

@jbblanchet
Copy link

Looks good to me. "Single __typename field" and one of the "Single user-chosen field" could potentially be combined in a convention-over-configuration way, so __typename is the overridable default.

@jbblanchet
Copy link

Another possibility is to define the discriminator at the Union level, not at the type, something like:

inputUnion MetricInputUnion = SummaryMetric | CounterMetric | HistogramMetric on metricType

That would support some fancy-pants scenario where the same type could be used in two different unions with different discriminators.

@@ -0,0 +1,119 @@
RFC: GraphQL Input Union
----------

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we include an explanation of the use cases/requirements/anti-requirements for input unions? They're currently scattered across lots of Github issue/PR comments which makes it hard to ensure we're all trying to solve the same problem.

RFC: GraphQL Input Union
----------

## Possible Solutions
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The "problems" section for each solution is great. Would it make sense to also add a pros/cons section for each possible solution?


These options rely on the _structure_ of the input to determine the concrete type.

#### Unique structure
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This solution only considers the set of required fields. There's a further generalization that also considers the type of each field and can discriminate fields that have the same name but non-overlapping value domains. For example you could have the following if you wanted to define a filtering API:

input StringFilter {
  propertyID: ID!
  value: String!
}

input IntegerFilter {
  propertyID: ID!
  value: Integer!
}

input union Filter = StringFilter | IntegerFilter

This could probably also be considered a further generalization of the "Single user-chosen field; value is a literal" listed earlier.

Note: I think this approach can get complicated with coercion and nested types so I'm not advocating for it but if we're trying to catalog possible solutions, it's probably worth adding to the list.

@benjie
Copy link
Member

benjie commented May 20, 2019

Should the @oneField directive alternative be included in this? It's not strictly an input union, but can be used to solve the same problem with minimal additional complexity.

@mwilliammyers
Copy link

mwilliammyers commented Jun 10, 2019

I might have missed something in #395, but does this (or #586) support unions comprising of scalars, collections and input objects?

I understand that this is not supported by output unions but maybe it is worth while to discuss adding it to input unions while we are at it?

e.g.:

input FooInput {
  foo: Int
}

input BarInput {
  bar: Int
}

input union SomeInput = Int | [Int!] | Float | String | FooInput | BarInput

and

input union SomeInput = Int | Float | String | Boolean | ID

(as well as just unions of input objects)

I believe #586 supports this, but does this RFC address this?

@benjie
Copy link
Member

benjie commented Jun 13, 2019

#586 would indeed support this:

input FooInput {
  foo: Int
}

input BarInput {
  bar: Int
}

input SomeInput @oneField {
  int: Int
  ints: [Int!]
  float: Float
  string: String
  foo: FooInput
  bar: BarInput
}

and

input SomeInput2 @oneField {
  int: Int
  float: Float
  string: String
  boolean: Boolean
  id: ID
}

If you have a concrete need for this (rather than an abstract one), please submit your example and we can add it to the list of use-cases 👍

@IvanGoncharov
Copy link
Member

@binaryseed Thanks for volunteering to work on this document 👍
We actually discussed it on the last WG (in the context of @benjie oneField proposal) please see notes here: https://github.com/graphql/graphql-wg/blob/master/notes/2019-06-06.md#onefield-directive-input-unions--benjie-gillam-benjie
So here are a few key points from that discussion:

  • Document in its current form is the good starting point and is ready to be merged. So just ping me when you ready and I will merge it.
  • We need to add a description for a set of problems that proposed solutions are trying to address.
  • We should focus on this advancing this RFC instead of individual proposals.

@binaryseed
Copy link
Collaborator Author

I'm happy to get this merged anytime and let the community PR the doc to keep it moving forward

@jsshapiro
Copy link

Apologies for my ignorance, but where is the appropriate place to offer input to the working group? Having done a fair bit of PL design work in my career, it seems possible that I may be able to help converge the options. They are perhaps more similar than the current discussions appear to recognize.

Thank you.

@benjie
Copy link
Member

benjie commented Apr 14, 2020

A pull request against the InputUnion RFC document is the preferred mode of discussion currently.

@benjie
Copy link
Member

benjie commented Apr 14, 2020

Oh, also the #wg channel on the GraphQL Slack, though TBH it's very low traffic and a PR against the InputUnion RFC is more likely to gain engagement.

@binaryseed
Copy link
Collaborator Author

@jsshapiro I'm extremely interested. I've tried doing a little cursory research into this, and would love to hear your thoughts..

A PR to the RFC even with minimal changes (perhaps additional links in the "prior-art") would be a nice way to start a conversation

https://github.com/graphql/graphql-spec/blob/master/rfcs/InputUnion.md#-prior-art

@jsshapiro
Copy link

jsshapiro commented Apr 14, 2020 via email

@binaryseed
Copy link
Collaborator Author

If you get a PR started, we can have a broader conversation there!

@Nik-Novak
Copy link

I've been following this for over a year now and can not believe this still doesn't exist. It seems like a fundamental feature.

@SRachamim
Copy link

Still no support of input union. This is an elementary feature, people.

@b-jsshapiro
Copy link

b-jsshapiro commented Oct 29, 2020

One thought arises about the __inputName approach: perhaps there is no need for a new reserved identifier. When __typeName is present, it appears to me that its value will overlap exactly with __inputName, and there is already a convention established for the meaning of __typeName. Would it suffice simply to say that __typeName becomes mandatory in a union (in either direction)?

Can someone explain the difference in intended meaning between the two fields? Am I missing some issue in the client-side handling or the server-side resolver processing that motivates a new reserved field name for this?

Thanks!

@b-jsshapiro
Copy link

b-jsshapiro commented Oct 30, 2020

My other thought concerns the tagged union proposal. In my view, it is better if input and output unions are symmetric. For better or worse, output unions are untagged.

If untagged input unions are Available, then it seems to me that tagged unions can be implemented using existing features of the GraphQL specification:

union U = A|B|C
enum UTag = { A B C }

input TaggedUnion {
  UTag myTagName;
  U value;
}

the only thing missing here is an enforced correlation between the tag and the value type, but this is equally true of output unions, and it has not created undue issues for C and C++ programmers. The idiom can be used in both directions, which seems like a good thing.

If we had the luxury of starting over, I think I might have advocated for tagged output unions from the beginning. But that ship has sailed. The question before us now is whether we will introduce an asymmetry between input and output types.

While I really do think that symmetry should be a significant criteria here, I should probably acknowledge an opinion that may be biasing my view: I feel that the entire idea that input and output types should be distinct is one of the very few truly boneheaded ideas in GraphQL, and over the long haul I'd really like to see that distinction go away. I understand the desire to imagine that input and output are different, but if you want two types for that, go ahead and declare a second type! That isn't an input/output distinction. It's a usage distinction! The current split "bakes in" a restriction that graphs cannot be round-tripped. Which in turn means that GraphQL cannot describe message queues interfaces. Which means GraphQL isn't very useful for describing (e.g.) microservice interactions.

Types are mathematical things; they have nothing to do with directionality. The present division yields an explosion of types to no obvious purpose in the majority of use cases. It does so in the service of enforcing an opinionated misconception about the semantics and mathematics of types, and does so while removing expressiveness rather than increasing expressiveness.

Sorry about the rant. Tempted to file a bug report as a place for discussion. For the purposes of this discussion, all I'm trying to say is: let's not make the divergence worse than it already is.

@b-jsshapiro
Copy link

b-jsshapiro commented Oct 30, 2020

Finally, a small comment about syntax. If we are adopting keywords here, I think it would be better to use "input union" rather than "inputunion". Note that we don't actually need a new keyword here at all. It would work equally well to use the existing union keyword and require that that all constituents of a union definition must have the same directionality. The directionality of the union follows from the directionality of its constituents.

@graphql graphql locked and limited conversation to collaborators Oct 30, 2020
@leebyron leebyron added the 📣 RFC document PR creates or changes document inside "rfc" folder label Apr 9, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
📣 RFC document PR creates or changes document inside "rfc" folder
Projects
None yet
Development

Successfully merging this pull request may close these issues.