From c0751fdda596c714d0dc5d2f741aaa86c0c6a8ba Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Sun, 19 Apr 2026 23:02:31 -0400 Subject: [PATCH] =?UTF-8?q?ExternalIdReservation=20merge=20inside=20Finali?= =?UTF-8?q?seBatchAsync.=20Closes=20task=20#197.=20The=20FinaliseBatch=20d?= =?UTF-8?q?ocstring=20called=20this=20out=20as=20a=20narrower=20follow-up?= =?UTF-8?q?=20pending=20a=20concurrent-insert=20test=20matrix,=20and=20the?= =?UTF-8?q?=20CSV=20import=20UI=20PR=20(#163)=20noted=20that=20operators?= =?UTF-8?q?=20would=20see=20raw=20DbUpdate=20UNIQUE-constraint=20messages?= =?UTF-8?q?=20on=20ZTag/SAPID=20collision=20until=20this=20landed.=20Now?= =?UTF-8?q?=20every=20finalised-batch=20row=20reserves=20ZTag=20+=20SAPID?= =?UTF-8?q?=20in=20the=20same=20EF=20transaction=20as=20the=20Equipment=20?= =?UTF-8?q?inserts,=20so=20either=20both=20commit=20atomically=20or=20neit?= =?UTF-8?q?her=20does.=20New=20MergeReservation=20helper=20handles=20the?= =?UTF-8?q?=20four=20outcomes=20per=20(Kind,=20Value)=20pair:=20(1)=20valu?= =?UTF-8?q?e=20empty/whitespace=20=E2=86=92=20skip=20the=20reservation=20e?= =?UTF-8?q?ntirely=20(operator=20left=20the=20optional=20identifier=20blan?= =?UTF-8?q?k);=20(2)=20active=20reservation=20exists=20for=20same=20Equipm?= =?UTF-8?q?entUuid=20=E2=86=92=20bump=20LastPublishedAt=20+=20reuse=20(re-?= =?UTF-8?q?finalising=20a=20batch=20against=20the=20same=20equipment=20mus?= =?UTF-8?q?t=20be=20idempotent,=20e.g.=20a=20retry=20after=20a=20transient?= =?UTF-8?q?=20DB=20blip);=20(3)=20active=20reservation=20exists=20for=20a?= =?UTF-8?q?=20DIFFERENT=20EquipmentUuid=20=E2=86=92=20throw=20ExternalIdRe?= =?UTF-8?q?servationConflictException=20with=20the=20conflicting=20UUID=20?= =?UTF-8?q?+=20originating=20cluster=20+=20first-published=20timestamp=20s?= =?UTF-8?q?o=20operator=20sees=20exactly=20who=20owns=20the=20value=20+=20?= =?UTF-8?q?where=20to=20resolve=20it=20(release=20via=20sp=5FReleaseExtern?= =?UTF-8?q?alIdReservation=20or=20pick=20a=20new=20ZTag);=20(4)=20no=20act?= =?UTF-8?q?ive=20reservation=20=E2=86=92=20create=20a=20fresh=20row=20with?= =?UTF-8?q?=20FirstPublishedBy=20=3D=20batch.CreatedBy=20+=20FirstPublishe?= =?UTF-8?q?dAt=20=3D=20transaction=20time.=20Pre-commit=20overlap=20scan?= =?UTF-8?q?=20uses=20one=20round-trip=20(WHERE=20Kind+Value=20IN=20the=20b?= =?UTF-8?q?atch's=20distinct=20sets,=20filtered=20to=20ReleasedAt=20IS=20N?= =?UTF-8?q?ULL=20so=20explicitly-released=20values=20can=20be=20re-issued?= =?UTF-8?q?=20per=20decision=20#124)=20+=20caches=20the=20results=20in=20a?= =?UTF-8?q?=20Dictionary=20keyed=20on=20(Kind,=20value.ToLowerInvariant())?= =?UTF-8?q?=20for=20O(1)=20lookup=20during=20the=20row=20loop.=20Race-safe?= =?UTF-8?q?ty=20catch:=20if=20another=20finalise=20commits=20between=20our?= =?UTF-8?q?=20cache-load=20+=20our=20SaveChanges,=20SQL=20Server=20surface?= =?UTF-8?q?s=20a=202601/2627=20unique-index=20violation=20against=20UX=5FE?= =?UTF-8?q?xternalIdReservation=5FKindValue=5FActive=20=E2=80=94=20IsReser?= =?UTF-8?q?vationUniquenessViolation=20walks=20the=20inner-exception=20cha?= =?UTF-8?q?in=20for=20that=20specific=20signature=20+=20rethrows=20as=20Ex?= =?UTF-8?q?ternalIdReservationConflictException=20so=20the=20UI=20shows=20?= =?UTF-8?q?a=20clean=20message=20instead=20of=20a=20raw=20DbUpdateExceptio?= =?UTF-8?q?n.=20The=20index-name=20match=20means=20unrelated=20filtered-un?= =?UTF-8?q?ique=20violations=20(future=20indices)=20don't=20get=20mis-clas?= =?UTF-8?q?sified.=20Test-fixture=20Row()=20helper=20updated=20to=20genera?= =?UTF-8?q?te=20unique=20SAPID=20per=20row=20(sap-{ZTag})=20=E2=80=94=20th?= =?UTF-8?q?e=20prior=20shared=20SAPID=3D"sap"=20worked=20only=20because=20?= =?UTF-8?q?reservations=20didn't=20exist;=20two=20rows=20sharing=20a=20SAP?= =?UTF-8?q?ID=20under=20different=20EquipmentUuids=20now=20collide=20as=20?= =?UTF-8?q?intended=20by=20decision=20#124's=20fleet-wide=20uniqueness=20r?= =?UTF-8?q?ule.=20Four=20new=20tests:=20(a)=20finalise=20creates=20both=20?= =?UTF-8?q?ZTag=20+=20SAPID=20reservations=20with=20expected=20Kind=20+=20?= =?UTF-8?q?Value;=20(b)=20re-finalising=20same=20EquipmentUuid's=20ZTag=20?= =?UTF-8?q?from=20a=20different=20batch=20does=20not=20create=20a=20duplic?= =?UTF-8?q?ate=20(LastPublishedAt=20refresh=20only);=20(c)=20different=20E?= =?UTF-8?q?quipmentUuid=20claiming=20the=20same=20ZTag=20throws=20External?= =?UTF-8?q?IdReservationConflictException=20with=20the=20ZTag=20value=20in?= =?UTF-8?q?=20the=20message=20+=20Equipment=20row=20for=20the=20second=20b?= =?UTF-8?q?atch=20is=20NOT=20inserted=20(transaction=20rolled=20back=20cle?= =?UTF-8?q?anly);=20(d)=20row=20with=20empty=20ZTag=20+=20empty=20SAPID=20?= =?UTF-8?q?skips=20reservation=20entirely.=20Full=20Admin.Tests=20suite=20?= =?UTF-8?q?85/85=20passing=20(was=2081=20before=20this=20PR,=20+4).=20Admi?= =?UTF-8?q?n=20project=20builds=200=20errors.=20Note:=20the=20InMemory=20E?= =?UTF-8?q?F=20provider=20doesn't=20enforce=20filtered-unique=20indices,?= =?UTF-8?q?=20so=20the=20IsReservationUniquenessViolation=20catch=20is=20e?= =?UTF-8?q?xercised=20only=20in=20the=20SQL=20Server=20integration=20path?= =?UTF-8?q?=20=E2=80=94=20the=20in-memory=20tests=20cover=20the=20cache-le?= =?UTF-8?q?vel=20conflict=20detection=20in=20MergeReservation=20instead,?= =?UTF-8?q?=20which=20is=20the=20first=20line=20of=20defence=20+=20catches?= =?UTF-8?q?=20the=20same-batch=20+=20published-vs-staged=20cases.=20The=20?= =?UTF-8?q?DbUpdate=20catch=20protects=20only=20the=20last-second=20race?= =?UTF-8?q?=20where=20two=20concurrent=20transactions=20both=20passed=20th?= =?UTF-8?q?e=20cache=20check.?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-Authored-By: Claude Opus 4.7 (1M context) --- .../Services/EquipmentImportBatchService.cs | 125 +++++++++++++++++- .../EquipmentImportBatchServiceTests.cs | 93 ++++++++++++- 2 files changed, 213 insertions(+), 5 deletions(-) diff --git a/src/ZB.MOM.WW.OtOpcUa.Admin/Services/EquipmentImportBatchService.cs b/src/ZB.MOM.WW.OtOpcUa.Admin/Services/EquipmentImportBatchService.cs index d5bb7dd..eb2c9ba 100644 --- a/src/ZB.MOM.WW.OtOpcUa.Admin/Services/EquipmentImportBatchService.cs +++ b/src/ZB.MOM.WW.OtOpcUa.Admin/Services/EquipmentImportBatchService.cs @@ -2,6 +2,7 @@ using Microsoft.EntityFrameworkCore; using ZB.MOM.WW.OtOpcUa.Admin.Services; using ZB.MOM.WW.OtOpcUa.Configuration; using ZB.MOM.WW.OtOpcUa.Configuration.Entities; +using ZB.MOM.WW.OtOpcUa.Configuration.Enums; namespace ZB.MOM.WW.OtOpcUa.Admin.Services; @@ -152,14 +153,37 @@ public sealed class EquipmentImportBatchService(OtOpcUaConfigDbContext db) try { - foreach (var row in batch.Rows.Where(r => r.IsAccepted)) + // Snapshot active reservations that overlap this batch's ZTag + SAPID set — one + // round-trip instead of N. Released rows (ReleasedAt IS NOT NULL) are ignored so + // an explicitly-released value can be reused. + var accepted = batch.Rows.Where(r => r.IsAccepted).ToList(); + var zTags = accepted.Where(r => !string.IsNullOrWhiteSpace(r.ZTag)) + .Select(r => r.ZTag).Distinct(StringComparer.OrdinalIgnoreCase).ToList(); + var sapIds = accepted.Where(r => !string.IsNullOrWhiteSpace(r.SAPID)) + .Select(r => r.SAPID).Distinct(StringComparer.OrdinalIgnoreCase).ToList(); + + var existingReservations = await db.ExternalIdReservations + .Where(r => r.ReleasedAt == null && + ((r.Kind == ReservationKind.ZTag && zTags.Contains(r.Value)) || + (r.Kind == ReservationKind.SAPID && sapIds.Contains(r.Value)))) + .ToListAsync(ct).ConfigureAwait(false); + var resByKey = existingReservations.ToDictionary( + r => (r.Kind, r.Value.ToLowerInvariant()), + r => r); + + var nowUtc = DateTime.UtcNow; + var firstPublishedBy = batch.CreatedBy; + + foreach (var row in accepted) { + var equipmentUuid = Guid.TryParse(row.EquipmentUuid, out var u) ? u : Guid.NewGuid(); + db.Equipment.Add(new Equipment { EquipmentRowId = Guid.NewGuid(), GenerationId = generationId, EquipmentId = row.EquipmentId, - EquipmentUuid = Guid.TryParse(row.EquipmentUuid, out var u) ? u : Guid.NewGuid(), + EquipmentUuid = equipmentUuid, DriverInstanceId = driverInstanceIdForRows, UnsLineId = unsLineIdForRows, Name = row.Name, @@ -176,10 +200,25 @@ public sealed class EquipmentImportBatchService(OtOpcUaConfigDbContext db) ManufacturerUri = row.ManufacturerUri, DeviceManualUri = row.DeviceManualUri, }); + + MergeReservation(row.ZTag, ReservationKind.ZTag, equipmentUuid, batch.ClusterId, + firstPublishedBy, nowUtc, resByKey); + MergeReservation(row.SAPID, ReservationKind.SAPID, equipmentUuid, batch.ClusterId, + firstPublishedBy, nowUtc, resByKey); } - batch.FinalisedAtUtc = DateTime.UtcNow; - await db.SaveChangesAsync(ct).ConfigureAwait(false); + batch.FinalisedAtUtc = nowUtc; + try + { + await db.SaveChangesAsync(ct).ConfigureAwait(false); + } + catch (DbUpdateException ex) when (IsReservationUniquenessViolation(ex)) + { + throw new ExternalIdReservationConflictException( + "Finalise rejected: one or more ZTag/SAPID values were reserved by another operator " + + "between batch preview and commit. Inspect active reservations + retry after resolving the conflict.", + ex); + } if (tx is not null) await tx.CommitAsync(ct).ConfigureAwait(false); } catch @@ -193,6 +232,71 @@ public sealed class EquipmentImportBatchService(OtOpcUaConfigDbContext db) } } + /// + /// Merge one external-ID reservation for an equipment row. Three outcomes: + /// (1) value is empty → skip; (2) reservation exists for same + /// → bump LastPublishedAt; (3) reservation exists for a different EquipmentUuid + /// → throw with the conflicting UUID + /// so the caller sees which equipment already owns the value; (4) no reservation → create new. + /// + private void MergeReservation( + string? value, + ReservationKind kind, + Guid equipmentUuid, + string clusterId, + string firstPublishedBy, + DateTime nowUtc, + Dictionary<(ReservationKind, string), ExternalIdReservation> cache) + { + if (string.IsNullOrWhiteSpace(value)) return; + + var key = (kind, value.ToLowerInvariant()); + if (cache.TryGetValue(key, out var existing)) + { + if (existing.EquipmentUuid != equipmentUuid) + throw new ExternalIdReservationConflictException( + $"{kind} '{value}' is already reserved by EquipmentUuid {existing.EquipmentUuid} " + + $"(first published {existing.FirstPublishedAt:u} on cluster '{existing.ClusterId}'). " + + $"Refusing to re-assign to {equipmentUuid}."); + + existing.LastPublishedAt = nowUtc; + return; + } + + var fresh = new ExternalIdReservation + { + ReservationId = Guid.NewGuid(), + Kind = kind, + Value = value, + EquipmentUuid = equipmentUuid, + ClusterId = clusterId, + FirstPublishedAt = nowUtc, + FirstPublishedBy = firstPublishedBy, + LastPublishedAt = nowUtc, + }; + db.ExternalIdReservations.Add(fresh); + cache[key] = fresh; + } + + /// + /// True when the root-cause was the filtered-unique + /// index UX_ExternalIdReservation_KindValue_Active — i.e. another transaction + /// won the race between our cache-load + commit. SQL Server surfaces this as 2601 / 2627. + /// + private static bool IsReservationUniquenessViolation(DbUpdateException ex) + { + for (Exception? inner = ex; inner is not null; inner = inner.InnerException) + { + if (inner is Microsoft.Data.SqlClient.SqlException sql && + (sql.Number == 2601 || sql.Number == 2627) && + sql.Message.Contains("UX_ExternalIdReservation_KindValue_Active", StringComparison.OrdinalIgnoreCase)) + { + return true; + } + } + return false; + } + /// List batches created by the given user. Finalised batches are archived; include them on demand. public async Task> ListByUserAsync(string createdBy, bool includeFinalised, CancellationToken ct) { @@ -205,3 +309,16 @@ public sealed class EquipmentImportBatchService(OtOpcUaConfigDbContext db) public sealed class ImportBatchNotFoundException(string message) : Exception(message); public sealed class ImportBatchAlreadyFinalisedException(string message) : Exception(message); + +/// +/// Thrown when a FinaliseBatchAsync call detects that one of its ZTag/SAPID values is +/// already reserved by a different EquipmentUuid — either from a prior published generation +/// or a concurrent finalise that won the race. The operator sees the message + the conflicting +/// equipment ownership so they can resolve the conflict (pick a new ZTag, release the existing +/// reservation via sp_ReleaseExternalIdReservation, etc.) and retry the finalise. +/// +public sealed class ExternalIdReservationConflictException : Exception +{ + public ExternalIdReservationConflictException(string message) : base(message) { } + public ExternalIdReservationConflictException(string message, Exception inner) : base(message, inner) { } +} diff --git a/tests/ZB.MOM.WW.OtOpcUa.Admin.Tests/EquipmentImportBatchServiceTests.cs b/tests/ZB.MOM.WW.OtOpcUa.Admin.Tests/EquipmentImportBatchServiceTests.cs index efcee02..d28db99 100644 --- a/tests/ZB.MOM.WW.OtOpcUa.Admin.Tests/EquipmentImportBatchServiceTests.cs +++ b/tests/ZB.MOM.WW.OtOpcUa.Admin.Tests/EquipmentImportBatchServiceTests.cs @@ -23,11 +23,13 @@ public sealed class EquipmentImportBatchServiceTests : IDisposable public void Dispose() => _db.Dispose(); + // Unique SAPID per row — FinaliseBatch reserves ZTag + SAPID via filtered-unique index, so + // two rows sharing a SAPID under different EquipmentUuids collide as intended. private static EquipmentCsvRow Row(string zTag, string name = "eq-1") => new() { ZTag = zTag, MachineCode = "mc", - SAPID = "sap", + SAPID = $"sap-{zTag}", EquipmentId = "eq-id", EquipmentUuid = Guid.NewGuid().ToString(), Name = name, @@ -162,4 +164,93 @@ public sealed class EquipmentImportBatchServiceTests : IDisposable await _svc.DropBatchAsync(Guid.NewGuid(), CancellationToken.None); // no throw } + + [Fact] + public async Task FinaliseBatch_Creates_ExternalIdReservations_ForZTagAndSAPID() + { + var batch = await _svc.CreateBatchAsync("c1", "alice", CancellationToken.None); + await _svc.StageRowsAsync(batch.Id, [Row("z-new-1")], [], CancellationToken.None); + + await _svc.FinaliseBatchAsync(batch.Id, 1, "drv", "line", CancellationToken.None); + + var active = await _db.ExternalIdReservations.AsNoTracking() + .Where(r => r.ReleasedAt == null) + .ToListAsync(); + active.Count.ShouldBe(2); + active.ShouldContain(r => r.Kind == ZB.MOM.WW.OtOpcUa.Configuration.Enums.ReservationKind.ZTag && r.Value == "z-new-1"); + active.ShouldContain(r => r.Kind == ZB.MOM.WW.OtOpcUa.Configuration.Enums.ReservationKind.SAPID && r.Value == "sap-z-new-1"); + } + + [Fact] + public async Task FinaliseBatch_SameEquipmentUuid_ReusesExistingReservation() + { + var batch1 = await _svc.CreateBatchAsync("c1", "alice", CancellationToken.None); + var sharedUuid = Guid.NewGuid(); + var row = new EquipmentCsvRow + { + ZTag = "z-shared", MachineCode = "mc", SAPID = "sap-shared", + EquipmentId = "eq-1", EquipmentUuid = sharedUuid.ToString(), + Name = "eq-1", UnsAreaName = "a", UnsLineName = "l", + }; + await _svc.StageRowsAsync(batch1.Id, [row], [], CancellationToken.None); + await _svc.FinaliseBatchAsync(batch1.Id, 1, "drv", "line", CancellationToken.None); + + var countAfterFirst = _db.ExternalIdReservations.Count(r => r.ReleasedAt == null); + + // Second finalise with same EquipmentUuid + same ZTag — should NOT create a duplicate. + var batch2 = await _svc.CreateBatchAsync("c1", "alice", CancellationToken.None); + await _svc.StageRowsAsync(batch2.Id, [row], [], CancellationToken.None); + await _svc.FinaliseBatchAsync(batch2.Id, 2, "drv", "line", CancellationToken.None); + + _db.ExternalIdReservations.Count(r => r.ReleasedAt == null).ShouldBe(countAfterFirst); + } + + [Fact] + public async Task FinaliseBatch_DifferentEquipmentUuid_SameZTag_Throws_Conflict() + { + var batchA = await _svc.CreateBatchAsync("c1", "alice", CancellationToken.None); + var rowA = new EquipmentCsvRow + { + ZTag = "z-collide", MachineCode = "mc-a", SAPID = "sap-a", + EquipmentId = "eq-a", EquipmentUuid = Guid.NewGuid().ToString(), + Name = "a", UnsAreaName = "ar", UnsLineName = "ln", + }; + await _svc.StageRowsAsync(batchA.Id, [rowA], [], CancellationToken.None); + await _svc.FinaliseBatchAsync(batchA.Id, 1, "drv", "line", CancellationToken.None); + + var batchB = await _svc.CreateBatchAsync("c1", "bob", CancellationToken.None); + var rowB = new EquipmentCsvRow + { + ZTag = "z-collide", MachineCode = "mc-b", SAPID = "sap-b", // same ZTag, different EquipmentUuid + EquipmentId = "eq-b", EquipmentUuid = Guid.NewGuid().ToString(), + Name = "b", UnsAreaName = "ar", UnsLineName = "ln", + }; + await _svc.StageRowsAsync(batchB.Id, [rowB], [], CancellationToken.None); + + var ex = await Should.ThrowAsync(() => + _svc.FinaliseBatchAsync(batchB.Id, 2, "drv", "line", CancellationToken.None)); + ex.Message.ShouldContain("z-collide"); + + // Second finalise must have rolled back — no partial Equipment row for batch B. + var equipmentB = await _db.Equipment.AsNoTracking() + .Where(e => e.EquipmentId == "eq-b") + .ToListAsync(); + equipmentB.ShouldBeEmpty(); + } + + [Fact] + public async Task FinaliseBatch_EmptyZTagAndSAPID_SkipsReservation() + { + var batch = await _svc.CreateBatchAsync("c1", "alice", CancellationToken.None); + var row = new EquipmentCsvRow + { + ZTag = "", MachineCode = "mc", SAPID = "", + EquipmentId = "eq-nil", EquipmentUuid = Guid.NewGuid().ToString(), + Name = "nil", UnsAreaName = "ar", UnsLineName = "ln", + }; + await _svc.StageRowsAsync(batch.Id, [row], [], CancellationToken.None); + await _svc.FinaliseBatchAsync(batch.Id, 1, "drv", "line", CancellationToken.None); + + _db.ExternalIdReservations.Count().ShouldBe(0); + } }