camunda / dmn-scala

DMN engine written in Scala
Apache License 2.0
34 stars 15 forks source link

incorrect cycle detection #213

Closed nhomble closed 1 year ago

nhomble commented 1 year ago

Describe the bug The cycle detection doesn't like the fact that I have a leaf node that is a dependency of an intermediate node and a dependency of the root.

To Reproduce

<?xml version="1.0" encoding="UTF-8"?>
<definitions xmlns="https://www.omg.org/spec/DMN/20191111/MODEL/" xmlns:dmndi="https://www.omg.org/spec/DMN/20191111/DMNDI/" xmlns:dc="http://www.omg.org/spec/DMN/20180521/DC/" xmlns:modeler="http://camunda.org/schema/modeler/1.0" xmlns:di="http://www.omg.org/spec/DMN/20180521/DI/" id="Definitions_0h9zlqh" name="DRD" namespace="http://camunda.org/schema/1.0/dmn" exporter="Camunda Modeler" exporterVersion="5.10.0" modeler:executionPlatform="Camunda Cloud" modeler:executionPlatformVersion="8.2.0">
  <decision id="Decision_0lsyapa" name="C">
    <variable id="InformationItem_0s8k0bl" name="out" />
    <informationRequirement id="InformationRequirement_1rlbqu9">
      <requiredDecision href="#Decision_0fqzk9r" />
    </informationRequirement>
    <informationRequirement id="InformationRequirement_05m4j2x">
      <requiredDecision href="#Decision_1m0vnmw" />
    </informationRequirement>
    <literalExpression id="LiteralExpression_04arz50">
      <text>[a, b]</text>
    </literalExpression>
  </decision>
  <decision id="Decision_1m0vnmw" name="B">
    <variable id="InformationItem_16zbv4v" name="b" />
    <informationRequirement id="InformationRequirement_00uf5b7">
      <requiredDecision href="#Decision_0fqzk9r" />
    </informationRequirement>
    <literalExpression id="LiteralExpression_0wt75p4">
      <text>a + 1</text>
    </literalExpression>
  </decision>
  <decision id="Decision_0fqzk9r" name="A">
    <variable id="InformationItem_0kwllv5" name="a" />
    <literalExpression id="LiteralExpression_10sjfdq">
      <text>1</text>
    </literalExpression>
  </decision>
  <dmndi:DMNDI>
    <dmndi:DMNDiagram>
      <dmndi:DMNEdge id="DMNEdge_0tire0n" dmnElementRef="InformationRequirement_00uf5b7">
        <di:waypoint x="250" y="460" />
        <di:waypoint x="470" y="380" />
        <di:waypoint x="470" y="360" />
      </dmndi:DMNEdge>
      <dmndi:DMNEdge id="DMNEdge_1qeflao" dmnElementRef="InformationRequirement_1rlbqu9">
        <di:waypoint x="250" y="460" />
        <di:waypoint x="240" y="180" />
        <di:waypoint x="240" y="160" />
      </dmndi:DMNEdge>
      <dmndi:DMNEdge id="DMNEdge_1j44nm2" dmnElementRef="InformationRequirement_05m4j2x">
        <di:waypoint x="470" y="280" />
        <di:waypoint x="300" y="180" />
        <di:waypoint x="300" y="160" />
      </dmndi:DMNEdge>
      <dmndi:DMNShape id="DMNShape_0elqq7s" dmnElementRef="Decision_0lsyapa">
        <dc:Bounds height="80" width="180" x="180" y="80" />
      </dmndi:DMNShape>
      <dmndi:DMNShape id="DMNShape_182794f" dmnElementRef="Decision_1m0vnmw">
        <dc:Bounds height="80" width="180" x="380" y="280" />
      </dmndi:DMNShape>
      <dmndi:DMNShape id="DMNShape_0321hm2" dmnElementRef="Decision_0fqzk9r">
        <dc:Bounds height="80" width="180" x="160" y="460" />
      </dmndi:DMNShape>
    </dmndi:DMNDiagram>
  </dmndi:DMNDI>
</definitions>

Expected behavior The xml above should not throw (and works with the previous version of the parser and deployed on zeebe 8.1.x).

Environment

nhomble commented 1 year ago

Looking at the hasDependencyCycle method.. I'm not totally sure how this works with tail ++ dependencies but tracing my graph via DFS shows that there isn't a cycle.

nikku commented 1 year ago

This causes a regression in Camunda 8.2 (works in Camunda 8.1.x). Reported through SUPPORT-16665.

Potentially introduced via https://github.com/camunda/dmn-scala/pull/207?

saig0 commented 1 year ago

@nhomble thank you for reporting and providing the data. :+1:

We will have a look and provide a fix.

nhomble commented 1 year ago

Hey folks, I saw 8.2.4 was cut but I didn't see this in the changelog. Any idea when this would get released?

remcowesterhoud commented 1 year ago

@nhomble it is released as part of 8.2.4. You don't see it in the changelog of Zeebe as it wasn't a bug in Zeebe, but in the dmn-engine.