diff --git a/SW.Bitween.Api/Resources/Adapters/RetiringMapper.cs b/SW.Bitween.Api/Resources/Adapters/RetiringMapper.cs new file mode 100644 index 00000000..c6b45e10 --- /dev/null +++ b/SW.Bitween.Api/Resources/Adapters/RetiringMapper.cs @@ -0,0 +1,69 @@ +using System; +using System.Collections.Generic; +using System.Linq; +using System.Threading.Tasks; +using Microsoft.EntityFrameworkCore; +using SW.Bitween.Domain; +using SW.Bitween.NativeAdapters; + +namespace SW.Bitween.Resources.Adapters; + +/// +/// The Scriban mapper the native mapper replaced, offered only while something still runs on it. +/// +/// +/// +/// This is how the old mapper retires. A subscription already using it is untouched — it keeps +/// running, and it keeps its own editor — but the mapper stops being offered as a choice once the +/// last subscription has moved across. The list of mappers then shrinks by itself as the migration +/// finishes, rather than when someone remembers to delete the adapter. +/// +/// +/// One-way on purpose: past that point there is no picking it again. Which is why the question of +/// what counts as "in use" is answered generously — an inactive subscription still holds its +/// template and can be switched back on, so it counts. Withholding a mapper a subscription is +/// still saved with would leave that subscription's own mapper missing from the list its picker +/// draws from, which reads as no mapper chosen at all. +/// +/// +/// The same idea already exists a level down: withholds +/// the Rebex-backed adapters while no license key is set, so that a picker never offers something +/// that could only fail. This withholds one that would only lengthen a migration. +/// +/// +internal static class RetiringMapper +{ + /// The adapter id, which is the class name the discovery service reports. + public const string Id = nameof(NativeJSONMapper); + + private static readonly string Lowered = Id.ToLowerInvariant(); + + /// + /// without the retiring mapper, unless a subscription still names it. + /// + /// + /// The database is only asked when the retiring mapper is in the list to begin with, so the + /// other adapter kinds — receivers, handlers, validators — cost nothing. + /// + public static async Task> ExceptRetiring( + this IEnumerable adapters, BitweenDbContext dbContext) + { + var listed = adapters.ToList(); + + if (!listed.Any(Matches)) return listed; + + // Compared lowered rather than as stored: the lookup that resolves a mapper at run time is + // itself case-insensitive, so a row whose casing drifted is still a subscription running on + // this mapper, and answering "no" to that would take the mapper away from it. + var stillInUse = await dbContext.Set() + .AnyAsync(s => s.MapperId != null && s.MapperId.ToLower() == Lowered); + + if (stillInUse) return listed; + + listed.RemoveAll(Matches); + return listed; + } + + private static bool Matches(string adapter) => + adapter.Equals(Id, StringComparison.OrdinalIgnoreCase); +} diff --git a/SW.Bitween.Api/Resources/Adapters/Search.cs b/SW.Bitween.Api/Resources/Adapters/Search.cs index eb096181..caa44c37 100644 --- a/SW.Bitween.Api/Resources/Adapters/Search.cs +++ b/SW.Bitween.Api/Resources/Adapters/Search.cs @@ -31,7 +31,8 @@ public async Task Handle(AdapterSearchRequest request) await _requestContext.EnsurePermission(_dbContext, Model.Permissions.Subscriptions.View); // Get native adapters first - var nativeAdapters = _nativeAdapterDiscovery.GetNativeAdapters(request.Prefix).ToList(); + var nativeAdapters = await _nativeAdapterDiscovery.GetNativeAdapters(request.Prefix) + .ExceptRetiring(_dbContext); // Get external adapters from storage var cloudFilesList = diff --git a/SW.Bitween.Api/Resources/Adapters/SearchVersioned.cs b/SW.Bitween.Api/Resources/Adapters/SearchVersioned.cs index 5d29ae43..a7b373a8 100644 --- a/SW.Bitween.Api/Resources/Adapters/SearchVersioned.cs +++ b/SW.Bitween.Api/Resources/Adapters/SearchVersioned.cs @@ -35,7 +35,8 @@ public async Task Handle(AdapterSearchRequest request) var index = _serverlessOptions.AdapterRemotePath.Length + 1; // Get native adapters first (they don't have versions) - var nativeAdapters = _nativeAdapterDiscovery.GetNativeAdapters(request.Prefix) + var nativeAdapters = (await _nativeAdapterDiscovery.GetNativeAdapters(request.Prefix) + .ExceptRetiring(_dbContext)) .Select(key => new { Key = key, diff --git a/SW.Bitween.IntegrationTests/Tests/RetiringMapperTests.cs b/SW.Bitween.IntegrationTests/Tests/RetiringMapperTests.cs new file mode 100644 index 00000000..66e36c4d --- /dev/null +++ b/SW.Bitween.IntegrationTests/Tests/RetiringMapperTests.cs @@ -0,0 +1,173 @@ +using System.Collections.Generic; +using System.Linq; +using System.Threading; +using System.Threading.Tasks; +using Microsoft.EntityFrameworkCore; +using Microsoft.Extensions.DependencyInjection; +using Newtonsoft.Json; +using Newtonsoft.Json.Linq; +using SW.Bitween.Domain; +using SW.Bitween.IntegrationTests.Fixtures; +using SW.Bitween.Model; +using Xunit; + +namespace SW.Bitween.IntegrationTests.Tests; + +/// +/// The old mapper is offered only while a subscription still uses it. +/// +/// +/// Both directions are asserted in one test on purpose. The answer is a fact about the whole +/// database, so a test that only asserted one direction would be passing or failing on whatever +/// the other tests in this collection happened to leave behind. +/// +[Collection("Bitween")] +public class RetiringMapperTests +{ + private const string OldMapper = "NativeJSONMapper"; + private const string NewMapper = "NativeMapper"; + + private readonly BitweenFixture _fixture; + + public RetiringMapperTests(BitweenFixture fixture) + { + _fixture = fixture; + } + + private static int _seq; + private static string Unique(string prefix) => $"{prefix}-{Interlocked.Increment(ref _seq)}"; + + /// The mapper ids the adapter catalog offers, as the picker would list them. + private async Task> ListedMappers() + { + await using var scope = _fixture.CreateScope(); + scope.Superuser(); + var handler = ActivatorUtilities + .CreateInstance(scope.ServiceProvider); + + var result = await handler.Handle(new AdapterSearchRequest { Prefix = "mappers" }); + + // The handler returns anonymous types; going through JSON reads them the way the browser + // does rather than through reflection. + return JArray.Parse(JsonConvert.SerializeObject(result)) + .Select(row => row["Key"]!.ToString()) + .ToList(); + } + + /// A subscription saved with . + private async Task SubscriptionUsing(string mapperId) + { + await using var scope = _fixture.CreateScope(); + var db = scope.ServiceProvider.GetRequiredService(); + + var document = new Document(null, Unique("Retiring doc"), DocumentFormat.Json); + db.Set().Add(document); + var partner = new Partner(Unique("Retiring partner")); + db.Set().Add(partner); + await db.SaveChangesAsync(); + + var subscription = new Subscription( + Unique("Retiring sub"), document.Id, SubscriptionType.Internal, partner.Id) + { + MapperId = mapperId, + }; + db.Set().Add(subscription); + await db.SaveChangesAsync(); + + return subscription.Id; + } + + private async Task SetMapper(int subscriptionId, string? mapperId) + { + await using var scope = _fixture.CreateScope(); + var db = scope.ServiceProvider.GetRequiredService(); + + var subscription = await db.Set().SingleAsync(s => s.Id == subscriptionId); + subscription.MapperId = mapperId; + await db.SaveChangesAsync(); + } + + /// + /// Every subscription this database already holds on the old mapper, moved off it. + /// + /// + /// Safe to do to a shared database: the only other tests that put a subscription on the old + /// mapper build a fresh one inside each test, so none of them reads a row left by an earlier + /// one. A collection's tests also run one at a time, so nothing is arranging while this runs. + /// + private async Task> ParkExistingUsers() + { + await using var scope = _fixture.CreateScope(); + var db = scope.ServiceProvider.GetRequiredService(); + + var existing = await db.Set() + .Where(s => s.MapperId != null && s.MapperId.ToLower() == "nativejsonmapper") + .ToListAsync(); + + foreach (var subscription in existing) subscription.MapperId = null; + await db.SaveChangesAsync(); + + return existing.Select(s => s.Id).ToList(); + } + + [Fact] + public async Task The_old_mapper_is_listed_only_while_a_subscription_still_uses_it() + { + await ParkExistingUsers(); + + // Nothing on it: it is gone from the picker, and the new mapper is still there — a filter + // that took out more than it should would pass an assertion about the old one alone. + var withoutUsers = await ListedMappers(); + Assert.DoesNotContain(OldMapper, withoutUsers); + Assert.Contains(NewMapper, withoutUsers); + + // One subscription on it anywhere in the database is enough to bring it back everywhere. + var subscriptionId = await SubscriptionUsing(OldMapper); + Assert.Contains(OldMapper, await ListedMappers()); + + // And moving that last one across takes it away again, which is the whole point: the + // migration finishing is what retires the mapper. + await SetMapper(subscriptionId, NewMapper); + Assert.DoesNotContain(OldMapper, await ListedMappers()); + } + + [Fact] + public async Task An_inactive_subscription_still_counts_as_using_it() + { + await ParkExistingUsers(); + + await using (var scope = _fixture.CreateScope()) + { + var db = scope.ServiceProvider.GetRequiredService(); + var document = new Document(null, Unique("Retiring inactive doc"), DocumentFormat.Json); + db.Set().Add(document); + var partner = new Partner(Unique("Retiring inactive partner")); + db.Set().Add(partner); + await db.SaveChangesAsync(); + + var subscription = new Subscription( + Unique("Retiring inactive sub"), document.Id, SubscriptionType.Internal, partner.Id) + { + MapperId = OldMapper, + Inactive = true, + }; + db.Set().Add(subscription); + await db.SaveChangesAsync(); + } + + // It holds a template and can be switched back on, so it keeps the mapper listed. Withheld + // instead, this subscription's own picker would show nothing selected. + Assert.Contains(OldMapper, await ListedMappers()); + } + + [Fact] + public async Task Casing_that_drifted_still_counts() + { + await ParkExistingUsers(); + await SubscriptionUsing("nativejsonmapper"); + + // The run-time lookup that resolves a mapper matches case-insensitively, so a row stored + // this way is a subscription running on the old mapper and has to keep it listed. + Assert.Contains(OldMapper, await ListedMappers()); + } +} diff --git a/SW.Bitween.Web/ClientApp/e2e/native-mapper.spec.ts b/SW.Bitween.Web/ClientApp/e2e/native-mapper.spec.ts index 050f5a5b..d7285358 100644 --- a/SW.Bitween.Web/ClientApp/e2e/native-mapper.spec.ts +++ b/SW.Bitween.Web/ClientApp/e2e/native-mapper.spec.ts @@ -164,6 +164,14 @@ test("stored rules survive a switch to a list-shaped output and back", async ({ test("choosing the new mapper offers its editor, and the old mapper keeps its own", async ({ page, }) => { + // The old mapper is listed only while a subscription somewhere still uses it, so this + // test has to put one on it before it can pick it. A separate subscription rather than + // the one under test: pinning that one would give it a mapper already, and the first + // thing asserted below is that it has none. + await writeMapperProperties(await createSubscription(page), "NativeJSONMapper", { + ScribanTemplate: "{}", + }); + const subscriptionId = await createSubscription(page); await page.goto(`subscriptions/${subscriptionId}`);