diff --git a/src/Novelly.Api/Agent/AgentConversation.cs b/src/Novelly.Api/Agent/AgentConversation.cs index 20596cd..f3a917c 100644 --- a/src/Novelly.Api/Agent/AgentConversation.cs +++ b/src/Novelly.Api/Agent/AgentConversation.cs @@ -5,16 +5,16 @@ namespace Novelly.Api.Agent; /// A chat thread between the writer and the embedded agent, scoped to one project. public class AgentConversation { - public Guid Id { get; set; } = Guid.NewGuid(); - public Guid ProjectId { get; set; } - public Project? Project { get; set; } + public Guid Id { get; init; } = Guid.NewGuid(); + public Guid ProjectId { get; init; } + public Project? Project { get; init; } - public string Title { get; set; } = "New conversation"; + public string Title { get; init; } = "New conversation"; - public DateTimeOffset CreatedAt { get; set; } = DateTimeOffset.UtcNow; + public DateTimeOffset CreatedAt { get; init; } = DateTimeOffset.UtcNow; public DateTimeOffset UpdatedAt { get; set; } = DateTimeOffset.UtcNow; - public List Messages { get; set; } = []; + public List Messages { get; init; } = []; } /// @@ -24,26 +24,26 @@ public class AgentConversation /// public class AgentMessage { - public Guid Id { get; set; } = Guid.NewGuid(); - public Guid ConversationId { get; set; } - public AgentConversation? Conversation { get; set; } + public Guid Id { get; init; } = Guid.NewGuid(); + public Guid ConversationId { get; init; } + public AgentConversation? Conversation { get; init; } - public AgentRole Role { get; set; } + public AgentRole Role { get; init; } /// /// Position in the conversation, 0-based. Timestamps are not enough to order a /// transcript: a fast turn can produce two messages inside the same tick. /// - public int Sequence { get; set; } + public int Sequence { get; init; } /// The visible text of the turn. - public string Content { get; set; } = string.Empty; + public string Content { get; init; } = string.Empty; /// /// JSON array of { name, input, result } objects describing tool calls made /// during this turn. Null on user turns and on assistant turns that used no tools. /// - public string? ToolCallsJson { get; set; } + public string? ToolCallsJson { get; init; } - public DateTimeOffset CreatedAt { get; set; } = DateTimeOffset.UtcNow; + public DateTimeOffset CreatedAt { get; init; } = DateTimeOffset.UtcNow; } diff --git a/src/Novelly.Api/Agent/AgentDtos.cs b/src/Novelly.Api/Agent/AgentDtos.cs index d60eb70..38a0050 100644 --- a/src/Novelly.Api/Agent/AgentDtos.cs +++ b/src/Novelly.Api/Agent/AgentDtos.cs @@ -1,3 +1,5 @@ +using Novelly.Api.Common.Validation; + namespace Novelly.Api.Agent; public record ConversationSummaryDto( @@ -26,4 +28,19 @@ public record ToolCallDto(string Name, string Input, string Result); public record SendAgentMessageRequest(string Message, Guid? ConversationId = null); +public class SendAgentMessageRequestValidator : IModelValidator +{ + public ValidationResult Validate(SendAgentMessageRequest model) + { + var result = new ValidationResult(); + + if (string.IsNullOrWhiteSpace(model.Message)) + result.AddError("Message", "'Message' must not be empty."); + else if (model.Message.Length > 20000) + result.AddError("Message", "'Message' must be 20,000 characters or fewer."); + + return result; + } +} + public record AgentTurnDto(Guid ConversationId, AgentMessageDto Message); diff --git a/src/Novelly.Api/Agent/AgentEndpoints.cs b/src/Novelly.Api/Agent/AgentEndpoints.cs index 6540a9b..addf1b0 100644 --- a/src/Novelly.Api/Agent/AgentEndpoints.cs +++ b/src/Novelly.Api/Agent/AgentEndpoints.cs @@ -1,4 +1,5 @@ using Novelly.Api.Common; +using Novelly.Api.Common.Validation; namespace Novelly.Api.Agent; @@ -6,7 +7,9 @@ public static class AgentEndpoints { public static IEndpointRouteBuilder MapAgentEndpoints(this IEndpointRouteBuilder app) { - var projectScoped = app.MapGroup("/api/projects/{projectId:guid}/agent").WithTags("Agent").AddEndpointFilter(); + var projectScoped = app.MapGroup("/api/projects/{projectId:guid}/agent").WithTags("Agent") + .AddEndpointFilter() + .AddEndpointFilter(); projectScoped.MapGet("/conversations", async ( Guid projectId, NovelAgentService agent, CancellationToken ct) => @@ -21,17 +24,16 @@ public static class AgentEndpoints Results.Ok(await agent.SendMessageAsync(projectId, request, ct))) .WithSummary("Send a message to the writing agent and run it to completion."); - var conversations = app.MapGroup("/api/conversations").WithTags("Agent").AddEndpointFilter(); + var conversations = app.MapGroup("/api/conversations").WithTags("Agent") + .AddEndpointFilter() + .AddEndpointFilter(); conversations.MapGet("/{id:guid}", async (Guid id, NovelAgentService agent, CancellationToken ct) => - Results.Ok(await agent.GetConversationAsync(id, ct))) + (await agent.GetConversationAsync(id, ct)).ToApiResult()) .WithSummary("Read a conversation's full transcript."); conversations.MapDelete("/{id:guid}", async (Guid id, NovelAgentService agent, CancellationToken ct) => - { - await agent.DeleteConversationAsync(id, ct); - return Results.NoContent(); - }) + await agent.DeleteConversationAsync(id, ct) ? Results.NoContent() : Results.NotFound()) .WithSummary("Delete a conversation."); return app; diff --git a/src/Novelly.Api/Agent/NovelAgentService.cs b/src/Novelly.Api/Agent/NovelAgentService.cs index d28eb71..383e0e0 100644 --- a/src/Novelly.Api/Agent/NovelAgentService.cs +++ b/src/Novelly.Api/Agent/NovelAgentService.cs @@ -4,6 +4,7 @@ using Microsoft.EntityFrameworkCore; using Microsoft.Extensions.Logging; using Microsoft.Extensions.Options; using Novelly.Api.Common; +using Novelly.Api.Common.Validation; using Novelly.Api.Data; using Novelly.Api.Projects; @@ -18,7 +19,8 @@ public class NovelAgentService( IAgentModelClient model, NovelAgentToolset toolset, IOptions options, - ILogger logger) + ILogger logger, + IModelValidator sendMessageValidator) { private static readonly JsonSerializerOptions JsonOptions = new() { @@ -39,11 +41,18 @@ public class NovelAgentService( .ToListAsync(ct); } - public async Task GetConversationAsync(Guid conversationId, CancellationToken ct = default) + /// Null when no conversation has this id — a lookup miss is expected, not exceptional. + public async Task GetConversationAsync(Guid conversationId, CancellationToken ct = default) { + Guard.Default(conversationId, nameof(conversationId)); + logger.LogInformation("Getting agent conversation {ConversationId}", conversationId); - var conversation = await LoadConversationAsync(conversationId, ct); + var conversation = await FindConversationAsync(conversationId, ct); + if (conversation is null) + { + return null; + } return new ConversationDto( conversation.Id, @@ -53,13 +62,22 @@ public class NovelAgentService( conversation.UpdatedAt); } - public async Task DeleteConversationAsync(Guid conversationId, CancellationToken ct = default) + /// True if a conversation was deleted; false if no conversation had this id. + public async Task DeleteConversationAsync(Guid conversationId, CancellationToken ct = default) { + Guard.Default(conversationId, nameof(conversationId)); + logger.LogInformation("Deleting agent conversation {ConversationId}", conversationId); - var conversation = await LoadConversationAsync(conversationId, ct); + var conversation = await FindConversationAsync(conversationId, ct); + if (conversation is null) + { + return false; + } + db.Conversations.Remove(conversation); await db.SaveChangesAsync(ct); + return true; } /// @@ -69,12 +87,19 @@ public class NovelAgentService( public async Task SendMessageAsync( Guid projectId, SendAgentMessageRequest request, CancellationToken ct = default) { + Guard.Default(projectId, nameof(projectId)); + Guard.Null(request, nameof(request)); + sendMessageValidator.Validate(request).ThrowIfInvalid(); + logger.LogInformation( "Sending agent message for project {ProjectId}, conversation {ConversationId}, message length {MessageLength}", projectId, request.ConversationId, request.Message.Length); var conversation = request.ConversationId is { } id - ? await LoadConversationAsync(id, ct) + ? await FindConversationAsync(id, ct) + // The id came from the request body, not the route — an unknown id here + // is bad input to this call, not a direct "fetch conversation" lookup. + ?? throw new NotFoundException(nameof(AgentConversation), id) : await StartConversationAsync(projectId, request.Message, ct); // Persist the user's turn before running the loop. The tools save through the @@ -204,9 +229,9 @@ public class NovelAgentService( return conversation; } - private async Task LoadConversationAsync(Guid conversationId, CancellationToken ct) + private async Task FindConversationAsync(Guid conversationId, CancellationToken ct) { - logger.LogDebug("Loading agent conversation {ConversationId}", conversationId); + logger.LogDebug("Finding agent conversation {ConversationId}", conversationId); var conversation = await db.Conversations .Include(c => c.Messages) @@ -214,8 +239,7 @@ public class NovelAgentService( if (conversation is null) { - logger.LogWarning("AgentConversation {ConversationId} not found", conversationId); - throw new NotFoundException(nameof(AgentConversation), conversationId); + logger.LogInformation("AgentConversation {ConversationId} not found", conversationId); } return conversation; diff --git a/src/Novelly.Api/Agent/NovelAgentToolset.cs b/src/Novelly.Api/Agent/NovelAgentToolset.cs index 1007e71..c1c017c 100644 --- a/src/Novelly.Api/Agent/NovelAgentToolset.cs +++ b/src/Novelly.Api/Agent/NovelAgentToolset.cs @@ -13,6 +13,13 @@ namespace Novelly.Api.Agent; /// The outcome of running a tool: what to hand back to the model, and whether it failed. public record AgentToolResult(string Content, bool IsError); +/// +/// A lookup a tool performed came back empty. Not an exception — the underlying service +/// already said so by returning null/false — just a value +/// recognises and turns into the same error-result shape a caught exception would produce. +/// +internal record ToolNotFound(string Message); + /// A tool the agent can call, bound to a handler that runs against the project's data. public record AgentTool( string Name, @@ -44,7 +51,7 @@ public class NovelAgentToolset( private Dictionary? _byName; - public IReadOnlyList Tools => [.. ByName.Values]; + private IReadOnlyList Tools => [.. ByName.Values]; public IReadOnlyList Definitions => [.. Tools.Select(t => new AgentToolDefinition(t.Name, t.Description, t.InputSchema))]; @@ -53,8 +60,7 @@ public class NovelAgentToolset( /// Runs a tool and serialises its result. Failures come back as text rather than /// exceptions so the model can read the message and correct itself. /// - public async Task ExecuteAsync( - string name, Guid projectId, JsonElement input, CancellationToken ct = default) + public async Task ExecuteAsync(string name, Guid projectId, JsonElement input, CancellationToken ct = default) { if (!ByName.TryGetValue(name, out var tool)) { @@ -67,6 +73,13 @@ public class NovelAgentToolset( try { var result = await tool.Handler(projectId, input, ct); + + if (result is ToolNotFound notFound) + { + logger.LogInformation("Tool {Tool} for project {ProjectId} found nothing: {Message}", name, projectId, notFound.Message); + return new AgentToolResult(notFound.Message, true); + } + logger.LogDebug("Tool {Tool} for project {ProjectId} succeeded", name, projectId); return new AgentToolResult(JsonSerializer.Serialize(result, SerializerOptions), false); } @@ -87,6 +100,14 @@ public class NovelAgentToolset( } } + /// Turns a nullable lookup into either the value or a the model can read. + private static async Task OrNotFound(Task lookup, string entity, Guid id) where T : class => + await lookup as object ?? new ToolNotFound($"{entity} '{id}' was not found."); + + /// Turns a delete's success flag into either a confirmation or a . + private static async Task DeletedOrNotFound(Task delete, string entity, Guid id) => + await delete ? new { deleted = true } : new ToolNotFound($"{entity} '{id}' was not found."); + private Dictionary ByName => _byName ??= Build().ToDictionary(t => t.Name); private IEnumerable Build() @@ -96,7 +117,7 @@ public class NovelAgentToolset( "Read the project's title, logline, synopsis, genre, notes and word-count target. " + "Call this first in a conversation to ground yourself in what the book is.", new JsonSchemaBuilder().Build(), - async (projectId, _, ct) => await projects.GetAsync(projectId, ct)); + async (projectId, _, ct) => await OrNotFound(projects.GetAsync(projectId, ct), "Project", projectId)); yield return new AgentTool( "update_project_brief", @@ -111,14 +132,14 @@ public class NovelAgentToolset( .Str("notes", "Free-form notes on theme, tone, comparable titles.") .Int("target_word_count", "Target manuscript length in words.") .Build(), - async (projectId, input, ct) => await projects.UpdateAsync(projectId, new UpdateProjectRequest( + async (projectId, input, ct) => await OrNotFound(projects.UpdateAsync(projectId, new UpdateProjectRequest( JsonInput.String(input, "title"), JsonInput.String(input, "author"), JsonInput.String(input, "genre"), JsonInput.String(input, "logline"), JsonInput.String(input, "synopsis"), JsonInput.String(input, "notes"), - JsonInput.Int(input, "target_word_count")), ct)); + JsonInput.Int(input, "target_word_count")), ct), "Project", projectId)); yield return new AgentTool( "list_characters", @@ -156,26 +177,30 @@ public class NovelAgentToolset( CharacterSchema(includeName: true, nameRequired: false) .Str("character_id", "Id of the character to update.", required: true) .Build(), - async (_, input, ct) => await characters.UpdateAsync( - JsonInput.RequiredGuid(input, "character_id"), - new UpdateCharacterRequest( - JsonInput.String(input, "name"), - JsonInput.Enum(input, "role"), - JsonInput.Enum(input, "importance"), - JsonInput.String(input, "age"), - JsonInput.String(input, "pronouns"), - JsonInput.String(input, "occupation"), - JsonInput.String(input, "appearance"), - JsonInput.String(input, "personality"), - JsonInput.String(input, "backstory"), - JsonInput.String(input, "want"), - JsonInput.String(input, "need"), - JsonInput.String(input, "internal_conflict"), - JsonInput.String(input, "external_conflict"), - JsonInput.String(input, "arc_summary"), - JsonInput.String(input, "voice"), - JsonInput.String(input, "notes"), - JsonInput.Strings(input, "tags")), ct)); + async (_, input, ct) => + { + var characterId = JsonInput.RequiredGuid(input, "character_id"); + return await OrNotFound(characters.UpdateAsync( + characterId, + new UpdateCharacterRequest( + JsonInput.String(input, "name"), + JsonInput.Enum(input, "role"), + JsonInput.Enum(input, "importance"), + JsonInput.String(input, "age"), + JsonInput.String(input, "pronouns"), + JsonInput.String(input, "occupation"), + JsonInput.String(input, "appearance"), + JsonInput.String(input, "personality"), + JsonInput.String(input, "backstory"), + JsonInput.String(input, "want"), + JsonInput.String(input, "need"), + JsonInput.String(input, "internal_conflict"), + JsonInput.String(input, "external_conflict"), + JsonInput.String(input, "arc_summary"), + JsonInput.String(input, "voice"), + JsonInput.String(input, "notes"), + JsonInput.Strings(input, "tags")), ct), "Character", characterId); + }); yield return new AgentTool( "get_chapter_outline", @@ -213,16 +238,20 @@ public class NovelAgentToolset( .Str("beat_id", "Id of the beat to update.", required: true) .Str("title", "Three to five words naming the beat.") .Build(), - async (_, input, ct) => await beats.UpdateAsync( - JsonInput.RequiredGuid(input, "beat_id"), - new UpdateBeatRequest( - JsonInput.String(input, "title"), - JsonInput.Int(input, "sort_order"), - JsonInput.Guid(input, "character_id"), - JsonInput.String(input, "what_happened"), - JsonInput.String(input, "whats_next"), - JsonInput.Guid(input, "scene_id"), - JsonInput.Strings(input, "tags")), ct)); + async (_, input, ct) => + { + var beatId = JsonInput.RequiredGuid(input, "beat_id"); + return await OrNotFound(beats.UpdateAsync( + beatId, + new UpdateBeatRequest( + JsonInput.String(input, "title"), + JsonInput.Int(input, "sort_order"), + JsonInput.Guid(input, "character_id"), + JsonInput.String(input, "what_happened"), + JsonInput.String(input, "whats_next"), + JsonInput.Guid(input, "scene_id"), + JsonInput.Strings(input, "tags")), ct), "Beat", beatId); + }); yield return new AgentTool( "delete_beat", @@ -232,8 +261,8 @@ public class NovelAgentToolset( .Build(), async (_, input, ct) => { - await beats.DeleteAsync(JsonInput.RequiredGuid(input, "beat_id"), ct); - return new { deleted = true }; + var beatId = JsonInput.RequiredGuid(input, "beat_id"); + return await DeletedOrNotFound(beats.DeleteAsync(beatId, ct), "Beat", beatId); }); yield return new AgentTool( @@ -265,7 +294,11 @@ public class NovelAgentToolset( new JsonSchemaBuilder() .Str("tag_id", "Id of the tag to trace.", required: true) .Build(), - async (_, input, ct) => await tags.GetReferencesAsync(JsonInput.RequiredGuid(input, "tag_id"), ct)); + async (_, input, ct) => + { + var tagId = JsonInput.RequiredGuid(input, "tag_id"); + return await OrNotFound(tags.GetReferencesAsync(tagId, ct), "Tag", tagId); + }); yield return new AgentTool( "list_chapters", @@ -279,7 +312,11 @@ public class NovelAgentToolset( new JsonSchemaBuilder() .Str("chapter_id", "Id of the chapter to read.", required: true) .Build(), - async (_, input, ct) => await chapters.GetAsync(JsonInput.RequiredGuid(input, "chapter_id"), ct)); + async (_, input, ct) => + { + var chapterId = JsonInput.RequiredGuid(input, "chapter_id"); + return await OrNotFound(chapters.GetAsync(chapterId, ct), "Chapter", chapterId); + }); yield return new AgentTool( "create_chapter", @@ -321,18 +358,22 @@ public class NovelAgentToolset( .Int("target_word_count", "Target length in words.") .StringArray("tags", "Tags for cross-referencing. Replaces the existing tags.") .Build(), - async (_, input, ct) => await chapters.UpdateAsync( - JsonInput.RequiredGuid(input, "chapter_id"), - new UpdateChapterRequest( - JsonInput.String(input, "title"), - JsonInput.Int(input, "number"), - JsonInput.String(input, "summary"), - JsonInput.Guid(input, "pov_character_id"), - JsonInput.String(input, "setting"), - JsonInput.String(input, "notes"), - JsonInput.Enum(input, "status"), - JsonInput.Int(input, "target_word_count"), - JsonInput.Strings(input, "tags")), ct)); + async (_, input, ct) => + { + var chapterId = JsonInput.RequiredGuid(input, "chapter_id"); + return await OrNotFound(chapters.UpdateAsync( + chapterId, + new UpdateChapterRequest( + JsonInput.String(input, "title"), + JsonInput.Int(input, "number"), + JsonInput.String(input, "summary"), + JsonInput.Guid(input, "pov_character_id"), + JsonInput.String(input, "setting"), + JsonInput.String(input, "notes"), + JsonInput.Enum(input, "status"), + JsonInput.Int(input, "target_word_count"), + JsonInput.Strings(input, "tags")), ct), "Chapter", chapterId); + }); yield return new AgentTool( "create_scene", @@ -364,19 +405,23 @@ public class NovelAgentToolset( .Str("scene_id", "Id of the scene to update.", required: true) .Str("title", "New title.") .Build(), - async (_, input, ct) => await scenes.UpdateAsync( - JsonInput.RequiredGuid(input, "scene_id"), - new UpdateSceneRequest( - JsonInput.String(input, "title"), - JsonInput.Int(input, "sort_order"), - JsonInput.String(input, "summary"), - JsonInput.String(input, "goal"), - JsonInput.String(input, "conflict"), - JsonInput.String(input, "outcome"), - JsonInput.Guid(input, "pov_character_id"), - JsonInput.String(input, "location"), - JsonInput.String(input, "prose"), - JsonInput.Enum(input, "status")), ct)); + async (_, input, ct) => + { + var sceneId = JsonInput.RequiredGuid(input, "scene_id"); + return await OrNotFound(scenes.UpdateAsync( + sceneId, + new UpdateSceneRequest( + JsonInput.String(input, "title"), + JsonInput.Int(input, "sort_order"), + JsonInput.String(input, "summary"), + JsonInput.String(input, "goal"), + JsonInput.String(input, "conflict"), + JsonInput.String(input, "outcome"), + JsonInput.Guid(input, "pov_character_id"), + JsonInput.String(input, "location"), + JsonInput.String(input, "prose"), + JsonInput.Enum(input, "status")), ct), "Scene", sceneId); + }); yield return new AgentTool( "get_character_beats", @@ -386,8 +431,11 @@ public class NovelAgentToolset( new JsonSchemaBuilder() .Str("character_id", "Id of the character.", required: true) .Build(), - async (_, input, ct) => await beats.ListForCharacterAsync( - JsonInput.RequiredGuid(input, "character_id"), ct)); + async (_, input, ct) => + { + var characterId = JsonInput.RequiredGuid(input, "character_id"); + return await OrNotFound(beats.ListForCharacterAsync(characterId, ct), "Character", characterId); + }); yield return new AgentTool( "get_character_arc", @@ -422,13 +470,17 @@ public class NovelAgentToolset( .Str("arc_stage_id", "Id of the arc stage to update.", required: true) .Str("title", "New title for the stage.") .Build(), - async (_, input, ct) => await arcs.UpdateAsync( - JsonInput.RequiredGuid(input, "arc_stage_id"), - new UpdateArcStageRequest( - JsonInput.String(input, "title"), - JsonInput.Int(input, "sort_order"), - JsonInput.String(input, "description"), - JsonInput.Guid(input, "chapter_id")), ct)); + async (_, input, ct) => + { + var arcStageId = JsonInput.RequiredGuid(input, "arc_stage_id"); + return await OrNotFound(arcs.UpdateAsync( + arcStageId, + new UpdateArcStageRequest( + JsonInput.String(input, "title"), + JsonInput.Int(input, "sort_order"), + JsonInput.String(input, "description"), + JsonInput.Guid(input, "chapter_id")), ct), "CharacterArcStage", arcStageId); + }); yield return new AgentTool( "delete_arc_stage", @@ -438,8 +490,8 @@ public class NovelAgentToolset( .Build(), async (_, input, ct) => { - await arcs.DeleteAsync(JsonInput.RequiredGuid(input, "arc_stage_id"), ct); - return new { deleted = true }; + var arcStageId = JsonInput.RequiredGuid(input, "arc_stage_id"); + return await DeletedOrNotFound(arcs.DeleteAsync(arcStageId, ct), "CharacterArcStage", arcStageId); }); yield return new AgentTool( @@ -499,11 +551,15 @@ public class NovelAgentToolset( .Str("resolution", "What was decided.", required: true) .Bool("append_to_notes", "Also append the resolution to the associated notes.") .Build(), - async (_, input, ct) => await questions.ResolveAsync( - JsonInput.RequiredGuid(input, "question_id"), - new ResolveOpenQuestionRequest( - JsonInput.RequiredString(input, "resolution"), - JsonInput.Bool(input, "append_to_notes") ?? false), ct)); + async (_, input, ct) => + { + var questionId = JsonInput.RequiredGuid(input, "question_id"); + return await OrNotFound(questions.ResolveAsync( + questionId, + new ResolveOpenQuestionRequest( + JsonInput.RequiredString(input, "resolution"), + JsonInput.Bool(input, "append_to_notes") ?? false), ct), "OpenQuestion", questionId); + }); yield return new AgentTool( "reopen_question", @@ -511,8 +567,11 @@ public class NovelAgentToolset( new JsonSchemaBuilder() .Str("question_id", "Id of the question to reopen.", required: true) .Build(), - async (_, input, ct) => await questions.ReopenAsync( - JsonInput.RequiredGuid(input, "question_id"), ct)); + async (_, input, ct) => + { + var questionId = JsonInput.RequiredGuid(input, "question_id"); + return await OrNotFound(questions.ReopenAsync(questionId, ct), "OpenQuestion", questionId); + }); yield return new AgentTool( "delete_open_question", @@ -522,8 +581,8 @@ public class NovelAgentToolset( .Build(), async (_, input, ct) => { - await questions.DeleteAsync(JsonInput.RequiredGuid(input, "question_id"), ct); - return new { deleted = true }; + var questionId = JsonInput.RequiredGuid(input, "question_id"); + return await DeletedOrNotFound(questions.DeleteAsync(questionId, ct), "OpenQuestion", questionId); }); } diff --git a/src/Novelly.Api/Beats/BeatDtos.cs b/src/Novelly.Api/Beats/BeatDtos.cs index 169d4b3..fc199fc 100644 --- a/src/Novelly.Api/Beats/BeatDtos.cs +++ b/src/Novelly.Api/Beats/BeatDtos.cs @@ -1,3 +1,4 @@ +using Novelly.Api.Common.Validation; using Novelly.Api.Tags; namespace Novelly.Api.Beats; @@ -25,6 +26,23 @@ public record CreateBeatRequest( Guid? SceneId = null, IReadOnlyList? Tags = null); +public class CreateBeatRequestValidator : IModelValidator +{ + public ValidationResult Validate(CreateBeatRequest model) + { + var result = new ValidationResult(); + + if (string.IsNullOrWhiteSpace(model.Title)) + result.AddError("Title", "'Title' must not be empty."); + else if (model.Title.Length > 200) + result.AddError("Title", "'Title' must be 200 characters or fewer."); + + BeatValidation.OptionalFields(model.SortOrder, model.WhatHappened, model.WhatsNext, model.Tags, result); + + return result; + } +} + /// /// Patch-style update. A null field is left alone; an empty string clears it. Passing a /// list replaces the beat's tags outright. @@ -38,6 +56,44 @@ public record UpdateBeatRequest( Guid? SceneId = null, IReadOnlyList? Tags = null); +public class UpdateBeatRequestValidator : IModelValidator +{ + public ValidationResult Validate(UpdateBeatRequest model) + { + var result = new ValidationResult(); + + if (model.Title is not null) + { + if (model.Title.Length == 0) + result.AddError("Title", "'Title' can not be cleared — a beat always needs one."); + else if (model.Title.Length > 200) + result.AddError("Title", "'Title' must be 200 characters or fewer."); + } + + BeatValidation.OptionalFields(model.SortOrder, model.WhatHappened, model.WhatsNext, model.Tags, result); + + return result; + } +} + +file static class BeatValidation +{ + public static void OptionalFields(int? sortOrder, string? whatHappened, string? whatsNext, IReadOnlyList? tags, ValidationResult result) + { + if (sortOrder is < 0) + result.AddError("SortOrder", "'Sort Order' must be zero or greater."); + + if (whatHappened is { Length: > 20000 }) + result.AddError("WhatHappened", "'What Happened' must be 20,000 characters or fewer."); + + if (whatsNext is { Length: > 20000 }) + result.AddError("WhatsNext", "'Whats Next' must be 20,000 characters or fewer."); + + if (tags is not null && tags.Any(string.IsNullOrWhiteSpace)) + result.AddError("Tags", "'Tags' must not contain blank entries."); + } +} + /// /// A beat this character appears in, carrying enough of its chapter to link straight to /// the row in that chapter's outline. @@ -57,6 +113,19 @@ public record CharacterBeatDto( /// Reorders a chapter's beats in one call, by listing their ids in the order wanted. public record ReorderBeatsRequest(IReadOnlyList BeatIds); +public class ReorderBeatsRequestValidator : IModelValidator +{ + public ValidationResult Validate(ReorderBeatsRequest model) + { + var result = new ValidationResult(); + + if (model.BeatIds is null || model.BeatIds.Count == 0) + result.AddError("BeatIds", "'Beat Ids' must not be empty."); + + return result; + } +} + public static class BeatMapping { public static BeatDto ToDto(this Beat b) => new( diff --git a/src/Novelly.Api/Beats/BeatEndpoints.cs b/src/Novelly.Api/Beats/BeatEndpoints.cs index 01a9c43..319f620 100644 --- a/src/Novelly.Api/Beats/BeatEndpoints.cs +++ b/src/Novelly.Api/Beats/BeatEndpoints.cs @@ -1,4 +1,5 @@ using Novelly.Api.Common; +using Novelly.Api.Common.Validation; namespace Novelly.Api.Beats; @@ -6,7 +7,9 @@ public static class BeatEndpoints { public static IEndpointRouteBuilder MapBeatEndpoints(this IEndpointRouteBuilder app) { - var chapterScoped = app.MapGroup("/api/chapters/{chapterId:guid}/beats").WithTags("Beats").AddEndpointFilter(); + var chapterScoped = app.MapGroup("/api/chapters/{chapterId:guid}/beats").WithTags("Beats") + .AddEndpointFilter() + .AddEndpointFilter(); chapterScoped.MapGet("/", async (Guid chapterId, BeatService service, CancellationToken ct) => Results.Ok(await service.ListAsync(chapterId, ct))) @@ -27,26 +30,25 @@ public static class BeatEndpoints app.MapGet("/api/characters/{characterId:guid}/beats", async ( Guid characterId, BeatService service, CancellationToken ct) => - Results.Ok(await service.ListForCharacterAsync(characterId, ct))) + (await service.ListForCharacterAsync(characterId, ct)).ToApiResult()) .WithTags("Beats") .WithSummary("Every beat this character appears in, in manuscript order."); - var beats = app.MapGroup("/api/beats").WithTags("Beats").AddEndpointFilter(); + var beats = app.MapGroup("/api/beats").WithTags("Beats") + .AddEndpointFilter() + .AddEndpointFilter(); beats.MapGet("/{id:guid}", async (Guid id, BeatService service, CancellationToken ct) => - Results.Ok(await service.GetAsync(id, ct))) + (await service.GetAsync(id, ct)).ToApiResult()) .WithSummary("Read one beat."); beats.MapPatch("/{id:guid}", async ( Guid id, UpdateBeatRequest request, BeatService service, CancellationToken ct) => - Results.Ok(await service.UpdateAsync(id, request, ct))) + (await service.UpdateAsync(id, request, ct)).ToApiResult()) .WithSummary("Update a beat. Sending a tag list replaces the beat's tags."); beats.MapDelete("/{id:guid}", async (Guid id, BeatService service, CancellationToken ct) => - { - await service.DeleteAsync(id, ct); - return Results.NoContent(); - }) + await service.DeleteAsync(id, ct) ? Results.NoContent() : Results.NotFound()) .WithSummary("Delete a beat."); return app; diff --git a/src/Novelly.Api/Beats/BeatService.cs b/src/Novelly.Api/Beats/BeatService.cs index 4a29dac..6f9a5a8 100644 --- a/src/Novelly.Api/Beats/BeatService.cs +++ b/src/Novelly.Api/Beats/BeatService.cs @@ -2,6 +2,7 @@ using Microsoft.EntityFrameworkCore; using Novelly.Api.Chapters; using Novelly.Api.Characters; using Novelly.Api.Common; +using Novelly.Api.Common.Validation; using Novelly.Api.Data; using Novelly.Api.Tags; @@ -11,10 +12,18 @@ namespace Novelly.Api.Beats; /// Beats are a chapter's outline: a flat, ordered table rather than a tree. Everything /// here is scoped to one chapter. /// -public class BeatService(INovelDbContext db, TagService tags, ILogger logger) +public class BeatService( + INovelDbContext db, + TagService tags, + ILogger logger, + IModelValidator createValidator, + IModelValidator updateValidator, + IModelValidator reorderValidator) { public async Task> ListAsync(Guid chapterId, CancellationToken ct = default) { + Guard.Default(chapterId, nameof(chapterId)); + logger.LogInformation("Listing beats for chapter {ChapterId}", chapterId); var beats = await Query() @@ -25,26 +34,32 @@ public class BeatService(INovelDbContext db, TagService tags, ILogger b.ToDto())]; } - public async Task GetAsync(Guid id, CancellationToken ct = default) + /// Null when no beat has this id — a lookup miss is expected, not exceptional. + public async Task GetAsync(Guid id, CancellationToken ct = default) { + Guard.Default(id, nameof(id)); + logger.LogInformation("Getting beat {BeatId}", id); - return (await FindAsync(id, ct)).ToDto(); + return (await FindAsync(id, ct))?.ToDto(); } /// /// Every beat this character appears in, in manuscript order. This is the character /// page's view onto the outlines: each row carries its chapter so the UI can link - /// straight to the beat in that chapter's outline. + /// straight to the beat in that chapter's outline. Null when no character has this id; + /// an empty list means the character exists but has no beats yet. /// - public async Task> ListForCharacterAsync( + public async Task?> ListForCharacterAsync( Guid characterId, CancellationToken ct = default) { + Guard.Default(characterId, nameof(characterId)); + logger.LogInformation("Listing beats for character {CharacterId}", characterId); if (!await db.Characters.AnyAsync(c => c.Id == characterId, ct)) { - logger.LogWarning("Character {CharacterId} not found", characterId); - throw new NotFoundException(nameof(Character), characterId); + logger.LogInformation("Character {CharacterId} not found", characterId); + return null; } var beats = await db.Beats @@ -74,12 +89,16 @@ public class BeatService(INovelDbContext db, TagService tags, ILogger CreateAsync(Guid chapterId, CreateBeatRequest request, CancellationToken ct = default) { + Guard.Default(chapterId, nameof(chapterId)); + Guard.Null(request, nameof(request)); + createValidator.Validate(request).ThrowIfInvalid(); + logger.LogInformation("Creating beat {Title} for chapter {ChapterId}", request.Title, chapterId); var chapter = await db.Chapters.FirstOrDefaultAsync(c => c.Id == chapterId, ct); if (chapter is null) { - logger.LogWarning("Chapter {ChapterId} not found", chapterId); + logger.LogWarning("Rejected beat creation: chapter {ChapterId} not found", chapterId); throw new NotFoundException(nameof(Chapter), chapterId); } @@ -103,18 +122,31 @@ public class BeatService(INovelDbContext db, TagService tags, ILogger UpdateAsync(Guid id, UpdateBeatRequest request, CancellationToken ct = default) + public async Task UpdateAsync(Guid id, UpdateBeatRequest request, CancellationToken ct = default) { + Guard.Default(id, nameof(id)); + Guard.Null(request, nameof(request)); + updateValidator.Validate(request).ThrowIfInvalid(); + logger.LogInformation("Updating beat {BeatId}", id); var beat = await FindAsync(id, ct); + if (beat is null) + { + return null; + } + var chapter = await db.Chapters.FirstOrDefaultAsync(c => c.Id == beat.ChapterId, ct); if (chapter is null) { - logger.LogWarning("Chapter {ChapterId} not found", beat.ChapterId); + // The beat's own chapter should always exist via the FK — this is an + // invariant failing, not a caller mistake, so it stays exceptional. + logger.LogError("Beat {BeatId} references chapter {ChapterId} which does not exist", id, beat.ChapterId); throw new NotFoundException(nameof(Chapter), beat.ChapterId); } @@ -134,16 +166,25 @@ public class BeatService(INovelDbContext db, TagService tags, ILoggerTrue if a beat was deleted; false if no beat had this id. + public async Task DeleteAsync(Guid id, CancellationToken ct = default) { + Guard.Default(id, nameof(id)); + logger.LogInformation("Deleting beat {BeatId}", id); var beat = await FindAsync(id, ct); + if (beat is null) + { + return false; + } + db.Beats.Remove(beat); await db.SaveChangesAsync(ct); + return true; } /// @@ -153,6 +194,10 @@ public class BeatService(INovelDbContext db, TagService tags, ILogger> ReorderAsync( Guid chapterId, ReorderBeatsRequest request, CancellationToken ct = default) { + Guard.Default(chapterId, nameof(chapterId)); + Guard.Null(request, nameof(request)); + reorderValidator.Validate(request).ThrowIfInvalid(); + logger.LogInformation("Reordering {Count} beats for chapter {ChapterId}", request.BeatIds.Count, chapterId); var beats = await db.Beats.Where(b => b.ChapterId == chapterId).ToListAsync(ct); @@ -229,18 +274,20 @@ public class BeatService(INovelDbContext db, TagService tags, ILogger b.Scene) .Include(b => b.Tags); - private async Task FindAsync(Guid id, CancellationToken ct) + private async Task FindAsync(Guid id, CancellationToken ct) { logger.LogDebug("Finding beat {BeatId}", id); var beat = await Query().FirstOrDefaultAsync(b => b.Id == id, ct); if (beat is null) { - logger.LogWarning("Beat {BeatId} not found", id); - throw new NotFoundException(nameof(Beat), id); + logger.LogInformation("Beat {BeatId} not found", id); + } + else + { + logger.LogDebug("Found beat {BeatId}", id); } - logger.LogDebug("Found beat {BeatId}", id); return beat; } } diff --git a/src/Novelly.Api/Chapters/ChapterDtos.cs b/src/Novelly.Api/Chapters/ChapterDtos.cs index 0f3bf5c..d342ba8 100644 --- a/src/Novelly.Api/Chapters/ChapterDtos.cs +++ b/src/Novelly.Api/Chapters/ChapterDtos.cs @@ -1,5 +1,6 @@ using Novelly.Api.Beats; using Novelly.Api.Common; +using Novelly.Api.Common.Validation; using Novelly.Api.Scenes; using Novelly.Api.Tags; @@ -53,6 +54,23 @@ public record CreateChapterRequest( int? TargetWordCount = null, IReadOnlyList? Tags = null); +public class CreateChapterRequestValidator : IModelValidator +{ + public ValidationResult Validate(CreateChapterRequest model) + { + var result = new ValidationResult(); + + if (string.IsNullOrWhiteSpace(model.Title)) + result.AddError("Title", "'Title' must not be empty."); + else if (model.Title.Length > 200) + result.AddError("Title", "'Title' must be 200 characters or fewer."); + + ChapterValidation.OptionalFields(model.Number, model.Summary, model.Setting, model.Notes, model.TargetWordCount, model.Tags, result); + + return result; + } +} + /// /// Patch-style update. A null field is left alone; an empty string clears it. Passing a /// list replaces the chapter's tags outright. @@ -68,6 +86,51 @@ public record UpdateChapterRequest( int? TargetWordCount = null, IReadOnlyList? Tags = null); +public class UpdateChapterRequestValidator : IModelValidator +{ + public ValidationResult Validate(UpdateChapterRequest model) + { + var result = new ValidationResult(); + + if (model.Title is not null) + { + if (model.Title.Length == 0) + result.AddError("Title", "'Title' can not be cleared — a chapter always needs one."); + else if (model.Title.Length > 200) + result.AddError("Title", "'Title' must be 200 characters or fewer."); + } + + ChapterValidation.OptionalFields(model.Number, model.Summary, model.Setting, model.Notes, model.TargetWordCount, model.Tags, result); + + return result; + } +} + +file static class ChapterValidation +{ + public static void OptionalFields( + int? number, string? summary, string? setting, string? notes, int? targetWordCount, IReadOnlyList? tags, ValidationResult result) + { + if (number is <= 0) + result.AddError("Number", "'Number' must be greater than zero."); + + if (summary is { Length: > 20000 }) + result.AddError("Summary", "'Summary' must be 20,000 characters or fewer."); + + if (setting is { Length: > 500 }) + result.AddError("Setting", "'Setting' must be 500 characters or fewer."); + + if (notes is { Length: > 20000 }) + result.AddError("Notes", "'Notes' must be 20,000 characters or fewer."); + + if (targetWordCount is < 0) + result.AddError("TargetWordCount", "'Target Word Count' must be zero or greater."); + + if (tags is not null && tags.Any(string.IsNullOrWhiteSpace)) + result.AddError("Tags", "'Tags' must not contain blank entries."); + } +} + public static class ChapterMapping { public static ChapterDto ToDto(this Chapter c) => new( diff --git a/src/Novelly.Api/Chapters/ChapterEndpoints.cs b/src/Novelly.Api/Chapters/ChapterEndpoints.cs index f6f1d10..a880d69 100644 --- a/src/Novelly.Api/Chapters/ChapterEndpoints.cs +++ b/src/Novelly.Api/Chapters/ChapterEndpoints.cs @@ -1,4 +1,5 @@ using Novelly.Api.Common; +using Novelly.Api.Common.Validation; namespace Novelly.Api.Chapters; @@ -6,7 +7,9 @@ public static class ChapterEndpoints { public static IEndpointRouteBuilder MapChapterEndpoints(this IEndpointRouteBuilder app) { - var projectScoped = app.MapGroup("/api/projects/{projectId:guid}/chapters").WithTags("Chapters").AddEndpointFilter(); + var projectScoped = app.MapGroup("/api/projects/{projectId:guid}/chapters").WithTags("Chapters") + .AddEndpointFilter() + .AddEndpointFilter(); projectScoped.MapGet("/", async (Guid projectId, ChapterService service, CancellationToken ct) => Results.Ok(await service.ListAsync(projectId, ct))) @@ -20,22 +23,21 @@ public static class ChapterEndpoints }) .WithSummary("Add a chapter."); - var chapters = app.MapGroup("/api/chapters").WithTags("Chapters").AddEndpointFilter(); + var chapters = app.MapGroup("/api/chapters").WithTags("Chapters") + .AddEndpointFilter() + .AddEndpointFilter(); chapters.MapGet("/{id:guid}", async (Guid id, ChapterService service, CancellationToken ct) => - Results.Ok(await service.GetAsync(id, ct))) + (await service.GetAsync(id, ct)).ToApiResult()) .WithSummary("Read a chapter with all of its scenes."); chapters.MapPatch("/{id:guid}", async ( Guid id, UpdateChapterRequest request, ChapterService service, CancellationToken ct) => - Results.Ok(await service.UpdateAsync(id, request, ct))) + (await service.UpdateAsync(id, request, ct)).ToApiResult()) .WithSummary("Update a chapter."); chapters.MapDelete("/{id:guid}", async (Guid id, ChapterService service, CancellationToken ct) => - { - await service.DeleteAsync(id, ct); - return Results.NoContent(); - }) + await service.DeleteAsync(id, ct) ? Results.NoContent() : Results.NotFound()) .WithSummary("Delete a chapter and its scenes."); return app; diff --git a/src/Novelly.Api/Chapters/ChapterService.cs b/src/Novelly.Api/Chapters/ChapterService.cs index 156be29..34f79e9 100644 --- a/src/Novelly.Api/Chapters/ChapterService.cs +++ b/src/Novelly.Api/Chapters/ChapterService.cs @@ -1,15 +1,23 @@ using Microsoft.EntityFrameworkCore; using Novelly.Api.Common; +using Novelly.Api.Common.Validation; using Novelly.Api.Data; using Novelly.Api.Projects; using Novelly.Api.Tags; namespace Novelly.Api.Chapters; -public class ChapterService(INovelDbContext db, TagService tags, ILogger logger) +public class ChapterService( + INovelDbContext db, + TagService tags, + ILogger logger, + IModelValidator createValidator, + IModelValidator updateValidator) { public async Task> ListAsync(Guid projectId, CancellationToken ct = default) { + Guard.Default(projectId, nameof(projectId)); + logger.LogInformation("Listing chapters for project {ProjectId}", projectId); var chapters = await db.Chapters @@ -24,19 +32,26 @@ public class ChapterService(INovelDbContext db, TagService tags, ILogger c.ToSummaryDto())]; } - public async Task GetAsync(Guid id, CancellationToken ct = default) + /// Null when no chapter has this id — a lookup miss is expected, not exceptional. + public async Task GetAsync(Guid id, CancellationToken ct = default) { + Guard.Default(id, nameof(id)); + logger.LogInformation("Getting chapter {ChapterId}", id); - return (await FindAsync(id, ct)).ToDto(); + return (await FindAsync(id, ct))?.ToDto(); } public async Task CreateAsync(Guid projectId, CreateChapterRequest request, CancellationToken ct = default) { + Guard.Default(projectId, nameof(projectId)); + Guard.Null(request, nameof(request)); + createValidator.Validate(request).ThrowIfInvalid(); + logger.LogInformation("Creating chapter {Title} for project {ProjectId}", request.Title, projectId); if (!await db.Projects.AnyAsync(p => p.Id == projectId, ct)) { - logger.LogWarning("Project {ProjectId} not found", projectId); + logger.LogWarning("Rejected chapter creation: project {ProjectId} not found", projectId); throw new NotFoundException(nameof(Project), projectId); } @@ -60,14 +75,24 @@ public class ChapterService(INovelDbContext db, TagService tags, ILogger UpdateAsync(Guid id, UpdateChapterRequest request, CancellationToken ct = default) + public async Task UpdateAsync(Guid id, UpdateChapterRequest request, CancellationToken ct = default) { + Guard.Default(id, nameof(id)); + Guard.Null(request, nameof(request)); + updateValidator.Validate(request).ThrowIfInvalid(); + logger.LogInformation("Updating chapter {ChapterId}", id); var chapter = await FindAsync(id, ct); + if (chapter is null) + { + return null; + } chapter.Title = Patch.Apply(chapter.Title, request.Title) ?? chapter.Title; chapter.Number = request.Number ?? chapter.Number; @@ -85,16 +110,25 @@ public class ChapterService(INovelDbContext db, TagService tags, ILoggerTrue if a chapter was deleted; false if no chapter had this id. + public async Task DeleteAsync(Guid id, CancellationToken ct = default) { + Guard.Default(id, nameof(id)); + logger.LogInformation("Deleting chapter {ChapterId}", id); var chapter = await FindAsync(id, ct); + if (chapter is null) + { + return false; + } + db.Chapters.Remove(chapter); await db.SaveChangesAsync(ct); + return true; } private async Task NextChapterNumberAsync(Guid projectId, CancellationToken ct) @@ -110,7 +144,7 @@ public class ChapterService(INovelDbContext db, TagService tags, ILogger FindAsync(Guid id, CancellationToken ct) + private async Task FindAsync(Guid id, CancellationToken ct) { logger.LogDebug("Finding chapter {ChapterId}", id); @@ -125,11 +159,13 @@ public class ChapterService(INovelDbContext db, TagService tags, ILogger -public class CharacterArcService(INovelDbContext db, ILogger logger) +public class CharacterArcService( + INovelDbContext db, + ILogger logger, + IModelValidator createValidator, + IModelValidator updateValidator, + IModelValidator reorderValidator) { public async Task> ListAsync(Guid characterId, CancellationToken ct = default) { + Guard.Default(characterId, nameof(characterId)); + logger.LogInformation("Listing arc stages for character {CharacterId}", characterId); var stages = await Query() @@ -27,21 +35,28 @@ public class CharacterArcService(INovelDbContext db, ILogger s.ToDto())]; } - public async Task GetAsync(Guid id, CancellationToken ct = default) + /// Null when no arc stage has this id — a lookup miss is expected, not exceptional. + public async Task GetAsync(Guid id, CancellationToken ct = default) { + Guard.Default(id, nameof(id)); + logger.LogInformation("Getting arc stage {ArcStageId}", id); - return (await FindAsync(id, ct)).ToDto(); + return (await FindAsync(id, ct))?.ToDto(); } public async Task CreateAsync( Guid characterId, CreateArcStageRequest request, CancellationToken ct = default) { + Guard.Default(characterId, nameof(characterId)); + Guard.Null(request, nameof(request)); + createValidator.Validate(request).ThrowIfInvalid(); + logger.LogInformation("Creating arc stage {Title} for character {CharacterId}", request.Title, characterId); var character = await db.Characters.FirstOrDefaultAsync(c => c.Id == characterId, ct); if (character is null) { - logger.LogWarning("Character {CharacterId} not found", characterId); + logger.LogWarning("Rejected arc stage creation: character {CharacterId} not found", characterId); throw new NotFoundException(nameof(Character), characterId); } @@ -58,20 +73,32 @@ public class CharacterArcService(INovelDbContext db, ILogger UpdateAsync( + public async Task UpdateAsync( Guid id, UpdateArcStageRequest request, CancellationToken ct = default) { + Guard.Default(id, nameof(id)); + Guard.Null(request, nameof(request)); + updateValidator.Validate(request).ThrowIfInvalid(); + logger.LogInformation("Updating arc stage {ArcStageId}", id); var stage = await FindAsync(id, ct); + if (stage is null) + { + return null; + } var character = await db.Characters.FirstOrDefaultAsync(c => c.Id == stage.CharacterId, ct); if (character is null) { - logger.LogWarning("Character {CharacterId} not found", stage.CharacterId); + // The stage's own character should always exist via the FK — this is an + // invariant failing, not a caller mistake, so it stays exceptional. + logger.LogError("Arc stage {ArcStageId} references character {CharacterId} which does not exist", id, stage.CharacterId); throw new NotFoundException(nameof(Character), stage.CharacterId); } @@ -84,16 +111,25 @@ public class CharacterArcService(INovelDbContext db, ILoggerTrue if an arc stage was deleted; false if no stage had this id. + public async Task DeleteAsync(Guid id, CancellationToken ct = default) { + Guard.Default(id, nameof(id)); + logger.LogInformation("Deleting arc stage {ArcStageId}", id); var stage = await FindAsync(id, ct); + if (stage is null) + { + return false; + } + db.CharacterArcStages.Remove(stage); await db.SaveChangesAsync(ct); + return true; } /// @@ -103,6 +139,10 @@ public class CharacterArcService(INovelDbContext db, ILogger> ReorderAsync( Guid characterId, ReorderArcStagesRequest request, CancellationToken ct = default) { + Guard.Default(characterId, nameof(characterId)); + Guard.Null(request, nameof(request)); + reorderValidator.Validate(request).ThrowIfInvalid(); + logger.LogInformation("Reordering {Count} arc stages for character {CharacterId}", request.StageIds.Count, characterId); var stages = await db.CharacterArcStages @@ -164,18 +204,20 @@ public class CharacterArcService(INovelDbContext db, ILogger Query() => db.CharacterArcStages.Include(s => s.Chapter); - private async Task FindAsync(Guid id, CancellationToken ct) + private async Task FindAsync(Guid id, CancellationToken ct) { logger.LogDebug("Finding arc stage {ArcStageId}", id); var stage = await Query().FirstOrDefaultAsync(s => s.Id == id, ct); if (stage is null) { - logger.LogWarning("CharacterArcStage {ArcStageId} not found", id); - throw new NotFoundException(nameof(CharacterArcStage), id); + logger.LogInformation("CharacterArcStage {ArcStageId} not found", id); + } + else + { + logger.LogDebug("Found arc stage {ArcStageId}", id); } - logger.LogDebug("Found arc stage {ArcStageId}", id); return stage; } } diff --git a/src/Novelly.Api/Characters/CharacterDtos.cs b/src/Novelly.Api/Characters/CharacterDtos.cs index 8c78264..96a70f7 100644 --- a/src/Novelly.Api/Characters/CharacterDtos.cs +++ b/src/Novelly.Api/Characters/CharacterDtos.cs @@ -1,3 +1,4 @@ +using Novelly.Api.Common.Validation; using Novelly.Api.Tags; namespace Novelly.Api.Characters; @@ -52,6 +53,26 @@ public record CreateCharacterRequest( string? Notes = null, IReadOnlyList? Tags = null); +public class CreateCharacterRequestValidator : IModelValidator +{ + public ValidationResult Validate(CreateCharacterRequest model) + { + var result = new ValidationResult(); + + if (string.IsNullOrWhiteSpace(model.Name)) + result.AddError("Name", "'Name' must not be empty."); + else if (model.Name.Length > 200) + result.AddError("Name", "'Name' must be 200 characters or fewer."); + + CharacterValidation.OptionalFields( + model.Age, model.Pronouns, model.Occupation, model.Appearance, model.Personality, model.Backstory, + model.Want, model.Need, model.InternalConflict, model.ExternalConflict, model.ArcSummary, model.Voice, + model.Notes, model.Tags, result); + + return result; + } +} + /// /// Patch-style update. A null field is left alone; an empty string clears it. Passing a /// list replaces the character's tags outright. @@ -75,11 +96,87 @@ public record UpdateCharacterRequest( string? Notes = null, IReadOnlyList? Tags = null); +public class UpdateCharacterRequestValidator : IModelValidator +{ + public ValidationResult Validate(UpdateCharacterRequest model) + { + var result = new ValidationResult(); + + if (model.Name is not null) + { + if (model.Name.Length == 0) + result.AddError("Name", "'Name' can not be cleared — a character always needs one."); + else if (model.Name.Length > 200) + result.AddError("Name", "'Name' must be 200 characters or fewer."); + } + + CharacterValidation.OptionalFields( + model.Age, model.Pronouns, model.Occupation, model.Appearance, model.Personality, model.Backstory, + model.Want, model.Need, model.InternalConflict, model.ExternalConflict, model.ArcSummary, model.Voice, + model.Notes, model.Tags, result); + + return result; + } +} + +file static class CharacterValidation +{ + public static void OptionalFields( + string? age, string? pronouns, string? occupation, string? appearance, string? personality, string? backstory, + string? want, string? need, string? internalConflict, string? externalConflict, string? arcSummary, string? voice, + string? notes, IReadOnlyList? tags, ValidationResult result) + { + Cap(age, "Age", 100, result); + Cap(pronouns, "Pronouns", 100, result); + Cap(occupation, "Occupation", 200, result); + Cap(appearance, "Appearance", 20000, result); + Cap(personality, "Personality", 20000, result); + Cap(backstory, "Backstory", 20000, result); + Cap(want, "Want", 2000, result); + Cap(need, "Need", 2000, result); + Cap(internalConflict, "InternalConflict", 2000, result); + Cap(externalConflict, "ExternalConflict", 2000, result); + Cap(arcSummary, "ArcSummary", 20000, result); + Cap(voice, "Voice", 2000, result); + Cap(notes, "Notes", 20000, result); + + if (tags is not null && tags.Any(string.IsNullOrWhiteSpace)) + result.AddError("Tags", "'Tags' must not contain blank entries."); + } + + private static void Cap(string? value, string field, int max, ValidationResult result) + { + if (value is { Length: var length } && length > max) + result.AddError(field, $"'{field}' must be {max:N0} characters or fewer."); + } +} + public record CreateRelationshipRequest( Guid RelatedCharacterId, string RelationshipType, string? Description = null); +public class CreateRelationshipRequestValidator : IModelValidator +{ + public ValidationResult Validate(CreateRelationshipRequest model) + { + var result = new ValidationResult(); + + if (model.RelatedCharacterId == Guid.Empty) + result.AddError("RelatedCharacterId", "'Related Character Id' must not be empty."); + + if (string.IsNullOrWhiteSpace(model.RelationshipType)) + result.AddError("RelationshipType", "'Relationship Type' must not be empty."); + else if (model.RelationshipType.Length > 100) + result.AddError("RelationshipType", "'Relationship Type' must be 100 characters or fewer."); + + if (model.Description is { Length: > 2000 }) + result.AddError("Description", "'Description' must be 2,000 characters or fewer."); + + return result; + } +} + public record ArcStageDto( Guid Id, Guid CharacterId, @@ -97,6 +194,23 @@ public record CreateArcStageRequest( string? Description = null, Guid? ChapterId = null); +public class CreateArcStageRequestValidator : IModelValidator +{ + public ValidationResult Validate(CreateArcStageRequest model) + { + var result = new ValidationResult(); + + if (string.IsNullOrWhiteSpace(model.Title)) + result.AddError("Title", "'Title' must not be empty."); + else if (model.Title.Length > 200) + result.AddError("Title", "'Title' must be 200 characters or fewer."); + + ArcStageValidation.OptionalFields(model.SortOrder, model.Description, result); + + return result; + } +} + /// Patch-style update. A null field is left alone; an empty string clears it. public record UpdateArcStageRequest( string? Title = null, @@ -104,9 +218,54 @@ public record UpdateArcStageRequest( string? Description = null, Guid? ChapterId = null); +public class UpdateArcStageRequestValidator : IModelValidator +{ + public ValidationResult Validate(UpdateArcStageRequest model) + { + var result = new ValidationResult(); + + if (model.Title is not null) + { + if (model.Title.Length == 0) + result.AddError("Title", "'Title' can not be cleared — an arc stage always needs one."); + else if (model.Title.Length > 200) + result.AddError("Title", "'Title' must be 200 characters or fewer."); + } + + ArcStageValidation.OptionalFields(model.SortOrder, model.Description, result); + + return result; + } +} + +file static class ArcStageValidation +{ + public static void OptionalFields(int? sortOrder, string? description, ValidationResult result) + { + if (sortOrder is < 0) + result.AddError("SortOrder", "'Sort Order' must be zero or greater."); + + if (description is { Length: > 20000 }) + result.AddError("Description", "'Description' must be 20,000 characters or fewer."); + } +} + /// Reorders a character's arc in one call, by listing the stage ids in the order wanted. public record ReorderArcStagesRequest(IReadOnlyList StageIds); +public class ReorderArcStagesRequestValidator : IModelValidator +{ + public ValidationResult Validate(ReorderArcStagesRequest model) + { + var result = new ValidationResult(); + + if (model.StageIds is null || model.StageIds.Count == 0) + result.AddError("StageIds", "'Stage Ids' must not be empty."); + + return result; + } +} + public static class CharacterMapping { diff --git a/src/Novelly.Api/Characters/CharacterEndpoints.cs b/src/Novelly.Api/Characters/CharacterEndpoints.cs index 6f1564e..d214696 100644 --- a/src/Novelly.Api/Characters/CharacterEndpoints.cs +++ b/src/Novelly.Api/Characters/CharacterEndpoints.cs @@ -1,4 +1,5 @@ using Novelly.Api.Common; +using Novelly.Api.Common.Validation; namespace Novelly.Api.Characters; @@ -6,7 +7,9 @@ public static class CharacterEndpoints { public static IEndpointRouteBuilder MapCharacterEndpoints(this IEndpointRouteBuilder app) { - var projectScoped = app.MapGroup("/api/projects/{projectId:guid}/characters").WithTags("Characters").AddEndpointFilter(); + var projectScoped = app.MapGroup("/api/projects/{projectId:guid}/characters").WithTags("Characters") + .AddEndpointFilter() + .AddEndpointFilter(); projectScoped.MapGet("/", async (Guid projectId, CharacterService service, CancellationToken ct) => Results.Ok(await service.ListAsync(projectId, ct))) @@ -20,35 +23,31 @@ public static class CharacterEndpoints }) .WithSummary("Add a character dossier."); - var characters = app.MapGroup("/api/characters").WithTags("Characters").AddEndpointFilter(); + var characters = app.MapGroup("/api/characters").WithTags("Characters") + .AddEndpointFilter() + .AddEndpointFilter(); characters.MapGet("/{id:guid}", async (Guid id, CharacterService service, CancellationToken ct) => - Results.Ok(await service.GetAsync(id, ct))) + (await service.GetAsync(id, ct)).ToApiResult()) .WithSummary("Read a character dossier."); characters.MapPatch("/{id:guid}", async ( Guid id, UpdateCharacterRequest request, CharacterService service, CancellationToken ct) => - Results.Ok(await service.UpdateAsync(id, request, ct))) + (await service.UpdateAsync(id, request, ct)).ToApiResult()) .WithSummary("Update a character dossier."); characters.MapDelete("/{id:guid}", async (Guid id, CharacterService service, CancellationToken ct) => - { - await service.DeleteAsync(id, ct); - return Results.NoContent(); - }) + await service.DeleteAsync(id, ct) ? Results.NoContent() : Results.NotFound()) .WithSummary("Delete a character."); characters.MapPost("/{id:guid}/relationships", async ( Guid id, CreateRelationshipRequest request, CharacterService service, CancellationToken ct) => - Results.Ok(await service.AddRelationshipAsync(id, request, ct))) + (await service.AddRelationshipAsync(id, request, ct)).ToApiResult()) .WithSummary("Relate this character to another in the same project."); characters.MapDelete("/relationships/{relationshipId:guid}", async ( Guid relationshipId, CharacterService service, CancellationToken ct) => - { - await service.RemoveRelationshipAsync(relationshipId, ct); - return Results.NoContent(); - }) + await service.RemoveRelationshipAsync(relationshipId, ct) ? Results.NoContent() : Results.NotFound()) .WithSummary("Remove a relationship."); characters.MapGet("/{id:guid}/arc", async ( @@ -69,22 +68,21 @@ public static class CharacterEndpoints Results.Ok(await service.ReorderAsync(id, request, ct))) .WithSummary("Renumber a character's arc to match the order given."); - var arcStages = app.MapGroup("/api/arc-stages").WithTags("Characters").AddEndpointFilter(); + var arcStages = app.MapGroup("/api/arc-stages").WithTags("Characters") + .AddEndpointFilter() + .AddEndpointFilter(); arcStages.MapGet("/{id:guid}", async (Guid id, CharacterArcService service, CancellationToken ct) => - Results.Ok(await service.GetAsync(id, ct))) + (await service.GetAsync(id, ct)).ToApiResult()) .WithSummary("Read one arc stage."); arcStages.MapPatch("/{id:guid}", async ( Guid id, UpdateArcStageRequest request, CharacterArcService service, CancellationToken ct) => - Results.Ok(await service.UpdateAsync(id, request, ct))) + (await service.UpdateAsync(id, request, ct)).ToApiResult()) .WithSummary("Update an arc stage."); arcStages.MapDelete("/{id:guid}", async (Guid id, CharacterArcService service, CancellationToken ct) => - { - await service.DeleteAsync(id, ct); - return Results.NoContent(); - }) + await service.DeleteAsync(id, ct) ? Results.NoContent() : Results.NotFound()) .WithSummary("Delete an arc stage."); return app; diff --git a/src/Novelly.Api/Characters/CharacterService.cs b/src/Novelly.Api/Characters/CharacterService.cs index 3926d2b..8f65979 100644 --- a/src/Novelly.Api/Characters/CharacterService.cs +++ b/src/Novelly.Api/Characters/CharacterService.cs @@ -1,12 +1,19 @@ using Microsoft.EntityFrameworkCore; using Novelly.Api.Common; +using Novelly.Api.Common.Validation; using Novelly.Api.Data; using Novelly.Api.Projects; using Novelly.Api.Tags; namespace Novelly.Api.Characters; -public class CharacterService(INovelDbContext db, TagService tags, ILogger logger) +public class CharacterService( + INovelDbContext db, + TagService tags, + ILogger logger, + IModelValidator createValidator, + IModelValidator updateValidator, + IModelValidator relationshipValidator) { /// /// Main characters first, then by the part they play, then by name. @@ -20,6 +27,8 @@ public class CharacterService(INovelDbContext db, TagService tags, ILogger public async Task> ListAsync(Guid projectId, CancellationToken ct = default) { + Guard.Default(projectId, nameof(projectId)); + logger.LogInformation("Listing characters for project {ProjectId}", projectId); var characters = await Query() @@ -36,14 +45,21 @@ public class CharacterService(INovelDbContext db, TagService tags, ILogger GetAsync(Guid id, CancellationToken ct = default) + /// Null when no character has this id — a lookup miss is expected, not exceptional. + public async Task GetAsync(Guid id, CancellationToken ct = default) { + Guard.Default(id, nameof(id)); + logger.LogInformation("Getting character {CharacterId}", id); - return (await FindAsync(id, ct)).ToDto(); + return (await FindAsync(id, ct))?.ToDto(); } public async Task CreateAsync(Guid projectId, CreateCharacterRequest request, CancellationToken ct = default) { + Guard.Default(projectId, nameof(projectId)); + Guard.Null(request, nameof(request)); + createValidator.Validate(request).ThrowIfInvalid(); + logger.LogInformation("Creating character {Name} for project {ProjectId}, role {Role}, importance {Importance}", request.Name, projectId, request.Role, request.Importance); await EnsureProjectExists(projectId, ct); @@ -76,14 +92,24 @@ public class CharacterService(INovelDbContext db, TagService tags, ILogger UpdateAsync(Guid id, UpdateCharacterRequest request, CancellationToken ct = default) + public async Task UpdateAsync(Guid id, UpdateCharacterRequest request, CancellationToken ct = default) { + Guard.Default(id, nameof(id)); + Guard.Null(request, nameof(request)); + updateValidator.Validate(request).ThrowIfInvalid(); + logger.LogInformation("Updating character {CharacterId}", id); var character = await FindAsync(id, ct); + if (character is null) + { + return null; + } character.Name = Patch.Apply(character.Name, request.Name) ?? character.Name; character.Role = request.Role ?? character.Role; @@ -109,29 +135,47 @@ public class CharacterService(INovelDbContext db, TagService tags, ILoggerTrue if a character was deleted; false if no character had this id. + public async Task DeleteAsync(Guid id, CancellationToken ct = default) { + Guard.Default(id, nameof(id)); + logger.LogInformation("Deleting character {CharacterId}", id); var character = await FindAsync(id, ct); + if (character is null) + { + return false; + } + db.Characters.Remove(character); await db.SaveChangesAsync(ct); + return true; } - public async Task AddRelationshipAsync( + /// Null when the subject character () doesn't exist. + public async Task AddRelationshipAsync( Guid characterId, CreateRelationshipRequest request, CancellationToken ct = default) { + Guard.Default(characterId, nameof(characterId)); + Guard.Null(request, nameof(request)); + relationshipValidator.Validate(request).ThrowIfInvalid(); + logger.LogInformation("Adding relationship {RelationshipType} from character {CharacterId} to {RelatedCharacterId}", request.RelationshipType, characterId, request.RelatedCharacterId); var character = await FindAsync(characterId, ct); + if (character is null) + { + return null; + } var related = await db.Characters.FirstOrDefaultAsync(c => c.Id == request.RelatedCharacterId, ct); if (related is null) { - logger.LogWarning("Character {RelatedCharacterId} not found", request.RelatedCharacterId); + logger.LogWarning("Rejected relationship: related character {RelatedCharacterId} not found", request.RelatedCharacterId); throw new NotFoundException(nameof(Character), request.RelatedCharacterId); } @@ -150,22 +194,26 @@ public class CharacterService(INovelDbContext db, TagService tags, ILoggerTrue if a relationship was removed; false if no relationship had this id. + public async Task RemoveRelationshipAsync(Guid relationshipId, CancellationToken ct = default) { + Guard.Default(relationshipId, nameof(relationshipId)); + logger.LogInformation("Removing relationship {RelationshipId}", relationshipId); var relationship = await db.CharacterRelationships.FirstOrDefaultAsync(r => r.Id == relationshipId, ct); if (relationship is null) { - logger.LogWarning("CharacterRelationship {RelationshipId} not found", relationshipId); - throw new NotFoundException(nameof(CharacterRelationship), relationshipId); + logger.LogInformation("CharacterRelationship {RelationshipId} not found", relationshipId); + return false; } db.CharacterRelationships.Remove(relationship); await db.SaveChangesAsync(ct); + return true; } private IQueryable Query() => @@ -176,18 +224,20 @@ public class CharacterService(INovelDbContext db, TagService tags, ILogger c.ArcStages) .ThenInclude(s => s.Chapter); - private async Task FindAsync(Guid id, CancellationToken ct) + private async Task FindAsync(Guid id, CancellationToken ct) { logger.LogDebug("Finding character {CharacterId}", id); var character = await Query().FirstOrDefaultAsync(c => c.Id == id, ct); if (character is null) { - logger.LogWarning("Character {CharacterId} not found", id); - throw new NotFoundException(nameof(Character), id); + logger.LogInformation("Character {CharacterId} not found", id); + } + else + { + logger.LogDebug("Found character {CharacterId}", id); } - logger.LogDebug("Found character {CharacterId}", id); return character; } @@ -197,7 +247,7 @@ public class CharacterService(INovelDbContext db, TagService tags, ILogger p.Id == projectId, ct)) { - logger.LogWarning("Project {ProjectId} not found", projectId); + logger.LogWarning("Rejected character creation: project {ProjectId} not found", projectId); throw new NotFoundException(nameof(Project), projectId); } } diff --git a/src/Novelly.Api/Common/ApiResultExtensions.cs b/src/Novelly.Api/Common/ApiResultExtensions.cs new file mode 100644 index 0000000..2c58fdb --- /dev/null +++ b/src/Novelly.Api/Common/ApiResultExtensions.cs @@ -0,0 +1,12 @@ +namespace Novelly.Api.Common; + +public static class ApiResultExtensions +{ + /// + /// A missing entity is not exceptional, so lookups return null instead of throwing. + /// This is where that null finally becomes an HTTP 404 — the one place the API layer + /// needs to know about it. + /// + public static IResult ToApiResult(this T? value) where T : class => + value is null ? Results.NotFound() : Results.Ok(value); +} diff --git a/src/Novelly.Api/Common/Guard.cs b/src/Novelly.Api/Common/Guard.cs new file mode 100644 index 0000000..00e1d41 --- /dev/null +++ b/src/Novelly.Api/Common/Guard.cs @@ -0,0 +1,40 @@ +namespace Novelly.Api.Common; + +public static class Guard +{ + public static void Null(T t, string parameterName) where T : class + { + if (t is null) + throw new ArgumentNullException(parameterName, $"{nameof(parameterName)} can not be null"); + } + + public static void Empty(string value, string parameterName) + { + if (string.IsNullOrWhiteSpace(value)) + throw new ArgumentException($"{parameterName} can not be empty", parameterName); + } + + public static void Empty(IEnumerable collection, string parameterName) + { + if (collection == null || !collection.Any()) + throw new ArgumentException($"{parameterName} can not be empty", parameterName); + } + + public static void Negative(int value, string parameterName) + { + if (value < 0) + throw new ArgumentOutOfRangeException(parameterName, $"{parameterName} must be a positive number or zero"); + } + + public static void NegativeOrZero(int value, string parameterName) + { + if (value <= 0) + throw new ArgumentOutOfRangeException(parameterName, $"{nameof(parameterName)} must be a positive number greater then zero"); + } + + public static void Default(T value, string parameterName) + { + if (EqualityComparer.Default.Equals(value, default)) + throw new ArgumentException($"{parameterName} can not be a default value", parameterName); + } +} diff --git a/src/Novelly.Api/Common/NovellyServiceRegistration.cs b/src/Novelly.Api/Common/NovellyServiceRegistration.cs index 42109db..a1884cc 100644 --- a/src/Novelly.Api/Common/NovellyServiceRegistration.cs +++ b/src/Novelly.Api/Common/NovellyServiceRegistration.cs @@ -3,6 +3,7 @@ using Novelly.Api.Agent; using Novelly.Api.Beats; using Novelly.Api.Chapters; using Novelly.Api.Characters; +using Novelly.Api.Common.Validation; using Novelly.Api.Data; using Novelly.Api.Projects; using Novelly.Api.Questions; @@ -40,6 +41,8 @@ public static class NovellyServiceRegistration services.Configure(configuration.GetSection(AgentOptions.SectionName)); services.AddScoped(); + services.AddModelValidatorsFromAssemblyContaining(); + return services; } } diff --git a/src/Novelly.Api/Common/Validation/IModelValidator.cs b/src/Novelly.Api/Common/Validation/IModelValidator.cs new file mode 100644 index 0000000..19055a5 --- /dev/null +++ b/src/Novelly.Api/Common/Validation/IModelValidator.cs @@ -0,0 +1,13 @@ +namespace Novelly.Api.Common.Validation; + +public interface IModelValidator +{ + ValidationResult Validate(object model); +} + +public interface IModelValidator : IModelValidator +{ + ValidationResult Validate(T model); + + ValidationResult IModelValidator.Validate(object model) => Validate((T)model); +} diff --git a/src/Novelly.Api/Common/Validation/ModelValidatorServiceCollectionExtensions.cs b/src/Novelly.Api/Common/Validation/ModelValidatorServiceCollectionExtensions.cs new file mode 100644 index 0000000..4f6d013 --- /dev/null +++ b/src/Novelly.Api/Common/Validation/ModelValidatorServiceCollectionExtensions.cs @@ -0,0 +1,18 @@ +namespace Novelly.Api.Common.Validation; + +public static class ModelValidatorServiceCollectionExtensions +{ + public static IServiceCollection AddModelValidatorsFromAssemblyContaining(this IServiceCollection services) + { + var registrations = typeof(TMarker).Assembly.GetTypes() + .Where(type => !type.IsAbstract && !type.IsInterface) + .SelectMany(type => type.GetInterfaces() + .Where(i => i.IsGenericType && i.GetGenericTypeDefinition() == typeof(IModelValidator<>)) + .Select(i => (Interface: i, Implementation: type))); + + foreach (var (@interface, implementation) in registrations) + services.AddScoped(@interface, implementation); + + return services; + } +} diff --git a/src/Novelly.Api/Common/Validation/ValidationEndpointFilter.cs b/src/Novelly.Api/Common/Validation/ValidationEndpointFilter.cs new file mode 100644 index 0000000..e57fcc6 --- /dev/null +++ b/src/Novelly.Api/Common/Validation/ValidationEndpointFilter.cs @@ -0,0 +1,38 @@ +namespace Novelly.Api.Common.Validation; + +/// +/// Minimal-API equivalent of mic-check's MVC ModelValidationActionFilter. Runs every +/// endpoint argument that has a registered through it and, +/// if any fail, short-circuits with a 400 naming every field and message a caller can act on. +/// +public class ValidationEndpointFilter : IEndpointFilter +{ + public async ValueTask InvokeAsync(EndpointFilterInvocationContext context, EndpointFilterDelegate next) + { + var errors = new Dictionary(); + + foreach (var argument in context.Arguments) + { + if (argument is null) continue; + + var validatorType = typeof(IModelValidator<>).MakeGenericType(argument.GetType()); + if (context.HttpContext.RequestServices.GetService(validatorType) is not IModelValidator validator) continue; + + var result = validator.Validate(argument); + if (result.IsInvalid) + { + foreach (var group in result.Errors.GroupBy(e => e.PropertyName)) + { + errors[group.Key] = [.. group.Select(e => e.Message)]; + } + } + } + + if (errors.Count > 0) + { + return Results.ValidationProblem(errors); + } + + return await next(context); + } +} diff --git a/src/Novelly.Api/Common/Validation/ValidationResult.cs b/src/Novelly.Api/Common/Validation/ValidationResult.cs new file mode 100644 index 0000000..4419693 --- /dev/null +++ b/src/Novelly.Api/Common/Validation/ValidationResult.cs @@ -0,0 +1,14 @@ +namespace Novelly.Api.Common.Validation; + +public record ValidationError(string PropertyName, string Message); + +public class ValidationResult +{ + private readonly List _errors = []; + + public IReadOnlyList Errors => _errors; + public bool IsValid => _errors.Count == 0; + public bool IsInvalid => _errors.Count > 0; + + public void AddError(string propertyName, string message) => _errors.Add(new ValidationError(propertyName, message)); +} diff --git a/src/Novelly.Api/Common/Validation/ValidationResultExtensions.cs b/src/Novelly.Api/Common/Validation/ValidationResultExtensions.cs new file mode 100644 index 0000000..99f0be7 --- /dev/null +++ b/src/Novelly.Api/Common/Validation/ValidationResultExtensions.cs @@ -0,0 +1,17 @@ +namespace Novelly.Api.Common.Validation; + +public static class ValidationResultExtensions +{ + /// + /// The service-level half of "validate again and throw if invalid": callers that reach + /// a service directly (agent tools, MCP, tests) skip the API's , + /// so services re-run the same validator and throw rather than act on bad data. + /// + public static void ThrowIfInvalid(this ValidationResult result) + { + if (result.IsInvalid) + { + throw new ArgumentException(string.Join("; ", result.Errors.Select(e => $"{e.PropertyName}: {e.Message}"))); + } + } +} diff --git a/src/Novelly.Api/Projects/ProjectDtos.cs b/src/Novelly.Api/Projects/ProjectDtos.cs index abd5bde..335bfab 100644 --- a/src/Novelly.Api/Projects/ProjectDtos.cs +++ b/src/Novelly.Api/Projects/ProjectDtos.cs @@ -1,3 +1,5 @@ +using Novelly.Api.Common.Validation; + namespace Novelly.Api.Projects; public record ProjectSummaryDto( @@ -33,6 +35,19 @@ public record CreateProjectRequest( string? Notes = null, int? TargetWordCount = null); +public class CreateProjectRequestValidator : IModelValidator +{ + public ValidationResult Validate(CreateProjectRequest model) + { + var result = new ValidationResult(); + + ProjectValidation.Title(model.Title, result); + ProjectValidation.OptionalFields(model.Author, model.Genre, model.Logline, model.Synopsis, model.Notes, model.TargetWordCount, result); + + return result; + } +} + /// /// Patch-style update: every field is optional and null means "leave alone". /// Clearing a field is done by sending an empty string. @@ -46,6 +61,59 @@ public record UpdateProjectRequest( string? Notes = null, int? TargetWordCount = null); +public class UpdateProjectRequestValidator : IModelValidator +{ + public ValidationResult Validate(UpdateProjectRequest model) + { + var result = new ValidationResult(); + + if (model.Title is not null) + { + if (model.Title.Length == 0) + result.AddError("Title", "'Title' can not be cleared — a project always needs one."); + else if (model.Title.Length > 200) + result.AddError("Title", "'Title' must be 200 characters or fewer."); + } + + ProjectValidation.OptionalFields(model.Author, model.Genre, model.Logline, model.Synopsis, model.Notes, model.TargetWordCount, result); + + return result; + } +} + +file static class ProjectValidation +{ + public static void Title(string title, ValidationResult result) + { + if (string.IsNullOrWhiteSpace(title)) + result.AddError("Title", "'Title' must not be empty."); + else if (title.Length > 200) + result.AddError("Title", "'Title' must be 200 characters or fewer."); + } + + public static void OptionalFields( + string? author, string? genre, string? logline, string? synopsis, string? notes, int? targetWordCount, ValidationResult result) + { + if (author is { Length: > 200 }) + result.AddError("Author", "'Author' must be 200 characters or fewer."); + + if (genre is { Length: > 100 }) + result.AddError("Genre", "'Genre' must be 100 characters or fewer."); + + if (logline is { Length: > 500 }) + result.AddError("Logline", "'Logline' must be 500 characters or fewer."); + + if (synopsis is { Length: > 20000 }) + result.AddError("Synopsis", "'Synopsis' must be 20,000 characters or fewer."); + + if (notes is { Length: > 20000 }) + result.AddError("Notes", "'Notes' must be 20,000 characters or fewer."); + + if (targetWordCount is < 0) + result.AddError("TargetWordCount", "'Target Word Count' must be zero or greater."); + } +} + public static class ProjectMapping { public static ProjectDto ToDto(this Project p) => new( diff --git a/src/Novelly.Api/Projects/ProjectEndpoints.cs b/src/Novelly.Api/Projects/ProjectEndpoints.cs index c2202e1..0888d08 100644 --- a/src/Novelly.Api/Projects/ProjectEndpoints.cs +++ b/src/Novelly.Api/Projects/ProjectEndpoints.cs @@ -1,4 +1,5 @@ using Novelly.Api.Common; +using Novelly.Api.Common.Validation; namespace Novelly.Api.Projects; @@ -6,14 +7,16 @@ public static class ProjectEndpoints { public static IEndpointRouteBuilder MapProjectEndpoints(this IEndpointRouteBuilder app) { - var group = app.MapGroup("/api/projects").WithTags("Projects").AddEndpointFilter(); + var group = app.MapGroup("/api/projects").WithTags("Projects") + .AddEndpointFilter() + .AddEndpointFilter(); group.MapGet("/", async (ProjectService service, CancellationToken ct) => Results.Ok(await service.ListAsync(ct))) .WithSummary("List all novel projects."); group.MapGet("/{id:guid}", async (Guid id, ProjectService service, CancellationToken ct) => - Results.Ok(await service.GetAsync(id, ct))) + (await service.GetAsync(id, ct)).ToApiResult()) .WithSummary("Read a project's brief."); group.MapPost("/", async (CreateProjectRequest request, ProjectService service, CancellationToken ct) => @@ -25,14 +28,11 @@ public static class ProjectEndpoints group.MapPatch("/{id:guid}", async ( Guid id, UpdateProjectRequest request, ProjectService service, CancellationToken ct) => - Results.Ok(await service.UpdateAsync(id, request, ct))) + (await service.UpdateAsync(id, request, ct)).ToApiResult()) .WithSummary("Update a project's brief."); group.MapDelete("/{id:guid}", async (Guid id, ProjectService service, CancellationToken ct) => - { - await service.DeleteAsync(id, ct); - return Results.NoContent(); - }) + await service.DeleteAsync(id, ct) ? Results.NoContent() : Results.NotFound()) .WithSummary("Delete a project and everything in it."); return app; diff --git a/src/Novelly.Api/Projects/ProjectService.cs b/src/Novelly.Api/Projects/ProjectService.cs index d58a57c..5cbfa0b 100644 --- a/src/Novelly.Api/Projects/ProjectService.cs +++ b/src/Novelly.Api/Projects/ProjectService.cs @@ -1,10 +1,15 @@ using Microsoft.EntityFrameworkCore; using Novelly.Api.Common; +using Novelly.Api.Common.Validation; using Novelly.Api.Data; namespace Novelly.Api.Projects; -public class ProjectService(INovelDbContext db, ILogger logger) +public class ProjectService( + INovelDbContext db, + ILogger logger, + IModelValidator createValidator, + IModelValidator updateValidator) { public async Task> ListAsync(CancellationToken ct = default) { @@ -26,14 +31,20 @@ public class ProjectService(INovelDbContext db, ILogger logger) .ToListAsync(ct); } - public async Task GetAsync(Guid id, CancellationToken ct = default) + /// Null when no project has this id — a lookup miss is expected, not exceptional. + public async Task GetAsync(Guid id, CancellationToken ct = default) { + Guard.Default(id, nameof(id)); + logger.LogInformation("Getting project {ProjectId}", id); - return (await FindAsync(id, ct)).ToDto(); + return (await FindAsync(id, ct))?.ToDto(); } public async Task CreateAsync(CreateProjectRequest request, CancellationToken ct = default) { + Guard.Null(request, nameof(request)); + createValidator.Validate(request).ThrowIfInvalid(); + logger.LogInformation("Creating project {Title}", request.Title); var project = new Project @@ -52,11 +63,19 @@ public class ProjectService(INovelDbContext db, ILogger logger) return project.ToDto(); } - public async Task UpdateAsync(Guid id, UpdateProjectRequest request, CancellationToken ct = default) + public async Task UpdateAsync(Guid id, UpdateProjectRequest request, CancellationToken ct = default) { + Guard.Default(id, nameof(id)); + Guard.Null(request, nameof(request)); + updateValidator.Validate(request).ThrowIfInvalid(); + logger.LogInformation("Updating project {ProjectId}", id); var project = await FindAsync(id, ct); + if (project is null) + { + return null; + } project.Title = Patch.Apply(project.Title, request.Title) ?? project.Title; project.Author = Patch.Apply(project.Author, request.Author); @@ -71,27 +90,38 @@ public class ProjectService(INovelDbContext db, ILogger logger) return project.ToDto(); } - public async Task DeleteAsync(Guid id, CancellationToken ct = default) + /// True if a project was deleted; false if no project had this id. + public async Task DeleteAsync(Guid id, CancellationToken ct = default) { + Guard.Default(id, nameof(id)); + logger.LogInformation("Deleting project {ProjectId}", id); var project = await FindAsync(id, ct); + if (project is null) + { + return false; + } + db.Projects.Remove(project); await db.SaveChangesAsync(ct); + return true; } - private async Task FindAsync(Guid id, CancellationToken ct) + private async Task FindAsync(Guid id, CancellationToken ct) { logger.LogDebug("Finding project {ProjectId}", id); var project = await db.Projects.FirstOrDefaultAsync(p => p.Id == id, ct); if (project is null) { - logger.LogWarning("Project {ProjectId} not found", id); - throw new NotFoundException(nameof(Project), id); + logger.LogInformation("Project {ProjectId} not found", id); + } + else + { + logger.LogDebug("Found project {ProjectId}", id); } - logger.LogDebug("Found project {ProjectId}", id); return project; } } diff --git a/src/Novelly.Api/Questions/OpenQuestionDtos.cs b/src/Novelly.Api/Questions/OpenQuestionDtos.cs index 98b1661..6aefc58 100644 --- a/src/Novelly.Api/Questions/OpenQuestionDtos.cs +++ b/src/Novelly.Api/Questions/OpenQuestionDtos.cs @@ -1,3 +1,5 @@ +using Novelly.Api.Common.Validation; + namespace Novelly.Api.Questions; public record OpenQuestionDto( @@ -22,6 +24,24 @@ public record CreateOpenQuestionRequest( Guid? ChapterId = null, Guid? CharacterId = null); +public class CreateOpenQuestionRequestValidator : IModelValidator +{ + public ValidationResult Validate(CreateOpenQuestionRequest model) + { + var result = new ValidationResult(); + + if (string.IsNullOrWhiteSpace(model.Question)) + result.AddError("Question", "'Question' must not be empty."); + else if (model.Question.Length > 1000) + result.AddError("Question", "'Question' must be 1,000 characters or fewer."); + + if (model.Detail is { Length: > 20000 }) + result.AddError("Detail", "'Detail' must be 20,000 characters or fewer."); + + return result; + } +} + /// /// Patch-style update. A null field is left alone; an empty string clears it. Use /// / to detach a question, since a @@ -35,6 +55,27 @@ public record UpdateOpenQuestionRequest( bool ClearChapter = false, bool ClearCharacter = false); +public class UpdateOpenQuestionRequestValidator : IModelValidator +{ + public ValidationResult Validate(UpdateOpenQuestionRequest model) + { + var result = new ValidationResult(); + + if (model.Question is not null) + { + if (model.Question.Length == 0) + result.AddError("Question", "'Question' can not be cleared — a question always needs one."); + else if (model.Question.Length > 1000) + result.AddError("Question", "'Question' must be 1,000 characters or fewer."); + } + + if (model.Detail is { Length: > 20000 }) + result.AddError("Detail", "'Detail' must be 20,000 characters or fewer."); + + return result; + } +} + /// /// Settles a question. The resolution is kept on the question itself; setting /// also appends it to the notes of whatever the question is @@ -42,6 +83,21 @@ public record UpdateOpenQuestionRequest( /// public record ResolveOpenQuestionRequest(string Resolution, bool AppendToNotes = false); +public class ResolveOpenQuestionRequestValidator : IModelValidator +{ + public ValidationResult Validate(ResolveOpenQuestionRequest model) + { + var result = new ValidationResult(); + + if (string.IsNullOrWhiteSpace(model.Resolution)) + result.AddError("Resolution", "'Resolution' must not be empty."); + else if (model.Resolution.Length > 20000) + result.AddError("Resolution", "'Resolution' must be 20,000 characters or fewer."); + + return result; + } +} + public static class OpenQuestionMapping { public static OpenQuestionDto ToDto(this OpenQuestion q) => new( diff --git a/src/Novelly.Api/Questions/OpenQuestionEndpoints.cs b/src/Novelly.Api/Questions/OpenQuestionEndpoints.cs index fe86633..8152480 100644 --- a/src/Novelly.Api/Questions/OpenQuestionEndpoints.cs +++ b/src/Novelly.Api/Questions/OpenQuestionEndpoints.cs @@ -1,4 +1,5 @@ using Novelly.Api.Common; +using Novelly.Api.Common.Validation; namespace Novelly.Api.Questions; @@ -6,7 +7,9 @@ public static class OpenQuestionEndpoints { public static IEndpointRouteBuilder MapOpenQuestionEndpoints(this IEndpointRouteBuilder app) { - var projectScoped = app.MapGroup("/api/projects/{projectId:guid}/questions").WithTags("Questions").AddEndpointFilter(); + var projectScoped = app.MapGroup("/api/projects/{projectId:guid}/questions").WithTags("Questions") + .AddEndpointFilter() + .AddEndpointFilter(); projectScoped.MapGet("/", async ( Guid projectId, @@ -26,31 +29,30 @@ public static class OpenQuestionEndpoints }) .WithSummary("Raise an open question, optionally against a chapter outline and/or a character."); - var questions = app.MapGroup("/api/questions").WithTags("Questions").AddEndpointFilter(); + var questions = app.MapGroup("/api/questions").WithTags("Questions") + .AddEndpointFilter() + .AddEndpointFilter(); questions.MapGet("/{id:guid}", async (Guid id, OpenQuestionService service, CancellationToken ct) => - Results.Ok(await service.GetAsync(id, ct))) + (await service.GetAsync(id, ct)).ToApiResult()) .WithSummary("Read one question."); questions.MapPatch("/{id:guid}", async ( Guid id, UpdateOpenQuestionRequest request, OpenQuestionService service, CancellationToken ct) => - Results.Ok(await service.UpdateAsync(id, request, ct))) + (await service.UpdateAsync(id, request, ct)).ToApiResult()) .WithSummary("Update a question or change what it is attached to."); questions.MapPost("/{id:guid}/resolve", async ( Guid id, ResolveOpenQuestionRequest request, OpenQuestionService service, CancellationToken ct) => - Results.Ok(await service.ResolveAsync(id, request, ct))) + (await service.ResolveAsync(id, request, ct)).ToApiResult()) .WithSummary("Settle a question, optionally appending the resolution to the notes it hangs off."); questions.MapPost("/{id:guid}/reopen", async (Guid id, OpenQuestionService service, CancellationToken ct) => - Results.Ok(await service.ReopenAsync(id, ct))) + (await service.ReopenAsync(id, ct)).ToApiResult()) .WithSummary("Put a resolved question back on the list."); questions.MapDelete("/{id:guid}", async (Guid id, OpenQuestionService service, CancellationToken ct) => - { - await service.DeleteAsync(id, ct); - return Results.NoContent(); - }) + await service.DeleteAsync(id, ct) ? Results.NoContent() : Results.NotFound()) .WithSummary("Delete a question."); return app; diff --git a/src/Novelly.Api/Questions/OpenQuestionService.cs b/src/Novelly.Api/Questions/OpenQuestionService.cs index 85fe4fa..540a5e2 100644 --- a/src/Novelly.Api/Questions/OpenQuestionService.cs +++ b/src/Novelly.Api/Questions/OpenQuestionService.cs @@ -2,6 +2,7 @@ using Microsoft.EntityFrameworkCore; using Novelly.Api.Chapters; using Novelly.Api.Characters; using Novelly.Api.Common; +using Novelly.Api.Common.Validation; using Novelly.Api.Data; using Novelly.Api.Projects; @@ -11,7 +12,12 @@ namespace Novelly.Api.Questions; /// The project's open questions — the decisions still outstanding. A question can be /// attached to a chapter outline, a character, both, or neither. /// -public class OpenQuestionService(INovelDbContext db, ILogger logger) +public class OpenQuestionService( + INovelDbContext db, + ILogger logger, + IModelValidator createValidator, + IModelValidator updateValidator, + IModelValidator resolveValidator) { /// /// Lists a project's questions, open ones first and newest first within each group. @@ -25,6 +31,8 @@ public class OpenQuestionService(INovelDbContext db, ILogger GetAsync(Guid id, CancellationToken ct = default) + /// Null when no open question has this id — a lookup miss is expected, not exceptional. + public async Task GetAsync(Guid id, CancellationToken ct = default) { + Guard.Default(id, nameof(id)); + logger.LogInformation("Getting open question {QuestionId}", id); - return (await FindAsync(id, ct)).ToDto(); + return (await FindAsync(id, ct))?.ToDto(); } public async Task CreateAsync( Guid projectId, CreateOpenQuestionRequest request, CancellationToken ct = default) { + Guard.Default(projectId, nameof(projectId)); + Guard.Null(request, nameof(request)); + createValidator.Validate(request).ThrowIfInvalid(); + logger.LogInformation("Creating open question for project {ProjectId}", projectId); if (!await db.Projects.AnyAsync(p => p.Id == projectId, ct)) { - logger.LogWarning("Project {ProjectId} not found", projectId); + logger.LogWarning("Rejected open question creation: project {ProjectId} not found", projectId); throw new NotFoundException(nameof(Project), projectId); } - if (string.IsNullOrWhiteSpace(request.Question)) - { - logger.LogWarning("Rejected open question creation for project {ProjectId}: question text was blank", projectId); - throw new ArgumentException("A question needs to say something."); - } - await ValidateAssociationsAsync(projectId, request.ChapterId, request.CharacterId, ct); var question = new OpenQuestion @@ -93,15 +102,25 @@ public class OpenQuestionService(INovelDbContext db, ILogger UpdateAsync( + public async Task UpdateAsync( Guid id, UpdateOpenQuestionRequest request, CancellationToken ct = default) { + Guard.Default(id, nameof(id)); + Guard.Null(request, nameof(request)); + updateValidator.Validate(request).ThrowIfInvalid(); + logger.LogInformation("Updating open question {QuestionId}", id); var question = await FindAsync(id, ct); + if (question is null) + { + return null; + } await ValidateAssociationsAsync(question.ProjectId, request.ChapterId, request.CharacterId, ct); @@ -112,25 +131,28 @@ public class OpenQuestionService(INovelDbContext db, ILogger /// Settles a question. With AppendToNotes the resolution is also appended to the /// notes of the chapter and character it hangs off, so the decision ends up where the - /// writer reads rather than only in a list they have stopped looking at. + /// writer reads rather than only in a list they have stopped looking at. Null when no + /// open question has this id. /// - public async Task ResolveAsync( + public async Task ResolveAsync( Guid id, ResolveOpenQuestionRequest request, CancellationToken ct = default) { + Guard.Default(id, nameof(id)); + Guard.Null(request, nameof(request)); + resolveValidator.Validate(request).ThrowIfInvalid(); + logger.LogInformation("Resolving open question {QuestionId}, appendToNotes {AppendToNotes}", id, request.AppendToNotes); var question = await FindAsync(id, ct); - - if (string.IsNullOrWhiteSpace(request.Resolution)) + if (question is null) { - logger.LogWarning("Rejected resolution for open question {QuestionId}: resolution text was blank", id); - throw new ArgumentException("A resolution needs to say what was decided."); + return null; } question.Resolution = request.Resolution.Trim(); @@ -167,31 +189,46 @@ public class OpenQuestionService(INovelDbContext db, ILoggerPuts a question back on the list. The resolution goes; anything already appended to notes stays. - public async Task ReopenAsync(Guid id, CancellationToken ct = default) + /// Puts a question back on the list. The resolution goes; anything already appended to notes stays. Null when no open question has this id. + public async Task ReopenAsync(Guid id, CancellationToken ct = default) { + Guard.Default(id, nameof(id)); + logger.LogInformation("Reopening open question {QuestionId}", id); var question = await FindAsync(id, ct); + if (question is null) + { + return null; + } question.Resolution = null; question.ResolvedAt = null; question.UpdatedAt = DateTimeOffset.UtcNow; await db.SaveChangesAsync(ct); - return (await FindAsync(id, ct)).ToDto(); + return (await FindAsync(id, ct))!.ToDto(); } - public async Task DeleteAsync(Guid id, CancellationToken ct = default) + /// True if an open question was deleted; false if no question had this id. + public async Task DeleteAsync(Guid id, CancellationToken ct = default) { + Guard.Default(id, nameof(id)); + logger.LogInformation("Deleting open question {QuestionId}", id); var question = await FindAsync(id, ct); + if (question is null) + { + return false; + } + db.OpenQuestions.Remove(question); await db.SaveChangesAsync(ct); + return true; } /// Blank line between entries, so appended resolutions stay readable as notes accumulate. @@ -223,18 +260,20 @@ public class OpenQuestionService(INovelDbContext db, ILogger Query() => db.OpenQuestions.Include(q => q.Chapter).Include(q => q.Character); - private async Task FindAsync(Guid id, CancellationToken ct) + private async Task FindAsync(Guid id, CancellationToken ct) { logger.LogDebug("Finding open question {QuestionId}", id); var question = await Query().FirstOrDefaultAsync(q => q.Id == id, ct); if (question is null) { - logger.LogWarning("OpenQuestion {QuestionId} not found", id); - throw new NotFoundException(nameof(OpenQuestion), id); + logger.LogInformation("OpenQuestion {QuestionId} not found", id); + } + else + { + logger.LogDebug("Found open question {QuestionId}", id); } - logger.LogDebug("Found open question {QuestionId}", id); return question; } } diff --git a/src/Novelly.Api/Scenes/SceneDtos.cs b/src/Novelly.Api/Scenes/SceneDtos.cs index 7d94204..994faeb 100644 --- a/src/Novelly.Api/Scenes/SceneDtos.cs +++ b/src/Novelly.Api/Scenes/SceneDtos.cs @@ -1,4 +1,5 @@ using Novelly.Api.Common; +using Novelly.Api.Common.Validation; namespace Novelly.Api.Scenes; @@ -31,6 +32,23 @@ public record CreateSceneRequest( string? Prose = null, DraftStatus Status = DraftStatus.Planned); +public class CreateSceneRequestValidator : IModelValidator +{ + public ValidationResult Validate(CreateSceneRequest model) + { + var result = new ValidationResult(); + + if (string.IsNullOrWhiteSpace(model.Title)) + result.AddError("Title", "'Title' must not be empty."); + else if (model.Title.Length > 200) + result.AddError("Title", "'Title' must be 200 characters or fewer."); + + SceneValidation.OptionalFields(model.SortOrder, model.Summary, model.Goal, model.Conflict, model.Outcome, model.Location, model.Prose, result); + + return result; + } +} + public record UpdateSceneRequest( string? Title = null, int? SortOrder = null, @@ -43,6 +61,54 @@ public record UpdateSceneRequest( string? Prose = null, DraftStatus? Status = null); +public class UpdateSceneRequestValidator : IModelValidator +{ + public ValidationResult Validate(UpdateSceneRequest model) + { + var result = new ValidationResult(); + + if (model.Title is not null) + { + if (model.Title.Length == 0) + result.AddError("Title", "'Title' can not be cleared — a scene always needs one."); + else if (model.Title.Length > 200) + result.AddError("Title", "'Title' must be 200 characters or fewer."); + } + + SceneValidation.OptionalFields(model.SortOrder, model.Summary, model.Goal, model.Conflict, model.Outcome, model.Location, model.Prose, result); + + return result; + } +} + +file static class SceneValidation +{ + public static void OptionalFields( + int? sortOrder, string? summary, string? goal, string? conflict, string? outcome, string? location, string? prose, ValidationResult result) + { + if (sortOrder is < 0) + result.AddError("SortOrder", "'Sort Order' must be zero or greater."); + + if (summary is { Length: > 20000 }) + result.AddError("Summary", "'Summary' must be 20,000 characters or fewer."); + + if (goal is { Length: > 20000 }) + result.AddError("Goal", "'Goal' must be 20,000 characters or fewer."); + + if (conflict is { Length: > 20000 }) + result.AddError("Conflict", "'Conflict' must be 20,000 characters or fewer."); + + if (outcome is { Length: > 20000 }) + result.AddError("Outcome", "'Outcome' must be 20,000 characters or fewer."); + + if (location is { Length: > 500 }) + result.AddError("Location", "'Location' must be 500 characters or fewer."); + + if (prose is { Length: > 100000 }) + result.AddError("Prose", "'Prose' must be 100,000 characters or fewer."); + } +} + public static class SceneMapping { public static SceneDto ToDto(this Scene s) => new( diff --git a/src/Novelly.Api/Scenes/SceneEndpoints.cs b/src/Novelly.Api/Scenes/SceneEndpoints.cs index 53aa6a4..2784b9a 100644 --- a/src/Novelly.Api/Scenes/SceneEndpoints.cs +++ b/src/Novelly.Api/Scenes/SceneEndpoints.cs @@ -1,4 +1,5 @@ using Novelly.Api.Common; +using Novelly.Api.Common.Validation; namespace Novelly.Api.Scenes; @@ -6,7 +7,9 @@ public static class SceneEndpoints { public static IEndpointRouteBuilder MapSceneEndpoints(this IEndpointRouteBuilder app) { - var chapterScoped = app.MapGroup("/api/chapters/{chapterId:guid}/scenes").WithTags("Scenes").AddEndpointFilter(); + var chapterScoped = app.MapGroup("/api/chapters/{chapterId:guid}/scenes").WithTags("Scenes") + .AddEndpointFilter() + .AddEndpointFilter(); chapterScoped.MapGet("/", async (Guid chapterId, SceneService service, CancellationToken ct) => Results.Ok(await service.ListAsync(chapterId, ct))) @@ -20,22 +23,21 @@ public static class SceneEndpoints }) .WithSummary("Add a scene to a chapter."); - var scenes = app.MapGroup("/api/scenes").WithTags("Scenes").AddEndpointFilter(); + var scenes = app.MapGroup("/api/scenes").WithTags("Scenes") + .AddEndpointFilter() + .AddEndpointFilter(); scenes.MapGet("/{id:guid}", async (Guid id, SceneService service, CancellationToken ct) => - Results.Ok(await service.GetAsync(id, ct))) + (await service.GetAsync(id, ct)).ToApiResult()) .WithSummary("Read a scene, including its prose."); scenes.MapPatch("/{id:guid}", async ( Guid id, UpdateSceneRequest request, SceneService service, CancellationToken ct) => - Results.Ok(await service.UpdateAsync(id, request, ct))) + (await service.UpdateAsync(id, request, ct)).ToApiResult()) .WithSummary("Update a scene. Sending prose recomputes the word count."); scenes.MapDelete("/{id:guid}", async (Guid id, SceneService service, CancellationToken ct) => - { - await service.DeleteAsync(id, ct); - return Results.NoContent(); - }) + await service.DeleteAsync(id, ct) ? Results.NoContent() : Results.NotFound()) .WithSummary("Delete a scene."); return app; diff --git a/src/Novelly.Api/Scenes/SceneService.cs b/src/Novelly.Api/Scenes/SceneService.cs index 6ac1042..79ae9ed 100644 --- a/src/Novelly.Api/Scenes/SceneService.cs +++ b/src/Novelly.Api/Scenes/SceneService.cs @@ -1,14 +1,21 @@ using Microsoft.EntityFrameworkCore; using Novelly.Api.Chapters; using Novelly.Api.Common; +using Novelly.Api.Common.Validation; using Novelly.Api.Data; namespace Novelly.Api.Scenes; -public class SceneService(INovelDbContext db, ILogger logger) +public class SceneService( + INovelDbContext db, + ILogger logger, + IModelValidator createValidator, + IModelValidator updateValidator) { public async Task> ListAsync(Guid chapterId, CancellationToken ct = default) { + Guard.Default(chapterId, nameof(chapterId)); + logger.LogInformation("Listing scenes for chapter {ChapterId}", chapterId); var scenes = await Query() @@ -19,19 +26,26 @@ public class SceneService(INovelDbContext db, ILogger logger) return [.. scenes.Select(s => s.ToDto())]; } - public async Task GetAsync(Guid id, CancellationToken ct = default) + /// Null when no scene has this id — a lookup miss is expected, not exceptional. + public async Task GetAsync(Guid id, CancellationToken ct = default) { + Guard.Default(id, nameof(id)); + logger.LogInformation("Getting scene {SceneId}", id); - return (await FindAsync(id, ct)).ToDto(); + return (await FindAsync(id, ct))?.ToDto(); } public async Task CreateAsync(Guid chapterId, CreateSceneRequest request, CancellationToken ct = default) { + Guard.Default(chapterId, nameof(chapterId)); + Guard.Null(request, nameof(request)); + createValidator.Validate(request).ThrowIfInvalid(); + logger.LogInformation("Creating scene {Title} for chapter {ChapterId}, prose length {ProseLength}", request.Title, chapterId, request.Prose?.Length ?? 0); if (!await db.Chapters.AnyAsync(c => c.Id == chapterId, ct)) { - logger.LogWarning("Chapter {ChapterId} not found", chapterId); + logger.LogWarning("Rejected scene creation: chapter {ChapterId} not found", chapterId); throw new NotFoundException(nameof(Chapter), chapterId); } @@ -53,14 +67,24 @@ public class SceneService(INovelDbContext db, ILogger logger) db.Scenes.Add(scene); await db.SaveChangesAsync(ct); - return (await FindAsync(scene.Id, ct)).ToDto(); + + // Just created it — the reload is only to pick up includes, not to check existence. + return (await FindAsync(scene.Id, ct))!.ToDto(); } - public async Task UpdateAsync(Guid id, UpdateSceneRequest request, CancellationToken ct = default) + public async Task UpdateAsync(Guid id, UpdateSceneRequest request, CancellationToken ct = default) { + Guard.Default(id, nameof(id)); + Guard.Null(request, nameof(request)); + updateValidator.Validate(request).ThrowIfInvalid(); + logger.LogInformation("Updating scene {SceneId}, prose length {ProseLength}", id, request.Prose?.Length ?? 0); var scene = await FindAsync(id, ct); + if (scene is null) + { + return null; + } scene.Title = Patch.Apply(scene.Title, request.Title) ?? scene.Title; scene.SortOrder = request.SortOrder ?? scene.SortOrder; @@ -81,16 +105,25 @@ public class SceneService(INovelDbContext db, ILogger logger) scene.UpdatedAt = DateTimeOffset.UtcNow; await db.SaveChangesAsync(ct); - return (await FindAsync(id, ct)).ToDto(); + return (await FindAsync(id, ct))!.ToDto(); } - public async Task DeleteAsync(Guid id, CancellationToken ct = default) + /// True if a scene was deleted; false if no scene had this id. + public async Task DeleteAsync(Guid id, CancellationToken ct = default) { + Guard.Default(id, nameof(id)); + logger.LogInformation("Deleting scene {SceneId}", id); var scene = await FindAsync(id, ct); + if (scene is null) + { + return false; + } + db.Scenes.Remove(scene); await db.SaveChangesAsync(ct); + return true; } private async Task NextSortOrderAsync(Guid chapterId, CancellationToken ct) @@ -106,18 +139,20 @@ public class SceneService(INovelDbContext db, ILogger logger) private IQueryable Query() => db.Scenes.Include(s => s.PovCharacter); - private async Task FindAsync(Guid id, CancellationToken ct) + private async Task FindAsync(Guid id, CancellationToken ct) { logger.LogDebug("Finding scene {SceneId}", id); var scene = await Query().FirstOrDefaultAsync(s => s.Id == id, ct); if (scene is null) { - logger.LogWarning("Scene {SceneId} not found", id); - throw new NotFoundException(nameof(Scene), id); + logger.LogInformation("Scene {SceneId} not found", id); + } + else + { + logger.LogDebug("Found scene {SceneId}", id); } - logger.LogDebug("Found scene {SceneId}", id); return scene; } } diff --git a/src/Novelly.Api/Tags/TagDtos.cs b/src/Novelly.Api/Tags/TagDtos.cs index e2d0171..fb9c26e 100644 --- a/src/Novelly.Api/Tags/TagDtos.cs +++ b/src/Novelly.Api/Tags/TagDtos.cs @@ -1,3 +1,5 @@ +using Novelly.Api.Common.Validation; + namespace Novelly.Api.Tags; public record TagDto(Guid Id, string Name, string? Color); @@ -15,8 +17,47 @@ public record TagSummaryDto( public record CreateTagRequest(string Name, string? Color = null); +public class CreateTagRequestValidator : IModelValidator +{ + public ValidationResult Validate(CreateTagRequest model) + { + var result = new ValidationResult(); + + if (string.IsNullOrWhiteSpace(model.Name)) + result.AddError("Name", "'Name' must not be empty."); + else if (model.Name.Length > 100) + result.AddError("Name", "'Name' must be 100 characters or fewer."); + + if (model.Color is { Length: > 50 }) + result.AddError("Color", "'Color' must be 50 characters or fewer."); + + return result; + } +} + public record UpdateTagRequest(string? Name = null, string? Color = null); +public class UpdateTagRequestValidator : IModelValidator +{ + public ValidationResult Validate(UpdateTagRequest model) + { + var result = new ValidationResult(); + + if (model.Name is not null) + { + if (model.Name.Length == 0) + result.AddError("Name", "'Name' can not be cleared — a tag always needs one."); + else if (model.Name.Length > 100) + result.AddError("Name", "'Name' must be 100 characters or fewer."); + } + + if (model.Color is { Length: > 50 }) + result.AddError("Color", "'Color' must be 50 characters or fewer."); + + return result; + } +} + /// /// Everything carrying one tag, gathered in a single response. This is the whole point of /// tags — seeing that a motif touches two characters, a chapter and four beats is what diff --git a/src/Novelly.Api/Tags/TagEndpoints.cs b/src/Novelly.Api/Tags/TagEndpoints.cs index 58c9940..be8f229 100644 --- a/src/Novelly.Api/Tags/TagEndpoints.cs +++ b/src/Novelly.Api/Tags/TagEndpoints.cs @@ -1,4 +1,5 @@ using Novelly.Api.Common; +using Novelly.Api.Common.Validation; namespace Novelly.Api.Tags; @@ -6,7 +7,9 @@ public static class TagEndpoints { public static IEndpointRouteBuilder MapTagEndpoints(this IEndpointRouteBuilder app) { - var projectScoped = app.MapGroup("/api/projects/{projectId:guid}/tags").WithTags("Tags").AddEndpointFilter(); + var projectScoped = app.MapGroup("/api/projects/{projectId:guid}/tags").WithTags("Tags") + .AddEndpointFilter() + .AddEndpointFilter(); projectScoped.MapGet("/", async (Guid projectId, TagService service, CancellationToken ct) => Results.Ok(await service.ListAsync(projectId, ct))) @@ -20,22 +23,21 @@ public static class TagEndpoints }) .WithSummary("Create a tag. Tags are also created on demand when applied by name."); - var tags = app.MapGroup("/api/tags").WithTags("Tags").AddEndpointFilter(); + var tags = app.MapGroup("/api/tags").WithTags("Tags") + .AddEndpointFilter() + .AddEndpointFilter(); tags.MapGet("/{id:guid}/references", async (Guid id, TagService service, CancellationToken ct) => - Results.Ok(await service.GetReferencesAsync(id, ct))) + (await service.GetReferencesAsync(id, ct)).ToApiResult()) .WithSummary("Cross-reference: every character, chapter and beat carrying this tag."); tags.MapPatch("/{id:guid}", async ( Guid id, UpdateTagRequest request, TagService service, CancellationToken ct) => - Results.Ok(await service.UpdateAsync(id, request, ct))) + (await service.UpdateAsync(id, request, ct)).ToApiResult()) .WithSummary("Rename or recolour a tag."); tags.MapDelete("/{id:guid}", async (Guid id, TagService service, CancellationToken ct) => - { - await service.DeleteAsync(id, ct); - return Results.NoContent(); - }) + await service.DeleteAsync(id, ct) ? Results.NoContent() : Results.NotFound()) .WithSummary("Delete a tag. Whatever carried it is left alone."); return app; diff --git a/src/Novelly.Api/Tags/TagService.cs b/src/Novelly.Api/Tags/TagService.cs index a32f358..ebe8ffe 100644 --- a/src/Novelly.Api/Tags/TagService.cs +++ b/src/Novelly.Api/Tags/TagService.cs @@ -1,14 +1,21 @@ using Microsoft.EntityFrameworkCore; using Novelly.Api.Common; +using Novelly.Api.Common.Validation; using Novelly.Api.Data; using Novelly.Api.Projects; namespace Novelly.Api.Tags; -public class TagService(INovelDbContext db, ILogger logger) +public class TagService( + INovelDbContext db, + ILogger logger, + IModelValidator createValidator, + IModelValidator updateValidator) { public async Task> ListAsync(Guid projectId, CancellationToken ct = default) { + Guard.Default(projectId, nameof(projectId)); + logger.LogInformation("Listing tags for project {ProjectId}", projectId); return await db.Tags @@ -20,9 +27,11 @@ public class TagService(INovelDbContext db, ILogger logger) .ToListAsync(ct); } - /// Everything in the project carrying this tag. - public async Task GetReferencesAsync(Guid tagId, CancellationToken ct = default) + /// Everything in the project carrying this tag. Null when no tag has this id. + public async Task GetReferencesAsync(Guid tagId, CancellationToken ct = default) { + Guard.Default(tagId, nameof(tagId)); + logger.LogInformation("Getting references for tag {TagId}", tagId); var tag = await db.Tags @@ -34,8 +43,8 @@ public class TagService(INovelDbContext db, ILogger logger) if (tag is null) { - logger.LogWarning("Tag {TagId} not found", tagId); - throw new NotFoundException(nameof(Tag), tagId); + logger.LogInformation("Tag {TagId} not found", tagId); + return null; } return new TagReferencesDto( @@ -62,20 +71,19 @@ public class TagService(INovelDbContext db, ILogger logger) public async Task CreateAsync(Guid projectId, CreateTagRequest request, CancellationToken ct = default) { + Guard.Default(projectId, nameof(projectId)); + Guard.Null(request, nameof(request)); + createValidator.Validate(request).ThrowIfInvalid(); + logger.LogInformation("Creating tag {Name} for project {ProjectId}", request.Name, projectId); if (!await db.Projects.AnyAsync(p => p.Id == projectId, ct)) { - logger.LogWarning("Project {ProjectId} not found", projectId); + logger.LogWarning("Rejected tag creation: project {ProjectId} not found", projectId); throw new NotFoundException(nameof(Project), projectId); } var name = TagMapping.Normalise(request.Name); - if (string.IsNullOrWhiteSpace(name)) - { - logger.LogWarning("Rejected tag creation for project {ProjectId}: name was blank", projectId); - throw new ArgumentException("A tag needs a name."); - } var existing = await FindByNameAsync(projectId, name, ct); if (existing is not null) @@ -90,25 +98,24 @@ public class TagService(INovelDbContext db, ILogger logger) return tag.ToDto(); } - public async Task UpdateAsync(Guid tagId, UpdateTagRequest request, CancellationToken ct = default) + public async Task UpdateAsync(Guid tagId, UpdateTagRequest request, CancellationToken ct = default) { + Guard.Default(tagId, nameof(tagId)); + Guard.Null(request, nameof(request)); + updateValidator.Validate(request).ThrowIfInvalid(); + logger.LogInformation("Updating tag {TagId}", tagId); var tag = await db.Tags.FirstOrDefaultAsync(t => t.Id == tagId, ct); if (tag is null) { - logger.LogWarning("Tag {TagId} not found", tagId); - throw new NotFoundException(nameof(Tag), tagId); + logger.LogInformation("Tag {TagId} not found", tagId); + return null; } if (request.Name is not null) { var name = TagMapping.Normalise(request.Name); - if (string.IsNullOrWhiteSpace(name)) - { - logger.LogWarning("Rejected update for tag {TagId}: name was blank", tagId); - throw new ArgumentException("A tag needs a name."); - } var clash = await FindByNameAsync(tag.ProjectId, name, ct); if (clash is not null && clash.Id != tag.Id) @@ -125,20 +132,23 @@ public class TagService(INovelDbContext db, ILogger logger) return tag.ToDto(); } - /// Deletes a tag. Whatever carried it keeps existing — only the label goes. - public async Task DeleteAsync(Guid tagId, CancellationToken ct = default) + /// Deletes a tag. Whatever carried it keeps existing — only the label goes. True if deleted. + public async Task DeleteAsync(Guid tagId, CancellationToken ct = default) { + Guard.Default(tagId, nameof(tagId)); + logger.LogInformation("Deleting tag {TagId}", tagId); var tag = await db.Tags.FirstOrDefaultAsync(t => t.Id == tagId, ct); if (tag is null) { - logger.LogWarning("Tag {TagId} not found", tagId); - throw new NotFoundException(nameof(Tag), tagId); + logger.LogInformation("Tag {TagId} not found", tagId); + return false; } db.Tags.Remove(tag); await db.SaveChangesAsync(ct); + return true; } /// @@ -149,6 +159,9 @@ public class TagService(INovelDbContext db, ILogger logger) internal async Task> ResolveAsync( Guid projectId, IReadOnlyList names, CancellationToken ct) { + Guard.Default(projectId, nameof(projectId)); + Guard.Null(names, nameof(names)); + logger.LogDebug("Resolving {Count} tag names for project {ProjectId}", names.Count, projectId); var wanted = names diff --git a/tests/Novelly.Api.Tests/BeatServiceTests.cs b/tests/Novelly.Api.Tests/BeatServiceTests.cs index a3e0cec..64f5824 100644 --- a/tests/Novelly.Api.Tests/BeatServiceTests.cs +++ b/tests/Novelly.Api.Tests/BeatServiceTests.cs @@ -131,7 +131,7 @@ public class BeatServiceTests : ServiceTestFixture await Scenes.DeleteAsync(scene.Id); // The plan outlives a decision about prose — the beat is simply ungrouped. - var survivor = await Beats.GetAsync(beat.Id); + var survivor = (await Beats.GetAsync(beat.Id))!; Assert.Multiple(() => { @@ -148,7 +148,7 @@ public class BeatServiceTests : ServiceTestFixture WhatHappened: "Behind the lining of the case.", WhatsNext: "She books passage.")); - var renamed = await Beats.UpdateAsync(beat.Id, new UpdateBeatRequest(Title: "She finds it")); + var renamed = (await Beats.UpdateAsync(beat.Id, new UpdateBeatRequest(Title: "She finds it")))!; Assert.Multiple(() => { @@ -156,7 +156,7 @@ public class BeatServiceTests : ServiceTestFixture Assert.That(renamed.WhatsNext, Is.EqualTo("She books passage.")); }); - var cleared = await Beats.UpdateAsync(beat.Id, new UpdateBeatRequest(WhatsNext: "")); + var cleared = (await Beats.UpdateAsync(beat.Id, new UpdateBeatRequest(WhatsNext: "")))!; Assert.Multiple(() => { diff --git a/tests/Novelly.Api.Tests/CharacterArcTests.cs b/tests/Novelly.Api.Tests/CharacterArcTests.cs index da88fdd..3f636d7 100644 --- a/tests/Novelly.Api.Tests/CharacterArcTests.cs +++ b/tests/Novelly.Api.Tests/CharacterArcTests.cs @@ -28,8 +28,8 @@ public class CharacterArcTests : ServiceTestFixture Assert.That(mara.Importance, Is.EqualTo(CharacterImportance.Supporting)); - var promoted = await Characters.UpdateAsync( - mara.Id, new UpdateCharacterRequest(Importance: CharacterImportance.Main)); + var promoted = (await Characters.UpdateAsync( + mara.Id, new UpdateCharacterRequest(Importance: CharacterImportance.Main)))!; Assert.That(promoted.Importance, Is.EqualTo(CharacterImportance.Main)); } @@ -102,7 +102,7 @@ public class CharacterArcTests : ServiceTestFixture await Arcs.CreateAsync(_characterId, new CreateArcStageRequest( "She trusts the map", Description: "Because her mother drew it.")); - var character = await Characters.GetAsync(_characterId); + var character = (await Characters.GetAsync(_characterId))!; Assert.Multiple(() => { @@ -163,7 +163,7 @@ public class CharacterArcTests : ServiceTestFixture await Chapters.DeleteAsync(chapter.Id); // How a character changes outlives a decision about where the chapter break falls. - var survivor = await Arcs.GetAsync(stage.Id); + var survivor = (await Arcs.GetAsync(stage.Id))!; Assert.Multiple(() => { @@ -201,7 +201,7 @@ public class CharacterArcTests : ServiceTestFixture await Beats.CreateAsync(first.Id, new CreateBeatRequest("Mara lies", CharacterId: mara.Id)); await Beats.CreateAsync(first.Id, new CreateBeatRequest("Nobody's beat")); - var beats = await Beats.ListForCharacterAsync(_characterId); + var beats = (await Beats.ListForCharacterAsync(_characterId))!; Assert.Multiple(() => { @@ -213,8 +213,6 @@ public class CharacterArcTests : ServiceTestFixture } [Test] - public void Asking_for_the_beats_of_a_character_who_does_not_exist_reports_not_found() => - Assert.That( - async () => await Beats.ListForCharacterAsync(Guid.NewGuid()), - Throws.TypeOf()); + public async Task Asking_for_the_beats_of_a_character_who_does_not_exist_returns_null_rather_than_throwing() => + Assert.That(await Beats.ListForCharacterAsync(Guid.NewGuid()), Is.Null); } diff --git a/tests/Novelly.Api.Tests/ExceptionHandlingTests.cs b/tests/Novelly.Api.Tests/ExceptionHandlingTests.cs new file mode 100644 index 0000000..9d58df5 --- /dev/null +++ b/tests/Novelly.Api.Tests/ExceptionHandlingTests.cs @@ -0,0 +1,57 @@ +using Novelly.Api.Chapters; +using Novelly.Api.Common; +using Novelly.Api.Common.Validation; +using Novelly.Api.Projects; + +namespace Novelly.Api.Tests; + +/// +/// Covers the exception-handling rework: a missing entity is an ordinary result, not a +/// thrown exception; rejects missing required arguments; and a +/// service re-validates a request even when a direct caller skips the API's own filter. +/// +[TestFixture] +public class ExceptionHandlingTests : ServiceTestFixture +{ + [Test] + public void Guard_rejects_an_empty_guid_passed_as_a_required_id() => + Assert.That(() => Projects.GetAsync(Guid.Empty), Throws.TypeOf()); + + [Test] + public void Guard_rejects_a_null_request_object() => + Assert.That( + () => Projects.CreateAsync(null!), + Throws.TypeOf()); + + [Test] + public async Task Deleting_a_missing_project_returns_false_rather_than_throwing() => + Assert.That(await Projects.DeleteAsync(Guid.NewGuid()), Is.False); + + [Test] + public void A_blank_title_fails_the_create_project_validator() + { + var result = new CreateProjectRequestValidator().Validate(new CreateProjectRequest("")); + + Assert.Multiple(() => + { + Assert.That(result.IsInvalid, Is.True); + Assert.That(result.Errors.Select(e => e.PropertyName), Has.Member("Title")); + }); + } + + [Test] + public void Calling_a_service_directly_with_an_invalid_request_throws_rather_than_silently_accepting_it() => + Assert.That( + () => Projects.CreateAsync(new CreateProjectRequest("")), + Throws.TypeOf()); + + [Test] + public async Task An_embedded_reference_to_a_missing_parent_still_throws() + { + // Creating a chapter under a nonexistent project isn't a "look this up" miss — it's + // an invalid precondition for the create, so it stays exceptional. + Assert.That( + async () => await Chapters.CreateAsync(Guid.NewGuid(), new CreateChapterRequest("Landfall")), + Throws.TypeOf()); + } +} diff --git a/tests/Novelly.Api.Tests/ListingTests.cs b/tests/Novelly.Api.Tests/ListingTests.cs index f9e3fa2..6f781f5 100644 --- a/tests/Novelly.Api.Tests/ListingTests.cs +++ b/tests/Novelly.Api.Tests/ListingTests.cs @@ -99,7 +99,8 @@ public class ListingTests : ServiceTestFixture new ScriptedModelClient([[new AgentTextBlock("Reply.")]]), new NovelAgentToolset(Projects, Characters, Arcs, Chapters, Beats, Scenes, Tags, Questions, NullLogger.Instance), Options.Create(new AgentOptions()), - NullLogger.Instance); + NullLogger.Instance, + new SendAgentMessageRequestValidator()); await agent.SendMessageAsync(project.Id, new SendAgentMessageRequest("First question.")); await agent.SendMessageAsync(project.Id, new SendAgentMessageRequest("Second question.")); diff --git a/tests/Novelly.Api.Tests/LoggingTests.cs b/tests/Novelly.Api.Tests/LoggingTests.cs index b4f30a1..a829eab 100644 --- a/tests/Novelly.Api.Tests/LoggingTests.cs +++ b/tests/Novelly.Api.Tests/LoggingTests.cs @@ -15,14 +15,20 @@ namespace Novelly.Api.Tests; public class LoggingTests : ServiceTestFixture { [Test] - public void Fetching_a_missing_chapter_logs_a_warning_before_throwing() + public async Task Fetching_a_missing_chapter_returns_null_and_logs_at_information_not_warning() { var missingId = Guid.NewGuid(); - Assert.That(() => Chapters.GetAsync(missingId), Throws.TypeOf()); + var result = await Chapters.GetAsync(missingId); - var warning = ChapterLogs.Entries.Single(e => e.Level == LogLevel.Warning); - Assert.That(warning.Message, Does.Contain(missingId.ToString())); + Assert.Multiple(() => + { + Assert.That(result, Is.Null); + Assert.That(ChapterLogs.Entries.Where(e => e.Level == LogLevel.Warning), Is.Empty); + Assert.That( + ChapterLogs.Entries, + Has.Some.Matches(e => e.Level == LogLevel.Information && e.Message.Contains(missingId.ToString()))); + }); } [Test] diff --git a/tests/Novelly.Api.Tests/NovelAgentServiceTests.cs b/tests/Novelly.Api.Tests/NovelAgentServiceTests.cs index 3408867..872827f 100644 --- a/tests/Novelly.Api.Tests/NovelAgentServiceTests.cs +++ b/tests/Novelly.Api.Tests/NovelAgentServiceTests.cs @@ -19,7 +19,8 @@ public class NovelAgentServiceTests : ServiceTestFixture model, _toolset, Options.Create(new AgentOptions { MaxIterations = 4 }), - NullLogger.Instance); + NullLogger.Instance, + new SendAgentMessageRequestValidator()); [Test] public async Task A_plain_reply_is_persisted_as_a_conversation() @@ -32,7 +33,7 @@ public class NovelAgentServiceTests : ServiceTestFixture Assert.That(turn.Message.Content, Is.EqualTo("Tell me about the ending.")); - var conversation = await agent.GetConversationAsync(turn.ConversationId); + var conversation = (await agent.GetConversationAsync(turn.ConversationId))!; Assert.Multiple(() => { @@ -171,7 +172,7 @@ public class NovelAgentServiceTests : ServiceTestFixture var second = await agent.SendMessageAsync( projectId, new SendAgentMessageRequest("Question two.", first.ConversationId)); - var conversation = await agent.GetConversationAsync(first.ConversationId); + var conversation = (await agent.GetConversationAsync(first.ConversationId))!; Assert.Multiple(() => { diff --git a/tests/Novelly.Api.Tests/OpenQuestionTests.cs b/tests/Novelly.Api.Tests/OpenQuestionTests.cs index 8f09873..9c1d663 100644 --- a/tests/Novelly.Api.Tests/OpenQuestionTests.cs +++ b/tests/Novelly.Api.Tests/OpenQuestionTests.cs @@ -100,8 +100,8 @@ public class OpenQuestionTests : ServiceTestFixture var question = await Questions.CreateAsync( _projectId, new CreateOpenQuestionRequest("Where does the chapter break?")); - var resolved = await Questions.ResolveAsync( - question.Id, new ResolveOpenQuestionRequest("After the harbour burns.")); + var resolved = (await Questions.ResolveAsync( + question.Id, new ResolveOpenQuestionRequest("After the harbour burns.")))!; Assert.Multiple(() => { @@ -123,8 +123,8 @@ public class OpenQuestionTests : ServiceTestFixture question.Id, new ResolveOpenQuestionRequest("After the harbour burns.", AppendToNotes: true)); - var chapter = await Chapters.GetAsync(_chapterId); - var character = await Characters.GetAsync(_characterId); + var chapter = (await Chapters.GetAsync(_chapterId))!; + var character = (await Characters.GetAsync(_characterId))!; Assert.Multiple(() => { @@ -143,7 +143,7 @@ public class OpenQuestionTests : ServiceTestFixture await Questions.ResolveAsync(question.Id, new ResolveOpenQuestionRequest("After the harbour.")); - Assert.That((await Chapters.GetAsync(_chapterId)).Notes, Is.Null); + Assert.That((await Chapters.GetAsync(_chapterId))!.Notes, Is.Null); } [Test] @@ -155,8 +155,8 @@ public class OpenQuestionTests : ServiceTestFixture await Questions.ResolveAsync( question.Id, new ResolveOpenQuestionRequest("After the harbour.", AppendToNotes: true)); - var reopened = await Questions.ReopenAsync(question.Id); - var chapter = await Chapters.GetAsync(_chapterId); + var reopened = (await Questions.ReopenAsync(question.Id))!; + var chapter = (await Chapters.GetAsync(_chapterId))!; Assert.Multiple(() => { @@ -172,8 +172,8 @@ public class OpenQuestionTests : ServiceTestFixture var question = await Questions.CreateAsync(_projectId, new CreateOpenQuestionRequest( "Where does the chapter break?", ChapterId: _chapterId, CharacterId: _characterId)); - var detached = await Questions.UpdateAsync( - question.Id, new UpdateOpenQuestionRequest(ClearChapter: true)); + var detached = (await Questions.UpdateAsync( + question.Id, new UpdateOpenQuestionRequest(ClearChapter: true)))!; Assert.Multiple(() => { @@ -192,7 +192,7 @@ public class OpenQuestionTests : ServiceTestFixture await Chapters.DeleteAsync(_chapterId); - var survivor = await Questions.GetAsync(question.Id); + var survivor = (await Questions.GetAsync(question.Id))!; Assert.Multiple(() => { @@ -212,9 +212,7 @@ public class OpenQuestionTests : ServiceTestFixture Assert.Multiple(async () => { Assert.That(await Questions.ListAsync(_projectId, includeResolved: true), Is.Empty); - Assert.That( - async () => await Questions.GetAsync(question.Id), - Throws.TypeOf()); + Assert.That(await Questions.GetAsync(question.Id), Is.Null); }); } diff --git a/tests/Novelly.Api.Tests/ProjectDataTests.cs b/tests/Novelly.Api.Tests/ProjectDataTests.cs index 2d99db6..9310822 100644 --- a/tests/Novelly.Api.Tests/ProjectDataTests.cs +++ b/tests/Novelly.Api.Tests/ProjectDataTests.cs @@ -19,7 +19,7 @@ public class ProjectDataTests : ServiceTestFixture var id = (await Projects.CreateAsync( new CreateProjectRequest("Draft", Genre: "Fantasy", Logline: "A cartographer goes to sea."))).Id; - var afterPartialUpdate = await Projects.UpdateAsync(id, new UpdateProjectRequest(Title: "The Salt Road")); + var afterPartialUpdate = (await Projects.UpdateAsync(id, new UpdateProjectRequest(Title: "The Salt Road")))!; Assert.Multiple(() => { @@ -28,7 +28,7 @@ public class ProjectDataTests : ServiceTestFixture Assert.That(afterPartialUpdate.Logline, Is.EqualTo("A cartographer goes to sea.")); }); - var afterClear = await Projects.UpdateAsync(id, new UpdateProjectRequest(Genre: "")); + var afterClear = (await Projects.UpdateAsync(id, new UpdateProjectRequest(Genre: "")))!; Assert.Multiple(() => { @@ -63,12 +63,12 @@ public class ProjectDataTests : ServiceTestFixture Assert.That(scene.WordCount, Is.EqualTo(5)); - var rewritten = await Scenes.UpdateAsync(scene.Id, new UpdateSceneRequest( - Prose: "Now\nthere are seven words in total")); + var rewritten = (await Scenes.UpdateAsync(scene.Id, new UpdateSceneRequest( + Prose: "Now\nthere are seven words in total")))!; Assert.That(rewritten.WordCount, Is.EqualTo(7)); - var cleared = await Scenes.UpdateAsync(scene.Id, new UpdateSceneRequest(Prose: "")); + var cleared = (await Scenes.UpdateAsync(scene.Id, new UpdateSceneRequest(Prose: "")))!; Assert.Multiple(() => { @@ -85,7 +85,7 @@ public class ProjectDataTests : ServiceTestFixture var scene = await Scenes.CreateAsync(chapter.Id, new CreateSceneRequest( "The dock at dawn", Prose: "The tide came in slow.")); - var updated = await Scenes.UpdateAsync(scene.Id, new UpdateSceneRequest(Status: DraftStatus.Revised)); + var updated = (await Scenes.UpdateAsync(scene.Id, new UpdateSceneRequest(Status: DraftStatus.Revised)))!; Assert.Multiple(() => { @@ -138,8 +138,8 @@ public class ProjectDataTests : ServiceTestFixture var ines = await Characters.CreateAsync(projectId, new CreateCharacterRequest("Ines")); var mara = await Characters.CreateAsync(projectId, new CreateCharacterRequest("Mara")); - var updated = await Characters.AddRelationshipAsync( - ines.Id, new CreateRelationshipRequest(mara.Id, "sister", "Estranged since the fire.")); + var updated = (await Characters.AddRelationshipAsync( + ines.Id, new CreateRelationshipRequest(mara.Id, "sister", "Estranged since the fire.")))!; Assert.Multiple(() => { @@ -149,8 +149,6 @@ public class ProjectDataTests : ServiceTestFixture } [Test] - public void Reading_a_missing_project_reports_not_found() => - Assert.That( - async () => await Projects.GetAsync(Guid.NewGuid()), - Throws.TypeOf()); + public async Task Reading_a_missing_project_returns_null_rather_than_throwing() => + Assert.That(await Projects.GetAsync(Guid.NewGuid()), Is.Null); } diff --git a/tests/Novelly.Api.Tests/ServiceTestFixture.cs b/tests/Novelly.Api.Tests/ServiceTestFixture.cs index d4d4db9..3d07a8c 100644 --- a/tests/Novelly.Api.Tests/ServiceTestFixture.cs +++ b/tests/Novelly.Api.Tests/ServiceTestFixture.cs @@ -52,14 +52,22 @@ public abstract class ServiceTestFixture ArcLogs = new CapturingLogger(); QuestionLogs = new CapturingLogger(); - Tags = new TagService(Db.Context, TagLogs); - Projects = new ProjectService(Db.Context, ProjectLogs); - Characters = new CharacterService(Db.Context, Tags, CharacterLogs); - Chapters = new ChapterService(Db.Context, Tags, ChapterLogs); - Scenes = new SceneService(Db.Context, SceneLogs); - Beats = new BeatService(Db.Context, Tags, BeatLogs); - Arcs = new CharacterArcService(Db.Context, ArcLogs); - Questions = new OpenQuestionService(Db.Context, QuestionLogs); + Tags = new TagService(Db.Context, TagLogs, new CreateTagRequestValidator(), new UpdateTagRequestValidator()); + Projects = new ProjectService(Db.Context, ProjectLogs, new CreateProjectRequestValidator(), new UpdateProjectRequestValidator()); + Characters = new CharacterService( + Db.Context, Tags, CharacterLogs, + new CreateCharacterRequestValidator(), new UpdateCharacterRequestValidator(), new CreateRelationshipRequestValidator()); + Chapters = new ChapterService(Db.Context, Tags, ChapterLogs, new CreateChapterRequestValidator(), new UpdateChapterRequestValidator()); + Scenes = new SceneService(Db.Context, SceneLogs, new CreateSceneRequestValidator(), new UpdateSceneRequestValidator()); + Beats = new BeatService( + Db.Context, Tags, BeatLogs, + new CreateBeatRequestValidator(), new UpdateBeatRequestValidator(), new ReorderBeatsRequestValidator()); + Arcs = new CharacterArcService( + Db.Context, ArcLogs, + new CreateArcStageRequestValidator(), new UpdateArcStageRequestValidator(), new ReorderArcStagesRequestValidator()); + Questions = new OpenQuestionService( + Db.Context, QuestionLogs, + new CreateOpenQuestionRequestValidator(), new UpdateOpenQuestionRequestValidator(), new ResolveOpenQuestionRequestValidator()); OnSetUp(); } diff --git a/tests/Novelly.Api.Tests/TagServiceTests.cs b/tests/Novelly.Api.Tests/TagServiceTests.cs index 277c119..9b85d2f 100644 --- a/tests/Novelly.Api.Tests/TagServiceTests.cs +++ b/tests/Novelly.Api.Tests/TagServiceTests.cs @@ -54,8 +54,8 @@ public class TagServiceTests : ServiceTestFixture var character = await Characters.CreateAsync( _projectId, new CreateCharacterRequest("Ines", Tags: ["betrayal", "the sea"])); - var updated = await Characters.UpdateAsync( - character.Id, new UpdateCharacterRequest(Tags: ["the sea", "maps"])); + var updated = (await Characters.UpdateAsync( + character.Id, new UpdateCharacterRequest(Tags: ["the sea", "maps"])))!; Assert.That(updated.Tags.Select(t => t.Name), Is.EquivalentTo(new[] { "the sea", "maps" })); } @@ -66,8 +66,8 @@ public class TagServiceTests : ServiceTestFixture var character = await Characters.CreateAsync( _projectId, new CreateCharacterRequest("Ines", Tags: ["betrayal"])); - var updated = await Characters.UpdateAsync( - character.Id, new UpdateCharacterRequest(Occupation: "Cartographer")); + var updated = (await Characters.UpdateAsync( + character.Id, new UpdateCharacterRequest(Occupation: "Cartographer")))!; Assert.Multiple(() => { @@ -88,7 +88,7 @@ public class TagServiceTests : ServiceTestFixture await Beats.CreateAsync(chapter.Id, new CreateBeatRequest("Unrelated beat")); var tagId = (await Tags.ListAsync(_projectId)).Single().Id; - var references = await Tags.GetReferencesAsync(tagId); + var references = (await Tags.GetReferencesAsync(tagId))!; Assert.Multiple(() => { @@ -165,7 +165,7 @@ public class TagServiceTests : ServiceTestFixture await Tags.DeleteAsync(tagId); - var survivor = await Characters.GetAsync(character.Id); + var survivor = (await Characters.GetAsync(character.Id))!; Assert.Multiple(() => {