Plugin db upgrade infrastructure

[Felix Schwarz <felix.schwarz@…>]

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.

[Felix Schwarz <felix.schwarz@…>]

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

[Remy Blank]

Sounds useful.

[Felix Schwarz <felix.schwarz@…>]

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

[Remy Blank]

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.

[Christian Boos]

Milestone 1.0 deleted

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

[Christian Boos]

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

[Christian Boos]

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

[osimons]

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

[Remy Blank]

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…

[Peter Suter]

DB upgrade helper functions

[Peter Suter]

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?

[Remy Blank]

Moving "experimental" stuff post-1.0.

[Peter Suter]

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)

[Ryan J Ollos]

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.

[Ryan J Ollos]

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.

[Ryan J Ollos]

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.

[Peter Suter]

How about insert_into_tables?

[Ryan J Ollos]

insert_into_tables sounds good. I'll make the change.

[Peter Suter]

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

[Peter Suter]

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']})
  

[Jun Omae]

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.

[Ryan J Ollos]

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

[Ryan J Ollos]

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.

[Peter Suter]

Thanks for the reviews!

[Peter Suter]

Committed in [13926].

[Peter Suter]

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.

[Ryan J Ollos]

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

[Peter Suter]

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

[Peter Suter]

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.

[Ryan J Ollos]

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

[Peter Suter]

OK thanks. Committed in [13927].

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

[Peter Suter]

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

[Ryan J Ollos]

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.

[Peter Suter]

OK, I opened #12008 for that.

[Ryan J Ollos]

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

[Ryan J Ollos]

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

[Jun Omae]

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.

[Ryan J Ollos]

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.

[Peter Suter]

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.

[Ryan J Ollos]

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.

[Ryan J Ollos]

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.

[Ryan J Ollos]

Edited TracDev/PluginDevelopment/ExtensionPoints/trac.env.IEnvironmentSetupParticipant@19.

[Ryan J Ollos]

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

@@ -544 +544 @@
                       name, dbver, version)
         return True
 
-    def upgrade(self, version, name='database_version', pkg=None):
+    def upgrade(self, version, pkg, name='database_version'):
         """Invokes `do_upgrade(env, version, cursor)` in module
         `"%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

@@ -544 +544 @@
                       name, dbver, version)
         return True
 
-    def upgrade(self, version, name='database_version', pkg=None):
+    def upgrade(self, version, name='database_version', pkg='trac.upgrades'):
         """Invokes `do_upgrade(env, version, cursor)` in module
         `"%s/db%i.py" % (pkg, version)`, for each required version upgrade.
 

trac/env.py

@@ -883 +883 @@
         return DatabaseManager(self.env).needs_upgrade(db_default.db_version)
 
     def upgrade_environment(self):
-        DatabaseManager(self.env).upgrade(db_default.db_version,
-                                          pkg='trac.upgrades')
+        DatabaseManager(self.env).upgrade(db_default.db_version)
         self._update_sample_config()
 
     # 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.

[Ryan J Ollos]

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