ljmotta commented on code in PR #3575:
URL:
https://github.com/apache/incubator-kie-tools/pull/3575#discussion_r3290415968
##########
packages/bpmn-editor/src/normalization/normalize.ts:
##########
@@ -31,6 +32,95 @@ type WithRequiredDeep<T, K extends keyof any> = T extends
undefined
? { [P in K]-?: NonNullable<WithRequiredDeep<T[P], K>> }
: T);
+// Get the array for `key`, or create an empty one if missing.
+function addOrGetGroup<K, V>(map: Map<K, V[]>, key: K): V[] {
+ let group = map.get(key);
+ if (!group) {
+ group = [];
+ map.set(key, group);
+ }
+ return group;
+}
Review Comment:
Please, consider inlining this function. The function is used just one time
and adds complexity. Here is an example of how you could do it, but you can use
`if/else` block if you think it is better.
```suggestion
if (rootElement.__$$element === "itemDefinition") {
const itemDefinitionStructureRef = rootElement["@_structureRef"] ?? "";
itemDefinitionsGroupedStructureRef.set(
itemDefinitionStructureRef,
[...(itemDefinitionsGroupedStructureRef.get(itemDefinitionStructureRef) ?? []),
rootElement]
)
}
```
##########
packages/bpmn-editor/src/normalization/normalize.ts:
##########
@@ -31,6 +32,95 @@ type WithRequiredDeep<T, K extends keyof any> = T extends
undefined
? { [P in K]-?: NonNullable<WithRequiredDeep<T[P], K>> }
: T);
+// Get the array for `key`, or create an empty one if missing.
+function addOrGetGroup<K, V>(map: Map<K, V[]>, key: K): V[] {
+ let group = map.get(key);
+ if (!group) {
+ group = [];
+ map.set(key, group);
+ }
+ return group;
+}
+
+function getRefToRemap(itemDefinitionIdsToRemap: Map<string, string>, id:
string | undefined): string | undefined {
+ return id === undefined ? undefined : itemDefinitionIdsToRemap.get(id) ?? id;
+}
+
+function remapDataItems(
+ itemDefinitionIdsToRemap: Map<string, string>,
+ items: Array<{ "@_itemSubjectRef"?: string }> | undefined
+) {
+ if (!items) {
+ return;
+ }
+ for (const item of items) {
+ if (item["@_itemSubjectRef"] !== undefined) {
+ item["@_itemSubjectRef"] = getRefToRemap(itemDefinitionIdsToRemap,
item["@_itemSubjectRef"]);
+ }
Review Comment:
```suggestion
if (item["@_itemSubjectRef"] !== undefined) {
item["@_itemSubjectRef"] =
itemDefinitionIdsToRemap.get(item["@_itemSubjectRef"]) ??
item["@_itemSubjectRef"];
}
```
##########
packages/bpmn-editor/src/normalization/normalize.ts:
##########
@@ -31,6 +32,95 @@ type WithRequiredDeep<T, K extends keyof any> = T extends
undefined
? { [P in K]-?: NonNullable<WithRequiredDeep<T[P], K>> }
: T);
+// Get the array for `key`, or create an empty one if missing.
+function addOrGetGroup<K, V>(map: Map<K, V[]>, key: K): V[] {
+ let group = map.get(key);
+ if (!group) {
+ group = [];
+ map.set(key, group);
+ }
+ return group;
+}
+
+function getRefToRemap(itemDefinitionIdsToRemap: Map<string, string>, id:
string | undefined): string | undefined {
+ return id === undefined ? undefined : itemDefinitionIdsToRemap.get(id) ?? id;
+}
Review Comment:
I believe we can inline this to make the operation explict (we are hiding
somehting important with it). As far I could see, we always want to check if
`"@_itemSubjectRef"]` exist before attributing it. If `["@_itemSubjectRef"]`
doesn't exist its value will be left as `undefined`.
##########
packages/bpmn-editor/src/normalization/normalize.ts:
##########
@@ -89,7 +179,63 @@ export function normalize(model: BpmnLatestModel):
State["bpmn"]["model"] {
}
});
+ // Merge itemDefinitions that share the same structureRef.
+ deduplicateItemDefinitions(model.definitions);
+
const normalizedModel = model as Normalized<BpmnLatestModel>;
Review Comment:
Consider using the `deduplicateItemDefinition` after the casting, as all
elements from the `normalizedModel` have `@_id` element
##########
packages/bpmn-editor/src/normalization/normalize.ts:
##########
@@ -89,7 +179,63 @@ export function normalize(model: BpmnLatestModel):
State["bpmn"]["model"] {
}
});
+ // Merge itemDefinitions that share the same structureRef.
+ deduplicateItemDefinitions(model.definitions);
+
const normalizedModel = model as Normalized<BpmnLatestModel>;
return normalizedModel;
}
+
+// Keep one itemDefinition per structureRef and update references to point to
it.
+export function deduplicateItemDefinitions(definitions:
BpmnLatestModel["definitions"]): void {
+ const itemDefinitionsGroupedStructureRef = new Map<string, Array<{ "@_id"?:
string }>>();
+ for (const rootElement of definitions.rootElement ?? []) {
+ if (rootElement.__$$element === "itemDefinition") {
+ const itemDefinitionStructureRef = rootElement["@_structureRef"] ?? "";
+ addOrGetGroup(itemDefinitionsGroupedStructureRef,
itemDefinitionStructureRef).push(rootElement);
+ }
+ }
+
+ // Map each duplicate id to the id of the first occurrence we keep.
+ const itemDefinitionIdsToRemap = new Map<string, string>();
+ for (const itemDefinitionsGroup of
itemDefinitionsGroupedStructureRef.values()) {
+ if (itemDefinitionsGroup.length <= 1) {
+ continue;
+ }
+ const survivorId = itemDefinitionsGroup[0]["@_id"];
+ if (!survivorId) {
+ continue;
+ }
+ for (let i = 1; i < itemDefinitionsGroup.length; i++) {
+ const duplicateId = itemDefinitionsGroup[i]["@_id"];
+ if (duplicateId) {
+ itemDefinitionIdsToRemap.set(duplicateId, survivorId);
+ }
+ }
+ }
+
+ if (itemDefinitionIdsToRemap.size === 0) {
+ return;
+ }
+
+ // Remove the duplicate itemDefinitions and update every itemSubjectRef.
+ const itemDefinitionIdsToRemove = new Set(itemDefinitionIdsToRemap.keys());
Review Comment:
This isn't necessary, like a Set, a Map does not support two entries with
the same key and it implements the `has` method.
##########
packages/bpmn-editor/src/normalization/normalize.ts:
##########
@@ -89,7 +179,63 @@ export function normalize(model: BpmnLatestModel):
State["bpmn"]["model"] {
}
});
+ // Merge itemDefinitions that share the same structureRef.
+ deduplicateItemDefinitions(model.definitions);
+
const normalizedModel = model as Normalized<BpmnLatestModel>;
return normalizedModel;
}
+
+// Keep one itemDefinition per structureRef and update references to point to
it.
+export function deduplicateItemDefinitions(definitions:
BpmnLatestModel["definitions"]): void {
+ const itemDefinitionsGroupedStructureRef = new Map<string, Array<{ "@_id"?:
string }>>();
+ for (const rootElement of definitions.rootElement ?? []) {
+ if (rootElement.__$$element === "itemDefinition") {
+ const itemDefinitionStructureRef = rootElement["@_structureRef"] ?? "";
+ addOrGetGroup(itemDefinitionsGroupedStructureRef,
itemDefinitionStructureRef).push(rootElement);
+ }
+ }
+
+ // Map each duplicate id to the id of the first occurrence we keep.
+ const itemDefinitionIdsToRemap = new Map<string, string>();
+ for (const itemDefinitionsGroup of
itemDefinitionsGroupedStructureRef.values()) {
+ if (itemDefinitionsGroup.length <= 1) {
+ continue;
+ }
+ const survivorId = itemDefinitionsGroup[0]["@_id"];
+ if (!survivorId) {
+ continue;
+ }
+ for (let i = 1; i < itemDefinitionsGroup.length; i++) {
+ const duplicateId = itemDefinitionsGroup[i]["@_id"];
+ if (duplicateId) {
+ itemDefinitionIdsToRemap.set(duplicateId, survivorId);
+ }
+ }
+ }
+
+ if (itemDefinitionIdsToRemap.size === 0) {
+ return;
+ }
+
+ // Remove the duplicate itemDefinitions and update every itemSubjectRef.
+ const itemDefinitionIdsToRemove = new Set(itemDefinitionIdsToRemap.keys());
+ definitions.rootElement = definitions.rootElement?.filter(
+ (e) => !(e.__$$element === "itemDefinition" && e["@_id"] &&
itemDefinitionIdsToRemove.has(e["@_id"]))
+ );
+
+ for (const rootElement of definitions.rootElement ?? []) {
+ if (rootElement.__$$element === "dataStore") {
+ rootElement["@_itemSubjectRef"] =
getRefToRemap(itemDefinitionIdsToRemap, rootElement["@_itemSubjectRef"]);
+ }
+ if (rootElement.__$$element === "process") {
+ remapDataItems(itemDefinitionIdsToRemap,
rootElement.ioSpecification?.dataInput);
+ remapDataItems(itemDefinitionIdsToRemap,
rootElement.ioSpecification?.dataOutput);
+ remapProperties(itemDefinitionIdsToRemap, rootElement.property);
+ if (rootElement.flowElement) {
+ remapFlowElements(itemDefinitionIdsToRemap, rootElement.flowElement);
+ }
+ }
Review Comment:
Please, consider making the same modification here with `if/else if/else`
blocks. This list does not take into account the `@_type` property on
`correlationProperty` element.
##########
packages/bpmn-editor/src/normalization/normalize.ts:
##########
@@ -31,6 +32,95 @@ type WithRequiredDeep<T, K extends keyof any> = T extends
undefined
? { [P in K]-?: NonNullable<WithRequiredDeep<T[P], K>> }
: T);
+// Get the array for `key`, or create an empty one if missing.
+function addOrGetGroup<K, V>(map: Map<K, V[]>, key: K): V[] {
+ let group = map.get(key);
+ if (!group) {
+ group = [];
+ map.set(key, group);
+ }
+ return group;
+}
+
+function getRefToRemap(itemDefinitionIdsToRemap: Map<string, string>, id:
string | undefined): string | undefined {
+ return id === undefined ? undefined : itemDefinitionIdsToRemap.get(id) ?? id;
+}
+
+function remapDataItems(
+ itemDefinitionIdsToRemap: Map<string, string>,
+ items: Array<{ "@_itemSubjectRef"?: string }> | undefined
+) {
+ if (!items) {
+ return;
+ }
+ for (const item of items) {
+ if (item["@_itemSubjectRef"] !== undefined) {
+ item["@_itemSubjectRef"] = getRefToRemap(itemDefinitionIdsToRemap,
item["@_itemSubjectRef"]);
+ }
+ }
+}
+
+function remapProperties(
+ itemDefinitionIdsToRemap: Map<string, string>,
+ properties: Array<{ "@_itemSubjectRef"?: string }> | undefined
+) {
+ if (!properties) {
+ return;
+ }
+ for (const p of properties) {
+ p["@_itemSubjectRef"] = getRefToRemap(itemDefinitionIdsToRemap,
p["@_itemSubjectRef"]);
+ }
Review Comment:
```suggestion
for (const p of properties) {
if (p["@_itemSubjectRef"]) {
p["@_itemSubjectRef"] =
itemDefinitionIdsToRemap.get(p["@_itemSubjectRef"]) ?? p["@_itemSubjectRef"]
}
}
```
##########
packages/bpmn-editor/src/normalization/normalize.ts:
##########
@@ -31,6 +32,95 @@ type WithRequiredDeep<T, K extends keyof any> = T extends
undefined
? { [P in K]-?: NonNullable<WithRequiredDeep<T[P], K>> }
: T);
+// Get the array for `key`, or create an empty one if missing.
+function addOrGetGroup<K, V>(map: Map<K, V[]>, key: K): V[] {
+ let group = map.get(key);
+ if (!group) {
+ group = [];
+ map.set(key, group);
+ }
+ return group;
+}
+
+function getRefToRemap(itemDefinitionIdsToRemap: Map<string, string>, id:
string | undefined): string | undefined {
+ return id === undefined ? undefined : itemDefinitionIdsToRemap.get(id) ?? id;
+}
+
+function remapDataItems(
+ itemDefinitionIdsToRemap: Map<string, string>,
+ items: Array<{ "@_itemSubjectRef"?: string }> | undefined
+) {
+ if (!items) {
+ return;
+ }
+ for (const item of items) {
+ if (item["@_itemSubjectRef"] !== undefined) {
+ item["@_itemSubjectRef"] = getRefToRemap(itemDefinitionIdsToRemap,
item["@_itemSubjectRef"]);
+ }
+ }
+}
+
+function remapProperties(
+ itemDefinitionIdsToRemap: Map<string, string>,
+ properties: Array<{ "@_itemSubjectRef"?: string }> | undefined
+) {
+ if (!properties) {
+ return;
+ }
+ for (const p of properties) {
+ p["@_itemSubjectRef"] = getRefToRemap(itemDefinitionIdsToRemap,
p["@_itemSubjectRef"]);
+ }
+}
+
+function remapFlowElements(
Review Comment:
Please, consider using an `if/else if/else` structure to ensure all cases
are covered. The `else` block can be empty with a comment informing that does
not have an `itemSubjectRef` on the other elements (if it is the case).
##########
packages/bpmn-editor/src/normalization/normalize.ts:
##########
@@ -89,7 +179,63 @@ export function normalize(model: BpmnLatestModel):
State["bpmn"]["model"] {
}
});
+ // Merge itemDefinitions that share the same structureRef.
+ deduplicateItemDefinitions(model.definitions);
+
const normalizedModel = model as Normalized<BpmnLatestModel>;
return normalizedModel;
}
+
+// Keep one itemDefinition per structureRef and update references to point to
it.
+export function deduplicateItemDefinitions(definitions:
BpmnLatestModel["definitions"]): void {
+ const itemDefinitionsGroupedStructureRef = new Map<string, Array<{ "@_id"?:
string }>>();
+ for (const rootElement of definitions.rootElement ?? []) {
+ if (rootElement.__$$element === "itemDefinition") {
+ const itemDefinitionStructureRef = rootElement["@_structureRef"] ?? "";
+ addOrGetGroup(itemDefinitionsGroupedStructureRef,
itemDefinitionStructureRef).push(rootElement);
+ }
+ }
+
+ // Map each duplicate id to the id of the first occurrence we keep.
+ const itemDefinitionIdsToRemap = new Map<string, string>();
+ for (const itemDefinitionsGroup of
itemDefinitionsGroupedStructureRef.values()) {
+ if (itemDefinitionsGroup.length <= 1) {
+ continue;
+ }
+ const survivorId = itemDefinitionsGroup[0]["@_id"];
+ if (!survivorId) {
+ continue;
+ }
Review Comment:
This should never happen.
##########
packages/bpmn-editor/src/normalization/normalize.ts:
##########
@@ -89,7 +179,63 @@ export function normalize(model: BpmnLatestModel):
State["bpmn"]["model"] {
}
});
+ // Merge itemDefinitions that share the same structureRef.
+ deduplicateItemDefinitions(model.definitions);
+
const normalizedModel = model as Normalized<BpmnLatestModel>;
return normalizedModel;
}
+
+// Keep one itemDefinition per structureRef and update references to point to
it.
+export function deduplicateItemDefinitions(definitions:
BpmnLatestModel["definitions"]): void {
+ const itemDefinitionsGroupedStructureRef = new Map<string, Array<{ "@_id"?:
string }>>();
Review Comment:
By using the normalized model, you can have an `Array<string>`
```suggestion
export function deduplicateItemDefinitions(definitions:
Normalized<BpmnLatestModel["definitions"]>): void {
const itemDefinitionsGroupedStructureRef = new Map<string,
Array<string>>();
```
##########
packages/bpmn-editor/src/normalization/normalize.ts:
##########
@@ -89,7 +179,63 @@ export function normalize(model: BpmnLatestModel):
State["bpmn"]["model"] {
}
});
+ // Merge itemDefinitions that share the same structureRef.
+ deduplicateItemDefinitions(model.definitions);
+
const normalizedModel = model as Normalized<BpmnLatestModel>;
return normalizedModel;
}
+
+// Keep one itemDefinition per structureRef and update references to point to
it.
+export function deduplicateItemDefinitions(definitions:
BpmnLatestModel["definitions"]): void {
+ const itemDefinitionsGroupedStructureRef = new Map<string, Array<{ "@_id"?:
string }>>();
+ for (const rootElement of definitions.rootElement ?? []) {
+ if (rootElement.__$$element === "itemDefinition") {
+ const itemDefinitionStructureRef = rootElement["@_structureRef"] ?? "";
+ addOrGetGroup(itemDefinitionsGroupedStructureRef,
itemDefinitionStructureRef).push(rootElement);
+ }
+ }
+
+ // Map each duplicate id to the id of the first occurrence we keep.
+ const itemDefinitionIdsToRemap = new Map<string, string>();
+ for (const itemDefinitionsGroup of
itemDefinitionsGroupedStructureRef.values()) {
+ if (itemDefinitionsGroup.length <= 1) {
+ continue;
+ }
+ const survivorId = itemDefinitionsGroup[0]["@_id"];
+ if (!survivorId) {
+ continue;
+ }
+ for (let i = 1; i < itemDefinitionsGroup.length; i++) {
+ const duplicateId = itemDefinitionsGroup[i]["@_id"];
+ if (duplicateId) {
+ itemDefinitionIdsToRemap.set(duplicateId, survivorId);
+ }
+ }
+ }
+
+ if (itemDefinitionIdsToRemap.size === 0) {
+ return;
+ }
+
+ // Remove the duplicate itemDefinitions and update every itemSubjectRef.
+ const itemDefinitionIdsToRemove = new Set(itemDefinitionIdsToRemap.keys());
+ definitions.rootElement = definitions.rootElement?.filter(
+ (e) => !(e.__$$element === "itemDefinition" && e["@_id"] &&
itemDefinitionIdsToRemove.has(e["@_id"]))
+ );
+
+ for (const rootElement of definitions.rootElement ?? []) {
+ if (rootElement.__$$element === "dataStore") {
+ rootElement["@_itemSubjectRef"] =
getRefToRemap(itemDefinitionIdsToRemap, rootElement["@_itemSubjectRef"]);
+ }
+ if (rootElement.__$$element === "process") {
+ remapDataItems(itemDefinitionIdsToRemap,
rootElement.ioSpecification?.dataInput);
+ remapDataItems(itemDefinitionIdsToRemap,
rootElement.ioSpecification?.dataOutput);
+ remapProperties(itemDefinitionIdsToRemap, rootElement.property);
+ if (rootElement.flowElement) {
+ remapFlowElements(itemDefinitionIdsToRemap, rootElement.flowElement);
+ }
+ }
Review Comment:
It would be good to include "message" elements too, which will start to use
the `itemRef` after https://github.com/apache/incubator-kie-tools/pull/3555
--
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]
