openerp-expert-framework team mailing list archive

[Olivier Dony <odo@xxxxxxxxxxx>]

> On 03/13/2014 08:05 PM, Valentin LAB wrote:
> > Here's a MP to propose a simple way to add transaction where it could be
> > needed without breaking legacy code:
> >
> > https://code.launchpad.net/~0k.io/openobject-server/trunk-new-prevent-cr-commit-to-allow-super-transaction/+merge/210884
> >
> >
> > If this is okay, it's trivial to encompass openerp's initialize / update
> > sequence in one go.
> >
> > Any comments ?


I find this change quite dangerous. Any magic that affects all running 
transactions is particularly likely to introduce subtle bugs that we will only 
notice long after merging it. The rowcount one is just one example of 
not-so-obvious regressions that will happen.

As you've seen, calling legacy code is simply not safe in the context of 
savepoints (even after your patch), so we should just be careful with the use 
of cr.savepoint() and avoid extra magic.
The point of the cr.savepoint() contextmanager is to automatically close your 
savepoint, not to be a safeguard for legacy code.

In addition, I think the API should preserve programmers from surprises as much 
as possible. Anyone using commit() explicitly is either:
 1. A clueless programmer who should not be using them, and will manage to 
screw it, regardless of the API we choose ;
 2. Someone who actually knows what SAVEPOINT, COMMIT, ROLLBACK and ROLLBACK 
TO SAVEPOINT mean, and will expect cr.commit() and cr.rollback() to do what 
they say at all times.

Regarding the issue of commits in auto_init(), I had tried in the past to get 
rid of them (they have no reason to be there indeed), but never merged that 
patch because it was causing obscure errors that I had no time to investigate. 
I would be glad if anyone had the time to look at it.