From 646f1f780c806e751c7da12a3c7a8f6b4c904f2e Mon Sep 17 00:00:00 2001 From: Neil South Date: Tue, 9 Jan 2024 10:40:06 +0000 Subject: [PATCH 1/5] adding file deletion Signed-off-by: Neil South --- .../Configuration/WorkflowManagerOptions.cs | 3 + .../Common/Interfaces/IPayloadService.cs | 8 +++ ...Monai.Deploy.WorkflowManager.Common.csproj | 1 + .../Common/Services/PayloadService.cs | 36 +++++++++- .../Migrations/M004_Payload_expires.cs | 42 +++++++++++ .../M004_Workflow_addDataRetension.cs | 45 ++++++++++++ .../Contracts/Models/Payload.cs | 8 ++- .../Contracts/Models/Workflow.cs | 13 +++- .../Database/Interfaces/IPayloadRepository.cs | 17 +++++ .../Interfaces/IWorkflowRepository.cs | 1 - .../Repositories/PayloadRepository.cs | 55 +++++++++++++++ .../Logging/Log.800000.Database.cs | 3 + .../MonaiBackgroundService/Worker.cs | 67 ++++++++++++++++++ .../PayloadListener/packages.lock.json | 1 + .../Services/packages.lock.json | 1 + .../WorkflowExecuter/packages.lock.json | 1 + .../WorkflowManager/appsettings.json | 3 +- .../WorkflowManager/packages.lock.json | 1 + .../Services/PayloadServiceTests.cs | 69 ++++++++++++++++++- .../WorkerTests.cs | 26 ++++++- .../WorkflowManager.Tests/packages.lock.json | 1 + 21 files changed, 394 insertions(+), 8 deletions(-) create mode 100644 src/WorkflowManager/Contracts/Migrations/M004_Payload_expires.cs create mode 100644 src/WorkflowManager/Contracts/Migrations/M004_Workflow_addDataRetension.cs diff --git a/src/Common/Configuration/WorkflowManagerOptions.cs b/src/Common/Configuration/WorkflowManagerOptions.cs index 4ed820a4a..dce87277c 100644 --- a/src/Common/Configuration/WorkflowManagerOptions.cs +++ b/src/Common/Configuration/WorkflowManagerOptions.cs @@ -74,6 +74,9 @@ public class WorkflowManagerOptions : PagedOptions [ConfigurationKeyName("migExternalAppPlugins")] public string[] MigExternalAppPlugins { get; set; } + [ConfigurationKeyName("dataRetentionDays")] + public int DataRetentionDays { get; set; } + public WorkflowManagerOptions() { Messaging = new MessageBrokerConfiguration(); diff --git a/src/WorkflowManager/Common/Interfaces/IPayloadService.cs b/src/WorkflowManager/Common/Interfaces/IPayloadService.cs index 5362ffca9..6c28f99a0 100644 --- a/src/WorkflowManager/Common/Interfaces/IPayloadService.cs +++ b/src/WorkflowManager/Common/Interfaces/IPayloadService.cs @@ -54,5 +54,13 @@ Task> GetAllAsync(int? skip = null, /// /// Task UpdateWorkflowInstanceIdsAsync(string payloadId, IEnumerable workflowInstances); + + /// + /// Gets the expiry date for a payload. + /// + /// + /// + /// date of expiry or null + Task GetExpiry(DateTime now, string? workflowInstanceId); } } diff --git a/src/WorkflowManager/Common/Monai.Deploy.WorkflowManager.Common.csproj b/src/WorkflowManager/Common/Monai.Deploy.WorkflowManager.Common.csproj index f7db645d0..863da17c5 100644 --- a/src/WorkflowManager/Common/Monai.Deploy.WorkflowManager.Common.csproj +++ b/src/WorkflowManager/Common/Monai.Deploy.WorkflowManager.Common.csproj @@ -37,6 +37,7 @@ + diff --git a/src/WorkflowManager/Common/Services/PayloadService.cs b/src/WorkflowManager/Common/Services/PayloadService.cs index 72eb342f5..7fd62a216 100644 --- a/src/WorkflowManager/Common/Services/PayloadService.cs +++ b/src/WorkflowManager/Common/Services/PayloadService.cs @@ -25,6 +25,8 @@ using Monai.Deploy.WorkflowManager.Common.Database.Interfaces; using Monai.Deploy.WorkflowManager.Common.Logging; using Monai.Deploy.WorkflowManager.Common.Storage.Services; +using Microsoft.Extensions.Options; +using Monai.Deploy.WorkflowManager.Common.Configuration; namespace Monai.Deploy.WorkflowManager.Common.Miscellaneous.Services { @@ -34,23 +36,31 @@ public class PayloadService : IPayloadService private readonly IWorkflowInstanceRepository _workflowInstanceRepository; + private readonly IWorkflowRepository _workflowRepository; + private readonly IDicomService _dicomService; private readonly IStorageService _storageService; + private readonly WorkflowManagerOptions _options; + private readonly ILogger _logger; public PayloadService( IPayloadRepository payloadRepository, IDicomService dicomService, IWorkflowInstanceRepository workflowInstanceRepository, + IWorkflowRepository workflowRepository, IServiceScopeFactory serviceScopeFactory, + IOptions options, ILogger logger) { _payloadRepository = payloadRepository ?? throw new ArgumentNullException(nameof(payloadRepository)); _workflowInstanceRepository = workflowInstanceRepository ?? throw new ArgumentNullException(nameof(workflowInstanceRepository)); _dicomService = dicomService ?? throw new ArgumentNullException(nameof(dicomService)); _logger = logger ?? throw new ArgumentNullException(nameof(logger)); + _workflowRepository = workflowRepository ?? throw new ArgumentNullException(nameof(workflowRepository)); + _options = options?.Value ?? throw new ArgumentNullException(nameof(options)); var scopeFactory = serviceScopeFactory ?? throw new ArgumentNullException(nameof(serviceScopeFactory)); var scope = scopeFactory.CreateScope(); @@ -85,7 +95,8 @@ public PayloadService( DataTrigger = eventPayload.DataTrigger, Timestamp = eventPayload.Timestamp, PatientDetails = patientDetails, - PayloadDeleted = PayloadDeleted.No + PayloadDeleted = PayloadDeleted.No, + Expires = await GetExpiry(DateTime.UtcNow, eventPayload.WorkflowInstanceId) }; if (await _payloadRepository.CreateAsync(payload)) @@ -106,6 +117,29 @@ public PayloadService( return null; } + public async Task GetExpiry(DateTime now, string? workflowInstanceId) + { + var daysToKeep = await GetWorkflowDataExpiry(workflowInstanceId); + daysToKeep ??= _options.DataRetentionDays; + + if (daysToKeep == -1) { return null; } + + return now.AddDays(daysToKeep.Value); + } + + private async Task GetWorkflowDataExpiry(string? workflowInstanceId) + { + if (string.IsNullOrWhiteSpace(workflowInstanceId)) { return null; } + + var workflowInstance = await _workflowInstanceRepository.GetByWorkflowInstanceIdAsync(workflowInstanceId); + + if (workflowInstance is null) { return null; } + + var t = await _workflowRepository.GetByWorkflowIdAsync(workflowInstance.WorkflowId); + + return (await _workflowRepository.GetByWorkflowIdAsync(workflowInstance.WorkflowId))?.Workflow?.DataRetentionDays ?? null; + } + public async Task GetByIdAsync(string payloadId) { Guard.Against.NullOrWhiteSpace(payloadId, nameof(payloadId)); diff --git a/src/WorkflowManager/Contracts/Migrations/M004_Payload_expires.cs b/src/WorkflowManager/Contracts/Migrations/M004_Payload_expires.cs new file mode 100644 index 000000000..91fcff9bf --- /dev/null +++ b/src/WorkflowManager/Contracts/Migrations/M004_Payload_expires.cs @@ -0,0 +1,42 @@ +// +// 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 Monai.Deploy.WorkflowManager.Common.Contracts.Models; +using Mongo.Migration.Migrations.Document; +using MongoDB.Bson; + +namespace Monai.Deploy.WorkflowManager.Common.Contracts.Migrations +{ + public class M004_Payload_expires : DocumentMigration + { + public M004_Payload_expires() : base("1.0.4") { } + + public override void Up(BsonDocument document) + { + document.Add("Expires", BsonNull.Create(null).ToJson(), true); //null = never expires + } + + public override void Down(BsonDocument document) + { + try + { + document.Remove("DataTrigger"); + } + catch + { // can ignore we dont want failures stopping startup ! + } + } + } +} diff --git a/src/WorkflowManager/Contracts/Migrations/M004_Workflow_addDataRetension.cs b/src/WorkflowManager/Contracts/Migrations/M004_Workflow_addDataRetension.cs new file mode 100644 index 000000000..ea4d91f68 --- /dev/null +++ b/src/WorkflowManager/Contracts/Migrations/M004_Workflow_addDataRetension.cs @@ -0,0 +1,45 @@ +// +// 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 Monai.Deploy.WorkflowManager.Common.Contracts.Models; +using Mongo.Migration.Migrations.Document; +using MongoDB.Bson; + + +namespace Monai.Deploy.WorkflowManager.Common.Contracts.Migrations +{ + public class M004_Workflow_addDataRetension : DocumentMigration + { + public M004_Workflow_addDataRetension() : base("1.0.1") { } + + public override void Up(BsonDocument document) + { + // will also add version as it has a default + document.Add("DataRetentionDays", BsonNull.Create(null).ToJson(), true); + } + + public override void Down(BsonDocument document) + { + try + { + document.Remove("DataRetentionDays"); + document.Remove("Version"); + } + catch + { // can ignore we dont want failures stopping startup ! + } + } + } +} diff --git a/src/WorkflowManager/Contracts/Models/Payload.cs b/src/WorkflowManager/Contracts/Models/Payload.cs index 4164ed9a3..96033100e 100755 --- a/src/WorkflowManager/Contracts/Models/Payload.cs +++ b/src/WorkflowManager/Contracts/Models/Payload.cs @@ -27,11 +27,11 @@ namespace Monai.Deploy.WorkflowManager.Common.Contracts.Models { - [CollectionLocation("Payloads"), RuntimeVersion("1.0.3")] + [CollectionLocation("Payloads"), RuntimeVersion("1.0.4")] public class Payload : IDocument { [JsonConverter(typeof(DocumentVersionConvert)), BsonSerializer(typeof(DocumentVersionConverBson))] - public DocumentVersion Version { get; set; } = new DocumentVersion(1, 0, 3); + public DocumentVersion Version { get; set; } = new DocumentVersion(1, 0, 4); [JsonProperty(PropertyName = "id")] public string Id { get; set; } = string.Empty; @@ -67,6 +67,10 @@ public class Payload : IDocument public PatientDetails PatientDetails { get; set; } = new PatientDetails(); public DataOrigin DataTrigger { get; set; } = new DataOrigin { DataService = DataService.DIMSE }; + + [JsonProperty(PropertyName = "expires")] + public DateTime? Expires { get; set; } + } public enum PayloadDeleted diff --git a/src/WorkflowManager/Contracts/Models/Workflow.cs b/src/WorkflowManager/Contracts/Models/Workflow.cs index da6435c49..2851deab1 100755 --- a/src/WorkflowManager/Contracts/Models/Workflow.cs +++ b/src/WorkflowManager/Contracts/Models/Workflow.cs @@ -14,13 +14,21 @@ * limitations under the License. */ +using Monai.Deploy.WorkflowManager.Common.Contracts.Migrations; +using Mongo.Migration.Documents; +using Mongo.Migration.Documents.Attributes; +using MongoDB.Bson.Serialization.Attributes; using Newtonsoft.Json; namespace Monai.Deploy.WorkflowManager.Common.Contracts.Models { + [CollectionLocation("Workflows"), RuntimeVersion("1.0.1")] - public class Workflow + public class Workflow : IDocument { + [JsonConverter(typeof(DocumentVersionConvert)), BsonSerializer(typeof(DocumentVersionConverBson))] + DocumentVersion IDocument.Version { get; set; } = new DocumentVersion(1, 0, 1); + [JsonProperty(PropertyName = "name")] public string Name { get; set; } = string.Empty; @@ -36,5 +44,8 @@ public class Workflow [JsonProperty(PropertyName = "tasks")] public TaskObject[] Tasks { get; set; } = System.Array.Empty(); + [JsonProperty(PropertyName = "dataRetentionDays")] + public int? DataRetentionDays { get; set; } // note. -1 = never delete + } } diff --git a/src/WorkflowManager/Database/Interfaces/IPayloadRepository.cs b/src/WorkflowManager/Database/Interfaces/IPayloadRepository.cs index 61fed986d..92f55c37c 100644 --- a/src/WorkflowManager/Database/Interfaces/IPayloadRepository.cs +++ b/src/WorkflowManager/Database/Interfaces/IPayloadRepository.cs @@ -14,6 +14,7 @@ * limitations under the License. */ +using System; using System.Collections.Generic; using System.Threading.Tasks; using Monai.Deploy.WorkflowManager.Common.Contracts.Models; @@ -52,11 +53,27 @@ public interface IPayloadRepository /// The updated payload. Task UpdateAsync(Payload payload); + /// /// Updates a payload in the database. /// /// /// /// Task UpdateAssociatedWorkflowInstancesAsync(string payloadId, IEnumerable workflowInstances); + + /// + /// Gets all the payloads that might need deleted + /// + /// the current datetime + /// + Task> GetPayloadsToDelete(DateTime now); + + /// + /// Marks a bunch of payloads as a new deleted state + /// + /// a list of payloadIds to mark in new status + /// the status to mark as + /// + Task MarkDeletedState(IList Ids, PayloadDeleted status); } } diff --git a/src/WorkflowManager/Database/Interfaces/IWorkflowRepository.cs b/src/WorkflowManager/Database/Interfaces/IWorkflowRepository.cs index b8b8107be..c821753be 100644 --- a/src/WorkflowManager/Database/Interfaces/IWorkflowRepository.cs +++ b/src/WorkflowManager/Database/Interfaces/IWorkflowRepository.cs @@ -17,7 +17,6 @@ using System; using System.Collections.Generic; using System.Threading.Tasks; -using Monai.Deploy.Messaging.Events; using Monai.Deploy.WorkflowManager.Common.Contracts.Models; namespace Monai.Deploy.WorkflowManager.Common.Database.Interfaces diff --git a/src/WorkflowManager/Database/Repositories/PayloadRepository.cs b/src/WorkflowManager/Database/Repositories/PayloadRepository.cs index 78736de45..673099490 100644 --- a/src/WorkflowManager/Database/Repositories/PayloadRepository.cs +++ b/src/WorkflowManager/Database/Repositories/PayloadRepository.cs @@ -16,6 +16,7 @@ using System; using System.Collections.Generic; +using System.Linq; using System.Threading.Tasks; using Ardalis.GuardClauses; using Microsoft.Extensions.Logging; @@ -47,6 +48,28 @@ public PayloadRepository( _logger = logger ?? throw new ArgumentNullException(nameof(logger)); var mongoDatabase = client.GetDatabase(databaseSettings.Value.DatabaseName); _payloadCollection = mongoDatabase.GetCollection("Payloads"); + EnsureIndex().GetAwaiter().GetResult(); + } + + private async Task EnsureIndex() + { + var indexName = "PayloadDeletedIndex"; + + var model = new CreateIndexModel( + Builders.IndexKeys.Ascending(s => s.PayloadDeleted), + new CreateIndexOptions { Name = indexName } + ); + + + var asyncCursor = (await _payloadCollection.Indexes.ListAsync()); + var bsonDocuments = (await asyncCursor.ToListAsync()); + var indexes = bsonDocuments.Select(_ => _.GetElement("name").Value.ToString()).ToList(); + + // If index not present create it else skip. + if (!indexes.Any(i => i is not null && i.Equals(indexName))) + { + await _payloadCollection.Indexes.CreateOneAsync(model); + } } public Task CountAsync() => CountAsync(_payloadCollection, null); @@ -137,5 +160,37 @@ await _payloadCollection.FindOneAndUpdateAsync( return false; } } + + public async Task> GetPayloadsToDelete(DateTime now) + { + try + { + var filter = (Builders.Filter.Eq(p => p.PayloadDeleted, PayloadDeleted.No) | + Builders.Filter.Eq(p => p.PayloadDeleted, PayloadDeleted.Failed)) & + Builders.Filter.Lt(p => p.Expires, now); + + return await (await _payloadCollection.FindAsync(filter)).ToListAsync(); + + } + catch (Exception ex) + { + _logger.DbGetPayloadsToDeleteError(ex); + return new List(); + } + } + + public async Task MarkDeletedState(IList Ids, PayloadDeleted status) + { + try + { + var filter = Builders.Filter.In(p => p.PayloadId, Ids); + var update = Builders.Update.Set(p => p.PayloadDeleted, status); + await _payloadCollection.UpdateManyAsync(filter, update); + } + catch (Exception ex) + { + _logger.DbGetPayloadsToDeleteError(ex); + } + } } } diff --git a/src/WorkflowManager/Logging/Log.800000.Database.cs b/src/WorkflowManager/Logging/Log.800000.Database.cs index fc2a9c854..e67a57249 100644 --- a/src/WorkflowManager/Logging/Log.800000.Database.cs +++ b/src/WorkflowManager/Logging/Log.800000.Database.cs @@ -63,5 +63,8 @@ public static partial class Log [LoggerMessage(EventId = 800014, Level = LogLevel.Error, Message = "Failed to update payload: '{payloadId}'.")] public static partial void DbUpdatePayloadError(this ILogger logger, string payloadId, Exception ex); + + [LoggerMessage(EventId = 800015, Level = LogLevel.Error, Message = "Failed to get payloads to delete.")] + public static partial void DbGetPayloadsToDeleteError(this ILogger logger, Exception ex); } } diff --git a/src/WorkflowManager/MonaiBackgroundService/Worker.cs b/src/WorkflowManager/MonaiBackgroundService/Worker.cs index cd9de5368..bc36ea273 100644 --- a/src/WorkflowManager/MonaiBackgroundService/Worker.cs +++ b/src/WorkflowManager/MonaiBackgroundService/Worker.cs @@ -23,6 +23,8 @@ using Monai.Deploy.WorkflowManager.Common.Logging; using Monai.Deploy.WorkflowManager.Common.WorkflowExecuter.Common; using Monai.Deploy.WorkflowManager.MonaiBackgroundService.Logging; +using Monai.Deploy.WorkflowManager.Common.Database.Interfaces; +using Monai.Deploy.Storage.API; namespace Monai.Deploy.WorkflowManager.Common.MonaiBackgroundService { @@ -33,18 +35,24 @@ public class Worker : BackgroundService private readonly ITasksService _tasksService; private readonly IMessageBrokerPublisherService _publisherService; private readonly IOptions _options; + private readonly IPayloadRepository _payloadRepository; + private readonly IStorageService _storageService; public bool IsRunning { get; set; } = false; public Worker( ILogger logger, ITasksService tasksService, IMessageBrokerPublisherService publisherService, + IPayloadRepository payloadRepository, + IStorageService storageService, IOptions options) { _logger = logger ?? throw new ArgumentNullException(nameof(logger)); _tasksService = tasksService ?? throw new ArgumentNullException(nameof(tasksService)); _publisherService = publisherService ?? throw new ArgumentNullException(nameof(publisherService)); _options = options ?? throw new ArgumentNullException(nameof(options)); + _payloadRepository = payloadRepository ?? throw new ArgumentNullException(nameof(payloadRepository)); + _storageService = storageService ?? throw new ArgumentNullException(nameof(_storageService)); } public static string ServiceName => "Monai Background Service"; @@ -68,6 +76,12 @@ protected override async Task ExecuteAsync(CancellationToken stoppingToken) } public async Task DoWork() + { + await ProcessTimedoutTasks().ConfigureAwait(false); + await ProcessExpiredPayloads().ConfigureAwait(false); + } + + private async Task ProcessTimedoutTasks() { try { @@ -89,6 +103,59 @@ public async Task DoWork() } } + private async Task ProcessExpiredPayloads() + { + var payloads = new List(); + try + { + payloads = (await _payloadRepository.GetPayloadsToDelete(DateTime.UtcNow).ConfigureAwait(false)).ToList(); + + if (payloads.Any()) + { + var ids = payloads.Select(p => p.PayloadId).ToList(); + + await _payloadRepository.MarkDeletedState(ids, PayloadDeleted.InProgress).ConfigureAwait(false); + } + + } + catch (Exception e) + { + _logger.WorkerException(e.Message); + } + + try + { + await RemoveStoredFiles(payloads.ToList()); + } + catch (Exception e) + { + + _logger.WorkerException(e.Message); + } + } + + private async Task RemoveStoredFiles(List payloads) + { + var tasks = new List(); + + foreach (var payload in payloads) + { + var filepaths = (payload.Files.Select(f => f.Path)).ToList(); + + var all = await _storageService.ListObjectsAsync(payload.Bucket, payload.PayloadId, true); + + filepaths.AddRange(all.Select(f => f.FilePath)); + + foreach (var filepath in filepaths) + { + await _storageService.RemoveObjectAsync(payload.Bucket, filepath); + } + + tasks.Add(_payloadRepository.MarkDeletedState(new List { payload.PayloadId }, PayloadDeleted.Yes)); + } + await Task.WhenAll(tasks); + } + private async Task PublishCancellationEvent(TaskExecution task, string correlationId, string identity, string workflowInstanceId) { _logger.TimingOutTaskCancellationEvent(identity, task.WorkflowInstanceId); diff --git a/src/WorkflowManager/PayloadListener/packages.lock.json b/src/WorkflowManager/PayloadListener/packages.lock.json index d4e29c207..cc5f89061 100644 --- a/src/WorkflowManager/PayloadListener/packages.lock.json +++ b/src/WorkflowManager/PayloadListener/packages.lock.json @@ -771,6 +771,7 @@ "monai.deploy.workflowmanager.common": { "type": "Project", "dependencies": { + "Monai.Deploy.WorkflowManager.Common.Configuration": "[1.0.0, )", "Monai.Deploy.WorkflowManager.Contracts": "[1.0.0, )", "Monai.Deploy.WorkflowManager.Database": "[1.0.0, )", "Monai.Deploy.WorkflowManager.Storage": "[1.0.0, )" diff --git a/src/WorkflowManager/Services/packages.lock.json b/src/WorkflowManager/Services/packages.lock.json index e33b7f8dc..43e9a6d30 100644 --- a/src/WorkflowManager/Services/packages.lock.json +++ b/src/WorkflowManager/Services/packages.lock.json @@ -725,6 +725,7 @@ "monai.deploy.workflowmanager.common": { "type": "Project", "dependencies": { + "Monai.Deploy.WorkflowManager.Common.Configuration": "[1.0.0, )", "Monai.Deploy.WorkflowManager.Contracts": "[1.0.0, )", "Monai.Deploy.WorkflowManager.Database": "[1.0.0, )", "Monai.Deploy.WorkflowManager.Storage": "[1.0.0, )" diff --git a/src/WorkflowManager/WorkflowExecuter/packages.lock.json b/src/WorkflowManager/WorkflowExecuter/packages.lock.json index 1d9eac5c0..562b1c6ed 100644 --- a/src/WorkflowManager/WorkflowExecuter/packages.lock.json +++ b/src/WorkflowManager/WorkflowExecuter/packages.lock.json @@ -772,6 +772,7 @@ "monai.deploy.workflowmanager.common": { "type": "Project", "dependencies": { + "Monai.Deploy.WorkflowManager.Common.Configuration": "[1.0.0, )", "Monai.Deploy.WorkflowManager.Contracts": "[1.0.0, )", "Monai.Deploy.WorkflowManager.Database": "[1.0.0, )", "Monai.Deploy.WorkflowManager.Storage": "[1.0.0, )" diff --git a/src/WorkflowManager/WorkflowManager/appsettings.json b/src/WorkflowManager/WorkflowManager/appsettings.json index 502dabf97..4b946a500 100755 --- a/src/WorkflowManager/WorkflowManager/appsettings.json +++ b/src/WorkflowManager/WorkflowManager/appsettings.json @@ -104,7 +104,8 @@ } }, "dicomTagsDisallowed": "PatientName,PatientID,IssuerOfPatientID,TypeOfPatientID,IssuerOfPatientIDQualifiersSequence,SourcePatientGroupIdentificationSequence,GroupOfPatientsIdentificationSequence,SubjectRelativePositionInImage,PatientBirthDate,PatientBirthTime,PatientBirthDateInAlternativeCalendar,PatientDeathDateInAlternativeCalendar,PatientAlternativeCalendar,PatientSex,PatientInsurancePlanCodeSequence,PatientPrimaryLanguageCodeSequence,PatientPrimaryLanguageModifierCodeSequence,QualityControlSubject,QualityControlSubjectTypeCodeSequence,StrainDescription,StrainNomenclature,StrainStockNumber,StrainSourceRegistryCodeSequence,StrainStockSequence,StrainSource,StrainAdditionalInformation,StrainCodeSequence,GeneticModificationsSequence,GeneticModificationsDescription,GeneticModificationsNomenclature,GeneticModificationsCodeSequence,OtherPatientIDsRETIRED,OtherPatientNames,OtherPatientIDsSequence,PatientBirthName,PatientAge,PatientSize,PatientSizeCodeSequence,PatientBodyMassIndex,MeasuredAPDimension,MeasuredLateralDimension,PatientWeight,PatientAddress,InsurancePlanIdentificationRETIRED,PatientMotherBirthName,MilitaryRank,BranchOfService,MedicalRecordLocatorRETIRED,ReferencedPatientPhotoSequence,MedicalAlerts,Allergies,CountryOfResidence,RegionOfResidence,PatientTelephoneNumbers,PatientTelecomInformation,EthnicGroup,Occupation,SmokingStatus,AdditionalPatientHistory,PregnancyStatus,LastMenstrualDate,PatientReligiousPreference,PatientSpeciesDescription,PatientSpeciesCodeSequence,PatientSexNeutered,AnatomicalOrientationType,PatientBreedDescription,PatientBreedCodeSequence,BreedRegistrationSequence,BreedRegistrationNumber,BreedRegistryCodeSequence,ResponsiblePerson,ResponsiblePersonRole,ResponsibleOrganization,PatientComments,ExaminedBodyThickness", - "migExternalAppPlugins": [ "Monai.Deploy.InformaticsGateway.PlugIns.RemoteAppExecution.DicomDeidentifier, Monai.Deploy.InformaticsGateway.PlugIns.RemoteAppExecution, Version=0.0.0.0" ] + "migExternalAppPlugins": [ "Monai.Deploy.InformaticsGateway.PlugIns.RemoteAppExecution.DicomDeidentifier, Monai.Deploy.InformaticsGateway.PlugIns.RemoteAppExecution, Version=0.0.0.0" ], + "dataRetentionDays": 10 // note. -1 = never delete }, "InformaticsGateway": { "apiHost": "http://localhost:5010", diff --git a/src/WorkflowManager/WorkflowManager/packages.lock.json b/src/WorkflowManager/WorkflowManager/packages.lock.json index f53a1b24e..4bd31a22b 100644 --- a/src/WorkflowManager/WorkflowManager/packages.lock.json +++ b/src/WorkflowManager/WorkflowManager/packages.lock.json @@ -1051,6 +1051,7 @@ "monai.deploy.workflowmanager.common": { "type": "Project", "dependencies": { + "Monai.Deploy.WorkflowManager.Common.Configuration": "[1.0.0, )", "Monai.Deploy.WorkflowManager.Contracts": "[1.0.0, )", "Monai.Deploy.WorkflowManager.Database": "[1.0.0, )", "Monai.Deploy.WorkflowManager.Storage": "[1.0.0, )" diff --git a/tests/UnitTests/Common.Tests/Services/PayloadServiceTests.cs b/tests/UnitTests/Common.Tests/Services/PayloadServiceTests.cs index dcc1ed626..5c5af2584 100644 --- a/tests/UnitTests/Common.Tests/Services/PayloadServiceTests.cs +++ b/tests/UnitTests/Common.Tests/Services/PayloadServiceTests.cs @@ -27,6 +27,9 @@ using Monai.Deploy.WorkflowManager.Common.Storage.Services; using Moq; using Xunit; +using Monai.Deploy.WorkflowManager.Common.Database.Repositories; +using Microsoft.Extensions.Options; +using Monai.Deploy.WorkflowManager.Common.Configuration; namespace Monai.Deploy.WorkflowManager.Common.Miscellaneous.Tests.Services { @@ -36,6 +39,7 @@ public class PayloadServiceTests private readonly Mock _payloadRepository; private readonly Mock _workflowInstanceRepository; + private readonly Mock _workflowRepository; private readonly Mock _dicomService; private readonly Mock _serviceScopeFactory; private readonly Mock _serviceProvider; @@ -47,6 +51,7 @@ public PayloadServiceTests() { _payloadRepository = new Mock(); _workflowInstanceRepository = new Mock(); + _workflowRepository = new Mock(); _dicomService = new Mock(); _serviceProvider = new Mock(); _storageService = new Mock(); @@ -65,7 +70,16 @@ public PayloadServiceTests() .Setup(x => x.GetService(typeof(IStorageService))) .Returns(_storageService.Object); - PayloadService = new PayloadService(_payloadRepository.Object, _dicomService.Object, _workflowInstanceRepository.Object, _serviceScopeFactory.Object, _logger.Object); + var opts = Options.Create(new WorkflowManagerOptions { DataRetentionDays = 99 }); + + PayloadService = new PayloadService( + _payloadRepository.Object, + _dicomService.Object, + _workflowInstanceRepository.Object, + _workflowRepository.Object, + _serviceScopeFactory.Object, + opts, + _logger.Object); } [Fact] @@ -372,5 +386,58 @@ public async Task DeletePayloadFromStorageAsync_ThrowsMonaiBadRequestExceptionWh await Assert.ThrowsAsync(async () => await PayloadService.DeletePayloadFromStorageAsync(payloadId)); } + + + [Fact] + public async Task GetExpiry_Should_use_Config_if_not_set() + { + _workflowInstanceRepository.Setup(r => + r.GetByPayloadIdsAsync(It.IsAny>()) + ).ReturnsAsync(() => new List()); + var workflow = new WorkflowRevision { Workflow = new Workflow { DataRetentionDays = null } }; + + + _workflowRepository.Setup(r => + r.GetByWorkflowIdAsync(It.IsAny()) + ).ReturnsAsync(workflow); + + var now = new DateTime(2021, 1, 1); + var expires = await PayloadService.GetExpiry(now, "workflowInstanceId"); + Assert.Equal(now.AddDays(99), expires); + } + + [Fact] + public async Task GetExpiry_Should_return_null_if_minusOne() + { + _workflowInstanceRepository.Setup(r => + r.GetByWorkflowInstanceIdAsync(It.IsAny()) + ).ReturnsAsync(() => new WorkflowInstance()); + var workflow = new WorkflowRevision { Workflow = new Workflow { DataRetentionDays = -1 } }; + + + _workflowRepository.Setup(r => + r.GetByWorkflowIdAsync(It.IsAny()) + ).ReturnsAsync(workflow); + + var now = new DateTime(2021, 1, 1); + var expires = await PayloadService.GetExpiry(now, "workflowInstanceId"); + Assert.Null(expires); + } + + [Fact] + public async Task GetExpiry_Should_use_Workflow_Value_if_set() + { + _workflowInstanceRepository.Setup(r => + r.GetByWorkflowInstanceIdAsync(It.IsAny()) + ).ReturnsAsync(() => new WorkflowInstance()); + var workflow = new WorkflowRevision { Workflow = new Workflow { DataRetentionDays = 4 } }; + + _workflowRepository.Setup(r => + r.GetByWorkflowIdAsync(It.IsAny()) + ).ReturnsAsync(workflow); + var now = new DateTime(2021, 1, 1); + var expires = await PayloadService.GetExpiry(now, "workflowInstanceId"); + Assert.Equal(now.AddDays(4), expires); + } } } diff --git a/tests/UnitTests/MonaiBackgroundService.Tests/WorkerTests.cs b/tests/UnitTests/MonaiBackgroundService.Tests/WorkerTests.cs index bd715e18c..47f2a0f76 100644 --- a/tests/UnitTests/MonaiBackgroundService.Tests/WorkerTests.cs +++ b/tests/UnitTests/MonaiBackgroundService.Tests/WorkerTests.cs @@ -24,6 +24,9 @@ using Monai.Deploy.WorkflowManager.Common.Contracts.Models; using Monai.Deploy.WorkflowManager.Common.Database.Interfaces; using Moq; +using Monai.Deploy.Storage.API; +using Monai.Deploy.WorkflowManager.Common.Database.Repositories; +using System.Threading.Tasks; namespace Monai.Deploy.WorkflowManager.Common.MonaiBackgroundService.Tests { @@ -33,6 +36,8 @@ public class WorkerTests private readonly Worker _service; private readonly Mock _pubService; private readonly IOptions _options; + private readonly Mock _storageService; + private readonly Mock _payloadRepository; private readonly Mock _repo; public WorkerTests() @@ -42,7 +47,9 @@ public WorkerTests() var taskService = new TasksService(_repo.Object); _pubService = new Mock(); _options = Options.Create(new WorkflowManagerOptions()); - _service = new Worker(logger.Object, taskService, _pubService.Object, _options); + _storageService = new Mock(); + _payloadRepository = new Mock(); + _service = new Worker(logger.Object, taskService, _pubService.Object, _payloadRepository.Object, _storageService.Object, _options); } [Fact] @@ -89,5 +96,22 @@ public async Task MonaiBackgroundService_DoWork_ShouldPublishMessages() Assert.False(_service.IsRunning); } + + [Fact] + public async Task MonaiBackgroundService_DoWork_Should_Delete_Expired_Payload_Files() + { + var payloadToRemove = new Payload { PayloadId = "removeMe " }; + + + _payloadRepository.Setup(p => p.GetPayloadsToDelete(It.IsAny())).ReturnsAsync(() => new List { payloadToRemove }); + _storageService.Setup(s => s.ListObjectsAsync(It.IsAny(), It.IsAny(), true, It.IsAny())) + .ReturnsAsync(() => new List { new VirtualFileInfo(payloadToRemove.PayloadId, payloadToRemove.PayloadId, "", 5) }); + + await _service.DoWork(); + + _storageService.Verify(s => s.RemoveObjectAsync(It.IsAny(), It.IsAny(), It.IsAny()), Times.Once()); + _payloadRepository.Verify(p => p.MarkDeletedState(It.IsAny>(), It.IsAny()), Times.Exactly(2)); //once for in-progress once for deleted + Assert.False(_service.IsRunning); + } } } diff --git a/tests/UnitTests/WorkflowManager.Tests/packages.lock.json b/tests/UnitTests/WorkflowManager.Tests/packages.lock.json index 2eda2408f..cf0b1b43a 100644 --- a/tests/UnitTests/WorkflowManager.Tests/packages.lock.json +++ b/tests/UnitTests/WorkflowManager.Tests/packages.lock.json @@ -1884,6 +1884,7 @@ "monai.deploy.workflowmanager.common": { "type": "Project", "dependencies": { + "Monai.Deploy.WorkflowManager.Common.Configuration": "[1.0.0, )", "Monai.Deploy.WorkflowManager.Contracts": "[1.0.0, )", "Monai.Deploy.WorkflowManager.Database": "[1.0.0, )", "Monai.Deploy.WorkflowManager.Storage": "[1.0.0, )" From 88b4ee31228a613f91ab79e75f015088b7e6789f Mon Sep 17 00:00:00 2001 From: Neil South Date: Tue, 9 Jan 2024 12:10:01 +0000 Subject: [PATCH 2/5] moved retention days Signed-off-by: Neil South --- .../Common/Services/PayloadService.cs | 2 +- ...=> M004_WorkflowRevision_addDataRetension.cs} | 6 ++---- src/WorkflowManager/Contracts/Models/Workflow.cs | 12 +----------- .../Contracts/Models/WorkflowRevision.cs | 3 +++ .../Database/Repositories/WorkflowRepository.cs | 16 ++++++++++++++++ .../Common.Tests/Services/PayloadServiceTests.cs | 7 +++---- .../MonaiBackgroundService.Tests/WorkerTests.cs | 2 -- 7 files changed, 26 insertions(+), 22 deletions(-) rename src/WorkflowManager/Contracts/Migrations/{M004_Workflow_addDataRetension.cs => M004_WorkflowRevision_addDataRetension.cs} (83%) diff --git a/src/WorkflowManager/Common/Services/PayloadService.cs b/src/WorkflowManager/Common/Services/PayloadService.cs index 7fd62a216..920d0ca0a 100644 --- a/src/WorkflowManager/Common/Services/PayloadService.cs +++ b/src/WorkflowManager/Common/Services/PayloadService.cs @@ -137,7 +137,7 @@ public PayloadService( var t = await _workflowRepository.GetByWorkflowIdAsync(workflowInstance.WorkflowId); - return (await _workflowRepository.GetByWorkflowIdAsync(workflowInstance.WorkflowId))?.Workflow?.DataRetentionDays ?? null; + return (await _workflowRepository.GetByWorkflowIdAsync(workflowInstance.WorkflowId))?.DataRetentionDays ?? null; } public async Task GetByIdAsync(string payloadId) diff --git a/src/WorkflowManager/Contracts/Migrations/M004_Workflow_addDataRetension.cs b/src/WorkflowManager/Contracts/Migrations/M004_WorkflowRevision_addDataRetension.cs similarity index 83% rename from src/WorkflowManager/Contracts/Migrations/M004_Workflow_addDataRetension.cs rename to src/WorkflowManager/Contracts/Migrations/M004_WorkflowRevision_addDataRetension.cs index ea4d91f68..1fb0eb344 100644 --- a/src/WorkflowManager/Contracts/Migrations/M004_Workflow_addDataRetension.cs +++ b/src/WorkflowManager/Contracts/Migrations/M004_WorkflowRevision_addDataRetension.cs @@ -20,13 +20,12 @@ namespace Monai.Deploy.WorkflowManager.Common.Contracts.Migrations { - public class M004_Workflow_addDataRetension : DocumentMigration + public class M004_WorkflowRevision_addDataRetension : DocumentMigration { - public M004_Workflow_addDataRetension() : base("1.0.1") { } + public M004_WorkflowRevision_addDataRetension() : base("1.0.1") { } public override void Up(BsonDocument document) { - // will also add version as it has a default document.Add("DataRetentionDays", BsonNull.Create(null).ToJson(), true); } @@ -35,7 +34,6 @@ public override void Down(BsonDocument document) try { document.Remove("DataRetentionDays"); - document.Remove("Version"); } catch { // can ignore we dont want failures stopping startup ! diff --git a/src/WorkflowManager/Contracts/Models/Workflow.cs b/src/WorkflowManager/Contracts/Models/Workflow.cs index 2851deab1..c902d9d83 100755 --- a/src/WorkflowManager/Contracts/Models/Workflow.cs +++ b/src/WorkflowManager/Contracts/Models/Workflow.cs @@ -14,21 +14,15 @@ * limitations under the License. */ -using Monai.Deploy.WorkflowManager.Common.Contracts.Migrations; -using Mongo.Migration.Documents; using Mongo.Migration.Documents.Attributes; -using MongoDB.Bson.Serialization.Attributes; using Newtonsoft.Json; namespace Monai.Deploy.WorkflowManager.Common.Contracts.Models { [CollectionLocation("Workflows"), RuntimeVersion("1.0.1")] - public class Workflow : IDocument + public class Workflow { - [JsonConverter(typeof(DocumentVersionConvert)), BsonSerializer(typeof(DocumentVersionConverBson))] - DocumentVersion IDocument.Version { get; set; } = new DocumentVersion(1, 0, 1); - [JsonProperty(PropertyName = "name")] public string Name { get; set; } = string.Empty; @@ -43,9 +37,5 @@ public class Workflow : IDocument [JsonProperty(PropertyName = "tasks")] public TaskObject[] Tasks { get; set; } = System.Array.Empty(); - - [JsonProperty(PropertyName = "dataRetentionDays")] - public int? DataRetentionDays { get; set; } // note. -1 = never delete - } } diff --git a/src/WorkflowManager/Contracts/Models/WorkflowRevision.cs b/src/WorkflowManager/Contracts/Models/WorkflowRevision.cs index af34bdbd9..fb42add72 100755 --- a/src/WorkflowManager/Contracts/Models/WorkflowRevision.cs +++ b/src/WorkflowManager/Contracts/Models/WorkflowRevision.cs @@ -48,5 +48,8 @@ public class WorkflowRevision : ISoftDeleteable, IDocument [JsonIgnore] public bool IsDeleted { get => Deleted is not null; } + [JsonProperty(PropertyName = "dataRetentionDays")] + public int? DataRetentionDays { get; set; } = 3;// note. -1 = never delete + } } diff --git a/src/WorkflowManager/Database/Repositories/WorkflowRepository.cs b/src/WorkflowManager/Database/Repositories/WorkflowRepository.cs index e4bd22af0..03da21814 100755 --- a/src/WorkflowManager/Database/Repositories/WorkflowRepository.cs +++ b/src/WorkflowManager/Database/Repositories/WorkflowRepository.cs @@ -206,6 +206,20 @@ public async Task> GetWorkflowsForWorkflowRequestAsync(s Guard.Against.NullOrEmpty(calledAeTitle, nameof(calledAeTitle)); Guard.Against.NullOrEmpty(callingAeTitle, nameof(callingAeTitle)); + var t = _workflowCollection + .Find(x => + x.Workflow != null && + x.Workflow.InformaticsGateway != null && + ((x.Workflow.InformaticsGateway.AeTitle == calledAeTitle && + (x.Workflow.InformaticsGateway.DataOrigins == null || + x.Workflow.InformaticsGateway.DataOrigins.Length == 0)) || + x.Workflow.InformaticsGateway.AeTitle == calledAeTitle && + x.Workflow.InformaticsGateway.DataOrigins != null && + x.Workflow.InformaticsGateway.DataOrigins.Any(d => d == callingAeTitle)) && + x.Deleted == null); + + var coll = t.ToList(); + var wfs = await _workflowCollection .Find(x => x.Workflow != null && @@ -218,6 +232,8 @@ public async Task> GetWorkflowsForWorkflowRequestAsync(s x.Workflow.InformaticsGateway.DataOrigins.Any(d => d == callingAeTitle)) && x.Deleted == null) .ToListAsync(); + + return wfs; } diff --git a/tests/UnitTests/Common.Tests/Services/PayloadServiceTests.cs b/tests/UnitTests/Common.Tests/Services/PayloadServiceTests.cs index 5c5af2584..068f23b47 100644 --- a/tests/UnitTests/Common.Tests/Services/PayloadServiceTests.cs +++ b/tests/UnitTests/Common.Tests/Services/PayloadServiceTests.cs @@ -27,7 +27,6 @@ using Monai.Deploy.WorkflowManager.Common.Storage.Services; using Moq; using Xunit; -using Monai.Deploy.WorkflowManager.Common.Database.Repositories; using Microsoft.Extensions.Options; using Monai.Deploy.WorkflowManager.Common.Configuration; @@ -394,7 +393,7 @@ public async Task GetExpiry_Should_use_Config_if_not_set() _workflowInstanceRepository.Setup(r => r.GetByPayloadIdsAsync(It.IsAny>()) ).ReturnsAsync(() => new List()); - var workflow = new WorkflowRevision { Workflow = new Workflow { DataRetentionDays = null } }; + var workflow = new WorkflowRevision { Workflow = new Workflow(), DataRetentionDays = null }; _workflowRepository.Setup(r => @@ -412,7 +411,7 @@ public async Task GetExpiry_Should_return_null_if_minusOne() _workflowInstanceRepository.Setup(r => r.GetByWorkflowInstanceIdAsync(It.IsAny()) ).ReturnsAsync(() => new WorkflowInstance()); - var workflow = new WorkflowRevision { Workflow = new Workflow { DataRetentionDays = -1 } }; + var workflow = new WorkflowRevision { Workflow = new Workflow(), DataRetentionDays = -1 }; _workflowRepository.Setup(r => @@ -430,7 +429,7 @@ public async Task GetExpiry_Should_use_Workflow_Value_if_set() _workflowInstanceRepository.Setup(r => r.GetByWorkflowInstanceIdAsync(It.IsAny()) ).ReturnsAsync(() => new WorkflowInstance()); - var workflow = new WorkflowRevision { Workflow = new Workflow { DataRetentionDays = 4 } }; + var workflow = new WorkflowRevision { Workflow = new Workflow(), DataRetentionDays = 4 }; _workflowRepository.Setup(r => r.GetByWorkflowIdAsync(It.IsAny()) diff --git a/tests/UnitTests/MonaiBackgroundService.Tests/WorkerTests.cs b/tests/UnitTests/MonaiBackgroundService.Tests/WorkerTests.cs index 47f2a0f76..7999b66df 100644 --- a/tests/UnitTests/MonaiBackgroundService.Tests/WorkerTests.cs +++ b/tests/UnitTests/MonaiBackgroundService.Tests/WorkerTests.cs @@ -25,8 +25,6 @@ using Monai.Deploy.WorkflowManager.Common.Database.Interfaces; using Moq; using Monai.Deploy.Storage.API; -using Monai.Deploy.WorkflowManager.Common.Database.Repositories; -using System.Threading.Tasks; namespace Monai.Deploy.WorkflowManager.Common.MonaiBackgroundService.Tests { From 1f94b01c04502170a5eeae00cc263d761bc7d1c2 Mon Sep 17 00:00:00 2001 From: Neil South Date: Wed, 10 Jan 2024 12:53:11 +0000 Subject: [PATCH 3/5] fixups Signed-off-by: Neil South --- src/WorkflowManager/Common/Services/PayloadService.cs | 2 +- .../Migrations/M004_WorkflowRevision_addDataRetension.cs | 7 +++++-- src/WorkflowManager/Contracts/Models/Workflow.cs | 6 ++++-- src/WorkflowManager/Contracts/Models/WorkflowRevision.cs | 7 ++----- src/WorkflowManager/Logging/Log.200000.Workflow.cs | 2 +- .../UnitTests/Common.Tests/Services/PayloadServiceTests.cs | 6 +++--- 6 files changed, 16 insertions(+), 14 deletions(-) diff --git a/src/WorkflowManager/Common/Services/PayloadService.cs b/src/WorkflowManager/Common/Services/PayloadService.cs index 920d0ca0a..7fd62a216 100644 --- a/src/WorkflowManager/Common/Services/PayloadService.cs +++ b/src/WorkflowManager/Common/Services/PayloadService.cs @@ -137,7 +137,7 @@ public PayloadService( var t = await _workflowRepository.GetByWorkflowIdAsync(workflowInstance.WorkflowId); - return (await _workflowRepository.GetByWorkflowIdAsync(workflowInstance.WorkflowId))?.DataRetentionDays ?? null; + return (await _workflowRepository.GetByWorkflowIdAsync(workflowInstance.WorkflowId))?.Workflow?.DataRetentionDays ?? null; } public async Task GetByIdAsync(string payloadId) diff --git a/src/WorkflowManager/Contracts/Migrations/M004_WorkflowRevision_addDataRetension.cs b/src/WorkflowManager/Contracts/Migrations/M004_WorkflowRevision_addDataRetension.cs index 1fb0eb344..1cd33a80f 100644 --- a/src/WorkflowManager/Contracts/Migrations/M004_WorkflowRevision_addDataRetension.cs +++ b/src/WorkflowManager/Contracts/Migrations/M004_WorkflowRevision_addDataRetension.cs @@ -26,14 +26,17 @@ public M004_WorkflowRevision_addDataRetension() : base("1.0.1") { } public override void Up(BsonDocument document) { - document.Add("DataRetentionDays", BsonNull.Create(null).ToJson(), true); +// document.Add("Workflow.DataRetentionDays", BsonNull.Create(null).ToJson(), true); + var workflow = document["Workflow"].AsBsonDocument; + workflow.Add("DataRetentionDays", BsonNull.Create(null).ToJson(), true); } public override void Down(BsonDocument document) { try { - document.Remove("DataRetentionDays"); + var workflow = document["Workflow"].AsBsonDocument; + workflow.Remove("DataRetentionDays"); } catch { // can ignore we dont want failures stopping startup ! diff --git a/src/WorkflowManager/Contracts/Models/Workflow.cs b/src/WorkflowManager/Contracts/Models/Workflow.cs index c902d9d83..35aae2649 100755 --- a/src/WorkflowManager/Contracts/Models/Workflow.cs +++ b/src/WorkflowManager/Contracts/Models/Workflow.cs @@ -14,12 +14,10 @@ * limitations under the License. */ -using Mongo.Migration.Documents.Attributes; using Newtonsoft.Json; namespace Monai.Deploy.WorkflowManager.Common.Contracts.Models { - [CollectionLocation("Workflows"), RuntimeVersion("1.0.1")] public class Workflow { @@ -37,5 +35,9 @@ public class Workflow [JsonProperty(PropertyName = "tasks")] public TaskObject[] Tasks { get; set; } = System.Array.Empty(); + + [JsonProperty(PropertyName = "dataRetentionDays")] + public int? DataRetentionDays { get; set; } = 3;// note. -1 = never delete + } } diff --git a/src/WorkflowManager/Contracts/Models/WorkflowRevision.cs b/src/WorkflowManager/Contracts/Models/WorkflowRevision.cs index fb42add72..e28abebee 100755 --- a/src/WorkflowManager/Contracts/Models/WorkflowRevision.cs +++ b/src/WorkflowManager/Contracts/Models/WorkflowRevision.cs @@ -23,7 +23,7 @@ namespace Monai.Deploy.WorkflowManager.Common.Contracts.Models { - [CollectionLocation("Workflows"), RuntimeVersion("1.0.0")] + [CollectionLocation("Workflows"), RuntimeVersion("1.0.1")] public class WorkflowRevision : ISoftDeleteable, IDocument { [BsonId] @@ -31,7 +31,7 @@ public class WorkflowRevision : ISoftDeleteable, IDocument public string? Id { get; set; } [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 = "workflow_id")] public string WorkflowId { get; set; } = string.Empty; @@ -48,8 +48,5 @@ public class WorkflowRevision : ISoftDeleteable, IDocument [JsonIgnore] public bool IsDeleted { get => Deleted is not null; } - [JsonProperty(PropertyName = "dataRetentionDays")] - public int? DataRetentionDays { get; set; } = 3;// note. -1 = never delete - } } diff --git a/src/WorkflowManager/Logging/Log.200000.Workflow.cs b/src/WorkflowManager/Logging/Log.200000.Workflow.cs index fff85817a..c3bc807dc 100644 --- a/src/WorkflowManager/Logging/Log.200000.Workflow.cs +++ b/src/WorkflowManager/Logging/Log.200000.Workflow.cs @@ -109,7 +109,7 @@ public static partial class Log [LoggerMessage(EventId = 210007, Level = LogLevel.Information, Message = "Exporting to MIG task Id {taskid}, export destination {destination} number of files {fileCount} Mig data plugins {plugins}.")] public static partial void LogMigExport(this ILogger logger, string taskid, string destination, int fileCount, string plugins); - [LoggerMessage(EventId = 200018, Level = LogLevel.Error, Message = "ExportList or Artifacts are empty! workflowInstanceId {workflowInstanceId} TaskId {taskId}")] + [LoggerMessage(EventId = 210018, Level = LogLevel.Error, Message = "ExportList or Artifacts are empty! workflowInstanceId {workflowInstanceId} TaskId {taskId}")] public static partial void ExportListOrArtifactsAreEmpty(this ILogger logger, string taskId, string workflowInstanceId); } } diff --git a/tests/UnitTests/Common.Tests/Services/PayloadServiceTests.cs b/tests/UnitTests/Common.Tests/Services/PayloadServiceTests.cs index 068f23b47..dfca2b04e 100644 --- a/tests/UnitTests/Common.Tests/Services/PayloadServiceTests.cs +++ b/tests/UnitTests/Common.Tests/Services/PayloadServiceTests.cs @@ -393,7 +393,7 @@ public async Task GetExpiry_Should_use_Config_if_not_set() _workflowInstanceRepository.Setup(r => r.GetByPayloadIdsAsync(It.IsAny>()) ).ReturnsAsync(() => new List()); - var workflow = new WorkflowRevision { Workflow = new Workflow(), DataRetentionDays = null }; + var workflow = new WorkflowRevision { Workflow = new Workflow { DataRetentionDays = null } }; _workflowRepository.Setup(r => @@ -411,7 +411,7 @@ public async Task GetExpiry_Should_return_null_if_minusOne() _workflowInstanceRepository.Setup(r => r.GetByWorkflowInstanceIdAsync(It.IsAny()) ).ReturnsAsync(() => new WorkflowInstance()); - var workflow = new WorkflowRevision { Workflow = new Workflow(), DataRetentionDays = -1 }; + var workflow = new WorkflowRevision { Workflow = new Workflow { DataRetentionDays = -1 } }; _workflowRepository.Setup(r => @@ -429,7 +429,7 @@ public async Task GetExpiry_Should_use_Workflow_Value_if_set() _workflowInstanceRepository.Setup(r => r.GetByWorkflowInstanceIdAsync(It.IsAny()) ).ReturnsAsync(() => new WorkflowInstance()); - var workflow = new WorkflowRevision { Workflow = new Workflow(), DataRetentionDays = 4 }; + var workflow = new WorkflowRevision { Workflow = new Workflow { DataRetentionDays = 4 } }; _workflowRepository.Setup(r => r.GetByWorkflowIdAsync(It.IsAny()) From f5494526a7f095eaa5d3d9a72301e3584edc8425 Mon Sep 17 00:00:00 2001 From: Neil South Date: Wed, 10 Jan 2024 17:19:48 +0000 Subject: [PATCH 4/5] new unit tests Signed-off-by: Neil South --- .../Services/PayloadServiceTests.cs | 50 +++++++++++++++++++ 1 file changed, 50 insertions(+) diff --git a/tests/UnitTests/Common.Tests/Services/PayloadServiceTests.cs b/tests/UnitTests/Common.Tests/Services/PayloadServiceTests.cs index dfca2b04e..9c04b72a8 100644 --- a/tests/UnitTests/Common.Tests/Services/PayloadServiceTests.cs +++ b/tests/UnitTests/Common.Tests/Services/PayloadServiceTests.cs @@ -438,5 +438,55 @@ public async Task GetExpiry_Should_use_Workflow_Value_if_set() var expires = await PayloadService.GetExpiry(now, "workflowInstanceId"); Assert.Equal(now.AddDays(4), expires); } + + [Fact] + public void PayloadServiceCreate_Should_Throw_If_No_Options_Passed() + { + Assert.Throws(() => new PayloadService( + _payloadRepository.Object, + _dicomService.Object, + _workflowInstanceRepository.Object, + _workflowRepository.Object, + _serviceScopeFactory.Object, + null!, + _logger.Object)); + } + + [Fact] + public void PayloadServiceCreate_Should_Throw_If_No_workflowRepository_Passed() + { + var opts = Options.Create(new WorkflowManagerOptions { DataRetentionDays = 99 }); + + Assert.Throws(() => new PayloadService( + _payloadRepository.Object, + _dicomService.Object, + _workflowInstanceRepository.Object, + null!, + _serviceScopeFactory.Object, + opts, + _logger.Object)); + } + + [Fact] + public async Task PayloadServiceCreate_Should_Call_GetExpiry() + { + _payloadRepository.Setup(p => p.CreateAsync(It.IsAny())).ReturnsAsync(true); + + var payload = await PayloadService.CreateAsync(new WorkflowRequestEvent + { + Timestamp = DateTime.UtcNow, + Bucket = "bucket", + DataTrigger = new DataOrigin { Source = "aetitle", Destination = "aetitle" }, + CorrelationId = Guid.NewGuid().ToString(), + PayloadId = Guid.NewGuid(), + Workflows = new List { Guid.NewGuid().ToString() }, + FileCount = 0 + }); + + var daysdiff = (payload!.Expires - DateTime.UtcNow).Value.TotalDays + 0.5; + + Assert.Equal(99, (int)daysdiff); + } + } } From f2b19c86ac1ec4c356f7eb1007cabb1b2efc98e5 Mon Sep 17 00:00:00 2001 From: Neil South Date: Wed, 10 Jan 2024 17:44:30 +0000 Subject: [PATCH 5/5] fixups from sonar cloud Signed-off-by: Neil South --- .../Common/Services/PayloadService.cs | 2 -- .../Contracts/Migrations/M004_Payload_expires.cs | 4 ++-- .../M004_WorkflowRevision_addDataRetension.cs | 4 ++-- .../Database/Repositories/PayloadRepository.cs | 2 +- .../Database/Repositories/WorkflowRepository.cs | 15 --------------- .../MonaiBackgroundService/Worker.cs | 4 ++-- .../Common.Tests/Services/PayloadServiceTests.cs | 2 +- 7 files changed, 8 insertions(+), 25 deletions(-) diff --git a/src/WorkflowManager/Common/Services/PayloadService.cs b/src/WorkflowManager/Common/Services/PayloadService.cs index 97058f825..15d90bc3b 100644 --- a/src/WorkflowManager/Common/Services/PayloadService.cs +++ b/src/WorkflowManager/Common/Services/PayloadService.cs @@ -135,8 +135,6 @@ public PayloadService( if (workflowInstance is null) { return null; } - var t = await _workflowRepository.GetByWorkflowIdAsync(workflowInstance.WorkflowId); - return (await _workflowRepository.GetByWorkflowIdAsync(workflowInstance.WorkflowId))?.Workflow?.DataRetentionDays ?? null; } diff --git a/src/WorkflowManager/Contracts/Migrations/M004_Payload_expires.cs b/src/WorkflowManager/Contracts/Migrations/M004_Payload_expires.cs index 91fcff9bf..44a52090e 100644 --- a/src/WorkflowManager/Contracts/Migrations/M004_Payload_expires.cs +++ b/src/WorkflowManager/Contracts/Migrations/M004_Payload_expires.cs @@ -19,9 +19,9 @@ namespace Monai.Deploy.WorkflowManager.Common.Contracts.Migrations { - public class M004_Payload_expires : DocumentMigration + public class M004_Payload_Expires : DocumentMigration { - public M004_Payload_expires() : base("1.0.4") { } + public M004_Payload_Expires() : base("1.0.4") { } public override void Up(BsonDocument document) { diff --git a/src/WorkflowManager/Contracts/Migrations/M004_WorkflowRevision_addDataRetension.cs b/src/WorkflowManager/Contracts/Migrations/M004_WorkflowRevision_addDataRetension.cs index 1cd33a80f..ca2aae938 100644 --- a/src/WorkflowManager/Contracts/Migrations/M004_WorkflowRevision_addDataRetension.cs +++ b/src/WorkflowManager/Contracts/Migrations/M004_WorkflowRevision_addDataRetension.cs @@ -20,9 +20,9 @@ namespace Monai.Deploy.WorkflowManager.Common.Contracts.Migrations { - public class M004_WorkflowRevision_addDataRetension : DocumentMigration + public class M004_WorkflowRevision_AddDataRetension : DocumentMigration { - public M004_WorkflowRevision_addDataRetension() : base("1.0.1") { } + public M004_WorkflowRevision_AddDataRetension() : base("1.0.1") { } public override void Up(BsonDocument document) { diff --git a/src/WorkflowManager/Database/Repositories/PayloadRepository.cs b/src/WorkflowManager/Database/Repositories/PayloadRepository.cs index ca3219be9..a4fc8cee8 100644 --- a/src/WorkflowManager/Database/Repositories/PayloadRepository.cs +++ b/src/WorkflowManager/Database/Repositories/PayloadRepository.cs @@ -66,7 +66,7 @@ private async Task EnsureIndex() var indexes = bsonDocuments.Select(_ => _.GetElement("name").Value.ToString()).ToList(); // If index not present create it else skip. - if (!indexes.Any(i => i is not null && i.Equals(indexName))) + if (!indexes.Exists(i => i is not null && i.Equals(indexName))) { await _payloadCollection.Indexes.CreateOneAsync(model); } diff --git a/src/WorkflowManager/Database/Repositories/WorkflowRepository.cs b/src/WorkflowManager/Database/Repositories/WorkflowRepository.cs index aadbd7cec..8ac32ec05 100755 --- a/src/WorkflowManager/Database/Repositories/WorkflowRepository.cs +++ b/src/WorkflowManager/Database/Repositories/WorkflowRepository.cs @@ -206,20 +206,6 @@ public async Task> GetWorkflowsForWorkflowRequestAsync(s ArgumentNullException.ThrowIfNullOrEmpty(calledAeTitle, nameof(calledAeTitle)); ArgumentNullException.ThrowIfNullOrEmpty(callingAeTitle, nameof(callingAeTitle)); - var t = _workflowCollection - .Find(x => - x.Workflow != null && - x.Workflow.InformaticsGateway != null && - ((x.Workflow.InformaticsGateway.AeTitle == calledAeTitle && - (x.Workflow.InformaticsGateway.DataOrigins == null || - x.Workflow.InformaticsGateway.DataOrigins.Length == 0)) || - x.Workflow.InformaticsGateway.AeTitle == calledAeTitle && - x.Workflow.InformaticsGateway.DataOrigins != null && - x.Workflow.InformaticsGateway.DataOrigins.Any(d => d == callingAeTitle)) && - x.Deleted == null); - - var coll = t.ToList(); - var wfs = await _workflowCollection .Find(x => x.Workflow != null && @@ -233,7 +219,6 @@ public async Task> GetWorkflowsForWorkflowRequestAsync(s x.Deleted == null) .ToListAsync(); - return wfs; } diff --git a/src/WorkflowManager/MonaiBackgroundService/Worker.cs b/src/WorkflowManager/MonaiBackgroundService/Worker.cs index bc36ea273..f68c2e715 100644 --- a/src/WorkflowManager/MonaiBackgroundService/Worker.cs +++ b/src/WorkflowManager/MonaiBackgroundService/Worker.cs @@ -52,7 +52,7 @@ public Worker( _publisherService = publisherService ?? throw new ArgumentNullException(nameof(publisherService)); _options = options ?? throw new ArgumentNullException(nameof(options)); _payloadRepository = payloadRepository ?? throw new ArgumentNullException(nameof(payloadRepository)); - _storageService = storageService ?? throw new ArgumentNullException(nameof(_storageService)); + _storageService = storageService ?? throw new ArgumentNullException(nameof(storageService)); } public static string ServiceName => "Monai Background Service"; @@ -110,7 +110,7 @@ private async Task ProcessExpiredPayloads() { payloads = (await _payloadRepository.GetPayloadsToDelete(DateTime.UtcNow).ConfigureAwait(false)).ToList(); - if (payloads.Any()) + if (payloads.Count != 0) { var ids = payloads.Select(p => p.PayloadId).ToList(); diff --git a/tests/UnitTests/Common.Tests/Services/PayloadServiceTests.cs b/tests/UnitTests/Common.Tests/Services/PayloadServiceTests.cs index 9c04b72a8..6b3f9557d 100644 --- a/tests/UnitTests/Common.Tests/Services/PayloadServiceTests.cs +++ b/tests/UnitTests/Common.Tests/Services/PayloadServiceTests.cs @@ -483,7 +483,7 @@ public async Task PayloadServiceCreate_Should_Call_GetExpiry() FileCount = 0 }); - var daysdiff = (payload!.Expires - DateTime.UtcNow).Value.TotalDays + 0.5; + var daysdiff = (payload!.Expires! - DateTime.UtcNow).Value.TotalDays + 0.5; Assert.Equal(99, (int)daysdiff); }