tiagobento commented on code in PR #2508:
URL:
https://github.com/apache/incubator-kie-tools/pull/2508#discussion_r1752123645
##########
packages/dmn-editor/src/externalNodes/DmnObjectListItem.tsx:
##########
@@ -50,14 +52,70 @@ export function DmnObjectListItem({
);
const isAlternativeInputDataShape = useDmnEditorStore((s) =>
s.computed(s).isAlternativeInputDataShape());
- const displayName = dmnObject
- ? buildFeelQNameFromNamespace({
- namedElement: dmnObject,
- importsByNamespace,
- namespace,
- relativeToNamespace,
- }).full
- : dmnObjectHref;
+ // The dmnObject represented here can be a node from a 3rd model that is not
included in this model.
+ // For example, consider a "Local Decision Service" with an encapsulated
"Decision-A" from "Model A",
+ // but that "Decision-A" have an "Input-B" that is from "Model B", which is
not included in local model.
+ //
+ // Model Name: Local Model.dmn
+ // Nodes: Local Decision Service
+ // Included Models: Model-A.dmn
+ //
+ // Model Name: Model-A.dmn
+ // Nodes: Decision-A
+ // Included Models: Model-B.dmn
+ //
+ // Model Name: Model-B.dmn
+ // Nodes: Input-B
+ // Included Models: [none]
+ //
+ // So, the "Local Model" only "knows" the nodes from "Model-A" and NOT from
"Model-B".
+ // That's why we have different logic to build description for "known
dmnObjects" and "unknown dmnObjects".
+ const isNamespaceIncluded = useMemo(
+ () => namespace === relativeToNamespace ||
importsByNamespace.has(namespace),
+ [importsByNamespace, namespace, relativeToNamespace]
+ );
+
+ const notIncludedNamespaceDescription = useMemo(() => {
Review Comment:
Maybe `transitivelyImportedNamespaceDescription`?
##########
packages/dmn-editor/src/diagram/Diagram.tsx:
##########
@@ -1125,6 +1132,7 @@ export const Diagram = React.forwardRef<DiagramRef, {
container: React.RefObject
shapeId: targetNode.data.shape["@_id"],
},
keepWaypoints: true,
+ externalModelsByNamespace: externalModelsByNamespace,
Review Comment:
externalModelsByNamespace
##########
packages/dmn-editor/src/mutations/addExistingDecisionServiceToDrd.ts:
##########
@@ -92,11 +92,15 @@ export function
getStrategyToAddExistingDecisionServiceToDrd({
id: __readonly_drgElement["@_id"]!,
});
+ const drgElementsByNamespace = new Map([[__readonly_namespace,
__readonly_definitions.drgElement]]);
+ __readonly_externalDmnsIndex.forEach((value, key) => {
+ drgElementsByNamespace.set(key, value.model.definitions.drgElement);
+ });
+
const containingDecisionServiceHrefsByDecisionHrefsRelativeToThisDmn =
computeContainingDecisionServiceHrefsByDecisionHrefs({
thisDmnsNamespace: __readonly_namespace,
- drgElementsNamespace: __readonly_decisionServiceNamespace,
- drgElements: decisionServiceDmnDefinitions.drgElement,
+ drgElementsByNamespace: drgElementsByNamespace,
Review Comment:
drgElementsByNamespace
##########
packages/dmn-editor/src/mutations/repopulateInputDataAndDecisionsOnDecisionService.ts:
##########
@@ -76,6 +85,47 @@ export function
repopulateInputDataAndDecisionsOnDecisionService({
});
}
+ for (const hrefString of hrefsToDecisionsInsideDecisionService) {
Review Comment:
This for is complementary to the for above, but it ALSO treats external
nodes. You can have the for above be part of this, checking whether `href` has
a namespace or not. No need to have all these "continue"s.
##########
packages/dmn-editor/src/propertiesPanel/DecisionServiceProperties.tsx:
##########
@@ -449,3 +462,10 @@ function DecisionServiceEquivalentFunction({
</Alert>
);
}
+
+function buildDisplayName(
+ dmnObject: Unpacked<Normalized<DMN15__tDefinitions>["drgElement"]> |
undefined,
+ namespace: string
+) {
+ return `${namespace.substring(0,
11)}...${namespace.substring(namespace.length - 4)}.${dmnObject?.["@_name"]}`;
+}
Review Comment:
Maybe `buildDisplayNameForDmnObject`?
##########
packages/dmn-editor/src/mutations/repopulateInputDataAndDecisionsOnDecisionService.ts:
##########
@@ -76,6 +85,47 @@ export function
repopulateInputDataAndDecisionsOnDecisionService({
});
}
+ for (const hrefString of hrefsToDecisionsInsideDecisionService) {
Review Comment:
See the for above, you have a string with `#` there, hard coded... That's
something I think we can't have anymore...
##########
packages/dmn-editor/src/mutations/deleteDecisionFromDecisionService.ts:
##########
@@ -44,12 +66,11 @@ export function deleteDecisionFromDecisionService({
);
}
- decisionService.outputDecision = (decisionService.outputDecision ??
[]).filter(
- (s) => s["@_href"] !== `#${decisionId}`
- );
+ const xmlHref = buildXmlHref({ namespace: href.namespace, id: href.id });
Review Comment:
`xmlHref` is exactly the same as `href`... you're even building it with the
same values. No need to have this `xmlHref` variable...
##########
packages/dmn-editor/src/includedModels/IncludedModels.tsx:
##########
@@ -471,6 +472,7 @@ function IncludedModelCard({
definitions: state.dmn.model.definitions,
__readonly_index: index,
__readonly_externalModelTypesByNamespace:
externalModelTypesByNamespace,
+ externalModelsByNamespace,
Review Comment:
__readonly_externalModelsByNamespace
##########
packages/dmn-editor/src/mutations/deleteNode.ts:
##########
@@ -208,16 +212,15 @@ export function canRemoveNodeFromDrdOnly({
id: __readonly_dmnObjectId!,
});
- const drgElements =
- definitions["@_namespace"] === __readonly_dmnObjectNamespace
- ? definitions.drgElement ?? []
- :
__readonly_externalDmnsIndex.get(__readonly_dmnObjectNamespace)?.model.definitions.drgElement
?? [];
+ const drgElementsByNamespace = new Map([[__readonly_dmnObjectNamespace,
definitions.drgElement]]);
+ __readonly_externalDmnsIndex.forEach((value, key) => {
+ drgElementsByNamespace.set(key, value.model.definitions.drgElement);
+ });
const containingDecisionServiceHrefsByDecisionHrefsRelativeToThisDmn =
computeContainingDecisionServiceHrefsByDecisionHrefs({
thisDmnsNamespace: definitions["@_namespace"],
- drgElementsNamespace: __readonly_dmnObjectNamespace,
- drgElements,
+ drgElementsByNamespace: drgElementsByNamespace,
Review Comment:
drgElementsByNamespace
##########
packages/dmn-editor/src/mutations/addEdge.ts:
##########
@@ -188,7 +191,10 @@ export function addEdge({
// Replace with the new one.
diagramElements.push(newDmnEdge);
- repopulateInputDataAndDecisionsOnAllDecisionServices({ definitions });
+ repopulateInputDataAndDecisionsOnAllDecisionServices({
+ definitions,
+ externalModelsByNamespace: externalModelsByNamespace,
Review Comment:
externalModelsByNamespace
##########
packages/dmn-editor/src/externalNodes/DmnObjectListItem.tsx:
##########
@@ -50,14 +52,70 @@ export function DmnObjectListItem({
);
const isAlternativeInputDataShape = useDmnEditorStore((s) =>
s.computed(s).isAlternativeInputDataShape());
- const displayName = dmnObject
- ? buildFeelQNameFromNamespace({
- namedElement: dmnObject,
- importsByNamespace,
- namespace,
- relativeToNamespace,
- }).full
- : dmnObjectHref;
+ // The dmnObject represented here can be a node from a 3rd model that is not
included in this model.
+ // For example, consider a "Local Decision Service" with an encapsulated
"Decision-A" from "Model A",
+ // but that "Decision-A" have an "Input-B" that is from "Model B", which is
not included in local model.
+ //
+ // Model Name: Local Model.dmn
+ // Nodes: Local Decision Service
+ // Included Models: Model-A.dmn
+ //
+ // Model Name: Model-A.dmn
+ // Nodes: Decision-A
+ // Included Models: Model-B.dmn
+ //
+ // Model Name: Model-B.dmn
+ // Nodes: Input-B
+ // Included Models: [none]
+ //
+ // So, the "Local Model" only "knows" the nodes from "Model-A" and NOT from
"Model-B".
+ // That's why we have different logic to build description for "known
dmnObjects" and "unknown dmnObjects".
+ const isNamespaceIncluded = useMemo(
Review Comment:
Maybe `isNamespaceDirectlyImported`?
##########
packages/dmn-editor/src/propertiesPanel/DecisionServiceProperties.tsx:
##########
@@ -410,14 +423,14 @@ function DecisionServiceEquivalentFunction({
const dmnObject = allDrgElementsByHref.get(potentialExternalHref);
- return dmnObject
+ return dmnObject && (importsByNamespace.has(resolvedNamespace) ||
resolvedNamespace === thisDmnsNamespace)
Review Comment:
Maybe extract `(importsByNamespace.has(resolvedNamespace) ||
resolvedNamespace === thisDmnsNamespace)` to a variable and give it a
descriptive name?
##########
packages/dmn-editor/src/propertiesPanel/DecisionServiceProperties.tsx:
##########
@@ -66,14 +69,24 @@ export function DecisionServiceProperties({
const thisDmn = useDmnEditorStore((s) => s.dmn);
const { externalModelsByNamespace } = useExternalModels();
- const externalDmnsByNamespace = useDmnEditorStore(
- (s) =>
s.computed(s).getExternalModelTypesByNamespace(externalModelsByNamespace).dmns
- );
+
+ const allExternalDmns = Object.entries(externalModelsByNamespace ??
{}).reduce((acc, [namespace, externalModel]) => {
Review Comment:
Maybe `useDmnEditorStore((s) =>
s.computed(s).getExternalModelTypesByNamespace(externalModelsByNamespace).dmns)`?
Not sure if it is exactly the same, though, but looks like it.
##########
packages/dmn-editor/src/mutations/addExistingDecisionServiceToDrd.ts:
##########
@@ -308,6 +312,7 @@ export async function addAutoGeneratedDecisionServiceToDrd({
__readonly_drdIndex: dummyDrdIndex,
__readonly_dmnObjectNamespace: __readonly_decisionServiceNamespace,
__readonly_externalDmnsIndex: externalModelTypesByNamespace.dmns,
+ __readonly_externalModelsByNamespace: __readonly_externalModelsByNamespace,
Review Comment:
:D
##########
packages/dmn-editor/src/mutations/deleteDecisionFromDecisionService.ts:
##########
@@ -20,21 +20,43 @@
import { DMN15__tDefinitions } from
"@kie-tools/dmn-marshaller/dist/schemas/dmn-1_5/ts-gen/types";
import { repopulateInputDataAndDecisionsOnDecisionService } from
"./repopulateInputDataAndDecisionsOnDecisionService";
import { Normalized } from "../normalization/normalize";
+import { buildXmlHref, parseXmlHref } from "../xml/xmlHrefs";
+import { DmnLatestModel } from "@kie-tools/dmn-marshaller/dist";
+import { ExternalModelsIndex } from "../DmnEditor";
export function deleteDecisionFromDecisionService({
definitions,
- decisionId,
+ decisionHref,
decisionServiceId,
+ externalModelsByNamespace,
}: {
definitions: Normalized<DMN15__tDefinitions>;
- decisionId: string;
+ decisionHref: string;
decisionServiceId: string;
+ externalModelsByNamespace: ExternalModelsIndex | undefined;
}) {
- console.debug(`DMN MUTATION: Deleting Decision '${decisionId}' from Decision
Service '${decisionServiceId}'`);
+ console.debug(`DMN MUTATION: Deleting Decision '${decisionHref}' from
Decision Service '${decisionServiceId}'`);
- const decision = definitions.drgElement?.find((s) => s["@_id"] ===
decisionId);
- if (decision?.__$$element !== "decision") {
- throw new Error(`DMN MUTATION: DRG Element with id '${decisionId}' is
either not a Decision or doesn't exist.`);
+ const href = parseXmlHref(decisionHref);
+
+ const externalModel = externalModelsByNamespace?.[href.namespace ?? ""];
+ if (href.namespace && !externalModel) {
+ throw new Error(`DMN MUTATION: Namespace '${href.namespace}' not found.`);
+ }
+
+ if (href.namespace) {
+ const externalDrgs = (externalModel?.model as
Normalized<DmnLatestModel>).definitions.drgElement;
Review Comment:
Instead of casting, you can check if `externalMode.type === "dmn"`, no?
##########
packages/dmn-editor/src/mutations/repopulateInputDataAndDecisionsOnDecisionService.ts:
##########
@@ -76,6 +85,47 @@ export function
repopulateInputDataAndDecisionsOnDecisionService({
});
}
+ for (const hrefString of hrefsToDecisionsInsideDecisionService) {
+ const href = parseXmlHref(hrefString);
+ const externalModel = externalModelsByNamespace?.[href.namespace ?? ""];
+ if (!externalModel) {
+ continue;
+ }
+
+ const externalDecision = (externalModel?.model as
Normalized<DmnLatestModel>).definitions.drgElement?.find(
+ (drgElement) => drgElement["@_id"] === href.id
+ ) as Normalized<DMN15__tDecision>;
+ if (!externalDecision) {
+ continue;
Review Comment:
Same comment as the other place. You can check if `externalModel` is a DMN
to avoid the casting. Also, if you don't find the `externalDecision` here, it
means there's a problem, right?
##########
packages/dmn-editor-envelope/src/DmnEditorRoot.tsx:
##########
@@ -563,6 +563,94 @@ function ExternalModelsManager({
};
}, [thisDmnsNormalizedPosixPathRelativeToTheWorkspaceRoot]);
+ const getIncludedNamespacesFromModel = useCallback((model:
Normalized<DmnLatestModel>) => {
+ return (model.definitions.import ?? [])
+ .map((i) => getNamespaceOfDmnImport({ dmnImport: i }))
+ .join(NAMESPACES_EFFECT_SEPARATOR);
+ }, []);
Review Comment:
Duplicated code, and there's no need for this to be a `useCallback`, it can
be a function outside of the component. Please extract it and use to compute
the `namespaces` variable too.
##########
packages/dmn-editor-envelope/src/DmnEditorRoot.tsx:
##########
@@ -563,6 +563,94 @@ function ExternalModelsManager({
};
}, [thisDmnsNormalizedPosixPathRelativeToTheWorkspaceRoot]);
+ const getIncludedNamespacesFromModel = useCallback((model:
Normalized<DmnLatestModel>) => {
+ return (model.definitions.import ?? [])
+ .map((i) => getNamespaceOfDmnImport({ dmnImport: i }))
+ .join(NAMESPACES_EFFECT_SEPARATOR);
+ }, []);
+
+ const getDmnsByNamespace = useCallback((resources: (ResourceContent |
undefined)[]) => {
+ const ret = new Map<string, ResourceContent>();
+ for (let i = 0; i < resources.length; i++) {
+ const resource = resources[i];
+ if (!resource) {
+ continue;
+ }
+
+ const content = resource.content ?? "";
+ const ext =
__path.extname(resource.normalizedPosixPathRelativeToTheWorkspaceRoot);
+ if (ext === ".dmn") {
+ const namespace =
domParser.getDomDocument(content).documentElement.getAttribute("namespace");
+ if (namespace) {
+ // Check for multiplicity of namespaces on DMN models
+ if (ret.has(namespace)) {
+ console.warn(
+ `DMN EDITOR ROOT: Multiple DMN models encountered with the same
namespace '${namespace}': '${
+ resource.normalizedPosixPathRelativeToTheWorkspaceRoot
+ }' and '${
+
ret.get(namespace)!.normalizedPosixPathRelativeToTheWorkspaceRoot
+ }'. The latter will be considered.`
+ );
+ }
+
+ ret.set(namespace, resource);
+ }
+ }
+ }
+
+ return ret;
+ }, []);
+
+ // Load all included models from the model and the included models of those
models, recursively.
+ const loadDependentModels = useCallback(
+ (
+ model: Normalized<DmnLatestModel>,
+ externalModelsIndex: DmnEditor.ExternalModelsIndex,
+ resourcesByNamespace: Map<string, ResourceContent>,
+ loadedDmnsByPathRelativeToTheWorkspaceRoot: Set<string>,
+ thisDmnsNormalizedPosixPathRelativeToTheWorkspaceRoot: string
+ ) => {
+ const includedNamespaces = new
Set(getIncludedNamespacesFromModel(model).split(NAMESPACES_EFFECT_SEPARATOR));
+
+ for (const includedNamespace of includedNamespaces) {
+ if (!resourcesByNamespace.has(includedNamespace)) {
+ console.warn(
+ `DMN EDITOR ROOT: The included namespace '${includedNamespace}'
for the model '${model.definitions["@_id"]}' can not be found.`
+ );
+ } else {
+ const resource = resourcesByNamespace.get(includedNamespace)!;
+ if
(loadedDmnsByPathRelativeToTheWorkspaceRoot.has(resource.normalizedPosixPathRelativeToTheWorkspaceRoot))
{
+ continue;
+ }
+
+ const normalizedPosixPathRelativeToTheOpenFile = __path.relative(
+
__path.dirname(thisDmnsNormalizedPosixPathRelativeToTheWorkspaceRoot),
+ resource.normalizedPosixPathRelativeToTheWorkspaceRoot
+ );
+ const content = resource.content ?? "";
+ const includedModel = normalize(getMarshaller(content, { upgradeTo:
"latest" }).parser.parse());
+ externalModelsIndex[includedNamespace] = {
+ normalizedPosixPathRelativeToTheOpenFile,
+ model: includedModel,
+ type: "dmn",
+ svg: "",
+ };
+
+
loadedDmnsByPathRelativeToTheWorkspaceRoot.add(resource.normalizedPosixPathRelativeToTheWorkspaceRoot);
+
+ loadDependentModels(
+ includedModel,
+ externalModelsIndex,
+ resourcesByNamespace,
+ loadedDmnsByPathRelativeToTheWorkspaceRoot,
+ thisDmnsNormalizedPosixPathRelativeToTheWorkspaceRoot
+ );
Review Comment:
This `useCallback` depends on itself.. This is not very good I'd say. Not
even sure how ESLint didn't complain about this, as you're inhenritenly
depending on `loadDependentModels` from a previous tick (I.e., outdated
`getIncludedNamespacesFromModel`).
##########
packages/dmn-editor/src/diagram/Diagram.tsx:
##########
@@ -876,6 +879,7 @@ export const Diagram = React.forwardRef<DiagramRef, {
container: React.RefObject
__readonly_externalModelTypesByNamespace: state
.computed(state)
.getExternalModelTypesByNamespace(externalModelsByNamespace),
+ externalModelsByNamespace,
Review Comment:
__readonly_externalModelsByNamespace
--
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]
