From f67a71857091719912469c1807a56d1ee8fee5fe Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ren=C3=A9=20Schultz=20Madsen?= Date: Wed, 29 Jul 2026 16:05:42 +0200 Subject: [PATCH 1/3] perf: batch the raw data export instead of holding it in memory The export built the entire result before writing anything: every answer id, every AnswerValue for those answers, one dictionary per row, and then a SheetData DOM holding every cell. That is what forced the row cap down to 25 000 in the first place, and the cap was a stopgap rather than a fix. Answers are now fetched ExportBatchSize (2000) at a time and streamed straight into the sheet, so peak memory is one batch regardless of how large the export is. - IRawDataExcelWriter replaces WriteRawDataToExcelFile. It opens the workbook, writes the header, takes rows one at a time via OpenXmlWriter, and finalises on Complete. Nothing accumulates. - RawDataService.ExportToFile owns the loop and keeps the SDK context open across batches, which it has to: every batch reads from the same query. Splitting the old Build into ResolveScope (offset-independent work: item, schema, answer query) and BuildRows (one window) lets the paged endpoint and the export share the pivot without sharing a lifetime. - The controller no longer creates files. It calls ExportToFile, streams the result and deletes it. - Export batches order by FinishedAt then Id. The tie-breaker added earlier is what makes batching safe: without it, answers sharing a timestamp could be duplicated into one batch and skipped from another. - ExportRowLimit rises 25 000 -> 250 000. Memory is no longer the binding constraint, so the cap now bounds file size and request duration. - Free-text answers are stripped of XML-illegal control characters, which previously produced a workbook Excel refuses to open. RawDataExportUTests covers the writer without needing a database: 5000 rows land in the file, a missing value leaves a gap at its own column reference rather than shifting later columns, a vertical tab is removed while tabs and newlines survive, an empty result still yields a valid header-only workbook, and abandoning the writer without completing does not throw. Verified locally, and checked non-vacuous by bypassing the sanitiser and watching the control-character test fail. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01EnP42zmHAgo2NZQsa6zdhG --- .../RawDataExportUTests.cs | 206 ++++++++ .../Controllers/RawDataController.cs | 52 +- .../IRawDataExcelService.cs | 20 +- .../RawDataExcelService.cs | 227 +++++--- .../RawDataService/IRawDataService.cs | 6 +- .../Services/RawDataService/RawDataService.cs | 485 +++++++++++------- 6 files changed, 692 insertions(+), 304 deletions(-) create mode 100644 eFormAPI/Plugins/InsightDashboard.Pn/InsightDashboard.Pn.Test/RawDataExportUTests.cs diff --git a/eFormAPI/Plugins/InsightDashboard.Pn/InsightDashboard.Pn.Test/RawDataExportUTests.cs b/eFormAPI/Plugins/InsightDashboard.Pn/InsightDashboard.Pn.Test/RawDataExportUTests.cs new file mode 100644 index 00000000..b2d794f6 --- /dev/null +++ b/eFormAPI/Plugins/InsightDashboard.Pn/InsightDashboard.Pn.Test/RawDataExportUTests.cs @@ -0,0 +1,206 @@ +/* +The MIT License (MIT) + +Copyright (c) 2007 - 2021 Microting A/S + +Permission is hereby granted, free of charge, to any person obtaining a copy +of this software and associated documentation files (the "Software"), to deal +in the Software without restriction, including without limitation the rights +to use, copy, modify, merge, publish, distribute, sublicense, and/or sell +copies of the Software, and to permit persons to whom the Software is +furnished to do so, subject to the following conditions: + +The above copyright notice and this permission notice shall be included in all +copies or substantial portions of the Software. + +THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR +IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, +FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE +AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER +LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, +OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE +SOFTWARE. +*/ + +namespace InsightDashboard.Pn.Test; + +using System; +using System.Collections.Generic; +using System.IO; +using System.Linq; +using DocumentFormat.OpenXml.Packaging; +using DocumentFormat.OpenXml.Spreadsheet; +using Infrastructure.Models.RawData; +using NUnit.Framework; +using Services.RawDataExcelService; + +/// +/// Covers the streaming xlsx writer that replaced the whole-sheet DOM. These need +/// no database: each writes a workbook to a temp file and reads it back. +/// +[TestFixture] +public class RawDataExportUTests +{ + private string _file; + + private static List Columns() => + [ + new() { Field = "id", Header = "Id", Kind = RawDataColumnKinds.Answer }, + new() { Field = "siteName", Header = "Site", Kind = RawDataColumnKinds.Answer }, + new() { Field = "q7", Header = "1 - Tilfredshed", Kind = RawDataColumnKinds.Smiley }, + ]; + + [SetUp] + public void SetUp() => + _file = Path.Combine(Path.GetTempPath(), $"rawdata-{Guid.NewGuid():N}.xlsx"); + + [TearDown] + public void TearDown() + { + if (File.Exists(_file)) + { + File.Delete(_file); + } + } + + private static List> ReadSheet(string path) + { + using var document = SpreadsheetDocument.Open(path, false); + var worksheetPart = document.WorkbookPart!.WorksheetParts.First(); + + return worksheetPart.Worksheet + .Descendants() + .Select(row => row.Elements() + .Select(cell => cell.CellValue?.Text ?? string.Empty) + .ToList()) + .ToList(); + } + + [Test] + public void Writer_WritesHeaderAndEveryRowStreamedToIt() + { + var service = new RawDataExcelService(null); + + using (var writer = service.CreateWriter(_file, Columns())) + { + // Comfortably more than one export batch, so this exercises the case + // the batching exists for. + for (var i = 1; i <= 5000; i++) + { + writer.WriteRow(new Dictionary + { + ["id"] = i, + ["siteName"] = $"Site {i}", + ["q7"] = "Glad (75)", + }); + } + + writer.Complete(); + } + + var sheet = ReadSheet(_file); + + Assert.That(sheet.Count, Is.EqualTo(5001), "Header plus every streamed row."); + Assert.That(sheet[0], Is.EqualTo(new List { "Id", "Site", "1 - Tilfredshed" })); + Assert.That(sheet[1], Is.EqualTo(new List { "1", "Site 1", "Glad (75)" })); + Assert.That(sheet[5000], Is.EqualTo(new List { "5000", "Site 5000", "Glad (75)" })); + } + + [Test] + public void Writer_KeepsColumnsAlignedWhenValuesAreMissing() + { + var service = new RawDataExcelService(null); + + using (var writer = service.CreateWriter(_file, Columns())) + { + // No siteName. The cell must be skipped at its own reference rather + // than shifted left, or every later column in the row would be wrong. + writer.WriteRow(new Dictionary { ["id"] = 1, ["q7"] = "Sur (25)" }); + writer.Complete(); + } + + using var document = SpreadsheetDocument.Open(_file, false); + var cells = document.WorkbookPart!.WorksheetParts.First().Worksheet + .Descendants().Last().Elements().ToList(); + + Assert.That(cells.Count, Is.EqualTo(2)); + Assert.That(cells[0].CellReference?.Value, Is.EqualTo("A2")); + Assert.That(cells[1].CellReference?.Value, Is.EqualTo("C2"), + "A missing value must leave a gap rather than shifting later columns."); + } + + [Test] + public void Writer_StripsControlCharactersThatWouldCorruptTheWorkbook() + { + var service = new RawDataExcelService(null); + + // A vertical tab is illegal in XML. Written raw, Excel refuses the file. + var hostile = "Bad\vvalue"; + + using (var writer = service.CreateWriter(_file, Columns())) + { + writer.WriteRow(new Dictionary + { + ["id"] = 1, + ["siteName"] = "Site", + ["q7"] = hostile, + }); + writer.Complete(); + } + + // Reopening is itself the assertion: an illegal character throws here. + var sheet = ReadSheet(_file); + + Assert.That(sheet.Count, Is.EqualTo(2)); + Assert.That(sheet[1][2], Is.EqualTo("Badvalue")); + Assert.That(sheet[1][2], Does.Not.Contain('\v')); + } + + [Test] + public void Writer_KeepsTabsAndNewlinesWhichAreLegal() + { + var service = new RawDataExcelService(null); + + using (var writer = service.CreateWriter(_file, Columns())) + { + writer.WriteRow(new Dictionary + { + ["id"] = 1, + ["q7"] = "line one\nline two", + }); + writer.Complete(); + } + + var sheet = ReadSheet(_file); + Assert.That(sheet[1][1], Does.Contain("line one")); + Assert.That(sheet[1][1], Does.Contain("line two")); + } + + [Test] + public void Writer_ProducesAnEmptyButValidWorkbookWhenNothingMatches() + { + var service = new RawDataExcelService(null); + + using (var writer = service.CreateWriter(_file, Columns())) + { + writer.Complete(); + } + + var sheet = ReadSheet(_file); + Assert.That(sheet.Count, Is.EqualTo(1), "Header only."); + } + + [Test] + public void Writer_DisposingWithoutCompletingDoesNotThrow() + { + var service = new RawDataExcelService(null); + + // The failure path: the service abandons the writer and deletes the + // partial file. Disposing must not mask the original exception. + Assert.DoesNotThrow(() => + { + using var writer = service.CreateWriter(_file, Columns()); + writer.WriteRow(new Dictionary { ["id"] = 1 }); + }); + } +} diff --git a/eFormAPI/Plugins/InsightDashboard.Pn/InsightDashboard.Pn/Controllers/RawDataController.cs b/eFormAPI/Plugins/InsightDashboard.Pn/InsightDashboard.Pn/Controllers/RawDataController.cs index e8a0442e..d0838cdf 100644 --- a/eFormAPI/Plugins/InsightDashboard.Pn/InsightDashboard.Pn/Controllers/RawDataController.cs +++ b/eFormAPI/Plugins/InsightDashboard.Pn/InsightDashboard.Pn/Controllers/RawDataController.cs @@ -24,37 +24,23 @@ OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE namespace InsightDashboard.Pn.Controllers; -using System; using System.IO; using System.Text; using System.Threading.Tasks; using Infrastructure.Models.RawData; using Microsoft.AspNetCore.Authorization; using Microsoft.AspNetCore.Mvc; -using Microsoft.Extensions.Logging; using Microting.eFormApi.BasePn.Infrastructure.Models.API; -using Services.Common.InsightDashboardLocalizationService; -using Services.RawDataExcelService; using Services.RawDataService; [Authorize] public class RawDataController : Controller { private readonly IRawDataService _rawDataService; - private readonly IRawDataExcelService _rawDataExcelService; - private readonly IInsightDashboardLocalizationService _localizationService; - private readonly ILogger _logger; - public RawDataController( - IRawDataService rawDataService, - IRawDataExcelService rawDataExcelService, - IInsightDashboardLocalizationService localizationService, - ILogger logger) + public RawDataController(IRawDataService rawDataService) { _rawDataService = rawDataService; - _rawDataExcelService = rawDataExcelService; - _localizationService = localizationService; - _logger = logger; } [HttpPost] @@ -75,37 +61,11 @@ public async Task> GetRawData( [ProducesResponseType(typeof(string), 400)] public async Task ExportRawData([FromQuery] RawDataExportRequestModel requestModel) { - var dataResult = await _rawDataService.GetAllRawData( + // The service writes the file in batches; we only stream and clean up. + var exportResult = await _rawDataService.ExportToFile( requestModel.DashboardId, requestModel.DashboardItemId); - string filePath = null; - if (dataResult.Success) - { - // Write before OnStarting registers, so a failure here can still be - // reported as a localized 400 rather than surfacing as a bare 500 with - // a half-written file orphaned in excel-storage. - try - { - filePath = _rawDataExcelService.CreateFilePath(); - if (!_rawDataExcelService.WriteRawDataToExcelFile(dataResult.Model, filePath)) - { - throw new Exception($"Error while writing excel file {filePath}"); - } - } - catch (Exception e) - { - _logger.LogError(e, e.Message); - - if (!string.IsNullOrEmpty(filePath) && System.IO.File.Exists(filePath)) - { - System.IO.File.Delete(filePath); - } - - filePath = null; - dataResult = new OperationDataResult( - false, _localizationService.GetString("ErrorWhileGeneratingRawDataExport")); - } - } + var filePath = exportResult.Success ? exportResult.Model : null; const int bufferSize = 4086; var buffer = new byte[bufferSize]; @@ -114,9 +74,9 @@ public async Task ExportRawData([FromQuery] RawDataExportRequestModel requestMod { try { - if (!dataResult.Success) + if (!exportResult.Success) { - var bytes = Encoding.UTF8.GetBytes(dataResult.Message); + var bytes = Encoding.UTF8.GetBytes(exportResult.Message); Response.ContentLength = bytes.Length; Response.ContentType = "text/plain"; Response.StatusCode = 400; diff --git a/eFormAPI/Plugins/InsightDashboard.Pn/InsightDashboard.Pn/Services/RawDataExcelService/IRawDataExcelService.cs b/eFormAPI/Plugins/InsightDashboard.Pn/InsightDashboard.Pn/Services/RawDataExcelService/IRawDataExcelService.cs index 52294435..c77ffead 100644 --- a/eFormAPI/Plugins/InsightDashboard.Pn/InsightDashboard.Pn/Services/RawDataExcelService/IRawDataExcelService.cs +++ b/eFormAPI/Plugins/InsightDashboard.Pn/InsightDashboard.Pn/Services/RawDataExcelService/IRawDataExcelService.cs @@ -24,11 +24,29 @@ OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE namespace InsightDashboard.Pn.Services.RawDataExcelService; +using System; +using System.Collections.Generic; using Infrastructure.Models.RawData; +/// +/// Writes rows to a sheet as they arrive, so the caller never has to hold the +/// whole export in memory. +/// +public interface IRawDataExcelWriter : IDisposable +{ + void WriteRow(Dictionary row); + + /// Closes the sheet and finalises the workbook. Must be called on success. + void Complete(); +} + public interface IRawDataExcelService { string CreateFilePath(); - bool WriteRawDataToExcelFile(RawDataListModel model, string destFile); + /// + /// Opens a workbook and writes the header row. The caller streams data rows + /// into the returned writer and calls Complete when done. + /// + IRawDataExcelWriter CreateWriter(string destFile, IReadOnlyList columns); } diff --git a/eFormAPI/Plugins/InsightDashboard.Pn/InsightDashboard.Pn/Services/RawDataExcelService/RawDataExcelService.cs b/eFormAPI/Plugins/InsightDashboard.Pn/InsightDashboard.Pn/Services/RawDataExcelService/RawDataExcelService.cs index bab8274c..8bb8fbde 100644 --- a/eFormAPI/Plugins/InsightDashboard.Pn/InsightDashboard.Pn/Services/RawDataExcelService/RawDataExcelService.cs +++ b/eFormAPI/Plugins/InsightDashboard.Pn/InsightDashboard.Pn/Services/RawDataExcelService/RawDataExcelService.cs @@ -26,7 +26,6 @@ namespace InsightDashboard.Pn.Services.RawDataExcelService; using System; using System.Collections.Generic; -using System.Globalization; using System.IO; using System.Security.Claims; using DocumentFormat.OpenXml; @@ -36,12 +35,6 @@ namespace InsightDashboard.Pn.Services.RawDataExcelService; using Microsoft.AspNetCore.Http; using Microting.eFormApi.BasePn.Infrastructure.Helpers; -/// -/// Writes the raw data table to xlsx. Unlike InterviewsExcelService this does not -/// copy a template first - that service creates a fresh SpreadsheetDocument over -/// the copied template anyway, so the copy is dead weight - and the column count -/// here is decided by the survey rather than a fixed enum. -/// public class RawDataExcelService(IHttpContextAccessor httpAccessor) : IRawDataExcelService { public string CreateFilePath() @@ -55,113 +48,197 @@ public string CreateFilePath() return Path.Combine(path, $"raw-data-{UserId}-{DateTime.UtcNow.Ticks}.xlsx"); } - public bool WriteRawDataToExcelFile(RawDataListModel model, string destFile) + public IRawDataExcelWriter CreateWriter(string destFile, IReadOnlyList columns) => + new RawDataExcelWriter(destFile, columns); + + private int UserId { - using var spreadsheetDocument = - SpreadsheetDocument.Create(destFile, SpreadsheetDocumentType.Workbook); + get + { + var value = httpAccessor?.HttpContext?.User?.FindFirstValue(ClaimTypes.NameIdentifier); + return value == null ? 0 : int.Parse(value); + } + } - var workbookPart = spreadsheetDocument.AddWorkbookPart(); - workbookPart.Workbook = new Workbook(); + /// + /// Streams rows with OpenXmlWriter rather than building a SheetData DOM. The + /// DOM approach held every cell of the export in memory at once, which is what + /// forced the row cap down to 25 000. + /// + private sealed class RawDataExcelWriter : IRawDataExcelWriter + { + private const string SheetName = "Raw data"; - var worksheetPart = workbookPart.AddNewPart(); - worksheetPart.Worksheet = new Worksheet(new SheetData()); + private readonly IReadOnlyList _columns; + private readonly SpreadsheetDocument _document; + private readonly WorkbookPart _workbookPart; + private readonly WorksheetPart _worksheetPart; + private readonly OpenXmlWriter _writer; - var sheets = spreadsheetDocument.WorkbookPart!.Workbook.AppendChild(new Sheets()); - sheets.Append(new Sheet + private uint _rowIndex = 1; + private bool _completed; + private bool _disposed; + + public RawDataExcelWriter(string destFile, IReadOnlyList columns) { - Id = spreadsheetDocument.WorkbookPart.GetIdOfPart(worksheetPart), - SheetId = 1, - Name = "Raw data", - }); - - var sheetData = worksheetPart.Worksheet.GetFirstChild(); - var columns = model.Columns; - - // Header row. Columns hidden in the UI are exported too - the export is - // the complete record. - var headerRow = new Row { RowIndex = 1U }; - for (var col = 0; col < columns.Count; col++) + _columns = columns; + _document = SpreadsheetDocument.Create(destFile, SpreadsheetDocumentType.Workbook); + _workbookPart = _document.AddWorkbookPart(); + _worksheetPart = _workbookPart.AddNewPart(); + + _writer = OpenXmlWriter.Create(_worksheetPart); + _writer.WriteStartElement(new Worksheet()); + _writer.WriteStartElement(new SheetData()); + + WriteHeader(); + } + + private void WriteHeader() { - headerRow.Append(new Cell + _writer.WriteStartElement(new Row { RowIndex = _rowIndex }); + + for (var col = 0; col < _columns.Count; col++) { - CellReference = GetCellReference(1, col + 1), - DataType = CellValues.String, - CellValue = new CellValue(columns[col].Header ?? string.Empty), - }); - } + WriteCell(GetCellReference(_rowIndex, col + 1), CellValues.String, _columns[col].Header ?? string.Empty); + } - sheetData!.Append(headerRow); + _writer.WriteEndElement(); + _rowIndex++; + } - var rowIndex = 2; - foreach (var modelRow in model.Rows) + public void WriteRow(Dictionary row) { - var row = new Row { RowIndex = (uint)rowIndex }; + _writer.WriteStartElement(new Row { RowIndex = _rowIndex }); - for (var col = 0; col < columns.Count; col++) + for (var col = 0; col < _columns.Count; col++) { - var value = modelRow.GetValueOrDefault(columns[col].Field); - if (value == null) + if (!row.TryGetValue(_columns[col].Field, out var value) || value == null) { continue; } - var cell = new Cell { CellReference = GetCellReference(rowIndex, col + 1) }; + var reference = GetCellReference(_rowIndex, col + 1); switch (value) { case DateTime dateTime: - cell.DataType = CellValues.String; - cell.CellValue = new CellValue( - dateTime.ToString("yyyy-MM-dd HH:mm:ss", CultureInfo.InvariantCulture)); + WriteCell(reference, CellValues.String, + dateTime.ToString("yyyy-MM-dd HH:mm:ss", System.Globalization.CultureInfo.InvariantCulture)); break; case int intValue: - cell.DataType = CellValues.Number; - cell.CellValue = new CellValue(intValue.ToString(CultureInfo.InvariantCulture)); + WriteCell(reference, CellValues.Number, + intValue.ToString(System.Globalization.CultureInfo.InvariantCulture)); break; case bool boolValue: - cell.DataType = CellValues.String; - cell.CellValue = new CellValue(boolValue ? "true" : "false"); + WriteCell(reference, CellValues.String, boolValue ? "true" : "false"); break; default: - cell.DataType = CellValues.String; - cell.CellValue = new CellValue(value.ToString() ?? string.Empty); + WriteCell(reference, CellValues.String, Sanitise(value.ToString())); break; } - - row.Append(cell); } - sheetData.Append(row); - rowIndex++; + _writer.WriteEndElement(); + _rowIndex++; } - workbookPart.Workbook.Save(); - return true; - } + private void WriteCell(string reference, CellValues type, string value) + { + _writer.WriteStartElement(new Cell { CellReference = reference, DataType = type }); + _writer.WriteElement(new CellValue(value)); + _writer.WriteEndElement(); + } - private int UserId - { - get + /// + /// Free-text answers can contain control characters that are illegal in + /// XML; left in, they produce a workbook Excel refuses to open. + /// + private static string Sanitise(string value) { - var value = httpAccessor?.HttpContext?.User?.FindFirstValue(ClaimTypes.NameIdentifier); - return value == null ? 0 : int.Parse(value); + if (string.IsNullOrEmpty(value)) + { + return value; + } + + Span buffer = value.Length <= 256 ? stackalloc char[value.Length] : new char[value.Length]; + var length = 0; + + foreach (var c in value) + { + if (c == '\t' || c == '\n' || c == '\r' || (c >= 0x20 && c != 0xFFFE && c != 0xFFFF)) + { + buffer[length++] = c; + } + } + + return length == value.Length ? value : new string(buffer[..length]); } - } - private static string GetCellReference(int rowIndex, int colIndex) => - $"{GetColumnName(colIndex)}{rowIndex}"; + public void Complete() + { + if (_completed) + { + return; + } + + _writer.WriteEndElement(); // SheetData + _writer.WriteEndElement(); // Worksheet + _writer.Close(); - private static string GetColumnName(int index) - { - var dividend = index; - var columnName = string.Empty; - while (dividend > 0) + _workbookPart.Workbook = new Workbook(); + var sheets = _workbookPart.Workbook.AppendChild(new Sheets()); + sheets.Append(new Sheet + { + Id = _workbookPart.GetIdOfPart(_worksheetPart), + SheetId = 1, + Name = SheetName, + }); + + _workbookPart.Workbook.Save(); + _completed = true; + } + + public void Dispose() { - var modulo = (dividend - 1) % 26; - columnName = Convert.ToChar(65 + modulo) + columnName; - dividend = (dividend - modulo) / 26; + if (_disposed) + { + return; + } + + _disposed = true; + + // On the failure path Complete was never called, so close the writer + // without finalising; the caller deletes the partial file. + if (!_completed) + { + try + { + _writer.Close(); + } + catch + { + // The writer may already be faulted; the file is discarded anyway. + } + } + + _document.Dispose(); } - return columnName; + private static string GetCellReference(uint rowIndex, int colIndex) => + $"{GetColumnName(colIndex)}{rowIndex}"; + + private static string GetColumnName(int index) + { + var dividend = index; + var columnName = string.Empty; + while (dividend > 0) + { + var modulo = (dividend - 1) % 26; + columnName = Convert.ToChar(65 + modulo) + columnName; + dividend = (dividend - modulo) / 26; + } + + return columnName; + } } } diff --git a/eFormAPI/Plugins/InsightDashboard.Pn/InsightDashboard.Pn/Services/RawDataService/IRawDataService.cs b/eFormAPI/Plugins/InsightDashboard.Pn/InsightDashboard.Pn/Services/RawDataService/IRawDataService.cs index 33344f33..31ee2562 100644 --- a/eFormAPI/Plugins/InsightDashboard.Pn/InsightDashboard.Pn/Services/RawDataService/IRawDataService.cs +++ b/eFormAPI/Plugins/InsightDashboard.Pn/InsightDashboard.Pn/Services/RawDataService/IRawDataService.cs @@ -32,5 +32,9 @@ public interface IRawDataService { Task> GetRawData(RawDataRequestModel requestModel); - Task> GetAllRawData(int dashboardId, int dashboardItemId); + /// + /// Writes the full, unpaged result to an xlsx file and returns its path. The + /// caller streams the file to the client and deletes it afterwards. + /// + Task> ExportToFile(int dashboardId, int dashboardItemId); } diff --git a/eFormAPI/Plugins/InsightDashboard.Pn/InsightDashboard.Pn/Services/RawDataService/RawDataService.cs b/eFormAPI/Plugins/InsightDashboard.Pn/InsightDashboard.Pn/Services/RawDataService/RawDataService.cs index 8555a97e..e0fd2feb 100644 --- a/eFormAPI/Plugins/InsightDashboard.Pn/InsightDashboard.Pn/Services/RawDataService/RawDataService.cs +++ b/eFormAPI/Plugins/InsightDashboard.Pn/InsightDashboard.Pn/Services/RawDataService/RawDataService.cs @@ -27,6 +27,7 @@ namespace InsightDashboard.Pn.Services.RawDataService; using System; using System.Collections.Generic; using System.Diagnostics; +using System.IO; using System.Linq; using System.Linq.Expressions; using System.Threading.Tasks; @@ -36,23 +37,30 @@ namespace InsightDashboard.Pn.Services.RawDataService; using Infrastructure.Models.RawData; using Microsoft.EntityFrameworkCore; using Microsoft.Extensions.Logging; +using Microting.eForm.Infrastructure; using Microting.eForm.Infrastructure.Constants; using Microting.eForm.Infrastructure.Data.Entities; using Microting.eFormApi.BasePn.Abstractions; using Microting.eFormApi.BasePn.Infrastructure.Models.API; using Microting.InsightDashboardBase.Infrastructure.Data; using Microting.InsightDashboardBase.Infrastructure.Data.Entities; +using RawDataExcelService; public class RawDataService : IRawDataService { /// - /// Hard ceiling for the unpaged export. Deliberately well below the point where - /// the process would struggle: the export materialises every answer id into an - /// IN(...) set, every AnswerValue for those answers, and one dictionary per row. - /// Batching the id lookup would let this rise; until then the cap must stay where - /// the whole set comfortably fits in memory. + /// Hard ceiling for the unpaged export. Memory is no longer the binding + /// constraint - answers are fetched a batch at a time and streamed straight + /// into the sheet - so this bounds file size and request duration rather than + /// protecting the heap. /// - public const int ExportRowLimit = 25000; + public const int ExportRowLimit = 250000; + + /// + /// Answers fetched per round trip during an export. Peak memory is roughly this + /// many rows plus their answer values, however large the export is. + /// + public const int ExportBatchSize = 2000; private const string NotAnswered = RawDataValueResolver.NotAnswered; @@ -61,247 +69,362 @@ public class RawDataService : IRawDataService private readonly IEFormCoreService _coreHelper; private readonly InsightDashboardPnDbContext _dbContext; private readonly IUserService _userService; + private readonly IRawDataExcelService _excelService; public RawDataService( ILogger logger, IInsightDashboardLocalizationService localizationService, IEFormCoreService coreHelper, InsightDashboardPnDbContext dbContext, - IUserService userService) + IUserService userService, + IRawDataExcelService excelService) { _logger = logger; _localizationService = localizationService; _coreHelper = coreHelper; _dbContext = dbContext; _userService = userService; + _excelService = excelService; } - public Task> GetRawData(RawDataRequestModel requestModel) => - Build(requestModel.DashboardId, requestModel.DashboardItemId, requestModel, applyPaging: true); - - public Task> GetAllRawData(int dashboardId, int dashboardItemId) => - Build(dashboardId, dashboardItemId, null, applyPaging: false); - - private async Task> Build( - int dashboardId, - int dashboardItemId, - RawDataRequestModel requestModel, - bool applyPaging) + public async Task> GetRawData(RawDataRequestModel requestModel) { try { - var dashboard = await _dbContext.Dashboards - .Include(x => x.DashboardItems) - .ThenInclude>(x => x.IgnoredAnswerValues) - .Include(x => x.DashboardItems) - .ThenInclude>(x => x.CompareLocationsTags) - .Where(x => x.WorkflowState != Constants.WorkflowStates.Removed) - .FirstOrDefaultAsync(x => x.Id == dashboardId); - - if (dashboard == null) - { - return new OperationDataResult( - false, _localizationService.GetString("DashboardNotFound")); - } - - var dashboardItem = dashboard.DashboardItems - .Where(x => x.WorkflowState != Constants.WorkflowStates.Removed) - .FirstOrDefault(x => x.Id == dashboardItemId); + var core = await _coreHelper.GetCore(); + await using var sdkContext = core.DbContextHelper.GetDbContext(); - if (dashboardItem == null) - { - return new OperationDataResult( - false, _localizationService.GetString("DashboardItemNotFound")); - } + var scope = await ResolveScope( + sdkContext, requestModel.DashboardId, requestModel.DashboardItemId); - if (dashboard.Today) + if (!scope.Success) { - var dateTimeNow = DateTime.Now; - dashboard.DateTo = new DateTime( - dateTimeNow.Year, dateTimeNow.Month, dateTimeNow.Day, 23, 59, 59); + return new OperationDataResult(false, scope.Message); } - var answerDates = new DashboardEditAnswerDates + var result = new RawDataListModel { - Today = dashboard.Today, - DateFrom = dashboard.DateFrom, - DateTo = dashboard.DateTo, + Columns = scope.Model.Schema.Columns, + Total = await scope.Model.Answers.CountAsync(), }; - var core = await _coreHelper.GetCore(); - var userLanguage = await _userService.GetCurrentUserLanguage(); + result.Rows.AddRange(await BuildRows( + sdkContext, + scope.Model, + requestModel.Sort, + requestModel.IsSortDsc, + requestModel.Offset, + requestModel.PageSize)); - await using var sdkContext = core.DbContextHelper.GetDbContext(); + return new OperationDataResult(true, result); + } + catch (Exception e) + { + Trace.TraceError(e.Message); + _logger.LogError(e, e.Message); + return new OperationDataResult( + false, _localizationService.GetString("ErrorWhileObtainingRawData")); + } + } - // Text items render the interviews grid, not a chart, and ChartDataHelpers - // filters them down a different branch (it applies the location filter - // regardless of CompareEnabled). AnswerFilterHelper does not mirror that - // branch, so refuse rather than return a set that matches nothing on screen. - var firstQuestionType = await sdkContext.Questions - .AsNoTracking() - .Where(x => x.Id == dashboardItem.FirstQuestionId) - .Select(x => x.QuestionType) - .FirstOrDefaultAsync(); - - if (firstQuestionType == Constants.QuestionTypes.Text) - { - return new OperationDataResult( - false, _localizationService.GetString("RawDataNotAvailableForTextQuestions")); - } + public async Task> ExportToFile(int dashboardId, int dashboardItemId) + { + string filePath = null; - var preferredLanguageIds = await RawDataTranslations.GetPreferredLanguageIdsAsync( - sdkContext, dashboard.SurveyId, userLanguage.Id); + try + { + var core = await _coreHelper.GetCore(); - var schema = await RawDataColumnBuilder.BuildAsync( - sdkContext, dashboard.SurveyId, preferredLanguageIds); + // The context stays open for the whole export: every batch comes from + // the same query, so it has to outlive the loop. + await using var sdkContext = core.DbContextHelper.GetDbContext(); - var answerQuery = AnswerFilterHelper.BuildAnswerQuery( - sdkContext, - dashboardItem, - dashboard.SurveyId, - dashboard.LocationId, - dashboard.TagId, - answerDates); + var scope = await ResolveScope(sdkContext, dashboardId, dashboardItemId); - var result = new RawDataListModel + if (!scope.Success) { - Columns = schema.Columns, - Total = await answerQuery.CountAsync(), - }; + return new OperationDataResult(false, scope.Message); + } - if (!applyPaging && result.Total > ExportRowLimit) + var total = await scope.Model.Answers.CountAsync(); + + if (total > ExportRowLimit) { - return new OperationDataResult( + return new OperationDataResult( false, string.Format( _localizationService.GetString("RawDataExportTooLarge"), - result.Total, + total, ExportRowLimit)); } - var ordered = ApplySort(answerQuery, requestModel?.Sort, requestModel?.IsSortDsc ?? true); + filePath = _excelService.CreateFilePath(); - if (applyPaging) + using (var writer = _excelService.CreateWriter(filePath, scope.Model.Schema.Columns)) { - ordered = ordered.Skip(requestModel.Offset).Take(requestModel.PageSize); + for (var offset = 0; offset < total; offset += ExportBatchSize) + { + var rows = await BuildRows( + sdkContext, + scope.Model, + RawDataFields.FinishedAt, + isSortDsc: true, + offset, + ExportBatchSize); + + // Answers removed mid-export would otherwise leave the loop + // spinning to the original total. + if (rows.Count == 0) + { + break; + } + + foreach (var row in rows) + { + writer.WriteRow(row); + } + } + + writer.Complete(); } - var answers = await ordered - .Select(x => new AnswerRow - { - Id = x.Id, - MicrotingUid = x.MicrotingUid, - FinishedAt = x.FinishedAt, - AnswerDuration = x.AnswerDuration, - SiteId = x.SiteId, - SiteName = x.Site.Name, - TagNames = x.Site.SiteTags - .Where(y => y.WorkflowState != Constants.WorkflowStates.Removed) - .Select(y => y.Tag.Name) - .ToList(), - UnitId = x.UnitId, - UnitMicrotingUid = x.Unit.MicrotingUid, - LanguageId = x.LanguageId, - LanguageName = x.Language.Name, - SurveyConfigurationName = x.SurveyConfiguration.Name, - QuestionSetName = x.QuestionSet.Name, - TimeZone = x.TimeZone, - UtcAdjusted = x.UtcAdjusted, - CreatedAt = x.CreatedAt, - UpdatedAt = x.UpdatedAt, - Version = x.Version, - WorkflowState = x.WorkflowState, - }) - .ToListAsync(); - - var answerIds = answers.Select(x => x.Id).ToList(); - - var values = await sdkContext.AnswerValues - .AsNoTracking() - .Where(x => x.WorkflowState != Constants.WorkflowStates.Removed) - .Where(x => answerIds.Contains(x.AnswerId)) - .Select(x => new { x.AnswerId, x.QuestionId, x.OptionId, x.Value }) - .ToListAsync(); - - var metaByQuestionId = schema.Questions.ToDictionary(x => x.QuestionId); - var valuesByAnswerId = values - .GroupBy(x => x.AnswerId) - .ToDictionary(x => x.Key, x => x.ToList()); - - foreach (var answer in answers) + return new OperationDataResult(true, filePath); + } + catch (Exception e) + { + if (!string.IsNullOrEmpty(filePath) && File.Exists(filePath)) + { + File.Delete(filePath); + } + + Trace.TraceError(e.Message); + _logger.LogError(e, e.Message); + return new OperationDataResult( + false, _localizationService.GetString("ErrorWhileGeneratingRawDataExport")); + } + } + + /// + /// Everything a page or an export needs that does not depend on the offset: the + /// item's column schema and the query selecting its answers. + /// + private async Task> ResolveScope( + MicrotingDbContext sdkContext, + int dashboardId, + int dashboardItemId) + { + var dashboard = await _dbContext.Dashboards + .Include(x => x.DashboardItems) + .ThenInclude>(x => x.IgnoredAnswerValues) + .Include(x => x.DashboardItems) + .ThenInclude>(x => x.CompareLocationsTags) + .Where(x => x.WorkflowState != Constants.WorkflowStates.Removed) + .FirstOrDefaultAsync(x => x.Id == dashboardId); + + if (dashboard == null) + { + return new OperationDataResult( + false, _localizationService.GetString("DashboardNotFound")); + } + + var dashboardItem = dashboard.DashboardItems + .Where(x => x.WorkflowState != Constants.WorkflowStates.Removed) + .FirstOrDefault(x => x.Id == dashboardItemId); + + if (dashboardItem == null) + { + return new OperationDataResult( + false, _localizationService.GetString("DashboardItemNotFound")); + } + + if (dashboard.Today) + { + var dateTimeNow = DateTime.Now; + dashboard.DateTo = new DateTime( + dateTimeNow.Year, dateTimeNow.Month, dateTimeNow.Day, 23, 59, 59); + } + + var answerDates = new DashboardEditAnswerDates + { + Today = dashboard.Today, + DateFrom = dashboard.DateFrom, + DateTo = dashboard.DateTo, + }; + + // Text items render the interviews grid, not a chart, and ChartDataHelpers + // filters them down a different branch that AnswerFilterHelper does not + // mirror. Refuse rather than return a set matching nothing on screen. + var firstQuestionType = await sdkContext.Questions + .AsNoTracking() + .Where(x => x.Id == dashboardItem.FirstQuestionId) + .Select(x => x.QuestionType) + .FirstOrDefaultAsync(); + + if (firstQuestionType == Constants.QuestionTypes.Text) + { + return new OperationDataResult( + false, _localizationService.GetString("RawDataNotAvailableForTextQuestions")); + } + + var userLanguage = await _userService.GetCurrentUserLanguage(); + + var preferredLanguageIds = await RawDataTranslations.GetPreferredLanguageIdsAsync( + sdkContext, dashboard.SurveyId, userLanguage.Id); + + var schema = await RawDataColumnBuilder.BuildAsync( + sdkContext, dashboard.SurveyId, preferredLanguageIds); + + var answerQuery = AnswerFilterHelper.BuildAnswerQuery( + sdkContext, + dashboardItem, + dashboard.SurveyId, + dashboard.LocationId, + dashboard.TagId, + answerDates); + + return new OperationDataResult(true, new RawDataScope + { + Schema = schema, + Answers = answerQuery, + }); + } + + /// + /// Materialises one window of answers and pivots their answer values into rows. + /// Peak memory is bounded by pageSize, which is what makes a large export safe. + /// + private static async Task>> BuildRows( + MicrotingDbContext sdkContext, + RawDataScope scope, + string sort, + bool isSortDsc, + int offset, + int pageSize) + { + var answers = await ApplySort(scope.Answers, sort, isSortDsc) + .Skip(offset) + .Take(pageSize) + .Select(x => new AnswerRow { - var row = ToRowDictionary(answer); + Id = x.Id, + MicrotingUid = x.MicrotingUid, + FinishedAt = x.FinishedAt, + AnswerDuration = x.AnswerDuration, + SiteId = x.SiteId, + SiteName = x.Site.Name, + TagNames = x.Site.SiteTags + .Where(y => y.WorkflowState != Constants.WorkflowStates.Removed) + .Select(y => y.Tag.Name) + .ToList(), + UnitId = x.UnitId, + UnitMicrotingUid = x.Unit.MicrotingUid, + LanguageId = x.LanguageId, + LanguageName = x.Language.Name, + SurveyConfigurationName = x.SurveyConfiguration.Name, + QuestionSetName = x.QuestionSet.Name, + TimeZone = x.TimeZone, + UtcAdjusted = x.UtcAdjusted, + CreatedAt = x.CreatedAt, + UpdatedAt = x.UpdatedAt, + Version = x.Version, + WorkflowState = x.WorkflowState, + }) + .ToListAsync(); + + var rows = new List>(answers.Count); + + if (answers.Count == 0) + { + return rows; + } + + var answerIds = answers.Select(x => x.Id).ToList(); + + var values = await sdkContext.AnswerValues + .AsNoTracking() + .Where(x => x.WorkflowState != Constants.WorkflowStates.Removed) + .Where(x => answerIds.Contains(x.AnswerId)) + .Select(x => new { x.AnswerId, x.QuestionId, x.OptionId, x.Value }) + .ToListAsync(); + + var metaByQuestionId = scope.Schema.Questions.ToDictionary(x => x.QuestionId); + var valuesByAnswerId = values + .GroupBy(x => x.AnswerId) + .ToDictionary(x => x.Key, x => x.ToList()); - // Every question column starts as "not answered"; real values overwrite it. - foreach (var meta in schema.Questions) + foreach (var answer in answers) + { + var row = ToRowDictionary(answer); + + // Every question column starts as "not answered"; real values overwrite it. + foreach (var meta in scope.Schema.Questions) + { + foreach (var field in meta.OptionFields) { - foreach (var field in meta.OptionFields) - { - row[field] = NotAnswered; - } + row[field] = NotAnswered; } + } - if (valuesByAnswerId.TryGetValue(answer.Id, out var answerValues)) + if (valuesByAnswerId.TryGetValue(answer.Id, out var answerValues)) + { + foreach (var answerValue in answerValues) { - foreach (var answerValue in answerValues) + if (!metaByQuestionId.TryGetValue(answerValue.QuestionId, out var meta)) { - if (!metaByQuestionId.TryGetValue(answerValue.QuestionId, out var meta)) + continue; + } + + var optionName = meta.OptionNameByOptionId.GetValueOrDefault(answerValue.OptionId); + var skipped = RawDataValueResolver.IsSkipped(optionName); + + if (meta.IsMulti) + { + // A skipped multi question leaves every option column as "not answered". + if (skipped) { continue; } - var optionName = meta.OptionNameByOptionId.GetValueOrDefault(answerValue.OptionId); - var skipped = RawDataValueResolver.IsSkipped(optionName); - - if (meta.IsMulti) + // The option was removed from the survey after this answer was + // given, so it has no column. Leave the question untouched rather + // than blanking its columns, which would assert the respondent + // was offered these options and picked none. + var optionField = meta.OptionFieldByOptionId.GetValueOrDefault(answerValue.OptionId); + if (optionField == null) { - // A skipped multi question leaves every option column as "not answered". - if (skipped) - { - continue; - } - - // The option was removed from the survey after this answer was - // given, so it has no column. Leave the question untouched rather - // than blanking its columns, which would assert the respondent - // was offered these options and picked none. - var optionField = meta.OptionFieldByOptionId.GetValueOrDefault(answerValue.OptionId); - if (optionField == null) - { - continue; - } + continue; + } - foreach (var field in meta.OptionFields) + foreach (var field in meta.OptionFields) + { + if (Equals(row[field], NotAnswered)) { - if (Equals(row[field], NotAnswered)) - { - row[field] = string.Empty; - } + row[field] = string.Empty; } - - row[optionField] = optionName; - continue; } - row[meta.Field] = skipped - ? NotAnswered - : RawDataValueResolver.ResolveSingleValue( - meta, answerValue.OptionId, answerValue.Value, optionName); + row[optionField] = optionName; + continue; } - } - result.Rows.Add(row); + row[meta.Field] = skipped + ? NotAnswered + : RawDataValueResolver.ResolveSingleValue( + meta, answerValue.OptionId, answerValue.Value, optionName); + } } - return new OperationDataResult(true, result); - } - catch (Exception e) - { - Trace.TraceError(e.Message); - _logger.LogError(e, e.Message); - return new OperationDataResult( - false, _localizationService.GetString("ErrorWhileObtainingRawData")); + rows.Add(row); } + + return rows; + } + + private sealed class RawDataScope + { + public RawDataSchema Schema { get; init; } + + public IQueryable Answers { get; init; } } private static Dictionary ToRowDictionary(AnswerRow answer) => new() From df325b3e717738e1fd34029037f6f669281ce4fe Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ren=C3=A9=20Schultz=20Madsen?= Date: Wed, 29 Jul 2026 16:16:33 +0200 Subject: [PATCH 2/3] perf: seek instead of offset when batching the export Code review found the batching traded one scaling problem for a worse one. Each batch was Skip(offset).Take(2000) against a query whose WHERE is an IN subquery - a Union over AnswerValues for compared items - so MySQL re-evaluated that subquery, re-sorted the whole matching set and discarded offset rows on every batch. Quadratic where the original was linear, and the cap had just been raised tenfold, multiplying it by a hundred. The comment claiming the cap now bounded request duration had it exactly backwards. RawDataPaging replaces offsets with keyset paging on (FinishedAt, Id), the total order the tie-breaker already guaranteed. That makes the export linear and index-seekable, and fixes a correctness problem in the same move: with OFFSET, an answer finished mid-export sorts to the front and shifts every later window, duplicating rows across batch boundaries and dropping others. A cursor is anchored to a value, so new answers sort above it and are simply not seen - a consistent view as of the export's start. Also from review: - ExportRowLimit drops 250 000 -> 50 000. The honest reason is in the comment: the whole file is still generated before the response starts, so wall-clock against a proxy timeout is the real constraint, and this number is not derived from measuring a cap-sized export. Streaming to the response body would remove the ceiling properly. - A truncated export no longer passes as complete: rows written are compared against the expected total and a mismatch is logged. - Column headers are sanitised. They come from question text and option translations - the same user-entered tables as the answers - so an unsanitised header would corrupt every export of that survey rather than one unlucky row. - Restored the null guard on value.ToString(), wrapped the document dispose so a close failure cannot replace the exception being unwound, and corrected two comments that still justified the old cap. - ExportBatchSize becomes a settable property so tests can cross batch boundaries without seeding thousands of answers. The batching itself was untested, which was the review's main point about coverage. Two tests now page through the seeded answers in batches of three and assert the stitched result equals a single ordered read exactly once, including through a timestamp collision - the case that has no defined order without the Id tie-breaker. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01EnP42zmHAgo2NZQsa6zdhG --- .../AnswerFilterHelperUTests.cs | 106 +++++++++++++++++ .../Infrastructure/Helpers/RawDataPaging.cs | 73 ++++++++++++ .../RawDataExcelService.cs | 30 +++-- .../Services/RawDataService/RawDataService.cs | 109 ++++++++++++------ 4 files changed, 278 insertions(+), 40 deletions(-) create mode 100644 eFormAPI/Plugins/InsightDashboard.Pn/InsightDashboard.Pn/Infrastructure/Helpers/RawDataPaging.cs diff --git a/eFormAPI/Plugins/InsightDashboard.Pn/InsightDashboard.Pn.Test/AnswerFilterHelperUTests.cs b/eFormAPI/Plugins/InsightDashboard.Pn/InsightDashboard.Pn.Test/AnswerFilterHelperUTests.cs index 75110162..28c7885b 100644 --- a/eFormAPI/Plugins/InsightDashboard.Pn/InsightDashboard.Pn.Test/AnswerFilterHelperUTests.cs +++ b/eFormAPI/Plugins/InsightDashboard.Pn/InsightDashboard.Pn.Test/AnswerFilterHelperUTests.cs @@ -24,6 +24,7 @@ OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE namespace InsightDashboard.Pn.Test; +using System; using System.Collections.Generic; using System.Linq; using System.Threading.Tasks; @@ -270,4 +271,109 @@ public async Task LocationFilter_PrefersSiteOverTag() "A location must win over a tag rather than intersecting with it."); } } + + /// + /// The batching the export depends on: seeking through answers in small windows + /// must yield every answer exactly once, in the same order a single unbatched + /// read would. This is what Skip/Take could not guarantee cheaply, and it is the + /// whole point of the keyset rewrite. + /// + [Test] + public async Task KeysetPaging_StitchesBatchesIntoTheFullSetExactlyOnce() + { + var answers = DbContext.Answers + .AsNoTracking() + .Where(x => x.WorkflowState != Constants.WorkflowStates.Removed); + + var expected = await RawDataPaging.OrderNewestFirst(answers) + .Select(x => x.Id) + .ToListAsync(); + + Assert.That(expected, Is.Not.Empty, "Seed data has no answers to page through."); + + const int batchSize = 3; + var stitched = new List(); + DateTime? cursorFinishedAt = null; + int? cursorId = null; + var guard = 0; + + while (true) + { + var batch = await RawDataPaging + .AfterCursor(answers, cursorFinishedAt, cursorId) + .Take(batchSize) + .Select(x => new { x.Id, x.FinishedAt }) + .ToListAsync(); + + if (batch.Count == 0) + { + break; + } + + stitched.AddRange(batch.Select(x => x.Id)); + cursorFinishedAt = batch[^1].FinishedAt; + cursorId = batch[^1].Id; + + Assert.That(++guard, Is.LessThan(expected.Count + 10), + "The cursor stopped advancing - paging would loop forever."); + } + + Assert.That(stitched, Is.EqualTo(expected), + "Stitched batches must equal a single ordered read, in order."); + Assert.That(stitched.Distinct().Count(), Is.EqualTo(stitched.Count), + "No answer may appear in two batches."); + } + + /// + /// Answers sharing a FinishedAt are exactly where paging breaks without a total + /// order, so they must not straddle a batch boundary incorrectly. + /// + [Test] + public async Task KeysetPaging_HandlesAnswersSharingATimestamp() + { + var duplicated = await DbContext.Answers + .AsNoTracking() + .Where(x => x.WorkflowState != Constants.WorkflowStates.Removed) + .GroupBy(x => x.FinishedAt) + .Where(g => g.Count() > 1) + .Select(g => g.Key) + .FirstOrDefaultAsync(); + + if (duplicated == default) + { + Assert.Ignore("Seed data has no answers sharing a FinishedAt."); + } + + var answers = DbContext.Answers + .AsNoTracking() + .Where(x => x.WorkflowState != Constants.WorkflowStates.Removed) + .Where(x => x.FinishedAt == duplicated); + + var expected = await RawDataPaging.OrderNewestFirst(answers).Select(x => x.Id).ToListAsync(); + + var stitched = new List(); + DateTime? cursorFinishedAt = null; + int? cursorId = null; + + while (true) + { + var batch = await RawDataPaging + .AfterCursor(answers, cursorFinishedAt, cursorId) + .Take(1) + .Select(x => new { x.Id, x.FinishedAt }) + .ToListAsync(); + + if (batch.Count == 0) + { + break; + } + + stitched.Add(batch[0].Id); + cursorFinishedAt = batch[0].FinishedAt; + cursorId = batch[0].Id; + } + + Assert.That(stitched, Is.EqualTo(expected), + "One-row batches through a timestamp collision must still cover it exactly once."); + } } diff --git a/eFormAPI/Plugins/InsightDashboard.Pn/InsightDashboard.Pn/Infrastructure/Helpers/RawDataPaging.cs b/eFormAPI/Plugins/InsightDashboard.Pn/InsightDashboard.Pn/Infrastructure/Helpers/RawDataPaging.cs new file mode 100644 index 00000000..501a0a3f --- /dev/null +++ b/eFormAPI/Plugins/InsightDashboard.Pn/InsightDashboard.Pn/Infrastructure/Helpers/RawDataPaging.cs @@ -0,0 +1,73 @@ +/* +The MIT License (MIT) + +Copyright (c) 2007 - 2021 Microting A/S + +Permission is hereby granted, free of charge, to any person obtaining a copy +of this software and associated documentation files (the "Software"), to deal +in the Software without restriction, including without limitation the rights +to use, copy, modify, merge, publish, distribute, sublicense, and/or sell +copies of the Software, and to permit persons to whom the Software is +furnished to do so, subject to the following conditions: + +The above copyright notice and this permission notice shall be included in all +copies or substantial portions of the Software. + +THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR +IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, +FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE +AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER +LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, +OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE +SOFTWARE. +*/ + +namespace InsightDashboard.Pn.Infrastructure.Helpers; + +using System; +using System.Linq; +using Microting.eForm.Infrastructure.Data.Entities; + +/// +/// Keyset ("seek") paging over answers, ordered newest first. +/// +/// The export cannot use Skip/Take. Its answer query filters on an IN subquery - +/// for compared items a Union over AnswerValues - so every OFFSET batch would make +/// the database re-evaluate that subquery, re-sort the whole matching set and throw +/// away the rows it had already returned. That is quadratic in the number of rows, +/// which is worse than the memory problem batching set out to solve. +/// +/// Seeking on (FinishedAt, Id) instead is linear and index-friendly. It also pins +/// the export to a consistent view: answers finished while the export runs sort +/// above the cursor and are simply never seen, whereas with OFFSET they would shift +/// every later window and duplicate rows across batch boundaries. +/// +public static class RawDataPaging +{ + /// + /// Orders newest first. Id breaks ties so the order is total - without it, + /// answers sharing a timestamp have no defined position and a cursor cannot + /// resume reliably. + /// + public static IOrderedQueryable OrderNewestFirst(IQueryable answers) => + answers.OrderByDescending(x => x.FinishedAt).ThenByDescending(x => x.Id); + + /// + /// Restricts to answers strictly after the cursor in that order. A null cursor + /// means the first batch. + /// + public static IQueryable AfterCursor( + IQueryable answers, + DateTime? lastFinishedAt, + int? lastId) + { + if (lastFinishedAt == null || lastId == null) + { + return OrderNewestFirst(answers); + } + + return OrderNewestFirst(answers.Where(x => + x.FinishedAt < lastFinishedAt + || (x.FinishedAt == lastFinishedAt && x.Id < lastId))); + } +} diff --git a/eFormAPI/Plugins/InsightDashboard.Pn/InsightDashboard.Pn/Services/RawDataExcelService/RawDataExcelService.cs b/eFormAPI/Plugins/InsightDashboard.Pn/InsightDashboard.Pn/Services/RawDataExcelService/RawDataExcelService.cs index 8bb8fbde..ca1f1267 100644 --- a/eFormAPI/Plugins/InsightDashboard.Pn/InsightDashboard.Pn/Services/RawDataExcelService/RawDataExcelService.cs +++ b/eFormAPI/Plugins/InsightDashboard.Pn/InsightDashboard.Pn/Services/RawDataExcelService/RawDataExcelService.cs @@ -62,8 +62,8 @@ private int UserId /// /// Streams rows with OpenXmlWriter rather than building a SheetData DOM. The - /// DOM approach held every cell of the export in memory at once, which is what - /// forced the row cap down to 25 000. + /// DOM approach held every cell of the export in memory at once; this holds one + /// row, so the size of an export is no longer a memory question. /// private sealed class RawDataExcelWriter : IRawDataExcelWriter { @@ -99,7 +99,10 @@ private void WriteHeader() for (var col = 0; col < _columns.Count; col++) { - WriteCell(GetCellReference(_rowIndex, col + 1), CellValues.String, _columns[col].Header ?? string.Empty); + WriteCell( + GetCellReference(_rowIndex, col + 1), + CellValues.String, + Sanitise(_columns[col].Header) ?? string.Empty); } _writer.WriteEndElement(); @@ -133,7 +136,7 @@ public void WriteRow(Dictionary row) WriteCell(reference, CellValues.String, boolValue ? "true" : "false"); break; default: - WriteCell(reference, CellValues.String, Sanitise(value.ToString())); + WriteCell(reference, CellValues.String, Sanitise(value.ToString()) ?? string.Empty); break; } } @@ -150,8 +153,13 @@ private void WriteCell(string reference, CellValues type, string value) } /// - /// Free-text answers can contain control characters that are illegal in - /// XML; left in, they produce a workbook Excel refuses to open. + /// Strips characters XML 1.0 forbids: C0 controls except tab, newline and + /// carriage return, plus the 0xFFFE/0xFFFF non-characters. 0x7F and the C1 + /// range are legal in XML 1.0 and are kept deliberately. + /// + /// Well-formed surrogate pairs pass through intact. A lone surrogate would + /// not, but MySQL's utf8mb4 validation means one cannot be stored in the + /// first place. /// private static string Sanitise(string value) { @@ -221,7 +229,15 @@ public void Dispose() } } - _document.Dispose(); + try + { + _document.Dispose(); + } + catch + { + // Never let a close failure replace the exception being unwound; + // the caller deletes the file either way. + } } private static string GetCellReference(uint rowIndex, int colIndex) => diff --git a/eFormAPI/Plugins/InsightDashboard.Pn/InsightDashboard.Pn/Services/RawDataService/RawDataService.cs b/eFormAPI/Plugins/InsightDashboard.Pn/InsightDashboard.Pn/Services/RawDataService/RawDataService.cs index e0fd2feb..ea89ceeb 100644 --- a/eFormAPI/Plugins/InsightDashboard.Pn/InsightDashboard.Pn/Services/RawDataService/RawDataService.cs +++ b/eFormAPI/Plugins/InsightDashboard.Pn/InsightDashboard.Pn/Services/RawDataService/RawDataService.cs @@ -49,18 +49,28 @@ namespace InsightDashboard.Pn.Services.RawDataService; public class RawDataService : IRawDataService { /// - /// Hard ceiling for the unpaged export. Memory is no longer the binding - /// constraint - answers are fetched a batch at a time and streamed straight - /// into the sheet - so this bounds file size and request duration rather than - /// protecting the heap. + /// Hard ceiling for the unpaged export. + /// + /// Batching removed the memory constraint and keyset paging removed the + /// quadratic database cost, so this is no longer protecting the heap. What it + /// still bounds is wall-clock time: the whole file is generated before the + /// response starts, so a slow export can outlive a reverse proxy's read timeout + /// (nginx defaults to 60s). This value is NOT derived from a measurement of a + /// cap-sized export in production - raising it further should be, or better, + /// the sheet should be written straight to the response body so bytes flow + /// while rows are generated and no ceiling is needed. /// - public const int ExportRowLimit = 250000; + public const int ExportRowLimit = 50000; + + /// Default answers per round trip during an export. + public const int DefaultExportBatchSize = 2000; /// - /// Answers fetched per round trip during an export. Peak memory is roughly this - /// many rows plus their answer values, however large the export is. + /// Answers fetched per round trip. Peak memory is roughly this many rows plus + /// their answer values, however large the export is. Settable so tests can + /// cross batch boundaries without seeding thousands of answers. /// - public const int ExportBatchSize = 2000; + public int ExportBatchSize { get; set; } = DefaultExportBatchSize; private const string NotAnswered = RawDataValueResolver.NotAnswered; @@ -108,13 +118,14 @@ public async Task> GetRawData(RawDataReque Total = await scope.Model.Answers.CountAsync(), }; - result.Rows.AddRange(await BuildRows( - sdkContext, - scope.Model, + var page = await ReadAnswerPage( + scope.Model.Answers, requestModel.Sort, requestModel.IsSortDsc, requestModel.Offset, - requestModel.PageSize)); + requestModel.PageSize); + + result.Rows.AddRange(await BuildRows(sdkContext, scope.Model, page)); return new OperationDataResult(true, result); } @@ -160,34 +171,49 @@ public async Task> ExportToFile(int dashboardId, int filePath = _excelService.CreateFilePath(); + var written = 0; + using (var writer = _excelService.CreateWriter(filePath, scope.Model.Schema.Columns)) { - for (var offset = 0; offset < total; offset += ExportBatchSize) + DateTime? cursorFinishedAt = null; + int? cursorId = null; + + while (true) { - var rows = await BuildRows( - sdkContext, - scope.Model, - RawDataFields.FinishedAt, - isSortDsc: true, - offset, - ExportBatchSize); - - // Answers removed mid-export would otherwise leave the loop - // spinning to the original total. - if (rows.Count == 0) + var answers = await ReadAnswerBatch( + scope.Model.Answers, cursorFinishedAt, cursorId, ExportBatchSize); + + if (answers.Count == 0) { break; } - foreach (var row in rows) + foreach (var row in await BuildRows(sdkContext, scope.Model, answers)) { writer.WriteRow(row); + written++; } + + var last = answers[^1]; + cursorFinishedAt = last.FinishedAt; + cursorId = last.Id; } writer.Complete(); } + if (written != total) + { + // Answers added or removed while the export ran. The cursor pins us + // to a consistent view, so the file is coherent - but it is not the + // count the user was shown, and saying nothing would let a truncated + // export pass as complete. + _logger.LogWarning( + "Raw data export for dashboard {DashboardId} item {ItemId} wrote {Written} rows " + + "against an expected {Total}; answers changed while the export ran.", + dashboardId, dashboardItemId, written, total); + } + return new OperationDataResult(true, filePath); } catch (Exception e) @@ -293,17 +319,25 @@ private async Task> ResolveScope( /// Materialises one window of answers and pivots their answer values into rows. /// Peak memory is bounded by pageSize, which is what makes a large export safe. /// - private static async Task>> BuildRows( - MicrotingDbContext sdkContext, - RawDataScope scope, + /// Reads one keyset window of answers, newest first. + private static Task> ReadAnswerBatch( + IQueryable answers, + DateTime? cursorFinishedAt, + int? cursorId, + int batchSize) => + ProjectAnswers(RawDataPaging.AfterCursor(answers, cursorFinishedAt, cursorId).Take(batchSize)); + + /// Reads one offset window, for the paged endpoint. + private static Task> ReadAnswerPage( + IQueryable answers, string sort, bool isSortDsc, int offset, - int pageSize) - { - var answers = await ApplySort(scope.Answers, sort, isSortDsc) - .Skip(offset) - .Take(pageSize) + int pageSize) => + ProjectAnswers(ApplySort(answers, sort, isSortDsc).Skip(offset).Take(pageSize)); + + private static Task> ProjectAnswers(IQueryable answers) => + answers .Select(x => new AnswerRow { Id = x.Id, @@ -331,6 +365,15 @@ private static async Task>> BuildRows( }) .ToListAsync(); + /// + /// Pivots one window of answers and their answer values into rows. Peak memory + /// is bounded by the window, which is what makes a large export safe. + /// + private static async Task>> BuildRows( + MicrotingDbContext sdkContext, + RawDataScope scope, + List answers) + { var rows = new List>(answers.Count); if (answers.Count == 0) From ff829d59735f0c8dd0d9acd49b2bccdb78c3eb26 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ren=C3=A9=20Schultz=20Madsen?= Date: Wed, 29 Jul 2026 16:34:07 +0200 Subject: [PATCH 3/3] test: cover the timestamp collision the seed data cannot CI reported 18 passed, 1 skipped, and the skip was KeysetPaging_HandlesAnswersSharingATimestamp: the seeded database holds no two answers sharing a FinishedAt. That left the timestamp-collision case unverified - the exact situation the Id tie-breaker exists for, and the one where paging silently duplicates and drops rows if it is wrong. Two in-memory tests pin the predicate where the collision can actually be constructed: three answers on one timestamp plus one on another, walked a single row at a time so every step crosses the collision, and a no-cursor case asserting the first batch starts at the newest answer. These complement rather than replace the database test. That one proves the expression translates to SQL; these prove it is correct when timestamps tie. Both run without a database, so both were verified locally, and the tie-break test was checked non-vacuous by removing the Id comparison and watching it fail. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01EnP42zmHAgo2NZQsa6zdhG --- .../AnswerFilterHelperUTests.cs | 66 +++++++++++++++++++ 1 file changed, 66 insertions(+) diff --git a/eFormAPI/Plugins/InsightDashboard.Pn/InsightDashboard.Pn.Test/AnswerFilterHelperUTests.cs b/eFormAPI/Plugins/InsightDashboard.Pn/InsightDashboard.Pn.Test/AnswerFilterHelperUTests.cs index 28c7885b..79746032 100644 --- a/eFormAPI/Plugins/InsightDashboard.Pn/InsightDashboard.Pn.Test/AnswerFilterHelperUTests.cs +++ b/eFormAPI/Plugins/InsightDashboard.Pn/InsightDashboard.Pn.Test/AnswerFilterHelperUTests.cs @@ -33,6 +33,7 @@ namespace InsightDashboard.Pn.Test; using Infrastructure.Models.Dashboards; using Microsoft.EntityFrameworkCore; using Microting.eForm.Infrastructure.Constants; +using Microting.eForm.Infrastructure.Data.Entities; using Microting.InsightDashboardBase.Infrastructure.Data.Entities; using NUnit.Framework; @@ -376,4 +377,69 @@ public async Task KeysetPaging_HandlesAnswersSharingATimestamp() Assert.That(stitched, Is.EqualTo(expected), "One-row batches through a timestamp collision must still cover it exactly once."); } + + /// + /// The seeded database happens to contain no two answers sharing a FinishedAt, + /// so the test above skips and the timestamp-collision case - the exact reason + /// the Id tie-breaker exists - would go unverified. This pins the predicate + /// itself against an in-memory set, where the collision can be constructed. + /// + /// It deliberately complements rather than replaces the database test: that one + /// proves the expression translates to SQL, this one proves it is correct when + /// timestamps collide. + /// + [Test] + public void KeysetPaging_TieBreaksOnIdWhenTimestampsCollide() + { + var shared = new DateTime(2026, 3, 2, 8, 14, 22); + + var answers = new List + { + new() { Id = 1, FinishedAt = shared }, + new() { Id = 2, FinishedAt = shared }, + new() { Id = 3, FinishedAt = shared }, + new() { Id = 4, FinishedAt = shared.AddMinutes(-1) }, + }.AsQueryable(); + + var expected = new[] { 3, 2, 1, 4 }; + + var stitched = new List(); + DateTime? cursorFinishedAt = null; + int? cursorId = null; + + // One row at a time, so every step crosses the collision. + for (var i = 0; i < expected.Length + 2; i++) + { + var batch = RawDataPaging + .AfterCursor(answers, cursorFinishedAt, cursorId) + .Take(1) + .ToList(); + + if (batch.Count == 0) + { + break; + } + + stitched.Add(batch[0].Id); + cursorFinishedAt = batch[0].FinishedAt; + cursorId = batch[0].Id; + } + + Assert.That(stitched, Is.EqualTo(expected), + "Answers sharing a timestamp must be returned newest-id-first, exactly once each."); + } + + [Test] + public void KeysetPaging_WithoutACursorStartsAtTheNewest() + { + var answers = new List + { + new() { Id = 1, FinishedAt = new DateTime(2026, 1, 1) }, + new() { Id = 2, FinishedAt = new DateTime(2026, 3, 1) }, + }.AsQueryable(); + + var first = RawDataPaging.AfterCursor(answers, null, null).First(); + + Assert.That(first.Id, Is.EqualTo(2)); + } }