-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
RFC: OneOf Input Objects #825
base: main
Are you sure you want to change the base?
Conversation
Is |
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 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. |
@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. |
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.
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?:
cons:
|
@cometkim Can you show how that syntax would be expanded to input objects too, please? And yes we can absolutely consider alternative syntaxes. |
Why should it be something else than a directive? Actually, it's already (almost) possible to implement 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). In fact, I don't really see the interest of making |
For input types |
spec/Section 4 -- Introspection.md
Outdated
@@ -156,6 +159,7 @@ type __Field { | |||
type: __Type! | |||
isDeprecated: Boolean! | |||
deprecationReason: String | |||
oneArgument: Boolean! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or oneArg
to inline with args
?
spec/Section 5 -- Validation.md
Outdated
* {arguments} must contain exactly one entry. | ||
* For the sole {argument} in {arguments}: | ||
* Let {value} be the value of {argument}. | ||
* {value} must not be the {null} literal. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the word literal appropriate here in case of using variables? The same question about Oneof for Input Object.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe so; I've modeled it on the language already used in this section, namely: https://spec.graphql.org/draft/#sel-LALTHHDHFFFJDAAACDJ-3S
@benjie I don't understand. You wrote about |
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 }
}
} |
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. |
OK. In my opinion if something is presented as a directive than ... it is just a directive. |
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 |
spec/Section 3 -- Type System.md
Outdated
`$var` | `{ var: { a: "abc" } }` | `{ a: "abc" }` | ||
`{ a: "abc", b: null }` | `{}` | Error: Exactly one key must be specified | ||
`{ b: $var }` | `{ var: null }` | Error: Value for member field {b} must be non-null | ||
`{ b: 123, c: "xyz" }` | `{}` | Error: Exactly one key must be specified |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing { a: $varA, b: $varB }
with various combinations of values for varA and varB.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My in meeting proposal was that this case could just be invalid at start.
This L1441 in Validation file in this PR sounds like it would do just that:
https://github.com/graphql/graphql-spec/pull/825/files#diff-607ee7e6b71821eecadde7d92451b978e8a75e23d596150950799dc5f8afa43eR1441
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are exactly the same as for input objects (which also don't specify what happens if you have multiple variables); but I'll add some for clarity.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@leebyron Good catch; that was not my intent. I have updated the PR with better validation and more examples.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've since revisited my thoughts on this and for the sake of defining types of variables on the client I've adopted the suggestion: #825 (comment)
I think the example you give indicates the problem: you've tried to apply The GraphQL grammar does not afford applying directives to multiple argument types (see TypeSystemDirectiveLocation, the closest you can get is |
I was very unsure about the allowed locations of directive, so I looked it up in the spec (draft version): a directive is allowed on a FIELD_DEFINITION, i.e. a simplified version of example No 91: directive @example on FIELD_DEFINITION
type SomeType {
field(arg: Int): String @example
} |
I don't think it would be "such a bad idea", but it does open a can of worms. For example, consider this (partial) schema: type Query {
userById(id: ID!): User
userByUsername(username: String!): User
userByRegistrationNumber(registrationNumber: String!): User
} In the current proposal you could collapse this to something like: input UserFinder @oneOf {
id: ID
username: String
registrationNumber: String
}
type Query {
user(by: UserFinder!): User
} What you're proposing is to condense this further: type Query {
user(
id: ID
username: String
registrationNumber: String
): User @oneOf
} At first blush, this looks like a great win, right? But one issue is that it severely inhibits schema evolution. Let's imagine that you now want the ability to see what a user looks like when viewed as another user; our original schema would look like: type Query {
userById(id: ID!, asUser: ID): User
userByUsername(username: String!, asUser: ID): User
userByRegistrationNumber(registrationNumber: String!, asUser: ID): User
} The current proposal allows a similar change: input UserFinder @oneOf {
id: ID
username: String
registrationNumber: String
}
type Query {
- user(by: UserFinder!): User
+ user(by: UserFinder!, asUser: ID): User
} But your proposed type Query {
user(
id: ID
username: String
registrationNumber: String
): User @oneOf
+ userAsUser(by: UserFinder!, asUser: ID): User
}
+input UserFinder @oneOf {
+ id: ID
+ username: String
+ registrationNumber: String
+} And the structure of the new field would likely be something like So by preventing We did discuss applying Further, one of the GraphQL guiding principles is:
So again, since it's already achievable with input objects there doesn't seem to be a pressing need to extend it to arguments. I hope this helps you understand why the decision was made to not apply |
@benjie: yes, that explains it very well. I'm not sure I fully agree, but now I understand and fully accept your reasoning. Thank you! |
Hey guys, sorry for bothering, but is there something still blocking this PR? Are there any chances this is finally going to be merged any time soon? |
I think we're mostly just awaiting feedback from users. So far I've not really heard any complaints so I'm hopeful it will be accepted. If there's sufficient positive feedback behind it then I need to update it against the latest version of the spec, make sure it's in the latest release of GraphQL.js, and then bring it to a GraphQL WG for approval. |
graphql-java has implemented and released oneOf support in its latest version and its being used successfully at a number of companies including Atlassian where I work. I say this to leave positive feedback on this proposal. |
I've also been following this for a long time and want to express that I'm very happy with where this landed. We'd use it internally at my company. |
More positive feedback: We are using it at HubSpot through the |
Likewise, some more positive feedback from a library dev perspective: I implemented support for OneOf input objects for the Caliban GraphQL library (Scala): ghostdogpr/caliban#1846. It hasn't been merged in yet as we're waiting for this RFC to become part of the spec first, but overall it's been fairly straightforward implementing support for it and I couldn't really identify any areas that could have been improved. Great job overall! 👍 |
We also added support for oneOf in Strawberry GraphQL recently, hopefully we'll get some feedback there too 😊 https://github.com/strawberry-graphql/strawberry/releases/tag/0.230.0 |
We have been eagerly waiting at Jobber for this to land as well. We use |
Seconding what @bbakerman said, colleagues at work (Atlassian) have been really happy with I also really like the migration pathway, it's simple and easy to safely adopt. I can't suggest anything to improve it. @benjie anything else you need from us to get this officially in the spec? |
- If the input object literal or unordered map does not contain exactly one | ||
entry an error must be thrown. | ||
|
||
- Within the input object literal or unordered map, if the single entry is | ||
{null} an error must be thrown. | ||
|
||
- If the coerced unordered map does not contain exactly one entry an error must | ||
be thrown. | ||
|
||
- If the value of the single entry in the coerced unordered map is {null} an | ||
error must be thrown. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the difference between the first two conditions and the second two conditions?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The first two apply before coercion, the second two apply after coercion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See #825 (comment) discussion there still seems somewhat open, pending input from @leebyron
Co-authored-by: Shane Krueger <shane@acdmail.com>
Thanks for the input everyone! I'll try and find some time to backport the implementation in GraphQL.js into GraphQL v16 and get a release out; then bring it to a WG to get it advanced! |
Question: Do variables mapped to OneOf Input Objects' fields need to be typed as non-null? Example schema: input OneOfInput @oneOf {
item1: String
item2: String
}
type Query {
test(arg: OneOfInput!): String
} Is this valid? (assuming that the value of query ($item1var: String) {
test(arg: { item1: $item1var })
} Or only this? query ($item1var: String!) {
test(arg: { item1: $item1var })
} Since no change has been made to the 'All Variable Usages are Allowed' rule, it would seem both are still valid, even though it is clearly illegal for |
@Shane32 Great catches. I think you're right, the variable must be defined non-nullable. I'll add a change to reflect this when I get around to implementing the GraphQL.js work. |
Ok. GraphQL.NET v8 will incorporate this rule change; it can always be relaxed in the future without breaking any clients if the WG decides against it. |
First came
the @oneField directive.Then there was
the Tagged type.Introducing: OneOf Input Objects
and OneOf Fields.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.oneField: Boolean
field, and in SDL via the@oneOf
directive on the input object.OneOf Fields are a special variant of Object Type fields where the type system asserts that exactly one of the field's arguments must be set and non-null, all others being omitted. This is represented in introspection with the__Field.oneArgument: Boolean!
field, and in SDL via the@oneOf
directive on the field.(Why a directive? See the FAQ below.)
This variant introduces a form of input polymorphism to GraphQL. For example, the following
PetInput
input object lets you choose between a number of potential input types:Previously you may have had a situation where you had multiple ways to locate a user:
with OneOf Input Objects you can now express this via a single field without loss of type safety:
FAQ
Why is this a directive?
It's not. Well, not really - its an internal property of the type that's exposed through introspection - much in the same way that deprecation is. It just happens to be that after I analysed a number of potential syntaxes (including keywords and alternative syntax) I've found that the directive approach 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 😁
Why are all the fields nullable? Shouldn't they be non-nullable?
To make this change minimally invasive I wanted:
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!
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:
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 thatvalue
is supplied with a non-null int:Can we expand
@oneOf
to output types to allow for unions of objects, interfaces, scalars, enums and lists; potentially replacing the union type?🤫 👀 😉