Merge pull request #778 from Project-MONAI/samrooke/AC-2008-workflow-manager-delete-payload

samrooke · web-flow · commit c97df309bce8 · 2023-05-04T11:59:18.000+01:00
Samrooke/ac 2008 workflow manager delete payload
diff --git a/src/WorkflowManager/Common/Interfaces/IPayloadService.cs b/src/WorkflowManager/Common/Interfaces/IPayloadService.cs
@@ -41,6 +41,12 @@ Task<IList<Payload>> GetAllAsync(int? skip = null,
                                          string? patientId = "",
                                          string? patientName = "");
 
+        /// <summary>
+        /// Deletes a payload by id.
+        /// </summary>
+        /// <param name="payloadId">payload id to delete.</param>
+        Task<bool> DeletePayloadFromStorageAsync(string payloadId);
+
         /// <summary>
         /// Updates a payload
         /// </summary>
diff --git a/src/WorkflowManager/Common/Services/PayloadService.cs b/src/WorkflowManager/Common/Services/PayloadService.cs
@@ -15,8 +15,11 @@
  */
 
 using Ardalis.GuardClauses;
+using Microsoft.Extensions.DependencyInjection;
 using Microsoft.Extensions.Logging;
 using Monai.Deploy.Messaging.Events;
+using Monai.Deploy.Storage.API;
+using Monai.Deploy.WorkflowManager.Common.Exceptions;
 using Monai.Deploy.WorkflowManager.Common.Interfaces;
 using Monai.Deploy.WorkflowManager.Contracts.Models;
 using Monai.Deploy.WorkflowManager.Database.Interfaces;
@@ -27,17 +30,28 @@ namespace Monai.Deploy.WorkflowManager.Common.Services
 {
     public class PayloadService : IPayloadService
     {
-        private readonly IPayloadRepsitory _payloadRepsitory;
+        private readonly IPayloadRepsitory _payloadRepository;
 
         private readonly IDicomService _dicomService;
 
+        private readonly IStorageService _storageService;
+
         private readonly ILogger<PayloadService> _logger;
 
-        public PayloadService(IPayloadRepsitory payloadRepsitory, IDicomService dicomService, ILogger<PayloadService> logger)
+        public PayloadService(
+            IPayloadRepsitory payloadRepsitory,
+            IDicomService dicomService,
+            IServiceScopeFactory serviceScopeFactory,
+            ILogger<PayloadService> logger)
         {
-            _payloadRepsitory = payloadRepsitory ?? throw new ArgumentNullException(nameof(payloadRepsitory));
+            _payloadRepository = payloadRepsitory ?? throw new ArgumentNullException(nameof(payloadRepsitory));
             _dicomService = dicomService ?? throw new ArgumentNullException(nameof(dicomService));
             _logger = logger ?? throw new ArgumentNullException(nameof(logger));
+
+            var _serviceScopeFactory = serviceScopeFactory ?? throw new ArgumentNullException(nameof(serviceScopeFactory));
+            var scope = _serviceScopeFactory.CreateScope();
+
+            _storageService = scope.ServiceProvider.GetService<IStorageService>() ?? throw new ArgumentNullException(nameof(IStorageService));
         }
 
         public async Task<Payload?> CreateAsync(WorkflowRequestEvent eventPayload)
@@ -70,7 +84,7 @@ public PayloadService(IPayloadRepsitory payloadRepsitory, IDicomService dicomSer
                     PatientDetails = patientDetails
                 };
 
-                if (await _payloadRepsitory.CreateAsync(payload))
+                if (await _payloadRepository.CreateAsync(payload))
                 {
                     _logger.PayloadCreated(payload.Id);
                     return payload;
@@ -92,23 +106,67 @@ public async Task<Payload> GetByIdAsync(string payloadId)
         {
             Guard.Against.NullOrWhiteSpace(payloadId);
 
-            return await _payloadRepsitory.GetByIdAsync(payloadId);
+            return await _payloadRepository.GetByIdAsync(payloadId);
         }
 
         public async Task<IList<Payload>> GetAllAsync(int? skip = null,
                                                       int? limit = null,
                                                       string? patientId = "",
                                                       string? patientName = "")
-            => await _payloadRepsitory.GetAllAsync(skip, limit, patientId, patientName);
+            => await _payloadRepository.GetAllAsync(skip, limit, patientId, patientName);
 
         public async Task<IList<Payload>> GetAllAsync(int? skip = null, int? limit = null)
-            => await _payloadRepsitory.GetAllAsync(skip, limit);
+            => await _payloadRepository.GetAllAsync(skip, limit);
+
+        public async Task<long> CountAsync() => await _payloadRepository.CountAsync();
+
+        public async Task<bool> DeletePayloadFromStorageAsync(string payloadId)
+        {
+            Guard.Against.NullOrWhiteSpace(payloadId);
+
+            var payload = await GetByIdAsync(payloadId);
+
+            if (payload is null)
+            {
+                throw new MonaiNotFoundException($"Payload with ID: {payloadId} not found");
+            }
+
+            if (payload.PayloadDeleted == PayloadDeleted.InProgress)
+            {
+                throw new MonaiBadRequestException($"Deletion of files for payload ID: {payloadId} already in progress");
+            }
+
+            // update the payload to in progress before we request deletion form MinIO
+            payload.PayloadDeleted = PayloadDeleted.InProgress;
+            await _payloadRepository.UpdateAsync(payload);
+
+            // run deletion in alternative thread so the user isn't held up
+#pragma warning disable CS4014 // Because this call is not awaited, execution of the current method continues before the call is completed
+            Task.Run(async () =>
+            {
+                try
+                {
+                    await _storageService.RemoveObjectsAsync(payload.Bucket, payload.Files.Select(f => f.Path));
+                    payload.PayloadDeleted = PayloadDeleted.Yes;
+                }
+                catch
+                {
+                    _logger.PayloadUpdateFailed(payloadId);
+                    payload.PayloadDeleted = PayloadDeleted.Failed;
+                }
+                finally
+                {
+                    await _payloadRepository.UpdateAsync(payload);
+                }
+            });
+#pragma warning restore CS4014 // Because this call is not awaited, execution of the current method continues before the call is completed
 
-        public async Task<long> CountAsync() => await _payloadRepsitory.CountAsync();
+            return true;
+        }
 
         public async Task<bool> UpdateWorkflowInstanceIdsAsync(string payloadId, IEnumerable<string> workflowInstances)
         {
-            if (await _payloadRepsitory.UpdateAssociatedWorkflowInstancesAsync(payloadId, workflowInstances))
+            if (await _payloadRepository.UpdateAssociatedWorkflowInstancesAsync(payloadId, workflowInstances))
             {
                 _logger.PayloadUpdated(payloadId);
                 return true;
diff --git a/src/WorkflowManager/Contracts/Migrations/M002_Payload_addPayloadDeleted.cs b/src/WorkflowManager/Contracts/Migrations/M002_Payload_addPayloadDeleted.cs
@@ -0,0 +1,43 @@
+﻿// 
+// Copyright 2023 Guy’s and St Thomas’ NHS Foundation Trust
+// 
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+// 
+// http://www.apache.org/licenses/LICENSE-2.0
+// 
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+using System;
+using Monai.Deploy.WorkflowManager.Contracts.Models;
+using Mongo.Migration.Migrations.Document;
+using MongoDB.Bson;
+
+namespace Monai.Deploy.WorkflowManager.Contracts.Migrations
+{
+    public class M002_Payload_addPayloadDeleted : DocumentMigration<Payload>
+    {
+        public M002_Payload_addPayloadDeleted() : base("1.0.1") { }
+
+        public override void Up(BsonDocument document)
+        {
+            document.Add("PayloadDeleted", PayloadDeleted.No);
+        }
+
+        public override void Down(BsonDocument document)
+        {
+            try
+            {
+                document.Remove("PayloadDeleted");
+            }
+            catch (Exception)
+            {
+            }
+        }
+    }
+}
diff --git a/src/WorkflowManager/Contracts/Models/Payload.cs b/src/WorkflowManager/Contracts/Models/Payload.cs
@@ -26,11 +26,11 @@
 
 namespace Monai.Deploy.WorkflowManager.Contracts.Models
 {
-    [CollectionLocation("Payloads"), RuntimeVersion("1.0.0")]
+    [CollectionLocation("Payloads"), RuntimeVersion("1.0.1")]
     public class Payload : IDocument
     {
         [JsonConverter(typeof(DocumentVersionConvert)), BsonSerializer(typeof(DocumentVersionConverBson))]
-        public DocumentVersion Version { get; set; } = new DocumentVersion(1, 0, 0);
+        public DocumentVersion Version { get; set; } = new DocumentVersion(1, 0, 1);
 
         [JsonProperty(PropertyName = "id")]
         public string Id { get; set; } = string.Empty;
@@ -62,10 +62,21 @@ public class Payload : IDocument
         [JsonProperty(PropertyName = "timestamp")]
         public DateTime Timestamp { get; set; }
 
+        [JsonProperty(PropertyName = "payload_deleted")]
+        public PayloadDeleted PayloadDeleted { get; set; } = PayloadDeleted.No;
+
         [JsonProperty(PropertyName = "files")]
         public IList<BlockStorageInfo> Files { get; set; } = new List<BlockStorageInfo>();
 
         [JsonProperty(PropertyName = "patient_details")]
         public PatientDetails PatientDetails { get; set; } = new PatientDetails();
     }
+
+    public enum PayloadDeleted
+    {
+        No,
+        Yes,
+        InProgress,
+        Failed
+    }
 }
diff --git a/src/WorkflowManager/Database/Interfaces/IPayloadRepsitory.cs b/src/WorkflowManager/Database/Interfaces/IPayloadRepsitory.cs
@@ -45,6 +45,13 @@ public interface IPayloadRepsitory
         /// <returns>Count of objects.</returns>
         Task<long> CountAsync();
 
+        /// <summary>
+        /// Updates a payload in the database.
+        /// </summary>
+        /// <param name="payload">The payload to update.</param>
+        /// <returns>The updated payload.</returns>
+        Task<bool> UpdateAsync(Payload payload);
+
         /// Updates a payload in the database.
         /// </summary>
         /// <param name="payloadId"></param>
diff --git a/src/WorkflowManager/Database/Repositories/PayloadRepository.cs b/src/WorkflowManager/Database/Repositories/PayloadRepository.cs
@@ -100,6 +100,25 @@ public async Task<Payload> GetByIdAsync(string payloadId)
             return payload;
         }
 
+        public async Task<bool> UpdateAsync(Payload payload)
+        {
+            Guard.Against.Null(payload, nameof(payload));
+
+            try
+            {
+                var filter = Builders<Payload>.Filter.Eq(p => p.PayloadId, payload.PayloadId);
+                var update = Builders<Payload>.Update.Set(p => p, payload);
+                await _payloadCollection.UpdateOneAsync(filter, update);
+
+                return true;
+            }
+            catch (Exception ex)
+            {
+                _logger.DbUpdatePayloadError(payload.PayloadId, ex);
+                return false;
+            }
+        }
+
         public async Task<bool> UpdateAssociatedWorkflowInstancesAsync(string payloadId, IEnumerable<string> workflowInstances)
         {
             Guard.Against.NullOrEmpty(workflowInstances);
diff --git a/src/WorkflowManager/Logging/Log.800000.Database.cs b/src/WorkflowManager/Logging/Log.800000.Database.cs
@@ -60,5 +60,8 @@ public static partial class Log
 
         [LoggerMessage(EventId = 800013, Level = LogLevel.Error, Message = "Database call failed in {methodName}.")]
         public static partial void DatabaseException(this ILogger logger, string methodName, Exception ex);
+
+        [LoggerMessage(EventId = 800014, Level = LogLevel.Error, Message = "Failed to update payload: '{payloadId}'.")]
+        public static partial void DbUpdatePayloadError(this ILogger logger, string payloadId, Exception ex);
     }
 }
diff --git a/src/WorkflowManager/WorkflowManager/Controllers/PayloadsController.cs b/src/WorkflowManager/WorkflowManager/Controllers/PayloadsController.cs
@@ -22,6 +22,7 @@
 using Microsoft.AspNetCore.Mvc;
 using Microsoft.Extensions.Logging;
 using Microsoft.Extensions.Options;
+using Monai.Deploy.WorkflowManager.Common.Exceptions;
 using Monai.Deploy.WorkflowManager.Common.Interfaces;
 using Monai.Deploy.WorkflowManager.Configuration;
 using Monai.Deploy.WorkflowManager.Contracts.Models;
@@ -140,5 +141,46 @@ public async Task<IActionResult> GetAsync([FromRoute] string id)
                 return Problem($"Unexpected error occurred: {e.Message}", $"/payload/{nameof(id)}", InternalServerError);
             }
         }
+
+        /// <summary>
+        /// Delete a payload by ID.
+        /// </summary>
+        /// <param name="id">The payload id.</param>
+        /// <returns>Boolean status of the success of the delete request.</returns>
+        [Route("{id}")]
+        [HttpDelete]
+        [ProducesResponseType(typeof(bool), StatusCodes.Status202Accepted)]
+        [ProducesResponseType(typeof(ProblemDetails), StatusCodes.Status404NotFound)]
+        [ProducesResponseType(typeof(ProblemDetails), StatusCodes.Status404NotFound)]
+        [ProducesResponseType(typeof(ProblemDetails), StatusCodes.Status500InternalServerError)]
+        public async Task<IActionResult> DeleteAsync([FromRoute] string id)
+        {
+            if (string.IsNullOrWhiteSpace(id) || !Guid.TryParse(id, out _))
+            {
+                _logger.LogDebug($"{nameof(DeleteAsync)} - Failed to validate {nameof(id)}");
+
+                return Problem($"Failed to validate {nameof(id)}, not a valid guid", $"/payload/{id}", BadRequest);
+            }
+
+            try
+            {
+                return Accepted(await _payloadService.DeletePayloadFromStorageAsync(id));
+            }
+            catch (MonaiBadRequestException e)
+            {
+                _logger.PayloadGetAsyncError(id, e);
+                return Problem(e.Message, $"/payload/{nameof(id)}", BadRequest);
+            }
+            catch (MonaiNotFoundException e)
+            {
+                _logger.PayloadGetAsyncError(id, e);
+                return Problem(e.Message, $"/payload/{nameof(id)}", NotFound);
+            }
+            catch (Exception e)
+            {
+                _logger.PayloadGetAsyncError(id, e);
+                return Problem($"Unexpected error occurred: {e.Message}", $"/payload/{nameof(id)}", InternalServerError);
+            }
+        }
     }
 }
diff --git a/src/WorkflowManager/WorkflowManager/Program.cs b/src/WorkflowManager/WorkflowManager/Program.cs
@@ -153,8 +153,6 @@ private static void ConfigureServices(HostBuilderContext hostContext, IServiceCo
             services.AddMonaiDeployMessageBrokerPublisherService(hostContext.Configuration.GetSection("WorkflowManager:messaging:publisherServiceAssemblyName").Value);
             services.AddMonaiDeployMessageBrokerSubscriberService(hostContext.Configuration.GetSection("WorkflowManager:messaging:subscriberServiceAssemblyName").Value);
 
-            services.AddHostedService(p => p.GetService<DataRetentionService>());
-
             services.AddWorkflowExecutor(hostContext);
 
             services.AddHttpContextAccessor();
diff --git a/tests/IntegrationTests/WorkflowExecutor.IntegrationTests/Support/WorkflowExecutorStartup.cs b/tests/IntegrationTests/WorkflowExecutor.IntegrationTests/Support/WorkflowExecutorStartup.cs
@@ -95,9 +95,7 @@ private static IHostBuilder CreateHostBuilder() =>
 
                 services.AddSingleton<DataRetentionService>();
 
-#pragma warning disable CS8603 // Possible null reference return.
-                services.AddHostedService<DataRetentionService>(p => p.GetService<DataRetentionService>());
-#pragma warning restore CS8603 // Possible null reference return.
+                services.AddHostedService(p => p.GetService<DataRetentionService>());
 
                 // Services
                 services.AddTransient<IFileSystem, FileSystem>();
@@ -107,16 +105,16 @@ private static IHostBuilder CreateHostBuilder() =>
                 services.Configure<WorkloadManagerDatabaseSettings>(hostContext.Configuration.GetSection("WorkloadManagerDatabase"));
                 services.Configure<ExecutionStatsDatabaseSettings>(hostContext.Configuration.GetSection("WorkloadManagerDatabase"));
                 services.AddSingleton<IMongoClient, MongoClient>(s => new MongoClient(hostContext.Configuration["WorkloadManagerDatabase:ConnectionString"]));
-                services.AddMigration(new MongoMigrationSettings
-                {
-                    ConnectionString = hostContext.Configuration.GetSection("WorkloadManagerDatabase:ConnectionString").Value,
-                    Database = hostContext.Configuration.GetSection("WorkloadManagerDatabase:DatabaseName").Value,
-                });
                 services.AddTransient<IWorkflowRepository, WorkflowRepository>();
                 services.AddTransient<IWorkflowInstanceRepository, WorkflowInstanceRepository>();
                 services.AddTransient<IPayloadRepsitory, PayloadRepository>();
                 services.AddTransient<ITasksRepository, TasksRepository>();
                 services.AddTransient<ITaskExecutionStatsRepository, TaskExecutionStatsRepository>();
+                services.AddMigration(new MongoMigrationSettings
+                {
+                    ConnectionString = hostContext.Configuration.GetSection("WorkloadManagerDatabase:ConnectionString").Value,
+                    Database = hostContext.Configuration.GetSection("WorkloadManagerDatabase:DatabaseName").Value,
+                });
 
                 // StorageService - Since mc.exe is unavailable during e2e, skip admin check
                 services.AddMonaiDeployStorageService(hostContext.Configuration.GetSection("WorkflowManager:storage:serviceAssemblyName").Value, HealthCheckOptions.ServiceHealthCheck);
diff --git a/tests/UnitTests/Common.Tests/Services/PayloadServiceTests.cs b/tests/UnitTests/Common.Tests/Services/PayloadServiceTests.cs
diff --git a/tests/UnitTests/WorkflowManager.Tests/Controllers/PayloadControllerTests.cs b/tests/UnitTests/WorkflowManager.Tests/Controllers/PayloadControllerTests.cs

Original file line number	Diff line number	Diff line change
`@@ -60,5 +60,8 @@ public static partial class Log`
`60`	`60`
`61`	`61`	`[LoggerMessage(EventId = 800013, Level = LogLevel.Error, Message = "Database call failed in {methodName}.")]`
`62`	`62`	`public static partial void DatabaseException(this ILogger logger, string methodName, Exception ex);`
	`63`	`+`
	`64`	`+ [LoggerMessage(EventId = 800014, Level = LogLevel.Error, Message = "Failed to update payload: '{payloadId}'.")]`
	`65`	`+ public static partial void DbUpdatePayloadError(this ILogger logger, string payloadId, Exception ex);`
`63`	`66`	`}`
`64`	`67`	`}`