kie-issues#886: On the DMN Editor, adding waypoints to edges that don't have a corresponding DMNEdge associated with it should create the DMNEdge and add the waypoint normally #2546

jomarko · 2024-08-23T13:16:39Z

Checklist

Current Behavior

In non default DRD, notice the edges waypoints

…t have a corresponding DMNEdge associated with it should create the DMNEdge and add the waypoint normally. Closes: apache/incubator-kie-issues#886

tiagobento · 2024-08-23T21:20:24Z

packages/dmn-editor/src/mutations/addEdge.ts

@@ -195,20 +194,22 @@ export function addEdge({

 function doesInformationRequirementsPointTo(a: Normalized<DMN15__tInformationRequirement>, nodeId: string) {
  return (
-    a.requiredInput?.["@_href"] === `${nodeId}` || //
-    a.requiredDecision?.["@_href"] === `${nodeId}`
+    // use endsWith because @_href is sometimes prefixed with '#' and sometimes it is not


I know this is in draft, but let me tell you already that using "endsWith" is not the way to go here. hrefs are structured strings, composed by a namespace and an id. They have this format:

#id; or

namespace#id; where namespace is an URL.

We have a special parsing method to treat those and separate namespace and id. See parseXmlHref.

tiagobento · 2024-08-27T13:26:41Z

@jomarko I think this PR is fixing the problem in the wrong place. There's a mutation that adds waypoints... we only need to create an Edge at that moment, if the edge doesn't exist.

ljmotta · 2024-08-28T19:04:37Z

@jomarko @tiagobento The code makes sense to me, but I'm not certain about the solution. I'm just joining the discussion on this issue, so I might have missed something. Why are we adding the edge lazily? Can't we add the DMNEdge to the DRD at the same time we add the Node?

tiagobento · 2024-08-29T01:36:41Z

@ljmotta We could, but it's more places to handle, as there are multiple places that can add a node to a DRD. If a DMNEdge doesn't exist in the DRD, we render a default edge between the nodes, based on the DRG. Now when we want to add the waypoint, that's when the DMNEdge becomes necessary, otherwise there's no place to add the waypoint to.

ljmotta · 2024-08-29T12:54:16Z

@tiagobento I understand, but we will have an inconsistency between the model and what is visualized in the editor. I mean, the DRD depicts something that isn't on it. Taking a look into another issue, like apache/incubator-kie-issues#1450 we will fall into the same problem, as it wouldn't be possible to tweak the edge properties.

tiagobento · 2024-08-29T19:37:52Z

Thanks @ljmotta. As we discussed, the DRG should be understood as the primary source of truth, and the DRD should be used to provide a customized depiction of it. DMNEdges, like all DMN DI elements, are optional, so we need to do the best we can to have the DRG correctly depicted even if the DRD doesn't contain visual information about it.

danielzhe

Nice feature! I leave some comments.

danielzhe · 2024-09-17T19:45:47Z

packages/dmn-editor/src/diagram/connections/isValidConnection.ts

-  if (targetNode.data.dmnObjectQName.prefix) {
+  // External nodes cannot be targeted by default
+  // However there are exceptions, for example adding a waypoint on the edge
+  if (!extraArg?.allowExternalTarget && targetNode.data.dmnObjectQName.prefix) {


In the future, will we have local nodes with a prefix, @tiagobento ? We were talking about that in pvt chat a few weeks ago.
Anyway, I don't feel confident concluding that nodes with a prefix are always remote. Maybe if someone else in the future looks at this code doesn't understand why we're checking if there is a prefix or not.

@danielzhe thank you for a comment.

the logic that marks node as external or not according to the prefix targetNode.data.dmnObjectQName.prefix was in codebase even before my PR. I would suggest the following:

if there is an existing, more robust way, how to decide if node is or is not external, I can incorporate it as part of this PR

if there is not, what I understand is the situation, we should probably change this behavior as part of a separate ticker ticket you mention - "In the future, will we have local nodes with a prefix" - I see this as a ticket that should handle what you suggest

please let me know what do you think

Yeah, you're right! It doesn't make sense change it now since we're using that everywhere.

Maybe we can just extract to a variable:

const isExternalNode = targetNode.data.dmnObjectQName.prefix != undefined

@danielzhe done

danielzhe · 2024-09-17T20:09:07Z

packages/dmn-editor/src/diagram/edges/usePotentialWaypointControls.ts

      return;
    }

+    if (edgeIndex === undefined) {
+      /**
+       * This means we are adding a first wayipoint to one of following edges:


Typo in "wayipoint" = "waypoint"

well spotted, thank you, will address in a new commit

danielzhe · 2024-09-17T20:12:00Z

packages/dmn-editor/src/diagram/edges/usePotentialWaypointControls.ts

+          edge.data?.dmnShapeSource === undefined ||
+          edge.data?.dmnShapeTarget === undefined
+        ) {
+          console.debug(`DMN MUTATION: We can not add DMNEdge for '${edgeId}' edge into diagram.`);


Maybe we can add more details about why we can not add the DMNEdge here. We don't know if we can't because the edge is not defined, type, data, shapeSource, or shapeTarget and we have the exactly same message bellow in other 2 places. So, just looking at the console, we can not determine what caused the error or if it was in the first, second or third place.

packages/dmn-editor/src/diagram/edges/usePotentialWaypointControls.ts

danielzhe · 2024-09-17T20:24:40Z

packages/dmn-editor/src/mutations/addEdge.ts

@@ -74,14 +74,20 @@ export function addEdge({
    autoPositionedEdgeMarker: AutoPositionedEdgeMarker | undefined;
  };
  keepWaypoints: boolean;
+  extraArg?: {


I'm being annoying here: I don't like the extraArgs. Why they are "extra"?

If are optional args, I think we can use requirementEdgeTargetingExternalNodeId?: string for example.

This is a trick for forcing named parameters

do you have other names suggestions @danielzhe ?

Can't we use requirementEdgeTargetingExternalNodeId directly, @tiagobento ?

Sorry I overlook this a little bit. requirementEdgeTargetingExternalNodeId is already inside an object, meaning the "named arguments" trick is already in effect. Please lose the extraArg @jomarko. Thanks.

packages/dmn-editor/src/mutations/addEdgeWaypoint.ts

danielzhe · 2024-09-17T20:27:33Z

packages/dmn-editor/src/mutations/addStandaloneNode.ts

@@ -18,7 +18,7 @@
 */

 import { switchExpression } from "@kie-tools-core/switch-expression-ts";
-import { DmnBuiltInDataType, generateUuid } from "@kie-tools/boxed-expression-component/dist/api";


I've been doing that kind of cleanup every time I see it. Nice!

danielzhe · 2024-09-17T20:31:20Z

packages/dmn-editor/tests-e2e/drds/modelDrd.spec.ts

@@ -136,13 +136,13 @@ test.describe("Model DRD", () => {
        await drds.toggle();
        await drds.navigateTo({ name: "Second DRD" });
        await drds.toggle();
-        await drgNodes.open();


danielzhe · 2024-09-19T15:06:09Z

I didn't find any issues in the manual tests. I tried some not-common scenarios too using external models and it seems everything is working.

tiagobento

Getting there @jomarko. I like the intentionality of the code, especially where you added comments. Left some inline comments, please consider them. Thanks!

tiagobento · 2024-09-24T21:51:05Z

packages/dmn-editor/src/diagram/edges/usePotentialWaypointControls.ts

+          console.debug(
+            `DMN MUTATION: We can not add DMNEdge for '${edgeId}' edge into diagram. There are missing data edge: ${edge}, edge.data: ${edge?.data}`
+          );


This is not a mutation error. Please adjust the logs.

packages/dmn-editor/src/diagram/edges/usePotentialWaypointControls.ts

tiagobento · 2024-09-24T21:52:13Z

packages/dmn-editor/src/diagram/edges/usePotentialWaypointControls.ts

+          console.debug(
+            `DMN MUTATION: We can not add DMNEdge for '${edgeId}' edge into diagram. There are missing data edgeSourceBounds: ${edgeSourceBounds}, edgeTargetBounds: ${edgeTargetBounds}`
+          );


Not in a mutation. Please adjust the logs.

tiagobento · 2024-09-24T21:52:19Z

packages/dmn-editor/src/diagram/edges/usePotentialWaypointControls.ts

+          console.debug(
+            `DMN MUTATION: We can not add DMNEdge for '${edgeId}' edge into diagram. There are missing data sourceNode: ${sourceNode}, targetNode: ${targetNode}`
+          );


Not in a mutation. Please adjust the logs.

tiagobento · 2024-09-24T21:54:36Z

packages/dmn-editor/src/diagram/edges/usePotentialWaypointControls.ts

+          requirementEdgeTargetingExternalNodeId: isTargetExternalNode ? edgeId : undefined,
+        });
+
+        console.debug(`DMN MUTATION: DMNEdge for '${edgeId}' edge was added into diagram.`);


Not inside a mutation.

tiagobento · 2024-09-24T21:58:08Z

packages/dmn-editor/src/diagram/edges/usePotentialWaypointControls.ts

+      const dmnEdgeIndex = state.computed(state).indexedDrd().dmnEdgesByDmnElementRef.get(edgeId)?.index;
+      if (dmnEdgeIndex === undefined) {
+        console.debug(`DMN MUTATION: DMNEdge for '${edgeId}' edge has missing index.`);
+        return;
+      }


I think we should throw here, as at this point, this operation should never fail. If edgeIndex doesn't exist, you create a new edge, thus querying for the edgeId again should always give you an edge. If that doesn't happen, something exceptional happened, so we throw. Also not in a mutation.

tiagobento · 2024-09-24T21:59:27Z

packages/dmn-editor/src/diagram/edges/usePotentialWaypointControls.ts

@@ -96,18 +173,27 @@ export function usePotentialWaypointControls(
    }

    dmnEditorStoreApi.setState((state) => {
+      const dmnEdgeIndex = state.computed(state).indexedDrd().dmnEdgesByDmnElementRef.get(edgeId)?.index;


I think it's worth to add a comment here explaining why you're getting dmnEdgeIndex when you already have edgeIndex in scope.

tiagobento · 2024-09-24T21:59:54Z

packages/dmn-editor/src/diagram/edges/usePotentialWaypointControls.ts

      addEdgeWaypoint({
        definitions: state.dmn.model.definitions,
        drdIndex,
        beforeIndex: i - 1,
-        edgeIndex,
+        edgeIndex: dmnEdgeIndex,
        waypoint: snappedPotentialWaypoint,
      });


Probably worth changing the name of this parameter to dmnEdgeIndex, since it represents a DMNEdge object.

tiagobento · 2024-09-24T22:07:39Z

packages/dmn-editor/src/diagram/connections/isValidConnection.ts

 export function _checkIsValidConnection(
  sourceNode: { type?: string; data: DmnDiagramNodeData } | undefined,
  targetNode: { type?: string; data: DmnDiagramNodeData } | undefined,
-  edgeType: string | null | undefined
+  edgeType: string | null | undefined,
+  extraArg?: {
+    allowExternalTarget: boolean;
+  }
 ) {
  if (!sourceNode?.type || !targetNode?.type || !edgeType) {
    return false;
  }

-  // External nodes cannot be targeted
-  if (targetNode.data.dmnObjectQName.prefix) {
+  // External nodes cannot be targeted by default
+  // However there are exceptions, for example adding a waypoint on the edge
+  const isTargetExternalNode = targetNode.data.dmnObjectQName.prefix != undefined;
+  if (!extraArg?.allowExternalTarget && isTargetExternalNode) {
    return false;
  }


This way, it is clear that false is the default for when extraArg is undefined.

export function _checkIsValidConnection( sourceNode: { type?: string; data: DmnDiagramNodeData } | undefined, targetNode: { type?: string; data: DmnDiagramNodeData } | undefined, edgeType: string | null | undefined, extraArg?: { allowExternalTarget: boolean; } ) { if (!sourceNode?.type || !targetNode?.type || !edgeType) { return false; } // External nodes cannot be targeted by default. However there are exceptions, // for example adding a waypoint on an edge between two external nodes. const isTargetExternalNode = targetNode.data.dmnObjectQName.prefix != undefined; const allowExternalTarget = extraArg?.allowExternalTarget ?? false; if (isTargetExternalNode && !allowExternalTarget) { return false; } }

tiagobento · 2024-09-24T22:13:15Z

packages/dmn-editor/src/store/computed/computeDiagramData.ts

        ackEdge({
-          id: ir["@_id"]!,
+          id: (namespaceIncludedPrefix ? `${namespaceIncludedPrefix}:` : "") + ir["@_id"]!,


Why not

Suggested change

id: (namespaceIncludedPrefix ? `${namespaceIncludedPrefix}:` : "") + ir["@_id"]!,

id: (drgElementsNamespace === thisDmnsNamespace

? ir["@_id"]

: buildXmlHref({ namespace: drgElementsNamespace, id: ir["@_id"] }),

?

External nodes are currently using an HREF (namespace#id) as ID, I think we should keep the same strategy for edges, instead of using a QName (prefix:id).

You can remove the definitions parameter you added too, if that ^ makes sense.

@tiagobento thank you for this topic. actually, nodes/shapes, currently uses prefix:id , not namespace#id, that is why implemented that way for edges.

Applying your suggestion produces xml like attached:

<dmndi:DMNDI> <dmndi:DMNDiagram id="_B90C2A89-A2E3-4610-AC66-A6DC0F9C1CEF" name="Default DRD" useAlternativeInputDataShape="false"> <di:extension> <kie:ComponentsWidthsExtension> </kie:ComponentsWidthsExtension> </di:extension> <dmndi:DMNEdge id="_D763F5B1-FAF0-4790-A905-A5B1D63F0D2C" dmnElementRef="included2:_90FB4683-E1A0-4149-A1B8-444DEE06E749" sourceElement="_A9428417-FA8E-47B7-9F4A-10921FEA34BA" targetElement="_CAFE156E-4FB5-4285-9079-E3165FB92DDE"> <di:waypoint x="1300" y="260" /> <di:waypoint x="900" y="240" /> <di:waypoint x="600" y="400" /> </dmndi:DMNEdge> <dmndi:DMNShape id="_CD133A74-86D5-4DA7-81BF-F122A69A7927" dmnElementRef="included0:_2081EEE3-7B1B-48B3-B3B9-944F0A013CF1"> <dc:Bounds x="960" y="160" width="160" height="80" /> </dmndi:DMNShape> <dmndi:DMNShape id="_AA2DC9CB-C337-4E8F-8258-ECA14E0CDB10" dmnElementRef="included0:_92040BB1-AB7A-46D1-A4DE-A4FD1448ADF5"> <dc:Bounds x="700" y="220" width="160" height="80" /> </dmndi:DMNShape> <dmndi:DMNEdge id="_68653841-7F18-46EB-B3F0-37082CC0A3E8" dmnElementRef="https://kie.org/dmn/_AE5E9361-3580-4587-AE13-53A36057853B#_0A18CF15-F3E5-4B87-82A1-2851A246FDF4" sourceElement="_AA2DC9CB-C337-4E8F-8258-ECA14E0CDB10" targetElement="_CD133A74-86D5-4DA7-81BF-F122A69A7927"> <di:waypoint x="780" y="260" /> <di:waypoint x="940" y="260" /> <di:waypoint x="860" y="200" /> <di:waypoint x="1040" y="200" /> </dmndi:DMNEdge> </dmndi:DMNDiagram> </dmndi:DMNDI>

from this discussion my understanding is, I should update the code to use namespace#id also for nodes/shapes? Is that correct?

Ok, so let's take a step back. This id here is only the ID of the RF.Edge object we're telling ReactFlow exists. How it makes its way to the DMN XML is another story. I believe that ReactFlow objects (nodes and edges) should have their id in the HREF (I.e., namespace#id) format. That's one thing.

Now, you're correct to say that it is wrong to have an HREF in the dmnElementRef attribute, as, per the XSD, it's a QName (I.e., prefix:id). Now, let's try and see how the id of the RF.Edge is getting into the dmnElementRef....

The only place that adds DMNEdges to the XML is the addEdge mutation, so it must be an invocation to it. You added a new invocation to it on this PR... and there's this line:

A. requirementEdgeTargetingExternalNodeId: targetsExternalNode ? edgeId : undefined

See how the edgeId is now inside the addEdge mutation? Now let's look how this new requirementEdgeTargetingExternalNodeId parameter is used inside the addEdge mutation:

B. let existingEdgeId: string | undefined = requirementEdgeTargetingExternalNodeId;; and
C. "@_dmnElementRef": existingEdgeId ?? newEdgeId,

Hmm... something is not right :)

Now, all this is really confusing, because there are multiple ""types"" of id, and there are multiple formats for ids. We have:

The id of the ReactFlow object (I.e., RF.Edge), which I'm saying should be in HREF format.

The id attribute (@_id) of the Information Requirement (which is a plain UUID).

The id attribute (@_id) of the DMNEdge (which is a plain UUID).

The id we need to write to dmnElementRef attribute (@_dmnElementRef), which is in QName format.

They're all "ids" in some way, so it's really hard to name things properly.

Ok, let's go back.

I'd say:

A. should be:
dmnElementRefOfDmnEdge: targetsExternalNode ? informationRequirementQNameRelativeToThisDmn : undefined (this renaming requirementEdgeTargetingExternalNodeId)

B. should be:
let existingRequirementOrAssociationId: string | undefined; (thus renaming existingEdgeId)

C. should be:
"@_dmnElementRef": dmnElementRefOfDmnEdge ?? existingRequirementOrAssociationId ?? newEdgeId

And I believe we should rename newEdgeId to newRequirementOrAssociationId.

Let me know if you have questions :)

Btw, we have xmlHrefToQName to help you on building informationRequirementQNameRelativeToThisDmn from edgeId (I.e., Information Requirement's HREF relative to this DMN`).

…rols.ts Co-authored-by: Tiago Bento <[email protected]>

This reverts commit 6d6cc5b.

kie-issues#886 On the DMN Editor, adding waypoints to edges that don'…

efa4577

…t have a corresponding DMNEdge associated with it should create the DMNEdge and add the waypoint normally. Closes: apache/incubator-kie-issues#886

jomarko added pr: DO NOT MERGE Draft PR, not ready for merging pr: wip PR is still under development area:dmn labels Aug 23, 2024

tiagobento reviewed Aug 23, 2024

View reviewed changes

jomarko added 6 commits August 26, 2024 08:18

Merge branch 'main' into kie-issues#886

8eac192

information requirement tests

7a79145

add TODO tasks

de4b79f

incorporate review feedback - @_href

5b1f21f

drgNodes.toggle

05c48a2

add tests

dc80ddc

jomarko added 5 commits August 28, 2024 14:21

add missing test screenshots

94db899

cleanup spec.ts javadoc

8cfc2f5

Cleanup Diagram.tsx

eadc55e

addEdge in usePotentialWaypointControls

b25ca85

add logs

babe9a2

jomarko removed pr: DO NOT MERGE Draft PR, not ready for merging pr: wip PR is still under development labels Aug 28, 2024

jomarko marked this pull request as ready for review August 28, 2024 13:56

jomarko added 2 commits August 30, 2024 16:05

display waypoints on the edges targetting external nodes

c2cbace

create DMNEdge targeting external node

632adeb

jomarko added pr: DO NOT MERGE Draft PR, not ready for merging pr: wip PR is still under development labels Aug 30, 2024

tiagobento changed the title ~~kie-issues#886 On the DMN Editor, adding waypoints to edges that don'…~~ kie-issues#886 On the DMN Editor, adding waypoints to edges that don't have a corresponding DMNEdge associated with it should create the DMNEdge and add the waypoint normally Sep 5, 2024

danielzhe requested changes Sep 17, 2024

View reviewed changes

jomarko added 6 commits September 18, 2024 13:28

fix typo

3de7056

logs

b28961d

simplified null checks

1b7d807

Merge branch 'main' into kie-issues#886

dd67a91

review feedback

302ec19

review feedback

cb56865

remove extraArg

63c9187

danielzhe approved these changes Sep 19, 2024

View reviewed changes

tiagobento requested changes Sep 24, 2024

View reviewed changes

jomarko and others added 12 commits September 25, 2024 13:52

Merge branch 'main' into kie-issues#886

269d5a5

MUTATION -> DIAGRAM

20ebd51

Update packages/dmn-editor/src/diagram/edges/usePotentialWaypointCont…

e5eb1a0

…rols.ts Co-authored-by: Tiago Bento <[email protected]>

Update packages/dmn-editor/src/diagram/edges/usePotentialWaypointCont…

f4207a8

…rols.ts Co-authored-by: Tiago Bento <[email protected]>

targetsExternalNode

bc8495e

throw error

1da182f

is valid connection

4ac0a99

dmnEdgeIndex

8eaa8b9

incorporate review feedback

6d6cc5b

Merge branch 'main' into kie-issues#886

9be924f

fix != vs !==

a5044c8

Revert "incorporate review feedback"

d3fc589

This reverts commit 6d6cc5b.

jomarko marked this pull request as draft September 26, 2024 12:17

jomarko added 3 commits September 26, 2024 16:42

incorporate review feedback - v2

79f64d2

Merge branch 'main' into kie-issues#886

38ce320

fix deleteEdge

006000e

jomarko marked this pull request as ready for review September 27, 2024 08:52

tiagobento approved these changes Sep 27, 2024

View reviewed changes

tiagobento requested a review from danielzhe September 27, 2024 21:49

-          id: (namespaceIncludedPrefix ? `${namespaceIncludedPrefix}:` : "") + ir["@_id"]!,
+          id: (drgElementsNamespace === thisDmnsNamespace
+            ? ir["@_id"]
+            : buildXmlHref({ namespace: drgElementsNamespace, id: ir["@_id"]  }),

kie-issues#886: On the DMN Editor, adding waypoints to edges that don't have a corresponding DMNEdge associated with it should create the DMNEdge and add the waypoint normally #2546

Are you sure you want to change the base?

kie-issues#886: On the DMN Editor, adding waypoints to edges that don't have a corresponding DMNEdge associated with it should create the DMNEdge and add the waypoint normally #2546

Conversation

jomarko commented Aug 23, 2024 • edited Loading

Checklist

Current Behavior

Choose a reason for hiding this comment

tiagobento commented Aug 27, 2024

ljmotta commented Aug 28, 2024

tiagobento commented Aug 29, 2024

ljmotta commented Aug 29, 2024

tiagobento commented Aug 29, 2024

danielzhe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danielzhe commented Sep 19, 2024

tiagobento left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tiagobento Sep 24, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tiagobento Sep 25, 2024 • edited Loading

Choose a reason for hiding this comment

tiagobento Sep 25, 2024 • edited Loading

Choose a reason for hiding this comment

jomarko commented Aug 23, 2024 •

edited

Loading

tiagobento Sep 24, 2024 •

edited

Loading

tiagobento Sep 25, 2024 •

edited

Loading

tiagobento Sep 25, 2024 •

edited

Loading