From 3ce01e6fa19c1522aab0b73df6766d687be19db3 Mon Sep 17 00:00:00 2001 From: "jack.lewis" Date: Mon, 3 Jun 2024 11:34:15 +0100 Subject: [PATCH] further code review comment fixes for asset modified cleanup --- .../Features/Image/Ingest/AssetProcessor.cs | 7 ++-- .../CleanupHandler/AssetUpdatedHandler.cs | 36 ++++++++++++------- .../Infrastructure/ServiceCollectionX.cs | 6 +--- .../CleanupHandlerAssetRepository.cs | 5 +-- .../ICleanupHandlerAssetRepository.cs | 2 +- src/protagonist/DLCS.AWS/SQS/SqsListener.cs | 3 +- .../Policies/DeliveryChannelPolicyXTests.cs | 23 ++++++++++++ .../DLCS.Model/Assets/ImageDeliveryChannel.cs | 7 ++-- .../Metadata/AssetApplicationMetadata.cs | 7 ++-- .../Policies/DeliveryChannelPolicyX.cs | 16 +++++++++ .../Messaging/EngineClientTests.cs | 8 ++--- .../DLCS.Repository/Messaging/EngineClient.cs | 10 ++++-- .../Http/ControllableHttpMessageHandler.cs | 2 +- 13 files changed, 89 insertions(+), 43 deletions(-) diff --git a/src/protagonist/API/Features/Image/Ingest/AssetProcessor.cs b/src/protagonist/API/Features/Image/Ingest/AssetProcessor.cs index 29608bfd6..cbf08eebc 100644 --- a/src/protagonist/API/Features/Image/Ingest/AssetProcessor.cs +++ b/src/protagonist/API/Features/Image/Ingest/AssetProcessor.cs @@ -1,5 +1,4 @@ -using System.Text.Json; -using API.Exceptions; +using API.Exceptions; using API.Features.Assets; using API.Infrastructure.Requests; using API.Settings; @@ -53,7 +52,6 @@ public async Task Process(AssetBeforeProcessing assetBeforeP try { var assetFromDatabase = await assetRepository.GetAsset(assetBeforeProcessing.Asset.Id, true, true); - var existingAsset = assetFromDatabase?.Clone(); if (assetFromDatabase == null) { @@ -104,7 +102,8 @@ public async Task Process(AssetBeforeProcessing assetBeforeP ) }; } - + + var existingAsset = assetFromDatabase?.Clone(); var assetPreparationResult = AssetPreparer.PrepareAssetForUpsert(assetFromDatabase, assetBeforeProcessing.Asset, false, isBatchUpdate, settings.RestrictedAssetIdCharacters); diff --git a/src/protagonist/CleanupHandler/AssetUpdatedHandler.cs b/src/protagonist/CleanupHandler/AssetUpdatedHandler.cs index 9f694d257..7f0ca03e8 100644 --- a/src/protagonist/CleanupHandler/AssetUpdatedHandler.cs +++ b/src/protagonist/CleanupHandler/AssetUpdatedHandler.cs @@ -1,6 +1,5 @@ -using System.Diagnostics; +using System.Diagnostics.CodeAnalysis; using System.IO.Enumeration; -using System.Text.Json; using CleanupHandler.Infrastructure; using CleanupHandler.Repository; using DLCS.AWS.ElasticTranscoder; @@ -71,12 +70,16 @@ public async Task HandleMessage(QueueMessage message, CancellationToken ca var assetBefore = request.AssetBeforeUpdate; - var assetAfter = cleanupHandlerAssetRepository.RetrieveAssetWithDeliveryChannels(assetBefore.Id); + var assetAfter = await cleanupHandlerAssetRepository.RetrieveAssetWithDeliveryChannels(assetBefore.Id); + + if (assetAfter == null) + { + logger.LogInformation("Asset {AssetId} was not found in the database for use in after calculation", + assetBefore.Id); + return false; + } if (NoCleanupRequired(message, assetAfter, assetBefore)) return true; - - Debug.Assert(assetAfter != null, nameof(assetAfter) + " != null"); - if (AssetStillIngesting(assetAfter, assetBefore)) return false; var (modifiedOrAdded, removed) = GetChangeSets(assetAfter, assetBefore); @@ -139,10 +142,10 @@ private static bool AssetStillIngesting(Asset assetAfter, Asset assetBefore) { return assetAfter.Ingesting == true && assetBefore.Finished > assetAfter.Finished; } - - private static bool NoCleanupRequired(QueueMessage message, Asset? assetAfter, Asset assetBefore) + + private static bool NoCleanupRequired([NotNullWhen(true)] QueueMessage message, Asset? assetAfter, Asset assetBefore) { - return assetAfter == null || !message.MessageAttributes.Keys.Contains("engineNotified") && + return !message.MessageAttributes.Keys.Contains("engineNotified") && (assetBefore.Roles ?? string.Empty) == (assetAfter.Roles ?? string.Empty); } @@ -209,18 +212,27 @@ private async Task CleanupChangedPolicy(ImageDeliveryChannel deliveryChannelModi } } - private async Task CleanupChangedTimebasedDeliveryChannel(ImageDeliveryChannel imageDeliveryChannel, Asset assetAfter, HashSet objectsToRemove) + private async Task CleanupChangedTimebasedDeliveryChannel(ImageDeliveryChannel imageDeliveryChannel, + Asset assetAfter, HashSet objectsToRemove) { - var presetList = JsonSerializer.Deserialize>(imageDeliveryChannel.DeliveryChannelPolicy.PolicyData); + var presetList = imageDeliveryChannel.DeliveryChannelPolicy.AsTimebasedPresets(); var keys = new List(); var extensions = new List(); var mediaPath = RetrieveMediaPath(assetAfter); var presetDictionary = await engineClient.GetAvPresets(); + if (presetDictionary.IsNullOrEmpty()) + { + logger.LogWarning( + "retrieved no timebased presets from engine, {AssetId} will not be cleaned up for the timebased channel", + assetAfter.Id); + return; + } + foreach (var presetIdentifier in presetList ?? new List()) { - if (!presetDictionary.IsNullOrEmpty() && presetDictionary.TryGetValue(presetIdentifier, out var transcoderPreset)) + if (presetDictionary.TryGetValue(presetIdentifier, out var transcoderPreset)) { var timebasedFolder = storageKeyGenerator.GetStorageLocationRoot(assetAfter.Id); diff --git a/src/protagonist/CleanupHandler/Infrastructure/ServiceCollectionX.cs b/src/protagonist/CleanupHandler/Infrastructure/ServiceCollectionX.cs index 320cd187f..2710769c6 100644 --- a/src/protagonist/CleanupHandler/Infrastructure/ServiceCollectionX.cs +++ b/src/protagonist/CleanupHandler/Infrastructure/ServiceCollectionX.cs @@ -1,7 +1,6 @@ using CleanupHandler.Repository; using DLCS.AWS.Cloudfront; using DLCS.AWS.Configuration; -using DLCS.AWS.ElasticTranscoder; using DLCS.AWS.S3; using DLCS.AWS.SQS; using DLCS.Core.Caching; @@ -39,8 +38,7 @@ public static IServiceCollection AddAws(this IServiceCollection services, .SetupAWS(configuration, hostEnvironment) .WithAmazonS3() .WithAmazonCloudfront() - .WithAmazonSQS() - .WithAmazonElasticTranscoder(); + .WithAmazonSQS(); return services; } @@ -68,10 +66,8 @@ public static IServiceCollection AddDataAccess(this IServiceCollection services, CleanupHandlerSettings cleanupHandlerSettings) { services - .AddSingleton() .AddScoped() .AddSingleton() - .AddSingleton() .AddTransient() .AddScoped() .AddDlcsContext(configuration); diff --git a/src/protagonist/CleanupHandler/Repository/CleanupHandlerAssetRepository.cs b/src/protagonist/CleanupHandler/Repository/CleanupHandlerAssetRepository.cs index d0841082d..4e6984ad5 100644 --- a/src/protagonist/CleanupHandler/Repository/CleanupHandlerAssetRepository.cs +++ b/src/protagonist/CleanupHandler/Repository/CleanupHandlerAssetRepository.cs @@ -2,6 +2,7 @@ using DLCS.Model.Assets; using DLCS.Repository; using DLCS.Repository.Assets; +using Microsoft.EntityFrameworkCore; namespace CleanupHandler.Repository; @@ -14,8 +15,8 @@ public CleanupHandlerAssetRepository(DlcsContext dbContext) this.dbContext = dbContext; } - public Asset? RetrieveAssetWithDeliveryChannels(AssetId assetId) + public async Task RetrieveAssetWithDeliveryChannels(AssetId assetId) { - return dbContext.Images.IncludeDeliveryChannelsWithPolicy().SingleOrDefault(x => x.Id == assetId); + return await dbContext.Images.IncludeDeliveryChannelsWithPolicy().SingleOrDefaultAsync(x => x.Id == assetId); } } \ No newline at end of file diff --git a/src/protagonist/CleanupHandler/Repository/ICleanupHandlerAssetRepository.cs b/src/protagonist/CleanupHandler/Repository/ICleanupHandlerAssetRepository.cs index 2323cf123..c529ebb29 100644 --- a/src/protagonist/CleanupHandler/Repository/ICleanupHandlerAssetRepository.cs +++ b/src/protagonist/CleanupHandler/Repository/ICleanupHandlerAssetRepository.cs @@ -10,5 +10,5 @@ public interface ICleanupHandlerAssetRepository /// /// The asset id to retrieve details for /// an asset - Asset? RetrieveAssetWithDeliveryChannels(AssetId assetId); + Task RetrieveAssetWithDeliveryChannels(AssetId assetId); } \ No newline at end of file diff --git a/src/protagonist/DLCS.AWS/SQS/SqsListener.cs b/src/protagonist/DLCS.AWS/SQS/SqsListener.cs index 9909c8cd9..e03b15108 100644 --- a/src/protagonist/DLCS.AWS/SQS/SqsListener.cs +++ b/src/protagonist/DLCS.AWS/SQS/SqsListener.cs @@ -155,8 +155,7 @@ private async Task HandleMessage(Message message, CancellationToken cancel } var messageAttributes = message.MessageAttributes - .Select(x => new KeyValuePair(x.Key, x.Value.StringValue)) - .ToDictionary(pair => pair.Key, pair => pair.Value); + .ToDictionary(pair => pair.Key, pair => pair.Value.StringValue); var queueMessage = new QueueMessage { diff --git a/src/protagonist/DLCS.Model.Tests/Policies/DeliveryChannelPolicyXTests.cs b/src/protagonist/DLCS.Model.Tests/Policies/DeliveryChannelPolicyXTests.cs index b4b69c2e7..632f7ab9e 100644 --- a/src/protagonist/DLCS.Model.Tests/Policies/DeliveryChannelPolicyXTests.cs +++ b/src/protagonist/DLCS.Model.Tests/Policies/DeliveryChannelPolicyXTests.cs @@ -34,6 +34,29 @@ public void ThumbsDataAsSizeParameters_ReturnsSizeParameters() actual.Should().BeEquivalentTo(expected); } + [Fact] + public void AsTimebasedPresets_Throws_IfPolicyNotTimebased() + { + var policy = new DeliveryChannelPolicy { Channel = "iiif-img" }; + + Action action = () => policy.AsTimebasedPresets(); + action.Should().ThrowExactly(); + } + + [Fact] + public void AsTimebasedPresets_ReturnsExpected() + { + // Arrange + var expected = new List { "foo", "bar", "foobar" }; + var policy = new DeliveryChannelPolicy { Channel = "iiif-av", PolicyData = "[\"foo\",\"bar\",\"foobar\"]" }; + + // Act + var actual = policy.AsTimebasedPresets(); + + // Assert + actual.Should().BeEquivalentTo(expected); + } + [Fact] public void PolicyDataAs_ReturnsExpected() { diff --git a/src/protagonist/DLCS.Model/Assets/ImageDeliveryChannel.cs b/src/protagonist/DLCS.Model/Assets/ImageDeliveryChannel.cs index 237ac4b6b..e71899ef3 100644 --- a/src/protagonist/DLCS.Model/Assets/ImageDeliveryChannel.cs +++ b/src/protagonist/DLCS.Model/Assets/ImageDeliveryChannel.cs @@ -29,8 +29,7 @@ public class ImageDeliveryChannel : ICloneable /// public int DeliveryChannelPolicyId { get; set; } - public object Clone() - { - return (ImageDeliveryChannel)MemberwiseClone(); - } + public ImageDeliveryChannel Clone() => (ImageDeliveryChannel)MemberwiseClone(); + + object ICloneable.Clone() => Clone(); } \ No newline at end of file diff --git a/src/protagonist/DLCS.Model/Assets/Metadata/AssetApplicationMetadata.cs b/src/protagonist/DLCS.Model/Assets/Metadata/AssetApplicationMetadata.cs index 77288f5fd..30a955000 100644 --- a/src/protagonist/DLCS.Model/Assets/Metadata/AssetApplicationMetadata.cs +++ b/src/protagonist/DLCS.Model/Assets/Metadata/AssetApplicationMetadata.cs @@ -32,8 +32,7 @@ public class AssetApplicationMetadata : ICloneable /// public DateTime Modified { get; set; } - public object Clone() - { - return (AssetApplicationMetadata)MemberwiseClone(); - } + public AssetApplicationMetadata Clone() => (AssetApplicationMetadata)MemberwiseClone(); + + object ICloneable.Clone() => Clone(); } \ No newline at end of file diff --git a/src/protagonist/DLCS.Model/Policies/DeliveryChannelPolicyX.cs b/src/protagonist/DLCS.Model/Policies/DeliveryChannelPolicyX.cs index f602de776..6a8de8c50 100644 --- a/src/protagonist/DLCS.Model/Policies/DeliveryChannelPolicyX.cs +++ b/src/protagonist/DLCS.Model/Policies/DeliveryChannelPolicyX.cs @@ -29,6 +29,22 @@ public static List ThumbsDataAsSizeParameters(this DeliveryChanne .ToList(); } + /// + /// Get timebased PolicyData as a list of strings + /// + /// Current + /// Collection of strings representing timebased policies + /// Thrown if specified policy is not for thumbs channel + public static List AsTimebasedPresets(this DeliveryChannelPolicy deliveryChannelPolicy) + { + if (deliveryChannelPolicy.Channel != AssetDeliveryChannels.Timebased) + { + throw new InvalidOperationException("Policy is not for timebased channel"); + } + + return deliveryChannelPolicy.PolicyDataAs>() ?? new List(); + } + /// /// Deserialise PolicyData as specified type /// diff --git a/src/protagonist/DLCS.Repository.Tests/Messaging/EngineClientTests.cs b/src/protagonist/DLCS.Repository.Tests/Messaging/EngineClientTests.cs index 3527da21f..efb64c22d 100644 --- a/src/protagonist/DLCS.Repository.Tests/Messaging/EngineClientTests.cs +++ b/src/protagonist/DLCS.Repository.Tests/Messaging/EngineClientTests.cs @@ -173,11 +173,10 @@ public async Task GetAvPresets_RetrievesAllowedAvPresets() } [Fact] - public async Task GetAvPresets_ReturnsEmpty_IfEngineAvPolicyEndpointUnreachable() + public async Task GetAvPresets_ReturnsEmpty_IfEngineAvPolicyEndpointThrowsError() { // Arrange - HttpRequestMessage message = null; - httpHandler.RegisterCallback(r => message = r); + httpHandler.RegisterCallback(r => throw new Exception("error")); httpHandler.GetResponseMessage("Not found", HttpStatusCode.NotFound); // Act @@ -185,7 +184,6 @@ public async Task GetAvPresets_ReturnsEmpty_IfEngineAvPolicyEndpointUnreachable( // Assert httpHandler.CallsMade.Should().ContainSingle().Which.Should().Be("http://engine.dlcs/av-presets"); - message.Method.Should().Be(HttpMethod.Get); - returnedAvPresets.Should().BeNull(); + returnedAvPresets.Should().BeEmpty(); } } \ No newline at end of file diff --git a/src/protagonist/DLCS.Repository/Messaging/EngineClient.cs b/src/protagonist/DLCS.Repository/Messaging/EngineClient.cs index c4f1b8bb9..33dbaae4d 100644 --- a/src/protagonist/DLCS.Repository/Messaging/EngineClient.cs +++ b/src/protagonist/DLCS.Repository/Messaging/EngineClient.cs @@ -36,6 +36,9 @@ public class EngineClient : IEngineClient { DefaultIgnoreCondition = JsonIgnoreCondition.WhenWritingNull, }; + + private static readonly IReadOnlyDictionary NullPresetDictionary = + new Dictionary(); public EngineClient( IQueueLookup queueLookup, @@ -159,18 +162,19 @@ public async Task AsynchronousIngestBatch(IReadOnlyCollection assets public async Task?> GetAvPresets(CancellationToken cancellationToken = default) { const string key = "avPresetList"; - return await appCache.GetOrAddAsync(key, async () => + return await appCache.GetOrAddAsync(key, async entry => { try { var response = await httpClient.GetAsync("av-presets", cancellationToken); return await response.Content.ReadFromJsonAsync>( - cancellationToken: cancellationToken); + cancellationToken: cancellationToken) ?? new Dictionary(); } catch (Exception ex) { logger.LogError(ex, "Failed to retrieve allowed iiif-av policy options from Engine"); - return null; + entry.AbsoluteExpirationRelativeToNow = TimeSpan.FromSeconds(cacheSettings.GetTtl(CacheDuration.Short)); + return NullPresetDictionary; } }, cacheSettings.GetMemoryCacheOptions(CacheDuration.Long)); } diff --git a/src/protagonist/Test.Helpers/Http/ControllableHttpMessageHandler.cs b/src/protagonist/Test.Helpers/Http/ControllableHttpMessageHandler.cs index 89e0c7fde..942f58870 100644 --- a/src/protagonist/Test.Helpers/Http/ControllableHttpMessageHandler.cs +++ b/src/protagonist/Test.Helpers/Http/ControllableHttpMessageHandler.cs @@ -35,7 +35,7 @@ public HttpResponseMessage GetResponseMessage(string content, HttpStatusCode htt }; return response; } - + /// /// Set a pre-canned response ///