Avoid db.rollback() in upgrade_environment()

[pipern]

db.rollback() should be avoided in upgrade_environment(db), as this would rollback the actions of other plugins which are also upgrading at the same time.

I don't know if it's the best way - for now, I just commented out the db.rollback(). I'm not certain if it's needed after a SELECT throws an exception due to missing table.

[ejucovy]

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.

[Ryan J Ollos]

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!

[Ryan J Ollos]

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

[Ryan J Ollos]

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

[ejucovy]

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:

tracpaste/db.py

@@ -93 +93 @@
             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.

[Steffen Hoffmann]

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.

[Ryan J Ollos]

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.

[Steffen Hoffmann]

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

[Ryan J Ollos]

In 14798:

0.3.0dev: Adapt to Trac 1.1.6dev API. Fixes #5658, #7204.