fix(bpmn): let the graph hash alone decide BPMN source staleness (#8079)

* fix(bpmn): let the graph hash alone decide BPMN source staleness

A metadata-only save (a rename, a variable edit) bumps a published definition
to a new draft version without touching the graph, but the stale check
compared the version unconditionally, refusing export/document GET even
though the graph the stored source describes had not moved. Once the
graph-hash marker is present it now decides staleness on its own; the version
check remains only as a fallback for definitions imported before that marker
existed.

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

* test(bpmn): share the simulated-publish helper

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

---------

Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
Sipke Schoorstra 2026-09-12 10:10:39 -07:00 committed by GitHub
parent 9f4d33269d
commit fcb46a6b6f
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
5 changed files with 220 additions and 48 deletions

View file

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

View file

@ -54,12 +54,13 @@ namespace Elsa.Bpmn.Interchange.Services;
/// in the first place; <see cref="Export(WorkflowDefinition)"/> 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. <see cref="SourceVersionCustomPropertyKey"/>
/// 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. <see cref="SourceGraphHashCustomPropertyKey"/> 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: <see cref="SourceGraphHashCustomPropertyKey"/> 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. <see cref="SourceVersionCustomPropertyKey"/> is what a definition imported
/// before the graph hash existed falls back to.
/// </para>
/// <para>
/// <b>A whole-definition import and a document edit disagree about what else gets replaced.</b> <see cref="ImportAsync"/>
@ -101,11 +102,11 @@ public sealed class BpmnInterchangeDocumentService(
/// </summary>
/// <remarks>
/// <see cref="Export(WorkflowDefinition)"/> compares this against the definition's current version to tell a
/// still-current source from a stale one. The version number is what <see cref="WorkflowDefinition"/> 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
/// <see cref="SourceGraphHashCustomPropertyKey"/> is for.
/// still-current source from a stale one, but only for a definition that carries no <see cref="SourceGraphHashCustomPropertyKey"/> —
/// 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.
/// </remarks>
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 <see cref="WorkflowDefinition.StringData"/> without changing
/// <see cref="WorkflowDefinition"/>'s own <c>Version</c>, which <see cref="SourceVersionCustomPropertyKey"/> alone
/// cannot tell apart from no change at all. This marker closes that gap: <see cref="Export(WorkflowDefinition)"/> and
/// <see cref="ReadDocument"/> 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, <see cref="Export(WorkflowDefinition)"/>
/// and <see cref="ReadDocument"/> 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
/// <see cref="Elsa.Bpmn.Interchange.Endpoints.Bpmn.Document.BpmnDocumentETag"/> uses for the graph field, via
/// <see cref="BpmnContentHash"/>, so the two never disagree about what "the graph changed" means.
/// <para>
@ -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<string>(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<string>(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;
}

View file

@ -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<int>(BpmnInterchangeDocumentService.SourceVersionCustomPropertyKey, out var sourceVersion));
Assert.Equal(stored.Version, sourceVersion);
Assert.True(stored.CustomProperties.TryGetValue<string>(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);
}
/// <summary>
/// Marks the latest version of <paramref name="definitionId"/> published, directly on the stored row. See
/// <see cref="PublishSimulation.MarkLatestPublishedAsync"/> for why publish is simulated rather than real.
/// </summary>
private async Task MarkLatestPublishedAsync(string definitionId)
{
using var scope = _app!.Services.CreateScope();
var store = scope.ServiceProvider.GetRequiredService<IWorkflowDefinitionStore>();
await PublishSimulation.MarkLatestPublishedAsync(store, definitionId);
}
/// <summary>
/// Renames the latest draft of <paramref name="definitionId"/> the way the workflow-definition save endpoint
/// Studio's designer calls does — <see cref="IWorkflowDefinitionPublisher.GetDraftAsync"/> then
/// <see cref="IWorkflowDefinitionPublisher.SaveDraftAsync"/> — 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.
/// </summary>
private async Task RenameLatestDraftThroughTheDesignerAsync(string definitionId, string name)
{
using var scope = _app!.Services.CreateScope();
var publisher = scope.ServiceProvider.GetRequiredService<IWorkflowDefinitionPublisher>();
var draft = await publisher.GetDraftAsync(definitionId, VersionOptions.Latest);
Assert.NotNull(draft);
draft!.Name = name;
await publisher.SaveDraftAsync(draft);
}
private async Task<int> LatestVersionOfAsync(string definitionId) => (await LatestStoredAsync(definitionId)).Version;
/// <summary>

View file

@ -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<BpmnExportUnavailableException>(() => 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!;
}
/// <summary>
/// Imports the standard fixture and marks the resulting draft published, so a test can go on to exercise a
/// designer save — <see cref="IWorkflowDefinitionPublisher.GetDraftAsync(string, VersionOptions, CancellationToken)"/>
/// then <see cref="IWorkflowDefinitionPublisher.SaveDraftAsync"/> — that carries the published version 1 to a
/// draft version 2, the shape a publish followed by any ordinary save takes. See
/// <see cref="PublishSimulation.MarkLatestPublishedAsync"/> for why publish is simulated rather than real.
/// </summary>
private async Task<string> 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;
}
/// <summary>
/// Saves the latest draft of <paramref name="definitionId"/> the way the workflow-definition save endpoint
/// Studio's designer calls does — <see cref="IWorkflowDefinitionPublisher.GetDraftAsync(string, VersionOptions, CancellationToken)"/>

View file

@ -0,0 +1,33 @@
using Elsa.Common.Models;
using Elsa.Workflows.Management;
using Elsa.Workflows.Models;
using Xunit;
namespace Elsa.Bpmn.Interchange.IntegrationTests.Support;
/// <summary>
/// Marks the latest version of a definition published, directly on the stored row, without going through
/// <see cref="IWorkflowDefinitionPublisher.PublishAsync(string, CancellationToken)"/> — the one place every test
/// that needs a published-then-edited definition goes through, rather than each duplicating this shortcut.
/// </summary>
/// <remarks>
/// The real publisher's <c>ValidateWorkflowRequestHandler</c> 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
/// <see cref="IWorkflowDefinitionPublisher.GetDraftAsync"/> 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 <see cref="IWorkflowDefinitionPublisher.PublishAsync(string, CancellationToken)"/>
/// call once #8078 lands.
/// </remarks>
internal static class PublishSimulation
{
/// <summary>Marks the latest version of <paramref name="definitionId"/> published, directly on the stored row.</summary>
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);
}
}