-
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] Tagged type #733
[RFC] Tagged type #733
Conversation
Great job @benjie, this looks really solid overall. |
All but one piece of feedback addressed. Will ponder the final point. Thanks @spawnia 🙌 Changing from WIP to ready for review, and adding it to the next GraphQL Spec WG 👍 |
Great work @benjie and everybody else involved.
These questions are relevant to all type systems changes I believe. |
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 is looking amazing, great work!
spec/Section 3 -- Type System.md
Outdated
@@ -266,6 +267,10 @@ A `Union` defines a list of possible types; similar to interfaces, whenever the | |||
type system claims a union will be returned, one of the possible types will be | |||
returned. | |||
|
|||
A `Tagged Type` defines a list of possible members; whenever the type system |
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.
at this moment it's useful to explain, briefly, what a "member" is - a tag named in the form of a field, and the type of that field.
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 addressed this, but I'm not happy with the wording so I'm not marking this conversation resolved.
spec/Section 3 -- Type System.md
Outdated
* If {type} is a Tagged type: | ||
* If there exists a member in {type} where {IsInputType(memberType)} is {false} | ||
* Return {false} | ||
* Return {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.
This is nice recursion. I like that IsInputType
remains fully defined in terms of itself.
spec/Section 3 -- Type System.md
Outdated
no two members may share the same name. | ||
2. The member must not have a name which begins with the | ||
characters {"__"} (two underscores). | ||
3. {IsOutputType(taggedType)} and {IsInputType(taggedType)} cannot both be {false}. |
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.
<3
@andimarek Excellent questions, I agree they're very relevant.
I think from a fundamentals point of view, this shouldn't make static analysis or query rewriting any different. In most ways a Tagged type operates equivalently to an Object type and provides the same level of static knowledge of types. Some cases we should want to stress test are the edge cases and corner cases of using fragment spreads along with the additional constraints Tagged types offer. That will inform how strict we need to be about these constraints. For example, we have an implicit rule that a fragment spread should always be able to be inlined without changing meaning or validity. eg Can you think of other scenarios which could pose problems for static analysis or query rewriting?
This is an excellent question because this moves counter to the good design principle that there should be one way to model something. That's not a principle explicitly listed but it's still a good one. Now when you consider a case that you have a field which can return one of many options, we're expanding your choices from 2 (interface, object) to 3 (+ tagged). There was already pushback during original design years ago from @schrockn that perhaps there should be only one (interface) where unions are modeled as interfaces which have no fields. There are other modeling tradeoffs there, but the benefit was ensuring only one way to do abstract types - potentially creating a pit of success. Something critical we'll need to do is well document the tradeoffs for when you should consider using a Union vs Tagged type (or Interface), and in fact if we can be terse and clear about it that would be quite valuable to include within the spec itself so there's a canonical explanation of why multiple options exist. Can you see other schema design pitfalls? |
Thanks for the review @leebyron! I've 👍'd the input I agree with without hesitation. Some others need discussion, or I need to think about more. I'm unlikely to have time in August to implement the suggestions, but hopefully will have more time in September - I welcome others' feedback in the mean time 👍
I currently propose that this is definitely legal, it's allowed in the RFC as is, I see no reason to forbid it, and it simplifes the concerns you raise 👍 The main reason for this is that If we chose not to allow |
Thanks for putting this together! My one concern here is in that we're allowing For example, I can write an "input union", but then discover that my type got accidentally restricted by someone putting it in the output type set. i.e. if I write an input union tagged Filter {
startsWith: String
endsWith: String
} and then come back two days later to add an optional input object parameter: tagged Filter {
startsWith: String
endsWith: String
range: Range
}
input Range {
start: Int
end: Int
} I might not be able to, if someone decided to include That is to say: right now, there's no way for any type to "change" its input/output availability. Scalars and Enums are always "both"; input types are always "input only"; objects types, interfaces, and unions are always "output only". But As I understand it, the impetus for this proposal was to enable input unions. If the solution for input unions naturally extended to the output types, then it might make sense to include that extension for cleanliness, and it seems like that was the intention here. In this case, though, I think adding output type system support might add complexity, and I don't immediately see the benefit that it yields since we already have unions in the output type system. So I'd consider restricting |
I do think this is an interesting problem in general -- the desire to have a type that can be shared between input and output makes a lot of sense to me. My concern is more around trying to solve that problem at the same time that we're solving the "input unions" problem, when those feel like separate issues to me. |
@dschafer This is my biggest concern too, as discussed on the WG. However tagged brings “unions” of objects, unions, interfaces, scalars, enums and lists to the output schema, whereas GraphQLUnion only allows for objects, so I think it definitely has value there (see e.g. #215). As mentioned on the WG, I think an alternative solution to your specific issue would be a schema validation rule stating that a specific tagged type may only be used in an input context or in an output context but not both. This validation rule could be dropped in future if we find value in using the same tagged in both contexts. That said, I trust users to make this decision for themselves and implement the restriction in their own tooling should they need it. |
e5d241d
to
6c81ed8
Compare
Changes: * Comparison operators: `{eq: 1, not: yes}` => `{notEqual: 1}` * Logical: `{anyOf: [...], not: yes}` => `not { anyOf: [...]} The `not` became more consistent with `allOf`/`anyOf` and compatible with [Tagged Type](https://github.com/graphql/graphql-spec/pull/733)/[oneOf input](graphql/graphql-spec#825). Also resolved ambiguity for Relation: ``` # this means "where count(...) != 2" now relation: { where: {....} eq: 2 not: yes } ```
Changes: * Comparison operators: `{eq: 1, not: yes}` => `{notEqual: 1}` * Logical: `{anyOf: [...], not: yes}` => `not { anyOf: [...]} The `not` became more consistent with `allOf`/`anyOf` and compatible with [Tagged Type](https://github.com/graphql/graphql-spec/pull/733)/[oneOf input](graphql/graphql-spec#825). Also resolved ambiguity for Relation: ``` # this means "where count(...) != 2" now relation: { where: {....} eq: 2 not: yes } ```
Considering how ubiquitous and well-understood |
There's plenty of alternative syntaxes than directives that we can use the represent the exact same thing; which of those would you prefer and would it overcome your "directives" concern? https://gist.github.com/benjie/5e7324c64f42dd818b9c3ac2a91b6b12
Please note that both this PR #733 and the oneof PR #825 would involve you sending the same data to the server: It sounds like your comment is in favour of solution 1 in the InputUnions proposal; one of the main issues that we found with this was that it would cause complexity around when to/not to include |
Is this primary due to overloading I apologise if I'm rehashing previous discussions. I guess my main point is that so-called tagged unions/variants are such a well-established pattern in programming languages, that it would be a shame to deviate from them. That said oneOf is present in both JSON Schema and Protobufs (for better or for worse). |
@jamiehodge Actually, I did kind of a proposal using directive. You can look at it in my comment of #825. I've even implemented it (for input only, but it's easily extensible to output). |
No, that's not a concern. I tried to describe this concern above; namely that (TypeScript syntax) you'd sometimes have: const user: UserInput = { name: "Benjie" }; and sometimes you'd instead have to have: const user: UserInputWithTypename = { name: "Benjie", __typename: "User" } depending on where the object is to be submitted. With oneof you would always just have the first one, and when you send it in as part of a oneof it would be explicitly wrapped ( |
I guess wouldn't think of the entity example as the primary use case, nor do I see the requirement of adding a tag property as onerous. Isn't a more realistic use of a tagged union, using a contrived example, a payment where it may be cash or credit and the two inputs are not entities, but have distinct fields? Another example, which I am dealing with is a list of changes to an aggregate, each pf which contain the type of change and then appropriate fields. Neither of these examples are entities and they are constructed at submission time. In neither case would the tag property be an unreasonable requirement. |
I... don't follow at all. When you say "entity" do you mean "input object" or something else? If they're not "input objects" then what are they? I used input Foo { ... }
input Bar { ... }
tagged Tagged {
foo: Foo
bar: Bar
}
type Mutation {
doSomethingWithFoo(foo: Foo!): Int
doSomethingWithBar(bar: Foo!): Int
doSomething(input: Tagged!): Int
} Here |
I see. Now I understand your reuse example. My mind went to entities when you initially described reuse, but I can see that something as simple as a union of scalar types would be a compelling argument for externalising the tagging. I don't know enough about the GraphQL parser to know how difficult it would be allow for an untagged union of scalar types, making object types the exception. I don't think I have any real objections to the above example. |
Basically impossible - we can't tell the difference between an Int and a Float (unless it has a decimal period in it) or a String and an ID, and of course there's custom scalars which we have absolutely no understanding of whatsoever. The types have to be declared, or there needs to be some kind of rule to help determine which it's coerced as. |
For reference, io-ts has adopted a somewhat similar solution, albeit a mixture of external/internal tagging: https://github.com/gcanti/io-ts/blob/master/Decoder.md#the-sum-combinator. There is a separate combinator for untagged unions: https://github.com/gcanti/io-ts/blob/master/Decoder.md#the-union-combinator. If anything the language distinction is helpful. |
I've been imitating the tagged input pattern for some schema and can confirm that it feels pretty natural. I will say that I'm unsure about the reuse story. For the sake of flexibility/extensibility, I would probably error on the side of keeping the input types distinct, even when they are identical at first. |
Closing in favour of #825 |
THIS RFC HAS BEEN SUPERSEDED by
@oneof
, for now at least... See: #825This is an RFC for a new "Tagged type" to be added to GraphQL. It replaces the "@oneField directive" proposal following feedback from the Input Unions Working Group. Please note that "Tagged type" is the working name, and may change if we come up with a better name for it.
A Tagged type defines a list of named members each with an associated type (like the fields in Object types and Input Object types), but differs from Object types and Input Object types in that exactly one of those members must be present.
The aim of the Tagged type is to introduce a form of polymorphism in GraphQL that can be symmetric between input and output. In output, it can generally be used as an alternative to Union (the differences will be outlined below). It goes beyond interfaces and unions in that it allows the same type to be specified more than once, which is particularly useful to represent filters such as this pseudocode
{greaterThan: Int} | {lessThan: Int}
.If merged, Tagged would be the first non-leaf type kind (i.e. not a Scalar, not an Enum) that could be valid in both input and output. It is also the first kind of type where types of that kind may have different input/output suitability.
In SDL, a tagged type could look like one of these:
(Note a number of alternative syntaxes were mooted by the Input Unions working group; the one above was chosen to be the preferred syntax.)
If we queried a
StringFilter
with the following selection set:then this could yield one of the following objects:
{ "contains": "Awesome" }
{ "lengthAtLeast": 3 }
{ "lengthAtMost": 42 }
Note that each of these objects specify exactly one key.
Similarly the above JSON objects would be valid input values for the
StringFilter
where it was used as an input.Tagged vs Union for output
Tagged does not replace Union; there are things that Union can do that tagged cannot:
And things that Tagged can do that Union cannot:
Tagged allows for exploring the various polymorphic outputs without requiring fragments:
When carefully designed and queried, the data output by a tagged output could also be usable as input to another (or the same, if it's suitable for both input and output) tagged input, giving polymorphic symmetry to your schema.
Nullability
Tagged is designed in the way that it is so that it may leverage the existing field logic relating to nullability and errors. In particular, if you had a schema such as:
and you issued the following query:
and for some reason the
name
field on Cat were to throw, the the result might come out as:where we can tell an error occurred and the result would have been a
Cat
but something went wrong. This may potentially be useful, particularly for debugging, compared to returning"pets": null
or"pets": [null, {"dog": {...}}]
. It also makes implementation easier because it's the same algorithm as for object field return types.FAQ
Can a tagged type be part of a union?
Not as currently specified.
Can a tagged type implement an interface?
No.
What does
__typename
return?It returns the name of the tagged type. (This is a new behaviour, previously
__typename
would always return the name of an object type, but now we have two concrete composite output types.)What happens if I don't request the relevant tagged member?
You'll receive an empty object. For example if you issue the selection set
{ cat }
against the tagged type below, but the result is a dog, you'll receive{}
.How can I determine which field would have been returned without specifying all fields?
There is currently no way of finding out what the field should have been other than querying every field; however there's room to solve this later with an introspection field like
__typename
(e.g.__membername
) should this show sufficient utility.Open questions
isInputType
/isOutputType
to__Type
for introspection? [Author opinion: separate RFC.]TAGGED_INPUT
andTAGGED_OUTPUT
types separately, rather than sharing just one type? [Author opinion: no.]{a: $a, b: $b}
[Author opinion: as currently specified.]