Use drop scripts to cleanup,validate quartz tables #1573

gmunozfe · 2019-12-05T11:11:27Z

Fixing tests for most of the databases (still pending db2 and sybase, only for upgrading scripts) in the community-tests-jbpm-db-matrix pipeline.

Instead of delegating in hibernate hbm2ddl to drop constraints and tables, tests now are invoking drop scripts and hence, these scripts are tested too.
Removed tests with hbm2ddl.auto "create-drop" or "update", as constraint hashes generated by Hibernate are now different than those coming from the DDL scripts (kept these for backward compatibility).
In some upgrade-scripts, there were incorrect delimiters and syntax that have been fixed.
Added support for new dialect SybaseASE157Dialect; this change also implies modification in the sybase-jbpm-schema.sql because this dialect (the latest for sybase) is overriding
registerColumnType( Types.BIGINT, "bigint" );
instead of SybaseDialect that keeps:
registerColumnType( Types.BIGINT, "numeric(19,0)" );
Therefore, for right validation, it's needed to switch in the script the type "numeric(19,0)" to "bigint".
Added new sql Quartz drop scripts to handle dropping through these files, and keep the database cleaned at the end of the tests.
Quartz tables are now also validated with the hdm2ddl validate strategy and using mocked Quartz entities and a new persistence unit dbQuartzValidate. Hence, we make sure the JPA entity mapping schema is in sync with the database one (as it's done with jBPM tables).
Entity classes define the mapping for all databases, therefore, to keep the scripts as they are, some tuning has been needed for validating through custom dialects.
This persistence unit does not delegate into maven dialect property (as for jBPM tables), but a new dialect resolver "resolves" the custom dialect which overrides some mappings SQL-java. For example, for the boolean type, there are Quartz scripts defining it as varchar (mysql, sql server and oracle), bit (sybase) or integer (db2). Even db2 defines both -varchar and integer- that's the reason of keeping just one of them for correct mapping.

MarianMacik

Finally went over it. Very good job! I would be interested mainly where you found that Hibernate Dialect adaptation because I found almost nothing when I tried to find it on the web.

There are some comments, just to be sure I am on the same page as you are.

MarianMacik · 2019-12-06T14:40:09Z

jbpm-installer/src/main/resources/db/ddl-scripts/db2/quartz_tables_db2.sql

-    BOOL_PROP_1 VARCHAR(1),
-    BOOL_PROP_2 VARCHAR(1),
+    BOOL_PROP_1 INTEGER,
+    BOOL_PROP_2 INTEGER,


Not sure if this is backward compatible, I mean if somebody will try to migrate data from one DB to the newly created DB. I presume these scripts shouldn't be changed as they come directly from Quartz?

In the quartz db2 schemas (tables_db2 and tables_db2_v72) boolean are mapped always to varchar(1). This is different for schemas (tables_db2_v8, tables_db2_v95), where SQL boolean type is mapped into:

varchar(1) for BOOL_PROP_1, BOOL_PROP_2 at qrtz_simprop_triggers table

integer for is_durable, is_nonconcurrent, is_update_data, requests_recovery at qrtz_job_details table and is_nonconcurrent, requests_recovery at qrtz_fired_triggers table.

Now I've unified like in tables_db2, but if you don't feel comfortable because this change is not backward-compatible, I can leave it as it was (db2 is already failing for upgrading scripts)

jbpm-installer/src/main/resources/db/ddl-scripts/derby/quartz_tables_drop_derby.sql

MarianMacik · 2019-12-06T14:46:03Z

jbpm-installer/src/main/resources/db/ddl-scripts/mysqlinnodb/quartz_tables_mysql_innodb.sql

-DROP TABLE IF EXISTS QRTZ_TRIGGERS;
-DROP TABLE IF EXISTS QRTZ_JOB_DETAILS;
-DROP TABLE IF EXISTS QRTZ_CALENDARS;
-


Although this is backward-compatible, I think it would be good to mention in release notes or in documentation in general that drop scripts were introduced, meaning that if previously drop commands were a part of normal scripts, now they are in a separate file.

jbpm-installer/src/main/resources/db/ddl-scripts/sybase/quartz_tables_sybase.sql

jbpm-installer/src/main/resources/db/ddl-scripts/sybase/sybase-jbpm-schema.sql

MarianMacik · 2019-12-09T13:26:55Z

...-installer/src/test/java/org/jbpm/persistence/scripts/quartzdialects/MySQLCustomDialect.java

+
+    public MySQLCustomDialect() {
+        super();
+        registerColumnType(Types.BLOB, "blob");


This is for DDL to properly validate the type?

yes, it assigns the "blob" from the script to the sql code of hibernate for managing BLOB

MarianMacik · 2019-12-09T13:35:27Z

...-installer/src/test/java/org/jbpm/persistence/scripts/quartzdialects/MySQLCustomDialect.java

+    @Override
+    public SqlTypeDescriptor remapSqlTypeDescriptor(SqlTypeDescriptor sqlTypeDescriptor) {
+        if (sqlTypeDescriptor.getSqlType() == java.sql.Types.BLOB) {
+            return BinaryTypeDescriptor.INSTANCE;


Why is this change needed?

In fact, this is not needed, removed

MarianMacik · 2019-12-09T13:47:56Z

...-installer/src/test/java/org/jbpm/persistence/scripts/quartzdialects/MySQLCustomDialect.java

+    @Override
+    public void contributeTypes(TypeContributions typeContributions, ServiceRegistry serviceRegistry) {
+        super.contributeTypes(typeContributions,serviceRegistry);
+        typeContributions.contributeType(new Boolean2Varchar());


Couldn't we just remap the SQLTypeDescriptor? If I understand it correctly, when Hibernate finds a boolean field, it then converts this using the standard org.hibernate.type.BooleanType class, which has this( org.hibernate.type.descriptor.sql.BooleanTypeDescriptor.INSTANCE, BooleanTypeDescriptor.INSTANCE ); as SQL type descriptors and Java type descriptors. By remapping the sql descriptors we would just remap Boolean one to the Varchar one and we don't need the custom Boolean2Varchar class. Wdyt?

yes, you're right, we can register it as:
registerColumnType(Types.BOOLEAN, "varchar(1)");
and those classes (Boolean2Varchar, etc.) are not needed

...nstaller/src/test/java/org/jbpm/persistence/scripts/quartzmockentities/QrtzBlobTriggers.java

jbpm-installer/src/test/java/org/jbpm/persistence/scripts/util/SQLScriptUtil.java

gmunozfe · 2019-12-11T07:49:54Z

@MarianMacik I've gone through all your comments, thanks a lot for your review!

jbpm-installer/src/test/java/org/jbpm/persistence/scripts/util/SQLScriptUtil.java

jbpm-installer/src/test/java/org/jbpm/persistence/scripts/TestPersistenceContext.java

MarianMacik · 2019-12-11T12:11:42Z

jbpm-installer/src/test/java/org/jbpm/persistence/scripts/TestPersistenceContext.java

        testIsInitialized();
-        final File[] sqlScripts = TestsUtil.getDDLScriptFilesByDatabaseType(scriptsRootFolder, databaseType, true);
+        final File[] sqlScripts = TestsUtil.getDDLScriptFilesByDatabaseType(scriptsRootFolder, databaseType, true, dropFilesExcluded);
+        if (sqlScripts.length == 0 && databaseType!=DERBY && dropFilesExcluded) {


So what if there are no drop scripts, are we OK with that?

MarianMacik

Looks good. I am glad that we don't have to invent custom types now :)

jbpm-installer/src/test/java/org/jbpm/persistence/scripts/TestPersistenceContext.java

jbpm-installer/src/main/resources/db/ddl-scripts/sybase/quartz_tables_sybase.sql

...-installer/src/test/java/org/jbpm/persistence/scripts/quartzmockentities/QrtzJobDetails.java

gmunozfe · 2019-12-12T09:54:03Z

After looking at ddl60 drop scripts, we cannot remove them: they are needed because the constraint hashes for 6.0 are different than 7.x; therefore, we need to drop these constraints with its proper name before dropping the table, because some of them are foreign keys: h2, derby and oracle don't need this, that's the reason of missing drop scripts in their ddl60 folders, and that's why I was only checking the list of scripts were not null for "create" scenarios (not drop).

MarianMacik

Just one question, otherwise looks very good!

jbpm-installer/src/test/resources/ddl60/mysql5/mysql5-jbpm-drop-schema.sql

jbpm-installer/src/test/java/org/jbpm/persistence/scripts/UpgradeScriptsTest.java

MarianMacik · 2019-12-16T16:44:21Z

jenkins retest this

gmunozfe · 2019-12-17T09:02:44Z

jenkins retest this

MarianMacik · 2019-12-17T12:26:36Z

@sutaakar can you look at it as well? We are green, but just to have another pair of eyes :)

sutaakar · 2019-12-17T12:56:57Z

Briefly checked.

gmunozfe requested a review from MarianMacik December 5, 2019 11:12

MarianMacik reviewed Dec 9, 2019

View reviewed changes

Use drop scripts to cleanup,validate quartz tables

ac88d87

gmunozfe force-pushed the ddl_community branch from 6355366 to eafa760 Compare December 11, 2019 07:29

MarianMacik reviewed Dec 11, 2019

View reviewed changes

jbpm-installer/src/test/java/org/jbpm/persistence/scripts/TestPersistenceContext.java Outdated Show resolved Hide resolved

jbpm-installer/src/test/java/org/jbpm/persistence/scripts/TestPersistenceContext.java Outdated Show resolved Hide resolved

MarianMacik reviewed Dec 11, 2019

View reviewed changes

jbpm-installer/src/main/resources/db/ddl-scripts/sybase/quartz_tables_sybase.sql Show resolved Hide resolved

MarianMacik reviewed Dec 11, 2019

View reviewed changes

...-installer/src/test/java/org/jbpm/persistence/scripts/quartzmockentities/QrtzJobDetails.java Outdated Show resolved Hide resolved

gmunozfe force-pushed the ddl_community branch from eafa760 to 0f8d61f Compare December 12, 2019 09:46

MarianMacik reviewed Dec 12, 2019

View reviewed changes

jbpm-installer/src/test/resources/ddl60/mysql5/mysql5-jbpm-drop-schema.sql Outdated Show resolved Hide resolved

jbpm-installer/src/test/java/org/jbpm/persistence/scripts/UpgradeScriptsTest.java Outdated Show resolved Hide resolved

Fixes after first review

2a2816a

gmunozfe force-pushed the ddl_community branch from 0f8d61f to 2a2816a Compare December 12, 2019 22:00

MarianMacik approved these changes Dec 16, 2019

View reviewed changes

sutaakar approved these changes Dec 17, 2019

View reviewed changes

MarianMacik merged commit 2504404 into kiegroup:master Dec 17, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use drop scripts to cleanup,validate quartz tables #1573

Use drop scripts to cleanup,validate quartz tables #1573

gmunozfe commented Dec 5, 2019

MarianMacik left a comment

MarianMacik Dec 6, 2019

gmunozfe Dec 11, 2019

MarianMacik Dec 6, 2019

MarianMacik Dec 9, 2019

gmunozfe Dec 10, 2019

MarianMacik Dec 9, 2019

gmunozfe Dec 10, 2019

MarianMacik Dec 9, 2019

gmunozfe Dec 10, 2019

gmunozfe commented Dec 11, 2019

MarianMacik Dec 11, 2019

MarianMacik left a comment

gmunozfe commented Dec 12, 2019

MarianMacik left a comment

MarianMacik commented Dec 16, 2019

gmunozfe commented Dec 17, 2019

MarianMacik commented Dec 17, 2019

sutaakar commented Dec 17, 2019

Use drop scripts to cleanup,validate quartz tables #1573

Use drop scripts to cleanup,validate quartz tables #1573

Conversation

gmunozfe commented Dec 5, 2019

MarianMacik left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gmunozfe commented Dec 11, 2019

Choose a reason for hiding this comment

MarianMacik left a comment

Choose a reason for hiding this comment

gmunozfe commented Dec 12, 2019

MarianMacik left a comment

Choose a reason for hiding this comment

MarianMacik commented Dec 16, 2019

gmunozfe commented Dec 17, 2019

MarianMacik commented Dec 17, 2019

sutaakar commented Dec 17, 2019