From 6c8a2e1515784ffcca50e1443bf8486de9709865 Mon Sep 17 00:00:00 2001 From: Rex Lorenzo Date: Sat, 22 Aug 2026 08:09:45 -0700 Subject: [PATCH 1/2] fix(raps): accept a create body without a role template id RoleTemplateId was marked required, so System.Text.Json rejected the create body before PostRoleTemplate ran: the id is database-generated, the form has none yet, and JSON.stringify drops the undefined property. Every "Add Role Template" save returned 400. Editing was unaffected, because the selected row carries an id. Nullable rather than a plain int so under-posting stays visible to the analyzer, and to match the other create/update models in the codebase; PutRoleTemplate's route-vs-body guard rejects a null id. --- test/RAPS/RoleTemplateCreateUpdateTests.cs | 38 ++++++ test/RAPS/RoleTemplateCrudTests.cs | 110 ++++++++++++++++++ .../RAPS/Models/RoleTemplateCreateUpdate.cs | 2 +- 3 files changed, 149 insertions(+), 1 deletion(-) create mode 100644 test/RAPS/RoleTemplateCreateUpdateTests.cs create mode 100644 test/RAPS/RoleTemplateCrudTests.cs diff --git a/test/RAPS/RoleTemplateCreateUpdateTests.cs b/test/RAPS/RoleTemplateCreateUpdateTests.cs new file mode 100644 index 000000000..182039f0b --- /dev/null +++ b/test/RAPS/RoleTemplateCreateUpdateTests.cs @@ -0,0 +1,38 @@ +using System.Text.Json; +using Viper.Areas.RAPS.Models; + +namespace Viper.test.RAPS +{ + // Controller tests build the model in C#, so only these catch a body shape that 400s + // during model binding, before the action runs. + public class RoleTemplateCreateUpdateTests + { + [Fact] + public void CreateBody_OmittingTheId_StillBinds() + { + // The create form has no id yet, so JSON.stringify drops roleTemplateId entirely. + var model = Deserialize(@"{""templateName"":""Reception"",""description"":""Front desk staff""}"); + + Assert.Null(model.RoleTemplateId); + Assert.Equal("Reception", model.TemplateName); + Assert.Equal("Front desk staff", model.Description); + } + + [Fact] + public void UpdateBody_KeepsTheId() + { + var model = Deserialize(@"{""roleTemplateId"":5,""templateName"":""Reception""}"); + + Assert.Equal(5, model.RoleTemplateId); + } + + // JsonSerializerOptions.Web matches how MVC binds JSON bodies. + private static RoleTemplateCreateUpdate Deserialize(string body) + { + RoleTemplateCreateUpdate? model = JsonSerializer.Deserialize(body, JsonSerializerOptions.Web); + + Assert.NotNull(model); + return model; + } + } +} diff --git a/test/RAPS/RoleTemplateCrudTests.cs b/test/RAPS/RoleTemplateCrudTests.cs new file mode 100644 index 000000000..31cc8760b --- /dev/null +++ b/test/RAPS/RoleTemplateCrudTests.cs @@ -0,0 +1,110 @@ +using Microsoft.AspNetCore.Mvc; +using Microsoft.Data.Sqlite; +using Microsoft.EntityFrameworkCore; +using Viper.Areas.RAPS.Controllers; +using Viper.Areas.RAPS.Models; +using Viper.Classes.SQLContext; +using Viper.Models.RAPS; + +namespace Viper.test.RAPS +{ + public class RoleTemplateCrudTests + { + [Theory] + [InlineData("Front desk staff", "Front desk staff")] + [InlineData(null, "")] + public async Task PostRoleTemplate_Creates_WhenBodyOmitsTheId(string? description, string expected) + { + using var connection = await OpenConnectionAsync(); + using var context = await CreateContextAsync(connection); + var controller = CreateController(context); + + var result = await controller.PostRoleTemplate("VIPER", new RoleTemplateCreateUpdate + { + TemplateName = "Reception", + Description = description + }); + + var saved = Assert.IsType(Assert.IsType(result.Result).Value); + Assert.True(saved.RoleTemplateId > 0); + Assert.Equal("Reception", saved.TemplateName); + Assert.Equal(expected, saved.Description); + } + + [Fact] + public async Task PutRoleTemplate_Updates_WhenBodyIdMatchesRoute() + { + using var connection = await OpenConnectionAsync(); + using var context = await CreateContextAsync(connection); + var existing = await SeedTemplateAsync(context); + var controller = CreateController(context); + + var result = await controller.PutRoleTemplate("VIPER", existing.RoleTemplateId, new RoleTemplateCreateUpdate + { + RoleTemplateId = existing.RoleTemplateId, + TemplateName = "Renamed", + Description = "Updated" + }); + + Assert.IsType(result); + + // Reload untracked: asserting on the tracked entity would pass on an in-memory + // mutation even if the save never reached the database. + context.ChangeTracker.Clear(); + RoleTemplate reloaded = await context.RoleTemplates.AsNoTracking() + .SingleAsync(t => t.RoleTemplateId == existing.RoleTemplateId, TestContext.Current.CancellationToken); + Assert.Equal("Renamed", reloaded.TemplateName); + Assert.Equal("Updated", reloaded.Description); + } + + [Fact] + public async Task PutRoleTemplate_ReturnsBadRequest_WhenBodyOmitsTheId() + { + // Nullable for the create form, but an update still has to agree with the route id. + using var connection = await OpenConnectionAsync(); + using var context = await CreateContextAsync(connection); + var existing = await SeedTemplateAsync(context); + var controller = CreateController(context); + + var result = await controller.PutRoleTemplate("VIPER", existing.RoleTemplateId, new RoleTemplateCreateUpdate + { + TemplateName = "Renamed" + }); + + Assert.IsType(result); + } + + private static async Task OpenConnectionAsync() + { + var connection = new SqliteConnection("Filename=:memory:"); + await connection.OpenAsync(TestContext.Current.CancellationToken); + return connection; + } + + private static async Task CreateContextAsync(SqliteConnection connection) + { + var options = new DbContextOptionsBuilder() + .UseSqlite(connection) + .Options; + var context = new RAPSContext(options); + await context.Database.EnsureCreatedAsync(TestContext.Current.CancellationToken); + return context; + } + + // The AAUD context only reaches the cache service, which neither create nor update touches. + private static RoleTemplatesController CreateController(RAPSContext context) + { + var aaudContext = new AAUDContext(new DbContextOptionsBuilder() + .UseInMemoryDatabase("AAUD_" + Guid.NewGuid()).Options); + return new RoleTemplatesController(context, aaudContext); + } + + private static async Task SeedTemplateAsync(RAPSContext context) + { + var template = new RoleTemplate { TemplateName = "Reception", Description = "Front desk staff" }; + context.RoleTemplates.Add(template); + await context.SaveChangesAsync(TestContext.Current.CancellationToken); + return template; + } + } +} diff --git a/web/Areas/RAPS/Models/RoleTemplateCreateUpdate.cs b/web/Areas/RAPS/Models/RoleTemplateCreateUpdate.cs index 4de4a5162..74859b656 100644 --- a/web/Areas/RAPS/Models/RoleTemplateCreateUpdate.cs +++ b/web/Areas/RAPS/Models/RoleTemplateCreateUpdate.cs @@ -2,7 +2,7 @@ namespace Viper.Areas.RAPS.Models { public class RoleTemplateCreateUpdate { - public required int RoleTemplateId { get; set; } + public int? RoleTemplateId { get; set; } public string TemplateName { get; set; } = null!; public string? Description { get; set; } } From 81af1f53588ca84f01de6bc6e8b73b8aa827ffe1 Mon Sep 17 00:00:00 2001 From: Rex Lorenzo Date: Sat, 22 Aug 2026 08:13:34 -0700 Subject: [PATCH 2/2] docs: warn against required primary keys on bound models The RAPS role template create broke this way: System.Text.Json rejects a body that omits a required property, so a create form with no id yet gets a 400 before the action runs. Four other endpoints are one client edit away from the same failure, saved only by a hardcoded 0. --- CLAUDE.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CLAUDE.md b/CLAUDE.md index 0f143b3d9..00b56ab0d 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -39,6 +39,7 @@ When I ask a question or make an observation, respond with an answer - do NOT ju - **Paths**: `Path.Join()` not `Path.Combine()` (`Combine` silently discards everything before a rooted segment) | **DateTime**: prefer `DateTimeKind.Local` - **Mapperly**: Prefer over manual property mapping. Static partial mapper class per area with `[Mapper(RequiredMappingStrategy = RequiredMappingStrategy.None)]`. Use `[MapperIgnoreTarget]` for computed properties, manual wrappers for transforms. Align entity/DTO names: use EF `HasColumnName()` to decouple from DB columns. - **Scrutor**: Convention-based DI auto-registers `*Service`/`*Validator` from configured namespaces, prefer over manual `AddScoped`. Follow `IFooService`/`FooService` naming. Explicit `AddScoped` before Scrutor takes precedence (`RegistrationStrategy.Skip`). +- **`required` on bound models**: never on a server-generated primary key. A create body has no id yet, so System.Text.Json 400s it before the action runs. Use `int?` (a client sending `0` only masks it). - **Bug fixes**: Check for duplicate/parallel implementations of the affected logic and fix consistently, or DRY into a shared method. - **Log injection**: Sanitize user input before logging via `LogSanitizer` (`SanitizeId()`, `SanitizeString()`, `SanitizeYear()`). Skip hard-coded strings, enums, DB values.