Merge process causes duplicate entries in parent transitions. [SWF-1094]

[spring-operator]

Maksar opened SWF-1094 and commented

As a result, after merging in flow3:list:show will be 6 actions (which is correct), but in flow1:list:show will be the 6 objects too (which is wrong).

Wrong behavior takes place, because actions shares the same LinkedList.

In your source code:
public void merge(Model model) {
TransitionModel transition = (TransitionModel) model;
setOnException(merge(getOnException(), transition.getOnException()));
setTo(merge(getTo(), transition.getTo()));
setBind(merge(getBind(), transition.getBind()));
setBind(merge(getValidate(), transition.getValidate())); <---- besides, are you sure this string is correct?
setHistory(merge(getHistory(), transition.getHistory()));
setAttributes(merge(getAttributes(), transition.getAttributes()));
setSecured((SecuredModel) merge(getSecured(), transition.getSecured()));
setActions(merge(getActions(), transition.getActions(), false)); <------------- wrong.
}

Proposed change:
LinkedList mergeResult = merge(getActions(), transition.getActions(), false);
setActions(mergeResult != null ? new LinkedList(mergeResult) : null);

---

Affects: 2.0.5

Attachments:

• SWF-1094.patch (24.34 kB)
• swf1094.zip (1.80 kB)
• testProject.zip (18.26 kB)
• testProject2.zip (12.35 kB)

[spring-operator]

Maksar commented

The same with merging lists. You should make a new list instead of using same.

Another side-effect: for example, the same TransitionModel object instances can exist in different flows (after merging flows). If after that any other merge process will be occured (which touches TransitionModel instance) list of transition actions in parent flow can be changed.

[spring-operator]

Keith Donald commented

Hi there,

We have been unable to reproduce the problem using the example you provided. I've attached a test case we ran against 2.0.7 maintenance which does pass. We did make a change to the merge method. Here is the diff we made:

Index: spring-webflow/src/main/java/org/springframework/webflow/engine/model/AbstractModel.java

---

--- spring-webflow/src/main/java/org/springframework/webflow/engine/model/AbstractModel.java (revision 2363)
+++ spring-webflow/src/main/java/org/springframework/webflow/engine/model/AbstractModel.java (working copy)
@@ -90,7 +90,11 @@
*/
protected LinkedList merge(LinkedList child, LinkedList parent, boolean addAtEnd) {
if (child == null) {

return parent;

  if (parent == null) {

  return null;

  } else {

  return new LinkedList(parent);

          }
  } else if (parent == null) {
          return child;
  } else {

Basically, I don't see in this design how the parent can be modified--the child instance is local to the object being merged so returning a direct reference back to it should be fine, unless I'm missing something. I agree that sharing references between the parent and child was not good, hence the fix above.

If you can take a look at the tests and modify it as needed to reproduce the issue you're seeing that'd be great.

[spring-operator]

Maksar commented

Sorry, my mistake in environment conditions.

I've attached new test with flow definitions.

Here is the idea how I've fixed the defect. It was done by assigning a "deep copy" of merged Model to the child merging object. In this case all Model-s in child tree will be different instances from target Model tree. Deep copy must be assignes not only when merging collections, but also when merging Model-s.

[spring-operator]

Keith Donald commented

I reviewed your project and the merge algorithm is actually executing as designed - 9 is correct, not 6. This is because actions are not mergeable, and your flow is extending flow1 and flow2, where flow2 also extends flow1. With the current design, flow1 will be merged with flow3, then flow2 will be merged with flow3. Because flow2 extends from flow1 as well, the 'list' state gets merged into flow3 twice--once from flow3 directly extending from flow1, and again from the fact flow3 extends flow2 which extends flow1. This then applies the merge algorithm to flow3's list state twice, which then causes 6 additional actions to be added, giving a total number of 9.

If you believe you have a solution that employs a better approach for this scenario, feel free to attach a patch and we'll review it.

[spring-operator]

Maksar commented

Hi there,

Please review new achive, attached by((testProject2.zip) me once more time (it have differect flow xml-s in comparison to the your first attachment).

In third attachment (testProject2.zip) flow3 contains 6 actions (correct), but flow1 (and flow2 too) contains 6 actions too (flow1 and flow3 shares the same TransitionModel instance) -- which is NOT correct.

Test failure was occured by flow1

ViewStateModel state = (ViewStateModel) flow1.getStateById("list");
TransitionModel t = (TransitionModel) state.getTransitions().get(0);
assertEquals(3, t.getActions().size()); <--- we have 6 here.

and flow2

state = (ViewStateModel) flow2.getStateById("list");
t = (TransitionModel) state.getTransitions().get(0);
assertEquals(3, t.getActions().size()); <--- we have 6 here too.

In first original attachment (swf1094.zip) provided by you, flow3 must contain 9 actions and this is correct, absolutelly agree with you.

[spring-operator]

Keith Donald commented

I see, thanks for hanging in there. Definitely a bug.

So I propose the following fix.

First, add a createCopy() method to Model interface:

Index: spring-webflow/src/main/java/org/springframework/webflow/engine/model/Model.java

---

--- spring-webflow/src/main/java/org/springframework/webflow/engine/model/Model.java (revision 2363)
+++ spring-webflow/src/main/java/org/springframework/webflow/engine/model/Model.java (working copy)
@@ -35,4 +35,10 @@
*/
public void merge(Model model);

  /**

  * Create a deep copy of this model. Needed when merging models and collections.

  * @return a deep copy of this model

  */

  public Model createCopy();

}

Then in AbstractModel:

Index: spring-webflow/src/main/java/org/springframework/webflow/engine/model/AbstractModel.java

---

--- spring-webflow/src/main/java/org/springframework/webflow/engine/model/AbstractModel.java (revision 2366)
+++ spring-webflow/src/main/java/org/springframework/webflow/engine/model/AbstractModel.java (working copy)
@@ -59,7 +59,8 @@
*/
protected Model merge(Model child, Model parent) {
if (child == null) {

return parent;

          return parent.createCopy();
  } else if (parent == null) {
          return child;
  } else {

@@ -114,9 +115,11 @@
}
if (!matchFound) {
if (addAtEnd) {

child.addLast(parentElement);

          child.addLast(parentElement.createCopy());
  } else {

  child.addFirst(parentElement);

                          child.addFirst(parentElement.createCopy());
                  }
          }
  }

[spring-operator]

Keith Donald commented

BTW, if you agree with this approach, we could use some help implementing these 21 createCopy methods on each of the models! We're also fighting a few more fires in other JIRA open for 2.0.7 so any assistance you can provide here in terms of a patch would be very helpful and help the fix get done faster.

[spring-operator]

Maksar commented

Ok, I'll provide a patch in a day or two. The solution must be 1.4 compilant, right?

[spring-operator]

Keith Donald commented

Yes, must be 1.4 compliant. We'll standby for your patch. Thanks.

[spring-operator]

Maksar commented

Hi,

I've attached a patch. Should be applied to the SVNROOT/springframework/spring-webflow/trunk/spring-webflow folder.

[spring-operator]

Keith Donald commented

Patched applied. Much appreciated. Would you mind doing a quick review and test in your environment to ensure the patch took care of all issues?