Reject an approved change set and publish items in a stable order

src/FluentCMS.Admin/Components/Pages/ChangeSetView.razor

@@ -107,7 +107,7 @@
                         <Button Type="submit" data-action="publish" aria-keyshortcuts="P">@L["Publish"] <Kbd Class="bg-primary-foreground/20 text-primary-foreground" aria-hidden="true">P</Kbd></Button>
                     </form>
                 }
-                @if (set.State is ChangeSetState.AwaitingReview or ChangeSetState.ChecksFailed)
+                @if (set.State is ChangeSetState.AwaitingReview or ChangeSetState.ChecksFailed or ChangeSetState.Approved)
                 {
                     <Dialog>
                         <DialogTrigger Variant="outline" data-action="reject" aria-keyshortcuts="R">@L["Reject"] <Kbd aria-hidden="true">R</Kbd></DialogTrigger>
@@ -153,7 +153,7 @@
             {
                 <form method="post" @onsubmit="PublishAsync" @formname="publish" hidden></form>
             }
-            @if (set.State is not (ChangeSetState.AwaitingReview or ChangeSetState.ChecksFailed))
+            @if (set.State is not (ChangeSetState.AwaitingReview or ChangeSetState.ChecksFailed or ChangeSetState.Approved))
             {
                 <form method="post" @onsubmit="RejectAsync" @formname="reject" hidden></form>
             }
@@ -578,7 +578,7 @@
     {
         ChangeSetState.AwaitingReview => L["Approve the set, or reject it with a reason."],
         ChangeSetState.ChecksFailed => L["The checks failed. Whoever opened the set can fix and resubmit it, or you can reject it with a reason."],
-        ChangeSetState.Approved => L["The set is approved. Publishing it makes all its items live together."],
+        ChangeSetState.Approved => L["The set is approved. Publishing it makes all its items live together; if it can no longer publish, reject it with a reason."],
         ChangeSetState.Published => L["The set is live. Reverting it opens a set that undoes it, which is reviewed like any other."],
         ChangeSetState.Open => L["The set is still open. Submit it when it holds every change; the checks run, then a person reviews it."],
         ChangeSetState.ChecksRunning => L["The checks are running, so it cannot be reviewed yet."],

src/FluentCMS.Core/ChangeSets/ChangeSetTransitions.cs

@@ -17,6 +17,9 @@ public static class ChangeSetTransitions
         (ChangeSetState.AwaitingReview, ChangeSetState.Approved),
         (ChangeSetState.AwaitingReview, ChangeSetState.Rejected),
         (ChangeSetState.Approved, ChangeSetState.Published),
+
+        // An approved set that can no longer publish, such as one whose base another set published first, is cleared by a person.
+        (ChangeSetState.Approved, ChangeSetState.Rejected),
         (ChangeSetState.Published, ChangeSetState.Reverted),
     ];
 

src/FluentCMS.Infrastructure/ChangeSets/ChangeSetDirectory.cs

@@ -225,6 +225,25 @@ public async Task<Result<ChangeSet>> PublishAsync(Guid changeSetId, Cancellation
         }
 
         await using var scope = await unitOfWork.BeginAsync(cancellationToken);
+        try
+        {
+            return await PublishInScopeAsync(scope, stored, reverted, items.Value, cancellationToken);
+        }
+        catch (Exception exception) when (DatabaseErrors.IsConcurrencyFailure(exception))
+        {
+            // The scope rolls back when it is disposed.
+            return Conflict(set.Id);
+        }
+    }
+
+    private async Task<Result<ChangeSet>> PublishInScopeAsync(
+        IUnitOfWorkScope scope,
+        StoredChangeSet stored,
+        StoredChangeSet? reverted,
+        IReadOnlyList<PublishableItem> items,
+        CancellationToken cancellationToken)
+    {
+        var set = stored.ChangeSet;
         if (!await UpdateAsync(stored.Revision, set, cancellationToken))
         {
             return Conflict(set.Id);
@@ -235,7 +254,9 @@ public async Task<Result<ChangeSet>> PublishAsync(Guid changeSetId, Cancellation
             return Conflict(reverted.ChangeSet.Id);
         }
 
-        foreach (var item in items.Value)
+        // Items move by kind and id, whatever order the set lists them in, so two sets that publish the same items take their row
+        // locks in the same order and one waits for the other instead of deadlocking. The events keep the set's order.
+        foreach (var item in items.OrderBy(item => item.Operation.ItemKind).ThenBy(item => item.Operation.ItemId))
         {
             // The scope rolls back the items moved before this one.
             if (!await MovePublishedVersionAsync(item.Operation, set.RevertsChangeSetId is not null, cancellationToken))
@@ -244,7 +265,10 @@ public async Task<Result<ChangeSet>> PublishAsync(Guid changeSetId, Cancellation
                     ChangeSetErrorCodes.BaseVersionStale,
                     $"Another change set published {item.Name} at the same time, so it is no longer at the version this change set was based on.");
             }
+        }
 
+        foreach (var item in items)
+        {
             var subjectType = ItemSubjectType(item.Operation.ItemKind);
             await auditLog.AppendAsync(
                 new NewAuditEvent(item.Operation.Kind == OperationKind.Delete ? $"{subjectType}.unpublished" : $"{subjectType}.published")
@@ -1220,7 +1244,8 @@ private static async Task<Result<ChangeSet>> CommitAsync(
         {
             await scope.CommitAsync(cancellationToken);
         }
-        catch (DbUpdateException exception) when (DatabaseErrors.IsUniqueViolation(exception))
+        catch (Exception exception) when (exception is DbUpdateException update && DatabaseErrors.IsUniqueViolation(update)
+            || DatabaseErrors.IsConcurrencyFailure(exception))
         {
             // The scope rolls back when the caller disposes it.
             return conflict;

src/FluentCMS.Infrastructure/Persistence/DatabaseErrors.cs

@@ -21,4 +21,12 @@ public static bool IsUniqueViolation(DbUpdateException exception)
             _ => false,
         };
     }
+
+    // PostgreSQL aborts one of two writers that each wait for a row the other holds, or a serializable transaction that read what
+    // another one changed; the aborted writer answers as it would had the other committed first.
+    public static bool IsConcurrencyFailure(Exception exception)
+    {
+        var postgres = exception as PostgresException ?? exception.InnerException as PostgresException;
+        return postgres?.SqlState is PostgresErrorCodes.DeadlockDetected or PostgresErrorCodes.SerializationFailure;
+    }
 }

tests/FluentCMS.IntegrationTests/ChangeSets/ChangeSetPublishTests.cs

@@ -227,6 +227,35 @@ public async Task Of_two_concurrent_publishes_over_one_item_one_wins_and_the_oth
         Assert.Equal(loserEvents, (await scenario.ChangeSetEventsAsync(loser.Id)).Count);
     }
 
+    // Items move in one order whatever order the sets list them in, so the later publish waits for the earlier one and finds its
+    // base gone, instead of the two deadlocking on each other's rows.
+    [Theory]
+    [InlineData(DatabaseProviders.Sqlite)]
+    [PostgresData]
+    public async Task Two_sets_that_list_the_same_items_in_opposite_orders_publish_at_once_and_one_is_stale(string provider)
+    {
+        await using var scenario = await ScenarioAsync(provider);
+        for (var round = 0; round < 5; round++)
+        {
+            var product = await scenario.ContentTypeAsync($"product-{round}", revised: true);
+            var category = await scenario.ContentTypeAsync($"category-{round}", revised: true);
+            await scenario.PublishAsync(Create(product), Create(category));
+            var forward = await scenario.ApproveAsync(Update(product), Update(category));
+            var backward = await scenario.ApproveAsync(Update(category), Update(product));
+
+            var results = await Task.WhenAll(
+                Task.Run(() => scenario.RunAsync(directory => directory.PublishAsync(forward.Id))),
+                Task.Run(() => scenario.RunAsync(directory => directory.PublishAsync(backward.Id))));
+
+            Assert.Single(results, result => result.IsSuccess);
+            var lost = Assert.Single(results, result => !result.IsSuccess);
+            Assert.NotEmpty(lost.Errors);
+            Assert.All(lost.Errors, error => Assert.Equal(ChangeSetErrorCodes.BaseVersionStale, error.Code));
+            Assert.Equal(2, await scenario.PublishedVersionAsync(product.Id));
+            Assert.Equal(2, await scenario.PublishedVersionAsync(category.Id));
+        }
+    }
+
     [Theory]
     [InlineData(DatabaseProviders.Sqlite)]
     [PostgresData]

tests/FluentCMS.IntegrationTests/ChangeSets/EntryStateTests.cs

@@ -1,6 +1,7 @@
 using FluentCMS.Core.ChangeSets;
 using FluentCMS.Core.Content;
 using FluentCMS.Infrastructure.Persistence;
+using Microsoft.EntityFrameworkCore;
 using static FluentCMS.IntegrationTests.ChangeSets.EntryChangeSetTests;
 
 namespace FluentCMS.IntegrationTests.ChangeSets;
@@ -65,6 +66,39 @@ [new EntrySummary(page, 2, 1, EntryState.Published)],
             (await EntriesAsync(scenario, entries => entries.ListAsync("page"))).Value);
     }
 
+    // Two reverts of one set can both be approved. Once one publishes the other is stale, and a person's rejection clears it,
+    // which takes its entries out of review.
+    [Theory]
+    [InlineData(DatabaseProviders.Sqlite)]
+    [PostgresData]
+    public async Task A_stale_approved_revert_is_rejected_with_a_reason_and_its_entries_leave_review(string provider)
+    {
+        await using var scenario = await ScenarioAsync(provider);
+        var (page, _) = await CatalogAsync(scenario);
+        var published = await scenario.PublishAsync(Operation(OperationKind.Create, ItemKind.Entry, page, null, 1));
+        var first = (await scenario.RunAsync(directory => directory.RevertAsync(published.Id, "The page is not ready."))).Value;
+        var second = (await scenario.RunAsync(directory => directory.RevertAsync(published.Id, "The page is not ready."))).Value;
+        foreach (var revert in new[] { first, second })
+        {
+            await MoveAsync(scenario, revert.Id, ChangeSetState.ChecksRunning);
+            await MoveAsync(scenario, revert.Id, ChangeSetState.Approved);
+        }
+
+        Assert.True((await scenario.RunAsync(directory => directory.PublishAsync(first.Id))).IsSuccess);
+        var stale = await scenario.RunAsync(directory => directory.PublishAsync(second.Id));
+        Assert.Equal(ChangeSetErrorCodes.BaseVersionStale, Assert.Single(stale.Errors).Code);
+        Assert.Equal(EntryState.InReview, await StateAsync(scenario, page));
+
+        var blank = await scenario.RunAsync(directory => directory.RejectAsync(second.Id, " "));
+        var rejected = await scenario.RunAsync(directory => directory.RejectAsync(second.Id, "The first revert already took the page down."));
+
+        Assert.Equal(ChangeSetErrorCodes.RejectionReasonMissing, Assert.Single(blank.Errors).Code);
+        Assert.Equal(ChangeSetState.Rejected, rejected.Value.State);
+        Assert.Equal(EntryState.Reverted, await StateAsync(scenario, page));
+        Assert.Equal(ChangeSetState.Reverted, (await scenario.ChangeSetRowAsync(published.Id)).State);
+        Assert.Equal(1, await scenario.QueryAsync(context => context.AuditEvents.CountAsync(row => row.Operation == "change_set.reverted")));
+    }
+
     [Theory]
     [InlineData(DatabaseProviders.Sqlite)]
     [PostgresData]

tests/FluentCMS.Tests/Admin/ChangeSetViewTests.cs

@@ -130,7 +130,7 @@ public void A_set_still_being_checked_shows_the_job_progress()
     [Theory]
     [InlineData(ChangeSetState.AwaitingReview, new[] { "approve", "reject" })]
     [InlineData(ChangeSetState.ChecksFailed, new[] { "reject" })]
-    [InlineData(ChangeSetState.Approved, new[] { "publish" })]
+    [InlineData(ChangeSetState.Approved, new[] { "publish", "reject" })]
     [InlineData(ChangeSetState.Published, new[] { "revert" })]
     [InlineData(ChangeSetState.Open, new[] { "submit" })]
     [InlineData(ChangeSetState.ChecksRunning, new string[0])]

tests/FluentCMS.Tests/ChangeSets/ChangeSetTransitionTests.cs

@@ -43,6 +43,7 @@ public void The_table_holds_exactly_the_chg_04_transitions()
                 (ChangeSetState.AwaitingReview, ChangeSetState.Approved),
                 (ChangeSetState.AwaitingReview, ChangeSetState.Rejected),
                 (ChangeSetState.Approved, ChangeSetState.Published),
+                (ChangeSetState.Approved, ChangeSetState.Rejected),
                 (ChangeSetState.Published, ChangeSetState.Reverted),
             ],
             ChangeSetTransitions.All);
@@ -102,7 +103,6 @@ public void Every_transition_outside_the_table_is_refused_and_changes_nothing(Ch
     [Theory]
     [InlineData(ChangeSetState.Open)]
     [InlineData(ChangeSetState.ChecksRunning)]
-    [InlineData(ChangeSetState.Approved)]
     [InlineData(ChangeSetState.Published)]
     [InlineData(ChangeSetState.Rejected)]
     [InlineData(ChangeSetState.Reverted)]
@@ -136,6 +136,7 @@ public void A_refusal_names_the_states_the_set_can_move_to()
     [Theory]
     [InlineData(ChangeSetState.ChecksFailed)]
     [InlineData(ChangeSetState.AwaitingReview)]
+    [InlineData(ChangeSetState.Approved)]
     public void A_rejection_without_a_reason_is_refused(ChangeSetState from)
     {
         var changeSet = TestChangeSets.InState(from);

tests/FluentCMS.Tests/Identity/SystemInitiatorTests.cs

@@ -93,6 +93,7 @@ public SystemPermissionTests()
     [InlineData(ChangeSetState.ChecksFailed, ChangeSetState.Open, false)]
     [InlineData(ChangeSetState.AwaitingReview, ChangeSetState.Approved, false)]
     [InlineData(ChangeSetState.Approved, ChangeSetState.Published, false)]
+    [InlineData(ChangeSetState.Approved, ChangeSetState.Rejected, false)]
     [InlineData(ChangeSetState.ChecksFailed, ChangeSetState.Rejected, false)]
     [InlineData(ChangeSetState.Published, ChangeSetState.Reverted, false)]
     public async Task The_check_engine_makes_only_the_moves_it_owns(ChangeSetState from, ChangeSetState to, bool allowed)