Skip to content
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] Support empty composite types #606

Closed
wants to merge 4 commits into from

Conversation

victorandree
Copy link

@victorandree victorandree commented Aug 5, 2019

Supporting empty types, e.g. objects, input objects and interfaces without any fields, has concrete use cases.

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:

    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 #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).

@victorandree victorandree changed the title Support empty composite types [RFC] Support empty composite types Aug 5, 2019
@IvanGoncharov IvanGoncharov added the 💭 Strawman (RFC 0) RFC Stage 0 (See CONTRIBUTING.md) label Aug 5, 2019
@@ -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.
Copy link
Member

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:

#236

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.

Copy link
Collaborator

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.

victorandree pushed a commit to victorandree/graphql-js that referenced this pull request Aug 14, 2019
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.
@victorandree
Copy link
Author

victorandree commented Aug 28, 2019

I've pushed 3 commits to this PR. The first adds a note saying you can always query for __typename on an Object. The second adds empty input objects to the examples for input objects. The third adds a note about empty query root operation types (#490).

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 objects

There 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 struct without any fields. So, building a GraphQL library in C might be more complicated.

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
Copy link
Author

@leebyron @benjie Is there something I can do to get this moving (or close it)?

@IvanGoncharov
Copy link
Member

@victorandree You can add it to the WG agenda:
https://github.com/graphql/graphql-wg/blob/master/agendas/2019-10-10.md#agenda
We will have it tomorrow would you be able to join?
If so you can add yourself and this proposal to the agenda (e.g. graphql/graphql-wg#262)

victorandree added a commit to victorandree/graphql-wg that referenced this pull request Oct 9, 2019
Also adds Victor A to attendees list (author of the [RFC](graphql/graphql-spec#606))
leebyron pushed a commit to graphql/graphql-wg that referenced this pull request Oct 9, 2019
Also adds Victor A to attendees list (author of the [RFC](graphql/graphql-spec#606))
@leebyron
Copy link
Collaborator

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.

Support for GraphQL APIs without any Query operation type fields, for example, if the API only support mutations or subscriptions (see #490). This allows defining an empty Query object type, while still supporting introspection.

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.

Support for algebraic data types (see #568), where __typename is the only relevant field

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.

Potentially to support "well-known" (but empty) extensions of the introspection schema (see #300)

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

@leebyron
Copy link
Collaborator

Also something to consider is whether we should support not just empty definitions but empty queries?

Should you be able to write {} as a legal query? or { field {} }? These were originally excluded because we thought it was more likely to be a mistake than an intentional decision.

@benjie
Copy link
Member

benjie commented Oct 10, 2019

I can envision some alternatives for this specific problem, like allowing not providing a Query type

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.

Also something to consider is whether we should support not just empty definitions but empty queries?

If we have:

type MyType {}
type Query {
  field: MyType
}

then the query { field {} } could (assuming no errors) return { "field": null } or { "field": {} }. There is a tiny bit of information here (e.g. you can tell that the MyType object exists without forcing the backend to retrieve it). I think the slightly more verbose query { field { __typename } } is preferable, it should be ~equivalent in terms of backend effort, and it helps to rule out accident.

In conclusion, I don't see any need to support empty queries, { __typename } is valid against any GraphQL API/composite type.

@victorandree
Copy link
Author

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)

Motivation

Not all GraphQL APIs need a query interface, but do fine with just subscription or mutation. This is especially apparent when writing GraphQL micro services. These APIs are forced to expose a dummy Query type because a "query root operation type must be provided" (3.2.1 Root Operation Types)

The current spec requires a query root operation type, because schema introspection happens "on the root of a query operation" (4.5 Schema introspection). Thus, there's a dependency on a root query type for introspection to work.

Solution: Allow the query root type to be empty

By 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 type

This 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. __schema and __type don't show up if you introspect the query operation type, so conceptually (and implementation wise) they just happen to have the same entry point.

This would require changing the __Schema introspection type to make queryType nullable.

Support for algebraic data types (#568)

Motivation

The 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 __typename

By allowing empty objects, the __typename field can still be used to differentiate between different empty variants. This enables a pattern of switching directly on the __typename for different error conditions.

This allows encoding variants inside GraphQL's type system directly. This adds a lot of flexibility and extensibility since you can add more properties to different variants later.

Alternative: Use regular fields for switching, null for Nothing

With regards to errors specifically, you can use a name (or type or code) to differentiate between types of errors based on your application logic:

type CreateUserError {
  "Machine-readable error name, such as `UserWithEmailAlreadyExists`"
  name: String!
}

union CreateUserPayload = CreateUserSuccess | CreateUserError

Since these errors all have the same structure from a GraphQL perspective, does it really make sense to differentiate them in GraphQL's type system? You can't do anything interesting with them anyway (you'd probably do a switch in your application code).

A potential downside is that the set of possible errors is not encoded in the union type. You could, however, still use an enum for name to signal that.

Finally, there shouldn't be a need for a Maybe type in GraphQL, since that's captured by simply allowing a field to be nullable.

Extending the introspection schema (#300)

Motivation

In a comment on #300, I sketch some ideas for how extending the introspection schema could work (building on a lot of prior art).

In line with how extensions is already used in errors, I think it's desirable to namespace extensions of the __Type or __Field types rather than extending them directly.

Solution: Empty placeholder types

If e.g. __Type is guaranteed to contain a field extensions: __TypeExtensions of type type __TypeExtensions, we would have a standard type to extend.

However, such a type should probably be empty unless there are extensions.

A well-known type is preferable to allowing users to provide their own TypeExtensions type if you have extensions from different sources (say for Apollo Federation extensions and for some authorization or validation library).

Alternative: Use machine-readable descriptions

You can use field and type descriptions to put schema metadata in, using some standard to signal what the primary keys are on an object type. Here's an Apollo Federation directive simply moved to the description field.

"""
@key(fields: "id")
"""
type User {
  "@external"
  id: ID!
  username: String
}

I wouldn't favor this, because I don't think descriptions are meant for computers.

Alternative: Parsing the SDL

This is how Apollo Federation implements additional meta data for its federation types currently, since it's the only way to expose directives.

I don't think this is a good approach, because it relies on the SDL (so you can't programmatically construct these schemas) and I think it doesn't use directives "correctly" (see the lengthy discussion in #300).

@victorandree
Copy link
Author

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've opened a new PR to make the root query operation optional (see [RFC] Make root query operation type optional #631)
  • I think using "marker types/interfaces" for algebraic data types is the wrong way to go because the types wouldn't be structurally different from the perspective of the GraphQL type system. Just put a kind: ErrorKind or type: ErrorType field to differentiate.
  • The extensible introspection use-case is being worked on elsewhere and I don't want to derail that work. I hadn't read the notes from the 2019-05-02 WG meeting where @IvanGoncharov presented 2-stage introspection when I started this.

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.

@IvanGoncharov IvanGoncharov added 🗑 Rejected (RFC X) RFC Stage X (See CONTRIBUTING.md) and removed 💭 Strawman (RFC 0) RFC Stage 0 (See CONTRIBUTING.md) labels Oct 23, 2019
@alamothe
Copy link

alamothe commented Nov 26, 2019

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.

I think using "marker types/interfaces" for algebraic data types is the wrong way to go because the types wouldn't be structurally different from the perspective of the GraphQL type system. Just put a kind: ErrorKind or type: ErrorType field to differentiate.

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 __typename.

So, GraphQL spec forces us to put a dummy inside every empty type.

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.

@victorandree
Copy link
Author

@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.

What does the first part mean? Why does GraphQL needs structural difference?

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 message: String! (for people) field. If you want to classify errors for computers, use a code: ErrorCode! field. If you want some errors to have additional properties, then add a new type.*

* 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.

@alamothe
Copy link

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.

@akdor1154
Copy link

akdor1154 commented Feb 23, 2020

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.

@yaacovCR yaacovCR mentioned this pull request Apr 4, 2022
mattstern31 pushed a commit to mattstern31/graphql-wg-membership-note that referenced this pull request Dec 1, 2022
Also adds Victor A to attendees list (author of the [RFC](graphql/graphql-spec#606))
@mathieuprog
Copy link

mathieuprog commented May 27, 2023

I keep wasting my time thinking like, now what insignificant dummy value should I add inside my EmailAlreadyUsed object? 🤦

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🗑 Rejected (RFC X) RFC Stage X (See CONTRIBUTING.md)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

The ability to represent empty objects Proposal: Do not require query as a root operation type
7 participants