From 9f4d33269d56f87c5aaf1692ba24afbc89f52963 Mon Sep 17 00:00:00 2001 From: Sipke Schoorstra Date: Sat, 12 Sep 2026 09:50:43 -0700 Subject: [PATCH] fix(bpmn): keep subprocess bodies through the document PUT (#8077) * fix(bpmn): keep subprocess bodies through the document PUT BpmnDefinitions lists only top-level processes: Bpmn.Interchange carries the body of an embedded subprocess, transaction or event subprocess as the subprocess element's NestedProcess work binding. The document GET therefore never returned the bodies, and the PUT wrote the posted document without any bindings, so BpmnXmlWriter wrote every subprocess empty and the re-import replaced the definition with one whose subprocesses were empty. The PUT now re-reads the stored source and hands the writer the stored body of every subprocess element the posted document still declares, matched by element id, together with everything bound inside it (a call activity's vw:waitForCompletion lives only on its CallProcess binding). A removed subprocess's body is never written back, not even under a new element that reuses its bindingRef, and a subprocess with a stored body but no bindingRef is refused rather than emptied. The GET body, the ETag and the error codes are unchanged; nested content stays uneditable through the document. Bpmn.Interchange 0.2.0 also retains a copy of a subprocess's interpreted multiInstanceLoopCharacteristics inside its body. Handed back verbatim, that copy would override a posted marker on the next read and add another copy on every PUT, so the PUT drops it wherever the element carries a marker in the model. The D4 round-trip test now covers subprocess-boundary-events.bpmn and transaction-compensation.bpmn, copied from Studio's fixtures, plus two nested-scope assets, going through the endpoints' JSON options. Refs #8072 Co-Authored-By: Claude Opus 5 * docs(bpmn): link the loop-marker workaround to its upstream issue Co-Authored-By: Claude Opus 5 --------- Co-authored-by: Claude Opus 5 --- doc/wiki/bpmn-workflows.md | 34 ++- .../BpmnInterchangeDocumentService.cs | 143 ++++++++++- .../camunda-multi-instance-subprocess.bpmn | 57 +++++ .../Assets/nested-subprocesses.bpmn | 53 ++++ .../Assets/subprocess-boundary-events.bpmn | 222 ++++++++++++++++ .../Assets/transaction-compensation.bpmn | 169 +++++++++++++ .../Endpoints/BpmnInterchangeEndpointTests.cs | 35 ++- .../Interchange/BpmnDocumentRoundTripTests.cs | 239 +++++++++++++++--- 8 files changed, 894 insertions(+), 58 deletions(-) create mode 100644 test/integration/Elsa.Bpmn.Interchange.IntegrationTests/Assets/camunda-multi-instance-subprocess.bpmn create mode 100644 test/integration/Elsa.Bpmn.Interchange.IntegrationTests/Assets/nested-subprocesses.bpmn create mode 100644 test/integration/Elsa.Bpmn.Interchange.IntegrationTests/Assets/subprocess-boundary-events.bpmn create mode 100644 test/integration/Elsa.Bpmn.Interchange.IntegrationTests/Assets/transaction-compensation.bpmn diff --git a/doc/wiki/bpmn-workflows.md b/doc/wiki/bpmn-workflows.md index 4fee8e06c..d5d929fcb 100644 --- a/doc/wiki/bpmn-workflows.md +++ b/doc/wiki/bpmn-workflows.md @@ -105,8 +105,8 @@ An exported `.bpmn` is self-contained: all binding configuration, including inpu | `POST bpmn/analyze` | `read:workflow-definitions` | Uploads a single `.bpmn` file (multipart) and returns the Info/Degraded/Dropped findings a read would produce, without persisting anything. | | `POST bpmn/import` | `write:workflow-definitions` | Uploads a single `.bpmn` file and persists it as a new or updated workflow definition (as a draft; it is not published). Optional form fields: `DefinitionId` (update an existing definition instead of creating one), `Name`, `ProcessId` (required when the document declares more than one process). | | `GET bpmn/definitions/{definitionId}/export` | `read:workflow-definitions` | Writes the workflow definition's BPMN source back out as `.bpmn` XML. Optional `VersionOptions` query parameter (`Latest`, `Published`, or a specific version), defaulting to `Latest`. | -| `GET bpmn/definitions/{definitionId}/document` | `read:workflow-definitions` | Reads the workflow definition's stored BPMN source with the `Bpmn.Model`/`Bpmn.Interchange` reader and returns the whole `bpmnDefinitions` document as the library's own JSON (payload format `1.0.0`), rather than as `.bpmn` XML. Same refusals as `Export` when the definition was never imported from BPMN or its stored source is stale. Carries an `ETag` response header for the returned revision — see below. | -| `PUT bpmn/definitions/{definitionId}/document` | `write:workflow-definitions` | Accepts a `bpmnDefinitions` JSON document — the shape `GET` on the same route returns — writes it back out as `.bpmn` XML, and runs it through the same path `Import` runs: analyze, capability check, bind, persist as a new draft, refresh the stored source. Only the activity graph and the `Bpmn:*` custom properties change; the definition's name, description, variables, inputs, outputs, outcomes, options, tool version and any other custom property are carried forward unchanged, unlike `POST bpmn/import`, which stays a whole-definition import (see below). Never edits a published version in place, exactly like `Import`. Returns the same `Id`/`DefinitionId`/`Version`/`Analysis` shape `Import` returns, plus the new `ETag`. Requires an `If-Match` request header — see below. | +| `GET bpmn/definitions/{definitionId}/document` | `read:workflow-definitions` | Reads the workflow definition's stored BPMN source with the `Bpmn.Model`/`Bpmn.Interchange` reader and returns the whole `bpmnDefinitions` document as the library's own JSON (payload format `1.0.0`), rather than as `.bpmn` XML. That document declares every subprocess but not what is inside one — see *Nested scopes* below. Same refusals as `Export` when the definition was never imported from BPMN or its stored source is stale. Carries an `ETag` response header for the returned revision — see below. | +| `PUT bpmn/definitions/{definitionId}/document` | `write:workflow-definitions` | Accepts a `bpmnDefinitions` JSON document — the shape `GET` on the same route returns — writes it back out as `.bpmn` XML, with every subprocess it still declares keeping the body stored for it (see *Nested scopes* below), and runs it through the same path `Import` runs: analyze, capability check, bind, persist as a new draft, refresh the stored source. Only the activity graph and the `Bpmn:*` custom properties change; the definition's name, description, variables, inputs, outputs, outcomes, options, tool version and any other custom property are carried forward unchanged, unlike `POST bpmn/import`, which stays a whole-definition import (see below). Never edits a published version in place, exactly like `Import`. Returns the same `Id`/`DefinitionId`/`Version`/`Analysis` shape `Import` returns, plus the new `ETag`. Requires an `If-Match` request header — see below. | Both `Analyze` and `Import` require exactly one uploaded file; zero or more than one returns `400 Bad Request`. @@ -154,6 +154,31 @@ integer — **not** Elsa's own API-wide JSON conventions (which add a string-enu through). A client reading or writing this JSON should use a plain `System.Text.Json` serializer with default options, not whatever conventions the rest of the Elsa API uses. +**Nested scopes.** `bpmnDefinitions` lists only top-level processes. The body of an embedded subprocess, a +transaction or an event subprocess — its elements, flows and `elsa:` bindings, and any subprocess nested further in — +is not part of that document: the library carries it as the subprocess element's work binding instead, so `GET` +returns each subprocess element with nothing inside it. `PUT` does not take the body from the posted document; it +restores it from the definition's stored BPMN source: + +- Every subprocess element the posted document still declares, matched by element id, is written back with the body + stored for it, exactly as stored — including anything the stored source carries only in a work binding, such as a + call activity's `vw:waitForCompletion="false"`. +- A subprocess element the posted document no longer declares, or has turned into another kind of element, takes its + stored body with it; nothing of that body is written back, not even under a new element that reuses its + `bindingRef`. A subprocess element with no stored body — one this edit adds — is written as posted, empty. +- The subprocess element itself is the posted one: its name, flags, boundary events and multi-instance marker come + from the document, and a changed or removed marker is written as posted. (`Bpmn.Interchange` 0.2.0 also keeps a + copy of an interpreted marker inside the body it reads; `PUT` drops that copy so it cannot override the posted + marker or pile up on every write. `Export` still writes that copy alongside the element's own marker.) +- A subprocess element that has a stored body but no `bindingRef` is refused with `400 Bad Request` before anything + is persisted: the writer attaches a body through the element's `bindingRef`, so writing it would silently empty + the subprocess. Send each element back with the `bindingRef` `GET` returned. + +Nothing inside a nested scope can therefore be edited through the document endpoints, only its subprocess element +and its layout: the document's BPMN DI carries the shapes of nested elements too, and `PUT` writes them as posted. A +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 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. @@ -240,8 +265,9 @@ FastEndpoints' process-wide `Config.ErrOpts.ResponseBuilder`, which would have r response, not just these — see `Elsa.Bpmn.Interchange.Endpoints.Bpmn.BpmnErrorResponse`'s remarks. Not every 4xx these endpoints send carries a code: a plain `404 Not Found` for a `definitionId` that does not exist, -a `400 Bad Request` from malformed JSON or an unparseable `VersionOptions`, and a `400 Bad Request` for a document -that names more than one process without saying which, are unchanged and uncoded. +a `400 Bad Request` from malformed JSON or an unparseable `VersionOptions`, a `400 Bad Request` for a document +that names more than one process without saying which, and a document `PUT`'s `400 Bad Request` for a subprocess +element that has a stored body but no `bindingRef`, are uncoded. | Code (`Elsa.Bpmn.Interchange.BpmnErrorCodes`) | Sent by | Status | `data` | | --- | --- | --- | --- | diff --git a/src/modules/Elsa.Bpmn.Interchange/Services/BpmnInterchangeDocumentService.cs b/src/modules/Elsa.Bpmn.Interchange/Services/BpmnInterchangeDocumentService.cs index fe6013a09..44590a4ee 100644 --- a/src/modules/Elsa.Bpmn.Interchange/Services/BpmnInterchangeDocumentService.cs +++ b/src/modules/Elsa.Bpmn.Interchange/Services/BpmnInterchangeDocumentService.cs @@ -310,6 +310,12 @@ public sealed class BpmnInterchangeDocumentService( /// retained them. Refuses rather than guessing when that source is missing or no longer trustworthy; see this /// type's remarks for what "missing" and "stale" mean. /// + /// + /// lists only top-level processes, so the returned document declares every + /// subprocess element but carries none of their bodies: the library hands those out as work bindings, not as + /// part of the document. restores them from the stored source rather than from + /// the document, which is also why nothing inside a nested scope can be edited through the document. + /// /// The workflow definition to read, as read from the store. /// /// The definition does not currently carry BPMN source, or it does but the definition has changed since the @@ -336,6 +342,12 @@ public sealed class BpmnInterchangeDocumentService( /// import logic's preserveMetadataFrom parameter, which this passes the existing definition to. Only the activity graph /// and the // /// / custom properties move. + /// + /// Nested scopes come from the stored source, not from , which cannot carry them (see + /// ): every subprocess element still declares is written back + /// 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. + /// /// /// The edited document, deserialized through the library's own JSON converters. /// The workflow definition to update. @@ -351,7 +363,10 @@ public sealed class BpmnInterchangeDocumentService( /// whole-definition import path, which would silently create a definition under /// with reset metadata instead of reporting that this PUT's target disappeared. /// - /// The document declares more than one process and does not pick one. + /// + /// The document declares more than one process and does not pick one, or it declares a + /// subprocess element that has a stored body but no bindingRef to write that body back under. + /// /// The document needs a host capability this deployment does not declare. /// A work binding cannot be turned into an Elsa activity. public async Task ImportDocumentAsync(BpmnDefinitions document, string definitionId, string? processId, CancellationToken cancellationToken) @@ -365,10 +380,134 @@ public sealed class BpmnInterchangeDocumentService( $"Workflow definition '{definitionId}' does not exist, so its BPMN document cannot be edited."); } - var xml = writer.Write(document); + var xml = writer.Write(document, StoredNestedScopesStillDeclaredBy(document, existingDefinition)); return await ImportCoreAsync(xml, definitionId, name: null, processId, preserveMetadataFrom: existingDefinition, cancellationToken); } + /// + /// 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 + /// longer declares. + /// + /// + /// + /// lists only top-level processes. The body of a nested scope is not on its + /// subprocess element: the reader hands it out as that element's + /// binding, and writes a subprocess whose binding it is not given as empty. The + /// document returns therefore never carries a nested body, so a document written back + /// without these bindings would silently replace every subprocess with an empty one. The stored source is the + /// only place the bodies still exist, so they are re-read from it here. + /// + /// + /// A stored body is matched to a subprocess element of the posted document by element id, which BPMN makes + /// unique across the whole document. It is never matched by bindingRef, so a subprocess element the posted + /// document removed matches nothing and its stored body is never written back, and a new element that reuses a + /// removed one's bindingRef does not inherit its body. The writer looks a body up by the element's own + /// bindingRef, so a kept body is handed over under the bindingRef the posted element carries. + /// + /// + /// Everything bound inside a kept scope comes along with it, not just the bodies of scopes nested further in: + /// a call activity's vw:waitForCompletion="false", for one, lives only on its + /// binding. Nothing inside a kept scope can differ from what is stored, + /// because the document the client edited never carried it, so the stored bindings describe it exactly. + /// + /// + /// One thing the stored body holds is not its own: Bpmn.Interchange 0.2.0 reads a subprocess's + /// multiInstanceLoopCharacteristics onto the subprocess element, which is what the writer emits it from and, + /// at the top level, what the client edits, but also retains a copy as foreign content of the nested process the + /// element opens. Handed back, that copy would come first on the next read, overriding a marker the client changed + /// or removed, and every write would add another. So it is dropped from a kept body whenever the element carries a + /// marker in the model, as stored or as posted, and kept only as the sole record of one the reader could not + /// interpret. This is tracked upstream as valence-works/bpmn#21; + /// remove this workaround once a Bpmn.Interchange release containing that fix is adopted. + /// + /// + /// A definition without stored source has no body to keep; its subprocesses are written as the document declares + /// them. The document PUT reaches that case only if the source disappears between its If-Match check + /// and this read: the precondition only matches a stored state a successful GET or PUT described, + /// and both need the source. + /// + /// + /// + /// 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) + { + if (!definition.CustomProperties.TryGetValue(SourceXmlCustomPropertyKey, out var storedXml) || string.IsNullOrEmpty(storedXml)) + 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. + var loopingElementIds = document.Processes + .Concat(stored.Definitions.Processes) + .Concat(storedBindings.OfType().Select(nested => nested.Definition)) + .SelectMany(process => process.Elements) + .Where(element => element.LoopCharacteristics is not null) + .Select(element => element.ElementId) + .ToHashSet(StringComparer.Ordinal); + + var kept = new List(); + + foreach (var process in document.Processes) + { + foreach (var subprocess in process.Elements.Where(element => element.ElementType == BpmnElementTypes.SubProcess)) + { + // Last match wins, as it does inside the writer itself, should a malformed document repeat an id. + var body = storedBindings.OfType().LastOrDefault(nested => nested.ElementId == subprocess.ElementId); + + if (body is null) + continue; + + if (subprocess.BindingRef is null) + { + throw new BpmnInterchangeException( + $"Subprocess element '{subprocess.ElementId}' of process '{process.ProcessId}' carries no bindingRef, so the body stored for it cannot be written back with it. " + + "The document does not carry a subprocess's body, so writing it without one would silently empty the subprocess. Send the element with the bindingRef the document GET returned."); + } + + kept.Add(HandOver(body) with { BindingRef = subprocess.BindingRef }); + KeepEverythingBoundInside(body.ElementId); + } + } + + return kept; + + // A nested process's bindings name the subprocess element's id as their process (see BpmnWorkBinding.ProcessId). + void KeepEverythingBoundInside(string scopeId) + { + foreach (var binding in storedBindings.Where(binding => binding.ProcessId == scopeId)) + { + if (binding is not BpmnWorkBinding.NestedProcess nested) + { + kept.Add(binding); + continue; + } + + kept.Add(HandOver(nested)); + KeepEverythingBoundInside(nested.ElementId); + } + } + + BpmnWorkBinding.NestedProcess HandOver(BpmnWorkBinding.NestedProcess nested) => + loopingElementIds.Contains(nested.ElementId) ? WithoutRetainedLoopMarker(nested) : nested; + } + + /// + /// without the copy of its subprocess element's multi-instance marker the reader also + /// retained on it; see 's remarks. + /// + private static BpmnWorkBinding.NestedProcess WithoutRetainedLoopMarker(BpmnWorkBinding.NestedProcess nested) + { + var marker = new BpmnQName(BpmnXmlNames.Model.NamespaceName, "multiInstanceLoopCharacteristics"); + var extensions = nested.Definition.Extensions; + var foreignChildren = extensions.ForeignChildren.Where(child => child.Element.Name != marker).ToList(); + return nested with { Definition = nested.Definition with { Extensions = extensions with { ForeignChildren = foreignChildren } } }; + } + /// /// 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/camunda-multi-instance-subprocess.bpmn b/test/integration/Elsa.Bpmn.Interchange.IntegrationTests/Assets/camunda-multi-instance-subprocess.bpmn new file mode 100644 index 000000000..380b0c0a2 --- /dev/null +++ b/test/integration/Elsa.Bpmn.Interchange.IntegrationTests/Assets/camunda-multi-instance-subprocess.bpmn @@ -0,0 +1,57 @@ + + + + + Flow_1 + + + + Flow_1 + Flow_2 + + + Orders_Flow_1 + + + + Orders_Flow_1 + Orders_Flow_2 + + + Lines_Flow_1 + + + + + {"typeName":"String","expression":{"type":"Literal","value":"ship"}} + + + Lines_Flow_1 + Lines_Flow_2 + + + Lines_Flow_2 + + + + + + + Orders_Flow_2 + + + + + + + Flow_2 + + + + + + diff --git a/test/integration/Elsa.Bpmn.Interchange.IntegrationTests/Assets/nested-subprocesses.bpmn b/test/integration/Elsa.Bpmn.Interchange.IntegrationTests/Assets/nested-subprocesses.bpmn new file mode 100644 index 000000000..0a638e213 --- /dev/null +++ b/test/integration/Elsa.Bpmn.Interchange.IntegrationTests/Assets/nested-subprocesses.bpmn @@ -0,0 +1,53 @@ + + + + + Flow_1 + + + + + + + Flow_1 + Flow_2 + + Outer_Flow_1 + + + + Outer_Flow_1 + Outer_Flow_2 + + + Inner_Flow_1 + + + Inner_Flow_1 + Inner_Flow_2 + + + Inner_Flow_2 + + + + + + + Outer_Flow_2 + + + + + + + Flow_2 + + + + + + diff --git a/test/integration/Elsa.Bpmn.Interchange.IntegrationTests/Assets/subprocess-boundary-events.bpmn b/test/integration/Elsa.Bpmn.Interchange.IntegrationTests/Assets/subprocess-boundary-events.bpmn new file mode 100644 index 000000000..3acbcfd83 --- /dev/null +++ b/test/integration/Elsa.Bpmn.Interchange.IntegrationTests/Assets/subprocess-boundary-events.bpmn @@ -0,0 +1,222 @@ + + + + + + + + + + + + + Flow_1 + + + + Flow_1 + Flow_2 + + + Sub_Flow_1 + + + + + {"typeName":"String","expression":{"type":"Literal","value":"pick"}} + + + Sub_Flow_1 + Sub_Flow_2 + + + Sub_Flow_2 + + + + + + + Flow_Timeout + + PT1H + + + + + Flow_Nudge + + + + + Flow_Failed + + + + + + + {"typeName":"String","expression":{"type":"Literal","value":"escalate"}} + + + Flow_Timeout + Flow_Nudge + Flow_Failed + Flow_3 + + + + Flow_3 + Flow_4 + + + + + Flow_4 + + + + Flow_2 + + + + + + + Recall_Flow_1 + + + + + {"typeName":"String","expression":{"type":"Literal","value":"recall"}} + + + Recall_Flow_1 + Recall_Flow_2 + + + Recall_Flow_2 + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + diff --git a/test/integration/Elsa.Bpmn.Interchange.IntegrationTests/Assets/transaction-compensation.bpmn b/test/integration/Elsa.Bpmn.Interchange.IntegrationTests/Assets/transaction-compensation.bpmn new file mode 100644 index 000000000..475e54362 --- /dev/null +++ b/test/integration/Elsa.Bpmn.Interchange.IntegrationTests/Assets/transaction-compensation.bpmn @@ -0,0 +1,169 @@ + + + + + Flow_1 + + + + Flow_1 + Flow_2 + + Tx_Flow_1 + + + + + {"typeName":"String","expression":{"type":"Literal","value":"charge"}} + + + Tx_Flow_1 + Tx_Flow_2 + + + + + + + + {"typeName":"String","expression":{"type":"Literal","value":"refund"}} + + + + + + Tx_Flow_3 + + + + Tx_Flow_2 + Tx_Flow_3 + Tx_Flow_4 + + + Tx_Flow_4 + + + + + + + + + Flow_Cancelled + + + + + + + {"typeName":"String","expression":{"type":"Literal","value":"notify"}} + + + Flow_Cancelled + Flow_3 + + + + Flow_3 + + + + Flow_2 + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + diff --git a/test/integration/Elsa.Bpmn.Interchange.IntegrationTests/Endpoints/BpmnInterchangeEndpointTests.cs b/test/integration/Elsa.Bpmn.Interchange.IntegrationTests/Endpoints/BpmnInterchangeEndpointTests.cs index 6ffd2a056..20ad2f823 100644 --- a/test/integration/Elsa.Bpmn.Interchange.IntegrationTests/Endpoints/BpmnInterchangeEndpointTests.cs +++ b/test/integration/Elsa.Bpmn.Interchange.IntegrationTests/Endpoints/BpmnInterchangeEndpointTests.cs @@ -350,7 +350,7 @@ public class BpmnInterchangeEndpointTests(ITestOutputHelper testOutputHelper) : [Fact] public async Task DocumentPut_WithTheCurrentIfMatch_ReturnsOkWithANewETagWhenTheContentChanged() { - var definitionId = await ImportCamundaOrderProcessWrittenBackAsync(); + var definitionId = await ImportWrittenBackAsync("camunda-order-process.bpmn"); var (currentETag, documentJson) = await GetDocumentAsync(definitionId); var putResponse = await PutDocumentAsync(definitionId, WithFirstShapeMoved(documentJson), currentETag); @@ -363,10 +363,17 @@ public class BpmnInterchangeEndpointTests(ITestOutputHelper testOutputHelper) : Assert.Equal(newETag, (await GetDocumentAsync(definitionId)).ETag); } - [Fact] - public async Task DocumentPut_OfUnchangedContent_ReturnsTheETagTheGetReturned() + [Theory] + [InlineData("camunda-order-process.bpmn")] + [InlineData("subprocess-boundary-events.bpmn")] + [InlineData("transaction-compensation.bpmn")] + [InlineData("nested-subprocesses.bpmn")] + public async Task DocumentPut_OfUnchangedContent_ReturnsTheETagTheGetReturned(string assetFileName) { - var definitionId = await ImportCamundaOrderProcessWrittenBackAsync(); + // The nested fixtures prove the subprocess bodies a PUT writes back from the stored document come out + // byte-identical every time, including a multi-instance subprocess, top-level or nested, whose marker the library + // also retains in the body and would otherwise write one more copy of on every PUT. + var definitionId = await ImportWrittenBackAsync(assetFileName); var (currentETag, documentJson) = await GetDocumentAsync(definitionId); var putResponse = await PutDocumentAsync(definitionId, documentJson, currentETag); @@ -379,7 +386,7 @@ public class BpmnInterchangeEndpointTests(ITestOutputHelper testOutputHelper) : [Fact] public async Task DocumentPut_AfterAnInterveningDocumentPut_ReturnsPreconditionFailedAndOverwritesNothing() { - var definitionId = await ImportCamundaOrderProcessWrittenBackAsync(); + var definitionId = await ImportWrittenBackAsync("camunda-order-process.bpmn"); var (staleETag, documentJson) = await GetDocumentAsync(definitionId); var storedBeforeInterveningPut = await LatestStoredAsync(definitionId); @@ -856,10 +863,13 @@ public class BpmnInterchangeEndpointTests(ITestOutputHelper testOutputHelper) : } /// Imports camunda-order-process.bpmn through the real endpoint and returns the resulting definitionId. - private async Task ImportCamundaOrderProcessAsync() + private Task ImportCamundaOrderProcessAsync() => ImportAssetAsync("camunda-order-process.bpmn"); + + /// Imports the fixture through the real endpoint and returns the resulting definitionId. + private async Task ImportAssetAsync(string assetFileName) { using var content = new MultipartFormDataContent(); - AddBpmnFile(content, ReadAsset("camunda-order-process.bpmn"), "file"); + AddBpmnFile(content, ReadAsset(assetFileName), "file"); var response = await PostAuthenticatedAsync("bpmn/import", content, "workflows/definitions:write"); Assert.Equal(HttpStatusCode.OK, response.StatusCode); @@ -868,13 +878,14 @@ public class BpmnInterchangeEndpointTests(ITestOutputHelper testOutputHelper) : } /// - /// Imports camunda-order-process.bpmn and writes its document straight back once through the document PUT, so - /// what is stored is the writer's own rendering of it rather than the uploaded bytes. From there only an actual edit - /// changes the stored content, which is what lets a test attribute an ETag change — or its absence — to one write. + /// Imports the fixture and writes its document straight back once through the + /// document PUT, so what is stored is the writer's own rendering of it rather than the uploaded bytes. From there only + /// an actual edit changes the stored content, which is what lets a test attribute an ETag change — or its absence — to + /// one write. /// - private async Task ImportCamundaOrderProcessWrittenBackAsync() + private async Task ImportWrittenBackAsync(string assetFileName) { - var definitionId = await ImportCamundaOrderProcessAsync(); + var definitionId = await ImportAssetAsync(assetFileName); var (etag, documentJson) = await GetDocumentAsync(definitionId); var response = await PutDocumentAsync(definitionId, documentJson, etag); Assert.Equal(HttpStatusCode.OK, response.StatusCode); 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 a27e2beaa..de8fc406a 100644 --- a/test/integration/Elsa.Bpmn.Interchange.IntegrationTests/Scenarios/Interchange/BpmnDocumentRoundTripTests.cs +++ b/test/integration/Elsa.Bpmn.Interchange.IntegrationTests/Scenarios/Interchange/BpmnDocumentRoundTripTests.cs @@ -1,7 +1,11 @@ using System.Text; +using System.Text.Json; +using System.Text.Json.Nodes; using System.Xml.Linq; +using Bpmn.Interchange; using Bpmn.Model; using Elsa.Bpmn.Interchange.Binding; +using Elsa.Bpmn.Interchange.Endpoints.Bpmn; using Elsa.Bpmn.Interchange.Exceptions; using Elsa.Bpmn.Interchange.IntegrationTests.Scenarios.Binding; using Elsa.Bpmn.Interchange.IntegrationTests.Support; @@ -20,7 +24,8 @@ namespace Elsa.Bpmn.Interchange.IntegrationTests.Scenarios.Interchange; /// /// The round trip W21 exists for: and /// — the document endpoints' shared service path — -/// let a document round-trip through JSON without losing what Export already proves survives XML alone. +/// let a document round-trip through JSON without losing what Export already proves survives XML alone, +/// including the bodies of nested scopes, which the JSON document cannot carry and the stored source supplies. /// /// /// Derives from , not , because building the @@ -44,39 +49,34 @@ public class BpmnDocumentRoundTripTests : BpmnBindingTestBase private IWorkflowDefinitionStore DefinitionStore { get; } - [Fact(DisplayName = "Reading a document then posting it back unchanged exports content-equal to the original")] - public async Task ReadDocument_ThenImportDocumentAsyncUnchanged_ExportsContentEqualToTheOriginal() + [Theory(DisplayName = "Reading a document then posting it back unchanged exports content-equal to the original, nested scopes included")] + [InlineData("camunda-order-process.bpmn")] + [InlineData("subprocess-boundary-events.bpmn")] + [InlineData("transaction-compensation.bpmn")] + [InlineData("nested-subprocesses.bpmn")] + [InlineData("camunda-multi-instance-subprocess.bpmn")] + public async Task ReadDocument_ThenImportDocumentAsyncUnchanged_ExportsContentEqualToTheOriginal(string assetFileName) { - var xml = ReadAsset("camunda-order-process.bpmn"); - var imported = await DocumentService.ImportAsync(xml, definitionId: null, name: null, processId: null, CancellationToken.None); - Assert.True(imported.ImportResult.Succeeded, string.Join("; ", imported.ImportResult.ValidationErrors.Select(error => error.Message))); + var stored = await ImportAssetAsync(assetFileName); + var originalExport = ExportOf(stored); - var definitionId = imported.ImportResult.WorkflowDefinition.DefinitionId; - var beforeStored = await FindLatestAsync(definitionId); - var originalExportedXml = Encoding.UTF8.GetString(DocumentService.Export(beforeStored)); + var roundTripped = await PutAsync(stored, ThroughTheDocumentEndpoints(DocumentService.ReadDocument(stored))); - var document = DocumentService.ReadDocument(beforeStored); - var processId = ProcessIdOf(beforeStored); + // Every element the fixture nests inside a subprocess or transaction is still there, in the same scope, so the + // comparison below cannot pass by comparing two equally emptied documents... + Assert.Equal(NestedElementIdsOf(XDocument.Parse(ReadAsset(assetFileName))), NestedElementIdsOf(roundTripped)); - var putResult = await DocumentService.ImportDocumentAsync(document, definitionId, processId, CancellationToken.None); - Assert.True(putResult.ImportResult.Succeeded, string.Join("; ", putResult.ImportResult.ValidationErrors.Select(error => error.Message))); - - var afterStored = await FindLatestAsync(definitionId); - var roundTrippedExportedXml = Encoding.UTF8.GetString(DocumentService.Export(afterStored)); - - AssertContentEquivalent(XDocument.Parse(originalExportedXml), XDocument.Parse(roundTrippedExportedXml)); + // ...and nothing else changed either: retained extensions, elsa: bindings, a fire-and-forget call's + // vw:waitForCompletion (carried only by its work binding), a multi-instance marker the reader could not interpret + // (kept only as retained content) and BPMN DI included. + Assert.Equal(ComparableContentOf(originalExport.Root!), ComparableContentOf(roundTripped.Root!)); } [Fact(DisplayName = "Posting a document back with a new bound task added shows the binding on export and leaves everything else unchanged")] public async Task ImportDocumentAsync_WithANewBoundTaskAdded_ExportsTheAdditionAndLeavesTheRestUnchanged() { - var xml = ReadAsset("camunda-order-process.bpmn"); - var imported = await DocumentService.ImportAsync(xml, definitionId: null, name: null, processId: null, CancellationToken.None); - Assert.True(imported.ImportResult.Succeeded, string.Join("; ", imported.ImportResult.ValidationErrors.Select(error => error.Message))); - - var definitionId = imported.ImportResult.WorkflowDefinition.DefinitionId; - var stored = await FindLatestAsync(definitionId); - var originalExportedXml = Encoding.UTF8.GetString(DocumentService.Export(stored)); + var stored = await ImportAssetAsync("camunda-order-process.bpmn"); + var originalDocument = ExportOf(stored); var document = DocumentService.ReadDocument(stored); var process = Assert.Single(document.Processes); @@ -88,16 +88,7 @@ public class BpmnDocumentRoundTripTests : BpmnBindingTestBase var editedProcess = process with { Elements = process.Elements.Append(newTask).ToList() }; var editedDocument = document with { Processes = [editedProcess] }; - var processId = ProcessIdOf(stored); - - var putResult = await DocumentService.ImportDocumentAsync(editedDocument, definitionId, processId, CancellationToken.None); - Assert.True(putResult.ImportResult.Succeeded, string.Join("; ", putResult.ImportResult.ValidationErrors.Select(error => error.Message))); - - var updated = await FindLatestAsync(definitionId); - var updatedExportedXml = Encoding.UTF8.GetString(DocumentService.Export(updated)); - - var originalDocument = XDocument.Parse(originalExportedXml); - var updatedDocument = XDocument.Parse(updatedExportedXml); + var updatedDocument = await PutAsync(stored, editedDocument); // Everything that was there before the edit is still there, unchanged. AssertContentEquivalent(originalDocument, updatedDocument); @@ -112,11 +103,7 @@ public class BpmnDocumentRoundTripTests : BpmnBindingTestBase [Fact(DisplayName = "Importing a document against a definition id that does not exist refuses rather than creating one")] public async Task ImportDocumentAsync_WhenTheDefinitionDoesNotExist_ThrowsAndCreatesNothing() { - var xml = ReadAsset("camunda-order-process.bpmn"); - var imported = await DocumentService.ImportAsync(xml, definitionId: null, name: null, processId: null, CancellationToken.None); - Assert.True(imported.ImportResult.Succeeded, string.Join("; ", imported.ImportResult.ValidationErrors.Select(error => error.Message))); - - var stored = await FindLatestAsync(imported.ImportResult.WorkflowDefinition.DefinitionId); + var stored = await ImportAssetAsync("camunda-order-process.bpmn"); var document = DocumentService.ReadDocument(stored); var missingDefinitionId = $"{Guid.NewGuid()}-does-not-exist"; @@ -129,6 +116,95 @@ public class BpmnDocumentRoundTripTests : BpmnBindingTestBase Assert.Null(afterAttempt); } + [Fact(DisplayName = "Posting a document back without a subprocess drops its stored body, even under a new subprocess reusing its bindingRef, and keeps every other body")] + public async Task ImportDocumentAsync_WithASubprocessRemoved_DropsItsStoredBodyAndKeepsTheOthers() + { + // Replaces the event subprocess with a new, empty embedded subprocess that took over its bindingRef: what a + // client cloning the element's JSON and changing its id posts. + var updated = await PutWithOnRecallEditedAsync(json => + { + var onRecall = ElementOf(json, "OnRecall"); + var elements = onRecall.Parent!.AsArray(); + elements.Remove(onRecall); + elements.Add(new JsonObject { ["elementId"] = "Archive", ["elementType"] = BpmnElementTypes.SubProcess, ["bindingRef"] = onRecall["bindingRef"]!.DeepClone() }); + }); + + Assert.DoesNotContain(NestedScopesOf(updated), scope => scope.Attribute("id")?.Value == "OnRecall"); + Assert.DoesNotContain(ScopeOf(updated, "Archive").Elements(), child => child.Attribute("id") is not null); + } + + [Fact(DisplayName = "Posting a document back with a subprocess turned into another kind of element drops its stored body")] + public async Task ImportDocumentAsync_WithASubprocessTurnedIntoAnotherKindOfElement_DropsItsStoredBody() + { + var updated = await PutWithOnRecallEditedAsync(json => + ElementOf(json, "OnRecall").ReplaceWith(new JsonObject { ["elementId"] = "OnRecall", ["elementType"] = BpmnElementTypes.EndEvent })); + + Assert.Single(updated.Descendants(Bpmn + "endEvent"), element => element.Attribute("id")?.Value == "OnRecall"); + } + + [Theory(DisplayName = "Posting a document back with a kept subprocess's multi-instance marker changed, removed or added writes exactly the posted marker")] + [InlineData("subprocess-boundary-events.bpmn", "Fulfil", """{"isSequential":false,"cardinality":null,"collectionVariable":"orderLines","itemVariable":"lineItem"}""")] + [InlineData("subprocess-boundary-events.bpmn", "Fulfil", null)] + [InlineData("camunda-multi-instance-subprocess.bpmn", "ShipOrders", """{"isSequential":true,"cardinality":3,"collectionVariable":null,"itemVariable":"item"}""")] + public async Task ImportDocumentAsync_WithAKeptSubprocessLoopMarkerEdited_WritesExactlyThePostedMarker(string assetFileName, string subprocessId, string? loopCharacteristics) + { + var stored = await ImportAssetAsync(assetFileName); + var document = ThroughTheDocumentEndpoints(DocumentService.ReadDocument(stored), json => + ElementOf(json, subprocessId)["loopCharacteristics"] = loopCharacteristics is null ? null : JsonNode.Parse(loopCharacteristics)); + + await PutAsync(stored, document); + + // What the next GET hands back is the posted marker, not the copy of the stored one the library also keeps in + // the subprocess's body, which would otherwise be written back ahead of it and win the next read. + var reread = DocumentService.ReadDocument(await FindLatestAsync(stored.DefinitionId)).Processes.Single().Elements.Single(element => element.ElementId == subprocessId); + Assert.Equal(loopCharacteristics ?? "null", JsonSerializer.Serialize(reread.LoopCharacteristics, BpmnDocumentJsonOptions.Value)); + } + + [Fact(DisplayName = "Posting a document back with a kept subprocess under another bindingRef still writes its stored body")] + public async Task ImportDocumentAsync_WhenAKeptSubprocessCarriesAnotherBindingRef_StillWritesItsStoredBody() + { + var stored = await ImportAssetAsync("transaction-compensation.bpmn"); + var originalExport = ExportOf(stored); + + var updated = await PutAsync(stored, WithBindingRef(DocumentService.ReadDocument(stored), "BookTrip", "renamed-by-the-client")); + + Assert.Equal(ComparableContentOf(ScopeOf(originalExport, "BookTrip")), ComparableContentOf(ScopeOf(updated, "BookTrip"))); + } + + [Fact(DisplayName = "Posting a document back with a kept subprocess stripped of its bindingRef refuses rather than emptying it")] + public async Task ImportDocumentAsync_WhenAKeptSubprocessCarriesNoBindingRef_RefusesAndPersistsNothing() + { + var stored = await ImportAssetAsync("transaction-compensation.bpmn"); + var storedBefore = (SourceXmlOf(stored), stored.StringData); + var document = WithBindingRef(DocumentService.ReadDocument(stored), "BookTrip", null); + + var exception = await Assert.ThrowsAsync(() => + DocumentService.ImportDocumentAsync(document, stored.DefinitionId, ProcessIdOf(stored), CancellationToken.None)); + + Assert.Contains("'BookTrip'", exception.Message); + var afterAttempt = await FindLatestAsync(stored.DefinitionId); + Assert.Equal(storedBefore, (SourceXmlOf(afterAttempt), afterAttempt.StringData)); + } + + /// + /// Imports subprocess-boundary-events.bpmn, posts its document back with applied to the + /// JSON, and asserts that nothing the OnRecall event subprocess's stored body held is written back anywhere + /// while the Fulfil subprocess the edit left alone keeps its body exactly as stored. Returns the export. + /// + private async Task PutWithOnRecallEditedAsync(Action edit) + { + var stored = await ImportAssetAsync("subprocess-boundary-events.bpmn"); + var originalExport = ExportOf(stored); + var onRecallBodyIds = ScopeOf(originalExport, "OnRecall").Descendants().Select(element => element.Attribute("id")?.Value).OfType().ToList(); + Assert.Contains("HandleRecall", onRecallBodyIds); + + var updated = await PutAsync(stored, ThroughTheDocumentEndpoints(DocumentService.ReadDocument(stored), edit)); + + Assert.DoesNotContain(updated.Descendants(Bpmn + "process").Single().Descendants(), element => element.Attribute("id")?.Value is { } id && onRecallBodyIds.Contains(id)); + Assert.Equal(ComparableContentOf(ScopeOf(originalExport, "Fulfil")), ComparableContentOf(ScopeOf(updated, "Fulfil"))); + return updated; + } + /// Everything camunda-order-process.bpmn carries that an edit must not disturb: foreign attributes, foreign extension elements, the elsa: binding on the untouched task, and BPMN DI waypoints. private static void AssertContentEquivalent(XDocument expected, XDocument actual) { @@ -172,6 +248,9 @@ public class BpmnDocumentRoundTripTests : BpmnBindingTestBase private static string? ProcessIdOf(WorkflowDefinition definition) => definition.CustomProperties.TryGetValue(BpmnInterchangeDocumentService.SourceProcessIdCustomPropertyKey, out var processId) ? processId : null; + private static string? SourceXmlOf(WorkflowDefinition definition) => + definition.CustomProperties.TryGetValue(BpmnInterchangeDocumentService.SourceXmlCustomPropertyKey, out var xml) ? xml : null; + private async Task FindLatestAsync(string definitionId) { var filter = WorkflowDefinitionHandle.ByDefinitionId(definitionId, VersionOptions.Latest).ToFilter(); @@ -180,6 +259,86 @@ public class BpmnDocumentRoundTripTests : BpmnBindingTestBase return definition!; } + /// Imports a fixture as a new definition, the way Import does, and returns it as stored. + private async Task ImportAssetAsync(string assetFileName) + { + var imported = await DocumentService.ImportAsync(ReadAsset(assetFileName), definitionId: null, name: null, processId: null, CancellationToken.None); + Assert.True(imported.ImportResult.Succeeded, string.Join("; ", imported.ImportResult.ValidationErrors.Select(error => error.Message))); + return await FindLatestAsync(imported.ImportResult.WorkflowDefinition.DefinitionId); + } + + /// Posts back onto the way the document PUT does, and returns what exporting the result gives. + private async Task PutAsync(WorkflowDefinition stored, BpmnDefinitions document) + { + var putResult = await DocumentService.ImportDocumentAsync(document, stored.DefinitionId, ProcessIdOf(stored), CancellationToken.None); + Assert.True(putResult.ImportResult.Succeeded, string.Join("; ", putResult.ImportResult.ValidationErrors.Select(error => error.Message))); + return ExportOf(await FindLatestAsync(stored.DefinitionId)); + } + + private XDocument ExportOf(WorkflowDefinition definition) => XDocument.Parse(Encoding.UTF8.GetString(DocumentService.Export(definition))); + + /// + /// as a client of the document endpoints holds it: serialized the way GET writes + /// it, optionally edited as JSON, and deserialized the way PUT reads it. + /// + private static BpmnDefinitions ThroughTheDocumentEndpoints(BpmnDefinitions document, Action? edit = null) + { + var json = JsonNode.Parse(JsonSerializer.Serialize(document, BpmnDocumentJsonOptions.Value))!; + edit?.Invoke(json); + return json.Deserialize(BpmnDocumentJsonOptions.Value)!; + } + + private static BpmnDefinitions WithBindingRef(BpmnDefinitions document, string elementId, string? bindingRef) => + ThroughTheDocumentEndpoints(document, json => ElementOf(json, elementId)["bindingRef"] = bindingRef); + + private static JsonNode ElementOf(JsonNode document, string elementId) => + document["processes"]!.AsArray().SelectMany(process => process!["elements"]!.AsArray()).Single(element => element!["elementId"]!.GetValue() == elementId)!; + + /// The subProcess or transaction element with the given id. + private static XElement ScopeOf(XDocument document, string id) => + NestedScopesOf(document).Single(scope => scope.Attribute("id")?.Value == id); + + private static IEnumerable NestedScopesOf(XDocument document) => document.Descendants().Where(IsNestedScope); + + private static bool IsNestedScope(XElement element) => element.Name == Bpmn + "subProcess" || element.Name == Bpmn + "transaction"; + + /// + /// as text, with each nested scope's multiInstanceLoopCharacteristics reduced to the + /// distinct markers it carries and moved last. Bpmn.Interchange 0.2.0 both reads a multi-instance subprocess's marker + /// onto its element and retains a copy as foreign content of the nested process it opens, so Export, which + /// writes exactly what it read, emits the marker twice: once from the element, once from the retained copy, at the + /// position the stored document had it. That position moves when a document PUT rewrites the stored document, + /// even though the marker does not. Comparing the distinct markers still fails on a marker lost or changed. Tracked + /// upstream as valence-works/bpmn#21; remove this + /// workaround once a Bpmn.Interchange release containing that fix is adopted. + /// + private static string ComparableContentOf(XElement element) + { + var comparable = new XElement(element); + + foreach (var scope in comparable.DescendantsAndSelf().Where(IsNestedScope).ToList()) + { + var markers = scope.Elements(Bpmn + "multiInstanceLoopCharacteristics").ToList(); + markers.Remove(); + scope.Add(markers.DistinctBy(marker => marker.ToString())); + } + + return comparable.ToString(); + } + + /// + /// Every flow element and sequence flow the document nests inside a subprocess or transaction, as + /// scope/kind/id. An association is left out: the model records it only as the compensation boundary event's + /// handler, so the writer derives its id rather than keeping the source's. + /// + private static IReadOnlyList NestedElementIdsOf(XDocument document) => + NestedScopesOf(document) + .SelectMany(scope => scope.Elements() + .Where(child => child.Name != Bpmn + "association" && child.Attribute("id") is not null) + .Select(child => $"{scope.Attribute("id")!.Value}/{child.Name.LocalName}/{child.Attribute("id")!.Value}")) + .Order(StringComparer.Ordinal) + .ToList(); + /// Reads a fixture from the Assets directory shipped alongside this test project. private static string ReadAsset(string fileName) => BpmnAssetReader.Read(fileName); }