adam/shoc-backend
1
0
Fork 0
mirror of https://github.com/Sea-Haven-Industries/shoc-backend.git synced 2026-09-30 08:23:12 +00:00
Code Issues Projects Releases Packages Wiki Activity Actions 1

fix(vendor-roster): conflict on colliding rename, reject no-op company update (SH-250)

Browse source 
Two review findings on the additive PATCH path:

- AddTechniciansAsync can rename via CompanyFields.Name and write NormalizedName
  against the unique index, but the save had no guard. A colliding rename
  surfaced as an unhandled 500 from the PATCH action instead of a stable client
  conflict. Pre-check the normalized name against other live companies and throw
  VendorRosterDuplicateNameException, with a scoped catch around the save for the
  race where a competing rename commits in between. The controller maps it to a
  409 alongside the existing concurrency conflict.
- Empty-payload validation only rejected a null CompanyFields, so an all-blank
  CompanyFields object was forwarded as a company update, bumping RowVersion and
  rewriting every technician's LastModificationTime without changing any company
  data. Blank fields now collapse to no company change, and a request with
  neither technicians nor a real company value fails validation.
This commit is contained in:
Alexandre Brandizzi 2026-08-18 17:33:16 -03:00
parent 11a355bb7a
commit 9ef2512e14
6 changed files with 203 additions and 5 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

63
Api.SeaHavenIndustries.Tests/VendorCompanyRosterDataServiceTests.cs
View file

@ -500,6 +500,69 @@ public class VendorCompanyRosterDataServiceTests
private static byte[] InitialRowVersion() => new byte[] { 0, 0, 0, 0, 0, 0, 0, 1 };
[Fact]
public async Task AddTechnicians_RenameToTakenName_ThrowsDuplicateNameConflict()
{
var dbName = Guid.NewGuid().ToString();
int companyId;
using (var seed = NewContext(dbName))
{
var (cid, _) = await SeedCompanyWithTechnicianAsync(seed, "Gateway Plumbing", InitialRowVersion(), "First");
companyId = cid;
await SeedCompanyWithTechnicianAsync(seed, "Harbor Electric", InitialRowVersion(), "Other");
}
using (var act = NewContext(dbName))
{
var service = new VendorCompanyRosterDataService(act);
var call = () => service.AddTechniciansAsync(new VendorCompanyRosterAddWriteModel
{
CompanyId = companyId,
RowVersion = InitialRowVersion(),
ActorUserId = "42",
CompanyFields = new VendorRosterCompanyFieldsWriteModel { Name = "Harbor Electric" },
AddTechnicians = new List<RosterTechnicianWriteModel>()
}, CancellationToken.None);
// SH-250: a colliding rename must surface as a stable client conflict, not as an
// unhandled DbUpdateException that the controller reports as a 500.
await call.Should().ThrowAsync<VendorRosterDuplicateNameException>();
}
using var verify = NewContext(dbName);
verify.VendorCompanies.Single(c => c.Id == companyId).Name.Should().Be("Gateway Plumbing");
}
[Fact]
public async Task AddTechnicians_RenameToFreeName_Succeeds()
{
var dbName = Guid.NewGuid().ToString();
int companyId;
using (var seed = NewContext(dbName))
{
var (cid, _) = await SeedCompanyWithTechnicianAsync(seed, "Gateway Plumbing", InitialRowVersion(), "First");
companyId = cid;
}
using (var act = NewContext(dbName))
{
var service = new VendorCompanyRosterDataService(act);
await service.AddTechniciansAsync(new VendorCompanyRosterAddWriteModel
{
CompanyId = companyId,
RowVersion = InitialRowVersion(),
ActorUserId = "42",
CompanyFields = new VendorRosterCompanyFieldsWriteModel { Name = "Gateway Plumbing & Drain" },
AddTechnicians = new List<RosterTechnicianWriteModel>()
}, CancellationToken.None);
}
using var verify = NewContext(dbName);
var company = verify.VendorCompanies.Single(c => c.Id == companyId);
company.Name.Should().Be("Gateway Plumbing & Drain");
company.NormalizedName.Should().Be("gateway plumbing & drain");
}
[Fact]
public async Task AddTechnicians_LeavesAllPreExistingTechniciansIntact()
{

47
Api.SeaHavenIndustries.Tests/VendorCompanyRosterServiceTests.cs
View file

@ -508,9 +508,52 @@ public class VendorCompanyRosterServiceTests
await NewService(data).AddTechniciansAsync(7, dto, "42", CancellationToken.None);
// SH-250: every field is blank, so there is no company change to apply. Forwarding a
// non-null write model made the data layer bump RowVersion and rewrite every
// technician's LastModificationTime for a request that changes no company data.
captured!.CompanyFields.Should().BeNull();
}
[Fact]
public async Task AddTechnicians_AllBlankCompanyFieldsAndNoTechnicians_FailsValidation()
{
var data = new Mock<IVendorCompanyRosterDataService>();
var dto = new AddTechniciansVendorRosterDTO
{
RowVersion = "AAAAAAAAD8I=",
AddTechnicians = new List<RosterTechnicianInputDTO>(),
CompanyFields = new VendorRosterCompanyFieldsDTO { CompanyPhone = " ", Email = "" }
};
var act = () => NewService(data).AddTechniciansAsync(7, dto, "42", CancellationToken.None);
await act.Should().ThrowAsync<ValidationException>();
data.Verify(
x => x.AddTechniciansAsync(It.IsAny<VendorCompanyRosterAddWriteModel>(), It.IsAny<CancellationToken>()),
Times.Never);
}
[Fact]
public async Task AddTechnicians_RealCompanyValueStillForwarded()
{
var data = new Mock<IVendorCompanyRosterDataService>();
VendorCompanyRosterAddWriteModel? captured = null;
data.Setup(x => x.AddTechniciansAsync(It.IsAny<VendorCompanyRosterAddWriteModel>(), It.IsAny<CancellationToken>()))
.Callback<VendorCompanyRosterAddWriteModel, CancellationToken>((model, _) => captured = model)
.ReturnsAsync(SampleReadModel(7, 1));
var dto = new AddTechniciansVendorRosterDTO
{
RowVersion = "AAAAAAAAD8I=",
AddTechnicians = new List<RosterTechnicianInputDTO>(),
CompanyFields = new VendorRosterCompanyFieldsDTO { City = "Norfolk" }
};
await NewService(data).AddTechniciansAsync(7, dto, "42", CancellationToken.None);
captured!.CompanyFields.Should().NotBeNull();
captured.CompanyFields!.CompanyPhone.Should().BeNull();
captured.CompanyFields.Email.Should().BeNull();
captured.CompanyFields!.City.Should().Be("Norfolk");
}
[Fact]

10
Api.SeaHavenIndustries/Controllers/VendorCompanyRosterController.cs
View file

@ -171,6 +171,16 @@ namespace Api.SeaHavenIndustries.Controllers
{
return NotFound(new Response { Status = "Error", Message = "Vendor company not found" });
}
catch (VendorRosterDuplicateNameException dup)
{
// SH-250: a colliding rename is a client conflict, not a server fault.
return Conflict(new
{
Status = "Conflict",
Message = _logger.Sanitize(dup, "Another vendor company already uses that name."),
Code = 409
});
}
catch (DbUpdateConcurrencyException)
{
return Conflict(new

39
SeaHaven.DataServices/Implementation/VendorCompanyRosterDataService.cs
View file

@ -286,6 +286,7 @@ namespace SeaHaven.DataServices.Implementation
var now = DateTime.UtcNow;
int? actorId = int.TryParse(roster.ActorUserId, out var parsedActorId) ? parsedActorId : null;
string? renamedTo = null;
// Partial company update: only fields present in CompanyFields are applied;
// a null field leaves the stored value untouched. The company row is always
@ -297,8 +298,28 @@ namespace SeaHaven.DataServices.Implementation
if (fields.Name != null)
{
var trimmedName = fields.Name.Trim();
var normalized = trimmedName.ToLowerInvariant();
if (normalized != company.NormalizedName)
{
var nameTaken = await _context.VendorCompanies
.AnyAsync(
other => other.Id != company.Id
&& (other.IsDeleted == null || other.IsDeleted == false)
&& other.NormalizedName == normalized,
cancellationToken);
if (nameTaken)
throw new VendorRosterDuplicateNameException(
"Another vendor company already uses that name.",
new InvalidOperationException(
$"NormalizedName '{normalized}' is already in use."));
renamedTo = normalized;
}
company.Name = trimmedName;
company.NormalizedName = trimmedName.ToLowerInvariant();
company.NormalizedName = normalized;
}
if (fields.CompanyPhone != null)
@ -371,7 +392,21 @@ namespace SeaHaven.DataServices.Implementation
await _context.Vendors.AddAsync(vendor, cancellationToken);
}
await _context.SaveChangesAsync(cancellationToken);
// SH-250: a rename can collide with the unique NormalizedName index. Without
// this guard the DbUpdateException reached the controller's generic handler
// and surfaced as a 500, even though SQL Server rolls the batch back cleanly.
// The pre-check above handles the ordinary case; this covers the race where a
// competing rename commits between that check and this save.
try
{
await _context.SaveChangesAsync(cancellationToken);
}
catch (DbUpdateException ex) when (renamedTo != null && ex is not DbUpdateConcurrencyException)
{
throw new VendorRosterDuplicateNameException(
"Another vendor company already uses that name.",
ex);
}
return await GetRosterAsync(null, company.Id, cancellationToken)
?? throw new InvalidOperationException("Roster could not be reloaded after save.");

15
SeaHaven.DataServices/Models/VendorCompanyRosterModels.cs
View file

@ -86,6 +86,21 @@ namespace SeaHaven.DataServices.Models
// Raised by the roster data service when a technician removed from the snapshot
// still has open linked work orders, so the whole reconcile must fail. Carries the
// blocking work orders so the API can surface a stable 409 without leaking internals.
/// <summary>
/// Raised when a company rename collides with the unique NormalizedName index.
/// Distinct from <see cref="VendorRosterConflictException"/>, which reports open
/// linked work orders, so callers can return a stable client conflict instead of
/// letting the raw <see cref="Microsoft.EntityFrameworkCore.DbUpdateException"/>
/// surface as a 500.
/// </summary>
public sealed class VendorRosterDuplicateNameException : Exception
{
public VendorRosterDuplicateNameException(string message, Exception inner)
: base(message, inner)
{
}
}
public sealed class VendorRosterConflictException : Exception
{
public IReadOnlyList<LinkedWorkOrderInfo> BlockedWorkOrders { get; }

34
SeaHaven.Services/Implementation/VendorCompanyRosterService.cs
View file

@ -126,7 +126,8 @@ namespace SeaHaven.Services.Implementation
nameof(AddTechniciansVendorRosterDTO.AddTechnicians),
"Technician ids are not allowed when adding technicians."));
if (dto.AddTechnicians.Count == 0 && dto.CompanyFields == null)
var hasCompanyChange = dto.CompanyFields != null && HasAnyCompanyValue(dto.CompanyFields);
if (dto.AddTechnicians.Count == 0 && !hasCompanyChange)
failures.Add(new ValidationFailure(
nameof(AddTechniciansVendorRosterDTO.AddTechnicians),
"At least one technician to add or a company update is required."));
@ -135,8 +136,17 @@ namespace SeaHaven.Services.Implementation
VendorRosterCompanyFieldsWriteModel? companyFields = null;
if (dto.CompanyFields != null)
{
companyFields = ValidateAndMapCompanyFields(dto.CompanyFields, failures);
// SH-250: blank company fields all map to null ("unchanged"), so an empty
// or all-blank CompanyFields object carries no company update. Forwarding
// it as a non-null write model made the data layer bump RowVersion and
// rewrite every technician's LastModificationTime for a no-op request.
if (HasNoCompanyChange(companyFields))
companyFields = null;
}
if (failures.Count > 0)
throw new ValidationException(failures);
@ -250,6 +260,28 @@ namespace SeaHaven.Services.Implementation
// Validates optional company-level updates for the additive path and maps them
// to the write model. A null (or blank) field means "leave unchanged"; blank
// values normalize to null so a partial update can never clear data by accident.
private static bool HasAnyCompanyValue(VendorRosterCompanyFieldsDTO fields) =>
!string.IsNullOrWhiteSpace(fields.Name)
|| !string.IsNullOrWhiteSpace(fields.CompanyPhone)
|| !string.IsNullOrWhiteSpace(fields.Email)
|| !string.IsNullOrWhiteSpace(fields.Address)
|| !string.IsNullOrWhiteSpace(fields.City)
|| !string.IsNullOrWhiteSpace(fields.State)
|| !string.IsNullOrWhiteSpace(fields.Zip)
|| !string.IsNullOrWhiteSpace(fields.GoogleMapsUrl)
|| !string.IsNullOrWhiteSpace(fields.Notes);
private static bool HasNoCompanyChange(VendorRosterCompanyFieldsWriteModel model) =>
model.Name == null
&& model.CompanyPhone == null
&& model.Email == null
&& model.Address == null
&& model.City == null
&& model.State == null
&& model.Zip == null
&& model.GoogleMapsUrl == null
&& model.Notes == null;
private static VendorRosterCompanyFieldsWriteModel ValidateAndMapCompanyFields(
VendorRosterCompanyFieldsDTO fields,
List<ValidationFailure> failures)