smallrye
Transactions
The spec should at least mention transactions, as users must be aware of the consequences: when you have multiple mutations within one request, they will be executed in separate transactions. In order to combine them in a single transaction, you'll have to write a wrapper mutation including all possible requests as sub-mutations.
The same thing happens with multiple queries, and also with a query and it's resolvers. This is not that often an issue, but read consistency may be important in some cases.
I've created a little demo war project that counts the creation of transaction and request scoped beans: https://github.com/t1/transactional-graphql-demo The @Stateless annotation on the GraphQL API class makes it an EJB, i.e. transactional and pooled and a few more, convenient adjectives.
In addition to mentioning the impact of multiple transactions in the spec, it would be nice to make the mutations wrapper mutation invisible in the API. The schema is now:
type Mutation {
bar(i: Int, s: String): String
foo(input: String): String
mutations(input: MutationsInputInput): Mutations
}
type Mutations {
bar: String
foo: String
}
input MutationsInputInput {
bar: Int
foo: String
}
We could hide the mutations method, the Mutations type and the MutationsInputInput... maybe with a @InlineMutations annotation or so.
Reminder to self: generate the wrapper classes with an annotation processor.
There's one problem left, I guess: if we write the mutation-collector method by hand, then the order of the mutations is defined by that method, not by the order in the request... so this issue has a bigger impact than just having to write boiler plate code.
another thought: a client might want to repeat the same mutation with different parameters... then order is even more important.
After thinking a bit more about transactions, I guess a transaction should be started by the servlet (when available). This is what application developers would expect. When they require more fine grained control, e.g. to explicitly start a new transaction for one mutation, they can still add appropriate annotations.
@phillip-kruger @andymc12 @jmartisk WDYT?
Agree with the transactions. We still need to confirm we support @Transaction. We should since it's a CDI Bean I think.
I suppose users should be able to cover their transaction needs by applying the @javax.transaction.Transactional annotations (or use an injected UserTransaction programmatically) as necessary on the beans that handle mutations, and so ideally we should not do need to do anything on our side.
Of course, that's only when the used runtime (WildFly/Quarkus/etc) supports JTA. Which might not be the case for all MP-compatible runtimes, AFAIK MicroProfile does not require any kind of transaction API to be supported. If we want SmallRye GraphQL to stay compatible with all runtimes, I think we can't rely on the presence of any transactional capabilities. But I don't think we need to.
I'm not completely sure how this will work with asynchronous operations - these might become a problem. In most cases with purely synchronous operations (all handling is done by one thread), I guess it should be enough just to annotate the @Mutation method with @Transactional(Transactional.TxType.REQUIRES_NEW) and everything will be handled in one JTA transaction.
Might need to give it a try, and perhaps add some new tests in the server/integration-tests module to demonstrate it?!
ideally we should not do need to do anything on our side.
That's what I had hoped for in the beginning, but it's not enough. When one request contains multiple mutations, I'd expect them to run in a single transaction. I can't control that with an annotation in my mutation methods, so it has to happen from within SmallRye. Note that while not as obviously an issue, this is also true for query methods and resolvers: read consistency sometimes is important, too.
When one request contains multiple mutations, I'd expect them to run in a single transaction
Why? Is that really necessary? If such behavior is necessary in a particular application, I think the app can go with the "wrapper" that you mentioned in the description.
SmallRye GraphQL can't assume anything about what operations will be combined in one request, and each operation can have different transactional semantics and you have no way to guess how to combine them together:
Imagine a request that runs three operations, one of them is TxType.NEVER and the other two are TxType.REQUIRED. How should SmallRye handle that? It can't wrap them all into a single tranasaction.
I think assuming anything on the SmallRye side is dangerous, and also potentially limits our portability, because not all MP runtimes will support JTA.
If a client updates three things in one request, she will assume that they all worked or none. This should be the default without the service developers having to do anything. If there is some special requirement, the service developers could annotate, e.g., one mutation with REQUIRES_NEW to have it run in a separate transaction; NOT_SUPPORTED would have it run outside of any transaction; NEVER clearly doesn't make any sense here.
And we're not assuming anything: if there is no JTA, nothing happens. Only when JTA is available, SmallRye integrates into the existing environment, just as it does with other standards.
If a client updates three things in one request, she will assume that they all worked or none.
I think this is a faulty assumption. Transactions don't seem to be part of the GraphQL spec, and articles like this ( https://graphqlme.com/2018/05/13/transactions-mutations-and-graphql/ ) provide users with the best practices of refactoring their mutations so that it is essentially transactional (i.e. debitAccount(accountId: ID!, amount: Float): Account and creditAccount(accountId: ID!, amount: Float): Account should be replaced with transfer(from: ID!, to: ID!, amount: Float): TransferDetails!).
I think the transactional semantics should be determined by the annotations. In cases where multiple mutations are submitted in the same request, we should follow the GraphQL spec. It seems like there has been some discussions around transactions at the spec level - and some interesting ideas about specifying transactional contexts in the client request (see https://discuss.dgraph.io/t/transactions-in-graphql/6861 ).
That said, it seems like it should be fairly trivial to have a startup config property (something like transactional.endpoint=true|false - default to false for backwards compatibility) that would then enable a TRANSACTION_REQUIRED tx at the servlet entry point. Wdyt?
I understand that transactions are not part of the GraphQL spec. I've read the blog post you linked; the author seemed to have had some trouble implementing transactions in his server, which is very understandable: that's a surprisingly complex topic. And of course it's better to model your objects so you don't need transactions. Not only transferring money can be handled in a single operation. It's also advisable to create an order and its items in a single object graph, not as separate requests. Some NoSQL databases work this way, too. Actually the whole transaction topic is much less relevant than a few years ago.
While there's still a place for transactions and a solid foundation in JEE, they may also hurt, as it would mean you need distributed transactions when one operation writes to one db and another operation to another. So the developer needs control. I think the startup config is not a good place, as it's too far away from the code and too global. I'd prefer a class annotation, maybe @TransactionalOperations? wdyt?
I might not be following properly, why can a dev not just use @Transactional from the server. Even if it's outside the MP GraphQL Spec, most runtimes would have it. I am not sure why we need to do anything, appart from maybe say that if the runtime support transactions, we should adhere. Same as bean validation.
@t1 - why are you saying " I can't control that with an annotation in my mutation methods" ??
If a client updates three things in one request, she will assume that they all worked or none.
If I imagine us doing it this way, I can imagine a situation that
- operation A does something in the database and successfully returns some data
- operaton B does something else in the database and fails, rolling back the whole transaction
Should we send an error for the whole operation instead of returning a partial result with the data from A, which might or might not be still valid data (we can't know for sure)? GraphQL should be about supporting partial results, but in this case you don't know whether you can return a partial result or not.
@phillip-kruger: I can control the transaction of a single method. Even when I annotate the class, it applies only to all methods in the class, but they still all will get their own transaction. What I need is a way to group multiple methods into a single transaction.
So if you do a Required if creates a new transaction for every execution ? If so that we can fix in the runtime I think
Basically, the runtime needs to check if there is a Required, and if start a transaction before it starts calling.
(But I don't think we need any new annotations) We just need to make sure that transactions work as expected. Assuming that the runtime has got JTA, I would expect the following:
If the method is marked as Required - we need to make sure that a transaction is started before we call the method Mandatory - we need to make sure that a transaction is started before we call the method
RequiresNew - we should not need to do anything. NotSupported - we should not need to do anything. Never - we should not need to do anything. Supports - we should not need to do anything.
Something like that ?
You're right, this would be possible. If there are six methods, each with one of those transactional requirements; then the Required and Mandatory run in a single, shared transaction, while Supports and methods without any annotation do not; and Never doesn't fail? I think this is quite complex to understand. With a new annotation, things would be more explicit and the mental model much simpler.
I just found out that the GraphQL executor also needs to commit the transaction after the json response is fully built; otherwise it might fail, e.g., if a bean is @TransactionScoped.