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

[Feat] Nested mutations #207

Merged
merged 3 commits into from
Aug 7, 2018
Merged

[Feat] Nested mutations #207

merged 3 commits into from
Aug 7, 2018

Conversation

spawnia
Copy link
Collaborator

@spawnia spawnia commented Jul 17, 2018

Related Issue(s)

Resolves #233 #208 #131

PR Type

Feature

Changes

This introduces the option for crud mutations to contain data for multiple models in one single mutation.

I used some ideas from https://www.prisma.io/docs/reference/prisma-api/mutations-ol0yuoz6go/#nested-mutations and they way they are doing nested mutations, and tried to combine that with the way Eloquent models work.

Example Schema

type User {
  id: ID!
  email: String!
  posts: [Post!] @hasMany
}

type Post {
  id: ID!
  title: String!
  author: User! @belongsTo
}

type Mutation {
  createUser(input: CreateUserInput!): User @create(flatten: true)
  createPost(input: CreatePostInput!): Post @create(flatten: true)
  updateUser(input: UpdateUserInput!): User @update(flatten: true)
  updatePost(input: UpdatePostInput): Post @update(flatten: true)
  deleteUser(id: ID!): User @delete
  deletePost(id: ID!): Post @delete
}

input CreateUserInput {
  email: String!
  posts: CreatePostRelation
}

input CreatePostRelation {
  create: [CreatePostInput!]
}

input CreatePostInput {
  title: String!
  author: ID
}

input UpdateUserInput {
  id: ID
  email: String
  posts: UpdatePostRelation
}

input UpdatePostRelation{
  create: [CreatePostInput!]
  update: [UpdatePostInput!]
  delete: [ID!]
}

input UpdatePostInput {
  id: ID
  title: String!
  author: ID
}

Example Create

mutation {
  createUser(
    input: {
      email: "adf@asdf.de"
      posts: {
        create: [
          { title: "post a" }
          { title: "post b" }
        ]
      }
    }
  ) {
    id
  }
}

Example Update

mutation {
  updateUser(
    input: {
	  id: 2
      email: "adf@asdf.de"
      posts: {
        create: [
          { title: "post c" }
        ]
        update: [
          { id: 2, title: "post cysadf" }
        ]
        delete: [ 23, 4 ]
      }
    }
  ) {
    id
  }
}

@olivernybroe
Copy link
Collaborator

I like the concept a lot. This is really something I would find useful, and I actually think this should belong in Lighthouse, not as a package for itself.

How much of it would we be able to autogenerate? Can you add a schema example for the generated and non generated?

@spawnia
Copy link
Collaborator Author

spawnia commented Jul 17, 2018

@olivernybroe Right now for my personal use case i auto-generated the whole schema from a database schema, which i also used to generate my models.

I like the approach that @chrissm79 outlined here, where the only required schema definition is the Object Type, and all the Queries, Mutations and InputObjectTypes are then generated from that.

As a first step, i would try to form the basic building blocks of nested mutations, and then build further abstraction on top.

@chrissm79
Copy link
Contributor

@spawnia Looks like a great start, pretty excited about this one!!! Quick question, are you planning on using a Schema Manipulator directive (i.e., using a new @crud directive or adding a boolean argument to the @model directive) that would be defined on the User and Post types?

@spawnia
Copy link
Collaborator Author

spawnia commented Jul 17, 2018

Tbh i am not all that happy with how @model does automatically make the type a relay node. I would like to move that to a more specific directive name in the future, and couple the @model directive just with Eloquent functionality.

The @crud directive might be a nice way to go regardless, since it pretty clearly indicates a lot of magic happening. Let's see how configurable we need it to be, i would go with an opionionated approach at first and if use cases pop up possibly expand from there.

@maarten00
Copy link
Contributor

maarten00 commented Jul 27, 2018

This would be great. I was looking to implement exactly this schema into my own project, but ran into the same issue as described in #208. This allows for much cleaner syntax in mutations and potentially code reuse, especially for complex, multi-model mutations.

My schema is as follows:

type Club {
    id: ID!
    name: String!
    description: String
    abbreviation: String
    founding_date: String
    dissolving_date: String
    federation_reference_number: String
    federation: Federation
    district: District
    members: [ClubMember]!
    created_at: DateTime!
    updated_at: DateTime
}

type Mutation{
    createClub(federation_id: ID!, district_id: ID!, input: createClubInput!): Club @create(model: "Club")
}

input createClubInput {
    name: String!
    description: String
    abbreviation: String
    founding_date: String
    dissolving_date: String
    federation_reference_number: String
}

I took the liberty of pulling in your changes and testing it with my schema, it works brilliantly. I would be really excited if these new directives could be included in the 2.2 release!

@spawnia
Copy link
Collaborator Author

spawnia commented Jul 27, 2018

So, after discussing this with @olivernybroe my plan is to include this as an experimental feature of v2.2

We can kill two birds with one stone and solve #233 #208 as well. I plan to add an argument to the create directive, like @create(flatten: true) which unpacks the first argument and uses that to create the model.

How about transactions? I was thinking to always wrap the whole mutation in a transaction, since a failure in one of the nested mutations should cause the mutation to fail atomically.

@maarten00
Copy link
Contributor

maarten00 commented Jul 27, 2018

Edit: Issue moved to #234

@spawnia
Copy link
Collaborator Author

spawnia commented Jul 27, 2018

@maarten00 I ran into this issue before, it is related to how we handle arguments/input objects. As you noticed, it works fine but it prevents validation of the schema, which i find to be an essential feature as part of a CI pipeline.

Could you open a seperate issue for that?

What do you think about my previous comment?

@maarten00
Copy link
Contributor

Well actually, I ran into the issue when using the code from this PR, so I don't think it should be in a seperate issue, right? Unless you're not running into this problem with your own tests, could you verify that?

As for your previous comment;
I'm not sure what the @flatten is supposed to fix/change as opposed to the behavior in your current proposal. Could you explain further?

I think transactions are a great idea, no issue there.

@spawnia
Copy link
Collaborator Author

spawnia commented Jul 27, 2018

I want to make nested mutation the default and integrate it with the previous @create directive. As of now, it expects flat field arguments, and i want to avoid breaking that.
So, the flatten flag makes it so that Lighthouse expects a single InputObject instead. It will default to false now, but might be changed to default to true in the next major version.

@spawnia
Copy link
Collaborator Author

spawnia commented Jul 27, 2018

Well actually, I ran into the issue when using the code from this PR

It came up in a bunch of other cases, i think it has to do with the missing distinction of InputObjectValues from Fields. So i think it is more closely related to #184 , especially since i have not done anything in regards to how arguments are generally handled in this PR.

@maarten00
Copy link
Contributor

It came up in a bunch of other cases, i think it has to do with the missing distinction of InputObjectValues from Fields. So i think it is more closely related to #184 , especially since i have not done anything in regards to how arguments are generally handled in this PR.

Ah right, I hadn't realized it was not specifically the new commit causing that but the Input type itself. I've added a new issue: #234

I want to make nested mutation the default and integrate it with the previous @create directive

Ah I see. That sounds like a good idea, but maybe a little bit of flexibility would be nice. In my example above I want to pass the required (relation) ID fields without the input type. Maybe like this:

type Mutation{
    createClub(federation_id: ID!, district_id: ID!, input: createClubInput!): Club @create(model: "Club", flatten: "input")
}

Maybe you could even allow passing an array or comma-seperated values to flatten multiple arguments?

@spawnia
Copy link
Collaborator Author

spawnia commented Jul 27, 2018

Seperating out the args like in your example does indeed make flattening way more complicated. It also reduces the reusability of your input type, considering you may use it as a child input of another nested mutation. You can still require the args in the single Input, should that be your concern.

The way the current nested mutations in here work, you can just have the relations as simple fields on the input (assuming they are belongsTo relationships).

input createClubInput {
    name: String!
	federation: ID!
	district: ID!
    description: String
    abbreviation: String
    founding_date: String
    dissolving_date: String
    federation_reference_number: String
}

@maarten00
Copy link
Contributor

True, you could add those there. I do think it should always be possible add more arguments besides the input type itself. So always flattening only the first argument should work just fine.

…ctives

- Add tests for basic functionality of create and update
- Add flag "flatten" to enable use with a single input object
- Fully define all relationships on test models
- Get rid of PHP 7.1 annotations
@spawnia spawnia changed the title [RFC] Nested mutations prototype/discussion [Feat] Nested mutations Jul 27, 2018
@spawnia
Copy link
Collaborator Author

spawnia commented Jul 27, 2018

I think with the changes in 7198183 this is now ready to get merged. @chrissm79

Nested mutations are now integrated with the existing @create and @update directives, but require proper definition of the Input Objects. This does not break existing use cases, but gives users the chance to try this out.

I would mark this as Experimental for now, as the very nature of nested mutations requires the setup to be quite opinionated. While this works as is, i do not feel confident in providing a stable API for it quite yet.

@maarten00
Copy link
Contributor

Ran some more tests with your new code, still works great. Will have to add some explanation to the docs though about use, because of the extra argument in @create

@maarten00
Copy link
Contributor

@chrissm79 I'm eagerly waiting for this to get merged, is there anything I can do to help? I'd be happy to write documentation or run some extra tests if that speeds up the process.

@chrissm79
Copy link
Contributor

@maarten00 @spawnia Apologizes, missed the mention in the comment earlier! Currently reviewing and will try to get this merged in tonight!

@shaz-r
Copy link

shaz-r commented Oct 16, 2018

@spawnia Is the above example still valid? I've just tried to use this feature; the parent (User) gets saved but none of the children (posts) do.

Code:

type User {
  id: ID!
  email: String!
  posts: [Post!] @hasMany
}

type Post {
  id: ID!
  title: String!
  user: User! @belongsTo
}

type Mutation {
  createUser(input: CreateUserInput!): User
}

input CreateUserInput {
  email: String!
  posts: CreatePostRelation
}

input CreatePostRelation {
  create: [CreatePostInput!]
}

input CreatePostInput {
  title: String!
}

And query:

mutation {
  createUser(
    input: {
      email: "john@example.com"
      posts: {
        create: [
          { title: "First Post!" }
        ]
      }
    }
  ) {
    email
  }
}

@spawnia
Copy link
Collaborator Author

spawnia commented Oct 16, 2018

Hey @SHAZM thanks for getting involved.

The main thing missing from your schema was the @create directive. Apart from that, it does look fine. Added a few ! where i think they are appropriate.

type User {
  id: ID!
  email: String!
  posts: [Post!]! @hasMany
}

type Post {
  id: ID!
  title: String!
  user: User! @belongsTo
}

type Mutation {
  createUser(input: CreateUserInput!): User! @create(flatten: true)
}

input CreateUserInput {
  email: String!
  posts: CreatePostRelation
}

input CreatePostRelation {
  create: [CreatePostInput!]!
}

input CreatePostInput {
  title: String!
}

Does your User model have a relationship method posts that returns an instanceof HasMany?

@shaz-r
Copy link

shaz-r commented Oct 16, 2018

Awesome that was it; just missing the @create(flatten: true). I'll send through a pull request a bit later for adding this to the docs; it's super handy! Thanks @spawnia!

@ouily
Copy link

ouily commented Oct 17, 2018

Hello,

I just tested but it doesn't work with belongsToMany directive:

type Post {
    id: ID!
    subject: String!
    text: String!
    created_at: DateTime!
    updated_at: DateTime!
    tags: [Tag!] @belongsToMany
}

type Tag {
    id: ID!
    name: String!
    slug: String!
    created_at: DateTime!
    updated_at: DateTime!
    posts: [Post!] @belongsToMany
}

type Query {
    post(id: ID @eq): Post @find(model: "App\\Post")
}

type Mutation { 
    createPost(input: CreatePostInput!): Post @create(model: "App\\Post", flatten: true)
}

input CreatePostInput {
  user_id: ID!
  topic_id: ID!
  subject: String!
  text: String!
  tags: CreateTagRelation
}

input CreateTagRelation {
  create: [CreateTagInput!]!
}

input CreateTagInput {
  name: String!
  slug: String!
}

:(

@spawnia
Copy link
Collaborator Author

spawnia commented Oct 17, 2018

@ouily Please mark your code blocks as ```graphql for syntax highlighting.

You are correct, currently only BelongsTo and HasMany are supported. I opened an issue over at #396 to track process on supporting BelongsToMany. Accepting PR's.

@sadnub
Copy link
Contributor

sadnub commented Nov 16, 2018

First of all, Excellent work on this package! I'm debating on moving to Lighthouse from FolkLore. This is the only hangup I am having replicating my schema. This fixes it for the Mutations but wondering if the (flatten: true) also works for Queries?

Something like:

#types
type User {
    id: id!
    name: String
    email: String!
    role: String
    active: Boolean
    active_string: String
    created_at: DateTime
    updated_at: DateTime
}

#Inputs
input UserInputQuery {
    id: Int!
}


#Add Queries to Base Query
extend type Query {
  users: [User!]! @all
  user(input: UserInputQuery! @eq): User @find(flatten: true)
}

This is a basic example. I am using Vue-Apollo on my front end.

Thanks!

@enzonotario
Copy link
Collaborator

enzonotario commented Nov 16, 2018

@sadnub I think that you can write that schema like:

type User {
    id: ID!
    name: String
    email: String!
    role: String
    active: Boolean
    active_string: String
    created_at: DateTime
    updated_at: DateTime
}

#Inputs
input UserInputQuery {
    id: Int!
}


#Add Queries to Base Query
extend type Query {
  users: [User!]! @all

  user(id: ID! @eq): User @find(model: "App\\Models\\User")
}

@sadnub
Copy link
Contributor

sadnub commented Nov 16, 2018

Do I need the ID scalar and the specifying the model in the mutations? I thought that was only for Relay but I could be mistaken

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

Successfully merging this pull request may close these issues.

8 participants