⚠️ Feat/broker report #810
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
added 2 commits
November 10, 2025 08:18
Contributor
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughAdds a statistics API with two POST endpoints to generate and optionally download daily Parquet summary reports; implements a handler that aggregates file-transfer data, Parquet generation and optional upload/download, API-key + per‑IP rate limiting filter, SSN validation helper, new enums/DTOs/repos, DI registration, Parquet dependency and infra secrets. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ 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)
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 |
mSunberg
added
the
kind/feature
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (3)
src/Altinn.Broker.Core/Repositories/IFileTransferRepository.cs (1)
33-33: Consider adding XML documentation for the new method.The method name is generic and doesn't indicate what filtering criteria or scope is applied. Consider adding XML documentation to clarify:
- What file transfers are included (all? filtered by date/status?)
- Whether this includes enriched data (sender, recipients, etc.)
src/Altinn.Broker.Common/StringExtensions.cs (2)
10-10: Remove unused element from weight array.The
SocialSecurityNumberWeights1array contains 10 elements, but the validation loop at line 53 only uses the first 9 elements (indices 0-8). The 10th element (value1at index 9) is never accessed.Apply this diff to remove the unused element:
- private static readonly int[] SocialSecurityNumberWeights1 = [3, 7, 6, 1, 8, 9, 4, 5, 2, 1]; + private static readonly int[] SocialSecurityNumberWeights1 = [3, 7, 6, 1, 8, 9, 4, 5, 2];
46-49: Remove redundant length check.The length check is redundant because
SsnPatternat line 9 uses^\d{11}$, which matches exactly 11 digits. IfSsnPattern.IsMatch(identifier)passes at line 40, the identifier is guaranteed to be exactly 11 characters long.Apply this diff to remove the redundant check:
- // Mod11 validation - if (identifier.Length < 11) - { - return false; - } - // Calculate first control digit
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
src/Altinn.Broker.API/Controllers/StatisticsController.cs(1 hunks)src/Altinn.Broker.API/Filters/StatisticsApiKeyFilter.cs(1 hunks)src/Altinn.Broker.API/Program.cs(2 hunks)src/Altinn.Broker.Application/Altinn.Broker.Application.csproj(1 hunks)src/Altinn.Broker.Application/DependencyInjection.cs(2 hunks)src/Altinn.Broker.Application/Errors.cs(1 hunks)src/Altinn.Broker.Application/GenerateReport/DailySummaryData.cs(1 hunks)src/Altinn.Broker.Application/GenerateReport/GenerateAndDownloadDailySummaryReportResponse.cs(1 hunks)src/Altinn.Broker.Application/GenerateReport/GenerateDailySummaryReportHandler.cs(1 hunks)src/Altinn.Broker.Application/GenerateReport/GenerateDailySummaryReportRequest.cs(1 hunks)src/Altinn.Broker.Application/GenerateReport/GenerateDailySummaryReportResponse.cs(1 hunks)src/Altinn.Broker.Application/GenerateReport/ParquetDailySummaryData.cs(1 hunks)src/Altinn.Broker.Common/StringExtensions.cs(2 hunks)src/Altinn.Broker.Core/Domain/Enums/AltinnVersion.cs(1 hunks)src/Altinn.Broker.Core/Domain/Enums/RecipientType.cs(1 hunks)src/Altinn.Broker.Core/Repositories/IAltinnResourceRepoistory.cs(1 hunks)src/Altinn.Broker.Core/Repositories/IFileTransferRepository.cs(1 hunks)src/Altinn.Broker.Integrations/Altinn/ResourceRegistry/AltinnResourceRegistryRepository.cs(1 hunks)src/Altinn.Broker.Persistence/Repositories/FileTransferRepository.cs(1 hunks)
🧰 Additional context used
🧠 Learnings (17)
📚 Learning: 2024-10-29T12:12:50.346Z
Learnt from: Ceredron
Repo: Altinn/altinn-broker PR: 560
File: tests/Altinn.Broker.Tests/Factories/FileTransferEntityFactory.cs:8-45
Timestamp: 2024-10-29T12:12:50.346Z
Learning: When suggesting code improvements in `tests/Altinn.Broker.Tests/Factories/FileTransferEntityFactory.cs`, avoid adding excessive comments and unnecessary code changes.
Applied to files:
src/Altinn.Broker.Application/Errors.cssrc/Altinn.Broker.Application/DependencyInjection.cssrc/Altinn.Broker.Application/GenerateReport/GenerateDailySummaryReportRequest.cssrc/Altinn.Broker.Core/Repositories/IFileTransferRepository.cssrc/Altinn.Broker.Application/GenerateReport/GenerateAndDownloadDailySummaryReportResponse.cssrc/Altinn.Broker.Application/Altinn.Broker.Application.csprojsrc/Altinn.Broker.Persistence/Repositories/FileTransferRepository.cssrc/Altinn.Broker.API/Program.cssrc/Altinn.Broker.Common/StringExtensions.cssrc/Altinn.Broker.Application/GenerateReport/GenerateDailySummaryReportHandler.cs
📚 Learning: 2024-10-29T12:50:05.699Z
Learnt from: Ceredron
Repo: Altinn/altinn-broker PR: 560
File: tests/Altinn.Broker.Tests/Helpers/ManifestDownloadStreamHelpers.cs:18-28
Timestamp: 2024-10-29T12:50:05.699Z
Learning: In `tests/Altinn.Broker.Tests/Helpers/ManifestDownloadStreamHelpers.cs`, the test code is static and will not change, so refactoring suggestions on this file are unnecessary.
Applied to files:
src/Altinn.Broker.Application/Errors.cssrc/Altinn.Broker.Application/DependencyInjection.cssrc/Altinn.Broker.Application/GenerateReport/GenerateAndDownloadDailySummaryReportResponse.cssrc/Altinn.Broker.Application/Altinn.Broker.Application.csprojsrc/Altinn.Broker.Persistence/Repositories/FileTransferRepository.cssrc/Altinn.Broker.Common/StringExtensions.cs
📚 Learning: 2024-11-14T07:12:14.214Z
Learnt from: Ceredron
Repo: Altinn/altinn-broker PR: 582
File: src/Altinn.Broker.Persistence/Repositories/FileTransferRepository.cs:176-177
Timestamp: 2024-11-14T07:12:14.214Z
Learning: In `src/Altinn.Broker.Persistence/Repositories/FileTransferRepository.cs`, avoid suggesting to extract inline SQL queries into constants.
Applied to files:
src/Altinn.Broker.Application/Errors.cssrc/Altinn.Broker.Application/DependencyInjection.cssrc/Altinn.Broker.Core/Repositories/IFileTransferRepository.cssrc/Altinn.Broker.Persistence/Repositories/FileTransferRepository.cs
📚 Learning: 2024-11-14T07:20:44.903Z
Learnt from: Ceredron
Repo: Altinn/altinn-broker PR: 582
File: src/Altinn.Broker.Persistence/Repositories/ServiceOwnerRepository.cs:30-31
Timestamp: 2024-11-14T07:20:44.903Z
Learning: In `src/Altinn.Broker.Persistence/Repositories/ServiceOwnerRepository.cs`, the `service_owner_name` column in the `service_owner` table is not nullable, so null checks before calling `reader.GetString` are unnecessary.
Applied to files:
src/Altinn.Broker.Integrations/Altinn/ResourceRegistry/AltinnResourceRegistryRepository.cssrc/Altinn.Broker.Core/Repositories/IAltinnResourceRepoistory.cs
📚 Learning: 2024-11-14T07:20:20.158Z
Learnt from: Ceredron
Repo: Altinn/altinn-broker PR: 582
File: src/Altinn.Broker.Persistence/Repositories/ServiceOwnerRepository.cs:36-47
Timestamp: 2024-11-14T07:20:20.158Z
Learning: In `src/Altinn.Broker.Persistence/Repositories/ServiceOwnerRepository.cs`, the `created` field retrieved from the database is non-nullable, so null checks for the `created` field are not necessary.
Applied to files:
src/Altinn.Broker.Integrations/Altinn/ResourceRegistry/AltinnResourceRegistryRepository.cssrc/Altinn.Broker.Core/Repositories/IAltinnResourceRepoistory.cs
📚 Learning: 2024-10-29T12:51:11.540Z
Learnt from: Ceredron
Repo: Altinn/altinn-broker PR: 560
File: tests/Altinn.Broker.Tests/ManifestDownloadStreamTests.cs:24-39
Timestamp: 2024-10-29T12:51:11.540Z
Learning: In `tests/Altinn.Broker.Tests/ManifestDownloadStreamTests.cs`, refactoring suggestions aimed at enhancing test organization and coverage may be considered less readable and of dubious value.
Applied to files:
src/Altinn.Broker.Integrations/Altinn/ResourceRegistry/AltinnResourceRegistryRepository.cssrc/Altinn.Broker.Application/DependencyInjection.cssrc/Altinn.Broker.Application/GenerateReport/GenerateAndDownloadDailySummaryReportResponse.cssrc/Altinn.Broker.Application/Altinn.Broker.Application.csprojsrc/Altinn.Broker.Persistence/Repositories/FileTransferRepository.cssrc/Altinn.Broker.API/Program.cssrc/Altinn.Broker.Common/StringExtensions.cssrc/Altinn.Broker.Application/GenerateReport/GenerateDailySummaryReportHandler.cs
📚 Learning: 2024-11-14T07:00:48.796Z
Learnt from: Ceredron
Repo: Altinn/altinn-broker PR: 582
File: tests/Altinn.Broker.Tests.LargeFile/Program.cs:80-82
Timestamp: 2024-11-14T07:00:48.796Z
Learning: In test programs like `tests/Altinn.Broker.Tests.LargeFile/Program.cs` in the `Altinn.Broker` project, it's acceptable to proceed without checking the success of HTTP responses or adding additional error handling.
Applied to files:
src/Altinn.Broker.Application/DependencyInjection.cssrc/Altinn.Broker.API/Program.cs
📚 Learning: 2024-10-29T11:25:42.867Z
Learnt from: Andreass2
Repo: Altinn/altinn-broker PR: 560
File: src/Altinn.Broker.Application/DownloadFile/DownloadFileHandler.cs:49-55
Timestamp: 2024-10-29T11:25:42.867Z
Learning: In `DownloadFileHandler` in `src/Altinn.Broker.Application/DownloadFile/DownloadFileHandler.cs`, due to the 1 GB file size limit, loading the entire file into memory is acceptable in this context.
Applied to files:
src/Altinn.Broker.Application/DependencyInjection.cssrc/Altinn.Broker.Application/GenerateReport/GenerateAndDownloadDailySummaryReportResponse.cssrc/Altinn.Broker.Application/GenerateReport/GenerateDailySummaryReportHandler.cs
📚 Learning: 2024-11-14T07:00:59.444Z
Learnt from: Ceredron
Repo: Altinn/altinn-broker PR: 582
File: src/Altinn.Broker.Core/Domain/ServiceOwnerEntity.cs:11-21
Timestamp: 2024-11-14T07:00:59.444Z
Learning: In the Altinn.Broker project, exceptions should not be used for control flow or logic; prefer returning null or appropriate results instead.
Applied to files:
src/Altinn.Broker.Application/DependencyInjection.cssrc/Altinn.Broker.API/Program.cs
📚 Learning: 2024-11-27T14:04:55.778Z
Learnt from: RagnarFatland-Avanade
Repo: Altinn/altinn-broker PR: 593
File: src/Altinn.Broker.Application/ExpireFileTransfer/ExpireFileTransferHandler.cs:96-96
Timestamp: 2024-11-27T14:04:55.778Z
Learning: In the Altinn Broker codebase, when throwing exceptions in C# code, prefer to use generic exceptions rather than creating custom exception classes, aiming to keep the code simple.
Applied to files:
src/Altinn.Broker.Application/DependencyInjection.cssrc/Altinn.Broker.API/Program.cs
📚 Learning: 2024-10-29T10:43:16.160Z
Learnt from: Andreass2
Repo: Altinn/altinn-broker PR: 561
File: src/Altinn.Broker.API/Controllers/LegacyFileController.cs:80-81
Timestamp: 2024-10-29T10:43:16.160Z
Learning: In the Altinn Broker API, specifically in `src/Altinn.Broker.API/Controllers/LegacyFileController.cs`, when handling file uploads in the `UploadFileStreamed` method, we can assume that all uploaded files are small and that the `Content-Length` header is always provided. Therefore, additional error handling for missing `Content-Length` is not required.
Applied to files:
src/Altinn.Broker.Application/DependencyInjection.cs
📚 Learning: 2025-09-30T11:29:20.339Z
Learnt from: tomshag
Repo: Altinn/altinn-broker PR: 790
File: src/Altinn.Broker.Persistence/Repositories/FileTransferRepository.cs:349-352
Timestamp: 2025-09-30T11:29:20.339Z
Learning: In the Altinn Broker file transfer system, file transfer status and actor file transfer status use status_description_id_fk values that are ordered by progression level. Ordering by status_description_id_fk DESC is intentional to get the highest/most advanced status, not the most recent by date. This applies to queries in FileTransferRepository.cs.
Applied to files:
src/Altinn.Broker.Application/DependencyInjection.cssrc/Altinn.Broker.Persistence/Repositories/FileTransferRepository.cs
📚 Learning: 2024-11-14T07:01:04.663Z
Learnt from: Ceredron
Repo: Altinn/altinn-broker PR: 582
File: tests/Altinn.Broker.Tests.LargeFile/Program.cs:147-147
Timestamp: 2024-11-14T07:01:04.663Z
Learning: In the project `Altinn.Broker.Tests.LargeFile`, the codebase uses C# 12 features like collection expressions, such as initializing collections with `PropertyList = []`. Do not flag this syntax as invalid in future reviews.
Applied to files:
src/Altinn.Broker.Application/Altinn.Broker.Application.csprojsrc/Altinn.Broker.Common/StringExtensions.cs
📚 Learning: 2024-10-29T12:15:40.824Z
Learnt from: Ceredron
Repo: Altinn/altinn-broker PR: 560
File: src/Altinn.Broker.Core/Helpers/ManifestDownloadStream.cs:239-249
Timestamp: 2024-10-29T12:15:40.824Z
Learning: In the `ManifestDownloadStream` class (`src/Altinn.Broker.Core/Helpers/ManifestDownloadStream.cs`), ZIP file validation is performed later in the code within the `AddManifestFile` method, so enhancing the `IsZipFile` method is unnecessary.
Applied to files:
src/Altinn.Broker.Application/Altinn.Broker.Application.csprojsrc/Altinn.Broker.Common/StringExtensions.cs
📚 Learning: 2024-10-29T12:45:23.330Z
Learnt from: Ceredron
Repo: Altinn/altinn-broker PR: 560
File: src/Altinn.Broker.Core/Helpers/ManifestDownloadStream.cs:199-237
Timestamp: 2024-10-29T12:45:23.330Z
Learning: In the `ManifestDownloadStream` class (`src/Altinn.Broker.Core/Helpers/ManifestDownloadStream.cs`), optimizing memory usage in `AddManifestFile` by modifying the ZIP archive without loading the entire content into memory is currently not feasible because the base stream is a `ReadOnlyMemoryStream` based on a download stream. This optimization will be evaluated when the download stream is re-written in the "Support bigger files"-PR.
Applied to files:
src/Altinn.Broker.Persistence/Repositories/FileTransferRepository.cs
📚 Learning: 2024-12-08T22:55:30.559Z
Learnt from: Ceredron
Repo: Altinn/altinn-broker PR: 610
File: src/Altinn.Broker.Common/StringExtensions.cs:23-30
Timestamp: 2024-12-08T22:55:30.559Z
Learning: In `src/Altinn.Broker.Common/StringExtensions.cs`, the `WithoutPrefix` method in the C# project checks if `orgNumber` is null or whitespace and returns an empty string if it is. Therefore, calling `orgNumber.Split(':').Last()` is safe, as the method ensures `orgNumber` is non-empty before splitting.
Applied to files:
src/Altinn.Broker.Common/StringExtensions.cs
📚 Learning: 2024-12-08T23:15:58.682Z
Learnt from: Ceredron
Repo: Altinn/altinn-broker PR: 610
File: src/Altinn.Broker.Core/StringExtensions.cs:0-0
Timestamp: 2024-12-08T23:15:58.682Z
Learning: In `src/Altinn.Broker.Core/StringExtensions.cs`, prefer keeping the `WithoutPrefix` method simple and readable, avoiding complex modifications that reduce readability.
Applied to files:
src/Altinn.Broker.Common/StringExtensions.cs
🧬 Code graph analysis (11)
src/Altinn.Broker.Integrations/Altinn/ResourceRegistry/AltinnResourceRegistryRepository.cs (2)
src/Altinn.Broker.Core/Repositories/IAltinnResourceRepoistory.cs (2)
Task(6-6)Task(12-12)src/Altinn.Broker.Integrations/Altinn/ResourceRegistry/GetResourceResponse.cs (2)
GetResourceResponse(5-57)HasCompetentAuthority(59-69)
src/Altinn.Broker.Application/DependencyInjection.cs (1)
src/Altinn.Broker.Application/GenerateReport/GenerateDailySummaryReportHandler.cs (1)
GenerateDailySummaryReportHandler(15-362)
src/Altinn.Broker.API/Controllers/StatisticsController.cs (4)
src/Altinn.Broker.API/Filters/StatisticsApiKeyFilter.cs (3)
StatisticsApiKeyFilter(12-234)StatisticsApiKeyFilter(21-31)Task(154-233)src/Altinn.Broker.Application/GenerateReport/GenerateDailySummaryReportResponse.cs (1)
GenerateDailySummaryReportResponse(3-45)src/Altinn.Broker.Application/GenerateReport/GenerateDailySummaryReportHandler.cs (6)
Task(23-68)Task(219-236)Task(238-268)Task(270-307)Task(309-361)GenerateDailySummaryReportHandler(15-362)src/Altinn.Broker.Application/GenerateReport/GenerateDailySummaryReportRequest.cs (1)
GenerateDailySummaryReportRequest(6-15)
src/Altinn.Broker.Core/Repositories/IFileTransferRepository.cs (2)
src/Altinn.Broker.Application/GenerateReport/GenerateDailySummaryReportHandler.cs (1)
List(70-125)src/Altinn.Broker.Core/Domain/FileTransferEntity.cs (1)
FileTransferEntity(3-21)
src/Altinn.Broker.Core/Domain/Enums/RecipientType.cs (1)
src/Altinn.Broker.Application/GenerateReport/GenerateDailySummaryReportHandler.cs (1)
RecipientType(141-164)
src/Altinn.Broker.Persistence/Repositories/FileTransferRepository.cs (4)
src/Altinn.Broker.Core/Domain/FileTransferEntity.cs (1)
FileTransferEntity(3-21)src/Altinn.Broker.Core/Domain/ActorEntity.cs (1)
ActorEntity(3-7)src/Altinn.Broker.Core/Domain/FileTransferStatusEntity.cs (1)
FileTransferStatusEntity(2-8)src/Altinn.Broker.Core/Domain/ActorFileTransferStatusEntity.cs (1)
ActorFileTransferStatusEntity(3-9)
src/Altinn.Broker.API/Program.cs (1)
src/Altinn.Broker.API/Filters/StatisticsApiKeyFilter.cs (2)
StatisticsApiKeyFilter(12-234)StatisticsApiKeyFilter(21-31)
src/Altinn.Broker.Application/GenerateReport/GenerateDailySummaryReportHandler.cs (10)
src/Altinn.Broker.Core/Repositories/IAltinnResourceRepoistory.cs (2)
Task(6-6)Task(12-12)src/Altinn.Broker.Integrations/Altinn/ResourceRegistry/AltinnResourceRegistryRepository.cs (2)
Task(28-62)Task(64-85)src/Altinn.Broker.Core/Repositories/IFileTransferRepository.cs (10)
Task(7-18)Task(19-19)Task(20-20)Task(21-21)Task(22-22)Task(23-23)Task(24-24)Task(25-31)Task(32-32)Task(33-33)src/Altinn.Broker.Persistence/Repositories/FileTransferRepository.cs (1)
Task(112-172)src/Altinn.Broker.Application/GenerateReport/GenerateDailySummaryReportResponse.cs (1)
GenerateDailySummaryReportResponse(3-45)src/Altinn.Broker.Application/Errors.cs (1)
StatisticsErrors(34-38)src/Altinn.Broker.Application/GenerateReport/DailySummaryData.cs (1)
DailySummaryData(9-77)src/Altinn.Broker.Common/StringExtensions.cs (3)
WithoutPrefix(77-84)IsOrganizationNumber(18-21)IsSocialSecurityNumber(28-31)src/Altinn.Broker.Application/GenerateReport/ParquetDailySummaryData.cs (1)
ParquetDailySummaryData(9-89)src/Altinn.Broker.Application/GenerateReport/GenerateAndDownloadDailySummaryReportResponse.cs (1)
GenerateAndDownloadDailySummaryReportResponse(3-50)
src/Altinn.Broker.API/Filters/StatisticsApiKeyFilter.cs (1)
src/Altinn.Broker.Core/Repositories/IFileTransferRepository.cs (10)
Task(7-18)Task(19-19)Task(20-20)Task(21-21)Task(22-22)Task(23-23)Task(24-24)Task(25-31)Task(32-32)Task(33-33)
src/Altinn.Broker.Application/GenerateReport/DailySummaryData.cs (1)
src/Altinn.Broker.Application/GenerateReport/GenerateDailySummaryReportHandler.cs (1)
RecipientType(141-164)
src/Altinn.Broker.Application/GenerateReport/ParquetDailySummaryData.cs (1)
src/Altinn.Broker.Application/GenerateReport/GenerateDailySummaryReportHandler.cs (1)
RecipientType(141-164)
🔇 Additional comments (11)
src/Altinn.Broker.Core/Domain/Enums/AltinnVersion.cs (1)
1-17: LGTM!The enum is well-defined with clear documentation and explicit values.
src/Altinn.Broker.API/Program.cs (2)
6-6: LGTM!Adding the using directive for the new filter namespace.
117-118: LGTM!Proper registration of the new statistics API key filter.
src/Altinn.Broker.Core/Domain/Enums/RecipientType.cs (1)
1-23: LGTM!Well-defined enum with clear documentation and explicit underlying type.
src/Altinn.Broker.Application/DependencyInjection.cs (2)
8-8: LGTM!Adding the using directive for the new report generation namespace.
35-35: LGTM!Proper registration of the new handler with scoped lifetime.
src/Altinn.Broker.Application/Errors.cs (1)
34-38: LGTM!The new error definitions follow the existing pattern and use appropriate HTTP status codes. The 6000 range nicely segregates statistics-related errors.
src/Altinn.Broker.Application/Altinn.Broker.Application.csproj (1)
15-15: Parquet.Net version 5.3.0 is current and secure.Version 5.3.0 is the latest stable release, and no known security vulnerabilities were detected. The package dependency is appropriately configured.
src/Altinn.Broker.Application/GenerateReport/GenerateDailySummaryReportHandler.cs (1)
127-204: Avoid blocking per-row metadata lookupsEach call to
GetServiceOwnerId,GetServiceOwnerName, andGetResourceTitledoes a synchronous.GetAwaiter().GetResult()on asynchronous repository methods inside the grouping loop. For large reports this triggers hundreds/thousands of sequential HTTP/database calls and blocks the request thread, risking massive latency or thread-pool starvation. Please refactor to fetch metadata asynchronously with properawait(e.g., prefetch distinct IDs withawait+ caching) so the aggregation remains scalable.⛔ Skipped due to learnings
Learnt from: Ceredron Repo: Altinn/altinn-broker PR: 582 File: src/Altinn.Broker.Persistence/Repositories/ServiceOwnerRepository.cs:30-31 Timestamp: 2024-11-14T07:20:44.903Z Learning: In `src/Altinn.Broker.Persistence/Repositories/ServiceOwnerRepository.cs`, the `service_owner_name` column in the `service_owner` table is not nullable, so null checks before calling `reader.GetString` are unnecessary.Learnt from: Ceredron Repo: Altinn/altinn-broker PR: 582 File: src/Altinn.Broker.Persistence/Repositories/FileTransferRepository.cs:176-177 Timestamp: 2024-11-14T07:12:14.214Z Learning: In `src/Altinn.Broker.Persistence/Repositories/FileTransferRepository.cs`, avoid suggesting to extract inline SQL queries into constants.Learnt from: Ceredron Repo: Altinn/altinn-broker PR: 560 File: tests/Altinn.Broker.Tests/Factories/FileTransferEntityFactory.cs:8-45 Timestamp: 2024-10-29T12:12:50.346Z Learning: When suggesting code improvements in `tests/Altinn.Broker.Tests/Factories/FileTransferEntityFactory.cs`, avoid adding excessive comments and unnecessary code changes.src/Altinn.Broker.Common/StringExtensions.cs (2)
1-2: LGTM!The
System.Globalizationimport is necessary for the culture-invariant parsing used in the SSN validation logic.
28-31: LGTM!The method properly delegates to the helper after stripping any prefix, maintaining consistency with the existing
IsOrganizationNumberpattern.
src/Altinn.Broker.Application/GenerateReport/GenerateDailySummaryReportHandler.cs
Show resolved
Hide resolved
src/Altinn.Broker.Application/GenerateReport/GenerateDailySummaryReportRequest.cs
Show resolved
Hide resolved
src/Altinn.Broker.Persistence/Repositories/FileTransferRepository.cs
Outdated
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
src/Altinn.Broker.API/Filters/StatisticsApiKeyFilter.cs (1)
78-104: Consider implementing IAsyncAuthorizationFilter for proper async support.Line 80 uses
GetAwaiter().GetResult()to call the async rate limit check synchronously, which can lead to deadlocks in certain scenarios. Consider implementingIAsyncAuthorizationFilterinstead ofIAuthorizationFilterto properly support async operations withOnAuthorizationAsync.Apply this diff:
-public class StatisticsApiKeyFilter : IAuthorizationFilter +public class StatisticsApiKeyFilter : IAsyncAuthorizationFilter { // ... existing fields ... - public void OnAuthorization(AuthorizationFilterContext context) + public async Task OnAuthorizationAsync(AuthorizationFilterContext context) { // ... existing checks ... var clientIp = GetClientIpAddress(context.HttpContext) ?? "unknown"; - var rateLimitResult = CheckRateLimitAsync(clientIp).GetAwaiter().GetResult(); + var rateLimitResult = await CheckRateLimitAsync(clientIp); // ... rest of the method ... } }src/Altinn.Broker.Core/Repositories/IFileTransferRepository.cs (1)
33-33: Consider using IReadOnlyList for consistency.Line 33 returns
Task<List<FileTransferEntity>>, while line 20 returnsTask<IReadOnlyList<FileTransferEntity>>. Consider standardizing onIReadOnlyList<T>for repository read operations to signal immutability and align with line 20's pattern.- Task<List<FileTransferEntity>> GetFileTransfersForReport(CancellationToken cancellationToken); + Task<IReadOnlyList<FileTransferEntity>> GetFileTransfersForReport(CancellationToken cancellationToken);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
src/Altinn.Broker.API/Controllers/StatisticsController.cs(1 hunks)src/Altinn.Broker.API/Filters/StatisticsApiKeyFilter.cs(1 hunks)src/Altinn.Broker.API/Program.cs(2 hunks)src/Altinn.Broker.Application/Altinn.Broker.Application.csproj(1 hunks)src/Altinn.Broker.Application/DependencyInjection.cs(2 hunks)src/Altinn.Broker.Application/Errors.cs(1 hunks)src/Altinn.Broker.Application/GenerateReport/DailySummaryData.cs(1 hunks)src/Altinn.Broker.Application/GenerateReport/GenerateAndDownloadDailySummaryReportResponse.cs(1 hunks)src/Altinn.Broker.Application/GenerateReport/GenerateDailySummaryReportHandler.cs(1 hunks)src/Altinn.Broker.Application/GenerateReport/GenerateDailySummaryReportRequest.cs(1 hunks)src/Altinn.Broker.Application/GenerateReport/GenerateDailySummaryReportResponse.cs(1 hunks)src/Altinn.Broker.Application/GenerateReport/ParquetDailySummaryData.cs(1 hunks)src/Altinn.Broker.Common/StringExtensions.cs(2 hunks)src/Altinn.Broker.Core/Domain/Enums/AltinnVersion.cs(1 hunks)src/Altinn.Broker.Core/Domain/Enums/RecipientType.cs(1 hunks)src/Altinn.Broker.Core/Repositories/IAltinnResourceRepoistory.cs(1 hunks)src/Altinn.Broker.Core/Repositories/IFileTransferRepository.cs(1 hunks)src/Altinn.Broker.Integrations/Altinn/ResourceRegistry/AltinnResourceRegistryRepository.cs(1 hunks)src/Altinn.Broker.Persistence/Repositories/FileTransferRepository.cs(1 hunks)
🧰 Additional context used
🧠 Learnings (16)
📚 Learning: 2024-10-29T12:12:50.346Z
Learnt from: Ceredron
Repo: Altinn/altinn-broker PR: 560
File: tests/Altinn.Broker.Tests/Factories/FileTransferEntityFactory.cs:8-45
Timestamp: 2024-10-29T12:12:50.346Z
Learning: When suggesting code improvements in `tests/Altinn.Broker.Tests/Factories/FileTransferEntityFactory.cs`, avoid adding excessive comments and unnecessary code changes.
Applied to files:
src/Altinn.Broker.Application/GenerateReport/GenerateDailySummaryReportRequest.cssrc/Altinn.Broker.Application/Errors.cssrc/Altinn.Broker.Application/GenerateReport/DailySummaryData.cssrc/Altinn.Broker.Application/GenerateReport/GenerateDailySummaryReportResponse.cssrc/Altinn.Broker.API/Program.cssrc/Altinn.Broker.Application/Altinn.Broker.Application.csprojsrc/Altinn.Broker.Application/GenerateReport/ParquetDailySummaryData.cssrc/Altinn.Broker.Core/Repositories/IFileTransferRepository.cssrc/Altinn.Broker.Application/GenerateReport/GenerateDailySummaryReportHandler.cssrc/Altinn.Broker.Application/GenerateReport/GenerateAndDownloadDailySummaryReportResponse.cssrc/Altinn.Broker.Persistence/Repositories/FileTransferRepository.cssrc/Altinn.Broker.Application/DependencyInjection.cs
📚 Learning: 2024-11-14T07:20:44.903Z
Learnt from: Ceredron
Repo: Altinn/altinn-broker PR: 582
File: src/Altinn.Broker.Persistence/Repositories/ServiceOwnerRepository.cs:30-31
Timestamp: 2024-11-14T07:20:44.903Z
Learning: In `src/Altinn.Broker.Persistence/Repositories/ServiceOwnerRepository.cs`, the `service_owner_name` column in the `service_owner` table is not nullable, so null checks before calling `reader.GetString` are unnecessary.
Applied to files:
src/Altinn.Broker.Core/Repositories/IAltinnResourceRepoistory.cssrc/Altinn.Broker.Integrations/Altinn/ResourceRegistry/AltinnResourceRegistryRepository.cs
📚 Learning: 2024-11-14T07:20:20.158Z
Learnt from: Ceredron
Repo: Altinn/altinn-broker PR: 582
File: src/Altinn.Broker.Persistence/Repositories/ServiceOwnerRepository.cs:36-47
Timestamp: 2024-11-14T07:20:20.158Z
Learning: In `src/Altinn.Broker.Persistence/Repositories/ServiceOwnerRepository.cs`, the `created` field retrieved from the database is non-nullable, so null checks for the `created` field are not necessary.
Applied to files:
src/Altinn.Broker.Core/Repositories/IAltinnResourceRepoistory.cssrc/Altinn.Broker.Integrations/Altinn/ResourceRegistry/AltinnResourceRegistryRepository.cs
📚 Learning: 2024-10-29T12:51:11.540Z
Learnt from: Ceredron
Repo: Altinn/altinn-broker PR: 560
File: tests/Altinn.Broker.Tests/ManifestDownloadStreamTests.cs:24-39
Timestamp: 2024-10-29T12:51:11.540Z
Learning: In `tests/Altinn.Broker.Tests/ManifestDownloadStreamTests.cs`, refactoring suggestions aimed at enhancing test organization and coverage may be considered less readable and of dubious value.
Applied to files:
src/Altinn.Broker.API/Program.cssrc/Altinn.Broker.Application/Altinn.Broker.Application.csprojsrc/Altinn.Broker.Application/GenerateReport/ParquetDailySummaryData.cssrc/Altinn.Broker.Integrations/Altinn/ResourceRegistry/AltinnResourceRegistryRepository.cssrc/Altinn.Broker.Application/GenerateReport/GenerateDailySummaryReportHandler.cssrc/Altinn.Broker.Application/GenerateReport/GenerateAndDownloadDailySummaryReportResponse.cssrc/Altinn.Broker.Application/DependencyInjection.cs
📚 Learning: 2024-10-29T12:50:05.699Z
Learnt from: Ceredron
Repo: Altinn/altinn-broker PR: 560
File: tests/Altinn.Broker.Tests/Helpers/ManifestDownloadStreamHelpers.cs:18-28
Timestamp: 2024-10-29T12:50:05.699Z
Learning: In `tests/Altinn.Broker.Tests/Helpers/ManifestDownloadStreamHelpers.cs`, the test code is static and will not change, so refactoring suggestions on this file are unnecessary.
Applied to files:
src/Altinn.Broker.API/Program.cssrc/Altinn.Broker.Application/Altinn.Broker.Application.csprojsrc/Altinn.Broker.Application/GenerateReport/ParquetDailySummaryData.cssrc/Altinn.Broker.Application/GenerateReport/GenerateDailySummaryReportHandler.cssrc/Altinn.Broker.Application/GenerateReport/GenerateAndDownloadDailySummaryReportResponse.cssrc/Altinn.Broker.Application/DependencyInjection.cs
📚 Learning: 2024-11-14T07:00:48.796Z
Learnt from: Ceredron
Repo: Altinn/altinn-broker PR: 582
File: tests/Altinn.Broker.Tests.LargeFile/Program.cs:80-82
Timestamp: 2024-11-14T07:00:48.796Z
Learning: In test programs like `tests/Altinn.Broker.Tests.LargeFile/Program.cs` in the `Altinn.Broker` project, it's acceptable to proceed without checking the success of HTTP responses or adding additional error handling.
Applied to files:
src/Altinn.Broker.API/Program.cssrc/Altinn.Broker.Application/DependencyInjection.cs
📚 Learning: 2024-11-14T07:00:59.444Z
Learnt from: Ceredron
Repo: Altinn/altinn-broker PR: 582
File: src/Altinn.Broker.Core/Domain/ServiceOwnerEntity.cs:11-21
Timestamp: 2024-11-14T07:00:59.444Z
Learning: In the Altinn.Broker project, exceptions should not be used for control flow or logic; prefer returning null or appropriate results instead.
Applied to files:
src/Altinn.Broker.API/Program.cssrc/Altinn.Broker.Application/DependencyInjection.cs
📚 Learning: 2024-11-27T14:04:55.778Z
Learnt from: RagnarFatland-Avanade
Repo: Altinn/altinn-broker PR: 593
File: src/Altinn.Broker.Application/ExpireFileTransfer/ExpireFileTransferHandler.cs:96-96
Timestamp: 2024-11-27T14:04:55.778Z
Learning: In the Altinn Broker codebase, when throwing exceptions in C# code, prefer to use generic exceptions rather than creating custom exception classes, aiming to keep the code simple.
Applied to files:
src/Altinn.Broker.API/Program.cssrc/Altinn.Broker.Application/DependencyInjection.cs
📚 Learning: 2024-12-08T22:55:30.559Z
Learnt from: Ceredron
Repo: Altinn/altinn-broker PR: 610
File: src/Altinn.Broker.Common/StringExtensions.cs:23-30
Timestamp: 2024-12-08T22:55:30.559Z
Learning: In `src/Altinn.Broker.Common/StringExtensions.cs`, the `WithoutPrefix` method in the C# project checks if `orgNumber` is null or whitespace and returns an empty string if it is. Therefore, calling `orgNumber.Split(':').Last()` is safe, as the method ensures `orgNumber` is non-empty before splitting.
Applied to files:
src/Altinn.Broker.Common/StringExtensions.cs
📚 Learning: 2024-12-08T23:15:58.682Z
Learnt from: Ceredron
Repo: Altinn/altinn-broker PR: 610
File: src/Altinn.Broker.Core/StringExtensions.cs:0-0
Timestamp: 2024-12-08T23:15:58.682Z
Learning: In `src/Altinn.Broker.Core/StringExtensions.cs`, prefer keeping the `WithoutPrefix` method simple and readable, avoiding complex modifications that reduce readability.
Applied to files:
src/Altinn.Broker.Common/StringExtensions.cs
📚 Learning: 2024-11-14T07:01:04.663Z
Learnt from: Ceredron
Repo: Altinn/altinn-broker PR: 582
File: tests/Altinn.Broker.Tests.LargeFile/Program.cs:147-147
Timestamp: 2024-11-14T07:01:04.663Z
Learning: In the project `Altinn.Broker.Tests.LargeFile`, the codebase uses C# 12 features like collection expressions, such as initializing collections with `PropertyList = []`. Do not flag this syntax as invalid in future reviews.
Applied to files:
src/Altinn.Broker.Application/Altinn.Broker.Application.csprojsrc/Altinn.Broker.Application/GenerateReport/ParquetDailySummaryData.cs
📚 Learning: 2024-11-14T07:12:14.214Z
Learnt from: Ceredron
Repo: Altinn/altinn-broker PR: 582
File: src/Altinn.Broker.Persistence/Repositories/FileTransferRepository.cs:176-177
Timestamp: 2024-11-14T07:12:14.214Z
Learning: In `src/Altinn.Broker.Persistence/Repositories/FileTransferRepository.cs`, avoid suggesting to extract inline SQL queries into constants.
Applied to files:
src/Altinn.Broker.Core/Repositories/IFileTransferRepository.cssrc/Altinn.Broker.Persistence/Repositories/FileTransferRepository.cssrc/Altinn.Broker.Application/DependencyInjection.cs
📚 Learning: 2024-10-29T11:25:42.867Z
Learnt from: Andreass2
Repo: Altinn/altinn-broker PR: 560
File: src/Altinn.Broker.Application/DownloadFile/DownloadFileHandler.cs:49-55
Timestamp: 2024-10-29T11:25:42.867Z
Learning: In `DownloadFileHandler` in `src/Altinn.Broker.Application/DownloadFile/DownloadFileHandler.cs`, due to the 1 GB file size limit, loading the entire file into memory is acceptable in this context.
Applied to files:
src/Altinn.Broker.Application/GenerateReport/GenerateDailySummaryReportHandler.cssrc/Altinn.Broker.Application/GenerateReport/GenerateAndDownloadDailySummaryReportResponse.cssrc/Altinn.Broker.Application/DependencyInjection.cs
📚 Learning: 2024-10-29T12:45:23.330Z
Learnt from: Ceredron
Repo: Altinn/altinn-broker PR: 560
File: src/Altinn.Broker.Core/Helpers/ManifestDownloadStream.cs:199-237
Timestamp: 2024-10-29T12:45:23.330Z
Learning: In the `ManifestDownloadStream` class (`src/Altinn.Broker.Core/Helpers/ManifestDownloadStream.cs`), optimizing memory usage in `AddManifestFile` by modifying the ZIP archive without loading the entire content into memory is currently not feasible because the base stream is a `ReadOnlyMemoryStream` based on a download stream. This optimization will be evaluated when the download stream is re-written in the "Support bigger files"-PR.
Applied to files:
src/Altinn.Broker.Application/GenerateReport/GenerateAndDownloadDailySummaryReportResponse.cs
📚 Learning: 2025-09-30T11:29:20.339Z
Learnt from: tomshag
Repo: Altinn/altinn-broker PR: 790
File: src/Altinn.Broker.Persistence/Repositories/FileTransferRepository.cs:349-352
Timestamp: 2025-09-30T11:29:20.339Z
Learning: In the Altinn Broker file transfer system, file transfer status and actor file transfer status use status_description_id_fk values that are ordered by progression level. Ordering by status_description_id_fk DESC is intentional to get the highest/most advanced status, not the most recent by date. This applies to queries in FileTransferRepository.cs.
Applied to files:
src/Altinn.Broker.Persistence/Repositories/FileTransferRepository.cssrc/Altinn.Broker.Application/DependencyInjection.cs
📚 Learning: 2024-10-29T10:43:16.160Z
Learnt from: Andreass2
Repo: Altinn/altinn-broker PR: 561
File: src/Altinn.Broker.API/Controllers/LegacyFileController.cs:80-81
Timestamp: 2024-10-29T10:43:16.160Z
Learning: In the Altinn Broker API, specifically in `src/Altinn.Broker.API/Controllers/LegacyFileController.cs`, when handling file uploads in the `UploadFileStreamed` method, we can assume that all uploaded files are small and that the `Content-Length` header is always provided. Therefore, additional error handling for missing `Content-Length` is not required.
Applied to files:
src/Altinn.Broker.Application/DependencyInjection.cs
🧬 Code graph analysis (10)
src/Altinn.Broker.Core/Domain/Enums/RecipientType.cs (1)
src/Altinn.Broker.Application/GenerateReport/GenerateDailySummaryReportHandler.cs (1)
RecipientType(141-164)
src/Altinn.Broker.API/Controllers/StatisticsController.cs (5)
src/Altinn.Broker.API/Filters/StatisticsApiKeyFilter.cs (3)
StatisticsApiKeyFilter(12-234)StatisticsApiKeyFilter(21-31)Task(154-233)src/Altinn.Broker.Application/GenerateReport/GenerateDailySummaryReportResponse.cs (1)
GenerateDailySummaryReportResponse(3-45)src/Altinn.Broker.Application/GenerateReport/GenerateDailySummaryReportHandler.cs (6)
Task(23-68)Task(219-236)Task(238-268)Task(270-307)Task(309-361)GenerateDailySummaryReportHandler(15-362)src/Altinn.Broker.Core/Repositories/IFileTransferRepository.cs (10)
Task(7-18)Task(19-19)Task(20-20)Task(21-21)Task(22-22)Task(23-23)Task(24-24)Task(25-31)Task(32-32)Task(33-33)src/Altinn.Broker.Application/GenerateReport/GenerateDailySummaryReportRequest.cs (1)
GenerateDailySummaryReportRequest(6-15)
src/Altinn.Broker.Application/GenerateReport/DailySummaryData.cs (1)
src/Altinn.Broker.Application/GenerateReport/GenerateDailySummaryReportHandler.cs (1)
RecipientType(141-164)
src/Altinn.Broker.API/Program.cs (1)
src/Altinn.Broker.API/Filters/StatisticsApiKeyFilter.cs (2)
StatisticsApiKeyFilter(12-234)StatisticsApiKeyFilter(21-31)
src/Altinn.Broker.Application/GenerateReport/ParquetDailySummaryData.cs (1)
src/Altinn.Broker.Application/GenerateReport/GenerateDailySummaryReportHandler.cs (1)
RecipientType(141-164)
src/Altinn.Broker.Core/Repositories/IFileTransferRepository.cs (2)
src/Altinn.Broker.Application/GenerateReport/GenerateDailySummaryReportHandler.cs (1)
List(70-125)src/Altinn.Broker.Core/Domain/FileTransferEntity.cs (1)
FileTransferEntity(3-21)
src/Altinn.Broker.Integrations/Altinn/ResourceRegistry/AltinnResourceRegistryRepository.cs (2)
src/Altinn.Broker.Core/Repositories/IAltinnResourceRepoistory.cs (2)
Task(6-6)Task(12-12)src/Altinn.Broker.Integrations/Altinn/ResourceRegistry/GetResourceResponse.cs (2)
GetResourceResponse(5-57)HasCompetentAuthority(59-69)
src/Altinn.Broker.Application/GenerateReport/GenerateDailySummaryReportHandler.cs (10)
src/Altinn.Broker.Core/Repositories/IAltinnResourceRepoistory.cs (2)
Task(6-6)Task(12-12)src/Altinn.Broker.Integrations/Altinn/ResourceRegistry/AltinnResourceRegistryRepository.cs (2)
Task(28-62)Task(64-85)src/Altinn.Broker.Core/Repositories/IFileTransferRepository.cs (10)
Task(7-18)Task(19-19)Task(20-20)Task(21-21)Task(22-22)Task(23-23)Task(24-24)Task(25-31)Task(32-32)Task(33-33)src/Altinn.Broker.Persistence/Repositories/FileTransferRepository.cs (1)
Task(112-172)src/Altinn.Broker.Application/GenerateReport/GenerateDailySummaryReportResponse.cs (1)
GenerateDailySummaryReportResponse(3-45)src/Altinn.Broker.Application/GenerateReport/GenerateDailySummaryReportRequest.cs (1)
GenerateDailySummaryReportRequest(6-15)src/Altinn.Broker.Application/Errors.cs (1)
StatisticsErrors(34-38)src/Altinn.Broker.Application/GenerateReport/DailySummaryData.cs (1)
DailySummaryData(9-77)src/Altinn.Broker.Common/StringExtensions.cs (3)
WithoutPrefix(77-84)IsOrganizationNumber(18-21)IsSocialSecurityNumber(28-31)src/Altinn.Broker.Application/GenerateReport/ParquetDailySummaryData.cs (1)
ParquetDailySummaryData(9-89)
src/Altinn.Broker.Persistence/Repositories/FileTransferRepository.cs (5)
src/Altinn.Broker.Application/GenerateReport/GenerateDailySummaryReportHandler.cs (1)
List(70-125)src/Altinn.Broker.Core/Domain/FileTransferEntity.cs (1)
FileTransferEntity(3-21)src/Altinn.Broker.Core/Domain/ActorEntity.cs (1)
ActorEntity(3-7)src/Altinn.Broker.Core/Domain/FileTransferStatusEntity.cs (1)
FileTransferStatusEntity(2-8)src/Altinn.Broker.Core/Domain/ActorFileTransferStatusEntity.cs (1)
ActorFileTransferStatusEntity(3-9)
src/Altinn.Broker.Application/DependencyInjection.cs (1)
src/Altinn.Broker.Application/GenerateReport/GenerateDailySummaryReportHandler.cs (1)
GenerateDailySummaryReportHandler(15-362)
🔇 Additional comments (11)
src/Altinn.Broker.Core/Domain/Enums/AltinnVersion.cs (1)
1-18: LGTM!The enum is well-defined with clear documentation. The naming and values appropriately represent legacy (Altinn2) and current (Altinn3) system versions.
src/Altinn.Broker.API/Filters/StatisticsApiKeyFilter.cs (5)
35-76: LGTM!The API key validation logic is secure and well-structured. The constant-time comparison prevents timing attacks, and appropriate logging and error responses are in place.
116-130: LGTM!IP address extraction correctly handles proxy and load balancer scenarios by checking forwarded headers before falling back to the direct connection IP.
135-147: LGTM!The constant-time comparison implementation correctly prevents timing attacks by accumulating differences via XOR operations. The early length check is acceptable for API key validation.
220-232: Verify fail-open behavior on cache errors is intentional.Lines 224-231 implement a fail-open strategy when rate limit checking encounters an error, allowing the request to proceed. This is a reasonable default to prevent availability issues, but confirm this aligns with your security requirements. Consider logging at a higher severity level (Warning or Error) for production monitoring.
236-253: LGTM!The data classes are well-structured with appropriate visibility modifiers. The use of collection expressions for initialization is clean and modern.
src/Altinn.Broker.Application/DependencyInjection.cs (1)
8-8: LGTM!The dependency injection registration for
GenerateDailySummaryReportHandleris consistent with other handler registrations in the file.Also applies to: 35-35
src/Altinn.Broker.API/Program.cs (1)
118-118: LGTM!The filter registration is correct and follows the established pattern in the codebase.
src/Altinn.Broker.Core/Domain/Enums/RecipientType.cs (1)
1-23: LGTM!The enum is well-defined with appropriate documentation. The inclusion of an
Unknownmember is a good practice for handling indeterminate recipient types.src/Altinn.Broker.Core/Repositories/IAltinnResourceRepoistory.cs (1)
7-12: LGTM!The new method is well-documented and appropriately returns a nullable string to handle cases where the resource or service owner might not be found. The XML documentation clearly explains what field is being retrieved.
src/Altinn.Broker.Application/Altinn.Broker.Application.csproj (1)
15-15: No issues found — Parquet.Net 5.3.0 is the latest stable release and contains no known security vulnerabilities.5.3.0 includes BYTE_STREAM_SPLIT encoding support, CancellationToken additions, validation improvements, and critical bug fixes. No deprecations or known issues were reported for this release.
src/Altinn.Broker.Application/GenerateReport/GenerateDailySummaryReportHandler.cs
Outdated
Show resolved
Hide resolved
src/Altinn.Broker.Application/GenerateReport/GenerateDailySummaryReportHandler.cs
Show resolved
Hide resolved
src/Altinn.Broker.Persistence/Repositories/FileTransferRepository.cs
Outdated
Show resolved
Hide resolved
added 2 commits
November 11, 2025 06:56
Contributor
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/Altinn.Broker.Application/GenerateReport/GenerateDailySummaryReportHandler.cs (2)
208-213: Offer to implement database storage calculation.The TODO comment indicates this calculation is not yet implemented. If you'd like, I can help generate the logic to estimate database storage based on file transfer metadata (e.g., property counts, recipient counts, status records, etc.).
Do you want me to propose an implementation for this method or open an issue to track this task?
301-304: Avoid extra memory allocation in hash computation.Calling
memoryStream.ToArray()creates an unnecessary copy of the entire stream content. For larger reports, this doubles peak memory usage. Consider computing the hash from the stream buffer directly.Apply this diff to use the internal buffer:
// Calculate MD5 hash using var md5 = MD5.Create(); -var hash = Convert.ToBase64String(md5.ComputeHash(memoryStream.ToArray())); +var hash = Convert.ToBase64String(md5.ComputeHash(memoryStream.GetBuffer(), 0, (int)memoryStream.Length)); memoryStream.Position = 0; // Reset position for reading
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/Altinn.Broker.Application/GenerateReport/GenerateDailySummaryReportHandler.cs(1 hunks)
🧰 Additional context used
🧠 Learnings (8)
📚 Learning: 2024-10-29T12:51:11.540Z
Learnt from: Ceredron
Repo: Altinn/altinn-broker PR: 560
File: tests/Altinn.Broker.Tests/ManifestDownloadStreamTests.cs:24-39
Timestamp: 2024-10-29T12:51:11.540Z
Learning: In `tests/Altinn.Broker.Tests/ManifestDownloadStreamTests.cs`, refactoring suggestions aimed at enhancing test organization and coverage may be considered less readable and of dubious value.
Applied to files:
src/Altinn.Broker.Application/GenerateReport/GenerateDailySummaryReportHandler.cs
📚 Learning: 2024-10-29T12:12:50.346Z
Learnt from: Ceredron
Repo: Altinn/altinn-broker PR: 560
File: tests/Altinn.Broker.Tests/Factories/FileTransferEntityFactory.cs:8-45
Timestamp: 2024-10-29T12:12:50.346Z
Learning: When suggesting code improvements in `tests/Altinn.Broker.Tests/Factories/FileTransferEntityFactory.cs`, avoid adding excessive comments and unnecessary code changes.
Applied to files:
src/Altinn.Broker.Application/GenerateReport/GenerateDailySummaryReportHandler.cs
📚 Learning: 2024-10-29T12:50:05.699Z
Learnt from: Ceredron
Repo: Altinn/altinn-broker PR: 560
File: tests/Altinn.Broker.Tests/Helpers/ManifestDownloadStreamHelpers.cs:18-28
Timestamp: 2024-10-29T12:50:05.699Z
Learning: In `tests/Altinn.Broker.Tests/Helpers/ManifestDownloadStreamHelpers.cs`, the test code is static and will not change, so refactoring suggestions on this file are unnecessary.
Applied to files:
src/Altinn.Broker.Application/GenerateReport/GenerateDailySummaryReportHandler.cs
📚 Learning: 2024-11-14T07:12:14.214Z
Learnt from: Ceredron
Repo: Altinn/altinn-broker PR: 582
File: src/Altinn.Broker.Persistence/Repositories/FileTransferRepository.cs:176-177
Timestamp: 2024-11-14T07:12:14.214Z
Learning: In `src/Altinn.Broker.Persistence/Repositories/FileTransferRepository.cs`, avoid suggesting to extract inline SQL queries into constants.
Applied to files:
src/Altinn.Broker.Application/GenerateReport/GenerateDailySummaryReportHandler.cs
📚 Learning: 2024-11-14T07:20:20.158Z
Learnt from: Ceredron
Repo: Altinn/altinn-broker PR: 582
File: src/Altinn.Broker.Persistence/Repositories/ServiceOwnerRepository.cs:36-47
Timestamp: 2024-11-14T07:20:20.158Z
Learning: In `src/Altinn.Broker.Persistence/Repositories/ServiceOwnerRepository.cs`, the `created` field retrieved from the database is non-nullable, so null checks for the `created` field are not necessary.
Applied to files:
src/Altinn.Broker.Application/GenerateReport/GenerateDailySummaryReportHandler.cs
📚 Learning: 2024-11-14T07:20:44.903Z
Learnt from: Ceredron
Repo: Altinn/altinn-broker PR: 582
File: src/Altinn.Broker.Persistence/Repositories/ServiceOwnerRepository.cs:30-31
Timestamp: 2024-11-14T07:20:44.903Z
Learning: In `src/Altinn.Broker.Persistence/Repositories/ServiceOwnerRepository.cs`, the `service_owner_name` column in the `service_owner` table is not nullable, so null checks before calling `reader.GetString` are unnecessary.
Applied to files:
src/Altinn.Broker.Application/GenerateReport/GenerateDailySummaryReportHandler.cs
📚 Learning: 2024-11-14T07:00:50.257Z
Learnt from: Ceredron
Repo: Altinn/altinn-broker PR: 582
File: src/Altinn.Broker.Integrations/Azure/AzureResourceManagerService.cs:55-67
Timestamp: 2024-11-14T07:00:50.257Z
Learning: In the Altinn.Broker project, exceptions during background job enqueuing are already handled and logged elsewhere, so adding try-catch blocks in methods like `CreateStorageProviders` is unnecessary.
Applied to files:
src/Altinn.Broker.Application/GenerateReport/GenerateDailySummaryReportHandler.cs
📚 Learning: 2024-10-29T11:25:42.867Z
Learnt from: Andreass2
Repo: Altinn/altinn-broker PR: 560
File: src/Altinn.Broker.Application/DownloadFile/DownloadFileHandler.cs:49-55
Timestamp: 2024-10-29T11:25:42.867Z
Learning: In `DownloadFileHandler` in `src/Altinn.Broker.Application/DownloadFile/DownloadFileHandler.cs`, due to the 1 GB file size limit, loading the entire file into memory is acceptable in this context.
Applied to files:
src/Altinn.Broker.Application/GenerateReport/GenerateDailySummaryReportHandler.cs
🧬 Code graph analysis (1)
src/Altinn.Broker.Application/GenerateReport/GenerateDailySummaryReportHandler.cs (11)
src/Altinn.Broker.Core/Repositories/IAltinnResourceRepoistory.cs (2)
Task(6-6)Task(12-12)src/Altinn.Broker.Integrations/Altinn/ResourceRegistry/AltinnResourceRegistryRepository.cs (2)
Task(28-62)Task(64-85)src/Altinn.Broker.Core/Repositories/IFileTransferRepository.cs (10)
Task(7-18)Task(19-19)Task(20-20)Task(21-21)Task(22-22)Task(23-23)Task(24-24)Task(25-31)Task(32-32)Task(33-33)src/Altinn.Broker.Persistence/Repositories/FileTransferRepository.cs (1)
Task(112-172)src/Altinn.Broker.Application/GenerateReport/GenerateDailySummaryReportResponse.cs (1)
GenerateDailySummaryReportResponse(3-45)src/Altinn.Broker.Application/GenerateReport/GenerateDailySummaryReportRequest.cs (1)
GenerateDailySummaryReportRequest(6-15)src/Altinn.Broker.Application/Errors.cs (1)
StatisticsErrors(34-38)src/Altinn.Broker.Application/GenerateReport/DailySummaryData.cs (1)
DailySummaryData(9-77)src/Altinn.Broker.Common/StringExtensions.cs (3)
WithoutPrefix(77-84)IsOrganizationNumber(18-21)IsSocialSecurityNumber(28-31)src/Altinn.Broker.Application/GenerateReport/ParquetDailySummaryData.cs (1)
ParquetDailySummaryData(9-89)src/Altinn.Broker.Application/GenerateReport/GenerateAndDownloadDailySummaryReportResponse.cs (1)
GenerateAndDownloadDailySummaryReportResponse(3-50)
⏰ 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). (1)
- GitHub Check: Analyze (csharp)
🔇 Additional comments (1)
src/Altinn.Broker.Application/GenerateReport/GenerateDailySummaryReportHandler.cs (1)
346-347: Correct count calculation here.Unlike line 53 in the
Processmethod, this correctly usesfileTransfers.CountforTotalFileTransferCount, which represents the actual number of unique file transfers rather than the expanded recipient count.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/Altinn.Broker.Application/GenerateReport/ParquetDailySummaryData.cs (2)
11-33: Consider validating consistency between Date string and Year/Month/Day fields.The date information is stored redundantly (Date string in YYYY-MM-DD format plus separate Year, Month, Day integers). While this may optimize different query patterns, it creates potential for inconsistency if the fields are set independently.
If the redundancy is intentional for analytics queries, consider adding validation in the handler that populates these fields to ensure they remain synchronized.
9-93: Consider init-only setters for immutability.Since this is a data model populated by the handler and then serialized to Parquet, consider using init-only setters to prevent accidental modification after initialization:
- public string Date { get; set; } = string.Empty; + public string Date { get; init; } = string.Empty;Apply the same pattern to all properties if immutability is desired.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/Altinn.Broker.Application/GenerateReport/ParquetDailySummaryData.cs(1 hunks)src/Altinn.Broker.Persistence/Repositories/FileTransferRepository.cs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/Altinn.Broker.Persistence/Repositories/FileTransferRepository.cs
🧰 Additional context used
🧬 Code graph analysis (1)
src/Altinn.Broker.Application/GenerateReport/ParquetDailySummaryData.cs (1)
src/Altinn.Broker.Application/GenerateReport/GenerateDailySummaryReportHandler.cs (1)
RecipientType(143-166)
⏰ 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). (1)
- GitHub Check: Deploy
There was a problem hiding this comment.
Pull Request Overview
This PR introduces a statistics and reporting feature for Altinn Broker that generates daily summary reports in Parquet format. The reports aggregate file transfer data by service owner, resource, and recipient type, with file download capabilities and storage in Azure blob storage.
Key Changes:
- New statistics endpoints with API key authentication and IP-based rate limiting
- Parquet report generation with aggregated daily metrics (message counts, storage bytes)
- Social security number validation utility for recipient type classification
- Azure storage integration for report persistence
Reviewed Changes
Copilot reviewed 22 out of 22 changed files in this pull request and generated 11 comments.
Show a summary per file
| File | Description |
|---|---|
FileTransferRepository.cs |
Adds optimized query to fetch all file transfers with sender and recipient data for reporting |
AltinnResourceRegistryRepository.cs |
Adds method to retrieve service owner name from Resource Registry |
StringExtensions.cs |
Adds social security number validation with mod11 checksum |
ParquetDailySummaryData.cs |
Defines Parquet-optimized data model for daily summary reports |
GenerateDailySummaryReportHandler.cs |
Core report generation logic with data aggregation and Parquet file creation |
StatisticsApiKeyFilter.cs |
API key validation and IP-based rate limiting for statistics endpoints |
StatisticsController.cs |
REST endpoints for report generation and download |
| Infrastructure files | Adds secrets and configuration for report storage |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
src/Altinn.Broker.Application/GenerateReport/GenerateDailySummaryReportHandler.cs
Outdated
Show resolved
Hide resolved
src/Altinn.Broker.Application/GenerateReport/GenerateDailySummaryReportHandler.cs
Outdated
Show resolved
Hide resolved
src/Altinn.Broker.Application/GenerateReport/GenerateDailySummaryReportHandler.cs
Outdated
Show resolved
Hide resolved
src/Altinn.Broker.Application/GenerateReport/GenerateDailySummaryReportHandler.cs
Outdated
Show resolved
Hide resolved
src/Altinn.Broker.Application/GenerateReport/GenerateDailySummaryReportHandler.cs
Outdated
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (3)
src/Altinn.Broker.API/Filters/StatisticsApiKeyFilter.cs (2)
251-254: Consider moving internal class for better organization.The
RateLimitDataclass is defined at the bottom of the file. For improved code organization, consider moving it closer to its usage (nearCheckRateLimitAsync) or to a separate file if the filter grows more complex.Based on past review comments.
97-109: Extract duplicate rate limit header logic.The rate limit headers are set identically at lines 97-101 and 107-109. Extract this into a helper method to improve maintainability.
Apply this refactor:
+ private void SetRateLimitHeaders(HttpContext httpContext, RateLimitResult result) + { + httpContext.Response.Headers["X-RateLimit-Limit"] = RateLimitAttempts.ToString(); + httpContext.Response.Headers["X-RateLimit-Remaining"] = result.RemainingAttempts.ToString(); + httpContext.Response.Headers["X-RateLimit-Reset"] = result.ResetTime.ToUnixTimeSeconds().ToString(); + } + ... - // Add rate limit headers - context.HttpContext.Response.Headers["X-RateLimit-Limit"] = RateLimitAttempts.ToString(); - context.HttpContext.Response.Headers["X-RateLimit-Remaining"] = rateLimitResult.RemainingAttempts.ToString(); - context.HttpContext.Response.Headers["X-RateLimit-Reset"] = rateLimitResult.ResetTime.ToUnixTimeSeconds().ToString(); + SetRateLimitHeaders(context.HttpContext, rateLimitResult); context.HttpContext.Response.Headers["Retry-After"] = rateLimitResult.RetryAfterSeconds.ToString(); ... - // Add rate limit headers for successful requests - context.HttpContext.Response.Headers["X-RateLimit-Limit"] = RateLimitAttempts.ToString(); - context.HttpContext.Response.Headers["X-RateLimit-Remaining"] = rateLimitResult.RemainingAttempts.ToString(); - context.HttpContext.Response.Headers["X-RateLimit-Reset"] = rateLimitResult.ResetTime.ToUnixTimeSeconds().ToString(); + SetRateLimitHeaders(context.HttpContext, rateLimitResult);Based on past review comments.
src/Altinn.Broker.Application/GenerateReport/GenerateDailySummaryReportHandler.cs (1)
55-55: Fix double-counted TotalFileTransferCount.
summaryDataexpands each transfer per recipient, so summingMessageCountinflates the total and diverges from the unique transfer count we expose elsewhere. Use the originalfileTransfers.Countin both handlers to keep the metadata accurate.- TotalFileTransferCount = summaryData.Sum(d => d.MessageCount), + TotalFileTransferCount = fileTransfers.Count, @@ - TotalFileTransferCount = summaryData.Sum(d => d.MessageCount), + TotalFileTransferCount = fileTransfers.Count,Also applies to: 387-387
🧹 Nitpick comments (1)
src/Altinn.Broker.API/Filters/StatisticsApiKeyFilter.cs (1)
170-172: Consider token bucket or counter-based rate limiting for better performance.The current sliding window implementation stores individual request timestamps and filters the list on every request. While this works fine for the current limit (10 requests), it's less efficient than alternative approaches.
For better performance and simpler atomic operations:
Option 1: Token bucket algorithm
- Store a count and last refill timestamp
- Refill tokens based on elapsed time
- Atomic decrement of tokens
- Better cache efficiency and easier to make atomic
Option 2: Fixed window counter
- Store a counter per time window (e.g., per hour)
- Atomic increment
- Simpler but allows bursts at window boundaries (up to 2× limit)
Current sliding window pros:
- Smoother rate limiting (no boundary bursts)
- Accurate limit enforcement (ignoring the race condition)
For statistics endpoints with modest limits, the current approach is acceptable. Only refactor if you need better performance or atomic operations (see race condition comment).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/Altinn.Broker.API/Filters/StatisticsApiKeyFilter.cs(1 hunks)src/Altinn.Broker.Application/GenerateReport/GenerateDailySummaryReportHandler.cs(1 hunks)
🧰 Additional context used
🧠 Learnings (8)
📚 Learning: 2024-10-29T12:51:11.540Z
Learnt from: Ceredron
Repo: Altinn/altinn-broker PR: 560
File: tests/Altinn.Broker.Tests/ManifestDownloadStreamTests.cs:24-39
Timestamp: 2024-10-29T12:51:11.540Z
Learning: In `tests/Altinn.Broker.Tests/ManifestDownloadStreamTests.cs`, refactoring suggestions aimed at enhancing test organization and coverage may be considered less readable and of dubious value.
Applied to files:
src/Altinn.Broker.Application/GenerateReport/GenerateDailySummaryReportHandler.cs
📚 Learning: 2024-10-29T12:12:50.346Z
Learnt from: Ceredron
Repo: Altinn/altinn-broker PR: 560
File: tests/Altinn.Broker.Tests/Factories/FileTransferEntityFactory.cs:8-45
Timestamp: 2024-10-29T12:12:50.346Z
Learning: When suggesting code improvements in `tests/Altinn.Broker.Tests/Factories/FileTransferEntityFactory.cs`, avoid adding excessive comments and unnecessary code changes.
Applied to files:
src/Altinn.Broker.Application/GenerateReport/GenerateDailySummaryReportHandler.cs
📚 Learning: 2024-10-29T12:50:05.699Z
Learnt from: Ceredron
Repo: Altinn/altinn-broker PR: 560
File: tests/Altinn.Broker.Tests/Helpers/ManifestDownloadStreamHelpers.cs:18-28
Timestamp: 2024-10-29T12:50:05.699Z
Learning: In `tests/Altinn.Broker.Tests/Helpers/ManifestDownloadStreamHelpers.cs`, the test code is static and will not change, so refactoring suggestions on this file are unnecessary.
Applied to files:
src/Altinn.Broker.Application/GenerateReport/GenerateDailySummaryReportHandler.cs
📚 Learning: 2024-10-29T11:25:42.867Z
Learnt from: Andreass2
Repo: Altinn/altinn-broker PR: 560
File: src/Altinn.Broker.Application/DownloadFile/DownloadFileHandler.cs:49-55
Timestamp: 2024-10-29T11:25:42.867Z
Learning: In `DownloadFileHandler` in `src/Altinn.Broker.Application/DownloadFile/DownloadFileHandler.cs`, due to the 1 GB file size limit, loading the entire file into memory is acceptable in this context.
Applied to files:
src/Altinn.Broker.Application/GenerateReport/GenerateDailySummaryReportHandler.cs
📚 Learning: 2024-11-14T07:12:14.214Z
Learnt from: Ceredron
Repo: Altinn/altinn-broker PR: 582
File: src/Altinn.Broker.Persistence/Repositories/FileTransferRepository.cs:176-177
Timestamp: 2024-11-14T07:12:14.214Z
Learning: In `src/Altinn.Broker.Persistence/Repositories/FileTransferRepository.cs`, avoid suggesting to extract inline SQL queries into constants.
Applied to files:
src/Altinn.Broker.Application/GenerateReport/GenerateDailySummaryReportHandler.cs
📚 Learning: 2024-11-14T07:20:20.158Z
Learnt from: Ceredron
Repo: Altinn/altinn-broker PR: 582
File: src/Altinn.Broker.Persistence/Repositories/ServiceOwnerRepository.cs:36-47
Timestamp: 2024-11-14T07:20:20.158Z
Learning: In `src/Altinn.Broker.Persistence/Repositories/ServiceOwnerRepository.cs`, the `created` field retrieved from the database is non-nullable, so null checks for the `created` field are not necessary.
Applied to files:
src/Altinn.Broker.Application/GenerateReport/GenerateDailySummaryReportHandler.cs
📚 Learning: 2024-11-14T07:20:44.903Z
Learnt from: Ceredron
Repo: Altinn/altinn-broker PR: 582
File: src/Altinn.Broker.Persistence/Repositories/ServiceOwnerRepository.cs:30-31
Timestamp: 2024-11-14T07:20:44.903Z
Learning: In `src/Altinn.Broker.Persistence/Repositories/ServiceOwnerRepository.cs`, the `service_owner_name` column in the `service_owner` table is not nullable, so null checks before calling `reader.GetString` are unnecessary.
Applied to files:
src/Altinn.Broker.Application/GenerateReport/GenerateDailySummaryReportHandler.cs
📚 Learning: 2024-11-14T07:00:50.257Z
Learnt from: Ceredron
Repo: Altinn/altinn-broker PR: 582
File: src/Altinn.Broker.Integrations/Azure/AzureResourceManagerService.cs:55-67
Timestamp: 2024-11-14T07:00:50.257Z
Learning: In the Altinn.Broker project, exceptions during background job enqueuing are already handled and logged elsewhere, so adding try-catch blocks in methods like `CreateStorageProviders` is unnecessary.
Applied to files:
src/Altinn.Broker.Application/GenerateReport/GenerateDailySummaryReportHandler.cs
🧬 Code graph analysis (1)
src/Altinn.Broker.Application/GenerateReport/GenerateDailySummaryReportHandler.cs (11)
src/Altinn.Broker.Core/Options/ReportStorageOptions.cs (1)
ReportStorageOptions(3-6)src/Altinn.Broker.Core/Repositories/IAltinnResourceRepoistory.cs (2)
Task(6-6)Task(12-12)src/Altinn.Broker.Integrations/Altinn/ResourceRegistry/AltinnResourceRegistryRepository.cs (2)
Task(28-62)Task(64-85)src/Altinn.Broker.Core/Repositories/IFileTransferRepository.cs (10)
Task(7-18)Task(19-19)Task(20-20)Task(21-21)Task(22-22)Task(23-23)Task(24-24)Task(25-31)Task(32-32)Task(33-33)src/Altinn.Broker.Application/GenerateReport/GenerateDailySummaryReportResponse.cs (1)
GenerateDailySummaryReportResponse(3-45)src/Altinn.Broker.Application/GenerateReport/GenerateDailySummaryReportRequest.cs (1)
GenerateDailySummaryReportRequest(6-15)src/Altinn.Broker.Application/Errors.cs (1)
StatisticsErrors(34-38)src/Altinn.Broker.Application/GenerateReport/DailySummaryData.cs (1)
DailySummaryData(9-77)src/Altinn.Broker.Common/StringExtensions.cs (3)
WithoutPrefix(77-84)IsOrganizationNumber(18-21)IsSocialSecurityNumber(28-31)src/Altinn.Broker.Application/GenerateReport/ParquetDailySummaryData.cs (1)
ParquetDailySummaryData(9-93)src/Altinn.Broker.Application/GenerateReport/GenerateAndDownloadDailySummaryReportResponse.cs (1)
GenerateAndDownloadDailySummaryReportResponse(3-50)
⏰ 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). (1)
- GitHub Check: Analyze (csharp)
🔇 Additional comments (3)
src/Altinn.Broker.API/Filters/StatisticsApiKeyFilter.cs (3)
135-147: LGTM: Correct constant-time comparison implementation.The implementation correctly prevents timing attacks by:
- XOR'ing all characters regardless of early mismatches
- Using bitwise OR to accumulate differences
- Only checking the result after the full comparison
The early return on length mismatch is safe since length comparison is constant-time.
48-48: IP logging is essential for rate limiting; verify organizational privacy compliance requirements.The IP logging at lines 48, 56, and 73 serves legitimate security purposes: authentication failure tracking and rate limiting. However, you must verify with your organization's privacy and legal teams:
- Identify your legal basis: IP logging for rate limiting and security monitoring typically relies on "legitimate interest" (security) under GDPR. Confirm this aligns with your privacy policy.
- Check data retention: Verify log retention periods comply with GDPR/CCPA and your policy. Configure log lifecycle/archival accordingly.
- Note: IP hashing is not viable here — the rate limiting mechanism (lines 79–86) requires readable IPs as cache keys.
If stricter privacy controls are required, consider:
- Adjusting log levels for auth failures (e.g., reduce to Debug in production)
- Implementing separate, shorter-retention logs for rate limit data
- Adding a privacy notice in filter comments explaining the security rationale
116-130: Clarify trusted proxy assumption and consider ForwardedHeadersMiddleware for defense-in-depth.The security concern about header spoofing is valid but context-dependent. The application is deployed behind Azure API Management (APIM) per Infrastructure.md, which serves as the trusted proxy and entry point—meaning direct internet traffic is not accepted. However, the implementation lacks explicit validation of this assumption:
Issues:
- No ForwardedHeadersMiddleware configured despite checking proxy headers, so the application implicitly trusts headers without ASP.NET Core's validation layer
- Rate limiting relies on IP extraction from headers (line 79), making it vulnerable if APIM stripping fails or configuration changes
- No documentation indicating that security depends on APIM stripping/validating headers
Recommendation:
Add ForwardedHeadersMiddleware in Program.cs with APIM's IP ranges configured, even behind a trusted proxy. This provides defense-in-depth and explicit, verifiable header validation:var forwardedHeadersOptions = new ForwardedHeadersOptions { ForwardedHeaders = ForwardedHeaders.XForwardedFor | ForwardedHeaders.XForwardedProto, KnownNetworks = { /* APIM network CIDR */ }, }; app.UseForwardedHeaders(forwardedHeadersOptions);Then
GetClientIpAddress()can safely usecontext.Connection.RemoteIpAddressdirectly after middleware processing. Document the deployment assumption to prevent future regressions.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
src/Altinn.Broker.Application/GenerateReport/GenerateDailySummaryReportHandler.cs (2)
54-55: TotalFileTransferCount is inflated by per-recipient expansion.
AggregateDailyDataAsyncflattens each file transfer into one row per recipient (lines 78-95), then groups and counts. A single transfer with three recipients yields three rows, each contributing toMessageCount. Summing those counts reports the same transfer thrice, inflatingTotalFileTransferCountand misrepresenting the actual number of unique transfers.Apply this fix to report the unique transfer count:
- TotalFileTransferCount = summaryData.Sum(d => d.MessageCount), + TotalFileTransferCount = fileTransfers.Count,
419-420: Same inflation issue in ProcessAndDownload.Identical problem: summing
MessageCountafter per-recipient flattening double-counts transfers.Apply the same fix:
- TotalFileTransferCount = summaryData.Sum(d => d.MessageCount), + TotalFileTransferCount = fileTransfers.Count,
🧹 Nitpick comments (2)
src/Altinn.Broker.Persistence/Repositories/FileTransferRepository.cs (1)
787-801: Redundant duplicate check when DISTINCT ON already guarantees uniqueness.The SQL query uses
DISTINCT ON (f.file_transfer_id_pk, recipient.actor_id_pk)ordered by status date descending, which guarantees exactly one row per (file transfer, recipient) pair with the latest status. The.Any()check at line 788 is therefore redundant—each iteration processes a distinct recipient.Remove the duplicate check to simplify the code:
if (!reader.IsDBNull(reader.GetOrdinal("recipient_actor_id_pk"))) { var recipientActorId = reader.GetInt64(reader.GetOrdinal("recipient_actor_id_pk")); var recipientActorExternalId = reader.GetString(reader.GetOrdinal("recipient_actor_external_id")); var recipientStatus = reader.GetInt32(reader.GetOrdinal("actor_file_transfer_status_description_id_fk")); var recipientDate = reader.GetDateTime(reader.GetOrdinal("actor_file_transfer_status_date")); - // Check if this recipient already exists (avoid duplicates) - if (!fileTransfer.RecipientCurrentStatuses.Any(r => r.Actor.ActorId == recipientActorId)) - { fileTransfer.RecipientCurrentStatuses.Add(new ActorFileTransferStatusEntity { FileTransferId = fileTransferId, Actor = new ActorEntity { ActorId = recipientActorId, ActorExternalId = recipientActorExternalId }, Status = (ActorFileTransferStatus)recipientStatus, Date = recipientDate }); - } }(Apply the same change at line 906 in
GetFileTransfersForReportWithServiceOwnerIds.)src/Altinn.Broker.Application/GenerateReport/GenerateDailySummaryReportHandler.cs (1)
280-285: Database storage calculation is a stub.Line 283 returns
0with a TODO comment indicating database storage calculation is not implemented. Verify whether accurate database storage metrics are required for the initial release or can be added later.Do you want me to suggest an implementation for calculating database storage based on file transfer metadata, or should this be tracked separately?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/Altinn.Broker.Application/GenerateReport/GenerateDailySummaryReportHandler.cs(1 hunks)src/Altinn.Broker.Core/Repositories/IFileTransferRepository.cs(1 hunks)src/Altinn.Broker.Persistence/Repositories/FileTransferRepository.cs(1 hunks)
🧰 Additional context used
🧠 Learnings (10)
📚 Learning: 2024-10-29T12:12:50.346Z
Learnt from: Ceredron
Repo: Altinn/altinn-broker PR: 560
File: tests/Altinn.Broker.Tests/Factories/FileTransferEntityFactory.cs:8-45
Timestamp: 2024-10-29T12:12:50.346Z
Learning: When suggesting code improvements in `tests/Altinn.Broker.Tests/Factories/FileTransferEntityFactory.cs`, avoid adding excessive comments and unnecessary code changes.
Applied to files:
src/Altinn.Broker.Core/Repositories/IFileTransferRepository.cssrc/Altinn.Broker.Persistence/Repositories/FileTransferRepository.cssrc/Altinn.Broker.Application/GenerateReport/GenerateDailySummaryReportHandler.cs
📚 Learning: 2024-11-14T07:12:14.214Z
Learnt from: Ceredron
Repo: Altinn/altinn-broker PR: 582
File: src/Altinn.Broker.Persistence/Repositories/FileTransferRepository.cs:176-177
Timestamp: 2024-11-14T07:12:14.214Z
Learning: In `src/Altinn.Broker.Persistence/Repositories/FileTransferRepository.cs`, avoid suggesting to extract inline SQL queries into constants.
Applied to files:
src/Altinn.Broker.Core/Repositories/IFileTransferRepository.cssrc/Altinn.Broker.Persistence/Repositories/FileTransferRepository.cssrc/Altinn.Broker.Application/GenerateReport/GenerateDailySummaryReportHandler.cs
📚 Learning: 2025-09-30T11:29:20.339Z
Learnt from: tomshag
Repo: Altinn/altinn-broker PR: 790
File: src/Altinn.Broker.Persistence/Repositories/FileTransferRepository.cs:349-352
Timestamp: 2025-09-30T11:29:20.339Z
Learning: In the Altinn Broker file transfer system, file transfer status and actor file transfer status use status_description_id_fk values that are ordered by progression level. Ordering by status_description_id_fk DESC is intentional to get the highest/most advanced status, not the most recent by date. This applies to queries in FileTransferRepository.cs.
Applied to files:
src/Altinn.Broker.Core/Repositories/IFileTransferRepository.cssrc/Altinn.Broker.Persistence/Repositories/FileTransferRepository.cs
📚 Learning: 2024-11-14T07:20:20.158Z
Learnt from: Ceredron
Repo: Altinn/altinn-broker PR: 582
File: src/Altinn.Broker.Persistence/Repositories/ServiceOwnerRepository.cs:36-47
Timestamp: 2024-11-14T07:20:20.158Z
Learning: In `src/Altinn.Broker.Persistence/Repositories/ServiceOwnerRepository.cs`, the `created` field retrieved from the database is non-nullable, so null checks for the `created` field are not necessary.
Applied to files:
src/Altinn.Broker.Persistence/Repositories/FileTransferRepository.cssrc/Altinn.Broker.Application/GenerateReport/GenerateDailySummaryReportHandler.cs
📚 Learning: 2024-10-29T12:51:11.540Z
Learnt from: Ceredron
Repo: Altinn/altinn-broker PR: 560
File: tests/Altinn.Broker.Tests/ManifestDownloadStreamTests.cs:24-39
Timestamp: 2024-10-29T12:51:11.540Z
Learning: In `tests/Altinn.Broker.Tests/ManifestDownloadStreamTests.cs`, refactoring suggestions aimed at enhancing test organization and coverage may be considered less readable and of dubious value.
Applied to files:
src/Altinn.Broker.Persistence/Repositories/FileTransferRepository.cssrc/Altinn.Broker.Application/GenerateReport/GenerateDailySummaryReportHandler.cs
📚 Learning: 2024-11-14T07:20:44.903Z
Learnt from: Ceredron
Repo: Altinn/altinn-broker PR: 582
File: src/Altinn.Broker.Persistence/Repositories/ServiceOwnerRepository.cs:30-31
Timestamp: 2024-11-14T07:20:44.903Z
Learning: In `src/Altinn.Broker.Persistence/Repositories/ServiceOwnerRepository.cs`, the `service_owner_name` column in the `service_owner` table is not nullable, so null checks before calling `reader.GetString` are unnecessary.
Applied to files:
src/Altinn.Broker.Persistence/Repositories/FileTransferRepository.cssrc/Altinn.Broker.Application/GenerateReport/GenerateDailySummaryReportHandler.cs
📚 Learning: 2024-10-29T12:50:05.699Z
Learnt from: Ceredron
Repo: Altinn/altinn-broker PR: 560
File: tests/Altinn.Broker.Tests/Helpers/ManifestDownloadStreamHelpers.cs:18-28
Timestamp: 2024-10-29T12:50:05.699Z
Learning: In `tests/Altinn.Broker.Tests/Helpers/ManifestDownloadStreamHelpers.cs`, the test code is static and will not change, so refactoring suggestions on this file are unnecessary.
Applied to files:
src/Altinn.Broker.Application/GenerateReport/GenerateDailySummaryReportHandler.cs
📚 Learning: 2024-10-29T12:45:23.330Z
Learnt from: Ceredron
Repo: Altinn/altinn-broker PR: 560
File: src/Altinn.Broker.Core/Helpers/ManifestDownloadStream.cs:199-237
Timestamp: 2024-10-29T12:45:23.330Z
Learning: In the `ManifestDownloadStream` class (`src/Altinn.Broker.Core/Helpers/ManifestDownloadStream.cs`), optimizing memory usage in `AddManifestFile` by modifying the ZIP archive without loading the entire content into memory is currently not feasible because the base stream is a `ReadOnlyMemoryStream` based on a download stream. This optimization will be evaluated when the download stream is re-written in the "Support bigger files"-PR.
Applied to files:
src/Altinn.Broker.Application/GenerateReport/GenerateDailySummaryReportHandler.cs
📚 Learning: 2024-11-14T07:00:50.257Z
Learnt from: Ceredron
Repo: Altinn/altinn-broker PR: 582
File: src/Altinn.Broker.Integrations/Azure/AzureResourceManagerService.cs:55-67
Timestamp: 2024-11-14T07:00:50.257Z
Learning: In the Altinn.Broker project, exceptions during background job enqueuing are already handled and logged elsewhere, so adding try-catch blocks in methods like `CreateStorageProviders` is unnecessary.
Applied to files:
src/Altinn.Broker.Application/GenerateReport/GenerateDailySummaryReportHandler.cs
📚 Learning: 2024-10-29T11:25:42.867Z
Learnt from: Andreass2
Repo: Altinn/altinn-broker PR: 560
File: src/Altinn.Broker.Application/DownloadFile/DownloadFileHandler.cs:49-55
Timestamp: 2024-10-29T11:25:42.867Z
Learning: In `DownloadFileHandler` in `src/Altinn.Broker.Application/DownloadFile/DownloadFileHandler.cs`, due to the 1 GB file size limit, loading the entire file into memory is acceptable in this context.
Applied to files:
src/Altinn.Broker.Application/GenerateReport/GenerateDailySummaryReportHandler.cs
🧬 Code graph analysis (3)
src/Altinn.Broker.Core/Repositories/IFileTransferRepository.cs (1)
src/Altinn.Broker.Core/Domain/FileTransferEntity.cs (1)
FileTransferEntity(3-21)
src/Altinn.Broker.Persistence/Repositories/FileTransferRepository.cs (6)
src/Altinn.Broker.Core/Repositories/IFileTransferRepository.cs (11)
Task(7-18)Task(19-19)Task(20-20)Task(21-21)Task(22-22)Task(23-23)Task(24-24)Task(25-31)Task(32-32)Task(33-33)Task(34-34)src/Altinn.Broker.API/Mappers/FileStatusOverviewExtMapper.cs (1)
List(70-85)src/Altinn.Broker.Core/Domain/FileTransferEntity.cs (1)
FileTransferEntity(3-21)src/Altinn.Broker.Core/Domain/ActorEntity.cs (1)
ActorEntity(3-7)src/Altinn.Broker.Core/Domain/ActorFileTransferStatusEntity.cs (1)
ActorFileTransferStatusEntity(3-9)src/Altinn.Broker.Core/Domain/FileTransferStatusEntity.cs (1)
FileTransferStatusEntity(2-8)
src/Altinn.Broker.Application/GenerateReport/GenerateDailySummaryReportHandler.cs (10)
src/Altinn.Broker.Core/Options/ReportStorageOptions.cs (1)
ReportStorageOptions(3-6)src/Altinn.Broker.Core/Repositories/IFileTransferRepository.cs (11)
Task(7-18)Task(19-19)Task(20-20)Task(21-21)Task(22-22)Task(23-23)Task(24-24)Task(25-31)Task(32-32)Task(33-33)Task(34-34)src/Altinn.Broker.Persistence/Repositories/FileTransferRepository.cs (3)
Task(112-172)Task(174-223)Task(238-274)src/Altinn.Broker.Application/GenerateReport/GenerateDailySummaryReportResponse.cs (1)
GenerateDailySummaryReportResponse(3-45)src/Altinn.Broker.Application/GenerateReport/GenerateDailySummaryReportRequest.cs (1)
GenerateDailySummaryReportRequest(6-15)src/Altinn.Broker.Application/Errors.cs (1)
StatisticsErrors(34-38)src/Altinn.Broker.Application/GenerateReport/DailySummaryData.cs (1)
DailySummaryData(9-77)src/Altinn.Broker.Common/StringExtensions.cs (3)
WithoutPrefix(77-84)IsOrganizationNumber(18-21)IsSocialSecurityNumber(28-31)src/Altinn.Broker.Application/GenerateReport/ParquetDailySummaryData.cs (1)
ParquetDailySummaryData(9-93)src/Altinn.Broker.Application/GenerateReport/GenerateAndDownloadDailySummaryReportResponse.cs (1)
GenerateAndDownloadDailySummaryReportResponse(3-50)
⏰ 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). (1)
- GitHub Check: Analyze (csharp)
🔇 Additional comments (4)
src/Altinn.Broker.Core/Repositories/IFileTransferRepository.cs (1)
33-34: LGTM! Clean interface additions for report generation.The two new methods follow existing patterns and provide appropriate return types. The second method's tuple return value efficiently bundles file transfers with their service owner IDs, avoiding subsequent lookups.
src/Altinn.Broker.Application/GenerateReport/GenerateDailySummaryReportHandler.cs (2)
110-161: Well done—N+1 lookups are now properly batched and parallelized.Lines 110-161 correctly gather distinct IDs up front, then use
Task.WhenAllto fetch service-owner and resource metadata in parallel. This eliminates the blocking.GetAwaiter().GetResult()anti-pattern flagged in previous reviews and avoids cascading N+1 queries.
312-343: Properly uses injected storage configuration.The method now relies on
reportStorageOptions.Value.ConnectionStringfrom DI (line 318) and validates its presence before creating theBlobServiceClient. This addresses the previous concern about hard-coded environment variables and emulator fallbacks.src/Altinn.Broker.Persistence/Repositories/FileTransferRepository.cs (1)
696-721: Clarify intended scope: daily report should filter by date range, not fetch all transfers.The review comment identifies a real concern, but verification reveals additional context. The actual method called is
GetFileTransfersForReportWithServiceOwnerIds(lines 809–839), which accepts no date parameters and fetches all file transfers into memory, then groups by date in the handler (line 213 of GenerateDailySummaryReportHandler).The problem: A method named for a "daily" summary report should not load all historical file transfers without filtering. The method signature lacks date parameters entirely, making it impossible for callers to scope results to a specific date range.
Two issues to address:
Add date range parameters to
GetFileTransfersForReportWithServiceOwnerIdsand update the query to filter byf.created(not fetching all transfers).Clarify handler logic: GenerateDailySummaryReportHandler should specify which date range to report on (e.g., previous day, last 7 days, configurable). Currently it aggregates all transfers across dates, which may not be the intended behavior for a "daily" report.
Without understanding whether fetching all transfers is intentional (for historical summarization) or an oversight (should be one day only), and without access to production database size, this cannot be fully resolved. Manual verification required to confirm expected scope.
src/Altinn.Broker.Application/GenerateReport/GenerateDailySummaryReportHandler.cs
Show resolved
Hide resolved
src/Altinn.Broker.Persistence/Repositories/FileTransferRepository.cs
Outdated
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/Altinn.Broker.Persistence/Repositories/FileTransferRepository.cs (1)
788-801: Redundant deduplication check if DISTINCT ON works correctly.The
DISTINCT ON (f.file_transfer_id_pk, recipient.actor_id_pk)in the query (line 697) should already ensure each recipient appears once per file transfer. The in-memory check usingAny()at line 788 is redundant and adds O(n×m) complexity.If you're confident the DISTINCT ON clause works correctly, simplify to:
// Add recipient if present (check for duplicates) if (!reader.IsDBNull(reader.GetOrdinal("recipient_actor_id_pk"))) { var recipientActorId = reader.GetInt64(reader.GetOrdinal("recipient_actor_id_pk")); var recipientActorExternalId = reader.GetString(reader.GetOrdinal("recipient_actor_external_id")); var recipientStatus = reader.GetInt32(reader.GetOrdinal("actor_file_transfer_status_description_id_fk")); var recipientDate = reader.GetDateTime(reader.GetOrdinal("actor_file_transfer_status_date")); - // Check if this recipient already exists (avoid duplicates) - if (!fileTransfer.RecipientCurrentStatuses.Any(r => r.Actor.ActorId == recipientActorId)) - { - fileTransfer.RecipientCurrentStatuses.Add(new ActorFileTransferStatusEntity - { - FileTransferId = fileTransferId, - Actor = new ActorEntity - { - ActorId = recipientActorId, - ActorExternalId = recipientActorExternalId - }, - Status = (ActorFileTransferStatus)recipientStatus, - Date = recipientDate - }); - } + fileTransfer.RecipientCurrentStatuses.Add(new ActorFileTransferStatusEntity + { + FileTransferId = fileTransferId, + Actor = new ActorEntity + { + ActorId = recipientActorId, + ActorExternalId = recipientActorExternalId + }, + Status = (ActorFileTransferStatus)recipientStatus, + Date = recipientDate + }); }Otherwise, replace the O(n)
Any()with aHashSet<long>per file transfer to track seen recipient IDs in O(1).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/Altinn.Broker.Core/Repositories/IFileTransferRepository.cs(1 hunks)src/Altinn.Broker.Persistence/Repositories/FileTransferRepository.cs(1 hunks)
🧰 Additional context used
🧠 Learnings (5)
📚 Learning: 2024-11-14T07:12:14.214Z
Learnt from: Ceredron
Repo: Altinn/altinn-broker PR: 582
File: src/Altinn.Broker.Persistence/Repositories/FileTransferRepository.cs:176-177
Timestamp: 2024-11-14T07:12:14.214Z
Learning: In `src/Altinn.Broker.Persistence/Repositories/FileTransferRepository.cs`, avoid suggesting to extract inline SQL queries into constants.
Applied to files:
src/Altinn.Broker.Core/Repositories/IFileTransferRepository.cssrc/Altinn.Broker.Persistence/Repositories/FileTransferRepository.cs
📚 Learning: 2024-10-29T12:12:50.346Z
Learnt from: Ceredron
Repo: Altinn/altinn-broker PR: 560
File: tests/Altinn.Broker.Tests/Factories/FileTransferEntityFactory.cs:8-45
Timestamp: 2024-10-29T12:12:50.346Z
Learning: When suggesting code improvements in `tests/Altinn.Broker.Tests/Factories/FileTransferEntityFactory.cs`, avoid adding excessive comments and unnecessary code changes.
Applied to files:
src/Altinn.Broker.Core/Repositories/IFileTransferRepository.cssrc/Altinn.Broker.Persistence/Repositories/FileTransferRepository.cs
📚 Learning: 2025-09-30T11:29:20.339Z
Learnt from: tomshag
Repo: Altinn/altinn-broker PR: 790
File: src/Altinn.Broker.Persistence/Repositories/FileTransferRepository.cs:349-352
Timestamp: 2025-09-30T11:29:20.339Z
Learning: In the Altinn Broker file transfer system, file transfer status and actor file transfer status use status_description_id_fk values that are ordered by progression level. Ordering by status_description_id_fk DESC is intentional to get the highest/most advanced status, not the most recent by date. This applies to queries in FileTransferRepository.cs.
Applied to files:
src/Altinn.Broker.Persistence/Repositories/FileTransferRepository.cs
📚 Learning: 2024-11-14T07:20:44.903Z
Learnt from: Ceredron
Repo: Altinn/altinn-broker PR: 582
File: src/Altinn.Broker.Persistence/Repositories/ServiceOwnerRepository.cs:30-31
Timestamp: 2024-11-14T07:20:44.903Z
Learning: In `src/Altinn.Broker.Persistence/Repositories/ServiceOwnerRepository.cs`, the `service_owner_name` column in the `service_owner` table is not nullable, so null checks before calling `reader.GetString` are unnecessary.
Applied to files:
src/Altinn.Broker.Persistence/Repositories/FileTransferRepository.cs
📚 Learning: 2024-11-14T07:20:20.158Z
Learnt from: Ceredron
Repo: Altinn/altinn-broker PR: 582
File: src/Altinn.Broker.Persistence/Repositories/ServiceOwnerRepository.cs:36-47
Timestamp: 2024-11-14T07:20:20.158Z
Learning: In `src/Altinn.Broker.Persistence/Repositories/ServiceOwnerRepository.cs`, the `created` field retrieved from the database is non-nullable, so null checks for the `created` field are not necessary.
Applied to files:
src/Altinn.Broker.Persistence/Repositories/FileTransferRepository.cs
🧬 Code graph analysis (2)
src/Altinn.Broker.Core/Repositories/IFileTransferRepository.cs (2)
src/Altinn.Broker.API/Mappers/LegacyFileStatusOverviewExtMapper.cs (2)
List(12-15)List(107-122)src/Altinn.Broker.Core/Domain/FileTransferEntity.cs (1)
FileTransferEntity(3-21)
src/Altinn.Broker.Persistence/Repositories/FileTransferRepository.cs (5)
src/Altinn.Broker.Core/Repositories/IFileTransferRepository.cs (10)
Task(7-18)Task(19-19)Task(20-20)Task(21-21)Task(22-22)Task(23-23)Task(24-24)Task(25-31)Task(32-32)Task(33-33)src/Altinn.Broker.Core/Domain/FileTransferEntity.cs (1)
FileTransferEntity(3-21)src/Altinn.Broker.Core/Domain/ActorEntity.cs (1)
ActorEntity(3-7)src/Altinn.Broker.Core/Domain/ActorFileTransferStatusEntity.cs (1)
ActorFileTransferStatusEntity(3-9)src/Altinn.Broker.Core/Domain/FileTransferStatusEntity.cs (1)
FileTransferStatusEntity(2-8)
⏰ 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). (1)
- GitHub Check: Analyze (csharp)
🔇 Additional comments (2)
src/Altinn.Broker.Persistence/Repositories/FileTransferRepository.cs (1)
747-777: LGTM: Entity construction optimized for reporting.The entity construction correctly:
- Uses dictionary lookup to avoid duplicates
- Sets default FileTransferStatus (UploadStarted) and empty PropertyList to avoid N+1 queries
- Handles NULL values appropriately for optional fields
The intentional deviations from full entity hydration are acceptable for reporting scenarios where these fields aren't needed.
src/Altinn.Broker.Core/Repositories/IFileTransferRepository.cs (1)
33-33: No changes needed - method design is appropriate for daily batch reporting context.The method is intentionally designed to retrieve all file transfers without filtering parameters. This is not a flaw but rather an appropriate design for its specific use case:
- Scheduled batch operation: Only called from
GenerateDailySummaryReportHandlerfor daily report generation, not an API endpoint- Aggregation pattern: Retrieves all transfers → aggregates by day and service owner → generates report—filtering parameters would be inappropriate here
- Time boundary exists implicitly: The daily execution schedule provides the time boundary, not query parameters
- Performance optimized: The query uses
DISTINCT ONand optimized joins specifically to avoid N+1 problems for this batch operationThe original concern about unbounded data retrieval doesn't apply to batch reporting scenarios where all data must be aggregated.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/Altinn.Broker.Common/StringExtensions.cs (1)
10-11: Trim unused weight constants to match Mod11 definitions.Only the first 9/10 entries are ever read, so the trailing
1values are dead weight and risk confusing the next reviewer. Aligning the arrays with the documented Mod11 sequences keeps the intent obvious.(lefdal.cc)- private static readonly int[] SocialSecurityNumberWeights1 = [3, 7, 6, 1, 8, 9, 4, 5, 2, 1]; - private static readonly int[] SocialSecurityNumberWeights2 = [5, 4, 3, 2, 7, 6, 5, 4, 3, 2, 1]; + private static readonly int[] SocialSecurityNumberWeights1 = [3, 7, 6, 1, 8, 9, 4, 5, 2]; + private static readonly int[] SocialSecurityNumberWeights2 = [5, 4, 3, 2, 7, 6, 5, 4, 3, 2];
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.github/actions/update-infrastructure/action.yml(2 hunks).github/workflows/deploy-to-environment.yml(1 hunks)src/Altinn.Broker.Common/StringExtensions.cs(2 hunks)
🧰 Additional context used
🧠 Learnings (5)
📚 Learning: 2024-12-08T22:55:30.559Z
Learnt from: Ceredron
Repo: Altinn/altinn-broker PR: 610
File: src/Altinn.Broker.Common/StringExtensions.cs:23-30
Timestamp: 2024-12-08T22:55:30.559Z
Learning: In `src/Altinn.Broker.Common/StringExtensions.cs`, the `WithoutPrefix` method in the C# project checks if `orgNumber` is null or whitespace and returns an empty string if it is. Therefore, calling `orgNumber.Split(':').Last()` is safe, as the method ensures `orgNumber` is non-empty before splitting.
Applied to files:
src/Altinn.Broker.Common/StringExtensions.cs
📚 Learning: 2024-12-08T23:15:58.682Z
Learnt from: Ceredron
Repo: Altinn/altinn-broker PR: 610
File: src/Altinn.Broker.Core/StringExtensions.cs:0-0
Timestamp: 2024-12-08T23:15:58.682Z
Learning: In `src/Altinn.Broker.Core/StringExtensions.cs`, prefer keeping the `WithoutPrefix` method simple and readable, avoiding complex modifications that reduce readability.
Applied to files:
src/Altinn.Broker.Common/StringExtensions.cs
📚 Learning: 2024-10-29T12:15:40.824Z
Learnt from: Ceredron
Repo: Altinn/altinn-broker PR: 560
File: src/Altinn.Broker.Core/Helpers/ManifestDownloadStream.cs:239-249
Timestamp: 2024-10-29T12:15:40.824Z
Learning: In the `ManifestDownloadStream` class (`src/Altinn.Broker.Core/Helpers/ManifestDownloadStream.cs`), ZIP file validation is performed later in the code within the `AddManifestFile` method, so enhancing the `IsZipFile` method is unnecessary.
Applied to files:
src/Altinn.Broker.Common/StringExtensions.cs
📚 Learning: 2024-10-23T12:43:47.413Z
Learnt from: Ceredron
Repo: Altinn/altinn-broker PR: 552
File: src/Altinn.Broker.API/Helpers/AltinnTokenEventsHelper.cs:11-22
Timestamp: 2024-10-23T12:43:47.413Z
Learning: In the Altinn Broker API, when handling authentication failures in `AltinnTokenEventsHelper.OnAuthenticationFailed`, issuer values are case-sensitive, so issuer comparisons should be case-sensitive.
Applied to files:
src/Altinn.Broker.Common/StringExtensions.cs
📚 Learning: 2024-10-29T12:12:50.346Z
Learnt from: Ceredron
Repo: Altinn/altinn-broker PR: 560
File: tests/Altinn.Broker.Tests/Factories/FileTransferEntityFactory.cs:8-45
Timestamp: 2024-10-29T12:12:50.346Z
Learning: When suggesting code improvements in `tests/Altinn.Broker.Tests/Factories/FileTransferEntityFactory.cs`, avoid adding excessive comments and unnecessary code changes.
Applied to files:
src/Altinn.Broker.Common/StringExtensions.cs
⏰ 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). (1)
- GitHub Check: Analyze (csharp)
🔇 Additional comments (3)
.github/actions/update-infrastructure/action.yml (2)
48-50: Input definition looks good.The new STATISTICS_API_KEY input is properly defined with a clear description and follows the existing pattern for required secret inputs in this action.
97-97: Environment variable propagation is correct.The STATISTICS_API_KEY is properly passed to the Azure deployment step, following the established pattern for other secrets.
.github/workflows/deploy-to-environment.yml (1)
75-75: Secret is correctly passed to the action.The STATISTICS_API_KEY secret is properly propagated to the update-infrastructure action, following the established pattern for other secrets.
Verify that the
STATISTICS_API_KEYsecret is configured in GitHub organization or environment settings for the appropriate environments (test, staging, production). Otherwise, the deployment will fail when this action attempts to use the secret.
Ceredron
approved these changes
Nov 14, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Made the same report endpoint as in Correspondence.
Related Issue(s)
Verification
Husk
Koordinere for å få på plass excel rapport.
Documentation
Summary by CodeRabbit
New Features
Security
Chores
New Utilities