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