graphql / graphql-spec

GraphQL is a query language and execution engine tied to any backend service.
https://spec.graphql.org
14.31k stars 1.12k forks source link

The ability to represent empty objects #568

Open MOZGIII opened 5 years ago

MOZGIII commented 5 years ago

Based on this: https://github.com/graphql/graphql-js/issues/937#issuecomment-432857507

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

Type extension with extend type works already btw:

type Query

extend type Query {
  a: Boolean;
}
stubailo commented 5 years ago

Yes - I'm agreeing with you!

alamothe commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

For background, the requirement to have at least one field was introduced to the spec by https://github.com/graphql/graphql-spec/commit/059941486fcea9b93c5a156fe80df03d2021c0b4. This change references https://github.com/graphql/graphql-js/pull/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:

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

vanga commented 5 years ago

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 commented 4 years ago

@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 commented 4 years ago

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 commented 4 years ago

https://github.com/graphql/graphql-spec/pull/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 commented 4 years ago

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.

n1ru4l commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

@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 commented 4 years ago

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

n1ru4l commented 4 years ago

@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 commented 4 years ago

@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 commented 4 years ago

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

jdehaan commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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

m-rutter commented 4 years ago

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 commented 4 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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.

2) 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 https://github.com/graphql/graphql-spec/pull/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 commented 3 years ago

Mutations should (arguably) return the mutated object. But the Analysis example from https://github.com/graphql/graphql-spec/issues/568#issuecomment-630660616 is actually a very valid case imo, especially when you're trying to describe events log.

Enum-based solution which was proposed in https://github.com/graphql/graphql-spec/issues/568#issuecomment-630676300 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).

brandonchinn178 commented 1 year ago

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.

https://github.com/graphql/graphql-spec/pull/606#issuecomment-540544237

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.

https://github.com/graphql/graphql-spec/issues/568#issuecomment-630660616

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
  }
}