Stop roles and tags from being wiped on PATCH

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<string> { "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<int?, string[], int?, string[], string> 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" },

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<Asset>.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<DeliverableUpdatedNotification<Asset>>(JsonSerializerOptions.Web);
+        updated!.DeliverableAfterUpdate!.Roles.Should().BeEquivalentTo(after.Roles);
+        updated.DeliverableAfterUpdate!.Tags.Should().BeEquivalentTo(after.Tags);
+    }
+
     [Fact]
     public async Task SendDeliverableModifiedMessage_Adjunct_OmitsExpectedProperties()
     {

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

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)

src/protagonist/CleanupHandler/Asset/AssetUpdatedHandler.cs

@@ -57,8 +57,8 @@ public async Task<bool> 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);
 

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(

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<string> coll = null;
+
+        // ReSharper disable once ExpressionIsAlwaysNull
+        coll.ToSeparatedString().Should().BeEmpty();
+    }
+
+    [Fact]
+    public void ToSeparatedString_ReturnsEmptyString_IfEmpty()
+    {
+        var coll = Enumerable.Empty<string>();
+
+        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<string> 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()
     {

src/protagonist/DLCS.Core/Collections/CollectionX.cs

@@ -97,6 +97,18 @@ public static IEnumerable<T> GetDuplicates<T>(this IEnumerable<T> source)
     /// <returns>List of one item</returns>
     public static T[] AsArray<T>(this T item) => [item];
 
+    /// <summary>
+    /// Join collection of strings into a single string, separated by specified separator.
+    /// </summary>
+    /// <param name="collection">Collection of strings to join</param>
+    /// <param name="separator">Separator to place between elements</param>
+    /// <returns>Separator-delimited string, or <see cref="string.Empty"/> if collection is null or empty.</returns>
+    public static string ToSeparatedString(this IEnumerable<string>? collection, char separator = ',')
+    {
+        var values = collection?.ToArray();
+        return values.IsNullOrEmpty() ? string.Empty : string.Join(separator, values);
+    }
+
     /// <summary>
     /// Helper for adding multiple items to <see cref="ICollection{T}"/>
     /// </summary>

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()
     {