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

Feature request: support for composite Input Types #179

Closed
GreenAsJade opened this issue Oct 18, 2016 · 12 comments
Closed

Feature request: support for composite Input Types #179

GreenAsJade opened this issue Oct 18, 2016 · 12 comments

Comments

@GreenAsJade
Copy link

GreenAsJade commented Oct 18, 2016

Most of my query types need corresponding input types.

This is a lot of duplication of information to type into a schema.

makeExecutableSchema could support generation of InputTypes automatically with trivial extension to the input syntax.

instead of

type IntermediateThing {
    id: Int!
    Description: String!
 }

type  CompositeThing {
   id: Int!
   leftThing: IntermediateThing!
   rightThing: IntermediateThing!
   thingStatus: String!
}

input IntermediateThingInput {
   Description: String!
}

input CompositeThingInput {
   leftThing: IntermediateThingInput!
   rightThing: IntermediateThingInput!
}

We could say

entity IntermediateThing {
   (id: Int!)
   Description: String!
}

entity CompositeThing {
   (id: Int!)
   leftThing: IntermediateThing!
   rightThing: IntermediateThing!
   (thingStatus: String!)     //  < parens indicate this is server-side computation, not for InputType
}

and makeExecutableSchema could provide both the query and input schema elements that we need.

@GreenAsJade
Copy link
Author

Note: if this isn't crazy, has a chance of being accepted, I'd be happy to look at implementing. Would like to know if it has legs first.

@stubailo
Copy link
Contributor

I don't think we'll accept something that isn't part of the "official" GraphQL schema language at the moment, especially if it can't be parsed with GraphQL.js.

I'm not really sure if auto-generating input types is a good idea, because then whenever you add a field to an object type you still need to decide if it also makes sense to add it to the input, and possibly update the method to insert it?

One thing that could be interesting is extending the type generation with custom directives. So maybe graphql-tools can work with custom plugins which get the directives as an argument?

type IntermediateThing @input {
  id: Int!
  description: String! @input
}

type CompositeThing @input {
  id: Int!
  leftThing: IntermediateThing @input
  thingStatus: String!
}

A few things to keep in mind:

  1. Fields shared between input and output types won't be able to take arguments, or use union or interface types
  2. I think not being included should be the default, which will make adding fields to the output type safer

My main issue with this is that it makes it harder for graphql-tools to use standard tools like buildASTSchema, and deviates from the commonly used schema language syntax.

I'm curious, does duplication of these fields cause a maintenance burden, or it's just more to type up front?

@GreenAsJade
Copy link
Author

GreenAsJade commented Oct 18, 2016

Thanks for the feedback.

I'm concerned about both typing up front (though that is actually just cut and paste of most of the body) and maintenance. Also readability of the schema (high noise level due to unnecessary repetition).

I will think about a suggestion that would be parseable, and also able to support arguments (which is important) not interfaces or unions (which so far seem to be rarer and acceptable to require all the typing).

@stubailo
Copy link
Contributor

also able to support arguments (which is important)

I'm saying that input types literally don't have arguments in GraphQL, so fields will either only be shared if they have no arguments, or perhaps the argument list can just be removed. But then it seems kind of weird to share it.

interfaces or unions

Same with those - input types can't use interfaces or unions in any of the field types.

@GreenAsJade
Copy link
Author

I suspect that this deals with this issue. However, at first look it is rather more heavyweight than a nice solution for this point problem might be.

@advance512
Copy link

advance512 commented Mar 10, 2017

Conceptually, every field of a type can be always settable, settable on creation, settable on update and never settable.

e.g.
When you create a user, you'll supply email and password.
When you read the user, you'll get the email and server-generated userNumber and server-generated language for your app.
When you update the user, you can only modify the password and the language.

email: settable on creation
password: always settable
language: settable on update
userNumber: never settable

This, unfortuantely, forces you to create the following schema:

input UserCreate {
  email: String!
  password: String!
}

input UserUpdate {
  password: String
  language: String
}

type User {
  userNumber: Number
  email: String
  password: String
  language: String
}

Sure, for such a small schema it isn't a big issue. However, you typically have tens of more objects, perhaps even containing other objects, and in practice it gets real complex, real fast - keeping all the types and inputs in sync.

input UserCreate {
  email: String!
  password: String!
}

input UserUpdate {
  password: String
  settings: UserSettingsUpdate
}

input UserSettingsUpdate {
  language: String
}

type UserSettings {
  language: String
}

type User {
  userNumber: Number
  email: String
  password: String
  settings: UserSettings
}

And so forth. I think that using directives is definitely the way to go.

type UserSettings {
  @inputNames(UserSettingsCreate, UserSettingsUpdate)
  language: String @update
}

type User {
  @inputNames(UserCreate, UserUpdate)
  userNumber: Number
  email: String @create
  password: String @create @update
  settings: UserSettings @update
}

UserSettingsCreate won't be generated (or it can be, but it won't be used anywhere in the schema).
This is a LOT easier. You can then user UserCreate, UserUpdate, UserSettingsUpdate in mutations.

Note that this can also be done to a schema prior to passing it forward to makeExecutableSchema, or perhaps a flag parameter can decide whether to handle or ignore just directives.

--

OpenAPI attempted to solved this issue with readonly and required.
Here's an interesting discussion about it:
OAI/OpenAPI-Specification#228

Normal property - creation: optional, update: optional, read: optional
readonly property - creation: never, update: never, read: optional
required property - creation: always, update: always, read: always
required readonly - creation: never, update: never, read: always

Having return object schema validation helps for server debugging, and is disabled in production.
Note that GraphQL makes thing simpler, since types are intrinsically output objects, so they always include server-generated properties, so @create and @update are basically modifiers of the inputs, only - no modification of output.

@advance512
Copy link

advance512 commented Mar 10, 2017

Oh, also.

To improve readability, you could also be a bit more explicit:

type UserSettings {
  @inputNames(UserSettingsCreate, UserSettingsUpdate)
  language: String @update
}

type User {
  @inputNames(UserCreate, UserUpdate)
  userNumber: Number
  email: String @create
  password: String @create @update
  settings: UserSettings @update
}

input UserCreate {
  @fromType(User, true)
}

input UserUpdate {
  @fromType(User, false)
}

input UserSettingsUpdate {
  @fromType(UserSettings, false)
}

This is way more readable and less error-prone than the existing copy/paste/modify method.

As for typing. There is a bit of parsing here: you'll need to be able to handle multiline field definitions, ignore fields not marked with @update or @create, and replace non-scalars with their @inputNames(create, update)-marked inputs. But, no need (as far as I can see) to do complete typing.

It's more like a C preprocessor. :)

@helfer
Copy link
Contributor

helfer commented Mar 21, 2017

Going to close this since it's something that would have to be added to graphql-js first, but feel free to keep the discussion going!

@helfer helfer closed this as completed Mar 21, 2017
@advance512
Copy link

Why would graphql-js need to add this first?
Not sure I got that..

@outkine
Copy link

outkine commented Nov 27, 2017

Any update on this? Have you seen any projects that tried to implement this feature?

@kbrandwijk
Copy link
Contributor

kbrandwijk commented Nov 27, 2017

@Jetmate As this isn't supported by graphql-js directly, I have put this on the todo list for my GraphQL Gateway Framework, Qewl, along with a couple of other things on top of the spec, like namespacing and imports. I will update here when it's in.

@outkine
Copy link

outkine commented Nov 27, 2017

@kbrandwijk Thanks! I'd love to see this feature implemented.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants