further code review comment fixes for asset modified cleanup

dlcs · Jun 3, 2024 · 3ce01e6 · 3ce01e6
1 parent ac541d9
commit 3ce01e6
Show file tree

Hide file tree

Showing 13 changed files with 89 additions and 43 deletions.
diff --git 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<ProcessAssetResult> 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<ProcessAssetResult> 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
@@ -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<bool> 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<ObjectInBucket> objectsToRemove)
+    private async Task CleanupChangedTimebasedDeliveryChannel(ImageDeliveryChannel imageDeliveryChannel,
+        Asset assetAfter, HashSet<ObjectInBucket> objectsToRemove)
     {
-        var presetList = JsonSerializer.Deserialize<List<string>>(imageDeliveryChannel.DeliveryChannelPolicy.PolicyData);
+        var presetList = imageDeliveryChannel.DeliveryChannelPolicy.AsTimebasedPresets();
         var keys = new List<string>();
         var extensions = new List<string>();
         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<string>())
         {
-            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
@@ -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<IAssetRepository, DapperAssetRepository>()
             .AddScoped<IAssetApplicationMetadataRepository, AssetApplicationMetadataRepository>()
             .AddSingleton<IThumbRepository, ThumbRepository>()
-            .AddSingleton<AssetCachingHelper>()
             .AddTransient<TimingHandler>()
             .AddScoped<ICleanupHandlerAssetRepository, CleanupHandlerAssetRepository>()
             .AddDlcsContext(configuration);

diff --git 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<Asset?> RetrieveAssetWithDeliveryChannels(AssetId assetId)
     {
-        return dbContext.Images.IncludeDeliveryChannelsWithPolicy().SingleOrDefault(x => x.Id == assetId);
+        return await dbContext.Images.IncludeDeliveryChannelsWithPolicy().SingleOrDefaultAsync(x => x.Id == assetId);
     }
 }
diff --git a/src/protagonist/CleanupHandler/Repository/ICleanupHandlerAssetRepository.cs b/src/protagonist/CleanupHandler/Repository/ICleanupHandlerAssetRepository.cs
@@ -10,5 +10,5 @@ public interface ICleanupHandlerAssetRepository
     /// </summary>
     /// <param name="assetId">The asset id to retrieve details for</param>
     /// <returns>an asset</returns>
-    Asset? RetrieveAssetWithDeliveryChannels(AssetId assetId);
+    Task<Asset?> RetrieveAssetWithDeliveryChannels(AssetId assetId);
 }
diff --git a/src/protagonist/DLCS.AWS/SQS/SqsListener.cs b/src/protagonist/DLCS.AWS/SQS/SqsListener.cs
@@ -155,8 +155,7 @@ private async Task<bool> HandleMessage(Message message, CancellationToken cancel
             }
 
             var messageAttributes = message.MessageAttributes
-                .Select(x => new KeyValuePair<string, string>(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
@@ -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<InvalidOperationException>();
+    }
+
+    [Fact]
+    public void AsTimebasedPresets_ReturnsExpected()
+    {
+        // Arrange
+        var expected = new List<string> { "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
@@ -29,8 +29,7 @@ public class ImageDeliveryChannel : ICloneable
     /// </summary>
     public int DeliveryChannelPolicyId { get; set; }
 
-    public object Clone()
-    {
-        return (ImageDeliveryChannel)MemberwiseClone();
-    }
+    public ImageDeliveryChannel Clone() => (ImageDeliveryChannel)MemberwiseClone();
+
+    object ICloneable.Clone() => Clone();
 }
diff --git 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
     /// </summary>
     public DateTime Modified { get; set; }
 
-    public object Clone()
-    {
-        return (AssetApplicationMetadata)MemberwiseClone();
-    }
+    public AssetApplicationMetadata Clone() => (AssetApplicationMetadata)MemberwiseClone();
+
+    object ICloneable.Clone() => Clone();
 }
diff --git a/src/protagonist/DLCS.Model/Policies/DeliveryChannelPolicyX.cs b/src/protagonist/DLCS.Model/Policies/DeliveryChannelPolicyX.cs
@@ -29,6 +29,22 @@ public static List<SizeParameter> ThumbsDataAsSizeParameters(this DeliveryChanne
             .ToList();
     }
 
+    /// <summary>
+    /// Get timebased PolicyData as a list of strings
+    /// </summary>
+    /// <param name="deliveryChannelPolicy">Current <see cref="DeliveryChannelPolicy"/></param>
+    /// <returns>Collection of strings representing timebased policies</returns>
+    /// <exception cref="InvalidOperationException">Thrown if specified policy is not for thumbs channel</exception>
+    public static List<string> AsTimebasedPresets(this DeliveryChannelPolicy deliveryChannelPolicy)
+    {
+        if (deliveryChannelPolicy.Channel != AssetDeliveryChannels.Timebased)
+        {
+            throw new InvalidOperationException("Policy is not for timebased channel");
+        }
+
+        return deliveryChannelPolicy.PolicyDataAs<List<string>>() ?? new List<string>();
+    }
+
     /// <summary>
     /// Deserialise PolicyData as specified type
     /// </summary>

diff --git a/src/protagonist/DLCS.Repository.Tests/Messaging/EngineClientTests.cs b/src/protagonist/DLCS.Repository.Tests/Messaging/EngineClientTests.cs
@@ -173,19 +173,17 @@ 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
         var returnedAvPresets = await sut.GetAvPresets();
 
         // 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();
     }
 }
diff --git 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<string, TranscoderPreset> NullPresetDictionary =
+        new Dictionary<string, TranscoderPreset>();
 
     public EngineClient(
         IQueueLookup queueLookup,
@@ -159,18 +162,19 @@ public async Task<int> AsynchronousIngestBatch(IReadOnlyCollection<Asset> assets
     public async Task<IReadOnlyDictionary<string, TranscoderPreset>?> 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<IReadOnlyDictionary<string, TranscoderPreset>>(
-                    cancellationToken: cancellationToken);
+                    cancellationToken: cancellationToken) ?? new Dictionary<string, TranscoderPreset>();
             }
             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
@@ -35,7 +35,7 @@ public HttpResponseMessage GetResponseMessage(string content, HttpStatusCode htt
         };
         return response;
     }
-    
+
     /// <summary>
     /// Set a pre-canned response 
     /// </summary>