Feature/1211 tools project #1212
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 📝 WalkthroughWalkthroughThis PR introduces a new Tools console application for processing dead delivery reports from PostgreSQL. It includes a main program entry point, configuration utilities for dependency injection, Kafka producer implementation with Event Grid integration, database query utilities, and comprehensive test coverage. The Tools project and ToolsTests project are added to the solution alongside updates to the core and persistence layers. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
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. Comment |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (1)
Tools/CommonProducer.cs (1)
53-57: Logging message content on error is flagged by security analysis.This has been flagged by the security scanner. While learnings indicate Kafka messages in this system typically contain non-sensitive status info, this is a generic producer that could be used with sensitive data.
Consider logging only a truncated preview or hash of the message, or omitting the message content entirely.
🧹 Nitpick comments (9)
Altinn.Notifications.sln (1)
26-27: Minor inconsistency: Project type GUID differs from other projects.The Tools project uses the legacy C# project type GUID (
FAE04EC0-301F-11D3-BF4B-00C04F79EFBC) while all other projects use the SDK-style GUID (9A19103F-16F7-4668-BE54-9A1E7A4F7556). This is functional but inconsistent.Consider updating for consistency:
-Project("{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}") = "Tools", "Tools\Tools.csproj", "{4CC89BF4-4A77-4560-A6AF-50E1C655AF5D}" +Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "Tools", "Tools\Tools.csproj", "{4CC89BF4-4A77-4560-A6AF-50E1C655AF5D}"Tools/appsettings.json (1)
8-11: Placeholder URL should be documented or removed.The
BaseUrlcontains a placeholder value"https://your-event-grid-endpoint.com"that could cause confusion. Consider either:
- Setting it to
nulllikeAccessKeyto indicate it requires configuration- Adding a comment in the README about required configuration
"EventGrid": { - "BaseUrl": "https://your-event-grid-endpoint.com", + "BaseUrl": null, "AccessKey": null },Tools/Program.cs (2)
65-66: Hardcoded ID range limits reusability.The ID range is hardcoded, making this tool a one-off script. Consider making these configurable via command-line arguments or configuration:
- var fromId = 43716; - var toId = 43717; + var fromId = args.Length > 0 ? long.Parse(args[0]) : throw new ArgumentException("fromId required as first argument"); + var toId = args.Length > 1 ? long.Parse(args[1]) : throw new ArgumentException("toId required as second argument");Or via configuration in
appsettings.json.
41-49: Consider scoped lifetime for EventGridClient.
EventGridClientimplementsIDisposablebut is registered as a singleton. For a short-lived console app this works, but considerAddScopedfor proper disposal within the using scope, or ensure the host is properly disposed at the end.Tools/CommonProducer.cs (1)
84-94: Blocking.Wait()in constructor may cause issues.Using
.Wait()on an async method can cause deadlocks in certain contexts and blocks the calling thread. For a console app constructor this is generally acceptable, but consider usingGetAwaiter().GetResult()for clearer exception handling or making initialization async via a factory pattern.- adminClient.CreateTopicsAsync(new TopicSpecification[] - { - new TopicSpecification() - { - Name = topic, - NumPartitions = _sharedClientConfig.TopicSpecification.NumPartitions, - ReplicationFactor = _sharedClientConfig.TopicSpecification.ReplicationFactor, - Configs = _sharedClientConfig.TopicSpecification.Configs - } - }).Wait(); + adminClient.CreateTopicsAsync([ + new TopicSpecification + { + Name = topic, + NumPartitions = _sharedClientConfig.TopicSpecification.NumPartitions, + ReplicationFactor = _sharedClientConfig.TopicSpecification.ReplicationFactor, + Configs = _sharedClientConfig.TopicSpecification.Configs + } + ]).GetAwaiter().GetResult();src/Altinn.Notifications.Persistence/Repository/DeadDeliveryReportRepository.cs (1)
82-99: MoveGetOrdinalcalls outside the loop for better performance.The
reasonOrdinalandmessageOrdinalvalues are constant for all rows in the result set. Computing them inside the loop adds unnecessary overhead, especially for large result sets.var reports = new List<DeadDeliveryReport>(); + var reasonOrdinal = reader.GetOrdinal("reason"); + var messageOrdinal = reader.GetOrdinal("message"); + while (await reader.ReadAsync(cancellationToken)) { - var reasonOrdinal = reader.GetOrdinal("reason"); - var messageOrdinal = reader.GetOrdinal("message"); - var report = new DeadDeliveryReport {Note: The existing
GetAsyncmethod doesn't have this issue since it processes only a single row.Tools/EventGridClient.cs (1)
72-75: Consider implementing the full dispose pattern.The class implements
IDisposablecorrectly for this use case, but the null-conditional operator on_httpClientis unnecessary since it's always assigned in the constructor.public void Dispose() { - _httpClient?.Dispose(); + _httpClient.Dispose(); + GC.SuppressFinalize(this); }Tools/Util.cs (2)
26-33: Type mismatch:intparameters vslongrepository method.The
fromIdandtoIdparameters are declared asint, butIDeadDeliveryReportRepository.GetAllAsyncacceptslongparameters. While implicit conversion works, this limits the usable ID range toint.MaxValue(~2.1 billion). If IDs could exceed this, consider usinglongfor consistency.internal static async Task<List<EmailSendOperationResult>> GetAndMapDeadDeliveryReports( IDeadDeliveryReportRepository repository, - int fromId, - int toId, + long fromId, + long toId, Altinn.Notifications.Core.Enums.DeliveryReportChannel channel, CancellationToken cancellationToken)
45-61: Consider using a cancellation token forIsEmailNotificationSucceeded.Unlike other methods in this class, this method doesn't accept a
CancellationToken. For consistency and to support cancellation during long-running operations, consider adding this parameter.internal static async Task<bool> IsEmailNotificationSucceeded( NpgsqlDataSource dataSource, - string operationId) + string operationId, + CancellationToken cancellationToken = default) { const string sql = @" SELECT COUNT(1) FROM notifications.emailnotifications WHERE operationid = @operationId AND result = 'Succeeded'"; - await using var connection = await dataSource.OpenConnectionAsync(); + await using var connection = await dataSource.OpenConnectionAsync(cancellationToken); await using var command = new NpgsqlCommand(sql, connection); command.Parameters.AddWithValue("operationId", operationId); - var count = (long)(await command.ExecuteScalarAsync() ?? 0L); + var count = (long)(await command.ExecuteScalarAsync(cancellationToken) ?? 0L); return count > 0; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
Altinn.Notifications.sln(2 hunks)Tools/CommonProducer.cs(1 hunks)Tools/EventGridClient.cs(1 hunks)Tools/ICommonProducer.cs(1 hunks)Tools/IEventGridClient.cs(1 hunks)Tools/Program.cs(1 hunks)Tools/SharedClientConfig.cs(1 hunks)Tools/Tools.csproj(1 hunks)Tools/Util.cs(1 hunks)Tools/appsettings.json(1 hunks)src/Altinn.Notifications.Core/Persistence/IDeadDeliveryReportRepository.cs(2 hunks)src/Altinn.Notifications.Persistence/Repository/DeadDeliveryReportRepository.cs(3 hunks)
🧰 Additional context used
🧠 Learnings (23)
📓 Common learnings
Learnt from: eskebab
Repo: Altinn/altinn-notifications PR: 1148
File: src/Altinn.Notifications.Persistence/Migration/v0.67/01-functions-and-procedures.sql:632-644
Timestamp: 2025-11-12T08:49:13.705Z
Learning: In the Altinn/altinn-notifications repository, files like `src/Altinn.Notifications.Persistence/Migration/v0.67/01-functions-and-procedures.sql` are auto-generated from DbTools and contain many deprecated functions. When reviewing these migration files, only focus on the specific functions relevant to the PR's objective (e.g., for issue #1122 concurrent execution work) rather than reviewing all functions in the file.
Learnt from: Ahmed-Ghanam
Repo: Altinn/altinn-notifications PR: 1015
File: src/Altinn.Notifications.Integrations/Kafka/Consumers/NotificationStatusConsumerBase.cs:158-161
Timestamp: 2025-09-15T09:58:06.991Z
Learning: In Altinn.Notifications status consumers, Kafka messages for status updates contain only references and send-status information (non-sensitive data), so logging the full Kafka message payload is acceptable according to maintainer Ahmed-Ghanam.
Learnt from: Ahmed-Ghanam
Repo: Altinn/altinn-notifications PR: 1033
File: src/Altinn.Notifications.Core/Services/SmsNotificationService.cs:71-91
Timestamp: 2025-09-17T08:45:54.485Z
Learning: In Altinn.Notifications, the IKafkaProducer.ProduceAsync method includes internal try-catch handling and returns false if any exception occurs during publishing. This means external callers can rely on the boolean return value rather than needing explicit exception handling. Additionally, once GetNewNotifications() claims a batch of SMS notifications (atomically changing status from "New" to "Sending" via SQL), the design expects those claimed notifications to be processed to completion or reset to "New" status, rather than supporting mid-batch cancellation.
Learnt from: Ahmed-Ghanam
Repo: Altinn/altinn-notifications PR: 1021
File: src/Altinn.Notifications.Core/Services/SmsNotificationService.cs:0-0
Timestamp: 2025-09-16T06:48:40.494Z
Learning: In Altinn.Notifications SMS processing (SmsNotificationService.SendNotifications), when GetNewNotifications() retrieves SMS notifications, their status is atomically changed from "New" to "Sending". If publishing to Kafka fails (ProduceAsync returns false or throws), the status must be reset to "New" via UpdateSendStatus to allow retry processing in future batches. This prevents notifications from getting stuck in "Sending" state.
Learnt from: Ahmed-Ghanam
Repo: Altinn/altinn-notifications PR: 1015
File: src/Altinn.Notifications.Integrations/Kafka/Consumers/EmailStatusConsumer.cs:28-35
Timestamp: 2025-09-02T09:27:21.022Z
Learning: In Altinn.Notifications consumers, using concrete logger types like ILogger<EmailStatusConsumer> instead of the base class generic logger type is the preferred pattern. This provides clearer log categories while still satisfying DI requirements, as the .NET DI container handles the type mapping correctly.
Learnt from: SandGrainOne
Repo: Altinn/altinn-notifications PR: 895
File: src/Altinn.Notifications/Program.cs:215-233
Timestamp: 2025-06-13T14:04:57.506Z
Learning: The Altinn.Notifications team intends to replace the current client-secret based Key Vault integration with Workload Identity authentication in a future update; the present code is a temporary measure.
Learnt from: Ahmed-Ghanam
Repo: Altinn/altinn-notifications PR: 1015
File: src/Altinn.Notifications.Integrations/Kafka/Consumers/EmailStatusConsumer.cs:28-35
Timestamp: 2025-09-02T09:27:21.022Z
Learning: In Altinn.Notifications EmailStatusConsumer, using ILogger<EmailStatusConsumer> instead of the base class generic logger type is intentional and preferred, as it provides clearer log categories while still satisfying DI requirements through the container's type mapping capabilities.
Learnt from: Ahmed-Ghanam
Repo: Altinn/altinn-notifications PR: 1199
File: src/Altinn.Notifications.Core/Services/OrderProcessingService.cs:58-101
Timestamp: 2025-12-01T09:21:44.389Z
Learning: In Altinn.Notifications, the KafkaProducer.ProduceAsync method never throws OperationCanceledException. Instead, it translates cancellation into "failed/not-produced" outcomes: if cancellation is requested before producing starts, it increments the failed counter and returns all valid messages as unpublished; if cancellation occurs after tasks have started, it waits for Task.WhenAll completion and categorizes results, returning both invalid and not-produced messages. Successful messages are not returned.
Learnt from: eskebab
Repo: Altinn/altinn-notifications PR: 852
File: src/Altinn.Notifications.Persistence/Repository/StatusFeedRepository.cs:31-54
Timestamp: 2025-05-21T10:58:35.854Z
Learning: In the Altinn Notifications project, parameter validation is handled at the service layer rather than in the repository layer to maintain clear separation of concerns.
Learnt from: eskebab
Repo: Altinn/altinn-notifications PR: 1068
File: src/Altinn.Notifications.Integrations/Kafka/Consumers/EmailStatusConsumer.cs:27-27
Timestamp: 2025-10-09T10:24:50.624Z
Learning: In Altinn.Notifications status consumers (EmailStatusConsumer, SmsStatusConsumer), the base constructor accepts three topic parameters: topicName (to consume from), retryTopicName (for general exceptions - typically the same as topicName for immediate retry), and sendStatusUpdateRetryTopicName (for SendStatusUpdateException - routed to dedicated retry topics like EmailStatusUpdatedRetryTopicName). This implements a two-tier retry strategy: immediate retries via same topic, and threshold-based retries via dedicated retry consumer.
Learnt from: Ahmed-Ghanam
Repo: Altinn/altinn-notifications PR: 1091
File: test/Altinn.Notifications.IntegrationTests/Notifications.Integrations/TestingConsumers/NotificationStatusConsumerBaseTests.cs:0-0
Timestamp: 2025-10-15T08:05:01.433Z
Learning: In Altinn.Notifications NotificationStatusConsumerBase.ProcessStatus: ArgumentException and InvalidOperationException are caught and logged with message-only (no exception details). The LogProcessingError method intentionally does not pass the exception parameter to the logger. Tests should verify logging occurred without expecting the exception parameter.
📚 Learning: 2025-10-06T09:01:36.029Z
Learnt from: Ahmed-Ghanam
Repo: Altinn/altinn-notifications PR: 1066
File: src/Altinn.Notifications.Persistence/Migration/v0.58/02-functions-and-procedures.sql:0-0
Timestamp: 2025-10-06T09:01:36.029Z
Learning: In Altinn.Notifications EmailNotificationRepository: When `_alternateid` is provided to `updateemailnotification` function but the corresponding row no longer exists, the system redirects the delivery report to a retry topic and then to the dead delivery reports table (see issue #1017). No fallback to `operationid` should occur in this scenario.
Applied to files:
src/Altinn.Notifications.Core/Persistence/IDeadDeliveryReportRepository.csTools/Program.cssrc/Altinn.Notifications.Persistence/Repository/DeadDeliveryReportRepository.csTools/Util.csTools/CommonProducer.cs
📚 Learning: 2025-09-02T09:27:21.022Z
Learnt from: Ahmed-Ghanam
Repo: Altinn/altinn-notifications PR: 1015
File: src/Altinn.Notifications.Integrations/Kafka/Consumers/EmailStatusConsumer.cs:28-35
Timestamp: 2025-09-02T09:27:21.022Z
Learning: In Altinn.Notifications consumers, using concrete logger types like ILogger<EmailStatusConsumer> instead of the base class generic logger type is the preferred pattern. This provides clearer log categories while still satisfying DI requirements, as the .NET DI container handles the type mapping correctly.
Applied to files:
src/Altinn.Notifications.Core/Persistence/IDeadDeliveryReportRepository.csTools/Program.cssrc/Altinn.Notifications.Persistence/Repository/DeadDeliveryReportRepository.cs
📚 Learning: 2025-09-04T16:17:57.879Z
Learnt from: Ahmed-Ghanam
Repo: Altinn/altinn-notifications PR: 1021
File: src/Altinn.Notifications.Persistence/Migration/FunctionsAndProcedures/claimsmsbatchforsending.sql:27-29
Timestamp: 2025-09-04T16:17:57.879Z
Learning: Altinn.Notifications: SendingTimePolicy enum maps 1 = Anytime, 2 = Daytime; database functions treat NULL sendingtimepolicy as Daytime (included when filtering for Daytime, excluded for Anytime).
Applied to files:
src/Altinn.Notifications.Core/Persistence/IDeadDeliveryReportRepository.cssrc/Altinn.Notifications.Persistence/Repository/DeadDeliveryReportRepository.cs
📚 Learning: 2025-11-19T08:43:50.344Z
Learnt from: eskebab
Repo: Altinn/altinn-notifications PR: 1141
File: src/Altinn.Notifications.Persistence/Repository/NotificationRepositoryBase.cs:221-221
Timestamp: 2025-11-19T08:43:50.344Z
Learning: In Altinn.Notifications NotificationRepositoryBase.ReadRecipients: The team has decided not to add a null-guard for the status column when reading from the database. If status is NULL, it indicates data corruption and the code should throw an exception (InvalidCastException from GetFieldValueAsync<string>) to surface the issue immediately, rather than handling it gracefully.
Applied to files:
src/Altinn.Notifications.Core/Persistence/IDeadDeliveryReportRepository.cssrc/Altinn.Notifications.Persistence/Repository/DeadDeliveryReportRepository.cs
📚 Learning: 2025-09-02T09:27:21.022Z
Learnt from: Ahmed-Ghanam
Repo: Altinn/altinn-notifications PR: 1015
File: src/Altinn.Notifications.Integrations/Kafka/Consumers/EmailStatusConsumer.cs:28-35
Timestamp: 2025-09-02T09:27:21.022Z
Learning: In Altinn.Notifications EmailStatusConsumer, using ILogger<EmailStatusConsumer> instead of the base class generic logger type is intentional and preferred, as it provides clearer log categories while still satisfying DI requirements through the container's type mapping capabilities.
Applied to files:
src/Altinn.Notifications.Core/Persistence/IDeadDeliveryReportRepository.csTools/Program.cssrc/Altinn.Notifications.Persistence/Repository/DeadDeliveryReportRepository.cs
📚 Learning: 2025-10-15T08:05:01.433Z
Learnt from: Ahmed-Ghanam
Repo: Altinn/altinn-notifications PR: 1091
File: test/Altinn.Notifications.IntegrationTests/Notifications.Integrations/TestingConsumers/NotificationStatusConsumerBaseTests.cs:0-0
Timestamp: 2025-10-15T08:05:01.433Z
Learning: In Altinn.Notifications NotificationStatusConsumerBase.ProcessStatus: ArgumentException and InvalidOperationException are caught and logged with message-only (no exception details). The LogProcessingError method intentionally does not pass the exception parameter to the logger. Tests should verify logging occurred without expecting the exception parameter.
Applied to files:
src/Altinn.Notifications.Core/Persistence/IDeadDeliveryReportRepository.cssrc/Altinn.Notifications.Persistence/Repository/DeadDeliveryReportRepository.cs
📚 Learning: 2025-09-18T10:37:21.188Z
Learnt from: Ahmed-Ghanam
Repo: Altinn/altinn-notifications PR: 1008
File: test/k6/src/data/orders/order-with-reminders-for-organizations.json:34-38
Timestamp: 2025-09-18T10:37:21.188Z
Learning: In Altinn.Notifications test data files, channelSchema enum values can use both "SMS" and "Sms" formats - both are valid in the system, contrary to initial assumptions about strict camel-case requirements.
Applied to files:
src/Altinn.Notifications.Core/Persistence/IDeadDeliveryReportRepository.cssrc/Altinn.Notifications.Persistence/Repository/DeadDeliveryReportRepository.cs
📚 Learning: 2025-09-17T08:45:54.485Z
Learnt from: Ahmed-Ghanam
Repo: Altinn/altinn-notifications PR: 1033
File: src/Altinn.Notifications.Core/Services/SmsNotificationService.cs:71-91
Timestamp: 2025-09-17T08:45:54.485Z
Learning: In Altinn.Notifications, the IKafkaProducer.ProduceAsync method includes internal try-catch handling and returns false if any exception occurs during publishing. This means external callers can rely on the boolean return value rather than needing explicit exception handling. Additionally, once GetNewNotifications() claims a batch of SMS notifications (atomically changing status from "New" to "Sending" via SQL), the design expects those claimed notifications to be processed to completion or reset to "New" status, rather than supporting mid-batch cancellation.
Applied to files:
src/Altinn.Notifications.Core/Persistence/IDeadDeliveryReportRepository.csTools/Program.csTools/ICommonProducer.cssrc/Altinn.Notifications.Persistence/Repository/DeadDeliveryReportRepository.csTools/Util.csTools/CommonProducer.cs
📚 Learning: 2025-09-16T06:48:40.494Z
Learnt from: Ahmed-Ghanam
Repo: Altinn/altinn-notifications PR: 1021
File: src/Altinn.Notifications.Core/Services/SmsNotificationService.cs:0-0
Timestamp: 2025-09-16T06:48:40.494Z
Learning: In Altinn.Notifications SMS processing (SmsNotificationService.SendNotifications), when GetNewNotifications() retrieves SMS notifications, their status is atomically changed from "New" to "Sending". If publishing to Kafka fails (ProduceAsync returns false or throws), the status must be reset to "New" via UpdateSendStatus to allow retry processing in future batches. This prevents notifications from getting stuck in "Sending" state.
Applied to files:
src/Altinn.Notifications.Core/Persistence/IDeadDeliveryReportRepository.csTools/Util.cs
📚 Learning: 2025-07-16T15:22:50.953Z
Learnt from: Ahmed-Ghanam
Repo: Altinn/altinn-notifications PR: 940
File: test/Altinn.Notifications.IntegrationTests/Notifications/InstantOrdersController/InstantOrdersControllerTests.cs:506-543
Timestamp: 2025-07-16T15:22:50.953Z
Learning: In the Altinn Notifications system, [Required] attributes on model classes like ShortMessageContentExt, ShortMessageDeliveryDetailsExt, InstantNotificationRecipientExt, and InstantNotificationOrderRequestExt cause deserialization to fail when the provided JSON string lacks the required values. This is different from typical ASP.NET Core behavior where [Required] attributes cause model validation failures after successful deserialization.
Applied to files:
src/Altinn.Notifications.Core/Persistence/IDeadDeliveryReportRepository.cs
📚 Learning: 2025-09-17T08:45:54.485Z
Learnt from: Ahmed-Ghanam
Repo: Altinn/altinn-notifications PR: 1033
File: src/Altinn.Notifications.Core/Services/SmsNotificationService.cs:71-91
Timestamp: 2025-09-17T08:45:54.485Z
Learning: The KafkaProducer.ProduceAsync method in Altinn.Notifications includes comprehensive internal error handling with try-catch blocks that catch both ProduceException and general Exception types. It returns false on any error condition (exceptions or delivery failures) and only returns true when the message is successfully persisted. This means callers can rely on the boolean return value rather than needing additional exception handling.
Applied to files:
Tools/ICommonProducer.csTools/Util.csTools/CommonProducer.cs
📚 Learning: 2025-12-01T09:21:44.389Z
Learnt from: Ahmed-Ghanam
Repo: Altinn/altinn-notifications PR: 1199
File: src/Altinn.Notifications.Core/Services/OrderProcessingService.cs:58-101
Timestamp: 2025-12-01T09:21:44.389Z
Learning: In Altinn.Notifications, the KafkaProducer.ProduceAsync method never throws OperationCanceledException. Instead, it translates cancellation into "failed/not-produced" outcomes: if cancellation is requested before producing starts, it increments the failed counter and returns all valid messages as unpublished; if cancellation occurs after tasks have started, it waits for Task.WhenAll completion and categorizes results, returning both invalid and not-produced messages. Successful messages are not returned.
Applied to files:
Tools/ICommonProducer.csTools/Util.csTools/CommonProducer.cs
📚 Learning: 2025-08-29T10:23:03.256Z
Learnt from: Ahmed-Ghanam
Repo: Altinn/altinn-notifications PR: 1013
File: src/Altinn.Notifications.Persistence/Repository/SmsNotificationRepository.cs:202-207
Timestamp: 2025-08-29T10:23:03.256Z
Learning: In Altinn.Notifications SMS notifications: The gatewayreference column in notifications.smsnotifications table is expected to be unique for each row. This uniqueness should be enforced at the database level with a UNIQUE index to prevent ExecuteScalarAsync() from returning arbitrary rows when multiple matches exist.
Applied to files:
src/Altinn.Notifications.Persistence/Repository/DeadDeliveryReportRepository.cs
📚 Learning: 2025-05-21T10:58:35.854Z
Learnt from: eskebab
Repo: Altinn/altinn-notifications PR: 852
File: src/Altinn.Notifications.Persistence/Repository/StatusFeedRepository.cs:31-54
Timestamp: 2025-05-21T10:58:35.854Z
Learning: In the Altinn Notifications project, parameter validation is handled at the service layer rather than in the repository layer to maintain clear separation of concerns.
Applied to files:
src/Altinn.Notifications.Persistence/Repository/DeadDeliveryReportRepository.cs
📚 Learning: 2025-10-15T09:47:14.949Z
Learnt from: Ahmed-Ghanam
Repo: Altinn/altinn-notifications PR: 1091
File: src/Altinn.Notifications.Integrations/Kafka/Consumers/EmailStatusRetryConsumer.cs:40-54
Timestamp: 2025-10-15T09:47:14.949Z
Learning: In Altinn.Notifications retry consumers (EmailStatusRetryConsumer, SmsStatusRetryConsumer), messages with invalid or unparseable data (null/empty SendOperationResult, deserialization failures) should not be retried. The implementation should log the error and return early without throwing exceptions, as throwing would trigger the base class retry mechanism unintentionally.
Applied to files:
Tools/Util.csTools/CommonProducer.cs
📚 Learning: 2025-09-01T12:20:45.235Z
Learnt from: Ahmed-Ghanam
Repo: Altinn/altinn-notifications PR: 1012
File: src/Altinn.Notifications.Integrations/Kafka/Consumers/EmailStatusConsumer.cs:59-61
Timestamp: 2025-09-01T12:20:45.235Z
Learning: In Altinn.Notifications EmailStatusConsumer, the EmailSendOperationResult messages contain non-sensitive information according to the maintainer Ahmed-Ghanam, so logging the full message payload is acceptable.
Applied to files:
Tools/Util.csTools/CommonProducer.cs
📚 Learning: 2025-10-09T10:24:50.624Z
Learnt from: eskebab
Repo: Altinn/altinn-notifications PR: 1068
File: src/Altinn.Notifications.Integrations/Kafka/Consumers/EmailStatusConsumer.cs:27-27
Timestamp: 2025-10-09T10:24:50.624Z
Learning: In Altinn.Notifications status consumers (EmailStatusConsumer, SmsStatusConsumer), the base constructor accepts three topic parameters: topicName (to consume from), retryTopicName (for general exceptions - typically the same as topicName for immediate retry), and sendStatusUpdateRetryTopicName (for SendStatusUpdateException - routed to dedicated retry topics like EmailStatusUpdatedRetryTopicName). This implements a two-tier retry strategy: immediate retries via same topic, and threshold-based retries via dedicated retry consumer.
Applied to files:
Tools/Util.cs
📚 Learning: 2025-08-28T14:19:36.888Z
Learnt from: Ahmed-Ghanam
Repo: Altinn/altinn-notifications PR: 1010
File: src/Altinn.Notifications.Persistence/Repository/EmailNotificationRepository.cs:28-34
Timestamp: 2025-08-28T14:19:36.888Z
Learning: In Altinn.Notifications EmailNotificationRepository: Both `alternateid` and `operationid` are unique per row in the notifications.emailnotifications table, and when both parameters are provided, they must always point to the same row. This means OR conditions between these fields are safe and won't cause multi-row updates.
Applied to files:
Tools/Util.csTools/CommonProducer.cs
📚 Learning: 2025-10-06T09:01:36.029Z
Learnt from: Ahmed-Ghanam
Repo: Altinn/altinn-notifications PR: 1066
File: src/Altinn.Notifications.Persistence/Migration/v0.58/02-functions-and-procedures.sql:0-0
Timestamp: 2025-10-06T09:01:36.029Z
Learning: In Altinn.Notifications EmailNotificationRepository: When both `_alternateid` and `operationid` are provided to the `updateemailnotification` function, the system should identify the row by `_alternateid`, set the `operationid`, result, and result time, then return the `_alternateid`. The `_alternateid` takes priority over `operationid` for row identification.
Applied to files:
Tools/Util.csTools/CommonProducer.cs
📚 Learning: 2025-09-15T09:58:06.991Z
Learnt from: Ahmed-Ghanam
Repo: Altinn/altinn-notifications PR: 1015
File: src/Altinn.Notifications.Integrations/Kafka/Consumers/NotificationStatusConsumerBase.cs:158-161
Timestamp: 2025-09-15T09:58:06.991Z
Learning: In Altinn.Notifications status consumers, Kafka messages for status updates contain only references and send-status information (non-sensitive data), so logging the full Kafka message payload is acceptable according to maintainer Ahmed-Ghanam.
Applied to files:
Tools/CommonProducer.cs
📚 Learning: 2025-11-12T08:49:13.705Z
Learnt from: eskebab
Repo: Altinn/altinn-notifications PR: 1148
File: src/Altinn.Notifications.Persistence/Migration/v0.67/01-functions-and-procedures.sql:632-644
Timestamp: 2025-11-12T08:49:13.705Z
Learning: In the Altinn/altinn-notifications repository, files like `src/Altinn.Notifications.Persistence/Migration/v0.67/01-functions-and-procedures.sql` are auto-generated from DbTools and contain many deprecated functions. When reviewing these migration files, only focus on the specific functions relevant to the PR's objective (e.g., for issue #1122 concurrent execution work) rather than reviewing all functions in the file.
Applied to files:
Altinn.Notifications.sln
📚 Learning: 2025-08-28T13:53:37.392Z
Learnt from: eskebab
Repo: Altinn/altinn-notifications PR: 1011
File: src/Altinn.Notifications.Persistence/Migration/v0.55/01-functions-and-procedures.sql:0-0
Timestamp: 2025-08-28T13:53:37.392Z
Learning: In the Altinn/altinn-notifications repository, files named "functions-and-procedures.sql" (like src/Altinn.Notifications.Persistence/Migration/v0.55/01-functions-and-procedures.sql) are auto-generated based on scripts and used by Yuniql for database migrations. The standalone SQL files (like get_metrics_v2.sql) are the source of truth, so apparent "duplicates" between these files are intentional and part of the build process, not issues to be fixed.
Applied to files:
Altinn.Notifications.sln
🧬 Code graph analysis (5)
Tools/EventGridClient.cs (1)
Tools/IEventGridClient.cs (1)
Task(11-13)
Tools/ICommonProducer.cs (1)
Tools/CommonProducer.cs (1)
Task(44-66)
src/Altinn.Notifications.Persistence/Repository/DeadDeliveryReportRepository.cs (2)
Tools/Util.cs (3)
Task(26-43)Task(45-61)Task(63-95)src/Altinn.Notifications.Core/Persistence/IDeadDeliveryReportRepository.cs (3)
Task(18-18)Task(32-32)Task(40-40)
Tools/Util.cs (2)
src/Altinn.Notifications.Core/Persistence/IDeadDeliveryReportRepository.cs (3)
Task(18-18)Task(32-32)Task(40-40)src/Altinn.Notifications.Persistence/Repository/DeadDeliveryReportRepository.cs (3)
Task(20-37)Task(40-68)Task(71-102)
Tools/CommonProducer.cs (2)
Tools/SharedClientConfig.cs (2)
SharedClientConfig(11-94)SharedClientConfig(36-93)Tools/ICommonProducer.cs (1)
Task(13-13)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Analyze (csharp, autobuild)
- GitHub Check: Build, test & analyze
🔇 Additional comments (5)
Tools/ICommonProducer.cs (1)
1-14: LGTM!Clean interface definition following established patterns. The
Task<bool>return type aligns with the existing Kafka producer conventions in this codebase where callers rely on the boolean return value rather than exception handling.Tools/IEventGridClient.cs (1)
1-14: LGTM!Well-designed interface with appropriate
IDisposableinheritance for resource management and a tuple return type that provides actionable feedback on failures.src/Altinn.Notifications.Core/Persistence/IDeadDeliveryReportRepository.cs (1)
20-32: LGTM!Well-documented method addition with clear parameter descriptions. The inclusive/exclusive bounds for
fromId/toIdare properly documented, which helps prevent off-by-one errors for callers.Tools/SharedClientConfig.cs (1)
79-79: Clarify behavior whenRetentionTimeis exactly 0.A
RetentionTimeof 0 will result in"0"milliseconds retention, which effectively means immediate deletion. Consider whether this is the intended behavior, or if 0 should be treated as "use default" or map to-1(infinite).- string retentionTime = kafkaSettings.Admin.RetentionTime < 0 ? "-1" : TimeSpan.FromDays(kafkaSettings.Admin.RetentionTime).TotalMilliseconds.ToString(); + string retentionTime = kafkaSettings.Admin.RetentionTime <= 0 ? "-1" : TimeSpan.FromDays(kafkaSettings.Admin.RetentionTime).TotalMilliseconds.ToString();Tools/Util.cs (1)
63-95: Sequential Kafka production may be slow for large batches.The current implementation produces messages one at a time. For a tools project this is acceptable, but if performance becomes a concern with large result sets, consider batching or parallel production.
The implementation correctly tracks success/failure counts and provides useful console output for debugging. Based on learnings, the
IKafkaProducer.ProduceAsyncmethod includes internal try-catch handling, so relying on the boolean return value is appropriate.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (6)
Tools/Kafka/CommonProducer.cs (1)
68-68: Consider removing or sanitizing message content from log.CodeQL flags that the message value may contain private information. While learnings indicate that status update messages contain non-sensitive data, consider whether logging the full message content is necessary here, or if logging just the topic and status would suffice.
🔎 Proposed alternative
- _logger.LogError("// KafkaProducer // ProduceAsync // Message not ack'd by all brokers (value: '{Message}'). Delivery status: {Status}", message, result.Status); + _logger.LogError("// KafkaProducer // ProduceAsync // Message not ack'd by all brokers. Delivery status: {Status}", result.Status);ToolsTests/CommonProducerTests.cs (1)
320-333: Misleading test name: This test doesn't actually verify null producer handling.The test name
Dispose_HandlesNullProducersuggests it tests the scenario where_produceris null, but it actually creates a non-null mock producer. The production code handles null via the null-conditional operator (_producer?.Flush()), but this test only verifies that Dispose doesn't throw with a valid producer.Consider either:
- Renaming to
Dispose_DoesNotThrowto match the actual behavior tested, or- Adding a separate test that actually validates null producer handling (though this would require exposing a way to set
_producerto null for testing)ToolsTests/EventGridClientTests.cs (1)
1-4: Unusedusingdirective.The
using Tools;directive on line 3 doesn't appear to be needed—the test only uses types fromTools.EventGridnamespace.🔎 Proposed fix
using Microsoft.Extensions.Options; using System.Net; -using Tools; using Tools.EventGrid;ToolsTests/UtilTests.cs (1)
490-507: Test name doesn't match behavior: method returns empty string, not null.The test name
MapStatus_ReturnsNull_WhenSendResultIsNullis misleading. The assertions on lines 505-506 verify that the result isNotNullandEmpty, meaningMapStatusreturns an empty string whenSendResultis null, not null itself.🔎 Proposed fix
[Fact] - public void MapStatus_ReturnsNull_WhenSendResultIsNull() + public void MapStatus_ReturnsEmptyString_WhenSendResultIsNull() { // Arrange var result = new EmailSendOperationResultTools/EventGrid/EventGridClient.cs (1)
3-3: Unusedusingdirective.
System.Net.Http.Jsonis imported but not used. The code usesJsonSerializer.SerializewithStringContentinstead of the extension methods from this namespace.🔎 Proposed fix
using Microsoft.Extensions.Options; using System.Diagnostics.CodeAnalysis; -using System.Net.Http.Json; using System.Text; using System.Text.Json;ToolsTests/ProgramIntegrationTests.cs (1)
14-80: Consider refactoring to use ConfigurationUtil instead of duplicating DI setup.The test manually duplicates the service configuration logic (lines 32-60) that should match
Tools.ConfigurationUtil.ConfigureServices. This creates maintenance burden—if the actual configuration changes, these tests must be updated separately.🔎 Proposed refactor to eliminate duplication
[Fact] public void Program_ConfiguresServices_Correctly() { // Arrange var builder = Host.CreateApplicationBuilder(); - // Set up minimal configuration var inMemorySettings = new Dictionary<string, string> { {"PostgreSQLSettings:ConnectionString", "Host=localhost;Database=test;Username=test;Password=test"}, {"EventGrid:BaseUrl", "https://test.eventgrid.azure.net/api/events"}, {"EventGrid:AccessKey", "test-key"}, {"KafkaSettings:BrokerAddress", "localhost:9092"} }; - builder.Configuration.AddInMemoryCollection(inMemorySettings!); + builder.Configuration.AddInMemoryCollection(inMemorySettings); - // Configure services (same as Program.cs) - builder.Services.Configure<PostgreSqlSettings>( - builder.Configuration.GetSection("PostgreSQLSettings")); - - var connectionString = builder.Configuration["PostgreSQLSettings:ConnectionString"]; - builder.Services.AddSingleton<NpgsqlDataSource>(sp => - { - var dataSourceBuilder = new NpgsqlDataSourceBuilder(connectionString!); - return dataSourceBuilder.Build(); - }); - - builder.Services.AddSingleton(sp => - { - var kafkaSettings = new KafkaSettings(); - builder.Configuration.GetSection("KafkaSettings").Bind(kafkaSettings); - return kafkaSettings; - }); - - builder.Services.Configure<EventGridSettings>(builder.Configuration.GetSection("EventGrid")); - builder.Services.AddHttpClient<IEventGridClient, EventGridClient>((sp, client) => - { - var cfg = sp.GetRequiredService<IOptions<EventGridSettings>>().Value; - if (string.IsNullOrWhiteSpace(cfg.BaseUrl)) - { - throw new InvalidOperationException("EventGrid:BaseUrl is not configured"); - } - - client.BaseAddress = new Uri(cfg.BaseUrl); - client.Timeout = TimeSpan.FromSeconds(30); - }); + // Use the actual configuration utility to ensure tests match production + ConfigurationUtil.ConfigureDatabase(builder); + ConfigurationUtil.ConfigureKafka(builder); + ConfigurationUtil.ConfigureEventGrid(builder); var host = builder.Build(); // Act & Assert - Verify services can be resolved using var scope = host.Services.CreateScope(); var dataSource = scope.ServiceProvider.GetService<NpgsqlDataSource>(); Assert.NotNull(dataSource); var kafkaSettings = scope.ServiceProvider.GetService<KafkaSettings>(); Assert.NotNull(kafkaSettings); Assert.Equal("localhost:9092", kafkaSettings.BrokerAddress); var eventGridClient = scope.ServiceProvider.GetService<IEventGridClient>(); Assert.NotNull(eventGridClient); var eventGridSettings = scope.ServiceProvider.GetService<IOptions<EventGridSettings>>(); Assert.NotNull(eventGridSettings); Assert.Equal("https://test.eventgrid.azure.net/api/events", eventGridSettings.Value.BaseUrl); }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
Altinn.Notifications.slnTools/AssemblyInfo.csTools/ConfigurationUtil.csTools/EventGrid/EventGridClient.csTools/EventGrid/IEventGridClient.csTools/EventGridUtil.csTools/Kafka/CommonProducer.csTools/Kafka/ICommonProducer.csTools/PostgresUtil.csTools/Program.csTools/Tools.csprojTools/Util.csTools/appsettings.jsonToolsTests/CommonProducerTests.csToolsTests/EventGridClientTests.csToolsTests/ProgramIntegrationTests.csToolsTests/ToolsTests.csprojToolsTests/UtilTests.cstest/Altinn.Notifications.IntegrationTests/Notifications.Persistence/DeadDeliveryReportRepositoryTests.cs
🚧 Files skipped from review as they are similar to previous changes (2)
- Altinn.Notifications.sln
- Tools/Program.cs
🧰 Additional context used
🧠 Learnings (27)
📓 Common learnings
Learnt from: eskebab
Repo: Altinn/altinn-notifications PR: 1148
File: src/Altinn.Notifications.Persistence/Migration/v0.67/01-functions-and-procedures.sql:632-644
Timestamp: 2025-11-12T08:49:13.705Z
Learning: In the Altinn/altinn-notifications repository, files like `src/Altinn.Notifications.Persistence/Migration/v0.67/01-functions-and-procedures.sql` are auto-generated from DbTools and contain many deprecated functions. When reviewing these migration files, only focus on the specific functions relevant to the PR's objective (e.g., for issue #1122 concurrent execution work) rather than reviewing all functions in the file.
📚 Learning: 2025-12-17T14:54:42.946Z
Learnt from: eskebab
Repo: Altinn/altinn-notifications PR: 1212
File: Tools/Program.cs:86-91
Timestamp: 2025-12-17T14:54:42.946Z
Learning: In Altinn.Notifications Tools project, when processing dead delivery reports to update email notification status: only notifications in the "Succeeded" state should be updated to "Delivered". All other states—including terminal states (Delivered, Failed, Failed_RecipientReserved, Failed_RecipientNotIdentified, Failed_InvalidEmailFormat, Failed_SupressedRecipient, Failed_Bounced, Failed_FilteredSpam, Failed_Quarantined, Failed_TTL) and other intermediate states (New, Sending)—should be skipped. The logic must check for result = 'Succeeded' specifically, not for the absence of 'Delivered'.
Applied to files:
ToolsTests/UtilTests.csTools/PostgresUtil.csTools/Util.cstest/Altinn.Notifications.IntegrationTests/Notifications.Persistence/DeadDeliveryReportRepositoryTests.cs
📚 Learning: 2025-05-23T05:52:42.786Z
Learnt from: Ahmed-Ghanam
Repo: Altinn/altinn-notifications PR: 861
File: test/Altinn.Notifications.IntegrationTests/Notifications.Persistence/NotificationDeliveryManifestRepositoryTests.cs:165-169
Timestamp: 2025-05-23T05:52:42.786Z
Learning: In NotificationDeliveryManifestRepositoryTests, it's acceptable to bypass status transition logic and directly set order statuses when testing status mapping behavior, as the primary objective is to validate how the mapping logic interprets various order statuses rather than testing the transition logic itself.
Applied to files:
ToolsTests/UtilTests.cstest/Altinn.Notifications.IntegrationTests/Notifications.Persistence/DeadDeliveryReportRepositoryTests.cs
📚 Learning: 2025-10-15T09:47:14.949Z
Learnt from: Ahmed-Ghanam
Repo: Altinn/altinn-notifications PR: 1091
File: src/Altinn.Notifications.Integrations/Kafka/Consumers/EmailStatusRetryConsumer.cs:40-54
Timestamp: 2025-10-15T09:47:14.949Z
Learning: In Altinn.Notifications retry consumers (EmailStatusRetryConsumer, SmsStatusRetryConsumer), messages with invalid or unparseable data (null/empty SendOperationResult, deserialization failures) should not be retried. The implementation should log the error and return early without throwing exceptions, as throwing would trigger the base class retry mechanism unintentionally.
Applied to files:
ToolsTests/UtilTests.csTools/Kafka/CommonProducer.csTools/Util.cs
📚 Learning: 2025-09-17T08:45:54.485Z
Learnt from: Ahmed-Ghanam
Repo: Altinn/altinn-notifications PR: 1033
File: src/Altinn.Notifications.Core/Services/SmsNotificationService.cs:71-91
Timestamp: 2025-09-17T08:45:54.485Z
Learning: The KafkaProducer.ProduceAsync method in Altinn.Notifications includes comprehensive internal error handling with try-catch blocks that catch both ProduceException and general Exception types. It returns false on any error condition (exceptions or delivery failures) and only returns true when the message is successfully persisted. This means callers can rely on the boolean return value rather than needing additional exception handling.
Applied to files:
Tools/Kafka/CommonProducer.csToolsTests/CommonProducerTests.csTools/Kafka/ICommonProducer.csTools/Util.cs
📚 Learning: 2025-12-01T09:21:44.389Z
Learnt from: Ahmed-Ghanam
Repo: Altinn/altinn-notifications PR: 1199
File: src/Altinn.Notifications.Core/Services/OrderProcessingService.cs:58-101
Timestamp: 2025-12-01T09:21:44.389Z
Learning: In Altinn.Notifications, the KafkaProducer.ProduceAsync method never throws OperationCanceledException. Instead, it translates cancellation into "failed/not-produced" outcomes: if cancellation is requested before producing starts, it increments the failed counter and returns all valid messages as unpublished; if cancellation occurs after tasks have started, it waits for Task.WhenAll completion and categorizes results, returning both invalid and not-produced messages. Successful messages are not returned.
Applied to files:
Tools/Kafka/CommonProducer.csToolsTests/CommonProducerTests.csTools/Kafka/ICommonProducer.csTools/Util.cs
📚 Learning: 2025-09-17T08:45:54.485Z
Learnt from: Ahmed-Ghanam
Repo: Altinn/altinn-notifications PR: 1033
File: src/Altinn.Notifications.Core/Services/SmsNotificationService.cs:71-91
Timestamp: 2025-09-17T08:45:54.485Z
Learning: In Altinn.Notifications, the IKafkaProducer.ProduceAsync method includes internal try-catch handling and returns false if any exception occurs during publishing. This means external callers can rely on the boolean return value rather than needing explicit exception handling. Additionally, once GetNewNotifications() claims a batch of SMS notifications (atomically changing status from "New" to "Sending" via SQL), the design expects those claimed notifications to be processed to completion or reset to "New" status, rather than supporting mid-batch cancellation.
Applied to files:
Tools/Kafka/CommonProducer.csToolsTests/CommonProducerTests.csTools/Kafka/ICommonProducer.csTools/Util.cstest/Altinn.Notifications.IntegrationTests/Notifications.Persistence/DeadDeliveryReportRepositoryTests.cs
📚 Learning: 2025-09-01T12:20:45.235Z
Learnt from: Ahmed-Ghanam
Repo: Altinn/altinn-notifications PR: 1012
File: src/Altinn.Notifications.Integrations/Kafka/Consumers/EmailStatusConsumer.cs:59-61
Timestamp: 2025-09-01T12:20:45.235Z
Learning: In Altinn.Notifications EmailStatusConsumer, the EmailSendOperationResult messages contain non-sensitive information according to the maintainer Ahmed-Ghanam, so logging the full message payload is acceptable.
Applied to files:
Tools/Kafka/CommonProducer.csTools/Util.cs
📚 Learning: 2025-10-06T09:01:36.029Z
Learnt from: Ahmed-Ghanam
Repo: Altinn/altinn-notifications PR: 1066
File: src/Altinn.Notifications.Persistence/Migration/v0.58/02-functions-and-procedures.sql:0-0
Timestamp: 2025-10-06T09:01:36.029Z
Learning: In Altinn.Notifications EmailNotificationRepository: When both `_alternateid` and `operationid` are provided to the `updateemailnotification` function, the system should identify the row by `_alternateid`, set the `operationid`, result, and result time, then return the `_alternateid`. The `_alternateid` takes priority over `operationid` for row identification.
Applied to files:
Tools/Kafka/CommonProducer.cs
📚 Learning: 2025-08-28T14:19:36.888Z
Learnt from: Ahmed-Ghanam
Repo: Altinn/altinn-notifications PR: 1010
File: src/Altinn.Notifications.Persistence/Repository/EmailNotificationRepository.cs:28-34
Timestamp: 2025-08-28T14:19:36.888Z
Learning: In Altinn.Notifications EmailNotificationRepository: Both `alternateid` and `operationid` are unique per row in the notifications.emailnotifications table, and when both parameters are provided, they must always point to the same row. This means OR conditions between these fields are safe and won't cause multi-row updates.
Applied to files:
Tools/Kafka/CommonProducer.csTools/PostgresUtil.cs
📚 Learning: 2025-09-15T09:58:06.991Z
Learnt from: Ahmed-Ghanam
Repo: Altinn/altinn-notifications PR: 1015
File: src/Altinn.Notifications.Integrations/Kafka/Consumers/NotificationStatusConsumerBase.cs:158-161
Timestamp: 2025-09-15T09:58:06.991Z
Learning: In Altinn.Notifications status consumers, Kafka messages for status updates contain only references and send-status information (non-sensitive data), so logging the full Kafka message payload is acceptable according to maintainer Ahmed-Ghanam.
Applied to files:
Tools/Kafka/CommonProducer.csTools/Util.cstest/Altinn.Notifications.IntegrationTests/Notifications.Persistence/DeadDeliveryReportRepositoryTests.cs
📚 Learning: 2025-10-06T09:01:36.029Z
Learnt from: Ahmed-Ghanam
Repo: Altinn/altinn-notifications PR: 1066
File: src/Altinn.Notifications.Persistence/Migration/v0.58/02-functions-and-procedures.sql:0-0
Timestamp: 2025-10-06T09:01:36.029Z
Learning: In Altinn.Notifications EmailNotificationRepository: When `_alternateid` is provided to `updateemailnotification` function but the corresponding row no longer exists, the system redirects the delivery report to a retry topic and then to the dead delivery reports table (see issue #1017). No fallback to `operationid` should occur in this scenario.
Applied to files:
Tools/Kafka/CommonProducer.csTools/Util.cstest/Altinn.Notifications.IntegrationTests/Notifications.Persistence/DeadDeliveryReportRepositoryTests.cs
📚 Learning: 2025-11-12T08:49:13.705Z
Learnt from: eskebab
Repo: Altinn/altinn-notifications PR: 1148
File: src/Altinn.Notifications.Persistence/Migration/v0.67/01-functions-and-procedures.sql:632-644
Timestamp: 2025-11-12T08:49:13.705Z
Learning: In the Altinn/altinn-notifications repository, files like `src/Altinn.Notifications.Persistence/Migration/v0.67/01-functions-and-procedures.sql` are auto-generated from DbTools and contain many deprecated functions. When reviewing these migration files, only focus on the specific functions relevant to the PR's objective (e.g., for issue #1122 concurrent execution work) rather than reviewing all functions in the file.
Applied to files:
Tools/EventGrid/EventGridClient.cs
📚 Learning: 2025-08-28T13:53:37.392Z
Learnt from: eskebab
Repo: Altinn/altinn-notifications PR: 1011
File: src/Altinn.Notifications.Persistence/Migration/v0.55/01-functions-and-procedures.sql:0-0
Timestamp: 2025-08-28T13:53:37.392Z
Learning: In the Altinn/altinn-notifications repository, files named "functions-and-procedures.sql" (like src/Altinn.Notifications.Persistence/Migration/v0.55/01-functions-and-procedures.sql) are auto-generated based on scripts and used by Yuniql for database migrations. The standalone SQL files (like get_metrics_v2.sql) are the source of truth, so apparent "duplicates" between these files are intentional and part of the build process, not issues to be fixed.
Applied to files:
Tools/EventGrid/EventGridClient.cs
📚 Learning: 2025-05-19T14:19:20.558Z
Learnt from: eskebab
Repo: Altinn/altinn-notifications PR: 850
File: src/Altinn.Notifications.Persistence/Migration/v0.44/01-new-table.sql:1-1
Timestamp: 2025-05-19T14:19:20.558Z
Learning: In the Altinn/altinn-notifications repository, explicit schema creation statements (`CREATE SCHEMA IF NOT EXISTS`) are not necessary in SQL migration scripts as the schema existence is likely handled elsewhere in the deployment process.
Applied to files:
Tools/EventGrid/EventGridClient.cs
📚 Learning: 2025-10-06T08:33:51.260Z
Learnt from: martivj
Repo: Altinn/altinn-notifications PR: 1066
File: src/Altinn.Notifications.Persistence/Migration/v0.58/01-setup-indexes.sql:0-0
Timestamp: 2025-10-06T08:33:51.260Z
Learning: In the Altinn/altinn-notifications repository, the automated migration runner cannot use `CREATE INDEX CONCURRENTLY` because it runs migrations within transaction blocks. When applying index creation migrations manually to production, `CREATE INDEX CONCURRENTLY` should be used to avoid blocking writes.
Applied to files:
Tools/EventGrid/EventGridClient.cs
📚 Learning: 2025-09-16T06:48:40.494Z
Learnt from: Ahmed-Ghanam
Repo: Altinn/altinn-notifications PR: 1021
File: src/Altinn.Notifications.Core/Services/SmsNotificationService.cs:0-0
Timestamp: 2025-09-16T06:48:40.494Z
Learning: In Altinn.Notifications SMS processing (SmsNotificationService.SendNotifications), when GetNewNotifications() retrieves SMS notifications, their status is atomically changed from "New" to "Sending". If publishing to Kafka fails (ProduceAsync returns false or throws), the status must be reset to "New" via UpdateSendStatus to allow retry processing in future batches. This prevents notifications from getting stuck in "Sending" state.
Applied to files:
Tools/Util.cs
📚 Learning: 2025-10-09T10:24:50.624Z
Learnt from: eskebab
Repo: Altinn/altinn-notifications PR: 1068
File: src/Altinn.Notifications.Integrations/Kafka/Consumers/EmailStatusConsumer.cs:27-27
Timestamp: 2025-10-09T10:24:50.624Z
Learning: In Altinn.Notifications status consumers (EmailStatusConsumer, SmsStatusConsumer), the base constructor accepts three topic parameters: topicName (to consume from), retryTopicName (for general exceptions - typically the same as topicName for immediate retry), and sendStatusUpdateRetryTopicName (for SendStatusUpdateException - routed to dedicated retry topics like EmailStatusUpdatedRetryTopicName). This implements a two-tier retry strategy: immediate retries via same topic, and threshold-based retries via dedicated retry consumer.
Applied to files:
Tools/Util.cs
📚 Learning: 2025-09-18T12:34:10.363Z
Learnt from: Ahmed-Ghanam
Repo: Altinn/altinn-notifications PR: 1008
File: test/k6/src/tests/order-with-reminders-for-persons.js:39-41
Timestamp: 2025-09-18T12:34:10.363Z
Learning: The 'missingResource' order type has been removed from the Altinn notifications system and should not be included in any test implementations, including the persons test file (test/k6/src/tests/order-with-reminders-for-persons.js). The persons test should only support 'valid', 'invalid', and 'duplicate' order types, following the same pattern as the email address and mobile number tests.
Applied to files:
test/Altinn.Notifications.IntegrationTests/Notifications.Persistence/DeadDeliveryReportRepositoryTests.cs
📚 Learning: 2025-11-19T08:43:50.344Z
Learnt from: eskebab
Repo: Altinn/altinn-notifications PR: 1141
File: src/Altinn.Notifications.Persistence/Repository/NotificationRepositoryBase.cs:221-221
Timestamp: 2025-11-19T08:43:50.344Z
Learning: In Altinn.Notifications NotificationRepositoryBase.ReadRecipients: The team has decided not to add a null-guard for the status column when reading from the database. If status is NULL, it indicates data corruption and the code should throw an exception (InvalidCastException from GetFieldValueAsync<string>) to surface the issue immediately, rather than handling it gracefully.
Applied to files:
test/Altinn.Notifications.IntegrationTests/Notifications.Persistence/DeadDeliveryReportRepositoryTests.cs
📚 Learning: 2025-07-16T15:22:50.953Z
Learnt from: Ahmed-Ghanam
Repo: Altinn/altinn-notifications PR: 940
File: test/Altinn.Notifications.IntegrationTests/Notifications/InstantOrdersController/InstantOrdersControllerTests.cs:506-543
Timestamp: 2025-07-16T15:22:50.953Z
Learning: In the Altinn Notifications system, [Required] attributes on model classes like ShortMessageContentExt, ShortMessageDeliveryDetailsExt, InstantNotificationRecipientExt, and InstantNotificationOrderRequestExt cause deserialization to fail when the provided JSON string lacks the required values. This is different from typical ASP.NET Core behavior where [Required] attributes cause model validation failures after successful deserialization.
Applied to files:
test/Altinn.Notifications.IntegrationTests/Notifications.Persistence/DeadDeliveryReportRepositoryTests.cs
📚 Learning: 2025-09-02T09:27:21.022Z
Learnt from: Ahmed-Ghanam
Repo: Altinn/altinn-notifications PR: 1015
File: src/Altinn.Notifications.Integrations/Kafka/Consumers/EmailStatusConsumer.cs:28-35
Timestamp: 2025-09-02T09:27:21.022Z
Learning: In Altinn.Notifications consumers, using concrete logger types like ILogger<EmailStatusConsumer> instead of the base class generic logger type is the preferred pattern. This provides clearer log categories while still satisfying DI requirements, as the .NET DI container handles the type mapping correctly.
Applied to files:
test/Altinn.Notifications.IntegrationTests/Notifications.Persistence/DeadDeliveryReportRepositoryTests.cs
📚 Learning: 2025-09-02T09:27:21.022Z
Learnt from: Ahmed-Ghanam
Repo: Altinn/altinn-notifications PR: 1015
File: src/Altinn.Notifications.Integrations/Kafka/Consumers/EmailStatusConsumer.cs:28-35
Timestamp: 2025-09-02T09:27:21.022Z
Learning: In Altinn.Notifications EmailStatusConsumer, using ILogger<EmailStatusConsumer> instead of the base class generic logger type is intentional and preferred, as it provides clearer log categories while still satisfying DI requirements through the container's type mapping capabilities.
Applied to files:
test/Altinn.Notifications.IntegrationTests/Notifications.Persistence/DeadDeliveryReportRepositoryTests.cs
📚 Learning: 2025-09-18T10:37:21.188Z
Learnt from: Ahmed-Ghanam
Repo: Altinn/altinn-notifications PR: 1008
File: test/k6/src/data/orders/order-with-reminders-for-organizations.json:34-38
Timestamp: 2025-09-18T10:37:21.188Z
Learning: In Altinn.Notifications test data files, channelSchema enum values can use both "SMS" and "Sms" formats - both are valid in the system, contrary to initial assumptions about strict camel-case requirements.
Applied to files:
test/Altinn.Notifications.IntegrationTests/Notifications.Persistence/DeadDeliveryReportRepositoryTests.cs
📚 Learning: 2025-06-13T14:04:57.506Z
Learnt from: SandGrainOne
Repo: Altinn/altinn-notifications PR: 895
File: src/Altinn.Notifications/Program.cs:215-233
Timestamp: 2025-06-13T14:04:57.506Z
Learning: The Altinn.Notifications team intends to replace the current client-secret based Key Vault integration with Workload Identity authentication in a future update; the present code is a temporary measure.
Applied to files:
test/Altinn.Notifications.IntegrationTests/Notifications.Persistence/DeadDeliveryReportRepositoryTests.cs
📚 Learning: 2025-10-15T08:05:01.433Z
Learnt from: Ahmed-Ghanam
Repo: Altinn/altinn-notifications PR: 1091
File: test/Altinn.Notifications.IntegrationTests/Notifications.Integrations/TestingConsumers/NotificationStatusConsumerBaseTests.cs:0-0
Timestamp: 2025-10-15T08:05:01.433Z
Learning: In Altinn.Notifications NotificationStatusConsumerBase.ProcessStatus: ArgumentException and InvalidOperationException are caught and logged with message-only (no exception details). The LogProcessingError method intentionally does not pass the exception parameter to the logger. Tests should verify logging occurred without expecting the exception parameter.
Applied to files:
test/Altinn.Notifications.IntegrationTests/Notifications.Persistence/DeadDeliveryReportRepositoryTests.cs
📚 Learning: 2025-04-22T14:33:38.561Z
Learnt from: Ahmed-Ghanam
Repo: Altinn/altinn-notifications PR: 806
File: src/Altinn.Notifications.Persistence/Repository/NotificationDeliveryManifestRepository.cs:0-0
Timestamp: 2025-04-22T14:33:38.561Z
Learning: The team has decided to remove the StatusDescription property from NotificationDeliveryManifest and not implement status parsing in the Altinn.Notifications codebase. Status values should be used as-is without additional parsing or description text.
Applied to files:
test/Altinn.Notifications.IntegrationTests/Notifications.Persistence/DeadDeliveryReportRepositoryTests.cs
🧬 Code graph analysis (10)
Tools/EventGridUtil.cs (1)
Tools/Util.cs (1)
Util(9-96)
Tools/Kafka/CommonProducer.cs (3)
src/Altinn.Notifications.Integrations/Kafka/Producers/KafkaProducer.cs (1)
ProducerConfig(279-297)Tools/Util.cs (2)
Task(27-44)Task(46-78)Tools/Kafka/ICommonProducer.cs (1)
Task(13-13)
ToolsTests/CommonProducerTests.cs (2)
Tools/Kafka/CommonProducer.cs (7)
Task(57-80)CommonProducer(13-125)CommonProducer(23-42)CommonProducer(47-54)EnsureTopicsExist(90-99)EnsureTopicsExist(106-124)Dispose(83-88)Tools/Kafka/ICommonProducer.cs (1)
Task(13-13)
ToolsTests/EventGridClientTests.cs (2)
Tools/EventGrid/EventGridClient.cs (3)
Task(43-75)EventGridSettings(12-16)EventGridClient(28-35)Tools/EventGrid/IEventGridClient.cs (1)
Task(11-13)
Tools/PostgresUtil.cs (1)
test/Altinn.Notifications.IntegrationTests/Utils/ServiceUtil.cs (1)
NpgsqlDataSource(22-47)
Tools/ConfigurationUtil.cs (3)
src/Altinn.Notifications.Persistence/Configuration/PostgreSQLSettings.cs (1)
PostgreSqlSettings(6-47)test/Altinn.Notifications.IntegrationTests/Utils/ServiceUtil.cs (1)
NpgsqlDataSource(22-47)Tools/EventGrid/EventGridClient.cs (2)
EventGridSettings(12-16)EventGridClient(28-35)
Tools/Kafka/ICommonProducer.cs (2)
Tools/Util.cs (2)
Task(27-44)Task(46-78)Tools/Kafka/CommonProducer.cs (1)
Task(57-80)
Tools/EventGrid/EventGridClient.cs (1)
Tools/EventGrid/IEventGridClient.cs (1)
Task(11-13)
Tools/Util.cs (7)
Tools/PostgresUtil.cs (1)
Task(9-25)Tools/EventGrid/EventGridClient.cs (1)
Task(43-75)Tools/EventGrid/IEventGridClient.cs (1)
Task(11-13)Tools/Kafka/CommonProducer.cs (1)
Task(57-80)Tools/Kafka/ICommonProducer.cs (1)
Task(13-13)src/Altinn.Notifications.Core/Persistence/IDeadDeliveryReportRepository.cs (3)
Task(18-18)Task(32-32)Task(40-40)src/Altinn.Notifications.Persistence/Repository/DeadDeliveryReportRepository.cs (3)
Task(20-37)Task(40-68)Task(71-102)
test/Altinn.Notifications.IntegrationTests/Notifications.Persistence/DeadDeliveryReportRepositoryTests.cs (2)
src/Altinn.Notifications.Core/Persistence/IDeadDeliveryReportRepository.cs (3)
Task(18-18)Task(32-32)Task(40-40)src/Altinn.Notifications.Persistence/Repository/DeadDeliveryReportRepository.cs (4)
Task(20-37)Task(40-68)Task(71-102)DeadDeliveryReportRepository(12-103)
🪛 GitHub Check: CodeQL
Tools/Kafka/CommonProducer.cs
[warning] 68-68: Exposure of private information
Private data returned by call to method Serialize is written to an external location.
🔇 Additional comments (24)
test/Altinn.Notifications.IntegrationTests/Notifications.Persistence/DeadDeliveryReportRepositoryTests.cs (1)
15-15: LGTM! Primary constructor syntax cleanup.Removing the unnecessary empty primary constructor syntax improves code clarity without affecting functionality.
Tools/EventGrid/IEventGridClient.cs (1)
1-14: LGTM! Clean Event Grid client interface.The interface design is well-structured with appropriate generic support, cancellation token, and informative return type.
Tools/appsettings.json (1)
1-32: LGTM! Configuration structure is appropriate.The configuration properly uses null placeholders for sensitive values, with User Secrets support enabled in the project file (UserSecretsId: 80322a93-a2dd-480a-8417-6655f767c6f9). Ensure that sensitive values are loaded from User Secrets or environment variables at runtime.
Tools/Kafka/CommonProducer.cs (1)
82-88: LGTM! Proper disposal pattern.The Dispose implementation correctly flushes pending messages and disposes the producer.
Tools/Tools.csproj (1)
1-35: LGTM! Project configuration is appropriate.The project structure and dependencies are well-configured for a .NET 9.0 console application with Event Grid, Kafka, and PostgreSQL integration. User Secrets support is properly enabled.
Tools/AssemblyInfo.cs (1)
1-3: LGTM! Standard test visibility configuration.The
InternalsVisibleToattribute appropriately grants the test project access to internal members.ToolsTests/ToolsTests.csproj (1)
1-28: LGTM! Test project properly configured.The test project has appropriate dependencies for unit testing with xUnit, Moq, and necessary infrastructure libraries.
Tools/Kafka/ICommonProducer.cs (1)
1-14: LGTM! Clean producer interface.The interface is well-designed with clear method signature and documentation. The boolean return type aligns with established patterns for producer error handling.
Tools/PostgresUtil.cs (1)
9-25: LGTM! Implementation correctly filters for 'Succeeded' state.The parameterized SQL query properly checks for
result = 'Succeeded'which aligns with the business requirement that only notifications in the "Succeeded" state should be processed. Based on learnings, this is the correct approach rather than checking for absence of 'Delivered'.The resource management with
await usingis appropriate. Consider adding a query timeout for long-running operations if this tool is used against large datasets.ToolsTests/CommonProducerTests.cs (2)
1-52: LGTM! Well-structured test setup and ProduceAsync success path test.The test properly verifies the ProduceAsync behavior when the producer persists the message, including verification of the correct topic and message being passed.
121-173: Good coverage of EnsureTopicsExist scenarios.The tests thoroughly cover topic creation logic including: existing topics, missing topics with correct specifications, and verification that TopicSpecification properties match SharedClientConfig values.
ToolsTests/EventGridClientTests.cs (2)
8-18: Good minimal FakeHandler implementation.The FakeHandler correctly implements HttpMessageHandler to return predetermined responses for testing.
20-59: LGTM! Tests cover success and error response paths.Both tests properly verify the return tuple values for success and failure scenarios.
ToolsTests/UtilTests.cs (3)
13-87: LGTM! Good coverage of MapToEmailSendOperationResult edge cases.Tests properly cover successful deserialization, invalid JSON, and empty delivery reports.
89-267: LGTM! Comprehensive tests for GetAndMapDeadDeliveryReports.Good coverage of empty results, valid results, filtering of invalid reports, and parameter verification.
269-406: LGTM! Thorough ProduceMessagesToKafka tests.Tests cover null/empty topic handling, empty results, full success, partial success, and serialization verification.
Tools/EventGrid/EventGridClient.cs (1)
43-75: LGTM! PostEventsAsync implementation is solid.The method properly handles success/failure cases with appropriate logging and exception handling. The access key in URL query parameter was previously reviewed and confirmed acceptable for this console application context.
Tools/ConfigurationUtil.cs (2)
19-26: LGTM! Clean orchestration of configuration setup.The method provides a clear, well-organized flow for service configuration with logical separation of concerns.
58-72: Good validation for EventGrid BaseUrl.The EventGrid configuration correctly validates that
BaseUrlis configured before attempting to use it (lines 64-67), providing a clear error message. This follows the same pattern that should be applied to the database connection string.Tools/EventGridUtil.cs (2)
11-28: Clarify the purpose of the 100ms delay between events.Line 24 introduces a 100ms delay between processing each event. For large batches, this could significantly impact performance (e.g., 1000 events = 100 seconds of artificial delay). If this is rate-limiting for the Event Grid endpoint, consider documenting the rationale or making it configurable. If it's not needed, remove it.
What is the purpose of this delay? Is it:
- Rate limiting to avoid overwhelming Event Grid?
- Ensuring events are posted in order?
- Something else?
If rate limiting is needed, consider a more sophisticated approach like batching events or using a token bucket algorithm.
30-40: Good defensive check before posting events.The eligibility check using
IsEmailNotificationInSucceededStatecorrectly prevents posting events for notifications that are not in the "Succeeded" state, aligning with the learned constraint that only "Succeeded" notifications should be processed.Based on learnings, this correctly implements the requirement to only process notifications in "Succeeded" state.
Tools/Util.cs (3)
13-25: Correct error handling pattern for unparseable messages.The method returns
nullfor deserialization failures and logs the error (lines 20-23). This follows the correct pattern for handling invalid/unparseable data in the Altinn.Notifications codebase—such messages should not be retried.Based on learnings, this correctly avoids triggering retry mechanisms for unparseable messages.
46-78: Good Kafka message production with proper error tracking.The method correctly handles the boolean return value from
ProduceAsync(lines 62-74), tracks success/failure counts, and logs outcomes per message. The early return when topic is unconfigured (lines 54-58) prevents unnecessary processing.Based on learnings, this correctly relies on the boolean return value from ProduceAsync rather than exception handling.
83-95: No changes required; code is correct for actual enum definition.The string comparison at line 89 correctly matches the actual enum value
EmailNotificationResultType.Failed_SupressedRecipient(single 's'). The test at ToolsTests/UtilTests.cs line 437 also confirms this is the correct spelling used throughout the codebase for this enum value. There is no typo in the code.Likely an incorrect or invalid review comment.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
|
2 participants
Description
Related Issue(s)
Verification
Documentation
Summary by CodeRabbit
Release Notes
New Features
Infrastructure
✏️ Tip: You can customize this high-level summary in your review settings.