From 9cce8d91af37eb3f41056a2db294c77b260e2a9e Mon Sep 17 00:00:00 2001 From: Sipke Schoorstra Date: Sat, 12 Sep 2026 11:41:50 -0700 Subject: [PATCH] fix(bpmn): keep a top-level call activity's fire-and-forget flag through the document PUT (#8082) * fix(bpmn): keep a top-level call activity's fire-and-forget flag through the document PUT A call activity's call options (vw:waitForCompletion) live only on its CallProcess work binding, which the bpmnDefinitions document cannot carry, so the document PUT rebuilt every call activity fresh and silently turned a fire-and-forget call into a waiting one. Reuse the stored options for a top-level call activity whose element id and calledElement are unchanged, mirroring how a kept subprocess already carries its own bindings across; a changed calledElement still binds fresh rather than inheriting options authored for a different process. Co-Authored-By: Claude Opus 5 * refactor(bpmn): read the stored BPMN source once per document PUT ImportDocumentAsync's two carry-across helpers each independently looked up SourceXmlCustomPropertyKey and parsed it, so every document PUT read and parsed the stored BPMN source twice. Read and parse it once in ImportDocumentAsync and pass the result to both helpers. Also extends the no-op PUT ETag-stability theory to the top-level-call-activity.bpmn fixture, which was not previously covered. Co-Authored-By: Claude Opus 5 * refactor(bpmn): filter call activities with Where Co-Authored-By: Claude Opus 5 --------- Co-authored-by: Claude Opus 5 --- doc/wiki/bpmn-workflows.md | 9 ++ .../BpmnInterchangeDocumentService.cs | 106 ++++++++++++++++-- .../Assets/top-level-call-activity.bpmn | 31 +++++ .../Endpoints/BpmnInterchangeEndpointTests.cs | 1 + .../Interchange/BpmnDocumentRoundTripTests.cs | 103 +++++++++++++++++ .../Support/BpmnXNamespaces.cs | 1 + 6 files changed, 243 insertions(+), 8 deletions(-) create mode 100644 test/integration/Elsa.Bpmn.Interchange.IntegrationTests/Assets/top-level-call-activity.bpmn diff --git a/doc/wiki/bpmn-workflows.md b/doc/wiki/bpmn-workflows.md index 38600b97f..edfaa4a24 100644 --- a/doc/wiki/bpmn-workflows.md +++ b/doc/wiki/bpmn-workflows.md @@ -192,6 +192,15 @@ and its layout: the document's BPMN DI carries the shapes of nested elements too definition without stored BPMN source has no bodies to restore; the `If-Match` precondition only matches a stored state a successful `GET` or `PUT` described, and both need the source. +**A top-level call activity's call options survive an unrelated `PUT`.** A call activity's options — currently just +`vw:waitForCompletion="false"`, the fire-and-forget flag — live only on its `CallProcess` work binding, the same way a +nested scope's contents do; `bpmnDefinitions` cannot carry them either. `PUT` reuses the stored options for a +top-level call activity element the posted document still declares under the same element id and the same +`calledElement`, so editing an unrelated task's binding does not silently turn a fire-and-forget call into a waiting +one on the next save. A call activity whose `calledElement` has changed does not inherit the old options: it is a call +to a different process, so it binds fresh (waiting, BPMN's default) instead. A call activity nested inside a kept +subprocess already keeps its options as part of that subprocess's whole stored body, described above. + A document that declares more than one `` is re-imported against the same `processId` it was originally imported with — recorded on the workflow definition the first time it is imported, whether from `Import` or from a `document` `PUT`, so an edit to a multi-process document does not have to name the process again on every save. diff --git a/src/modules/Elsa.Bpmn.Interchange/Services/BpmnInterchangeDocumentService.cs b/src/modules/Elsa.Bpmn.Interchange/Services/BpmnInterchangeDocumentService.cs index 02bad7e81..73c05a9ee 100644 --- a/src/modules/Elsa.Bpmn.Interchange/Services/BpmnInterchangeDocumentService.cs +++ b/src/modules/Elsa.Bpmn.Interchange/Services/BpmnInterchangeDocumentService.cs @@ -359,6 +359,14 @@ public sealed class BpmnInterchangeDocumentService( /// with the body stored for it, exactly as stored, and a subprocess element it no longer declares takes its stored /// body with it. A subprocess element with no stored body — one added by this edit — is written as declared, empty. /// + /// + /// A top-level call activity's call options — currently just — + /// live only on its work binding too, and cannot carry them either, so they come from the + /// stored source the same way: see . They are reused only for a call + /// activity whose element id and calledElement are unchanged; a changed calledElement is a call to a + /// different process, so it starts from the reader's default (waiting) rather than inheriting options authored for + /// the process it used to call. + /// /// /// The edited document, deserialized through the library's own JSON converters. /// The workflow definition to update. @@ -391,22 +399,42 @@ public sealed class BpmnInterchangeDocumentService( $"Workflow definition '{definitionId}' does not exist, so its BPMN document cannot be edited."); } - var storedNestedScopes = StoredNestedScopesStillDeclaredBy(document, existingDefinition); + var stored = ReadStoredSource(existingDefinition); + + var bindingsToCarryAcross = StoredNestedScopesStillDeclaredBy(document, stored) + .Concat(StoredCallOptionsStillApplicableTo(document, stored)) + .ToList(); // Unlike ImportAsync's xml, writer.Write itself is one of the sites that walks nested processes by matching // an id (see EnsureElementIdsUnique's remarks), and it runs before ImportCoreAsync — and the same check // inside it — ever sees this document. So it is checked here too, against exactly the inputs writer.Write is - // about to receive, before that call rather than after it. - EnsureElementIdsUnique(document.Processes, storedNestedScopes); + // about to receive, before that call rather than after it. EnsureElementIdsUnique only harvests elements from + // the NestedProcess bindings in this list, so including the CallProcess bindings alongside them changes + // nothing about what it checks. + EnsureElementIdsUnique(document.Processes, bindingsToCarryAcross); - var xml = writer.Write(document, storedNestedScopes); + var xml = writer.Write(document, bindingsToCarryAcross); return await ImportCoreAsync(xml, definitionId, name: null, processId, preserveMetadataFrom: existingDefinition, cancellationToken); } + /// + /// Reads and parses the BPMN source stored on 's + /// custom property once, for and + /// to share, rather than each independently re-reading and + /// re-parsing the same stored text. null when the definition carries no stored source at all. + /// + private BpmnImportResult? ReadStoredSource(WorkflowDefinition definition) + { + if (!definition.CustomProperties.TryGetValue(SourceXmlCustomPropertyKey, out var storedXml) || string.IsNullOrEmpty(storedXml)) + return null; + + return reader.Read(storedXml, new BpmnImportOptions()); + } + /// /// The stored work bindings needs to write every nested scope — embedded subprocess, /// transaction or event subprocess — that still declares back out exactly as - /// 's stored source has it, and nothing for a scope no + /// has it, and nothing for a scope no /// longer declares. /// /// @@ -452,12 +480,11 @@ public sealed class BpmnInterchangeDocumentService( /// A subprocess element with a stored body carries no bindingRef, so the writer cannot attach the body to it and /// would write the subprocess empty. /// - private IReadOnlyList StoredNestedScopesStillDeclaredBy(BpmnDefinitions document, WorkflowDefinition definition) + private IReadOnlyList StoredNestedScopesStillDeclaredBy(BpmnDefinitions document, BpmnImportResult? stored) { - if (!definition.CustomProperties.TryGetValue(SourceXmlCustomPropertyKey, out var storedXml) || string.IsNullOrEmpty(storedXml)) + if (stored is null) return []; - var stored = reader.Read(storedXml, new BpmnImportOptions()); var storedBindings = stored.Bindings; // Every element carrying a multi-instance marker in the model, as stored or as posted. @@ -527,6 +554,69 @@ public sealed class BpmnInterchangeDocumentService( return nested with { Definition = nested.Definition with { Extensions = extensions with { ForeignChildren = foreignChildren } } }; } + /// + /// The stored options for every top-level call activity + /// still declares under the same element id and the same calledElement. + /// + /// + /// + /// A call activity's options — currently just vw:waitForCompletion="false" — live only on its + /// work binding, never on the document + /// returns. otherwise rebuilds the whole document from + /// scratch, so a call activity it does not carry across here binds fresh and defaults to waiting, silently + /// resurrecting a wait a fire-and-forget author never asked for. + /// + /// + /// Reused only when it is safe to: the element must still exist, under the same id, and still name the same + /// calledElement as the stored binding. A changed calledElement is a call to a different process, + /// whose options this element never had, so it is left to bind fresh rather than inheriting them. + /// + /// + /// Only is scanned — exactly the call activities the document can declare. + /// One nested inside a kept subprocess is never listed there (see 's remarks); its + /// options are carried across as part of the whole kept scope by + /// instead, along with everything else bound inside it. + /// + /// + /// Matched to the stored binding by element id, then handed over under the bindingRef the posted element carries, + /// for the same reason hands a kept subprocess body over the same + /// way: the writer looks work up by bindingRef, not element id. An element with no bindingRef to hand it over + /// under is skipped rather than refused — unlike an emptied subprocess, the worst outcome is the same fresh, + /// waiting default this method exists to avoid, not data loss. + /// + /// + private IReadOnlyList StoredCallOptionsStillApplicableTo(BpmnDefinitions document, BpmnImportResult? stored) + { + if (stored is null) + return []; + + var storedCalls = stored.Bindings.OfType().ToList(); + + var kept = new List(); + + foreach (var process in document.Processes) + { + // An element with no bindingRef to hand a kept call over under is skipped rather than refused. + foreach (var element in process.Elements.Where(element => + element.ElementType == BpmnElementTypes.CallActivity && element.BindingRef is not null)) + { + // Last match wins, as it does inside the writer itself, should a malformed document repeat an id. + var call = storedCalls.LastOrDefault(candidate => candidate.ElementId == element.ElementId); + + if (call is null || !string.Equals(call.CalledElement, CalledElementOf(element), StringComparison.Ordinal)) + continue; + + kept.Add(call with { BindingRef = element.BindingRef! }); + } + } + + return kept; + } + + /// The BPMN calledElement a call activity element carries, kept by the reader for round-trip. + private static string? CalledElementOf(BpmnElement element) => + element.Properties.TryGetValue(BpmnXmlReader.CalledElementPropertyKey, out var calledElement) ? calledElement : null; + /// /// The BPMN source a workflow definition was imported from, refusing rather than guessing when it is missing or /// no longer trustworthy. See this type's remarks for what "missing" and "stale" mean and why each gets its own diff --git a/test/integration/Elsa.Bpmn.Interchange.IntegrationTests/Assets/top-level-call-activity.bpmn b/test/integration/Elsa.Bpmn.Interchange.IntegrationTests/Assets/top-level-call-activity.bpmn new file mode 100644 index 000000000..4e312c079 --- /dev/null +++ b/test/integration/Elsa.Bpmn.Interchange.IntegrationTests/Assets/top-level-call-activity.bpmn @@ -0,0 +1,31 @@ + + + + + Flow_1 + + + Flow_1 + Flow_2 + + + + + {"typeName":"String","expression":{"type":"Literal","value":"Completed"}} + + + Flow_2 + Flow_3 + + + Flow_3 + + + + + + diff --git a/test/integration/Elsa.Bpmn.Interchange.IntegrationTests/Endpoints/BpmnInterchangeEndpointTests.cs b/test/integration/Elsa.Bpmn.Interchange.IntegrationTests/Endpoints/BpmnInterchangeEndpointTests.cs index f4a231ab1..b59d89864 100644 --- a/test/integration/Elsa.Bpmn.Interchange.IntegrationTests/Endpoints/BpmnInterchangeEndpointTests.cs +++ b/test/integration/Elsa.Bpmn.Interchange.IntegrationTests/Endpoints/BpmnInterchangeEndpointTests.cs @@ -429,6 +429,7 @@ public class BpmnInterchangeEndpointTests(ITestOutputHelper testOutputHelper) : [InlineData("subprocess-boundary-events.bpmn")] [InlineData("transaction-compensation.bpmn")] [InlineData("nested-subprocesses.bpmn")] + [InlineData("top-level-call-activity.bpmn")] public async Task DocumentPut_OfUnchangedContent_ReturnsTheETagTheGetReturned(string assetFileName) { // The nested fixtures prove the subprocess bodies a PUT writes back from the stored document come out diff --git a/test/integration/Elsa.Bpmn.Interchange.IntegrationTests/Scenarios/Interchange/BpmnDocumentRoundTripTests.cs b/test/integration/Elsa.Bpmn.Interchange.IntegrationTests/Scenarios/Interchange/BpmnDocumentRoundTripTests.cs index de8fc406a..0a541668f 100644 --- a/test/integration/Elsa.Bpmn.Interchange.IntegrationTests/Scenarios/Interchange/BpmnDocumentRoundTripTests.cs +++ b/test/integration/Elsa.Bpmn.Interchange.IntegrationTests/Scenarios/Interchange/BpmnDocumentRoundTripTests.cs @@ -38,6 +38,7 @@ public class BpmnDocumentRoundTripTests : BpmnBindingTestBase private static readonly XNamespace Elsa = BpmnXNamespaces.Elsa; private static readonly XNamespace Di = BpmnXNamespaces.Di; private static readonly XNamespace Bpmn = BpmnXNamespaces.Bpmn; + private static readonly XNamespace Vw = BpmnXNamespaces.Vw; public BpmnDocumentRoundTripTests(ITestOutputHelper testOutputHelper) : base(testOutputHelper) { @@ -55,6 +56,7 @@ public class BpmnDocumentRoundTripTests : BpmnBindingTestBase [InlineData("transaction-compensation.bpmn")] [InlineData("nested-subprocesses.bpmn")] [InlineData("camunda-multi-instance-subprocess.bpmn")] + [InlineData("top-level-call-activity.bpmn")] public async Task ReadDocument_ThenImportDocumentAsyncUnchanged_ExportsContentEqualToTheOriginal(string assetFileName) { var stored = await ImportAssetAsync(assetFileName); @@ -100,6 +102,78 @@ public class BpmnDocumentRoundTripTests : BpmnBindingTestBase Assert.Contains("Archiving the order", addedBinding.Descendants(Elsa + "input").Single().Value); } + [Fact(DisplayName = "Posting a document back with an unrelated task's binding edited keeps a top-level call activity's fire-and-forget flag")] + public async Task ImportDocumentAsync_WithAnUnrelatedTasksBindingEdited_KeepsATopLevelCallActivitysFireAndForgetFlag() + { + var stored = await ImportAssetAsync("top-level-call-activity.bpmn"); + + var document = DocumentService.ReadDocument(stored); + var process = Assert.Single(document.Processes); + var logCompletion = process.Elements.Single(element => element.ElementId == "LogCompletion"); + + var newBinding = Format.Write(new WriteLine("Completed, for real this time")); + var editedLogCompletion = WithOverrides(logCompletion, extensions: BpmnActivityBindingFormat.Attach(logCompletion.Extensions, newBinding)); + var editedProcess = process with { Elements = process.Elements.Select(element => element.ElementId == "LogCompletion" ? editedLogCompletion : element).ToList() }; + + var updated = await PutAsync(stored, document with { Processes = [editedProcess] }); + + // The edit shows up... + var editedTask = updated.Descendants(Bpmn + "serviceTask").Single(element => element.Attribute("id")?.Value == "LogCompletion"); + Assert.Contains("Completed, for real this time", editedTask.Descendants(Elsa + "input").Single().Value); + + // ...and the unrelated call activity's fire-and-forget flag, which the document itself never carried, still + // came from its stored work binding rather than a freshly bound, default-waiting one. + var callActivity = updated.Descendants(Bpmn + "callActivity").Single(element => element.Attribute("id")?.Value == "NotifyDownstream"); + Assert.Equal("false", callActivity.Attribute(Vw + "waitForCompletion")?.Value); + Assert.Equal("notification-workflow", callActivity.Attribute("calledElement")?.Value); + } + + [Fact(DisplayName = "Posting a document back with a call activity's calledElement changed does not resurrect the stored fire-and-forget flag")] + public async Task ImportDocumentAsync_WhenACallActivitysCalledElementChanges_DoesNotResurrectTheStoredFireAndForgetFlag() + { + var stored = await ImportAssetAsync("top-level-call-activity.bpmn"); + + var document = DocumentService.ReadDocument(stored); + var process = Assert.Single(document.Processes); + var notifyDownstream = process.Elements.Single(element => element.ElementId == "NotifyDownstream"); + + const string newCalledElement = "a-different-workflow"; + var editedProperties = new Dictionary(notifyDownstream.Properties) { [BpmnXmlReader.CalledElementPropertyKey] = newCalledElement }; + var editedNotifyDownstream = WithOverrides(notifyDownstream, properties: editedProperties); + var editedProcess = process with { Elements = process.Elements.Select(element => element.ElementId == "NotifyDownstream" ? editedNotifyDownstream : element).ToList() }; + + var updated = await PutAsync(stored, document with { Processes = [editedProcess] }); + + // The client's new calledElement is honoured... + var callActivity = updated.Descendants(Bpmn + "callActivity").Single(element => element.Attribute("id")?.Value == "NotifyDownstream"); + Assert.Equal(newCalledElement, callActivity.Attribute("calledElement")?.Value); + + // ...and the call binds fresh rather than inheriting the fire-and-forget flag stored against the process it + // used to call: the client changed what is called, so the options that went with the old call do not survive. + Assert.Null(callActivity.Attribute(Vw + "waitForCompletion")); + } + + [Fact(DisplayName = "Posting a document back with a top-level call activity stripped of its bindingRef does not crash, and the call binds fresh")] + public async Task ImportDocumentAsync_WhenATopLevelCallActivityCarriesNoBindingRef_DoesNotCrashAndBindsFresh() + { + var stored = await ImportAssetAsync("top-level-call-activity.bpmn"); + + var document = DocumentService.ReadDocument(stored); + var process = Assert.Single(document.Processes); + var notifyDownstream = process.Elements.Single(element => element.ElementId == "NotifyDownstream"); + + var editedNotifyDownstream = WithOverrides(notifyDownstream, clearBindingRef: true); + var editedProcess = process with { Elements = process.Elements.Select(element => element.ElementId == "NotifyDownstream" ? editedNotifyDownstream : element).ToList() }; + + // Neither writing the document nor importing it back throws just because there is nothing to hand the stored + // call options over under; the worst outcome is the fresh, waiting default below, not a crashed PUT. + var updated = await PutAsync(stored, document with { Processes = [editedProcess] }); + + var callActivity = updated.Descendants(Bpmn + "callActivity").Single(element => element.Attribute("id")?.Value == "NotifyDownstream"); + Assert.Equal("notification-workflow", callActivity.Attribute("calledElement")?.Value); + Assert.Null(callActivity.Attribute(Vw + "waitForCompletion")); + } + [Fact(DisplayName = "Importing a document against a definition id that does not exist refuses rather than creating one")] public async Task ImportDocumentAsync_WhenTheDefinitionDoesNotExist_ThrowsAndCreatesNothing() { @@ -288,6 +362,35 @@ public class BpmnDocumentRoundTripTests : BpmnBindingTestBase return json.Deserialize(BpmnDocumentJsonOptions.Value)!; } + /// + /// A copy of with or + /// overridden and everything else carried across unchanged. is an immutable plain class, + /// not a record, so it has no with expression of its own. + /// + private static BpmnElement WithOverrides( + BpmnElement element, + BpmnExtensions? extensions = null, + IReadOnlyDictionary? properties = null, + string? bindingRef = null, + bool clearBindingRef = false) => new( + elementId: element.ElementId, + elementType: element.ElementType, + name: element.Name, + bindingRef: clearBindingRef ? null : bindingRef ?? element.BindingRef, + laneId: element.LaneId, + defaultFlowId: element.DefaultFlowId, + eventDefinitions: element.EventDefinitions, + properties: properties ?? element.Properties, + attachedToRef: element.AttachedToRef, + cancelActivity: element.CancelActivity, + loopCharacteristics: element.LoopCharacteristics, + isForCompensation: element.IsForCompensation, + compensationHandlerElementId: element.CompensationHandlerElementId, + isTransaction: element.IsTransaction, + triggeredByEvent: element.TriggeredByEvent, + listenerBindingRef: element.ListenerBindingRef, + extensions: extensions ?? element.Extensions); + private static BpmnDefinitions WithBindingRef(BpmnDefinitions document, string elementId, string? bindingRef) => ThroughTheDocumentEndpoints(document, json => ElementOf(json, elementId)["bindingRef"] = bindingRef); diff --git a/test/integration/Elsa.Bpmn.Interchange.IntegrationTests/Support/BpmnXNamespaces.cs b/test/integration/Elsa.Bpmn.Interchange.IntegrationTests/Support/BpmnXNamespaces.cs index b64828184..2b943e4f3 100644 --- a/test/integration/Elsa.Bpmn.Interchange.IntegrationTests/Support/BpmnXNamespaces.cs +++ b/test/integration/Elsa.Bpmn.Interchange.IntegrationTests/Support/BpmnXNamespaces.cs @@ -13,4 +13,5 @@ internal static class BpmnXNamespaces public static readonly XNamespace Dc = "http://www.omg.org/spec/DD/20100524/DC"; public static readonly XNamespace Di = "http://www.omg.org/spec/DD/20100524/DI"; public static readonly XNamespace Bpmn = "http://www.omg.org/spec/BPMN/20100524/MODEL"; + public static readonly XNamespace Vw = "https://bpmn.valenceworks.io/schema/bpmn"; }