From e6e59c1e72dd927bbb31fe0caae36a600738a34d Mon Sep 17 00:00:00 2001 From: Donald Gray Date: Wed, 5 Jun 2024 14:22:42 +0100 Subject: [PATCH 1/2] Ensure ImageDeliveryChannels are ordered in API results --- .../Assets/AssetQueryXTests.cs | 56 +++++++++++++++++++ .../DLCS.Repository/Assets/AssetQueryX.cs | 5 +- 2 files changed, 59 insertions(+), 2 deletions(-) create mode 100644 src/protagonist/DLCS.Repository.Tests/Assets/AssetQueryXTests.cs diff --git a/src/protagonist/DLCS.Repository.Tests/Assets/AssetQueryXTests.cs b/src/protagonist/DLCS.Repository.Tests/Assets/AssetQueryXTests.cs new file mode 100644 index 000000000..e216fa693 --- /dev/null +++ b/src/protagonist/DLCS.Repository.Tests/Assets/AssetQueryXTests.cs @@ -0,0 +1,56 @@ +using System.Collections.Generic; +using System.Linq; +using DLCS.Model.Assets; +using DLCS.Model.Policies; +using DLCS.Repository.Assets; +using Microsoft.EntityFrameworkCore; +using Test.Helpers.Data; +using Test.Helpers.Integration; + +namespace DLCS.Repository.Tests.Assets; + +[Trait("Category", "Database")] +[Collection(DatabaseCollection.CollectionName)] +public class AssetQueryXTests +{ + private readonly DlcsContext dbContext; + + public AssetQueryXTests(DlcsDatabaseFixture dbFixture) + { + dbContext = dbFixture.DbContext; + dbFixture.CleanUp(); + } + + [Fact] + public async Task IncludeDeliveryChannelsWithPolicy_ReturnsDeliveryChannels_ByOrderOfChannel() + { + var assetId = AssetIdGenerator.GetAssetId(); + await dbContext.ImageDeliveryChannels.AddRangeAsync( + new() + { + ImageId = assetId, Channel = "gamma", + DeliveryChannelPolicyId = KnownDeliveryChannelPolicies.ImageDefault + }, + new() + { + ImageId = assetId, Channel = "alpha", + DeliveryChannelPolicyId = KnownDeliveryChannelPolicies.ImageDefault + }, + new() + { + ImageId = assetId, Channel = "beta", + DeliveryChannelPolicyId = KnownDeliveryChannelPolicies.ImageDefault + }); + await dbContext.Images.AddTestAsset(assetId); + await dbContext.SaveChangesAsync(); + + // Act + var result = await dbContext.Images + .Where(i => i.Id == assetId) + .IncludeDeliveryChannelsWithPolicy() + .ToListAsync(); + + // Assert + result.Single().ImageDeliveryChannels.Should().BeInAscendingOrder(idc => idc.Channel); + } +} \ No newline at end of file diff --git a/src/protagonist/DLCS.Repository/Assets/AssetQueryX.cs b/src/protagonist/DLCS.Repository/Assets/AssetQueryX.cs index 7bf97f454..64cd06037 100644 --- a/src/protagonist/DLCS.Repository/Assets/AssetQueryX.cs +++ b/src/protagonist/DLCS.Repository/Assets/AssetQueryX.cs @@ -115,11 +115,12 @@ public static IQueryable ApplyAssetFilter(this IQueryable queryabl return filtered; } - + /// /// Include asset delivery channels and their associated policies. /// public static IQueryable IncludeDeliveryChannelsWithPolicy(this IQueryable assetQuery) - => assetQuery.Include(a => a.ImageDeliveryChannels) + => assetQuery + .Include(a => a.ImageDeliveryChannels.OrderBy(idc => idc.Channel)) .ThenInclude(dc => dc.DeliveryChannelPolicy); } \ No newline at end of file From 751a7dd8b79a71c216a698dbb1d768a0e237d751 Mon Sep 17 00:00:00 2001 From: Donald Gray Date: Wed, 5 Jun 2024 14:28:52 +0100 Subject: [PATCH 2/2] Small tweaks in API asset processing classes --- .../API/Features/Image/Ingest/AssetProcessor.cs | 5 ++--- .../Image/Ingest/DeliveryChannelProcessor.cs | 17 ++++++----------- .../DLCS.Repository/Assets/AssetQueryX.cs | 3 +-- 3 files changed, 9 insertions(+), 16 deletions(-) diff --git a/src/protagonist/API/Features/Image/Ingest/AssetProcessor.cs b/src/protagonist/API/Features/Image/Ingest/AssetProcessor.cs index e832730e1..1d7ca6d98 100644 --- a/src/protagonist/API/Features/Image/Ingest/AssetProcessor.cs +++ b/src/protagonist/API/Features/Image/Ingest/AssetProcessor.cs @@ -49,10 +49,9 @@ public async Task Process(AssetBeforeProcessing assetBeforeP Func? requiresReingestPreSave = null, CancellationToken cancellationToken = default) { - Asset? existingAsset; try { - existingAsset = await assetRepository.GetAsset(assetBeforeProcessing.Asset.Id, true); + var existingAsset = await assetRepository.GetAsset(assetBeforeProcessing.Asset.Id, true); if (existingAsset == null) { @@ -98,7 +97,7 @@ public async Task Process(AssetBeforeProcessing assetBeforeP return new ProcessAssetResult { Result = ModifyEntityResult.Failure( - "Delivery channels are required when updating an existing Asset via PUT", + "Delivery channels are required when updating an existing Asset", WriteResult.BadRequest ) }; diff --git a/src/protagonist/API/Features/Image/Ingest/DeliveryChannelProcessor.cs b/src/protagonist/API/Features/Image/Ingest/DeliveryChannelProcessor.cs index da8198c90..eb32c4648 100644 --- a/src/protagonist/API/Features/Image/Ingest/DeliveryChannelProcessor.cs +++ b/src/protagonist/API/Features/Image/Ingest/DeliveryChannelProcessor.cs @@ -45,12 +45,9 @@ public async Task ProcessImageDeliveryChannels(Asset? existingAsset, Asset deliveryChannelsBeforeProcessing, existingAsset != null); return deliveryChannelChanged; } - catch (InvalidOperationException) + catch (InvalidOperationException ioEx) { - throw new APIException("Failed to match delivery channel policy") - { - StatusCode = 400 - }; + throw new BadRequestException("Failed to match delivery channel policy", ioEx); } } @@ -160,7 +157,7 @@ private async Task SetImageDeliveryChannels(Asset asset, DeliveryChannelsB private async Task GetDeliveryChannelPolicy(Asset asset, DeliveryChannelsBeforeProcessing deliveryChannel) { DeliveryChannelPolicy deliveryChannelPolicy; - if (deliveryChannel.Policy.IsNullOrEmpty()) + if (string.IsNullOrEmpty(deliveryChannel.Policy)) { deliveryChannelPolicy = await defaultDeliveryChannelRepository.MatchDeliveryChannelPolicyForChannel( asset.MediaType!, asset.Space, asset.Customer, deliveryChannel.Channel); @@ -201,11 +198,9 @@ await defaultDeliveryChannelRepository.MatchedDeliveryChannels(asset.MediaType!, if (matchedDeliveryChannels.Any(x => x.Channel == AssetDeliveryChannels.None) && matchedDeliveryChannels.Count != 1) { - throw new APIException("An asset can only be automatically assigned a delivery channel of type 'None' when it is the only one available. " + - "Please check your default delivery channel configuration.") - { - StatusCode = 400 - }; + throw new BadRequestException( + "An asset can only be automatically assigned a delivery channel of type 'None' when it is the only one available. " + + "Please check your default delivery channel configuration."); } foreach (var deliveryChannel in matchedDeliveryChannels) diff --git a/src/protagonist/DLCS.Repository/Assets/AssetQueryX.cs b/src/protagonist/DLCS.Repository/Assets/AssetQueryX.cs index 64cd06037..f2d5986b8 100644 --- a/src/protagonist/DLCS.Repository/Assets/AssetQueryX.cs +++ b/src/protagonist/DLCS.Repository/Assets/AssetQueryX.cs @@ -26,7 +26,7 @@ public static IQueryable AsOrderedAssetQuery(this IQueryable asset /// The orderBy field can be the API version of property or the full property version. /// Defaults to "Created" field ordering if no field specified. /// - public static IQueryable AsOrderedAssetQuery(this IQueryable assetQuery, string? orderBy, + private static IQueryable AsOrderedAssetQuery(this IQueryable assetQuery, string? orderBy, bool descending = false) { var field = GetPropertyName(orderBy); @@ -57,7 +57,6 @@ private static string GetPropertyName(string? orderBy) }; } - // Create an Expression from the PropertyName. // I think Split(".") handles nested properties maybe - seems unnecessary but from an SO post // "x" means nothing when creating the Parameter, it's just used for debug messages