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

The ability to represent empty objects #568

Open
MOZGIII opened this issue Feb 28, 2019 · 29 comments
Open

The ability to represent empty objects #568

MOZGIII opened this issue Feb 28, 2019 · 29 comments
Labels
💭 Strawman (RFC 0) RFC Stage 0 (See CONTRIBUTING.md)

Comments

@MOZGIII
Copy link

MOZGIII commented Feb 28, 2019

Based on this: graphql/graphql-js#937 (comment)

Currently we can't represent an empty object type:

type Mutation {
  createUser(email: String!, password: String!): CreateUserPayload!
}

type UserWithEmailAlreadyExists
type WeakPassword
type AuthPayload {
  user: User!
  token: AccessToken!
}
union CreateUserPayload = UserWithEmailAlreadyExists | WeakPassword | AuthPayload

This fails with graphql-js with the following error:

Error: Type UserWithEmailAlreadyExists must define one or more fields.

Type WeakPassword must define one or more fields.
    at assertValidSchema (/Users/skainswo/dev/kumo/newer-world/api/node_modules/graphql/type/validate.js:71:11)
    at Object.validate (/Users/skainswo/dev/kumo/newer-world/api/node_modules/graphql/validation/validate.js:55:35)
    at doRunQuery (/Users/skainswo/dev/kumo/newer-world/api/node_modules/apollo-server-core/src/runQuery.ts:181:30)
    at /Users/skainswo/dev/kumo/newer-world/api/node_modules/apollo-server-core/src/runQuery.ts:80:39
    at process._tickCallback (internal/process/next_tick.js:68:7)

The usecase is legit though.

There is an explicit check to disallow types with zero fields at graphql-js:
https://github.com/graphql/graphql-js/blob/4116e2fc4fe36688f683258388f4a2d52076d199/src/type/validate.js#L273-L278

How about explicitly allowing empty fields in the spec? It's super useful for implementing algebraic types.

@stubailo
Copy link
Contributor

stubailo commented Mar 1, 2019

I think one interesting aspect of this is that you would still be able to query __typename on it, so it wouldn't break the query language to have an empty type! Like

fragment Something on WeakPassword {
  __typename
}

Side note having empty types is also useful if you want to declare a type for the sole purpose of using extend type on it later.

@MOZGIII
Copy link
Author

MOZGIII commented Mar 1, 2019

The problem is it doesn't allow you define a schema with an empty value.

Currently the workaround is to do the following:

type A {
  _: Boolean
}

@MOZGIII
Copy link
Author

MOZGIII commented Mar 1, 2019

Type extension with extend type works already btw:

type Query

extend type Query {
  a: Boolean;
}

@stubailo
Copy link
Contributor

stubailo commented Mar 1, 2019

Yes - I'm agreeing with you!

@alamothe
Copy link

alamothe commented May 14, 2019

What does it take to fix this issue?

I found that simply commenting out the check, everything just works.

(I'm using Apollo on the client and graphql-java on the server)

@IvanGoncharov
Copy link
Member

What does it take to fix this issue?

It's a spec change so somebody should champion it and move through all of the stages:
https://github.com/graphql/graphql-spec/blob/master/CONTRIBUTING.md#rfc-contribution-champions

@victorandree
Copy link

victorandree commented Aug 5, 2019

For background, the requirement to have at least one field was introduced to the spec by 0599414. This change references graphql/graphql-js#368 where this was previously discussed (and then rejected).

The main argument for rejection in that issue seems to me to have been:

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.

In my opinion:

  • Because of __typename, you can always select fields down to scalar values" if we consider __typename a scalar field.

  • With regards to input objects, I don't see why passing an empty object would not be allowed. The following seems to parse fine:

    mutation {
      update(input: {}) {
        name
      }
    }

Allowing empty object/input object types has concrete use cases:

I'd be happy to submit a PR to simply remove this validation from the spec, as a strawman or proposal (edit: see #606).

victorandree added a commit to victorandree/graphql-spec that referenced this issue Aug 5, 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 added a commit to victorandree/graphql-spec that referenced this issue Aug 28, 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.
@vanga
Copy link

vanga commented Nov 9, 2019

These restrictions also apply to interfaces, this makes it really difficult to represent domain objects in OOP that leverage interfaces. It's pretty common to have interfaces with no properties defined.
So, I would add this to the list of use cases mentioned above.

@michaelstaib
Copy link
Member

@vanga for this you actually the right thing to do is to use union types. Interfaces with no properties are used in oop as marker interfaces due to the lack of unions. With unions interfaces with no fields are not necessary.

@vanga
Copy link

vanga commented Nov 25, 2019

Hi @michaelstaib, Thanks for the suggestion.

But, I am not sure if it solves the real issue here. Interface represents a hierarchical relation between entities where as union type just a wrapper type for a group of entities (sibling and independent entities?) IMHO. This hierarchical relationship can be of any length. And, one needn't have fields specified at all levels of hierarchy at a given point in time depending on how one chooses to represent their entities. Unfortunately this is the kind of data model which we have to represent using graphql. And, I do think that it's not weird to have such data models in practice.

Ex:
A -> B -> C -> D
A -> B -> E -> F

I understand that graphql doesn't yet have support for one interface inheriting from another (but there is a feature request for this IIRC which is WIP). We solve this a bit differently by having one type inherit from multiple types.

B inherits A
C inherits A & B
D inherits A & B & C

I can have a schema such a way that a query on A will consider data from all the children B, C, D by default without having to explicitly specify fragments, because consumers don't have to know which/what all children exist, which is the point of having an interface.

@michaelstaib
Copy link
Member

#373

@vanga the interface inheriting interface is not meant to introduce those hierarchies but make validating fragment selections possible. If you read through this you will notice that on each level the interface has to be implemented.

If you still think that this should be introduced I would suggest opening another issue and describing your use case. Each change should be caused by a distinct need for a use-case and described in its own issue.

@duarten
Copy link

duarten commented Dec 9, 2019

This makes sense to me too. Since errors are already injected in the response, an empty payload object can be a simple indicator of success. It's less confusing and wasteful than adding the workaround boolean field.

dariuszkuc added a commit to dariuszkuc/graphql-kotlin that referenced this issue Jan 9, 2020
GraphQL specification requires Query type to be present as it is required to run introspection query. Per specification it also shouldn't be possible to generate empty complex types (objects, input objects or interfaces) and they should expose at least a single field. Since root Query type is a special GraphQLObjectType it also has to expose at least a single field.

Breaking changes:
* at least single Query is required when generating the schema
* split `didGenerateQueryType` hook (and corresponding `Mutation` and `Subscription` hooks) into `didGenerateQueryFieldType` (previous functionality) and `didGenerateQueryObjectType` hooks to allow more granular control when generating the schema
* default `SchemaGeneratorHooks` now performs validation of generated object types (including special query, mutation and subscription types), input object types and interfaces to ensure we don't generate empty complex object types

see: graphql/graphql-spec#490 and graphql/graphql-spec#568
dariuszkuc added a commit to ExpediaGroup/graphql-kotlin that referenced this issue Jan 10, 2020
)

* BREAKING CHANGE: empty complex types should fail schema generation

GraphQL specification requires Query type to be present as it is required to run introspection query. Per specification it also shouldn't be possible to generate empty complex types (objects, input objects or interfaces) and they should expose at least a single field. Since root Query type is a special GraphQLObjectType it also has to expose at least a single field.

Breaking changes:
* at least single Query is required when generating the schema
* split `didGenerateQueryType` hook (and corresponding `Mutation` and `Subscription` hooks) into `didGenerateQueryFieldType` (previous functionality) and `didGenerateQueryObjectType` hooks to allow more granular control when generating the schema
* default `SchemaGeneratorHooks` now performs validation of generated object types (including special query, mutation and subscription types), input object types and interfaces to ensure we don't generate empty complex object types

see: graphql/graphql-spec#490 and graphql/graphql-spec#568
@n1ru4l
Copy link

n1ru4l commented May 19, 2020

I am currently using union types for modeling a process with multiple steps. I also hit the limitation that sometimes I do not need any field on an object type. I wanna share the use-case so it can be considered as an example for a possible RFC.

type Result {
  some: String!
  otherValue: Boolean!
}
type Error {
  code: String!
  message: String!
}

type AnalysisNotStarted {
  # noop field
  _: Boolean
}
type AnalysisEnqueued {
  # noop field
  _: Boolean
}
type AnalysisProcessing {
  # noop field
  _: Boolean
}
type AnalysisFinished {
  result: Result!
}
type AnalysisFailed {
  error: Error!
}

union Analysis = AnalysisNotStarted | AnalysisEnqueued | AnalysisProcessing | AnalysisFinished | AnalysisFailed

type Record {
  id: ID!
  analysis: Analysis!
}

@jdehaan
Copy link

jdehaan commented May 19, 2020

I cannot understand why you don't simply use an enum in the scenario above that fits 100% the needs or am I missing something?

While I can understand it makes sense to have empty "tagging" types in general, in this particular place it looks like a misuse to me.

@n1ru4l
Copy link

n1ru4l commented May 19, 2020

@jdehaan I don't want a nullable field. When leveraging type generation on the graphql api consumer e.g. by using relay or apollo-client (with apollo-codegen) you end up having to do an enum check + a null check. I think a union is the "cleaner" and more type-safe/"state-machine"-like approach.

This becomes "weirder" when having multiple analysis types:

type Error {
  message: String!
}

type Analysis1Result {
  foo: String!
}

type Analysis2Result {
  bar: String!
}

union Analysis1ErrorOrResult = Error | Analysis1Result
union Analysis2ErrorOrResult = Error | Analysis2Result

enum AnalysisStatus { not_started enqueued processing finished failed } 

type Record {
  id: ID!
  analysis1Status: AnalysisStatus!
  analysis1ErrorOrResult: Analysis1ErrorOrResult
  analysis2Status: AnalysisStatus!
  analysis2ErrorOrResult: Analysis1ErrorOrResult
}

vs

union Analysis1 = ... # see above (https://github.com/graphql/graphql-spec/issues/568#issuecomment-630633807)
union Analysis2 = ... # see above (https://github.com/graphql/graphql-spec/issues/568#issuecomment-630633807)

type Record {
  id: ID!
  analysis1: Analysis1!
  analysis2: Analysis2!
}

Also, it might be possible that additional metadata such as an enqueuedDate or startedProcessingDate on the AnalysisEnqueued AnalysisProcessing types at some time in the future. I think polluting the parent type with a lot of (prefixed) fields is also not that nice.

@jdehaan
Copy link

jdehaan commented May 19, 2020

Thanks for the elaboration. I can understand better now how it can make sense.

@n1ru4l
Copy link

n1ru4l commented May 19, 2020

@jdehaan We are actually doing the enum + nullable field approach right now and are realizing that it limits us in the future extensibility of the schema. While figuring out how to do better I found this issue and wanted to provide my input :)

@victorandree
Copy link

victorandree commented May 19, 2020

@n1ru4l Since there is no structural difference between your "ongoing" analysis types, I'd argue that the following is how you can structure your schema.

type Result {
  some: String!
  otherValue: Boolean!
}

type Error {
  code: String!
  message: String!
}

enum AnalysisOngoingStatus {
  NOT_STARTED
  ENQUEUED
  PROCESSING
}

type AnalysisOngoing {
  status: AnalysisOngoingStatus!
}

type AnlaysisFinished {
  result: Result!
}

type AnalysisFailed {
  error: Error!
}

union Analysis = AnalysisOngoing | AnlaysisFinished | AnalysisFailed

type Record {
  id: ID!
  analysis: Analysis!
}

Basically you only create new types when you've actually got different fields to select. I think this is a good schema in itself – if this were a final schema, all your "ongoing analysis" types are just a single type because they all have a single, common field.

The main drawback of this is, in my opinion, extensibility and schema evolution. If you've already defined (possibly empty) types, it's easier to add more fields to them than to introduce a new type. I personally think this is an interesting argument, but it could lead to very dubious "best practices" evolving where everything should be its own type.

FWIW, I was the author behind a PR that would have allowed this (#606). I closed it in response to Working Group feedback but there may be reasons to pick it up again.

@n1ru4l
Copy link

n1ru4l commented May 19, 2020

@victorandree Allowing this could even question the necessity of having enum types at all since you could do the same with Object types 🤔

@jdehaan
Copy link

jdehaan commented May 19, 2020

This is the case in normal programming languages as well. Types define in general a possible set of values (potentially very large) and there are isomorphic constructs. Which construct you take for implementation is more a question of expressiveness and ease of use.

The reason why I initially looked for this thread was that we have a fully dynamic GraphQL. I mean really fully dynamic. It is initially empty. Well because of the errors Type Query/Mutation must define one or more fields we had to introduce a dummy query & mutation...

One could argue in that case we could have deleted Query and Mutation types all together... But our toolkit forces a definition for the query type and mutation type.

@jdehaan
Copy link

jdehaan commented May 19, 2020

But also all empty types are isomorphic... So we would need only one in theory. It is a bit difficult to understand exactly what are the problems that people try to solve. Maybe tackling the discussion that way: list up the use cases and see what are pros & cons for such empty types or alternatives will help in showing up what it can be good for. My own use case is purely a pragmatic one, we can maybe forget about it...

Some people need more than one empty types (maybe unjustified?). What is then the underlying reason. Is it about a provisioning of names for future possible/probable extension? In order to have a more stable schema evolution in the future?

@sungam3r
Copy link
Contributor

Query with __typename will return different result after renaming/splitting types.

@m-rutter
Copy link

m-rutter commented Jul 7, 2020

But also all empty types are isomorphic... So we would need only one in theory. It is a bit difficult to understand exactly what are the problems that people try to solve. Maybe tackling the discussion that way: list up the use cases and see what are pros & cons for such empty types or alternatives will help in showing up what it can be good for. My own use case is purely a pragmatic one, we can maybe forget about it...

Some people need more than one empty types (maybe unjustified?). What is then the underlying reason. Is it about a provisioning of names for future possible/probable extension? In order to have a more stable schema evolution in the future?

These "empty types" are espically important in languages that support the concept of pattern matching and sum types/disjoint uions/discriminated unions/variants/tagged unions/whatever you want to call it. These are a features in scala, haskell, rust, swift, typescript, flow, modern c++(std::variant), and most modern and functional languages. These "empty" values as you describe them aren't actually empty. They can repersent completelty different states of the system. You don't need only one "empty" type, because each empty type is discriminated on its __typename, which is all these modern languages need in order to correctly model these unions as sum types.

C-style enums combined with nullable properties don't cut it in terms of ergonomics or type safety. It seems like a big oversight that graphql supports tagged union types but doesn’t seem to understand not all members of the union need to have additional associated data to be meaningful.

@alamothe
Copy link

The funniest thing is that some GraphQL implementations have this "check" in place, and some don't (since this is not technical limitation, but rather an arbitrary validity rule).

@HunderlineK
Copy link

The use case I have for this is modularizing my code. In different modules, I have extend type Query { ...} but I have to define Query { } somewhere and it cannot be left empty, so I've to add some arbitrary but redundant type there

@87maza
Copy link

87maza commented Jul 8, 2021

The use case I have for this is modularizing my code. In different modules, I have extend type Query { ...} but I have to define Query { } somewhere and it cannot be left empty, so I've to add some arbitrary but redundant type there

I have a similar issue but instead of an arbitrary type i just did type Query

type Query {}  // yields errors when i use a schema parser like graphql-codegen

type Query // seems to build fine and schema parser doesn't spew errors 

@rocketraman
Copy link

Two simple use cases:

  1. Similar to the OPs case, but I want to represent an empty Success type i.e.
union SomeMutationPayload = SomeMutationSuccess | SomeMutationError

type SomeMutationSuccess {}

type SomeMutationError {
  ...
}

The success type in this use case is sufficient to indicate success, and needs no additional fields. Yet GQL forces me to define something in there, even if it is useless and confusing.

  1. Assume a financial concept "Cash on Hand", calculated based on a current cash position and a certain rate of revenue and expenditure. The result of this can basically be either a finite duration, or not applicable / infinity (if revenue less expenditures is positive).

One approach could be:

type CashOnHand {
  duration: Duration!
}

union Duration = FiniteDuration | InfiniteDuration

type FiniteDuration {
  days: Int!
}

type InfiniteDuration {}

There is no point to adding any kind of data field to InfiniteDuration -- it is unnecessary and would be confusing. As far as I can tell, the best way to do this in GraphQL is to ignore the type system and use C-style "magic values" e.g. the client just has to know that -1 means "infinite duration" / "not applicable" based on comments/documentation:

type CashOnHand {
  duration: Duration!
}

type Duration = {
  # -1 here means infinite duration
  days: Int!
}

There is a lot of pushback in the rejected RFC #606 to representing ADTs as errors, but neither of these use cases have anything to do with errors. Unless I'm missing some idiomatic approach to solving these simple problems, this is a bit silly.

@upcFrost
Copy link

Mutations should (arguably) return the mutated object. But the Analysis example from #568 (comment) is actually a very valid case imo, especially when you're trying to describe events log.

Enum-based solution which was proposed in #568 (comment) is definitely not a great thing to implement, as it undermines the ability to add fields to those events which were merged into enum. Removing enum fields can be dangerous, especially if the client has some sort of local DB (very common for mobile dev).

dariuszkuc added a commit to dariuszkuc/graphql-kotlin that referenced this issue Aug 5, 2022
…xpediaGroup#541)

* BREAKING CHANGE: empty complex types should fail schema generation

GraphQL specification requires Query type to be present as it is required to run introspection query. Per specification it also shouldn't be possible to generate empty complex types (objects, input objects or interfaces) and they should expose at least a single field. Since root Query type is a special GraphQLObjectType it also has to expose at least a single field.

Breaking changes:
* at least single Query is required when generating the schema
* split `didGenerateQueryType` hook (and corresponding `Mutation` and `Subscription` hooks) into `didGenerateQueryFieldType` (previous functionality) and `didGenerateQueryObjectType` hooks to allow more granular control when generating the schema
* default `SchemaGeneratorHooks` now performs validation of generated object types (including special query, mutation and subscription types), input object types and interfaces to ensure we don't generate empty complex object types

see: graphql/graphql-spec#490 and graphql/graphql-spec#568
@brandonchinn178
Copy link

Completely agree with everything people have been saying here. I noticed one comment in the corresponding PR that was never explicitly addressed, and I want to comment on it here:

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.

#606 (comment)

One difference is you can nest Maybe, but you can't nest nullable fields. If a field has two notions of "optional", you have no way of distinguishing which "optional" notion a null represents (e.g. an optional field that can be marked "inherit" or "override as null").

Also, to add on to an earlier comment:

When leveraging type generation on the graphql api consumer e.g. by using relay or apollo-client (with apollo-codegen) you end up having to do an enum check + a null check.

#568 (comment)

This becomes even more apparent the more fields you have

# desired
type Foo = A | B
type A {}
type B { x: Int!; y: Int!; z: Int! }

# workaround
enum FooType = A | B
type Foo {
  type: FooType!
  x: Int
  y: Int
  z: Int
}
// desired
switch (data.__typename) {
  case "A": return 0
  case "B": return data.x + data.y + data.z
}

// workaround
switch (data.type) {
  case FooType.A: return 0
  case FooType.B: {
    const { x, y, z } = data
    if (x === null || y === null || z === null) {
      throw new Error("Should not happen")
    }
    return x + y + z
  }
}

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

Successfully merging a pull request may close this issue.