Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 49 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 34 commits
Commits
Show all changes
49 commits
Select commit Hold shift + click to select a range
efa4577
kie-issues#886 On the DMN Editor, adding waypoints to edges that don'…
jomarko Aug 23, 2024
8eac192
Merge branch 'main' into kie-issues#886
jomarko Aug 26, 2024
7a79145
information requirement tests
jomarko Aug 26, 2024
de4b79f
add TODO tasks
jomarko Aug 26, 2024
5b1f21f
incorporate review feedback - @_href
jomarko Aug 26, 2024
05c48a2
drgNodes.toggle
jomarko Aug 26, 2024
dc80ddc
add tests
jomarko Aug 26, 2024
94db899
add missing test screenshots
jomarko Aug 28, 2024
8cfc2f5
cleanup spec.ts javadoc
jomarko Aug 28, 2024
eadc55e
Cleanup Diagram.tsx
jomarko Aug 28, 2024
b25ca85
addEdge in usePotentialWaypointControls
jomarko Aug 28, 2024
babe9a2
add logs
jomarko Aug 28, 2024
c2cbace
display waypoints on the edges targetting external nodes
jomarko Aug 30, 2024
632adeb
create DMNEdge targeting external node
jomarko Aug 30, 2024
f650d0a
Merge branch 'main' into kie-issues#886
jomarko Sep 9, 2024
d95740b
dmnElementRef in format [namespace]:[id]
jomarko Sep 9, 2024
ff2c7fc
documentation
jomarko Sep 9, 2024
77c96e5
Merge branch 'main' into kie-issues#886
jomarko Sep 10, 2024
fb13967
Merge branch 'main' into kie-issues#886
jomarko Sep 13, 2024
dda9611
allowExternalTarget refactoring
jomarko Sep 13, 2024
ba8b40d
remove target.qname.prefix usage
jomarko Sep 13, 2024
e3d0b79
remove non used code
jomarko Sep 13, 2024
4ea5a26
remove addEdgeTargetingExternal
jomarko Sep 13, 2024
8a2b8eb
Merge branch 'main' into kie-issues#886
jomarko Sep 16, 2024
411814d
fix onConnect
jomarko Sep 16, 2024
5bad3f8
fix deleteEdge
jomarko Sep 16, 2024
8b98787
Object.entries
jomarko Sep 17, 2024
3de7056
fix typo
jomarko Sep 18, 2024
b28961d
logs
jomarko Sep 18, 2024
1b7d807
simplified null checks
jomarko Sep 18, 2024
dd67a91
Merge branch 'main' into kie-issues#886
jomarko Sep 18, 2024
302ec19
review feedback
jomarko Sep 19, 2024
cb56865
review feedback
jomarko Sep 19, 2024
63c9187
remove extraArg
jomarko Sep 19, 2024
269d5a5
Merge branch 'main' into kie-issues#886
jomarko Sep 25, 2024
20ebd51
MUTATION -> DIAGRAM
jomarko Sep 25, 2024
e5eb1a0
Update packages/dmn-editor/src/diagram/edges/usePotentialWaypointCont…
jomarko Sep 25, 2024
f4207a8
Update packages/dmn-editor/src/diagram/edges/usePotentialWaypointCont…
jomarko Sep 25, 2024
bc8495e
targetsExternalNode
jomarko Sep 25, 2024
1da182f
throw error
jomarko Sep 25, 2024
4ac0a99
is valid connection
jomarko Sep 25, 2024
8eaa8b9
dmnEdgeIndex
jomarko Sep 25, 2024
6d6cc5b
incorporate review feedback
jomarko Sep 26, 2024
9be924f
Merge branch 'main' into kie-issues#886
jomarko Sep 26, 2024
a5044c8
fix != vs !==
jomarko Sep 26, 2024
d3fc589
Revert "incorporate review feedback"
jomarko Sep 26, 2024
79f64d2
incorporate review feedback - v2
jomarko Sep 26, 2024
38ce320
Merge branch 'main' into kie-issues#886
jomarko Sep 27, 2024
006000e
fix deleteEdge
jomarko Sep 27, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion packages/dmn-editor/src/diagram/Diagram.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -216,7 +216,7 @@
const sourceBounds = sourceNode.data.shape["dc:Bounds"];
const targetBounds = targetNode.data.shape["dc:Bounds"];
if (!sourceBounds || !targetBounds) {
throw new Error("Cannot create connection without target bounds!");
throw new Error("Cannot create connection without source and target bounds!");
}

// --------- This is where we draw the line between the diagram and the model.
Expand Down Expand Up @@ -897,7 +897,7 @@
}
});
},
[reactFlowInstance, dmnEditorStoreApi, externalModelsByNamespace]

Check warning on line 900 in packages/dmn-editor/src/diagram/Diagram.tsx

View workflow job for this annotation

GitHub Actions / run (ubuntu-latest, 2)

React Hook useCallback has an unnecessary dependency: 'reactFlowInstance'. Either exclude it or remove the dependency array

Check warning on line 900 in packages/dmn-editor/src/diagram/Diagram.tsx

View workflow job for this annotation

GitHub Actions / run (macos-13, 2)

React Hook useCallback has an unnecessary dependency: 'reactFlowInstance'. Either exclude it or remove the dependency array

Check warning on line 900 in packages/dmn-editor/src/diagram/Diagram.tsx

View workflow job for this annotation

GitHub Actions / run (windows-latest, 2)

React Hook useCallback has an unnecessary dependency: 'reactFlowInstance'. Either exclude it or remove the dependency array
);

const resetToBeforeEditingBegan = useCallback(() => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -39,14 +39,19 @@ export function checkIsValidConnection(
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;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;
  }
}


Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,11 @@ import { snapPoint } from "../SnapGrid";
import { DC__Point } from "@kie-tools/dmn-marshaller/dist/schemas/dmn-1_5/ts-gen/types";
import { DmnDiagramNodeData } from "../nodes/Nodes";
import { DmnDiagramEdgeData } from "./Edges";
import { useExternalModels } from "../../includedModels/DmnEditorDependenciesContext";
import { addEdge } from "../../mutations/addEdge";
import { EdgeType, NodeType } from "../connections/graphStructure";
import { PositionalNodeHandleId } from "../connections/PositionalNodeHandles";
import { getHandlePosition } from "../maths/DmnMaths";

export function usePotentialWaypointControls(
waypoints: DC__Point[],
Expand All @@ -38,6 +43,7 @@ export function usePotentialWaypointControls(
const isDraggingWaypoint = useDmnEditorStore((s) => !!s.diagram.draggingWaypoints.find((e) => e === edgeId));
const dmnEditorStoreApi = useDmnEditorStoreApi();
const reactFlowInstance = RF.useReactFlow<DmnDiagramNodeData, DmnDiagramEdgeData>();
const { externalModelsByNamespace } = useExternalModels();

const [potentialWaypoint, setPotentialWaypoint] = useState<ReturnType<typeof approximateClosestPoint> | undefined>(
undefined
Expand Down Expand Up @@ -74,10 +80,81 @@ export function usePotentialWaypointControls(
}, [snapGrid, potentialWaypoint]);

const onDoubleClick = useCallback(() => {
if (!potentialWaypoint || !snappedPotentialWaypoint || edgeIndex === undefined) {
if (!potentialWaypoint || !snappedPotentialWaypoint) {
return;
}

if (edgeIndex === undefined) {
/**
* This means we are adding a first waypoint to one of following edges:
* - an edge in a non default DRD
* - an edge targeting an external node
*/
dmnEditorStoreApi.setState((state) => {
const nodesById = state.computed(state).getDiagramData(externalModelsByNamespace).nodesById;
const edge = state.computed(state).getDiagramData(externalModelsByNamespace).edgesById.get(edgeId);
if (edge === undefined || edge.data?.dmnShapeSource === undefined || edge.data?.dmnShapeTarget == undefined) {
console.debug(
`DMN MUTATION: We can not add DMNEdge for '${edgeId}' edge into diagram. There are missing data edge: ${edge}, edge.data: ${edge?.data}`
);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

return;
}
const edgeSourceBounds = edge.data?.dmnShapeSource["dc:Bounds"];
const edgeTargetBounds = edge.data?.dmnShapeTarget["dc:Bounds"];

if (edgeSourceBounds === undefined || edgeTargetBounds === undefined) {
tiagobento marked this conversation as resolved.
Show resolved Hide resolved
jomarko marked this conversation as resolved.
Show resolved Hide resolved
console.debug(
`DMN MUTATION: We can not add DMNEdge for '${edgeId}' edge into diagram. There are missing data edgeSourceBounds: ${edgeSourceBounds}, edgeTargetBounds: ${edgeTargetBounds}`
);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not in a mutation. Please adjust the logs.

return;
}

const sourceNode = nodesById.get(edge.source);
const targetNode = nodesById.get(edge.target);

if (sourceNode === undefined || targetNode === undefined) {
console.debug(
`DMN MUTATION: We can not add DMNEdge for '${edgeId}' edge into diagram. There are missing data sourceNode: ${sourceNode}, targetNode: ${targetNode}`
);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not in a mutation. Please adjust the logs.

return;
}

const isTargetExternalNode = targetNode.data.dmnObjectQName.prefix !== undefined;
jomarko marked this conversation as resolved.
Show resolved Hide resolved

addEdge({
definitions: state.dmn.model.definitions,
drdIndex: state.computed(state).getDrdIndex(),
edge: {
type: edge.type as EdgeType,
targetHandle: getHandlePosition({ shapeBounds: edgeTargetBounds, waypoint: snappedPotentialWaypoint })
.handlePosition as PositionalNodeHandleId,
sourceHandle: getHandlePosition({ shapeBounds: edgeSourceBounds, waypoint: snappedPotentialWaypoint })
.handlePosition as PositionalNodeHandleId,
autoPositionedEdgeMarker: undefined,
},
sourceNode: {
type: sourceNode.type as NodeType,
data: sourceNode.data,
href: edge.source,
bounds: edgeSourceBounds,
shapeId: edge.data?.dmnShapeSource["@_id"],
},
targetNode: {
type: targetNode.type as NodeType,
href: edge.target,
data: targetNode.data,
bounds: edgeTargetBounds,
index: nodesById.get(edge.target)?.data.index ?? 0,
shapeId: edge.data?.dmnShapeTarget["@_id"],
},
keepWaypoints: false,
requirementEdgeTargetingExternalNodeId: isTargetExternalNode ? edgeId : undefined,
});

console.debug(`DMN MUTATION: DMNEdge for '${edgeId}' edge was added into diagram.`);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not inside a mutation.

});
}

if (isExistingWaypoint(snappedPotentialWaypoint)) {
console.debug("Preventing overlapping waypoint creation.");
return;
Expand All @@ -96,18 +173,27 @@ export function usePotentialWaypointControls(
}

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

if (dmnEdgeIndex === undefined) {
console.debug(`DMN MUTATION: DMNEdge for '${edgeId}' edge has missing index.`);
return;
}
Copy link
Contributor

@tiagobento tiagobento Sep 24, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


console.debug(`DMN MUTATION: Waypoint on the DMNEdge for '${edgeId}' edge was added.`);
});
}, [
drdIndex,
dmnEditorStoreApi,
edgeId,
edgeIndex,
externalModelsByNamespace,
isExistingWaypoint,
potentialWaypoint,
snappedPotentialWaypoint,
Expand Down
10 changes: 6 additions & 4 deletions packages/dmn-editor/src/mutations/addEdge.ts
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,6 @@ import {
DMN15__tInformationRequirement,
DMN15__tKnowledgeRequirement,
DMNDI15__DMNEdge,
DMNDI15__DMNShape,
} from "@kie-tools/dmn-marshaller/dist/schemas/dmn-1_5/ts-gen/types";
import { PositionalNodeHandleId } from "../diagram/connections/PositionalNodeHandles";
import { EdgeType, NodeType } from "../diagram/connections/graphStructure";
Expand All @@ -49,6 +48,7 @@ export function addEdge({
targetNode,
edge,
keepWaypoints,
requirementEdgeTargetingExternalNodeId,
}: {
definitions: Normalized<DMN15__tDefinitions>;
drdIndex: number;
Expand All @@ -74,14 +74,16 @@ export function addEdge({
autoPositionedEdgeMarker: AutoPositionedEdgeMarker | undefined;
};
keepWaypoints: boolean;
requirementEdgeTargetingExternalNodeId?: string;
}) {
if (!_checkIsValidConnection(sourceNode, targetNode, edge.type)) {
const externalTargetAllowed = requirementEdgeTargetingExternalNodeId !== undefined;
if (!_checkIsValidConnection(sourceNode, targetNode, edge.type, { allowExternalTarget: externalTargetAllowed })) {
throw new Error(`DMN MUTATION: Invalid structure: (${sourceNode.type}) --${edge.type}--> (${targetNode.type}) `);
}

const newEdgeId = generateUuid();

let existingEdgeId: string | undefined = undefined;
let existingEdgeId: string | undefined = requirementEdgeTargetingExternalNodeId;

// Associations
if (edge.type === EDGE_TYPES.association) {
Expand Down Expand Up @@ -109,7 +111,7 @@ export function addEdge({
});
}
// Requirements
else {
else if (!externalTargetAllowed) {
const requirements = getRequirementsFromEdge(sourceNode, newEdgeId, edge.type);
const drgElement = definitions.drgElement![targetNode.index] as Normalized<DMN15__tDecision>; // We cast to tDecision here because it has all three types of requirement.
if (requirements?.informationRequirement) {
Expand Down
2 changes: 1 addition & 1 deletion packages/dmn-editor/src/mutations/addEdgeWaypoint.ts
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ export function addEdgeWaypoint({

const diagramElement = diagramElements[edgeIndex];
if (diagramElement.__$$element !== "dmndi:DMNEdge") {
throw new Error("DMN MUTATION: Can't remove a waypoint from an element that is not a DMNEdge.");
throw new Error("DMN MUTATION: Can't add a waypoint for an element that is not a DMNEdge.");
tiagobento marked this conversation as resolved.
Show resolved Hide resolved
}

if (beforeIndex > (diagramElement["di:waypoint"]?.length ?? 0) - 1) {
Expand Down
1 change: 0 additions & 1 deletion packages/dmn-editor/src/mutations/addShape.ts
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@
import {
DC__Point,
DMN15__tDefinitions,
DMNDI15__DMNDecisionServiceDividerLine,
DMNDI15__DMNShape,
} from "@kie-tools/dmn-marshaller/dist/schemas/dmn-1_5/ts-gen/types";
import { NodeType } from "../diagram/connections/graphStructure";
Expand Down
2 changes: 1 addition & 1 deletion packages/dmn-editor/src/mutations/addStandaloneNode.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@
*/

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

import { generateUuid } from "@kie-tools/boxed-expression-component/dist/api";
import { DC__Bounds, DMN15__tDefinitions } from "@kie-tools/dmn-marshaller/dist/schemas/dmn-1_5/ts-gen/types";
import { NodeType } from "../diagram/connections/graphStructure";
import { NODE_TYPES } from "../diagram/nodes/NodeTypes";
Expand Down
55 changes: 27 additions & 28 deletions packages/dmn-editor/src/mutations/deleteEdge.ts
Original file line number Diff line number Diff line change
Expand Up @@ -44,41 +44,40 @@ export function deleteEdge({
edge: { id: string; dmnObject: DmnDiagramEdgeData["dmnObject"] };
mode: EdgeDeletionMode;
}) {
if (edge.dmnObject.namespace !== definitions["@_namespace"]) {
console.debug("DMN MUTATION: Can't delete an edge that's from an external node.");
return { dmnEdge: undefined };
}

const dmnObjects: Normalized<DMN15__tDefinitions>["drgElement" | "artifact"] =
switchExpression(edge?.dmnObject.type, {
association: definitions.artifact,
group: definitions.artifact,
default: definitions.drgElement,
}) ?? [];
if (edge.dmnObject.namespace === definitions["@_namespace"]) {
const dmnObjects: Normalized<DMN15__tDefinitions>["drgElement" | "artifact"] =
switchExpression(edge?.dmnObject.type, {
association: definitions.artifact,
group: definitions.artifact,
default: definitions.drgElement,
}) ?? [];

const dmnObjectIndex = dmnObjects.findIndex((d) => d["@_id"] === edge.dmnObject.id);
if (dmnObjectIndex < 0) {
throw new Error(`DMN MUTATION: Can't find DMN element with ID ${edge.dmnObject.id}`);
}
const dmnObjectIndex = dmnObjects.findIndex((d) => d["@_id"] === edge.dmnObject.id);
if (dmnObjectIndex < 0) {
throw new Error(`DMN MUTATION: Can't find DMN element with ID ${edge.dmnObject.id}`);
}

if (mode === EdgeDeletionMode.FROM_DRG_AND_ALL_DRDS) {
const requirements =
switchExpression(edge?.dmnObject.requirementType, {
// Casting to DMN15__tDecision because if has all types of requirement, but not necessarily that's true.
informationRequirement: (dmnObjects[dmnObjectIndex] as Normalized<DMN15__tDecision>).informationRequirement,
knowledgeRequirement: (dmnObjects[dmnObjectIndex] as Normalized<DMN15__tDecision>).knowledgeRequirement,
authorityRequirement: (dmnObjects[dmnObjectIndex] as Normalized<DMN15__tDecision>).authorityRequirement,
association: dmnObjects,
}) ?? [];
if (mode === EdgeDeletionMode.FROM_DRG_AND_ALL_DRDS) {
const requirements =
switchExpression(edge?.dmnObject.requirementType, {
// Casting to DMN15__tDecision because if has all types of requirement, but not necessarily that's true.
informationRequirement: (dmnObjects[dmnObjectIndex] as Normalized<DMN15__tDecision>).informationRequirement,
knowledgeRequirement: (dmnObjects[dmnObjectIndex] as Normalized<DMN15__tDecision>).knowledgeRequirement,
authorityRequirement: (dmnObjects[dmnObjectIndex] as Normalized<DMN15__tDecision>).authorityRequirement,
association: dmnObjects,
}) ?? [];

// Deleting the requirement
const requirementIndex = (requirements ?? []).findIndex((d) => d["@_id"] === edge.id);
if (requirementIndex >= 0) {
requirements?.splice(requirementIndex, 1);
// Deleting the requirement
const requirementIndex = (requirements ?? []).findIndex((d) => d["@_id"] === edge.id);
if (requirementIndex >= 0) {
requirements?.splice(requirementIndex, 1);
}
}
}

// Deleting the DMNEdge's
// needs to be executed even if edge.dmnObject.namespace !== definitions["@_namespace"]
// As they may be DMNEdge depictions for edges targeting external nodes
let deletedDmnEdgeOnCurrentDrd: Normalized<DMNDI15__DMNEdge> | undefined;

const drdCount = (definitions["dmndi:DMNDI"]?.["dmndi:DMNDiagram"] ?? []).length;
Expand Down
17 changes: 15 additions & 2 deletions packages/dmn-editor/src/store/computed/computeDiagramData.ts
Original file line number Diff line number Diff line change
Expand Up @@ -140,7 +140,13 @@ export function computeDiagramData(
};

// requirements
ackRequirementEdges(definitions["@_namespace"], definitions["@_namespace"], definitions.drgElement, ackEdge);
ackRequirementEdges(
definitions,
definitions["@_namespace"],
definitions["@_namespace"],
definitions.drgElement,
ackEdge
);

// associations
(definitions.artifact ?? []).forEach((dmnObject, index) => {
Expand Down Expand Up @@ -270,6 +276,7 @@ export function computeDiagramData(
(acc, [namespace, externalDmn]) => {
// Taking advantage of the loop to add the edges here...
ackRequirementEdges(
definitions,
definitions["@_namespace"],
externalDmn.model.definitions["@_namespace"],
externalDmn.model.definitions.drgElement,
Expand Down Expand Up @@ -390,6 +397,7 @@ export function computeDiagramData(
}

function ackRequirementEdges(
definitions: State["dmn"]["model"]["definitions"],
thisDmnsNamespace: string,
drgElementsNamespace: string,
drgElements: Normalized<DMN15__tDefinitions>["drgElement"],
Expand All @@ -402,8 +410,13 @@ function ackRequirementEdges(
if (dmnObject.__$$element === "decision") {
(dmnObject.informationRequirement ?? []).forEach((ir, index) => {
const irHref = parseXmlHref((ir.requiredDecision ?? ir.requiredInput)!["@_href"]);
// Search for definitions[`@_xmlns:included${includedIndex}`] that holds proper namespace
// and store the proper prefix: `included${includedIndex}` value
const namespaceIncludedPrefix = Object.entries(definitions)
.find(([key, val]) => val === namespace)?.[0]
?.replace("@_xmlns:", "");
ackEdge({
id: ir["@_id"]!,
id: (namespaceIncludedPrefix ? `${namespaceIncludedPrefix}:` : "") + ir["@_id"]!,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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?

Copy link
Contributor

@tiagobento tiagobento Sep 25, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. The id of the ReactFlow object (I.e., RF.Edge), which I'm saying should be in HREF format.
  2. The id attribute (@_id) of the Information Requirement (which is a plain UUID).
  3. The id attribute (@_id) of the DMNEdge (which is a plain UUID).
  4. 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 :)

Copy link
Contributor

@tiagobento tiagobento Sep 25, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

dmnObject: {
namespace: drgElementsNamespace,
type: dmnObject.__$$element,
Expand Down
2 changes: 1 addition & 1 deletion packages/dmn-editor/tests-e2e/__fixtures__/drgNodes.ts
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ export class DrgNodes {
public page: Page
) {}

public async open() {
public async toggle() {
await this.page.getByTitle("DRG Nodes").click();
}

Expand Down
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Loading