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 <noreply@anthropic.com>

* 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 <noreply@anthropic.com>

* refactor(bpmn): filter call activities with Where

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
Sipke Schoorstra 2026-09-12 11:41:50 -07:00 committed by GitHub
parent 933d1739bd
commit 9cce8d91af
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
6 changed files with 243 additions and 8 deletions

View file

@ -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 `<process>` 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.

View file

@ -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.
/// </para>
/// <para>
/// A top-level call activity's call options — currently just <see cref="BpmnWorkBinding.CallProcess.WaitForCompletion"/> —
/// live only on its work binding too, and <paramref name="document"/> cannot carry them either, so they come from the
/// stored source the same way: see <see cref="StoredCallOptionsStillApplicableTo"/>. They are reused only for a call
/// activity whose element id and <c>calledElement</c> are unchanged; a changed <c>calledElement</c> 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.
/// </para>
/// </remarks>
/// <param name="document">The edited document, deserialized through the library's own JSON converters.</param>
/// <param name="definitionId">The workflow definition to update.</param>
@ -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);
}
/// <summary>
/// Reads and parses the BPMN source stored on <paramref name="definition"/>'s <see cref="SourceXmlCustomPropertyKey"/>
/// custom property once, for <see cref="StoredNestedScopesStillDeclaredBy"/> and
/// <see cref="StoredCallOptionsStillApplicableTo"/> to share, rather than each independently re-reading and
/// re-parsing the same stored text. <c>null</c> when the definition carries no stored source at all.
/// </summary>
private BpmnImportResult? ReadStoredSource(WorkflowDefinition definition)
{
if (!definition.CustomProperties.TryGetValue<string>(SourceXmlCustomPropertyKey, out var storedXml) || string.IsNullOrEmpty(storedXml))
return null;
return reader.Read(storedXml, new BpmnImportOptions());
}
/// <summary>
/// The stored work bindings <see cref="BpmnXmlWriter"/> needs to write every nested scope — embedded subprocess,
/// transaction or event subprocess — that <paramref name="document"/> still declares back out exactly as
/// <paramref name="definition"/>'s stored source has it, and nothing for a scope <paramref name="document"/> no
/// <paramref name="stored"/> has it, and nothing for a scope <paramref name="document"/> no
/// longer declares.
/// </summary>
/// <remarks>
@ -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.
/// </exception>
private IReadOnlyList<BpmnWorkBinding> StoredNestedScopesStillDeclaredBy(BpmnDefinitions document, WorkflowDefinition definition)
private IReadOnlyList<BpmnWorkBinding> StoredNestedScopesStillDeclaredBy(BpmnDefinitions document, BpmnImportResult? stored)
{
if (!definition.CustomProperties.TryGetValue<string>(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 } } };
}
/// <summary>
/// The stored <see cref="BpmnWorkBinding.CallProcess"/> options for every top-level call activity
/// <paramref name="document"/> still declares under the same element id and the same <c>calledElement</c>.
/// </summary>
/// <remarks>
/// <para>
/// A call activity's options — currently just <c>vw:waitForCompletion="false"</c> — live only on its
/// <see cref="BpmnWorkBinding.CallProcess"/> work binding, never on the <see cref="BpmnDefinitions"/> document
/// <see cref="ReadDocument"/> returns. <see cref="ImportDocumentAsync"/> 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.
/// </para>
/// <para>
/// Reused only when it is safe to: the element must still exist, under the same id, and still name the same
/// <c>calledElement</c> as the stored binding. A changed <c>calledElement</c> is a call to a different process,
/// whose options this element never had, so it is left to bind fresh rather than inheriting them.
/// </para>
/// <para>
/// Only <see cref="BpmnDefinitions.Processes"/> is scanned — exactly the call activities the document can declare.
/// One nested inside a kept subprocess is never listed there (see <see cref="ReadDocument"/>'s remarks); its
/// options are carried across as part of the whole kept scope by <see cref="StoredNestedScopesStillDeclaredBy"/>
/// instead, along with everything else bound inside it.
/// </para>
/// <para>
/// Matched to the stored binding by element id, then handed over under the bindingRef the posted element carries,
/// for the same reason <see cref="StoredNestedScopesStillDeclaredBy"/> 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.
/// </para>
/// </remarks>
private IReadOnlyList<BpmnWorkBinding> StoredCallOptionsStillApplicableTo(BpmnDefinitions document, BpmnImportResult? stored)
{
if (stored is null)
return [];
var storedCalls = stored.Bindings.OfType<BpmnWorkBinding.CallProcess>().ToList();
var kept = new List<BpmnWorkBinding>();
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;
}
/// <summary>The BPMN <c>calledElement</c> a call activity element carries, kept by the reader for round-trip.</summary>
private static string? CalledElementOf(BpmnElement element) =>
element.Properties.TryGetValue(BpmnXmlReader.CalledElementPropertyKey, out var calledElement) ? calledElement : null;
/// <summary>
/// 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

View file

@ -0,0 +1,31 @@
<?xml version="1.0" encoding="UTF-8"?>
<bpmn:definitions xmlns:bpmn="http://www.omg.org/spec/BPMN/20100524/MODEL"
xmlns:vw="https://bpmn.valenceworks.io/schema/bpmn"
xmlns:elsa="https://elsaworkflows.io/schemas/bpmn/v1"
id="Definitions_top-level-call-activity"
targetNamespace="http://bpmn.io/schema/bpmn">
<bpmn:process id="top-level-call-activity" name="Top-Level Call Activity" isExecutable="true">
<bpmn:startEvent id="Start_1">
<bpmn:outgoing>Flow_1</bpmn:outgoing>
</bpmn:startEvent>
<bpmn:callActivity id="NotifyDownstream" name="Notify Downstream" calledElement="notification-workflow" vw:waitForCompletion="false">
<bpmn:incoming>Flow_1</bpmn:incoming>
<bpmn:outgoing>Flow_2</bpmn:outgoing>
</bpmn:callActivity>
<bpmn:serviceTask id="LogCompletion" name="Log Completion">
<bpmn:extensionElements>
<elsa:activityBinding activityType="Elsa.WriteLine">
<elsa:input name="text">{"typeName":"String","expression":{"type":"Literal","value":"Completed"}}</elsa:input>
</elsa:activityBinding>
</bpmn:extensionElements>
<bpmn:incoming>Flow_2</bpmn:incoming>
<bpmn:outgoing>Flow_3</bpmn:outgoing>
</bpmn:serviceTask>
<bpmn:endEvent id="End_1">
<bpmn:incoming>Flow_3</bpmn:incoming>
</bpmn:endEvent>
<bpmn:sequenceFlow id="Flow_1" sourceRef="Start_1" targetRef="NotifyDownstream" />
<bpmn:sequenceFlow id="Flow_2" sourceRef="NotifyDownstream" targetRef="LogCompletion" />
<bpmn:sequenceFlow id="Flow_3" sourceRef="LogCompletion" targetRef="End_1" />
</bpmn:process>
</bpmn:definitions>

View file

@ -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

View file

@ -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<string, string>(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<BpmnDefinitions>(BpmnDocumentJsonOptions.Value)!;
}
/// <summary>
/// A copy of <paramref name="element"/> with <paramref name="extensions"/> or <paramref name="properties"/>
/// overridden and everything else carried across unchanged. <see cref="BpmnElement"/> is an immutable plain class,
/// not a record, so it has no <c>with</c> expression of its own.
/// </summary>
private static BpmnElement WithOverrides(
BpmnElement element,
BpmnExtensions? extensions = null,
IReadOnlyDictionary<string, string>? 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);

View file

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