diff --git a/src/protagonist/API.Tests/Converters/AssetConverterTests.cs b/src/protagonist/API.Tests/Converters/AssetConverterTests.cs index 933f42d1d..d584ae340 100644 --- a/src/protagonist/API.Tests/Converters/AssetConverterTests.cs +++ b/src/protagonist/API.Tests/Converters/AssetConverterTests.cs @@ -112,7 +112,9 @@ public void ToDlcsModel_MapsMaxUnauthorised_ToOpenFullMax(int? maxUnauthorised, var asset = hydraImage.ToDlcsModel(1, 1, nameof(ToDlcsModel_MapsMaxUnauthorised_ToOpenFullMax)); asset.MaxWidth.Should().BeNull("MaxWidth is never set from MaxUnauthorised"); asset.OpenFullMax.Should().Be(openFullMax, reason); - asset.RolesList.Should().BeEquivalentTo(expectedRoles, reason); + // Roles is left null (rather than set to an empty collection) when nothing warrants assigning it, so + // that a PATCH that omits roles doesn't wipe them - expectedRoles is null for that case, not [] + asset.Roles.Should().BeEquivalentTo(expectedRoles, reason); } [Theory] @@ -194,8 +196,8 @@ public void ToDlcsModel_All_Fields_Should_Convert() asset.Reference1.Should().Be("1"); asset.Reference2.Should().Be("2"); asset.Reference3.Should().Be("3"); - asset.Roles.Split(',').Should().BeEquivalentTo(roles); - asset.Tags.Split(',').Should().BeEquivalentTo(tags); + asset.Roles.Should().BeEquivalentTo(roles); + asset.Tags.Should().BeEquivalentTo(tags); asset.DeliveryChannels.Should().BeEmpty(); asset.MaxWidth.Should().Be(512); asset.OpenFullMax.Should().Be(1000); @@ -210,8 +212,8 @@ public void ToHydra_All_Fields_Should_Convert() var created = DateTime.UtcNow.AddDays(-1).Date; var finished = DateTime.UtcNow; var origin = "https://example.org/origin"; - var roles = "role1,role2"; - var tags = "tag1tag2"; + var roles = new[] { "role1", "role2" }; + var tags = new[] { "tag1", "tag2" }; var mediaType = "image/jpeg"; var thumbnailPolicy = "thumb100"; var manifests = new List { "firstManifest" }; @@ -267,8 +269,8 @@ public void ToHydra_All_Fields_Should_Convert() hydraImage.String1.Should().Be("1"); hydraImage.String2.Should().Be("2"); hydraImage.String3.Should().Be("3"); - hydraImage.Roles.Should().BeEquivalentTo(roles.Split(',')); - hydraImage.Tags.Should().BeEquivalentTo(tags.Split(',')); + hydraImage.Roles.Should().BeEquivalentTo(roles); + hydraImage.Tags.Should().BeEquivalentTo(tags); hydraImage.DeliveryChannels.Should().BeEmpty(); hydraImage.MaxUnauthorised.Should().Be(400); hydraImage.MediaType.Should().Be(mediaType); @@ -365,7 +367,7 @@ private class MaxUnauthorisedData public static TheoryData Valid => new() { - { -1, null, 0, [], "All sizes/regions available" }, + { -1, null, 0, null, "All sizes/regions available" }, { -1, ["https://example.role"], 0, ["https://example.role"], "Nothing for anonymous" }, { 0, null, 0, ["https://dlcs.io/roles/unobtainable"], "No sizes/regions available" }, { 0, ["https://example.role"], 0, ["https://example.role"], "Nothing for anonymous" }, diff --git a/src/protagonist/API.Tests/Infrastructure/Messaging/DeliverableNotificationSenderTests.cs b/src/protagonist/API.Tests/Infrastructure/Messaging/DeliverableNotificationSenderTests.cs index a300fa6e9..a0e1426ed 100644 --- a/src/protagonist/API.Tests/Infrastructure/Messaging/DeliverableNotificationSenderTests.cs +++ b/src/protagonist/API.Tests/Infrastructure/Messaging/DeliverableNotificationSenderTests.cs @@ -169,6 +169,31 @@ public async Task SendDeliverableModifiedMessage_Asset_OmitsExpectedProperties() .And.Subject.Single().DeliveryChannelPolicy.Should().BeNull("DeliveryChannelPolicy ignored"); } + [Fact] + public async Task SendDeliverableModifiedMessage_Asset_SerialisesRolesAndTags_AsJsonArrays() + { + // Arrange - Roles/Tags are string[] on the model; confirm they hit the wire as JSON arrays, not + // comma-delimited strings (there used to be a custom converter responsible for this) + var assetId = AssetIdGenerator.GetAssetId(); + var before = new Asset(assetId) { Roles = ["clickthrough"], Tags = ["tag1", "tag2"] }; + var after = new Asset(assetId) { Roles = ["clickthrough", "logout"], Tags = [] }; + var assetModifiedRecord = NotificationRecord.Update(before, after, true); + var payload = CapturePayload(DeliverableTopicType.Asset); + + // Act + await sut.SendDeliverableModifiedMessage(assetModifiedRecord, CancellationToken.None); + + // Assert - message is serialised with camelCase property names (JsonSerializerDefaults.Web) + var messageJson = JsonNode.Parse(payload.Single().MessageContents)!; + var afterJson = messageJson["deliverableAfterUpdate"]!; + afterJson["roles"]!.GetValueKind().Should().Be(JsonValueKind.Array); + afterJson["tags"]!.GetValueKind().Should().Be(JsonValueKind.Array); + + var updated = messageJson.Deserialize>(JsonSerializerOptions.Web); + updated!.DeliverableAfterUpdate!.Roles.Should().BeEquivalentTo(after.Roles); + updated.DeliverableAfterUpdate!.Tags.Should().BeEquivalentTo(after.Tags); + } + [Fact] public async Task SendDeliverableModifiedMessage_Adjunct_OmitsExpectedProperties() { diff --git a/src/protagonist/API.Tests/Integration/GetAssetTests.cs b/src/protagonist/API.Tests/Integration/GetAssetTests.cs index 792948d5f..bb65bf2af 100644 --- a/src/protagonist/API.Tests/Integration/GetAssetTests.cs +++ b/src/protagonist/API.Tests/Integration/GetAssetTests.cs @@ -204,8 +204,8 @@ public async Task Get_Paged_Assets_Support_Ordering(int space, string assetPage, [InlineData("nonexistent")] [InlineData("ItemId")] // readonly prop on Asset [InlineData("HasRoles")] // readonly prop on Asset - [InlineData("RolesList")] // [NotMapped], can't be translated to EF query - [InlineData("TagsList")] // [NotMapped], can't be translated to EF query + [InlineData("RolesList")] // Removed field - kept for posterity + [InlineData("TagsList")] // Removed field - kept for posterity [InlineData("imageService")] // a Hydra model property but not a database-backed one [InlineData("x")] // previously silently ignored, falling back to created ordering [InlineData("adjuncts")] // a collection of related entities cannot be ordered on diff --git a/src/protagonist/API/Converters/AssetConverter.cs b/src/protagonist/API/Converters/AssetConverter.cs index 4a00d4ded..7e2387f91 100644 --- a/src/protagonist/API/Converters/AssetConverter.cs +++ b/src/protagonist/API/Converters/AssetConverter.cs @@ -52,7 +52,7 @@ public static Image ToHydra(this Asset dbAsset, UrlRoots urlRoots, bool includeA Finished = dbAsset.Finished, Ingesting = dbAsset.Ingesting, Error = dbAsset.Error, - Tags = dbAsset.TagsList.ToArray(), + Tags = dbAsset.Tags ?? [], String1 = dbAsset.Reference1, String2 = dbAsset.Reference2, String3 = dbAsset.Reference3, @@ -64,7 +64,7 @@ public static Image ToHydra(this Asset dbAsset, UrlRoots urlRoots, bool includeA Height = dbAsset.Height, MediaType = dbAsset.MediaType, Family = (AssetFamily)dbAsset.Family, - Roles = dbAsset.RolesList.ToArray(), + Roles = dbAsset.Roles ?? [], Manifests = dbAsset.Manifests?.ToArray() ?? [], Manifest = $"{urlRoots.ResourceRoot}iiif-manifest/{dbAsset.Id}", }; @@ -209,7 +209,7 @@ public static Asset ToDlcsModel(this Image hydraImage, int customerId, int? spac if (hydraImage.Tags != null) { - asset.TagsList = hydraImage.Tags; + asset.Tags = hydraImage.Tags; } SetSizeRestriction(hydraImage, asset); @@ -370,14 +370,14 @@ private static void SetSizeRestriction(Image hydraImage, Asset targetAsset) if (!hydraImage.Roles.IsNullOrEmpty()) { // If roles have been provided, use them - targetAsset.RolesList = hydraImage.Roles!; + targetAsset.Roles = hydraImage.Roles!; } else { // No roles provided but we may need to assign an unobtainable role to simulate behaviour if (maxUnauth >= 0) { - targetAsset.RolesList = [Asset.UnobtainableRole]; + targetAsset.Roles = [Asset.UnobtainableRole]; } } @@ -387,7 +387,7 @@ private static void SetSizeRestriction(Image hydraImage, Asset targetAsset) if (hydraImage.Roles != null) { - targetAsset.RolesList = hydraImage.Roles; + targetAsset.Roles = hydraImage.Roles; } if (hydraImage.MaxWidth != null) diff --git a/src/protagonist/CleanupHandler/Asset/AssetUpdatedHandler.cs b/src/protagonist/CleanupHandler/Asset/AssetUpdatedHandler.cs index 61262ef75..0438e8acd 100644 --- a/src/protagonist/CleanupHandler/Asset/AssetUpdatedHandler.cs +++ b/src/protagonist/CleanupHandler/Asset/AssetUpdatedHandler.cs @@ -57,8 +57,8 @@ public async Task HandleMessage(QueueMessage message, CancellationToken ca logger.LogDebug("Processing update Asset notification for {AssetId}", assetBefore.Id); // These are used in other checks - precompute for ease - var rolesChanged = !string.Equals(assetAfter.Roles ?? string.Empty, assetBefore.Roles ?? string.Empty, - StringComparison.OrdinalIgnoreCase); + var rolesChanged = !(assetAfter.Roles ?? []) + .SequenceEqual(assetBefore.Roles ?? [], StringComparer.OrdinalIgnoreCase); var maxWidthChanged = (assetAfter.MaxWidth ?? 0) != (assetBefore.MaxWidth ?? 0); var openFullMaxChanged = (assetBefore.OpenFullMax ?? 0) != (assetAfter.OpenFullMax ?? 0); diff --git a/src/protagonist/CleanupHandlerTests/AssetUpdatedHandlerTests.cs b/src/protagonist/CleanupHandlerTests/AssetUpdatedHandlerTests.cs index d6972da5f..0c4ce72b9 100644 --- a/src/protagonist/CleanupHandlerTests/AssetUpdatedHandlerTests.cs +++ b/src/protagonist/CleanupHandlerTests/AssetUpdatedHandlerTests.cs @@ -1274,12 +1274,13 @@ public async Task Handle_AllowsPathsToBeDelete_UsingLegacyMessageFormat() // roles [Theory] - [InlineData("", "new role")] - [InlineData(null, "new role")] - [InlineData("old role", null)] - [InlineData("old role", "")] - [InlineData("old role", "new role")] - public async Task Handle_DeletesInfoJson_WhenRolesChanged(string? rolesBefore, string? rolesAfter) + [InlineData(new string[0], new[] { "new role" })] + [InlineData(null, new[] { "new role" })] + [InlineData(new[] { "old role" }, null)] + [InlineData(new[] { "old role" }, new string[0])] + [InlineData(new[] { "old role" }, new[] { "new role" })] + [InlineData(new[] { "old role" }, new[] { "old role", "new role" })] + public async Task Handle_DeletesInfoJson_WhenRolesChanged(string[]? rolesBefore, string[]? rolesAfter) { // Arrange var requestDetails = CreateMinimalRequestDetails( @@ -1303,12 +1304,13 @@ public async Task Handle_DeletesInfoJson_WhenRolesChanged(string? rolesBefore, s } [Theory] - [InlineData("", null)] - [InlineData(null, "")] + [InlineData(new string[0], null)] + [InlineData(null, new string[0])] [InlineData(null, null)] - [InlineData("", "")] - [InlineData("ADMIN", "admin")] - public async Task Handle_DoesNotDeleteInfoJson_WhenRolesChangedBothNullOrEmptyorCaseOnly(string? rolesBefore, string? rolesAfter) + [InlineData(new string[0], new string[0])] + [InlineData(new[] { "ADMIN" }, new[] { "admin" })] + public async Task Handle_DoesNotDeleteInfoJson_WhenRolesChangedBothNullOrEmptyorCaseOnly(string[]? rolesBefore, + string[]? rolesAfter) { // Arrange var requestDetails = CreateMinimalRequestDetails( diff --git a/src/protagonist/DLCS.Core.Tests/Collections/CollectionXTests.cs b/src/protagonist/DLCS.Core.Tests/Collections/CollectionXTests.cs index 2843aa661..afdec27f0 100644 --- a/src/protagonist/DLCS.Core.Tests/Collections/CollectionXTests.cs +++ b/src/protagonist/DLCS.Core.Tests/Collections/CollectionXTests.cs @@ -182,6 +182,59 @@ public void AsArray_ReturnsExpected() list.Should().ContainSingle(i => i == item); } + [Fact] + public void ToSeparatedString_ReturnsEmptyString_IfNull() + { + IEnumerable coll = null; + + // ReSharper disable once ExpressionIsAlwaysNull + coll.ToSeparatedString().Should().BeEmpty(); + } + + [Fact] + public void ToSeparatedString_ReturnsEmptyString_IfEmpty() + { + var coll = Enumerable.Empty(); + + coll.ToSeparatedString().Should().BeEmpty(); + } + + [Fact] + public void ToSeparatedString_JoinsWithDefaultSeparator() + { + var coll = new[] { "a", "b", "c" }; + + coll.ToSeparatedString().Should().Be("a,b,c"); + } + + [Fact] + public void ToSeparatedString_JoinsWithSpecifiedSeparator() + { + var coll = new[] { "a", "b", "c" }; + + coll.ToSeparatedString('|').Should().Be("a|b|c"); + } + + [Fact] + public void ToSeparatedString_OnlyEnumeratesSourceOnce() + { + // Arrange - a lazy source that records each time it's enumerated from the start + var enumerationCount = 0; + IEnumerable Source() + { + enumerationCount++; + yield return "a"; + yield return "b"; + } + + // Act + var result = Source().ToSeparatedString(); + + // Assert + result.Should().Be("a,b"); + enumerationCount.Should().Be(1); + } + [Fact] public void AddRange_List() { diff --git a/src/protagonist/DLCS.Core/Collections/CollectionX.cs b/src/protagonist/DLCS.Core/Collections/CollectionX.cs index a7386e504..834b0cab7 100644 --- a/src/protagonist/DLCS.Core/Collections/CollectionX.cs +++ b/src/protagonist/DLCS.Core/Collections/CollectionX.cs @@ -97,6 +97,18 @@ public static IEnumerable GetDuplicates(this IEnumerable source) /// List of one item public static T[] AsArray(this T item) => [item]; + /// + /// Join collection of strings into a single string, separated by specified separator. + /// + /// Collection of strings to join + /// Separator to place between elements + /// Separator-delimited string, or if collection is null or empty. + public static string ToSeparatedString(this IEnumerable? collection, char separator = ',') + { + var values = collection?.ToArray(); + return values.IsNullOrEmpty() ? string.Empty : string.Join(separator, values); + } + /// /// Helper for adding multiple items to /// diff --git a/src/protagonist/DLCS.Model.Tests/Assets/AssetPreparerTests.cs b/src/protagonist/DLCS.Model.Tests/Assets/AssetPreparerTests.cs index 32fc993d9..c60443301 100644 --- a/src/protagonist/DLCS.Model.Tests/Assets/AssetPreparerTests.cs +++ b/src/protagonist/DLCS.Model.Tests/Assets/AssetPreparerTests.cs @@ -235,6 +235,90 @@ public void PrepareAssetForUpsert_IsBatchUpdate_DeterminesIfBatchCanBeChanged(bo result.Success.Should().Be(expectedSuccess); } + [Fact] + public void PrepareAssetForUpsert_PreservesRoles_IfNotInUpdateRequest() + { + // Arrange - simulates a PATCH that doesn't mention roles (see #1261) + var existingAsset = new Asset { Origin = "https://whatever", Roles = ["clickthrough"] }; + var updateAsset = new Asset { Origin = "https://whatever", Reference1 = "metadata edit" }; + + // Act + var result = AssetPreparer.PrepareAssetForUpsert(existingAsset, updateAsset, false, false, restrictedCharacters); + + // Assert + result.UpdatedAsset!.Roles.Should().BeEquivalentTo(["clickthrough"]); + } + + [Fact] + public void PrepareAssetForUpsert_PreservesTags_IfNotInUpdateRequest() + { + // Arrange - simulates a PATCH that doesn't mention tags (see #1261) + var existingAsset = new Asset { Origin = "https://whatever", Tags = ["existing-tag"] }; + var updateAsset = new Asset { Origin = "https://whatever", Reference1 = "metadata edit" }; + + // Act + var result = AssetPreparer.PrepareAssetForUpsert(existingAsset, updateAsset, false, false, restrictedCharacters); + + // Assert + result.UpdatedAsset!.Tags.Should().BeEquivalentTo(["existing-tag"]); + } + + [Fact] + public void PrepareAssetForUpsert_UpdatesRoles_IfInUpdateRequest() + { + // Arrange + var existingAsset = new Asset { Origin = "https://whatever", Roles = ["clickthrough"] }; + var updateAsset = new Asset { Origin = "https://whatever", Roles = ["logout"] }; + + // Act + var result = AssetPreparer.PrepareAssetForUpsert(existingAsset, updateAsset, false, false, restrictedCharacters); + + // Assert + result.UpdatedAsset!.Roles.Should().BeEquivalentTo(["logout"]); + } + + [Fact] + public void PrepareAssetForUpsert_UpdatesTags_IfInUpdateRequest() + { + // Arrange + var existingAsset = new Asset { Origin = "https://whatever", Tags = ["existing-tag"] }; + var updateAsset = new Asset { Origin = "https://whatever", Tags = ["new-tag"] }; + + // Act + var result = AssetPreparer.PrepareAssetForUpsert(existingAsset, updateAsset, false, false, restrictedCharacters); + + // Assert + result.UpdatedAsset!.Tags.Should().BeEquivalentTo(["new-tag"]); + } + + [Fact] + public void PrepareAssetForUpsert_ClearsRoles_IfEmptyCollectionInUpdateRequest() + { + // Arrange - an empty collection is an explicit "remove all roles", unlike null which is "not specified" + var existingAsset = new Asset { Origin = "https://whatever", Roles = ["clickthrough"] }; + var updateAsset = new Asset { Origin = "https://whatever", Roles = [] }; + + // Act + var result = AssetPreparer.PrepareAssetForUpsert(existingAsset, updateAsset, false, false, restrictedCharacters); + + // Assert + result.UpdatedAsset!.Roles.Should().BeEmpty(); + } + + [Fact] + public void PrepareAssetForUpsert_ClearsTags_IfEmptyCollectionInUpdateRequest() + { + // Arrange - an empty collection is an explicit "remove all tags", unlike null which is "not specified" + var existingAsset = new Asset { Origin = "https://whatever", Tags = ["existing-tag"] }; + var updateAsset = new Asset { Origin = "https://whatever", Tags = [] }; + + // Act + var result = AssetPreparer.PrepareAssetForUpsert(existingAsset, updateAsset, false, false, restrictedCharacters); + + // Assert + result.UpdatedAsset!.Tags.Should().BeEmpty(); + } + [Fact] public void PrepareAssetForUpsert_RequiresReingest_IfOriginUpdated() { diff --git a/src/protagonist/DLCS.Model.Tests/Assets/AssetTests.cs b/src/protagonist/DLCS.Model.Tests/Assets/AssetTests.cs index 6f4a1aba3..e8021efe9 100644 --- a/src/protagonist/DLCS.Model.Tests/Assets/AssetTests.cs +++ b/src/protagonist/DLCS.Model.Tests/Assets/AssetTests.cs @@ -11,11 +11,11 @@ public class AssetTests { [Theory] [InlineData(null, false)] - [InlineData("", false)] - [InlineData(" ", false)] - [InlineData("role", true)] - [InlineData("more,roles", true)] - public void HasRoles_True_IfHaveRoles(string roles, bool expected) + [InlineData(new string[0], false)] + [InlineData(new[] { " " }, false)] + [InlineData(new[] { "role" }, true)] + [InlineData(new[] { "more", "roles" }, true)] + public void HasRoles_True_IfHaveRoles(string[] roles, bool expected) { // Arrange var asset = new Asset { Roles = roles }; @@ -42,38 +42,6 @@ public void Ctor_SetsCustomerAndSpace() constructed.Space.Should().Be(assetId.Space); } - [Fact] - public void Roles_Convert_To_List() - { - var asset = new Asset { Roles = "a,b,c" }; - var expected = new[] { "a", "b", "c" }; - asset.RolesList.Should().BeEquivalentTo(expected); - } - - [Fact] - public void Roles_Convert_From_List() - { - var asset = new Asset { RolesList = ["a", "b", "c"] }; - var expected = "a,b,c"; - asset.Roles.Should().Be(expected); - } - - [Fact] - public void Tags_Convert_To_List() - { - var asset = new Asset { Tags = "a,b,c" }; - var expected = new[] { "a", "b", "c" }; - asset.TagsList.Should().BeEquivalentTo(expected); - } - - [Fact] - public void Tags_Convert_From_List() - { - var asset = new Asset { TagsList = ["a", "b", "c"] }; - var expected = "a,b,c"; - asset.Tags.Should().Be(expected); - } - [Fact] public void Clone_ClonesObject_From_List() { diff --git a/src/protagonist/DLCS.Model.Tests/Assets/AssetXTests.cs b/src/protagonist/DLCS.Model.Tests/Assets/AssetXTests.cs index 18f412ff8..55206fa62 100644 --- a/src/protagonist/DLCS.Model.Tests/Assets/AssetXTests.cs +++ b/src/protagonist/DLCS.Model.Tests/Assets/AssetXTests.cs @@ -35,7 +35,7 @@ public void GetAvailableThumbSizes_Correct_MaxWidthNoRoles() public void GetAvailableThumbSizes_Correct_IfRolesNoOpenFullMax(int? openFullMax) { // No thumb sizes are open - var asset = new Asset { Width = 5000, Height = 2500, Roles = "GoodGuys", OpenFullMax = openFullMax }; + var asset = new Asset { Width = 5000, Height = 2500, Roles = ["GoodGuys"], OpenFullMax = openFullMax }; // Act var sizes = asset.GetAvailableThumbSizes(sizeParameters, 5000); @@ -49,7 +49,7 @@ public void GetAvailableThumbSizes_Correct_IfRolesNoOpenFullMax(int? openFullMax public void GetAvailableThumbSizes_Correct_IfRolesOpenFullMax() { // Only thumbs 399px and below are available - var asset = new Asset { Width = 2500, Height = 5000, Roles = "GoodGuys", OpenFullMax = 399 }; + var asset = new Asset { Width = 2500, Height = 5000, Roles = ["GoodGuys"], OpenFullMax = 399 }; // Act var sizes = asset.GetAvailableThumbSizes(sizeParameters, 5000); @@ -174,7 +174,7 @@ public void GetLargestOpenFullSize_ReturnsMaxWidth_IfNoRoles(int? maxWidth, int [InlineData(0)] public void GetLargestOpenFullSize_Returns0_IfOpenFullMaxUnset_AndHasRoles(int? openFullMax) { - var asset = new Asset { OpenFullMax = openFullMax, Roles = "https://test.role" }; + var asset = new Asset { OpenFullMax = openFullMax, Roles = ["https://test.role"] }; asset.GetLargestOpenFullSize(1000).Should().Be(0); } @@ -186,7 +186,7 @@ public void GetLargestOpenFullSize_Returns0_IfOpenFullMaxUnset_AndHasRoles(int? public void GetLargestOpenFullSize_ReturnsSmallestOfAvailableValues_IfHasRoles(int? openFullMax, int? maxWidth, int systemMaxWidth, int expected, string because) { - var asset = new Asset { MaxWidth = maxWidth, OpenFullMax = openFullMax, Roles = "https://test.role" }; + var asset = new Asset { MaxWidth = maxWidth, OpenFullMax = openFullMax, Roles = ["https://test.role"] }; asset.GetLargestOpenFullSize(systemMaxWidth).Should().Be(expected, because); } diff --git a/src/protagonist/DLCS.Model/Assets/Asset.cs b/src/protagonist/DLCS.Model/Assets/Asset.cs index dd84e075a..739c2809a 100644 --- a/src/protagonist/DLCS.Model/Assets/Asset.cs +++ b/src/protagonist/DLCS.Model/Assets/Asset.cs @@ -1,8 +1,7 @@ using System; using System.Collections.Generic; -using System.ComponentModel.DataAnnotations.Schema; using System.Linq; -using DLCS.Core.Collections; +using DLCS.Core.Strings; using DLCS.Core.Types; using DLCS.Model.Assets.Metadata; @@ -25,8 +24,8 @@ public class Asset : IDeliverable public DateTime? Created { get; set; } /// public string? Origin { get; set; } - public string? Tags { get; set; } - public string? Roles { get; set; } + public string[]? Tags { get; set; } + public string[]? Roles { get; set; } public string? PreservedUri { get; set; } public string? Reference1 { get; set; } public string? Reference2 { get; set; } @@ -87,45 +86,10 @@ public class Asset : IDeliverable /// public string[] DeliveryChannels { get; set; } = Array.Empty(); - private IEnumerable? rolesList; - - // TODO - map this via Dapper on way out of DB? - [NotMapped] - public IEnumerable RolesList - { - get - { - if (rolesList == null && !string.IsNullOrEmpty(Roles)) - { - rolesList = Roles.Split(",", StringSplitOptions.RemoveEmptyEntries); - } - - return rolesList ??= Enumerable.Empty(); - } - set => Roles = value.IsNullOrEmpty() ? String.Empty : String.Join(',', value); - } - - private IEnumerable? tagsList; - - [NotMapped] - public IEnumerable TagsList - { - get - { - if (tagsList == null && !string.IsNullOrEmpty(Tags)) - { - tagsList = Tags.Split(",", StringSplitOptions.RemoveEmptyEntries); - } - - return tagsList ??= Enumerable.Empty(); - } - set => Tags = value.IsNullOrEmpty() ? String.Empty : String.Join(',', value); - } - /// /// Indicates whether this asset has any roles assigned to it. /// - public bool HasRoles => !string.IsNullOrWhiteSpace(Roles); + public bool HasRoles => Roles?.Any(r => r.HasText()) ?? false; /// /// A list of image delivery channels attached to this asset diff --git a/src/protagonist/DLCS.Model/Assets/AssetPreparer.cs b/src/protagonist/DLCS.Model/Assets/AssetPreparer.cs index c27df1d8b..59bee5828 100644 --- a/src/protagonist/DLCS.Model/Assets/AssetPreparer.cs +++ b/src/protagonist/DLCS.Model/Assets/AssetPreparer.cs @@ -344,8 +344,8 @@ static AssetPreparer() Space = 0, Created = DateTime.MinValue.ToUniversalTime(), Origin = string.Empty, - Tags = string.Empty, - Roles = string.Empty, + Tags = [], + Roles = [], PreservedUri = string.Empty, Reference1 = string.Empty, Reference2 = string.Empty, diff --git a/src/protagonist/DLCS.Repository.Tests/Messaging/EngineClientTests.cs b/src/protagonist/DLCS.Repository.Tests/Messaging/EngineClientTests.cs index a143c40ae..214e44f2f 100644 --- a/src/protagonist/DLCS.Repository.Tests/Messaging/EngineClientTests.cs +++ b/src/protagonist/DLCS.Repository.Tests/Messaging/EngineClientTests.cs @@ -54,8 +54,8 @@ public async Task SynchronousIngest_CallsEngine(int? batchId) var asset = new Asset(AssetId.FromString("99/1/ingest-asset")) { Family = AssetFamily.Image, - Tags = "whatever", - Roles = "secure", + Tags = ["whatever"], + Roles = ["secure"], NumberReference1 = 1234, Batch = batchId }; @@ -93,8 +93,8 @@ public async Task AsynchronousIngest_QueuesMessage(int? batchId) var asset = new Asset(AssetId.FromString("99/1/ingest-asset")) { Family = AssetFamily.Image, - Tags = "whatever", - Roles = "secure", + Tags = ["whatever"], + Roles = ["secure"], NumberReference1 = 1234, Batch = batchId }; diff --git a/src/protagonist/DLCS.Repository/DlcsContext.cs b/src/protagonist/DLCS.Repository/DlcsContext.cs index ddc2232b2..cf7490a7d 100644 --- a/src/protagonist/DLCS.Repository/DlcsContext.cs +++ b/src/protagonist/DLCS.Repository/DlcsContext.cs @@ -393,11 +393,19 @@ protected override void OnModelCreating(ModelBuilder modelBuilder) entity.Property(e => e.Roles) .IsRequired() - .HasMaxLength(1000); + .HasMaxLength(1000) + .HasConversion( + r => string.Join(",", r), + r => r.Split(",", StringSplitOptions.RemoveEmptyEntries), + stringArrayComparer); entity.Property(e => e.Tags) .IsRequired() - .HasMaxLength(1000); + .HasMaxLength(1000) + .HasConversion( + t => string.Join(",", t), + t => t.Split(",", StringSplitOptions.RemoveEmptyEntries), + stringArrayComparer); entity.Property(e => e.ThumbnailPolicy) .IsRequired() diff --git a/src/protagonist/Engine.Tests/Ingest/Image/ThumbCreatorTests.cs b/src/protagonist/Engine.Tests/Ingest/Image/ThumbCreatorTests.cs index dd23cbf75..225b9451d 100644 --- a/src/protagonist/Engine.Tests/Ingest/Image/ThumbCreatorTests.cs +++ b/src/protagonist/Engine.Tests/Ingest/Image/ThumbCreatorTests.cs @@ -246,7 +246,7 @@ public async Task CreateNewThumbs_UploadsExpected_LargestAuth_Roles(int openFull var asset = new Asset(assetId) { Width = 3030, Height = 5000, MaxWidth = maxWidth, OpenFullMax = openFullMax, - ImageDeliveryChannels = thumbsDeliveryChannel, Roles = "https://test" + ImageDeliveryChannels = thumbsDeliveryChannel, Roles = ["https://test"] }; var imagesOnDisk = new List @@ -292,7 +292,7 @@ public async Task CreateNewThumbs_UploadsExpected_LargestExcluded_Roles(int open var asset = new Asset(assetId) { Width = 3030, Height = 5000, MaxWidth = maxWidth, OpenFullMax = openFullMax, - ImageDeliveryChannels = thumbsDeliveryChannel, Roles = "https://test" + ImageDeliveryChannels = thumbsDeliveryChannel, Roles = ["https://test"] }; var imagesOnDisk = new List @@ -333,7 +333,7 @@ public async Task CreateNewThumbs_UploadsExpected_AuthMatchesImageSize_OpenFullM var assetId = new AssetId(10, 20, "foo"); var asset = new Asset(assetId) { - Width = 3030, Height = 5000, OpenFullMax = 500, Roles = "https://test", + Width = 3030, Height = 5000, OpenFullMax = 500, Roles = ["https://test"], ImageDeliveryChannels = thumbsDeliveryChannel }; @@ -463,7 +463,7 @@ public async Task CreateNewThumbs_UploadsExpected_AllAuth() var assetId = new AssetId(10, 20, "foo"); var asset = new Asset(assetId) { - Width = 3030, Height = 5000, OpenFullMax = 0, Roles = "https://test", + Width = 3030, Height = 5000, OpenFullMax = 0, Roles = ["https://test"], ImageDeliveryChannels = thumbsDeliveryChannel, MaxWidth = 0 }; @@ -506,7 +506,7 @@ public async Task CreateNewThumbs_UploadsNone_IfMaxWidthSmallerThanSmallestThumb var assetId = new AssetId(10, 20, "foo"); var asset = new Asset(assetId) { - Width = 3030, Height = 5000, OpenFullMax = 0, Roles = "https://test", + Width = 3030, Height = 5000, OpenFullMax = 0, Roles = ["https://test"], ImageDeliveryChannels = thumbsDeliveryChannel, MaxWidth = 90 }; diff --git a/src/protagonist/Orchestrator.Tests/Assets/MemoryAssetTrackerTests.cs b/src/protagonist/Orchestrator.Tests/Assets/MemoryAssetTrackerTests.cs index a9b89561f..abff50042 100644 --- a/src/protagonist/Orchestrator.Tests/Assets/MemoryAssetTrackerTests.cs +++ b/src/protagonist/Orchestrator.Tests/Assets/MemoryAssetTrackerTests.cs @@ -228,11 +228,11 @@ public async Task GetOrchestrationAssetT_ReturnsOrchestrationImage(string delive } [Theory] - [InlineData("", 0, null)] - [InlineData("", 100, null)] - [InlineData("role", 0, 0)] - [InlineData("role", 100, 100)] - public async Task GetOrchestrationAsset_SetsOpenFullMax_IfHasRole(string roles, int openFullMax, int? expected) + [InlineData(new string[0], 0, null)] + [InlineData(new string[0], 100, null)] + [InlineData(new[] { "role" }, 0, 0)] + [InlineData(new[] { "role" }, 100, 100)] + public async Task GetOrchestrationAsset_SetsOpenFullMax_IfHasRole(string[] roles, int openFullMax, int? expected) { // Arrange var imageDeliveryChannels = "iiif-img".GenerateDeliveryChannels(); @@ -360,9 +360,9 @@ public async Task GetOrchestrationAssetT_Null_IfWrongTypeAskedFor(string deliver } [Theory] - [InlineData("", false)] - [InlineData("role", true)] - public async Task GetOrchestrationAsset_SetsRequiresAuth_BaseOnRoles(string roles, bool requiresAuth) + [InlineData(new string[0], false)] + [InlineData(new[] { "role" }, true)] + public async Task GetOrchestrationAsset_SetsRequiresAuth_BaseOnRoles(string[] roles, bool requiresAuth) { // Arrange var imageDeliveryChannels = "iiif-img".GenerateDeliveryChannels(); diff --git a/src/protagonist/Orchestrator.Tests/Infrastructure/MetadataWithFallbackThumbSizeProviderTests.cs b/src/protagonist/Orchestrator.Tests/Infrastructure/MetadataWithFallbackThumbSizeProviderTests.cs index 3cdd7e862..05ed6c04a 100644 --- a/src/protagonist/Orchestrator.Tests/Infrastructure/MetadataWithFallbackThumbSizeProviderTests.cs +++ b/src/protagonist/Orchestrator.Tests/Infrastructure/MetadataWithFallbackThumbSizeProviderTests.cs @@ -109,7 +109,7 @@ private static Asset GetAssetWithThumbsChannel(AssetId assetId, int w, int h, in Width = w, Height = h, OpenFullMax = openFullMax, - Roles = "https://role.example", + Roles = ["https://role.example"], ImageDeliveryChannels = new List { new() diff --git a/src/protagonist/Orchestrator.Tests/Infrastructure/NamedQueries/PDF/FireballPdfCreatorTests.cs b/src/protagonist/Orchestrator.Tests/Infrastructure/NamedQueries/PDF/FireballPdfCreatorTests.cs index 3e7d67493..e92bef6f9 100644 --- a/src/protagonist/Orchestrator.Tests/Infrastructure/NamedQueries/PDF/FireballPdfCreatorTests.cs +++ b/src/protagonist/Orchestrator.Tests/Infrastructure/NamedQueries/PDF/FireballPdfCreatorTests.cs @@ -199,31 +199,31 @@ public async Task CreatePdf_RedactsNotWhitelistedRoles() { new() { - Roles = "whitelist", + Roles = ["whitelist"], Id = AssetId.FromString("/99/1/image1.jpg"), OpenFullMax = 0 }, new() { - Roles = "whitelist,notwhitelist", + Roles = ["whitelist", "notwhitelist"], Id = AssetId.FromString("/99/1/image1.jpg"), OpenFullMax = 0 }, new() { - Roles = "notwhitelist", + Roles = ["notwhitelist"], Id = AssetId.FromString("/99/1/image1.jpg"), OpenFullMax = 0 }, new() { - Roles = string.Empty, + Roles = [], Id = AssetId.FromString("/99/1/image1.jpg"), OpenFullMax = 0 }, new() { - Roles = string.Empty, + Roles = [], Id = AssetId.FromString("/99/1/image1.jpg"), OpenFullMax = 0 } diff --git a/src/protagonist/Orchestrator/Assets/MemoryAssetTracker.cs b/src/protagonist/Orchestrator/Assets/MemoryAssetTracker.cs index bd99887eb..fe0f1541b 100644 --- a/src/protagonist/Orchestrator/Assets/MemoryAssetTracker.cs +++ b/src/protagonist/Orchestrator/Assets/MemoryAssetTracker.cs @@ -1,9 +1,9 @@ using System; using System.Linq; using System.Threading.Tasks; -using DLCS.AWS.S3; using DLCS.Core.Caching; using DLCS.Core.Guard; +using DLCS.Core.Strings; using DLCS.Core.Types; using DLCS.Model.Assets; using DLCS.Model.Customers; @@ -229,7 +229,7 @@ OrchestrationAsset SetDefaults() orchestrationAsset.Channels |= AvailableDeliveryChannel.Timebased; orchestrationAsset.AssetId = assetId; - orchestrationAsset.Roles = asset.RolesList.ToList(); + orchestrationAsset.Roles = asset.Roles?.ToList() ?? []; orchestrationAsset.RequiresAuth = asset.HasRoles; return orchestrationAsset; } diff --git a/src/protagonist/Orchestrator/Infrastructure/DataAccess/DapperAssetRepository.cs b/src/protagonist/Orchestrator/Infrastructure/DataAccess/DapperAssetRepository.cs index 6763474b8..80c28b371 100644 --- a/src/protagonist/Orchestrator/Infrastructure/DataAccess/DapperAssetRepository.cs +++ b/src/protagonist/Orchestrator/Infrastructure/DataAccess/DapperAssetRepository.cs @@ -1,6 +1,7 @@ using System.Collections.Generic; using System.Linq; using System.Threading.Tasks; +using DLCS.Core.Strings; using DLCS.Core.Types; using DLCS.Model.Assets; using DLCS.Repository; @@ -66,9 +67,9 @@ public class DapperAssetRepository( Reference1 = firstAsset.Reference1, Reference2 = firstAsset.Reference2, Reference3 = firstAsset.Reference3, - Roles = firstAsset.Roles, + Roles = SplitDelimited(firstAsset.Roles), Space = firstAsset.Space, - Tags = firstAsset.Tags, + Tags = SplitDelimited(firstAsset.Tags), Width = firstAsset.Width, MaxUnauthorised = firstAsset.MaxUnauthorised, MaxWidth = firstAsset.MaxWidth, @@ -87,6 +88,10 @@ public class DapperAssetRepository( }; } + // Roles + Tags are stored as comma-delimited strings; EF handles this via a value-converter but Dapper doesn't + private static string[] SplitDelimited(string? value) + => value.SplitSeparatedString(",").ToArray(); + private List GenerateImageDeliveryChannels(List rawAsset) { var imageDeliveryChannels = new List(); diff --git a/src/protagonist/Orchestrator/Infrastructure/IIIF/Manifests/ManifestBuilderUtils.cs b/src/protagonist/Orchestrator/Infrastructure/IIIF/Manifests/ManifestBuilderUtils.cs index 21fa8c61f..0d12c9537 100644 --- a/src/protagonist/Orchestrator/Infrastructure/IIIF/Manifests/ManifestBuilderUtils.cs +++ b/src/protagonist/Orchestrator/Infrastructure/IIIF/Manifests/ManifestBuilderUtils.cs @@ -159,8 +159,8 @@ public static Dictionary GetCanvasMetadata(Asset asset) => { "Number 1", (asset.NumberReference1 ?? 0).ToString() }, { "Number 2", (asset.NumberReference2 ?? 0).ToString() }, { "Number 3", (asset.NumberReference3 ?? 0).ToString() }, - { "Tags", asset.Tags ?? string.Empty }, - { "Roles", asset.Roles ?? string.Empty } + { "Tags", asset.Tags.ToSeparatedString() }, + { "Roles", asset.Roles.ToSeparatedString() } }; public static Dictionary GetManifestMetadata() => diff --git a/src/protagonist/Orchestrator/Infrastructure/IIIF/Manifests/ManifestV3Builder.cs b/src/protagonist/Orchestrator/Infrastructure/IIIF/Manifests/ManifestV3Builder.cs index af733e2b8..83bf3b35e 100644 --- a/src/protagonist/Orchestrator/Infrastructure/IIIF/Manifests/ManifestV3Builder.cs +++ b/src/protagonist/Orchestrator/Infrastructure/IIIF/Manifests/ManifestV3Builder.cs @@ -463,7 +463,7 @@ private string GetFilePath(Asset asset, CustomerPathElement customerPathElement) var taskList = new List(assetsRequiringAuthCount); foreach (var asset in assetsRequiringAuth) { - taskList.Add(authBuilder.GetAuthServicesForAsset(asset.Id, asset.RolesList.ToList(), cancellationToken) + taskList.Add(authBuilder.GetAuthServicesForAsset(asset.Id, asset.Roles?.ToList() ?? [], cancellationToken) .ContinueWith(antecedent => { if (antecedent.Result is AuthProbeService2 probeService2) diff --git a/src/protagonist/Orchestrator/Infrastructure/NamedQueries/PDF/FireballPdfCreator.cs b/src/protagonist/Orchestrator/Infrastructure/NamedQueries/PDF/FireballPdfCreator.cs index fc776069e..68fec363e 100644 --- a/src/protagonist/Orchestrator/Infrastructure/NamedQueries/PDF/FireballPdfCreator.cs +++ b/src/protagonist/Orchestrator/Infrastructure/NamedQueries/PDF/FireballPdfCreator.cs @@ -9,6 +9,7 @@ using DLCS.AWS.S3; using DLCS.AWS.S3.Models; using DLCS.Core.Guard; +using DLCS.Core.Strings; using DLCS.Model.Assets; using DLCS.Model.Assets.NamedQueries; using DLCS.Web.Response; @@ -131,8 +132,8 @@ private CustomerOverride GetCustomerOverride(PdfParsedNamedQuery parsedNamedQuer ? overrides : new CustomerOverride(); - private static bool RolesAreOnWhitelist(Asset i, CustomerOverride overrides) - => i.RolesList.All(r => overrides.PdfRolesWhitelist.Contains(r)); + private static bool RolesAreOnWhitelist(Asset i, CustomerOverride overrides) + => i.Roles?.All(r => overrides.PdfRolesWhitelist.Contains(r)) ?? true; private async Task CallFireball(FireballPlaybook playbook, string pdfKey, CancellationToken cancellationToken) diff --git a/src/protagonist/Orchestrator/Infrastructure/NamedQueries/Persistence/BaseProjectionCreator.cs b/src/protagonist/Orchestrator/Infrastructure/NamedQueries/Persistence/BaseProjectionCreator.cs index 773e054e1..8b6c731b1 100644 --- a/src/protagonist/Orchestrator/Infrastructure/NamedQueries/Persistence/BaseProjectionCreator.cs +++ b/src/protagonist/Orchestrator/Infrastructure/NamedQueries/Persistence/BaseProjectionCreator.cs @@ -5,6 +5,7 @@ using System.Threading.Tasks; using DLCS.AWS.S3; using DLCS.Core.Collections; +using DLCS.Core.Strings; using DLCS.Model.Assets; using DLCS.Model.Assets.NamedQueries; using DLCS.Repository.NamedQueries.Models; @@ -101,7 +102,7 @@ private List GetRelevantRoles(List assets, StoredParsedNamedQuery ? overrides.PdfRolesWhitelist : Enumerable.Empty(); - var distinctRoles = assets.SelectMany(a => a.RolesList).Distinct().ToList(); + var distinctRoles = assets.SelectMany(a => a.Roles ?? []).Distinct().ToList(); var relevantRoles = distinctRoles.Intersect(whitelistRoles).ToList(); return relevantRoles; } diff --git a/src/protagonist/Test.Helpers/Integration/DatabaseTestDataPopulation.cs b/src/protagonist/Test.Helpers/Integration/DatabaseTestDataPopulation.cs index 0778e133a..65bb02486 100644 --- a/src/protagonist/Test.Helpers/Integration/DatabaseTestDataPopulation.cs +++ b/src/protagonist/Test.Helpers/Integration/DatabaseTestDataPopulation.cs @@ -3,6 +3,7 @@ using System.Linq; using System.Threading.Tasks; using DLCS.Core; +using DLCS.Core.Strings; using DLCS.Core.Types; using DLCS.Model.Assets; using DLCS.Model.Assets.CustomHeaders; @@ -54,12 +55,13 @@ public static ValueTask> AddTestAsset(this DbSet asset return assets.AddAsync(new Asset { Created = DateTime.UtcNow, Customer = customer, Space = space, Id = id, Origin = origin, - Width = width, Height = height, Roles = roles, Family = family, MediaType = mediaType, + Width = width, Height = height, Roles = roles.SplitSeparatedString(",").ToArray(), Family = family, + MediaType = mediaType, ThumbnailPolicy = thumbnailPolicy, MaxUnauthorised = -1, MaxWidth = maxWidth, OpenFullMax = openFullMax, Reference1 = ref1, Reference2 = ref2, Reference3 = ref3, NumberReference1 = num1, NumberReference2 = num2, NumberReference3 = num3, - NotForDelivery = notForDelivery, Tags = "", PreservedUri = "", Error = error, + NotForDelivery = notForDelivery, Tags = [], PreservedUri = "", Error = error, ImageOptimisationPolicy = imageOptimisationPolicy, Batch = batch, Ingesting = ingesting, Duration = duration, Finished = finished, Manifests = manifests, ImageDeliveryChannels = imageDeliveryChannels ?? new List()