Naming logic requiring transaction context consistently

[lucaro]

For the database abstraction classes, we have a consistent naming scheme: when a classes name starts with Db*, it requires a transaction context to work. There is, however, also other logic that can only be called inside of a transaction context, most notably parts of a run manager. This makes the whole thing tedious to deal with and probably has as a consequence that we have more and longer-lived transactions than actually necessary since one rather adds more, just to be sure. I would therefore suggest to consistently try and remove all these surrounding transactions and rather have them inside the relevant methods, spanning only exactly as much execution context as necessary. This would have the added benefit of being able to get rid of explicit transaction contexts in most API handlers. Thoughts?

[sauterl]

If I understand you correctly, the proposed change is that the `Db*' classes should manage the transactions themselves, and as such have the transaction span as short as possible?
This seems reasonable to me, especially as I like the resulting transactionless API handlers (insofar as the handler itself doesn't create a transaction).

EDIT: Anyways, at least for write transactions we agreed upon this policy in #429

[ppanopticon]

I fully agree. This is somehow a consequence of the principles outlined in #429 and I would add the following boundary conditions:

• Read-transaction do not interfere with the flow of things and are thus less critical.
• If a read-transaction is used for a direct database query, it can stay in the API layer. In fact it makes sense to leave it there and I would argue, we should resort to doing direct database lookups wherever possible, since it will potentially simplify the internal API.
• If a core-logic piece of the internal API (e.g., a RunManager method) requires a read-transaction, that transaction should be spawned as part of that logic and should be inlined to the site of use.
• Write-transactions should exclusively take place as part of some core-logic and should be inlined to the site of use, so as to keep them short.

[lucaro]

If I understand you correctly, the proposed change is that the `Db*' classes should manage the transactions themselves, and as such have the transaction span as short as possible? This seems reasonable to me, especially as I like the resulting transactionless API handlers (insofar as the handler itself doesn't create a transaction).

EDIT: Anyways, at least for write transactions we agreed upon this policy in #429

It's not that the Db* classes should handle the transactions themselves. If you are using a Db* class, you get a clear indication that you require a transaction context. Currently, we have logic that uses these Db* classes that may or may not already manage the relevant transactions and therefore the caller of that logic may or may not need an extra surrounding transaction. From a convention point of view, I would hence argue the following: If and only if any of the classes you are directly using have the Db* prefix, you need to take care of a transaction.

I agree with all the points listed by @ppanopticon above.

[ppanopticon]

I would discourage the movement of the transaction management to the Db* classes. By core-logic I mainly mean the RunManager (and adjacent) classes that handle the flow and logic of an evaluation. From my experiences, that is where:

• We orchestrate changes to the database and the internal state of the run (which may involve more than a single Db* class).
• We can distinguish between minor difference between the different types of evaluations / runs.
• We know, what type of transaction is required to perform an operation.

However, I did not really take into account all the logic surrounding template, media collection and user management, which is currently not bound to some internal logic but pretty much directly exposed via the API. Here I would probably build dedicated data access objects.

[lucaro]

Agreed. I'd also not put any transaction logic inside any of the Db* classes. Rather, transactions should be limited to logic directly operating on such classes.

[sauterl]

For user management, we already have the DbUserManager which requires a wrapping transaction. By the new convention above, this is as desired and might be worthwhile to have similar Db*Managers for templates and collection management?
I do like this approach and think we should start immediately with it. I am by no means a devops specialist, but this might be enforceable by some tooling? Then again, might be an overkill to enforce our policy that strictly.

[lucaro]

I'd argue that there should be a UserManager rather than a DbUserManager and that UserManager should take care of transactions as it sees fit in order to have them as short and read-only as possible. This could then be handled analogously for media collections, etc. That way, any API logic just wanting to look up some user details, etc., would not need to know anything about transactions.

[lucaro]

If you wanted to enforce this programmatically, context receivers would probably be the way to go. I tried this a while ago and they don't play nice with some of the mechanisms used in dnq, so getting this to work is probably more trouble than it's worth.