From b9765ea7c181e0093b377f1896e5daee14fc1048 Mon Sep 17 00:00:00 2001 From: Rex Lorenzo Date: Tue, 16 Jun 2026 18:10:48 -0700 Subject: [PATCH] fix(clinical): detect duplicate schedules by SQL error number Replace locale-fragile message-substring matching with SQL Server error numbers 2627/2601 (unique constraint / duplicate key in a unique index) to decide whether a save failure means the instructor is already scheduled. Other database errors (foreign-key, check constraints) now fall through to the generic message instead of mis-reporting "already scheduled", and the check no longer depends on English error text or a culture-specific ToLower(). --- .../ScheduleEditServiceTest.cs | 51 +++++++++++- test/ClinicalScheduler/SqlExceptionFactory.cs | 79 +++++++++++++++++++ test/ClinicalScheduler/ToggleThrowContext.cs | 27 +++++++ .../Services/ScheduleEditService.cs | 18 +++-- 4 files changed, 167 insertions(+), 8 deletions(-) create mode 100644 test/ClinicalScheduler/SqlExceptionFactory.cs create mode 100644 test/ClinicalScheduler/ToggleThrowContext.cs diff --git a/test/ClinicalScheduler/ScheduleEditServiceTest.cs b/test/ClinicalScheduler/ScheduleEditServiceTest.cs index 464ba082a..26a8289a1 100644 --- a/test/ClinicalScheduler/ScheduleEditServiceTest.cs +++ b/test/ClinicalScheduler/ScheduleEditServiceTest.cs @@ -1,3 +1,4 @@ +using Microsoft.Data.SqlClient; using Microsoft.EntityFrameworkCore; using Microsoft.Extensions.Logging; using Microsoft.Extensions.Options; @@ -23,7 +24,7 @@ public class ScheduleEditServiceTest : IDisposable private readonly IPermissionValidator _mockPermissionValidator; private readonly IUserHelper _mockUserHelper; private readonly IEmailTemplateRenderer _mockEmailTemplateRenderer; - private readonly ClinicalSchedulerContext _context; + private readonly ToggleThrowContext _context; private readonly TestableScheduleEditService _service; private bool _disposed; @@ -33,7 +34,7 @@ public ScheduleEditServiceTest() var options = new DbContextOptionsBuilder() .UseInMemoryDatabase(databaseName: Guid.NewGuid().ToString()) .Options; - _context = new ClinicalSchedulerContext(options); + _context = new ToggleThrowContext(options); _mockPermissionService = Substitute.For(); _mockAuditService = Substitute.For(); @@ -427,6 +428,52 @@ public async Task AddInstructorAsync_WithScheduleConflicts_ThrowsInvalidOperatio $"Expected duplicate schedule error but got: {ex.Message}"); } + [Fact] + public async Task AddInstructorAsync_DbSaveThrowsUniqueConstraint2627_ThrowsAlreadyScheduled() + { + // Arrange - pre-checks pass, but the database rejects the insert with a unique-constraint + // violation (a race condition slipping past the in-app duplicate check). SQL error 2627 + // must be classified as a duplicate and surfaced to callers as the "already scheduled" path. + _context.SaveException = SqlExceptionFactory.Create(2627); + var testYear = DateTime.Now.Year + 1; + + // Act & Assert + var ex = await Assert.ThrowsAsync(() => + _service.AddInstructorAsync("test123", 1, new[] { 1 }, testYear, false, TestContext.Current.CancellationToken)); + Assert.Contains("already be scheduled", ex.Message); + Assert.IsType(ex.InnerException); + } + + [Fact] + public async Task AddInstructorAsync_DbSaveThrowsDuplicateKey2601WrappedInDbUpdate_ThrowsAlreadyScheduled() + { + // Arrange - EF wraps provider errors in DbUpdateException. SQL error 2601 (duplicate key in a + // unique index) must be unwrapped from the inner SqlException and classified as a duplicate. + _context.SaveException = new DbUpdateException("save failed", SqlExceptionFactory.Create(2601)); + var testYear = DateTime.Now.Year + 1; + + // Act & Assert + var ex = await Assert.ThrowsAsync(() => + _service.AddInstructorAsync("test123", 1, new[] { 1 }, testYear, false, TestContext.Current.CancellationToken)); + Assert.Contains("already be scheduled", ex.Message); + Assert.IsType(ex.InnerException); + } + + [Fact] + public async Task AddInstructorAsync_DbSaveThrowsNonDuplicateSqlError_ThrowsGenericDatabaseError() + { + // Arrange - a non-duplicate database error (547 = constraint/foreign-key violation) must NOT be + // treated as a duplicate; it falls through to the generic "database operation failed" message. + _context.SaveException = SqlExceptionFactory.Create(547); + var testYear = DateTime.Now.Year + 1; + + // Act & Assert + var ex = await Assert.ThrowsAsync(() => + _service.AddInstructorAsync("test123", 1, new[] { 1 }, testYear, false, TestContext.Current.CancellationToken)); + Assert.Contains("Database operation failed", ex.Message); + Assert.IsType(ex.InnerException); + } + [Fact] public async Task RemoveInstructorScheduleAsync_ValidSchedule_RemovesSuccessfully() { diff --git a/test/ClinicalScheduler/SqlExceptionFactory.cs b/test/ClinicalScheduler/SqlExceptionFactory.cs new file mode 100644 index 000000000..59bff40b3 --- /dev/null +++ b/test/ClinicalScheduler/SqlExceptionFactory.cs @@ -0,0 +1,79 @@ +using System.Reflection; +using Microsoft.Data.SqlClient; + +namespace Viper.test.ClinicalScheduler +{ + /// + /// Builds instances carrying a specific SQL Server error number. + /// SqlException/SqlError/SqlErrorCollection have no public constructors, so this reaches the + /// library's internal members via reflection. Targets Microsoft.Data.SqlClient and is resilient + /// to constructor-signature changes across versions. + /// + internal static class SqlExceptionFactory + { + public static SqlException Create(int errorNumber) + { + var errors = NewErrorCollection(); + AddError(errors, NewError(errorNumber)); + return NewException(errors); + } + + private static SqlErrorCollection NewErrorCollection() + => (SqlErrorCollection)Activator.CreateInstance(typeof(SqlErrorCollection), nonPublic: true)!; + + private static SqlError NewError(int number) + { + var ctor = typeof(SqlError) + .GetConstructors(BindingFlags.Instance | BindingFlags.NonPublic | BindingFlags.Public) + .OrderByDescending(c => c.GetParameters().Length) + .FirstOrDefault() + ?? throw new InvalidOperationException("No SqlError constructor found via reflection."); + + var parameters = ctor.GetParameters(); + var numberIndex = Array.FindIndex(parameters, p => string.Equals(p.Name, "infoNumber", StringComparison.OrdinalIgnoreCase)); + if (numberIndex < 0) + { + numberIndex = Array.FindIndex(parameters, p => p.ParameterType == typeof(int)); + } + + var args = new object?[parameters.Length]; + for (var i = 0; i < parameters.Length; i++) + { + args[i] = i == numberIndex ? number : DefaultArg(parameters[i]); + } + + return (SqlError)ctor.Invoke(args); + } + + private static object? DefaultArg(ParameterInfo parameter) + { + var type = parameter.ParameterType; + if (type == typeof(string)) return "test"; + if (type == typeof(byte)) return (byte)0; + if (type == typeof(int)) return 0; + if (type == typeof(uint)) return 0u; + if (type == typeof(bool)) return false; + if (parameter.HasDefaultValue) return parameter.DefaultValue; + return type.IsValueType ? Activator.CreateInstance(type) : null; + } + + private static void AddError(SqlErrorCollection errors, SqlError error) + { + var add = typeof(SqlErrorCollection).GetMethod("Add", BindingFlags.Instance | BindingFlags.NonPublic) + ?? throw new InvalidOperationException("SqlErrorCollection.Add not found via reflection."); + add.Invoke(errors, new object[] { error }); + } + + private static SqlException NewException(SqlErrorCollection errors) + { + var create = typeof(SqlException).GetMethod( + "CreateException", + BindingFlags.Static | BindingFlags.NonPublic, + binder: null, + types: new[] { typeof(SqlErrorCollection), typeof(string) }, + modifiers: null) + ?? throw new InvalidOperationException("SqlException.CreateException(SqlErrorCollection, string) not found via reflection."); + return (SqlException)create.Invoke(null, new object[] { errors, "11.0.0" })!; + } + } +} diff --git a/test/ClinicalScheduler/ToggleThrowContext.cs b/test/ClinicalScheduler/ToggleThrowContext.cs new file mode 100644 index 000000000..b4a45f333 --- /dev/null +++ b/test/ClinicalScheduler/ToggleThrowContext.cs @@ -0,0 +1,27 @@ +using Microsoft.EntityFrameworkCore; +using Viper.Classes.SQLContext; + +namespace Viper.test.ClinicalScheduler +{ + /// + /// In-memory that can be told to throw a specific exception + /// from SaveChanges, so tests can exercise the database-failure handling in + /// . Leave + /// null for normal behavior (e.g. seeding test data). + /// + internal sealed class ToggleThrowContext : ClinicalSchedulerContext + { + public ToggleThrowContext(DbContextOptions options) : base(options) + { + } + + /// When set, the next SaveChanges call throws this exception instead of persisting. + public Exception? SaveException { get; set; } + + public override int SaveChanges() + => SaveException is not null ? throw SaveException : base.SaveChanges(); + + public override Task SaveChangesAsync(CancellationToken cancellationToken = default) + => SaveException is not null ? Task.FromException(SaveException) : base.SaveChangesAsync(cancellationToken); + } +} diff --git a/web/Areas/ClinicalScheduler/Services/ScheduleEditService.cs b/web/Areas/ClinicalScheduler/Services/ScheduleEditService.cs index b33f56777..73dac6b15 100644 --- a/web/Areas/ClinicalScheduler/Services/ScheduleEditService.cs +++ b/web/Areas/ClinicalScheduler/Services/ScheduleEditService.cs @@ -247,12 +247,9 @@ await _auditService.LogPrimaryEvaluatorSetAsync( _logger.LogError(saveEx, "Database save failed for MothraId='{MothraId}', RotationId={RotationId}, WeekIds=[{WeekIds}]", LogSanitizer.SanitizeId(mothraId), rotationId, string.Join(",", weekIds)); - // Check if this is a database constraint violation (typically a duplicate key error) - var errorMessage = saveEx.Message?.ToLower(); - var innerMessage = saveEx.InnerException?.Message?.ToLower(); - - if ((errorMessage != null && (errorMessage.Contains("duplicate") || errorMessage.Contains("unique") || errorMessage.Contains("constraint") || errorMessage.Contains("violation of primary key"))) || - (innerMessage != null && (innerMessage.Contains("duplicate") || innerMessage.Contains("unique") || innerMessage.Contains("constraint") || innerMessage.Contains("violation of primary key")))) + // Only a unique/duplicate-key violation means the instructor is already scheduled. + // Other DB errors (foreign-key, check constraints, etc.) fall through to the generic message. + if (IsDuplicateKeyViolation(saveEx)) { throw new InvalidOperationException($"Instructor {mothraId} appears to already be scheduled for one or more of the specified weeks. Please refresh the page and try again.", saveEx); } @@ -262,6 +259,15 @@ await _auditService.LogPrimaryEvaluatorSetAsync( } } + // SQL Server: 2627 = unique constraint violation, 2601 = duplicate key in a unique index. + // EF wraps the provider error in DbUpdateException, so unwrap to the underlying SqlException. + private static bool IsDuplicateKeyViolation(Exception ex) + { + var sqlEx = ex as SqlException ?? ex.InnerException as SqlException; + return sqlEx is not null + && sqlEx.Errors.Cast().Any(e => e.Number is 2627 or 2601); + } + public async Task<(bool success, bool wasPrimaryEvaluator, string? instructorName)> RemoveInstructorScheduleAsync( int instructorScheduleId, CancellationToken cancellationToken = default)