-
Notifications
You must be signed in to change notification settings - Fork 1.1k
[RFC] Support empty composite types #606
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
Conversation
@@ -1052,8 +1051,7 @@ Interfaces are never valid inputs. | |||
|
|||
Interface types have the potential to be invalid if incorrectly defined. | |||
|
|||
1. An Interface type must define one or more fields. |
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.
Interesting discussion around this point specifically:
This also drives the requirement that Interfaces must define at least one field - otherwise the Interface would not actually describe a contract to be fulfilled. This concept of contract-less interfaces is often called "marker interfaces" and in my opinion (hopefully not an uncommon opinion) are a bad practice borrowed from limited object-oriented languages.
-- @leebyron
I'm personally in favour of field-less interfaces as a placeholder for adding shared fields later. E.g. you might say that 3 different types are Searchable
and in future you may require that they have name
and url
in common so that you can display them in a search results page even if you don't have specific handling for some of the concrete types, but when you start designing your schema you might not know what those "Searchable" shared fields are/should be named yet.
I also think this can be useful for interfaces that implement other interfaces:
interface Searchable implements Node {}
e.g. to say that everything that's searchable is a Node, but not everything that's a Node is searchable.
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.
Based on the latest from the transitive interface implementation RFC
interface Searchable implements Node {}
would actually be an error, unless Node itself did not have a contract with any fields. implements
instead of extends
is intentional here, so Searchable
would have to implement (declare) all fields Node
's contract requires.
Supporting empty types, e.g. objects, input objects and interfaces without any fields, has concrete use cases. - Support for GraphQL APIs without any `Query` operation type fields, for example, if the API only support mutations or subscriptions (see graphql#490). This allows defining an empty `Query` object type, while still supporting introspection. - Support for algebraic data types (see graphql#568), where `__typename` is the only relevant field - Potentially to support "well-known" (but empty) extensions of the introspection schema (see graphql#300) This is a minimalist spec change, which simply removes the relevant items under the type validation sub sections. It would probably be helpful to motivate the change and mention that one or more fields _should_ be present for a (typically) useful schema. The requirement for composite types (object, input objects and interfaces) to define "one or more fields" was introduced in 0599414. This change references graphql/graphql-js#368, motivating the change with: > Since GraphQL always requires you to select fields down to scalar values, an Object type without any defined fields cannot be accessed in any way in a query. > This could be even more problematic for Input Objects where a required input object argument with no fields could result in a field that is impossible to query without producing an error. With regards to these objections: - It's always possible to select `__typename` and therefore even empty object types can be useful (as e.g. algebraic data types) - Passing an empty input object appears syntactically valid: ```gql mutation { update(input: {}) { name } } ``` I think this proposal fulfills the guiding principles by enabling new capabilities motivated by real use cases. This change does not make any previously valid schema invalid, so largely preseves backwards compatibility. Fixes graphql#568 and graphql#490 at the specification level.
0db4cdd
to
1845261
Compare
I've pushed 3 commits to this PR. The first adds a note saying you can always query for In my opinion, the spec doesn't have to motivate why these validations were removed, or make a judgment about the usability of empty types. Feedback and discussion is kindly requested. I'm not sure what I can do to move this ahead, or if it could be considered ready for the next stage (I'd be happy to be the champion if so). Empty objectsThere was some discussion in #490 if empty objects could cause problems in some languages. C is actually a language where this could be a problem, because you can't have a Other solution: Interfaces must represent a contract (only empty Objects/Input Objects)I can see the point in requiring interfaces to define one or more fields in a production ready, well-defined schema (as linked by @benjie). An alternative approach would therefore be to only remove validation for Objects and Input Objects in this RFC. Personally, I think enabling empty Object, Input Objects and Interfaces in one change better aligns with the Guiding Principles of "simplicity and consistency" while being worse from a "backwards compatibility" and "favor no change" perspective. I think it would allow for more flexibility in evolving a schema from "placeholder" types. However, I would agree with @leebyron that "marker interfaces" are commonly used as "metadata annotations for languages that don't have metadata annotations" and that this is bad practice if you have a good system for metadata annotations. I think this demonstrates the need for an extensible introspection system, so that GraphQL has a good system for metadata annotations. |
@victorandree You can add it to the WG agenda: |
Also adds Victor A to attendees list (author of the [RFC](graphql/graphql-spec#606))
Also adds Victor A to attendees list (author of the [RFC](graphql/graphql-spec#606))
I'm personally concerned about this approach. We were very intentional about this limitation to avoid problematic schema and type system anti-patterns (you've seen the past discussions about this). This may be the direction with the best solution, but we need to tread carefully to ensure such a change would be net-positive. I'd love to see some exploration around alternative approaches to solving the problems outlined to support a strong case that this approach is the best one.
I can envision some alternatives for this specific problem, like allowing not providing a Query type, similar to the existing behavior of allowing not providing a Mutation type. This might also be a bad idea, but it's worth exploring the tradeoffs with an empty Query type.
This is certainly interesting, and probably the most compelling reason for empty Object types. In the past we've viewed this as an API anti-pattern, since it over-relies on type differentiation which can be a sub-par API response to interpret. However it's clear that Unions are becoming more broadly used, and the algebraic type pattern is particularly useful for mutations. I still think this is probably an anti-pattern for other reasons, like error conditions for a mutation should probably come with data about how a client should behave (error message, persistent vs transient error, etc) that algebraic data types require the client to just have knowledge of ahead of time, but that's more of just my opinion that actually something we should design limitations for.
Empty interfaces feels like the wrong solution to this problem. Most of the extensions / metadata want to belong to fields or arguments instead of types and they often contain more than just a named flag. Empty interfaces are also an explicit anti-pattern we intentionally excluded to avoid marker-interfaces which are less powerful than union types |
Also something to consider is whether we should support not just empty definitions but empty queries? Should you be able to write |
How do you envision introspection working in this scenario ─ perhaps an introspection-only Query type is used if you don't provide your own Query? (If so: how do you rename this automatic Query type?) I think I'd prefer an explicit empty Query type be defined than implicitly adding one if I try to "opt-out" of Query.
If we have: type MyType {}
type Query {
field: MyType
} then the query In conclusion, I don't see any need to support empty queries, |
I really appreciate the comments @leebyron! Your comments highlight what problems an RFC should solve. I admit that this RFC is almost a solution looking for a problem given how the mentioned use cases are expressed. Unfortunately, out of the three, I'm personally most concerned about an approach to solving #300 – which this RFC isn't close to solving on its own. Maybe each problem deserves its own treatment, instead of muddying them together like this? I did a write-up on each problem below with alternatives I've seen. It's lengthy. Support schemas without a root query type (#490)MotivationNot all GraphQL APIs need a The current spec requires a Solution: Allow the query root type to be emptyBy not defining any own fields, the schema effectively doesn't expose a root query type, without breaking the introspection dependency. This approach maintains a large degree of backwards compatibility and only requires a small change. It does introduce an inconsistency, however, because the other root operation types can be not provided or they can be empty. Which one is better? Alternative: Allow not providing a Query typeThis approach puts the Query type on equal footing with other root operation types. This approach is more consistent in some ways since all root query types would be the same, but introspection would stick out more. Without moving the schema introspection to its own root operation type, you'd still be able to query these implicit fields on the non-existent root query operation. These already are implicit, so maybe it's not so bad. This would require changing the Support for algebraic data types (#568)MotivationThe original motivation is to support "empty variants" in union types, to differentiate between different kinds of errors (see #568). The original issue only discusses errors, but there may be other use cases. Solution: Allow empty objects differentiated by
|
In response to feedback at the latest WG meeting, I've decided to close this PR in favor of trying to solve the relevant problems instead of starting with a solution.
I think there could be a use case for making empty composite type validation optional or a "warning", when you're still developing schemas or are preparing them for future extension. |
It's pretty insane to see such a simple, widely useful removal of a completely arbitrary and artificial check in the GraphQL spec be rejected.
What does the first part mean? Why does GraphQL needs structural difference? "Marker interfaces" is a OOP concept and I don't see how you can draw parallels to ADT. It's a complete misnomer. The proposed solution is not OK, as some of the types in the union could have fields and some would not. Consider: type Mutation {
login(username: String!, password: String!): LoginResponse!
}
union LoginResponse = WrongUsernamePassword | TwoFactorRequired | LoginSuccessful
type WrongUsernamePassword = {}
type TwoFactorRequired = {
sessionKey: String!
}
type LoginSuccessful = {
token: String!
} In this case, the only way to write type-safe code is to discriminate using So, GraphQL spec forces us to put a I am also left wondering which other programming language does not allow for empty classes/types. I will continue to use a patched GraphQL package (two lines of meaningless check removed) to allow empty types in my schemas. |
@alamothe Note that the author (me, who is not officially affiliated with GraphQL) is the one abandoning this PR because I'm not interested in it anymore (for the reasons stated). I don't think this is "insane". If you're willing to champion it, feel free to pick it up – you may not be alone in thinking this is a good idea.
That's a personal interpretation on my part about some of the design goals and the context in which GraphQL is used. I perceive the goals of the type system to be about covering the shape of the data. In practice, it is about describing the shape of a simple JSON object. Therefore, if two objects don't differ in structure, they should be the same type. I'd make the case that a "good" error type in a schema should include a * Actually, I don't think errors should be part of a GraphQL schema definition, but that you should use the standard error mechanism for all errors. This is yet another personal opinion, but it does mean that I don't think errors are an interesting argument for ADTs. |
Error messages should not come from the server, error codes should. In any case, ADTs go much beyond errors. Is wrong username/password in my example an error? I doubt anyone who does functional programming would agree. Thank you for the time you put into this. It's a huge disappointment for me to see this being rejected. I pretty much lost faith in GraphQL at this point. I have no intention to champion it and waste my time with that as I don't see it going anywhere. I am happy that we can patch these libraries and fix obvious lack of foresight such as this. |
More support for this here - I am trying to represent ADTs in my schema, for domain objects, not errors. Working around this by including dummy variables which is frankly ridiculous. |
Also adds Victor A to attendees list (author of the [RFC](graphql/graphql-spec#606))
I keep wasting my time thinking like, now what insignificant dummy value should I add inside my |
Supporting empty types, e.g. objects, input objects and interfaces without any fields, has concrete use cases.
Query
operation type fields, for example, if the API only support mutations or subscriptions (see Proposal: Do not require query as a root operation type #490). This allows defining an emptyQuery
object type, while still supporting introspection.__typename
is the only relevant fieldThis is a minimalist spec change, which simply removes the relevant items under the type validation sub sections. It would probably be helpful to motivate the change and mention that one or more fields should be present for a (typically) useful schema.
The requirement for composite types (object, input objects and interfaces) to define "one or more fields" was introduced in 0599414. This change references graphql/graphql-js#368, motivating the change with:
With regards to these objections:
It's always possible to select
__typename
and therefore even empty object types can be useful (as e.g. algebraic data types)Passing an empty input object appears syntactically valid:
I think this proposal fulfills the guiding principles by enabling new capabilities motivated by real use cases. This change does not make any previously valid schema invalid, so largely preseves backwards compatibility.
Fixes #568 and fixes #490 at the specification level. I've forked graphql-js to remove the corresponding validations (see https://github.com/victorandree/graphql-js/tree/rfc/empty-objects).