platinummonkey/rexpro-python — pull request #9 by nvie (2014-08-13 09:22:16+00:00)
Title: Improve transaction management

This patch fixes some serious bugs related to transactions.

This code:

```python
with conn.transaction():
conn.execute('g.addVertex([x: 1])')
conn.execute('g.some_func_that_fails()')
```

Would emit the following messages (implicit commands are shown inline as they occur):

```groovy
g.stopTransaction(FAILURE) // (1a)
g.stopTransaction(SUCCESS) // (1b)
g.addVertex([x: 1]) // (2a)
g.stopTransaction(SUCCESS) // (2b)
g.some_func_that_fails() // (3a)
g.stopTransaction(SUCCESS) // (3b)
// g.stopTransaction(SUCCESS) // (4)
```

1. The context manager opens a fresh transaction context here. (1b) is caused by the implicit wrapping in a transaction, which is useless here.
2. Adds a new vertex. Since this is implicitly wrapped in a transaction (because that's the default), this means the new vertex is now committed to the graph, due to 2b—oops!
3. This step now causes failure, so the transaction should be aborted, but the vertex had been committed already.
4. This commit step would be performed by the context manager _if_ this cause did not throw an exception, which it does. There's no rollback performed ever.

This pull request addresses all of the issues above:

- It avoids the needless but implicit-by-default wrapping of `g.stopTransaction(FAILURE)` in a transaction itself. It's harmless, but useless. See 9d7ad16ba75ebc2fc7c96af160e53a249bc31e74.
- It makes sure no messages (`.execute()` calls) within the context manager get wrapped within an implicit transaction itself (this avoids 1b, 2b, 3b). See 0e989368c2e4ca011c8e89e2aae3306b107b5f96.
- It makes sure the transaction will properly be closed when the context manager exits, depending on whether an exception occurred or not. See 88bc0bc9fbae590c5ddd0fd48f6d6e1c40be5d3a.

Meaning the above code will now emit the following messages instead:

```groovy
g.stopTransaction(FAILURE)
g.addVertex([x: 1])
g.some_func_that_fails()
g.stopTransaction(FAILURE) // or g.stopTransaction(SUCCESS) is no exception occurs
```

I think the root cause for these bugs lie in the naming convention. It's pretty confusing to read the code because the identifiers "transaction" and "in_transaction" can be interpreted to refer to state (as in "is currently in a transaction"), or an action to perform (as in "please wrap this block in an implicit transaction"). The latter corresponds to the actual semantics, so my suggestion would be to either name this param "wrap_in_transaction", or "commit_on_success", which would make the code more obvious to read.