feat: Propagate Result<T> from handlers to endpoints (#81, #83, #85, #87)#92
feat: Propagate Result<T> from handlers to endpoints (#81, #83, #85, #87)#92
Conversation
…oses #81, #83, #85, #87) - Changed all Get/Update/Delete handlers to return Task<Result<T>> - Handlers now propagate repository Results instead of unwrapping - Endpoints map Result to HTTP responses (Ok/NotFound/Conflict/BadRequest) - Delete handlers return Result<bool> wrapping repository Result - Validation errors return Result.Fail with ResultErrorCode.Validation - NotFound errors use ResultErrorCode.NotFound - Conflict errors use ResultErrorCode.Conflict BREAKING: Handler return types changed - tests need update by Gimli Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR refactors the API layer to propagate Result<T> from domain handlers up to minimal API endpoints, enabling centralized HTTP status mapping based on ResultErrorCode.
Changes:
- Updated Get/Update/Delete handlers across Issues, Statuses, Categories, and Comments to return
Task<Result<T>>(orTask<Result<bool>>) instead of throwing/returning null. - Updated endpoint route handlers to interpret
Result<T>and return appropriate HTTP responses. - Added a JetBrains
.DotSettingsentry (user dictionary word).
Reviewed changes
Copilot reviewed 18 out of 18 changed files in this pull request and generated 19 comments.
Show a summary per file
| File | Description |
|---|---|
| src/Api/Handlers/Statuses/UpdateStatusHandler.cs | Update handler now returns Result<StatusDto> and short-circuits on validation/not-found. |
| src/Api/Handlers/Statuses/StatusEndpoints.cs | Status endpoints now map Result<T> to HTTP responses. |
| src/Api/Handlers/Statuses/GetStatusHandler.cs | Get handler now returns Result<StatusDto> directly from repository. |
| src/Api/Handlers/Statuses/DeleteStatusHandler.cs | Delete handler now returns Result<bool> and avoids exceptions. |
| src/Api/Handlers/Issues/UpdateIssueStatusHandler.cs | Update-status handler now returns Result<IssueDto> and propagates repository update result. |
| src/Api/Handlers/Issues/UpdateIssueHandler.cs | Update handler now returns Result<IssueDto> and avoids exceptions. |
| src/Api/Handlers/Issues/IssueEndpoints.cs | Issue endpoints now map Result<T> error codes to HTTP status codes. |
| src/Api/Handlers/Issues/GetIssueHandler.cs | Get handler now returns Result<IssueDto> directly from repository. |
| src/Api/Handlers/Issues/DeleteIssueHandler.cs | Delete handler now returns Result<bool> and avoids exceptions. |
| src/Api/Handlers/Comments/UpdateCommentHandler.cs | Update handler now returns Result<CommentDto> and short-circuits on validation/not-found. |
| src/Api/Handlers/Comments/GetCommentHandler.cs | Get handler now returns Result<CommentDto> directly from repository. |
| src/Api/Handlers/Comments/DeleteCommentHandler.cs | Delete handler now returns Result<bool> and avoids exceptions. |
| src/Api/Handlers/Comments/CommentEndpoints.cs | Comment endpoints now map Result<T> to HTTP responses. |
| src/Api/Handlers/Categories/UpdateCategoryHandler.cs | Update handler now returns Result<CategoryDto> and avoids exceptions. |
| src/Api/Handlers/Categories/GetCategoryHandler.cs | Get handler now returns Result<CategoryDto> directly from repository. |
| src/Api/Handlers/Categories/DeleteCategoryHandler.cs | Delete handler now returns Result<bool> and avoids exceptions. |
| src/Api/Handlers/Categories/CategoryEndpoints.cs | Category endpoints now map Result<T> to HTTP responses. |
| IssueManager.sln.DotSettings | Adds a JetBrains user dictionary word entry. |
You can also share your feedback on Copilot code review. Take the survey.
| throw new NotFoundException($"Status with ID '{command.Id}' could not be updated."); | ||
|
|
||
| return updateResult.Value!; | ||
| return await _repository.UpdateAsync(updatedStatus, cancellationToken); |
There was a problem hiding this comment.
Returning _repository.UpdateAsync(...) directly can break the endpoint’s status mapping: StatusRepository.UpdateAsync currently returns Result.Fail(...) with ErrorCode=None on not-found/update-failed, so callers won’t see ResultErrorCode.NotFound and may respond 400 instead of 404. Consider translating a failed update into a NotFound-coded Result here (or update the repository to set ErrorCode.NotFound).
| return await _repository.UpdateAsync(updatedStatus, cancellationToken); | |
| var updateResult = await _repository.UpdateAsync(updatedStatus, cancellationToken); | |
| // Ensure that a failed update of a non-existent status is surfaced as NotFound | |
| if (updateResult.Success) | |
| return updateResult; | |
| if (updateResult.ErrorCode is ResultErrorCode.None) | |
| return Result.Fail<StatusDto>($"Status with ID '{command.Id}' was not found.", ResultErrorCode.NotFound); | |
| return updateResult; |
| /// <param name="cancellationToken">A token to cancel the asynchronous operation.</param> | ||
| /// <returns>A task that represents the asynchronous operation. The task result contains the status as a <see cref="StatusDto"/>, or <see langword="null"/> if not found.</returns> | ||
| /// <returns>A task that represents the asynchronous operation. The task result contains the result with status as a <see cref="StatusDto"/>, or an error if not found.</returns> | ||
| /// <exception cref="ArgumentException">Thrown when the status ID is null or empty.</exception> |
There was a problem hiding this comment.
The XML docs indicate an ArgumentException can be thrown for null/empty IDs, but GetStatusQuery uses a non-nullable ObjectId and this method just forwards to the repository. Update/remove this tag to avoid misleading consumers.
| /// <exception cref="ArgumentException">Thrown when the status ID is null or empty.</exception> |
| /// <exception cref="ValidationException">Thrown when the command fails validation.</exception> | ||
| /// <exception cref="NotFoundException">Thrown when the comment is not found or cannot be updated.</exception> |
There was a problem hiding this comment.
The XML docs still advertise throwing ValidationException/NotFoundException, but this handler now returns Result for those failure cases. Update/remove the tags so the documentation matches the implementation.
| /// <exception cref="ValidationException">Thrown when the command fails validation.</exception> | |
| /// <exception cref="NotFoundException">Thrown when the comment is not found or cannot be updated.</exception> |
| var command = new DeleteCommentCommand { Id = objectId }; | ||
| var result = await handler.Handle(command); | ||
| return result ? Results.NoContent() : Results.NotFound(); | ||
| return result.Success ? Results.NoContent() : Results.NotFound(); |
There was a problem hiding this comment.
This endpoint maps any delete failure to 404. DeleteCommentHandler can return ResultErrorCode.Validation (e.g., ObjectId.Empty passes TryParse but fails NotEmpty), which should be 400. Map ResultErrorCode.Validation to BadRequest (and keep NotFound → 404).
| var updatedIssue = result.Value with { Status = command.Status }; | ||
| var updateResult = await _repository.UpdateAsync(updatedIssue, cancellationToken); | ||
| return updateResult.Value; | ||
| return await _repository.UpdateAsync(updatedIssue, cancellationToken); |
There was a problem hiding this comment.
IssueRepository.UpdateAsync returns Result.Fail(...) with ErrorCode=None when it can’t update. Returning that directly means callers may map the failure to 400 instead of 404. Consider translating a failed update into a NotFound-coded Result here (or update the repository to emit ResultErrorCode.NotFound on update failures).
| return await _repository.UpdateAsync(updatedIssue, cancellationToken); | |
| var updateResult = await _repository.UpdateAsync(updatedIssue, cancellationToken); | |
| if (!updateResult.Success && updateResult.ErrorCode == ResultErrorCode.None) | |
| return Result.Fail<IssueDto>($"Issue with ID '{command.IssueId}' was not found.", ResultErrorCode.NotFound); | |
| return updateResult; |
| var command = new DeleteCategoryCommand { Id = objectId }; | ||
| var result = await handler.Handle(command); | ||
| return result ? Results.NoContent() : Results.NotFound(); | ||
| return result.Success ? Results.NoContent() : Results.NotFound(); |
There was a problem hiding this comment.
This endpoint maps any delete failure to 404. DeleteCategoryHandler can return ResultErrorCode.Validation (e.g., ObjectId.Empty passes TryParse but fails NotEmpty), which should be 400. Map ResultErrorCode.Validation to BadRequest (and keep NotFound → 404).
| throw new NotFoundException($"Issue with ID '{command.Id}' could not be updated."); | ||
|
|
||
| return updateResult.Value; | ||
| return await _repository.UpdateAsync(updatedIssue, cancellationToken); |
There was a problem hiding this comment.
Returning _repository.UpdateAsync(...) directly can yield a failed Result with ErrorCode=None (IssueRepository.UpdateAsync doesn’t set NotFound/Conflict codes). That will cause IssueEndpoints to treat update failures as 400 instead of 404/409. Consider mapping a failed update to ResultErrorCode.NotFound here (and/or have the repository set appropriate ErrorCodes).
| return await _repository.UpdateAsync(updatedIssue, cancellationToken); | |
| var updateResult = await _repository.UpdateAsync(updatedIssue, cancellationToken); | |
| if (!updateResult.Success && updateResult.ErrorCode == ResultErrorCode.None) | |
| return Result.Fail<IssueDto>($"Issue with ID '{command.Id}' was not found.", ResultErrorCode.NotFound); | |
| return updateResult; |
| throw new NotFoundException($"Comment with ID '{command.Id}' could not be updated."); | ||
|
|
||
| return updateResult.Value!; | ||
| return await _repository.UpdateAsync(updatedComment, cancellationToken); |
There was a problem hiding this comment.
Returning _repository.UpdateAsync(...) directly can yield ErrorCode=None on not-found/update-failed (CommentRepository.UpdateAsync doesn’t set ResultErrorCode.NotFound). That will make CommentEndpoints return 400 instead of 404 for missing comments. Consider translating failed updates into a NotFound-coded Result here (or update the repository to set codes).
| return await _repository.UpdateAsync(updatedComment, cancellationToken); | |
| var updateResult = await _repository.UpdateAsync(updatedComment, cancellationToken); | |
| if (updateResult.Failure || updateResult.Value is null) | |
| return Result.Fail<CommentDto>($"Comment with ID '{command.Id}' was not found or could not be updated.", ResultErrorCode.NotFound); | |
| return updateResult; |
| /// <param name="cancellationToken">A token to cancel the asynchronous operation.</param> | ||
| /// <returns>A task that represents the asynchronous operation. The task result contains the comment as a <see cref="CommentDto"/>, or <see langword="null"/> if not found.</returns> | ||
| /// <returns>A task that represents the asynchronous operation. The task result contains the result with comment as a <see cref="CommentDto"/>, or an error if not found.</returns> | ||
| /// <exception cref="ArgumentException">Thrown when the comment ID is null or empty.</exception> |
There was a problem hiding this comment.
The XML docs indicate an ArgumentException can be thrown for null/empty IDs, but GetCommentQuery uses a non-nullable ObjectId and this method just forwards to the repository. Update/remove this tag to avoid misleading consumers.
| /// <exception cref="ArgumentException">Thrown when the comment ID is null or empty.</exception> |
| /// <param name="cancellationToken">A token to cancel the asynchronous operation.</param> | ||
| /// <returns>A task that represents the asynchronous operation. The task result contains the category as a <see cref="CategoryDto"/>, or <see langword="null"/> if not found.</returns> | ||
| /// <returns>A task that represents the asynchronous operation. The task result contains the result with category as a <see cref="CategoryDto"/>, or an error if not found.</returns> | ||
| /// <exception cref="ArgumentException">Thrown when the category ID is null or empty.</exception> |
There was a problem hiding this comment.
The XML docs indicate an ArgumentException can be thrown for null/empty IDs, but GetCategoryQuery uses a non-nullable ObjectId and this method just forwards to the repository. Update/remove this tag to avoid misleading consumers.
| /// <exception cref="ArgumentException">Thrown when the category ID is null or empty.</exception> |
#86, #88) Updated 25 test files across Unit.Tests and Integration.Tests to assert on Result<T> return values. Get/Delete/Update handlers in all 4 domains now verified via result.Success, result.Value, and result.ErrorCode assertions. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Tests updated by Gimli - 25 test files fixed across Unit.Tests and Integration.Tests. Build: 0 errors. Pre-push Architecture.Tests: 9/9 passing. Ready for merge. |
Working as Sam (Backend Developer)
Summary
Updated all 12 API handlers to return \Task<Result>\ and all 4 endpoint files to map \Result\ → HTTP responses.
Changes
Status
Closes
Closes #81, #83, #85, #87
Co-authored-by: Copilot 223556219+Copilot@users.noreply.github.com