[Feat] Make mutating directives transactional (@create and @update)

[liepaja]

• Added or updated tests
• Added Docs for all relevant versions / is applicable when explaining nested mutation

Wrap @create and @update directives in a transaction. This ensures that if any of the nested mutations within fail, no changes are made to the database.

Breaking changes

The default is now to use transactions for mutations, but this can be disabled in the config.

[liepaja]

assertDatabaseMissing has issue with 1 exact combination of PHP version, laravel and PHPunit version.

[spawnia]

@update should function the same, so please wrap that in a transaction closure as well.

[mfn]

This change does not seem to be optional for create/update, or am I mistaken?

[liepaja]

No, is that ever an issue @mfn ?

[mfn]

If you using events from models within transactions, the events get dispatched immediately and (non-sync background) jobs may pick up this change before the transaction is commited and may see a different state of the database.

(Very) Recent discussion: laravel/ideas#1441

This can be a hard to track source of problems, I therefore suggest to make this optional and not default and add a documentation about why.

[liepaja]

It looks like events indeed bring inconsistency. However its hard to decide whether having half of mutations being executed and an event after is better then no mutations and an event. Would you disagree @mfn?
I am not sure if laravel/ideas#1441 (comment) is possible in case of directives, as they are chained 'next' resolvers. I beleive this package tackles just that - https://github.com/fntneves/laravel-transactional-events.

[spawnia]

This is indeed a hard problem to solve.

I had a look at the GraphQL spec, it seems that it does not prescribe that an error in a mutation must fail the entire mutation. https://facebook.github.io/graphql/June2018/#sec-Mutation
That leaves us room for allowing the current behaviour, if it is wanted by the developer.

However, it makes more sense for me to make mutations transactional. This does complicate the issue of fired events that @mfn has brought up, but it gets rid of a whole slew of other issues that may arise if partial changes to the data can happen.

I can find little information on nested mutations, the most prominent example is Prisma. They do mention in their docs that they treat all mutations as transactional

There is definitely a tradeoff to be made, so i think we should make this option configurable. How about we add a switch transactional_mutations to the config.php that controls this setting globally? I advocate towards defaulting to transactions.

[liepaja]

I believe there is no such case where

Return an unordered map containing data and errors.

is the case. Its ether error or data in lighthouse.
I agree on having the option within config which defaults to transactions.

[spawnia]

If we allow partially successful mutations, we must ensure that we return the data of the successful operations until then and the error.

Otherwise, it just becomes impossibly hard for the client to know at what point it went wrong.

Should that prove to be too difficult, i suggest we only offer transactional mutations.

[yaquawa]

Transaction is good to have, but the performance is really bad.. I believe that not every mutation must to be a transaction. The directive should give user an option to select if it's going to use the transaction or not.(Considering using it with the @can directive, it should handle partial mutations.)

@spawnia
And this is exactly why I added the ErrorBuffer class in #441, this way we can gather partial errors through the nested mutation.

[yaquawa]

Additional suggestion, make it do not use transaction if it is not a nested mutation.

[liepaja]

It is quite hard to detect if it is nested or not. That is being done within MutationExecutor. And transaction is wrapped in the directive. I think its good to keep global conf selectable, so each can select the default, and select exact per mutation.

[yaquawa]

@liepaja

If I'm not missing something, at this time we can't select if it performs as transaction or not at the directive level like @update(transactional_mutations: false).

It is quite hard to detect if it is nested or not

I think it can be done by comparing the incoming argument value against the AST.

Another thing I noticed is that the connection which transaction applied on should be the model's connection but not the default connection.

[liepaja]

I will add this functionality shortly.
What do you mean by

Another thing I noticed is that the connection which transaction applied on should be the model's connection but not the default connection.

[yaquawa]

@liepaja Each model can have different connection, the transaction should be applied to each connection specific to each model.

see "Database Connection" in https://laravel.com/docs/5.7/eloquent

[liepaja]

@liepaja Each model can have different connection, the transaction should be applied to each connection specific to each model.

see "Database Connection" in https://laravel.com/docs/5.7/eloquent

Hmm, good point.

[yaquawa]

@liepaja if the database grows really large, actually you want to separate the each model to connect to different databases, and I think it's not uncommon use case.

[liepaja]

want to separate the each model to connect to different databases, and I think it's not uncommon use case.

I am not sure that this is common, but indeed it can happen. This is not really solvable as within nested mutation, another model can then have another connection.

[liepaja]

I believe there is no better solution than just using default connection. However it should be documented.

[spawnia]

if the database grows really large, actually you want to separate the each model to connect to different databases, and I think it's not uncommon use case.

I really doubt that. Performance for joins would by abysmal. Something like sharding would be the more obvious choice, and the database engine should still support transactions.

[spawnia]

@liepaja Can you copy over the config to the master docs?

An additional sentence or two in the Mutating Relationships section won't hurt either.

When the docs are updated, i am fine with merging this as is.

@yaquawa Proper errors for partially successful mutations and the usage of model connections are both seperate issues that we might take care of in the future.

[spawnia]

I do not like the per-directive overwrite of transactional handling. Think about the perspective of the client.

Unless it is somehow documented in each and every field, there is no way for them to know how each field behaves. And even if that is the case, it leads to a crazy inconsistent API.

It is better and easier to communicate to clients to have consistent behaviour for all fields. Let's remove that option.

[spawnia]

The config is duplicated in the docs: https://github.com/nuwave/lighthouse/blob/master/docs/master/getting-started/configuration.md

Probably not the best approach, but reasonably easy to keep up to date and it saves the user from leaving the docs to view the config options.

[yaquawa]

It is better and easier to communicate to clients to have consistent behaviour for all fields. Let's remove that option.

I think it's nothing bad to keep it as an option.. At the end of the day, the behavior of mutation is determined by the developers but not the clients.

[spawnia]

@yaquawa additional options add mental overhead and complexity. Lighthouse should be kept as simple as possible, with reasonable defaults.

If somebody really wants to change the behaviour, which i doubt anyone would want and which would be a really bad idea, they can still implement their own version of @create/@update.

We can't always prevent developers from shooting themselves in the foot, but at least we can make it hard for them.

[yaquawa]

@spawnia I understand what are you pointing, me too agree with most of your opinions.
But.. here is what I thought about this

additional options add mental overhead and complexity. Lighthouse should be kept as simple as possible, with reasonable defaults

In this particular case, actually it won't add much complexity to the code and the default is set to false.

doubt anyone would want and which would be a really bad idea

If the mutation is not a nested mutation, and the default option is set to using transaction we are forced to perform with transaction which can be a huge performance issue.

[mfn]

The directive should give user an option to select if it's going to use the transaction or not

I do like this.

This is also how I've seen "others" solve this problem, e.g. REST frameworks where you can specify per endpoint if it should be transactional, whether it's a read-only or write transaction, etc.

I don't know how the compatibility is with nested transactions. Last time I used them with MySQL (<8) they were a problem (not supported or didn't work with Laravel, whatever). They do work in Postgres "now" (first ->begin() start a transaction, other ->begin() set savepoints).

[liepaja]

I suggest for now merging transactions with global config, as it is possible to set it to false and have previous functionality. New PR should be created for optional features. Otherwise this is going back and forth.

[yaquawa]

@liepaja I didn’t get why the $model->refresh() should be called, if you want the default value to be set, you can set it in the $attributes property or in creating or updating event.

[liepaja]

@liepaja I didn’t get why the $model->refresh() should be called, if you want the default value to be set, you can set it in the $attributes property or in created event.

This was not added in this PR, but in #509. I just moved its position. Basically without it, if the model has default values, it returns null.

[yaquawa]

@liepaja got that! Thanks!
It seems like an extra perfermence issue to me.. maybe we can have an option to turn off the refresh invocation?

[liepaja]

@liepaja got that! Thanks!
It seems like an extra perfermence issue to me.. maybe we can have an option to turn off the refresh invocation?

That is true as refresh rehydrates. I believe this is something for a new PR.

[liepaja]

@chrissm79 , can this be merged?

[chrissm79]

@liepaja I should be able to get this merged in today

[chrissm79]

fantastic job on this one @liepaja 👍