diff --git a/doc/wiki/bpmn-workflows.md b/doc/wiki/bpmn-workflows.md index d96673c9e..38600b97f 100644 --- a/doc/wiki/bpmn-workflows.md +++ b/doc/wiki/bpmn-workflows.md @@ -215,6 +215,17 @@ capabilities and the offending element ids, rather than persisting a definition runs. `Analyze` never performs this check, since it does not persist; a document that `Analyze` reports cleanly can still be refused by `Import` on capability grounds. +### Duplicate element id refusal + +BPMN requires every element id to be unique within a document. `Import` and the document `PUT` both refuse, with +`422 Unprocessable Entity`, a document that repeats one — most often a subprocess nested inside another subprocess +that reuses its parent's id. This is not just an ordinary validation rule: reading or writing such a document walks +into a nested process by matching the repeated id back out of a flat binding list, in three different places (this +service's own capability walk, the work binder, and the interchange library's own writer), and each of those walks +would otherwise recurse without ever terminating and crash the process outright — .NET cannot catch a +`StackOverflowException`. Both endpoints check this before any of that recursion runs. `Analyze` never performs this +check, since the plain read it does never walks a nested process this way to begin with. + ### Export's limitation `Export` does not reconstruct a `.bpmn` document from the Elsa activity graph a definition runs — that would discard @@ -290,6 +301,7 @@ element that has a stored body but no `bindingRef`, are uncoded. | --- | --- | --- | --- | | `bpmn.import.capability-unsupported` | `POST bpmn/import`, document `PUT` | 422 | `capabilities: string[]` (missing capability names), `elementIds: string[]` (offending element ids, combined across every missing capability) | | `bpmn.import.binding-invalid` | `POST bpmn/import`, document `PUT` | 422 | — | +| `bpmn.import.duplicate-element-id` | `POST bpmn/import`, document `PUT` | 422 | `elementIds: string[]` (the duplicated ids) | | `bpmn.export.not-imported` | `GET .../export`, document `GET` | 422 | — | | `bpmn.export.source-stale` | `GET .../export`, document `GET` | 422 | — | | `bpmn.export.source-version-unknown` | `GET .../export`, document `GET` | 422 | — | diff --git a/src/modules/Elsa.Bpmn.Interchange/BpmnErrorCodes.cs b/src/modules/Elsa.Bpmn.Interchange/BpmnErrorCodes.cs index 98c38b31a..ae25cc2e1 100644 --- a/src/modules/Elsa.Bpmn.Interchange/BpmnErrorCodes.cs +++ b/src/modules/Elsa.Bpmn.Interchange/BpmnErrorCodes.cs @@ -27,6 +27,16 @@ public static class BpmnErrorCodes /// public const string ImportBindingInvalid = "bpmn.import.binding-invalid"; + /// + /// bpmn/import and the document PUT refuse a document that declares the same element id more than + /// once — most often a subprocess nested inside another subprocess that reuses its parent's id. Refused before + /// any recursion into nested processes runs, since a repeated id would otherwise make that recursion — in this + /// type's own capability walk, in BpmnWorkBinder.BindScope, and in Bpmn.Interchange's own + /// BpmnXmlWriter — loop without ever terminating and crash the process outright. Carries + /// data.elementIds (the duplicated ids). + /// + public const string ImportDuplicateElementId = "bpmn.import.duplicate-element-id"; + /// /// bpmn/definitions/{id}/export and the document GET refuse a workflow definition that does not /// currently carry BPMN source — either it was never imported from BPMN, or a later save replaced its custom diff --git a/src/modules/Elsa.Bpmn.Interchange/Endpoints/Bpmn/BpmnImportErrorResponses.cs b/src/modules/Elsa.Bpmn.Interchange/Endpoints/Bpmn/BpmnImportErrorResponses.cs index 6229e84f8..094e259a1 100644 --- a/src/modules/Elsa.Bpmn.Interchange/Endpoints/Bpmn/BpmnImportErrorResponses.cs +++ b/src/modules/Elsa.Bpmn.Interchange/Endpoints/Bpmn/BpmnImportErrorResponses.cs @@ -50,6 +50,11 @@ internal static class BpmnImportErrorResponses await BpmnErrorResponse.SendAsync(httpResponse, BindingInvalidResponseFor(exception), cancellationToken); return null; } + catch (BpmnDuplicateElementIdException exception) + { + await BpmnErrorResponse.SendAsync(httpResponse, DuplicateElementIdResponseFor(exception), cancellationToken); + return null; + } catch (BpmnCapabilityException exception) { await BpmnErrorResponse.SendAsync(httpResponse, CapabilityResponseFor(exception), cancellationToken); @@ -83,6 +88,17 @@ internal static class BpmnImportErrorResponses internal static BpmnErrorResponse BindingInvalidResponseFor(BpmnBindingException exception) => BpmnErrorResponse.Create(exception.Message, BpmnErrorCodes.ImportBindingInvalid, StatusCodes.Status422UnprocessableEntity); + /// + /// The response for , carrying + /// the duplicated ids as data.elementIds. + /// + internal static BpmnErrorResponse DuplicateElementIdResponseFor(BpmnDuplicateElementIdException exception) => + BpmnErrorResponse.Create( + exception.Message, + BpmnErrorCodes.ImportDuplicateElementId, + StatusCodes.Status422UnprocessableEntity, + new { ElementIds = exception.DuplicateElementIds }); + /// /// The response for , as its /// own pure, synchronous step so it can be asserted on directly: Bpmn.* 0.2.0 declares every capability diff --git a/src/modules/Elsa.Bpmn.Interchange/Exceptions/BpmnDuplicateElementIdException.cs b/src/modules/Elsa.Bpmn.Interchange/Exceptions/BpmnDuplicateElementIdException.cs new file mode 100644 index 000000000..d1f2f0fe7 --- /dev/null +++ b/src/modules/Elsa.Bpmn.Interchange/Exceptions/BpmnDuplicateElementIdException.cs @@ -0,0 +1,19 @@ +namespace Elsa.Bpmn.Interchange.Exceptions; + +/// +/// Thrown when a BPMN document declares the same element id more than once. +/// +/// +/// BPMN requires every element id to be unique within a document. A repeat — most often a subprocess nested inside +/// another subprocess that reuses its parent's id — is not merely invalid input: BpmnInterchangeDocumentService's +/// own capability walk, BpmnWorkBinder.BindScope and Bpmn.Interchange's own BpmnXmlWriter all read +/// "the nested processes belonging to this scope" back out of a flat binding list by matching on the repeated id, so a +/// document like this makes each of them recurse without ever terminating and crash the process outright — .NET +/// cannot catch a . This is thrown, and the document refused, before any of that +/// recursion runs. +/// +public class BpmnDuplicateElementIdException(string message, IReadOnlyList duplicateElementIds) : Exception(message) +{ + /// The element ids the document declares more than once. + public IReadOnlyList DuplicateElementIds { get; } = duplicateElementIds; +} diff --git a/src/modules/Elsa.Bpmn.Interchange/Services/BpmnInterchangeDocumentService.cs b/src/modules/Elsa.Bpmn.Interchange/Services/BpmnInterchangeDocumentService.cs index 5b07c609b..02bad7e81 100644 --- a/src/modules/Elsa.Bpmn.Interchange/Services/BpmnInterchangeDocumentService.cs +++ b/src/modules/Elsa.Bpmn.Interchange/Services/BpmnInterchangeDocumentService.cs @@ -226,6 +226,13 @@ public sealed class BpmnInterchangeDocumentService( CancellationToken cancellationToken) { var result = reader.Read(xml, new BpmnImportOptions { ProcessId = processId }); + + // Before anything below walks into a nested process by matching an id, refuse a document that repeats one: + // see EnsureElementIdsUnique's remarks for why that walk is otherwise not provably finite. reader.Read itself + // never recurses this way — it walks the XML's own element tree, not an id lookup — so it is safe to call + // first and check its result. + EnsureElementIdsUnique(result.Definitions.Processes, result.Bindings); + var rootDefinition = ResolveRootDefinition(result.Definitions, processId); EnsureCapabilitiesSatisfied(rootDefinition, result.Bindings); @@ -384,7 +391,15 @@ public sealed class BpmnInterchangeDocumentService( $"Workflow definition '{definitionId}' does not exist, so its BPMN document cannot be edited."); } - var xml = writer.Write(document, StoredNestedScopesStillDeclaredBy(document, existingDefinition)); + var storedNestedScopes = StoredNestedScopesStillDeclaredBy(document, existingDefinition); + + // Unlike ImportAsync's xml, writer.Write itself is one of the sites that walks nested processes by matching + // an id (see EnsureElementIdsUnique's remarks), and it runs before ImportCoreAsync — and the same check + // inside it — ever sees this document. So it is checked here too, against exactly the inputs writer.Write is + // about to receive, before that call rather than after it. + EnsureElementIdsUnique(document.Processes, storedNestedScopes); + + var xml = writer.Write(document, storedNestedScopes); return await ImportCoreAsync(xml, definitionId, name: null, processId, preserveMetadataFrom: existingDefinition, cancellationToken); } @@ -595,6 +610,83 @@ public sealed class BpmnInterchangeDocumentService( $"The document declares {definitions.Processes.Count} processes ({declared}); specify which one to import."); } + /// + /// Refuses a document that declares the same element id more than once, naming the duplicated ids. + /// + /// + /// + /// BPMN requires every element id to be unique within a document. The library's own reader tolerates a repeat — + /// BpmnXmlReader walks the XML's own element tree, so it terminates regardless of what any id says — but + /// nothing downstream of it does: this type's own EnsureCapabilitiesSatisfied and BpmnWorkBinder.BindScope + /// both find "the nested processes belonging to this scope" by matching + /// against the scope's own id, and Bpmn.Interchange's own BpmnXmlWriter does the same by matching + /// . A 's own + /// is always the element id of the subprocess element that opens + /// it, so a subprocess nested inside another subprocess that reuses its parent's id makes that lookup find its + /// own parent — or itself — again on every step down. Each of those three walks then recurses without ever + /// terminating and crashes the process outright: .NET cannot catch a . This + /// runs before any of them does, so a document like that is refused rather than crashing the server. + /// + /// + /// Once every element id is unique, that recursion is provably finite without a separate depth guard: a scope's + /// nested processes can then only ever be the ones its own or + /// actually names, so the walk can only ever follow the tree the + /// document's own nesting describes. + /// + /// + /// The document's own top-level process bodies. + /// + /// Every binding across the same processes, so every subprocess body nested inside them — which is not one of + /// itself, and carries elements does not enumerate — + /// is covered too. + /// + /// + /// A top-level 's own is a scope + /// id in exactly the same id-space as every element id below it: EnsureCapabilitiesSatisfied, + /// BpmnWorkBinder.BindScope and Bpmn.Interchange's own BpmnXmlWriter all find "the nested + /// processes belonging to this scope" by matching a 's owner id + /// against a — a top-level process's own id, not one of + /// its declared elements, so nothing below ever puts it in the pool checked for uniqueness on its own. A + /// subprocess reusing that id (e.g. <process id="P"><subProcess id="P">) makes that lookup + /// find the top-level scope again instead of terminating — the same class of infinite recursion a repeated + /// element id causes — so it is added here explicitly, once per top-level process. + /// + /// A nested process definition's own needs no equivalent + /// addition: it is always exactly the of the subprocess + /// element that opens it, by construction of the library's own reader, and that element id is already in the + /// pool below as one of its owner's elements. Adding it a second time would flag every ordinary + /// subprocess as a duplicate of itself; the legitimate pairing is counted once by not adding it again here. + /// + /// + /// An element id, or a top-level process id, is declared more than once. + internal static void EnsureElementIdsUnique(IEnumerable processes, IReadOnlyList bindings) + { + var processList = processes as IReadOnlyCollection ?? processes.ToList(); + + var processIds = processList.Select(process => process.ProcessId); + + var elementIds = processList + .Concat(bindings.OfType().Select(nested => nested.Definition)) + .SelectMany(process => process.Elements) + .Select(element => element.ElementId); + + var duplicateIds = processIds + .Concat(elementIds) + .GroupBy(id => id, StringComparer.Ordinal) + .Where(group => group.Count() > 1) + .Select(group => group.Key) + .ToList(); + + if (duplicateIds.Count == 0) + return; + + throw new BpmnDuplicateElementIdException( + $"The document declares the same element id more than once, which BPMN requires to be unique: {string.Join(", ", duplicateIds)}. " + + "This is most often a subprocess nested inside another subprocess that reuses its parent's id. Reading or writing such a document " + + "cannot be done safely, so it is refused rather than attempted.", + duplicateIds); + } + /// /// Refuses the definition, naming the missing capability and the offending element ids, when it or any process /// nested inside it needs a host capability does not cover. diff --git a/test/integration/Elsa.Bpmn.Interchange.IntegrationTests/Assets/nested-subprocess-duplicate-id.bpmn b/test/integration/Elsa.Bpmn.Interchange.IntegrationTests/Assets/nested-subprocess-duplicate-id.bpmn new file mode 100644 index 000000000..7c5460fe7 --- /dev/null +++ b/test/integration/Elsa.Bpmn.Interchange.IntegrationTests/Assets/nested-subprocess-duplicate-id.bpmn @@ -0,0 +1,47 @@ + + + + + Flow_1 + + + + + Flow_1 + Flow_2 + + Outer_Flow_1 + + + + Outer_Flow_1 + Outer_Flow_2 + + Inner_Flow_1 + + + Inner_Flow_1 + + + + + + Outer_Flow_2 + + + + + + + Flow_2 + + + + + + diff --git a/test/integration/Elsa.Bpmn.Interchange.IntegrationTests/Assets/subprocess-reuses-parent-process-id.bpmn b/test/integration/Elsa.Bpmn.Interchange.IntegrationTests/Assets/subprocess-reuses-parent-process-id.bpmn new file mode 100644 index 000000000..a172caf90 --- /dev/null +++ b/test/integration/Elsa.Bpmn.Interchange.IntegrationTests/Assets/subprocess-reuses-parent-process-id.bpmn @@ -0,0 +1,34 @@ + + + + + + Flow_1 + + + + Flow_1 + Flow_2 + + Inner_Flow_1 + + + Inner_Flow_1 + + + + + + Flow_2 + + + + + + diff --git a/test/integration/Elsa.Bpmn.Interchange.IntegrationTests/Assets/two-process-duplicate-id.bpmn b/test/integration/Elsa.Bpmn.Interchange.IntegrationTests/Assets/two-process-duplicate-id.bpmn new file mode 100644 index 000000000..1cbd45a4a --- /dev/null +++ b/test/integration/Elsa.Bpmn.Interchange.IntegrationTests/Assets/two-process-duplicate-id.bpmn @@ -0,0 +1,27 @@ + + + + + + Flow_1 + + + Flow_1 + + + + + + Flow_2 + + + 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 3a09e565a..f4a231ab1 100644 --- a/test/integration/Elsa.Bpmn.Interchange.IntegrationTests/Endpoints/BpmnInterchangeEndpointTests.cs +++ b/test/integration/Elsa.Bpmn.Interchange.IntegrationTests/Endpoints/BpmnInterchangeEndpointTests.cs @@ -148,6 +148,67 @@ public class BpmnInterchangeEndpointTests(ITestOutputHelper testOutputHelper) : Assert.Contains("nothing binds it to an Elsa activity", body); } + [Fact] + public async Task Import_OfADocumentWithASubprocessNestedInsideASubprocessThatReusesItsParentsId_ReturnsUnprocessableEntityAndTheServerStaysAlive() + { + using var content = new MultipartFormDataContent(); + AddBpmnFile(content, ReadAsset("nested-subprocess-duplicate-id.bpmn"), "file"); + + var response = await PostAuthenticatedAsync("bpmn/import", content, "workflows/definitions:write"); + + Assert.Equal(HttpStatusCode.UnprocessableEntity, response.StatusCode); + var body = await response.Content.ReadAsStringAsync(); + Assert.Equal(BpmnErrorCodes.ImportDuplicateElementId, CodeOf(body)); + Assert.Contains("Outer", body); + + // elsa-core#8074: before the fix, reading this document overflowed the stack and killed the process, which + // .NET cannot catch — there would be no HTTP response to assert on at all. Reaching the assertions above already + // proves the process survived; a further successful request proves the host is still serving requests, too. + using var followUpContent = new MultipartFormDataContent(); + AddBpmnFile(followUpContent, ReadAsset("camunda-order-process.bpmn"), "file"); + var followUpResponse = await PostAuthenticatedAsync("bpmn/analyze", followUpContent, "workflows/definitions:view"); + Assert.Equal(HttpStatusCode.OK, followUpResponse.StatusCode); + } + + [Fact] + public async Task Import_OfADocumentWithASubprocessReusingItsParentTopLevelProcessesOwnId_ReturnsUnprocessableEntityAndTheServerStaysAlive() + { + using var content = new MultipartFormDataContent(); + AddBpmnFile(content, ReadAsset("subprocess-reuses-parent-process-id.bpmn"), "file"); + + var response = await PostAuthenticatedAsync("bpmn/import", content, "workflows/definitions:write"); + + Assert.Equal(HttpStatusCode.UnprocessableEntity, response.StatusCode); + var body = await response.Content.ReadAsStringAsync(); + Assert.Equal(BpmnErrorCodes.ImportDuplicateElementId, CodeOf(body)); + Assert.Contains("P", body); + + // elsa-core#8074: a top-level process's own id was never in the pool checked for uniqueness (only its + // elements were), so a subprocess declared directly inside it that reuses that same id went undetected and + // overflowed the stack the same way a subprocess nested inside another subprocess does. See the equivalent + // nested-subprocess test's remarks: reaching the assertions above already proves the process survived; a + // further successful request proves the host is still serving requests, too. + using var followUpContent = new MultipartFormDataContent(); + AddBpmnFile(followUpContent, ReadAsset("camunda-order-process.bpmn"), "file"); + var followUpResponse = await PostAuthenticatedAsync("bpmn/analyze", followUpContent, "workflows/definitions:view"); + Assert.Equal(HttpStatusCode.OK, followUpResponse.StatusCode); + } + + [Fact] + public async Task Import_OfADocumentWithTwoTopLevelProcessesSharingAnId_ReturnsUnprocessableEntity() + { + using var content = new MultipartFormDataContent(); + AddBpmnFile(content, ReadAsset("two-process-duplicate-id.bpmn"), "file"); + content.Add(new StringContent("shared"), "ProcessId"); + + var response = await PostAuthenticatedAsync("bpmn/import", content, "workflows/definitions:write"); + + Assert.Equal(HttpStatusCode.UnprocessableEntity, response.StatusCode); + var body = await response.Content.ReadAsStringAsync(); + Assert.Equal(BpmnErrorCodes.ImportDuplicateElementId, CodeOf(body)); + Assert.Contains("shared", body); + } + [Fact] public async Task Import_OfAValidDocument_ReturnsOkAndPersistsADefinition() { @@ -491,6 +552,50 @@ public class BpmnInterchangeEndpointTests(ITestOutputHelper testOutputHelper) : Assert.Equal(versionBeforePut, await LatestVersionOfAsync(definitionId)); } + [Fact] + public async Task DocumentPut_WithARepeatedElementId_ReturnsUnprocessableEntityAndPersistsNoNewDraftAndTheServerStaysAlive() + { + var definitionId = await ImportCamundaOrderProcessAsync(); + var versionBeforePut = await LatestVersionOfAsync(definitionId); + + var (etag, documentJson) = await GetDocumentAsync(definitionId); + var putResponse = await PutDocumentAsync(definitionId, WithADuplicatedElementId(documentJson), etag); + + Assert.Equal(HttpStatusCode.UnprocessableEntity, putResponse.StatusCode); + var body = await putResponse.Content.ReadAsStringAsync(); + Assert.Equal(BpmnErrorCodes.ImportDuplicateElementId, CodeOf(body)); + Assert.Equal(versionBeforePut, await LatestVersionOfAsync(definitionId)); + + // See the equivalent Import test's remarks: reaching the assertions above already proves the process + // survived reading this document; a further successful request proves the host is still serving requests. + Assert.Equal(HttpStatusCode.OK, (await GetAuthenticatedAsync($"bpmn/definitions/{definitionId}/document", "workflows/definitions:view")).StatusCode); + } + + [Fact] + public async Task DocumentPut_WithATopLevelProcessIdReusingAnExistingSubprocessId_ReturnsUnprocessableEntityAndPersistsNoNewDraftAndTheServerStaysAlive() + { + // nested-subprocesses.bpmn already declares a subprocess with id "Outer"; renaming the top-level process's + // own id to "Outer" reproduces elsa-core#8074's collision through the document PUT, where the nested scope + // ("Outer"'s stored body) comes not from the edited document but from the definition's already-stored + // source (see ImportDocumentAsync's remarks on storedNestedScopes). + var definitionId = await ImportWrittenBackAsync("nested-subprocesses.bpmn"); + var versionBeforePut = await LatestVersionOfAsync(definitionId); + + var (etag, documentJson) = await GetDocumentAsync(definitionId); + var putResponse = await PutDocumentAsync(definitionId, WithTopLevelProcessIdReusingASubprocessId(documentJson, "Outer"), etag); + + Assert.Equal(HttpStatusCode.UnprocessableEntity, putResponse.StatusCode); + var body = await putResponse.Content.ReadAsStringAsync(); + Assert.Equal(BpmnErrorCodes.ImportDuplicateElementId, CodeOf(body)); + Assert.Contains("Outer", body); + Assert.Equal(versionBeforePut, await LatestVersionOfAsync(definitionId)); + + // See the equivalent repeated-element-id test's remarks: reaching the assertions above already proves the + // process survived reading this document; a further successful request proves the host is still serving + // requests, too. + Assert.Equal(HttpStatusCode.OK, (await GetAuthenticatedAsync($"bpmn/definitions/{definitionId}/document", "workflows/definitions:view")).StatusCode); + } + [Fact] public async Task DocumentPut_UnchangedDocument_ReturnsOkAndTheSameFindingsAsImport() { @@ -784,6 +889,24 @@ public class BpmnInterchangeEndpointTests(ITestOutputHelper testOutputHelper) : return document.ToJsonString(); } + /// Renames the document's last top-level element to its first element's id, so the two collide (elsa-core#8074). + private static string WithADuplicatedElementId(string documentJson) + { + var document = JsonNode.Parse(documentJson)!; + var elements = document["processes"]![0]!["elements"]!.AsArray(); + var firstElementId = elements[0]!["elementId"]!.GetValue(); + elements[^1]!["elementId"] = firstElementId; + return document.ToJsonString(); + } + + /// Renames the document's top-level process id to , so it collides with a subprocess that already declares that id (elsa-core#8074). + private static string WithTopLevelProcessIdReusingASubprocessId(string documentJson, string subprocessId) + { + var document = JsonNode.Parse(documentJson)!; + document["processes"]![0]!["processId"] = subprocessId; + return document.ToJsonString(); + } + /// /// Changes the literal text the NotifyWarehouse task's elsa:activityBinding configures its bound /// with — a real binding change, the kind Elsa Studio's binding UX makes, as opposed to diff --git a/test/unit/Elsa.Bpmn.Interchange.UnitTests/BpmnErrorResponseMappingTests.cs b/test/unit/Elsa.Bpmn.Interchange.UnitTests/BpmnErrorResponseMappingTests.cs index 87dd4d5c8..80eb7be5d 100644 --- a/test/unit/Elsa.Bpmn.Interchange.UnitTests/BpmnErrorResponseMappingTests.cs +++ b/test/unit/Elsa.Bpmn.Interchange.UnitTests/BpmnErrorResponseMappingTests.cs @@ -56,6 +56,21 @@ public class BpmnErrorResponseMappingTests Assert.Null(response.Data); } + [Fact(DisplayName = "A duplicate-element-id refusal is coded bpmn.import.duplicate-element-id, carrying the duplicated ids as data")] + public void DuplicateElementIdResponseFor_CarriesTheCodeAndTheStructuredData() + { + var exception = new BpmnDuplicateElementIdException("The document declares the same element id more than once: Outer.", ["Outer"]); + + var response = BpmnImportErrorResponses.DuplicateElementIdResponseFor(exception); + + Assert.Equal(BpmnErrorCodes.ImportDuplicateElementId, response.Code); + Assert.Equal(StatusCodes.Status422UnprocessableEntity, response.StatusCode); + Assert.Equal(exception.Message, Assert.Single(response.Errors["generalErrors"])); + + dynamic data = response.Data!; + Assert.Equal(new[] { "Outer" }, (IReadOnlyList)data.ElementIds); + } + [Fact(DisplayName = "A definition-not-found refusal is coded bpmn.document.not-found")] public void NotFoundResponseFor_CarriesTheCode() { diff --git a/test/unit/Elsa.Bpmn.Interchange.UnitTests/BpmnInterchangeDocumentServiceDuplicateElementIdTests.cs b/test/unit/Elsa.Bpmn.Interchange.UnitTests/BpmnInterchangeDocumentServiceDuplicateElementIdTests.cs new file mode 100644 index 000000000..d341948b0 --- /dev/null +++ b/test/unit/Elsa.Bpmn.Interchange.UnitTests/BpmnInterchangeDocumentServiceDuplicateElementIdTests.cs @@ -0,0 +1,107 @@ +using Bpmn.Interchange; +using Bpmn.Model; +using Elsa.Bpmn.Interchange.Exceptions; +using Elsa.Bpmn.Interchange.Services; + +namespace Elsa.Bpmn.Interchange.UnitTests; + +/// +/// Duplicate element id refusal at import: is the +/// internal seam the Import endpoint and the document PUT both call into, exercised here directly with the +/// exact shape a subprocess nested inside another subprocess that reuses its parent's id produces — the shape that +/// otherwise makes , BpmnWorkBinder.BindScope +/// and BpmnXmlWriter recurse without terminating (elsa-core#8074). +/// +public class BpmnInterchangeDocumentServiceDuplicateElementIdTests +{ + [Fact(DisplayName = "A document whose element ids are all unique is accepted")] + public void EnsureElementIdsUnique_AcceptsADocumentWithNoRepeatedIds() + { + var task = new BpmnElement("task-1", BpmnElementTypes.ServiceTask, bindingRef: "node-task-1"); + var root = new BpmnProcessDefinition("main", Elements: [task]); + + // No exception is the assertion: every element id in the document is unique. + BpmnInterchangeDocumentService.EnsureElementIdsUnique([root], []); + } + + [Fact(DisplayName = "A document declaring the same element id twice at the top level is refused, naming the id")] + public void EnsureElementIdsUnique_RefusesARepeatedTopLevelElementId() + { + var first = new BpmnElement("dup", BpmnElementTypes.ServiceTask, bindingRef: "node-dup-1"); + var second = new BpmnElement("dup", BpmnElementTypes.ServiceTask, bindingRef: "node-dup-2"); + var root = new BpmnProcessDefinition("main", Elements: [first, second]); + + var exception = Assert.Throws(() => + BpmnInterchangeDocumentService.EnsureElementIdsUnique([root], [])); + + Assert.Equal(["dup"], exception.DuplicateElementIds); + Assert.Contains("dup", exception.Message); + } + + [Fact(DisplayName = "A subprocess nested inside another subprocess that reuses its parent's id is refused, naming that id")] + public void EnsureElementIdsUnique_RefusesASubprocessNestedInsideASubprocessThatReusesItsParentsId() + { + var (root, bindings) = NestedSubprocessReusingItsOwnId(); + + var exception = Assert.Throws(() => + BpmnInterchangeDocumentService.EnsureElementIdsUnique([root], bindings)); + + Assert.Equal(["Outer"], exception.DuplicateElementIds); + } + + [Fact(DisplayName = "A subprocess reusing its parent top-level process's own id is refused, naming that id")] + public void EnsureElementIdsUnique_RefusesASubprocessReusingItsParentTopLevelProcessesOwnId() + { + // ...: the subprocess element id equals the + // top-level process's own ProcessId. Before the fix, a top-level process's own id was never added to the + // pool checked for uniqueness (only its Elements were), so "P" appeared only once — as the subprocess + // element inside root.Elements — and this collision went undetected. + var subProcessElement = new BpmnElement("P", BpmnElementTypes.SubProcess, bindingRef: "node-p"); + var root = new BpmnProcessDefinition("P", Elements: [subProcessElement]); + var subProcessBody = new BpmnProcessDefinition("P"); + + BpmnWorkBinding[] bindings = [new BpmnWorkBinding.NestedProcess("P", "P", "node-p", BpmnBindingSlot.Primary, subProcessBody)]; + + var exception = Assert.Throws(() => + BpmnInterchangeDocumentService.EnsureElementIdsUnique([root], bindings)); + + Assert.Equal(["P"], exception.DuplicateElementIds); + } + + [Fact(DisplayName = "Two top-level processes sharing the same id are refused, naming that id")] + public void EnsureElementIdsUnique_RefusesTwoTopLevelProcessesSharingAnId() + { + var first = new BpmnProcessDefinition("shared", Elements: [new BpmnElement("task-1", BpmnElementTypes.ServiceTask, bindingRef: "node-task-1")]); + var second = new BpmnProcessDefinition("shared", Elements: [new BpmnElement("task-2", BpmnElementTypes.ServiceTask, bindingRef: "node-task-2")]); + + var exception = Assert.Throws(() => + BpmnInterchangeDocumentService.EnsureElementIdsUnique([first, second], [])); + + Assert.Equal(["shared"], exception.DuplicateElementIds); + } + + /// + /// The exact document shape elsa-core#8074 reports: process main declares a subprocess Outer, + /// whose body declares another subprocess that reuses the id Outer rather than declaring one of its own. + /// + private static (BpmnProcessDefinition Root, IReadOnlyList Bindings) NestedSubprocessReusingItsOwnId() + { + var outerElement = new BpmnElement("Outer", BpmnElementTypes.SubProcess, bindingRef: "node-outer"); + var root = new BpmnProcessDefinition("main", Elements: [outerElement]); + + // The inner subprocess element, declared inside Outer's own body, reuses "Outer" as its id instead of + // declaring its own — the innermost NestedProcess.Definition's own ProcessId is "Outer" too, for the same + // reason (Bpmn.Interchange's reader gives a subprocess body the element id that opens it as its ProcessId). + var innerElement = new BpmnElement("Outer", BpmnElementTypes.SubProcess, bindingRef: "node-outer-inner"); + var outerBody = new BpmnProcessDefinition("Outer", Elements: [innerElement]); + var innerBody = new BpmnProcessDefinition("Outer"); + + BpmnWorkBinding[] bindings = + [ + new BpmnWorkBinding.NestedProcess("main", "Outer", "node-outer", BpmnBindingSlot.Primary, outerBody), + new BpmnWorkBinding.NestedProcess("Outer", "Outer", "node-outer-inner", BpmnBindingSlot.Primary, innerBody) + ]; + + return (root, bindings); + } +}