tiagobento commented on code in PR #2546:
URL:
https://github.com/apache/incubator-kie-tools/pull/2546#discussion_r1775349464
##########
packages/dmn-editor/src/store/computed/computeDiagramData.ts:
##########
@@ -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"]!,
Review Comment:
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 `id`s. 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 :)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
