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"; }