From b3659a783d9e4528ab17b7fa852a62b9e950b513 Mon Sep 17 00:00:00 2001 From: James Gunn Date: Thu, 19 Dec 2024 09:36:43 +0000 Subject: [PATCH] Prevent racy updates (#1756) --- .../src/TeachingRecordSystem.Api/Program.cs | 4 +++- .../HostApplicationBuilderExtensions.cs | 22 ++++++++++++++----- .../Filters/CheckAlertExistsFilter.cs | 3 ++- ...CheckMandatoryQualificationExistsFilter.cs | 3 ++- .../Filters/CheckPersonExistsFilter.cs | 22 ++++++++++++++----- .../Filters/CheckSupportTaskExistsFilter.cs | 4 +++- .../PageContextFeatures.cs | 10 --------- .../Alerts/CloseAlert/CheckAnswers.cshtml.cs | 3 +-- .../ConnectOneLoginUser/Connect.cshtml.cs | 1 - .../TeachingRecordSystem.SupportUi/Program.cs | 5 ++++- 10 files changed, 48 insertions(+), 29 deletions(-) diff --git a/TeachingRecordSystem/src/TeachingRecordSystem.Api/Program.cs b/TeachingRecordSystem/src/TeachingRecordSystem.Api/Program.cs index ae90820cf..b030e6447 100644 --- a/TeachingRecordSystem/src/TeachingRecordSystem.Api/Program.cs +++ b/TeachingRecordSystem/src/TeachingRecordSystem.Api/Program.cs @@ -31,6 +31,7 @@ using TeachingRecordSystem.Core.Services.GetAnIdentityApi; using TeachingRecordSystem.Core.Services.NameSynonyms; using TeachingRecordSystem.Core.Services.TrnGenerationApi; +using TeachingRecordSystem.Core.Services.TrsDataSync; using TeachingRecordSystem.Core.Services.Webhooks; using TeachingRecordSystem.ServiceDefaults; using TeachingRecordSystem.ServiceDefaults.Infrastructure.Logging; @@ -226,7 +227,8 @@ public static void Main(string[] args) .AddIdentityApi() .AddNameSynonyms() .AddDqtOutboxMessageSerializer() - .AddWebhookOptions(); + .AddWebhookOptions() + .AddTrsSyncHelper(); services.AddAccessYourTeachingQualificationsOptions(configuration, env); services.AddCertificateGeneration(); diff --git a/TeachingRecordSystem/src/TeachingRecordSystem.Core/Services/TrsDataSync/HostApplicationBuilderExtensions.cs b/TeachingRecordSystem/src/TeachingRecordSystem.Core/Services/TrsDataSync/HostApplicationBuilderExtensions.cs index 9ebb51e7d..0ce86ac29 100644 --- a/TeachingRecordSystem/src/TeachingRecordSystem.Core/Services/TrsDataSync/HostApplicationBuilderExtensions.cs +++ b/TeachingRecordSystem/src/TeachingRecordSystem.Core/Services/TrsDataSync/HostApplicationBuilderExtensions.cs @@ -2,7 +2,6 @@ using Microsoft.Extensions.DependencyInjection; using Microsoft.Extensions.DependencyInjection.Extensions; using Microsoft.Extensions.Hosting; -using Microsoft.Extensions.Options; using Microsoft.PowerPlatform.Dataverse.Client; using TeachingRecordSystem.Core.Dqt; @@ -12,6 +11,8 @@ public static class HostApplicationBuilderExtensions { public static IHostApplicationBuilder AddTrsSyncService(this IHostApplicationBuilder builder) { + builder.AddTrsSyncHelper(); + var runService = builder.Configuration.GetValue("TrsSyncService:RunService"); builder.Services.AddOptions() @@ -24,12 +25,21 @@ public static IHostApplicationBuilder AddTrsSyncService(this IHostApplicationBui builder.Services.AddSingleton(); } - builder.Services.AddNamedServiceClient( - TrsDataSyncService.CrmClientName, - ServiceLifetime.Singleton, - sp => new ServiceClient(sp.GetRequiredService>().Value.CrmConnectionString)); + return builder; + } - builder.Services.AddCrmEntityChangesService(name: TrsDataSyncService.CrmClientName); + public static IHostApplicationBuilder AddTrsSyncHelper(this IHostApplicationBuilder builder) + { + if (!builder.Environment.IsUnitTests() && !builder.Environment.IsEndToEndTests()) + { + builder.Services.AddNamedServiceClient( + TrsDataSyncService.CrmClientName, + ServiceLifetime.Singleton, + sp => new ServiceClient(sp.GetRequiredService() + .GetRequiredValue("TrsSyncService:CrmConnectionString"))); + + builder.Services.AddCrmEntityChangesService(name: TrsDataSyncService.CrmClientName); + } builder.Services.TryAddSingleton(); diff --git a/TeachingRecordSystem/src/TeachingRecordSystem.SupportUi/Infrastructure/Filters/CheckAlertExistsFilter.cs b/TeachingRecordSystem/src/TeachingRecordSystem.SupportUi/Infrastructure/Filters/CheckAlertExistsFilter.cs index 1e3a9fc7c..0f6435445 100644 --- a/TeachingRecordSystem/src/TeachingRecordSystem.SupportUi/Infrastructure/Filters/CheckAlertExistsFilter.cs +++ b/TeachingRecordSystem/src/TeachingRecordSystem.SupportUi/Infrastructure/Filters/CheckAlertExistsFilter.cs @@ -28,10 +28,11 @@ public async Task OnResourceExecutionAsync(ResourceExecutingContext context, Res } var currentAlert = await dbContext.Alerts + .FromSql($"select * from alerts where alert_id = {alertId} for update") // https://github.com/dotnet/efcore/issues/26042 .Include(a => a.AlertType) .ThenInclude(at => at.AlertCategory) .Include(a => a.Person) - .SingleOrDefaultAsync(a => a.AlertId == alertId); + .SingleOrDefaultAsync(); if (currentAlert is null) { diff --git a/TeachingRecordSystem/src/TeachingRecordSystem.SupportUi/Infrastructure/Filters/CheckMandatoryQualificationExistsFilter.cs b/TeachingRecordSystem/src/TeachingRecordSystem.SupportUi/Infrastructure/Filters/CheckMandatoryQualificationExistsFilter.cs index 30e957775..e014cace9 100644 --- a/TeachingRecordSystem/src/TeachingRecordSystem.SupportUi/Infrastructure/Filters/CheckMandatoryQualificationExistsFilter.cs +++ b/TeachingRecordSystem/src/TeachingRecordSystem.SupportUi/Infrastructure/Filters/CheckMandatoryQualificationExistsFilter.cs @@ -24,9 +24,10 @@ public async Task OnResourceExecutionAsync(ResourceExecutingContext context, Res } var currentMq = await dbContext.MandatoryQualifications + .FromSql($"select * from qualifications where qualification_id = {qualificationId} for update") // https://github.com/dotnet/efcore/issues/26042 .Include(mq => mq.Provider) .Include(mq => mq.Person) - .SingleOrDefaultAsync(mq => mq.QualificationId == qualificationId); + .SingleOrDefaultAsync(); if (currentMq is null) { diff --git a/TeachingRecordSystem/src/TeachingRecordSystem.SupportUi/Infrastructure/Filters/CheckPersonExistsFilter.cs b/TeachingRecordSystem/src/TeachingRecordSystem.SupportUi/Infrastructure/Filters/CheckPersonExistsFilter.cs index 0b576e65a..1a11ffeb8 100644 --- a/TeachingRecordSystem/src/TeachingRecordSystem.SupportUi/Infrastructure/Filters/CheckPersonExistsFilter.cs +++ b/TeachingRecordSystem/src/TeachingRecordSystem.SupportUi/Infrastructure/Filters/CheckPersonExistsFilter.cs @@ -1,9 +1,10 @@ +using System.Diagnostics; using Microsoft.AspNetCore.Mvc; using Microsoft.AspNetCore.Mvc.Filters; using TeachingRecordSystem.Core.DataStore.Postgres; +using TeachingRecordSystem.Core.DataStore.Postgres.Models; using TeachingRecordSystem.Core.Dqt.Models; using TeachingRecordSystem.Core.Dqt.Queries; -using TeachingRecordSystem.Core.Jobs.Scheduling; using TeachingRecordSystem.Core.Services.TrsDataSync; namespace TeachingRecordSystem.SupportUi.Infrastructure.Filters; @@ -22,7 +23,7 @@ namespace TeachingRecordSystem.SupportUi.Infrastructure.Filters; public class CheckPersonExistsFilter( TrsDbContext dbContext, ICrmQueryDispatcher crmQueryDispatcher, - IBackgroundJobScheduler backgroundJobScheduler) : IAsyncResourceFilter + TrsDataSyncHelper syncHelper) : IAsyncResourceFilter { public async Task OnResourceExecutionAsync(ResourceExecutingContext context, ResourceExecutionDelegate next) { @@ -33,7 +34,7 @@ public async Task OnResourceExecutionAsync(ResourceExecutingContext context, Res return; } - var person = await dbContext.Persons.SingleOrDefaultAsync(p => p.PersonId == personId && p.DqtState == 0); + var person = await GetPersonAsync(); if (person is not null) { @@ -58,9 +59,16 @@ public async Task OnResourceExecutionAsync(ResourceExecutingContext context, Res if (dqtContact is not null) { - context.HttpContext.SetCurrentPersonFeature(dqtContact); + var synced = await syncHelper.SyncPersonAsync(personId, /*ignoreInvalid: */ false, /*dryRun:*/ false, CancellationToken.None); + if (!synced) + { + throw new Exception($"Could not sync Person with contact ID: '{personId}'."); + } - await backgroundJobScheduler.EnqueueAsync(helper => helper.SyncPersonAsync(personId, /*ignoreInvalid: */ false, /*dryRun:*/ false, CancellationToken.None)); + person = await GetPersonAsync(); + Debug.Assert(person is not null); + + context.HttpContext.SetCurrentPersonFeature(person); } else { @@ -70,6 +78,10 @@ public async Task OnResourceExecutionAsync(ResourceExecutingContext context, Res } await next(); + + Task GetPersonAsync() => dbContext.Persons + .FromSql($"select * from persons where person_id = {personId} and dqt_state = 0 for update") // https://github.com/dotnet/efcore/issues/26042 + .SingleOrDefaultAsync(); } } diff --git a/TeachingRecordSystem/src/TeachingRecordSystem.SupportUi/Infrastructure/Filters/CheckSupportTaskExistsFilter.cs b/TeachingRecordSystem/src/TeachingRecordSystem.SupportUi/Infrastructure/Filters/CheckSupportTaskExistsFilter.cs index 2d8184c71..1ad7081d0 100644 --- a/TeachingRecordSystem/src/TeachingRecordSystem.SupportUi/Infrastructure/Filters/CheckSupportTaskExistsFilter.cs +++ b/TeachingRecordSystem/src/TeachingRecordSystem.SupportUi/Infrastructure/Filters/CheckSupportTaskExistsFilter.cs @@ -14,7 +14,9 @@ public async Task OnResourceExecutionAsync(ResourceExecutingContext context, Res return; } - var currentSupportTask = await dbContext.SupportTasks.SingleOrDefaultAsync(t => t.SupportTaskReference == supportTaskReference); + var currentSupportTask = await dbContext.SupportTasks + .FromSql($"select * from support_tasks where support_task_reference = {supportTaskReference} for update") // https://github.com/dotnet/efcore/issues/26042 + .SingleOrDefaultAsync(); if (currentSupportTask is null || (supportTaskType is SupportTaskType type && currentSupportTask.SupportTaskType != type) || diff --git a/TeachingRecordSystem/src/TeachingRecordSystem.SupportUi/PageContextFeatures.cs b/TeachingRecordSystem/src/TeachingRecordSystem.SupportUi/PageContextFeatures.cs index a7a01bf2b..257448bbd 100644 --- a/TeachingRecordSystem/src/TeachingRecordSystem.SupportUi/PageContextFeatures.cs +++ b/TeachingRecordSystem/src/TeachingRecordSystem.SupportUi/PageContextFeatures.cs @@ -1,6 +1,5 @@ using Microsoft.AspNetCore.Http.Features; using TeachingRecordSystem.Core.DataStore.Postgres.Models; -using TeachingRecordSystem.Core.Dqt.Models; namespace TeachingRecordSystem.SupportUi; @@ -21,15 +20,6 @@ public static void SetCurrentPersonFeature(this HttpContext context, Person pers person.MiddleName, person.LastName)); - public static void SetCurrentPersonFeature(this HttpContext context, ContactDetail contactDetail) => - SetCurrentPersonFeature( - context, - new CurrentPersonFeature( - contactDetail.Contact.Id, - contactDetail.Contact.FirstName, - contactDetail.Contact.MiddleName ?? "", - contactDetail.Contact.LastName)); - public static CurrentMandatoryQualificationFeature GetCurrentMandatoryQualificationFeature(this HttpContext context) => context.Features.GetRequiredFeature(); diff --git a/TeachingRecordSystem/src/TeachingRecordSystem.SupportUi/Pages/Alerts/CloseAlert/CheckAnswers.cshtml.cs b/TeachingRecordSystem/src/TeachingRecordSystem.SupportUi/Pages/Alerts/CloseAlert/CheckAnswers.cshtml.cs index 1579c5c07..ad8d72fb6 100644 --- a/TeachingRecordSystem/src/TeachingRecordSystem.SupportUi/Pages/Alerts/CloseAlert/CheckAnswers.cshtml.cs +++ b/TeachingRecordSystem/src/TeachingRecordSystem.SupportUi/Pages/Alerts/CloseAlert/CheckAnswers.cshtml.cs @@ -51,8 +51,7 @@ public async Task OnPostAsync() { var now = clock.UtcNow; - var alert = await dbContext.Alerts - .SingleAsync(a => a.AlertId == AlertId); + var alert = HttpContext.GetCurrentAlertFeature().Alert; var oldAlertEventModel = EventModels.Alert.FromModel(alert); alert.EndDate = EndDate; diff --git a/TeachingRecordSystem/src/TeachingRecordSystem.SupportUi/Pages/SupportTasks/ConnectOneLoginUser/Connect.cshtml.cs b/TeachingRecordSystem/src/TeachingRecordSystem.SupportUi/Pages/SupportTasks/ConnectOneLoginUser/Connect.cshtml.cs index bcd2205a4..3671895ee 100644 --- a/TeachingRecordSystem/src/TeachingRecordSystem.SupportUi/Pages/SupportTasks/ConnectOneLoginUser/Connect.cshtml.cs +++ b/TeachingRecordSystem/src/TeachingRecordSystem.SupportUi/Pages/SupportTasks/ConnectOneLoginUser/Connect.cshtml.cs @@ -29,7 +29,6 @@ public void OnGet() public async Task OnPostAsync() { - dbContext.SupportTasks.Attach(_supportTask!); var data = (ConnectOneLoginUserData)_supportTask!.Data; _supportTask.Data = data with { diff --git a/TeachingRecordSystem/src/TeachingRecordSystem.SupportUi/Program.cs b/TeachingRecordSystem/src/TeachingRecordSystem.SupportUi/Program.cs index 46a7514a4..a2cfed576 100644 --- a/TeachingRecordSystem/src/TeachingRecordSystem.SupportUi/Program.cs +++ b/TeachingRecordSystem/src/TeachingRecordSystem.SupportUi/Program.cs @@ -17,6 +17,7 @@ using TeachingRecordSystem.Core.Infrastructure; using TeachingRecordSystem.Core.Services.Files; using TeachingRecordSystem.Core.Services.PersonMatching; +using TeachingRecordSystem.Core.Services.TrsDataSync; using TeachingRecordSystem.ServiceDefaults; using TeachingRecordSystem.ServiceDefaults.Infrastructure.Logging; using TeachingRecordSystem.SupportUi; @@ -152,7 +153,9 @@ } } -builder.AddBlobStorage(); +builder + .AddBlobStorage() + .AddTrsSyncHelper(); builder.Services .AddTrsBaseServices()