SchemaCreation considering TablePrefix

[catandgrep]

Hi folks,

is there a need for extending DbSchema creation routines to take care of the table prefix and database schema? The current release does not allow schema creation with prefixes by the activiti engine. Instead admins need to manually create/update all the tables.

It would be quiet handy, to start a dedicated engine (e.g. with non-tx data source) and execute multiple schema creations through the management service for different runtime engines (e.g. for different tenants in larger activiti scenarios).

Is there any special reason why schema creation was not parameterized yet? If not I would contribute the change, but i would might need some help to test it on all databases. Most part of the change would go into DbSqlSession and (due to additional placeholders) to the schema sql files.

The management service of course already has the functionality to trigger schema creation/update, but without prefix.

Tell me what you think.

Regards
Andreas
——–
agito

[jkronegg]

I understand the idea of keeping the ".sql" scripts free of placeholders for raw usage in database, but the current situation is a bit embarassing in 5.15.1: you change the schema or databaseTablePrefix in a configuration with automatic table creation and get ugly exceptions about "Table <yourPrefix>XXX not found" because the table was created without the <yourPrefix> prefix. This makes you think that Activiti has some issues. I had to use the debugger to see how the table creation is done and look on the forum to find this post after several keyword search on Google and on the forum. I don't think most people evaluating Activiti will do that…

Why not have a kind of automatic placeholder for table creation in the DbSqlSession#executeSchemaResource instead of having them in the ".sql" file ?
The `sqlStatement` variable could be modified as such:
<code>
String prefix = … // determined from dbSqlSessionFactory.getDatabaseTablePrefix() and dbSqlSessionFactory.getDatabaseSchema ;
sqlStatement = sqlStatement.replaceFirst("create table (?!"+prefix+")", "create table "+prefix); // if there is no prefix, we add it
</code>

The alternative would be to raise an error when a configuration mismatch is present e.g.
<code>
if (isAutomaticSchemaCreationOrUpdateSpecified && isSchemaOrDatabaseTablePrefixSpecified) {
    throw new IllegalArgumentException("automatic schema creation/update is not allowed with custom schema or databaseTablePrefix");
}
</code>

[frederikherema1]

The replacing approach should be done with a lot of constructs (alter table, add index, …) and is error-prone IMHO. Having a warning/error might make sense, although the javadocs on ProcessEngineConfiguration are rather clear on the behaviour:


/**
   * Allows configuring a database table prefix which is used for all runtime operations of the process engine.
   * For example, if you specify a prefix named 'PRE1.', activiti will query for executions in a table named
   * 'PRE1.ACT_RU_EXECUTION_'.
   *
   * <p />
   * <strong>NOTE: the prefix is not respected by automatic database schema management. If you use
   * {@link ProcessEngineConfiguration#DB_SCHEMA_UPDATE_CREATE_DROP}
   * or {@link ProcessEngineConfiguration#DB_SCHEMA_UPDATE_TRUE}, activiti will create the database tables
   * using the default names, regardless of the prefix configured here.</strong> 
   *
   * @since 5.9
   */
  protected String databaseTablePrefix = "";
 
  /**
   * In some situations you want to set the schema to use for table checks / generation if the database metadata
   * doesn't return that correctly, see https://jira.codehaus.org/browse/ACT-1220,
   * https://jira.codehaus.org/browse/ACT-1062
   */
  protected String databaseSchema = null;


[jkronegg]

You're right, I hadn't thought about all the SQL constructs and the javadoc describes the issue. However, the javadoc is on the field and is not displayed when when you look at the getter/setter in an IDE such as Eclipse and only a few people will read the code before using the product.

I'm in favor of raising an exception because this would clearly notify the developper that some misconfiguration occured (the warning may not be seen). And this would prevent to polluate the database with table without prefixes when the developper wants tables with prefix.

But you could even make it friendlier to the community by adding a <code>protected String transformSqlStatement(String sqlStatement) { return sqlStatement; }</code> which is called on the "sqlStatement" just before executing it, and provide a way to make the DbSqlSession pluggable so that developpers can use their own implementation with an overrided "transformSqlStatement" method.

Julien

[frederikherema1]

Julien, makes sense. Any chance you're willing to contribute this fix to activiti, since you're deep into it already

Source-jar's and javadoc-jars should be in maven central (since 5.14), so if you're using maven in your IDE, it should download the sources and show javadoc.

[jkronegg]

Having studied the table generation deeper, I don't think its possible to have both the exception check and the "transformSqlStatement" if a "transformation plugin" is present because we will not be aware of the plugin behavior (would it implement the prefix stuff or only format the query for pretty printing when logged?).

Moreover, the behavior is really difficult to predict since the developper may want to change the configuration from one version to the other. For example if he first uses databaseTablePrefix=A then for an update a databaseTablePrefix=B (both with schemaUpdate=true), the script should drop A*, then create B*. So the "transformSqlRequest plugin" would need to know both prefixes to build the tables. Definitely a difficult task…

I ended up with the following patch:
<code>
— C:/partage/Softs/Java/Librairies/Activiti/activiti-5.15.1/libs/activiti-engine-5.15.1-sources/org/activiti/engine/impl/db/DbSqlSession.java Fri May 09 00:15:00 2014
+++ C:/Dev/workspace/activiti-src/activiti-engine-5.15.1-sources/org/activiti/engine/impl/db/DbSqlSession.java Fri May 09 00:03:13 2014
@@ -32,6 +32,7 @@
import java.util.regex.Pattern;

import org.activiti.engine.ActivitiException;
+import org.activiti.engine.ActivitiIllegalArgumentException;
import org.activiti.engine.ActivitiOptimisticLockingException;
import org.activiti.engine.ActivitiWrongDbException;
import org.activiti.engine.ProcessEngine;
@@ -1124,6 +1125,12 @@
  
   public void performSchemaOperationsProcessEngineBuild() {
     String databaseSchemaUpdate = Context.getProcessEngineConfiguration().getDatabaseSchemaUpdate();
+    String databaseTablePrefix = Context.getProcessEngineConfiguration().getDatabaseTablePrefix();
+ if (!org.activiti.engine.ProcessEngineConfiguration.DB_SCHEMA_UPDATE_FALSE.equals(databaseSchemaUpdate) &&
+  databaseTablePrefix!=null && !databaseTablePrefix.isEmpty()) {
+     // update/update_create_drop/drop_create not compatible with prefix => raise error
+  throw new ActivitiIllegalArgumentException("databaseTablePrefix is not supported when using databaseSchemaUpdate, see http://forums.activiti.org/content/schemacreation-considering-tableprefix");
+    }
     if (ProcessEngineConfigurationImpl.DB_SCHEMA_UPDATE_DROP_CREATE.equals(databaseSchemaUpdate)) {
       try {
         dbSchemaDrop();
</code>

It probably miss the trick between the databaseSchema and the databaseTablePrefix.