Avoid db.rollback() in upgrade_environment()

This fix is necessary for the plugin to work on a fresh installation of Trac 0.12 and above, because the database connection that's used in the needs_upgrade step is a read-only connection. So the db.rollback() fails, resulting (mysteriously) in an AttributeError when running trac-admin env upgrade, and the system never notices it needs an upgrade.

On a fresh install of the plugin in 1.0dev @ t:r11053, I didn't see the upgrade database request message, and when navigating to the Pastebin tab, I saw the following error:

Trac detected an internal error:

OperationalError: no such table: pastes

This is consistent with comment:1 's observations, I think. I'm not to the point yet that I feel comfortable with the implementing and manipulating IEnvironmentSetupParticipant, but since this is a blocker and the issue seems to be fixed by removing the db.rollback()s, I'm going to apply the fix. I also wonder whether if it is really necessary or desirable to wrap all of the DB queries with try/excepts.

Code review and comments are welcome and appreciated!

(In [11687]) Refs #7204: Attempt to introduce Trac 0.12 compatibility by removing db.rollback()s, which are thought to present a problem because the DB connection is read-only.

Tested with Trac 0.13dev (1.0dev) @ t:r11053.

I've been using FullBlogPlugin as a guide in implementing several features. I wouldn't be surprised if the original author had been looking at FullBlogPlugin as well, a lot of the code looks very similar.

FullBlogPlugin doesn't have a single db.rollback in fullblogplugin/0.11/tracfullblog/db.py.

FWIW, I've been happily running this plugin (http://trac-hacks.org/svn/tracpasteplugin/0.11@r10450) on my Trac-dev environment (http://svn.edgewall.org/repos/trac/trunk@r10896) for ~6 months with only one of the db.rollback() lines removed:

Index: tracpaste/db.py
===================================================================
--- tracpaste/db.py	(revision 11062)
+++ tracpaste/db.py	(working copy)
@@ -93,7 +93,6 @@
             row = cursor.fetchone()
             return row and 1 or 0
         except:
-            db.rollback()
             return 0
 
         return 0

That said, I think it makes sense to remove all the db.rollback()s. I also think it would probably be reasonable to remove all those try .. except blocks -- Trac should be trusted to handle unexpected exceptions in plugin code. This level of error handling in the plugin will just bury the errors.

See t:#10451 for the original issue reported for Trac.

In short: For plugin db schema version checking TagsPlugin resorted to testing for table existence instead of using canonical entries in Trac db table system. And some others re-used this approach too. This was not very smart, but worked; until the Trac db API change in t:changeset:10179, where the .rollback() method has been removed consequently from all read-only connections. And this applies i.e. to the connection, that is handed over in the method environment_needs_upgrade() to all classes implementing IEnvironmentSetupParticipant.

So the best way is to switch over to the aforementioned explicit version number storage. A temporary work-around is proposed i.e. in changeset:11041 to resolve #9521 reported against TagsPlugin. You'll find it quite similar to this ticket - for a reason.

I'm still not entirely understanding when I should be using rollbacks and handling them as in [11041], but removing them seems to have helped with #10156.

I will revisit this soon and try to better understand the issue.

The issue has been discussed in #9521 and further in #5552, and what I meant by 'explicit' plugin db schema version numbering is implemented now for TagsPlugin in [12069].