Stop roles and tags from being wiped on PATCH

[JackLewis-digirati]

What does this change?

Resolves #1261

This PR stops roles and tags from being cleared down on PATCH requests. It also removes the NotMapped RolesList and TagsList properties in favour of using Tags and Roles directly

[JackLewis-digirati]

How the bug worked

Two defects compounded (full mechanism in #1261):

1. The wipe. AssetPreparer.PrepareAssetForUpsert applies a PATCH via ChangeManager.ApplyChanges, which reflects over every public writable property of the candidate object, not just Roles/Tags. Asset.RolesList/TagsList were [NotMapped] convenience properties whose getters never returned null — for an omitted field they returned Enumerable.Empty<string>(). Reflection doesn't know or care about [NotMapped]; it saw a non-null "change" and wrote Roles = "" even though the request never mentioned roles.
2. The masking. RolesList's setter wrote the backing Roles string but never invalidated the private rolesList cache, which had just been populated moments earlier (reading the existing value for the ApplyChanges comparison). So the PATCH response kept showing the pre-wipe roles via the stale cache, while the DB now held "".

Why remove RolesList/TagsList entirely, rather than a narrower fix

Options considered, cheapest to most invasive:

• Add RolesList/TagsList to ApplyChanges' ignore list (the way Manifests already is) — smallest diff, but leaves the trap live for the next piece of reflection/mapping/serialization code that touches Asset (exactly the class of bug Manifests' own exclusion already hints at). Every future addition has to remember to extend the ignore list.
• Make them getter-only — ChangeManager.ApplyChanges already skips non-writable properties (if (!prop.CanWrite) continue;), so this would've fixed ApplyChanges specifically. But it keeps the shape that caused the problem: something that reads like ordinary object state to any other generic/reflective code (Clone()'s MemberwiseClone, a future AutoMapper profile, System.Text.Json, a future EF change that stops respecting [NotMapped]). The caching behaviour — the actual cause of the masking half of the bug — also isn't needed for anything: the lists are tiny and read a handful of times per request at most.
• Remove both properties entirely (what this PR does) — usage turned out to be narrow enough to make this the cheap option, not just the "correct" one:
  • Setter: only 4 call sites, all in AssetConverter.cs (mapping an inbound Hydra Image PATCH/PUT body onto an Asset).
  • Getter: 5 read sites (AssetConverter.cs's outbound response mapping, plus MemoryAssetTracker, ManifestV3Builder, BaseProjectionCreator, FireballPdfCreator in Orchestrator).
  • All 9 call sites now go through DLCS.Core.Strings.StringX.SplitSeparatedString (already existed, already tested) on read, and a new DLCS.Core.Collections.CollectionX.ToSeparatedString (added here) on write — the same join logic the old setter had, just as a static helper instead of object state, so nothing generic walking Asset's properties can mistake it for real data again.

RolesList/TagsList existed for Dapper's convenience per the old // TODO - map this via Dapper on way out of DB? comment on RolesList — but that was checked and never actually happened: DapperAssetRepository.GetAssetInternal sets Roles = firstAsset.Roles / Tags = firstAsset.Tags directly from the raw DB row, bypassing these properties entirely. So removing them doesn't give anything up; the one justification for their existence was aspirational and unused the whole time.

No EF/migration changes: Roles/Tags (the real mapped string? columns) are untouched, and RolesList/TagsList were [NotMapped] with no fluent-API .Ignore() config referencing them either — there was nothing to unmap.

Tests added

• AssetPreparerTests: PrepareAssetForUpsert_Preserves{Roles,Tags}_IfNotInUpdateRequest (pins the actual bug — omitted field survives an update) and PrepareAssetForUpsert_Updates{Roles,Tags}_IfInUpdateRequest (confirms normal updates still work).
• CollectionXTests: ToSeparatedString_*, including ToSeparatedString_OnlyEnumeratesSourceOnce, which pins that the new helper enumerates its source once rather than twice (the original version called IsNullOrEmpty() — which enumerates via .Any() — and then string.Join, over the same source).
• Updated AssetTests, AssetConverterTests, and the GetAssetTests orderby-rejection test's now-stale [NotMapped] comments to match.

Scope

Confirmed this affects PATCH/PUT /customers/{c}/spaces/{s}/images/{i}, bulk PATCH /customers/{c}/spaces/{s}/images, and batch creation (POST /customers/{c}/queue[/priority]) — anything going through AssetPreparer.PrepareAssetForUpsert on an existing asset. Confirmed not affected: PATCH /customers/{c}/allImages (BulkAssetPatcher doesn't use ApplyChanges) and POST .../reingest (ReingestAssetHandler mutates the loaded asset directly, no candidate merge).

Deliberately out of scope for this PR: the Asset.DeliveryChannels legacy string[] column looks like it has a similar-shaped problem (defaults non-null, never populated by AssetConverter, so ApplyChanges may reset it on every PATCH) but appears to be dead — nothing reads it back for delivery-channel logic (that's all on ImageDeliveryChannels now). Flagging for a follow-up rather than folding into this fix.