Edgewall Software
Modify

Opened 15 years ago

Closed 9 years ago

Last modified 6 years ago

#8172 closed enhancement (fixed)

Plugin db upgrade infrastructure

Reported by: Felix Schwarz <felix.schwarz@…> Owned by: Peter Suter
Priority: low Milestone: 1.1.5
Component: general Version: none
Severity: minor Keywords:
Cc: osimons, Peter Suter, Ryan J Ollos, olemis@…, olemis+trac@… Branch:
Release Notes:

New helper methods on DatabaseManager class for plugins to upgrade the database.

API Changes:

Added needs_upgrade() and upgrade() methods to the DatabaseManager class.

Internal Changes:

Description

Trac currently has its own infrastructure to manage db upgrades. However for plugin authors there is no mechanism available for that so every plugin author needs to re-invent this again.

Attached is a module which basically uses trac's mechanism but is a bit more generic so this could be used by plugins as well.

Attachments (3)

plugin_env_setup.py (7.0 KB ) - added by Felix Schwarz <felix.schwarz@…> 15 years ago.
If there is a chance to get this into trac main, we can polish this class (including docs) and make trac use it too
db-upgrade-helpers.patch (6.7 KB ) - added by Peter Suter 13 years ago.
DB upgrade helper functions
T8172-upgrade-helpers.diff (4.3 KB ) - added by Peter Suter 9 years ago.
Extract DatabaseManager.upgrade() and DatabaseManager.needs_upgrade()

Download all attachments as: .zip

Change History (51)

by Felix Schwarz <felix.schwarz@…>, 15 years ago

Attachment: plugin_env_setup.py added

If there is a chance to get this into trac main, we can polish this class (including docs) and make trac use it too

comment:1 by Remy Blank, 15 years ago

Milestone: 1.0

Sounds useful.

comment:2 by Felix Schwarz <felix.schwarz@…>, 15 years ago

Hmm, I hoped we could have this in 0.12… Do you think this feature could make it?

comment:3 by Remy Blank, 15 years ago

About milestones, 0.12 means a developer is committed to integrating the patch in trunk. 0.13 means a developer is committed to integrating the patch, but in the next version. Everything beyond that means we would like to do it eventually, but nobody has taken it up yet.

Personally, I don't have enough knowledge about this part of Trac that I could review and integrate your patch. Sorry about that. If you can persuade another dev to take this up, he will move the milestone closer as well, possibly to 0.12.

comment:4 by Christian Boos, 14 years ago

Milestone: 1.0unscheduled

Milestone 1.0 deleted

comment:5 by Remy Blank, 14 years ago

Milestone: triaging0.13
Owner: set to Remy Blank

I'd like to pick this up. However, rather than having an abstract component from which plugins would inherit, how about creating a new IDBUpgradeParticipant interface that would be implemented in plugins and provide the plugin name, the expected DB version, possibly the package containing the upgrade scripts, as well as a method for the initial DB setup? Then a single component could implement the upgrade logic, and be used by core as well.

comment:6 by Christian Boos, 14 years ago

I also think we don't need to inherit from an abstract component here.

However, I'm not sure if it's worth to introduce a new interface, we could perhaps do as well with a set of helper methods (factoring out the code already in env.py and mixing it with some parts of the proposed patch).

comment:7 by Christian Boos, 14 years ago

… and yes, this would not only be useful for plugins, but also for core sub-systems, to get finer grained upgrades (see #9222).

comment:8 by osimons, 14 years ago

#9222… Well, this is back to the core vs plugins vs future of Trac development…

Personally I don't see the need for new interfaces either. I think the IEnvironmentSetupParticipant interface is perfectly capable of handling all kinds of upgrades. What is missing is, as also mentioned in #9222, moving upgrades into the various components parts of Trac and letting each self-contained part of Trac be responsible for its own upgrades.

This was the original vision of the component code before things got more and more 'tangled' into each other. For a remaining example, see for instance how attachment.py implements the interface for the sole purpose of creating an 'attachments' folder for new projects.

Naturally, the Trac core code can improve its (optional) services/functions/helpers for module and plugin upgrades - just don't invent a whole new infrastructure for this that will never be able to cover all needs anyway, IMHO.

comment:9 by osimons, 14 years ago

Cc: osimons added

in reply to:  8 comment:10 by Remy Blank, 14 years ago

Replying to osimons:

just don't invent a whole new infrastructure for this that will never be able to cover all needs anyway, IMHO.

That's a bit pessimistic, but I think you (and Christian) are right. Let's try to add a few helper functions to more easily allow distributing upgrades into individual modules and plugins.

That's one risk when you have a cool component architecture: you tend to use it too much…

Last edited 14 years ago by Remy Blank (previous) (diff)

by Peter Suter, 13 years ago

Attachment: db-upgrade-helpers.patch added

DB upgrade helper functions

comment:11 by Peter Suter, 13 years ago

Would the helper functions in the patch attached above be a step in the right direction? They mostly refactor out functionality from Environment and EnvironmentSetup for DB related environment setup (schema setup, initial data insertion, version check and per-version upgrades).

A plugin could quite easily reuse them to manage its DB schema:

from trac.core import *
from trac.env import * 
from trac.db.schema import Table, Column

PLUGIN_NAME = 'ExamplePlugin'
PLUGIN_VERSION = 2
PLUGIN_SCHEMA = [
    Table('table1', key='name')[
        Column('name'),
        Column('value')]]
def get_initial_plugin_data(db):
    return (('table1',
              ('name', 'value'),
                (('name1', 'value1'),
                 ('name2', 'value2'))),)
                 
class ExamplePlugin(Component):

    implements(IEnvironmentSetupParticipant)

    # IEnvironmentSetupParticipant methods

    def environment_created(self):
        init_environment_schema_for_module(self.env, PLUGIN_SCHEMA)
        init_environment_data_for_module(self.env, get_initial_plugin_data)
        init_environment_version_for_module(self.env, PLUGIN_NAME,
                                            PLUGIN_VERSION)

    def environment_needs_upgrade(self, db):
        return environment_needs_upgrade_for_module(self.env, PLUGIN_NAME,
                                                    PLUGIN_NAME,
                                                    PLUGIN_VERSION)

    def upgrade_environment(self, db):
        upgrade_environment_for_module(self.env, PLUGIN_NAME, PLUGIN_NAME, 
                                       PLUGIN_VERSION,
                                       'ExamplePlugin_upgrades')

(It could also freely mix and match these functions with its own code, or ignore them altogether.)

I'm not too sure with the names. Maybe it would be good to drop or replace the for_module suffix, include _db_ somewhere, or even move them to trac.db.util?

What do you think?

comment:12 by Peter Suter, 12 years ago

Cc: Peter Suter added

comment:13 by Remy Blank, 12 years ago

Milestone: 1.0next-major-0.1X

Moving "experimental" stuff post-1.0.

in reply to:  11 ; comment:14 by Peter Suter, 10 years ago

I still like this idea with more helpers functions.

Replying to psuter:

I'm not too sure with the names. Maybe it would be good to drop or replace the for_module suffix, include _db_ somewhere, or even move them to trac.db.util?

Maybe they should be methods on Environment or DatabaseManager.

The proposed init_environment_schema_for_module is actually almost identical to the new DatabaseManager.create_tables(schema) added in #11512.

And the proposed get_environment_version_for_module is very similar to Environment.get_version(). (Although that was deprecated(?) in #11681.)

I think we could instead add more useful methods:

  • Environment.get_database_version(name='database_version')
  • Environment.set_database_version(name, version)
  • Environment.needs_database_upgrade(name, version)
  • Environment.run_database_upgrades(name, version, upgrades_pkg)
  • DatabaseManager.insert_data(data) (optional: data is callable to receive db?)

The implementations would be similar to the functions in the patch.

The example plugin would look like this:

from trac.core import *
from trac.env import * 
from trac.db.schema import Table, Column

PLUGIN_NAME = 'ExamplePlugin'
PLUGIN_VERSION = 2
PLUGIN_SCHEMA = [
    Table('table1', key='name')[
        Column('name'),
        Column('value')]]
INITIAL_PLUGIN_DATA = (
    ('table1',
      ('name', 'value'),
        (('name1', 'value1'),
         ('name2', 'value2'))),)
                 
class ExamplePlugin(Component):

    implements(IEnvironmentSetupParticipant)

    # IEnvironmentSetupParticipant methods

    def environment_created(self):
        DatabaseManager(self).create_tables(PLUGIN_SCHEMA)
        DatabaseManager(self).insert_data(INITIAL_PLUGIN_DATA)
        self.env.set_database_version(PLUGIN_NAME, PLUGIN_VERSION)

    def environment_needs_upgrade(self, db):
        return self.env.needs_database_upgrade(PLUGIN_NAME, PLUGIN_VERSION)

    def upgrade_environment(self, db):
        self.env.run_database_upgrades(PLUGIN_NAME, PLUGIN_VERSION)

in reply to:  14 ; comment:15 by Ryan J Ollos, 10 years ago

Cc: Ryan J Ollos added

Replying to psuter:

I still like this idea with more helpers functions.

Same here. It's been on my mind for a while and I wasn't aware of this ticket. Sounds like there are some good ideas here.

Maybe they should be methods on Environment or DatabaseManager.

That seems like the right way to go, maybe even have all the helpers exist as methods of the DatabaseManager class if that works.

The proposed init_environment_schema_for_module is actually almost identical to the new DatabaseManager.create_tables(schema) added in #11512.

And the proposed get_environment_version_for_module is very similar to Environment.get_version(). (Although that was deprecated(?) in #11681.)

Deprecated as a public method in favor of the new properties Environment.database_version and Environment.database_initial_version. Effectively only the calling syntax has changed. The return value is the same. So Environment.get_version() is replaced by Environment.database_version and Environment.get_version(initial=True) is replaced by Environment.database_initial_version. The properties avoid a database hit on every access.

I think we could instead add more useful methods:

  • Environment.get_database_version(name='database_version')
  • Environment.set_database_version(name, version)

get_schema_version and set_schema_version might be good names as well, however since the upgrades can change the database version without modifying the schema, database_version might be better. Having the methods exist on the DatabaseManager class could avoid ambiguity with database_version of the Environment class.

  • Environment.needs_database_upgrade(name, version)
  • Environment.run_database_upgrades(name, version, upgrades_pkg)
  • DatabaseManager.insert_data(data) (optional: data is callable to receive db?)

insert_data might be useful in Trac unit tests as well.

in reply to:  15 comment:16 by Ryan J Ollos, 9 years ago

Replying to rjollos:

I think we could instead add more useful methods:

  • Environment.get_database_version(name='database_version')
  • Environment.set_database_version(name, version)

get_schema_version and set_schema_version might be good names as well, however since the upgrades can change the database version without modifying the schema, database_version might be better. Having the methods exist on the DatabaseManager class could avoid ambiguity with database_version of the Environment class.

get_database_version and set_database_version are proposed in #11859.

comment:17 by Ryan J Ollos, 9 years ago

Milestone: next-major-releasesnext-dev-1.1.x

in reply to:  14 comment:18 by Ryan J Ollos, 9 years ago

Replying to psuter:

  • DatabaseManager.insert_data(data) (optional: data is callable to receive db?)

Adding populate_tables method to DatabaseManager is proposed in #11893. I'm okay with the name insert_data as well. I chose populate_tables for symmetry with create_tables, drop_tables and reset_tables.

comment:19 by Peter Suter, 9 years ago

How about insert_into_tables?

comment:20 by Ryan J Ollos, 9 years ago

insert_into_tables sounds good. I'll make the change.

comment:22 by Olemis Lang <olemis+trac@…>, 9 years ago

Cc: olemis@… olemis+trac@… added

by Peter Suter, 9 years ago

Attachment: T8172-upgrade-helpers.diff added

Extract DatabaseManager.upgrade() and DatabaseManager.needs_upgrade()

comment:23 by Peter Suter, 9 years ago

After #11893 (DatabaseManager.insert_into_tables()) and #11859 (DatabaseManager.get_database_version() and DatabaseManager.set_database_version()) the main missing functionality is running upgrade scripts. In the attached patch I propose extracting DatabaseManager.upgrade() and DatabaseManager.needs_upgrade().

Example plugin:

  • example/
    • upgrades
      • __init__.py (empty)
      • db2.py
        def do_upgrade(env, ver, cursor):
            pass
        
    • __init__.py (empty)
    • core.py
      from trac.core import *
      from trac.env import *
      from trac.db.api import DatabaseManager
      from trac.db.schema import Table, Column
      
      PLUGIN_NAME = 'ExamplePlugin'
      PLUGIN_VERSION = 2
      PLUGIN_SCHEMA = [
          Table('table1', key='name')[
              Column('name'),
              Column('value')]]
      INITIAL_PLUGIN_DATA = (
          ('table1',
            ('name', 'value'),
              (('name1', 'value1'),
               ('name2', 'value2'))),)
      
      class ExamplePlugin(Component):
      
          implements(IEnvironmentSetupParticipant)
      
          # IEnvironmentSetupParticipant methods
      
          def environment_created(self):
              dbm = DatabaseManager(self.env)
              dbm.create_tables(PLUGIN_SCHEMA)
              dbm.insert_into_tables(INITIAL_PLUGIN_DATA)
              dbm.set_database_version(PLUGIN_VERSION, PLUGIN_NAME)
      
          def environment_needs_upgrade(self, db):
              dbm = DatabaseManager(self.env)
              return dbm.needs_upgrade(PLUGIN_VERSION, PLUGIN_NAME)
      
          def upgrade_environment(self, db):
              dbm = DatabaseManager(self.env)
              if dbm.get_database_version(PLUGIN_NAME) == 0:
                  dbm.create_tables(PLUGIN_SCHEMA)
                  dbm.insert_into_tables(INITIAL_PLUGIN_DATA)
                  dbm.set_database_version(PLUGIN_VERSION, PLUGIN_NAME)
              else:
                  dbm.upgrade(PLUGIN_VERSION, PLUGIN_NAME, 'example.upgrades')
      
  • setup.py
    #!/usr/bin/env python
    from setuptools import setup
    setup(name = 'example', version = '1.0',
          packages = ['example', 'example.upgrades'],
          entry_points = {'trac.plugins': ['example.core = example.core']})
    

in reply to:  23 comment:24 by Jun Omae, 9 years ago

Replying to psuter:

After #11893 (DatabaseManager.insert_into_tables()) and #11859 (DatabaseManager.get_database_version() and DatabaseManager.set_database_version()) the main missing functionality is running upgrade scripts. In the attached patch I propose extracting DatabaseManager.upgrade() and DatabaseManager.needs_upgrade().

I think the pkg argument of DatabaseManager.upgrade() should have None for the default value. If plugin author neglects to specify own package to it, that would lead pretty bad.

comment:25 by Ryan J Ollos, 9 years ago

Besides issue noted in comment:24, looks good to me. I did some minor refactoring in log:rjollos.git:t8172. Test coverage could be added to DatabaseManagerTestCase.

comment:26 by Ryan J Ollos, 9 years ago

Regarding ExamplePlugin in comment:23, you may already be away of these, but in case not:

  • The db arguments of environment_needs_upgrade and upgrade_environment are optional and deprecated.
  • environment_created could just call upgrade_environment.

comment:27 by Peter Suter, 9 years ago

Milestone: next-dev-1.1.x1.1.5
Owner: changed from Remy Blank to Peter Suter
Status: newassigned

Thanks for the reviews!

comment:28 by Peter Suter, 9 years ago

API Changes: modified (diff)
Release Notes: modified (diff)
Resolution: fixed
Status: assignedclosed

Committed in [13926].

in reply to:  26 comment:29 by Peter Suter, 9 years ago

I added the example plugin to wiki:TracDev/PluginDevelopment/ExtensionPoints/trac.env.IEnvironmentSetupParticipant#DBupgrades

Replying to rjollos:

  • The db arguments of environment_needs_upgrade and upgrade_environment are optional and deprecated.

Thanks for the reminder.

  • environment_created could just call upgrade_environment.

For example code it feels slightly clearer to repeat the code explicitly though.

comment:30 by Ryan J Ollos, 9 years ago

I'm not sure if you are on the builds mailing list - in case not, the most recent build failed.

comment:31 by Peter Suter, 9 years ago

Resolution: fixed
Status: closedreopened

Thanks. No, I don't seem to be. Just got the same error though.

comment:32 by Peter Suter, 9 years ago

The @lazy attribute database_version is deleted, in this scenario it happens not to be used though.

Not quite sure what the best fix is. Shouldn't that just be a no-op?

diff -r c7667f5dc388 trac/util/__init__.py
--- a/trac/util/__init__.py     Thu Mar 26 19:44:54 2015 +0000
+++ b/trac/util/__init__.py     Thu Mar 26 21:55:51 2015 +0100
@@ -1148,7 +1148,8 @@
         instance.__dict__[self.fn.__name__] = value
 
     def __delete__(self, instance):
-        del instance.__dict__[self.fn.__name__]
+        if self.fn.__name__ in instance.__dict__:
+            del instance.__dict__[self.fn.__name__]
 
 
 # -- algorithmic utilities

I don't see a way to check if has_been_set(self.database_version): del self.database_version.

I guess we could just set self.database_version = None before deleting it, but that seems weird.

comment:33 by Ryan J Ollos, 9 years ago

I like your patch to make it a no-op, since the lazy attributes would behave more like regular attributes.

comment:34 by Peter Suter, 9 years ago

OK thanks. Committed in [13927].

Should I open a separate ticket for documenting this as an API change?

comment:35 by Peter Suter, 9 years ago

(Alternatively instance.__dict__.pop(self.fn.__name__, None) or try: ... except KeyError: pass could be used.)

comment:36 by Ryan J Ollos, 9 years ago

Yeah, one of those might be considered more Pythonic … I'm not sure. They all look good to me ;)

Regarding API Changes, my general approach has been:

  • If I'm working on a ticket that is targeted to 1.1.x and the change is against 1.0.x, make a new ticket.
  • If there's not currently an open ticket (e.g. the item was discussed in a ticket closed in a previous milestone), create a new ticket.

Otherwise I just append to the API Changes in the current ticket. So normally I'd suggest just appending to API Changes in this ticket, but now that you've raised the point I wonder if it would be a good idea to backport [13927] to 1.0-stable.

comment:37 by Peter Suter, 9 years ago

Resolution: fixed
Status: reopenedclosed

OK, I opened #12008 for that.

comment:38 by Ryan J Ollos, 9 years ago

Release Notes: modified (diff)

comment:39 by Ryan J Ollos, 9 years ago

API Changes: modified (diff)

comment:40 by Ryan J Ollos, 9 years ago

CodeReviewerPlugin utilizes the new Database API methods, with compat.py adding the methods for backwards compatibility with Trac 1.0 ([th 14612]). When the plugin codebase supports a minimum of Trac 1.2 I'll just delete compat.py and modify the import statements for DatabaseManager.

I'll add more references here when other plugins are modified to utilize the new Database API methods. FullBlogPlugin is probably next.

Last edited 9 years ago by Ryan J Ollos (previous) (diff)

comment:41 by Ryan J Ollos, 9 years ago

It looks like environment_created will never be called for a plugin since trac.ini.sample is always used when creating the environment and therefore no plugins can be active at the time the environment is created: tags/trac-1.1.5/trac/env.py@:567#L543. If that's the case, every plugin should have:

def environment_created(self):
    pass

In order for environment_created to be called for a plugin, we'd need a mechanism to pass a trac.ini file to the TracAdmin initenv command (comment:7:ticket:11333).

in reply to:  41 comment:42 by Jun Omae, 9 years ago

Replying to rjollos:

It looks like environment_created will never be called for a plugin since trac.ini.sample is always used when creating the environment and therefore no plugins can be active at the time the environment is created: tags/trac-1.1.5/trac/env.py@:567#L543. If that's the case, every plugin should have:

We can use --inherit option of initenv command with [components] settings to activate plugins on created and invoke environment_created in the plugins.

$ virtualenv -q /dev/shm/env-created
$ /dev/shm/env-created/bin/pip install -q Genshi==0.6
$ /dev/shm/env-created/bin/pip install -q Trac==1.0.6.post2
$ /dev/shm/env-created/bin/pip install -q -e svn+https://trac-hacks.org/svn/announcerplugin/trunk
$ cat >/dev/shm/inherit.ini
[components]
announcer.* = enabled
$ /dev/shm/env-created/bin/trac-admin /dev/shm/tracenv initenv --inherit=/dev/shm/inherit.ini Name sqlite:db/trac.db
$ sqlite3 /dev/shm/tracenv/db/trac.db
SQLite version 3.7.9 2011-11-01 00:52:41
Enter ".help" for instructions
Enter SQL statements terminated with a ";"
sqlite> .tables
attachment              permission              subscription_attribute
auth_cookie             report                  system
cache                   repository              ticket
component               revision                ticket_change
enum                    session                 ticket_custom
milestone               session_attribute       version
node_change             subscription            wiki

subscription and subscription_attribute tables in announcerplugin are created via initenv command.

comment:43 by Ryan J Ollos, 9 years ago

Okay, I see the relevance. AnnouncerPlugin is just doing the same work in environment_created that it does in environment_upgraded (th:browser:announcerplugin/trunk/announcer/api.py@:409,423#L406). I can't think of a situation in which a plugin would want the environment_created behavior to differ from environment_upgraded. Instead, they must be the same, right, since the developer cannot know whether the plugin will be installed at the time the environment is created, or after?

Creating the tables in environment_created rather than having a pass statement may be a minor optimization though in that the user won't be prompted to upgrade the environment for the case demonstrated in comment:42.

The example in comment:23 should probably be modified if it is to serve as a canonical example for a plugin so we don't mislead the plugin author to think that environment_created will always be called when a plugin is installed. I mistakenly made that assumption.

Perhaps the following is more clear for the example in TracDev/PluginDevelopment/ExtensionPoints/trac.env.IEnvironmentSetupParticipant,

    # IEnvironmentSetupParticipant methods

    def environment_created(self):
        dbm = DatabaseManager(self.env)
        dbm.create_tables(PLUGIN_SCHEMA)
        dbm.insert_into_tables(INITIAL_PLUGIN_DATA)
        dbm.set_database_version(PLUGIN_VERSION, PLUGIN_NAME)

    def environment_needs_upgrade(self, db):
        dbm = DatabaseManager(self.env)
        return dbm.needs_upgrade(PLUGIN_VERSION, PLUGIN_NAME)

    def upgrade_environment(self, db):
        dbm = DatabaseManager(self.env)
        if dbm.get_database_version(PLUGIN_NAME) == 0:
            self.environment_created()
        else:
            dbm.upgrade(PLUGIN_VERSION, PLUGIN_NAME, 'example.upgrades')

Also I've considered the change if not dbm.get_database_version(PLUGIN_NAME) might be more clear since False is returned rather than 0: tags/trac-1.1.5/trac/db/api.py@:421#L410.

th:TracPastePlugin has been modified to use the DatabaseManager methods: th:browser:/tracpasteplugin/trunk/tracpaste/db.py@14799.

in reply to:  43 comment:44 by Peter Suter, 9 years ago

Replying to rjollos:

I can't think of a situation in which a plugin would want the environment_created behavior to differ from environment_upgraded. Instead, they must be the same, right, since the developer cannot know whether the plugin will be installed at the time the environment is created, or after?

I think you are right, it's very rare that they should not do exactly the same thing. Trac itself has EnvironmentSetup, Chrome and ConfigurableTicketWorkflow that do different things, but it's unlikely that (m)any external plugins would need this. Perhaps for leaving existing environments in a "backward compatibility" mode while creating new environments in a "modern" mode? As pointed out this would only really work for new environments created with the plugin activate via the --inherit option.

The example in comment:23 should probably be modified if it is to serve as a canonical example for a plugin so we don't mislead the plugin author to think that environment_created will always be called when a plugin is installed. I mistakenly made that assumption.

I agree making that more obvious would be good, but I think this would best be explained in the documentation. Even if you change the example code as proposed, the text should point out that you don't have to call environment_created(), and that it will sometimes be called automatically.

Also I've considered the change if not dbm.get_database_version(PLUGIN_NAME) might be more clear since False is returned rather than 0: tags/trac-1.1.5/trac/db/api.py@:421#L410.

== 0 makes more sense to me intuitively, but I can see that it maybe hides the subtle details a bit too well.

comment:45 by Ryan J Ollos, 7 years ago

Edited trac.env.IEnvironmentSetupParticipant.

Regarding the first bullet in additional information and references:

  • It seems like data corruption is avoided by using a transaction context manager and putting all upgrade steps in a single transaction (upgrade tables, copy data, update database version in system table)
  • The database will be backed-up before running upgrade for SQLite, MySQL and PostgreSQL

Part of that paragraph existed back in version 4. If someone confirms those two points are correct I'll update the documentation.

Last edited 7 years ago by Ryan J Ollos (previous) (diff)

comment:46 by Ryan J Ollos, 7 years ago

Should we recommend wrapping multiple operations in a transaction context manager?:

    def upgrade_environment(self):
        dbm = DatabaseManager(self.env)
        if dbm.get_database_version(PLUGIN_NAME) == 0:
            with self.env.db_transaction:
                dbm.create_tables(PLUGIN_SCHEMA)
                dbm.insert_into_tables(INITIAL_PLUGIN_DATA)
                dbm.set_database_version(PLUGIN_VERSION, PLUGIN_NAME)
        else:
            dbm.upgrade(PLUGIN_VERSION, PLUGIN_NAME, 'example.upgrades')

The reasoning is, we don't want the tables to be created if the operation of inserting data into the tables fails. I might be misremembering, but with at least one database type I seem to recall that the tables would be created even if the transaction was not committed, so maybe it doesn't matter.

See also comment:4:ticket:12645.

comment:48 by Ryan J Ollos, 6 years ago

The pkg keyword argument of DatabaseManager.upgrade cannot left as None when calling the method. The argument is required by the implementation of upgrade. We probably should have made pkg a positional argument rather than a keyword argument:

  • trac/db/api.py

    diff --git a/trac/db/api.py b/trac/db/api.py
    index 2be066063..b43190b0b 100644
    a b class DatabaseManager(Component):  
    544544                      name, dbver, version)
    545545        return True
    546546
    547     def upgrade(self, version, name='database_version', pkg=None):
     547    def upgrade(self, version, pkg, name='database_version'):
    548548        """Invokes `do_upgrade(env, version, cursor)` in module
    549549        `"%s/db%i.py" % (pkg, version)`, for each required version upgrade.

However, if we are to preserve API compatibility, the following may be a better change:

  • trac/db/api.py

    diff --git a/trac/db/api.py b/trac/db/api.py
    index 2be066063..bc2339b13 100644
    a b class DatabaseManager(Component):  
    544544                      name, dbver, version)
    545545        return True
    546546
    547     def upgrade(self, version, name='database_version', pkg=None):
     547    def upgrade(self, version, name='database_version', pkg='trac.upgrades'):
    548548        """Invokes `do_upgrade(env, version, cursor)` in module
    549549        `"%s/db%i.py" % (pkg, version)`, for each required version upgrade.
    550550
  • trac/env.py

    diff --git a/trac/env.py b/trac/env.py
    index 6e57b6af7..98417adee 100644
    a b class EnvironmentSetup(Component):  
    883883        return DatabaseManager(self.env).needs_upgrade(db_default.db_version)
    884884
    885885    def upgrade_environment(self):
    886         DatabaseManager(self.env).upgrade(db_default.db_version,
    887                                           pkg='trac.upgrades')
     886        DatabaseManager(self.env).upgrade(db_default.db_version)
    888887        self._update_sample_config()
    889888
    890889    # Internal methods

Then at least name and pkg are defaulting to values used by Trac and it may be more obvious that a plugin will need to set both kw params.

comment:49 by Ryan J Ollos, 6 years ago

comment:48 change committed to 1.2-stable in r16605, merged to trunk in r16606.

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain Peter Suter.
The resolution will be deleted. Next status will be 'reopened'.
to The owner will be changed from Peter Suter 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.