Edgewall Software
Modify

Opened 12 years ago

Closed 12 years ago

Last modified 12 years ago

#9968 closed defect (wontfix)

with_transaction may skip transaction function when ommiting parameters

Reported by: Sebastian Krysmanski <sebastian@…> Owned by:
Priority: normal Milestone:
Component: database backend Version: 0.12.1
Severity: major Keywords:
Cc: Branch:
Release Notes:
API Changes:
Internal Changes:

Description

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.

Attachments (1)

db_api.diff (1014 bytes ) - added by Sebastian Krysmanski <sebastian@…> 12 years ago.
Patch against Trac 0.12.1

Download all attachments as: .zip

Change History (4)

by Sebastian Krysmanski <sebastian@…>, 12 years ago

Attachment: db_api.diff added

Patch against Trac 0.12.1

comment:1 by Christian Boos, 12 years ago

Component: generaldatabase backend
Resolution: wontfix
Status: newclosed

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.

in reply to:  1 comment:2 by Sebastian Krysmanski <sebastian@…>, 12 years ago

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.

comment:3 by Christian Boos, 12 years ago

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).

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The ticket will remain with no owner.
The resolution will be deleted. Next status will be 'reopened'.
to The owner will be changed from (none) to the specified user.

Add Comment


E-mail address and name can be saved in the Preferences .
 
Note: See TracTickets for help on using tickets.