Skip to content

Commit 6402331

Browse files
committed
feat(dashboard): validate --group-by requires --limit and --sort references --query
Add client-side validation that catches two common API errors early: 1. validateGroupByRequiresLimit: Widgets with --group-by must include --limit. The Sentry API rejects grouped widgets without a limit. Only fires for explicit --group-by, not auto-defaulted columns (e.g., issue dataset auto-defaults columns to ['issue']). 2. validateSortReferencesAggregate: The --sort expression must reference an aggregate present in --query. Prevents 400 errors from the API when sort references a field not in the widget's aggregates. Both validations run in the add path (buildWidgetFromFlags) and edit path (validateQueryConstraints), with the edit path only checking when the user actively changes query/group-by/sort flags.
1 parent b5ad3ad commit 6402331

File tree

3 files changed

+107
-18
lines changed

3 files changed

+107
-18
lines changed

src/commands/dashboard/resolve.ts

Lines changed: 53 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -324,6 +324,50 @@ export function resolveWidgetIndex(
324324
* @param opts - Widget configuration from parsed flags
325325
* @returns Validated widget with computed query fields
326326
*/
327+
/**
328+
* Validate that a sort expression references an aggregate present in the query.
329+
* The Sentry API returns 400 when the sort field isn't in the widget's aggregates.
330+
*
331+
* @param orderby - Parsed sort expression (e.g., "-count()", "p90(span.duration)")
332+
* @param aggregates - Parsed aggregate expressions from the query
333+
*/
334+
export function validateSortReferencesAggregate(
335+
orderby: string,
336+
aggregates: string[]
337+
): void {
338+
// Strip leading "-" for descending sorts
339+
const sortAgg = orderby.startsWith("-") ? orderby.slice(1) : orderby;
340+
if (!aggregates.includes(sortAgg)) {
341+
throw new ValidationError(
342+
`Sort expression "${orderby}" references "${sortAgg}" which is not in the query.\n\n` +
343+
"The --sort field must be one of the aggregate expressions in --query.\n" +
344+
`Current aggregates: ${aggregates.join(", ")}\n\n` +
345+
`Either add "${sortAgg}" to --query or sort by an existing aggregate.`,
346+
"sort"
347+
);
348+
}
349+
}
350+
351+
/**
352+
* Validate that grouped widgets (those with columns/group-by) include a limit.
353+
* The Sentry API rejects grouped widgets without a limit.
354+
*
355+
* @param columns - Group-by columns
356+
* @param limit - Widget limit (undefined if not set)
357+
*/
358+
export function validateGroupByRequiresLimit(
359+
columns: string[],
360+
limit: number | undefined
361+
): void {
362+
if (columns.length > 0 && limit === undefined) {
363+
throw new ValidationError(
364+
"Widgets with --group-by require --limit. " +
365+
"Add --limit <n> to specify the maximum number of groups to display.",
366+
"limit"
367+
);
368+
}
369+
}
370+
327371
export function buildWidgetFromFlags(opts: {
328372
title: string;
329373
display: string;
@@ -350,6 +394,15 @@ export function buildWidgetFromFlags(opts: {
350394
orderby = `-${aggregates[0]}`;
351395
}
352396

397+
// Only validate when user explicitly passes --group-by, not for auto-defaulted columns
398+
// (e.g., issue dataset auto-defaults columns to ["issue"] for table display)
399+
if (opts.groupBy) {
400+
validateGroupByRequiresLimit(columns, opts.limit);
401+
}
402+
if (orderby) {
403+
validateSortReferencesAggregate(orderby, aggregates);
404+
}
405+
353406
const raw = {
354407
title: opts.title,
355408
displayType: opts.display,

src/commands/dashboard/widget/edit.ts

Lines changed: 53 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,8 @@ import {
3232
resolveDashboardId,
3333
resolveOrgFromTarget,
3434
resolveWidgetIndex,
35+
validateGroupByRequiresLimit,
36+
validateSortReferencesAggregate,
3537
validateWidgetEnums,
3638
type WidgetQueryFlags,
3739
} from "../resolve.js";
@@ -101,38 +103,71 @@ function mergeLayout(
101103
};
102104
}
103105

104-
/** Build the replacement widget object by merging flags over existing */
105-
function buildReplacement(
106+
/**
107+
* Validate enum and aggregate constraints on the effective (merged) widget state.
108+
* Extracted from buildReplacement to stay under Biome's complexity limit.
109+
*/
110+
function validateEnumsAndAggregates(
106111
flags: EditFlags,
107-
existing: DashboardWidget
108-
): DashboardWidget {
109-
const mergedQueries = mergeQueries(flags, existing.queries?.[0]);
110-
111-
// Validate aggregates when query or dataset changes — prevents broken widgets
112-
// (e.g. switching --dataset from discover to spans with discover-only aggregates)
112+
existing: DashboardWidget,
113+
mergedQueries: DashboardWidgetQuery[] | undefined
114+
): void {
113115
const newDataset = flags.dataset ?? existing.widgetType;
114116
const aggregatesToValidate =
115117
mergedQueries?.[0]?.aggregates ?? existing.queries?.[0]?.aggregates;
116118
if ((flags.query || flags.dataset) && aggregatesToValidate) {
117119
validateAggregateNames(aggregatesToValidate, newDataset);
118120
}
119121

120-
const limit = flags.limit !== undefined ? flags.limit : existing.limit;
121-
122-
const effectiveDisplay = flags.display ?? existing.displayType;
123-
const effectiveDataset = flags.dataset ?? existing.widgetType;
124-
125-
// Re-validate after merging with existing values. validateWidgetEnums only
126-
// checks the cross-constraint when both args are provided, so it misses
127-
// e.g. `--dataset preprod-app-size` on a widget that's already `table`.
128-
// validateWidgetEnums itself skips untracked display types (text, wheel, etc.).
129122
if (flags.display || flags.dataset) {
123+
const effectiveDisplay = flags.display ?? existing.displayType;
124+
const effectiveDataset = flags.dataset ?? existing.widgetType;
130125
validateWidgetEnums(effectiveDisplay, effectiveDataset);
131126
}
127+
}
128+
129+
/**
130+
* Validate group-by+limit and sort constraints on the effective (merged) widget state.
131+
* Only runs when the user changes query, group-by, or sort — not when preserving
132+
* existing widget state which may predate these validations.
133+
*/
134+
function validateQueryConstraints(
135+
flags: EditFlags,
136+
existing: DashboardWidget,
137+
mergedQueries: DashboardWidgetQuery[] | undefined,
138+
limit: number | null | undefined
139+
): void {
140+
if (flags["group-by"] || flags.query) {
141+
const columns =
142+
mergedQueries?.[0]?.columns ?? existing.queries?.[0]?.columns ?? [];
143+
validateGroupByRequiresLimit(columns, limit ?? undefined);
144+
}
145+
146+
if (flags.sort || flags.query) {
147+
const orderby =
148+
mergedQueries?.[0]?.orderby ?? existing.queries?.[0]?.orderby;
149+
const aggregates =
150+
mergedQueries?.[0]?.aggregates ?? existing.queries?.[0]?.aggregates ?? [];
151+
if (orderby && aggregates.length > 0) {
152+
validateSortReferencesAggregate(orderby, aggregates);
153+
}
154+
}
155+
}
156+
157+
/** Build the replacement widget object by merging flags over existing */
158+
function buildReplacement(
159+
flags: EditFlags,
160+
existing: DashboardWidget
161+
): DashboardWidget {
162+
const mergedQueries = mergeQueries(flags, existing.queries?.[0]);
163+
const limit = flags.limit !== undefined ? flags.limit : existing.limit;
164+
165+
validateEnumsAndAggregates(flags, existing, mergedQueries);
166+
validateQueryConstraints(flags, existing, mergedQueries, limit);
132167

133168
const raw: Record<string, unknown> = {
134169
title: flags["new-title"] ?? existing.title,
135-
displayType: effectiveDisplay,
170+
displayType: flags.display ?? existing.displayType,
136171
queries: mergedQueries ?? existing.queries,
137172
layout: mergeLayout(flags, existing),
138173
};

test/commands/dashboard/widget/add.test.ts

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -286,6 +286,7 @@ describe("dashboard widget add", () => {
286286
display: "table",
287287
dataset: "issue",
288288
"group-by": ["project"],
289+
limit: 5,
289290
},
290291
"123",
291292
"Issues by Project"

0 commit comments

Comments
 (0)