Set database version in DatabaseManager.create_tables

[Ryan J Ollos]

This is a commonly-used snippet of code in upgrade_environment:

dbm = DatabaseManager(self.env)
dbm.create_tables(tables)
dbm.set_database_version(db_version, db_version_key)

Recently I've considered that it would be better to wrap the two database transactions in a context manager, so that the operations are atomic:

dbm = DatabaseManager(self.env)
with self.env.db_transaction:
    dbm.create_tables(tables)
    dbm.set_database_version(db_version, db_version_key)

(e.g. ​browser:componenthierarchyplugin/1.2/componenthierarchy/model.py@16131:34-36#L31).

To promote the practice, we could add an argument to create_tables that sets the value of the specified version key to 1.

dbm = DatabaseManager(self.env)
dbm.create_tables(tables, db_version_key)

The set_database_version method would then be called in create_tables in a way that all operations are atomic.

[Ryan J Ollos]

Proposed changes in log:rjollos.git:t12645, also including a fix for #12643.

[Jun Omae]

I consider we should wrap participant.upgrade_environment with with self.db_transaction rather the adding parameter. However, I'm not sure why removing self.db_transcation in [14888].

-            args = ()
-            with self.db_transaction as db:
-                if arity(participant.upgrade_environment) == 1:
-                    args = (db,)
-                participant.upgrade_environment(*args)
+            participant.upgrade_environment()

[Christian Boos]

I'm not sure I follow… If we remove the db parameter (r14888), the participant has to call db_transaction by itself in its upgrade_environment method, so wrapping the call to participant.upgrade_environment with db_transaction again doesn't do much…

If an environment upgrade participant needs to write to the db, the fact that it has to call with self.db_transaction itself is a good think:

• it makes it clear in the participant's code what the scope of the transaction is,
• no need to create a transaction when the upgrade step doesn't need it