diff --git a/.changeset/report-dataset-resolved-without-a-chart.md b/.changeset/report-dataset-resolved-without-a-chart.md new file mode 100644 index 0000000000..957668f212 --- /dev/null +++ b/.changeset/report-dataset-resolved-without-a-chart.md @@ -0,0 +1,19 @@ +--- +"@objectstack/lint": patch +--- + +A report's `dataset`, `rows`, `columns` and `values` are checked whether or not the report draws a chart (#16105) + +**Behaviour change — new findings on reports that used to publish clean.** `validateChartBindings` reached a report through one closure that opened `if (!isRec(chart)) return`, and that closure was the only place a report's `dataset` was ever passed to the resolver. Two gaps followed, and both are closed: + +- **A report authored without a `chart` was not checked at all.** Bind it to a dataset that does not exist and `os lint` exited 0 and named nothing. It now reports `chart-dataset-unknown` at `error`, the same as a charted report always did. +- **`rows` and `columns` were resolved on no report, charted or not.** On one and the same report object the measure selection (`values`) was resolved against the dataset and the dimension selection beside it was not. Both now report `chart-dimension-unknown` at `error` for a name the bound dataset does not declare as a dimension, at `reports[i].rows[j]` / `reports[i].columns[j]`. A chartless report's `values` is resolved for the first time too, under the existing `chart-measure-unknown`. + +`ReportSchema` is what makes these bindings rather than free text: it requires `dataset` + `values` on every non-`joined` report, and declares `rows` (the down axis) and `columns` (the across axis a `matrix` pivots on, ADR-0021 D2) as dimension names taken from that dataset. The chart is optional decoration on top of a binding the report already has. So a report bound to a missing dataset, or grouping on a dimension its dataset does not declare, now fails authoring instead of rendering blank or mis-grouped in production. + +No new rule id, no severity moved, and the charted path is unchanged — `chart-axis-not-selected` stays a `warning` and still resolves against the chart's own `chart.yAxis`. Two smaller corrections come with the restructure, both on messages an author reads: + +- The dataset finding on a report now points at `reports[i].dataset`, the key the author wrote. It used to say `reports[i].chart.dataset`, a position a report does not have. +- Its sentence ends "there is no data to render" rather than "the chart has no data to render", which is not true of a report that draws no chart. + +Blocks of a `joined` report carry the same keys and take the same checks. An unresolvable dataset is still exactly one finding per report or block. diff --git a/packages/lint/src/validate-chart-bindings.test.ts b/packages/lint/src/validate-chart-bindings.test.ts index bdb8b5c16f..f04dad5618 100644 --- a/packages/lint/src/validate-chart-bindings.test.ts +++ b/packages/lint/src/validate-chart-bindings.test.ts @@ -236,6 +236,193 @@ describe('validateChartBindings — report charts', () => { }); }); +/** + * #16105 — a report is dataset-bound whether or not it draws a chart. + * + * Four pins, mirroring the four injections the card measured on a real app, + * and each reads the findings ARRAY rather than a pass/fail: the tier that + * moved here is `error`, but `chart-axis-not-selected` rides along at + * `warning` and changes no exit code, so a test that only asked "did it fail" + * could not tell the two apart. Two of the four are NEGATIVE controls — the + * behaviour that already worked before the walk was restructured — because the + * failure mode this rule is now exposed to is a later refactor breaking the + * charted path while the new chartless assertions stay green. + */ +describe('validateChartBindings — a report binds a dataset with or without a chart', () => { + /** The card's `pipeline_coverage_by_quarter`: a matrix report, no chart. */ + const chartlessMatrix = (over: Record = {}) => ({ + ...baseStack(), + reports: [ + { + name: 'coverage_by_quarter', + type: 'matrix', + dataset: 'task_metrics', + rows: ['status'], + columns: ['priority'], + values: ['task_count'], + ...over, + }, + ], + }); + + /** The card's `opportunities_by_stage`: the same binding, plus a chart. */ + const chartedReport = (over: Record = {}) => ({ + ...baseStack(), + reports: [ + { + name: 'hours_by_status', + type: 'summary', + dataset: 'task_metrics', + rows: ['status'], + values: ['est_hours'], + chart: { type: 'bar', xAxis: 'status', yAxis: 'est_hours' }, + ...over, + }, + ], + }); + + // P1 — the card's Fact 1. Before the lift the ONLY site that received + // `report.dataset` was the chart closure, whose first line returned on a + // report with no `chart` key. + it('P1 · resolves a CHARTLESS report\'s dataset — the entrance the chart early-return used to own', () => { + const findings = validateChartBindings(chartlessMatrix({ dataset: 'task_metrics_nope' })); + expect(findings.map((f) => [f.rule, f.path, f.severity])).toEqual([ + [CHART_DATASET_UNKNOWN, 'reports[0].dataset', 'error'], + ]); + // The path names the key the author wrote. It used to be spelled + // `reports[0].chart.dataset` — a position a report does not have, and one + // a chartless report cannot have at all. + expect(findings[0].hint).toContain('Did you mean "task_metrics"?'); + // The dataset is unresolvable, so nothing downstream of it is resolved + // against anything: no dimension or measure finding piles on the typo. + expect(findings).toHaveLength(1); + }); + + // P2 — the card's Fact 2, on the CHARTED report: on one and the same report + // object the measure selection was resolved and the dimension selection + // beside it was not. + it('P2 · resolves `rows` on a charted report — the selection `values` was resolved without', () => { + const findings = validateChartBindings(chartedReport({ rows: ['status_nope'] })); + expect(findings.map((f) => [f.rule, f.path, f.severity])).toEqual([ + [CHART_DIMENSION_UNKNOWN, 'reports[0].rows[0]', 'error'], + ]); + expect(findings[0].message).toContain('not a dimension declared by dataset "task_metrics"'); + expect(findings[0].hint).toContain('Did you mean "status"?'); + }); + + it('P2 · resolves `columns` — the across axis a `matrix` pivots on (ADR-0021 D2)', () => { + const findings = validateChartBindings(chartlessMatrix({ columns: ['priority_nope'] })); + expect(findings.map((f) => [f.rule, f.path, f.severity])).toEqual([ + [CHART_DIMENSION_UNKNOWN, 'reports[0].columns[0]', 'error'], + ]); + }); + + it('P2 · resolves `rows` on a chartless report too — Fact 2 says every report', () => { + const findings = validateChartBindings(chartlessMatrix({ rows: ['status_nope'] })); + expect(findings.map((f) => [f.rule, f.path, f.severity])).toEqual([ + [CHART_DIMENSION_UNKNOWN, 'reports[0].rows[0]', 'error'], + ]); + }); + + // The same entrance, one collection down: `values` was resolved only when a + // chart existed, so a chartless report's measure selection was as invisible + // as its dimensions. + it('resolves a chartless report\'s `values` measures', () => { + const findings = validateChartBindings(chartlessMatrix({ values: ['task_count_nope'] })); + expect(findings.map((f) => [f.rule, f.path, f.severity])).toEqual([ + [CHART_MEASURE_UNKNOWN, 'reports[0].values[0]', 'error'], + ]); + expect(findings[0].message).toContain('this series comes back empty'); + }); + + // Every position of a joined report's block, which declares the same keys. + it('resolves a joined BLOCK\'s dataset and `rows` with no chart on the block', () => { + const findings = validateChartBindings({ + ...baseStack(), + reports: [ + { + name: 'overview', + type: 'joined', + blocks: [ + { name: 'b1', dataset: 'task_metrics', rows: ['status_nope'], values: ['task_count'] }, + { name: 'b2', dataset: 'task_metrics_nope', rows: ['status'], values: ['task_count'] }, + ], + }, + ], + }); + expect(findings.map((f) => [f.rule, f.path, f.where])).toEqual([ + [ + CHART_DIMENSION_UNKNOWN, + 'reports[0].blocks[0].rows[0]', + 'report "overview" · block "b1"', + ], + [ + CHART_DATASET_UNKNOWN, + 'reports[0].blocks[1].dataset', + 'report "overview" · block "b2"', + ], + ]); + }); + + // N1 — the working control the card ran beside P1: the SAME edit on a report + // that does have a chart. It gated before the lift and must still gate, and + // exactly once: the dataset is resolved per report surface, not per group of + // positions, so restructuring must not double-report the one typo. + it('N1 · a charted report\'s unresolvable dataset still gates — exactly ONE finding, not one per position group', () => { + const findings = validateChartBindings(chartedReport({ dataset: 'task_metrics_nope' })); + expect(findings).toHaveLength(1); + expect(findings[0].rule).toBe(CHART_DATASET_UNKNOWN); + expect(findings[0].severity).toBe('error'); + expect(findings[0].path).toBe('reports[0].dataset'); + }); + + // N2 — the card's other working control: the measure selection renamed on a + // charted report. Both tiers, in order, and the tiers are the point: the + // `error` is what an exit code sees, the `warning` rides along and does not + // change it (the card's third implementer note). + it('N2 · a charted report\'s renamed `values` still gates, with the advisory tier riding along unchanged', () => { + const findings = validateChartBindings( + chartedReport({ + values: ['est_hours_nope', 'est_hours'], + chart: { + type: 'bar', + xAxis: 'status', + yAxis: 'est_hours', + series: [{ name: 'task_count' }], + }, + }), + ); + expect(findings.map((f) => [f.rule, f.path, f.severity])).toEqual([ + [CHART_MEASURE_UNKNOWN, 'reports[0].values[0]', 'error'], + [CHART_AXIS_NOT_SELECTED, 'reports[0].chart.series[0].name', 'warning'], + ]); + expect(findings.filter((f) => f.severity === 'error')).toHaveLength(1); + }); + + // N3 — the floor on both shapes. A clean report of either kind says nothing, + // which is what makes the four pins above readings rather than noise. + it('N3 · a clean charted report and a clean chartless report both report nothing', () => { + expect(validateChartBindings(chartedReport())).toEqual([]); + expect(validateChartBindings(chartlessMatrix())).toEqual([]); + }); + + // A `joined` container selects nothing itself (`ReportSchema` puts its data + // on `blocks`), so it binds no dataset and has nothing to resolve. + it('N3 · a joined container with no dataset of its own reports nothing for itself', () => { + const findings = validateChartBindings({ + ...baseStack(), + reports: [ + { + name: 'overview', + type: 'joined', + blocks: [{ name: 'b1', dataset: 'task_metrics', rows: ['status'], values: ['task_count'] }], + }, + ], + }); + expect(findings).toEqual([]); + }); +}); + /** * #15575 — the per-position tier and consequence, pinned per surface against * the `@object-ui` revision `.objectui-sha` names. Each title names the @@ -735,7 +922,13 @@ describe('validateChartBindings — floor', () => { expect(validateChartBindings(null as unknown as Record)).toEqual([]); }); - it('ignores a report with no chart', () => { + // #16105 re-scoped this from "ignores a report with no chart" — a global + // zero that held for the wrong reason. The report below is CLEAN: its + // dataset resolves, `status` is a declared dimension and `task_count` a + // declared measure. The zero is now a reading about this fixture rather than + // about the walk skipping it; the readings about the walk are the pins in + // the "with or without a chart" block above. + it('says nothing about a chartless report whose every binding resolves', () => { const findings = validateChartBindings({ ...baseStack(), reports: [{ name: 'plain', dataset: 'task_metrics', rows: ['status'], values: ['task_count'] }], diff --git a/packages/lint/src/validate-chart-bindings.ts b/packages/lint/src/validate-chart-bindings.ts index 718a2cd582..084fc7b3eb 100644 --- a/packages/lint/src/validate-chart-bindings.ts +++ b/packages/lint/src/validate-chart-bindings.ts @@ -125,6 +125,42 @@ * * The list-view and page-component surfaces keep `values`: there it IS the * measure set the query asks for, so the existing resolution is correct. + * + * ## What a REPORT binds, chart or no chart (#16105) + * + * A report is dataset-bound in its own right — `ReportSchema` requires + * `dataset` + `values` on every non-`joined` report and declares `rows` (the + * down axis) and `columns` (the across axis a `matrix` pivots on, ADR-0021 D2) + * as dimension names "from the dataset". The chart is optional decoration on + * top of that binding, not the binding itself. + * + * The walk did not read it that way. Every report position reached the resolver + * through one closure that opened `if (!isRec(chart)) return`, and that closure + * was the only site `report.dataset` was ever passed to, so: + * + * - a report authored WITHOUT a chart was not checked at all — an unresolvable + * `dataset`, an unknown `rows`/`columns` dimension and an unknown `values` + * measure all published clean; and + * - `rows`/`columns` were passed to the resolver on NO report, charted or not. + * On one and the same charted report, `values` was resolved and the + * dimension selection beside it was not. + * + * So the dataset is resolved once per report and once per block — before the + * chart question is asked — and fed to two groups of positions: the report's + * own selection (`rows`/`columns` → `chart-dimension-unknown`, `values` → + * `chart-measure-unknown`) and, when a chart is present, its axis refs + * exactly as before. Not a second "chartless reports too" pass after the early + * return: one path, entered unconditionally, with the chart as the branch it + * always was. Which is also why an unresolvable dataset on a charted report is + * still ONE finding — `resolveDataset` runs once per surface, not once per + * group. + * + * `rows`/`columns` take `chart-dimension-unknown` rather than an id of their + * own: the position is a dataset DIMENSION reference resolved against the + * dataset's declared dimensions, which is what that rule already means on the + * list-view and page surfaces, and the spec's own words for the two report + * keys are "down axis" and "across" — so the rule's existing sentence about an + * axis rendering with no categories is true where it now fires. */ export const CHART_DIMENSION_UNKNOWN = 'chart-dimension-unknown'; @@ -208,8 +244,15 @@ function indexDatasets(stack: AnyRec): Map { */ interface ChartBinding { dataset?: string; - /** Selected dimension names (list-chart shape). */ - dimensions?: { names: string[]; path: string }; + /** + * Selected dimension names — a LIST of selections, because a report has two + * of them and the other surfaces have one (#16105). `rows` is a report's + * down axis and `columns` its across axis (`matrix`, ADR-0021 D2); a list + * chart and a page chart each carry a single `dimensions` array. Every entry + * keeps its own path, so a finding names `reports[i].columns[j]` rather than + * a position in some merged list the author never wrote. + */ + dimensions?: Array<{ names: string[]; path: string }>; /** Selected measure names (list-chart / report shape). */ values?: { names: string[]; path: string }; /** Single dimension ref (report `xAxis`). */ @@ -245,10 +288,20 @@ interface ChartBinding { */ ownSelection?: string[]; where: string; - /** Path of the chart container, for the dataset-level finding. */ + /** Path of the binding's container, for the dataset-level finding. */ path: string; } +/** + * A binding whose `dataset` has already been resolved — the positions alone. + * + * The report surface resolves its dataset once and then checks two groups of + * positions against it (the report's own selection, and its chart's axis refs), + * which is why resolution and position-checking are separate steps rather than + * one call (#16105). + */ +type ResolvedBinding = Omit; + /** * What a measure name is DOING at the position it was written (#15575). * @@ -313,26 +366,40 @@ export function validateChartBindings(stack: AnyRec): ChartBindingFinding[] { const datasets = indexDatasets(stack); if (datasets.size === 0 && !stack.reports && !stack.views && !stack.pages) return findings; - const check = (binding: ChartBinding) => { - const dsName = binding.dataset; - if (!dsName) return; // nothing bound — the shape rules own that case + /** + * Resolve one bound dataset NAME, reporting `chart-dataset-unknown` when it + * names nothing declared. Returns `undefined` in that case and when nothing + * is bound at all — either way there is no dataset to resolve positions + * against, and the shape rules own an absent binding. + * + * A step of its own (#16105) so a surface can resolve its dataset BEFORE it + * asks whether it has a chart. It reports at most once per call, which is + * what keeps a charted report's unresolvable dataset a single finding. + */ + const resolveDataset = ( + dsName: string | undefined, + where: string, + path: string, + ): DatasetNames | undefined => { + if (!dsName) return undefined; // nothing bound — the shape rules own that case const ds = datasets.get(dsName); - if (!ds) { - findings.push({ - severity: 'error', - rule: CHART_DATASET_UNKNOWN, - where: binding.where, - path: `${binding.path}.dataset`, - message: - `binds dataset "${dsName}", which resolves to no declared dataset — ` + - `the chart has no data to render.`, - hint: - `Declared datasets: ${list(datasets.keys())}.${suggestName(dsName, datasets.keys())} ` + - `Define it with defineDataset() or fix the reference (ADR-0021).`, - }); - return; - } + if (ds) return ds; + findings.push({ + severity: 'error', + rule: CHART_DATASET_UNKNOWN, + where, + path: `${path}.dataset`, + message: + `binds dataset "${dsName}", which resolves to no declared dataset — ` + + `there is no data to render.`, + hint: + `Declared datasets: ${list(datasets.keys())}.${suggestName(dsName, datasets.keys())} ` + + `Define it with defineDataset() or fix the reference (ADR-0021).`, + }); + return undefined; + }; + const checkAgainst = (ds: DatasetNames, dsName: string, binding: ResolvedBinding) => { const dimensionRef = (name: string, path: string) => { if (ds.dimensions.has(name)) return; findings.push({ @@ -404,8 +471,7 @@ export function validateChartBindings(stack: AnyRec): ChartBindingFinding[] { } }; - const dimSel = binding.dimensions; - if (dimSel) { + for (const dimSel of binding.dimensions ?? []) { for (let i = 0; i < dimSel.names.length; i++) { dimensionRef(dimSel.names[i], `${dimSel.path}[${i}]`); } @@ -437,6 +503,13 @@ export function validateChartBindings(stack: AnyRec): ChartBindingFinding[] { for (const s of binding.series ?? []) measureRef(s.name, s.path, s.kind, derived); }; + /** Resolve a binding's dataset and check its positions — the one-call form. */ + const check = (binding: ChartBinding) => { + const ds = resolveDataset(binding.dataset, binding.where, binding.path); + if (!ds || !binding.dataset) return; + checkAgainst(ds, binding.dataset, binding); + }; + // ── 1. Report charts (report.chart + report.blocks[].chart) ── const reports = recordsOf(stack.reports); for (let ri = 0; ri < reports.length; ri++) { @@ -444,25 +517,50 @@ export function validateChartBindings(stack: AnyRec): ChartBindingFinding[] { if (!isRec(report)) continue; const reportName = strName(report.name) ?? `#${ri}`; - const checkReportChart = ( - chart: unknown, - dataset: string | undefined, - values: string[], - where: string, - path: string, - ) => { + /** + * One report SURFACE — a top-level report, or one of its `blocks[]`. The + * two carry the same binding keys (`dataset` + `rows`/`columns`/`values`, + * plus an optional `chart`), so they are one shape applied twice. + * + * ONE pass, with the dataset resolved BEFORE the chart question is asked + * (#16105). The chart closure this replaced opened with + * `if (!isRec(chart)) return`, and it was the only site that ever received + * `report.dataset` — so a report authored without a chart was not checked + * at all, and no report ever had its `rows`/`columns` resolved. Both are + * dataset bindings that a report declares in its own right: `rows` names + * the dimensions it groups down by, `columns` the across axis a `matrix` + * pivots on (ADR-0021 D2), and `values` the measures its table shows. + * + * The report's own selection and its chart's axis refs are checked as two + * groups against the ONE resolved dataset, because they sit at different + * depths and their findings should say so: `reports[i].rows[j]` under + * `report "x"`, `reports[i].chart.yAxis` under `report "x" · chart`. + */ + const checkReportSurface = (container: AnyRec, where: string, path: string) => { + const dsName = strName(container.dataset); + const ds = resolveDataset(dsName, where, path); + if (!ds || !dsName) return; + + // The REPORT's own selection — what the table shows, independent of any + // chart. `chart-axis-not-selected` reads none of it: on this surface the + // set a presentation override is measured against is the chart's own + // `{ chart.yAxis }` (#15734), passed as `ownSelection` below. + checkAgainst(ds, dsName, { + dimensions: [ + { names: strList(container.rows), path: `${path}.rows` }, + { names: strList(container.columns), path: `${path}.columns` }, + ], + values: { names: strList(container.values), path: `${path}.values` }, + where, + path, + }); + + const chart = container.chart; if (!isRec(chart)) return; + const xAxisName = strName(chart.xAxis); const yAxisName = strName(chart.yAxis); - check({ - dataset, - // `values` is the REPORT's measure selection — what the table beneath - // the chart shows — not a chart ref. It is fed in so those names are - // resolved against the dataset (`chart-measure-unknown`) once, here, - // rather than reported twice or not at all. It is NOT what the chart - // queries, so it is no longer the set `chart-axis-not-selected` reads - // on this surface; `ownSelection` below is (#15734). - values: { names: values, path: `${path}.values` }, - xAxis: strName(chart.xAxis) ? { name: strName(chart.xAxis)!, path: `${path}.chart.xAxis` } : undefined, + checkAgainst(ds, dsName, { + xAxis: xAxisName ? { name: xAxisName, path: `${path}.chart.xAxis` } : undefined, yAxis: yAxisName ? { name: yAxisName, path: `${path}.chart.yAxis` } : undefined, // The chart's own selection: the ONE measure it queries and derives a // series from. Empty when no `yAxis` is authored — the chart plots @@ -476,28 +574,20 @@ export function validateChartBindings(stack: AnyRec): ChartBindingFinding[] { kind: 'report-series' as const, })) .filter((s): s is { name: string; path: string; kind: 'report-series' } => !!s.name), - where, + where: `${where} · chart`, path: `${path}.chart`, }); }; - checkReportChart( - report.chart, - strName(report.dataset), - strList(report.values), - `report "${reportName}" · chart`, - `reports[${ri}]`, - ); + checkReportSurface(report, `report "${reportName}"`, `reports[${ri}]`); const blocks = Array.isArray(report.blocks) ? report.blocks : []; for (let bi = 0; bi < blocks.length; bi++) { const block = blocks[bi]; if (!isRec(block)) continue; - checkReportChart( - block.chart, - strName(block.dataset), - strList(block.values), - `report "${reportName}" · block "${strName(block.name) ?? `#${bi}`}" chart`, + checkReportSurface( + block, + `report "${reportName}" · block "${strName(block.name) ?? `#${bi}`}"`, `reports[${ri}].blocks[${bi}]`, ); } @@ -510,7 +600,7 @@ export function validateChartBindings(stack: AnyRec): ChartBindingFinding[] { if (!isRec(chart)) return; check({ dataset: strName(chart.dataset), - dimensions: { names: strList(chart.dimensions), path: `${path}.chart.dimensions` }, + dimensions: [{ names: strList(chart.dimensions), path: `${path}.chart.dimensions` }], values: { names: strList(chart.values), path: `${path}.chart.values` }, where, path: `${path}.chart`, @@ -571,7 +661,7 @@ export function validateChartBindings(stack: AnyRec): ChartBindingFinding[] { .filter((s): s is { name: string; path: string; kind: 'page-series' } => !!s.name); check({ dataset: strName(props.dataset), - dimensions: { names: strList(props.dimensions), path: `${path}.properties.dimensions` }, + dimensions: [{ names: strList(props.dimensions), path: `${path}.properties.dimensions` }], values: { names: strList(props.values), path: `${path}.properties.values` }, // #15575 — two limbs, not one concatenated `series` array: the pin // refuses an axis `field` and a series `name` for different reasons.