[JBPM-8380] #1477

karlnicholas · 2019-04-08T21:45:18Z

Started with #1449 and did additional work to remove JBPMHelper.java and JBPMUnitTestCase.
Moved static routines from JBPMHelper to ProcessMain where they were exclusively used.
Moved a couple static fields from JBPMHelper to JbpmJunitBaseTestBase.java.

set proper default values in jBPM.properties

ghost · 2019-04-08T21:46:05Z

Can one of the admins verify this PR? Comment with 'ok to test' to start the build.

elguardian · 2019-04-09T07:48:59Z

@MarianMacik @mswiderski this is based on my previous commit.

@karlnicholas plz keep in mind that there are other places where this is used
https://github.com/kiegroup/droolsjbpm-tools/blob/401c43085cb1c00df11c1bf325a7e2c39a61d865/drools-eclipse/org.jbpm.eclipse/src/main/resources/org/jbpm/eclipse/wizard/project/ProcessMain-advanced.java.v5.template

Also claim the Jira when you are working on it. otherwise will cause a misscordination among contributors.

Thanks for the contribution !

elguardian · 2019-04-09T08:07:19Z

this is ok to test

jbpm-test/src/test/java/org/jbpm/test/ProcessMain.java

karlnicholas · 2019-04-09T14:48:27Z

@MarianMacik @mswiderski this is based on my previous commit.
@karlnicholas plz keep in mind that there are other places where this is used
https://github.com/kiegroup/droolsjbpm-tools/blob/401c43085cb1c00df11c1bf325a7e2c39a61d865/drools-eclipse/org.jbpm.eclipse/src/main/resources/org/jbpm/eclipse/wizard/project/ProcessMain-advanced.java.v5.template
Also claim the Jira when you are working on it. otherwise will cause a misscordination among contributors.
Thanks for the contribution !

Hi,

Should a new Jira be opened for the droolsjbpm-tools? Should be easy to change?
I cannot change the Assignee field in JBPM Jira. Is there another way to claim the Jira?

elguardian · 2019-04-10T06:58:06Z

@karlnicholas use the same naming convention as this one (same branch name)

karlnicholas · 2019-04-11T23:17:55Z

@elguardian Okay, I looked at the droolsjbpm-tools and created PR kiegroup/droolsjbpm-tools#93.
One of the plugin code samples in the droolsjbpm-tools project is an exact match of https://github.com/kiegroup/jbpm/blob/master/jbpm-test/src/test/java/org/jbpm/test/ProcessMain.java and both of them require JBPMHelper to function properly. This is because JBPMHelper handles two JPA instantiations which these codes require. Previously I had just moved the code to the above ProcessMain but now I see it is required in two separate locations.

ProcessMain (and the plugin snippet) is not currently functioning properly without the changes to https://github.com/kiegroup/jbpm/blob/master/jbpm-test/src/test/filtered-resources/META-INF/persistence.xml made by this PR. This is because the schema for persistence unit "org.jbpm.services.task" has changed. I copied an updated version from somewhere else that works.

This means that the ProcessMain code was allowed to fall into disrepair since it is not tested during the build processes. I would recommend a new Jira be opened to convert ProcessMain to a properly functioning JUnit test case.

So, I did not completely remove JBPMHelper but I took away all dependencies from it except for the two places mentioned above and changed it to use PersistenceUtil for database requirements.

elguardian · 2019-04-16T08:01:23Z

@MarianMacik ^^

mswiderski · 2019-04-17T16:18:44Z

ok to test

karlnicholas · 2019-04-17T22:44:05Z

Also please issue ok to test on Remove JBPMHelper #93 droolsjbpm-tools changes.

mswiderski · 2019-04-18T09:30:38Z

@MarianMacik @elguardian guys, could you please review and approve so it can be merged?

MarianMacik · 2019-04-18T09:34:01Z

@karlnicholas So if I understood correctly you didn't remove JBPMHelper because it manages 2 datasources? Is this what you mean by JPA installations? I can see that JBPMHelper is instantiating the jdbc/jbpm-ds datasource here:

https://github.com/kiegroup/jbpm/pull/1477/files#diff-2ac0d6e56076eabf2e312f84f53a4663R54

But I cannot see where it is instantiating the second one for task service which is later used in this method:

https://github.com/kiegroup/jbpm/pull/1477/files#diff-2ac0d6e56076eabf2e312f84f53a4663R62

I don't understand why we cannot simply get rid of this class and move the relevant code to the let's say AbstractMain class or just create a new ExamplesUtil method or whatever. And I am wondering how it works with that second datasource since it is not created anywhere :) Then Persistence.createEntityManagerFactory(properties.getProperty("taskservice.datasource.name", "org.jbpm.services.task"), map); should simply fail.

MarianMacik · 2019-04-18T09:35:54Z

jbpm-examples/src/main/resources/datasource.properties

+user=sa
+password=
+url=jdbc:h2:tcp://localhost/~/jbpm-db
+datasourceName=jdbc/jbpm-ds


You can leave only these properties here, others should not be used by PersistenceUtil class. They are only used by current jBPMHelper class.

These two, task service are used by ProcessMain which uses JBPMHelper. See additional comments below.

taskservice.enabled=true
taskservice.datasource.name=org.jbpm.services.task

So now we definitely don't need these properties.

karlnicholas · 2019-04-18T11:19:31Z

@MarianMacik @elguardian -- Horribly long but sufficient explanation follows:

ProcessMain is the issue. It's in src/test/java/org/jbpm/test. This code is an exact copy of the code in the EclipsePlugin in the droolsjbpm-tools (somewhere, see other PR 93 above). This is the "two" places that JBPMHelper is used. This project and the other droolsjbpm-tools project.

In the droolsjbpm-tools project the code seems to be part of a template for an eclipse plugin. So, I assume after looking at it briefly that a user in eclipse can ask for the template and the class will be injected a project somewhere. Sort of a quickstart.

I don't know why the class (ProcessMain and also ProcessKModuleMain) are here in this project as well. I assume so that it can be run and tested. However the two classes are not tested as part of the build. They are both included as template code in the droolsjbpm-tools project but only ProcessMain is complicated enough to use JBPMHelper. ProcessKModuleMain is simple enough to just use PersistenceUtil only.

So, if you remove JBPMHelper the eclipse plugin will have template code that fails.

Having template code that fails is probably no big deal since it's not working at the moment anyway. This PR will fix it so that ProcessMain here in jbpm project will work, but at the cost of leaving JBPMHelper in the project. Even with the PR I'm not sure that the eclipse plugin template version of ProcessMain will work or even ever worked. The reason is that the code depends on being run from the test directory. See next paragraph.

Regards the second datasource mentioned. The persistence.xml file used by JBPMHelper contains several datasources. Once JPA reads it all the datasources are available to the running application. ProcessMain demonstrates using a TaskService so it not only connects to org.jbpm.persistence.jpa but also org.jbpm.services.task. The persistence.xml used is in src/test/filtered-resources/META-INF, not /src/main/resources/META-INF.

If you remove the taskservice.enabled=true property from the properties file then ProcessMain doesn't run the TaskService portion of the code. It doesn't fail, it just doesn't run it. Most (or all) of the code that handles the TaskService work for ProcessMain is in JBPMHelper. So ProcessMain looks very minimal but actually most of what it is doing is in JBPMHelper.

The Problem with moving all the code to ProcessMain is that it requires the datasource.properties file and the persistence.xml file to function properly and that is too complicated for an Eclipse plugin. I guess the Eclipse plugin just injects a single thing. To get around this whoever did the eclipse plugin put most of the functionality in JBPMHelper and made jbpm-test.jar a dependency.

Personally I would just remove ProcessMain from the project and the eclipse plugin and then you could also remove JBPMHelper but that is not my call. I seriously doubt, even with this PR, that the eclipse plugin template code that is ProcessMain functions well if you actually add it to a project. This is because the template ProcessMain code included in a project won't be running under test and so probably won't find the correct persistence.xml anyway.

MarianMacik · 2019-04-23T08:56:58Z

OK, I see that it those two are persistence units reusing one datasource. So the property taskservice.datasource.name is quite misleading.

Never mind, regarding the jbpm-examples/src/main/resources/datasource.properties file, I still don't see how it is used by JBPMHelper since JBPMHelper getProperties() method only reads JBPM.properties file and not datasource.properties one. So I still think those other properties can be omitted since they are taken from JBPM.properties file.

Regarding deletion of JBPMHelper. I understand that JBPMHelper is used in Eclipse plugin etc. But if you replace JBPMHelper with a new ExamplesUtil as I mentioned and you just leave there what is actually used, I think we would be safe. The issue with leaving JBPMHelper is that it has been marked as deprecated so I think we have to come up with a new class and only leave there functionality which we really need.

I now realized that JBPMHelper says:

Since version 6.0 this class is deprecated. Instead <code>RuntimeManager</code> should be used directly.
See documentation on how to use <code>RuntimeManager</code>

So I think we need to just rewrite ProcessMain as ProcessKModuleMain to use runtime manager and engine and obtain ksession from there. Then we can get rid of the loadStatefulKnowledgeSession method etc.
So we have 2 possibilities I think - replace JBPMHelper with let's say ExamplesUtil and retain there only functionality which is needed - which means migrate to RuntimeManager way of setup, or delete JBPMHelper and move desired code to the ProcessMain/ProcessKModuleMain classes - but both of them will use runtime manager. Can you try that?

karlnicholas · 2019-04-25T08:31:11Z

Okay, it wasn't so bad after looking a little closer to put the code completely in the ProcessMain and ProcessKModuleMain in jbpm-test and the plugin template as well. I just did away with reading any kind of configuration file and hardcoded driver parameters for H2. I think it is fine since it is just example code anyway. Really, it seems much cleaner anyway since you can easily see what's going on.

MarianMacik

Looks very good now. Just minor suggestions and we are good to merge.

MarianMacik · 2019-04-30T13:43:57Z

jbpm-examples/src/main/resources/datasource.properties

+user=sa
+password=
+url=jdbc:h2:tcp://localhost/~/jbpm-db
+datasourceName=jdbc/jbpm-ds


So now we definitely don't need these properties.

jbpm-test/src/test/java/org/jbpm/test/ProcessMain.java

MarianMacik · 2019-05-02T07:27:18Z

looks good now, will try to run full downstream build just to check if nothing is broken

jenkins execute full downstream build

MarianMacik · 2019-05-02T08:56:48Z

jenkins execute full downstream build

karlnicholas · 2019-05-02T10:37:49Z

Also don't forget PR kiegroup/droolsjbpm-tools#93.

MarianMacik · 2019-05-02T15:00:14Z

Yes, I know about that one too. For that we don't have downstream build, just the repo build.
Here, in FDB there were Just 2 unrelated tests, will trigger it once more and if it is OK, we can merge.

jenkins execute full downstream build

MarianMacik · 2019-05-03T08:55:22Z

OK, I think we are good to go here. @elguardian can you approve?

karlnicholas · 2019-05-04T19:02:00Z

Are this build errors a problem with this PR?

mareknovotny · 2019-05-06T07:28:11Z

2 from 3 failed tests were seen before so they are not related, but I am not sure about org.kie.wb.selenium.ui.ProjectLibraryIntegrationTest.importAndBuildProjectFromStockRepository

karlnicholas · 2019-05-07T20:20:12Z

2 from 3 failed tests were seen before so they are not related, but I am not sure about org.kie.wb.selenium.ui.ProjectLibraryIntegrationTest.importAndBuildProjectFromStockRepository

I looked: This is something from the Workbench and seems to be importing/testing a project named OptaCloud which seems to be about OptaPlanner. Shouldn't have anything to do with these changes.

MarianMacik · 2019-05-09T07:53:08Z

I am 100% sure issues are not related. Can you rebase so we will merge this?

karlnicholas · 2019-05-09T17:54:16Z

@MarianMacik Okay, I merged the latest changes from the master branch. Since there were conflicts I needed to create a new commit to support the merge changes. Should be good to go.

MarianMacik · 2019-05-09T18:31:41Z

jenkins retest this

MarianMacik · 2019-05-09T18:31:51Z

jenkins execute full downstream build

MarianMacik · 2019-05-10T09:48:54Z

jenkins retest this

MarianMacik · 2019-05-10T09:49:02Z

jenkins execute full downstream build

MarianMacik · 2019-05-13T15:19:04Z

Looks good after full downstream. @mswiderski any comments?
Please check also kiegroup/droolsjbpm-tools#93

mswiderski

looks good to me

[JBPM-8097] jBPMHelper no longer sets default values for H2

a139853

set proper default values in jBPM.properties

MarianMacik mentioned this pull request Apr 9, 2019

[JBPM-8097] jBPMHelper no longer sets default values for H2 #1449

Closed

MarianMacik reviewed Apr 9, 2019

View reviewed changes

jbpm-test/src/test/java/org/jbpm/test/ProcessMain.java Outdated Show resolved Hide resolved

jbpm-test/src/test/java/org/jbpm/test/ProcessMain.java Outdated Show resolved Hide resolved

karlnicholas mentioned this pull request Apr 11, 2019

[JBPM-8380] Remove JBPMHelper kiegroup/droolsjbpm-tools#93

Merged

MarianMacik reviewed Apr 18, 2019

View reviewed changes

MarianMacik reviewed Apr 30, 2019

View reviewed changes

karlnicholas closed this May 2, 2019

karlnicholas reopened this May 2, 2019

MarianMacik approved these changes May 3, 2019

View reviewed changes

karlnicholas added 2 commits May 9, 2019 10:14

Merge remote-tracking branch 'origin/master' into JBPM-8097

72010c8

[JBPM-8380] Remove JBPMHelper

66d17be

mswiderski approved these changes May 13, 2019

View reviewed changes

MarianMacik merged commit ae8e83a into kiegroup:master May 14, 2019

karlnicholas deleted the JBPM-8380 branch May 14, 2019 13:24

[JBPM-8380] #1477

[JBPM-8380] #1477

Conversation

karlnicholas commented Apr 8, 2019

ghost commented Apr 8, 2019

elguardian commented Apr 9, 2019

elguardian commented Apr 9, 2019

karlnicholas commented Apr 9, 2019

elguardian commented Apr 10, 2019

karlnicholas commented Apr 11, 2019 • edited

elguardian commented Apr 16, 2019

mswiderski commented Apr 17, 2019

karlnicholas commented Apr 17, 2019 • edited

mswiderski commented Apr 18, 2019

MarianMacik commented Apr 18, 2019

MarianMacik Apr 18, 2019

Choose a reason for hiding this comment

karlnicholas Apr 18, 2019

Choose a reason for hiding this comment

MarianMacik Apr 30, 2019

Choose a reason for hiding this comment

karlnicholas commented Apr 18, 2019 • edited

MarianMacik commented Apr 23, 2019

karlnicholas commented Apr 25, 2019

MarianMacik left a comment

Choose a reason for hiding this comment

MarianMacik Apr 30, 2019

Choose a reason for hiding this comment

MarianMacik commented May 2, 2019

MarianMacik commented May 2, 2019

karlnicholas commented May 2, 2019 • edited

MarianMacik commented May 2, 2019

MarianMacik commented May 3, 2019

karlnicholas commented May 4, 2019

mareknovotny commented May 6, 2019

karlnicholas commented May 7, 2019

MarianMacik commented May 9, 2019

karlnicholas commented May 9, 2019

MarianMacik commented May 9, 2019

MarianMacik commented May 9, 2019

MarianMacik commented May 10, 2019

MarianMacik commented May 10, 2019

MarianMacik commented May 13, 2019 • edited

mswiderski left a comment

Choose a reason for hiding this comment

karlnicholas commented Apr 11, 2019 •

edited

karlnicholas commented Apr 17, 2019 •

edited

karlnicholas commented Apr 18, 2019 •

edited

karlnicholas commented May 2, 2019 •

edited

MarianMacik commented May 13, 2019 •

edited