From 81925f3db165886801e2d22e7d38e95c6f7dc705 Mon Sep 17 00:00:00 2001 From: Warren Lee <5959690+wrn14897@users.noreply.github.com> Date: Mon, 31 Aug 2026 18:45:38 -0700 Subject: [PATCH 1/8] feat: support inline chart alerts in external API v2 and MCP (HDX-5191) Inline alerts (source 'inline' + chartConfig, added by HDX-5090) were internal-API-only. This extends them to the public surfaces: - External API v2 accepts source 'inline' with a chartConfig in the same tile-config dialect v2 dashboards use (line/stacked_bar/number, builder and raw SQL), converted to the internal AlertChartConfig before the shared validateAlertInput rules run (display-type allowlist, metric formula validation, raw SQL template validation, team-scoped source/connection ownership, source<->connection consistency) - Single-alert responses (GET by id, POST, PUT) emit chartConfig in the external dialect; the paginated list stays lean - OpenAPI: AlertSource enum, AlertChartConfig component (reusing the dashboard chart-config components), request example - MCP clickstack_save_alert accepts source 'inline' + chartConfig (reusing the dashboard tile config schemas) and clickstack_get_alert detail includes it, with a name fallback to chartConfig.name - Inline alerts stay excluded from Terraform import eligibility (documented on isImportableAlert) --- .changeset/inline-alerts-external-api-mcp.md | 11 + packages/api/openapi.json | 56 ++- .../api/src/mcp/__tests__/alerts.int.test.ts | 195 ++++++++ .../tools/alerts/__tests__/schemas.test.ts | 48 ++ packages/api/src/mcp/tools/alerts/getAlert.ts | 10 +- .../api/src/mcp/tools/alerts/saveAlert.ts | 46 +- packages/api/src/mcp/tools/alerts/schemas.ts | 54 ++- .../api/src/mcp/tools/dashboards/schemas.ts | 8 +- .../external-api/__tests__/alerts.int.test.ts | 448 ++++++++++++++++-- .../api/src/routers/external-api/v2/alerts.ts | 93 +++- .../external-api/v2/utils/alertChartConfig.ts | 68 +++ .../external-api/v2/utils/dashboards.ts | 4 +- .../src/utils/__tests__/externalApi.test.ts | 111 +++++ packages/api/src/utils/externalApi.ts | 32 +- packages/api/src/utils/zod.ts | 122 ++++- .../common-utils/src/__tests__/iac.test.ts | 6 + packages/common-utils/src/iacEligibility.ts | 7 +- 17 files changed, 1258 insertions(+), 61 deletions(-) create mode 100644 .changeset/inline-alerts-external-api-mcp.md create mode 100644 packages/api/src/routers/external-api/v2/utils/alertChartConfig.ts diff --git a/.changeset/inline-alerts-external-api-mcp.md b/.changeset/inline-alerts-external-api-mcp.md new file mode 100644 index 0000000000..b5d0aa0374 --- /dev/null +++ b/.changeset/inline-alerts-external-api-mcp.md @@ -0,0 +1,11 @@ +--- +'@hyperdx/api': minor +--- + +Support inline chart alerts (source `inline` + `chartConfig`) in the external +API v2 and the MCP `clickstack_save_alert` / `clickstack_get_alert` tools. +Inline alerts can now be created, updated, listed, and deleted through +`/api/v2/alerts` using the same tile-config dialect as v2 dashboards, with the +same validation rules as the internal API (display-type allowlist, metric +formula validation, raw SQL template validation, and team-scoped +source/connection ownership). diff --git a/packages/api/openapi.json b/packages/api/openapi.json index 84248b8713..647f143b97 100644 --- a/packages/api/openapi.json +++ b/packages/api/openapi.json @@ -92,9 +92,32 @@ "type": "string", "enum": [ "saved_search", - "tile" + "tile", + "inline" ], - "description": "Alert source type." + "description": "Alert source type. \"saved_search\" alerts monitor a saved search, \"tile\" alerts monitor a dashboard tile, and \"inline\" alerts carry their own chart configuration (see AlertChartConfig) without requiring a saved search or dashboard.\n" + }, + "AlertChartConfig": { + "description": "The chart configuration an inline alert evaluates, in the same dialect as dashboard tile configs. Only the display types the alert evaluator supports are accepted: line, stacked_bar, and number, in both builder and Raw SQL (configType \"sql\") variants. Raw SQL templates must reference the evaluation window via the time-filter and interval macros (e.g. $__timeFilter and $__timeInterval), and the referenced source/connection must belong to the team.\n", + "oneOf": [ + { + "$ref": "#/components/schemas/LineChartConfig" + }, + { + "$ref": "#/components/schemas/BarChartConfig" + }, + { + "$ref": "#/components/schemas/NumberChartConfig" + } + ], + "discriminator": { + "propertyName": "displayType", + "mapping": { + "line": "#/components/schemas/LineChartConfig", + "stacked_bar": "#/components/schemas/BarChartConfig", + "number": "#/components/schemas/NumberChartConfig" + } + } }, "AlertState": { "type": "string", @@ -240,6 +263,10 @@ "nullable": true, "example": "ServiceName" }, + "chartConfig": { + "$ref": "#/components/schemas/AlertChartConfig", + "description": "Chart configuration for inline alerts. Required when source is \"inline\" and rejected otherwise. Returned on single-alert responses (GET by ID, POST, PUT); the list endpoint omits it.\n" + }, "threshold": { "type": "number", "description": "Threshold value for triggering the alert. For between and not_between threshold types, this is the lower bound.", @@ -5418,6 +5445,31 @@ ], "name": "Error Spike Alert" } + }, + "inlineAlert": { + "summary": "Create an inline alert that carries its own chart config", + "value": { + "source": "inline", + "chartConfig": { + "displayType": "line", + "sourceId": "65f5e4a3b9e77c001a123456", + "select": [ + { + "aggFn": "count", + "where": "level:error", + "whereLanguage": "lucene" + } + ] + }, + "threshold": 100, + "interval": "5m", + "thresholdType": "above", + "channel": { + "type": "webhook", + "webhookId": "65f5e4a3b9e77c001a789012" + }, + "name": "Error log spike" + } } } } diff --git a/packages/api/src/mcp/__tests__/alerts.int.test.ts b/packages/api/src/mcp/__tests__/alerts.int.test.ts index 5fbd0d87cd..aff5a0f305 100644 --- a/packages/api/src/mcp/__tests__/alerts.int.test.ts +++ b/packages/api/src/mcp/__tests__/alerts.int.test.ts @@ -3,6 +3,7 @@ import { SourceKind, } from '@hyperdx/common-utils/dist/types'; import { Client } from '@modelcontextprotocol/sdk/client/index.js'; +import mongoose from 'mongoose'; import * as config from '@/config'; import { @@ -513,6 +514,200 @@ describe('MCP Alert Tools', () => { }); }); + describe('inline alerts', () => { + // External tile-config dialect, same as the dashboard tools. + const makeChartConfig = (overrides: Record = {}) => ({ + displayType: 'line', + sourceId: traceSource._id.toString(), + select: [ + { + aggFn: 'count', + where: 'StatusCode:STATUS_CODE_ERROR', + whereLanguage: 'lucene', + }, + ], + ...overrides, + }); + + const makeInlineInput = ( + webhookId: string, + overrides: Record = {}, + ) => ({ + source: 'inline', + chartConfig: makeChartConfig(), + threshold: 10, + thresholdType: 'above', + interval: '5m', + channel: { type: 'webhook', webhookId }, + name: 'Inline MCP Alert', + ...overrides, + }); + + it('should create an inline alert and echo the external-dialect chartConfig', async () => { + const webhook = await createTestWebhook(); + + const result = await callTool( + client, + 'clickstack_save_alert', + makeInlineInput(webhook._id.toString()), + ); + + expect(result.isError).toBeFalsy(); + const output = JSON.parse(getFirstText(result)); + expect(output.source).toBe('inline'); + expect(output.savedSearchId).toBeUndefined(); + expect(output.dashboardId).toBeUndefined(); + expect(output.chartConfig).toMatchObject({ + displayType: 'line', + sourceId: traceSource._id.toString(), + select: [ + { + aggFn: 'count', + where: 'StatusCode:STATUS_CODE_ERROR', + whereLanguage: 'lucene', + }, + ], + }); + + // Mongo persists the internal dialect the check-alerts task reads. + const stored = await Alert.findById(output.id); + expect(stored!.source).toBe(AlertSource.INLINE); + expect(stored!.chartConfig).toMatchObject({ + source: traceSource._id.toString(), + select: [ + { + aggFn: 'count', + aggCondition: 'StatusCode:STATUS_CODE_ERROR', + aggConditionLanguage: 'lucene', + }, + ], + }); + }); + + it('should update an inline alert chart config', async () => { + const webhook = await createTestWebhook(); + + const createResult = await callTool( + client, + 'clickstack_save_alert', + makeInlineInput(webhook._id.toString()), + ); + const created = JSON.parse(getFirstText(createResult)); + + const updateResult = await callTool(client, 'clickstack_save_alert', { + ...makeInlineInput(webhook._id.toString(), { + chartConfig: makeChartConfig({ groupBy: 'ServiceName' }), + threshold: 42, + }), + id: created.id, + }); + + expect(updateResult.isError).toBeFalsy(); + const updated = JSON.parse(getFirstText(updateResult)); + expect(updated.threshold).toBe(42); + expect(updated.chartConfig).toMatchObject({ groupBy: 'ServiceName' }); + + const stored = await Alert.findById(created.id); + expect(stored!.threshold).toBe(42); + expect(stored!.chartConfig).toMatchObject({ groupBy: 'ServiceName' }); + }); + + it('should reject inline source without chartConfig', async () => { + const webhook = await createTestWebhook(); + + const result = await callTool( + client, + 'clickstack_save_alert', + makeInlineInput(webhook._id.toString(), { chartConfig: undefined }), + ); + + expect(result.isError).toBe(true); + expect(getFirstText(result)).toContain('chartConfig is required'); + }); + + it('should reject a chartConfig on a saved-search alert', async () => { + const savedSearch = await createTestSavedSearch(); + const webhook = await createTestWebhook(); + + const result = await callTool(client, 'clickstack_save_alert', { + source: 'saved_search', + savedSearchId: savedSearch._id.toString(), + chartConfig: makeChartConfig(), + threshold: 10, + thresholdType: 'above', + interval: '5m', + channel: { type: 'webhook', webhookId: webhook._id.toString() }, + }); + + expect(result.isError).toBe(true); + expect(getFirstText(result)).toContain( + 'only supported when source is "inline"', + ); + }); + + it('should reject a malformed metric formula', async () => { + const webhook = await createTestWebhook(); + + const result = await callTool( + client, + 'clickstack_save_alert', + makeInlineInput(webhook._id.toString(), { + chartConfig: makeChartConfig({ + formulas: [{ expression: 'B * 2' }], + }), + }), + ); + + expect(result.isError).toBe(true); + expect(getFirstText(result)).toContain('Invalid chartConfig'); + }); + + it("should reject a source that does not belong to the team's sources", async () => { + const webhook = await createTestWebhook(); + + const result = await callTool( + client, + 'clickstack_save_alert', + makeInlineInput(webhook._id.toString(), { + chartConfig: makeChartConfig({ + sourceId: new mongoose.Types.ObjectId().toString(), + }), + }), + ); + + expect(result.isError).toBe(true); + expect(getFirstText(result)).toContain('Source not found'); + }); + + it('clickstack_get_alert detail includes chartConfig and falls back to its name', async () => { + const webhook = await createTestWebhook(); + + const createResult = await callTool( + client, + 'clickstack_save_alert', + makeInlineInput(webhook._id.toString(), { name: undefined }), + ); + const created = JSON.parse(getFirstText(createResult)); + + // Seed a chartConfig name directly (the external dialect does not + // carry one) to exercise the name fallback. + await Alert.findByIdAndUpdate(created.id, { + $set: { 'chartConfig.name': 'Error Rate Query' }, + }); + + const detail = await callTool(client, 'clickstack_get_alert', { + id: created.id, + }); + expect(detail.isError).toBeFalsy(); + const output = JSON.parse(getFirstText(detail)); + expect(output.chartConfig).toMatchObject({ + displayType: 'line', + sourceId: traceSource._id.toString(), + }); + expect(output.name).toBe('Error Rate Query'); + }); + }); + describe('multiple channels', () => { const namedWebhook = (name: string) => Webhook.create({ diff --git a/packages/api/src/mcp/tools/alerts/__tests__/schemas.test.ts b/packages/api/src/mcp/tools/alerts/__tests__/schemas.test.ts index 55a09951b9..1476a95369 100644 --- a/packages/api/src/mcp/tools/alerts/__tests__/schemas.test.ts +++ b/packages/api/src/mcp/tools/alerts/__tests__/schemas.test.ts @@ -82,3 +82,51 @@ describe('validateSaveAlertInput channel comparison', () => { expect(result).toBeNull(); }); }); + +describe('validateSaveAlertInput inline alerts', () => { + const chartConfig: NonNullable = { + displayType: 'line', + sourceId: 'source-1', + select: [ + { + aggFn: 'count', + where: '', + whereLanguage: 'lucene', + }, + ], + fillNulls: true, + }; + + it('requires chartConfig when source is inline', () => { + const result = validateSaveAlertInput({ + ...baseInput, + source: 'inline', + savedSearchId: undefined, + channel: wh('a'), + }); + + expect(result).toContain('chartConfig is required'); + }); + + it('accepts an inline alert with a chartConfig', () => { + const result = validateSaveAlertInput({ + ...baseInput, + source: 'inline', + savedSearchId: undefined, + chartConfig, + channel: wh('a'), + }); + + expect(result).toBeNull(); + }); + + it('rejects a chartConfig on non-inline sources instead of silently dropping it', () => { + const result = validateSaveAlertInput({ + ...baseInput, + chartConfig, + channel: wh('a'), + }); + + expect(result).toContain('only supported when source is "inline"'); + }); +}); diff --git a/packages/api/src/mcp/tools/alerts/getAlert.ts b/packages/api/src/mcp/tools/alerts/getAlert.ts index 457ba4573d..ada1f63fba 100644 --- a/packages/api/src/mcp/tools/alerts/getAlert.ts +++ b/packages/api/src/mcp/tools/alerts/getAlert.ts @@ -18,6 +18,7 @@ function deriveAlertName(alert: { tileId?: string | null; savedSearch?: ISavedSearch | null; dashboard?: IDashboard | null; + chartConfig?: { name?: string } | null; }): string | null { // Prefer explicit alert name if (alert.name) { @@ -29,6 +30,11 @@ function deriveAlertName(alert: { return alert.savedSearch.name; } + // Inline alerts: fall back to the persisted chart config's name + if (alert.chartConfig?.name) { + return alert.chartConfig.name; + } + // Fall back to dashboard tile name or dashboard name if (alert.dashboard?.name) { if (alert.tileId) { @@ -118,7 +124,9 @@ export function registerGetAlert({ return mcpUserError('Alert not found'); } - const external = translateAlertDocumentToExternalAlert(alert); + const external = translateAlertDocumentToExternalAlert(alert, { + includeChartConfig: true, + }); // Populate refs so deriveAlertName can fall back to the // saved search / dashboard name when the alert has no explicit name. diff --git a/packages/api/src/mcp/tools/alerts/saveAlert.ts b/packages/api/src/mcp/tools/alerts/saveAlert.ts index 3434b57d61..b2e1fb07e3 100644 --- a/packages/api/src/mcp/tools/alerts/saveAlert.ts +++ b/packages/api/src/mcp/tools/alerts/saveAlert.ts @@ -10,16 +10,25 @@ import { } from '@/controllers/alerts'; import type { ToolRegistrar } from '@/mcp/tools/types'; import { + formatZodIssues, mcpServerError, mcpUserError, validateObjectId, } from '@/mcp/utils/errors'; import { AlertSource } from '@/models/alert'; +import { convertExternalAlertChartConfigToInternal } from '@/routers/external-api/v2/utils/alertChartConfig'; import { BaseError } from '@/utils/errors'; import { translateAlertDocumentToExternalAlert } from '@/utils/externalApi'; +import { externalAlertChartConfigSchema } from '@/utils/zod'; import { mcpSaveAlertSchema, validateSaveAlertInput } from './schemas'; +const MCP_SOURCE_TO_ALERT_SOURCE = { + saved_search: AlertSource.SAVED_SEARCH, + tile: AlertSource.TILE, + inline: AlertSource.INLINE, +} as const; + export function registerSaveAlert({ context, registerTool, @@ -34,8 +43,10 @@ export function registerSaveAlert({ annotations: { destructiveHint: true }, description: 'Create a new alert (omit id) or update an existing one (provide id). ' + - 'Alerts monitor a saved search or dashboard tile and fire when the ' + - 'metric crosses a threshold. At least one webhook notification channel ' + + 'Alerts monitor a saved search, a dashboard tile, or an inline chart ' + + 'config (source "inline" + chartConfig — no saved search or dashboard ' + + 'needed) and fire when the metric crosses a threshold. At least one ' + + 'webhook notification channel ' + 'is required: pass "channels" for 1-10 targets, or the legacy singular ' + '"channel" for one. Updates replace the alert configuration rather than ' + 'merging it, so read the alert first and resend its full "channels" ' + @@ -56,9 +67,27 @@ export function registerSaveAlert({ if (idError) return idError; } + // Inline alerts: run the chart config through the shared external + // schema (same one the v2 REST API parses) so formula validation and + // the number single-select rule cannot drift between the surfaces, + // then convert to the internal shape the controllers persist. + let internalChartConfig: AlertInput['chartConfig']; + if (input.source === 'inline') { + const parsed = externalAlertChartConfigSchema.safeParse( + input.chartConfig, + ); + if (!parsed.success) { + return mcpUserError( + `Invalid chartConfig:\n${formatZodIssues(parsed.error)}`, + ); + } + internalChartConfig = convertExternalAlertChartConfigToInternal( + parsed.data, + ); + } + // Build the alert input matching the shape expected by controllers. - const source = - input.source === 'tile' ? AlertSource.TILE : AlertSource.SAVED_SEARCH; + const source = MCP_SOURCE_TO_ALERT_SOURCE[input.source]; const alertInput: AlertInput = { source, // `channel` is omitted; makeAlert mirrors it from channels[0]. @@ -75,6 +104,7 @@ export function registerSaveAlert({ savedSearchId: input.savedSearchId, dashboardId: input.dashboardId, tileId: input.tileId, + chartConfig: internalChartConfig, }; // ── Validate referenced entities exist ── @@ -107,7 +137,9 @@ export function registerSaveAlert({ type: 'text' as const, text: JSON.stringify( { - ...translateAlertDocumentToExternalAlert(updated), + ...translateAlertDocumentToExternalAlert(updated, { + includeChartConfig: true, + }), ...(frontendUrl ? { url: `${frontendUrl}/alerts` } : {}), }, null, @@ -130,7 +162,9 @@ export function registerSaveAlert({ type: 'text' as const, text: JSON.stringify( { - ...translateAlertDocumentToExternalAlert(created), + ...translateAlertDocumentToExternalAlert(created, { + includeChartConfig: true, + }), ...(frontendUrl ? { url: `${frontendUrl}/alerts` } : {}), }, null, diff --git a/packages/api/src/mcp/tools/alerts/schemas.ts b/packages/api/src/mcp/tools/alerts/schemas.ts index 57c90f09cd..c22f421c7e 100644 --- a/packages/api/src/mcp/tools/alerts/schemas.ts +++ b/packages/api/src/mcp/tools/alerts/schemas.ts @@ -7,6 +7,13 @@ import { } from '@hyperdx/common-utils/dist/types'; import { z } from 'zod'; +import { + mcpBarTileSchema, + mcpLineTileSchema, + mcpNumberTileSchema, + mcpSqlTileSchema, +} from '@/mcp/tools/dashboards/schemas'; + // --------------------------------------------------------------------------- // MCP-compatible flat Zod schema for clickstack_save_alert. // @@ -16,6 +23,36 @@ import { z } from 'zod'; // validateSaveAlertInput(). // --------------------------------------------------------------------------- +// Inline alerts carry their chart config in the same dialect as dashboard +// tile configs, restricted to the display types the alert evaluator can run +// as a time series: line, stacked_bar, and number (builder or raw SQL). +// Reuses the dashboard tile config shapes so agents author both from one +// vocabulary; the raw SQL variant narrows displayType and drops the +// tile-only onClick affordance. Full validation (formulas, number +// single-select rule, source/connection ownership) runs at save time via the +// shared external alert schema and validateAlertInput. +const mcpAlertChartConfigSchema = z + .union([ + mcpLineTileSchema.shape.config, + mcpBarTileSchema.shape.config, + mcpNumberTileSchema.shape.config, + mcpSqlTileSchema.shape.config.omit({ onClick: true }).extend({ + displayType: z + .enum(['line', 'stacked_bar', 'number']) + .describe( + 'How to render the SQL results. Alerts evaluate line, stacked_bar, or number charts only.', + ), + }), + ]) + .describe( + 'Chart configuration for inline alerts (required when source is "inline"). ' + + 'Same shape as a dashboard tile config, limited to displayType line, ' + + 'stacked_bar, or number. Omit configType for the builder variant ' + + '(sourceId + select); set configType to "sql" for the Raw SQL variant ' + + '(connectionId + sqlTemplate; the template must use the $__timeFilter ' + + 'and $__timeInterval macros so each evaluation window can be queried).', + ); + const mcpAlertChannelSchema = z .object({ type: z @@ -37,8 +74,12 @@ export const mcpSaveAlertSchema = z.object({ // Source source: z - .enum(['saved_search', 'tile']) - .describe('Alert source type: saved_search or tile.'), + .enum(['saved_search', 'tile', 'inline']) + .describe( + 'Alert source type: "saved_search" monitors a saved search, "tile" ' + + 'monitors a dashboard tile, and "inline" carries its own chartConfig ' + + 'without requiring a saved search or dashboard.', + ), savedSearchId: z .string() .optional() @@ -57,6 +98,7 @@ export const mcpSaveAlertSchema = z.object({ .string() .optional() .describe('Group-by key for saved search alerts.'), + chartConfig: mcpAlertChartConfigSchema.optional(), // Threshold threshold: z.number().describe('Threshold value for triggering the alert.'), @@ -161,6 +203,14 @@ export function validateSaveAlertInput(data: McpSaveAlertInput): string | null { return 'savedSearchId is required when source is "saved_search"'; } } + if (data.source === 'inline' && data.chartConfig == null) { + return 'chartConfig is required when source is "inline"'; + } + // Reject rather than silently drop a whole config: the caller clearly + // intended an inline alert. + if (data.source !== 'inline' && data.chartConfig != null) { + return 'chartConfig is only supported when source is "inline"'; + } // Threshold range checks if (isRangeThresholdType(data.thresholdType)) { diff --git a/packages/api/src/mcp/tools/dashboards/schemas.ts b/packages/api/src/mcp/tools/dashboards/schemas.ts index 79352c171d..c96b68d881 100644 --- a/packages/api/src/mcp/tools/dashboards/schemas.ts +++ b/packages/api/src/mcp/tools/dashboards/schemas.ts @@ -614,7 +614,7 @@ const mcpTileLayoutSchema = z.object({ ), }); -const mcpLineTileSchema = mcpTileLayoutSchema.extend({ +export const mcpLineTileSchema = mcpTileLayoutSchema.extend({ config: z.object({ ...rejectedTileWhereFields, displayType: z.literal('line').describe('Line chart over time'), @@ -660,7 +660,7 @@ const mcpLineTileSchema = mcpTileLayoutSchema.extend({ }), }); -const mcpBarTileSchema = mcpTileLayoutSchema.extend({ +export const mcpBarTileSchema = mcpTileLayoutSchema.extend({ config: z.object({ ...rejectedTileWhereFields, displayType: z @@ -730,7 +730,7 @@ const mcpTableTileSchema = mcpTileLayoutSchema.extend({ }), }); -const mcpNumberTileSchema = mcpTileLayoutSchema.extend({ +export const mcpNumberTileSchema = mcpTileLayoutSchema.extend({ config: z.object({ ...rejectedTileWhereFields, displayType: z.literal('number').describe('Single aggregate scalar value'), @@ -974,7 +974,7 @@ const mcpMarkdownTileSchema = mcpTileLayoutSchema.extend({ }), }); -const mcpSqlTileSchema = mcpTileLayoutSchema.extend({ +export const mcpSqlTileSchema = mcpTileLayoutSchema.extend({ config: z.object({ configType: z .literal('sql') diff --git a/packages/api/src/routers/external-api/__tests__/alerts.int.test.ts b/packages/api/src/routers/external-api/__tests__/alerts.int.test.ts index 49c5c7db31..e04aed5b1b 100644 --- a/packages/api/src/routers/external-api/__tests__/alerts.int.test.ts +++ b/packages/api/src/routers/external-api/__tests__/alerts.int.test.ts @@ -1,4 +1,4 @@ -import { AlertErrorType } from '@hyperdx/common-utils/dist/types'; +import { AlertErrorType, SourceKind } from '@hyperdx/common-utils/dist/types'; import _ from 'lodash'; import { ObjectId } from 'mongodb'; import request from 'supertest'; @@ -11,8 +11,10 @@ import { } from '@/fixtures'; import { AlertSource, AlertThresholdType } from '@/models/alert'; import Alert from '@/models/alert'; +import Connection from '@/models/connection'; import Dashboard from '@/models/dashboard'; import { SavedSearch } from '@/models/savedSearch'; +import { Source } from '@/models/source'; import Webhook, { WebhookService } from '@/models/webhook'; // Constants @@ -657,42 +659,430 @@ describe('External API Alerts', () => { }); }); - describe('Input validation', () => { - it('rejects the internal-only inline alert source', async () => { - // Inline alerts (source: 'inline') are creatable through the internal API - // only; the external v2 contract (OpenAPI docs, Terraform provider) has - // not been extended to cover them yet. + describe('Inline alerts', () => { + // Team-scoped connection + source the inline chart configs reference. + const makeSource = async () => { + const connection = await Connection.create({ + team: team._id, + name: 'Default', + host: 'http://localhost:8123', + username: 'default', + password: '', + }); + const source = await Source.create({ + kind: SourceKind.Log, + team: team._id, + from: { databaseName: 'default', tableName: 'otel_logs' }, + timestampValueExpression: 'Timestamp', + connection: connection._id, + name: 'Logs', + }); + return { connection, source }; + }; + + // External tile-config dialect (sourceId, per-select where) — the + // internal API's dialect (source, aggCondition) is rejected here. + const makeBuilderChartConfig = ( + sourceId: string, + overrides: Record = {}, + ) => ({ + displayType: 'line', + sourceId, + select: [ + { + aggFn: 'count', + where: 'level:error', + whereLanguage: 'lucene', + }, + ], + ...overrides, + }); + + const makeInlineAlertInput = ( + chartConfig: unknown, + webhookId: string, + overrides: Record = {}, + ) => ({ + source: 'inline', + chartConfig, + threshold: 100, + interval: '1h', + thresholdType: AlertThresholdType.ABOVE, + channel: { + type: 'webhook', + webhookId, + }, + ...overrides, + }); + + it('creates an inline alert, persists the internal dialect, and round-trips chartConfig on single-alert responses', async () => { + const { source } = await makeSource(); const webhook = await createTestWebhook(); - const alertInput = { - threshold: 100, - interval: '1h', - source: 'inline', - chartConfig: { - name: 'Chart Alert Query', - source: new ObjectId().toString(), - displayType: 'line', - select: [ + const created = await authRequest('post', ALERTS_BASE_URL) + .send( + makeInlineAlertInput( + makeBuilderChartConfig(source._id.toString()), + webhook._id.toString(), + { name: 'Inline Alert' }, + ), + ) + .expect(200); + + expect(created.body.data.source).toBe(AlertSource.INLINE); + expect(created.body.data.savedSearchId).toBeUndefined(); + expect(created.body.data.dashboardId).toBeUndefined(); + expect(created.body.data.chartConfig).toMatchObject({ + displayType: 'line', + sourceId: source._id.toString(), + select: [ + { + aggFn: 'count', + where: 'level:error', + whereLanguage: 'lucene', + }, + ], + }); + + // Mongo persists the internal dialect the check-alerts task evaluates. + const stored = await Alert.findById(created.body.data.id); + expect(stored!.chartConfig).toMatchObject({ + displayType: 'line', + source: source._id.toString(), + select: [ + { + aggFn: 'count', + aggCondition: 'level:error', + aggConditionLanguage: 'lucene', + }, + ], + }); + + const single = await authRequest( + 'get', + `${ALERTS_BASE_URL}/${created.body.data.id}`, + ).expect(200); + expect(single.body.data.chartConfig).toMatchObject({ + displayType: 'line', + sourceId: source._id.toString(), + }); + + // The list endpoint stays lean: no chartConfig per row. + const list = await authRequest('get', ALERTS_BASE_URL).expect(200); + const listed = list.body.data.find(a => a.id === created.body.data.id); + expect(listed.source).toBe(AlertSource.INLINE); + expect(listed.chartConfig).toBeUndefined(); + }); + + it('updates an inline alert config and maps asRatio to the internal ratio return type', async () => { + const { source } = await makeSource(); + const webhook = await createTestWebhook(); + + const created = await authRequest('post', ALERTS_BASE_URL) + .send( + makeInlineAlertInput( + makeBuilderChartConfig(source._id.toString()), + webhook._id.toString(), + ), + ) + .expect(200); + + const updated = await authRequest( + 'put', + `${ALERTS_BASE_URL}/${created.body.data.id}`, + ) + .send( + makeInlineAlertInput( + makeBuilderChartConfig(source._id.toString(), { + groupBy: 'ServiceName', + asRatio: true, + select: [ + { aggFn: 'count', where: 'level:error' }, + { aggFn: 'count', where: '' }, + ], + }), + webhook._id.toString(), + { threshold: 42 }, + ), + ) + .expect(200); + + expect(updated.body.data.threshold).toBe(42); + expect(updated.body.data.chartConfig).toMatchObject({ + groupBy: 'ServiceName', + asRatio: true, + }); + + const stored = await Alert.findById(created.body.data.id); + expect(stored!.threshold).toBe(42); + expect(stored!.chartConfig).toMatchObject({ + groupBy: 'ServiceName', + seriesReturnType: 'ratio', + }); + }); + + it('clears source-specific references when switching between inline and tile sources', async () => { + const { source } = await makeSource(); + const webhook = await createTestWebhook(); + const dashboard = await createTestDashboard(); + const tileId = dashboard.tiles[0].id; + + const created = await authRequest('post', ALERTS_BASE_URL) + .send( + makeInlineAlertInput( + makeBuilderChartConfig(source._id.toString()), + webhook._id.toString(), + ), + ) + .expect(200); + + await authRequest('put', `${ALERTS_BASE_URL}/${created.body.data.id}`) + .send({ + source: AlertSource.TILE, + dashboardId: dashboard._id.toString(), + tileId, + threshold: 100, + interval: '1h', + thresholdType: AlertThresholdType.ABOVE, + channel: { type: 'webhook', webhookId: webhook._id.toString() }, + }) + .expect(200); + + let stored = await Alert.findById(created.body.data.id); + expect(stored!.chartConfig).toBeNull(); + expect(stored!.dashboard?.toString()).toBe(dashboard._id.toString()); + expect(stored!.tileId).toBe(tileId); + + await authRequest('put', `${ALERTS_BASE_URL}/${created.body.data.id}`) + .send( + makeInlineAlertInput( + makeBuilderChartConfig(source._id.toString()), + webhook._id.toString(), + ), + ) + .expect(200); + + stored = await Alert.findById(created.body.data.id); + expect(stored!.chartConfig).toMatchObject({ + source: source._id.toString(), + }); + expect(stored!.dashboard).toBeNull(); + expect(stored!.tileId).toBeNull(); + }); + + it('rejects an inline alert whose source does not exist in the team', async () => { + const webhook = await createTestWebhook(); + await authRequest('post', ALERTS_BASE_URL) + .send( + makeInlineAlertInput( + makeBuilderChartConfig(new ObjectId().toString()), + webhook._id.toString(), + ), + ) + .expect(400); + }); + + it('rejects an inline alert with an unsupported display type', async () => { + const { source } = await makeSource(); + const webhook = await createTestWebhook(); + await authRequest('post', ALERTS_BASE_URL) + .send( + makeInlineAlertInput( + makeBuilderChartConfig(source._id.toString(), { + displayType: 'table', + }), + webhook._id.toString(), + ), + ) + .expect(400); + }); + + it('rejects an inline alert with a PromQL config', async () => { + const webhook = await createTestWebhook(); + await authRequest('post', ALERTS_BASE_URL) + .send( + makeInlineAlertInput( { - aggFn: 'count', - aggCondition: '', - aggConditionLanguage: 'lucene', - valueExpression: '', + configType: 'promql', + promqlQuery: 'up', + sourceId: new ObjectId().toString(), }, - ], - where: '', - whereLanguage: 'lucene', - }, - thresholdType: AlertThresholdType.ABOVE, - channel: { - type: 'webhook', - webhookId: webhook._id.toString(), - }, + webhook._id.toString(), + ), + ) + .expect(400); + }); + + it('validates metric formulas on builder chart configs', async () => { + const { source } = await makeSource(); + const webhook = await createTestWebhook(); + const base = makeBuilderChartConfig(source._id.toString()); + + // Formula referencing a nonexistent series (only A exists) + await authRequest('post', ALERTS_BASE_URL) + .send( + makeInlineAlertInput( + { ...base, formulas: [{ expression: 'B * 2' }] }, + webhook._id.toString(), + ), + ) + .expect(400); + + // Malformed expression + await authRequest('post', ALERTS_BASE_URL) + .send( + makeInlineAlertInput( + { ...base, formulas: [{ expression: 'A +' }] }, + webhook._id.toString(), + ), + ) + .expect(400); + + // Formulas are mutually exclusive with the ratio toggle + await authRequest('post', ALERTS_BASE_URL) + .send( + makeInlineAlertInput( + { + ...base, + asRatio: true, + select: [ + { aggFn: 'count', where: '' }, + { aggFn: 'count', where: 'level:error' }, + ], + formulas: [{ expression: 'A * 2' }], + }, + webhook._id.toString(), + ), + ) + .expect(400); + + const created = await authRequest('post', ALERTS_BASE_URL) + .send( + makeInlineAlertInput( + { ...base, formulas: [{ expression: 'A * 2' }] }, + webhook._id.toString(), + ), + ) + .expect(200); + expect(created.body.data.chartConfig).toMatchObject({ + formulas: [{ expression: 'A * 2' }], + }); + }); + + it('accepts a raw SQL inline alert and validates its template and connection ownership', async () => { + const { connection } = await makeSource(); + const webhook = await createTestWebhook(); + + // Missing the required time-filter/interval macros + await authRequest('post', ALERTS_BASE_URL) + .send( + makeInlineAlertInput( + { + configType: 'sql', + displayType: 'line', + sqlTemplate: 'SELECT 1', + connectionId: connection._id.toString(), + }, + webhook._id.toString(), + ), + ) + .expect(400); + + // A connection outside the team is rejected + await authRequest('post', ALERTS_BASE_URL) + .send( + makeInlineAlertInput( + { + configType: 'sql', + displayType: 'line', + sqlTemplate: RAW_SQL_ALERT_TEMPLATE, + connectionId: new ObjectId().toString(), + }, + webhook._id.toString(), + ), + ) + .expect(400); + + const created = await authRequest('post', ALERTS_BASE_URL) + .send( + makeInlineAlertInput( + { + configType: 'sql', + displayType: 'line', + sqlTemplate: RAW_SQL_ALERT_TEMPLATE, + connectionId: connection._id.toString(), + }, + webhook._id.toString(), + ), + ) + .expect(200); + expect(created.body.data.chartConfig).toMatchObject({ + configType: 'sql', + displayType: 'line', + connectionId: connection._id.toString(), + sqlTemplate: RAW_SQL_ALERT_TEMPLATE, + }); + + // Stored internally with the internal field names + const stored = await Alert.findById(created.body.data.id); + expect(stored!.chartConfig).toMatchObject({ + configType: 'sql', + connection: connection._id.toString(), + }); + }); + + it('rejects a raw SQL inline alert whose source is on a different connection', async () => { + const { connection, source } = await makeSource(); + const webhook = await createTestWebhook(); + const otherConnection = await Connection.create({ + team: team._id, + name: 'Other', + host: 'http://localhost:8124', + username: 'default', + password: '', + }); + + const rawSqlConfig = { + configType: 'sql', + displayType: 'line', + sqlTemplate: RAW_SQL_ALERT_TEMPLATE, + sourceId: source._id.toString(), }; - await authRequest('post', ALERTS_BASE_URL).send(alertInput).expect(400); + // The source belongs to `connection`, not `otherConnection` — the + // worker would execute on one and expand $__sourceTable from the other. + await authRequest('post', ALERTS_BASE_URL) + .send( + makeInlineAlertInput( + { ...rawSqlConfig, connectionId: otherConnection._id.toString() }, + webhook._id.toString(), + ), + ) + .expect(400); + + await authRequest('post', ALERTS_BASE_URL) + .send( + makeInlineAlertInput( + { ...rawSqlConfig, connectionId: connection._id.toString() }, + webhook._id.toString(), + ), + ) + .expect(200); + }); + + it('rejects an inline alert without a chartConfig', async () => { + const webhook = await createTestWebhook(); + await authRequest('post', ALERTS_BASE_URL) + .send( + makeInlineAlertInput(undefined, webhook._id.toString(), { + chartConfig: undefined, + }), + ) + .expect(400); }); + }); + describe('Input validation', () => { describe('webhook validation', () => { it('should reject a non-existent webhook', async () => { const dashboard = await createTestDashboard(); diff --git a/packages/api/src/routers/external-api/v2/alerts.ts b/packages/api/src/routers/external-api/v2/alerts.ts index 693d21d64b..0565301713 100644 --- a/packages/api/src/routers/external-api/v2/alerts.ts +++ b/packages/api/src/routers/external-api/v2/alerts.ts @@ -11,6 +11,8 @@ import { updateAlert, validateAlertInput, } from '@/controllers/alerts'; +import { AlertSource } from '@/models/alert'; +import { convertExternalAlertChartConfigToInternal } from '@/routers/external-api/v2/utils/alertChartConfig'; import { processRequestWithEnhancedErrors as processRequest, validateRequestWithEnhancedErrors as validateRequest, @@ -21,7 +23,29 @@ import { paginationMeta, paginationQuerySchema, } from '@/utils/pagination'; -import { alertSchema, objectIdSchema } from '@/utils/zod'; +import { + alertSchema, + ExternalAlertInput, + InternalAlertInput, + objectIdSchema, +} from '@/utils/zod'; + +/** + * Maps a parsed external alert request body onto the internal alert input + * shape: inline alerts arrive with a chartConfig in the external tile-config + * dialect and are converted to the internal AlertChartConfig the controllers + * and check-alerts task operate on. + */ +function toInternalAlertInput(body: ExternalAlertInput): InternalAlertInput { + if (body.source === AlertSource.INLINE) { + const { chartConfig, ...rest } = body; + return { + ...rest, + chartConfig: convertExternalAlertChartConfigToInternal(chartConfig), + }; + } + return body; +} /** * @openapi @@ -44,8 +68,31 @@ import { alertSchema, objectIdSchema } from '@/utils/zod'; * description: Threshold comparison direction. * AlertSource: * type: string - * enum: [saved_search, tile] - * description: Alert source type. + * enum: [saved_search, tile, inline] + * description: > + * Alert source type. "saved_search" alerts monitor a saved search, + * "tile" alerts monitor a dashboard tile, and "inline" alerts carry + * their own chart configuration (see AlertChartConfig) without + * requiring a saved search or dashboard. + * AlertChartConfig: + * description: > + * The chart configuration an inline alert evaluates, in the same + * dialect as dashboard tile configs. Only the display types the alert + * evaluator supports are accepted: line, stacked_bar, and number, in + * both builder and Raw SQL (configType "sql") variants. Raw SQL + * templates must reference the evaluation window via the time-filter + * and interval macros (e.g. $__timeFilter and $__timeInterval), and + * the referenced source/connection must belong to the team. + * oneOf: + * - $ref: '#/components/schemas/LineChartConfig' + * - $ref: '#/components/schemas/BarChartConfig' + * - $ref: '#/components/schemas/NumberChartConfig' + * discriminator: + * propertyName: displayType + * mapping: + * line: '#/components/schemas/LineChartConfig' + * stacked_bar: '#/components/schemas/BarChartConfig' + * number: '#/components/schemas/NumberChartConfig' * AlertState: * type: string * enum: [ALERT, OK, INSUFFICIENT_DATA, DISABLED, PENDING] @@ -148,6 +195,12 @@ import { alertSchema, objectIdSchema } from '@/utils/zod'; * description: Group-by key for saved search alerts. * nullable: true * example: "ServiceName" + * chartConfig: + * $ref: '#/components/schemas/AlertChartConfig' + * description: > + * Chart configuration for inline alerts. Required when source is + * "inline" and rejected otherwise. Returned on single-alert + * responses (GET by ID, POST, PUT); the list endpoint omits it. * threshold: * type: number * description: Threshold value for triggering the alert. For between and not_between threshold types, this is the lower bound. @@ -395,7 +448,9 @@ router.get( } return res.json({ - data: translateAlertDocumentToExternalAlert(alert), + data: translateAlertDocumentToExternalAlert(alert, { + includeChartConfig: true, + }), }); } catch (e) { next(e); @@ -551,6 +606,24 @@ router.get( * - type: "webhook" * webhookId: "65f5e4a3b9e77c001a789013" * name: "Error Spike Alert" + * inlineAlert: + * summary: Create an inline alert that carries its own chart config + * value: + * source: "inline" + * chartConfig: + * displayType: "line" + * sourceId: "65f5e4a3b9e77c001a123456" + * select: + * - aggFn: "count" + * where: "level:error" + * whereLanguage: "lucene" + * threshold: 100 + * interval: "5m" + * thresholdType: "above" + * channel: + * type: "webhook" + * webhookId: "65f5e4a3b9e77c001a789012" + * name: "Error log spike" * responses: * '200': * description: Successfully created alert @@ -589,13 +662,15 @@ router.post( return res.status(403).json({ message: 'Forbidden' }); } try { - const alertInput = req.body; + const alertInput = toInternalAlertInput(req.body); await validateAlertInput(teamId, alertInput); const createdAlert = await createAlert(teamId, alertInput, userId); return res.json({ - data: translateAlertDocumentToExternalAlert(createdAlert), + data: translateAlertDocumentToExternalAlert(createdAlert, { + includeChartConfig: true, + }), }); } catch (e) { next(e); @@ -689,7 +764,7 @@ router.put( } const { id } = req.params; - const alertInput = req.body; + const alertInput = toInternalAlertInput(req.body); await validateAlertInput(teamId, alertInput); const alert = await updateAlert(id, teamId, alertInput); @@ -699,7 +774,9 @@ router.put( } res.json({ - data: translateAlertDocumentToExternalAlert(alert), + data: translateAlertDocumentToExternalAlert(alert, { + includeChartConfig: true, + }), }); } catch (e) { next(e); diff --git a/packages/api/src/routers/external-api/v2/utils/alertChartConfig.ts b/packages/api/src/routers/external-api/v2/utils/alertChartConfig.ts new file mode 100644 index 0000000000..0fce2e42b4 --- /dev/null +++ b/packages/api/src/routers/external-api/v2/utils/alertChartConfig.ts @@ -0,0 +1,68 @@ +import { + AlertChartConfig, + DisplayType, +} from '@hyperdx/common-utils/dist/types'; +import { omit } from 'lodash'; + +import logger from '@/utils/logger'; +import { ExternalAlertChartConfig } from '@/utils/zod'; + +import { + convertToExternalTileChartConfig, + convertToInternalTileConfig, +} from './dashboards'; + +/** + * Converts an inline alert's external-dialect chart config (the tile-config + * dialect v2 dashboards use: `sourceId`, per-select `where`, `asRatio`, + * `connectionId` + `sqlTemplate`) into the internal AlertChartConfig shape the + * alert document persists and the check-alerts task evaluates. + * + * Reuses the dashboard tile converter through a synthetic tile so the two + * surfaces cannot drift on field mapping (`where` -> `aggCondition`, + * `asRatio` -> `seriesReturnType`, `connectionId` -> `connection`, fillNulls + * defaulting, ...). Layout fields are placeholders — only `config` is kept. + */ +export function convertExternalAlertChartConfigToInternal( + external: ExternalAlertChartConfig, +): AlertChartConfig { + const { config } = convertToInternalTileConfig({ + id: '', + name: '', + x: 0, + y: 0, + w: 1, + h: 1, + config: external, + }); + // The tile converter writes the synthetic tile's (empty) name into the + // config and never emits a PromQL variant or an embedded `alert` for the + // display types the alert schema admits, so stripping `name`/`alert` yields + // a valid AlertChartConfig. + return omit(config, ['name', 'alert']) as AlertChartConfig; +} + +/** + * Converts an inline alert's persisted internal chart config to the external + * tile-config dialect for API responses. Returns undefined for configs the + * external contract cannot represent (a corrupt document with an unsupported + * display type or a PromQL config) — callers omit the field rather than + * emitting an invalid shape. + */ +export function convertAlertChartConfigToExternal( + config: AlertChartConfig, +): ExternalAlertChartConfig | undefined { + const external = convertToExternalTileChartConfig(config); + switch (external?.displayType) { + case DisplayType.Line: + case DisplayType.StackedBar: + case DisplayType.Number: + return external; + default: + logger.error( + { config }, + 'Inline alert chart config has no external representation', + ); + return undefined; + } +} diff --git a/packages/api/src/routers/external-api/v2/utils/dashboards.ts b/packages/api/src/routers/external-api/v2/utils/dashboards.ts index ac479f5069..8a430e3f5f 100644 --- a/packages/api/src/routers/external-api/v2/utils/dashboards.ts +++ b/packages/api/src/routers/external-api/v2/utils/dashboards.ts @@ -197,7 +197,9 @@ const toExternalColorRules = ( return resolved.length > 0 ? resolved : undefined; }; -const convertToExternalTileChartConfig = ( +// Exported for the inline-alert converter (v2/utils/alertChartConfig.ts), +// which reuses the tile-config translation for an alert's persisted config. +export const convertToExternalTileChartConfig = ( config: SavedChartConfig, ): ExternalDashboardTileConfig | undefined => { if (isRawSqlSavedChartConfig(config)) { diff --git a/packages/api/src/utils/__tests__/externalApi.test.ts b/packages/api/src/utils/__tests__/externalApi.test.ts index 0b1f61aff6..b8456c8347 100644 --- a/packages/api/src/utils/__tests__/externalApi.test.ts +++ b/packages/api/src/utils/__tests__/externalApi.test.ts @@ -105,6 +105,117 @@ describe('utils/externalApi', () => { }); }); + describe('chartConfig handling', () => { + const internalChartConfig = { + displayType: 'line', + source: '65f5e4a3b9e77c001a123456', + select: [ + { + aggFn: 'count', + aggCondition: 'level:error', + aggConditionLanguage: 'lucene', + valueExpression: '', + }, + ], + where: '', + whereLanguage: 'lucene', + seriesReturnType: 'ratio', + }; + + it('omits chartConfig by default (list responses stay lean)', () => { + const alert = createAlertDocument({ + source: AlertSource.INLINE, + chartConfig: internalChartConfig, + }); + + const translated = translateAlertDocumentToExternalAlert(alert); + + expect(translated.chartConfig).toBeUndefined(); + expect('chartConfig' in translated).toBe(false); + }); + + it('emits chartConfig in the external tile-config dialect when requested', () => { + const alert = createAlertDocument({ + source: AlertSource.INLINE, + chartConfig: internalChartConfig, + }); + + const translated = translateAlertDocumentToExternalAlert(alert, { + includeChartConfig: true, + }); + + // seriesReturnType 'ratio' becomes asRatio only with exactly two + // select items — this single-select config maps to false. + expect(translated.chartConfig).toMatchObject({ + displayType: 'line', + sourceId: '65f5e4a3b9e77c001a123456', + asRatio: false, + select: [ + { + aggFn: 'count', + where: 'level:error', + whereLanguage: 'lucene', + }, + ], + }); + }); + + it('emits raw SQL chartConfig with external field names', () => { + const alert = createAlertDocument({ + source: AlertSource.INLINE, + chartConfig: { + configType: 'sql', + displayType: 'line', + sqlTemplate: 'SELECT 1', + connection: '65f5e4a3b9e77c001a789012', + source: '65f5e4a3b9e77c001a123456', + }, + }); + + const translated = translateAlertDocumentToExternalAlert(alert, { + includeChartConfig: true, + }); + + expect(translated.chartConfig).toMatchObject({ + configType: 'sql', + displayType: 'line', + sqlTemplate: 'SELECT 1', + connectionId: '65f5e4a3b9e77c001a789012', + sourceId: '65f5e4a3b9e77c001a123456', + }); + }); + + it('does not emit chartConfig for non-inline alerts even when requested', () => { + const alert = createAlertDocument({ + source: AlertSource.TILE, + chartConfig: internalChartConfig, + }); + + const translated = translateAlertDocumentToExternalAlert(alert, { + includeChartConfig: true, + }); + + expect(translated.chartConfig).toBeUndefined(); + }); + + it('omits chartConfig when the persisted config has no external representation', () => { + const alert = createAlertDocument({ + source: AlertSource.INLINE, + chartConfig: { + configType: 'promql', + promqlQuery: 'up', + source: '65f5e4a3b9e77c001a123456', + }, + }); + + const translated = translateAlertDocumentToExternalAlert(alert, { + includeChartConfig: true, + }); + + expect(translated.chartConfig).toBeUndefined(); + }); + }); + describe('channel mirroring', () => { // translateAlertDocumentToExternalAlert mirrors channels[0] into // `channel`, same as makeAlert (controllers/__tests__/alerts.test.ts). diff --git a/packages/api/src/utils/externalApi.ts b/packages/api/src/utils/externalApi.ts index fcfa710060..1b0810166d 100644 --- a/packages/api/src/utils/externalApi.ts +++ b/packages/api/src/utils/externalApi.ts @@ -18,13 +18,18 @@ import { AlertChannel, AlertDocument, AlertInterval, + AlertSource, AlertState, getAlertChannels, IAlert, } from '@/models/alert'; import type { DashboardDocument } from '@/models/dashboard'; +import { convertAlertChartConfigToExternal } from '@/routers/external-api/v2/utils/alertChartConfig'; import { SeriesTile } from '@/routers/external-api/v2/utils/dashboards'; -import { ExternalDashboardFilterWithId } from '@/utils/zod'; +import { + ExternalAlertChartConfig, + ExternalDashboardFilterWithId, +} from '@/utils/zod'; /** Returns a new object containing only the truthy, requested keys from the original object */ const pickIfTruthy = (obj: T, keys: K[]): Partial => { @@ -267,6 +272,12 @@ export type ExternalAlert = { dashboardId?: string; savedSearchId?: string; groupBy?: string; + /** + * Inline alerts only, and only on single-alert responses (the list endpoint + * stays lean): the alert's persisted chart config in the external + * tile-config dialect. + */ + chartConfig?: ExternalAlertChartConfig; silenced?: { by?: string; at: string; @@ -340,6 +351,17 @@ function transformErrorsToExternalErrors( export function translateAlertDocumentToExternalAlert( alert: AlertDocument, + { + includeChartConfig = false, + }: { + /** + * Attach an inline alert's chartConfig (external dialect). Single-alert + * responses (GET by id, POST, PUT, MCP detail) opt in; list responses + * leave it off so a team with hundreds of raw-SQL inline alerts does not + * ship every template on each page. + */ + includeChartConfig?: boolean; + } = {}, ): ExternalAlert { // Convert to plain object if it's a Mongoose document const alertObj: AlertDocumentObject = alert.toJSON @@ -348,6 +370,13 @@ export function translateAlertDocumentToExternalAlert( const channels = getAlertChannels(alertObj); + const externalChartConfig = + includeChartConfig && + alertObj.source === AlertSource.INLINE && + alertObj.chartConfig != null + ? convertAlertChartConfigToExternal(alertObj.chartConfig) + : undefined; + // Copy all fields, renaming _id to id, ensuring ObjectId's are strings const result = { id: alertObj._id.toString(), @@ -376,6 +405,7 @@ export function translateAlertDocumentToExternalAlert( dashboardId: alertObj.dashboard?.toString(), savedSearchId: alertObj.savedSearch?.toString(), groupBy: alertObj.groupBy ?? undefined, + ...(externalChartConfig && { chartConfig: externalChartConfig }), silenced: transformSilencedToExternalSilenced(alertObj.silenced), executionErrors: transformErrorsToExternalErrors(alertObj.executionErrors), createdAt: hasCreatedAt(alertObj) diff --git a/packages/api/src/utils/zod.ts b/packages/api/src/utils/zod.ts index 18685d6956..db4cfe00ea 100644 --- a/packages/api/src/utils/zod.ts +++ b/packages/api/src/utils/zod.ts @@ -789,6 +789,110 @@ const validateInlineAlertFormulas = ( ); }; +// Inline-alert chart configs on the external surface use the same external +// tile-config dialect v2 dashboards use (`sourceId`, per-select `where`, +// `asRatio`, `connectionId` + `sqlTemplate`), restricted to the display types +// the alert task can evaluate as a time series: Line, Stacked Bar, and +// Number. Builder + raw SQL variants; no PromQL (PromQL charts cannot be +// alerted on). +const externalAlertBuilderChartConfigSchema = z.discriminatedUnion( + 'displayType', + [ + externalDashboardLineChartConfigSchema, + externalDashboardBarChartConfigSchema, + externalDashboardNumberChartConfigSchema, + ], +); + +const externalAlertRawSqlChartConfigSchema = z.discriminatedUnion( + 'displayType', + [ + externalDashboardLineRawSqlChartConfigSchema, + externalDashboardBarRawSqlChartConfigSchema, + externalDashboardNumberRawSqlChartConfigSchema, + ], +); + +export type ExternalAlertChartConfig = + | z.infer + | z.infer; + +// Mirrors the route-by-configType superRefine/transform pattern of +// externalDashboardTileConfigSchema above, so inline alert configs get the +// same targeted field-level errors, formula validation, and unknown-field +// stripping as dashboard tile configs. +export const externalAlertChartConfigSchema = z + .custom() + .superRefine((data, ctx) => { + const schema = + data !== null && + typeof data === 'object' && + 'configType' in data && + data.configType === 'sql' + ? externalAlertRawSqlChartConfigSchema + : externalAlertBuilderChartConfigSchema; + + const result = schema.safeParse(data); + if (!result.success) { + for (const issue of result.error.issues) { + ctx.addIssue(issue); + } + return; + } + + if ( + 'asRatio' in data && + data.asRatio && + (!Array.isArray(data.select) || data.select.length !== 2) + ) { + ctx.addIssue({ + code: z.ZodIssueCode.custom, + message: 'asRatio can only be used with exactly two select items', + }); + } + + if (!('configType' in data)) { + // Same shared metric-formula rules the tile-config refinement applies: + // expression parse + series-ref range checks, asRatio mutual exclusion, + // and the number single-formula cap. + validateChartConfigFormulas(data, ctx); + + // A number chart without formulas displays its single select item; the + // relaxed `.min(1).max(20)` on the number schema exists only so formula + // operands fit. + if ( + data.displayType === 'number' && + !('formulas' in data && (data.formulas?.length ?? 0) > 0) && + Array.isArray(data.select) && + data.select.length !== 1 + ) { + ctx.addIssue({ + code: z.ZodIssueCode.custom, + message: + 'Number charts support a single select item unless formulas are used (extra items are formula operands)', + path: ['select'], + }); + } + } + }) + .transform(data => { + // Re-parse through the appropriate sub-schema to strip unknown fields. + // Safe to call .parse() here — superRefine already validated the data. + const schema = + data !== null && + typeof data === 'object' && + 'configType' in data && + data.configType === 'sql' + ? externalAlertRawSqlChartConfigSchema + : externalAlertBuilderChartConfigSchema; + return schema.parse(data); + }); + +const zExternalInlineAlert = z.object({ + source: z.literal(AlertSource.INLINE), + chartConfig: externalAlertChartConfigSchema, +}); + const alertBaseSchema = z.object({ channel: zAlertChannel.optional(), channels: zAlertChannels.optional(), @@ -805,16 +909,22 @@ const alertBaseSchema = z.object({ numConsecutiveWindows: z.number().int().min(1).nullish(), }); +// External v2 alert schema. Inline alerts carry their chart config in the +// external tile-config dialect (see externalAlertChartConfigSchema); the +// router converts it to the internal AlertChartConfig shape before +// validateAlertInput/createAlert. export const alertSchema = alertBaseSchema - .and(zSavedSearchAlert.or(zTileAlert)) + .and(zSavedSearchAlert.or(zTileAlert).or(zExternalInlineAlert)) .superRefine(validateAlertChannelSelection) .superRefine(validateAlertScheduleOffsetMinutes) .superRefine(validateAlertThresholdMax); -// Superset of alertSchema for the internal API only: also accepts inline -// alerts (source: 'inline'), which persist their own chart config. The -// external v2 API keeps the narrower alertSchema until its contract (OpenAPI -// docs, Terraform provider) is extended to cover inline alerts. +export type ExternalAlertInput = z.infer; + +// Internal-API variant: inline alerts carry the persisted internal +// AlertChartConfig shape directly (per-select `aggCondition`, +// `seriesReturnType: 'ratio'`, `source`/`connection`) instead of the external +// dialect above. export const internalAlertSchema = alertBaseSchema .and(zSavedSearchAlert.or(zTileAlert).or(zInlineAlert)) .superRefine(validateAlertChannelSelection) @@ -822,6 +932,8 @@ export const internalAlertSchema = alertBaseSchema .superRefine(validateAlertThresholdMax) .superRefine(validateInlineAlertFormulas); +export type InternalAlertInput = z.infer; + // ============================== // Webhooks // ============================== diff --git a/packages/common-utils/src/__tests__/iac.test.ts b/packages/common-utils/src/__tests__/iac.test.ts index 7133e74b4a..a152fe6ecc 100644 --- a/packages/common-utils/src/__tests__/iac.test.ts +++ b/packages/common-utils/src/__tests__/iac.test.ts @@ -76,6 +76,12 @@ describe('isImportableAlert', () => { expect(isImportableAlert({ source: 'tile' })).toBe(false); }); + // The provider's alert resource does not model a chartConfig, so inline + // alerts stay excluded even though external API v2 can create/read them. + it('rejects an inline alert', () => { + expect(isImportableAlert({ source: 'inline' })).toBe(false); + }); + it('rejects an alert with no source and no saved search', () => { expect(isImportableAlert({})).toBe(false); }); diff --git a/packages/common-utils/src/iacEligibility.ts b/packages/common-utils/src/iacEligibility.ts index 9694105297..e975a29b0e 100644 --- a/packages/common-utils/src/iacEligibility.ts +++ b/packages/common-utils/src/iacEligibility.ts @@ -10,8 +10,11 @@ import { AlertSource, DisplayType, SourceKind } from './types'; /** * The provider models only saved-search alerts — a tile alert has no * corresponding resource, so offering one for import produces a command that - * fails. Shared by the bulk export and the per-alert popover so the two cannot - * disagree about what is eligible. + * fails. Inline alerts (source: 'inline') are excluded for the same reason: + * although the external API v2 can now create/read them, the provider's alert + * resource does not model a chartConfig yet, so importing one would generate + * a config the provider cannot apply. Shared by the bulk export and the + * per-alert popover so the two cannot disagree about what is eligible. */ export function isImportableAlert(alert: { source?: string; From 3dab96ce452301ea6722af53aa81c86f4692fe0b Mon Sep 17 00:00:00 2001 From: Warren Lee <5959690+wrn14897@users.noreply.github.com> Date: Tue, 1 Sep 2026 08:37:55 -0700 Subject: [PATCH 2/8] test: harden inline chart alert coverage (HDX-5191) - Unit tests for the alert chart config converters: external <-> internal round-trip fidelity (metric select items, asRatio, seriesLimit, fillNulls, numberFormat), synthetic tile name/alert stripping, and the undefined fallback for corrupt display types - External v2: number single-select rule (with and without formulas), asRatio two-select rule, GET -> PUT echo round-trip (the Terraform provider flow), unknown chartConfig fields stripped not persisted - MCP: raw SQL inline alert create + stored internal dialect, raw SQL displayType allowlist rejection at the tool input schema, slim get_alert list entries for inline alerts - translate: inline alert with a missing chartConfig does not throw --- .../api/src/mcp/__tests__/alerts.int.test.ts | 69 ++++++ .../external-api/__tests__/alerts.int.test.ts | 130 ++++++++++ .../utils/__tests__/alertChartConfig.test.ts | 224 ++++++++++++++++++ .../src/utils/__tests__/externalApi.test.ts | 16 ++ 4 files changed, 439 insertions(+) create mode 100644 packages/api/src/routers/external-api/v2/utils/__tests__/alertChartConfig.test.ts diff --git a/packages/api/src/mcp/__tests__/alerts.int.test.ts b/packages/api/src/mcp/__tests__/alerts.int.test.ts index aff5a0f305..0582ac11e9 100644 --- a/packages/api/src/mcp/__tests__/alerts.int.test.ts +++ b/packages/api/src/mcp/__tests__/alerts.int.test.ts @@ -11,6 +11,7 @@ import { DEFAULT_TRACES_TABLE, getLoggedInAgent, getServer, + RAW_SQL_ALERT_TEMPLATE, } from '@/fixtures'; import { McpContext } from '@/mcp/tools/types'; import Alert, { AlertSource, AlertState } from '@/models/alert'; @@ -679,6 +680,74 @@ describe('MCP Alert Tools', () => { expect(getFirstText(result)).toContain('Source not found'); }); + it('should create a raw SQL inline alert and validate the displayType allowlist', async () => { + const webhook = await createTestWebhook(); + + // Raw SQL charts allow alertable display types only: the tool's own + // input schema narrows displayType, so 'table' is rejected by MCP + // argument validation before the handler runs. + const rejected = await callTool( + client, + 'clickstack_save_alert', + makeInlineInput(webhook._id.toString(), { + chartConfig: { + configType: 'sql', + displayType: 'table', + connectionId: connection._id.toString(), + sqlTemplate: RAW_SQL_ALERT_TEMPLATE, + }, + }), + ); + expect(rejected.isError).toBe(true); + expect(getFirstText(rejected)).toContain('displayType'); + + const result = await callTool( + client, + 'clickstack_save_alert', + makeInlineInput(webhook._id.toString(), { + chartConfig: { + configType: 'sql', + displayType: 'line', + connectionId: connection._id.toString(), + sqlTemplate: RAW_SQL_ALERT_TEMPLATE, + }, + }), + ); + + expect(result.isError).toBeFalsy(); + const output = JSON.parse(getFirstText(result)); + expect(output.chartConfig).toMatchObject({ + configType: 'sql', + displayType: 'line', + connectionId: connection._id.toString(), + sqlTemplate: RAW_SQL_ALERT_TEMPLATE, + }); + + // Stored with the internal field names. + const stored = await Alert.findById(output.id); + expect(stored!.chartConfig).toMatchObject({ + configType: 'sql', + connection: connection._id.toString(), + }); + }); + + it('clickstack_get_alert list entries stay slim for inline alerts', async () => { + const webhook = await createTestWebhook(); + await callTool( + client, + 'clickstack_save_alert', + makeInlineInput(webhook._id.toString()), + ); + + const list = await callTool(client, 'clickstack_get_alert', {}); + expect(list.isError).toBeFalsy(); + const output = JSON.parse(getFirstText(list)); + expect(output).toHaveLength(1); + expect(output[0].source).toBe('inline'); + expect(output[0].name).toBe('Inline MCP Alert'); + expect(output[0].chartConfig).toBeUndefined(); + }); + it('clickstack_get_alert detail includes chartConfig and falls back to its name', async () => { const webhook = await createTestWebhook(); diff --git a/packages/api/src/routers/external-api/__tests__/alerts.int.test.ts b/packages/api/src/routers/external-api/__tests__/alerts.int.test.ts index e04aed5b1b..afba066453 100644 --- a/packages/api/src/routers/external-api/__tests__/alerts.int.test.ts +++ b/packages/api/src/routers/external-api/__tests__/alerts.int.test.ts @@ -1080,6 +1080,136 @@ describe('External API Alerts', () => { ) .expect(400); }); + + it('accepts a number chart and enforces the single-select rule', async () => { + const { source } = await makeSource(); + const webhook = await createTestWebhook(); + + const numberConfig = { + displayType: 'number', + sourceId: source._id.toString(), + select: [{ aggFn: 'count', where: 'level:error' }], + }; + + const created = await authRequest('post', ALERTS_BASE_URL) + .send(makeInlineAlertInput(numberConfig, webhook._id.toString())) + .expect(200); + expect(created.body.data.chartConfig).toMatchObject({ + displayType: 'number', + sourceId: source._id.toString(), + }); + + // A number chart without formulas displays a single select item; + // extra items are only valid as formula operands. + await authRequest('post', ALERTS_BASE_URL) + .send( + makeInlineAlertInput( + { + ...numberConfig, + select: [ + { aggFn: 'count', where: 'level:error' }, + { aggFn: 'count', where: '' }, + ], + }, + webhook._id.toString(), + ), + ) + .expect(400); + + // With a formula, the extra select items are its operands. + const withFormula = await authRequest('post', ALERTS_BASE_URL) + .send( + makeInlineAlertInput( + { + ...numberConfig, + select: [ + { aggFn: 'count', where: 'level:error' }, + { aggFn: 'count', where: '' }, + ], + formulas: [{ expression: 'A / B * 100' }], + }, + webhook._id.toString(), + ), + ) + .expect(200); + expect(withFormula.body.data.chartConfig).toMatchObject({ + formulas: [{ expression: 'A / B * 100' }], + }); + }); + + it('rejects asRatio without exactly two select items', async () => { + const { source } = await makeSource(); + const webhook = await createTestWebhook(); + + await authRequest('post', ALERTS_BASE_URL) + .send( + makeInlineAlertInput( + makeBuilderChartConfig(source._id.toString(), { asRatio: true }), + webhook._id.toString(), + ), + ) + .expect(400); + }); + + it('round-trips a single-alert GET body back through PUT unchanged', async () => { + // The Terraform-provider flow: read an alert, generate config from the + // response, apply it back. The echoed body must be valid PUT input and + // must not mutate the stored chart config. + const { source } = await makeSource(); + const webhook = await createTestWebhook(); + + const created = await authRequest('post', ALERTS_BASE_URL) + .send( + makeInlineAlertInput( + makeBuilderChartConfig(source._id.toString(), { + groupBy: 'ServiceName', + }), + webhook._id.toString(), + { name: 'Echo Test' }, + ), + ) + .expect(200); + const storedBefore = await Alert.findById(created.body.data.id); + + const single = await authRequest( + 'get', + `${ALERTS_BASE_URL}/${created.body.data.id}`, + ).expect(200); + + const echoed = await authRequest( + 'put', + `${ALERTS_BASE_URL}/${created.body.data.id}`, + ) + .send(single.body.data) + .expect(200); + expect(echoed.body.data.chartConfig).toEqual( + single.body.data.chartConfig, + ); + + const storedAfter = await Alert.findById(created.body.data.id); + expect(storedAfter!.chartConfig).toEqual(storedBefore!.chartConfig); + }); + + it('strips unknown chartConfig fields instead of persisting them', async () => { + const { source } = await makeSource(); + const webhook = await createTestWebhook(); + + const created = await authRequest('post', ALERTS_BASE_URL) + .send( + makeInlineAlertInput( + { + ...makeBuilderChartConfig(source._id.toString()), + unknownField: 'dropped', + }, + webhook._id.toString(), + ), + ) + .expect(200); + + expect(created.body.data.chartConfig).not.toHaveProperty('unknownField'); + const stored = await Alert.findById(created.body.data.id); + expect(stored!.chartConfig).not.toHaveProperty('unknownField'); + }); }); describe('Input validation', () => { diff --git a/packages/api/src/routers/external-api/v2/utils/__tests__/alertChartConfig.test.ts b/packages/api/src/routers/external-api/v2/utils/__tests__/alertChartConfig.test.ts new file mode 100644 index 0000000000..56d311544e --- /dev/null +++ b/packages/api/src/routers/external-api/v2/utils/__tests__/alertChartConfig.test.ts @@ -0,0 +1,224 @@ +import { DisplayType, MetricsDataType } from '@hyperdx/common-utils/dist/types'; + +import { + convertAlertChartConfigToExternal, + convertExternalAlertChartConfigToInternal, +} from '@/routers/external-api/v2/utils/alertChartConfig'; +import { ExternalAlertChartConfig } from '@/utils/zod'; + +describe('alertChartConfig converters', () => { + describe('builder configs', () => { + const externalLineConfig: ExternalAlertChartConfig = { + displayType: 'line', + sourceId: '65f5e4a3b9e77c001a123456', + select: [ + { + aggFn: 'count', + where: 'level:error', + whereLanguage: 'lucene', + alias: 'Errors', + }, + { + aggFn: 'avg', + valueExpression: 'Value', + where: '', + whereLanguage: 'lucene', + metricType: MetricsDataType.Gauge, + metricName: 'system.cpu.utilization', + periodAggFn: 'delta', + }, + ], + groupBy: 'ServiceName', + asRatio: true, + fillNulls: false, + seriesLimit: 5, + numberFormat: { output: 'number', mantissa: 2 }, + }; + + it('maps the external dialect onto the internal AlertChartConfig', () => { + const internal = + convertExternalAlertChartConfigToInternal(externalLineConfig); + + expect(internal).toMatchObject({ + displayType: DisplayType.Line, + source: '65f5e4a3b9e77c001a123456', + groupBy: 'ServiceName', + seriesReturnType: 'ratio', + fillNulls: false, + seriesLimit: 5, + select: [ + { + aggFn: 'count', + aggCondition: 'level:error', + aggConditionLanguage: 'lucene', + alias: 'Errors', + isDelta: false, + }, + { + aggFn: 'avg', + valueExpression: 'Value', + aggCondition: '', + aggConditionLanguage: 'lucene', + metricType: MetricsDataType.Gauge, + metricName: 'system.cpu.utilization', + isDelta: true, + }, + ], + }); + }); + + it('never persists the synthetic tile name or an embedded alert', () => { + const internal = + convertExternalAlertChartConfigToInternal(externalLineConfig); + + // The tile converter writes the synthetic tile's name ('') into the + // config; persisting it would attach junk to every alert document. + expect(internal).not.toHaveProperty('name'); + expect(internal).not.toHaveProperty('alert'); + }); + + it('round-trips external -> internal -> external without losing fields', () => { + const internal = + convertExternalAlertChartConfigToInternal(externalLineConfig); + const roundTripped = convertAlertChartConfigToExternal(internal); + + expect(roundTripped).toMatchObject({ + displayType: 'line', + sourceId: '65f5e4a3b9e77c001a123456', + groupBy: 'ServiceName', + asRatio: true, + fillNulls: false, + seriesLimit: 5, + numberFormat: { output: 'number', mantissa: 2 }, + select: [ + { + aggFn: 'count', + where: 'level:error', + whereLanguage: 'lucene', + alias: 'Errors', + }, + { + aggFn: 'avg', + valueExpression: 'Value', + where: '', + whereLanguage: 'lucene', + metricType: MetricsDataType.Gauge, + metricName: 'system.cpu.utilization', + periodAggFn: 'delta', + }, + ], + }); + }); + + it('converts internal stacked_bar and number configs to the external dialect', () => { + const stackedBar = convertAlertChartConfigToExternal({ + displayType: DisplayType.StackedBar, + source: '65f5e4a3b9e77c001a123456', + select: [ + { + aggFn: 'count', + aggCondition: '', + aggConditionLanguage: 'lucene', + valueExpression: '', + }, + ], + where: '', + }); + expect(stackedBar).toMatchObject({ + displayType: 'stacked_bar', + sourceId: '65f5e4a3b9e77c001a123456', + }); + + const number = convertAlertChartConfigToExternal({ + displayType: DisplayType.Number, + source: '65f5e4a3b9e77c001a123456', + select: [ + { + aggFn: 'count', + aggCondition: 'level:error', + aggConditionLanguage: 'lucene', + valueExpression: '', + }, + ], + where: '', + numberFormat: { output: 'percent' }, + }); + expect(number).toMatchObject({ + displayType: 'number', + sourceId: '65f5e4a3b9e77c001a123456', + numberFormat: { output: 'percent' }, + select: [{ aggFn: 'count', where: 'level:error' }], + }); + }); + }); + + describe('raw SQL configs', () => { + const externalRawSqlConfig: ExternalAlertChartConfig = { + configType: 'sql', + displayType: 'line', + connectionId: '65f5e4a3b9e77c001a789012', + sqlTemplate: + 'SELECT $__timeInterval(Timestamp) AS ts, count() FROM t WHERE $__timeFilter(Timestamp) GROUP BY ts', + sourceId: '65f5e4a3b9e77c001a123456', + fillNulls: false, + seriesLimit: 3, + }; + + it('maps external field names onto the internal ones', () => { + const internal = + convertExternalAlertChartConfigToInternal(externalRawSqlConfig); + + expect(internal).toMatchObject({ + configType: 'sql', + displayType: DisplayType.Line, + connection: '65f5e4a3b9e77c001a789012', + source: '65f5e4a3b9e77c001a123456', + fillNulls: false, + seriesLimit: 3, + }); + expect(internal).not.toHaveProperty('connectionId'); + expect(internal).not.toHaveProperty('sourceId'); + expect(internal).not.toHaveProperty('name'); + }); + + it('round-trips external -> internal -> external', () => { + const internal = + convertExternalAlertChartConfigToInternal(externalRawSqlConfig); + const roundTripped = convertAlertChartConfigToExternal(internal); + + expect(roundTripped).toMatchObject({ + configType: 'sql', + displayType: 'line', + connectionId: '65f5e4a3b9e77c001a789012', + sqlTemplate: externalRawSqlConfig.sqlTemplate, + sourceId: '65f5e4a3b9e77c001a123456', + fillNulls: false, + seriesLimit: 3, + }); + }); + }); + + describe('configs without an external representation', () => { + it('returns undefined for a corrupt config with an unsupported display type', () => { + // Reachable only via a direct DB write: both write paths reject + // non-alertable display types, but a Mixed Mongo field enforces + // nothing. The translate layer omits the field rather than emitting + // a shape the external contract cannot express. + const external = convertAlertChartConfigToExternal({ + displayType: DisplayType.Table, + source: '65f5e4a3b9e77c001a123456', + select: [ + { + aggFn: 'count', + aggCondition: '', + aggConditionLanguage: 'lucene', + valueExpression: '', + }, + ], + where: '', + }); + + expect(external).toBeUndefined(); + }); + }); +}); diff --git a/packages/api/src/utils/__tests__/externalApi.test.ts b/packages/api/src/utils/__tests__/externalApi.test.ts index b8456c8347..8dddca617b 100644 --- a/packages/api/src/utils/__tests__/externalApi.test.ts +++ b/packages/api/src/utils/__tests__/externalApi.test.ts @@ -198,6 +198,22 @@ describe('utils/externalApi', () => { expect(translated.chartConfig).toBeUndefined(); }); + it('does not throw on an inline alert missing its chartConfig', () => { + // Reachable via a direct DB write or a partial legacy document; the + // translate layer must not crash a whole list response over one row. + const alert = createAlertDocument({ + source: AlertSource.INLINE, + chartConfig: null, + }); + + const translated = translateAlertDocumentToExternalAlert(alert, { + includeChartConfig: true, + }); + + expect(translated.chartConfig).toBeUndefined(); + expect('chartConfig' in translated).toBe(false); + }); + it('omits chartConfig when the persisted config has no external representation', () => { const alert = createAlertDocument({ source: AlertSource.INLINE, From 339f7184ec9380214f143d43b90144fe983568de Mon Sep 17 00:00:00 2001 From: Warren Lee <5959690+wrn14897@users.noreply.github.com> Date: Tue, 1 Sep 2026 16:31:00 -0700 Subject: [PATCH 3/8] fix: address inline chart alert review findings (HDX-5191) - Reject chartConfig on tile/saved_search alerts (external v2 + MCP) instead of silently dropping it - Expose the config name and builder chart-level where/whereLanguage in the external alert dialect; raw SQL configs reject where - Preserve the gauge isDelta flag: convertToInternalSelectItem honors both spellings (also fixes v2 dashboards dropping it), the MCP tile select item accepts the periodAggFn alias, and MCP saveAlert maps isDelta onto the external dialect - Refuse to emit an external chartConfig for internally-authored configs the dialect cannot express (filters, having, orderBy, ...) so a GET -> PUT loop fails loudly instead of silently broadening the alert --- .changeset/inline-alerts-external-api-mcp.md | 8 +- packages/api/openapi.json | 65 ++- .../api/src/mcp/__tests__/alerts.int.test.ts | 90 +++- .../tools/alerts/__tests__/schemas.test.ts | 41 ++ .../api/src/mcp/tools/alerts/saveAlert.ts | 22 +- packages/api/src/mcp/tools/alerts/schemas.ts | 54 +- .../api/src/mcp/tools/dashboards/schemas.ts | 35 +- .../external-api/__tests__/alerts.int.test.ts | 205 ++++++++ .../api/src/routers/external-api/v2/alerts.ts | 62 ++- .../utils/__tests__/alertChartConfig.test.ts | 474 +++++++++++------- .../external-api/v2/utils/alertChartConfig.ts | 280 ++++++++++- .../external-api/v2/utils/dashboards.ts | 9 +- packages/api/src/utils/zod.ts | 87 +++- 13 files changed, 1195 insertions(+), 237 deletions(-) diff --git a/.changeset/inline-alerts-external-api-mcp.md b/.changeset/inline-alerts-external-api-mcp.md index b5d0aa0374..541bd149a3 100644 --- a/.changeset/inline-alerts-external-api-mcp.md +++ b/.changeset/inline-alerts-external-api-mcp.md @@ -8,4 +8,10 @@ Inline alerts can now be created, updated, listed, and deleted through `/api/v2/alerts` using the same tile-config dialect as v2 dashboards, with the same validation rules as the internal API (display-type allowlist, metric formula validation, raw SQL template validation, and team-scoped -source/connection ownership). +source/connection ownership). Passing a `chartConfig` to a `tile` or +`saved_search` alert is now rejected instead of silently dropped, and reading +an inline alert whose config carries internal-only fields omits `chartConfig` +rather than returning a lossy approximation. + +Also fixes the external v2 dashboards dialect dropping the gauge `isDelta` +flag when converting tile select items to the internal shape. diff --git a/packages/api/openapi.json b/packages/api/openapi.json index 647f143b97..9959031da1 100644 --- a/packages/api/openapi.json +++ b/packages/api/openapi.json @@ -97,27 +97,70 @@ ], "description": "Alert source type. \"saved_search\" alerts monitor a saved search, \"tile\" alerts monitor a dashboard tile, and \"inline\" alerts carry their own chart configuration (see AlertChartConfig) without requiring a saved search or dashboard.\n" }, - "AlertChartConfig": { - "description": "The chart configuration an inline alert evaluates, in the same dialect as dashboard tile configs. Only the display types the alert evaluator supports are accepted: line, stacked_bar, and number, in both builder and Raw SQL (configType \"sql\") variants. Raw SQL templates must reference the evaluation window via the time-filter and interval macros (e.g. $__timeFilter and $__timeInterval), and the referenced source/connection must belong to the team.\n", - "oneOf": [ + "AlertChartConfigOverlay": { + "type": "object", + "description": "Fields an alert's chart config carries on top of the dashboard tile config dialect. Unlike a tile (whose name lives on the tile, not its config), an inline alert's config is standalone.\n", + "properties": { + "name": { + "type": "string", + "description": "Display name for the alert query. Used in notification titles and as an alert-name fallback when the alert itself has no name.\n", + "example": "Error Rate Query" + }, + "where": { + "type": "string", + "maxLength": 10000, + "description": "Chart-level filter applied on top of every select item's own \"where\" (combined via AND). Builder variants only; rejected on Raw SQL variants (filter inside the sqlTemplate instead).\n", + "example": "ServiceName:api" + }, + "whereLanguage": { + "$ref": "#/components/schemas/QueryLanguage", + "description": "Language of the chart-level \"where\" filter." + } + } + }, + "AlertLineChartConfig": { + "allOf": [ { "$ref": "#/components/schemas/LineChartConfig" }, + { + "$ref": "#/components/schemas/AlertChartConfigOverlay" + } + ] + }, + "AlertBarChartConfig": { + "allOf": [ { "$ref": "#/components/schemas/BarChartConfig" }, + { + "$ref": "#/components/schemas/AlertChartConfigOverlay" + } + ] + }, + "AlertNumberChartConfig": { + "allOf": [ { "$ref": "#/components/schemas/NumberChartConfig" + }, + { + "$ref": "#/components/schemas/AlertChartConfigOverlay" } - ], - "discriminator": { - "propertyName": "displayType", - "mapping": { - "line": "#/components/schemas/LineChartConfig", - "stacked_bar": "#/components/schemas/BarChartConfig", - "number": "#/components/schemas/NumberChartConfig" + ] + }, + "AlertChartConfig": { + "description": "The chart configuration an inline alert evaluates, in the same dialect as dashboard tile configs plus the alert-only fields in AlertChartConfigOverlay. Only the display types the alert evaluator supports are accepted: line, stacked_bar, and number, in both builder and Raw SQL (configType \"sql\") variants. Raw SQL templates must reference the evaluation window via the time-filter and interval macros (e.g. $__timeFilter and $__timeInterval), and the referenced source/connection must belong to the team.\n", + "oneOf": [ + { + "$ref": "#/components/schemas/AlertLineChartConfig" + }, + { + "$ref": "#/components/schemas/AlertBarChartConfig" + }, + { + "$ref": "#/components/schemas/AlertNumberChartConfig" } - } + ] }, "AlertState": { "type": "string", diff --git a/packages/api/src/mcp/__tests__/alerts.int.test.ts b/packages/api/src/mcp/__tests__/alerts.int.test.ts index 0582ac11e9..1ff0e730ac 100644 --- a/packages/api/src/mcp/__tests__/alerts.int.test.ts +++ b/packages/api/src/mcp/__tests__/alerts.int.test.ts @@ -731,6 +731,92 @@ describe('MCP Alert Tools', () => { }); }); + it('preserves the gauge delta flag across the isDelta/periodAggFn dialect bridge', async () => { + const webhook = await createTestWebhook(); + + // MCP spells the flag `isDelta`; the external dialect (and the + // persisted internal shape) must not silently drop it — a lost flag + // makes the alert evaluate raw gauge values instead of deltas. + const createResult = await callTool( + client, + 'clickstack_save_alert', + makeInlineInput(webhook._id.toString(), { + chartConfig: makeChartConfig({ + select: [ + { + aggFn: 'avg', + metricType: 'gauge', + metricName: 'system.cpu.utilization', + isDelta: true, + }, + ], + }), + }), + ); + expect(createResult.isError).toBeFalsy(); + const created = JSON.parse(getFirstText(createResult)); + expect(created.chartConfig.select[0]).toMatchObject({ + periodAggFn: 'delta', + }); + + const stored = await Alert.findById(created.id); + expect(stored!.chartConfig).toMatchObject({ + select: [{ isDelta: true }], + }); + + // Read-then-resend: the detail response spells the flag + // periodAggFn: 'delta'; resending that config verbatim must keep it. + const detail = await callTool(client, 'clickstack_get_alert', { + id: created.id, + }); + const detailOutput = JSON.parse(getFirstText(detail)); + expect(detailOutput.chartConfig.select[0]).toMatchObject({ + periodAggFn: 'delta', + }); + + const resent = await callTool(client, 'clickstack_save_alert', { + ...makeInlineInput(webhook._id.toString(), { + chartConfig: detailOutput.chartConfig, + }), + id: created.id, + }); + expect(resent.isError).toBeFalsy(); + const storedAfter = await Alert.findById(created.id); + expect(storedAfter!.chartConfig).toMatchObject({ + select: [{ isDelta: true }], + }); + }); + + it('accepts the alert-only name and chart-level where fields', async () => { + const webhook = await createTestWebhook(); + + const result = await callTool( + client, + 'clickstack_save_alert', + makeInlineInput(webhook._id.toString(), { + chartConfig: makeChartConfig({ + name: 'Error Rate Query', + where: 'ServiceName:api', + whereLanguage: 'lucene', + }), + }), + ); + + expect(result.isError).toBeFalsy(); + const output = JSON.parse(getFirstText(result)); + expect(output.chartConfig).toMatchObject({ + name: 'Error Rate Query', + where: 'ServiceName:api', + }); + + const stored = await Alert.findById(output.id); + expect(stored!.chartConfig).toMatchObject({ + name: 'Error Rate Query', + where: 'ServiceName:api', + whereLanguage: 'lucene', + }); + }); + it('clickstack_get_alert list entries stay slim for inline alerts', async () => { const webhook = await createTestWebhook(); await callTool( @@ -758,8 +844,8 @@ describe('MCP Alert Tools', () => { ); const created = JSON.parse(getFirstText(createResult)); - // Seed a chartConfig name directly (the external dialect does not - // carry one) to exercise the name fallback. + // Seed a chartConfig name directly (as the internal UI flow would) + // to exercise the name fallback. await Alert.findByIdAndUpdate(created.id, { $set: { 'chartConfig.name': 'Error Rate Query' }, }); diff --git a/packages/api/src/mcp/tools/alerts/__tests__/schemas.test.ts b/packages/api/src/mcp/tools/alerts/__tests__/schemas.test.ts index 1476a95369..8fcae3066a 100644 --- a/packages/api/src/mcp/tools/alerts/__tests__/schemas.test.ts +++ b/packages/api/src/mcp/tools/alerts/__tests__/schemas.test.ts @@ -1,5 +1,6 @@ import { McpSaveAlertInput, + mcpSaveAlertSchema, validateSaveAlertInput, } from '@/mcp/tools/alerts/schemas'; @@ -130,3 +131,43 @@ describe('validateSaveAlertInput inline alerts', () => { expect(result).toContain('only supported when source is "inline"'); }); }); + +describe('mcpSaveAlertSchema isDelta dialect bridge', () => { + const parseSelectItem = (selectItem: Record) => { + const parsed = mcpSaveAlertSchema.parse({ + ...baseInput, + source: 'inline', + savedSearchId: undefined, + channel: wh('a'), + chartConfig: { + displayType: 'line', + sourceId: 'source-1', + select: [selectItem], + }, + }); + const config = parsed.chartConfig; + if (!config || !('select' in config)) { + throw new Error('expected a builder chartConfig'); + } + return config.select[0]; + }; + + const gaugeItem = { + aggFn: 'avg', + metricType: 'gauge', + metricName: 'system.cpu.utilization', + }; + + it('normalizes the REST-dialect periodAggFn: "delta" onto isDelta', () => { + expect( + parseSelectItem({ ...gaugeItem, periodAggFn: 'delta' }), + ).toMatchObject({ isDelta: true }); + }); + + it('passes an explicit isDelta through unchanged', () => { + expect(parseSelectItem({ ...gaugeItem, isDelta: true })).toMatchObject({ + isDelta: true, + }); + expect(parseSelectItem(gaugeItem).isDelta).toBeUndefined(); + }); +}); diff --git a/packages/api/src/mcp/tools/alerts/saveAlert.ts b/packages/api/src/mcp/tools/alerts/saveAlert.ts index b2e1fb07e3..7befc3c72c 100644 --- a/packages/api/src/mcp/tools/alerts/saveAlert.ts +++ b/packages/api/src/mcp/tools/alerts/saveAlert.ts @@ -73,9 +73,25 @@ export function registerSaveAlert({ // then convert to the internal shape the controllers persist. let internalChartConfig: AlertInput['chartConfig']; if (input.source === 'inline') { - const parsed = externalAlertChartConfigSchema.safeParse( - input.chartConfig, - ); + // The MCP tile dialect spells the gauge delta flag `isDelta`; the + // shared external schema spells it `periodAggFn: 'delta'` and strips + // unknown keys, so translate per select item before parsing or the + // flag is silently lost and the alert evaluates raw gauge values. + const chartConfig = + input.chartConfig != null && + 'select' in input.chartConfig && + Array.isArray(input.chartConfig.select) + ? { + ...input.chartConfig, + select: input.chartConfig.select.map(item => + item.isDelta + ? { ...item, periodAggFn: 'delta' as const } + : item, + ), + } + : input.chartConfig; + + const parsed = externalAlertChartConfigSchema.safeParse(chartConfig); if (!parsed.success) { return mcpUserError( `Invalid chartConfig:\n${formatZodIssues(parsed.error)}`, diff --git a/packages/api/src/mcp/tools/alerts/schemas.ts b/packages/api/src/mcp/tools/alerts/schemas.ts index c22f421c7e..d03b39f4aa 100644 --- a/packages/api/src/mcp/tools/alerts/schemas.ts +++ b/packages/api/src/mcp/tools/alerts/schemas.ts @@ -4,6 +4,7 @@ import { checkAlertChannelSelection, isRangeThresholdType, MAX_ALERT_CHANNELS, + SearchConditionTrimmedLanguageSchema, } from '@hyperdx/common-utils/dist/types'; import { z } from 'zod'; @@ -31,12 +32,59 @@ import { // tile-only onClick affordance. Full validation (formulas, number // single-select rule, source/connection ownership) runs at save time via the // shared external alert schema and validateAlertInput. +// +// Unlike tiles, an alert's config carries its own `name` (used in +// notification titles) and — builder variants only — a chart-level +// `where`/`whereLanguage` filter ANDed into every series, so those override +// the tile schemas' rejected-never fields here. +const mcpAlertChartConfigNameSchema = z + .string() + .optional() + .describe( + 'Display name for the alert query, used in notification titles when the ' + + 'alert has no explicit name.', + ); + +const mcpAlertChartLevelWhereSchema = z + .string() + .max(10000) + .optional() + .describe( + 'Chart-level filter applied to every select item (combined with each ' + + "item's own `where` via AND). Lucene syntax by default.", + ); + +const rejectedRawSqlAlertWhereField = z + .never({ + invalid_type_error: + 'Raw SQL alert configs have no chart-level where; filter inside the sqlTemplate instead', + }) + .optional() + .describe( + 'Not supported on Raw SQL alert configs. Filter inside the sqlTemplate.', + ); + const mcpAlertChartConfigSchema = z .union([ - mcpLineTileSchema.shape.config, - mcpBarTileSchema.shape.config, - mcpNumberTileSchema.shape.config, + mcpLineTileSchema.shape.config.extend({ + name: mcpAlertChartConfigNameSchema, + where: mcpAlertChartLevelWhereSchema, + whereLanguage: SearchConditionTrimmedLanguageSchema.optional(), + }), + mcpBarTileSchema.shape.config.extend({ + name: mcpAlertChartConfigNameSchema, + where: mcpAlertChartLevelWhereSchema, + whereLanguage: SearchConditionTrimmedLanguageSchema.optional(), + }), + mcpNumberTileSchema.shape.config.extend({ + name: mcpAlertChartConfigNameSchema, + where: mcpAlertChartLevelWhereSchema, + whereLanguage: SearchConditionTrimmedLanguageSchema.optional(), + }), mcpSqlTileSchema.shape.config.omit({ onClick: true }).extend({ + name: mcpAlertChartConfigNameSchema, + where: rejectedRawSqlAlertWhereField, + whereLanguage: rejectedRawSqlAlertWhereField, displayType: z .enum(['line', 'stacked_bar', 'number']) .describe( diff --git a/packages/api/src/mcp/tools/dashboards/schemas.ts b/packages/api/src/mcp/tools/dashboards/schemas.ts index c96b68d881..136abff5fe 100644 --- a/packages/api/src/mcp/tools/dashboards/schemas.ts +++ b/packages/api/src/mcp/tools/dashboards/schemas.ts @@ -267,6 +267,18 @@ const mcpTileSelectItemSchema = z 'METRIC SOURCES ONLY (gauge metrics). When true, computes the Prometheus-style ' + 'delta over each bucket. Default false.', ), + // The REST v2 dialect's spelling of the same flag. Accepted so a select + // item read back from the external API (or clickstack_get_alert / + // clickstack_get_dashboard, which emit the REST dialect) can be resent + // verbatim without silently clearing the delta flag; normalized onto + // `isDelta` in the transform below. + periodAggFn: z + .enum(['delta']) + .optional() + .describe( + 'Alias for isDelta accepted for round-tripping configs read from ' + + 'get tools: "delta" is equivalent to isDelta: true.', + ), }) .superRefine((data, ctx) => { const narrow = { @@ -275,7 +287,10 @@ const mcpTileSelectItemSchema = z typeof data.metricType === 'string' ? data.metricType : undefined, metricName: typeof data.metricName === 'string' ? data.metricName : undefined, - isDelta: typeof data.isDelta === 'boolean' ? data.isDelta : undefined, + isDelta: + typeof data.isDelta === 'boolean' + ? data.isDelta + : data.periodAggFn === 'delta' || undefined, level: typeof data.level === 'number' ? data.level : undefined, valueExpression: typeof data.valueExpression === 'string' @@ -290,11 +305,19 @@ const mcpTileSelectItemSchema = z }); } }) - .transform(data => - data.metricType && data.aggFn !== 'count' && !data.valueExpression - ? { ...data, valueExpression: 'Value' } - : data, - ); + .transform(data => { + // Normalize the REST-dialect spelling onto isDelta so downstream + // consumers only need to read one flag. + const withDelta = + data.periodAggFn === 'delta' && data.isDelta !== true + ? { ...data, isDelta: true } + : data; + return withDelta.metricType && + withDelta.aggFn !== 'count' && + !withDelta.valueExpression + ? { ...withDelta, valueExpression: 'Value' } + : withDelta; + }); // ─── Chart formulas ────────────────────────────────────────────────────────── diff --git a/packages/api/src/routers/external-api/__tests__/alerts.int.test.ts b/packages/api/src/routers/external-api/__tests__/alerts.int.test.ts index afba066453..0665c3f76d 100644 --- a/packages/api/src/routers/external-api/__tests__/alerts.int.test.ts +++ b/packages/api/src/routers/external-api/__tests__/alerts.int.test.ts @@ -1210,6 +1210,211 @@ describe('External API Alerts', () => { const stored = await Alert.findById(created.body.data.id); expect(stored!.chartConfig).not.toHaveProperty('unknownField'); }); + + it('rejects a chartConfig on non-inline sources instead of silently dropping it', async () => { + const { source } = await makeSource(); + const webhook = await createTestWebhook(); + const dashboard = await createTestDashboard(); + const savedSearch = await createTestSavedSearch(); + const chartConfig = makeBuilderChartConfig(source._id.toString()); + + await authRequest('post', ALERTS_BASE_URL) + .send({ + source: AlertSource.TILE, + dashboardId: dashboard._id.toString(), + tileId: dashboard.tiles[0].id, + chartConfig, + threshold: 100, + interval: '1h', + thresholdType: AlertThresholdType.ABOVE, + channel: { type: 'webhook', webhookId: webhook._id.toString() }, + }) + .expect(400); + + await authRequest('post', ALERTS_BASE_URL) + .send({ + source: AlertSource.SAVED_SEARCH, + savedSearchId: savedSearch._id.toString(), + chartConfig, + threshold: 100, + interval: '1h', + thresholdType: AlertThresholdType.ABOVE, + channel: { type: 'webhook', webhookId: webhook._id.toString() }, + }) + .expect(400); + + // PUT applies the same rule. + const { alert } = await createTestAlert(); + await authRequest('put', `${ALERTS_BASE_URL}/${alert.id}`) + .send({ + source: AlertSource.TILE, + dashboardId: alert.dashboardId, + tileId: alert.tileId, + chartConfig, + threshold: 100, + interval: '1h', + thresholdType: AlertThresholdType.ABOVE, + channel: alert.channel, + }) + .expect(400); + }); + + it('round-trips the config name and chart-level where', async () => { + const { source } = await makeSource(); + const webhook = await createTestWebhook(); + + const created = await authRequest('post', ALERTS_BASE_URL) + .send( + makeInlineAlertInput( + makeBuilderChartConfig(source._id.toString(), { + name: 'Error Rate Query', + where: 'ServiceName:api', + whereLanguage: 'lucene', + }), + webhook._id.toString(), + ), + ) + .expect(200); + + expect(created.body.data.chartConfig).toMatchObject({ + name: 'Error Rate Query', + where: 'ServiceName:api', + whereLanguage: 'lucene', + }); + + // Persisted on the internal shape the evaluator reads. + const stored = await Alert.findById(created.body.data.id); + expect(stored!.chartConfig).toMatchObject({ + name: 'Error Rate Query', + where: 'ServiceName:api', + whereLanguage: 'lucene', + }); + + const single = await authRequest( + 'get', + `${ALERTS_BASE_URL}/${created.body.data.id}`, + ).expect(200); + expect(single.body.data.chartConfig).toMatchObject({ + name: 'Error Rate Query', + where: 'ServiceName:api', + }); + + // Echoing the GET body through PUT keeps them intact — the + // read-modify-write loop must not degrade notification titles or + // silently broaden the alert's query. + await authRequest('put', `${ALERTS_BASE_URL}/${created.body.data.id}`) + .send(single.body.data) + .expect(200); + const storedAfter = await Alert.findById(created.body.data.id); + expect(storedAfter!.chartConfig).toEqual(stored!.chartConfig); + }); + + it('rejects a chart-level where on raw SQL configs', async () => { + const { connection } = await makeSource(); + const webhook = await createTestWebhook(); + + await authRequest('post', ALERTS_BASE_URL) + .send( + makeInlineAlertInput( + { + configType: 'sql', + displayType: 'line', + sqlTemplate: RAW_SQL_ALERT_TEMPLATE, + connectionId: connection._id.toString(), + where: 'ServiceName:api', + }, + webhook._id.toString(), + ), + ) + .expect(400); + }); + + it('omits chartConfig for internally-authored configs the dialect cannot express, and a blind echo-PUT fails loudly', async () => { + // An alert created through the internal API can persist fields the + // external dialect has no spelling for (chart-level `filters` here). + // Emitting a lossy approximation would let a GET -> PUT loop silently + // broaden the alert's query, so the GET omits chartConfig entirely and + // re-PUTting the echoed body 400s on the missing config. + const { source } = await makeSource(); + + const alert = await createTestAlertDirectly({ + source: AlertSource.INLINE, + dashboardId: undefined, + tileId: undefined, + chartConfig: { + displayType: 'line', + source: source._id.toString(), + select: [ + { + aggFn: 'count', + aggCondition: '', + aggConditionLanguage: 'lucene', + valueExpression: '', + }, + ], + where: '', + whereLanguage: 'lucene', + filters: [{ type: 'sql', condition: "ServiceName = 'api'" }], + }, + }); + + const single = await authRequest( + 'get', + `${ALERTS_BASE_URL}/${alert._id}`, + ).expect(200); + expect(single.body.data.source).toBe(AlertSource.INLINE); + expect(single.body.data.chartConfig).toBeUndefined(); + + await authRequest('put', `${ALERTS_BASE_URL}/${alert._id}`) + .send(single.body.data) + .expect(400); + + // The stored config is untouched by the failed write. + const stored = await Alert.findById(alert._id); + expect(stored!.chartConfig).toMatchObject({ + filters: [{ type: 'sql', condition: "ServiceName = 'api'" }], + }); + }); + + it('emits chartConfig for internally-authored configs the dialect can express', async () => { + // Guards against over-refusal: the shapes the UI's chart explorer + // persists (name, empty chart-level where, count select) must keep + // round-tripping. + const { source } = await makeSource(); + + const alert = await createTestAlertDirectly({ + source: AlertSource.INLINE, + dashboardId: undefined, + tileId: undefined, + chartConfig: { + name: 'Chart Alert Query', + displayType: 'line', + source: source._id.toString(), + select: [ + { + aggFn: 'count', + aggCondition: 'level:error', + aggConditionLanguage: 'lucene', + valueExpression: '', + }, + ], + where: '', + whereLanguage: 'lucene', + seriesReturnType: 'column', + }, + }); + + const single = await authRequest( + 'get', + `${ALERTS_BASE_URL}/${alert._id}`, + ).expect(200); + expect(single.body.data.chartConfig).toMatchObject({ + displayType: 'line', + name: 'Chart Alert Query', + sourceId: source._id.toString(), + select: [{ aggFn: 'count', where: 'level:error' }], + }); + }); }); describe('Input validation', () => { diff --git a/packages/api/src/routers/external-api/v2/alerts.ts b/packages/api/src/routers/external-api/v2/alerts.ts index 0565301713..7256d2125d 100644 --- a/packages/api/src/routers/external-api/v2/alerts.ts +++ b/packages/api/src/routers/external-api/v2/alerts.ts @@ -74,25 +74,57 @@ function toInternalAlertInput(body: ExternalAlertInput): InternalAlertInput { * "tile" alerts monitor a dashboard tile, and "inline" alerts carry * their own chart configuration (see AlertChartConfig) without * requiring a saved search or dashboard. - * AlertChartConfig: + * AlertChartConfigOverlay: + * type: object * description: > - * The chart configuration an inline alert evaluates, in the same - * dialect as dashboard tile configs. Only the display types the alert - * evaluator supports are accepted: line, stacked_bar, and number, in - * both builder and Raw SQL (configType "sql") variants. Raw SQL - * templates must reference the evaluation window via the time-filter - * and interval macros (e.g. $__timeFilter and $__timeInterval), and - * the referenced source/connection must belong to the team. - * oneOf: + * Fields an alert's chart config carries on top of the dashboard tile + * config dialect. Unlike a tile (whose name lives on the tile, not + * its config), an inline alert's config is standalone. + * properties: + * name: + * type: string + * description: > + * Display name for the alert query. Used in notification titles + * and as an alert-name fallback when the alert itself has no + * name. + * example: "Error Rate Query" + * where: + * type: string + * maxLength: 10000 + * description: > + * Chart-level filter applied on top of every select item's own + * "where" (combined via AND). Builder variants only; rejected on + * Raw SQL variants (filter inside the sqlTemplate instead). + * example: "ServiceName:api" + * whereLanguage: + * $ref: '#/components/schemas/QueryLanguage' + * description: Language of the chart-level "where" filter. + * AlertLineChartConfig: + * allOf: * - $ref: '#/components/schemas/LineChartConfig' + * - $ref: '#/components/schemas/AlertChartConfigOverlay' + * AlertBarChartConfig: + * allOf: * - $ref: '#/components/schemas/BarChartConfig' + * - $ref: '#/components/schemas/AlertChartConfigOverlay' + * AlertNumberChartConfig: + * allOf: * - $ref: '#/components/schemas/NumberChartConfig' - * discriminator: - * propertyName: displayType - * mapping: - * line: '#/components/schemas/LineChartConfig' - * stacked_bar: '#/components/schemas/BarChartConfig' - * number: '#/components/schemas/NumberChartConfig' + * - $ref: '#/components/schemas/AlertChartConfigOverlay' + * AlertChartConfig: + * description: > + * The chart configuration an inline alert evaluates, in the same + * dialect as dashboard tile configs plus the alert-only fields in + * AlertChartConfigOverlay. Only the display types the alert evaluator + * supports are accepted: line, stacked_bar, and number, in both + * builder and Raw SQL (configType "sql") variants. Raw SQL templates + * must reference the evaluation window via the time-filter and + * interval macros (e.g. $__timeFilter and $__timeInterval), and the + * referenced source/connection must belong to the team. + * oneOf: + * - $ref: '#/components/schemas/AlertLineChartConfig' + * - $ref: '#/components/schemas/AlertBarChartConfig' + * - $ref: '#/components/schemas/AlertNumberChartConfig' * AlertState: * type: string * enum: [ALERT, OK, INSUFFICIENT_DATA, DISABLED, PENDING] diff --git a/packages/api/src/routers/external-api/v2/utils/__tests__/alertChartConfig.test.ts b/packages/api/src/routers/external-api/v2/utils/__tests__/alertChartConfig.test.ts index 56d311544e..e846c9aaaf 100644 --- a/packages/api/src/routers/external-api/v2/utils/__tests__/alertChartConfig.test.ts +++ b/packages/api/src/routers/external-api/v2/utils/__tests__/alertChartConfig.test.ts @@ -1,119 +1,197 @@ -import { DisplayType, MetricsDataType } from '@hyperdx/common-utils/dist/types'; +import { + AlertChartConfig, + AlertChartConfigSchema, + DisplayType, +} from '@hyperdx/common-utils/dist/types'; import { convertAlertChartConfigToExternal, convertExternalAlertChartConfigToInternal, + EVALUATION_INERT_CONFIG_KEYS, + EXTERNAL_TO_INTERNAL_KEY, + KNOWN_LOSSY_CONFIG_KEYS, } from '@/routers/external-api/v2/utils/alertChartConfig'; -import { ExternalAlertChartConfig } from '@/utils/zod'; +import { + externalAlertBuilderChartConfigSchema, + ExternalAlertChartConfig, + externalAlertRawSqlChartConfigSchema, +} from '@/utils/zod'; + +const SOURCE_ID = '65f5e4a3b9e77c001a123456'; +const CONNECTION_ID = '65f5e4a3b9e77c001a789012'; + +const countItem = (where = '') => ({ + aggFn: 'count' as const, + where, + whereLanguage: 'lucene' as const, + valueExpression: '', +}); + +const baseInternal = ( + overrides: Partial> = {}, +): AlertChartConfig => + ({ + displayType: DisplayType.Line, + source: SOURCE_ID, + select: [ + { + aggFn: 'count', + aggCondition: '', + aggConditionLanguage: 'lucene', + valueExpression: '', + }, + ], + where: '', + whereLanguage: 'lucene', + ...overrides, + }) as AlertChartConfig; -describe('alertChartConfig converters', () => { - describe('builder configs', () => { - const externalLineConfig: ExternalAlertChartConfig = { - displayType: 'line', - sourceId: '65f5e4a3b9e77c001a123456', +describe('convertExternalAlertChartConfigToInternal', () => { + it('maps the external dialect onto the internal one', () => { + const internal = convertExternalAlertChartConfigToInternal({ + displayType: DisplayType.Line, + sourceId: SOURCE_ID, + name: 'Error Rate Query', + where: 'ServiceName:api', + whereLanguage: 'lucene', + groupBy: 'ServiceName', + asRatio: true, select: [ { - aggFn: 'count', + aggFn: 'avg', + valueExpression: 'Value', where: 'level:error', whereLanguage: 'lucene', - alias: 'Errors', + alias: 'CPU', + metricType: 'gauge', + metricName: 'system.cpu.utilization', + periodAggFn: 'delta', }, + countItem(), + ], + } as ExternalAlertChartConfig); + + expect(internal).toMatchObject({ + displayType: DisplayType.Line, + source: SOURCE_ID, + name: 'Error Rate Query', + where: 'ServiceName:api', + whereLanguage: 'lucene', + groupBy: 'ServiceName', + seriesReturnType: 'ratio', + select: [ { aggFn: 'avg', valueExpression: 'Value', - where: '', - whereLanguage: 'lucene', - metricType: MetricsDataType.Gauge, + aggCondition: 'level:error', + aggConditionLanguage: 'lucene', + alias: 'CPU', + metricType: 'gauge', metricName: 'system.cpu.utilization', - periodAggFn: 'delta', + isDelta: true, }, + { aggFn: 'count', aggCondition: '', isDelta: false }, ], + }); + expect('alert' in internal).toBe(false); + }); + + it('omits the synthetic tile name and never persists an alert field', () => { + const internal = convertExternalAlertChartConfigToInternal({ + displayType: DisplayType.Line, + sourceId: SOURCE_ID, + select: [countItem()], + } as ExternalAlertChartConfig); + + expect('name' in internal).toBe(false); + expect('alert' in internal).toBe(false); + }); +}); + +describe('external <-> internal round-trip', () => { + it('round-trips a builder line config, including alert-only and metric fields', () => { + const external = { + displayType: DisplayType.Line, + sourceId: SOURCE_ID, + name: 'Error Rate Query', + where: 'ServiceName:api', + whereLanguage: 'lucene' as const, groupBy: 'ServiceName', asRatio: true, - fillNulls: false, seriesLimit: 5, - numberFormat: { output: 'number', mantissa: 2 }, - }; - - it('maps the external dialect onto the internal AlertChartConfig', () => { - const internal = - convertExternalAlertChartConfigToInternal(externalLineConfig); - - expect(internal).toMatchObject({ - displayType: DisplayType.Line, - source: '65f5e4a3b9e77c001a123456', - groupBy: 'ServiceName', - seriesReturnType: 'ratio', - fillNulls: false, - seriesLimit: 5, - select: [ - { - aggFn: 'count', - aggCondition: 'level:error', - aggConditionLanguage: 'lucene', - alias: 'Errors', - isDelta: false, - }, - { - aggFn: 'avg', - valueExpression: 'Value', - aggCondition: '', - aggConditionLanguage: 'lucene', - metricType: MetricsDataType.Gauge, - metricName: 'system.cpu.utilization', - isDelta: true, - }, - ], - }); - }); + fillNulls: false, + numberFormat: { output: 'number' as const }, + select: [ + { + aggFn: 'avg' as const, + valueExpression: 'Value', + where: 'level:error', + whereLanguage: 'lucene' as const, + alias: 'CPU', + metricType: 'gauge' as const, + metricName: 'system.cpu.utilization', + periodAggFn: 'delta' as const, + }, + countItem(), + ], + } as ExternalAlertChartConfig; - it('never persists the synthetic tile name or an embedded alert', () => { - const internal = - convertExternalAlertChartConfigToInternal(externalLineConfig); + const roundTripped = convertAlertChartConfigToExternal( + convertExternalAlertChartConfigToInternal(external), + ); - // The tile converter writes the synthetic tile's name ('') into the - // config; persisting it would attach junk to every alert document. - expect(internal).not.toHaveProperty('name'); - expect(internal).not.toHaveProperty('alert'); - }); + expect(roundTripped).toEqual(external); + }); - it('round-trips external -> internal -> external without losing fields', () => { - const internal = - convertExternalAlertChartConfigToInternal(externalLineConfig); - const roundTripped = convertAlertChartConfigToExternal(internal); - - expect(roundTripped).toMatchObject({ - displayType: 'line', - sourceId: '65f5e4a3b9e77c001a123456', - groupBy: 'ServiceName', - asRatio: true, - fillNulls: false, - seriesLimit: 5, - numberFormat: { output: 'number', mantissa: 2 }, - select: [ - { - aggFn: 'count', - where: 'level:error', - whereLanguage: 'lucene', - alias: 'Errors', - }, - { - aggFn: 'avg', - valueExpression: 'Value', - where: '', - whereLanguage: 'lucene', - metricType: MetricsDataType.Gauge, - metricName: 'system.cpu.utilization', - periodAggFn: 'delta', - }, - ], - }); + it('round-trips a quantile select item with its level', () => { + const external = { + displayType: DisplayType.StackedBar, + sourceId: SOURCE_ID, + select: [ + { + aggFn: 'quantile' as const, + level: 0.95 as const, + valueExpression: 'Duration', + where: '', + whereLanguage: 'lucene' as const, + }, + ], + } as ExternalAlertChartConfig; + + const roundTripped = convertAlertChartConfigToExternal( + convertExternalAlertChartConfigToInternal(external), + ); + + expect(roundTripped).toMatchObject({ + displayType: DisplayType.StackedBar, + select: [{ aggFn: 'quantile', level: 0.95, valueExpression: 'Duration' }], }); + }); + + it('round-trips a raw SQL config', () => { + const external = { + configType: 'sql' as const, + displayType: DisplayType.Line, + connectionId: CONNECTION_ID, + sourceId: SOURCE_ID, + sqlTemplate: 'SELECT $__timeInterval(Timestamp) AS ts, count() ...', + name: 'Raw SQL Alert Query', + fillNulls: true, + } as ExternalAlertChartConfig; + + const roundTripped = convertAlertChartConfigToExternal( + convertExternalAlertChartConfigToInternal(external), + ); - it('converts internal stacked_bar and number configs to the external dialect', () => { - const stackedBar = convertAlertChartConfigToExternal({ - displayType: DisplayType.StackedBar, - source: '65f5e4a3b9e77c001a123456', + expect(roundTripped).toEqual(external); + }); +}); + +describe('convertAlertChartConfigToExternal', () => { + it('emits a number config', () => { + const external = convertAlertChartConfigToExternal( + baseInternal({ + displayType: DisplayType.Number, select: [ { aggFn: 'count', @@ -122,103 +200,155 @@ describe('alertChartConfig converters', () => { valueExpression: '', }, ], - where: '', - }); - expect(stackedBar).toMatchObject({ - displayType: 'stacked_bar', - sourceId: '65f5e4a3b9e77c001a123456', - }); - - const number = convertAlertChartConfigToExternal({ - displayType: DisplayType.Number, - source: '65f5e4a3b9e77c001a123456', + }), + ); + + expect(external).toMatchObject({ + displayType: DisplayType.Number, + sourceId: SOURCE_ID, + }); + }); + + it('still emits configs carrying evaluation-inert extras', () => { + const external = convertAlertChartConfigToExternal( + baseInternal({ + seriesReturnType: 'column', + fillNulls: 0, + markdown: '', + groupByColumnsOnLeft: true, + compareToPreviousPeriod: true, + color: 'chart-blue', select: [ { aggFn: 'count', - aggCondition: 'level:error', + aggCondition: '', aggConditionLanguage: 'lucene', valueExpression: '', + color: 'chart-red', }, ], - where: '', - numberFormat: { output: 'percent' }, - }); - expect(number).toMatchObject({ - displayType: 'number', - sourceId: '65f5e4a3b9e77c001a123456', - numberFormat: { output: 'percent' }, - select: [{ aggFn: 'count', where: 'level:error' }], - }); - }); + }), + ); + + expect(external).toMatchObject({ displayType: DisplayType.Line }); }); - describe('raw SQL configs', () => { - const externalRawSqlConfig: ExternalAlertChartConfig = { - configType: 'sql', - displayType: 'line', - connectionId: '65f5e4a3b9e77c001a789012', - sqlTemplate: - 'SELECT $__timeInterval(Timestamp) AS ts, count() FROM t WHERE $__timeFilter(Timestamp) GROUP BY ts', - sourceId: '65f5e4a3b9e77c001a123456', - fillNulls: false, - seriesLimit: 3, - }; - - it('maps external field names onto the internal ones', () => { - const internal = - convertExternalAlertChartConfigToInternal(externalRawSqlConfig); - - expect(internal).toMatchObject({ - configType: 'sql', - displayType: DisplayType.Line, - connection: '65f5e4a3b9e77c001a789012', - source: '65f5e4a3b9e77c001a123456', - fillNulls: false, - seriesLimit: 3, - }); - expect(internal).not.toHaveProperty('connectionId'); - expect(internal).not.toHaveProperty('sourceId'); - expect(internal).not.toHaveProperty('name'); - }); + it.each([ + [ + 'filters', + { filters: [{ type: 'sql', condition: "ServiceName = 'api'" }] }, + ], + ['having', { having: 'count() > 5', havingLanguage: 'sql' }], + ['orderBy', { orderBy: 'Timestamp DESC' }], + ['limit', { limit: { limit: 10 } }], + ['ratioMode', { ratioMode: 'total' }], + ['selectGroupBy', { selectGroupBy: true }], + ['granularity', { granularity: '5 minute' }], + ['implicitColumnExpression', { implicitColumnExpression: 'Body' }], + ['sampleWeightExpression', { sampleWeightExpression: 'SampleRate' }], + ['a raw SQL select expression string', { select: 'count() as count' }], + [ + 'an unknown future field', + { someFutureEvaluationKnob: 'on' } as Record, + ], + ])('refuses to emit a config carrying %s', (_label, overrides) => { + expect( + convertAlertChartConfigToExternal(baseInternal(overrides)), + ).toBeUndefined(); + }); - it('round-trips external -> internal -> external', () => { - const internal = - convertExternalAlertChartConfigToInternal(externalRawSqlConfig); - const roundTripped = convertAlertChartConfigToExternal(internal); - - expect(roundTripped).toMatchObject({ - configType: 'sql', - displayType: 'line', - connectionId: '65f5e4a3b9e77c001a789012', - sqlTemplate: externalRawSqlConfig.sqlTemplate, - sourceId: '65f5e4a3b9e77c001a123456', - fillNulls: false, - seriesLimit: 3, - }); - }); + it('refuses a chart-level where longer than the external cap', () => { + expect( + convertAlertChartConfigToExternal( + baseInternal({ where: 'a'.repeat(10001) }), + ), + ).toBeUndefined(); }); - describe('configs without an external representation', () => { - it('returns undefined for a corrupt config with an unsupported display type', () => { - // Reachable only via a direct DB write: both write paths reject - // non-alertable display types, but a Mixed Mongo field enforces - // nothing. The translate layer omits the field rather than emitting - // a shape the external contract cannot express. - const external = convertAlertChartConfigToExternal({ - displayType: DisplayType.Table, - source: '65f5e4a3b9e77c001a123456', - select: [ - { + it('refuses more than 20 select items', () => { + expect( + convertAlertChartConfigToExternal( + baseInternal({ + select: Array.from({ length: 21 }, () => ({ aggFn: 'count', aggCondition: '', aggConditionLanguage: 'lucene', valueExpression: '', - }, - ], - where: '', - }); + })), + }), + ), + ).toBeUndefined(); + }); - expect(external).toBeUndefined(); - }); + it('refuses a formula-less number config with multiple select items', () => { + expect( + convertAlertChartConfigToExternal( + baseInternal({ + displayType: DisplayType.Number, + select: [ + { + aggFn: 'count', + aggCondition: '', + aggConditionLanguage: 'lucene', + valueExpression: '', + }, + { + aggFn: 'count', + aggCondition: 'level:error', + aggConditionLanguage: 'lucene', + valueExpression: '', + }, + ], + }), + ), + ).toBeUndefined(); + }); + + it('refuses unsupported display types and PromQL configs', () => { + expect( + convertAlertChartConfigToExternal( + baseInternal({ displayType: DisplayType.Table }), + ), + ).toBeUndefined(); + expect( + convertAlertChartConfigToExternal({ + configType: 'promql', + promqlQuery: 'up', + source: SOURCE_ID, + } as unknown as AlertChartConfig), + ).toBeUndefined(); + }); +}); + +// Drift guard: every field the internal alert chart config can persist must +// be deliberately classified as externally representable, evaluation-inert, +// or known-lossy. A new field on the internal schema breaks this test and +// forces a decision instead of silently leaking through the external +// round-trip (unknown fields refuse at runtime either way — this test is +// about intent, not safety). +describe('internal schema field classification', () => { + const representableInternalKeys = new Set( + [ + ...externalAlertBuilderChartConfigSchema.options, + ...externalAlertRawSqlChartConfigSchema.options, + ].flatMap(member => + Object.keys(member.shape).map( + key => EXTERNAL_TO_INTERNAL_KEY[key] ?? key, + ), + ), + ); + + it.each( + AlertChartConfigSchema.options.flatMap((member, i) => + Object.keys(member.shape).map( + key => [i === 0 ? 'builder' : 'raw SQL', key] as const, + ), + ), + )('classifies the internal %s config field "%s"', (_variant, key) => { + const classified = + representableInternalKeys.has(key) || + EVALUATION_INERT_CONFIG_KEYS.has(key) || + KNOWN_LOSSY_CONFIG_KEYS.has(key); + expect(classified).toBe(true); }); }); diff --git a/packages/api/src/routers/external-api/v2/utils/alertChartConfig.ts b/packages/api/src/routers/external-api/v2/utils/alertChartConfig.ts index 0fce2e42b4..65d5fff4d1 100644 --- a/packages/api/src/routers/external-api/v2/utils/alertChartConfig.ts +++ b/packages/api/src/routers/external-api/v2/utils/alertChartConfig.ts @@ -5,7 +5,11 @@ import { import { omit } from 'lodash'; import logger from '@/utils/logger'; -import { ExternalAlertChartConfig } from '@/utils/zod'; +import { + externalAlertBuilderChartConfigSchema, + ExternalAlertChartConfig, + externalAlertRawSqlChartConfigSchema, +} from '@/utils/zod'; import { convertToExternalTileChartConfig, @@ -22,42 +26,292 @@ import { * surfaces cannot drift on field mapping (`where` -> `aggCondition`, * `asRatio` -> `seriesReturnType`, `connectionId` -> `connection`, fillNulls * defaulting, ...). Layout fields are placeholders — only `config` is kept. + * The alert-only fields the tile dialect lacks (`name`, chart-level + * `where`/`whereLanguage`) are threaded through on top. */ export function convertExternalAlertChartConfigToInternal( external: ExternalAlertChartConfig, ): AlertChartConfig { const { config } = convertToInternalTileConfig({ id: '', - name: '', + // The tile converter persists the tile's name into the config, which is + // exactly where an alert's config name lives. + name: external.name ?? '', x: 0, y: 0, w: 1, h: 1, config: external, }); - // The tile converter writes the synthetic tile's (empty) name into the - // config and never emits a PromQL variant or an embedded `alert` for the - // display types the alert schema admits, so stripping `name`/`alert` yields - // a valid AlertChartConfig. - return omit(config, ['name', 'alert']) as AlertChartConfig; + // The converter never emits a PromQL variant or an embedded `alert` for the + // display types the alert schema admits, so dropping `alert` yields a valid + // AlertChartConfig. + const internal = omit(config, ['alert']) as AlertChartConfig; + if (!external.name) { + // Synthetic-tile artifact: the converter wrote name: ''. + delete internal.name; + } + if (!('configType' in external) && !('configType' in internal)) { + // The tile converter hardcodes where: '' (the tile dialect has no + // chart-level filter); the alert dialect does, so apply it. + if (external.where) { + internal.where = external.where; + internal.whereLanguage = external.whereLanguage ?? 'lucene'; + } + } + return internal; +} + +const EXTERNAL_ALERT_DISPLAY_TYPES = [ + DisplayType.Line, + DisplayType.StackedBar, + DisplayType.Number, +] as const; + +type ExternalAlertDisplayType = (typeof EXTERNAL_ALERT_DISPLAY_TYPES)[number]; + +/** + * External dialect spelling -> internal spelling for top-level config keys. + * Exported for the schema drift-guard test. + */ +export const EXTERNAL_TO_INTERNAL_KEY: Record = { + sourceId: 'source', + connectionId: 'connection', + asRatio: 'seriesReturnType', +}; + +/** + * Internal fields the external round-trip drops but which have no effect on + * how the check-alerts task evaluates the config (render/UI-only concerns). + * Losing them on an external GET -> PUT does not change alert behavior. + * Exported for the schema drift-guard test. + */ +export const EVALUATION_INERT_CONFIG_KEYS: ReadonlySet = new Set([ + 'markdown', + // Only meaningful together with `filters`, which is itself lossy. + 'filtersLogicalOperator', + 'groupByColumnsOnLeft', + 'color', + 'colorRules', + 'backgroundChart', + 'compareToPreviousPeriod', + 'fitYAxisToData', + 'onClick', + 'alternateRowBackground', + // Client-side null-bucket rendering; the alert task ignores it. + 'fillNulls', + // Evaluation-relevant only with a groupBy, which (on variants whose shape + // lacks seriesLimit, i.e. number) is itself refused when present. + 'seriesLimit', +]); + +/** + * Internal fields that DO affect evaluation and that the external dialect + * cannot express. A non-empty value on any of these makes the config + * unrepresentable, so the GET response omits chartConfig and a blind echo-PUT + * fails loudly ("chartConfig is required") instead of silently rewriting the + * alert's query. Exported for the schema drift-guard test, which asserts every + * field of the internal AlertChartConfigSchema is deliberately classified. + */ +export const KNOWN_LOSSY_CONFIG_KEYS: ReadonlySet = new Set([ + 'filters', + 'having', + 'havingLanguage', + 'orderBy', + 'limit', + 'ratioMode', + 'selectGroupBy', + 'granularity', + 'implicitColumnExpression', + 'sampleWeightExpression', + 'eventTableSelect', + 'bodyExpression', + 'useTextIndexForImplicitColumn', + 'metricTables', +]); + +/** Select-item fields the external dialect round-trips. */ +const REPRESENTABLE_SELECT_ITEM_KEYS: ReadonlySet = new Set([ + 'aggFn', + 'aggCondition', + 'aggConditionLanguage', + 'valueExpression', + 'alias', + 'level', + 'metricType', + 'metricName', + 'numberFormat', + 'isDelta', +]); + +/** + * Select-item fields that are inert on line/stacked_bar/number charts + * (heatmap- or table-cell-only affordances) and may be dropped. + */ +const EVALUATION_INERT_SELECT_ITEM_KEYS: ReadonlySet = new Set([ + 'color', + 'colorRules', + 'countExpression', + 'heatmapScaleType', +]); + +const isEmptyValue = (value: unknown): boolean => + value === undefined || + value === null || + value === '' || + (Array.isArray(value) && value.length === 0); + +/** Values equivalent to the field being absent. */ +const isDefaultValue = (key: string, value: unknown): boolean => { + switch (key) { + case 'seriesReturnType': + return value === 'column'; + case 'whereLanguage': + case 'aggConditionLanguage': + case 'havingLanguage': + return value === 'lucene'; + default: + // Persisted boolean flags are off-by-default; fillNulls (number|false) + // is evaluation-inert and classified above, so `false` here only ever + // describes a flag in its default state. + return value === false; + } +}; + +const EXTERNAL_ALERT_WHERE_MAX = 10000; + +/** + * Whether the persisted internal config survives an external GET -> PUT + * round-trip without changing what the alert evaluates. Anything not + * expressible in the external dialect (and not evaluation-inert) makes the + * config unrepresentable. Allowlist semantics: unknown future fields refuse + * by default rather than leaking through silently. + */ +function isRepresentableExternally( + config: AlertChartConfig, + displayType: ExternalAlertDisplayType, +): boolean { + const isRawSql = 'configType' in config && config.configType === 'sql'; + const memberSchema = ( + isRawSql + ? externalAlertRawSqlChartConfigSchema + : externalAlertBuilderChartConfigSchema + ).optionsMap.get(displayType); + if (memberSchema == null) { + return false; + } + // The representable set is derived from the external member schema itself, + // so it cannot drift when the external dialect gains fields. + const representableKeys = new Set( + Object.keys(memberSchema.shape).map( + key => EXTERNAL_TO_INTERNAL_KEY[key] ?? key, + ), + ); + + for (const [key, value] of Object.entries(config)) { + if (isEmptyValue(value) || isDefaultValue(key, value)) continue; + if (representableKeys.has(key)) continue; + if (EVALUATION_INERT_CONFIG_KEYS.has(key)) continue; + return false; + } + + if (isRawSql) { + return true; + } + + if ('where' in config && typeof config.where === 'string') { + // The external schema caps the chart-level filter; a longer stored value + // would emit a body that cannot be PUT back. + if (config.where.length > EXTERNAL_ALERT_WHERE_MAX) { + return false; + } + } + + const select = 'select' in config ? config.select : undefined; + // A raw SQL select expression string has no external representation — the + // tile converter would silently substitute a default count() item. + if (!Array.isArray(select)) { + return false; + } + // The external schema caps select at 20 items. + if (select.length === 0 || select.length > 20) { + return false; + } + // The external converter keeps only select[0] on formula-less number + // configs, so extra items would be silently discarded. + if ( + displayType === DisplayType.Number && + select.length > 1 && + !('formulas' in config && (config.formulas?.length ?? 0) > 0) + ) { + return false; + } + for (const item of select) { + for (const [key, value] of Object.entries(item)) { + if (isEmptyValue(value) || isDefaultValue(key, value)) continue; + // Inert leftovers the external converter heals away: a level on a + // non-quantile agg and a valueExpression on a count are ignored by the + // renderer (see convertToExternalSelectItem). + if (key === 'level' && item.aggFn !== 'quantile') continue; + if (key === 'valueExpression' && item.aggFn === 'count') continue; + if (REPRESENTABLE_SELECT_ITEM_KEYS.has(key)) continue; + if (EVALUATION_INERT_SELECT_ITEM_KEYS.has(key)) continue; + return false; + } + } + + return true; } /** * Converts an inline alert's persisted internal chart config to the external - * tile-config dialect for API responses. Returns undefined for configs the - * external contract cannot represent (a corrupt document with an unsupported - * display type or a PromQL config) — callers omit the field rather than - * emitting an invalid shape. + * tile-config dialect for API responses. Returns undefined when the external + * contract cannot represent the config faithfully — an unsupported display + * type, a PromQL config, or a config carrying evaluation-affecting fields the + * dialect cannot express (see isRepresentableExternally). Callers omit the + * field, so a blind echo-PUT fails loudly instead of silently rewriting the + * alert's query. */ export function convertAlertChartConfigToExternal( config: AlertChartConfig, ): ExternalAlertChartConfig | undefined { + const displayType = config.displayType; + if ( + displayType == null || + !(EXTERNAL_ALERT_DISPLAY_TYPES as readonly DisplayType[]).includes( + displayType, + ) || + !isRepresentableExternally(config, displayType as ExternalAlertDisplayType) + ) { + logger.warn( + { displayType }, + 'Inline alert chart config has no faithful external representation; omitting chartConfig', + ); + return undefined; + } + const external = convertToExternalTileChartConfig(config); switch (external?.displayType) { case DisplayType.Line: case DisplayType.StackedBar: - case DisplayType.Number: - return external; + case DisplayType.Number: { + const name = config.name ? { name: config.name } : {}; + if ('configType' in external) { + return { ...external, ...name }; + } + return { + ...external, + ...name, + // Chart-level filter: builder configs only (the raw SQL internal + // shape has no `where`). + ...(!('configType' in config) && config.where + ? { + where: config.where, + whereLanguage: config.whereLanguage ?? 'lucene', + } + : {}), + }; + } default: logger.error( { config }, diff --git a/packages/api/src/routers/external-api/v2/utils/dashboards.ts b/packages/api/src/routers/external-api/v2/utils/dashboards.ts index 8a430e3f5f..b2d861f561 100644 --- a/packages/api/src/routers/external-api/v2/utils/dashboards.ts +++ b/packages/api/src/routers/external-api/v2/utils/dashboards.ts @@ -650,7 +650,12 @@ export function convertToExternalDashboard( // -------------------------------------------------------------------------------- const convertToInternalSelectItem = ( - item: ExternalDashboardSelectItem, + // `isDelta` is the MCP tile dialect's spelling of the gauge delta flag + // (`periodAggFn: 'delta'` in the REST dialect). MCP tiles are cast to + // ExternalDashboardTileWithId before reaching this converter + // (saveDashboard.ts), so honoring both spellings here is what keeps an + // MCP-authored gauge-delta series from silently evaluating raw values. + item: ExternalDashboardSelectItem & { isDelta?: boolean }, ): Exclude => { return { ...pick(item, [ @@ -663,7 +668,7 @@ const convertToInternalSelectItem = ( ]), aggCondition: item.where, aggConditionLanguage: item.whereLanguage, - isDelta: item.periodAggFn === 'delta', + isDelta: item.periodAggFn === 'delta' || item.isDelta === true, valueExpression: item.valueExpression ?? '', }; }; diff --git a/packages/api/src/utils/zod.ts b/packages/api/src/utils/zod.ts index db4cfe00ea..6a1de09ea7 100644 --- a/packages/api/src/utils/zod.ts +++ b/packages/api/src/utils/zod.ts @@ -795,21 +795,68 @@ const validateInlineAlertFormulas = ( // the alert task can evaluate as a time series: Line, Stacked Bar, and // Number. Builder + raw SQL variants; no PromQL (PromQL charts cannot be // alerted on). -const externalAlertBuilderChartConfigSchema = z.discriminatedUnion( +// +// Unlike dashboard tiles, an alert's chart config additionally carries fields +// that live on the tile (not its config) or are deliberately hidden from the +// tile dialect, but are meaningful for a standalone alert: +// - `name`: the config's display name, used in notification titles +// (buildAlertMessageTemplateTitle) and as an alert-name fallback. +// - `where`/`whereLanguage` (builder only): the chart-level filter that +// renderChartConfig ANDs into every series. The tile dialect omits it +// because the tile editor cannot display it, but the chart-explorer +// editor that authors inline alerts does; without it a GET -> PUT +// round-trip would silently broaden the alert's query. +const alertChartConfigNameSchema = z.string().optional(); + +// Raw SQL configs have no chart-level filter internally +// (RawSqlBaseChartConfigSchema); reject rather than silently strip so a +// caller who meant to filter learns the field does not exist here. +const rejectedAlertRawSqlWhereField = z + .never({ + invalid_type_error: + 'Raw SQL alert configs have no chart-level where; filter inside the sqlTemplate instead', + }) + .optional(); + +export const externalAlertBuilderChartConfigSchema = z.discriminatedUnion( 'displayType', [ - externalDashboardLineChartConfigSchema, - externalDashboardBarChartConfigSchema, - externalDashboardNumberChartConfigSchema, + externalDashboardLineChartConfigSchema.extend({ + name: alertChartConfigNameSchema, + where: z.string().max(10000).optional(), + whereLanguage: whereLanguageSchema, + }), + externalDashboardBarChartConfigSchema.extend({ + name: alertChartConfigNameSchema, + where: z.string().max(10000).optional(), + whereLanguage: whereLanguageSchema, + }), + externalDashboardNumberChartConfigSchema.extend({ + name: alertChartConfigNameSchema, + where: z.string().max(10000).optional(), + whereLanguage: whereLanguageSchema, + }), ], ); -const externalAlertRawSqlChartConfigSchema = z.discriminatedUnion( +export const externalAlertRawSqlChartConfigSchema = z.discriminatedUnion( 'displayType', [ - externalDashboardLineRawSqlChartConfigSchema, - externalDashboardBarRawSqlChartConfigSchema, - externalDashboardNumberRawSqlChartConfigSchema, + externalDashboardLineRawSqlChartConfigSchema.extend({ + name: alertChartConfigNameSchema, + where: rejectedAlertRawSqlWhereField, + whereLanguage: rejectedAlertRawSqlWhereField, + }), + externalDashboardBarRawSqlChartConfigSchema.extend({ + name: alertChartConfigNameSchema, + where: rejectedAlertRawSqlWhereField, + whereLanguage: rejectedAlertRawSqlWhereField, + }), + externalDashboardNumberRawSqlChartConfigSchema.extend({ + name: alertChartConfigNameSchema, + where: rejectedAlertRawSqlWhereField, + whereLanguage: rejectedAlertRawSqlWhereField, + }), ], ); @@ -893,6 +940,26 @@ const zExternalInlineAlert = z.object({ chartConfig: externalAlertChartConfigSchema, }); +// The branch schemas are strip-mode z.objects, so a stray chartConfig on a +// non-inline alert would be silently discarded before any superRefine could +// see it. Declaring the field as a rejected never (same pattern as the MCP +// tile-level `where` rejection) turns that into a 400, matching both the +// OpenAPI contract ("rejected otherwise") and the MCP surface. External-only: +// the internal schema keeps its historical strip semantics. +const rejectedNonInlineChartConfigField = z + .never({ + invalid_type_error: 'chartConfig is only supported when source is "inline"', + }) + .optional(); + +const zExternalSavedSearchAlert = zSavedSearchAlert.extend({ + chartConfig: rejectedNonInlineChartConfigField, +}); + +const zExternalTileAlert = zTileAlert.extend({ + chartConfig: rejectedNonInlineChartConfigField, +}); + const alertBaseSchema = z.object({ channel: zAlertChannel.optional(), channels: zAlertChannels.optional(), @@ -914,7 +981,9 @@ const alertBaseSchema = z.object({ // router converts it to the internal AlertChartConfig shape before // validateAlertInput/createAlert. export const alertSchema = alertBaseSchema - .and(zSavedSearchAlert.or(zTileAlert).or(zExternalInlineAlert)) + .and( + zExternalSavedSearchAlert.or(zExternalTileAlert).or(zExternalInlineAlert), + ) .superRefine(validateAlertChannelSelection) .superRefine(validateAlertScheduleOffsetMinutes) .superRefine(validateAlertThresholdMax); From 1d0a5d50818457f86ffc080a9290954b2cf3d92f Mon Sep 17 00:00:00 2001 From: Warren Lee <5959690+wrn14897@users.noreply.github.com> Date: Tue, 1 Sep 2026 16:44:03 -0700 Subject: [PATCH 4/8] refactor: address inline chart alert review minors (HDX-5191) - Apply the dashboard formula source-kind gate to inline builder configs, so a formula on a session source is rejected here too - Break the utils/externalApi -> v2 router-util require cycle: the chartConfig-attaching translator now lives in the v2 util and the three single-alert call sites use it directly - Extract the shared configType routing predicate and post-parse rule set (asRatio, formulas, number single-select) used by both the dashboard tile config and the inline alert config schemas - Cover the empty-select case that used to 500 on GET by id --- .changeset/inline-alerts-external-api-mcp.md | 12 +- packages/api/src/controllers/alerts.ts | 14 ++ packages/api/src/mcp/tools/alerts/getAlert.ts | 7 +- .../api/src/mcp/tools/alerts/saveAlert.ts | 18 +- .../external-api/__tests__/alerts.int.test.ts | 40 ++++ .../api/src/routers/external-api/v2/alerts.ts | 19 +- .../utils/__tests__/alertChartConfig.test.ts | 14 ++ .../external-api/v2/utils/alertChartConfig.ts | 26 +++ .../src/utils/__tests__/externalApi.test.ts | 26 +-- packages/api/src/utils/externalApi.ts | 27 +-- packages/api/src/utils/zod.ts | 190 ++++++++---------- 11 files changed, 226 insertions(+), 167 deletions(-) diff --git a/.changeset/inline-alerts-external-api-mcp.md b/.changeset/inline-alerts-external-api-mcp.md index 541bd149a3..1b7a31b802 100644 --- a/.changeset/inline-alerts-external-api-mcp.md +++ b/.changeset/inline-alerts-external-api-mcp.md @@ -6,12 +6,12 @@ Support inline chart alerts (source `inline` + `chartConfig`) in the external API v2 and the MCP `clickstack_save_alert` / `clickstack_get_alert` tools. Inline alerts can now be created, updated, listed, and deleted through `/api/v2/alerts` using the same tile-config dialect as v2 dashboards, with the -same validation rules as the internal API (display-type allowlist, metric -formula validation, raw SQL template validation, and team-scoped -source/connection ownership). Passing a `chartConfig` to a `tile` or -`saved_search` alert is now rejected instead of silently dropped, and reading -an inline alert whose config carries internal-only fields omits `chartConfig` -rather than returning a lossy approximation. +same validation rules as the internal API: display-type allowlist, metric +formula validation, the formula source-kind gate, raw SQL template validation, +and team-scoped source/connection ownership. Passing a `chartConfig` to a +`tile` or `saved_search` alert is now rejected instead of silently dropped, +and reading an inline alert whose config carries internal-only fields omits +`chartConfig` rather than returning a lossy approximation. Also fixes the external v2 dashboards dialect dropping the gauge `isDelta` flag when converting tile select items to the internal shape. diff --git a/packages/api/src/controllers/alerts.ts b/packages/api/src/controllers/alerts.ts index feba0444f9..44fe6662e3 100644 --- a/packages/api/src/controllers/alerts.ts +++ b/packages/api/src/controllers/alerts.ts @@ -1,6 +1,7 @@ import { displayTypeSupportsBuilderAlerts, displayTypeSupportsRawSqlAlerts, + isFormulaSourceKind, validateRawSqlForAlert, } from '@hyperdx/common-utils/dist/core/utils'; import { isRawSqlSavedChartConfig } from '@hyperdx/common-utils/dist/guards'; @@ -189,6 +190,19 @@ export const validateAlertInput = async ( if (source == null) { throw new Api400Error('Source not found'); } + + // Same formula source-kind gate as dashboard tiles ("Add Formula" is + // disabled in the editor for these kinds), so the API cannot persist a + // config the editor refuses. + if ( + 'formulas' in chartConfig && + (chartConfig.formulas?.length ?? 0) > 0 && + !isFormulaSourceKind(source.kind) + ) { + throw new Api400Error( + 'Alerts with formulas require a Metric, Log, or Trace source', + ); + } } } diff --git a/packages/api/src/mcp/tools/alerts/getAlert.ts b/packages/api/src/mcp/tools/alerts/getAlert.ts index ada1f63fba..6e8e53b5de 100644 --- a/packages/api/src/mcp/tools/alerts/getAlert.ts +++ b/packages/api/src/mcp/tools/alerts/getAlert.ts @@ -11,7 +11,7 @@ import { mcpUserError, validateObjectId } from '@/mcp/utils/errors'; import Alert from '@/models/alert'; import type { IDashboard } from '@/models/dashboard'; import type { ISavedSearch } from '@/models/savedSearch'; -import { translateAlertDocumentToExternalAlert } from '@/utils/externalApi'; +import { translateAlertDocumentToExternalAlertWithChartConfig } from '@/routers/external-api/v2/utils/alertChartConfig'; function deriveAlertName(alert: { name?: string | null; @@ -124,9 +124,8 @@ export function registerGetAlert({ return mcpUserError('Alert not found'); } - const external = translateAlertDocumentToExternalAlert(alert, { - includeChartConfig: true, - }); + const external = + translateAlertDocumentToExternalAlertWithChartConfig(alert); // Populate refs so deriveAlertName can fall back to the // saved search / dashboard name when the alert has no explicit name. diff --git a/packages/api/src/mcp/tools/alerts/saveAlert.ts b/packages/api/src/mcp/tools/alerts/saveAlert.ts index 7befc3c72c..fa3fee9418 100644 --- a/packages/api/src/mcp/tools/alerts/saveAlert.ts +++ b/packages/api/src/mcp/tools/alerts/saveAlert.ts @@ -16,9 +16,11 @@ import { validateObjectId, } from '@/mcp/utils/errors'; import { AlertSource } from '@/models/alert'; -import { convertExternalAlertChartConfigToInternal } from '@/routers/external-api/v2/utils/alertChartConfig'; +import { + convertExternalAlertChartConfigToInternal, + translateAlertDocumentToExternalAlertWithChartConfig, +} from '@/routers/external-api/v2/utils/alertChartConfig'; import { BaseError } from '@/utils/errors'; -import { translateAlertDocumentToExternalAlert } from '@/utils/externalApi'; import { externalAlertChartConfigSchema } from '@/utils/zod'; import { mcpSaveAlertSchema, validateSaveAlertInput } from './schemas'; @@ -153,9 +155,9 @@ export function registerSaveAlert({ type: 'text' as const, text: JSON.stringify( { - ...translateAlertDocumentToExternalAlert(updated, { - includeChartConfig: true, - }), + ...translateAlertDocumentToExternalAlertWithChartConfig( + updated, + ), ...(frontendUrl ? { url: `${frontendUrl}/alerts` } : {}), }, null, @@ -178,9 +180,9 @@ export function registerSaveAlert({ type: 'text' as const, text: JSON.stringify( { - ...translateAlertDocumentToExternalAlert(created, { - includeChartConfig: true, - }), + ...translateAlertDocumentToExternalAlertWithChartConfig( + created, + ), ...(frontendUrl ? { url: `${frontendUrl}/alerts` } : {}), }, null, diff --git a/packages/api/src/routers/external-api/__tests__/alerts.int.test.ts b/packages/api/src/routers/external-api/__tests__/alerts.int.test.ts index 0665c3f76d..2f7e99c391 100644 --- a/packages/api/src/routers/external-api/__tests__/alerts.int.test.ts +++ b/packages/api/src/routers/external-api/__tests__/alerts.int.test.ts @@ -1137,6 +1137,46 @@ describe('External API Alerts', () => { }); }); + it('rejects formulas on a formula-incapable source kind (session)', async () => { + // Same gate as dashboard tiles: the editor disables "Add Formula" for + // session sources, so the alert API must refuse the config too. + const { connection, source } = await makeSource(); + const webhook = await createTestWebhook(); + + const sessionSource = await Source.create({ + kind: SourceKind.Session, + team: team._id, + from: { databaseName: 'default', tableName: 'rrweb_events' }, + timestampValueExpression: 'Timestamp', + traceSourceId: source._id.toString(), + connection: connection._id, + name: 'Sessions', + }); + + const withFormulas = (sourceId: string) => + makeInlineAlertInput( + { + ...makeBuilderChartConfig(sourceId), + select: [ + { aggFn: 'count', where: 'level:error' }, + { aggFn: 'count', where: '' }, + ], + formulas: [{ expression: 'A / B * 100' }], + }, + webhook._id.toString(), + ); + + const rejected = await authRequest('post', ALERTS_BASE_URL) + .send(withFormulas(sessionSource._id.toString())) + .expect(400); + expect(JSON.stringify(rejected.body)).toContain('formulas require'); + + // The identical config on a formula-capable source is accepted. + await authRequest('post', ALERTS_BASE_URL) + .send(withFormulas(source._id.toString())) + .expect(200); + }); + it('rejects asRatio without exactly two select items', async () => { const { source } = await makeSource(); const webhook = await createTestWebhook(); diff --git a/packages/api/src/routers/external-api/v2/alerts.ts b/packages/api/src/routers/external-api/v2/alerts.ts index 7256d2125d..a1b44d7468 100644 --- a/packages/api/src/routers/external-api/v2/alerts.ts +++ b/packages/api/src/routers/external-api/v2/alerts.ts @@ -12,7 +12,10 @@ import { validateAlertInput, } from '@/controllers/alerts'; import { AlertSource } from '@/models/alert'; -import { convertExternalAlertChartConfigToInternal } from '@/routers/external-api/v2/utils/alertChartConfig'; +import { + convertExternalAlertChartConfigToInternal, + translateAlertDocumentToExternalAlertWithChartConfig, +} from '@/routers/external-api/v2/utils/alertChartConfig'; import { processRequestWithEnhancedErrors as processRequest, validateRequestWithEnhancedErrors as validateRequest, @@ -480,9 +483,7 @@ router.get( } return res.json({ - data: translateAlertDocumentToExternalAlert(alert, { - includeChartConfig: true, - }), + data: translateAlertDocumentToExternalAlertWithChartConfig(alert), }); } catch (e) { next(e); @@ -700,9 +701,9 @@ router.post( const createdAlert = await createAlert(teamId, alertInput, userId); return res.json({ - data: translateAlertDocumentToExternalAlert(createdAlert, { - includeChartConfig: true, - }), + data: translateAlertDocumentToExternalAlertWithChartConfig( + createdAlert, + ), }); } catch (e) { next(e); @@ -806,9 +807,7 @@ router.put( } res.json({ - data: translateAlertDocumentToExternalAlert(alert, { - includeChartConfig: true, - }), + data: translateAlertDocumentToExternalAlertWithChartConfig(alert), }); } catch (e) { next(e); diff --git a/packages/api/src/routers/external-api/v2/utils/__tests__/alertChartConfig.test.ts b/packages/api/src/routers/external-api/v2/utils/__tests__/alertChartConfig.test.ts index e846c9aaaf..e5eaee9602 100644 --- a/packages/api/src/routers/external-api/v2/utils/__tests__/alertChartConfig.test.ts +++ b/packages/api/src/routers/external-api/v2/utils/__tests__/alertChartConfig.test.ts @@ -265,6 +265,20 @@ describe('convertAlertChartConfigToExternal', () => { ).toBeUndefined(); }); + it('refuses an empty select instead of throwing on a number config', () => { + // A persisted number config with select: [] used to TypeError in the + // tile converter (select[0] is undefined) and surface as a 500 from + // GET /api/v2/alerts/:id. + expect( + convertAlertChartConfigToExternal( + baseInternal({ displayType: DisplayType.Number, select: [] }), + ), + ).toBeUndefined(); + expect( + convertAlertChartConfigToExternal(baseInternal({ select: [] })), + ).toBeUndefined(); + }); + it('refuses more than 20 select items', () => { expect( convertAlertChartConfigToExternal( diff --git a/packages/api/src/routers/external-api/v2/utils/alertChartConfig.ts b/packages/api/src/routers/external-api/v2/utils/alertChartConfig.ts index 65d5fff4d1..9e080ae416 100644 --- a/packages/api/src/routers/external-api/v2/utils/alertChartConfig.ts +++ b/packages/api/src/routers/external-api/v2/utils/alertChartConfig.ts @@ -4,6 +4,11 @@ import { } from '@hyperdx/common-utils/dist/types'; import { omit } from 'lodash'; +import { AlertDocument, AlertSource, IAlert } from '@/models/alert'; +import { + ExternalAlert, + translateAlertDocumentToExternalAlert, +} from '@/utils/externalApi'; import logger from '@/utils/logger'; import { externalAlertBuilderChartConfigSchema, @@ -320,3 +325,24 @@ export function convertAlertChartConfigToExternal( return undefined; } } + +/** + * `translateAlertDocumentToExternalAlert` plus the inline alert's chartConfig + * (external dialect) when one is faithfully representable. Single-alert + * responses (GET by id, POST, PUT, MCP detail) use this; list responses use + * the bare translator so they stay lean. Lives here (not utils/externalApi) + * to keep the utils -> router-utils import direction one-way. + */ +export function translateAlertDocumentToExternalAlertWithChartConfig( + alert: AlertDocument, +): ExternalAlert { + const external = translateAlertDocumentToExternalAlert(alert); + const alertObj: Pick = alert.toJSON + ? alert.toJSON() + : alert; + const chartConfig = + alertObj.source === AlertSource.INLINE && alertObj.chartConfig != null + ? convertAlertChartConfigToExternal(alertObj.chartConfig) + : undefined; + return { ...external, ...(chartConfig && { chartConfig }) }; +} diff --git a/packages/api/src/utils/__tests__/externalApi.test.ts b/packages/api/src/utils/__tests__/externalApi.test.ts index 8dddca617b..e0d79dad55 100644 --- a/packages/api/src/utils/__tests__/externalApi.test.ts +++ b/packages/api/src/utils/__tests__/externalApi.test.ts @@ -7,6 +7,7 @@ import { AlertState, AlertThresholdType, } from '@/models/alert'; +import { translateAlertDocumentToExternalAlertWithChartConfig } from '@/routers/external-api/v2/utils/alertChartConfig'; import { translateAlertDocumentToExternalAlert } from '@/utils/externalApi'; // A channel type this repo doesn't define -- see @@ -140,9 +141,8 @@ describe('utils/externalApi', () => { chartConfig: internalChartConfig, }); - const translated = translateAlertDocumentToExternalAlert(alert, { - includeChartConfig: true, - }); + const translated = + translateAlertDocumentToExternalAlertWithChartConfig(alert); // seriesReturnType 'ratio' becomes asRatio only with exactly two // select items — this single-select config maps to false. @@ -172,9 +172,8 @@ describe('utils/externalApi', () => { }, }); - const translated = translateAlertDocumentToExternalAlert(alert, { - includeChartConfig: true, - }); + const translated = + translateAlertDocumentToExternalAlertWithChartConfig(alert); expect(translated.chartConfig).toMatchObject({ configType: 'sql', @@ -191,9 +190,8 @@ describe('utils/externalApi', () => { chartConfig: internalChartConfig, }); - const translated = translateAlertDocumentToExternalAlert(alert, { - includeChartConfig: true, - }); + const translated = + translateAlertDocumentToExternalAlertWithChartConfig(alert); expect(translated.chartConfig).toBeUndefined(); }); @@ -206,9 +204,8 @@ describe('utils/externalApi', () => { chartConfig: null, }); - const translated = translateAlertDocumentToExternalAlert(alert, { - includeChartConfig: true, - }); + const translated = + translateAlertDocumentToExternalAlertWithChartConfig(alert); expect(translated.chartConfig).toBeUndefined(); expect('chartConfig' in translated).toBe(false); @@ -224,9 +221,8 @@ describe('utils/externalApi', () => { }, }); - const translated = translateAlertDocumentToExternalAlert(alert, { - includeChartConfig: true, - }); + const translated = + translateAlertDocumentToExternalAlertWithChartConfig(alert); expect(translated.chartConfig).toBeUndefined(); }); diff --git a/packages/api/src/utils/externalApi.ts b/packages/api/src/utils/externalApi.ts index 1b0810166d..60a9107f44 100644 --- a/packages/api/src/utils/externalApi.ts +++ b/packages/api/src/utils/externalApi.ts @@ -18,13 +18,11 @@ import { AlertChannel, AlertDocument, AlertInterval, - AlertSource, AlertState, getAlertChannels, IAlert, } from '@/models/alert'; import type { DashboardDocument } from '@/models/dashboard'; -import { convertAlertChartConfigToExternal } from '@/routers/external-api/v2/utils/alertChartConfig'; import { SeriesTile } from '@/routers/external-api/v2/utils/dashboards'; import { ExternalAlertChartConfig, @@ -349,19 +347,14 @@ function transformErrorsToExternalErrors( })); } +// Note: this translator does not attach an inline alert's `chartConfig`. +// Single-alert responses (GET by id, POST, PUT, MCP detail) attach it via +// `translateAlertDocumentToExternalAlertWithChartConfig` (v2 router util) — +// keeping the converter out of here avoids a runtime import cycle with the +// v2 utils, and list responses stay lean so a team with hundreds of raw-SQL +// inline alerts does not ship every template on each page. export function translateAlertDocumentToExternalAlert( alert: AlertDocument, - { - includeChartConfig = false, - }: { - /** - * Attach an inline alert's chartConfig (external dialect). Single-alert - * responses (GET by id, POST, PUT, MCP detail) opt in; list responses - * leave it off so a team with hundreds of raw-SQL inline alerts does not - * ship every template on each page. - */ - includeChartConfig?: boolean; - } = {}, ): ExternalAlert { // Convert to plain object if it's a Mongoose document const alertObj: AlertDocumentObject = alert.toJSON @@ -370,13 +363,6 @@ export function translateAlertDocumentToExternalAlert( const channels = getAlertChannels(alertObj); - const externalChartConfig = - includeChartConfig && - alertObj.source === AlertSource.INLINE && - alertObj.chartConfig != null - ? convertAlertChartConfigToExternal(alertObj.chartConfig) - : undefined; - // Copy all fields, renaming _id to id, ensuring ObjectId's are strings const result = { id: alertObj._id.toString(), @@ -405,7 +391,6 @@ export function translateAlertDocumentToExternalAlert( dashboardId: alertObj.dashboard?.toString(), savedSearchId: alertObj.savedSearch?.toString(), groupBy: alertObj.groupBy ?? undefined, - ...(externalChartConfig && { chartConfig: externalChartConfig }), silenced: transformSilencedToExternalSilenced(alertObj.silenced), executionErrors: transformErrorsToExternalErrors(alertObj.executionErrors), createdAt: hasCreatedAt(alertObj) diff --git a/packages/api/src/utils/zod.ts b/packages/api/src/utils/zod.ts index 6a1de09ea7..7544e96f7b 100644 --- a/packages/api/src/utils/zod.ts +++ b/packages/api/src/utils/zod.ts @@ -569,6 +569,69 @@ export type ExternalDashboardRawSqlTileConfig = z.infer< typeof externalDashboardRawSqlTileConfigSchema >; +/** + * Routing predicate for the configType-routed config schemas below + * (dashboard tiles and inline alert configs): raw SQL configs carry + * `configType: 'sql'`, everything else is the builder dialect. + */ +const isRawSqlRoutedConfig = (data: unknown): boolean => + data !== null && + typeof data === 'object' && + 'configType' in data && + data.configType === 'sql'; + +/** + * The shared post-parse rule set for configType-routed config schemas + * (dashboard tiles and inline alert configs) — kept in one place so the two + * surfaces cannot drift: + * - `asRatio` requires exactly two select items. + * - Builder configs: metric-formula validation (expression parse + + * series-ref range checks, asRatio mutual exclusion, number single-formula + * cap) — shared with the chart editor's save-time rules via common-utils. + * - Builder number configs without formulas display their single select + * item; the relaxed `.min(1).max(20)` on the number schema exists only so + * formula operands fit (the editor enforces the same in + * `validateChartForm`). + */ +const addConfigTypeRoutedIssues = ( + data: { + displayType?: string; + configType?: string; + select?: unknown; + formulas?: { expression: string }[]; + asRatio?: boolean; + }, + ctx: z.RefinementCtx, + { numberSelectMessage }: { numberSelectMessage: string }, +): void => { + if ( + data.asRatio && + (!Array.isArray(data.select) || data.select.length !== 2) + ) { + ctx.addIssue({ + code: z.ZodIssueCode.custom, + message: 'asRatio can only be used with exactly two select items', + }); + } + + if (!('configType' in data)) { + validateChartConfigFormulas(data, ctx); + + if ( + data.displayType === 'number' && + (data.formulas?.length ?? 0) === 0 && + Array.isArray(data.select) && + data.select.length !== 1 + ) { + ctx.addIssue({ + code: z.ZodIssueCode.custom, + message: numberSelectMessage, + path: ['select'], + }); + } + } +}; + const externalDashboardTileConfigSchema = z .custom< ExternalDashboardRawSqlTileConfig | ExternalDashboardBuilderTileConfig @@ -577,13 +640,9 @@ const externalDashboardTileConfigSchema = z // Route to the correct sub-schema based on configType so Zod's // discriminatedUnion can produce targeted field-level errors rather // than a generic union failure. - const schema = - data !== null && - typeof data === 'object' && - 'configType' in data && - data.configType === 'sql' - ? externalDashboardRawSqlTileConfigSchema - : externalDashboardBuilderTileConfigSchema; + const schema = isRawSqlRoutedConfig(data) + ? externalDashboardRawSqlTileConfigSchema + : externalDashboardBuilderTileConfigSchema; const result = schema.safeParse(data); if (!result.success) { @@ -593,55 +652,18 @@ const externalDashboardTileConfigSchema = z return; } - if ( - 'asRatio' in data && - data.asRatio && - (!Array.isArray(data.select) || data.select.length !== 2) - ) { - ctx.addIssue({ - code: z.ZodIssueCode.custom, - message: 'asRatio can only be used with exactly two select items', - }); - } - - if (!('configType' in data)) { - // Metric formulas (builder tiles only): expression parse + series-ref - // range checks, asRatio mutual exclusion, and the number-tile - // single-formula cap — shared with the chart editor's save-time rules - // via common-utils. - validateChartConfigFormulas(data, ctx); - - // A number tile without formulas displays its single select item; the - // relaxed `.min(1).max(20)` on the number schema exists only so - // formula operands fit (the editor enforces the same in - // `validateChartForm`). - if ( - data.displayType === 'number' && - !('formulas' in data && (data.formulas?.length ?? 0) > 0) && - Array.isArray(data.select) && - data.select.length !== 1 - ) { - ctx.addIssue({ - code: z.ZodIssueCode.custom, - message: - 'Number tiles support a single select item unless formulas are used (extra items are formula operands)', - path: ['select'], - }); - } - } + addConfigTypeRoutedIssues(data, ctx, { + numberSelectMessage: + 'Number tiles support a single select item unless formulas are used (extra items are formula operands)', + }); }) .transform(data => { // Re-parse through the appropriate sub-schema to strip unknown fields. // Safe to call .parse() here — superRefine already validated the data, // so this is guaranteed to succeed. - const schema = - data !== null && - typeof data === 'object' && - 'configType' in data && - data.configType === 'sql' - ? externalDashboardRawSqlTileConfigSchema - : externalDashboardBuilderTileConfigSchema; - return schema.parse(data); + return isRawSqlRoutedConfig(data) + ? externalDashboardRawSqlTileConfigSchema.parse(data) + : externalDashboardBuilderTileConfigSchema.parse(data); }); export type ExternalDashboardTileConfig = z.infer< @@ -865,19 +887,16 @@ export type ExternalAlertChartConfig = | z.infer; // Mirrors the route-by-configType superRefine/transform pattern of -// externalDashboardTileConfigSchema above, so inline alert configs get the -// same targeted field-level errors, formula validation, and unknown-field -// stripping as dashboard tile configs. +// externalDashboardTileConfigSchema above (same routing predicate and the +// shared addConfigTypeRoutedIssues rule set), so inline alert configs get +// the same targeted field-level errors, formula validation, and +// unknown-field stripping as dashboard tile configs. export const externalAlertChartConfigSchema = z .custom() .superRefine((data, ctx) => { - const schema = - data !== null && - typeof data === 'object' && - 'configType' in data && - data.configType === 'sql' - ? externalAlertRawSqlChartConfigSchema - : externalAlertBuilderChartConfigSchema; + const schema = isRawSqlRoutedConfig(data) + ? externalAlertRawSqlChartConfigSchema + : externalAlertBuilderChartConfigSchema; const result = schema.safeParse(data); if (!result.success) { @@ -887,52 +906,17 @@ export const externalAlertChartConfigSchema = z return; } - if ( - 'asRatio' in data && - data.asRatio && - (!Array.isArray(data.select) || data.select.length !== 2) - ) { - ctx.addIssue({ - code: z.ZodIssueCode.custom, - message: 'asRatio can only be used with exactly two select items', - }); - } - - if (!('configType' in data)) { - // Same shared metric-formula rules the tile-config refinement applies: - // expression parse + series-ref range checks, asRatio mutual exclusion, - // and the number single-formula cap. - validateChartConfigFormulas(data, ctx); - - // A number chart without formulas displays its single select item; the - // relaxed `.min(1).max(20)` on the number schema exists only so formula - // operands fit. - if ( - data.displayType === 'number' && - !('formulas' in data && (data.formulas?.length ?? 0) > 0) && - Array.isArray(data.select) && - data.select.length !== 1 - ) { - ctx.addIssue({ - code: z.ZodIssueCode.custom, - message: - 'Number charts support a single select item unless formulas are used (extra items are formula operands)', - path: ['select'], - }); - } - } + addConfigTypeRoutedIssues(data, ctx, { + numberSelectMessage: + 'Number charts support a single select item unless formulas are used (extra items are formula operands)', + }); }) .transform(data => { // Re-parse through the appropriate sub-schema to strip unknown fields. // Safe to call .parse() here — superRefine already validated the data. - const schema = - data !== null && - typeof data === 'object' && - 'configType' in data && - data.configType === 'sql' - ? externalAlertRawSqlChartConfigSchema - : externalAlertBuilderChartConfigSchema; - return schema.parse(data); + return isRawSqlRoutedConfig(data) + ? externalAlertRawSqlChartConfigSchema.parse(data) + : externalAlertBuilderChartConfigSchema.parse(data); }); const zExternalInlineAlert = z.object({ From 04dad3a8588b8b565b19ce0953eaba5eb8a17092 Mon Sep 17 00:00:00 2001 From: Warren Lee <5959690+wrn14897@users.noreply.github.com> Date: Tue, 1 Sep 2026 17:49:47 -0700 Subject: [PATCH 5/8] fix: close configType rule-set bypass and surface alert union errors (HDX-5191) - Gate the builder-only rules on isRawSqlRoutedConfig, the same predicate that picks the parsing schema: 'configType' in data let any non-'sql' value route to the builder union while skipping formula and number-select validation - Reject an unrecognized configType outright instead of stripping it and persisting the body as a builder config - Discriminate the alert source unions on 'source' so schema messages survive: a failing .or() branch collapses to 'Invalid input' because the external error formatter drops unionErrors - Assert on res.body.message in the inline alert tests so the collapse cannot regress unnoticed --- .changeset/inline-alerts-external-api-mcp.md | 9 ++- .../routers/api/__tests__/alerts.int.test.ts | 6 +- .../external-api/__tests__/alerts.int.test.ts | 78 ++++++++++++++++-- .../__tests__/dashboards.int.test.ts | 12 +++ .../utils/__tests__/alertChartConfig.test.ts | 79 +++++++++++++++++++ packages/api/src/utils/zod.ts | 61 +++++++++++++- 6 files changed, 233 insertions(+), 12 deletions(-) diff --git a/.changeset/inline-alerts-external-api-mcp.md b/.changeset/inline-alerts-external-api-mcp.md index 1b7a31b802..bf8d479943 100644 --- a/.changeset/inline-alerts-external-api-mcp.md +++ b/.changeset/inline-alerts-external-api-mcp.md @@ -13,5 +13,10 @@ and team-scoped source/connection ownership. Passing a `chartConfig` to a and reading an inline alert whose config carries internal-only fields omits `chartConfig` rather than returning a lossy approximation. -Also fixes the external v2 dashboards dialect dropping the gauge `isDelta` -flag when converting tile select items to the internal shape. +Also fixes three pre-existing issues on the external v2 dashboards path: the +dialect dropped the gauge `isDelta` flag when converting tile select items to +the internal shape; a tile config carrying an unrecognized `configType` (e.g. +`promql`) was silently stored as a builder config and skipped the formula and +number-select rules, and is now rejected; and validation errors on alert +bodies no longer collapse to `Invalid input` — the schema reports the failing +branch's own message. diff --git a/packages/api/src/routers/api/__tests__/alerts.int.test.ts b/packages/api/src/routers/api/__tests__/alerts.int.test.ts index acd05b60f8..2c549bac7c 100644 --- a/packages/api/src/routers/api/__tests__/alerts.int.test.ts +++ b/packages/api/src/routers/api/__tests__/alerts.int.test.ts @@ -1838,7 +1838,7 @@ describe('alerts router', () => { const base = makeAlertChartConfig({ sourceId: source._id.toString() }); // Formula referencing a nonexistent series (only A exists) - await agent + const unknownSeries = await agent .post('/alerts') .send( makeInlineAlertInput({ @@ -1847,6 +1847,10 @@ describe('alerts router', () => { }), ) .expect(400); + // The alert body's source union must be discriminated for this to + // surface: a plain `.or()` reports only a generic union failure and + // buries the real issue in unionErrors. + expect(JSON.stringify(unknownSeries.body)).toContain('Unknown series'); // Malformed expression await agent diff --git a/packages/api/src/routers/external-api/__tests__/alerts.int.test.ts b/packages/api/src/routers/external-api/__tests__/alerts.int.test.ts index 2f7e99c391..19bf4a246e 100644 --- a/packages/api/src/routers/external-api/__tests__/alerts.int.test.ts +++ b/packages/api/src/routers/external-api/__tests__/alerts.int.test.ts @@ -913,13 +913,61 @@ describe('External API Alerts', () => { .expect(400); }); + it('rejects an unsupported configType instead of routing it to the builder dialect', async () => { + // An otherwise-valid builder body carrying configType: 'promql' parses + // against the builder union (the unknown keys are stripped), so without + // an explicit check it would persist as a builder config AND skip the + // builder-only rules — here a formula referencing a nonexistent series, + // which then throws on every check-alerts evaluation. + const { source } = await makeSource(); + const webhook = await createTestWebhook(); + const base = makeBuilderChartConfig(source._id.toString()); + + const withFormula = await authRequest('post', ALERTS_BASE_URL) + .send( + makeInlineAlertInput( + { + ...base, + configType: 'promql', + formulas: [{ expression: 'B * 2' }], + }, + webhook._id.toString(), + ), + ) + .expect(400); + expect(withFormula.body.message).toContain( + 'configType must be "sql" or omitted', + ); + + // Same bypass for the number single-select rule. + const numberConfig = await authRequest('post', ALERTS_BASE_URL) + .send( + makeInlineAlertInput( + { + ...base, + configType: 'promql', + displayType: 'number', + select: [ + { aggFn: 'count', where: 'level:error' }, + { aggFn: 'count', where: '' }, + ], + }, + webhook._id.toString(), + ), + ) + .expect(400); + expect(numberConfig.body.message).toContain( + 'configType must be "sql" or omitted', + ); + }); + it('validates metric formulas on builder chart configs', async () => { const { source } = await makeSource(); const webhook = await createTestWebhook(); const base = makeBuilderChartConfig(source._id.toString()); // Formula referencing a nonexistent series (only A exists) - await authRequest('post', ALERTS_BASE_URL) + const unknownSeries = await authRequest('post', ALERTS_BASE_URL) .send( makeInlineAlertInput( { ...base, formulas: [{ expression: 'B * 2' }] }, @@ -927,6 +975,9 @@ describe('External API Alerts', () => { ), ) .expect(400); + // The schema-level message must survive the alert body's source union — + // a plain `.or()` collapses every branch issue to "Invalid input". + expect(unknownSeries.body.message).toContain('Unknown series "B"'); // Malformed expression await authRequest('post', ALERTS_BASE_URL) @@ -1101,7 +1152,7 @@ describe('External API Alerts', () => { // A number chart without formulas displays a single select item; // extra items are only valid as formula operands. - await authRequest('post', ALERTS_BASE_URL) + const multiSelect = await authRequest('post', ALERTS_BASE_URL) .send( makeInlineAlertInput( { @@ -1115,6 +1166,9 @@ describe('External API Alerts', () => { ), ) .expect(400); + expect(multiSelect.body.message).toContain( + 'Number charts support a single select item', + ); // With a formula, the extra select items are its operands. const withFormula = await authRequest('post', ALERTS_BASE_URL) @@ -1181,7 +1235,7 @@ describe('External API Alerts', () => { const { source } = await makeSource(); const webhook = await createTestWebhook(); - await authRequest('post', ALERTS_BASE_URL) + const res = await authRequest('post', ALERTS_BASE_URL) .send( makeInlineAlertInput( makeBuilderChartConfig(source._id.toString(), { asRatio: true }), @@ -1189,6 +1243,9 @@ describe('External API Alerts', () => { ), ) .expect(400); + expect(res.body.message).toContain( + 'asRatio can only be used with exactly two select items', + ); }); it('round-trips a single-alert GET body back through PUT unchanged', async () => { @@ -1258,7 +1315,7 @@ describe('External API Alerts', () => { const savedSearch = await createTestSavedSearch(); const chartConfig = makeBuilderChartConfig(source._id.toString()); - await authRequest('post', ALERTS_BASE_URL) + const tileRes = await authRequest('post', ALERTS_BASE_URL) .send({ source: AlertSource.TILE, dashboardId: dashboard._id.toString(), @@ -1270,8 +1327,11 @@ describe('External API Alerts', () => { channel: { type: 'webhook', webhookId: webhook._id.toString() }, }) .expect(400); + expect(tileRes.body.message).toContain( + 'chartConfig is only supported when source is "inline"', + ); - await authRequest('post', ALERTS_BASE_URL) + const savedSearchRes = await authRequest('post', ALERTS_BASE_URL) .send({ source: AlertSource.SAVED_SEARCH, savedSearchId: savedSearch._id.toString(), @@ -1282,6 +1342,9 @@ describe('External API Alerts', () => { channel: { type: 'webhook', webhookId: webhook._id.toString() }, }) .expect(400); + expect(savedSearchRes.body.message).toContain( + 'chartConfig is only supported when source is "inline"', + ); // PUT applies the same rule. const { alert } = await createTestAlert(); @@ -1353,7 +1416,7 @@ describe('External API Alerts', () => { const { connection } = await makeSource(); const webhook = await createTestWebhook(); - await authRequest('post', ALERTS_BASE_URL) + const res = await authRequest('post', ALERTS_BASE_URL) .send( makeInlineAlertInput( { @@ -1367,6 +1430,9 @@ describe('External API Alerts', () => { ), ) .expect(400); + expect(res.body.message).toContain( + 'Raw SQL alert configs have no chart-level where', + ); }); it('omits chartConfig for internally-authored configs the dialect cannot express, and a blind echo-PUT fails loudly', async () => { diff --git a/packages/api/src/routers/external-api/__tests__/dashboards.int.test.ts b/packages/api/src/routers/external-api/__tests__/dashboards.int.test.ts index bdc6b089f9..7b99439f34 100644 --- a/packages/api/src/routers/external-api/__tests__/dashboards.int.test.ts +++ b/packages/api/src/routers/external-api/__tests__/dashboards.int.test.ts @@ -6369,6 +6369,18 @@ describe('External API v2 Dashboards - new format', () => { ); }); + it('rejects an unsupported configType instead of routing it to the builder dialect', async () => { + // A body carrying an unrecognized configType parses fine against the + // builder union (the unknown keys are stripped), so without an explicit + // check it would persist as a builder tile AND skip the builder-only + // rules — here a formula referencing a nonexistent series. + const res = await postTile({ + configType: 'promql', + formulas: [{ expression: 'C * 2' }], + }).expect(400); + expect(res.body.message).toContain('configType must be "sql" or omitted'); + }); + it('round-trips formulas on a log/trace event source', async () => { // Event-source formulas are accepted and round-trip just like // metric formulas. diff --git a/packages/api/src/routers/external-api/v2/utils/__tests__/alertChartConfig.test.ts b/packages/api/src/routers/external-api/v2/utils/__tests__/alertChartConfig.test.ts index e5eaee9602..b06509ec22 100644 --- a/packages/api/src/routers/external-api/v2/utils/__tests__/alertChartConfig.test.ts +++ b/packages/api/src/routers/external-api/v2/utils/__tests__/alertChartConfig.test.ts @@ -14,6 +14,7 @@ import { import { externalAlertBuilderChartConfigSchema, ExternalAlertChartConfig, + externalAlertChartConfigSchema, externalAlertRawSqlChartConfigSchema, } from '@/utils/zod'; @@ -46,6 +47,84 @@ const baseInternal = ( ...overrides, }) as AlertChartConfig; +describe('externalAlertChartConfigSchema configType routing', () => { + const builderBody = (overrides: Record = {}) => ({ + displayType: DisplayType.Line, + sourceId: SOURCE_ID, + select: [{ aggFn: 'count', where: '' }], + ...overrides, + }); + + const messages = (body: unknown) => { + const result = externalAlertChartConfigSchema.safeParse(body); + return result.success + ? [] + : result.error.issues.map(i => `${i.path.join('.')}: ${i.message}`); + }; + + it('rejects a configType the router does not recognize', () => { + // Routing keys on configType === 'sql', so an unrecognized value would + // otherwise parse as a builder config (unknown keys stripped) and skip + // the builder-only rules below. + expect(messages(builderBody({ configType: 'promql' }))).toEqual([ + 'configType: configType must be "sql" or omitted (builder configs carry no configType)', + ]); + }); + + it('still applies the builder rules to a config carrying an unsupported configType', () => { + // The bypass this guards: both bodies parse against the builder union. + expect( + messages( + builderBody({ + configType: 'promql', + formulas: [{ expression: 'B * 2' }], + }), + ).join(' '), + ).toContain('configType must be "sql" or omitted'); + expect( + messages( + builderBody({ + configType: 'promql', + displayType: DisplayType.Number, + select: [ + { aggFn: 'count', where: '' }, + { aggFn: 'count', where: 'level:error' }, + ], + }), + ).join(' '), + ).toContain('configType must be "sql" or omitted'); + }); + + it('applies the builder rules when configType is omitted', () => { + expect( + messages(builderBody({ formulas: [{ expression: 'B * 2' }] })).join(' '), + ).toContain('Unknown series "B"'); + expect( + messages( + builderBody({ + displayType: DisplayType.Number, + select: [ + { aggFn: 'count', where: '' }, + { aggFn: 'count', where: 'level:error' }, + ], + }), + ).join(' '), + ).toContain('Number charts support a single select item'); + }); + + it('accepts the raw SQL dialect', () => { + expect( + messages({ + configType: 'sql', + displayType: DisplayType.Line, + connectionId: CONNECTION_ID, + sqlTemplate: + 'SELECT $__timeInterval(Timestamp) AS ts, count() FROM t WHERE $__timeFilter(Timestamp) GROUP BY ts', + }), + ).toEqual([]); + }); +}); + describe('convertExternalAlertChartConfigToInternal', () => { it('maps the external dialect onto the internal one', () => { const internal = convertExternalAlertChartConfigToInternal({ diff --git a/packages/api/src/utils/zod.ts b/packages/api/src/utils/zod.ts index 7544e96f7b..97511a62c5 100644 --- a/packages/api/src/utils/zod.ts +++ b/packages/api/src/utils/zod.ts @@ -580,6 +580,34 @@ const isRawSqlRoutedConfig = (data: unknown): boolean => 'configType' in data && data.configType === 'sql'; +/** + * Rejects a `configType` the routing predicate does not recognize. Without + * this, an unknown value (e.g. `configType: 'promql'`) routes to the builder + * union, which strips the unrecognized keys and parses a body that asked for + * something else — so the caller's PromQL config would silently persist as a + * builder config. Returns true when an issue was raised (caller stops). + */ +const addUnsupportedConfigTypeIssue = ( + data: unknown, + ctx: z.RefinementCtx, +): boolean => { + if ( + data !== null && + typeof data === 'object' && + 'configType' in data && + data.configType !== 'sql' + ) { + ctx.addIssue({ + code: z.ZodIssueCode.custom, + message: + 'configType must be "sql" or omitted (builder configs carry no configType)', + path: ['configType'], + }); + return true; + } + return false; +}; + /** * The shared post-parse rule set for configType-routed config schemas * (dashboard tiles and inline alert configs) — kept in one place so the two @@ -592,6 +620,10 @@ const isRawSqlRoutedConfig = (data: unknown): boolean => * item; the relaxed `.min(1).max(20)` on the number schema exists only so * formula operands fit (the editor enforces the same in * `validateChartForm`). + * + * The builder-only rules gate on `isRawSqlRoutedConfig` — the same predicate + * that picks the parsing schema — so a config parsed as builder always gets + * the builder rules. */ const addConfigTypeRoutedIssues = ( data: { @@ -614,7 +646,7 @@ const addConfigTypeRoutedIssues = ( }); } - if (!('configType' in data)) { + if (!isRawSqlRoutedConfig(data)) { validateChartConfigFormulas(data, ctx); if ( @@ -637,6 +669,10 @@ const externalDashboardTileConfigSchema = z ExternalDashboardRawSqlTileConfig | ExternalDashboardBuilderTileConfig >() .superRefine((data, ctx) => { + if (addUnsupportedConfigTypeIssue(data, ctx)) { + return; + } + // Route to the correct sub-schema based on configType so Zod's // discriminatedUnion can produce targeted field-level errors rather // than a generic union failure. @@ -894,6 +930,10 @@ export type ExternalAlertChartConfig = export const externalAlertChartConfigSchema = z .custom() .superRefine((data, ctx) => { + if (addUnsupportedConfigTypeIssue(data, ctx)) { + return; + } + const schema = isRawSqlRoutedConfig(data) ? externalAlertRawSqlChartConfigSchema : externalAlertBuilderChartConfigSchema; @@ -964,9 +1004,18 @@ const alertBaseSchema = z.object({ // external tile-config dialect (see externalAlertChartConfigSchema); the // router converts it to the internal AlertChartConfig shape before // validateAlertInput/createAlert. +// Discriminated on `source` (not a plain `.or()`): a failing `.or()` branch +// collapses to a single "Invalid input" issue with the real messages buried +// in unionErrors, which the external API's error formatter drops — so every +// chartConfig rule below would surface as "Body validation failed: Invalid +// input". Discriminating reports the matching branch's own issues. export const alertSchema = alertBaseSchema .and( - zExternalSavedSearchAlert.or(zExternalTileAlert).or(zExternalInlineAlert), + z.discriminatedUnion('source', [ + zExternalSavedSearchAlert, + zExternalTileAlert, + zExternalInlineAlert, + ]), ) .superRefine(validateAlertChannelSelection) .superRefine(validateAlertScheduleOffsetMinutes) @@ -979,7 +1028,13 @@ export type ExternalAlertInput = z.infer; // `seriesReturnType: 'ratio'`, `source`/`connection`) instead of the external // dialect above. export const internalAlertSchema = alertBaseSchema - .and(zSavedSearchAlert.or(zTileAlert).or(zInlineAlert)) + .and( + z.discriminatedUnion('source', [ + zSavedSearchAlert, + zTileAlert, + zInlineAlert, + ]), + ) .superRefine(validateAlertChannelSelection) .superRefine(validateAlertScheduleOffsetMinutes) .superRefine(validateAlertThresholdMax) From cef1198dd1a04f29aad77c984ddf0929cf095cb2 Mon Sep 17 00:00:00 2001 From: Warren Lee <5959690+wrn14897@users.noreply.github.com> Date: Tue, 1 Sep 2026 18:22:05 -0700 Subject: [PATCH 6/8] fix: keep the MCP gauge delta flag through the REST re-parse (HDX-5191) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit clickstack_save_dashboard re-parses MCP tiles through the external REST schema, which knows only periodAggFn and strips isDelta — so an MCP gauge tile spelled isDelta: true was persisted as a non-delta. The converter fix alone never saw the flag. The MCP select item now emits both spellings in agreement, which also clears a stale periodAggFn when an explicit isDelta: false wins, and makes saveAlert's per-item workaround redundant. Also validate emitted select-item values (not just key names) in isRepresentableExternally: an internal-only aggFn (sumIfMerge, quantileMerge, histogram) or an off-enum quantile level would be emitted as aggFn: 'none' or a level-less quantile, silently rewriting the query on a GET -> PUT. --- .changeset/inline-alerts-external-api-mcp.md | 7 +- .../dashboards/saveDashboard.int.test.ts | 92 +++++++++++++++++++ .../tools/alerts/__tests__/schemas.test.ts | 54 +++++++++-- .../api/src/mcp/tools/alerts/saveAlert.ts | 23 ++--- .../api/src/mcp/tools/dashboards/schemas.ts | 41 +++++++-- .../utils/__tests__/alertChartConfig.test.ts | 62 +++++++++++++ .../external-api/v2/utils/alertChartConfig.ts | 20 ++++ 7 files changed, 261 insertions(+), 38 deletions(-) diff --git a/.changeset/inline-alerts-external-api-mcp.md b/.changeset/inline-alerts-external-api-mcp.md index bf8d479943..398b782c73 100644 --- a/.changeset/inline-alerts-external-api-mcp.md +++ b/.changeset/inline-alerts-external-api-mcp.md @@ -13,9 +13,10 @@ and team-scoped source/connection ownership. Passing a `chartConfig` to a and reading an inline alert whose config carries internal-only fields omits `chartConfig` rather than returning a lossy approximation. -Also fixes three pre-existing issues on the external v2 dashboards path: the -dialect dropped the gauge `isDelta` flag when converting tile select items to -the internal shape; a tile config carrying an unrecognized `configType` (e.g. +Also fixes three pre-existing issues on the external v2 dashboards path: a +gauge tile saved through `clickstack_save_dashboard` with `isDelta: true` was +persisted as a non-delta (the flag was dropped converting MCP tiles to the +internal shape); a tile config carrying an unrecognized `configType` (e.g. `promql`) was silently stored as a builder config and skipped the formula and number-select rules, and is now rejected; and validation errors on alert bodies no longer collapse to `Invalid input` — the schema reports the failing diff --git a/packages/api/src/mcp/__tests__/dashboards/saveDashboard.int.test.ts b/packages/api/src/mcp/__tests__/dashboards/saveDashboard.int.test.ts index 9397acfddb..de0c84b3d1 100644 --- a/packages/api/src/mcp/__tests__/dashboards/saveDashboard.int.test.ts +++ b/packages/api/src/mcp/__tests__/dashboards/saveDashboard.int.test.ts @@ -3095,4 +3095,96 @@ describe('MCP Dashboard Tools - clickstack_save_dashboard', () => { expect(text).toContain(ghostDashboardId); }); }); + + describe('metric tiles', () => { + const makeMetricSource = () => + Source.create({ + kind: SourceKind.Metric, + team: ctx.team._id, + from: { databaseName: DEFAULT_DATABASE, tableName: '' }, + metricTables: { + [MetricsDataType.Gauge.toLowerCase()]: 'otel_metrics_gauge', + [MetricsDataType.Sum.toLowerCase()]: 'otel_metrics_sum', + [MetricsDataType.Histogram.toLowerCase()]: 'otel_metrics_histogram', + }, + timestampValueExpression: 'TimeUnix', + connection: ctx.connection._id, + name: 'Metrics', + }); + + const gaugeTile = ( + selectItem: Record, + sourceId: string, + ) => ({ + name: 'CPU delta', + x: 0, + y: 0, + w: 12, + h: 4, + config: { + displayType: 'line', + sourceId, + select: [ + { + aggFn: 'avg', + metricType: 'gauge', + metricName: 'system.cpu.utilization', + valueExpression: 'Value', + where: '', + ...selectItem, + }, + ], + }, + }); + + it.each([ + ['isDelta', { isDelta: true }], + ['periodAggFn', { periodAggFn: 'delta' }], + ])( + 'persists the gauge delta flag on a saved tile spelled via %s', + async (_label, deltaFlag) => { + // The tool accepts both spellings and the conversion to the internal + // tile config must keep the flag: a dropped isDelta silently makes + // the tile chart raw gauge values instead of per-bucket deltas. + const metricSource = await makeMetricSource(); + const result = await callTool( + ctx.client!, + 'clickstack_save_dashboard', + { + name: `Gauge delta dashboard (${_label})`, + tiles: [gaugeTile(deltaFlag, metricSource._id.toString())], + }, + ); + + expect(result.isError).toBeFalsy(); + const output = JSON.parse(getFirstText(result)); + + const stored = await Dashboard.findById(output.id); + expect(stored!.tiles[0].config).toMatchObject({ + select: [{ isDelta: true }], + }); + }, + ); + + it('rejects a delta on a non-gauge metric', async () => { + const metricSource = await makeMetricSource(); + const result = await callTool(ctx.client!, 'clickstack_save_dashboard', { + name: 'Sum delta dashboard', + tiles: [ + gaugeTile( + { + metricType: 'sum', + metricName: 'http.server.requests', + aggFn: 'sum', + isDelta: true, + }, + metricSource._id.toString(), + ), + ], + }); + + expect(result.isError).toBe(true); + expect(getFirstText(result)).toContain('only valid for gauge metrics'); + }); + }); }); diff --git a/packages/api/src/mcp/tools/alerts/__tests__/schemas.test.ts b/packages/api/src/mcp/tools/alerts/__tests__/schemas.test.ts index 8fcae3066a..ab280b7f69 100644 --- a/packages/api/src/mcp/tools/alerts/__tests__/schemas.test.ts +++ b/packages/api/src/mcp/tools/alerts/__tests__/schemas.test.ts @@ -158,16 +158,54 @@ describe('mcpSaveAlertSchema isDelta dialect bridge', () => { metricName: 'system.cpu.utilization', }; - it('normalizes the REST-dialect periodAggFn: "delta" onto isDelta', () => { - expect( - parseSelectItem({ ...gaugeItem, periodAggFn: 'delta' }), - ).toMatchObject({ isDelta: true }); + // Both spellings are emitted in agreement: `isDelta` for direct MCP + // consumers, `periodAggFn` because the external REST schema this config is + // re-parsed through knows only that one and strips the other. + it.each([ + ['periodAggFn: "delta"', { periodAggFn: 'delta' }], + ['isDelta: true', { isDelta: true }], + ])('emits both delta spellings for %s', (_label, deltaFlag) => { + expect(parseSelectItem({ ...gaugeItem, ...deltaFlag })).toMatchObject({ + isDelta: true, + periodAggFn: 'delta', + }); }); - it('passes an explicit isDelta through unchanged', () => { - expect(parseSelectItem({ ...gaugeItem, isDelta: true })).toMatchObject({ - isDelta: true, + it('emits neither spelling when the item is not a delta', () => { + const item = parseSelectItem(gaugeItem); + expect(item).not.toHaveProperty('isDelta'); + expect(item).not.toHaveProperty('periodAggFn'); + }); + + it('lets an explicit isDelta: false win over periodAggFn: "delta"', () => { + // The refinement and the transform must agree on precedence. When they + // disagreed, this body validated as non-delta (passing the gauge-only + // rule) and then persisted as a delta because the stale periodAggFn + // survived. + const item = parseSelectItem({ + ...gaugeItem, + isDelta: false, + periodAggFn: 'delta', }); - expect(parseSelectItem(gaugeItem).isDelta).toBeUndefined(); + expect(item).not.toHaveProperty('isDelta'); + expect(item).not.toHaveProperty('periodAggFn'); }); + + it.each([ + ['isDelta', { isDelta: true }], + ['periodAggFn', { periodAggFn: 'delta' }], + ])( + 'rejects a delta on a non-gauge metric spelled via %s', + (_label, deltaFlag) => { + expect(() => + parseSelectItem({ + aggFn: 'sum', + metricType: 'sum', + metricName: 'http.server.requests', + valueExpression: 'Value', + ...deltaFlag, + }), + ).toThrow('isDelta is only valid for gauge metrics'); + }, + ); }); diff --git a/packages/api/src/mcp/tools/alerts/saveAlert.ts b/packages/api/src/mcp/tools/alerts/saveAlert.ts index fa3fee9418..aa00c19d7c 100644 --- a/packages/api/src/mcp/tools/alerts/saveAlert.ts +++ b/packages/api/src/mcp/tools/alerts/saveAlert.ts @@ -77,23 +77,12 @@ export function registerSaveAlert({ if (input.source === 'inline') { // The MCP tile dialect spells the gauge delta flag `isDelta`; the // shared external schema spells it `periodAggFn: 'delta'` and strips - // unknown keys, so translate per select item before parsing or the - // flag is silently lost and the alert evaluates raw gauge values. - const chartConfig = - input.chartConfig != null && - 'select' in input.chartConfig && - Array.isArray(input.chartConfig.select) - ? { - ...input.chartConfig, - select: input.chartConfig.select.map(item => - item.isDelta - ? { ...item, periodAggFn: 'delta' as const } - : item, - ), - } - : input.chartConfig; - - const parsed = externalAlertChartConfigSchema.safeParse(chartConfig); + // unknown keys. The MCP select-item schema already emits both + // spellings in agreement (see mcpTileSelectItemSchema), so the flag + // survives this parse. + const parsed = externalAlertChartConfigSchema.safeParse( + input.chartConfig, + ); if (!parsed.success) { return mcpUserError( `Invalid chartConfig:\n${formatZodIssues(parsed.error)}`, diff --git a/packages/api/src/mcp/tools/dashboards/schemas.ts b/packages/api/src/mcp/tools/dashboards/schemas.ts index 136abff5fe..1554df526c 100644 --- a/packages/api/src/mcp/tools/dashboards/schemas.ts +++ b/packages/api/src/mcp/tools/dashboards/schemas.ts @@ -193,6 +193,21 @@ const mcpNumberFormatSchema = z.object({ .describe('Suffix appended to the value (e.g. " req/s")'), }); +/** + * The delta flag a select item resolves to, given that the tool accepts both + * spellings (`isDelta` and the REST dialect's `periodAggFn: 'delta'`). An + * explicit `isDelta` wins; `periodAggFn` only fills in when it is absent. + * Shared by the refinement and the transform so a body carrying both cannot + * validate as one value and persist as the other. + */ +const resolveIsDelta = (data: { + isDelta?: unknown; + periodAggFn?: unknown; +}): boolean | undefined => + typeof data.isDelta === 'boolean' + ? data.isDelta + : data.periodAggFn === 'delta' || undefined; + const mcpTileSelectItemSchema = z .object({ aggFn: AggregateFunctionSchema.describe( @@ -287,10 +302,11 @@ const mcpTileSelectItemSchema = z typeof data.metricType === 'string' ? data.metricType : undefined, metricName: typeof data.metricName === 'string' ? data.metricName : undefined, - isDelta: - typeof data.isDelta === 'boolean' - ? data.isDelta - : data.periodAggFn === 'delta' || undefined, + // Must use the same precedence as the transform below, or a body + // spelling both flags (`isDelta: false` + `periodAggFn: 'delta'`) is + // validated as non-delta — passing the gauge-only rule — and then + // persisted as a delta. + isDelta: resolveIsDelta(data), level: typeof data.level === 'number' ? data.level : undefined, valueExpression: typeof data.valueExpression === 'string' @@ -306,12 +322,17 @@ const mcpTileSelectItemSchema = z } }) .transform(data => { - // Normalize the REST-dialect spelling onto isDelta so downstream - // consumers only need to read one flag. - const withDelta = - data.periodAggFn === 'delta' && data.isDelta !== true - ? { ...data, isDelta: true } - : data; + // Emit BOTH spellings, agreeing, so neither downstream consumer can lose + // the flag: MCP tiles are re-parsed through the external REST schema + // (createDashboardBodySchema / externalAlertChartConfigSchema), which + // knows only `periodAggFn` and strips `isDelta`, while direct MCP + // consumers read `isDelta`. Writing the resolved value to both also + // clears a stale `periodAggFn: 'delta'` when `isDelta: false` wins, + // which would otherwise persist a delta the refinement never checked. + const { isDelta: _isDelta, periodAggFn: _periodAggFn, ...rest } = data; + const withDelta = resolveIsDelta(data) + ? { ...rest, isDelta: true as const, periodAggFn: 'delta' as const } + : rest; return withDelta.metricType && withDelta.aggFn !== 'count' && !withDelta.valueExpression diff --git a/packages/api/src/routers/external-api/v2/utils/__tests__/alertChartConfig.test.ts b/packages/api/src/routers/external-api/v2/utils/__tests__/alertChartConfig.test.ts index b06509ec22..4f02edfa06 100644 --- a/packages/api/src/routers/external-api/v2/utils/__tests__/alertChartConfig.test.ts +++ b/packages/api/src/routers/external-api/v2/utils/__tests__/alertChartConfig.test.ts @@ -344,6 +344,68 @@ describe('convertAlertChartConfigToExternal', () => { ).toBeUndefined(); }); + it('refuses a select item whose aggFn the external enum cannot express', () => { + // The internal schema admits aggregations the external one does not + // (histogram, quantileMerge, the *Merge combinators). + // convertToExternalSelectItem would emit aggFn: 'none' for these, so a + // GET -> PUT would silently rewrite the aggregation. + for (const aggFn of ['sumIfMerge', 'quantileMerge', 'histogram']) { + expect( + convertAlertChartConfigToExternal( + baseInternal({ + select: [ + { + aggFn, + aggCondition: '', + aggConditionLanguage: 'lucene', + valueExpression: 'Value', + ...(aggFn === 'quantileMerge' ? { level: 0.95 } : {}), + }, + ], + }), + ), + ).toBeUndefined(); + } + }); + + it('refuses a quantile level outside the external set', () => { + // The internal `level` is any number; the external dialect admits only + // 0.5/0.9/0.95/0.99, and the converter drops anything else — emitting a + // quantile with no level. + expect( + convertAlertChartConfigToExternal( + baseInternal({ + select: [ + { + aggFn: 'quantile', + level: 0.75, + aggCondition: '', + aggConditionLanguage: 'lucene', + valueExpression: 'Duration', + }, + ], + }), + ), + ).toBeUndefined(); + + // A level the dialect does admit still round-trips. + expect( + convertAlertChartConfigToExternal( + baseInternal({ + select: [ + { + aggFn: 'quantile', + level: 0.95, + aggCondition: '', + aggConditionLanguage: 'lucene', + valueExpression: 'Duration', + }, + ], + }), + ), + ).toMatchObject({ select: [{ aggFn: 'quantile', level: 0.95 }] }); + }); + it('refuses an empty select instead of throwing on a number config', () => { // A persisted number config with select: [] used to TypeError in the // tile converter (select[0] is undefined) and surface as a 500 from diff --git a/packages/api/src/routers/external-api/v2/utils/alertChartConfig.ts b/packages/api/src/routers/external-api/v2/utils/alertChartConfig.ts index 9e080ae416..725da4cad1 100644 --- a/packages/api/src/routers/external-api/v2/utils/alertChartConfig.ts +++ b/packages/api/src/routers/external-api/v2/utils/alertChartConfig.ts @@ -1,4 +1,5 @@ import { + AggregateFunctionSchema, AlertChartConfig, DisplayType, } from '@hyperdx/common-utils/dist/types'; @@ -14,6 +15,7 @@ import { externalAlertBuilderChartConfigSchema, ExternalAlertChartConfig, externalAlertRawSqlChartConfigSchema, + externalQuantileLevelSchema, } from '@/utils/zod'; import { @@ -252,6 +254,24 @@ function isRepresentableExternally( return false; } for (const item of select) { + // Values, not just key names: `convertToExternalSelectItem` falls back to + // `aggFn: 'none'` for an aggregation the external enum does not admit + // (the internal schema also allows `histogram`, `quantileMerge`, and the + // *Merge combinators) and drops a `level` outside the external quantile + // set. Emitting either would silently rewrite the aggregation on a + // GET -> PUT, which is what this gate exists to prevent. + if (!AggregateFunctionSchema.safeParse(item.aggFn).success) { + return false; + } + if ( + item.aggFn === 'quantile' && + !externalQuantileLevelSchema.safeParse( + 'level' in item ? item.level : undefined, + ).success + ) { + return false; + } + for (const [key, value] of Object.entries(item)) { if (isEmptyValue(value) || isDefaultValue(key, value)) continue; // Inert leftovers the external converter heals away: a level on a From 9af7b7d50d2e2bf36125ebeb9dd84645d45658f3 Mon Sep 17 00:00:00 2001 From: Warren Lee <5959690+wrn14897@users.noreply.github.com> Date: Wed, 2 Sep 2026 15:40:47 -0700 Subject: [PATCH 7/8] fix: gate external chartConfig emission on the external write schema (HDX-5191) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit isRepresentableExternally hand-checked a subset of the external contract, so a GET could emit a chartConfig that was invalid PUT input (uncapped aggCondition/valueExpression/alias, a non-count agg with an empty valueExpression) or that silently dropped query semantics (an array groupBy the tile converter omits while renderSelectList evaluates it). Split the responsibilities: the pre-conversion gate now covers only silent losses a parse of the converted output cannot see (inexpressible fields, aggFn -> 'none' substitution, dropped quantile levels, the new array-groupBy refusal, converter crash guards), and the emitted body is run through externalAlertChartConfigSchema.safeParse — the exact schema PUT enforces — so anything GET emits is valid PUT input by construction. The hand-rolled where-length and select-count caps (and the duplicated EXTERNAL_ALERT_WHERE_MAX constant) are gone with it. --- .../external-api/__tests__/alerts.int.test.ts | 71 ++++++++++++++ .../utils/__tests__/alertChartConfig.test.ts | 92 +++++++++++++++++++ .../external-api/v2/utils/alertChartConfig.ts | 92 ++++++++++++------- 3 files changed, 223 insertions(+), 32 deletions(-) diff --git a/packages/api/src/routers/external-api/__tests__/alerts.int.test.ts b/packages/api/src/routers/external-api/__tests__/alerts.int.test.ts index 19bf4a246e..2c04a00c0f 100644 --- a/packages/api/src/routers/external-api/__tests__/alerts.int.test.ts +++ b/packages/api/src/routers/external-api/__tests__/alerts.int.test.ts @@ -1482,6 +1482,77 @@ describe('External API Alerts', () => { }); }); + it('omits chartConfig when the config would convert lossily or violate the external write schema', async () => { + // Two classes the key-classification gate alone cannot catch: + // an array groupBy (the tile converter silently drops it, so an + // echo-PUT would strip the grouping from a live alert) and an + // aggCondition beyond the external cap (converts cleanly but the + // emitted body would 400 on echo-PUT). Both are legal internally. + const { source } = await makeSource(); + + const configs = [ + { + label: 'array groupBy', + chartConfig: { + displayType: 'line', + source: source._id.toString(), + select: [ + { + aggFn: 'count', + aggCondition: '', + aggConditionLanguage: 'lucene', + valueExpression: '', + }, + ], + where: '', + whereLanguage: 'lucene', + groupBy: [{ valueExpression: 'ServiceName' }], + }, + }, + { + label: 'over-cap aggCondition', + chartConfig: { + displayType: 'line', + source: source._id.toString(), + select: [ + { + aggFn: 'count', + aggCondition: 'a'.repeat(10001), + aggConditionLanguage: 'lucene', + valueExpression: '', + }, + ], + where: '', + whereLanguage: 'lucene', + }, + }, + ]; + + for (const { chartConfig } of configs) { + const alert = await createTestAlertDirectly({ + source: AlertSource.INLINE, + dashboardId: undefined, + tileId: undefined, + chartConfig, + }); + + const single = await authRequest( + 'get', + `${ALERTS_BASE_URL}/${alert._id}`, + ).expect(200); + expect(single.body.data.chartConfig).toBeUndefined(); + + // The echoed body has no chartConfig, so the write fails loudly + // instead of persisting a rewritten query. + await authRequest('put', `${ALERTS_BASE_URL}/${alert._id}`) + .send(single.body.data) + .expect(400); + + const stored = await Alert.findById(alert._id); + expect(stored!.chartConfig).toEqual(chartConfig); + } + }); + it('emits chartConfig for internally-authored configs the dialect can express', async () => { // Guards against over-refusal: the shapes the UI's chart explorer // persists (name, empty chart-level where, count select) must keep diff --git a/packages/api/src/routers/external-api/v2/utils/__tests__/alertChartConfig.test.ts b/packages/api/src/routers/external-api/v2/utils/__tests__/alertChartConfig.test.ts index 4f02edfa06..4b67e19de4 100644 --- a/packages/api/src/routers/external-api/v2/utils/__tests__/alertChartConfig.test.ts +++ b/packages/api/src/routers/external-api/v2/utils/__tests__/alertChartConfig.test.ts @@ -336,6 +336,10 @@ describe('convertAlertChartConfigToExternal', () => { ).toBeUndefined(); }); + // The internal schema leaves these strings uncapped / this shape legal; + // the post-conversion parse against externalAlertChartConfigSchema (the + // exact schema PUT enforces) is what refuses them, so an emitted body is + // valid PUT input by construction. it('refuses a chart-level where longer than the external cap', () => { expect( convertAlertChartConfigToExternal( @@ -344,6 +348,94 @@ describe('convertAlertChartConfigToExternal', () => { ).toBeUndefined(); }); + it('refuses select-item strings longer than the external caps', () => { + expect( + convertAlertChartConfigToExternal( + baseInternal({ + select: [ + { + aggFn: 'count', + aggCondition: 'a'.repeat(10001), + aggConditionLanguage: 'lucene', + valueExpression: '', + }, + ], + }), + ), + ).toBeUndefined(); + + expect( + convertAlertChartConfigToExternal( + baseInternal({ + select: [ + { + aggFn: 'avg', + aggCondition: '', + aggConditionLanguage: 'lucene', + valueExpression: 'a'.repeat(10001), + }, + ], + }), + ), + ).toBeUndefined(); + + expect( + convertAlertChartConfigToExternal( + baseInternal({ + select: [ + { + aggFn: 'count', + aggCondition: '', + aggConditionLanguage: 'lucene', + valueExpression: '', + alias: 'a'.repeat(10001), + }, + ], + }), + ), + ).toBeUndefined(); + }); + + it('refuses a non-count aggregation with an empty valueExpression', () => { + // Legal internally, but the external select-item schema requires a value + // expression for non-count aggregations — emitting it would produce a + // body that 400s on echo-PUT. + expect( + convertAlertChartConfigToExternal( + baseInternal({ + select: [ + { + aggFn: 'avg', + aggCondition: '', + aggConditionLanguage: 'lucene', + valueExpression: '', + }, + ], + }), + ), + ).toBeUndefined(); + }); + + it('refuses an array groupBy the tile dialect cannot carry', () => { + // renderSelectList evaluates array groupBys, so the alert task honors + // them; the tile converter silently omits the field, which would let an + // echo-PUT strip the grouping from a live alert. + expect( + convertAlertChartConfigToExternal( + baseInternal({ + groupBy: [{ valueExpression: 'ServiceName' }], + }), + ), + ).toBeUndefined(); + + // The string form the tile dialect does carry still round-trips. + expect( + convertAlertChartConfigToExternal( + baseInternal({ groupBy: 'ServiceName' }), + ), + ).toMatchObject({ groupBy: 'ServiceName' }); + }); + it('refuses a select item whose aggFn the external enum cannot express', () => { // The internal schema admits aggregations the external one does not // (histogram, quantileMerge, the *Merge combinators). diff --git a/packages/api/src/routers/external-api/v2/utils/alertChartConfig.ts b/packages/api/src/routers/external-api/v2/utils/alertChartConfig.ts index 725da4cad1..b26f69c8e6 100644 --- a/packages/api/src/routers/external-api/v2/utils/alertChartConfig.ts +++ b/packages/api/src/routers/external-api/v2/utils/alertChartConfig.ts @@ -14,6 +14,7 @@ import logger from '@/utils/logger'; import { externalAlertBuilderChartConfigSchema, ExternalAlertChartConfig, + externalAlertChartConfigSchema, externalAlertRawSqlChartConfigSchema, externalQuantileLevelSchema, } from '@/utils/zod'; @@ -185,14 +186,22 @@ const isDefaultValue = (key: string, value: unknown): boolean => { } }; -const EXTERNAL_ALERT_WHERE_MAX = 10000; - /** * Whether the persisted internal config survives an external GET -> PUT - * round-trip without changing what the alert evaluates. Anything not - * expressible in the external dialect (and not evaluation-inert) makes the - * config unrepresentable. Allowlist semantics: unknown future fields refuse - * by default rather than leaking through silently. + * round-trip without changing what the alert evaluates. + * + * This gate only covers what the post-conversion + * `externalAlertChartConfigSchema.safeParse` in + * `convertAlertChartConfigToExternal` cannot: SILENT losses, where the tile + * converter drops, substitutes, or crashes on an internal value and the + * converted output then parses cleanly (an inexpressible field vanishing, an + * out-of-enum aggFn becoming 'none', an array groupBy becoming no groupBy). + * Value-level contract rules the converter passes through verbatim (string + * caps, select-item shape, formula rules) are deliberately NOT re-implemented + * here — the final parse enforces them from the single source of truth. + * + * Allowlist semantics: unknown future fields refuse by default rather than + * leaking through silently. */ function isRepresentableExternally( config: AlertChartConfig, @@ -226,12 +235,13 @@ function isRepresentableExternally( return true; } - if ('where' in config && typeof config.where === 'string') { - // The external schema caps the chart-level filter; a longer stored value - // would emit a body that cannot be PUT back. - if (config.where.length > EXTERNAL_ALERT_WHERE_MAX) { - return false; - } + // The internal groupBy is a select list (string or array of derived + // columns) and the alert task evaluates both forms via renderSelectList; + // the tile dialect only carries the string form, and the converter + // silently omits an array. Emitting a groupBy-less body would let an + // echo-PUT strip the grouping from a live alert. + if ('groupBy' in config && Array.isArray(config.groupBy)) { + return false; } const select = 'select' in config ? config.select : undefined; @@ -240,8 +250,10 @@ function isRepresentableExternally( if (!Array.isArray(select)) { return false; } - // The external schema caps select at 20 items. - if (select.length === 0 || select.length > 20) { + // An empty select would make the number branch of the tile converter throw + // (it reads select[0]); every variant's external schema requires >= 1 item + // anyway, so refuse before converting. + if (select.length === 0) { return false; } // The external converter keeps only select[0] on formula-less number @@ -292,10 +304,11 @@ function isRepresentableExternally( * Converts an inline alert's persisted internal chart config to the external * tile-config dialect for API responses. Returns undefined when the external * contract cannot represent the config faithfully — an unsupported display - * type, a PromQL config, or a config carrying evaluation-affecting fields the - * dialect cannot express (see isRepresentableExternally). Callers omit the - * field, so a blind echo-PUT fails loudly instead of silently rewriting the - * alert's query. + * type, a PromQL config, a config carrying evaluation-affecting fields the + * dialect cannot express (see isRepresentableExternally), or a converted body + * the external write schema would reject. Callers omit the field, so a blind + * echo-PUT fails loudly instead of silently rewriting the alert's query, and + * anything that IS emitted is valid PUT input by construction. */ export function convertAlertChartConfigToExternal( config: AlertChartConfig, @@ -321,21 +334,36 @@ export function convertAlertChartConfigToExternal( case DisplayType.StackedBar: case DisplayType.Number: { const name = config.name ? { name: config.name } : {}; - if ('configType' in external) { - return { ...external, ...name }; + const candidate = + 'configType' in external + ? { ...external, ...name } + : { + ...external, + ...name, + // Chart-level filter: builder configs only (the raw SQL + // internal shape has no `where`). + ...(!('configType' in config) && config.where + ? { + where: config.where, + whereLanguage: config.whereLanguage ?? 'lucene', + } + : {}), + }; + // The emitted body must be valid PUT input by construction: run it + // through the exact schema the write path parses. The internal schema + // is looser in places the converter passes through verbatim (uncapped + // aggCondition/valueExpression/alias, an empty valueExpression on a + // non-count agg, >20 select items), so a stored config can convert + // cleanly yet still violate the external contract. + const parsed = externalAlertChartConfigSchema.safeParse(candidate); + if (!parsed.success) { + logger.warn( + { displayType, issues: parsed.error.issues }, + 'Inline alert chart config converts to a body the external schema rejects; omitting chartConfig', + ); + return undefined; } - return { - ...external, - ...name, - // Chart-level filter: builder configs only (the raw SQL internal - // shape has no `where`). - ...(!('configType' in config) && config.where - ? { - where: config.where, - whereLanguage: config.whereLanguage ?? 'lucene', - } - : {}), - }; + return parsed.data; } default: logger.error( From fe52e6e079047addbb19809b3c14871610a717a3 Mon Sep 17 00:00:00 2001 From: Warren Lee <5959690+wrn14897@users.noreply.github.com> Date: Thu, 3 Sep 2026 13:36:45 -0700 Subject: [PATCH 8/8] fix: treat stored granularity as evaluation-inert for inline alerts (HDX-5191) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The chart explorer persists granularity: 'auto' on every inline alert it authors, so classifying granularity as lossy blanked chartConfig on GET for the most common authoring path. The alert task derives the bucket size from alert.interval (checkAlerts writes `${windowSizeInMins} minute` unconditionally) and the alert detail chart uses intervalToGranularity, so the stored value only affects the unattached chart-editor preview — reclassify it as evaluation-inert and drop it on emission like fillNulls/compareToPreviousPeriod. --- .../external-api/__tests__/alerts.int.test.ts | 6 ++++-- .../v2/utils/__tests__/alertChartConfig.test.ts | 17 ++++++++++++++++- .../external-api/v2/utils/alertChartConfig.ts | 8 +++++++- 3 files changed, 27 insertions(+), 4 deletions(-) diff --git a/packages/api/src/routers/external-api/__tests__/alerts.int.test.ts b/packages/api/src/routers/external-api/__tests__/alerts.int.test.ts index 2c04a00c0f..4006fc6988 100644 --- a/packages/api/src/routers/external-api/__tests__/alerts.int.test.ts +++ b/packages/api/src/routers/external-api/__tests__/alerts.int.test.ts @@ -1555,8 +1555,9 @@ describe('External API Alerts', () => { it('emits chartConfig for internally-authored configs the dialect can express', async () => { // Guards against over-refusal: the shapes the UI's chart explorer - // persists (name, empty chart-level where, count select) must keep - // round-tripping. + // persists (name, empty chart-level where, count select, and its + // always-written granularity: 'auto' — evaluation derives the bucket + // size from alert.interval) must keep round-tripping. const { source } = await makeSource(); const alert = await createTestAlertDirectly({ @@ -1578,6 +1579,7 @@ describe('External API Alerts', () => { where: '', whereLanguage: 'lucene', seriesReturnType: 'column', + granularity: 'auto', }, }); diff --git a/packages/api/src/routers/external-api/v2/utils/__tests__/alertChartConfig.test.ts b/packages/api/src/routers/external-api/v2/utils/__tests__/alertChartConfig.test.ts index 4b67e19de4..c4da87435f 100644 --- a/packages/api/src/routers/external-api/v2/utils/__tests__/alertChartConfig.test.ts +++ b/packages/api/src/routers/external-api/v2/utils/__tests__/alertChartConfig.test.ts @@ -312,6 +312,22 @@ describe('convertAlertChartConfigToExternal', () => { expect(external).toMatchObject({ displayType: DisplayType.Line }); }); + it('still emits configs carrying a stored granularity', () => { + // The chart explorer persists granularity: 'auto' on every inline alert + // it authors, and the alert task derives the real bucket size from + // alert.interval — refusing on it would blank chartConfig for the most + // common authoring path. The field is dropped (the external dialect has + // no spelling for it), which only affects the unattached chart-editor + // preview. + for (const granularity of ['auto', '5 minute']) { + const external = convertAlertChartConfigToExternal( + baseInternal({ granularity }), + ); + expect(external).toMatchObject({ displayType: DisplayType.Line }); + expect(external).not.toHaveProperty('granularity'); + } + }); + it.each([ [ 'filters', @@ -322,7 +338,6 @@ describe('convertAlertChartConfigToExternal', () => { ['limit', { limit: { limit: 10 } }], ['ratioMode', { ratioMode: 'total' }], ['selectGroupBy', { selectGroupBy: true }], - ['granularity', { granularity: '5 minute' }], ['implicitColumnExpression', { implicitColumnExpression: 'Body' }], ['sampleWeightExpression', { sampleWeightExpression: 'SampleRate' }], ['a raw SQL select expression string', { select: 'count() as count' }], diff --git a/packages/api/src/routers/external-api/v2/utils/alertChartConfig.ts b/packages/api/src/routers/external-api/v2/utils/alertChartConfig.ts index b26f69c8e6..51bf1ac86c 100644 --- a/packages/api/src/routers/external-api/v2/utils/alertChartConfig.ts +++ b/packages/api/src/routers/external-api/v2/utils/alertChartConfig.ts @@ -111,6 +111,13 @@ export const EVALUATION_INERT_CONFIG_KEYS: ReadonlySet = new Set([ // Evaluation-relevant only with a groupBy, which (on variants whose shape // lacks seriesLimit, i.e. number) is itself refused when present. 'seriesLimit', + // The alert task derives granularity from alert.interval + // (tasks/checkAlerts/index.ts: `${windowSizeInMins} minute`) and the alert + // detail chart from intervalToGranularity, so the stored value only affects + // the unattached chart-editor preview. The chart explorer persists + // granularity: 'auto' on every inline alert it authors — refusing on it + // would blank chartConfig for the most common authoring path. + 'granularity', ]); /** @@ -129,7 +136,6 @@ export const KNOWN_LOSSY_CONFIG_KEYS: ReadonlySet = new Set([ 'limit', 'ratioMode', 'selectGroupBy', - 'granularity', 'implicitColumnExpression', 'sampleWeightExpression', 'eventTableSelect',