handle correlation properties better on rename (#145)

Before the fix, when a message’s correlation properties changed, the reconciliation logic updated what was newly present, but it did not fully clean up what had become stale. That left three bad remnants
behind when a property was renamed or removed:

- the old bpmn:correlationProperty could stay in definitions
- the old spiffworkflow:ProcessVariableCorrelation extension entry could stay on the message event
- retrieval expressions or matching references could still point at the old property id

That matters because a rename is really "remove old id, add new id," not just "edit a label." If the old id survives in XML, runtime matching can still see it and behave incorrectly.

The fix makes syncCorrelationProperties(...) reconcile in both directions:

- add or update the properties returned from the editor
- remove correlation properties for this message that are no longer returned
- remove stale ProcessVariableCorrelation extension elements tied to removed properties
- remove stale retrieval-expression references for the removed property ids

So the model is now treated as the editor response being the full source of truth for that message’s correlations, instead of only applying additive updates.

The regression test in MessagesSpec.js:179 covers the real failure shape: start with a message using singer_name, make the matching condition reference it, then simulate the editor returning singer_full_name instead. The test asserts the old id is gone from both the in-memory BPMN model and the serialized XML.

Co-authored-by: burnettk <burnettk@users.noreply.github.com>

Author: burnettk

app/spiffworkflow/messages/MessageHelpers.js

@@ -828,58 +828,76 @@ export function syncCorrelationProperties(
 ) {
   const { businessObject } = element;
   const correlationProps = findCorrelationProperties(businessObject, moddle);
-  const expressionsToDelete = [];
-
-  for (let cProperty of correlationProps) {
-    let isUsed = false;
-    for (const cpExpression of cProperty.correlationPropertyRetrievalExpression) {
-      const msgRef = cpExpression.messageRef
-        ? findMessageElement(
-            businessObject,
-            cpExpression.messageRef.id,
-            definitions
-          )
-        : undefined;
-      isUsed =
-        msgRef &&
-        msgObject &&
-        cpExpression.messageRef.id !== msgObject.identifier
-          ? true
-          : isUsed;
-      // if unused  false, delete retrival expression
+  const messageId = msgObject?.identifier;
+  const activePropertyIds = new Set(
+    (msgObject?.correlation_properties || []).map((obj) => obj.identifier)
+  );
+
+  for (const cProperty of correlationProps) {
+    const nextExpressions = (
+      cProperty.correlationPropertyRetrievalExpression || []
+    ).filter((cpExpression) => {
+      if (!cpExpression.messageRef?.id) {
+        return false;
+      }
+
+      const msgRef = findMessageElement(
+        businessObject,
+        cpExpression.messageRef.id,
+        definitions
+      );
+
       if (!msgRef) {
-        expressionsToDelete.push(cpExpression);
+        return false;
       }
-    }
 
-    // Delete the retrieval expressions that are not used
-    for (const expression of expressionsToDelete) {
-      const index =
-        cProperty.correlationPropertyRetrievalExpression.indexOf(expression);
-      if (index > -1) {
-        cProperty.correlationPropertyRetrievalExpression.splice(index, 1);
-        const cPropertyIndex = definitions
-          .get('rootElements')
-          .indexOf(cProperty);
-        definitions.rootElements.splice(cPropertyIndex, 1, cProperty);
+      if (messageId && cpExpression.messageRef.id === messageId) {
+        return activePropertyIds.has(cProperty.id);
       }
-    }
 
-    // If Unused, delete the correlation property
-    const propertyToBeDeleted =
-      isUsed ||
-      (msgObject &&
-        msgObject.correlation_properties &&
-        msgObject.correlation_properties.some(
-          (obj) => obj.identifier === cProperty.id
-        ));
-    if (!propertyToBeDeleted) {
-      const index = definitions.get('rootElements').indexOf(cProperty);
-      if (index > -1) {
-        definitions.rootElements.splice(index, 1);
+      return true;
+    });
+
+    cProperty.correlationPropertyRetrievalExpression = nextExpressions;
+
+    const hasOtherMessageReferences = nextExpressions.some(
+      (cpExpression) => cpExpression.messageRef?.id !== messageId
+    );
+    const keepProperty =
+      activePropertyIds.has(cProperty.id) || hasOtherMessageReferences;
+    const cPropertyIndex = definitions.get('rootElements').indexOf(cProperty);
+
+    if (!keepProperty) {
+      if (cPropertyIndex > -1) {
+        definitions.rootElements.splice(cPropertyIndex, 1);
       }
+      continue;
+    }
+
+    if (cPropertyIndex > -1) {
+      definitions.rootElements.splice(cPropertyIndex, 1, cProperty);
     }
   }
+
+  removeUnusedProcessVariableCorrelations(element, activePropertyIds);
+}
+
+function removeUnusedProcessVariableCorrelations(element, activePropertyIds) {
+  const extensionHost = isMessageEvent(element)
+    ? element.businessObject.eventDefinitions?.[0]
+    : element.businessObject;
+  const extensionElements = extensionHost?.get('extensionElements');
+
+  if (!extensionElements?.values) {
+    return;
+  }
+
+  extensionElements.values = extensionElements.values.filter((value) => {
+    return (
+      !value.$instanceOf('spiffworkflow:ProcessVariableCorrelation') ||
+      activePropertyIds.has(value.propertyId)
+    );
+  });
 }
 
 export function deleteMessage(definitions, messageId) {

test/spec/MessagesSpec.js

@@ -21,6 +21,7 @@ import messages from '../../app/spiffworkflow/messages';
 import { fireEvent } from '@testing-library/preact';
 import { getBpmnJS, inject } from 'bpmn-js/test/helper';
 import { findCorrelationProperties, findMessageModdleElements } from '../../app/spiffworkflow/messages/MessageHelpers';
+import { SPIFF_ADD_MESSAGE_RETURNED_EVENT } from '../../app/spiffworkflow/constants';
 
 
 describe('Messages should work', function () {
@@ -175,6 +176,73 @@ describe('Messages should work', function () {
     expect(updatedSelector.options[2].value).to.equal('msgName');
   });
 
+  it('should remove stale correlation properties and matching conditions after a rename', inject(async function (canvas, commandStack, moddle) {
+    const modeler = getBpmnJS();
+    const eventBus = modeler.get('eventBus');
+    const rootShape = canvas.getRootElement();
+
+    const receiveShape = await expectSelected('EventReceiveLetter');
+    expect(receiveShape, "Can't find Receive Event").to.exist;
+
+    const messageEventDefinition = receiveShape.businessObject.eventDefinitions[0];
+    const extensionElements = moddle.create('bpmn:ExtensionElements', {
+      values: [],
+    });
+
+    extensionElements.get('values').push(
+      moddle.create('spiffworkflow:ProcessVariableCorrelation', {
+        propertyId: 'singer_name',
+        expression: 'payload.singer',
+      })
+    );
+
+    messageEventDefinition.set('extensionElements', extensionElements);
+
+    commandStack.execute('element.updateProperties', {
+      element: receiveShape,
+      properties: {
+        'spiffworkflow:isMatchingCorrelation': true,
+      },
+    });
+
+    eventBus.fire(SPIFF_ADD_MESSAGE_RETURNED_EVENT, {
+      elementId: receiveShape.id,
+      name: 'love_letter_response',
+      correlation_properties: {
+        lover_instrument: { retrieval_expression: 'from.instrument' },
+        lover_name: { retrieval_expression: 'from.name' },
+        singer_full_name: { retrieval_expression: 'to.name' },
+      },
+    });
+
+    const updatedIds = findCorrelationProperties(rootShape.businessObject)
+      .filter((property) =>
+        property.correlationPropertyRetrievalExpression?.some(
+          (expr) => expr.messageRef?.id === 'love_letter_response'
+        )
+      )
+      .map((property) => property.id);
+
+    expect(updatedIds).to.include('singer_full_name');
+    expect(updatedIds).not.to.include('singer_name');
+
+    const variableCorrelationIds = (
+      messageEventDefinition.extensionElements?.values || []
+    )
+      .filter((value) =>
+        value.$instanceOf('spiffworkflow:ProcessVariableCorrelation')
+      )
+      .map((value) => value.propertyId);
+
+    expect(variableCorrelationIds).not.to.include('singer_name');
+
+    const { xml: updatedXml } = await modeler.saveXML({ format: true });
+
+    expect(updatedXml).to.include('singer_full_name');
+    expect(updatedXml).not.to.include('propertyId="singer_name"');
+    expect(updatedXml).not.to.include('<bpmn:correlationProperty id="singer_name"');
+  }));
+
 
 
   // 🔶🔶 OLD Features