issue on superstate hibernate mapping

[icyjamie]

Hello

there could be a problem on the superstate hibernate mapping. The list-index mapping to the nodes uses the column NODECOLLECTIONINDEX_. This column is also used in the processdefinition mapping, for the list-index mapping to the nodes.
If you create a process definition with some normal nodes and some superstate nodes and deploy it to the database, you will notice that the nodecollectionindex_ is filled with sequential numbers for BOTH the processdefinition and superstate. This can result in duplicate indexes (e.g. a '1' for a normal node, and a '1' for use within the superstate).
When you load a process definition, hibernate tries to load the nodes by using the nodecollectionindex_, to insert into a list. When it hits a node inside a superstate, it could get the same index as a previously loaded node, overwriting that node.
I will see if I can fix it, possibly by changing the reader, adding a superstateindex_ column, alter some hibernate mapping, in short, the works.

James

[icyjamie]

Of course, it's never easy. Superstates can have superstates etc. so simply adding a superstate index column will not do the trick.
Is there a reason why the nodesCollection is exposed a list (and a Map), and not a (Sorted)Set?
If it would be a (Sorted)Set, I guess things would be easier. You could sort on the ID_, as this is an ascending sequence, and could drop the nodecollectionindex_ column.

[icyjamie]

Last remark (and then I leave it to others to reply :-))
sorting on the id_ is not good, as there is no id (pun not intended) to start with. Hibernate provides it while persisting. The reader creates the processdefinition in memory, maybe the nodecollectionindex_ could be re-used for arbitrary ordening. Thoughts are welcome, because these are real design changes, and for now, I cannot use superstates.

[icyjamie]

Possible fix:

I noticed in the manual NodeCollection elements should be unique (by name, but let's see it as an identity). When adding nodes to the superstate (implementing NodeCollection), I noticed that the node had also the processdefinition filled in. This means that, when hibernate does its persisting bit, they are attached at the process definition level as well, so getting all the nodes from process definition will include these nodes as well, with all the trouble going on if it is mixed with superstates. Now for the fix: I've changed the addNode implementation in SuperState, setting the processdefinition to null. I ran the jbpm tests and they still succeed (apart from the milestone test, but I guess the change did not imply this failure). Now the test for my process definition succeeds also, so there is a good chance the fix does the job.

 public Node addNode(Node node) {
 if (node == null) throw new IllegalArgumentException("can't add a null node to a superstate");
 if (nodes == null) nodes = new ArrayList();
 nodes.add(node);
 node.superState = this;
 node.processDefinition = null; // this has been added
 nodesMap = null;
 return node;
 }

[icyjamie]

Sorry to bump, just wanting to know if anyone experienced the same problem if they used superstates

[icyjamie]

Hello

Should I post this issue in JIRA instead?

James

[koen.aers]

Sure, go ahead and enter a new JIRA issue. But can you please provide a simple unit test exposing the problem, please?

Thanks,
Koen

[icyjamie]

A testcase, using AbstractDbTestCase as base
This tests a couple of things:
scoping (maybe this should be in the non-db specific tests)
Persistency of superstate and reloading

package org.jbpm.db;

import org.jbpm.graph.def.ProcessDefinition;

public class SuperStateDbTest extends AbstractDbTestCase {

 public void testSuperStateMixedNormalNode() throws Exception {
 ProcessDefinition processDefinition = ProcessDefinition
 .parseXmlString("<process-definition>" +
 " <start-state name='s' />" +
 " <end-state name='end1' />" +
 " <node name='node1' />" +
 " <super-state name='superstate1'>" +
 " <node name='superstate1-node1'/>" +
 " <node name='superstate1-node2'/>" +
 " <node name='superstate1-node3'/>" +
 " </super-state>" +
 " <super-state name='superstate2'>" +
 " <node name='superstate2-node1'/>" +
 " <node name='superstate2-node2'/>" +
 " <node name='superstate2-node3'/>" +
 " </super-state>" +
 " <node name='node2' />" +
 "</process-definition>");
 graphSession.saveProcessDefinition( processDefinition );
 // get the assigned id
 long processDefinitionId = processDefinition.getId();
 // start a new transaction
 newTransaction();
 // load the process definition by the id
 processDefinition = graphSession.loadProcessDefinition( processDefinitionId );
 assertNotNull(processDefinition.findNode("node1"));
 assertNotNull(processDefinition.findNode("superstate1"));
 assertNotNull(processDefinition.findNode("superstate1/superstate1-node1"));
 assertNotNull(processDefinition.findNode("superstate1/superstate1-node2"));
 assertNotNull(processDefinition.findNode("superstate1/superstate1-node3"));
 assertNotNull(processDefinition.findNode("superstate2"));
 assertNotNull(processDefinition.findNode("superstate2/superstate2-node1"));
 assertNotNull(processDefinition.findNode("superstate2/superstate2-node2"));
 assertNotNull(processDefinition.findNode("superstate2/superstate2-node3"));
 assertNotNull(processDefinition.findNode("node2"));

 }

 public void testSuperStateScope() throws Exception {
 ProcessDefinition processDefinition = ProcessDefinition
 .parseXmlString("<process-definition>" +
 " <start-state name='s' />" +
 " <end-state name='end1' />" +
 " <node name='node1' />" +
 " <super-state name='superstate1'>" +
 " <node name='node1' />" +
 " <node name='nodea'/>" +
 " <node name='nodeb'/>" +
 " <node name='nodec'/>" +
 " </super-state>" +
 " <super-state name='superstate2'>" +
 " <node name='nodea'/>" +
 " <node name='nodeb'/>" +
 " <node name='nodec'/>" +
 " </super-state>" +
 " <node name='node2' />" +
 "</process-definition>");
 graphSession.saveProcessDefinition( processDefinition );
 // get the assigned id
 long processDefinitionId = processDefinition.getId();
 // start a new transaction
 newTransaction();
 // load the process definition by the id
 processDefinition = graphSession.loadProcessDefinition( processDefinitionId );
 assertNotNull(processDefinition.findNode("node1"));
 assertNotNull(processDefinition.findNode("superstate1"));
 assertNotNull(processDefinition.findNode("superstate1/node1"));
 assertNotNull(processDefinition.findNode("superstate1/nodea"));
 assertNotNull(processDefinition.findNode("superstate1/nodeb"));
 assertNotNull(processDefinition.findNode("superstate1/nodec"));
 assertNotNull(processDefinition.findNode("superstate2"));
 assertNotNull(processDefinition.findNode("superstate2/nodea"));
 assertNotNull(processDefinition.findNode("superstate2/nodeb"));
 assertNotNull(processDefinition.findNode("superstate2/nodec"));
 assertNotNull(processDefinition.findNode("node2"));

 }

 public void testSuperStateInSuperStateWithScope() throws Exception {
 ProcessDefinition processDefinition = ProcessDefinition
 .parseXmlString("<process-definition>" +
 " <start-state name='s' />" +
 " <end-state name='end1' />" +
 " <node name='node1' />" +
 " <super-state name='superstate1'>" +
 " <node name='nodea'/>" +
 " <node name='nodeb'/>" +
 " <node name='nodec'/>" +
 " <super-state name='superstate1.1'>" +
 " <node name='nodex'/>" +
 " <node name='nodey'/>" +
 " <node name='nodez'/>" +
 " </super-state>" +
 " <super-state name='superstate1.2'>" +
 " <node name='nodex'/>" +
 " <node name='nodey'/>" +
 " <node name='nodez'/>" +
 " </super-state>" +
 " <super-state name='superstate1.3'>" +
 " <node name='nodex'/>" +
 " <node name='nodey'/>" +
 " <node name='nodez'/>" +
 " </super-state>" +
 " </super-state>" +
 " <super-state name='superstate2'>" +
 " <node name='nodea'/>" +
 " <node name='nodeb'/>" +
 " <node name='nodec'/>" +
 " </super-state>" +
 " <node name='node2' />" +
 "</process-definition>");
 graphSession.saveProcessDefinition( processDefinition );
 // get the assigned id
 long processDefinitionId = processDefinition.getId();
 // start a new transaction
 newTransaction();
 // load the process definition by the id
 processDefinition = graphSession.loadProcessDefinition( processDefinitionId );
 assertNotNull(processDefinition.findNode("node1"));
 assertNotNull(processDefinition.findNode("superstate1"));
 assertNotNull(processDefinition.findNode("superstate1/nodea"));
 assertNotNull(processDefinition.findNode("superstate1/nodeb"));
 assertNotNull(processDefinition.findNode("superstate1/nodec"));
 assertNotNull(processDefinition.findNode("superstate1/superstate1.1"));
 assertNotNull(processDefinition.findNode("superstate1/superstate1.1/nodex"));
 assertNotNull(processDefinition.findNode("superstate1/superstate1.1/nodey"));
 assertNotNull(processDefinition.findNode("superstate1/superstate1.1/nodez"));
 assertNotNull(processDefinition.findNode("superstate1/superstate1.2"));
 assertNotNull(processDefinition.findNode("superstate1/superstate1.2/nodex"));
 assertNotNull(processDefinition.findNode("superstate1/superstate1.2/nodey"));
 assertNotNull(processDefinition.findNode("superstate1/superstate1.2/nodez"));
 assertNotNull(processDefinition.findNode("superstate1/superstate1.3"));
 assertNotNull(processDefinition.findNode("superstate1/superstate1.3/nodex"));
 assertNotNull(processDefinition.findNode("superstate1/superstate1.3/nodey"));
 assertNotNull(processDefinition.findNode("superstate1/superstate1.3/nodez"));
 assertNotNull(processDefinition.findNode("superstate2"));
 assertNotNull(processDefinition.findNode("superstate2/nodea"));
 assertNotNull(processDefinition.findNode("superstate2/nodeb"));
 assertNotNull(processDefinition.findNode("superstate2/nodec"));
 assertNotNull(processDefinition.findNode("node2"));
 }


}

Two changes must be performed to make it work:

In SuperState class

 public Node addNode(Node node) {
 if (node == null) throw new IllegalArgumentException("can't add a null node to a superstate");
 if (nodes == null) nodes = new ArrayList();
 nodes.add(node);
 node.superState = this;
 node.processDefinition = null; // this has been added
 nodesMap = null;
 return node;
 }

In JpdlXmlReader class
Order of setting name and adding node to parent has been reversed.

 public void readNode(Element nodeElement, Node node, NodeCollection nodeCollection) {

 // add the node to the parent
 nodeCollection.addNode(node);

 // get the action name
 String name = nodeElement.attributeValue("name");
 if (name!=null) {
 node.setName(name);
 }

.....

[koen.aers]

James,

Did you also post this in JIRA? If yes what is the issue number?

Regards,
Koen

[icyjamie]

http://jira.jboss.org/jira/browse/JBPM-345 for persisting bit

http://jira.jboss.org/jira/browse/JBPM-346 for scope bit

[icyjamie]

In Jira, there is no indication in which versions these two issues will be patched. Could someone give an indication?

James