graphql-spec icon indicating copy to clipboard operation
graphql-spec copied to clipboard

GraphQL can be less strict and complicated

Open moshest opened this issue 2 years ago • 11 comments

I'm a software engineer for almost 20 years and I recently started using GraphQL extensively.

While I understand the original thoughts of making GraphQL as simple as possible, we end up with a language that's too complicated to handle taking too much control out from the user.

For instance:

  1. Object type seems redundant, we can just use interfaces instead:
interface Query {
  tasks: [Task!]!
}

interface Task {
  name: String!
  done: Boolean
}
  1. input types are just making everything more complicated and harder to share the same interfaces between inputs and outputs. It's better to define a workaround for field input with arguments than making everything duplicated with input types (Ie. ignore fields with arguments on inputs or allow a way to provide field arguments on input queries as well).
interface Mutation {
  addTask(data: TaskData!): Task!
}

interface TaskData {
  name: String!
}

interface Task implements TaskData {
  name: String!
  done: Boolean!
}
  1. In most of my schemas, I end up make more fields as required than I have optional fields. It will be much better to reverse the type system and allow mark only the optional fields instead:
interface Query {
  tasks: [Task]
}

interface Task {
  name: String
  done: Boolean?
}
  1. Adding recursion support for queries: Same as SQL let you run queries that can overwhelm the server, I think GraphQL should enable the same and let the user handle the consequences. Security concerns should only apply on the server implementation and not restrict language abilities.
interface TreeNode {
  label: String
  children: [TreeNode]
}

query {
  getTree {
    ...Node
  }
}

fragment Node on TreeNode {
  label
  children {
    ...TreeNode @RecursionLimit(100)
  }
}

I understand that this may not be the best place to communicate my thoughts but all of those points are shared across the internet with many StackOverflow and questions from frustrated users. If at least a few more people will see and agree with me then we can start pushing for a change in the proper channels.

We love GraphQL, we just want to make it better ❤️

moshest avatar Feb 27 '22 10:02 moshest

regarding recursive refs in fragments, this is under discussion:

https://github.com/graphql/graphql-spec/issues/929

by the way, in the sample, did you mean "...Node" as fragment spread?

fragment Node on TreeNode {
  label
  children {
    ...Node @RecursionLimit(100)
  }
}

rivantsov avatar Feb 27 '22 15:02 rivantsov

Using interfaces instead of 'Object types' - interesting approach, I think it works already as-is, and for backward compatibility we have to keep 'type' forever. abandoning input types - I doubt it would work, because of the problems of fields with parameters cannot be used in 'inputs' or 'input interfaces'. I think a very good improvement would be to allow input types as return types (Object types). If working with interfaces - server can just allow only interfaces as inputs that are without such parameters fields, without special designation of them as 'input interfaces', and report an error if violated

rivantsov avatar Feb 27 '22 15:02 rivantsov

Here's some reasons why input (InputObject) and output object (Object) types probably shouldn't be the same thing:

  • Nullability: Changing an Object's field to be non-nullable is a non-breaking change, changing it to be nullable is a breaking change. Conversely, making an InputObject's field nullable is a non-breaking change, but making it non-nullable is a breaking change. Combine these two together (because the type is now both input and output) and you can no-longer change the nullability of the fields, which limits schema evolution options.
  • Field addition: Adding any field to an Object type is a non-breaking change (because GraphQL makes you explicitly state which fields you need, so a new field will not affect any existing queries). Adding a nullable field to an InputObject type is a non-breaking change (a non-nullable field would be a breaking change because it would make all previous operations using this InputObject invalid since this field will not have been supplied). Therefore you'd only be able to add nullable fields to the GraphQL schema, limiting schema evolution options.
  • Symmetry: If you use the same type for input as output then a user might expect to be able to supply an Object that they queried back as input to a future operation; however this may or may not be valid depending on what their selection set was, and as the schema evolves it all but guarantees that people doing this will lose data.
  • Asymmetry: Whereas output objects link to many other types, input objects much more rarely want inputs for these links, so when a link like this wants to be added to an existing type that's used in both input and output much thought will have to be given to whether or not to add the relating field (since it will also affect inputs). This again limits schema evolution options.
  • Cycles: It's perfectly valid for an Object to state that it references itself in a non-nullable way, e.g. type Query { query: Query! }, however a type like this cannot be valid for input due to cycles. Now consider that this cycle might not be between the type and itself but between any types - now you're requiring that the type tree is a directed acyclic graph which seriously limits the usefulness of the type system (e.g. a Post can have an Author, but the Author is not allowed to link back to Posts because that would be a cycle)

I'm sure we can come up with solutions to all these issues (and more) by adding complexity to GraphQL - e.g. making the nullability of a field dependent on whether it's input/output, making some fields input-only or output-only, etc - but the current solution is also the simple solution, i.e. that input types and output types are inherently separate because their concerns do not align.

benjie avatar Mar 01 '22 08:03 benjie

  • Cycles: It's perfectly valid for an Object to state that it references itself in a non-nullable way, e.g. type Query { query: Query! }, however a type like this cannot be valid for input due to cycles.

Consider these examples:

# This is fine.
type Query { a: A! }
type A { b: B! }
type B { a: A! name: String! }
query {a{b{a{b{a{b{
 name # cycle is broken
}}}}}}}

However, this schema can't be queried legally:

# this cycle can't be broken and should be illegal
# there is no valid way to query it.
type Query { a: A! }
type A { b: B! }
type B { a: A! }
query {a{b{a{b{a{b{
 a # illegal
}}}}}}}

The spec allows this construct but it really shouldn't. Probably a topic for another issue though.

romshark avatar Sep 14 '22 14:09 romshark

it's illegal not because of loop of non-null references, but because there are no LEAF type fields!; if you make fields a and b nullable, you still won't be able to construct the query, because there's no field that can end the query!

rivantsov avatar Sep 14 '22 14:09 rivantsov

it's illegal not because of loop of non-null references, but because there are no LEAF type fields!; if you make fields a and b nullable, you still won't be able to construct the query, because there's no field that can end the query!

That's correct.

What about the input types? I still haven't found the paragraph that explicitly forbids cyclic dependency in input types.

romshark avatar Sep 14 '22 14:09 romshark

Your query can be legal, you just need to provide a selection set since a is not a leaf. Here's a valid query:

query {a{b{a{b{a{b{
 a { __typename }
}}}}}}}

benjie avatar Sep 14 '22 14:09 benjie

What about the input types? I still haven't found the paragraph that explicitly forbids cyclic dependency in input types.

It's in the "Input Objects" section under the heading "Circular References":

https://spec.graphql.org/draft/#sec-Input-Objects.Circular-References

benjie avatar Sep 14 '22 14:09 benjie

So... There are three glaringly obvious reasons why input and output types should not be distinct:

  1. The partitioning means that data cannot be forwarded. A service cannot accept an input object and then turn around and pass it along as an output object without rewriting the darned thing into a (frequently identical) output object. As an example of why this is important, consider message queues. Which bear a strong resemblance to subscriptions...
  2. The partitioning forces a lot of replicated code at every layer of the software stack. Consider, as an incomplete illustration, the complexity of mapping an existing SQL schema to a corresponding GraphQL schema (and, of course, back, since what gets read eventually tends to get written).
  3. From a formal type system perspective, directionality of I/O is completely orthogonal to type. Formally speaking, the partition of input and output types makes the GraphQL type system unsound (I'm using "unsound" here in a strictly technical mathematical sense, not as a value judgement).

If there is a credible motivating explanation for this division, I haven't seen it. I've looked pretty hard, and if somebody can point me the right way I'd be grateful. The issues that @benjie identifies aren't things that a protocol can fix - they are fundamental to the evolution of the underlying information architecture. I really do understand why being able to rapidly evolve a protocol without going through protocol versioning hell is very appealing. The approach taken by Kenton Vardy with protocol buffers and CapnProto accomplishes a lot of the same goals without breaking the concept of types. In effect, they distinguish usefully between the static and the dynamic type of the object without giving up either.

The separation also adds obscurity to a more fundamental issue that arises from the interaction between the query language and the client cache merging strategy: inconsistency.

Suppose I do a query that requests fields A and B of some object. Later, I do a second query that accepts fields D and C of that same object. There is no reason to believe - and no way to check - that I now have a consistent version of the object in my client cache. It's entirely possible that there have been 85 mutations of the underlying object in the persistent store between these two queries, and that one or more of them modified the A field or the B field. If so, my cached copy on the client now blends two different versions of the object. The very last thing you want to do in this case is send that object back as an update.

This is one of the issues that ultimately made me put GraphQL down. A potential "fix" here, if you want one, is to ensure that all object [fragments] have a version number (or a lastStored field) whether you asked for it or not, and that the version number gets passed in both directions. This wouldn't significantly impeded the ability to expose a third party API through a GraphQL interface. A more extreme approach would make queries persistent, in effect creating a subscription on the server. The challenge with that is that it would require a significant amount of per-client server state, which makes service replication for scaling much harder. Ultimately I suspect this would run fairly hard into some of the scaling issues that Meteor pub/sub ran into.

The other issue that drove me away is that field-level selectivity and statically typed programming languages really play badly together. If all of your client targets are some variant of a browser (potentially including things like electron), great. But a GraphQL API is quite difficult to invoke from, say, C#. In my opinion, GraphQL fits the browser-based client niche extremely well, but there's more to the world than browsers. Which, ironically, is one of the reasons Geoff Schmidt left Meteor to found Apollo GraphQL. As an example, Shopify's adoption of a GraphQL-based API made life very dramatically harder for a large number of clients written in C#.

We've been building APIs since the earliest days of networking - the first API specification language I leraned came from Apollo Computer more than forty years ago. I'm not a fan of discarding new ideas without a clear reason, but skepticism is healthy. GraphQL's type partitioning idea has many strikes against it. The biggest one, from my perspective, is that it is mathematically unsound.

jsshapiro avatar Feb 22 '24 16:02 jsshapiro

@jsshapiro , agree 100%. Or maybe 99. I tried to push these issues but gave up, it's no use. The guys who make decisions there are absolutely against ANY change. One question, why are you saying consuming graphql from c# is a trouble/problem? GraphQL client (the one that I have), returns c# types/objects which are partially filled, depending on the query. Yes, this might be inconsistent depending on how you define types' internal consistency rules, but if you accept this as a basic premise - that the returned object fields (any of them) might be just defaults (because they were not listed in query) - then you can go with it just fine. I have a sizable app, built in web assembly Blazor, and the GraphQL client is a c# component, and all client logic around it is c#. It works great - except occasional situation like "why this new field/control is empty?! - I see value in the database... ah, I forgot to add it to the query".

rivantsov avatar Feb 23 '24 15:02 rivantsov

ah, and about input types and mutations - in my app. There are around 40 types to be loaded/updated, so we recognized a problem from the start - we didn't want to define extra 40+ input types and mutations, so we came up with a concept of 'one mutation', using one update endpoint, when you send a list of update packs, each is ObjectType and list of field-newValue pairs. we integrated it with server-side ORM, using mapping to entities which is already there for querying. So now adding a type does NOT need any extra input types and new mutation endpoints. I think this might be actual solution for this input types conundrum; hell with the spec, we have a nice workaround. (by the way, building an update pack from input form - fully automatic, c# code running thru model and comparing field values)

rivantsov avatar Feb 23 '24 15:02 rivantsov