Enrich exceptions logs in Boilerplate server exception handler (#9933)#9934
Enrich exceptions logs in Boilerplate server exception handler (#9933)#9934msynk merged 18 commits intobitfoundation:developfrom yasmoradi:release
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThis pull request introduces significant improvements in error handling, logging, and build configuration. The changes update the Windows build process to target 64-bit and enable ReadyToRun. Numerous modifications across client and server components add contextual data to exceptions using new extension methods and centralized handling via a ServerExceptionHandler. Service registration and configuration adjustments further streamline error reporting and telemetry, while updates in shared DTOs and logging configurations enhance serialization and log filtering. Overall, the modifications advance predictable exception flows and improve diagnostics. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant API_Controller
participant Service
participant ServerExceptionHandler
participant Logger
Client->>API_Controller: Request operation
API_Controller->>Service: Invoke business logic
Service-->>Service: Exception occurs with added context (WithData)
Service-->>API_Controller: Throws enhanced exception
API_Controller->>ServerExceptionHandler: Handle(exception, context)
ServerExceptionHandler->>Logger: Log exception details with additional data
Logger-->>ServerExceptionHandler: Confirm logged details
ServerExceptionHandler-->>API_Controller: Returns detailed problem response
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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: 3
🧹 Nitpick comments (22)
src/Templates/Boilerplate/Bit.Boilerplate/src/Server/Boilerplate.Server.Api/Controllers/Identity/IdentityController.SocialSignIn.cs (1)
95-95: Consider using object initialization for better readability.Good enhancement of exception handling with contextual information. Consider making the dictionary creation more concise and explicit:
-serverExceptionHandler.Handle(exp, new() { { "LoginProvider", info?.LoginProvider }, { "Principal", info?.Principal?.GetDisplayName() } }); +serverExceptionHandler.Handle(exp, new() +{ + ["LoginProvider"] = info?.LoginProvider ?? "(none)", + ["Principal"] = info?.Principal?.GetDisplayName() ?? "(unknown)" +});This makes the code more readable and provides explicit fallback values for logging when the values are null.
src/Templates/Boilerplate/Bit.Boilerplate/src/Server/Boilerplate.Server.Api/SignalR/AppHub.cs (1)
28-31: Consider adding more diagnostic context.While the addition of
ConnectionIdis valuable, consider enriching the exception data further with:
- Token expiration timestamp (if available)
- Token type (access/refresh)
This additional context could help identify patterns in token expiration issues.Example enhancement:
- throw new HubException(nameof(AppStrings.UnauthorizedException)).WithData("ConnectionId", Context.ConnectionId); + throw new HubException(nameof(AppStrings.UnauthorizedException)) + .WithData("ConnectionId", Context.ConnectionId) + .WithData("TokenType", Context.GetHttpContext()?.Request.Headers.Authorization.FirstOrDefault()?.StartsWith("Bearer ") == true ? "Bearer" : "Unknown") + .WithData("AuthorizationPresent", true);src/Templates/Boilerplate/Bit.Boilerplate/src/Server/Boilerplate.Server.Api/Controllers/Identity/UserController.cs (1)
395-395: Consider using a more explicit capacity for List initialization.The list initialization at line 395 uses an empty collection initializer. For better performance, consider specifying an initial capacity based on the maximum number of possible tasks (email, SMS, SignalR, push notification).
-List<Task> sendMessagesTasks = []; +List<Task> sendMessagesTasks = new(capacity: 4); // Max 4 tasks: email, SMS, SignalR, push notificationsrc/Templates/Boilerplate/Bit.Boilerplate/src/Server/Boilerplate.Server.Api/Controllers/Identity/IdentityController.PhoneConfirmation.cs (1)
39-42: Simplify the lockout duration calculation.While the error context enrichment is good, the lockout duration calculation can be simplified.
Consider this simpler calculation:
- var tryAgainIn = (user.LockoutEnd! - DateTimeOffset.UtcNow).Value; - throw new BadRequestException(Localizer[nameof(AppStrings.UserLockedOut), (DateTimeOffset.UtcNow - user.LockoutEnd!).Value.Humanize(culture: CultureInfo.CurrentUICulture)]).WithData("UserId", user.Id).WithExtensionData("TryAgainIn", tryAgainIn); + var tryAgainIn = user.LockoutEnd!.Value - DateTimeOffset.UtcNow; + throw new BadRequestException(Localizer[nameof(AppStrings.UserLockedOut), tryAgainIn.Humanize(culture: CultureInfo.CurrentUICulture)]) + .WithData("UserId", user.Id) + .WithExtensionData("TryAgainIn", tryAgainIn);src/Templates/Boilerplate/Bit.Boilerplate/src/Server/Boilerplate.Server.Api/Controllers/Identity/IdentityController.ResetPassword.cs (3)
21-21: LGTM! Enhanced exception with user context.Including the user ID in the exception is valuable for tracing issues. Consider also including the confirmation status (email/phone) to provide more specific context about which confirmation is missing.
-throw new BadRequestException(Localizer[nameof(AppStrings.UserIsNotConfirmed)]).WithData("UserId", user.Id); +throw new BadRequestException(Localizer[nameof(AppStrings.UserIsNotConfirmed)]) + .WithData("UserId", user.Id) + .WithData("EmailConfirmed", await userManager.IsEmailConfirmedAsync(user)) + .WithData("PhoneConfirmed", await userManager.IsPhoneNumberConfirmedAsync(user));
26-26: Fix typo in exception class name and approve enhanced context.The exception includes comprehensive context with both user ID and retry delay, which is excellent. However, there's a typo in the exception class name.
-throw new TooManyRequestsExceptions( +throw new TooManyRequestsException(
89-89: LGTM! Consider adding remaining attempts information.While adding the user ID is valuable, consider including the remaining attempts before account lockout to provide more context.
-throw new BadRequestException(nameof(AppStrings.InvalidToken)).WithData("UserId", user.Id); +throw new BadRequestException(nameof(AppStrings.InvalidToken)) + .WithData("UserId", user.Id) + .WithData("RemainingAttempts", await userManager.GetAccessFailedCountAsync(user));src/Templates/Boilerplate/Bit.Boilerplate/src/Shared/appsettings.json (1)
2-6: Preprocessor Directives in JSON Configuration
The file includes preprocessor-like comments (e.g.,//#if (appInsights == true)and//#endif) that are not standard JSON syntax and are triggering static analysis errors. Please ensure that your build or pre-processing pipeline properly strips or handles these directives before the file is consumed by a strict JSON parser.🧰 Tools
🪛 Biome (1.9.4)
[error] 2-3: Expected a property but instead found '//#if (appInsights == true)'.
Expected a property here.
(parse)
[error] 3-3: End of file expected
Use an array for a sequence of values:
[1, 2](parse)
[error] 3-3: End of file expected
Use an array for a sequence of values:
[1, 2](parse)
[error] 3-5: End of file expected
Use an array for a sequence of values:
[1, 2](parse)
src/Templates/Boilerplate/Bit.Boilerplate/src/Server/Boilerplate.Server.Api/Services/ServerExceptionHandler.cs (3)
10-13: Consider consolidating environment references if not strictly necessary.
You're injecting bothIHostEnvironmentandIWebHostEnvironment. If you only need the environment name, consider using one of them to simplify maintenance.
111-123: Use more granular log levels for known exceptions.
Currently, you useLogErrorfor known exceptions andLogCriticalfor the rest. Consider further distinctions (e.g.,LogWarningorLogInformation) for specific exceptions that do not constitute an error or critical condition.
125-130: Decide on a fallback approach wheninstanceortraceIdentifieris missing.
The code returns early if either is null, thus noProblemDetailsis written. In certain edge cases (e.g., diagnostics), you might prefer to provide partial details or a different fallback.src/Templates/Boilerplate/Bit.Boilerplate/src/Shared/Extensions/ExceptionExtensions.cs (1)
8-17: Consider updating dictionary insertion to avoid duplicate key exceptions.
exception.Data.Add(item.Key, item.Value)can throw if the key already exists. Using index assignment (e.g.,exception.Data[item.Key] = item.Value) might be safer if overwriting is acceptable.src/Templates/Boilerplate/Bit.Boilerplate/src/Server/Boilerplate.Server.Api/Extensions/KnownExceptionExtensions.cs (1)
9-18: Evaluate merging or reusing extension data logic.
WithExtensionDataiterates through a dictionary and repeatedly callsWithExtensionData(key, value). This is consistent but duplicates the logic for each pair. Consider a single logic path to maintain consistency and reduce code duplication.src/Templates/Boilerplate/Bit.Boilerplate/src/Shared/Exceptions/KnownException.cs (1)
34-52: LGTM! Well-implemented TryGet pattern with comprehensive type handling.The implementation safely handles both direct type casting and JSON deserialization scenarios.
Consider adding XML documentation for the type parameter and return value:
/// <summary> /// Read KnownExceptionExtensions.WithExtensionData comments. /// </summary> +/// <typeparam name="T">The type of the value to retrieve.</typeparam> +/// <param name="key">The key of the extension data to retrieve.</param> +/// <param name="value">When this method returns, contains the value associated with the specified key, if found; otherwise, the default value for the type.</param> +/// <returns>true if the key was found; otherwise, false.</returns> public bool TryGetExtensionDataValue<T>(string key, out T value)src/Templates/Boilerplate/Bit.Boilerplate/src/Server/Boilerplate.Server.Api/Services/PhoneService.cs (1)
41-63: Consider using a proper background job system.The current fire-and-forget pattern using
Task.Runcould lead to lost exceptions or unhandled failures if the application shuts down. Consider using a proper message queue or background job system like Hangfire, as mentioned in the comment.Example implementation using a background job interface:
-_ = Task.Run(async () => // Let's not wait for the sms to be sent. Consider using a proper message queue or background job system like Hangfire. -{ - await using var scope = rootServiceScopeProvider.Invoke(); - var serverExceptionHandler = scope.ServiceProvider.GetRequiredService<ServerExceptionHandler>(); - MessageResource? smsMessage = null; - try - { - var messageOptions = new CreateMessageOptions(new(phoneNumber)) - { - From = new(from), - Body = messageText - }; - - smsMessage = MessageResource.Create(messageOptions); - - if (smsMessage.ErrorCode is not null) - throw new InvalidOperationException(smsMessage.ErrorMessage).WithData(new() { { "Code", smsMessage.ErrorCode } }); - } - catch (Exception exp) - { - serverExceptionHandler.Handle(exp, new() { { "PhoneNumber", phoneNumber } }); - } -}, default); +await _backgroundJob.EnqueueAsync<IPhoneService>( + service => service.SendSmsBackground(messageText, phoneNumber, from), + cancellationToken);Add a new method for background processing:
public async Task SendSmsBackground(string messageText, string phoneNumber, string from) { MessageResource? smsMessage = null; try { var messageOptions = new CreateMessageOptions(new(phoneNumber)) { From = new(from), Body = messageText }; smsMessage = MessageResource.Create(messageOptions); if (smsMessage.ErrorCode is not null) throw new InvalidOperationException(smsMessage.ErrorMessage).WithData(new() { { "Code", smsMessage.ErrorCode } }); } catch (Exception exp) { var serverExceptionHandler = _serviceProvider.GetRequiredService<ServerExceptionHandler>(); serverExceptionHandler.Handle(exp, new() { { "PhoneNumber", phoneNumber } }); throw; // Re-throw to let the background job system handle the failure } }src/Templates/Boilerplate/Bit.Boilerplate/src/Client/Boilerplate.Client.Core/Components/Layout/DiagnosticModal.razor.cs (3)
136-138: LGTM! Consider adding more test data.The addition of contextual data to test exceptions using
WithDataaligns with the PR objective of enriching exception logs. Consider adding more test data fields to better simulate real-world scenarios.- ? new InvalidOperationException("Something critical happened.").WithData("TestData", 1) - : new DomainLogicException("Something bad happened.").WithData("TestData", 2); + ? new InvalidOperationException("Something critical happened.") + .WithData("TestData", 1) + .WithData("TestTimestamp", DateTimeOffset.UtcNow) + .WithData("TestSource", "DiagnosticModal") + : new DomainLogicException("Something bad happened.") + .WithData("TestData", 2) + .WithData("TestTimestamp", DateTimeOffset.UtcNow) + .WithData("TestSource", "DiagnosticModal");
137-138: LGTM! Consider adding more test data.The addition of contextual data to test exceptions using
WithDataaligns with the PR objective of enriching exception logs. Consider adding more test data to simulate real-world scenarios.- ? new InvalidOperationException("Something critical happened.").WithData("TestData", 1) - : new DomainLogicException("Something bad happened.").WithData("TestData", 2); + ? new InvalidOperationException("Something critical happened.") + .WithData("TestData", 1) + .WithData("TestTimestamp", DateTimeOffset.UtcNow) + .WithData("TestContext", "DiagnosticModal") + : new DomainLogicException("Something bad happened.") + .WithData("TestData", 2) + .WithData("TestTimestamp", DateTimeOffset.UtcNow) + .WithData("TestContext", "DiagnosticModal");
137-138: LGTM! Good enhancement of exception data.The addition of contextual data to exceptions using
WithDataimproves debugging capabilities by providing more context when exceptions are thrown.Consider adding more descriptive test data to better simulate real-world scenarios. For example:
- ? new InvalidOperationException("Something critical happened.").WithData("TestData", 1) - : new DomainLogicException("Something bad happened.").WithData("TestData", 2); + ? new InvalidOperationException("Something critical happened.").WithData("Operation", "CriticalTest").WithData("Severity", "High") + : new DomainLogicException("Something bad happened.").WithData("Domain", "TestModule").WithData("Impact", "Medium");src/Templates/Boilerplate/Bit.Boilerplate/src/Server/Boilerplate.Server.Api/Services/EmailService.cs (2)
140-141: LGTM! Consider adding more context data.The transition from direct logging to using
ServerExceptionHandlerwith contextual data aligns with the PR objective. Consider adding more context data to help with debugging.- serverExceptionHandler.Handle(exp, new() { { "Subject", subject }, { "ToEmailAddress", toEmailAddress } }); + serverExceptionHandler.Handle(exp, new() + { + { "Subject", subject }, + { "ToEmailAddress", toEmailAddress }, + { "FromEmail", defaultFromEmail }, + { "FromName", defaultFromName }, + { "IsHtml", true } + });Also applies to: 157-157
140-141: LGTM! Good transition to centralized exception handling.The change from direct logging to using
ServerExceptionHandlerimproves consistency in error handling across the application. The contextual data (SubjectandToEmailAddress) provides valuable information for debugging email-related issues.Consider adding more contextual data to enrich the exception further:
- serverExceptionHandler.Handle(exp, new() { { "Subject", subject }, { "ToEmailAddress", toEmailAddress } }); + serverExceptionHandler.Handle(exp, new() + { + { "Subject", subject }, + { "ToEmailAddress", toEmailAddress }, + { "EmailProvider", emailSettings.Host }, + { "IsTestEnvironment", emailSettings.UseLocalFolderForEmails } + });Also applies to: 157-158
src/Templates/Boilerplate/Bit.Boilerplate/src/Server/Boilerplate.Server.Api/Controllers/Identity/IdentityController.cs (2)
56-56: Excellent enhancement of exception handling with contextual data!The changes consistently enrich exceptions with relevant contextual data (user IDs, session IDs, etc.), which will significantly improve debugging and monitoring capabilities. This aligns perfectly with the PR's objective of enhancing exception logs.
However, consider adding structured logging for these exceptions to ensure the contextual data is properly captured in the logs.
Add structured logging after throwing exceptions:
throw new BadRequestException(Localizer[nameof(AppStrings.DuplicateEmailOrPhoneNumber)]).WithData("UserId", existingUser.Id); +logger.LogError("Duplicate email or phone number detected for user {UserId}", existingUser.Id);Also applies to: 96-96, 120-120, 124-126, 141-142, 149-150, 156-157, 231-231, 238-244, 301-301, 303-304, 308-309, 351-352, 361-361, 365-366, 370-371, 419-419
233-234: Consider using string interpolation for better readability.The session stamp value comparison could be more readable using string interpolation.
-var sessionStampValueInDatabase = (userSession.RenewedOn ?? userSession.StartedOn).ToUnixTimeSeconds(); -var sessionStampValueInJwtToken = long.Parse(refreshTicket.Principal.Claims.Single(c => c.Type == AppClaimTypes.SESSION_STAMP).Value); +var sessionStampValueInDatabase = (userSession.RenewedOn ?? userSession.StartedOn).ToUnixTimeSeconds(); +var sessionStampValueInJwtToken = long.Parse(refreshTicket.Principal.GetClaimValue(AppClaimTypes.SESSION_STAMP));Also applies to: 235-235
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (26)
.github/workflows/admin-sample.cd.yml(1 hunks)src/Templates/Boilerplate/Bit.Boilerplate/src/Client/Boilerplate.Client.Core/Components/Layout/DiagnosticModal.razor.cs(1 hunks)src/Templates/Boilerplate/Bit.Boilerplate/src/Client/Boilerplate.Client.Core/Components/Pages/Identity/SignIn/SignInPage.razor.cs(1 hunks)src/Templates/Boilerplate/Bit.Boilerplate/src/Client/Boilerplate.Client.Core/Services/ClientExceptionHandlerBase.cs(1 hunks)src/Templates/Boilerplate/Bit.Boilerplate/src/Client/Boilerplate.Client.Core/Services/HttpMessageHandlers/ExceptionDelegatingHandler.cs(1 hunks)src/Templates/Boilerplate/Bit.Boilerplate/src/Server/Boilerplate.Server.Api/Controllers/Identity/IdentityController.EmailConfirmation.cs(4 hunks)src/Templates/Boilerplate/Bit.Boilerplate/src/Server/Boilerplate.Server.Api/Controllers/Identity/IdentityController.PhoneConfirmation.cs(3 hunks)src/Templates/Boilerplate/Bit.Boilerplate/src/Server/Boilerplate.Server.Api/Controllers/Identity/IdentityController.ResetPassword.cs(2 hunks)src/Templates/Boilerplate/Bit.Boilerplate/src/Server/Boilerplate.Server.Api/Controllers/Identity/IdentityController.SocialSignIn.cs(3 hunks)src/Templates/Boilerplate/Bit.Boilerplate/src/Server/Boilerplate.Server.Api/Controllers/Identity/IdentityController.cs(10 hunks)src/Templates/Boilerplate/Bit.Boilerplate/src/Server/Boilerplate.Server.Api/Controllers/Identity/UserController.cs(4 hunks)src/Templates/Boilerplate/Bit.Boilerplate/src/Server/Boilerplate.Server.Api/Data/Configurations/Product/ProductConfiguration.cs(1 hunks)src/Templates/Boilerplate/Bit.Boilerplate/src/Server/Boilerplate.Server.Api/Extensions/KnownExceptionExtensions.cs(1 hunks)src/Templates/Boilerplate/Bit.Boilerplate/src/Server/Boilerplate.Server.Api/Program.Services.cs(1 hunks)src/Templates/Boilerplate/Bit.Boilerplate/src/Server/Boilerplate.Server.Api/Services/EmailService.cs(2 hunks)src/Templates/Boilerplate/Bit.Boilerplate/src/Server/Boilerplate.Server.Api/Services/NugetStatisticsService.cs(1 hunks)src/Templates/Boilerplate/Bit.Boilerplate/src/Server/Boilerplate.Server.Api/Services/PhoneService.cs(2 hunks)src/Templates/Boilerplate/Bit.Boilerplate/src/Server/Boilerplate.Server.Api/Services/PushNotificationService.cs(2 hunks)src/Templates/Boilerplate/Bit.Boilerplate/src/Server/Boilerplate.Server.Api/Services/ServerExceptionHandler.cs(1 hunks)src/Templates/Boilerplate/Bit.Boilerplate/src/Server/Boilerplate.Server.Api/SignalR/AppHub.cs(1 hunks)src/Templates/Boilerplate/Bit.Boilerplate/src/Shared/Dtos/AppJsonContext.cs(1 hunks)src/Templates/Boilerplate/Bit.Boilerplate/src/Shared/Dtos/Identity/IdentityRequestDto.cs(1 hunks)src/Templates/Boilerplate/Bit.Boilerplate/src/Shared/Exceptions/KnownException.cs(1 hunks)src/Templates/Boilerplate/Bit.Boilerplate/src/Shared/Extensions/ExceptionExtensions.cs(1 hunks)src/Templates/Boilerplate/Bit.Boilerplate/src/Shared/appsettings.Development.json(3 hunks)src/Templates/Boilerplate/Bit.Boilerplate/src/Shared/appsettings.json(3 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/Templates/Boilerplate/Bit.Boilerplate/src/Client/Boilerplate.Client.Core/Components/Pages/Identity/SignIn/SignInPage.razor.cs
🧰 Additional context used
🪛 Biome (1.9.4)
src/Templates/Boilerplate/Bit.Boilerplate/src/Shared/appsettings.json
[error] 13-14: Expected a property but instead found '//#if (appInsights == true)'.
Expected a property here.
(parse)
[error] 13-14: End of file expected
Use an array for a sequence of values: [1, 2]
(parse)
[error] 14-14: End of file expected
Use an array for a sequence of values: [1, 2]
(parse)
[error] 14-14: End of file expected
Use an array for a sequence of values: [1, 2]
(parse)
[error] 40-42: End of file expected
Use an array for a sequence of values: [1, 2]
(parse)
[error] 42-42: End of file expected
Use an array for a sequence of values: [1, 2]
(parse)
[error] 42-42: End of file expected
Use an array for a sequence of values: [1, 2]
(parse)
[error] 61-62: End of file expected
Use an array for a sequence of values: [1, 2]
(parse)
[error] 62-62: End of file expected
Use an array for a sequence of values: [1, 2]
(parse)
[error] 62-62: End of file expected
Use an array for a sequence of values: [1, 2]
(parse)
[error] 62-68: End of file expected
Use an array for a sequence of values: [1, 2]
(parse)
[error] 68-69: End of file expected
Use an array for a sequence of values: [1, 2]
(parse)
[error] 69-69: End of file expected
Use an array for a sequence of values: [1, 2]
(parse)
[error] 69-69: End of file expected
Use an array for a sequence of values: [1, 2]
(parse)
src/Templates/Boilerplate/Bit.Boilerplate/src/Shared/appsettings.Development.json
[error] 10-11: Expected a property but instead found '//#if (appInsights == true)'.
Expected a property here.
(parse)
[error] 11-11: End of file expected
Use an array for a sequence of values: [1, 2]
(parse)
[error] 11-11: End of file expected
Use an array for a sequence of values: [1, 2]
(parse)
[error] 36-38: End of file expected
Use an array for a sequence of values: [1, 2]
(parse)
[error] 38-38: End of file expected
Use an array for a sequence of values: [1, 2]
(parse)
[error] 38-38: End of file expected
Use an array for a sequence of values: [1, 2]
(parse)
[error] 61-62: End of file expected
Use an array for a sequence of values: [1, 2]
(parse)
[error] 62-62: End of file expected
Use an array for a sequence of values: [1, 2]
(parse)
[error] 62-62: End of file expected
Use an array for a sequence of values: [1, 2]
(parse)
[error] 62-69: End of file expected
Use an array for a sequence of values: [1, 2]
(parse)
[error] 69-70: End of file expected
Use an array for a sequence of values: [1, 2]
(parse)
[error] 70-70: End of file expected
Use an array for a sequence of values: [1, 2]
(parse)
[error] 70-70: End of file expected
Use an array for a sequence of values: [1, 2]
(parse)
⏰ Context from checks skipped due to timeout of 90000ms (20)
- GitHub Check: deploy api + blazor
- GitHub Check: deploy api + blazor
- GitHub Check: deploy api + blazor
- GitHub Check: deploy api + blazor
- GitHub Check: build blazor wasm standalone (small)
- GitHub Check: build blazor wasm standalone (Offline database)
- GitHub Check: build blazor wasm standalone (AOT)
- GitHub Check: build blazor hybrid (windows)
- GitHub Check: build blazor hybrid (android)
- GitHub Check: build blazor hybrid (iOS-macOS)
- GitHub Check: build blazor wasm standalone
- GitHub Check: build api + blazor web
- GitHub Check: build blazor hybrid (iOS-macOS)
- GitHub Check: build blazor hybrid (windows)
- GitHub Check: build blazor hybrid (android)
- GitHub Check: build api + blazor web
- GitHub Check: build blazor hybrid (windows)
- GitHub Check: build blazor hybrid (iOS-macOS)
- GitHub Check: build blazor hybrid (android)
- GitHub Check: build and test
🔇 Additional comments (43)
src/Templates/Boilerplate/Bit.Boilerplate/src/Server/Boilerplate.Server.Api/Controllers/Identity/IdentityController.SocialSignIn.cs (1)
11-11: LGTM! Clean dependency injection.The ServerExceptionHandler is properly injected following the existing patterns.
src/Templates/Boilerplate/Bit.Boilerplate/src/Server/Boilerplate.Server.Api/SignalR/AppHub.cs (1)
31-31: Great enhancement to exception context!The addition of
ConnectionIdto the exception data improves debugging capabilities by providing crucial context about which connection triggered the unauthorized access attempt. This aligns well with the PR's objective of enriching exception logs.src/Templates/Boilerplate/Bit.Boilerplate/src/Server/Boilerplate.Server.Api/Controllers/Identity/UserController.cs (4)
132-134: Improved user feedback for account lockouts.The enhanced error handling now includes the exact duration until the user can retry, improving the user experience.
163-163: Verify email token resend delay calculation.The resend delay calculation at line 160 uses
DateTimeOffset.Nowwhich can lead to time-related issues in distributed systems.Consider using
DateTimeOffset.UtcNowfor consistency:-var resendDelay = (DateTimeOffset.Now - user!.EmailTokenRequestedOn) - AppSettings.Identity.EmailTokenLifetime; +var resendDelay = (DateTimeOffset.UtcNow - user!.EmailTokenRequestedOn) - AppSettings.Identity.EmailTokenLifetime;
224-224: Verify phone token resend delay calculation.Similar to the email token, the phone token resend delay calculation at line 221 uses local time instead of UTC.
Consider using
DateTimeOffset.UtcNowfor consistency:-var resendDelay = (DateTimeOffset.Now - user!.PhoneNumberTokenRequestedOn) - AppSettings.Identity.PhoneNumberTokenLifetime; +var resendDelay = (DateTimeOffset.UtcNow - user!.PhoneNumberTokenRequestedOn) - AppSettings.Identity.PhoneNumberTokenLifetime;
381-381: Verify elevated access token resend delay calculation.The elevated access token resend delay calculation at line 377 also uses local time.
Consider using
DateTimeOffset.UtcNowfor consistency:-var resendDelay = (DateTimeOffset.Now - user!.ElevatedAccessTokenRequestedOn) - AppSettings.Identity.BearerTokenExpiration; +var resendDelay = (DateTimeOffset.UtcNow - user!.ElevatedAccessTokenRequestedOn) - AppSettings.Identity.BearerTokenExpiration;src/Templates/Boilerplate/Bit.Boilerplate/src/Server/Boilerplate.Server.Api/Controllers/Identity/IdentityController.PhoneConfirmation.cs (4)
18-18: LGTM! Enhanced error context for phone confirmation.The additions of phone number and user ID to the exceptions improve error traceability and debugging capabilities.
Also applies to: 21-21
49-49: LGTM! Consistent error context enrichment.Good addition of user ID context to all exceptions, improving error traceability throughout the phone confirmation process.
Also applies to: 54-54, 60-60
73-73: LGTM! Well-structured rate limit exception.Good separation of metadata and extension data, with proper localization and rate limit information.
79-79: LGTM! Consistent error context.Maintains consistency in error context enrichment throughout the file.
src/Templates/Boilerplate/Bit.Boilerplate/src/Server/Boilerplate.Server.Api/Controllers/Identity/IdentityController.ResetPassword.cs (4)
18-18: LGTM! Enhanced exception with request context.The addition of request data to the exception provides valuable debugging context while maintaining security by not exposing internal user data.
71-71: LGTM! Comprehensive exception context for password reset scenarios.The enhanced exceptions provide excellent context for debugging while maintaining security:
- Request data for user not found
- User ID for expired tokens
- User ID and humanized retry timing for locked accounts
Also applies to: 76-76, 80-82
95-95: LGTM! Enhanced validation exception with user context.Adding user ID to validation exceptions helps correlate validation failures with specific users.
101-101: LGTM! Enhanced update result exception with user context.Adding user ID to update result exceptions maintains consistency with other exception enhancements.
src/Templates/Boilerplate/Bit.Boilerplate/src/Shared/appsettings.json (7)
10-11: Setting ExceptionHandlerMiddleware LogLevel Globally
The addition of"Microsoft.AspNetCore.Diagnostics.ExceptionHandlerMiddleware": "None"under the global"Logging.LogLevel"section aligns with the PR’s objective by reducing logging noise from exception middleware. Confirm that suppressing these logs does not interfere with any critical diagnostic data required in production.
17-18: ApplicationInsights Logging Configuration Updated
Under the"ApplicationInsights"logging section, explicitly setting"Microsoft.AspNetCore.Diagnostics.ExceptionHandlerMiddleware": "None"ensures consistent log filtering across providers that are conditional on theappInsightsbuild flag.
36-38: Sentry Log Level Adjustment
The Sentry logger configuration now includes"Microsoft.AspNetCore.Diagnostics.ExceptionHandlerMiddleware": "None", which helps reduce redundant exception logs in Sentry. Please verify that this suppression does not prevent Sentry from capturing any exceptions that are critical for monitoring and alerting.
45-46: Console Logging Configuration Updated
Within the"Console"logger configuration, the change to set"Microsoft.AspNetCore.Diagnostics.ExceptionHandlerMiddleware": "None"is consistent with other configurations and should help keep console output focused by filtering out middleware exception logs.
58-59: EventLog Logger Configuration Updated
The EventLog logging section now explicitly sets the log level for"Microsoft.AspNetCore.Diagnostics.ExceptionHandlerMiddleware"to"None". This update is in line with the overall goal of reducing excessive logging from exception middleware across all providers.
65-66: EventSource Logger Configuration Updated
Similarly, the"EventSource"logger configuration now includes the property to disable logs from exception middleware. This helps to achieve a consistent log filtering strategy across different logging systems.
74-75: DiagnosticLogger Configuration Update
By adding"Microsoft.AspNetCore.Diagnostics.ExceptionHandlerMiddleware": "None"under the"DiagnosticLogger"section, the file ensures that verbose middleware exception logs are suppressed, thereby streamlining diagnostic outputs for easier analysis.src/Templates/Boilerplate/Bit.Boilerplate/src/Server/Boilerplate.Server.Api/Services/ServerExceptionHandler.cs (2)
16-18: Clarify use of a staticappSessionId.
A staticGuidset at application start might suffice for single-tenant scenarios. However, in multi-tenant or scaled-out environments, this could be misleading.
41-75: Monitor for key collisions in combined data dictionaries.
This block merges environment, request, and default data into a single dictionary. If keys overlap, their values may be overwritten. Confirm this behavior is intended. Also, be cautious about storing potentially sensitive or personal data.src/Templates/Boilerplate/Bit.Boilerplate/src/Shared/Extensions/ExceptionExtensions.cs (1)
22-28: Validate data keys to avoid accidental collisions.
When adding single key-value pairs, ensure the same collision handling strategy as the bulk-insertion method. Otherwise, repeated calls with the same key could raise an exception or overwrite data unexpectedly.src/Templates/Boilerplate/Bit.Boilerplate/src/Server/Boilerplate.Server.Api/Extensions/KnownExceptionExtensions.cs (1)
23-33: Validate potential overwrites when storing custom extension data.
The extension data dictionary keys are replaced if they already exist. Confirm this behavior aligns with your business requirements. If partial merges are desired, add a collision check.src/Templates/Boilerplate/Bit.Boilerplate/src/Shared/Dtos/Identity/IdentityRequestDto.cs (1)
32-35: LGTM! Well-structured ToString() implementation.The prioritization order (Email > PhoneNumber > UserName) makes sense, and the implementation is concise using null-coalescing operators.
src/Templates/Boilerplate/Bit.Boilerplate/src/Shared/Dtos/AppJsonContext.cs (1)
25-25: LGTM! TimeSpan serialization support added.The addition aligns with the existing pattern and supports the new exception extension data feature.
src/Templates/Boilerplate/Bit.Boilerplate/src/Server/Boilerplate.Server.Api/Services/PhoneService.cs (2)
44-44: LGTM! Centralized exception handling.Using
ServerExceptionHandlerimproves consistency in error handling across the application.
57-57: LGTM! Enhanced error context.Adding error code as extension data improves error diagnostics.
src/Templates/Boilerplate/Bit.Boilerplate/src/Client/Boilerplate.Client.Core/Services/ClientExceptionHandlerBase.cs (1)
28-35: Great enhancement to exception context!The addition of exception data to parameters enriches the logging context, making it easier to diagnose issues by capturing all relevant exception metadata.
src/Templates/Boilerplate/Bit.Boilerplate/src/Server/Boilerplate.Server.Api/Controllers/Identity/IdentityController.EmailConfirmation.cs (1)
17-17: Excellent enhancement to exception context!The consistent addition of contextual data (user IDs, email addresses, and retry times) to exceptions significantly improves debugging and troubleshooting capabilities. This implementation:
- Makes it easier to track user-specific issues
- Provides clear retry timing information
- Maintains consistency across different exception types
Also applies to: 20-20, 29-29, 34-34, 38-40, 47-47, 74-74
src/Templates/Boilerplate/Bit.Boilerplate/src/Client/Boilerplate.Client.Core/Services/HttpMessageHandlers/ExceptionDelegatingHandler.cs (1)
59-62: Excellent preservation of server-side exception context!The addition ensures that all extension data from the server's problem details is preserved in the client-side exception, maintaining the rich context throughout the error handling chain.
src/Templates/Boilerplate/Bit.Boilerplate/src/Server/Boilerplate.Server.Api/Services/PushNotificationService.cs (2)
82-82: Good move towards centralized exception handling!The switch from direct logging to using ServerExceptionHandler ensures consistent exception handling across services.
121-122: Great addition of push notification context!Adding the UserRelatedPush flag to the exception data helps distinguish between different types of push notification failures, making it easier to monitor and debug user-specific issues.
src/Templates/Boilerplate/Bit.Boilerplate/src/Server/Boilerplate.Server.Api/Data/Configurations/Product/ProductConfiguration.cs (3)
17-17:Details
❓ Verification inconclusive
Verify the property rename impact.
The configuration has been updated to use
ShortIdinstead ofNumberwith the "ProductShortId" sequence. This suggests a property rename that might require a database migration.Run the following script to verify the property rename impact:
🏁 Script executed:
#!/bin/bash # Description: Check for any remaining references to the old property name. # Test: Search for any remaining references to the 'Number' property. rg -A 5 'Product.*Number' # Test: Search for any migration files that might handle this rename. fd -e cs -e sql migrationLength of output: 818
Verify the Property Rename Impact in Database Migration
After running the verification scripts, no remaining references to the old "Number" property were found in the codebase. The configuration now correctly maps to
ShortIdwith the "ProductShortId" sequence. Additionally, migration files (including the one atsrc/Templates/Boilerplate/Bit.Boilerplate/src/Server/Boilerplate.Server.Api/Data/Migrations/20250217191441_InitialMigration.cs) are present, which should include the changes for this property update.Please double-check the migration file(s) to ensure that the column is renamed appropriately in the database schema.
17-17:Details
✅ Verification successful
LGTM! Verify sequence usage in the database.
The sequence configuration has been correctly updated to use the
ShortIdproperty, which has a unique index. This change ensures proper ID generation for products.Run the following script to verify the sequence usage:
🏁 Script executed:
#!/bin/bash # Description: Check for any remaining references to the old property name in sequence configurations. # Test: Search for any remaining references to 'Number' property in sequence configurations. rg -A 5 $'UseSequence.*Number' # Test: Verify that ShortId is used consistently in other configurations. rg -A 5 $'UseSequence.*ShortId'Length of output: 1058
Sequence configuration verified — no remaining issues.
The updated code in
src/Templates/Boilerplate/Bit.Boilerplate/src/Server/Boilerplate.Server.Api/Data/Configurations/Product/ProductConfiguration.csnow uses theShortIdproperty withUseSequence("ProductShortId")correctly. The verification script confirmed that there are no references to the old “Number” property, and theShortIdconfiguration is consistent.
17-17:Details
❓ Verification inconclusive
Verify the sequence change impact.
The sequence "ProductShortId" is now used for
ShortIdinstead ofNumber. This change could affect existing data and database migrations.Please run the following script to check for potential conflicts or issues:
🏁 Script executed:
#!/bin/bash # Description: Check for usage of Product.Number and existing migrations # Check for references to Product.Number rg -A 5 "Product\.Number" # Check for existing migrations that might be affected fd -e cs -p ".*Migration.*" -x cat {} \; | rg -A 10 -B 10 "ProductShortId|Product.*Number"Length of output: 123
Action Required: Manually Verify the Impact of the Sequence Change
The update now uses the sequence
"ProductShortId"forShortIdinstead of what was previously implied byNumber. Although our automated searches for references toProduct.Numberand for migration files mentioning eitherProductShortIdorProduct.*Numberproduced no output, the absence of evidence from these scripts isn’t conclusive. Please manually verify that:
- There are no lingering references in the code or migration files to the old
Product.Numberor its associated sequence.- The database migrations (if any) that manage sequence changes will handle the transition safely without data conflicts.
Location:
src/Templates/Boilerplate/Bit.Boilerplate/src/Server/Boilerplate.Server.Api/Data/Configurations/Product/ProductConfiguration.cs(line 17)src/Templates/Boilerplate/Bit.Boilerplate/src/Server/Boilerplate.Server.Api/Program.Services.cs (3)
93-94: LGTM! Clean service registration.The service registration has been simplified while maintaining the interface contract. This change supports the centralized exception handling strategy.
93-94: LGTM! Improved service registration for better flexibility.The changes improve the service registration by:
- Registering
ServerExceptionHandleras itself first- Then registering it as
IProblemDetailsWriterthrough a cast
This allows the handler to be injected both asServerExceptionHandlerandIProblemDetailsWriter, providing more flexibility in usage.
93-94: LGTM! Good improvement in service registration.The change improves service registration by:
- Registering
ServerExceptionHandlerdirectly as a singleton- Registering it as
IProblemDetailsWriterthrough type castingThis approach provides better flexibility while maintaining the required interface implementation.
src/Templates/Boilerplate/Bit.Boilerplate/src/Shared/appsettings.Development.json (1)
7-8: Consistent logging configuration across providers!The changes establish uniform logging levels across different providers by:
- Setting consistent log level "Warning" for HttpClient LogicalHandler
- Disabling logs for DeveloperExceptionPageMiddleware and ExceptionHandlerMiddleware
This aligns with the PR's goal of improving exception handling by ensuring middleware exceptions are handled by the custom exception handler instead.
Also applies to: 15-16, 33-34, 42-43, 58-59, 66-67, 74-76
.github/workflows/admin-sample.cd.yml (1)
209-209: Improved Windows build optimization!The changes enhance the Windows build process by:
- Targeting 64-bit architecture (
win-x64) instead of 32-bit- Enabling ReadyToRun compilation for better startup performance
- Updating the VPK packaging command to match the new architecture
These optimizations will improve application performance and compatibility with modern systems.
Also applies to: 211-211
src/Templates/Boilerplate/Bit.Boilerplate/src/Server/Boilerplate.Server.Api/Services/EmailService.cs (1)
140-141: LGTM! Improved error handling with enriched context.The changes enhance error handling by:
- Using the centralized
ServerExceptionHandler- Including relevant email context (subject and recipient) in the exception data
Also applies to: 157-158
...erplate/Bit.Boilerplate/src/Server/Boilerplate.Server.Api/Services/NugetStatisticsService.cs
Show resolved
Hide resolved
...erplate/Bit.Boilerplate/src/Server/Boilerplate.Server.Api/Services/ServerExceptionHandler.cs
Show resolved
Hide resolved
...erplate/Bit.Boilerplate/src/Server/Boilerplate.Server.Api/Services/ServerExceptionHandler.cs
Show resolved
Hide resolved
2 participants
closes #9933
Summary by CodeRabbit