search

bpmn-io / bpmn-js-differ

A diffing utility for BPMN 2.0 documents.
44 stars 16 forks source link

Edge is wrongly classified as layoutChange #18

Closed 38leinaD closed 1 month ago

38leinaD commented 6 months ago

Describe the Bug

I have a simple BPMN diagram where a new service was added. The edge going from A to End is now an edge from A to B. It is wrongly classified as a layoutchange only whereas it is a semantical change:

image

The JSON diff is this:

{
    "_layoutChanged": {
        "Flow_0dhn14f": {
            "$type": "bpmn:SequenceFlow",
            "id": "Flow_0dhn14f"
        }
    },
    "_changed": {},
    "_removed": {},
    "_added": {
        "Activity_0d0duli": {
            "$type": "bpmn:Task",
            "id": "Activity_0d0duli",
            "name": "B"
        },
        "Flow_19qppgm": {
            "$type": "bpmn:SequenceFlow",
            "id": "Flow_19qppgm"
        }
    }
}

Steps to Reproduce

  1. Create simple flow with Start -> A -> End.
  2. Save this as version 1
  3. Drag a new service on the edge between A and End.
  4. Save this as version 2
  5. Import version 1 and version 2 into https://demo.bpmn.io/diff

Expected Behavior

Exepected would have been for it to go into the "changed" section.

Environment

I used the only version of https://demo.bpmn.io/diff as well as running locally with "bpmn-js-differ": "2.0.2".

nikku commented 6 months ago

Not sure if I agree with your sentiment. The sequence flow technically did not change it's identity, but waypoints + target only.

I think it would be confusing to mark it as "removed" on one side and "added" on the other side. Or do you suggest that we should mark it as changed instead of layout changed?

Update: I miss read your expected behavior. I agree we should mark the flow as changed.

nikku commented 6 months ago

I can reproduce the issue. Moving to backlog for the time being. If you want to contribute a fix, PRs are open.

38leinaD commented 5 months ago

Hi, I would be willing to try myself. Can you provide some guidance maybe?

So, this my "before.bpmn":

<?xml version="1.0" encoding="UTF-8"?>
<definitions xmlns="http://www.omg.org/spec/BPMN/20100524/MODEL" xmlns:bpmndi="http://www.omg.org/spec/BPMN/20100524/DI" xmlns:omgdi="http://www.omg.org/spec/DD/20100524/DI" xmlns:omgdc="http://www.omg.org/spec/DD/20100524/DC" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" id="sid-38422fae-e03e-43a3-bef4-bd33b32041b2" targetNamespace="http://bpmn.io/bpmn" exporter="bpmn-js (https://demo.bpmn.io)" exporterVersion="17.0.2">
  <process id="Process_1" isExecutable="false">
    <startEvent id="Event_03i80dp">
      <outgoing>SequenceFlow_1</outgoing>
    </startEvent>
    <endEvent id="Event_0mtynoy">
      <incoming>SequenceFlow_2</incoming>
    </endEvent>
    <task id="Task_1" name="A">
      <incoming>SequenceFlow_1</incoming>
      <outgoing>SequenceFlow_2</outgoing>
    </task>
    <sequenceFlow id="SequenceFlow_1" sourceRef="Event_03i80dp" targetRef="Task_1" />
    <sequenceFlow id="SequenceFlow_2" sourceRef="Task_1" targetRef="Event_0mtynoy" />
  </process>
  <bpmndi:BPMNDiagram id="BpmnDiagram_1">
    <bpmndi:BPMNPlane id="BpmnPlane_1" bpmnElement="Process_1">
      <bpmndi:BPMNShape id="Event_03i80dp_di" bpmnElement="Event_03i80dp">
        <omgdc:Bounds x="152" y="102" width="36" height="36" />
      </bpmndi:BPMNShape>
      <bpmndi:BPMNShape id="Task_1_di" bpmnElement="Task_1">
        <omgdc:Bounds x="290" y="80" width="100" height="80" />
        <bpmndi:BPMNLabel />
      </bpmndi:BPMNShape>
      <bpmndi:BPMNShape id="Event_0mtynoy_di" bpmnElement="Event_0mtynoy">
        <omgdc:Bounds x="612" y="102" width="36" height="36" />
      </bpmndi:BPMNShape>
      <bpmndi:BPMNEdge id="SequenceFlow_1_di" bpmnElement="SequenceFlow_1">
        <omgdi:waypoint x="188" y="120" />
        <omgdi:waypoint x="290" y="120" />
      </bpmndi:BPMNEdge>
      <bpmndi:BPMNEdge id="SequenceFlow_2_di" bpmnElement="SequenceFlow_2">
        <omgdi:waypoint x="390" y="120" />
        <omgdi:waypoint x="612" y="120" />
      </bpmndi:BPMNEdge>
    </bpmndi:BPMNPlane>
  </bpmndi:BPMNDiagram>
</definitions>

And this is my "after,bpmn":

<?xml version="1.0" encoding="UTF-8"?>
<definitions xmlns="http://www.omg.org/spec/BPMN/20100524/MODEL" xmlns:bpmndi="http://www.omg.org/spec/BPMN/20100524/DI" xmlns:omgdi="http://www.omg.org/spec/DD/20100524/DI" xmlns:omgdc="http://www.omg.org/spec/DD/20100524/DC" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" id="sid-38422fae-e03e-43a3-bef4-bd33b32041b2" targetNamespace="http://bpmn.io/bpmn" exporter="bpmn-js (https://demo.bpmn.io)" exporterVersion="17.0.2">
  <process id="Process_1" isExecutable="false">
    <startEvent id="Event_03i80dp">
      <outgoing>SequenceFlow_1</outgoing>
    </startEvent>
    <endEvent id="Event_0mtynoy">
      <incoming>SequenceFlow_3</incoming>
    </endEvent>
    <task id="Task_1" name="A">
      <incoming>SequenceFlow_1</incoming>
      <outgoing>SequenceFlow_2</outgoing>
    </task>
    <sequenceFlow id="SequenceFlow_1" sourceRef="Event_03i80dp" targetRef="Task_1" />
    <sequenceFlow id="SequenceFlow_2" sourceRef="Task_1" targetRef="Task_2" />
    <task id="Task_2" name="B">
      <incoming>SequenceFlow_2</incoming>
      <outgoing>SequenceFlow_3</outgoing>
    </task>
    <sequenceFlow id="SequenceFlow_3" sourceRef="Task_2" targetRef="Event_0mtynoy" />
  </process>
  <bpmndi:BPMNDiagram id="BpmnDiagram_1">
    <bpmndi:BPMNPlane id="BpmnPlane_1" bpmnElement="Process_1">
      <bpmndi:BPMNShape id="Event_03i80dp_di" bpmnElement="Event_03i80dp">
        <omgdc:Bounds x="152" y="102" width="36" height="36" />
      </bpmndi:BPMNShape>
      <bpmndi:BPMNShape id="Task_1_di" bpmnElement="Task_1">
        <omgdc:Bounds x="290" y="80" width="100" height="80" />
        <bpmndi:BPMNLabel />
      </bpmndi:BPMNShape>
      <bpmndi:BPMNShape id="Event_0mtynoy_di" bpmnElement="Event_0mtynoy">
        <omgdc:Bounds x="612" y="102" width="36" height="36" />
      </bpmndi:BPMNShape>
      <bpmndi:BPMNShape id="Task_2_di" bpmnElement="Task_2">
        <omgdc:Bounds x="450" y="80" width="100" height="80" />
        <bpmndi:BPMNLabel />
      </bpmndi:BPMNShape>
      <bpmndi:BPMNEdge id="SequenceFlow_1_di" bpmnElement="SequenceFlow_1">
        <omgdi:waypoint x="188" y="120" />
        <omgdi:waypoint x="290" y="120" />
      </bpmndi:BPMNEdge>
      <bpmndi:BPMNEdge id="SequenceFlow_2_di" bpmnElement="SequenceFlow_2">
        <omgdi:waypoint x="390" y="120" />
        <omgdi:waypoint x="450" y="120" />
      </bpmndi:BPMNEdge>
      <bpmndi:BPMNEdge id="SequenceFlow_3_di" bpmnElement="SequenceFlow_3">
        <omgdi:waypoint x="550" y="120" />
        <omgdi:waypoint x="612" y="120" />
      </bpmndi:BPMNEdge>
    </bpmndi:BPMNPlane>
  </bpmndi:BPMNDiagram>
</definitions>

If i look at the output of the diffpatch, i get this:

{
  "rootElements": {
    "0": {
      "flowElements": {
        "5": [
          {
            "$type": "bpmn:Task",
            "id": "Task_2",
            "name": "B"
          }
        ],
        "6": [
          {
            "$type": "bpmn:SequenceFlow",
            "id": "SequenceFlow_3"
          }
        ],
        "_t": "a"
      }
    },
    "_t": "a"
  },
  "diagrams": {
    "0": {
      "plane": {
        "planeElement": {
          "3": [
            {
              "$type": "bpmndi:BPMNShape",
              "id": "Task_2_di",
              "bounds": {
                "$type": "dc:Bounds",
                "x": 450,
                "y": 80,
                "width": 100,
                "height": 80
              },
              "label": {
                "$type": "bpmndi:BPMNLabel"
              }
            }
          ],
          "5": {
            "waypoint": {
              "1": [
                {
                  "$type": "dc:Point",
                  "x": 450,
                  "y": 120
                }
              ],
              "_t": "a",
              "_1": [
                {
                  "$type": "dc:Point",
                  "x": 612,
                  "y": 120
                },
                0,
                0
              ]
            }
          },
          "6": [
            {
              "$type": "bpmndi:BPMNEdge",
              "id": "SequenceFlow_3_di",
              "waypoint": [
                {
                  "$type": "dc:Point",
                  "x": 550,
                  "y": 120
                },
                {
                  "$type": "dc:Point",
                  "x": 612,
                  "y": 120
                }
              ]
            }
          ],
          "_t": "a"
        }
      }
    },
    "_t": "a"
  }
}

There is no diff item reflecting that the targetRef of "SequenceFlow_2" changed. Is my understanding correct that the issue probably is in the diffpatch library already?

Update: As the diffpatch does the diff on JSON level, i looked at the JSON for the BPMN diagram representation as provided by the BpmnModdle.fromXML and i am slightly confused why in the JSON there is no information representing the interconnection between elements at all. It just lists out the elements but noone says "Task_2 is connected to Task_3 via SequenceFlow_2". What am i missing?

Here the moddle JSON of after.bpmn (i did JSON.stringify on the object reference):

{
  "$type": "bpmn:Definitions",
  "id": "sid-38422fae-e03e-43a3-bef4-bd33b32041b2",
  "targetNamespace": "http://bpmn.io/bpmn",
  "exporter": "bpmn-js (https://demo.bpmn.io)",
  "exporterVersion": "17.0.2",
  "rootElements": [
    {
      "$type": "bpmn:Process",
      "id": "Process_1",
      "isExecutable": false,
      "flowElements": [
        {
          "$type": "bpmn:StartEvent",
          "id": "Event_03i80dp"
        },
        {
          "$type": "bpmn:EndEvent",
          "id": "Event_0mtynoy"
        },
        {
          "$type": "bpmn:Task",
          "id": "Task_1",
          "name": "A"
        },
        {
          "$type": "bpmn:SequenceFlow",
          "id": "SequenceFlow_1"
        },
        {
          "$type": "bpmn:SequenceFlow",
          "id": "SequenceFlow_2"
        },
        {
          "$type": "bpmn:Task",
          "id": "Task_2",
          "name": "B"
        },
        {
          "$type": "bpmn:SequenceFlow",
          "id": "SequenceFlow_3"
        }
      ]
    }
  ],
  "diagrams": [
    {
      "$type": "bpmndi:BPMNDiagram",
      "id": "BpmnDiagram_1",
      "plane": {
        "$type": "bpmndi:BPMNPlane",
        "id": "BpmnPlane_1",
        "planeElement": [
          {
            "$type": "bpmndi:BPMNShape",
            "id": "Event_03i80dp_di",
            "bounds": {
              "$type": "dc:Bounds",
              "x": 152,
              "y": 102,
              "width": 36,
              "height": 36
            }
          },
          {
            "$type": "bpmndi:BPMNShape",
            "id": "Task_1_di",
            "bounds": {
              "$type": "dc:Bounds",
              "x": 290,
              "y": 80,
              "width": 100,
              "height": 80
            },
            "label": {
              "$type": "bpmndi:BPMNLabel"
            }
          },
          {
            "$type": "bpmndi:BPMNShape",
            "id": "Event_0mtynoy_di",
            "bounds": {
              "$type": "dc:Bounds",
              "x": 612,
              "y": 102,
              "width": 36,
              "height": 36
            }
          },
          {
            "$type": "bpmndi:BPMNShape",
            "id": "Task_2_di",
            "bounds": {
              "$type": "dc:Bounds",
              "x": 450,
              "y": 80,
              "width": 100,
              "height": 80
            },
            "label": {
              "$type": "bpmndi:BPMNLabel"
            }
          },
          {
            "$type": "bpmndi:BPMNEdge",
            "id": "SequenceFlow_1_di",
            "waypoint": [
              {
                "$type": "dc:Point",
                "x": 188,
                "y": 120
              },
              {
                "$type": "dc:Point",
                "x": 290,
                "y": 120
              }
            ]
          },
          {
            "$type": "bpmndi:BPMNEdge",
            "id": "SequenceFlow_2_di",
            "waypoint": [
              {
                "$type": "dc:Point",
                "x": 390,
                "y": 120
              },
              {
                "$type": "dc:Point",
                "x": 450,
                "y": 120
              }
            ]
          },
          {
            "$type": "bpmndi:BPMNEdge",
            "id": "SequenceFlow_3_di",
            "waypoint": [
              {
                "$type": "dc:Point",
                "x": 550,
                "y": 120
              },
              {
                "$type": "dc:Point",
                "x": 612,
                "y": 120
              }
            ]
          }
        ]
      }
    }
  ]
}
nikku commented 5 months ago

Thanks for root causing this! Based on what you found this may be a "hidden file property" issue.

Update: As the diffpatch does the diff on JSON level, i looked at the JSON for the BPMN diagram representation as provided by the BpmnModdle.fromXML and i am slightly confused why in the JSON there is no information representing the interconnection between elements at all. It just lists out the elements but noone says "Task_2 is connected to Task_3 via SequenceFlow_2". What am i missing?

The BPMN diagram is a cyclic graph with back and forth references. To be able to DIFF it in the first place we must flatten it.

The model itself contains cyclic references as hidden attributes; this allows folks to safely print diagram elements without printing the whole tree. So using the simple JSON serialization (or traversing visible properties) for diffing may not be sufficient; we'd need to find another form of serialization.

38leinaD commented 5 months ago

So using the simple JSON serialization for diffing may not be sufficient; we'd need to find another form of serialization.

So you are confirming that the diffing is done based on the JSON serialization and that is the issue here? To be honest, when i made this statement above ('As the diffpatch does the diff on JSON level'), i just had this mental model. But i understand/assume now that this probably is not how it works!? To me it looks like my analysis probably did not fully make sense. Let me summarize my understanding now: 1.) Printing the JSON via JSON.stringify does not tell us anything regarding the issue as this is not the basis for the diff anyway; diffpatch just walks/analyses the object graph. 2.) Yes, the issue probably is diffpatch library; beyond that i don't know; sounds like you are assuming it is related to a "hidden file property". But that has nothing to do with the JSON serialization that confused me, right? 3) Unrelated to the problem, but for debugging purposes should JSON.stringify not print the connection between services and edges anyway in some way anyhow? Otherwise, the JSON would not have much use. But maybe it is like it?

Let me know if it makes sense what I am wrinting.

nikku commented 5 months ago

Printing the JSON via JSON.stringify does not tell us anything regarding the issue as this is not the basis for the diff anyway; diffpatch just walks/analyses the object graph.

Both are related, hence I mentioned JSON.stringify along with the hidden property. We're not using JSON.stringify, but diffpatch uses the same mechanism for comparison: It cannot diff cyclic structures either, hence it ignores hidden properties. It only walks visible properties, and so does JSON.stringify.

Unrelated to the problem, but for debugging purposes should JSON.stringify not print the connection between services and edges anyway in some way anyhow?

Debugging does not happen through JSON.stringify but console.log(abc) or the like. Standard printers allow for handling cyclic dependencies.

nikku commented 1 month ago

Fixed via https://github.com/bpmn-io/bpmn-js-differ/pull/22.

nikku commented 1 month ago

Released fix with v3.0.0.