Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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?
RFC: OneOf Input Objects #825
Changes from all commits
c385058
f6bd659
39e593c
b6741c3
d17d5ec
dca3826
7e02f5a
4111476
6754e0a
7c4c1a2
bb225f7
05fde06
e8f6145
08abf49
59cb12d
c470afb
99aa5d9
691087d
7109dbc
05ab541
6a6be52
de87d2f
57e2388
5a966f2
e78d2b5
c6cd857
d106233
87d0b22
d88d62a
a810aef
a1563a9
b45c0e4
0c9830e
c4d0b50
340594e
dbccf84
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Why is this the case? extend is useful for tooling to "create types" in a peicemeal fashion. Not everyone needs to know all the details but it needs to be validated at the end at actual schema creation time
and
The following once combined is valid surely?
if we had an invalid combination say
and
then this can be validated at the time the actual runtime schema object is created.
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.
Twofold: it’s because a ”OneOf Input Object” may be constructed in a different way to traditional input objects (might use a different class or similar), and because if this was not the case then when the directive was added you’d have to go back and validate all previously added fields since they have different “potential to be invalid” text.
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.
Hmm kinda feels like this is projecting an implementation into the spec situation.
I would argue that the spec should not care how its implemented - sure it could be a new OneOfInputType class at implementation time (I toyed with this in graphql-java but rejected it as too much impact) but the spec shouldn't care.
Again an implementation detail - not sure that the spec should care.
Even merging them seems straightforward since they are a non repeating directive
And implementation can merge these easily enough.
Right now the spec says about input types
in other words it allows you to put extra directives on the input types during extension
I just feel like this is an unnecessary restriction
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 agree, I'd argue that implementing Input Objects and OneOf Input Objects as the same class in the implementation is an implementation detail that the spec doesn't need to worry about 😉
At one point, oneOf was going to be its own top-level type, like
oneof Foo { a: Int, b: Float }
rather thaninput Foo @oneof { a: Int, b: Float }
. Would you feel the same if we had chosen the alternative syntax?To me, a OneOf Input Object is a distinct type in GraphQL that happens to share a lot with Input Objects, but still has it's own distinct behaviors in many ways. As such, changing an Input Object into a OneOf Input Object would be like changing an Input Object into an Object for me - and that's not something I think the SDL should allow.
This is one of the arguments against using the directive syntactical representation in my opinion, and encouraging using a keyword instead - to make the separation clearer.
This second point is not an implementation detail, it's a spec detail - specifically it relates to lines 1724-1726 in
spec/Section 3 -- Type System.md
- that's the validation rules for the definition of aninput
- if it's defined asinput Foo @oneOf
then additional rules apply. Then similar rules are applied on lines 1756-1759 for fields added via an input type extension. If we allow the directive to be added during an input type extension then the previously performed rules in the input type definition would now be invalidated and we'd need to re-check them; I can't think of a place where we've done this kind of back reference and re-validation in a similar part of the spec.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.
Along the same thoughts that @benjie is pointing out, I personally would say a different syntactical representation in the SDL would make it appear as a "distinct type", whereas since it is a directive, it is simply additional validation rules on top of the existing input object type. Keep in mind that directives are described as such:
I think this definition fits
@oneOf
precisely - "evaluated differently by a validator". This PR also describes oneOf input objects as such:So the way this PR is written, I see
@oneOf
as a predefined directive, just like@skip
or@include
or@deprecated
, that converts the input object it is decorating into the 'special variant'. As such, I would personally agree with @bbakerman in that the directive should be able to be applied anywhere it has always been, and hence the extra validation checks applied if the schema requires it after being merged.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.
If directives were intended to merely annotate types, and not to modify their behavior, then perhaps
@oneOf
should not be allowed on extension types. But the spec clearly indicates that directives' purpose is to modify behavior, and that they are allowed on extension types.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.
Indeed, they are a special variant. In my opinion you wouldn't take a broad thing and then turn it into a special variant at a later stage - it needs to be a special variant from the start. But this is my opinion, I'm happy to test it against the broader working group.
Imagine for a second that we didn't have input objects. You could imagine taking an object type and tagging it
@input
:This syntactic change could indicate:
@input
types@input
typeThis is essentially the input object type at this point. But all we've done as annotated the existing object type with different validation logic and execution behaviors. Is the input object type really not distinct from the object type? Would turning an object type into an
@input
object type be a reasonable thing to do later down in the SDL? I, personally, don't think so. I think doing so would be extremely confusing for people reading the SDL - where something suddenly changes from one type of thing to another.This, again, is one of the big issues that I have with using a directive to represent this distinct type. (I'm also in favour of using the directive because it's the smallest change, and allows it to be compatible with existing clients.) This constraint: that you can't add the directive in an extension, is my compromise - to allow us to leverage the benefits of using a directive without the footguns in terms of changing one thing into another at a later stage.