[Cubicweb] NativeSQLSource.doexec auto-rollback

Christophe de Vienne christophe at unlish.com
Mon Dec 29 12:40:41 CET 2014



Le 28/12/2014 00:39, aurélien campéas a écrit :
> 
> 
> 2014-12-27 18:36 GMT+01:00 Christophe de Vienne <christophe at unlish.com
> <mailto:christophe at unlish.com>>:

[...]

>     Digging a little in the source and looking for the 'transaction has been
>     rolled back', I found the NativeSQLSource.doexec function, which is
>     basically:
> 
>         def doexec(self, cnx, query, args=None, rollback=True):
>             cursor = cnx.blablabla
>             try:
>                 cursor.execute()
>             except Exception as ex:
>                 self.critical('sql: xxx' % ex.xxx)
>                 if rollback:
>                     cnx.rollback()
>                     self.critical('transaction has been rolled back')
>                 # some clever code to translate the exception in a CW one
>                 # if possible
> 
>     Several things are bothering me:
> 
>     - The 'rollback' feature, if anyhow useful (I do not think it is),
>       should not have such a default: doexec is, in my understanding, a low
>       level function. And rolling back a transaction is a high-level
>       decision. But since the doexec function is called by many intermediate
>       functions without passing any 'rollback' parameter, we end up with a
>       high-level decision made in a low-level function.
> 
> 
> At this point in the flow, the low-level transaction has been rolled back,
> and we cannot defer for too long emitting a rollback event at the cw
> "cnx" level.

What do you mean by "the low-level transaction has been rolled back"?
Do you refer to something lower-level than doexec?
Why would an error rollback the transaction?


> 
>     - If an error occur at the sql level, it will be transmitted or
>       translated to a CW error, which is fine. But if the exception is
>       raised and handled in a higher level function, why do we make it a
>       critical error?
> 
>     - Admitting that rolling-back automatically on errors is wanted in some
>       cases, how can it be considered 'critical'? The caller will receive
>       the original exception anyway, it is its job to decide if it is
>       critical or not.
> 
> 
> 
>     So basically, I would like to propose 2 things:
> 
>     - Remove the 'critical' logs, and change them into into 'debug', or
>       'info'.
> 
> 
> +1

Ticket+patch: http://www.cubicweb.org/ticket/4801120

I chose 'debug' and 'info' levels for the rollback and sql error logs.
I wonder if using 'warning' could make sense.

> 
>     - Remove the 'rollback' option, or at least change its default value.
>       I really thing this feature has nothing to do in this place.
> 
> 
> Should this be moved up to the Connection object ?

It could be done in the connection context manager __exit__, if the
exceptions are not handled and if a 'auto-rollback' option is set when
opening the connection (which could be the default).

It would make more sense to me, and allow a user-provided exception
handler to make different decisions.


Christophe

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 473 bytes
Desc: OpenPGP digital signature
URL: <http://lists.cubicweb.org/pipermail/cubicweb/attachments/20141229/9fc9e46f/attachment-0050.sig>


More information about the Cubicweb mailing list