diff --git a/doc/wiki/bpmn-workflows.md b/doc/wiki/bpmn-workflows.md index d5d929fcb..fa5a13fd5 100644 --- a/doc/wiki/bpmn-workflows.md +++ b/doc/wiki/bpmn-workflows.md @@ -221,18 +221,22 @@ document, in three situations: - The definition does not currently carry BPMN source — either it was never imported from BPMN, or a later save replaced its custom properties wholesale (BPMN source travels on the same `CustomProperties` dictionary a workflow edit can overwrite). -- The definition has changed — by version — since the source was recorded, meaning the stored BPMN text no longer - corresponds to the current definition. -- The definition's activity graph has changed since the source was recorded, even though its version has not. An - unpublished draft is saved in place (same row, same version), so a designer save that edits a bound activity's - inputs — as Studio's binding UX (elsa-studio#1001) does — moves the graph without moving the version, which the - version check above cannot see. `Import` also records a SHA-256 hash of the graph - (`BpmnInterchangeDocumentService.SourceGraphHashCustomPropertyKey`, `Bpmn:SourceGraphHash`) at the moment it stores - the source, and `Export`/the document `GET` refuse when the current graph's hash no longer matches it. A definition - imported before this marker existed carries no value for it and falls back to the version-only check, so it is not - refused just for predating the marker. One practical consequence: a document `GET` performed after a designer save - now returns `422` too — Studio has to re-import (or PUT a fresh document) rather than edit a document that no - longer describes the current graph. +- The definition's activity graph has changed since the source was recorded. `Import` records a SHA-256 hash of the + graph (`BpmnInterchangeDocumentService.SourceGraphHashCustomPropertyKey`, `Bpmn:SourceGraphHash`) at the moment it + stores the source, and once a definition carries that marker, it alone decides staleness: `Export`/the document + `GET` refuse exactly when the current graph's hash no longer matches it, regardless of whether the definition's + version has also changed. That cuts both ways. An unpublished draft is saved in place (same row, same version), so + a designer save that edits a bound activity's inputs — as Studio's binding UX (elsa-studio#1001) does — moves the + graph without moving the version, and is refused as stale even though the version alone would have missed it. The + other way round, publishing a definition and then making a metadata-only save — a rename, a variable change — bumps + it to a new draft version without touching the graph, and is *not* refused: the version moved, but the stored + source still describes the graph exactly. One practical consequence: a document `GET` performed after a designer + save that edits the graph returns `422` — Studio has to re-import (or PUT a fresh document) rather than edit a + document that no longer describes the current graph — but a `GET` after a rename or other metadata-only save keeps + returning `200`. +- The definition has changed — by version — since the source was recorded, and it carries no graph-hash marker to + decide staleness by instead. This is the whole test for a definition imported before that marker existed; once one + exists, it takes over from the version check entirely, as above. A missing `definitionId` returns `404 Not Found`. diff --git a/src/modules/Elsa.Bpmn.Interchange/Services/BpmnInterchangeDocumentService.cs b/src/modules/Elsa.Bpmn.Interchange/Services/BpmnInterchangeDocumentService.cs index 44590a4ee..5b07c609b 100644 --- a/src/modules/Elsa.Bpmn.Interchange/Services/BpmnInterchangeDocumentService.cs +++ b/src/modules/Elsa.Bpmn.Interchange/Services/BpmnInterchangeDocumentService.cs @@ -54,12 +54,13 @@ namespace Elsa.Bpmn.Interchange.Services; /// in the first place; says so honestly rather than asserting the document /// was "never imported", which would be true in one case and false in the other. Separately, a save that DOES carry /// the key forward can still leave the definition materially changed — the graph, name, or anything else about it — -/// while the stored BPMN text still describes the pre-edit document. -/// records the definition's own version at the moment of import for exactly this: if the current version no longer -/// matches, the stored source is stale, and exporting it would silently hand back a document that is not what the -/// caller has, which is worse than refusing. A version bump alone misses one case, though: an unpublished draft is -/// saved in place — same row, same version — so a designer save of the draft moves the graph without moving the -/// version. closes that gap with a content hash of the graph itself. +/// while the stored BPMN text still describes the pre-edit document. What decides that is the graph itself, not the +/// version: records a content hash of the graph at the moment of +/// import, and once it is present it alone says whether the stored source is stale, because it is the only one of +/// the two markers a metadata-only save (a rename, a variable edit) and a graph-changing save always disagree on — a +/// version bump does not, by itself, mean the graph moved, and it does move without a version bump when an +/// unpublished draft is saved in place. is what a definition imported +/// before the graph hash existed falls back to. /// /// /// A whole-definition import and a document edit disagree about what else gets replaced. @@ -101,11 +102,11 @@ public sealed class BpmnInterchangeDocumentService( /// /// /// compares this against the definition's current version to tell a - /// still-current source from a stale one. The version number is what itself - /// already exposes for "has this definition changed", so this reuses it rather than inventing a second notion of - /// change for the case a version bump alone catches: a publish, or any other save that assigns a new version. - /// It does not, on its own, catch an unpublished draft saved in place — same row, same version — which is what - /// is for. + /// still-current source from a stale one, but only for a definition that carries no — + /// imported before that marker existed. Once the graph hash is present, it alone decides staleness and this + /// comparison is skipped: a version bump does not, by itself, mean the graph changed, and a metadata-only save + /// (a rename, a variable edit) that carries a definition from a published version N to draft N+1 must not be + /// judged stale on version alone when the graph the stored source describes has not moved. /// public const string SourceVersionCustomPropertyKey = "Bpmn:SourceVersion"; @@ -131,9 +132,12 @@ public sealed class BpmnInterchangeDocumentService( /// An unpublished draft is saved in place — same row, same version — so a designer save of the draft that edits /// a bound activity's inputs changes without changing /// 's own Version, which alone - /// cannot tell apart from no change at all. This marker closes that gap: and - /// also treat the stored source as stale when the current graph's hash differs from - /// the one recorded here, in addition to the existing version check. Computed with the same hashing + /// cannot tell apart from no change at all. Once this marker is present, + /// and decide staleness from it alone: the stored source is current exactly when the + /// current graph hashes to the value recorded here, whether or not the version has also changed. That matters + /// the other way around too — a metadata-only save (a rename, a variable edit) bumps a published definition to a + /// new draft version without touching the graph, and must not be judged stale on version alone once the graph + /// hash says the document still describes it exactly. Computed with the same hashing /// uses for the graph field, via /// , so the two never disagree about what "the graph changed" means. /// @@ -544,7 +548,24 @@ public sealed class BpmnInterchangeDocumentService( BpmnExportUnavailableReason.SourceVersionUnknown); } - if (sourceVersion != definition.Version) + // The graph hash, once recorded, is the sole word on staleness: it is unaffected by a metadata-only save + // (a rename, a variable change) that bumps the definition to a new draft version without touching the graph + // the stored source describes, which the version check below would otherwise flag as stale even though the + // document still matches exactly. A definition imported before this marker existed carries no value for it, + // so it falls back to the version check instead of refusing every definition imported under the older + // behaviour. + if (definition.CustomProperties.TryGetValue(SourceGraphHashCustomPropertyKey, out var sourceGraphHash) && !string.IsNullOrEmpty(sourceGraphHash)) + { + if (sourceGraphHash != BpmnContentHash.OfGraph(definition.StringData)) + { + throw new BpmnExportUnavailableException( + $"Workflow definition '{definition.DefinitionId}' has changed since it was imported from BPMN: its activity graph no longer matches " + + "the graph the stored source was imported against. The BPMN source stored on it no longer corresponds to this definition, so " + + "exporting it would silently return a document that is not what this definition currently is.", + BpmnExportUnavailableReason.SourceStale); + } + } + else if (sourceVersion != definition.Version) { throw new BpmnExportUnavailableException( $"Workflow definition '{definition.DefinitionId}' has changed since it was imported from BPMN (imported at version {sourceVersion}, " @@ -553,23 +574,6 @@ public sealed class BpmnInterchangeDocumentService( BpmnExportUnavailableReason.SourceStale); } - // The version check above catches a publish or any other save that assigns a new version, but an unpublished - // draft is saved in place — same row, same version — so a designer save of the draft (e.g. an edit to a bound - // activity's inputs) moves the graph without moving the version. SourceGraphHashCustomPropertyKey catches that: - // a definition imported before this marker existed carries no value for it, so it falls back to the - // version-only check above rather than refusing every definition imported under the older behaviour. - if (definition.CustomProperties.TryGetValue(SourceGraphHashCustomPropertyKey, out var sourceGraphHash) - && !string.IsNullOrEmpty(sourceGraphHash) - && sourceGraphHash != BpmnContentHash.OfGraph(definition.StringData)) - { - throw new BpmnExportUnavailableException( - $"Workflow definition '{definition.DefinitionId}' has changed since it was imported from BPMN: its activity graph no longer matches " - + "the graph the stored source was imported against, even though its version has not changed (an unpublished draft is saved in place). " - + "The BPMN source stored on it no longer corresponds to this definition, so exporting it would silently return a document that is " - + "not what this definition currently is.", - BpmnExportUnavailableReason.SourceStale); - } - return xml; } diff --git a/test/integration/Elsa.Bpmn.Interchange.IntegrationTests/Endpoints/BpmnInterchangeEndpointTests.cs b/test/integration/Elsa.Bpmn.Interchange.IntegrationTests/Endpoints/BpmnInterchangeEndpointTests.cs index 20ad2f823..02d32bd45 100644 --- a/test/integration/Elsa.Bpmn.Interchange.IntegrationTests/Endpoints/BpmnInterchangeEndpointTests.cs +++ b/test/integration/Elsa.Bpmn.Interchange.IntegrationTests/Endpoints/BpmnInterchangeEndpointTests.cs @@ -569,6 +569,30 @@ public class BpmnInterchangeEndpointTests(ITestOutputHelper testOutputHelper) : Assert.Null(metadataAfterImport.CustomPropertyValue); } + [Fact] + public async Task DocumentPut_AfterAMetadataOnlySaveCreatesANewDraftFromAPublishedVersion_SucceedsAndRecordsAFreshMarker() + { + var definitionId = await ImportCamundaOrderProcessAsync(); + await MarkLatestPublishedAsync(definitionId); + await RenameLatestDraftThroughTheDesignerAsync(definitionId, "Renamed through the designer"); + + // A metadata-only save through the designer path bumps a published definition to a new draft (N+1) without + // touching the graph, which is exactly the shape this issue is about: the document GET must not refuse that + // draft as stale, so the PUT that follows (W11's flow) has an ETag to send at all. + Assert.Equal(2, await LatestVersionOfAsync(definitionId)); + var (etag, documentJson) = await GetDocumentAsync(definitionId); + + var putResponse = await PutDocumentAsync(definitionId, WithFirstShapeMoved(documentJson), etag); + + Assert.Equal(HttpStatusCode.OK, putResponse.StatusCode); + + var stored = await FindLatestDefinitionAsync(definitionId); + Assert.True(stored.CustomProperties.TryGetValue(BpmnInterchangeDocumentService.SourceVersionCustomPropertyKey, out var sourceVersion)); + Assert.Equal(stored.Version, sourceVersion); + Assert.True(stored.CustomProperties.TryGetValue(BpmnInterchangeDocumentService.SourceGraphHashCustomPropertyKey, out var sourceGraphHash)); + Assert.False(string.IsNullOrEmpty(sourceGraphHash)); + } + [Fact] public async Task DocumentPut_WhenAuthenticatedWithoutTheRequiredPermission_ReturnsForbidden() { @@ -920,6 +944,34 @@ public class BpmnInterchangeEndpointTests(ITestOutputHelper testOutputHelper) : await store.SaveAsync(definition); } + /// + /// Marks the latest version of published, directly on the stored row. See + /// for why publish is simulated rather than real. + /// + private async Task MarkLatestPublishedAsync(string definitionId) + { + using var scope = _app!.Services.CreateScope(); + var store = scope.ServiceProvider.GetRequiredService(); + await PublishSimulation.MarkLatestPublishedAsync(store, definitionId); + } + + /// + /// Renames the latest draft of the way the workflow-definition save endpoint + /// Studio's designer calls does — then + /// — without touching the activity graph: a + /// metadata-only save, which carries a published definition to a new draft version without moving the graph the + /// stored BPMN source describes. + /// + private async Task RenameLatestDraftThroughTheDesignerAsync(string definitionId, string name) + { + using var scope = _app!.Services.CreateScope(); + var publisher = scope.ServiceProvider.GetRequiredService(); + var draft = await publisher.GetDraftAsync(definitionId, VersionOptions.Latest); + Assert.NotNull(draft); + draft!.Name = name; + await publisher.SaveDraftAsync(draft); + } + private async Task LatestVersionOfAsync(string definitionId) => (await LatestStoredAsync(definitionId)).Version; /// diff --git a/test/integration/Elsa.Bpmn.Interchange.IntegrationTests/Scenarios/Interchange/BpmnExportAvailabilityTests.cs b/test/integration/Elsa.Bpmn.Interchange.IntegrationTests/Scenarios/Interchange/BpmnExportAvailabilityTests.cs index 479241459..3a188cbf9 100644 --- a/test/integration/Elsa.Bpmn.Interchange.IntegrationTests/Scenarios/Interchange/BpmnExportAvailabilityTests.cs +++ b/test/integration/Elsa.Bpmn.Interchange.IntegrationTests/Scenarios/Interchange/BpmnExportAvailabilityTests.cs @@ -149,8 +149,8 @@ public class BpmnExportAvailabilityTests(ITestOutputHelper testOutputHelper) : B Assert.DoesNotContain("has changed since it was imported", exception.Message); } - [Fact(DisplayName = "Exporting a definition that changed since it was imported is refused, naming that as the reason rather than returning the pre-edit document")] - public async Task Export_OfADefinitionThatHasChangedSinceImport_IsRefused() + [Fact(DisplayName = "Exporting a definition whose version changed but whose graph did not is not refused: the graph hash, not the version, decides staleness")] + public async Task Export_OfADefinitionWhoseVersionChangedButGraphDidNot_StillSucceeds() { var xml = ReadAsset("camunda-order-process.bpmn"); var imported = await DocumentService.ImportAsync(xml, definitionId: null, name: null, processId: null, CancellationToken.None); @@ -158,8 +158,31 @@ public class BpmnExportAvailabilityTests(ITestOutputHelper testOutputHelper) : B var stored = await FindLatestAsync(imported.ImportResult.WorkflowDefinition.DefinitionId); - // Mirrors the case the source key survives a later save (unlike the test above), but the definition itself - // has moved on to a new version since the source was recorded — e.g. a publish followed by another edit. + // A version bump on its own — e.g. a publish followed by a metadata-only save such as a rename, which the + // designer path exercised end-to-end in Export_OfAPublishedDefinitionRenamedThroughTheDesigner_StillSucceeds + // below produces — never touches StringData, so the graph hash still matches. + stored.Version += 1; + await DefinitionStore.SaveAsync(stored); + + var refetched = await FindLatestAsync(stored.DefinitionId); + + var bytes = DocumentService.Export(refetched); + + Assert.NotEmpty(bytes); + } + + [Fact(DisplayName = "A definition imported before the graph-hash marker existed is still refused as stale on version alone")] + public async Task Export_OfADefinitionWithoutAGraphHashMarker_IsRefusedAsStaleWhenVersionChanges() + { + 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); + + var stored = await FindLatestAsync(imported.ImportResult.WorkflowDefinition.DefinitionId); + + // Simulates a definition imported before the graph-hash marker existed, whose version then moved on — + // e.g. a publish followed by another edit — without the marker present to say the graph did not. + stored.CustomProperties.Remove(BpmnInterchangeDocumentService.SourceGraphHashCustomPropertyKey); stored.Version += 1; await DefinitionStore.SaveAsync(stored); @@ -171,6 +194,43 @@ public class BpmnExportAvailabilityTests(ITestOutputHelper testOutputHelper) : B Assert.DoesNotContain("does not currently carry BPMN source", exception.Message); } + [Fact(DisplayName = "Exporting a definition published then renamed through the designer, with the same graph, still succeeds")] + public async Task Export_OfAPublishedDefinitionRenamedThroughTheDesigner_StillSucceeds() + { + var definitionId = await ImportThenPublishAsync(); + + // A metadata-only save through the designer path — a rename — bumps a published definition to a new draft + // (N+1) without touching the graph the stored BPMN source describes. + var draft = await DefinitionPublisher.GetDraftAsync(definitionId, VersionOptions.Latest); + Assert.NotNull(draft); + draft!.Name = "Renamed through the designer"; + await DefinitionPublisher.SaveDraftAsync(draft); + + var storedAfterRename = await FindLatestAsync(definitionId); + Assert.Equal(2, storedAfterRename.Version); + + var bytes = DocumentService.Export(storedAfterRename); + Assert.NotEmpty(bytes); + + var document = DocumentService.ReadDocument(storedAfterRename); + Assert.NotEmpty(document.Processes); + } + + [Fact(DisplayName = "Exporting a definition published then edited through the designer, with a changed graph, is refused as stale")] + public async Task Export_OfAPublishedDefinitionEditedThroughTheDesignerWithAChangedGraph_IsRefusedAsStale() + { + var definitionId = await ImportThenPublishAsync(); + + await SaveDraftFromTheDesignerAsync(definitionId); + + var storedAfterSave = await FindLatestAsync(definitionId); + Assert.Equal(2, storedAfterSave.Version); + + var exception = Assert.Throws(() => DocumentService.Export(storedAfterSave)); + + Assert.Contains("has changed since it was imported", exception.Message); + } + [Fact(DisplayName = "Exporting a definition that carries BPMN source without a recorded version is refused, distinctly from both other refusals")] public async Task Export_OfADefinitionCarryingSourceWithoutAVersionMarker_IsRefused() { @@ -246,6 +306,25 @@ public class BpmnExportAvailabilityTests(ITestOutputHelper testOutputHelper) : B return definition!; } + /// + /// Imports the standard fixture and marks the resulting draft published, so a test can go on to exercise a + /// designer save — + /// then — that carries the published version 1 to a + /// draft version 2, the shape a publish followed by any ordinary save takes. See + /// for why publish is simulated rather than real. + /// + private async Task ImportThenPublishAsync() + { + 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); + var definitionId = imported.ImportResult.WorkflowDefinition.DefinitionId; + + await PublishSimulation.MarkLatestPublishedAsync(DefinitionStore, definitionId); + + return definitionId; + } + /// /// Saves the latest draft of the way the workflow-definition save endpoint /// Studio's designer calls does — diff --git a/test/integration/Elsa.Bpmn.Interchange.IntegrationTests/Support/PublishSimulation.cs b/test/integration/Elsa.Bpmn.Interchange.IntegrationTests/Support/PublishSimulation.cs new file mode 100644 index 000000000..62454c8a7 --- /dev/null +++ b/test/integration/Elsa.Bpmn.Interchange.IntegrationTests/Support/PublishSimulation.cs @@ -0,0 +1,33 @@ +using Elsa.Common.Models; +using Elsa.Workflows.Management; +using Elsa.Workflows.Models; +using Xunit; + +namespace Elsa.Bpmn.Interchange.IntegrationTests.Support; + +/// +/// Marks the latest version of a definition published, directly on the stored row, without going through +/// — the one place every test +/// that needs a published-then-edited definition goes through, rather than each duplicating this shortcut. +/// +/// +/// The real publisher's ValidateWorkflowRequestHandler refuses a none-start BPMN process with "Trigger +/// should have a payload" (see elsa-workflows/elsa-core#8078), which the camunda-order-process.bpmn fixture these +/// tests otherwise read unmodified does not satisfy. Only "this row is the published version +/// branches on" matters to the tests that use this, so +/// this puts the definition in the same state a successful publish would without exercising that unrelated gate. +/// This helper should become a real +/// call once #8078 lands. +/// +internal static class PublishSimulation +{ + /// Marks the latest version of published, directly on the stored row. + public static async Task MarkLatestPublishedAsync(IWorkflowDefinitionStore store, string definitionId) + { + var filter = WorkflowDefinitionHandle.ByDefinitionId(definitionId, VersionOptions.Latest).ToFilter(); + var definition = await store.FindAsync(filter); + Assert.NotNull(definition); + definition!.IsPublished = true; + await store.SaveAsync(definition); + } +}