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

plans to implement mutations ? #1

Open
kapouer opened this issue Oct 28, 2016 · 16 comments
Open

plans to implement mutations ? #1

kapouer opened this issue Oct 28, 2016 · 16 comments

Comments

@kapouer
Copy link

kapouer commented Oct 28, 2016

I suppose it's not trivial at all to implement mutations as well - but i had to ask the question !

@koskimas
Copy link
Collaborator

I haven't looked into mutations yet, so I don't know yet. I'll post here when I do.

@nasushkov
Copy link
Contributor

I am also interested, are there any news on this topic?

@kapouer
Copy link
Author

kapouer commented Feb 2, 2017

It's somewhat related to Vincit/objection.js#211 ?

@Trellian
Copy link

Hi, I'm also interested in this. How can I either add mutations after the fact, or during the SchemaBuilder? I have forked the project and already made some other small changes. I'm also already generating the JSON schema directly from MySQL, and feeding it into SchemaBuilder, that's working great.

Can you perhaps give me some pointers, so I can take a stab at this myself?

@Trellian
Copy link

Trellian commented Nov 26, 2017

@koskimas Hi Sam, I really need to implement mutations in my model. I've had a look at the code, added duplicate functions for _resolveForModel(), _filterForArgs() as _mutationsForModel(), _filterForMutationArgs() etc, added 'upsertXXX' operations and created a 'Mutations' tag in the schema, but I'm missing something. I'm not the Guru you are. Everything appears to be correct in the schema, as far as I can tell.

My code seems to be calling the Objection.js update pathways, but I'm getting errors about missing properties, and I can't seem to track down where the problems are, and I'm obviously missing something fundamental.

I have a model that looks like this:

Type IwRegion {
  id: Int!
  country_id: Int!
  name: String! 
}

The mutation looks like this:

mutation regionUpsert ($Id: Int!, $Name: String!)
  {
  upsertIwRegion (id: $Id, name: $Name)
    {
      id
      name
    }
  }

with variables:
{
  "Id": 1,
  "Name": "New Region Name"
}

and getting errors like:

Error: {
  "name": [
    {
      "message": "should have required property 'name'",
      "keyword": "required",
      "params": {
        "missingProperty": "name"
      }
    }
  ],
  "country_id": [
    {
      "message": "should have required property 'country_id'",
      "keyword": "required",
      "params": {
        "missingProperty": "country_id"
      }
    }
  ]
}

Please can you assist, or at least point me in the right direction? I'd really appreciate it!

@nasushkov
Copy link
Contributor

@Trellian I'm also interested in Mutations support. However, I can't agree that it should be implemented the same way as the Query part, like autogenerated CRUD. I consider GraphQL as a CQRS pattern implementation in some way. In particular, you have Query part (Read Model in CQRS terms) which is about reporting and Mutations part (White Model in CQRS term), which is about changing your domain model. So in this terms each Mutation is a Command which name should be very precise about the intent (i.e. changeTaskStatus instead of changeTask with status field modified.) and the Command handler can have complex side effects and business logic. So, I don't think that overall it's a good idea to implement mutations in CRUD way and autogenerate them. However, I would really appreciate if it would be possible to supply custom Mutations part for the schema, for instance in build() function. @koskimas What do you think?

@Trellian
Copy link

Trellian commented Dec 2, 2017

FWIW to anyone here, I've made a PR that adds (very) basic Mutation support in PR #30

@Trellian
Copy link

Trellian commented Dec 2, 2017

@nasushkov Sorry, I didn't see your comment until now.

Yes, I agree with you in principle. However, I don't think that's the whole story. Some Mutations do affect the entire Model, such as a 'Create', and while CQRS is perfect for single field actions and state changes, there is also a requirement for multi-field mutations.
If for example, an user wishes to change their address in the DB because they've moved (a poor example, admittedly), or a more complex one for a business action.

Please have a look at the PR #30 I've just created. You can do Updates and Patches, and the definition of the mutation is left entirely up to the client side schema, so there is no violation of CQRS from the client side.

I also don't think it's feasible to write a Mutation for every single field in every table... The server side must allow for multiple-field or whole-record mutations by definition. I think that CRUD is more applicable to the backend, and CQRS to the frontend.

At any rate, I'm not going to argue the point. I just needed basic Mutation support. I'm hoping @koskimas and others will find the PR useful, and either reimplement it properly, or add to it.

@nasushkov
Copy link
Contributor

@Trellian I wouldn't say that CQRS is convenient only for single field changes. Actually, you can implement CRUD with CQRS. However, I wouldn't go with CQRS if you have only CRUD operations in your domain. The power of CQRS and Command pattern, in particular, is that it allows you to be much more expressive and flexible than with CRUD (the same analogy with Mutations and REST).

Anyway, regarding the topic: I've checked your PR, while it seems sound for some cases it's still not universal and doesn't cover all cases for me. For instance, if I want to implement mutation resetPassword I need to establish new api endpoint for that case. That's a bummer.

I've created a small PR #31 which allows enhancing the schema with a custom Mutations implementation. I also think that it would be better if your implementation for mutations can be used as a plugin for that case, i.e. in a separate module like objection-graphql-mutations and could be optionally merged with this query schema. I think it would be much more flexible.

@Trellian
Copy link

Trellian commented Dec 3, 2017

@nasushkov Thanks for the feedback. As I mentioned, my PR is for a really basic version of Mutation support, and I welcome additional input. At this point. objection-graphql currently has no support for Mutations at all, so I consider my PR a starting point only. A module-based implementation would be great.

As to the philosophy, as with anything IRL, both CRUD and CQRS come with their benefits and challenges. Also, I didn't only say that CQRS is only convenient for single field changes, I included state changes which loosely implies CQRS. I personally contend that CRUD is a lower implementation layer for CQRS (yes, I know that one can implement CRUD in CQRS, but that really is putting the cart before the horse).

CQRS implies Event Sourcing and the requirement for some sort of messaging system, and one pattern even suggests reconstitution of the current state of a system from the history of state changes (I have 15 years experience with that in a production env). All good and well. However, objection-js is an ORM. Adding objection-graphql doesn't change that. objection-graphql is a layer that generates a graphql API. Mutation support was missing, I suspect because it's a bit of a minefield.

As an aside, CQRS in general needs additional (complex) layers in order to implement a working solution for a real-world application, and I need to get work done. We're talking significant complexity, especially with Event Sourcing, Messaging and Eventual Consistency. This is not yet the purview of objection-graphql. It may well be, in time.

As it stood, I had a system with over 200 tables, and no way of declaring Mutations in objection-graphql. Now I can write my client-side mutations, knowing that they will work. Yes, it's not perfect, but it's a start. CQRS support will come in time, along with the goodness of scalability, uptime , ACL etc.

I really welcome your PR #31, I hadn't thought of doing it that way, and yes it is very flexible. I plan to use it in conjunction with my PR #30. At some point, I will try to implement some sort of 'extend/overide' mutation ability via this mechanism, so that I can get the basic CRUD facilities, but hide/replace them behind a CQRS implementation.

Further thoughts are very welcome!

@kibertoad
Copy link

@timhuff Are you still actively working on this project? My team is interested in using objection-graphql and I personally can contribute some effort to it, so I wonder if you would consider granting co-maintainership to some additional people :)

@timhuff
Copy link
Collaborator

timhuff commented Nov 2, 2018

@kibertoad I don't really have time to maintain this anymore. After using it to some extent, I've found I'm not a huge fan of the overall design of the project. At one point I was interested in rewriting it but I'm simply not utilizing it to a sufficient degree anymore. That said, I don't have control over maintainers.

@kibertoad
Copy link

@timhuff Whom should I ping for maintainership access?

@timhuff
Copy link
Collaborator

timhuff commented Nov 2, 2018

I believe that would be @koskimas

@haikyuu
Copy link

haikyuu commented Jan 25, 2019

@timhuff

I've found I'm not a huge fan of the overall design of the project. At one point I was interested in rewriting it but I'm simply not utilizing it to a sufficient degree anymore.

Can you summarize your thoughts about this, and how it could be re-written. So that the project doesn't just die?
Thank you

@timhuff
Copy link
Collaborator

timhuff commented Jan 25, 2019

GraphQL and Objection are both OOP as hell. If I were to rewrite it, I'd follow their lead.

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

No branches or pull requests

7 participants