graphql-spec icon indicating copy to clipboard operation
graphql-spec copied to clipboard

RFC: OneOf Input Objects

Open benjie opened this issue 4 years ago • 151 comments

Follow up of ~~the @oneField directive~~ and ~~the Tagged type~~.

Introducing: OneOf Input Objects.

OneOf Input Objects are a special variant of Input Objects where the type system asserts that exactly one of the fields must be set and non-null, all others being omitted. This is represented in introspection with the __Type.isOneOf: Boolean field, and in SDL via the @oneOf directive on the input object.

This variant of an input object introduces a form of input polymorphism to GraphQL.

Example 1 - addPet

The following PetInput oneof input object lets you choose between a number of potential input types:

input PetInput @oneOf {
  cat: CatInput
  dog: DogInput
  fish: FishInput
}

input CatInput { name: String!, numberOfLives: Int }
input DogInput { name: String!, wagsTail: Boolean }
input FishInput { name: String!, bodyLengthInMm: Int }

type Mutation {
  addPet(pet: PetInput!): Pet
}

Example 2 - user(by:)

Previously you may have had a situation where you had multiple ways to locate a user:

type Query {
  user(id: ID!): User
  userByEmail(email: String!): User
  userByUsername(username: String!): User
  userByRegistrationNumber(registrationNumber: Int!): User
}

with OneOf Input Objects you can now express this via a single field without loss of type safety:

input UserBy @oneOf {
  id: ID
  email: String
  username: String
  registrationNumber: Int
}
type Query {
  user(by: UserBy!): User
}

FAQ

Why is this a directive?

At its core, it's a property of the type that's exposed through introspection - much in the same way that deprecation is. There's nothing in introspection, nor in the types exposed through the reference implementation (new GraphQLInputObjectType({ name: "...", isOneOf: true, ... })) that relates to directives. It just happens to be that after I analysed a number of potential syntaxes (including keywords and alternative syntax) I've found that when representing the schema as SDL, using a directive to do so is the least invasive (all current GraphQL parsers can already parse it!) and none of the alternative syntaxes sufficiently justified the increased complexity they would introduce.

Why is this a good approach?

This approach, as a small change to existing types, is the easiest to adopt of any of the solutions we came up with to the InputUnion problem. It's also more powerful in that it allows additional types to be part of the "input union" - in fact any valid input type is allowed: input objects, scalars, enums, and lists of the same. Further it can be used on top of existing GraphQL tooling, so it can be adopted much sooner. Finally it's very explicit, so doesn't suffer the issues that "duck typed" input unions could face.

Why did you go full circle via the tagged type?

When the @oneField directive was proposed some members of the community felt that augmenting the behaviour of existing types might not be the best approach, so the Tagged type was born. (We also researched a lot of other approaches too.) However, the Tagged type brought with it a lot of complexity and controversy, and the Input Unions Working Group decided that we should revisit the simpler approach again. This time around I'm a lot better versed in writing spec edits :grin:

Why are all the fields nullable? Shouldn't they be non-nullable?

To make this change minimally invasive I wanted:

  • to make it so that existing GraphQL clients could still validate queries against a oneOf-enabled GraphQL schema (if the fields were non-nullable the clients would think the query was invalid because it didn't supply enough data)
  • to allow existing GraphQL implementations to change as little code as possible

To accomplish this, we add the "exactly one value, and that value is non-null" as a validation rule that runs after all the existing validation rules - it's an additive change.

Can this allow a field to accept both a scalar and an object?

Yes!

type Query {
  findUser(by: FindUserBy!): User
}

input FindUserBy @oneOf {
  id: ID
  organizationAndRegistrationNumber: OrganizationAndRegistrationNumberInput
}

input OrganizationAndRegistrationNumberInput {
  organizationId: ID!
  registrationNumber: Int!
}

Can I use existing GraphQL clients to issue requests to OneOf-enabled schemas?

Yes - so long as you stick to the rules of one field / one argument manually - note that GraphQL already differentiates between a field not being supplied and a field being supplied with the value null.

Without explicit client support you may lose a little type safety, but all major GraphQL clients can already speak this language. Given this nonsense schema:

type Query {
  foo(by: FooBy!): String
}
input FooBy @oneOf {
  id: ID
  str1: String
  str2: String
}

the following are valid queries that you could issue from existing GraphQL clients:

  • {foo(by:{id: "..."})}
  • {foo(by:{str1: "..."})}
  • {foo(by:{str2: "..."})}
  • query Foo($by: FooBy!) {foo(by: $by)}

If my input object has only one field, should I use @oneOf?

Doing so would preserve your option value - making a OneOf Input Object into a regular Input Object is a non-breaking change (the reverse is a breaking change). In the case of having one field on your type changing it from oneOf (and nullable) to regular and non-null is a non-breaking change (the reverse is also true in this degenerate case). The two Example types below are effectively equivalent - both require that value is supplied with a non-null int:

input Example @oneOf {
  value: Int
}

input Example {
  value: Int!
}

Can we expand @oneOf to output types to allow for unions of objects, interfaces, scalars, enums and lists; potentially replacing the union type?

:shushing_face: :eyes: :wink:

benjie avatar Feb 19 '21 16:02 benjie

Is @oneOf missing in the example of section Can this allow a field to accept both a scalar and an object?

wyfo avatar Feb 19 '21 18:02 wyfo

I’d worry that statements around type safety are a little hard to apply in practice.

It’s not the case typically that a directive would change a types underlying type yet @oneOf seems to imply that “when the server gets this, it expect one of these to be non-null”. Off the bat, a standard GraphQL to Typescript conversion could do something like

type PetInput = { cat?: CatInput; dog?: DogInput; fish?: FishInput; }

When instead I’d expect it to do something like:

type PetInput = { cat: CatInput; } | { dog: DogInput; } | { fish: FishInput; };

I totally understand the motivation around the change to make it as low impact as possible, but I'd worry about the adverse side affects introduced by this subtle change to the ways that the null/non-null properties are determined.

Maybe I’m just applying my understanding incorrectly, but I’d hope that any adoption doesn’t in fact mutate the type system of GraphQL using directives like this.

wyattjoh avatar Feb 19 '21 23:02 wyattjoh

@wyfo Thanks, fixed!

@wyattjoh It’s not a directive, it’s a new type system constraint that DOES model the type of the input differently and would have different types generated. Have a look at the alternative syntaxes document for other ways this could be exposed via SDL and let us know your preference, perhaps you would prefer the oneof keyword to make it clearer (in SDL only, this would not affect introspection) the change in behaviour.

benjie avatar Feb 20 '21 09:02 benjie

It looks like an existing syntax, but the semantics are different? I am worried that if it will end up asking for dirty exception handling for every directive code path.

Have a look at the alternative syntaxes document for other ways this could be exposed via SDL and let us know your preference

Could we consider a new syntax that hasn't been mentioned?

type  Query {
   user(id: ID!): User
   user(email: String!): User
   user(username: String!): User 
   user(registrationNumber: Int!): User
}

pros?:

  • it might be easy to apply because it just releases the existing constraints (that field names cannot duplicate on SDL)
  • it makes the schema can look intuitive for the possible input type.

cons:

  • Conversely, it makes it look like a variant is possible for the output.

cometkim avatar Feb 22 '21 15:02 cometkim

@cometkim Can you show how that syntax would be expanded to input objects too, please? And yes we can absolutely consider alternative syntaxes.

benjie avatar Feb 22 '21 23:02 benjie

It’s not a directive

Why should it be something else than a directive?

Actually, it's already (almost) possible to implement @oneOf as a directive in a few lines of code. I've made a Gist to show a possible implementation using Python and graphql-core (quite the reference translation of graphql-js in Python). In fact, if the field directive is trivial, the input type directive requires in my example a graphql-core specific feature. However, the proposal of input object validation (still opened) could bring the material needed to implement it with graphql-js.

By the way, GraphQL schema is kind of poor in validation stuff (compared to JSON schema for example), so part of the validation is already done by the resolvers/scalar parsing methods. In a schema-first approach, you can also defines directives for repetitive checks, maybe with JSON schema-like annotations, but your code/library will have to translate and inject them into your resolvers/scalar types(/input types when the mentioned proposal will pass). IMO, @oneOf should not be different as a directive, it could just be a validation marker used to add validation code in the resolvers/input type; no need of the type system validation. Also, in a code-first approach (no directives), it's already possible to support tagged unions, I do it in my own library; there is no need of the SDL.

In fact, I don't really see the interest of making @oneOf something else than a validation directive. And I'm wondering then if a validation directive would be appropriate in the GraphQL specification … Maybe it could be a kind of convention for schema-first libraries. Yet, having it in the specifications could help tooling (linters, code generators) and lighten GraphQL libraries. Anyway, night thoughts.

wyfo avatar Feb 23 '21 01:02 wyfo

Can we expand @oneOf to output types to allow for unions of objects, interfaces, scalars, enums and lists; potentially replacing the union type?

For input types @oneOf implies one more nesting level. What do you think @oneOf will look like for unions?

sungam3r avatar Feb 26 '21 08:02 sungam3r

It’s not a directive, it’s a new type system constraint that DOES model...

@benjie I don't understand. You wrote about @oneOf as a directive in the spec and at the same type talk here that it's not a directive... 😕

sungam3r avatar Feb 26 '21 09:02 sungam3r

For input types @oneOf implies one more nesting level. What do you think @oneOf will look like for unions?

Another nesting level; i.e. instead of querying like:

{
  allEntities {
    ... on User { username }
    ... on Pet { name }
    ... on Car { registrationNumber }
    ... on Building { numberOfFloors }
  }
}

it'd look like:

{
  allEntities {
    user { username }
    pet { name }
    car { registrationNumber }
    building { numberOfFloors }
  }
}

benjie avatar Feb 26 '21 09:02 benjie

@benjie I don't understand. You wrote about @oneOf as a directive in the spec and at the same type talk here that it's not a directive... confused

The input union working group have not decided what syntax to use for oneOf yet. It might end up as being presented as a directive, or it might be a keyword or any other combination of things. Check out this document for alternatives: https://gist.github.com/benjie/5e7324c64f42dd818b9c3ac2a91b6b12 and note that whichever alternative you pick only affects the IDL, it does not affect the functionality or appearance of GraphQL operations, validation, execution, etc. Please see the FAQ above.

TL;DR: do not judge the functionality of this RFC by its current IDL syntax. We can change the IDL syntax.

benjie avatar Feb 26 '21 09:02 benjie

It might end up as being presented as a directive

OK. In my opinion if something is presented as a directive than ... it is just a directive.

sungam3r avatar Feb 26 '21 09:02 sungam3r

Thanks for the review @sungam3r; good to have additional scrutiny! I don't think any modifications to the RFC are required to address your concerns (other than perhaps writing an alternative IDL syntax, but I don't plan to invest time in that until there's general concensus on what the syntax should be, for now the directive syntax can act as a placeholder). I think all the conversations in your review can be closed except for the oneArg suggestion; that one might require some more bike-shedding :wink:

benjie avatar Feb 26 '21 10:02 benjie

Another alternative syntax (probably not great, but I haven't seen it yet so worth thinking about): field<arg1: Int, arg2: Int> for fields, input < field1: Int, field2: Int > for types, i.e. using < (or another symbol) instead of ( to indicate one-of-ness.

eapache avatar Mar 04 '21 19:03 eapache

@benjie In my previous comment, I've brought the fact that oneof semantic could just be implemented as a directive by graphql libraries, without requiring the update of specifications.

Could you explain why do you think we would need a special construct compared to the directive approach ?

wyfo avatar Mar 04 '21 19:03 wyfo

I think I've actually missed the coercion rules, so I think the RFC as stated only actually validates literals in the document :man_facepalming: I'll try and get a fix for that out tomorrow, shouldn't be much text.

benjie avatar Mar 04 '21 19:03 benjie

@wyfo

Could you explain why do you think we would need a special construct compared to the directive approach ?

Please see: https://github.com/graphql/graphql-spec/blob/main/rfcs/InputUnion.md

We think that this requirement for input polymorphism is sufficiently universal that there's a need to standardise it so that all implementations and clients can understand it within the GraphQL ecosystem.

benjie avatar Mar 04 '21 20:03 benjie

In the working group, Lee suggested that when writing Oneof Input Object literals we should always only allow one field to be present (whether or not variables were involved). I did not want to do this because it makes oneofs more distinct from regular input objects, and it's frustrating that a user cannot do something like:

type Mutation {
  addPet(pet: PetInput): Pet
}

input CatInput { name: String!, nickname: String, meowVolume: Int }
input DogInput { name: String!, nickname: String, barkVolume: Int }

input PetInput @oneOf {
  cat: CatInput
  dog: DogInput
}

#----

mutation AddPet ($cat: CatInput, $dog: DogInput) {
  addPet(pet: {cat: $cat, dog: $dog}) { name }
}

This feels like a natural thing to do.


Until... (and this is where I've come around to Lee's suggestion) until you want to apply strong typing to the variables on the client side. Now you have to analyse every usage of the variables in the client in order to determine that the type of the variables is something like (TypeScript syntax): { cat: CatInput! } | { dog: DogInput! }. It's technically feasible, but it's a tonne of effort for the client, when previously you could just look at the variable definitions and you were done.

For that reason, I think I'm going to adopt Lee's suggested approach. One additional benefit of Lee's approach is it's easier to implement the validation rules for it. Further we can open it up in future should we wish without breaking any previously valid queries - so it's a decision that we can revisit later.

benjie avatar Mar 06 '21 12:03 benjie

All changes made; ready for re-review.

benjie avatar Mar 06 '21 13:03 benjie

I gave this another read and while I haven't gone over it in painstaking detail yet, I'm happy with the shape of what's here. @rmosolgo this is one to pay attention to for future versions of graphql-ruby.

eapache avatar Mar 09 '21 14:03 eapache

Having reviewed this in depth I'm finding that the weird hybrid approach (using a directive for the syntax but calling it a fundamental type difference internally) gets very confusing and feels inconsistent. I would prefer either:

  • Going to back to "it's just a directive" all the way through. There were some concerns that directives shouldn't modify type constraints like this, but I don't know why not? There are no specified limits on what directives are allowed to impact, and "this directive adds an extra validation rule" seems legit to me.
  • Switch to a non-directive syntax (plausibly a new keyword, since this is officially a new type).

This is a common refrain, but the GraphQL spec doesn't seem to think in terms of directives like that. For example, when you deprecate a field, the non-IDL portions of the GraphQL spec don't refer to that field having had the @deprecated directive applied to it - instead the field is simply a deprecated field. This is the same for Oneof Fields - it's not a field that has the @oneOf directive applied to it, it's simply a Oneof Field. If your GraphQL implementation does not implement the GraphQL IDL then there is literally no reason to think that a Oneof Field is affected by a "directive" at all, in the same way that deprecated fields in an IDL-less implementation also do not relate to directives. In both cases it's purely a "property" of the field: is it deprecated? Is it oneof?

Further, in introspection, both isDeprecated and oneOf (maybe this should be isOneOf?) are represented with regular fields on the __Field type. There is no mention of directives about them, that syntax is purely an artifact of the GraphQL IDL and should be limited to that part of the specification.

benjie avatar Apr 08 '21 11:04 benjie

Thanks for the thorough review @eapache; the majority of your concerns have been addressed (either via explanation or via spec edits); I've left a couple open that warrant more input from others.

benjie avatar Apr 08 '21 11:04 benjie

the non-IDL portions of the GraphQL spec don't refer to that field having had the @deprecated directive applied to it - instead the field is simply a deprecated field. This is the same for Oneof Fields - it's not a field that has the @oneOf directive applied to it, it's simply a Oneof Field.

That's sort of the issue though from my perspective. The output types already have a system to resolve these types of types, union types. In reality, the concept of OneOf really is a different form of a union output type. This keyword provides clear expectations that the output type could be "one of" the types provided in the union.

This solution has also been touted as a feature that is backwards compatible with existing generated tooling. The issue with this is that it is not. No other directive-style solutions (including the @deprecated directive) modifies the underlying type or shape that is utilized by generated tooling. The @deprecated directive simply provides an annotation that the field has been deprecated.

This essentially affects the correctness of the type system. When using this directive style, it will not enforce that exactly one of these parameters is passed. Contrast this to the entire ecosystem that's built around the GraphQL schema, that relies on the correctness. They will all require modifications to support this change to the type system.

Protocol buffers support the oneof feature, but it's using a keyword at the same level as an enum.

While it could be expected that the server would be the one to enforce this validation, it seems like a pretty clear breakage from a type point of view. It would be a clearer signal I would think with solutions such as the tagged type that changes the syntax for servers that adopt this optional feature.

wyattjoh avatar Apr 08 '21 15:04 wyattjoh

@wyattjoh I believe you're agreeing that there's nothing specifically "directive-like" to this RFC and are requesting an alternative syntax for oneof fields and oneof input objects when exposed via SDL. I've outlined a few potential syntaxes here; would you care to weigh in on which is your preference?

https://gist.github.com/benjie/5e7324c64f42dd818b9c3ac2a91b6b12

This solution has also been touted as a feature that is backwards compatible with existing generated tooling.

It's still an extension in functionality, so all tooling will need to be updated to correctly represent the new type constraints (of course we don't get this feature for free!). However, existing tooling should not break and doesn't need to be updated wholesale in order to support a schema with these features, the updates can happen piecemeal - it may not generate the correct validation errors within the tool, but valid queries can be written in and executed by legacy tooling (e.g. a version of GraphiQL that doesn't support oneof) - it does not break them. This is a minor concern though.

This essentially affects the correctness of the type system. When using this directive style, it will not enforce that exactly one of these parameters is passed.

Tooling that supports oneof will support the type safety therein, independent of the syntax that is used to represent oneof.

It would be a clearer signal I would think with solutions such as the tagged type that changes the syntax for servers that adopt this optional feature.

Many of the servers that you interact with don't expose GraphQL IDL. Take the GitHub GraphQL API for example, if the GitHub GraphQL API were to adopt oneof, the queries you write and the introspection results you fetch would be identical independent of the oneof syntax choice.

benjie avatar Apr 08 '21 15:04 benjie

I've outlined a few potential syntaxes here; would you care to weigh in on which is your preference?

The prefix keyword seems much more direct when it comes to changes to the shape. I would point out that your point that the prefix keyword:

feels like it applies to the type/field rather than its fields/arguments

Is the same when it comes to the directive. When I've seen directives like this in the past:

type Query {
  user(id: ID, username: String): User @oneOf
}

I would expect that "directive" would apply to the output type, not the arguments. So the sort of "weird" syntax feels of the prefix keyword:

oneof input PetInput {
  cat: CatInput
  dog: DogInput
  fish: FishInput
}

oneof type Pet {
  cat: Cat
  dog: Dog
  fish: Fish
}

type Query {
  oneof user(id: ID, username: String): User
  # or alternatively
  user oneof (id: ID, username: String): User
}

Seems a bit more tolerable as it's very explicit at where it applies to (the arguments).

(crazy ideas follow...)

The real only other alternative to this that would avoid this for arguments would cause more breaking changes:

oneof input PetInput {
  cat: CatInput
  dog: DogInput
  fish: FishInput
}

oneof type Pet {
  cat: Cat
  dog: Dog
  fish: Fish
}

type Query {
  # Finds a user by their ID.
  user(id: ID!): User

  # Finds a user by their username.
  user(username: String!): User
}

Which I think (from a schema perspective) reads the easiest, and gives the opportunity to document things in a better way. I would expect that a constraint of this would be that "polymorphic fields" must have the same return type (ie, you can't have user(id: ID!): User and user(username: String!): Pet). This might be similar to how typescript provides function overloading.

wyattjoh avatar Apr 08 '21 16:04 wyattjoh

Hmm.... this "argument overloading" is a very interesting idea, I think it might be worth analyzing it's implications a bit more - it doesn't look like any of the other proposed solutions in the Input Union RFC, but I'm not sure if it provides a new way solve the discrimination problem that most solutions suffer from

binaryseed avatar Apr 09 '21 17:04 binaryseed

I think that new kind of type inputUnion Filter = A | B | C will be more natural.

IMHO Dirictive @oneOf looks like a hack. Of course it has advantage that it's almost compatible with current tools. But we already live through the new commens format """.

STOP! But what if we want to deprecate one of the field?

InputUnion looks awkward:

inputUnion Filter = A | B @deprecated(reason: "") | C

And @oneOf in this area looks much better:

input PetInput @oneOf {
  cat: CatInput
  dog: DogInput @deprecated(reason: "")
  fish: FishInput
}

And also inputUnion is suffered by Duck typing issue.

So maybe @oneOf directive is the best solution 👍

And if @oneOf became available for output object types it will start to look more natural. Especially it simplifies query writing for "union" fields without writing fragments and simplifies type indetification on the client side at runtime.

@benjie the @oneOf directive looks very good especially if it became available for input & output types simultaneously 🚀

nodkz avatar May 28 '21 18:05 nodkz

Would it make sense to let @oneOf also to be applied to fields directly, to indicate that only one of those fields needs to be selected.

For example, the following is pretty close to the existing solution to the "union input" problem:

input AnimalDropOffInput {
  type: AnimalType!
  name: String!
  age: Int!
  dog: DogDetails @oneOf
  cat: CatDetails @oneOf
  snake: snakeDetails @oneOf
}

I guess something like this could be also handy for update mutations, where people sometimes prefer to have the ID not in the input:

addPet(cat: CatInput @oneOf, dog: DogInput @oneOf): Pet
updatePet(id: ID, cat: CatInput @oneOf, dog: DogInput @oneOf): Pet

Of course, the same can be achieved by introducing additional oneOf types.


I echo the feedback above that in

user(id: ID, username: String): User @oneOf

one would expect the @oneOf to apply to the return type instead of the arguments.


Otherwise, I really like the proposal! Nice work.

tobiasdiez avatar Aug 12 '21 17:08 tobiasdiez

At their talk "Automated GraphQL Schema Generation from ER-based Service Modeling" at GraphQL Conf yesterday, LinkedIn's Min Chen revealed how GraphQL union type was not sufficient to map their existing types, so they have converted it to regular object types where all fields are nullable and one is expected to be set - which is exactly how we would map Oneof to output types. They were mixing object types and scalars into their union - one of the big features that Oneof would unlock for GraphQL output polymorphism. It was interesting to hear this GraphQL limitation and see this solution serindipitously from a large vendor - I wonder how we can discover others in the ecosystem handling this same problem?

benjie avatar Sep 30 '21 08:09 benjie

Syntax-wise, I'm leaning towards the "keyword before parenthesis" variant:

# Input object type
input PetInput oneof {
  """
  A cat
  """
  cat: CatInput

  """
  A dog
  """
  dog: DogInput

  fish: FishInput
}
# Object type
type Pet oneof {
  """
  A cat
  """
  cat: Cat

  """
  A dog
  """
  dog: Dog

  fish: Fish
}
type Query {
  # For arguments
  user oneof (
    """
    By their globally unique identifier
    """
    byId: ID

    """
    By their unique username
    """
    byUserName: String

    byEmail: String
  ): User
}

The main difference from the directive syntax (other than it not being a directive and thus not opening that whole can of worms around people misunderstanding what a directive is) is that we can put the oneof keyword before the parenthesis for the field, which makes its intent more clear than a directive at the end of the line. It's also more readable: "user field accepts oneof the following args and returns a User" versus "user field accepts the following args and returns a User and, oh, by the way, we expect exactly one of those arguments to be set"

benjie avatar Sep 30 '21 09:09 benjie

Alternate proposal discussed in Oct WG meeting for posterity:

One of remains a directive, for arguments that require one of, use an input object:

# Input object type
input PetInput @oneof {
  """
  A cat
  """
  cat: CatInput

  """
  A dog
  """
  dog: DogInput

  fish: FishInput
}

type Query {
  # For arguments
  user(by: UserBy): User
}

input UserBy @oneof {
  """
  By their globally unique identifier
  """
  id: ID

  """
  By their unique username
  """
  userName: String

  email: String
}

leebyron avatar Oct 07 '21 18:10 leebyron