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

Proposal: Implicitly include properties of implemented interfaces #533

Open kaitlynbrown opened 5 years ago

kaitlynbrown commented 5 years ago

If an interface contains more than a few properties and is used in multiple places, you end up with a very significant amount of repetition that clutters up your schema. This means it requires a lot more scrolling to move around the schema, and the important details of a type (eg which properties are unique to it) are obscured by the noise added by all of the extra repetition.

As a contrived example, take the following currently legal schema, similar in structure to what I'm currently working on:

interface Vehicle {
  id: ID!
  make: VehicleMake
  wheelCount: Int
  acceleration: Float
  topSpeed: Float
  mass: Float
  capacity: Int
}

type Bicycle implements Vehicle {
  id: ID!
  make: BicycleMake
  wheelCount: Int
  acceleration: Float
  topSpeed: Float
  mass: Float
  capacity: Int
  gearCount: Int
}

interface Automobile implements Vehicle {
  id: ID!
  make: AutoMake
  wheelCount: Int
  acceleration: Float
  topSpeed: Float
  mass: Float
  capacity: Int
  engineCylinders: Int
  engineSize: Float
}

type Car implements Automobile {
  id: ID!
  make: CarMake
  wheelCount: Int
  acceleration: Float
  topSpeed: Float
  mass: Float
  capacity: Int
  engineCylinders: Int
  engineSize: Float
  trunkSize: Float
}

type Motorcycle implements Automobile {
  id: ID!
  make: MotorcycleMake
  class: MotorcycleClass
  wheelCount: Int
  acceleration: Float
  topSpeed: Float
  mass: Float
  capacity: Int
  engineCylinders: Int
  engineSize: Float
}

type Truck implements Automobile {
  id: ID!
  make: TruckMake
  wheelCount: Int
  acceleration: Float
  topSpeed: Float
  mass: Float
  capacity: Int
  engineCylinders: Int
  engineSize: Float
  maxLoad: Float
}

union AutoMake = CarMake | TruckMake | MotorcycleMake
union VehicleMake = AutoMake | BicycleMake

I propose that the following would be valid, and equivalent to the above:

interface Vehicle {
  id: ID!
  make: VehicleMake
  wheelCount: Int
  acceleration: Float
  topSpeed: Float
  mass: Float
  capacity: Int
}

type Bicycle implements Vehicle {
  make: BicycleMake
  gearCount: Int
}

interface Automobile implements Vehicle {
  make: AutoMake
  engineCylinders: Int
  engineSize: Float
}

type Car implements Automobile {
  make: CarMake
  trunkSize: Float
}

type Motorcycle implements Automobile {
  make: MotorcycleMake
  class: MotorcycleClass
}

type Truck implements Automobile {
  make: TruckMake
  maxLoad: Float
}

union AutoMake = CarMake | TruckMake | MotorcycleMake
union VehicleMake = AutoMake | BicycleMake

as you can see, even with this relatively simple example, we would be able to cut the number of lines of code in half. Instead of spanning multiple screens, it can now easily fit on a single screen. Each type clearly highlights the attributes which are unique to that type, and if one wants to know what other properties are included, one can easily glance up to see them (or examine the type in GraphiQL).

Furthermore, say I want to add another property to Vehicle, like say color. The way it currently is, that means I have to now remember to also add the property to Bicycle, Automobile, Car, Motorcycle and Truck. When I want to change something, the fewer places I have to make a change (and the fewer places I have to remember to change), the better. With the proposed change, I would only need to add the color property in one place to have it added to all Vehicles

mjmahone commented 5 years ago

For a single server schema, I'm not yet convinced this adds a lot of value: you still need to actually implement each field on each concrete type (unless you're using inheritance and a language that supports that, I guess). I also personally find it valuable to be able to look up a schema definition and immediately see all the fields on it. That may be solvable in tooling like GraphiQL, though.

However I can understand this being painful when you're coordinating multiple schema definitions (such as client-side type extensions): in this case you can end up in "schema deadlock" with the current validation rules. As an example, let's look at an evolution of a schema with both server and client definitions.

Server schema:

interface Pet {
  name: String
}
type Dog implements Pet {
  name: String
}
type Cat implements Pet {
  name: String
}

Client schema

extend interface Pet {
  sound: String
}
extend type Cat  {
  sound: String
}
extend type Dog {
  sound: String
}

If we add a new type on the server:

type Ferret implements Pet {
  name: String
}

the client schema extensions are now invalid, because there is no client extension (yet) for Ferret. But we made a compatible change, based on our current validation rules.

I'm not sure the best way to solve this. I'm solving it for my project by allowing interface extensions to automatically extend concrete types. But I don't know if there's another way to do it, or whether that should extend to interface definitions, too.

Which is a long-winded way of saying: there is probably value in someone championing a change in this space.

dancon commented 5 years ago

I really hope the proposal can be supported.

Interfaces are very useful to make our code reusable as much as possible, In some other type languages it indeed do work, but in GraphQL schema language, interface is useless to make it, I have to repeat the fields that have already declared in an Interface again in a type that implement the Interface, that make no sense!!

mdecurtins commented 5 years ago

I agree that this should be supported in some fashion.

Personally, I'm not very convinced by the argument in https://github.com/graphql/graphql-spec/issues/500 that allowing field inheritance from implemented interfaces reduces schema readability in favor of being DRY. GraphQL has quite a few client-side developer tools, such as GraphiQL and Playground, that make it very easy to see what the schema looks like. There's no reason I can think of that these tools shouldn't be able to stitch together the schema documentation for a type from its fields and the fields of any interfaces it implements. (I assume there's nothing inherent to introspection that would prevent this.) I don't think this would degrade readability for GraphQL server developers/maintainers, since almost any developer should be familiar with inheritance, and many IDEs are smart enough to be able to help with inheritance-related issues. (It would be especially nice if IDEs could parse SDL on the fly and provide intelligent hinting and code completion.)

I come from a Java background and am now working on a GraphQL API with a NodeJS back-end, and some of these typing/polymorphism issues are a real drag that I can see becoming a problem for future scalability and maintainability.

voxpelli commented 4 years ago

I'm 👍 on this. To get some real world experience with an approach like this I created a proof of concept of it some years ago: https://github.com/Sydsvenskan/node-graphql-partials (In response to https://github.com/graphql/graphql-js/issues/703#issuecomment-315776310)

To make a change like this backwards compatible, it introduced two new keywords: using and partial

(I guess one could argue that maybe partial could be seen as unnecessary and that using could use interfaces as well. This was implemented before https://github.com/graphql/graphql-spec/pull/373, so current design of it also solved the fact that interfaces couldn't implement other interfaces themselves)

Examples can be found here: https://github.com/Sydsvenskan/node-graphql-partials/tree/master/examples

To me, this makes the difference between a manageable and readable schema and a completely unwield one.

benjdlambert commented 4 years ago

My thoughts are that we solved this when querying graphql client side by using fragments and spreading, surely there's some solution for the backend schema too?

martinbonnin commented 3 years ago

+1 to this. The tooling is improving regularly and can help with this.

Also the argument that SDL should favor reading reading over writing could also be turned the other way around: reading only new fields (or changed fields in the covariant field case) allows to understand quickly what the differences are and what is characteristic of a specific type.

NavidMitchell commented 2 years ago

Without this inheritance does not allow for DRY. I am completely dumb founded that this is not supported 4 years later. What's the barrier to implementing this? I would gladly contribute time to helping implement this if the community will allow it.

micchickenburger commented 2 years ago

I completely agree. Looking at these other issues as well I think the people have spoken. I think that conciseness is key to readability and explicit repetition of fields is not concise.

If we have no champion for this proposal I am happy to volunteer myself as well.

benjie commented 2 years ago

@micchickenburger It's great that you'd like to get involved with the working group! I recommend that you familiarize yourself with the existing RFC, especially the editor rebuttals, the PR where it was added, and if possible have a look through where this may have been discussed at previous working groups (see the agendas to see where the topic was mentioned, and then read the accompanying notes). As Lee notes in the PR linked before:

You're absolutely welcome to join the next working group meeting. Proposals typically only move forward when their champions bring them to WG meetings to work through the details.

If you'd like to continue to champion this idea, please add yourself and the topic to an upcoming working group agenda:

https://github.com/graphql/graphql-wg/tree/main/agendas/2022

If you need any guidance, feel free to reach out to me here or in the #wg channel in https://discord.graphql.org (I'm @benjie there too).

rivantsov commented 2 years ago

while I am all for it (not repeating fields in implementor interface), my biggest concern is backward compatibility; I just don't see how it can be done in backward compatible manner. Adding partial or using keywords (instead of 'implements' I assume), seems quite confusing for a new-comer.