with_transaction may skip transaction function when ommiting parameters

[Sebastian Krysmanski <sebastian@…>]

Using the decorator @with_transaction without parameters skips the transaction function at all. This just happend to me by accident and it took me a while to figure out, why the function was being skipped.

Here's some example code:

def my_method(self):
  from trac.db import with_transaction
  print "Before do_transaction"

  # No parantheses by accident
  @with_transaction
  def do_transaction(cnx):
    print "In do_transaction"
      
  print "After do_transaction"

Running this method just outputs:

Before do_transaction
After do_transaction

The transaction function do_transaction() is completely skipped.

I've attached a patch that raises an exception in this case.

This patch also "corrects" the use of a deprecated function. Sorry for that but I was too lazy to create a new ticket for this.

[Sebastian Krysmanski <sebastian@…>]

Patch against Trac 0.12.1

[Christian Boos]

Well, if you look through the 0.12-stable code, the actual usage is nearly always @env.with_transaction(), that way you won't risk to forget env.

As for using env.get_db_cnx() instead of get_read_db() in this one case, there's a very good reason for that, it's unfortunately necessary for the unit tests, see ticket:9536#comment:6 for the detailed explanation. Fixing this was a major pain, but was finally done for trunk as you can see in the follow-up comments on that ticket.

In general, I don't think it's worth improving further the 0.12 db api, as it's a "transitional" API which has now been much improved on trunk. OTOH, feedback and possible further improvements on the 0.13dev db API would be welcome.

[Sebastian Krysmanski <sebastian@…>]

Replying to cboos:

Well, if you look through the 0.12-stable code, the actual usage is nearly always @env.with_transaction(), that way you won't risk to forget env.

Maybe not in Trac's own code but think of the plugin writers that may use this decorator. I don't understand what the harm is in "fixing" this problem.

[Christian Boos]

The plugins should follow the usage pattern in Trac, i.e. don't use the @with_transaction(env) form, but rather @env.with_transaction(). That would be the way to fix the problem, my fault for not having written TracDev/DatabaseApi in that direction from the start (this should be updated).