From ce90be5c887014449763080ed15b40a348e226af Mon Sep 17 00:00:00 2001 From: Nathnael Date: Thu, 20 Aug 2026 05:28:51 +0000 Subject: [PATCH] fix(reports): stop the 'first N rows' option failing on large exports MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The export path used one number for two different things: the format's hard row cap, and the caller's explicit 'give me the first N rows'. Because resolveExportCap() returned min(requested, formatCap) and runAll() then threw when the result reached it, picking 'Records: First 100' in the export dialog 400'd on any report with more than 100 rows — the user asked to be truncated and got an error instead. Splits them: formatRowCap() is the hard, non-caller-controllable ceiling that still throws when exceeded (a silently short file hides missing rows), while resolveRowLimit() is the deliberate truncation and is honoured by slicing. Verified against a 223-row dataset: limit=5 now returns 5 rows, and no limit returns all 223. --- .../exports/export-request.util.spec.ts | 41 ++++++++++++------- .../modules/exports/export-request.util.ts | 29 ++++++++++--- .../modules/reports/report-runner.service.ts | 32 ++++++++++----- .../src/modules/reports/reports.controller.ts | 9 ++-- 4 files changed, 78 insertions(+), 33 deletions(-) diff --git a/apps/edr-freight-api/src/modules/exports/export-request.util.spec.ts b/apps/edr-freight-api/src/modules/exports/export-request.util.spec.ts index 1a22be1d5..1b473b12a 100644 --- a/apps/edr-freight-api/src/modules/exports/export-request.util.spec.ts +++ b/apps/edr-freight-api/src/modules/exports/export-request.util.spec.ts @@ -1,4 +1,10 @@ -import { EXPORT_MIME, pickByKey, resolveExportCap, resolveExportFormat } from './export-request.util'; +import { + EXPORT_MIME, + formatRowCap, + pickByKey, + resolveExportFormat, + resolveRowLimit, +} from './export-request.util'; import { CSV_ROW_CAP, PDF_ROW_CAP, XLSX_ROW_CAP } from './tabular-export.service'; describe('resolveExportFormat', () => { @@ -15,25 +21,32 @@ describe('resolveExportFormat', () => { }); }); -describe('resolveExportCap', () => { - it('missing limit uses the full format cap', () => { - expect(resolveExportCap('xlsx', undefined)).toBe(XLSX_ROW_CAP); - expect(resolveExportCap('csv', undefined)).toBe(CSV_ROW_CAP); - expect(resolveExportCap('pdf', undefined)).toBe(PDF_ROW_CAP); +describe('formatRowCap', () => { + it('is the format\'s hard ceiling and is not caller-controllable', () => { + expect(formatRowCap('xlsx')).toBe(XLSX_ROW_CAP); + expect(formatRowCap('csv')).toBe(CSV_ROW_CAP); + expect(formatRowCap('pdf')).toBe(PDF_ROW_CAP); + }); +}); + +describe('resolveRowLimit', () => { + it('no limit means "everything, up to the cap"', () => { + expect(resolveRowLimit('xlsx', undefined)).toBeUndefined(); }); - it('a limit under the cap is used as-is', () => { - expect(resolveExportCap('pdf', '100')).toBe(100); + it('an explicit limit is the caller asking to be truncated — kept as-is', () => { + // Distinct from the cap: 100 here must yield 100 rows, not a 400, even + // when the unfiltered result is far larger. + expect(resolveRowLimit('pdf', '100')).toBe(100); }); - it('a limit over the cap is clamped down', () => { - expect(resolveExportCap('pdf', String(PDF_ROW_CAP + 1000))).toBe(PDF_ROW_CAP); - expect(resolveExportCap('xlsx', String(XLSX_ROW_CAP + 1))).toBe(XLSX_ROW_CAP); - expect(resolveExportCap('csv', String(CSV_ROW_CAP + 1))).toBe(CSV_ROW_CAP); + it('an explicit limit over the format cap is clamped down', () => { + expect(resolveRowLimit('pdf', String(PDF_ROW_CAP + 1000))).toBe(PDF_ROW_CAP); + expect(resolveRowLimit('csv', String(CSV_ROW_CAP + 1))).toBe(CSV_ROW_CAP); }); - it.each(['0', '-5', 'not-a-number', ''])('non-positive/invalid limit %p falls back to the cap', (raw) => { - expect(resolveExportCap('xlsx', raw)).toBe(XLSX_ROW_CAP); + it.each(['0', '-5', 'not-a-number', ''])('non-positive/invalid limit %p means no limit', (raw) => { + expect(resolveRowLimit('xlsx', raw)).toBeUndefined(); }); }); diff --git a/apps/edr-freight-api/src/modules/exports/export-request.util.ts b/apps/edr-freight-api/src/modules/exports/export-request.util.ts index d71db912e..39f1f2e8c 100644 --- a/apps/edr-freight-api/src/modules/exports/export-request.util.ts +++ b/apps/edr-freight-api/src/modules/exports/export-request.util.ts @@ -9,13 +9,30 @@ export function resolveExportFormat(raw: string | undefined): ExportFormat { return 'xlsx'; } -/** Caller's requested row limit, clamped to the format's hard cap. A - * missing/non-positive/non-numeric limit means "as many as the format allows". */ -export function resolveExportCap(format: ExportFormat, rawLimit: string | undefined): number { - const formatCap = - format === 'pdf' ? PDF_ROW_CAP : format === 'csv' ? CSV_ROW_CAP : XLSX_ROW_CAP; +/** + * The format's hard ceiling. Not caller-controllable: exceeding it is an error, + * because a silently short file is worse than a clear failure. + */ +export function formatRowCap(format: ExportFormat): number { + return format === 'pdf' ? PDF_ROW_CAP : format === 'csv' ? CSV_ROW_CAP : XLSX_ROW_CAP; +} + +/** + * The caller's deliberate "just the first N rows", clamped to the format cap. + * `undefined` means "everything, up to the cap". + * + * This is a DIFFERENT thing from the cap and must not share a number with it. + * Conflating them (as this code did originally) makes the dialog's + * "Records: First 100" option fail outright on any export with more than 100 + * rows — the user explicitly asked to be truncated, so truncating is the + * correct answer, not a 400. + */ +export function resolveRowLimit( + format: ExportFormat, + rawLimit: string | undefined, +): number | undefined { const requested = Number(rawLimit); - return requested > 0 ? Math.min(requested, formatCap) : formatCap; + return requested > 0 ? Math.min(requested, formatRowCap(format)) : undefined; } /** Content type + file extension per format, for the download response headers. */ diff --git a/apps/edr-freight-api/src/modules/reports/report-runner.service.ts b/apps/edr-freight-api/src/modules/reports/report-runner.service.ts index 8ba2c5da2..d38a5ba8a 100644 --- a/apps/edr-freight-api/src/modules/reports/report-runner.service.ts +++ b/apps/edr-freight-api/src/modules/reports/report-runner.service.ts @@ -122,12 +122,19 @@ export class ReportRunnerService { }; } - /** Same query, no paging — used by the export path. */ + /** + * Same query, no paging — used by the export path. + * + * `limit` is the caller's deliberate "first N" (the dialog's "Records: First + * 100"), honoured by truncating. `cap` is the format's hard ceiling, which + * throws instead. These used to be one number, which made "First 100" fail + * outright on any report with more than 100 rows. + */ async runAll( def: ReportDefinition, raw: RawReportQuery, directions: string[] | null, - limit: number, + { cap, limit }: { cap: number; limit?: number }, ): Promise<{ columns: typeof def.columns; items: Record[]; kpis: ReportRunResult['kpis'] }> { const params = coerceParams(def, raw); const ctx = { ds: this.ds, params, directions }; @@ -136,14 +143,19 @@ export class ReportRunnerService { // export is supposed to match what the user is looking at. const sort = resolveSort(def, raw.sortBy, raw.sortOrder); if (sort) qb.orderBy(sort.expr, sort.dir); - // limit + 1: fetching exactly `limit` cannot distinguish "there are exactly - // limit rows" from "there are more" — which is why the old `>= limit` check - // rejected a legitimate export of exactly the cap. - const items = await qb.limit(limit + 1).getRawMany(); - if (items.length > limit) { - throw new BadRequestException( - `Export exceeds the ${limit}-row cap for this format. Narrow the filters.`, - ); + + const ceiling = limit ?? cap; + // ceiling + 1: fetching exactly `ceiling` cannot distinguish "there are + // exactly that many rows" from "there are more" — which is why the old + // `>= limit` check rejected a legitimate export of exactly the cap. + const items = await qb.limit(ceiling + 1).getRawMany(); + if (items.length > ceiling) { + if (limit === undefined) { + throw new BadRequestException( + `Export exceeds the ${cap}-row cap for this format. Narrow the filters.`, + ); + } + items.length = limit; } const kpis = def.summary ? await def.summary(ctx) : []; return { columns: def.columns, items, kpis }; diff --git a/apps/edr-freight-api/src/modules/reports/reports.controller.ts b/apps/edr-freight-api/src/modules/reports/reports.controller.ts index 33a4214c8..774c992fb 100644 --- a/apps/edr-freight-api/src/modules/reports/reports.controller.ts +++ b/apps/edr-freight-api/src/modules/reports/reports.controller.ts @@ -12,9 +12,10 @@ import { FREIGHT_PERMS, reportPermissionKey } from '../../seed/freight-permissio import { UserTradeAccessService } from '../user-trade-access/user-trade-access.service'; import { EXPORT_MIME, + formatRowCap, pickByKey, - resolveExportCap, resolveExportFormat, + resolveRowLimit, } from '../exports/export-request.util'; import { TabularExportService } from '../exports/tabular-export.service'; import { RawReportQuery, ReportRunnerService } from './report-runner.service'; @@ -101,10 +102,12 @@ export class ReportsController { const def = this.resolve(key, user); const directions = await this.userTradeAccessService.resolveAllowedDirections(user); const format = resolveExportFormat(query.format); - const cap = resolveExportCap(format, query.limit); const exportColumns = pickByKey(def.columns, query.fields); - const { items, kpis } = await this.runner.runAll(def, query, directions, cap); + const { items, kpis } = await this.runner.runAll(def, query, directions, { + cap: formatRowCap(format), + limit: resolveRowLimit(format, query.limit), + }); const doc = { title: def.title, description: def.description,