Refactor/identity groups code review#1209
Closed
cesarcastrocuba wants to merge 17 commits intofullstackhero:developfrom
Closed
Refactor/identity groups code review#1209cesarcastrocuba wants to merge 17 commits intofullstackhero:developfrom
cesarcastrocuba wants to merge 17 commits intofullstackhero:developfrom
Conversation
added 9 commits
February 21, 2026 18:48
… and Auditing features ## Changes ### Identity - Groups feature **feat: Group entity implements IAuditableEntity** - Add IAuditableEntity interface to Group domain entity - Rename CreatedAt/ModifiedAt (DateTime) to CreatedOnUtc/LastModifiedOnUtc (DateTimeOffset) to align with FSH BuildingBlocks IAuditableEntity contract - Update GroupConfiguration.cs with HasColumnName() mapping to preserve DB column names (CreatedAt, ModifiedAt, ModifiedBy) - no migration required **fix: encapsulate ISoftDeletable mutation in Group entity** - Change IsDeleted, DeletedOnUtc, DeletedBy from public set to private set - Add Group.Delete(string? deletedBy) domain method to encapsulate soft-delete logic - Update DeleteGroupCommandHandler to call group.Delete() instead of directly mutating soft-delete properties **perf: optimize GetGroupByIdQueryHandler to single DB round-trip** - Replace 3 separate DB queries (group + member count + role names) with a single EF Select() projection - Eliminates N+1 risk on member count and role name lookups **perf: optimize CreateGroupCommandHandler to avoid double role query** - Fetch role Id+Name together in a single query for validation - Reuse result for both invalid-role detection and response DTO construction - Eliminates one DB round-trip per CreateGroup request **fix: wrap all Group endpoints with TypedResults** - CreateGroupEndpoint, GetGroupsEndpoint, GetGroupByIdEndpoint, UpdateGroupEndpoint, AddUsersToGroupEndpoint, GetGroupMembersEndpoint: TypedResults.Ok() - DeleteGroupEndpoint, RemoveUserFromGroupEndpoint: TypedResults.NoContent() (HTTP 204) **fix: update GroupDto and all handlers to use CreatedOnUtc** - GroupDto: DateTime CreatedAt -> DateTimeOffset CreatedOnUtc - GetGroupsQueryHandler, UpdateGroupCommandHandler, GetUserGroupsQueryHandler updated ### Auditing **fix: wrap all Auditing endpoints with TypedResults.Ok()** - GetAuditByIdEndpoint, GetAuditSummaryEndpoint, GetAuditsEndpoint, GetAuditsByCorrelationEndpoint, GetAuditsByTraceEndpoint, GetExceptionAuditsEndpoint, GetSecurityAuditsEndpoint ## Verification - Build: 0 errors - Tests: 462 passed, 1 pre-existing failure (RefreshTokenCommandHandlerTests, NullReferenceException in test setup - unrelated to these changes)
…ix Blazor client
The NSwag-generated Blazor client (Generated.cs) uses the OpenAPI response
codes declared via .Produces() to determine which HTTP status codes to handle.
Without .Produces(Status204NoContent), NSwag defaults to expecting 200 OK and
throws ApiException when the endpoint correctly returns 204 NoContent.
This pattern is already established in ResetTenantThemeEndpoint, which works
correctly because it declares .Produces(StatusCodes.Status204NoContent).
## Endpoints fixed
**Groups (introduced in previous commit):**
- DeleteGroupEndpoint: add Produces(204/401/403)
- RemoveUserFromGroupEndpoint: add Produces(204/401/403)
**Pre-existing bugs (same root cause, fixed opportunistically):**
- DeleteRoleEndpoint: add Produces(204/401/403)
- DeleteUserEndpoint: add Produces(204/401/403)
## Root cause
Without .Produces(StatusCodes.Status204NoContent), the OpenAPI spec documents
the response as 200 OK. NSwag then generates:
if (status_ == 200) { return; }
else { throw new ApiException(...); } // throws on 204
With .Produces(StatusCodes.Status204NoContent), NSwag generates:
if (status_ == 204) { return; } // correctly handles NoContent
…on delete endpoints The previous Generated.cs was generated when delete endpoints returned 200 OK. After adding TypedResults.NoContent() and .Produces(Status204NoContent) to all delete endpoints, the client must be regenerated so NSwag emits the correct status code checks. Changes in Generated.cs: - RolesDeleteAsync: status_ == 200 -> status_ == 204 - UsersDeleteAsync: status_ == 200 -> status_ == 204 - GroupsDeleteAsync: status_ == 200 -> status_ == 204 - RemoveUserFromGroupAsync: status_ == 200 -> status_ == 204 Generated with: NSwag.ConsoleCore v14.6.3 from https://localhost:7030/openapi/v1.json
…andling on delete endpoints" This reverts commit 087b3e5.
…ontent The NSwag-generated client only handled HTTP 200 for delete operations. Since the API now correctly returns 204 NoContent (TypedResults.NoContent), the Blazor client was throwing ApiException on every delete. Minimal surgical patch - 4 lines changed, no structural modifications: - RolesDeleteAsync: status_ == 200 -> status_ == 200 || status_ == 204 - UsersDeleteAsync: status_ == 200 -> status_ == 200 || status_ == 204 - GroupsDeleteAsync: status_ == 200 -> status_ == 200 || status_ == 204 - RemoveUserFromGroupAsync: status_ == 200 -> status_ == 200 || status_ == 204 Note: a full client regeneration was attempted but changed the client class structure, breaking existing type references. This patch is the safe alternative.
…k() with TODO The NSwag-generated GroupsDeleteAsync and RemoveUserFromGroupAsync return Task<Unit> and attempt to deserialize the response body. With TypedResults.NoContent() (HTTP 204 - empty body), this causes: 'Could not deserialize the response body stream as Unit. Status: 204' Root cause: the generated client contract uses Task<Unit> (expects a body) instead of Task (void). Fixing this requires regenerating Generated.cs with the original NSwag configuration, which is not yet available in the repo. Changes: - DeleteGroupEndpoint: NoContent() -> Ok(result) + Produces(200) + TODO comment - RemoveUserFromGroupEndpoint: NoContent() -> Ok(result) + Produces(200) + TODO comment - Generated.cs GroupsDeleteAsync: revert status_==200||204 back to status_==200 - Generated.cs RemoveUserFromGroupAsync: revert status_==200||204 back to status_==200 Note: DeleteRoleEndpoint and DeleteUserEndpoint correctly return NoContent() (their generated methods are Task void - no body deserialization) and remain properly fixed with the status_==200||204 patch in Generated.cs.
… future NoContent migration For consistency, all 4 delete endpoints now return TypedResults.Ok(result) while the Blazor NSwag client cannot properly handle 204 NoContent. Until the client is regenerated with the original NSwag configuration, both endpoints return 200 OK to avoid Blazor client failures. - DeleteRoleEndpoint: NoContent() -> Ok(result), Produces(200/401/403) + TODO - DeleteUserEndpoint: NoContent() -> Ok(result), Produces(200/401/403) + TODO
…=200 only RolesDeleteAsync and UsersDeleteAsync were incorrectly left with status_==200||status_==204 from a previous patch attempt. Since both endpoints now return TypedResults.Ok() (HTTP 200), the client only needs to handle status 200. Reverted to match the original generated code. Generated.cs is otherwise unchanged from its original state.
…dler
ValueTuple.Create() is not translatable to SQL by the EF Core LINQ provider,
causing a runtime exception when querying roles.
Fix: project to an anonymous type inside the EF query (which EF can translate),
then convert to value tuples in memory after ToListAsync().
Before:
.Select(r => ValueTuple.Create(r.Id, r.Name!)) // throws at runtime
After:
.Select(r => new { r.Id, r.Name }) // translated to SQL
then: rawRoles.Select(r => (r.Id, r.Name!)) // in-memory conversion
…Produces attributes
…lts and Produces attributes
…lts and Produces attributes
…set NoContent for ToggleUserStatus
…t extraction in RefreshTokenCommandHandler
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
refactor(identity,auditing): code review improvements — Groups feature & Auditing endpoints
Summary
This PR applies a set of code review improvements to the Identity Groups feature and all Auditing endpoints, focusing on correctness, performance, and consistency with FSH conventions.
Changes
🏗️ Identity — Group entity
IAuditableEntityimplementationIAuditableEntityCreatedAt/ModifiedAt(DateTime) →CreatedOnUtc/LastModifiedOnUtc(DateTimeOffset) to match the FSH BuildingBlocks contractGroupConfiguration.csuses.HasColumnName()to preserve existing DB column names (CreatedAt,ModifiedAt,ModifiedBy) — no migration neededEncapsulated soft-delete logic
IsDeleted,DeletedOnUtc,DeletedBysetters changed toprivateGroup.Delete(string? deletedBy)domain methodDeleteGroupCommandHandlernow callsgroup.Delete()instead of mutating properties directly⚡ Identity — Query optimizations
GetGroupByIdQueryHandlerSelect()projection — eliminates N+1 riskCreateGroupCommandHandler🔌 TypedResults on all endpoints
All endpoints updated to return
TypedResultsfor proper OpenAPI documentation and type safety:GetGroups,GetGroupById, UpdateGroup, AddUsersToGroup,GetGroupMembers→TypedResults.Ok()TypedResults.Ok(result)withTODOcommentGetAuditById,GetAuditSummary,GetAudits,GetAuditsByCorrelation,GetAuditsByTrace,GetExceptionAudits,GetSecurityAudits→TypedResults.Ok()📋 DTO & handler updates
DateTime CreatedAt→DateTimeOffset CreatedOnUtcGetGroupsQueryHandler,UpdateGroupCommandHandler,GetUserGroupsQueryHandlerVerification
dotnet build src/FSH.Framework.slnx → 0 errors dotnet test src/FSH.Framework.slnx → 462 passed