From c0d8bcbaef01bded416b5f8a153920b120913f5b Mon Sep 17 00:00:00 2001 From: Hamza Alqurneh Date: Wed, 9 Sep 2026 17:40:50 +0300 Subject: [PATCH] feat: the old mapper is offered only while something still uses it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The adapter catalog drops NativeJSONMapper once no subscription is saved with it, so the mapper leaves the pickers by itself as the migration finishes. Inactive subscriptions count, and so does drifted casing — a subscription still on it must not lose it from its own picker. Co-Authored-By: Claude Opus 5 (1M context) --- .../Resources/Adapters/RetiringMapper.cs | 69 +++++++ SW.Bitween.Api/Resources/Adapters/Search.cs | 3 +- .../Resources/Adapters/SearchVersioned.cs | 3 +- .../Tests/RetiringMapperTests.cs | 173 ++++++++++++++++++ .../ClientApp/e2e/native-mapper.spec.ts | 8 + 5 files changed, 254 insertions(+), 2 deletions(-) create mode 100644 SW.Bitween.Api/Resources/Adapters/RetiringMapper.cs create mode 100644 SW.Bitween.IntegrationTests/Tests/RetiringMapperTests.cs 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}`);