-
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] Input Objects accepting exactly @oneField #586
Conversation
Re: names.. This is called |
I do like that this seems like a minimal change to achieve a meaningful improvement.. The tricky part is the layer of nesting that it requires. To get a data structure that matches on the query side, it means you'd have to ignore I think this is used in the wild (we use it at New Relic) not because it's ideal, but because it's the only work around available... |
Due to confusion, I've removed the word |
@benjie I like your proposal. I want to add something regarding the following statement and in general some additional benefits I found while reading your proposal and thinking about it:
In some cases it would not even increase the depth compared to Example with input PostMatch @oneField {
ids: [Int!]
conditions: [PostCondition!]
} Example with inputUnion PostMatch = PostMatchByIds | PostMatchByConditions
# input PostMatchByIds = [Int!] <- not possible
# scalar PostMatchByIds = [Int!] <- not possible
# inputUnion PostMatch = [Int!] | [PostCondition!] <- not possible
# same depth:
input PostMatchByIds = {
ids: [Int!]
} How would you have to define the above inputUnion to have less depth than using If it is a scalar, you would have to declare a custom scalar type to preserve the intent, e. G. # what is Int?
inputType = int | PostConditions
# vs
scalar PostId
inputType = PostId | PostCondition Also additionally loosing the Int validation. While the Given your 2. example: """
Options available for matching a post title
"""
input PostTitleFilter @oneField {
equals: String
contains: String
doesNotContain: String
}
"""
You may search within the title of the post, or within it's full text.
"""
input PostCondition @oneField {
title: PostTitleFilter
fullTextSearch: String
}
"""
Identify the relevant posts either by a list of IDs, or a list of conditions
for them to match.
"""
input PostMatch @oneField {
ids: [Int!]
conditions: [PostCondition!]
}
type Query {
findPosts(matching: PostMatch!): [Post!]
} Using
*custom scalar: because input PostConditionTitleFilterEquals {
value: String!
} Hereby again having not less depth than required with Initially I was not very convinced by |
Interesting analysis, @akomm; thanks for sharing. I will definitely be referencing this when I next get a few minutes to advance this proposal. |
@binaryseed I was thinking more about your |
|
it's not. |
I noticed one thing, which I am not sure about whether this is a problem or not: You have added this section in validation: Input Object With Requires Exactly One Field Now I am not expert at spec writing, but when I would start implement the later, I am actually already doing it wrong, because I have at this point to take into account the new |
@akomm You wouldn't be doing it wrong if you did that; both sections need to be honoured for |
How about inlining input unions? input UserByID {
id: ID!
}
input UserByName {
name: String!
}
type Query {
getUser(where: UserByID | UserByName): User
}``` |
@jensneuse That's not something I would like to enable with this proposal. There's a lot of questions that raises, please see the discussion in #395 for more details. |
@benjie First: Thank you very much for all your contributions here and for trying to solve the problem with inputs. I like this proposal and it will match some use cases, but I feel that it is not universal and eventually, GraphQL may end up with 2 standards, cos other use cases will still not be covered.
input MediaBlock {
projectId: PostInputKind!
title: String!
photoContent: String
videoContent: String
} or input PageField {
pageId: ID!
code: String!,
singleValue: String
listOfValues: [String]
} In the examples above we use the tagged union approach which lacks the validation and input PageField {
pageId: ID!
code: String!,
singleValue: String?, // required if listOfValues is not present
listOfValues: [String]?, // required if singleValue is not present
anotherFieldThatIsNotRequired: String
} I mean that in schema we mark fields from which one is required somehow. |
@mazikwyry I disagree. The client is not in charge of (knowing/constraining) the schema. But this is the case in your It is not clear whether you think 'input union' is better than this, or that 'input union' is equaly wrong, because you did not mention it explicitly. If the later is the case, nvm., if the former is the case, than can you provide |
@akomm Hello and thanks for your comment.
I also disagree with myself. This is not what I meant. (Sorry, it was early morning, before coffee). I meant "I mean that in schema we mark fields from which one is required somehow. ? is just an example"
I'm not comparing those solutions. Not saying one is better. Just expressing my concerns about |
Oh, you are right, should have realized it from the schema above. :) However, what do you do if you need two groups of For example input PageField {
pageId: ID!
code: String!,
content: PageFieldContent!
anotherFieldThatIsNotRequired: String
}
input PageFieldContent @OneField {
singleValue: String
listOfValues: [String!]
} This allows you do make as many pick one of fields as you wish, solves your problem and makes a sum of things I will not enumerate here much better. |
@akomm Thank you very much. I this is possible with |
Regarding the @akomm has also outlined what the input Media {
title: String
caption: String
source: MediaSource!
linkTarget: LinkTarget
}
input MediaSource @oneOf {
url: String # e.g. "http://example.com/example.png"
clipart: String # e.g. "cats"
savedMedia: String # e.g. "MyPersonalUpload-150.png"
}
input LinkTarget @oneOf {
web: String # e.g. "http://example.com"
email: String # e.g. "foo@bar.com"
internal: String # e.g. "/other/page"
scriptAction: String # e.g. "flashPage()"
} * Click for some rambling about the `?` approach* (If you were to extend the input Foo {
int: Int @oneOf(tag: "int-or-float")
float: Float @oneOf(tag: "int-or-float")
bool: Boolean @oneOf(tag: "bool-or-string")
string: String @oneOf(tag: "bool-or-string")
} but if you do that then it is (a) cumbersome, and more importantly (b) you can't clearly and consistently specify if those one-ofs are required or not.) |
We have begun building a single RFC document for this feature, please see: The goal of this document is to consolidate all related ideas to help move the proposal forward, so we decided to close all overlapping issues including this one. If you want to add something to the discussion please feel free to submit PR against the RFC document. |
This comment from @derekdon is a good reason why Namely if you have the schema: type BoolVal {
value: Boolean
}
type StrVal {
value: String
}
union Union = BoolVal | StrVal
type Query {
test: Union
} Then this GraphQL query does not validate: {
test {
__typename
... on BoolVal {
value
}
... on StrVal {
value
}
}
} Because:
This is despite the type BoolVal {
value: Boolean
}
type StrVal {
value: String
}
type Value @oneField {
bool: BoolVal
str: StrVal
}
type Query {
test: Value
} {
test {
bool {
value
}
str {
value
}
}
} Code showing the existing errorconst {
GraphQLObjectType,
GraphQLSchema,
GraphQLBoolean,
GraphQLString,
GraphQLUnionType,
printSchema,
graphql,
} = require("graphql");
const BoolVal = new GraphQLObjectType({
name: "BoolVal",
fields: {
value: {
type: GraphQLBoolean,
resolve: val => val,
},
},
isTypeOf(value) {
return typeof value === "boolean";
},
});
const StrVal = new GraphQLObjectType({
name: "StrVal",
fields: {
value: {
type: GraphQLString,
resolve: val => val,
},
},
isTypeOf(value) {
return typeof value === "string";
},
});
const Union = new GraphQLUnionType({
name: "Union",
types: [BoolVal, StrVal],
});
const Query = new GraphQLObjectType({
name: "Query",
fields: {
test: {
type: Union,
resolve() {
return Math.random() >= 0.5 ? true : "string";
},
},
},
});
const schema = new GraphQLSchema({
query: Query,
});
console.log(printSchema(schema));
graphql(
schema,
`
{
test {
__typename
... on BoolVal {
__typename
value
}
... on StrVal {
__typename
value
}
}
}
`
).then(result => {
console.dir(result);
}); |
@benjie - Could your example be handled without |
I think that would work for unions, I'm not sure if it would work for interfaces though - I'd have to think on it harder. Was mostly just noting for my own future reference ;) |
* if `requiresExactlyOneField` is {true}: | ||
* for each {inputValue} in {inputFields} | ||
* the {type} of {inputValue} must not be Non-Null | ||
* the {defaultValue} of {inputValue} must be {null} |
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 think this would be no defaultValue instead of null
since you don't want to allow field: Type = null
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.
Spot on 👍
* `inputFields`: a list of `InputValue`. | ||
* if `requiresExactlyOneField` is {true}: | ||
* for each {inputValue} in {inputFields} | ||
* the {type} of {inputValue} must not be Non-Null |
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.
This answered my question on the call
* `inputFields`: a list of `InputValue`. | ||
* if `requiresExactlyOneField` is {true}: |
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.
In other places we have "Type Validation" sub-sections. That would be a better place for this list set
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.
Adding some thoughts here after our meeting since you're revisiting this path
Thanks Lee; I'll factor that feedback into the rewrite 👍 |
Rewritten (and resurrected) here: #825 |
This is an RFC for adding explicit GraphQL support (and with it improved type safety) for a pattern that supports inputs of multiple types (composite and scalar).
Explanation of the problem
Often a GraphQL field needs to accept one of many input types.
Example 1 (below) outlines the need for the
addContent
mutation to accept a range of different content types.Example 2 (below) outlines the need for the
findPosts
record finder to accept a range of different filtering criteria.Currently in GraphQL the type safe solution to this is to have different fields, one per type:
Example 1 (more details in "Example 1" section below):
addPostContent(post: [PostInput!]!)
addImageContent(image: [ImageInput!]!)
addHrefContent(href: [String!]!)
Note to add lots of content blocks of different types you must add multiple mutations to your GraphQL request, which is a lot less efficient and loses the transactional nature of the request.
Example 2 (more details in "Example 2" section below):
findPostsByIds(ids: [Int!])
findPostsMatchingConditions(conditions: [PostCondition!])
Another solution is the "tagged input union" pattern, which results in a less overwhelming schema thanks to fewer fields, solves the issue of inputting lots of data in a single mutation, but loses type safety:
(The loss of type safety is that each block should require that exactly one of the fields is set, however GraphQL does not enforce this so it must be done at run-time, and generic tooling will not be able to confirm the validity of inputs.)
Prior Art
This RFC is similar to the "tagged input union" pattern discussed as an alternative in the
inputUnion
RFC #395 (please see the reasons people may not be happy with it there also). Since we are failing to reach consensus of on theinputUnion
type in that proposal, I'd like to revisit Lee's original counter-proposal with the small addition of a directive to require that exactly one field be present on the input object (as originally proposed by Ivan).The input object
requiresExactlyOneField
property (exposed via@oneField
)This new
requiresExactlyOneField
introspection property for input objects can be exposed via IDL as the@oneField
directive (in a similar way to how thedeprecationReason
introspection property is exposed as@deprecated(reason: ...)
).This property applies to an input object type:
(In the IDL, we could alternatively expose it as a different type:
inputOne MyInput
, rather thaninput MyInput @oneField
, if using a directive was found to be confusing.)The fields of an input object type defined with
@oneField
must all be nullable, and must not have default values. On input, exactly one field must be specified, and its value must not be null.(For the avoidance of doubt: the field types may be scalars or input objects, and the same type may be specified for multiple fields. This is the same as for regular input objects.)
I've called the flag
@oneField
(and the related introspection fieldrequireExactlyOneField
), but it could equally be called@requireExactlyOneField
,@exactlyOne
,@one
, or many other variants depending on your position in the terseness vs explicitness spectrum.Example 1
Example input:
Example 2
Example inputs:
Example 3
Example inputs:
Guiding principles
Backwards Compatibility
This change involves a small addition to the introspection system. Old clients will continue to work as they do currently, and can issue requests involving
@oneField
input objects without needing to be updated, they simply will not benefit from the type validation of knowing exactly one field is required.All pre-existing queries (and schemas) will still be valid, and old clients can query servers that support this feature without loss of functionality.
Performance is a feature
There is minimal overhead in this change; input objects continue to work as they did before, with one additional validation rule (that exactly one field must be specified). This can be asserted during the regular Input Object Required Fields Validation at very little cost. (In the spec I've written it up as a separate rule, but implementations can implement this more efficiently.)
Favour no change
The change itself is small (and hopefully easy to implement), whilst bringing significant value to type safety. It's not possible to represent this use case in a type safe way in GraphQL currently, without having a proliferation of object type fields such as
addPostMediaBlock
,addImageMediaBlock
, ... (which gets even more complex when there's nested concerns).Enable new capabilities motivated by real use cases
It's clear from #395 and #488 (plus the multiple discussions during GraphQL working groups, and the community engagement) that a feature such as this is required. It's currently possible to express this in GraphQL (simply omit the
@oneField
directive in the examples above), however it is not expressed in a type-safe way and requires the schema's resolvers to perform validation, resulting in run-time rather than compile-time errors (and no hinting in editors/tooling).Simplicity and consistency over expressiveness and terseness
I believe this has been achieved (bike-shedding over the directive name notwithstanding).
Preserve option value
This RFC is only on using this directive with input objects, I have been deliberately strict to keep the conversation on topic. There is room for expansion (and even for the addition of an explicit
inputUnion
) in future.Understandability is just as important as correctness
I've done my best; please suggest edits that may make the spec changes more clear 👍
Comparison with inputUnion
The field name takes the place of the "discriminator" discussed in #395.
The implementation is significantly simpler, for example the only change required to support autocompletion in GraphiQL is to stop autocompleting once one field has been supplied.
The implementation is backwards compatible: older clients may query a schema that uses
@oneField
without loss of functionality.The implementation is forwards compatible: newer clients may query a schema that implements an older GraphQL version without causing any issues (whereas the unexpected addition of
__inputname
may break existing GraphQL servers, hence complexity of only specifying it where a union is used).There's no
__inputname
/__typename
required, and heterogeneous input types are still supported.The field types may be input objects or scalars.
This pattern is already in use in the wild, but with the execution layer handling validation, rather than the schema itself.
@oneField
does not mirror output unions/interfaces.@oneField
increases the depth of input objects vs theinputUnion
proposal (but, interestingly, does not increase the size of the request:Potential concerns, challenges and drawbacks
input union
, and does not mirror theunion
syntax for output typesrequiresExactlyOneField
This pattern is already used in the wild, and the introduction of this feature will not break any existing APIs. APIs that already use this pattern could benefit from this feature retroactively without needing to update any any existing clients.