Updated query for broker report #828
Conversation
📝 WalkthroughWalkthroughReplaced per-transfer retrieval with a new Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes
Pre-merge checks and finishing touches❌ Failed checks (2 warnings, 1 inconclusive)
✨ Finishing touches
🧪 Generate unit tests (beta)
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/chore
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/Altinn.Broker.Application/GenerateReport/GenerateDailySummaryReportHandler.cs (2)
139-153: Remove the unusedGetServiceOwnerIdAsyncmethod.The search confirms this method has no call sites in the codebase. After the refactoring to use
AggregatedDailySummaryData, this method is dead code and should be removed to maintain code clarity. Additionally, consider removing other potentially unused methods likeCalculateDatabaseStorageandCalculateAttachmentStorageif they're no longer called.
220-231: Remove dead code:CalculateDatabaseStorageandCalculateAttachmentStorageare unused after refactor.These private methods (lines 220–231) are no longer called anywhere in the handler. The refactored code retrieves storage values directly from SQL via
AggregatedDailySummaryData(which includesDatabaseStorageBytesandAttachmentStorageBytes), making these calculation methods redundant.CalculateDatabaseStorageis a TODO returning 0, andCalculateAttachmentStorageduplicates logic no longer needed. Delete both methods.
🧹 Nitpick comments (5)
src/Altinn.Broker.Core/Repositories/IFileTransferRepository.cs (2)
37-51: Consider movingAggregatedDailySummaryDatato a separate file in the Domain folder.Defining a data class within an interface file is unconventional. Consider placing it in
Altinn.Broker.Core.Domainor a dedicated DTOs folder for better discoverability and organization.Additionally, the
Year,Month, andDayproperties are redundant since they can be derived fromDate. If they're needed for SQL GROUP BY alignment, consider making them read-only computed properties:public class AggregatedDailySummaryData { public DateTime Date { get; set; } - public int Year { get; set; } - public int Month { get; set; } - public int Day { get; set; } + public int Year => Date.Year; + public int Month => Date.Month; + public int Day => Date.Day; public string ServiceOwnerId { get; set; } = string.Empty; // ... }
34-34: Consider adding date range parameters for scalability.The method fetches all aggregated data without any date range filtering. As the dataset grows, this could become a performance concern. Consider adding optional date range parameters:
Task<List<AggregatedDailySummaryData>> GetAggregatedDailySummaryData( DateOnly? from = null, DateOnly? to = null, CancellationToken cancellationToken = default);src/Altinn.Broker.Persistence/Repositories/FileTransferRepository.cs (2)
866-874: Redundant null checks due to COALESCE in SQL.The SQL query uses
COALESCE(..., 'unknown')forservice_owner_id,resource_id, andrecipient_id, so these columns will never be null. TheIsDBNullchecks are redundant:- ServiceOwnerId = reader.IsDBNull(reader.GetOrdinal("service_owner_id")) - ? "unknown" - : reader.GetString(reader.GetOrdinal("service_owner_id")), - ResourceId = reader.IsDBNull(reader.GetOrdinal("resource_id")) - ? "unknown" - : reader.GetString(reader.GetOrdinal("resource_id")), - RecipientId = reader.IsDBNull(reader.GetOrdinal("recipient_id")) - ? "unknown" - : reader.GetString(reader.GetOrdinal("recipient_id")), + ServiceOwnerId = reader.GetString(reader.GetOrdinal("service_owner_id")), + ResourceId = reader.GetString(reader.GetOrdinal("resource_id")), + RecipientId = reader.GetString(reader.GetOrdinal("recipient_id")),
851-851: 1-hour timeout may mask performance issues.A 3600-second timeout is very generous. While it may be necessary for large datasets, consider:
- Adding monitoring/logging for query duration to identify when optimization is needed
- Ensuring this query doesn't run frequently enough to exhaust the connection pool
- Adding an index on
broker.file_transfer(created)if not already present for theDATE(f.created)grouping#!/bin/bash # Check for existing indexes on the file_transfer table fd -e sql . | xargs rg -l "CREATE INDEX" | xargs rg -n "file_transfer" | head -20src/Altinn.Broker.Application/GenerateReport/GenerateDailySummaryReportHandler.cs (1)
335-335: Minor inconsistency in empty check.Line 335 uses
!aggregatedData.Any()while line 39 usesaggregatedData.Count == 0. Both work correctly, butCount == 0is slightly more efficient forList<T>as it avoids iterator allocation.- if (!aggregatedData.Any()) + if (aggregatedData.Count == 0)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/Altinn.Broker.Application/GenerateReport/GenerateDailySummaryReportHandler.cs(5 hunks)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.cssrc/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.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.Persistence/Repositories/FileTransferRepository.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.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
🧬 Code graph analysis (1)
src/Altinn.Broker.Persistence/Repositories/FileTransferRepository.cs (1)
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)AggregatedDailySummaryData(37-51)
⏰ 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)
803-885: Good optimization: SQL-level aggregation.The approach of moving aggregation to SQL with GROUP BY is a solid performance improvement over in-memory processing. The use of the denormalized
actor_file_transfer_latest_statustable aligns with existing patterns in this repository.src/Altinn.Broker.Application/GenerateReport/GenerateDailySummaryReportHandler.cs (1)
74-137: Good refactor: parallel enrichment eliminates N+1 queries.The
EnrichAggregatedDataAsyncmethod efficiently fetches unique service owner names and resource titles in parallel usingTask.WhenAll, then maps them to the aggregated data. This is a significant improvement over per-record lookups.
src/Altinn.Broker.Application/GenerateReport/GenerateDailySummaryReportHandler.cs
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/Altinn.Broker.Persistence/Repositories/FileTransferRepository.cs (1)
818-826: SQL recipient type classification doesn't match C# validation logic.The SQL patterns for recipient type classification are already flagged in a previous review. The regex patterns here don't align with the C# validation methods in
GenerateDailySummaryReportHandler.cs, which could cause inconsistent classification between the aggregated report data and other parts of the system.
🧹 Nitpick comments (2)
src/Altinn.Broker.Persistence/Repositories/FileTransferRepository.cs (2)
803-807: Consider adding date range parameters to limit data volume.The method aggregates all historical data without filtering, which could become slow as data accumulates. Consider adding
DateTimeOffset? fromDateandDateTimeOffset? toDateparameters to allow incremental report generation.
821-821: Optional: COALESCE is unnecessary around SPLIT_PART.
SPLIT_PARTreturns the original string when the delimiter isn't found, not NULL. The COALESCE wrapper adds no value but doesn't harm functionality either.Apply this diff if you want to simplify:
-WHEN COALESCE(SPLIT_PART(recipient.actor_external_id, ':', -1), recipient.actor_external_id) ~ '^\d{9}$' THEN 1 +WHEN SPLIT_PART(recipient.actor_external_id, ':', -1) ~ '^\d{9}$' THEN 1Repeat for lines 824, 845, 846.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/Altinn.Broker.Persistence/Repositories/FileTransferRepository.cs(1 hunks)
🧰 Additional context used
🧠 Learnings (4)
📚 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.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.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-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.cs
🧬 Code graph analysis (1)
src/Altinn.Broker.Persistence/Repositories/FileTransferRepository.cs (4)
src/Altinn.Broker.API/Mappers/FileTransferStatusDetailsExtMapper.cs (2)
List(34-39)List(41-50)src/Altinn.Broker.API/Mappers/LegacyFileStatusOverviewExtMapper.cs (2)
List(12-15)List(107-122)src/Altinn.Broker.API/Mappers/FileStatusOverviewExtMapper.cs (1)
List(70-85)src/Altinn.Broker.Application/GenerateReport/GenerateDailySummaryReportHandler.cs (1)
RecipientType(155-178)
🔇 Additional comments (3)
src/Altinn.Broker.Persistence/Repositories/FileTransferRepository.cs (3)
829-829: Verify that database_storage_bytes should be hardcoded to 0.The field is consistently set to 0 while
attachment_storage_bytesuses actual summed data. Confirm this is intentional and not a placeholder. If database storage isn't tracked separately in the broker, consider adding a comment explaining this.
898-918: LGTM! Proper NULL handling and clear data mapping.The data reader mapping correctly handles NULL values with appropriate defaults and uses explicit ordinal lookups for maintainability.
850-880: LGTM! UNION ALL structure correctly handles both recipient and non-recipient cases.The second part of the UNION properly captures file transfers without recipients using NOT EXISTS, ensuring complete data coverage. The ORDER BY is correctly placed to sort the entire result set.
src/Altinn.Broker.Persistence/Repositories/FileTransferRepository.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/Altinn.Broker.Persistence/Repositories/FileTransferRepository.cs (1)
803-808: Add date range parameters to enable incremental report generation.The method has no date range parameters and will scan the entire
file_transfertable history. As data accumulates over time, this will cause increasing memory usage, longer execution times, and potential timeouts. Without date filtering, generating daily/weekly/monthly reports incrementally is impossible.Add optional
DateTimeOffset? fromDateandDateTimeOffset? toDateparameters to the method signature and correspondingWHEREclause filters onf.createdin the SQL query. This allows callers to request specific date ranges and enables efficient incremental report generation.-public async Task<List<AggregatedDailySummaryData>> GetAggregatedDailySummaryData(CancellationToken cancellationToken) +public async Task<List<AggregatedDailySummaryData>> GetAggregatedDailySummaryData(DateTimeOffset? fromDate = null, DateTimeOffset? toDate = null, CancellationToken cancellationToken = default) { // Optimized query: Aggregate data directly in SQL using GROUP BY // Uses actor_file_transfer_status (history table) with CTE to get latest status per recipient // This matches the old query logic to ensure we get the same results // Includes file transfers with recipients AND file transfers without recipients - const string query = @" + var query = @" WITH latest_recipient_status AS ( -- Get the latest status per (file_transfer, recipient) pair SELECT DISTINCT ON (afs.file_transfer_id_fk, afs.actor_id_fk) @@ -843,6 +843,7 @@ FROM broker.file_transfer f INNER JOIN broker.actor sender ON sender.actor_id_pk = f.sender_actor_id_fk LEFT JOIN broker.altinn_resource r ON r.resource_id_pk = f.resource_id + WHERE 1=1 {0} LEFT JOIN latest_recipient_status lrs ON lrs.file_transfer_id_fk = f.file_transfer_id_pk LEFT JOIN broker.actor recipient ON recipient.actor_id_pk = lrs.actor_id_fk GROUP BYThen add parameter binding logic:
var dateFilter = new StringBuilder(); if (fromDate.HasValue) { dateFilter.Append(" AND f.created >= @fromDate"); } if (toDate.HasValue) { dateFilter.Append(" AND f.created <= @toDate"); } query = string.Format(query, dateFilter.ToString()); await using var command = dataSource.CreateCommand(query); command.CommandTimeout = 600; if (fromDate.HasValue) { command.Parameters.AddWithValue("@fromDate", fromDate.Value); } if (toDate.HasValue) { command.Parameters.AddWithValue("@toDate", toDate.Value); }
🧹 Nitpick comments (2)
src/Altinn.Broker.Persistence/Repositories/FileTransferRepository.cs (2)
832-832: Remove redundant COALESCE wrapper around SPLIT_PART.
SPLIT_PART(str, ':', -1)already returns the entire string when no delimiter is found, making theCOALESCEwrapper unnecessary.Apply this diff to simplify the expressions:
CASE -- Organization: Extract part after colon (if exists) and check if it's 9 digits -- Matches C# logic: WithoutPrefix() then IsOrganizationNumber() which accepts 9 digits WHEN recipient.actor_external_id IS NOT NULL - AND COALESCE(SPLIT_PART(recipient.actor_external_id, ':', -1), recipient.actor_external_id) ~ '^\d{9}$' THEN 1 + AND SPLIT_PART(recipient.actor_external_id, ':', -1) ~ '^\d{9}$' THEN 1 -- Person: Extract part after colon (if exists) and check if it's 11 digits -- Note: C# also validates mod11 checksum, but for aggregation format check is acceptable WHEN recipient.actor_external_id IS NOT NULL - AND COALESCE(SPLIT_PART(recipient.actor_external_id, ':', -1), recipient.actor_external_id) ~ '^\d{11}$' THEN 0 + AND SPLIT_PART(recipient.actor_external_id, ':', -1) ~ '^\d{11}$' THEN 0 ELSE 2 END as recipient_type,Also apply the same change in the GROUP BY clause (lines 857, 860).
Also applies to: 836-836
871-871: Consider making the timeout configurable or reducing it once date range filtering is implemented.The 10-minute timeout is a significant improvement over the previous 1-hour value mentioned in past reviews. However, it's still quite long and suggests the query may struggle with large datasets. Once date range parameters are added (as suggested above), typical execution times should drop significantly, allowing for a more reasonable timeout (e.g., 30-120 seconds).
Consider making the timeout configurable via application settings or reducing it once date range filtering reduces query execution time:
// Option 1: Use a more reasonable default once date filtering is in place command.CommandTimeout = 120; // 2 minutes // Option 2: Make it configurable (requires injecting configuration) command.CommandTimeout = _reportOptions.QueryTimeoutSeconds;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/Altinn.Broker.Persistence/Repositories/FileTransferRepository.cs(1 hunks)
🧰 Additional context used
🧠 Learnings (3)
📚 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.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.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
🧬 Code graph analysis (1)
src/Altinn.Broker.Persistence/Repositories/FileTransferRepository.cs (2)
src/Altinn.Broker.Core/Repositories/IFileTransferRepository.cs (1)
AggregatedDailySummaryData(37-51)src/Altinn.Broker.Application/GenerateReport/GenerateDailySummaryReportHandler.cs (1)
RecipientType(155-178)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Deploy
- GitHub Check: Analyze (csharp)
🔇 Additional comments (1)
src/Altinn.Broker.Persistence/Repositories/FileTransferRepository.cs (1)
873-904: LGTM! Result mapping is correct and follows best practices.The ADO.NET data reader usage is correct with proper NULL handling, ordinal-based column access, and appropriate fallback values for missing data. The retry wrapper adds resilience to transient failures.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/Altinn.Broker.Persistence/Repositories/FileTransferRepository.cs (1)
805-862: Consider adding date-range parameters to enable incremental report generation.The query currently processes all historical file transfers without date filtering. While the 10-minute timeout is an improvement over the previous 1-hour timeout, the absence of date-range parameters means:
- The query will slow down as data accumulates over time
- Incremental or date-specific reports cannot be generated
- The entire dataset must be reprocessed each time
Adding optional
@fromDateand@toDateparameters with aWHERE f.created BETWEEN @fromDate AND @toDateclause would enable more efficient, incremental reporting and reduce query execution time.src/Altinn.Broker.Application/GenerateReport/GenerateDailySummaryReportHandler.cs (1)
114-129: Consider defensive casting for RecipientType and AltinnVersion to prevent future runtime errors.The direct casts on lines 124-125 assume the SQL query always produces valid enum values. While the current SQL correctly generates values that align with the enum definitions (RecipientType: 0/1/2, AltinnVersion: 1), future query modifications could introduce invalid values, causing
InvalidCastExceptionat runtime.Apply this pattern for defensive casting:
- RecipientType = (RecipientType)d.RecipientType, - AltinnVersion = (AltinnVersion)d.AltinnVersion, + RecipientType = Enum.IsDefined(typeof(RecipientType), d.RecipientType) + ? (RecipientType)d.RecipientType + : RecipientType.Unknown, + AltinnVersion = Enum.IsDefined(typeof(AltinnVersion), d.AltinnVersion) + ? (AltinnVersion)d.AltinnVersion + : AltinnVersion.Altinn3,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/Altinn.Broker.Application/GenerateReport/GenerateDailySummaryReportHandler.cs(9 hunks)src/Altinn.Broker.Persistence/Repositories/FileTransferRepository.cs(1 hunks)
🧰 Additional context used
🧠 Learnings (12)
📚 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.Persistence/Repositories/FileTransferRepository.cssrc/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.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.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.Persistence/Repositories/FileTransferRepository.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.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: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/GenerateReport/GenerateDailySummaryReportHandler.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.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-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-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/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
🧬 Code graph analysis (1)
src/Altinn.Broker.Persistence/Repositories/FileTransferRepository.cs (5)
src/Altinn.Broker.Core/Repositories/IFileTransferRepository.cs (1)
AggregatedDailySummaryData(37-51)src/Altinn.Broker.API/Mappers/FileTransferStatusDetailsExtMapper.cs (2)
List(34-39)List(41-50)src/Altinn.Broker.API/Mappers/LegacyFileStatusOverviewExtMapper.cs (2)
List(12-15)List(107-122)src/Altinn.Broker.API/Mappers/FileStatusOverviewExtMapper.cs (1)
List(70-85)src/Altinn.Broker.Application/GenerateReport/GenerateDailySummaryReportHandler.cs (1)
RecipientType(146-169)
⏰ 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)
1 participant
Description
Optimized SQL query for generating report.
Related Issue(s)
Verification
Documentation
Summary by CodeRabbit
Refactor
New Features
✏️ Tip: You can customize this high-level summary in your review settings.