ExternalIdReservation merge inside FinaliseBatchAsync. Closes task #197. The FinaliseBatch docstring called this out as a narrower follow-up pending a concurrent-insert test matrix, and the CSV import UI PR (#163) noted that operators would see raw DbUpdate UNIQUE-constraint messages on ZTag/SAPID collision until this landed. Now every finalised-batch row reserves ZTag + SAPID in the same EF transaction as the Equipment inserts, so either both commit atomically or neither does. New MergeReservation helper handles the four outcomes per (Kind, Value) pair: (1) value empty/whitespace → skip the reservation entirely (operator left the optional identifier blank); (2) active reservation exists for same EquipmentUuid → bump LastPublishedAt + reuse (re-finalising a batch against the same equipment must be idempotent, e.g. a retry after a transient DB blip); (3) active reservation exists for a DIFFERENT EquipmentUuid → throw ExternalIdReservationConflictException with the conflicting UUID + originating cluster + first-published timestamp so operator sees exactly who owns the value + where to resolve it (release via sp_ReleaseExternalIdReservation or pick a new ZTag); (4) no active reservation → create a fresh row with FirstPublishedBy = batch.CreatedBy + FirstPublishedAt = transaction time. Pre-commit overlap scan uses one round-trip (WHERE Kind+Value IN the batch's distinct sets, filtered to ReleasedAt IS NULL so explicitly-released values can be re-issued per decision #124) + caches the results in a Dictionary keyed on (Kind, value.ToLowerInvariant()) for O(1) lookup during the row loop. Race-safety catch: if another finalise commits between our cache-load + our SaveChanges, SQL Server surfaces a 2601/2627 unique-index violation against UX_ExternalIdReservation_KindValue_Active — IsReservationUniquenessViolation walks the inner-exception chain for that specific signature + rethrows as ExternalIdReservationConflictException so the UI shows a clean message instead of a raw DbUpdateException. The index-name match means unrelated filtered-unique violations (future indices) don't get mis-classified. Test-fixture Row() helper updated to generate unique SAPID per row (sap-{ZTag}) — the prior shared SAPID="sap" worked only because reservations didn't exist; two rows sharing a SAPID under different EquipmentUuids now collide as intended by decision #124's fleet-wide uniqueness rule. Four new tests: (a) finalise creates both ZTag + SAPID reservations with expected Kind + Value; (b) re-finalising same EquipmentUuid's ZTag from a different batch does not create a duplicate (LastPublishedAt refresh only); (c) different EquipmentUuid claiming the same ZTag throws ExternalIdReservationConflictException with the ZTag value in the message + Equipment row for the second batch is NOT inserted (transaction rolled back cleanly); (d) row with empty ZTag + empty SAPID skips reservation entirely. Full Admin.Tests suite 85/85 passing (was 81 before this PR, +4). Admin project builds 0 errors. Note: the InMemory EF provider doesn't enforce filtered-unique indices, so the IsReservationUniquenessViolation catch is exercised only in the SQL Server integration path — the in-memory tests cover the cache-level conflict detection in MergeReservation instead, which is the first line of defence + catches the same-batch + published-vs-staged cases. The DbUpdate catch protects only the last-second race where two concurrent transactions both passed the cache check.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -2,6 +2,7 @@ using Microsoft.EntityFrameworkCore;
|
|||||||
using ZB.MOM.WW.OtOpcUa.Admin.Services;
|
using ZB.MOM.WW.OtOpcUa.Admin.Services;
|
||||||
using ZB.MOM.WW.OtOpcUa.Configuration;
|
using ZB.MOM.WW.OtOpcUa.Configuration;
|
||||||
using ZB.MOM.WW.OtOpcUa.Configuration.Entities;
|
using ZB.MOM.WW.OtOpcUa.Configuration.Entities;
|
||||||
|
using ZB.MOM.WW.OtOpcUa.Configuration.Enums;
|
||||||
|
|
||||||
namespace ZB.MOM.WW.OtOpcUa.Admin.Services;
|
namespace ZB.MOM.WW.OtOpcUa.Admin.Services;
|
||||||
|
|
||||||
@@ -152,14 +153,37 @@ public sealed class EquipmentImportBatchService(OtOpcUaConfigDbContext db)
|
|||||||
|
|
||||||
try
|
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
|
db.Equipment.Add(new Equipment
|
||||||
{
|
{
|
||||||
EquipmentRowId = Guid.NewGuid(),
|
EquipmentRowId = Guid.NewGuid(),
|
||||||
GenerationId = generationId,
|
GenerationId = generationId,
|
||||||
EquipmentId = row.EquipmentId,
|
EquipmentId = row.EquipmentId,
|
||||||
EquipmentUuid = Guid.TryParse(row.EquipmentUuid, out var u) ? u : Guid.NewGuid(),
|
EquipmentUuid = equipmentUuid,
|
||||||
DriverInstanceId = driverInstanceIdForRows,
|
DriverInstanceId = driverInstanceIdForRows,
|
||||||
UnsLineId = unsLineIdForRows,
|
UnsLineId = unsLineIdForRows,
|
||||||
Name = row.Name,
|
Name = row.Name,
|
||||||
@@ -176,10 +200,25 @@ public sealed class EquipmentImportBatchService(OtOpcUaConfigDbContext db)
|
|||||||
ManufacturerUri = row.ManufacturerUri,
|
ManufacturerUri = row.ManufacturerUri,
|
||||||
DeviceManualUri = row.DeviceManualUri,
|
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;
|
batch.FinalisedAtUtc = nowUtc;
|
||||||
await db.SaveChangesAsync(ct).ConfigureAwait(false);
|
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);
|
if (tx is not null) await tx.CommitAsync(ct).ConfigureAwait(false);
|
||||||
}
|
}
|
||||||
catch
|
catch
|
||||||
@@ -193,6 +232,71 @@ public sealed class EquipmentImportBatchService(OtOpcUaConfigDbContext db)
|
|||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/// <summary>
|
||||||
|
/// Merge one external-ID reservation for an equipment row. Three outcomes:
|
||||||
|
/// (1) value is empty → skip; (2) reservation exists for same <paramref name="equipmentUuid"/>
|
||||||
|
/// → bump <c>LastPublishedAt</c>; (3) reservation exists for a different EquipmentUuid
|
||||||
|
/// → throw <see cref="ExternalIdReservationConflictException"/> with the conflicting UUID
|
||||||
|
/// so the caller sees which equipment already owns the value; (4) no reservation → create new.
|
||||||
|
/// </summary>
|
||||||
|
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;
|
||||||
|
}
|
||||||
|
|
||||||
|
/// <summary>
|
||||||
|
/// True when the <see cref="DbUpdateException"/> root-cause was the filtered-unique
|
||||||
|
/// index <c>UX_ExternalIdReservation_KindValue_Active</c> — i.e. another transaction
|
||||||
|
/// won the race between our cache-load + commit. SQL Server surfaces this as 2601 / 2627.
|
||||||
|
/// </summary>
|
||||||
|
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;
|
||||||
|
}
|
||||||
|
|
||||||
/// <summary>List batches created by the given user. Finalised batches are archived; include them on demand.</summary>
|
/// <summary>List batches created by the given user. Finalised batches are archived; include them on demand.</summary>
|
||||||
public async Task<IReadOnlyList<EquipmentImportBatch>> ListByUserAsync(string createdBy, bool includeFinalised, CancellationToken ct)
|
public async Task<IReadOnlyList<EquipmentImportBatch>> 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 ImportBatchNotFoundException(string message) : Exception(message);
|
||||||
public sealed class ImportBatchAlreadyFinalisedException(string message) : Exception(message);
|
public sealed class ImportBatchAlreadyFinalisedException(string message) : Exception(message);
|
||||||
|
|
||||||
|
/// <summary>
|
||||||
|
/// Thrown when a <c>FinaliseBatchAsync</c> 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 <c>sp_ReleaseExternalIdReservation</c>, etc.) and retry the finalise.
|
||||||
|
/// </summary>
|
||||||
|
public sealed class ExternalIdReservationConflictException : Exception
|
||||||
|
{
|
||||||
|
public ExternalIdReservationConflictException(string message) : base(message) { }
|
||||||
|
public ExternalIdReservationConflictException(string message, Exception inner) : base(message, inner) { }
|
||||||
|
}
|
||||||
|
|||||||
@@ -23,11 +23,13 @@ public sealed class EquipmentImportBatchServiceTests : IDisposable
|
|||||||
|
|
||||||
public void Dispose() => _db.Dispose();
|
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()
|
private static EquipmentCsvRow Row(string zTag, string name = "eq-1") => new()
|
||||||
{
|
{
|
||||||
ZTag = zTag,
|
ZTag = zTag,
|
||||||
MachineCode = "mc",
|
MachineCode = "mc",
|
||||||
SAPID = "sap",
|
SAPID = $"sap-{zTag}",
|
||||||
EquipmentId = "eq-id",
|
EquipmentId = "eq-id",
|
||||||
EquipmentUuid = Guid.NewGuid().ToString(),
|
EquipmentUuid = Guid.NewGuid().ToString(),
|
||||||
Name = name,
|
Name = name,
|
||||||
@@ -162,4 +164,93 @@ public sealed class EquipmentImportBatchServiceTests : IDisposable
|
|||||||
await _svc.DropBatchAsync(Guid.NewGuid(), CancellationToken.None);
|
await _svc.DropBatchAsync(Guid.NewGuid(), CancellationToken.None);
|
||||||
// no throw
|
// 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<ExternalIdReservationConflictException>(() =>
|
||||||
|
_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);
|
||||||
|
}
|
||||||
}
|
}
|
||||||
|
|||||||
Reference in New Issue
Block a user