diff --git a/src/FluentCMS.Admin/Components/Pages/ChangeSetView.razor b/src/FluentCMS.Admin/Components/Pages/ChangeSetView.razor index 31f4a3b20..e0ca5c3e8 100644 --- a/src/FluentCMS.Admin/Components/Pages/ChangeSetView.razor +++ b/src/FluentCMS.Admin/Components/Pages/ChangeSetView.razor @@ -107,7 +107,7 @@ } - @if (set.State is ChangeSetState.AwaitingReview or ChangeSetState.ChecksFailed) + @if (set.State is ChangeSetState.AwaitingReview or ChangeSetState.ChecksFailed or ChangeSetState.Approved) { @L["Reject"] @@ -153,7 +153,7 @@ { } - @if (set.State is not (ChangeSetState.AwaitingReview or ChangeSetState.ChecksFailed)) + @if (set.State is not (ChangeSetState.AwaitingReview or ChangeSetState.ChecksFailed or ChangeSetState.Approved)) { } @@ -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."], diff --git a/src/FluentCMS.Core/ChangeSets/ChangeSetTransitions.cs b/src/FluentCMS.Core/ChangeSets/ChangeSetTransitions.cs index faa544cd5..26727f6e4 100644 --- a/src/FluentCMS.Core/ChangeSets/ChangeSetTransitions.cs +++ b/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), ]; diff --git a/src/FluentCMS.Infrastructure/ChangeSets/ChangeSetDirectory.cs b/src/FluentCMS.Infrastructure/ChangeSets/ChangeSetDirectory.cs index 960ef1153..05d60b915 100644 --- a/src/FluentCMS.Infrastructure/ChangeSets/ChangeSetDirectory.cs +++ b/src/FluentCMS.Infrastructure/ChangeSets/ChangeSetDirectory.cs @@ -225,6 +225,25 @@ public async Task> 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> PublishInScopeAsync( + IUnitOfWorkScope scope, + StoredChangeSet stored, + StoredChangeSet? reverted, + IReadOnlyList 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> 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> 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> 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; diff --git a/src/FluentCMS.Infrastructure/Persistence/DatabaseErrors.cs b/src/FluentCMS.Infrastructure/Persistence/DatabaseErrors.cs index b363171cf..d3aeb98c2 100644 --- a/src/FluentCMS.Infrastructure/Persistence/DatabaseErrors.cs +++ b/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; + } } diff --git a/tests/FluentCMS.IntegrationTests/ChangeSets/ChangeSetPublishTests.cs b/tests/FluentCMS.IntegrationTests/ChangeSets/ChangeSetPublishTests.cs index 48c7e6e56..268ddd939 100644 --- a/tests/FluentCMS.IntegrationTests/ChangeSets/ChangeSetPublishTests.cs +++ b/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] diff --git a/tests/FluentCMS.IntegrationTests/ChangeSets/EntryStateTests.cs b/tests/FluentCMS.IntegrationTests/ChangeSets/EntryStateTests.cs index 514aa60af..03fc4b4f7 100644 --- a/tests/FluentCMS.IntegrationTests/ChangeSets/EntryStateTests.cs +++ b/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] diff --git a/tests/FluentCMS.Tests/Admin/ChangeSetViewTests.cs b/tests/FluentCMS.Tests/Admin/ChangeSetViewTests.cs index 1422f9913..2dd99e981 100644 --- a/tests/FluentCMS.Tests/Admin/ChangeSetViewTests.cs +++ b/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])] diff --git a/tests/FluentCMS.Tests/ChangeSets/ChangeSetTransitionTests.cs b/tests/FluentCMS.Tests/ChangeSets/ChangeSetTransitionTests.cs index 937482ea0..516e9d19e 100644 --- a/tests/FluentCMS.Tests/ChangeSets/ChangeSetTransitionTests.cs +++ b/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); diff --git a/tests/FluentCMS.Tests/Identity/SystemInitiatorTests.cs b/tests/FluentCMS.Tests/Identity/SystemInitiatorTests.cs index 217e76049..0105442ec 100644 --- a/tests/FluentCMS.Tests/Identity/SystemInitiatorTests.cs +++ b/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)