Skip to content

Commit a1c804b

Browse files
claude[bot]claude
andauthored
fix(plugin-reports): infer export columns from every row, not the first 50 (#11840)
`pickFields()` sampled `rows.slice(0, 50)` while the projection it produced was applied to ALL rows. With no explicit `query.fields`, any key whose first occurrence fell at row 51+ was absent from the header and dropped from every row that carried it — in a well-formed export of uniform arity, with nothing marking a column as inferred rather than declared. Sparse columns are the normal shape of report output, and the sampled prefix is the query's first page in its own `orderBy`, so for a report sorted by status or created date the sample correlated with exactly the column it dropped. Affects `renderCsv` (also `renderReport`'s `default:` branch) and `renderHtmlTable`; `renderJson` was never affected. Route: scan every row, rather than projecting from the saved report's declared column set — `ReportQuery.fields` is optional and `renderReport` is exported with no access to a saved report at all, so the declared-set route would need a new declared surface, which this card does not carry. Cost is bounded twice over: the row array is already capped by `Math.min(query.limit, maxRows)`, and measured, the added pass is ~30% of the O(rows x cols) pass both renderers already make over the same array (5000x20: +4.2ms against 12.9ms of render). First-seen column order is preserved and late columns append, so an export that was already correct is byte-identical. Claude-Session: https://claude.ai/code/session_01APWX2AwT3a4xDcjPCe8bk4 Co-authored-by: Claude <noreply@anthropic.com>
1 parent e47d5ef commit a1c804b

3 files changed

Lines changed: 103 additions & 3 deletions

File tree

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
---
2+
"@objectstack/plugin-reports": patch
3+
---
4+
5+
Report CSV and HTML exports now infer their columns from **every** row of the result set, not from the first 50. Column inference sampled `rows.slice(0, 50)` while the projection it produced was applied to all rows, so when a report declared no explicit `query.fields`, any key whose first occurrence fell at row 51 or later was absent from the header *and* dropped from every row that carried it. The export gave no signal: the CSV was well-formed, every row had the same arity, and nothing marked a column as inferred rather than declared, so a recipient of a scheduled report attachment could not tell. Sparse columns are the normal shape of report output — an optional field, a formula only some records satisfy, a lookup that resolves for a subset — and the sampled prefix is the query's first page in its own `orderBy`, so for a report sorted by status or created date the sample correlated with exactly the column it dropped. Both affected renderers are fixed (`csv`, which is also the `default:` format branch, and `html_table`); `json` was never affected. Already-inferred columns keep their position and late-appearing ones are appended, so an export that was correct before is byte-identical now.

‎packages/plugins/plugin-reports/src/report-service.test.ts‎

Lines changed: 79 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -98,9 +98,65 @@ describe('renderReport', () => {
9898
expect(JSON.parse(out)).toEqual([{ a: 1 }]);
9999
});
100100

101-
it('auto-detects fields from first 50 rows when none specified', () => {
101+
it('auto-detects fields from the rows when none specified', () => {
102102
const out = renderReport([{ a: 1 }, { b: 2 }], 'csv');
103-
expect(out.split('\r\n')[0]).toMatch(/a|b/);
103+
expect(out.split('\r\n')[0].split(',')).toEqual(['a', 'b']);
104+
});
105+
106+
// [#11774] Column inference sampled only `rows.slice(0, 50)` while the
107+
// projection was applied to ALL rows, so a key whose first occurrence was at
108+
// row 51+ never reached the header and its values were dropped from every
109+
// row that carried it - a well-formed CSV of uniform arity with nothing
110+
// marking the loss. Sparse columns are the normal shape of report output,
111+
// and the first 50 rows are the query's first page IN ITS SORT ORDER, so
112+
// the sample is precisely the correlated case.
113+
describe('a column whose first row is past the old 50-row sample boundary', () => {
114+
const LATE_ROW = 55;
115+
const LATE_COL = 'escalation_note';
116+
const LATE_VALUE = 'breached SLA';
117+
118+
/** 60 rows sharing `id` + `status`; only row 55 carries LATE_COL. */
119+
function sparseRows(): Array<Record<string, unknown>> {
120+
return Array.from({ length: 60 }, (_v, i) => ({
121+
id: `l${String(i).padStart(2, '0')}`,
122+
status: 'open',
123+
...(i === LATE_ROW ? { [LATE_COL]: LATE_VALUE } : {}),
124+
}));
125+
}
126+
127+
it('csv: reaches the header, and its value reaches the row that carries it', () => {
128+
const out = renderReport(sparseRows(), 'csv');
129+
const lines = out.split('\r\n');
130+
const header = lines[0].split(',');
131+
132+
expect(header).toContain(LATE_COL);
133+
// Identity and order, not arity: already-inferred columns keep their
134+
// position and the late one is appended.
135+
expect(header).toEqual(['id', 'status', LATE_COL]);
136+
137+
// The value arrives, under its own column - `toContain(LATE_COL)` alone
138+
// would pass on an all-empty column.
139+
const col = header.indexOf(LATE_COL);
140+
expect(lines[1 + LATE_ROW].split(',')[col]).toBe(LATE_VALUE);
141+
// ...and only in that row.
142+
expect(lines[1].split(',')[col]).toBe('');
143+
});
144+
145+
it('html_table: reaches the header, and its value reaches the row that carries it', () => {
146+
const out = renderReport(sparseRows(), 'html_table');
147+
// `<th[^>]*>` would also match `<thead>` and swallow the first cell —
148+
// require a space or the closing angle right after the tag name.
149+
const header = [...out.matchAll(/<th(?:\s[^>]*)?>(.*?)<\/th>/g)].map(m => m[1]);
150+
const body = [...out.matchAll(/<tr>((?:<td[^>]*>.*?<\/td>)+)<\/tr>/g)]
151+
.map(m => [...m[1].matchAll(/<td[^>]*>(.*?)<\/td>/g)].map(c => c[1]));
152+
153+
expect(header).toContain(LATE_COL);
154+
expect(header).toEqual(['id', 'status', LATE_COL]);
155+
156+
const col = header.indexOf(LATE_COL);
157+
expect(body[LATE_ROW][col]).toBe(LATE_VALUE);
158+
expect(body[0][col]).toBe('');
159+
});
104160
});
105161
});
106162

@@ -210,6 +266,27 @@ describe('ReportService', () => {
210266
expect(stored.last_run_at).toBe(now.toISOString());
211267
});
212268

269+
it('run: a field first appearing past row 50 still reaches the export body', async () => {
270+
// [#11774] The scheduled-attachment path end to end: the saved report
271+
// declares no `query.fields`, so the header is inferred from the result
272+
// set. Only row 55 carries `escalation_note`.
273+
engine._tables['lead'] = Array.from({ length: 60 }, (_v, i) => ({
274+
id: `l${String(i).padStart(2, '0')}`,
275+
status: 'open',
276+
...(i === 55 ? { escalation_note: 'breached SLA' } : {}),
277+
}));
278+
const r = await svc.saveReport({
279+
name: 'Open', object: 'lead', query: { filter: { status: 'open' } }, format: 'csv',
280+
}, CTX);
281+
const result = await svc.run(r.id, CTX);
282+
283+
expect(result.rowCount).toBe(60);
284+
const lines = result.body.split('\r\n');
285+
const header = lines[0].split(',');
286+
expect(header).toContain('escalation_note');
287+
expect(lines[1 + 55].split(',')[header.indexOf('escalation_note')]).toBe('breached SLA');
288+
});
289+
213290
it('run: throws REPORT_NOT_FOUND for unknown id', async () => {
214291
await expect(svc.run('nope', CTX)).rejects.toThrow(/REPORT_NOT_FOUND/);
215292
});

‎packages/plugins/plugin-reports/src/report-service.ts‎

Lines changed: 19 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -118,10 +118,28 @@ function escapeCsvCell(v: unknown): string {
118118
return s;
119119
}
120120

121+
/**
122+
* Column set for an export, in first-seen order.
123+
*
124+
* [#11774] EVERY row, never a sample. The projection these columns drive is
125+
* applied to ALL rows, so inferring them from a prefix made any key whose
126+
* first occurrence fell past the boundary vanish from the header *and* from
127+
* every row that carried it — silently, in a well-formed export of uniform
128+
* arity with nothing marking a column as inferred rather than declared. The
129+
* prefix was also not a random sample: it is the query's first page in its
130+
* `orderBy`, so for a report sorted by status or date it correlates with
131+
* exactly the sparse column it drops.
132+
*
133+
* Cost is one `Object.keys` pass per row, and is bounded twice over: the row
134+
* array is already capped by `Math.min(query.limit, maxRows)` before it gets
135+
* here, and both renderers already walk the same array once per row per
136+
* column. Inference is therefore a fraction of the render it feeds, not a new
137+
* order of magnitude on the export path.
138+
*/
121139
function pickFields(rows: any[], explicit?: string[]): string[] {
122140
if (explicit && explicit.length > 0) return explicit;
123141
const seen = new Set<string>();
124-
for (const r of rows.slice(0, 50)) {
142+
for (const r of rows) {
125143
if (r && typeof r === 'object') for (const k of Object.keys(r)) seen.add(k);
126144
}
127145
return Array.from(seen);

0 commit comments

Comments
 (0)