feat: use centralized error handling #16
Conversation
…ss authentication token
|
Warning Rate limit exceeded@osm-Jatin has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 10 minutes and 0 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (9)
WalkthroughThis update introduces JWT authentication and standardized API responses across the application. Controllers now wrap all responses in a Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant UserController
participant UserService
participant TokenService
participant UserRepository
participant RoleRepository
Client->>UserController: POST /login (email, password)
UserController->>UserService: Login(email, password)
UserService->>UserRepository: GetUserByEmailAsync(email)
UserRepository-->>UserService: User
UserService->>UserService: VerifyPassword()
UserService->>TokenService: GenerateToken(User)
TokenService-->>UserService: JWT Token
UserService-->>UserController: Token
UserController-->>Client: BaseResponse<string> (token)
sequenceDiagram
participant Client
participant UserController
participant UserService
participant UserRepository
Client->>UserController: GET /users?pageNumber=1&pageSize=10
UserController->>UserService: GetAllUsersAsync(pageNumber, pageSize)
UserService->>UserRepository: GetAllUsersAsync(pageNumber, pageSize)
UserRepository-->>UserService: List<User>
UserService->>UserRepository: GetTotalUsersCountAsync()
UserRepository-->>UserService: TotalCount
UserService-->>UserController: List<User>, TotalCount
UserController-->>Client: BaseResponse<List<User>> with PaginationMetadata
Possibly related PRs
Suggested reviewers
Poem
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (16)
src/Core/Entities/DTOs/UserDTOs.cs (1)
37-44: Properly structured LoginRequestDto with appropriate validationsThe new LoginRequestDto contains appropriate validations for both email (format and length) and password (required). This supports the new JWT authentication flow mentioned in the PR summary.
Consider adding a minimum length requirement for the password similar to the UserCreateDto for consistency.
- [Required] + [Required, MinLength(8)] public string Password { get; set; } = string.Empty;src/Api/appsettings.json (1)
18-23: JWT configuration added correctly, but ensure key replacement before deploymentThe JWT configuration is well-structured with all necessary parameters. However, while using a placeholder with clear instructions is appropriate for development, it's critical to ensure this placeholder is replaced with a strong key in production environments.
Consider implementing one of these approaches for production environments:
- Use a secrets management service like Azure Key Vault, AWS Secrets Manager, or HashiCorp Vault
- Use environment variables or other secure configuration methods
- Implement validation at startup to verify that placeholder values have been replaced
This would better protect your JWT signing key while maintaining the developer-friendly placeholder in development configurations.
🧰 Tools
🪛 Gitleaks (8.21.2)
19-19: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
src/Core/Services/Interfaces/ITokenService.cs (1)
1-8: Well-designed interface with clear single responsibilityThe
ITokenServiceinterface is appropriately focused on token generation with a clean, simple signature. The implementation (seen in the provided code snippet) properly extracts user claims and configures the token parameters.Consider adding XML documentation comments to describe the method, parameters, return value, and potential exceptions (like the
InvalidOperationExceptionthrown in the implementation when JWT settings are invalid). This would make the interface more self-documenting:+ /// <summary> + /// Generates a JWT token for the specified user + /// </summary> + /// <param name="user">The user to generate a token for</param> + /// <returns>A JWT token string</returns> + /// <exception cref="InvalidOperationException">Thrown when JWT configuration is invalid</exception> string GenerateToken(User user);Also, depending on your authentication flow, you might want to consider adding token validation/verification methods to this interface if they'll be needed elsewhere in the application.
src/Core/Exceptions/DatabaseOperationException.cs (1)
1-10: Good basic custom exception implementationThe
DatabaseOperationExceptionclass follows standard exception patterns with appropriate constructors. This is part of the centralized error handling strategy mentioned in the PR objectives.Consider these enhancements to make the exception more useful:
- Add XML documentation:
+/// <summary> +/// Exception thrown when a database operation fails +/// </summary> public class DatabaseOperationException : Exception
- Consider adding contextual properties to provide more details about the failed operation:
public class DatabaseOperationException : Exception { + /// <summary> + /// Gets or sets the name of the affected entity + /// </summary> + public string? EntityName { get; set; } + + /// <summary> + /// Gets or sets the type of operation that failed + /// </summary> + public string? OperationType { get; set; } + public DatabaseOperationException() : base() { } public DatabaseOperationException(string msg) : base(msg) { } public DatabaseOperationException(string message, Exception innerException) : base(message, innerException) { } + + /// <summary> + /// Initializes a new instance with entity and operation information + /// </summary> + public DatabaseOperationException(string message, string entityName, string operationType) + : base(message) + { + EntityName = entityName; + OperationType = operationType; + } }This would allow service methods to provide more context when throwing the exception, making debugging and error reporting more effective.
src/Api/Program.cs (2)
57-103: Enhanced Swagger configuration with JWT supportThe Swagger configuration is well-structured with appropriate security definitions for JWT authentication. This improves API documentation and testing capabilities.
Consider adding a version parameter to make the Swagger version configurable:
- c.SwaggerDoc("v1", new OpenApiInfo + string apiVersion = builder.Configuration["ApiVersion"] ?? "v1"; + c.SwaggerDoc(apiVersion, new OpenApiInfo { - Title = "Dotnet Foundation", - Version = "v1", + Title = "Dotnet Foundation", + Version = apiVersion, Description = "API documentation for Dotnet Foundation" });
120-146: JWT authentication configuration addedThe JWT authentication is properly configured with validation parameters. The error handling for missing JWT key configuration at line 123 is well implemented.
You should consider adding a try-catch block when reading JWT settings to provide a more descriptive error message if any required JWT setting is missing:
// Read JWT settings IConfigurationSection jwtSettings = builder.Configuration.GetSection("JwtSettings"); -byte[] key = Encoding.UTF8.GetBytes(jwtSettings["Key"] ?? throw new InvalidOperationException("JWT Key is missing")); +try { + byte[] key = Encoding.UTF8.GetBytes(jwtSettings["Key"] ?? throw new InvalidOperationException("JWT Key is missing")); + string issuer = jwtSettings["Issuer"] ?? throw new InvalidOperationException("JWT Issuer is missing"); + string audience = jwtSettings["Audience"] ?? throw new InvalidOperationException("JWT Audience is missing");src/Core/Services/TokenService.cs (2)
22-34: JWT token creation with user claimsThe token generation logic correctly includes essential user claims and conditionally adds the role claim. This provides good security context for authenticated requests.
Consider adding more standardized claims such as JTI (JWT ID) for token uniqueness and potential revocation support:
List<Claim> claims = new List<Claim> { new(ClaimTypes.NameIdentifier, user.Id.ToString()), new(ClaimTypes.Email, user.Email), + new(JwtRegisteredClaimNames.Jti, Guid.NewGuid().ToString()), };
42-54: Security token descriptor and generationThe token descriptor is properly configured with all required properties. The token generation and serialization are implemented correctly.
Consider adding a token refresh mechanism or access/refresh token pair design for better security:
SecurityTokenDescriptor tokenDescriptor = new SecurityTokenDescriptor { Subject = new ClaimsIdentity(claims), Expires = DateTime.UtcNow.AddMinutes(minutes), + NotBefore = DateTime.UtcNow, Issuer = jwtSettings["Issuer"], Audience = jwtSettings["Audience"], SigningCredentials = credentials };src/Core/Services/Interfaces/IUserService.cs (1)
8-8: Rename toLoginAsyncfor consistency with asynchronous naming conventions
All other methods in this interface follow the*Asyncsuffix, which is the de‑facto .NET convention forTask‑returning methods. Renaming keeps the API predictable and avoids forcing callers to remember an exception.- Task<string?> Login(string email, string password); + Task<string?> LoginAsync(string email, string password);src/Api/Controllers/RoleController.cs (3)
53-65: Return 201 Created with location header for resource creation
Changing fromCreatedAtActiontoOkloses REST semantics and makes it harder for clients to discover the URL of the new resource. You can still wrap the payload inBaseResponsewhile returning status 201:return CreatedAtAction(nameof(GetRoleById), new { id = response.Data!.Id }, response);
68-81: Updates should ideally return 204 NoContent (or the updated entity) but not both
If you decide to send the updated entity, keep using 200 OK; if you prefer a lightweight acknowledgement, use 204 with an empty body. Mixing status codes and semantics across controllers can confuse API consumers. Align this controller with your UserController for consistency.
83-92: Boolean wrapper offers little value after successful deletion
A 204 NoContent without body is the common idiom for successful DELETE. Returning a wrappedbooldelivers no extra information while inflating payload size. Consider switching toNoContent()and let global exception handling carry failure cases.src/Core/Services/RoleService.cs (3)
26-36:id <= 0guard is good — add similar validation for overflow scenarios
Althoughintmax is large, passingint.MaxValuemay still reach the database and produce overflow/format issues downstream. A simple upper‑bound (e.g.,< 1_000_000) or relying on database type limits via validation attributes can harden the API.
43-50:NotFoundExceptionon empty list conflates “no data” with “bad request”
An empty page (especially for highpageNumber) is not exceptional; returning an empty list with pagination metadata is usually preferable. Throwing forces clients to treat absence of data as an error, complicating front‑end logic.
78-82: Name uniqueness check should be case‑insensitive
GetRoleByNameAsyncmay perform a case‑sensitive lookup depending on DB collation. Convert both values to a common case or delegate to a normalized column to avoid “Admin” vs “admin” duplication.src/Api/Controllers/UserController.cs (1)
21-43: Duplicate success‑wrapping logic – consider a private helper
GetUserByIdandGetUserByEmailshare identical “wrap inBaseResponse” boilerplate. A small helper (CreateSuccessResponse<T>) would keep the controller leaner and less error‑prone.No code change required for this PR, but worth adding in a follow‑up.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (21)
src/Api/Api.csproj(2 hunks)src/Api/Controllers/RoleController.cs(2 hunks)src/Api/Controllers/UserController.cs(2 hunks)src/Api/Filters/CustomExceptionFilter.cs(1 hunks)src/Api/Models/Common/BaseResponse.cs(2 hunks)src/Api/Program.cs(5 hunks)src/Api/appsettings.Development.json(1 hunks)src/Api/appsettings.json(1 hunks)src/Core/Core.csproj(1 hunks)src/Core/Entities/DTOs/UserDTOs.cs(2 hunks)src/Core/Exceptions/DatabaseOperationException.cs(1 hunks)src/Core/Repositories/Interfaces/IRoleRepository.cs(1 hunks)src/Core/Repositories/Interfaces/IUserRepository.cs(1 hunks)src/Core/Repositories/RoleRepository.cs(2 hunks)src/Core/Repositories/UserRepository.cs(1 hunks)src/Core/Services/Interfaces/IRoleService.cs(1 hunks)src/Core/Services/Interfaces/ITokenService.cs(1 hunks)src/Core/Services/Interfaces/IUserService.cs(1 hunks)src/Core/Services/RoleService.cs(3 hunks)src/Core/Services/TokenService.cs(1 hunks)src/Core/Services/UserService.cs(1 hunks)
🧰 Additional context used
🧠 Learnings (3)
src/Api/appsettings.Development.json (1)
Learnt from: osm-Jatin
PR: OsmosysSoftware/dotnet-foundation-v2#8
File: src/Api/appsettings.Development.json:19-19
Timestamp: 2025-03-05T07:10:16.015Z
Learning: In the .NET Foundation project, JWT key placeholders with clear instructions (like "PLACEHOLDER_REPLACE_WITH_STRONG_KEY_MIN_32CHARS_BEFORE_USE") are intentionally kept in development configuration files (appsettings.Development.json) to guide developers on required replacements.
src/Api/appsettings.json (1)
Learnt from: osm-Jatin
PR: OsmosysSoftware/dotnet-foundation-v2#8
File: src/Api/appsettings.Development.json:19-19
Timestamp: 2025-03-05T07:10:16.015Z
Learning: In the .NET Foundation project, JWT key placeholders with clear instructions (like "PLACEHOLDER_REPLACE_WITH_STRONG_KEY_MIN_32CHARS_BEFORE_USE") are intentionally kept in development configuration files (appsettings.Development.json) to guide developers on required replacements.
src/Core/Services/RoleService.cs (1)
Learnt from: osm-Jatin
PR: OsmosysSoftware/dotnet-foundation-v2#5
File: src/Core/Services/RoleService.cs:56-60
Timestamp: 2025-03-02T12:30:17.440Z
Learning: In future PRs, the error handling in RoleService.cs will be updated to throw exceptions with descriptive messages instead of returning null values for clearer error reporting.
🧬 Code Graph Analysis (6)
src/Core/Services/Interfaces/ITokenService.cs (1)
src/Core/Services/TokenService.cs (1)
GenerateToken(20-54)
src/Core/Services/TokenService.cs (1)
src/Core/Services/Interfaces/ITokenService.cs (1)
GenerateToken(7-7)
src/Core/Services/Interfaces/IRoleService.cs (1)
src/Core/Entities/DTOs/RoleDTOs.cs (3)
RoleResponseDto(20-23)RoleCreateDto(12-14)RoleUpdateDto(16-18)
src/Api/Filters/CustomExceptionFilter.cs (5)
src/Api/Models/Common/BaseResponse.cs (2)
BaseResponse(11-48)BaseResponse(17-17)src/Core/Exceptions/NotFoundException.cs (4)
NotFoundException(3-16)NotFoundException(5-7)NotFoundException(9-11)NotFoundException(13-15)src/Core/Exceptions/BadRequestException.cs (4)
BadRequestException(3-16)BadRequestException(5-7)BadRequestException(9-11)BadRequestException(13-15)src/Core/Exceptions/AlreadyExistsException.cs (4)
AlreadyExistsException(3-16)AlreadyExistsException(5-7)AlreadyExistsException(9-11)AlreadyExistsException(13-15)src/Core/Exceptions/DatabaseOperationException.cs (4)
DatabaseOperationException(3-10)DatabaseOperationException(5-5)DatabaseOperationException(7-7)DatabaseOperationException(9-9)
src/Core/Services/Interfaces/IUserService.cs (1)
src/Core/Entities/DTOs/UserDTOs.cs (3)
UserResponseDto(28-35)UserCreateDto(20-24)UserUpdateDto(26-26)
src/Core/Services/UserService.cs (8)
src/Core/Services/RoleService.cs (6)
Task(24-36)Task(38-41)Task(43-51)Task(53-68)Task(70-91)Task(93-107)src/Core/Services/Interfaces/IUserService.cs (8)
Task(8-8)Task(9-9)Task(10-10)Task(11-11)Task(12-12)Task(13-13)Task(14-14)Task(15-15)src/Core/Repositories/Interfaces/IUserRepository.cs (2)
Task(7-7)Task(8-8)src/Core/Entities/DTOs/UserDTOs.cs (3)
UserResponseDto(28-35)UserCreateDto(20-24)UserUpdateDto(26-26)src/Core/Exceptions/BadRequestException.cs (4)
BadRequestException(3-16)BadRequestException(5-7)BadRequestException(9-11)BadRequestException(13-15)src/Core/Exceptions/NotFoundException.cs (4)
NotFoundException(3-16)NotFoundException(5-7)NotFoundException(9-11)NotFoundException(13-15)src/Core/Exceptions/AlreadyExistsException.cs (4)
AlreadyExistsException(3-16)AlreadyExistsException(5-7)AlreadyExistsException(9-11)AlreadyExistsException(13-15)src/Core/Entities/Models/User.cs (1)
SetRole(46-50)
🪛 Gitleaks (8.21.2)
src/Api/appsettings.json
19-19: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🔇 Additional comments (29)
src/Api/appsettings.Development.json (1)
18-23: JWT configuration properly structuredThe JWT configuration follows the project's established pattern with a properly formatted placeholder for the key that clearly indicates it needs replacement before use. This approach aligns with the project's practice of using instructive placeholders in development configuration files.
A 60-minute expiry time is reasonable for development, though consider implementing refresh token functionality if longer sessions are needed in production rather than extending this timeout.
src/Core/Entities/DTOs/UserDTOs.cs (1)
16-16: Improved RoleId validation with range checkAdding the Range validation attribute ensures that the RoleId is always greater than zero, which prevents invalid foreign key references and aligns with the centralized error handling approach by failing fast with descriptive validation errors.
src/Api/Models/Common/BaseResponse.cs (2)
53-71: Improved model validation error handlingThe updated
GenerateErrorResponsemethod now directly accepts aModelStateDictionaryparameter, making it cleaner and more straightforward to use in controllers. This change aligns with the PR's goal of centralizing error handling by providing a standard way to format validation errors.
77-108: Enhanced pagination metadata with page-based navigationThe pagination metadata has been updated to use a page-based approach instead of cursor-based pagination, which is more appropriate for the standardized API responses mentioned in the PR. The addition of navigation indicators (
HasPreviousPage,HasNextPage) and page counts will help clients implement proper pagination UI.src/Core/Core.csproj (2)
11-11: Added JWT Bearer authentication packageAdding the
Microsoft.AspNetCore.Authentication.JwtBearerpackage is appropriate for implementing JWT-based authentication as mentioned in the AI summary.
14-14:❓ Verification inconclusive
Added JWT token handling package
The
System.IdentityModel.Tokens.Jwtpackage is required for generating and validating JWT tokens and complements the JWT Bearer authentication package.However, there's a potential version compatibility issue to verify.
🏁 Script executed:
#!/bin/bash # Check if the JWT package versions are compatible with each other # Microsoft.AspNetCore.Authentication.JwtBearer 8.0.2 vs System.IdentityModel.Tokens.Jwt 8.6.0 # Query NuGet API for package dependencies echo "Checking JWT package versions compatibility..." curl -s "https://api.nuget.org/v3/registration5-semver1/microsoft.aspnetcore.authentication.jwtbearer/8.0.2.json" | jq -r '.catalogEntry.dependencyGroups[].dependencies[] | select(.id | contains("System.IdentityModel.Tokens.Jwt")) | .range'Length of output: 420
Let’s pull the JwtBearer dependencies from the NuGet registration index instead:
🏁 Script executed:
#!/bin/bash echo "Checking System.IdentityModel.Tokens.Jwt version range required by JwtBearer 8.0.2..." curl -s "https://api.nuget.org/v3/registration5-semver1/microsoft.aspnetcore.authentication.jwtbearer/index.json" \ | jq -r ' .items[].items[] | select(.catalogEntry.version=="8.0.2") | .catalogEntry.dependencyGroups[].dependencies[]? | select(.id=="System.IdentityModel.Tokens.Jwt") | .range 'Length of output: 565
Let’s inspect the package’s nuspec directly to see if there’s an explicit dependency on System.IdentityModel.Tokens.Jwt:
🏁 Script executed:
#!/bin/bash echo "Fetching nuspec for Microsoft.AspNetCore.Authentication.JwtBearer 8.0.2..." curl -s https://api.nuget.org/v3-flatcontainer/microsoft.aspnetcore.authentication.jwtbearer/8.0.2/microsoft.aspnetcore.authentication.jwtbearer.nuspec \ | grep -oP '<dependency id="System.IdentityModel.Tokens.Jwt" version="\K[^"]+' \ || echo "No explicit dependency found"Length of output: 458
Let’s list all declared dependencies in the JwtBearer 8.0.2 nuspec to see if it pulls in any version of System.IdentityModel.Tokens.Jwt (or Microsoft.IdentityModel.Tokens):
🏁 Script executed:
#!/bin/bash echo "Extracting dependencies for Microsoft.AspNetCore.Authentication.JwtBearer 8.0.2..." curl -s https://api.nuget.org/v3-flatcontainer/microsoft.aspnetcore.authentication.jwtbearer/8.0.2/microsoft.aspnetcore.authentication.jwtbearer.nuspec \ | grep -oP '<dependency id="[^"]+" version="[^"]+"' \ | sed -E 's/<dependency id="([^"]+)" version="([^"]+)"/\1@\2/'Length of output: 497
Let’s check whether Microsoft.IdentityModel.Protocols.OpenIdConnect 7.1.2 (a dependency of JwtBearer) pulls in System.IdentityModel.Tokens.Jwt:
🏁 Script executed:
#!/bin/bash echo "Listing dependencies for Microsoft.IdentityModel.Protocols.OpenIdConnect 7.1.2..." curl -s https://api.nuget.org/v3-flatcontainer/microsoft.identitymodel.protocols.openidconnect/7.1.2/microsoft.identitymodel.protocols.openidconnect.nuspec \ | grep -oP '<dependency id="[^"]+" version="[^"]+"' \ | sed -E 's/.*id="([^"]+)" version="([^"]+)".*/\1@\2/'Length of output: 1125
Verify JWT package version override compatibility
Microsoft.AspNetCore.Authentication.JwtBearer8.0.2 pulls in
Microsoft.IdentityModel.Protocols.OpenIdConnect7.1.2 →System.IdentityModel.Tokens.Jwt7.1.2- Adding a direct reference to
System.IdentityModel.Tokens.Jwt8.6.0 will override the transitive 7.1.2 dependency.- Please confirm that the newer major version of
System.IdentityModel.Tokens.Jwtremains compatible with your OpenID Connect and JWT‑Bearer flows—run integration tests or review the 8.x release notes for breaking changes.File to check:
• src/Core/Core.csproj @ line 14src/Api/Api.csproj (1)
11-11:✅ Verification successful
Appropriate JWT and Swagger packages added
The JWT and Swagger-related package additions are appropriate for implementing authentication and enhancing API documentation.
Consider aligning the versions of JWT-related packages for better compatibility. Currently using:
- System.IdentityModel.Tokens.Jwt (8.6.0)
- Microsoft.AspNetCore.Authentication.JwtBearer (8.0.2)
Let's verify if these versions are fully compatible:
Also applies to: 25-26
🌐 Web query:
Are Microsoft.AspNetCore.Authentication.JwtBearer 8.0.2 and System.IdentityModel.Tokens.Jwt 8.6.0 compatible?💡 Result:
Compatibility of Microsoft.AspNetCore.Authentication.JwtBearer 8.0.2 and System.IdentityModel.Tokens.Jwt 8.6.0
Summary:
Yes, Microsoft.AspNetCore.Authentication.JwtBearer version 8.0.2 and System.IdentityModel.Tokens.Jwt version 8.6.0 are compatible and can be used together in an ASP.NET Core 8 project.
Compatibility Details
Microsoft.AspNetCore.Authentication.JwtBearer 8.0.2 is designed for .NET 8 and works as middleware to handle JWT bearer authentication in ASP.NET Core applications[6].
This middleware, under the hood, depends on types and token validation logic from the Microsoft.IdentityModel.Tokens and System.IdentityModel.Tokens.Jwt libraries, using them for parsing and validating JWT tokens[1][6].
System.IdentityModel.Tokens.Jwt 8.6.0 includes updates and new features but remains fully compatible as it continues to provide the core
JwtSecurityTokenHandlerused by the authentication middleware[7].The JwtBearer middleware is designed to work with the latest 8.x versions of these identity model libraries, including System.IdentityModel.Tokens.Jwt and Microsoft.IdentityModel.Tokens, unless there is a breaking change, which is not present in 8.6.0[6][7].
Community reports confirm successful use of current 8.x versions of these libraries together without compatibility issues[8].
How the Libraries Work Together
- When you configure JWT authentication using
Microsoft.AspNetCore.Authentication.JwtBearer, the middleware usesSystem.IdentityModel.Tokens.Jwtto read, validate, and parse JWT tokens in incoming HTTP requests[1][6].- The
TokenValidationParametersyou supply are passed through to the JWT library, specifying issuer, audience, signing keys, and more, without any version conflicts in the 8.x series[1].
Best Practices
- When using these packages, ensure all related Microsoft.IdentityModel.* packages (such as Microsoft.IdentityModel.Tokens) are kept at matching or close minor versions to avoid subtle issues with dependency resolution[8].
- Always test authentication flows after upgrades, particularly if you rely on newer features introduced in the latest System.IdentityModel.Tokens.Jwt[7].
Conclusion
You can safely use Microsoft.AspNetCore.Authentication.JwtBearer 8.0.2 and System.IdentityModel.Tokens.Jwt 8.6.0 together in the same ASP.NET Core project targeting .NET 8.0 or later. There are no known compatibility issues between these versions, and this combination follows the intended usage for JWT authentication in ASP.NET Core 8 applications.[6][7][8]
Citations:
- 1: https://www.nuget.org/packages/Microsoft.AspNetCore.Authentication.JwtBearer
- 2: Microsoft.AspNetCore.Authentication.JwtBearer 7.0.11 requires >= 7.0.0 of Microsoft.IdentityModel.Protocols.OpenIdConnect dotnet/aspnetcore#50943
- 3: https://learn.microsoft.com/en-us/aspnet/core/security/authentication/configure-jwt-bearer-authentication?view=aspnetcore-9.0
- 4: https://www.nuget.org/packages/Microsoft.AspNetCore.Authentication.JwtBearer/9.0.1
- 5: https://github.com/dotnet/docs/blob/main/docs/core/compatibility/aspnet-core/8.0/securitytoken-events.md
- 6: https://www.nuget.org/packages/Microsoft.AspNetCore.Authentication.JwtBearer/8.0.2
- 7: https://github.com/AzureAD/azure-activedirectory-identitymodel-extensions-for-dotnet/blob/dev/CHANGELOG.md
- 8: SecurityTokenMalformedException after updating to .NET 8 dotnet/aspnetcore#52286
- 9: https://learn.microsoft.com/en-us/answers/questions/2006436/authentication-with-jwt-doent-work-for-minimal-api
- 10: https://learn.microsoft.com/en-us/aspnet/core/release-notes/aspnetcore-8.0?view=aspnetcore-9.0
JWT and Swagger package versions are compatible
The package additions in
src/Api/Api.csprojare appropriate for JWT authentication (Microsoft.AspNetCore.Authentication.JwtBearer v8.0.2, System.IdentityModel.Tokens.Jwt v8.6.0) and enhanced Swagger documentation (Swashbuckle.AspNetCore.Annotations v7.2.0). These JWT package versions are confirmed compatible for ASP.NET Core 8.0+. As a best practice, keep all related Microsoft.IdentityModel.* dependencies aligned at similar minor versions and validate authentication flows after any upgrade.src/Core/Services/Interfaces/IRoleService.cs (2)
9-10: Pagination support improvements addedThe addition of
GetTotalRolesCountAsync()and paginatedGetAllRolesAsync()methods improves the API's scalability by preventing potential performance issues when dealing with large data sets. This change aligns with the centralized error handling approach by allowing more granular control over data retrieval.
12-12: Improved return type for better error handlingChanging
UpdateRoleAsyncto returnRoleResponseDto?instead of a boolean improves error handling by providing the actual updated entity when successful, which aligns with the centralized exception handling approach in the PR. The service can now throw specific exceptions for failure cases rather than returning ambiguous boolean values.src/Core/Repositories/Interfaces/IRoleRepository.cs (2)
9-10: Pagination support implementation is consistent with service layerThe repository interface now correctly supports pagination with the addition of
GetTotalRolesCountAsync()and the updatedGetAllRolesAsync(int pageNumber, int pageSize)method. This maintains consistency with the changes in the service layer interface.
12-12: Repository now returns entity objects instead of booleansChanging
UpdateRoleAsyncto returnRole?instead of a boolean improves error handling capability. This allows the service layer to receive the actual updated entity and throw appropriate exceptions based on detailed information rather than just success/failure flags.src/Api/Program.cs (2)
37-37: TokenService registration addedTokenService registration as a scoped service is appropriate for JWT token generation. This supports the centralized error handling approach by enabling proper authentication.
158-159: Authentication middleware addedThe
UseAuthenticationmiddleware is correctly placed beforeUseAuthorizationin the request pipeline, which is essential for the JWT authentication to work properly.src/Core/Services/TokenService.cs (1)
36-40: Proper validation of token expiry configurationGood implementation of validation for the expiry time configuration with a clear error message when it's missing or invalid.
src/Core/Repositories/RoleRepository.cs (3)
29-32: Added count method for pagination support - looks goodThis method properly counts all roles and will be useful for pagination metadata. The use of
ConfigureAwait(false)is consistent with the rest of the codebase.
34-43: Well-implemented pagination for rolesThe implementation of pagination follows best practices:
- Using
AsNoTracking()for read-only queries- Consistent ordering by ID to ensure stable pagination
- Proper Skip/Take pattern for efficient data fetching
This change aligns well with the centralized error handling approach by enabling the service layer to provide paginated responses.
52-57: Improved return type for UpdateRoleAsyncChanging the return type from
booltoRole?provides more context to callers about the operation result and makes it consistent withAddRoleAsync. This supports the centralized error handling by allowing the service layer to throw specific exceptions based on the operation outcome.src/Core/Repositories/Interfaces/IUserRepository.cs (2)
9-10: Added pagination support to user repository interfaceThe addition of
GetTotalUsersCountAsync()and modifyingGetAllUsersAsync()to accept pagination parameters are good changes that will support paginated API responses. This change is consistent with the approach used in the role repository.
11-12: Improved return types for user operationsChanging return types from
booltoUser?for add and update operations is a positive change that provides more detailed information to service layers. This allows for better error handling and supports the centralized exception handling approach by enabling services to throw specific exceptions when operations fail.src/Core/Repositories/UserRepository.cs (4)
27-30: Added user count method for pagination - looks goodThis method correctly implements the interface for getting total users count, which will be used for pagination metadata.
32-42: Well-implemented user pagination with relationship loadingThe paginated user retrieval implementation:
- Properly includes the Role relationship with
Include(u => u.Role)- Uses
AsNoTracking()for better read performance- Implements consistent ordering and efficient Skip/Take pagination
- Correctly configures async behavior with
ConfigureAwait(false)This implementation aligns with the interface changes and supports the centralized error handling approach.
43-48: Improved return value for AddUserAsyncThe method now explicitly returns the user entity or null instead of a boolean success indicator. This provides more context to callers and supports centralized exception handling by allowing service layers to throw specific exceptions based on the detailed result.
50-55: Improved return value for UpdateUserAsyncSimilar to AddUserAsync, this method now returns the entity or null instead of a boolean. This change is consistent with the pattern used elsewhere and supports better error handling in the service layer.
src/Api/Filters/CustomExceptionFilter.cs (3)
21-31: Well-implemented centralized exception handling with status mappingThe exception filter now properly maps different exception types to appropriate HTTP status codes and response statuses using pattern matching. The addition of
DatabaseOperationExceptionhandling with a 500 status is particularly important for the repository changes that now return entities instead of booleans.This is the core of the centralized error handling mechanism and it looks well-designed.
32-33: Good response status assignmentSetting the response status based on the exception type provides consistent error reporting to clients and aligns with the BaseResponse structure.
40-44: Improved unexpected exception handlingThe method now returns a tuple that includes the ResponseStatus enum value, which provides more context about the error type. Using a generic error message for unexpected exceptions is a good security practice to avoid leaking sensitive information.
src/Core/Services/Interfaces/IUserService.cs (1)
13-14:AddUserAsync/UpdateUserAsyncreturn nullable DTOs despite throwing exceptions on failure
If the implementations now throwAlreadyExistsException,NotFoundException, etc., the methods will never legitimately returnnull. Keeping the nullable annotation can mislead consumers into unnecessary null‑checks. Review whethernullcan still be produced; if not, drop the nullable suffix.src/Api/Controllers/RoleController.cs (1)
20-28: Success response may carrynulldata
GetRoleByIdAsyncthrows when the role is not found, which is correct, yet the method signature still allowsRoleResponseDto?. If mapping ever returnsnull(e.g., AutoMapper mis‑configuration) the controller would emit HTTP 200 withdata=nulland"Role retrieved successfully", which is misleading. Either make the DTO non‑nullable or add a guard that throws if mapping fails.src/Core/Services/RoleService.cs (1)
84-90: Throwing onUpdateRoleAsyncmapper failure hides mapping issues
If AutoMapper fails to project the DTO onto the entity,updatedRolemight end up identical toexistingRoleand repository could optimistically return it unchanged. Consider detecting no‑op updates to prevent misleading “updated successfully” messages when nothing changed.
| Task<int> GetTotalUsersCountAsync(); | ||
| Task<IEnumerable<UserResponseDto>> GetAllUsersAsync(int pageNumber, int pageSize); | ||
| Task<UserResponseDto?> AddUserAsync(UserCreateDto user); | ||
| Task<UserResponseDto?> UpdateUserAsync(int id, UserUpdateDto user); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider returning a dedicated paged result instead of raw IEnumerable + extra count call
Requiring consumers to make two separate calls (GetAllUsersAsync + GetTotalUsersCountAsync) increases round‑trips and opens the door to race conditions between queries. Returning a single PagedResult<T> (data + totalCount + page metadata) simplifies the API surface and guarantees both values are consistent.
There was a problem hiding this comment.
Can you help me with this ?
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for any pagination-related classes
echo "Searching for PaginationMetadata class..."
rg "PaginationMetadata" -A 10 -B 2
echo -e "\nLooking for other pagination-related classes..."
rg "Pag(ed|ination)" -t cs
Length of output: 4310
🏁 Script executed:
#!/bin/bash
# Look at the UserController's GetAllUsers method to see how it's using the pagination
echo "Checking UserController GetAllUsers implementation..."
rg "GetAllUsers" "src/Api/Controllers/UserController.cs" -A 20 -B 2
# Also check the RoleController for comparison
echo -e "\nChecking RoleController GetAllRoles implementation..."
rg "GetAllRoles" "src/Api/Controllers/RoleController.cs" -A 20 -B 2
# Check the BaseResponse class definition
echo -e "\nChecking BaseResponse class definition..."
head -n 25 src/Api/Models/Common/BaseResponse.cs
Length of output: 3623
@osm-Jatin I'd be happy to help you implement the suggested approach! Here's what we'll need to do:
- Create a generic
PagedResult<T>class - Update the
IUserServiceinterface - Implement the updated service method
- Simplify the controller implementation
Here's my recommended implementation:
flowchart TB
A[Create PagedResult class] --o B[Update IUserService interface]
B --o C[Update UserService implementation]
C --o D[Update UserController]
A --o E[Update IRoleService and RoleService]
E --o F[Update RoleController]
🧬 Code Graph Analysis Results
Relevant snippets from src/Core/Entities/DTOs/UserDTOs.cs
Lines 20-24:
public class UserCreateDto : UserBaseDto
{
[Required, MinLength(8)]
public string Password { get; set; } = string.Empty;
}- Represents the data transfer object used when creating a new user.
- Inherits from
UserBaseDto(not shown here). - Contains a required
Passwordproperty with a minimum length of 8 characters.
Line 26:
public class UserUpdateDto : UserBaseDto { }- Represents the data transfer object used when updating an existing user.
- Inherits from
UserBaseDto. - No additional properties beyond those in
UserBaseDto.
Lines 28-35:
public class UserResponseDto
{
public int Id { get; set; }
public string FirstName { get; set; } = string.Empty;
public string LastName { get; set; } = string.Empty;
public string Email { get; set; } = string.Empty;
public string Role { get; set; } = string.Empty;
}- Represents the data transfer object returned in responses related to user data.
- Contains user identification and profile information:
Id,FirstName,LastName,Email, andRole.
These DTO classes are relevant to understanding the method signatures in the IUserService interface, as they define the input and output data structures used by the service methods.
⏳ Preparing changes...
There was a problem hiding this comment.
A summary of the edits CodeRabbit can apply:
We will update the IUserService interface in src/Core/Services/Interfaces/IUserService.cs to return PagedResult for GetAllUsersAsync and remove the redundant GetTotalUsersCountAsync method.
A plan of each step that CodeRabbit will take in order to execute the edits for each file (with inline rough draft snippets):
✏️ src/Core/Services/Interfaces/IUserService.cs
After the existing using directives, insert the namespace for PagedResult so the top of the file reads:
using Core.Entities.DTOs;
using Core.Entities.Models;
using Core.Entities.Common;Remove these two lines from the interface:
Task<int> GetTotalUsersCountAsync();
Task<IEnumerable<UserResponseDto>> GetAllUsersAsync(int pageNumber, int pageSize);
```Then insert the single paged signature in their place:
```csharp
Task<PagedResult<UserResponseDto>> GetAllUsersAsync(int pageNumber, int pageSize);Ensure the remaining methods (Login, GetUserByIdAsync, GetUserByEmailAsync, AddUserAsync, UpdateUserAsync, DeleteUserAsync) remain unchanged under the interface.
A summary of the context that CodeRabbit is considering across the codebase:
Locate the definition of PagedResult to confirm its namespace for import into IUserService.
Inspect the top of IUserService to determine where to add the using directive and which lines to replace.
-
✅ Create PR with these edits
-
📋 Get copyable edits
-
📌 Create commit in current branch
-
🔄 Try a different approach
-
⏪ Go back to ordinary chat
There was a problem hiding this comment.
And what should be my PagedResult
There was a problem hiding this comment.
No description provided by you, can you reply again
osm-Jatin
force-pushed
the
feat/exceptions
branch
from
34cfebc to
a47c770
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (7)
src/Api/Controllers/UserController.cs (2)
47-66:⚠️ Potential issueGuard
pageNumber&pageSizeto prevent invalid paging & division-by-zeroThe controller doesn't validate pagination parameters before passing them to the service. This could lead to:
- Division by zero when calculating
totalPages(line 52) ifpageSizeis 0- Invalid queries if negative values are passed
- Inefficient database queries
public async Task<ActionResult<BaseResponse<IEnumerable<UserResponseDto>>>> GetAllUsers([FromQuery] int pageNumber = 1, [FromQuery] int pageSize = 10) { + if (pageNumber < 1 || pageSize < 1) + { + throw new BadRequestException("pageNumber and pageSize must be greater than 0."); + } BaseResponse<IEnumerable<UserResponseDto>> response = new(ResponseStatus.Success); IEnumerable<UserResponseDto> users = await _userService.GetAllUsersAsync(pageNumber, pageSize).ConfigureAwait(false); int totalCount = await _userService.GetTotalUsersCountAsync().ConfigureAwait(false); int totalPages = (int)Math.Ceiling(totalCount / (double)pageSize);
68-73:⚠️ Potential issue
Loginbreaks the "uniform response" contract & lacksAllowAnonymousThis endpoint doesn't follow your standardized response pattern:
- It returns a bare anonymous object, violating the consistency goal
- It's missing the
[AllowAnonymous]attribute, which could prevent successful login if global authorization is applied+[AllowAnonymous] [HttpPost("login")] -public async Task<IActionResult> Login([FromBody] LoginRequestDto loginDto) +public async Task<ActionResult<BaseResponse<string>>> Login([FromBody] LoginRequestDto loginDto) { string? token = await _userService.Login(loginDto.Email, loginDto.Password).ConfigureAwait(false); - return Ok(new { token }); + var response = new BaseResponse<string>(ResponseStatus.Success) + { + Data = token, + Message = "Login successful" + }; + return Ok(response); }src/Core/Services/UserService.cs (3)
25-35:⚠️ Potential issueValidate
passwordinput before querying DBThe method doesn't validate input parameters before querying the database, which could lead to:
- Inefficient database lookups for obviously invalid inputs
- Potential
ArgumentNullExceptioninBCrypt.Verifyifpasswordis null-public async Task<string?> Login(string email, string password) +public async Task<string> Login(string email, string password) { + if (string.IsNullOrWhiteSpace(email) || string.IsNullOrWhiteSpace(password)) + { + throw new BadRequestException("Email and password are required."); + } User? user = await _userRepository.GetUserByEmailAsync(email).ConfigureAwait(false); if (user == null || !VerifyPassword(password, user.PasswordHash)) { throw new UnauthorizedAccessException("Invalid email or password."); } return _tokenService.GenerateToken(user); }
67-75: 🛠️ Refactor suggestionMissing guard clauses for paging parameters
Similar to the controller issue, the service method should validate pagination parameters to prevent errors with negative or zero values that could break database queries.
public async Task<IEnumerable<UserResponseDto>> GetAllUsersAsync(int pageNumber, int pageSize) { + if (pageNumber < 1 || pageSize < 1) + { + throw new BadRequestException("pageNumber and pageSize must be greater than 0."); + } IEnumerable<User> users = await _userRepository.GetAllUsersAsync(pageNumber, pageSize).ConfigureAwait(false); if (!users.Any()) { throw new NotFoundException("No users found."); } return _mapper.Map<IEnumerable<UserResponseDto>>(users); }
103-126:⚠️ Potential issue
UpdateUserAsyncdoesn't verify the newRoleIdWhen updating a user, you map the DTO to the entity and set the RoleId without verifying that the role exists. This could lead to:
- Foreign key constraint violations at the database level
- Orphaned references to non-existent roles
- Inconsistent data state
public async Task<UserResponseDto?> UpdateUserAsync(int id, UserUpdateDto userDto) { User? user = await _userRepository.GetUserByIdAsync(id).ConfigureAwait(false); if (user == null) { throw new NotFoundException($"User with ID {id} not found."); } User? existingUser = await _userRepository.GetUserByEmailAsync(userDto.Email).ConfigureAwait(false); if (existingUser != null && existingUser.Id != user.Id) { throw new AlreadyExistsException($"User with email {userDto.Email} already exists."); } _mapper.Map(userDto, user); + + Role? role = await _roleRepository.GetRoleByIdAsync(userDto.RoleId).ConfigureAwait(false); + if (role == null) + { + throw new NotFoundException($"Role with ID {userDto.RoleId} not found."); + } + user.SetRole(userDto.RoleId); User? updatedUser = await _userRepository.UpdateUserAsync(user).ConfigureAwait(false); if (updatedUser == null) { throw new DatabaseOperationException("Failed to update user."); } return _mapper.Map<UserResponseDto>(updatedUser); }src/Core/Services/RoleService.cs (2)
93-107: DeleteRoleAsync properly implements exception handling, but could benefit from CancellationToken support.The implementation correctly throws exceptions for error cases, but doesn't address the previous suggestion about adding CancellationToken support.
Consider adding CancellationToken support to all async methods, particularly for operations like delete that might involve cascading operations:
-public async Task<bool> DeleteRoleAsync(int id) +public async Task<bool> DeleteRoleAsync(int id, CancellationToken cancellationToken = default) { Role? role = await _roleRepository.GetRoleByIdAsync(id).ConfigureAwait(false); if (role == null) { throw new NotFoundException($"Role with ID {id} not found."); } - bool success = await _roleRepository.DeleteRoleAsync(role).ConfigureAwait(false); + bool success = await _roleRepository.DeleteRoleAsync(role, cancellationToken).ConfigureAwait(false); if (!success) { throw new DatabaseOperationException("Failed to delete role."); } return success; }Similarly, consider adding this pattern to other async methods in this service.
43-51: 🛠️ Refactor suggestionAdded pagination to GetAllRolesAsync but missing parameter validation.
The method has been updated to support pagination by accepting pageNumber and pageSize parameters, which is good. However, there's no validation to ensure these parameters have valid values.
Add parameter validation for
pageNumberandpageSizesimilar to the ID validation:public async Task<IEnumerable<RoleResponseDto>> GetAllRolesAsync(int pageNumber, int pageSize) { + if (pageNumber <= 0 || pageSize <= 0) + { + throw new BadRequestException("Page number and page size should be greater than 0."); + } IEnumerable<Role> roles = await _roleRepository.GetAllRolesAsync(pageNumber, pageSize).ConfigureAwait(false); if (!roles.Any()) { throw new NotFoundException("No roles found."); } return _mapper.Map<IEnumerable<RoleResponseDto>>(roles); }
🧹 Nitpick comments (2)
src/Core/Services/RoleService.cs (2)
31-36: Properly implemented not found exception handling.The implementation correctly throws a NotFoundException with a descriptive message when a role is not found, which aligns with the centralized error handling approach. However, there's a potential issue in the method signature.
The method signature at line 24 specifies a nullable return type (
RoleResponseDto?), but line 35 always returns a non-nullable value. Since you're now throwing exceptions for error cases, consider changing the method signature to:-public async Task<RoleResponseDto?> GetRoleByIdAsync(int id) +public async Task<RoleResponseDto> GetRoleByIdAsync(int id)
1-108: Consider adding XML documentation for public methods.To improve code readability and maintainability, consider adding XML documentation for all public methods, including descriptions of parameters, return values, and thrown exceptions.
For example:
/// <summary> /// Gets a role by its unique identifier. /// </summary> /// <param name="id">The role ID.</param> /// <returns>The role information.</returns> /// <exception cref="BadRequestException">Thrown when the ID is invalid.</exception> /// <exception cref="NotFoundException">Thrown when the role is not found.</exception> public async Task<RoleResponseDto> GetRoleByIdAsync(int id) { // ... }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
src/Api/Controllers/RoleController.cs(2 hunks)src/Api/Controllers/UserController.cs(2 hunks)src/Api/Filters/CustomExceptionFilter.cs(1 hunks)src/Api/Program.cs(5 hunks)src/Core/Entities/DTOs/UserDTOs.cs(2 hunks)src/Core/Exceptions/DatabaseOperationException.cs(1 hunks)src/Core/Services/RoleService.cs(3 hunks)src/Core/Services/UserService.cs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- src/Core/Exceptions/DatabaseOperationException.cs
- src/Api/Filters/CustomExceptionFilter.cs
- src/Api/Program.cs
- src/Core/Entities/DTOs/UserDTOs.cs
- src/Api/Controllers/RoleController.cs
🧰 Additional context used
🧠 Learnings (2)
src/Core/Services/RoleService.cs (1)
Learnt from: osm-Jatin
PR: OsmosysSoftware/dotnet-foundation-v2#5
File: src/Core/Services/RoleService.cs:56-60
Timestamp: 2025-03-02T12:30:17.440Z
Learning: In future PRs, the error handling in RoleService.cs will be updated to throw exceptions with descriptive messages instead of returning null values for clearer error reporting.
src/Core/Services/UserService.cs (1)
Learnt from: osm-Jatin
PR: OsmosysSoftware/dotnet-foundation-v2#14
File: src/Api/Controllers/UserController.cs:82-103
Timestamp: 2025-04-22T07:27:02.585Z
Learning: When implementing pagination in controllers, validate that pageNumber and pageSize are positive integers, convert IEnumerable results to List to prevent multiple enumeration, and use .Count instead of .Any() for checking if collections have items.
🧬 Code Graph Analysis (1)
src/Core/Services/RoleService.cs (6)
src/Core/Exceptions/BadRequestException.cs (4)
BadRequestException(3-16)BadRequestException(5-7)BadRequestException(9-11)BadRequestException(13-15)src/Core/Exceptions/NotFoundException.cs (4)
NotFoundException(3-16)NotFoundException(5-7)NotFoundException(9-11)NotFoundException(13-15)src/Core/Entities/DTOs/RoleDTOs.cs (2)
RoleResponseDto(20-23)RoleUpdateDto(16-18)src/Core/Services/UserService.cs (8)
Task(25-34)Task(36-49)Task(51-60)Task(62-65)Task(67-75)Task(77-101)Task(103-126)Task(128-142)src/Core/Repositories/Interfaces/IRoleRepository.cs (7)
Task(7-7)Task(8-8)Task(9-9)Task(10-10)Task(11-11)Task(12-12)Task(13-13)src/Core/Exceptions/AlreadyExistsException.cs (4)
AlreadyExistsException(3-16)AlreadyExistsException(5-7)AlreadyExistsException(9-11)AlreadyExistsException(13-15)
🔇 Additional comments (6)
src/Core/Services/RoleService.cs (6)
4-4: Correctly imported the necessary exceptions namespace.Adding the Core.Exceptions namespace is appropriate for the introduction of centralized error handling.
26-29: Good validation and error message for ID parameter.The validation for ID > 0 with a clear error message is a good practice. This follows the learning from previous PRs about throwing exceptions with descriptive messages instead of returning null values.
38-41: New method to retrieve total roles count.This is a good addition to support pagination functionality. The method directly passes through to the repository layer.
58-59: Good use of AlreadyExistsException in CreateRoleAsync.The implementation correctly throws an AlreadyExistsException with a descriptive message when a role with the same name already exists, replacing the previous approach of returning null.
63-68: Proper database operation exception handling.The implementation correctly throws a DatabaseOperationException with a descriptive message when role creation fails, improving error reporting.
70-91: Updated UpdateRoleAsync with proper exception handling.The method has been updated to throw appropriate exceptions and return the updated role. This is consistent with the rest of the service and the repository interface changes.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
src/Api/Controllers/UserController.cs (2)
47-66: 🛠️ Refactor suggestionMissing pagination parameter validation in
GetAllUsersThe method doesn't validate that
pageNumberandpageSizeare positive values. Although the UserService validates these parameters, it's best practice to validate input at the controller level to fail fast and avoid unnecessary service calls.Apply this diff:
public async Task<ActionResult<BaseResponse<IEnumerable<UserResponseDto>>>> GetAllUsers([FromQuery] int pageNumber = 1, [FromQuery] int pageSize = 10) { + if (pageNumber < 1 || pageSize < 1) + { + throw new BadRequestException("pageNumber and pageSize must be greater than 0."); + } + BaseResponse<IEnumerable<UserResponseDto>> response = new(ResponseStatus.Success); IEnumerable<UserResponseDto> users = await _userService.GetAllUsersAsync(pageNumber, pageSize).ConfigureAwait(false); int totalCount = await _userService.GetTotalUsersCountAsync().ConfigureAwait(false); int totalPages = (int)Math.Ceiling(totalCount / (double)pageSize);
68-76:⚠️ Potential issueMissing
[AllowAnonymous]attribute on the Login endpointThe Login endpoint is missing the
[AllowAnonymous]attribute. If a global authorization filter is configured in your application, this endpoint will be inaccessible to unauthenticated users, preventing them from logging in.Apply this diff to fix the issue:
-[HttpPost("login")] +[AllowAnonymous] +[HttpPost("login")] public async Task<ActionResult<BaseResponse<string>>> Login([FromBody] LoginRequestDto loginDto) { BaseResponse<string> response = new(ResponseStatus.Success); string? token = await _userService.Login(loginDto.Email, loginDto.Password).ConfigureAwait(false); response.Data = token; response.Message = "Login Successful"; return Ok(response); }
🧹 Nitpick comments (5)
src/Api/Controllers/UserController.cs (1)
50-53: Convert to List before calculating pagination valuesBased on your past feedback, it's best to convert the
IEnumerabletoListbefore proceeding with pagination calculations to prevent multiple enumeration of the collection. This is especially important when you need to check if there are any items.-IEnumerable<UserResponseDto> users = await _userService.GetAllUsersAsync(pageNumber, pageSize).ConfigureAwait(false); +List<UserResponseDto> users = (await _userService.GetAllUsersAsync(pageNumber, pageSize).ConfigureAwait(false)).ToList(); int totalCount = await _userService.GetTotalUsersCountAsync().ConfigureAwait(false); int totalPages = (int)Math.Ceiling(totalCount / (double)pageSize);src/Core/Services/UserService.cs (4)
25-26: Update return type to non-nullable stringThe
Loginmethod always either throws an exception or returns a valid token. Therefore, the return type should be non-nullablestringinstead ofstring?.-public async Task<string?> Login(string email, string password) +public async Task<string> Login(string email, string password)
79-82: Consider returning empty collection instead of throwing NotFoundExceptionIt's generally better to return an empty collection when no data is found during a list operation, rather than treating it as an error with a
NotFoundException. This aligns with RESTful API best practices and allows clients to handle empty results gracefully without error handling.-if (!users.Any()) -{ - throw new NotFoundException("No users found."); -} +// Empty collections are a valid response, no need to throw an exception
166-166: Add exception handling around BCrypt.VerifyThe
BCrypt.Verifymethod can throw anArgumentNullExceptionif the password or hash is null. While your input validation prevents null inputs, it's still good practice to add exception handling for robustness.private static bool VerifyPassword(string password, string hashedPassword) { - return BCrypt.Net.BCrypt.Verify(password, hashedPassword); + try + { + return BCrypt.Net.BCrypt.Verify(password, hashedPassword); + } + catch (ArgumentNullException) + { + return false; + } }
74-77: Consistent validation with UserControllerThe parameter validation here correctly checks if values are greater than zero, but make sure it's consistent with the validation logic in UserController. It's better to have the same validation message and logic in both places.
- throw new BadRequestException("pageNumber and pageSize must be greater than zero."); + throw new BadRequestException("pageNumber and pageSize must be greater than 0.");
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/Api/Controllers/UserController.cs(2 hunks)src/Core/Services/RoleService.cs(3 hunks)src/Core/Services/UserService.cs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/Core/Services/RoleService.cs
🧰 Additional context used
🧠 Learnings (1)
src/Core/Services/UserService.cs (1)
Learnt from: osm-Jatin
PR: OsmosysSoftware/dotnet-foundation-v2#14
File: src/Api/Controllers/UserController.cs:82-103
Timestamp: 2025-04-22T07:27:02.585Z
Learning: When implementing pagination in controllers, validate that pageNumber and pageSize are positive integers, convert IEnumerable results to List to prevent multiple enumeration, and use .Count instead of .Any() for checking if collections have items.
48c644a to
41c2e9f
Compare
Task ID
REST-1137
Description
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Chores