From d8c780bbe849157044cc8402b88ebd93697d8735 Mon Sep 17 00:00:00 2001 From: Drew Davis Date: Fri, 28 Aug 2026 09:09:10 -0400 Subject: [PATCH 1/3] feat: Add static filters to schemas and APIs --- .changeset/static-list-dashboard-filters.md | 7 + packages/api/openapi.json | 100 +++++++- .../api/src/mcp/tools/dashboards/schemas.ts | 3 +- .../api/__tests__/dashboard.int.test.ts | 237 ++++++++++++++++++ packages/api/src/routers/api/dashboards.ts | 5 +- .../__tests__/dashboards.int.test.ts | 137 ++++++++++ .../src/routers/external-api/v2/dashboards.ts | 98 +++++++- .../external-api/v2/utils/dashboards.ts | 6 +- .../__tests__/provisionDashboards.int.test.ts | 54 ++++ .../src/tasks/provisionDashboards/index.ts | 8 +- packages/api/src/utils/externalApi.ts | 7 + packages/api/src/utils/zod.ts | 33 ++- packages/app/src/DBDashboardImportPage.tsx | 97 ++++--- packages/app/src/DashboardFilters.tsx | 21 +- .../OnClickForm/OnClickDrawer.tsx | 18 +- .../DashboardFilterEditForm.tsx | 12 +- .../DashboardFiltersList.tsx | 56 +++-- .../DashboardFiltersModal/index.tsx | 3 +- .../__tests__/useDashboardFilters.test.tsx | 3 +- ... useQueriedDashboardFilterValues.test.tsx} | 76 +++--- .../app/src/hooks/useDashboardFilters.tsx | 29 ++- .../src/hooks/usePresetDashboardFilters.tsx | 4 + ...sx => useQueriedDashboardFilterValues.tsx} | 14 +- .../__tests__/dashboardFilterValues.test.ts | 59 ++++- .../src/__tests__/dashboardValidation.test.ts | 84 +++++++ .../src/__tests__/filters.test.ts | 133 +++++++++- .../common-utils/src/__tests__/types.test.ts | 196 +++++++++++++++ .../common-utils/src/__tests__/utils.test.ts | 79 +++++- .../src/__tests__/variables.test.ts | 80 ++++++ packages/common-utils/src/core/utils.ts | 13 +- .../common-utils/src/dashboardFilterValues.ts | 21 +- .../common-utils/src/dashboardValidation.ts | 32 +++ packages/common-utils/src/filters.ts | 49 +++- packages/common-utils/src/types.ts | 88 +++++-- 34 files changed, 1663 insertions(+), 199 deletions(-) create mode 100644 .changeset/static-list-dashboard-filters.md rename packages/app/src/hooks/__tests__/{useDashboardFilterValues.test.tsx => useQueriedDashboardFilterValues.test.tsx} (96%) rename packages/app/src/hooks/{useDashboardFilterValues.tsx => useQueriedDashboardFilterValues.tsx} (97%) diff --git a/.changeset/static-list-dashboard-filters.md b/.changeset/static-list-dashboard-filters.md new file mode 100644 index 0000000000..455b5383bb --- /dev/null +++ b/.changeset/static-list-dashboard-filters.md @@ -0,0 +1,7 @@ +--- +'@hyperdx/common-utils': patch +'@hyperdx/api': patch +'@hyperdx/app': patch +--- + +feat: Add static filters to schemas and APIs \ No newline at end of file diff --git a/packages/api/openapi.json b/packages/api/openapi.json index 7d8ed1c040..84248b8713 100644 --- a/packages/api/openapi.json +++ b/packages/api/openapi.json @@ -3040,8 +3040,26 @@ ] }, "FilterInput": { + "description": "A drop-down filter on a dashboard. Depending on type, filters can either broadcast\nselected value(s) to every tile (isBroadcastEnabled), expose selected\nvalues as a variable (isVariableEnabled) or both. Typically a filter should\ndo just one of these things.\n", + "oneOf": [ + { + "$ref": "#/components/schemas/QueryExpressionFilterInput" + }, + { + "$ref": "#/components/schemas/StaticListFilterInput" + } + ], + "discriminator": { + "propertyName": "type", + "mapping": { + "QUERY_EXPRESSION": "#/components/schemas/QueryExpressionFilterInput", + "STATIC_LIST": "#/components/schemas/StaticListFilterInput" + } + } + }, + "QueryExpressionFilterInput": { "type": "object", - "description": "A drop-down filter on a dashboard. Every filter can either broadcast its\nselected value(s) to every tile (isBroadcastEnabled), expose its selected\nvalue as a variable (isVariableEnabled) or both. Typically a filter should\ndo just one of these things.\n", + "description": "A filter whose dropdown values are queried from ClickHouse: \"expression\"\nnames the column and \"sourceId\" the source to read them from. Its\nselection can be broadcast into matching tiles' WHERE clauses.\n", "required": [ "type", "name", @@ -3054,7 +3072,7 @@ "enum": [ "QUERY_EXPRESSION" ], - "description": "Filter type. Must be \"QUERY_EXPRESSION\".", + "description": "Filter type discriminator. Must be \"QUERY_EXPRESSION\".", "example": "QUERY_EXPRESSION" }, "name": { @@ -3132,6 +3150,72 @@ } } }, + "StaticListFilterInput": { + "type": "object", + "description": "A filter whose dropdown offers a static custom \"options\" list. It carries no\nexpression, so it cannot be broadcast, and supports only variable mode.\n", + "required": [ + "type", + "name", + "options" + ], + "properties": { + "type": { + "type": "string", + "enum": [ + "STATIC_LIST" + ], + "description": "Discriminator. Must be \"STATIC_LIST\".", + "example": "STATIC_LIST" + }, + "name": { + "type": "string", + "minLength": 1, + "description": "Display name for the dashboard filter key", + "example": "Environment" + }, + "options": { + "type": "array", + "minItems": 1, + "maxItems": 1000, + "items": { + "type": "string", + "minLength": 1, + "maxLength": 10000 + }, + "description": "The values this filter's dropdown offers. Must be non-empty and\nfree of duplicates.\n", + "example": [ + "prod", + "staging", + "dev" + ] + }, + "isBroadcastEnabled": { + "type": "boolean", + "enum": [ + false + ], + "default": false, + "description": "Must be false — there is no expression to broadcast. Omit it and it\nis false.\n", + "example": false + }, + "isVariableEnabled": { + "type": "boolean", + "enum": [ + true + ], + "default": true, + "description": "Must be true if provided, and is true when omitted. Tiles reference\nthe selection as `$variableName`, or with the `$__filter(, $)`\nand `$__conditionalAll(, $)` macros. The\none-argument `$__filter($)` form renders the filter's\nown expression, which this kind of filter does not have, so it\nreports that the expression must be passed explicitly.\n", + "example": true + }, + "variableName": { + "type": "string", + "maxLength": 64, + "pattern": "^[a-zA-Z][a-zA-Z0-9_]*$", + "description": "Token tiles reference this filter's selected value by, as\n`$variableName`. Must start with a letter and may contain only\nletters, numbers, and underscores. Defaults to the display name with\nwhitespace replaced by underscores and remaining illegal characters\nremoved, so a variable-enabled filter whose name derives nothing\nusable must send this field explicitly. Variable names must be unique\nacross a dashboard's variable-enabled filters. Names the variable\nonly, so the field is rejected when isVariableEnabled is not true, and\nis omitted from responses for such a filter.\n", + "example": "environment" + } + } + }, "Filter": { "allOf": [ { @@ -6147,6 +6231,18 @@ "name": "Service", "expression": "service_name", "sourceId": "65f5e4a3b9e77c001a111111" + }, + { + "type": "STATIC_LIST", + "name": "Environment", + "options": [ + "prod", + "staging", + "dev" + ], + "isBroadcastEnabled": false, + "isVariableEnabled": true, + "variableName": "env" } ] } diff --git a/packages/api/src/mcp/tools/dashboards/schemas.ts b/packages/api/src/mcp/tools/dashboards/schemas.ts index fcd003c6c1..79352c171d 100644 --- a/packages/api/src/mcp/tools/dashboards/schemas.ts +++ b/packages/api/src/mcp/tools/dashboards/schemas.ts @@ -1152,7 +1152,8 @@ const mcpDashboardFilterSchema = z 'one. On CREATE (no top-level `id` on the dashboard call), filter `id` may ' + 'be omitted and one will be generated server-side.', ), - type: DashboardFilterType.describe( + // TODO: Add static value filter type when variables are supported in MCP + type: DashboardFilterType.extract(['QUERY_EXPRESSION']).describe( 'Filter type. Currently only "QUERY_EXPRESSION" is supported.', ), name: z diff --git a/packages/api/src/routers/api/__tests__/dashboard.int.test.ts b/packages/api/src/routers/api/__tests__/dashboard.int.test.ts index c41aed29d4..527b38dfc3 100644 --- a/packages/api/src/routers/api/__tests__/dashboard.int.test.ts +++ b/packages/api/src/routers/api/__tests__/dashboard.int.test.ts @@ -1038,6 +1038,195 @@ describe('dashboard router', () => { }); }); + describe('static-list filters', () => { + const makeStaticFilter = (overrides = {}) => ({ + id: new Types.ObjectId().toString(), + type: 'STATIC_LIST' as const, + name: 'Environment', + options: ['prod', 'staging', 'dev'], + isBroadcastEnabled: false, + isVariableEnabled: true, + variableName: 'env', + ...overrides, + }); + + it('persists a static-list filter on create and returns it on GET', async () => { + const filter = makeStaticFilter(); + + const created = await agent + .post('/dashboards') + .send({ ...MOCK_DASHBOARD, filters: [filter] }) + .expect(200); + + expect(created.body.filters).toEqual([filter]); + + const listed = await agent.get('/dashboards').expect(200); + expect( + listed.body.find( + (dashboard: { _id: string }) => dashboard._id === created.body.id, + )?.filters, + ).toEqual([filter]); + + const stored = await Dashboard.findById(created.body.id).lean(); + expect(stored?.filters).toEqual([filter]); + // The queried-filter fields have to stay absent, not be materialized as + // null: `expression` falsiness is what makes `$__filter($env)` report + // that the expression must be passed explicitly. + expect(stored?.filters?.[0]).not.toHaveProperty('expression'); + expect(stored?.filters?.[0]).not.toHaveProperty('source'); + }); + + it('preserves the authored option order', async () => { + const filter = makeStaticFilter({ options: ['prod', 'staging', 'dev'] }); + + const created = await agent + .post('/dashboards') + .send({ ...MOCK_DASHBOARD, filters: [filter] }) + .expect(200); + + expect(created.body.filters[0].options).toEqual([ + 'prod', + 'staging', + 'dev', + ]); + }); + + it('persists an updated option list on PATCH', async () => { + const filter = makeStaticFilter(); + const created = await agent + .post('/dashboards') + .send({ ...MOCK_DASHBOARD, filters: [filter] }) + .expect(200); + + const updatedFilter = { ...filter, options: ['prod', 'canary'] }; + await agent + .patch(`/dashboards/${created.body.id}`) + .send({ filters: [updatedFilter] }) + .expect(200); + + const stored = await Dashboard.findById(created.body.id).lean(); + expect(stored?.filters).toEqual([updatedFilter]); + }); + + it('accepts a static filter alongside a queried one', async () => { + await agent + .post('/dashboards') + .send({ + ...MOCK_DASHBOARD, + filters: [ + makeStaticFilter(), + { + id: new Types.ObjectId().toString(), + type: 'QUERY_EXPRESSION' as const, + name: 'Service', + expression: 'ServiceName', + source: new Types.ObjectId().toString(), + }, + ], + }) + .expect(200); + }); + + // The modes are literals on the static variant and `options` is `.min(1)`, + // so these are structural rejections; duplicate options are the one rule + // still carried by a refinement. + it.each([ + ['broadcast enabled', { isBroadcastEnabled: true }], + ['broadcast unset', { isBroadcastEnabled: undefined }], + ['not variable-enabled', { isVariableEnabled: false }], + ['variables unset', { isVariableEnabled: undefined }], + ['no options', { options: undefined }], + ['an empty option list', { options: [] }], + ['duplicate options', { options: ['prod', 'prod'] }], + ])('rejects a static filter with %s', async (_label, overrides) => { + await agent + .post('/dashboards') + .send({ + ...MOCK_DASHBOARD, + filters: [makeStaticFilter(overrides)], + }) + .expect(400); + }); + + // A query-expression field on a static filter is accepted and persisted + // verbatim, exactly like any other unknown key on this route: the internal + // filter variants are not strict (a strict variant would turn a stray key + // left in Mongo by an older version into a failed save, since the edit form + // spreads the stored filter back into its payload), and `validateRequest` + // validates without replacing `req.body`, so nothing is stripped either. + // The extra fields are inert — every reader narrows on `type` — and the + // external API, which is strict, rejects the same payload outright. + it('persists a query-expression field sent on a static filter, inert', async () => { + const filter = makeStaticFilter(); + const withStrayFields = { + ...filter, + expression: 'ServiceName', + where: "ServiceName = 'api'", + }; + + const created = await agent + .post('/dashboards') + .send({ ...MOCK_DASHBOARD, filters: [withStrayFields] }) + .expect(200); + + expect(created.body.filters).toEqual([withStrayFields]); + + // Still reads back as a valid static filter, and template export drops + // the stray keys (see `convertToDashboardTemplate` in common-utils). + const stored = await Dashboard.findById(created.body.id).lean(); + expect(stored?.filters?.[0]).toMatchObject({ + type: 'STATIC_LIST', + options: ['prod', 'staging', 'dev'], + isBroadcastEnabled: false, + isVariableEnabled: true, + }); + }); + + it('rejects the state when PATCH introduces it, leaving the stored filter alone', async () => { + const filter = makeStaticFilter(); + const created = await agent + .post('/dashboards') + .send({ ...MOCK_DASHBOARD, filters: [filter] }) + .expect(200); + + await agent + .patch(`/dashboards/${created.body.id}`) + .send({ filters: [{ ...filter, options: [] }] }) + .expect(400); + + const stored = await Dashboard.findById(created.body.id).lean(); + expect(stored?.filters).toEqual([filter]); + }); + + // The requiredness of both fields moved out of the field-level schema when + // `STATIC_LIST` made them optional, so it has to keep holding here. + it.each(['expression', 'source'])( + 'still rejects a query-expression filter with no %s', + async field => { + const response = await agent + .post('/dashboards') + .send({ + ...MOCK_DASHBOARD, + filters: [ + { + id: new Types.ObjectId().toString(), + type: 'QUERY_EXPRESSION' as const, + name: 'Service', + expression: 'ServiceName', + source: new Types.ObjectId().toString(), + [field]: undefined, + }, + ], + }) + .expect(400); + + expect(response.body[0].errors.issues).toEqual([ + expect.objectContaining({ path: ['filters', 0, field] }), + ]); + }, + ); + }); + describe('preset dashboards', () => { const MOCK_SOURCE: Omit, 'id'> = { kind: SourceKind.Log, @@ -1245,6 +1434,54 @@ describe('dashboard router', () => { .expect(400); }); + // Preset dashboards render the filter form with variables hidden, and a + // static filter is variable-only, so there is no way to author one here. + it('returns 400 for a STATIC_LIST preset filter', async () => { + const response = await agent + .post(`/dashboards/preset/${PresetDashboard.Services}/filter`) + .send({ + filter: { + id: new Types.ObjectId().toString(), + type: 'STATIC_LIST', + name: 'Environment', + options: ['prod', 'staging', 'dev'], + isBroadcastEnabled: false, + isVariableEnabled: true, + variableName: 'env', + presetDashboard: PresetDashboard.Services, + }, + }) + .expect(400); + + // The preset schema extends the query-expression variant, so the type + // literal fails alongside the fields that variant requires. + expect(response.body[0].errors.issues).toContainEqual( + expect.objectContaining({ path: ['filter', 'type'] }), + ); + }); + + it.each(['expression', 'source'])( + 'returns 400 for a preset filter with no %s', + async field => { + const source = await Source.create({ + ...MOCK_SOURCE, + team: team._id, + }); + + await agent + .post(`/dashboards/preset/${PresetDashboard.Services}/filter`) + .send({ + filter: { + ...MOCK_PRESET_DASHBOARD_FILTER, + id: new Types.ObjectId().toString(), + source: source._id.toString(), + [field]: undefined, + }, + }) + .expect(400); + }, + ); + it('returns 400 when filter is missing required fields', async () => { const source = await Source.create({ ...MOCK_SOURCE, diff --git a/packages/api/src/routers/api/dashboards.ts b/packages/api/src/routers/api/dashboards.ts index c4b3ebf143..1561d41217 100644 --- a/packages/api/src/routers/api/dashboards.ts +++ b/packages/api/src/routers/api/dashboards.ts @@ -1,5 +1,6 @@ import { validateDashboardFilterModes, + validateDashboardFilterOptionUniqueness, validateDashboardFilterVariableNames, } from '@hyperdx/common-utils/dist/dashboardValidation'; import { @@ -35,12 +36,13 @@ import { objectIdSchema } from '@/utils/zod'; const router = express.Router(); /** - * Additional filter validation (uniqueness, at least one mode enabled). + * Additional filter validation (variable name and option uniqueness, at least one mode enabled). */ const addFilterIssues = ( data: { filters?: { name: string; + options?: string[]; variableName?: string; isBroadcastEnabled?: boolean; isVariableEnabled?: boolean; @@ -50,6 +52,7 @@ const addFilterIssues = ( ) => { validateDashboardFilterVariableNames(data.filters ?? [], ctx); validateDashboardFilterModes(data.filters ?? [], ctx); + validateDashboardFilterOptionUniqueness(data.filters ?? [], ctx); }; /** 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 a3b5f942cb..bdc6b089f9 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 @@ -5553,6 +5553,143 @@ describe('External API v2 Dashboards - new format', () => { expect(response.status).toBe(200); }); }); + + describe('static-list filters', () => { + const staticFilterInput = (overrides = {}) => ({ + id: new ObjectId().toString(), + type: 'STATIC_LIST' as const, + name: 'Environment', + options: ['prod', 'staging', 'dev'], + isBroadcastEnabled: false, + isVariableEnabled: true, + variableName: 'env', + ...overrides, + }); + + it('accepts a static-list filter and round-trips it through GET', async () => { + const response = await sendFilters([staticFilterInput()]); + expect(response.status).toBe(200); + + const [filter] = response.body.data.filters; + expect(filter).toMatchObject({ + type: 'STATIC_LIST', + name: 'Environment', + // Author order, not sorted. + options: ['prod', 'staging', 'dev'], + isBroadcastEnabled: false, + isVariableEnabled: true, + variableName: 'env', + }); + // A sourceless filter emits no `sourceId` key at all rather than a + // null one, so the response body can be PUT straight back. + expect(filter).not.toHaveProperty('sourceId'); + expect(filter).not.toHaveProperty('expression'); + + const getResponse = await authRequest( + 'get', + `${BASE_URL}/${response.body.data.id}`, + ).expect(200); + expect(getResponse.body.data.filters).toEqual( + response.body.data.filters, + ); + }); + + it('accepts a static filter alongside a queried one', async () => { + const response = await sendFilters([ + filterInput(), + staticFilterInput(), + ]); + + expect(response.status).toBe(200); + expect(response.body.data.filters).toHaveLength(2); + }); + + // Each mode flag accepts exactly one value, so omitting it fills that + // value in rather than falling back to the queried filter's defaults. + it('defaults the mode flags when they are omitted', async () => { + const response = await sendFilters([ + staticFilterInput({ + isBroadcastEnabled: undefined, + isVariableEnabled: undefined, + }), + ]); + + expect(response.status).toBe(200); + expect(response.body.data.filters[0]).toMatchObject({ + isBroadcastEnabled: false, + isVariableEnabled: true, + variableName: 'env', + }); + }); + + it.each([ + ['broadcast enabled', { isBroadcastEnabled: true }], + ['not variable-enabled', { isVariableEnabled: false }], + ['no options', { options: undefined }], + ['an empty option list', { options: [] }], + ['duplicate options', { options: ['prod', 'prod'] }], + ])('rejects a static filter with %s', async (_label, overrides) => { + const response = await sendFilters([staticFilterInput(overrides)]); + + expect(response.status).toBe(400); + }); + + // Unlike the internal variants, which strip them, the external ones are + // strict: a field belonging to the other variant is an unrecognized key, + // named in the error. + it.each([ + ['an expression', { expression: 'ServiceName' }], + ['a sourceId', { sourceId: '65f5e4a3b9e77c001a111111' }], + ['a where clause', { where: "ServiceName = 'api'" }], + ['a where language', { whereLanguage: 'sql' }], + [ + 'applies-to sources', + { appliesToSourceIds: ['65f5e4a3b9e77c001a111111'] }, + ], + ])('rejects a static filter carrying %s', async (_label, overrides) => { + const response = await sendFilters([staticFilterInput(overrides)]); + + expect(response.status).toBe(400); + expect(JSON.stringify(response.body)).toContain( + `Unrecognized key(s) in object: '${Object.keys(overrides)[0]}'`, + ); + }); + + it('rejects options on a queried filter', async () => { + const response = await sendFilters([ + filterInput({ options: ['prod'] }), + ]); + + expect(response.status).toBe(400); + expect(JSON.stringify(response.body)).toContain( + "Unrecognized key(s) in object: 'options'", + ); + }); + + it.each(['expression', 'sourceId'])( + 'still rejects a queried filter with no %s', + async field => { + const response = await sendFilters([ + filterInput({ [field]: undefined }), + ]); + + expect(response.status).toBe(400); + expect(JSON.stringify(response.body)).toContain(field); + }, + ); + + // A bogus type must not fall through to either variant. + it('rejects an unknown filter type at the discriminator', async () => { + const response = await sendFilters([ + staticFilterInput({ type: 'STATIC' }), + ]); + + expect(response.status).toBe(400); + expect(JSON.stringify(response.body)).toContain( + 'Invalid discriminator value', + ); + }); + }); }); describe('Number tile color (HDX-1360)', () => { diff --git a/packages/api/src/routers/external-api/v2/dashboards.ts b/packages/api/src/routers/external-api/v2/dashboards.ts index a6ffcc93b7..898cb7b933 100644 --- a/packages/api/src/routers/external-api/v2/dashboards.ts +++ b/packages/api/src/routers/external-api/v2/dashboards.ts @@ -1753,12 +1753,26 @@ const EXTERNAL_DASHBOARD_PROJECTION = { * $ref: '#/components/schemas/DashboardChartSeries' * * FilterInput: - * type: object * description: | - * A drop-down filter on a dashboard. Every filter can either broadcast its - * selected value(s) to every tile (isBroadcastEnabled), expose its selected - * value as a variable (isVariableEnabled) or both. Typically a filter should + * A drop-down filter on a dashboard. Depending on type, filters can either broadcast + * selected value(s) to every tile (isBroadcastEnabled), expose selected + * values as a variable (isVariableEnabled) or both. Typically a filter should * do just one of these things. + * oneOf: + * - $ref: '#/components/schemas/QueryExpressionFilterInput' + * - $ref: '#/components/schemas/StaticListFilterInput' + * discriminator: + * propertyName: type + * mapping: + * QUERY_EXPRESSION: '#/components/schemas/QueryExpressionFilterInput' + * STATIC_LIST: '#/components/schemas/StaticListFilterInput' + * + * QueryExpressionFilterInput: + * type: object + * description: | + * A filter whose dropdown values are queried from ClickHouse: "expression" + * names the column and "sourceId" the source to read them from. Its + * selection can be broadcast into matching tiles' WHERE clauses. * required: * - type * - name @@ -1768,7 +1782,7 @@ const EXTERNAL_DASHBOARD_PROJECTION = { * type: * type: string * enum: [QUERY_EXPRESSION] - * description: Filter type. Must be "QUERY_EXPRESSION". + * description: Filter type discriminator. Must be "QUERY_EXPRESSION". * example: "QUERY_EXPRESSION" * name: * type: string @@ -1845,6 +1859,74 @@ const EXTERNAL_DASHBOARD_PROJECTION = { * is omitted from responses for such a filter. * example: "environment" * + * StaticListFilterInput: + * type: object + * description: | + * A filter whose dropdown offers a static custom "options" list. It carries no + * expression, so it cannot be broadcast, and supports only variable mode. + * required: + * - type + * - name + * - options + * properties: + * type: + * type: string + * enum: [STATIC_LIST] + * description: Discriminator. Must be "STATIC_LIST". + * example: "STATIC_LIST" + * name: + * type: string + * minLength: 1 + * description: Display name for the dashboard filter key + * example: "Environment" + * options: + * type: array + * minItems: 1 + * maxItems: 1000 + * items: + * type: string + * minLength: 1 + * maxLength: 10000 + * description: | + * The values this filter's dropdown offers. Must be non-empty and + * free of duplicates. + * example: ["prod", "staging", "dev"] + * isBroadcastEnabled: + * type: boolean + * enum: [false] + * default: false + * description: | + * Must be false — there is no expression to broadcast. Omit it and it + * is false. + * example: false + * isVariableEnabled: + * type: boolean + * enum: [true] + * default: true + * description: | + * Must be true if provided, and is true when omitted. Tiles reference + * the selection as `$variableName`, or with the `$__filter(, $)` + * and `$__conditionalAll(, $)` macros. The + * one-argument `$__filter($)` form renders the filter's + * own expression, which this kind of filter does not have, so it + * reports that the expression must be passed explicitly. + * example: true + * variableName: + * type: string + * maxLength: 64 + * pattern: '^[a-zA-Z][a-zA-Z0-9_]*$' + * description: | + * Token tiles reference this filter's selected value by, as + * `$variableName`. Must start with a letter and may contain only + * letters, numbers, and underscores. Defaults to the display name with + * whitespace replaced by underscores and remaining illegal characters + * removed, so a variable-enabled filter whose name derives nothing + * usable must send this field explicitly. Variable names must be unique + * across a dashboard's variable-enabled filters. Names the variable + * only, so the field is rejected when isVariableEnabled is not true, and + * is omitted from responses for such a filter. + * example: "environment" + * * Filter: * allOf: * - $ref: '#/components/schemas/FilterInput' @@ -2444,6 +2526,12 @@ router.post('/validate', async (req, res, next) => { * name: "Service" * expression: "service_name" * sourceId: "65f5e4a3b9e77c001a111111" + * - type: "STATIC_LIST" + * name: "Environment" + * options: ["prod", "staging", "dev"] + * isBroadcastEnabled: false + * isVariableEnabled: true + * variableName: "env" * responses: * '200': * description: Successfully created dashboard 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 63b149e18d..ac479f5069 100644 --- a/packages/api/src/routers/external-api/v2/utils/dashboards.ts +++ b/packages/api/src/routers/external-api/v2/utils/dashboards.ts @@ -7,9 +7,11 @@ import { validateDashboardContainersStructure, validateDashboardFilterFieldGating, validateDashboardFilterModes, + validateDashboardFilterOptionUniqueness, validateDashboardFilterVariableNames, validateDashboardTileContainerRefs, } from '@hyperdx/common-utils/dist/dashboardValidation'; +import { isQueryExpressionFilter } from '@hyperdx/common-utils/dist/filters'; import { isBuilderSavedChartConfig, isHeatmapCompatibleSource, @@ -1022,7 +1024,7 @@ function getMissingSources( if (filters?.length) { for (const filter of filters) { - if ('sourceId' in filter) { + if (isQueryExpressionFilter(filter)) { sourceIds.add(filter.sourceId); } } @@ -1573,6 +1575,8 @@ function buildDashboardBodySchema(filterSchema: z.ZodTypeAny): z.ZodEffects< validateDashboardFilterVariableNames(data.filters ?? [], ctx); validateDashboardFilterModes(data.filters ?? [], ctx); validateDashboardFilterFieldGating(data.filters ?? [], ctx); + + validateDashboardFilterOptionUniqueness(data.filters ?? [], ctx); }); } diff --git a/packages/api/src/tasks/provisionDashboards/__tests__/provisionDashboards.int.test.ts b/packages/api/src/tasks/provisionDashboards/__tests__/provisionDashboards.int.test.ts index 5991156d6f..b4fb380da4 100644 --- a/packages/api/src/tasks/provisionDashboards/__tests__/provisionDashboards.int.test.ts +++ b/packages/api/src/tasks/provisionDashboards/__tests__/provisionDashboards.int.test.ts @@ -82,6 +82,60 @@ describe('provisionDashboards', () => { expect(result[0].name).toBe('Test'); }); + it('parses a dashboard file carrying a static-list filter', () => { + const filter = { + id: 'filter-static', + type: 'STATIC_LIST', + name: 'Environment', + options: ['prod', 'staging', 'dev'], + isBroadcastEnabled: false, + isVariableEnabled: true, + variableName: 'env', + }; + fs.writeFileSync( + path.join(tmpDir, 'static-filter.json'), + JSON.stringify({ + name: 'Static Filter', + tiles: [makeTile()], + tags: [], + filters: [filter], + }), + ); + + const result = readDashboardFiles(tmpDir); + expect(result).toHaveLength(1); + expect(result[0].filters).toEqual([filter]); + }); + + // `expression` and `source` are optional at the field level so a + // `STATIC_LIST` filter can exist, so the per-type refinement is what keeps + // an unqueryable query-expression filter out of a provisioned file. + it.each(['expression', 'source'])( + 'skips a file whose query-expression filter has no %s', + field => { + fs.writeFileSync( + path.join(tmpDir, `no-${field}.json`), + JSON.stringify({ + name: 'Bad Filter', + tiles: [makeTile()], + tags: [], + filters: [ + { + id: 'filter-1', + type: 'QUERY_EXPRESSION', + name: 'Service', + ...(field === 'expression' + ? { source: 'source-1' } + : { expression: 'ServiceName' }), + }, + ], + }), + ); + + expect(readDashboardFiles(tmpDir)).toEqual([]); + }, + ); + // Pins the inlined `migrateLegacyDashboardTileColorsRaw` walker // (which delegates to `walkRawDashboardTileColors` in common-utils). // Without the migration the strict `DashboardWithoutIdSchema` diff --git a/packages/api/src/tasks/provisionDashboards/index.ts b/packages/api/src/tasks/provisionDashboards/index.ts index 8c002b314c..cf10427c72 100644 --- a/packages/api/src/tasks/provisionDashboards/index.ts +++ b/packages/api/src/tasks/provisionDashboards/index.ts @@ -1,3 +1,4 @@ +import { validateDashboardFilterOptionUniqueness } from '@hyperdx/common-utils/dist/dashboardValidation'; import { DashboardWithoutId, DashboardWithoutIdSchema, @@ -28,6 +29,11 @@ function migrateLegacyDashboardTileColorsRaw(raw: unknown): unknown { }); } +const provisionedDashboardSchema = DashboardWithoutIdSchema.superRefine( + (data, ctx) => + validateDashboardFilterOptionUniqueness(data.filters ?? [], ctx), +); + export function readDashboardFiles(dir: string): DashboardWithoutId[] { let files: string[]; try { @@ -43,7 +49,7 @@ export function readDashboardFiles(dir: string): DashboardWithoutId[] { const raw = migrateLegacyDashboardTileColorsRaw( JSON.parse(fs.readFileSync(path.join(dir, file), 'utf8')), ) as Record | null | undefined; - const parsed = DashboardWithoutIdSchema.safeParse({ + const parsed = provisionedDashboardSchema.safeParse({ tags: [], ...(raw as object), }); diff --git a/packages/api/src/utils/externalApi.ts b/packages/api/src/utils/externalApi.ts index 6f5f2044d3..fcfa710060 100644 --- a/packages/api/src/utils/externalApi.ts +++ b/packages/api/src/utils/externalApi.ts @@ -1,6 +1,7 @@ import { isFilterBroadcastEnabled, isFilterVariableEnabled, + isStaticListFilter, } from '@hyperdx/common-utils/dist/filters'; import { AlertErrorType, @@ -216,6 +217,9 @@ export function translateExternalChartToTileConfig( export function translateFilterToExternalFilter( filter: DashboardFilter, ): ExternalDashboardFilterWithId { + // Static filters require no translation + if (isStaticListFilter(filter)) return filter; + // Ignore variableName and appliesToSourceIds if the filter is not in a mode that uses them const ignoredKeys = [ ...(isFilterVariableEnabled(filter) ? [] : (['variableName'] as const)), @@ -232,6 +236,9 @@ export function translateFilterToExternalFilter( export function translateExternalFilterToFilter( filter: ExternalDashboardFilterWithId, ): DashboardFilter { + // Static filters require no translation + if (isStaticListFilter(filter)) return filter; + return { ...omit(filter, 'sourceId'), source: filter.sourceId, diff --git a/packages/api/src/utils/zod.ts b/packages/api/src/utils/zod.ts index dccf8cba85..947b889053 100644 --- a/packages/api/src/utils/zod.ts +++ b/packages/api/src/utils/zod.ts @@ -9,7 +9,6 @@ import { ChartPaletteTokenSchema, DASHBOARD_CONTAINER_ID_MAX, DASHBOARD_MAX_TILES, - DashboardFilterSchema, MAX_TAG_LENGTH, MAX_TAGS, MetricFormulaSchema, @@ -19,8 +18,10 @@ import { OnClickDashboardSchema, OnClickExternalSchema, OnClickSearchSchema, + QueryExpressionDashboardFilterSchema, scheduleStartAtSchema, SearchConditionLanguageSchema as whereLanguageSchema, + StaticListDashboardFilterSchema, tagsSchema, validateAlertChannelSelection, validateAlertScheduleOffsetMinutes, @@ -140,18 +141,34 @@ const chartSeriesSchema = z.discriminatedUnion('type', [ // while the canonical definition lives in the shared package. export { MAX_TAG_LENGTH, MAX_TAGS, tagsSchema }; -export const externalDashboardFilterSchemaWithId = DashboardFilterSchema.omit({ - source: true, -}) - .extend({ sourceId: objectIdSchema }) - .strict(); +const externalQueryExpressionFilterShape = + QueryExpressionDashboardFilterSchema.omit({ source: true }).extend({ + sourceId: objectIdSchema, + }); + +// A static filter's mode flags each accept exactly one value, so callers may +// omit them and get that value. +const externalStaticListFilterShape = StaticListDashboardFilterSchema.extend({ + isBroadcastEnabled: z.literal(false).default(false), + isVariableEnabled: z.literal(true).default(true), +}); + +export const externalDashboardFilterSchemaWithId = z.discriminatedUnion( + 'type', + [ + externalQueryExpressionFilterShape.strict(), + externalStaticListFilterShape.strict(), + ], +); export type ExternalDashboardFilterWithId = z.infer< typeof externalDashboardFilterSchemaWithId >; -export const externalDashboardFilterSchema = - externalDashboardFilterSchemaWithId.omit({ id: true }); +export const externalDashboardFilterSchema = z.discriminatedUnion('type', [ + externalQueryExpressionFilterShape.omit({ id: true }).strict(), + externalStaticListFilterShape.omit({ id: true }).strict(), +]); export type ExternalDashboardFilter = z.infer< typeof externalDashboardFilterSchema diff --git a/packages/app/src/DBDashboardImportPage.tsx b/packages/app/src/DBDashboardImportPage.tsx index c293b8b612..b4053ad27d 100644 --- a/packages/app/src/DBDashboardImportPage.tsx +++ b/packages/app/src/DBDashboardImportPage.tsx @@ -17,6 +17,8 @@ import { zodResolver } from '@hookform/resolvers/zod'; import { convertToDashboardDocument } from '@hyperdx/common-utils/dist/core/utils'; import { type DashboardFilterQueryIssue, + isQueryExpressionFilter, + isStaticListFilter, type SavedFilterValueIssue, type SavedQueryIssue, validateDashboardFilterQueries, @@ -25,6 +27,7 @@ import { } from '@hyperdx/common-utils/dist/filters'; import { isRawSqlSavedChartConfig } from '@hyperdx/common-utils/dist/guards'; import { + type DashboardFilter, type DashboardTemplate, DashboardTemplateSchema, isLogSource, @@ -406,6 +409,14 @@ const MappingFormStateSchema = z.object({ type MappingFormState = z.infer; +/** + * The query-expression half of a template filter, or `undefined` for a static + * one. Only a queried filter references a source or an applies-to list, and + * those references are the only thing this page remaps. + */ +const queriedTemplateFilter = (filter: DashboardFilter | undefined) => + filter && isQueryExpressionFilter(filter) ? filter : undefined; + export function Mapping({ input }: { input: DashboardTemplate }) { const router = useRouter(); const { data: sources } = useSources(); @@ -452,6 +463,7 @@ export function Mapping({ input }: { input: DashboardTemplate }) { }); const filterSourceMappings = input.filters?.map(filter => { + if (isStaticListFilter(filter)) return ''; const match = sources.find( source => source.name.toLowerCase() === filter.source.toLowerCase(), ); @@ -463,7 +475,11 @@ export function Mapping({ input }: { input: DashboardTemplate }) { // resolve are dropped — the surviving IDs become the multiselect's // initial value. const filterAppliesToSourceMappings = input.filters?.map( - filter => resolveAppliesToSources(filter.appliesToSourceIds, sources).ids, + filter => + resolveAppliesToSources( + queriedTemplateFilter(filter)?.appliesToSourceIds, + sources, + ).ids, ); // onClick targets in a template carry the source/dashboard *name* in @@ -555,7 +571,7 @@ export function Mapping({ input }: { input: DashboardTemplate }) { ); if (idx !== -1) { prevFilterSourceMappingsRef.current = filterSourceMappings; - inputSourceName = input.filters?.[idx]?.source; + inputSourceName = queriedTemplateFilter(input.filters?.[idx])?.source; selectedSourceId = filterSourceMappings[idx] ?? ''; } } @@ -586,8 +602,11 @@ export function Mapping({ input }: { input: DashboardTemplate }) { const keysForFiltersWithMatchingSource = input.filters - ?.map((filter, index) => ({ ...filter, index })) - .filter(f => f.source === inputSourceName) + ?.map((filter, index) => ({ filter, index })) + .filter( + ({ filter }) => + queriedTemplateFilter(filter)?.source === inputSourceName, + ) .map(({ index }) => `filterSourceMappings.${index}` as const) ?? []; const keysForOnClicksWithMatchingSource = input.tiles @@ -618,6 +637,7 @@ export function Mapping({ input }: { input: DashboardTemplate }) { // the resolver's stripped-list index so it lines up with the form-state // array (which has unresolved template names dropped). input.filters?.forEach((filter, filterIdx) => { + if (isStaticListFilter(filter)) return; if (!filter.appliesToSourceIds?.includes(inputSourceName)) return; const key = `filterAppliesToSourceMappings.${filterIdx}` as const; if (getFieldState(key).isDirty) return; @@ -817,7 +837,9 @@ export function Mapping({ input }: { input: DashboardTemplate }) { ); return { ...filter, - source: source!.id, + // A sourceless filter (`STATIC_LIST`) gets no mapping row, so + // `source` is undefined here + ...(source ? { source: source.id } : {}), appliesToSourceIds: appliesTo?.length ? appliesTo : undefined, }; }); @@ -987,45 +1009,52 @@ export function Mapping({ input }: { input: DashboardTemplate }) { {/** Map filter sources */} {input.filters?.map((filter, i) => ( - - {filter.name} (Filter) - Data Source - {filter.source} - - ({ - value: source.id, - label: source.name, - }))} - placeholder="Select a source" - /> - - - - - {!!filter.appliesToSourceIds?.length && ( + {/* A static filter reads its values from the list stored on it, + so it has neither a source nor an applies-to list to map. */} + {!isStaticListFilter(filter) && ( {filter.name} (Filter) + Data Source + {filter.source} - - Applies to Sources - - - {filter.appliesToSourceIds.join(', ')} - - ({ + value: source.id, + label: source.name, + }))} + placeholder="Select a source" /> )} + {!isStaticListFilter(filter) && + !!filter.appliesToSourceIds?.length && ( + + {filter.name} (Filter) + + + Applies to Sources + + + + {filter.appliesToSourceIds.join(', ')} + + + + + + + + )} ))} diff --git a/packages/app/src/DashboardFilters.tsx b/packages/app/src/DashboardFilters.tsx index c691e23647..d17bada2bb 100644 --- a/packages/app/src/DashboardFilters.tsx +++ b/packages/app/src/DashboardFilters.tsx @@ -1,10 +1,11 @@ import { useMemo, useState } from 'react'; import { FilterSelection } from '@hyperdx/common-utils/dist/dashboardFilterValues'; import { + getFilterBroadcastTarget, getFilterVariableName, getPendingFilterValuesVariables, - isFilterBroadcastEnabled, isFilterVariableEnabled, + isQueryExpressionFilter, } from '@hyperdx/common-utils/dist/filters'; import { ChartVariable, @@ -15,7 +16,7 @@ import { IconAlertTriangle, IconHelp, IconRefresh } from '@tabler/icons-react'; import { FilterLinkToggle } from './components/FilterLinkToggle'; import { VirtualMultiSelect } from './components/VirtualMultiSelect/VirtualMultiSelect'; -import { useDashboardFilterValues } from './hooks/useDashboardFilterValues'; +import { useQueriedDashboardFilterValues } from './hooks/useQueriedDashboardFilterValues'; interface DashboardFilterSelectProps { filter: DashboardFilter; @@ -56,8 +57,9 @@ export const getFilterEffect = ( ): { hasEffect: boolean; tooltip: string } => { const parts: string[] = []; - if (isFilterBroadcastEnabled(filter)) { - const count = filter.appliesToSourceIds?.length ?? 0; + const broadcast = getFilterBroadcastTarget(filter); + if (broadcast) { + const count = broadcast.appliesToSourceIds?.length ?? 0; parts.push( count === 0 ? 'Filters all sources' @@ -196,13 +198,18 @@ const DashboardFilters = ({ // on, all of a source's facets are computed in a single groupUniqArrayIf scan. const [linked, setLinked] = useState(false); + const queriedFilters = useMemo( + () => filters.filter(f => isQueryExpressionFilter(f)), + [filters], + ); + const { data: filterValuesById, erroredFilterIds, filterErrorMessages, isFetching, - } = useDashboardFilterValues({ - filters, + } = useQueriedDashboardFilterValues({ + filters: queriedFilters, dateRange, variables, // Only narrow by sibling selections when linked. @@ -211,7 +218,7 @@ const DashboardFilters = ({ return ( - {Object.values(filters).map(filter => { + {queriedFilters.map(filter => { const queriedFilterValues = filterValuesById?.get(filter.id); const included = selectionByFilterId.get(filter.id)?.included; const selectedValues = included diff --git a/packages/app/src/components/DBEditTimeChartForm/OnClickForm/OnClickDrawer.tsx b/packages/app/src/components/DBEditTimeChartForm/OnClickForm/OnClickDrawer.tsx index baf3e13272..a5dcd2f1e7 100644 --- a/packages/app/src/components/DBEditTimeChartForm/OnClickForm/OnClickDrawer.tsx +++ b/packages/app/src/components/DBEditTimeChartForm/OnClickForm/OnClickDrawer.tsx @@ -8,6 +8,7 @@ import { } from 'react-hook-form'; import { zodResolver } from '@hookform/resolvers/zod'; import { validateOnClickTemplate } from '@hyperdx/common-utils/dist/core/linkUrlBuilder'; +import { getFilterExpression } from '@hyperdx/common-utils/dist/filters'; import { isSearchableSource, OnClick, @@ -119,11 +120,18 @@ function DashboardOnClickFields({ setValue( 'onClick.filters', - dashboardFilters.map(f => ({ - kind: 'expressionTemplate' as const, - expression: f.expression, - template: '', - })), + dashboardFilters.flatMap(f => { + const expression = getFilterExpression(f); + return expression != null + ? [ + { + kind: 'expressionTemplate' as const, + expression, + template: '', + }, + ] + : []; + }), ); }, [dashboards, setValue, getValues], diff --git a/packages/app/src/components/DashboardFiltersModal/DashboardFilterEditForm.tsx b/packages/app/src/components/DashboardFiltersModal/DashboardFilterEditForm.tsx index e407d0f60a..bae781ffde 100644 --- a/packages/app/src/components/DashboardFiltersModal/DashboardFilterEditForm.tsx +++ b/packages/app/src/components/DashboardFiltersModal/DashboardFilterEditForm.tsx @@ -11,6 +11,7 @@ import { import { DashboardFilter, MetricsDataType, + QueryExpressionDashboardFilter, SourceKind, TSource, } from '@hyperdx/common-utils/dist/types'; @@ -46,7 +47,8 @@ import { MODAL_SIZE } from './constants'; import { CustomInputWrapper } from './CustomInputWrapper'; interface DashboardFilterEditFormProps { - filter: DashboardFilter; + /** TODO: Support editing static list filters. */ + filter: QueryExpressionDashboardFilter; isNew: boolean; source: TSource | undefined; /** Filters that already exist on the dashboard, used to keep variable names unique. */ @@ -66,7 +68,9 @@ const TOOLTIP_PORTAL_TARGET = typeof document !== 'undefined' ? document.body : null; /** Normalize a stored filter into form values. */ -const toFormValues = (filter: DashboardFilter): DashboardFilter => ({ +const toFormValues = ( + filter: QueryExpressionDashboardFilter, +): QueryExpressionDashboardFilter => ({ ...filter, where: filter.where ?? '', whereLanguage: filter.whereLanguage ?? getStoredLanguage() ?? 'sql', @@ -94,7 +98,7 @@ export const DashboardFilterEditForm = ({ reset, setValue, trigger, - } = useForm({ + } = useForm({ defaultValues: toFormValues(filter), }); @@ -433,7 +437,7 @@ export const DashboardFilterEditForm = ({ data-testid="filter-variable-enabled-checkbox" rules={{ validate: validateFilterModes }} /> - {formIsVariableEnabled && ( + {!!formIsVariableEnabled && ( void; } +function getValuesSourceName( + filter: DashboardFilter, + sources?: { id: string; name: string }[], +) { + if (isQueryExpressionFilter(filter)) { + return sources?.find(s => s.id === filter?.source)?.name; + } + + return 'Custom values'; +} + +function getBroadcastTargetDisplay( + filter: DashboardFilter, + sources?: { id: string; name: string }[], +) { + const broadcastTarget = getFilterBroadcastTarget(filter); + if (!broadcastTarget) return undefined; + + if (!broadcastTarget?.appliesToSourceIds?.length) { + return 'All sources'; + } + + const appliedSourceNames = broadcastTarget.appliesToSourceIds + .map(id => sources?.find(s => s.id === id)?.name) + .filter((name): name is string => !!name); + + return appliedSourceNames.join(', '); +} + export const DashboardFiltersList = ({ filters, isLoading, @@ -59,17 +89,9 @@ export const DashboardFiltersList = ({ data-testid="dashboard-filters-list" > {filters.map(filter => { - const queriedSourceName = sources?.find( - s => s.id === filter.source, - )?.name; - const appliedSourceNames = filter.appliesToSourceIds?.length - ? filter.appliesToSourceIds - .map(id => sources?.find(s => s.id === id)?.name) - .filter((name): name is string => !!name) - : undefined; - const appliedDisplay = appliedSourceNames - ? appliedSourceNames.join(', ') - : 'All sources'; + const valuesSourceName = getValuesSourceName(filter, sources); + const broadcastTarget = getBroadcastTargetDisplay(filter, sources); + const variableName = showVariableOptions && isFilterVariableEnabled(filter) ? getFilterVariableName(filter) @@ -79,13 +101,9 @@ export const DashboardFiltersList = ({ key={filter.id} name={filter.name} nameSuffix={variableName ? ` ($${variableName})` : undefined} - queriedFrom={queriedSourceName ?? ''} - queriedFromTooltip="Source the dropdown values are queried from" - appliedTo={ - !hideAppliesTo && isFilterBroadcastEnabled(filter) - ? appliedDisplay - : undefined - } + queriedFrom={valuesSourceName ?? ''} + queriedFromTooltip="Source of the available filter values" + appliedTo={!hideAppliesTo ? broadcastTarget : undefined} actions={ <> ); - } else if (selectedFilter) { + } else if (selectedFilter && isQueryExpressionFilter(selectedFilter)) { return ( ({ })); describe('useDashboardFilters', () => { - const mockFilters: DashboardFilter[] = [ + const mockFilters: QueryExpressionDashboardFilter[] = [ { id: 'filter1', type: 'QUERY_EXPRESSION', diff --git a/packages/app/src/hooks/__tests__/useDashboardFilterValues.test.tsx b/packages/app/src/hooks/__tests__/useQueriedDashboardFilterValues.test.tsx similarity index 96% rename from packages/app/src/hooks/__tests__/useDashboardFilterValues.test.tsx rename to packages/app/src/hooks/__tests__/useQueriedDashboardFilterValues.test.tsx index 404f5b6db3..beffd09d99 100644 --- a/packages/app/src/hooks/__tests__/useDashboardFilterValues.test.tsx +++ b/packages/app/src/hooks/__tests__/useQueriedDashboardFilterValues.test.tsx @@ -9,14 +9,15 @@ import { FilterSelection } from '@hyperdx/common-utils/dist/dashboardFilterValue import { DashboardFilter, MetricsDataType, + QueryExpressionDashboardFilter, SourceKind, TSource, } from '@hyperdx/common-utils/dist/types'; import { QueryClient, QueryClientProvider } from '@tanstack/react-query'; import { renderHook, waitFor } from '@testing-library/react'; -import { useDashboardFilterValues } from '@/hooks/useDashboardFilterValues'; import * as useMetadataModule from '@/hooks/useMetadata'; +import { useQueriedDashboardFilterValues } from '@/hooks/useQueriedDashboardFilterValues'; import * as sourceModule from '@/source'; // Mock modules @@ -34,7 +35,7 @@ jest.mock('@hyperdx/common-utils/dist/core/materializedViews', () => ({ .mockImplementation(async ({ chartConfig }) => chartConfig), })); -describe('useDashboardFilterValues', () => { +describe('useQueriedDashboardFilterValues', () => { let queryClient: QueryClient; let wrapper: React.ComponentType<{ children: any }>; let mockMetadata: jest.Mocked; @@ -172,7 +173,7 @@ describe('useDashboardFilterValues', () => { // Act const { result } = renderHook( () => - useDashboardFilterValues({ + useQueriedDashboardFilterValues({ filters: [ { id: 'filterSevNumber', @@ -213,7 +214,7 @@ describe('useDashboardFilterValues', () => { // Act const { result } = renderHook( () => - useDashboardFilterValues({ + useQueriedDashboardFilterValues({ filters: mockFilters, dateRange: mockDateRange, }), @@ -339,7 +340,7 @@ describe('useDashboardFilterValues', () => { // Act const { result } = renderHook( () => - useDashboardFilterValues({ + useQueriedDashboardFilterValues({ filters: sameSourceFilters, dateRange: mockDateRange, }), @@ -388,7 +389,7 @@ describe('useDashboardFilterValues', () => { // Act const { result } = renderHook( () => - useDashboardFilterValues({ + useQueriedDashboardFilterValues({ filters: sameSourceFiltersDifferentWhere, dateRange: mockDateRange, }), @@ -417,7 +418,7 @@ describe('useDashboardFilterValues', () => { // Act const { result } = renderHook( () => - useDashboardFilterValues({ + useQueriedDashboardFilterValues({ filters: [], dateRange: mockDateRange, }), @@ -439,7 +440,7 @@ describe('useDashboardFilterValues', () => { // Act renderHook( () => - useDashboardFilterValues({ + useQueriedDashboardFilterValues({ filters: mockFilters, dateRange: mockDateRange, }), @@ -471,7 +472,7 @@ describe('useDashboardFilterValues', () => { // Act const { result } = renderHook( () => - useDashboardFilterValues({ + useQueriedDashboardFilterValues({ filters: filtersWithInvalidSource, dateRange: mockDateRange, }), @@ -499,7 +500,7 @@ describe('useDashboardFilterValues', () => { // Act const { result } = renderHook( () => - useDashboardFilterValues({ + useQueriedDashboardFilterValues({ filters: mockFilters, dateRange: mockDateRange, }), @@ -522,7 +523,7 @@ describe('useDashboardFilterValues', () => { // Act const { result } = renderHook( () => - useDashboardFilterValues({ + useQueriedDashboardFilterValues({ filters: [mockFilters[0]], // Only first filter (logs-source) dateRange: mockDateRange, }), @@ -574,7 +575,7 @@ describe('useDashboardFilterValues', () => { // Act const { result, rerender } = renderHook( ({ filters, dateRange }) => - useDashboardFilterValues({ + useQueriedDashboardFilterValues({ filters, dateRange, }), @@ -640,7 +641,7 @@ describe('useDashboardFilterValues', () => { // Act const { result } = renderHook( () => - useDashboardFilterValues({ + useQueriedDashboardFilterValues({ filters: multiFilters, dateRange: mockDateRange, }), @@ -728,7 +729,7 @@ describe('useDashboardFilterValues', () => { // Act const { result } = renderHook( () => - useDashboardFilterValues({ + useQueriedDashboardFilterValues({ filters: filtersForSameSource, dateRange: mockDateRange, }), @@ -810,7 +811,7 @@ describe('useDashboardFilterValues', () => { // Act const { result } = renderHook( () => - useDashboardFilterValues({ + useQueriedDashboardFilterValues({ filters: mockFilters.slice(0, 2), // Only first two filters dateRange: mockDateRange, }), @@ -871,7 +872,7 @@ describe('useDashboardFilterValues', () => { // Act const { result } = renderHook( () => - useDashboardFilterValues({ + useQueriedDashboardFilterValues({ filters: mockFilters.slice(0, 2), // Only first two filters dateRange: mockDateRange, }), @@ -924,7 +925,7 @@ describe('useDashboardFilterValues', () => { // Act const { result } = renderHook( () => - useDashboardFilterValues({ + useQueriedDashboardFilterValues({ filters: [mockFilters[0]], dateRange: mockDateRange, }), @@ -980,7 +981,7 @@ describe('useDashboardFilterValues', () => { // Act - Initial render const { result, rerender } = renderHook( ({ filters, dateRange }) => - useDashboardFilterValues({ + useQueriedDashboardFilterValues({ filters, dateRange, }), @@ -1074,7 +1075,8 @@ describe('useDashboardFilterValues', () => { .filter(([arg]) => JSON.stringify(arg.keys) === JSON.stringify(keys)) .at(-1)?.[0]; - const envAndStatus: DashboardFilter[] = [ + // Typed on the queried variant: the spreads below stay in one union member. + const envAndStatus: QueryExpressionDashboardFilter[] = [ { id: 'filter1', type: 'QUERY_EXPRESSION', @@ -1134,7 +1136,7 @@ describe('useDashboardFilterValues', () => { it('resolves every key in one faceted scan, constraining each by the others (exclude-self)', async () => { const { result } = renderHook( () => - useDashboardFilterValues({ + useQueriedDashboardFilterValues({ filters: envAndStatus, dateRange: mockDateRange, selectionByFilterId: envProductionSelection, @@ -1157,7 +1159,7 @@ describe('useDashboardFilterValues', () => { it('runs one unconstrained query when nothing is selected', async () => { const { result } = renderHook( () => - useDashboardFilterValues({ + useQueriedDashboardFilterValues({ filters: envAndStatus, dateRange: mockDateRange, selectionByFilterId: new Map(), @@ -1188,7 +1190,7 @@ describe('useDashboardFilterValues', () => { const { result } = renderHook( () => - useDashboardFilterValues({ + useQueriedDashboardFilterValues({ filters, dateRange: mockDateRange, selectionByFilterId: envProductionSelection, @@ -1217,7 +1219,7 @@ describe('useDashboardFilterValues', () => { const { result } = renderHook( () => - useDashboardFilterValues({ + useQueriedDashboardFilterValues({ filters, dateRange: mockDateRange, selectionByFilterId: new Map([ @@ -1273,7 +1275,7 @@ describe('useDashboardFilterValues', () => { const { result } = renderHook( () => - useDashboardFilterValues({ + useQueriedDashboardFilterValues({ filters, dateRange: mockDateRange, selectionByFilterId: envProductionSelection, @@ -1316,7 +1318,7 @@ describe('useDashboardFilterValues', () => { const { result } = renderHook( () => - useDashboardFilterValues({ + useQueriedDashboardFilterValues({ filters, dateRange: mockDateRange, selectionByFilterId: new Map([ @@ -1341,7 +1343,7 @@ describe('useDashboardFilterValues', () => { it('refetches with updated conditions when a selection changes', async () => { const { result, rerender } = renderHook( ({ selectionByFilterId }) => - useDashboardFilterValues({ + useQueriedDashboardFilterValues({ filters: envAndStatus, dateRange: mockDateRange, selectionByFilterId, @@ -1384,7 +1386,7 @@ describe('useDashboardFilterValues', () => { const { result } = renderHook( () => - useDashboardFilterValues({ + useQueriedDashboardFilterValues({ filters: envAndStatus, dateRange: mockDateRange, selectionByFilterId: envProductionSelection, @@ -1407,7 +1409,7 @@ describe('useDashboardFilterValues', () => { it('expands variable references in the where clause without dropping the conditions', async () => { const { result } = renderHook( () => - useDashboardFilterValues({ + useQueriedDashboardFilterValues({ filters: [ envAndStatus[0], { @@ -1444,7 +1446,7 @@ describe('useDashboardFilterValues', () => { const statusFilter = ( where: string, whereLanguage: 'lucene' | 'sql' = 'sql', - ): DashboardFilter => ({ + ): QueryExpressionDashboardFilter => ({ id: 'status', type: 'QUERY_EXPRESSION', name: 'Status', @@ -1475,7 +1477,7 @@ describe('useDashboardFilterValues', () => { it('expands a macro against the selected values', async () => { const { result } = renderHook( () => - useDashboardFilterValues({ + useQueriedDashboardFilterValues({ filters: [statusFilter('$__filter(service_name, $svc)')], dateRange: mockDateRange, variables: svcVariable(['api']), @@ -1499,7 +1501,7 @@ describe('useDashboardFilterValues', () => { it('still queries when the variable it depends on has no selection', async () => { const { result } = renderHook( () => - useDashboardFilterValues({ + useQueriedDashboardFilterValues({ filters: [statusFilter('$__filter(service_name, $svc)')], dateRange: mockDateRange, variables: svcVariable([]), @@ -1524,7 +1526,7 @@ describe('useDashboardFilterValues', () => { it('expands a lucene clause in the lucene format', async () => { const { result } = renderHook( () => - useDashboardFilterValues({ + useQueriedDashboardFilterValues({ filters: [statusFilter('service_name:$svc', 'lucene')], dateRange: mockDateRange, variables: svcVariable(['api']), @@ -1540,7 +1542,7 @@ describe('useDashboardFilterValues', () => { it('refetches when a referenced variable changes', async () => { const { result, rerender } = renderHook( ({ values }: { values: string[] }) => - useDashboardFilterValues({ + useQueriedDashboardFilterValues({ filters: [statusFilter('$__filter(service_name, $svc)')], dateRange: mockDateRange, variables: svcVariable(values), @@ -1561,7 +1563,7 @@ describe('useDashboardFilterValues', () => { it('does not refetch when an unreferenced variable changes', async () => { const { result, rerender } = renderHook( ({ values }: { values: string[] }) => - useDashboardFilterValues({ + useQueriedDashboardFilterValues({ filters: [statusFilter("service_name = 'api'")], dateRange: mockDateRange, variables: [ @@ -1585,7 +1587,7 @@ describe('useDashboardFilterValues', () => { it('groups filters whose different clauses expand to the same SQL', async () => { const { result } = renderHook( () => - useDashboardFilterValues({ + useQueriedDashboardFilterValues({ filters: [ // Written out exactly as the macro below expands. statusFilter("(service_name IN ('api'))"), @@ -1613,7 +1615,7 @@ describe('useDashboardFilterValues', () => { it('reports a macro naming an undeclared variable, and still runs the query', async () => { const { result } = renderHook( () => - useDashboardFilterValues({ + useQueriedDashboardFilterValues({ filters: [statusFilter('$__filter(service_name, $nope)')], dateRange: mockDateRange, variables: svcVariable(['api']), @@ -1634,7 +1636,7 @@ describe('useDashboardFilterValues', () => { it('leaves the clause as written when no variables are in scope', async () => { const { result } = renderHook( () => - useDashboardFilterValues({ + useQueriedDashboardFilterValues({ filters: [statusFilter('service_name IN ($svc)')], dateRange: mockDateRange, }), diff --git a/packages/app/src/hooks/useDashboardFilters.tsx b/packages/app/src/hooks/useDashboardFilters.tsx index 5b93160ba8..2307a0e0fd 100644 --- a/packages/app/src/hooks/useDashboardFilters.tsx +++ b/packages/app/src/hooks/useDashboardFilters.tsx @@ -12,7 +12,11 @@ import { FilterState, filtersToQuery, getDashboardVariableFilters, + getFilterBroadcastTarget, + getFilterExpression, isFilterBroadcastEnabled, + isQueryExpressionFilter, + isStaticListFilter, } from '@hyperdx/common-utils/dist/filters'; import { ChartVariable, @@ -30,8 +34,9 @@ const definitionAppliesToSource = ( definition: DashboardFilter, sourceId: string | undefined, ): boolean => { - if (!isFilterBroadcastEnabled(definition)) return false; - const appliesTo = definition.appliesToSourceIds; + const target = getFilterBroadcastTarget(definition); + if (!target) return false; + const appliesTo = target.appliesToSourceIds; if (!appliesTo || appliesTo.length === 0) return true; return !!sourceId && appliesTo.includes(sourceId); }; @@ -64,7 +69,9 @@ const rebuildEntries = ( const declaredVariableNames = new Set(); for (const filter of filters) { - declaredExpressions.add(filter.expression); + const expression = getFilterExpression(filter); + if (expression) declaredExpressions.add(expression); + const key = filterSelectionKey(filter); if (key.kind !== 'variable') continue; declaredVariableNames.add(key.name); @@ -72,8 +79,8 @@ const rebuildEntries = ( const selection = resolveFilterSelection(filter, parsed); if (!selection) continue; - if (!isRepresentableAsVariable(selection)) { - byExpression[filter.expression] = selection; + if (!isRepresentableAsVariable(selection) && expression) { + byExpression[expression] = selection; } else if (selection.included.size > 0) { byVariable.set(key.name, Array.from(selection.included).map(String)); } @@ -83,10 +90,11 @@ const rebuildEntries = ( // the loop above so that when a plain and a variable-enabled filter share an // expression, the surviving entry carries the plain filter's selection. for (const filter of filters) { - if (filterSelectionKey(filter).kind === 'variable') continue; + const key = filterSelectionKey(filter); + if (key.kind !== 'expression') continue; const selection = resolveFilterSelection(filter, parsed); if (selection && hasSelection(selection)) { - byExpression[filter.expression] = selection; + byExpression[key.expression] = selection; } } @@ -164,7 +172,9 @@ const useDashboardFilters = (filters: DashboardFilter[]) => { } // Find state that doesn't correspond to any declared filter, so the caller can surface a warning. - const knownExpressions = new Set(filters.map(f => f.expression)); + const knownExpressions = new Set( + filters.filter(isQueryExpressionFilter).map(f => f.expression), + ); const ignoredExpressions = Object.keys(parsed.byExpression).filter( expression => !knownExpressions.has(expression), ); @@ -180,7 +190,7 @@ const useDashboardFilters = (filters: DashboardFilter[]) => { const selection = selectionByFilterId.get(filter.id); return { name, - expression: filter.expression, + expression: getFilterExpression(filter), // Sorted for deterministic react-query keys. Built explicitly rather // than by spreading the definition, so no extra key leaks into them. values: selection @@ -205,6 +215,7 @@ const useDashboardFilters = (filters: DashboardFilter[]) => { const queries: Filter[] = []; for (const filter of filters) { if (!predicate(filter)) continue; + if (isStaticListFilter(filter)) continue; const selection = selectionByFilterId.get(filter.id); if (!selection) continue; // Wrap keys in `toString()` to support JSON/Dynamic-type columns. diff --git a/packages/app/src/hooks/usePresetDashboardFilters.tsx b/packages/app/src/hooks/usePresetDashboardFilters.tsx index f23091e5d3..e6e2fe764a 100644 --- a/packages/app/src/hooks/usePresetDashboardFilters.tsx +++ b/packages/app/src/hooks/usePresetDashboardFilters.tsx @@ -1,4 +1,5 @@ import { useCallback } from 'react'; +import { isStaticListFilter } from '@hyperdx/common-utils/dist/filters'; import { DashboardFilter, PresetDashboard, @@ -51,6 +52,9 @@ export default function usePresetDashboardFilters({ const handleSaveFilter = useCallback( (dashboardFilter: DashboardFilter) => { + // Preset dashboards are broadcast-only, so static list filters are not supported. + if (isStaticListFilter(dashboardFilter)) return; + const presetDashboardFilter = { ...dashboardFilter, presetDashboard, diff --git a/packages/app/src/hooks/useDashboardFilterValues.tsx b/packages/app/src/hooks/useQueriedDashboardFilterValues.tsx similarity index 97% rename from packages/app/src/hooks/useDashboardFilterValues.tsx rename to packages/app/src/hooks/useQueriedDashboardFilterValues.tsx index f15eb2fd9f..f15bde6e87 100644 --- a/packages/app/src/hooks/useDashboardFilterValues.tsx +++ b/packages/app/src/hooks/useQueriedDashboardFilterValues.tsx @@ -15,9 +15,9 @@ import { import { BuilderChartConfigWithDateRange, ChartVariable, - DashboardFilter, isLogSource, isTraceSource, + QueryExpressionDashboardFilter, } from '@hyperdx/common-utils/dist/types'; import { useQueries, @@ -42,7 +42,7 @@ type FilterSourceKey = { }; const filterToKey = ( - filter: DashboardFilter, + filter: QueryExpressionDashboardFilter, resolved: ResolvedFilterValuesQuery, ): string => JSON.stringify({ @@ -58,7 +58,7 @@ const filterToKey = ( /** The resolved dropdown query for `filter`, falling back to the clause as written. */ const resolvedFor = ( resolvedByFilterId: Map, - filter: DashboardFilter, + filter: QueryExpressionDashboardFilter, ): ResolvedFilterValuesQuery => resolvedByFilterId.get(filter.id) ?? { where: filter.where ?? '', @@ -105,7 +105,7 @@ function useOptimizedKeyValuesCalls({ selectionByFilterId, variables, }: { - filters: DashboardFilter[]; + filters: QueryExpressionDashboardFilter[]; dateRange: [Date, Date]; selectionByFilterId: ReadonlyMap; variables?: ChartVariable[]; @@ -176,7 +176,7 @@ function useOptimizedKeyValuesCalls({ // optimizer (one batched, rollup-eligible query); a constrained group runs a // single faceted `groupUniqArrayIf` scan carrying a per-key condition. const filtersByGroupKey = useMemo(() => { - const byGroupKey = new Map(); + const byGroupKey = new Map(); for (const filter of filters) { const key = filterToKey(filter, resolvedFor(resolvedByFilterId, filter)); const existing = byGroupKey.get(key); @@ -319,13 +319,13 @@ function useOptimizedKeyValuesCalls({ }; } -export function useDashboardFilterValues({ +export function useQueriedDashboardFilterValues({ filters, dateRange, selectionByFilterId = EMPTY_SELECTIONS, variables, }: { - filters: DashboardFilter[]; + filters: QueryExpressionDashboardFilter[]; dateRange: [Date, Date]; /** Each filter's current selection, keyed by `filter.id`. */ selectionByFilterId?: ReadonlyMap; diff --git a/packages/common-utils/src/__tests__/dashboardFilterValues.test.ts b/packages/common-utils/src/__tests__/dashboardFilterValues.test.ts index 7346bf941f..dec0de9b82 100644 --- a/packages/common-utils/src/__tests__/dashboardFilterValues.test.ts +++ b/packages/common-utils/src/__tests__/dashboardFilterValues.test.ts @@ -5,9 +5,15 @@ import { serializeDashboardFilterValues, } from '@/dashboardFilterValues'; import { FilterState, filtersToQuery } from '@/filters'; -import type { DashboardFilter, DashboardFilterValue } from '@/types'; - -const filter = (overrides: Partial = {}): DashboardFilter => ({ +import type { + DashboardFilterValue, + QueryExpressionDashboardFilter, + StaticListDashboardFilter, +} from '@/types'; + +const filter = ( + overrides: Partial = {}, +): QueryExpressionDashboardFilter => ({ id: 'f1', type: 'QUERY_EXPRESSION', name: 'Service', @@ -16,6 +22,19 @@ const filter = (overrides: Partial = {}): DashboardFilter => ({ ...overrides, }); +const staticFilter = ( + overrides: Partial = {}, +): StaticListDashboardFilter => ({ + id: 'f1', + type: 'STATIC_LIST', + name: 'Environment', + options: ['prod', 'staging', 'dev'], + isBroadcastEnabled: false, + isVariableEnabled: true, + variableName: 'env', + ...overrides, +}); + const included = (...values: (string | boolean)[]) => ({ included: new Set(values), excluded: new Set(), @@ -319,6 +338,22 @@ describe('dashboardFilterValues', () => { filterSelectionKey(filter({ name: '环境', isVariableEnabled: true })), ).toEqual({ kind: 'expression', expression: 'ServiceName' }); }); + + it('keys a static-list filter by its variable name', () => { + expect(filterSelectionKey(staticFilter())).toEqual({ + kind: 'variable', + name: 'env', + }); + }); + + // Unreachable through the write paths — a static filter is variable-only by + // construction — but it has no expression to fall back to, so the key stays + // variable-kind even when no name can be derived from it. + it('keys a static-list filter by variable even when no name can be derived', () => { + expect( + filterSelectionKey(staticFilter({ name: '环境', variableName: '' })), + ).toEqual({ kind: 'variable', name: '' }); + }); }); describe('resolveFilterSelection', () => { @@ -380,6 +415,24 @@ describe('dashboardFilterValues', () => { expect(resolveFilterSelection(filter(), parsed)).toBeUndefined(); }); + it('resolves a static-list filter from its variable entry', () => { + const parsed = parseDashboardFilterValues([ + { type: 'variable', name: 'env', values: ['prod'] }, + ]); + + expect(resolveFilterSelection(staticFilter(), parsed)).toEqual( + included('prod'), + ); + }); + + it('returns undefined for an expressionless filter with no variable entry', () => { + const parsed = parseDashboardFilterValues([ + { type: 'sql', condition: "ServiceName IN ('legacy')" }, + ]); + + expect(resolveFilterSelection(staticFilter(), parsed)).toBeUndefined(); + }); + it('resolves two filters sharing an expression independently', () => { const parsed = parseDashboardFilterValues([ { type: 'sql', condition: "ServiceName IN ('plain')" }, diff --git a/packages/common-utils/src/__tests__/dashboardValidation.test.ts b/packages/common-utils/src/__tests__/dashboardValidation.test.ts index 0906fa0e9a..ad823b40be 100644 --- a/packages/common-utils/src/__tests__/dashboardValidation.test.ts +++ b/packages/common-utils/src/__tests__/dashboardValidation.test.ts @@ -4,6 +4,7 @@ import { validateChartConfigFormulas, validateDashboardFilterFieldGating, validateDashboardFilterModes, + validateDashboardFilterOptionUniqueness, validateDashboardFilterVariableNames, } from '@/dashboardValidation'; @@ -421,6 +422,89 @@ describe('validateDashboardFilterFieldGating', () => { }); }); +type OptionsFilter = Parameters< + typeof validateDashboardFilterOptionUniqueness +>[0][number]; + +const collectOptionIssues = ( + filters: OptionsFilter[], + paths?: { filtersPath?: (string | number)[] }, +) => { + const schema = z + .object({}) + .superRefine((_, ctx) => + validateDashboardFilterOptionUniqueness(filters, ctx, paths), + ); + const result = schema.safeParse({}); + return result.success ? [] : result.error.issues; +}; + +// Everything else per-type — which fields exist, `options` being non-empty, the +// literal variable-only modes — is carried by the `DashboardFilterSchema` +// variants and covered in `types.test.ts`. Uniqueness is what a shape cannot +// state, so it lives here. +describe('validateDashboardFilterOptions', () => { + it('accepts an empty filter list', () => { + expect(collectOptionIssues([])).toEqual([]); + }); + + it('accepts a filter with no options at all', () => { + expect(collectOptionIssues([{ name: 'Service' }])).toEqual([]); + }); + + it('accepts distinct options', () => { + expect( + collectOptionIssues([ + { name: 'Environment', options: ['prod', 'staging', 'dev'] }, + ]), + ).toEqual([]); + }); + + it('reports a repeated option once, naming it', () => { + const issues = collectOptionIssues([ + { name: 'Environment', options: ['prod', 'dev', 'prod', 'dev'] }, + ]); + + expect(issues).toHaveLength(1); + expect(issues[0].path).toEqual(['filters', 0, 'options']); + expect(issues[0].message).toBe( + 'Filter "Environment" repeats the option "prod"; options must be unique', + ); + }); + + // Case and whitespace make a distinct value: the dropdown offers them as + // written and a selection carries them verbatim. + it('compares options exactly', () => { + expect( + collectOptionIssues([ + { name: 'Environment', options: ['prod', 'PROD', ' prod'] }, + ]), + ).toEqual([]); + }); + + it('reports every offending filter, by index', () => { + const issues = collectOptionIssues([ + { name: 'Fine', options: ['a', 'b'] }, + { name: 'Broken A', options: ['a', 'a'] }, + { name: 'Broken B', options: ['b', 'b'] }, + ]); + + expect(issues.map(i => i.path)).toEqual([ + ['filters', 1, 'options'], + ['filters', 2, 'options'], + ]); + }); + + it('honors a custom filters path', () => { + const issues = collectOptionIssues( + [{ name: 'Environment', options: ['prod', 'prod'] }], + { filtersPath: ['body', 'filters'] }, + ); + + expect(issues[0].path).toEqual(['body', 'filters', 0, 'options']); + }); +}); + describe('validateChartConfigFormulas', () => { type TestConfig = Parameters[0]; diff --git a/packages/common-utils/src/__tests__/filters.test.ts b/packages/common-utils/src/__tests__/filters.test.ts index 3e30f8e6af..74ab7ccaea 100644 --- a/packages/common-utils/src/__tests__/filters.test.ts +++ b/packages/common-utils/src/__tests__/filters.test.ts @@ -5,11 +5,14 @@ import { filtersToQuery, getDashboardVariableDeclarations, getDashboardVariableFilters, + getFilterBroadcastTarget, + getFilterExpression, getFilterVariableName, getPendingFilterValuesVariables, hasFilterEffect, isFilterBroadcastEnabled, isFilterVariableEnabled, + isQueryExpressionFilter, isRenderablePinnedFilter, parseQuery, resolveFilterValuesWhere, @@ -19,7 +22,12 @@ import { validateSavedQuery, validateVariableName, } from '@/filters'; -import type { ChartVariable, DashboardFilter, Filter } from '@/types'; +import type { + ChartVariable, + DashboardFilter, + Filter, + QueryExpressionDashboardFilter, +} from '@/types'; import { DASHBOARD_VARIABLE_NAME_MAX_LENGTH, DASHBOARD_VARIABLE_NAME_PATTERN_ANCHORED, @@ -651,7 +659,9 @@ describe('filters', () => { }); describe('validateDashboardFilterQueries', () => { - const filter = (overrides: Partial): DashboardFilter => ({ + const filter = ( + overrides: Partial, + ): QueryExpressionDashboardFilter => ({ id: 'f1', type: 'QUERY_EXPRESSION', name: 'ServiceName', @@ -678,6 +688,22 @@ describe('filters', () => { ).toEqual([]); }); + it('skips a static-list filter, which has no values query', () => { + expect( + validateDashboardFilterQueries([ + { + id: 'f1', + type: 'STATIC_LIST', + name: 'Environment', + options: ['prod', 'staging', 'dev'], + isBroadcastEnabled: false, + isVariableEnabled: true, + variableName: 'env', + }, + ]), + ).toEqual([]); + }); + it('accepts a valid lucene where clause', () => { expect( validateDashboardFilterQueries([ @@ -1432,6 +1458,78 @@ describe('filters', () => { }); }); + describe('isQueryExpressionFilter / getFilterExpression', () => { + const queried: QueryExpressionDashboardFilter = { + id: 'f1', + type: 'QUERY_EXPRESSION', + name: 'Service', + expression: 'ServiceName', + source: 'logs', + }; + const staticList: DashboardFilter = { + id: 'f2', + type: 'STATIC_LIST', + name: 'Environment', + options: ['prod'], + isBroadcastEnabled: false, + isVariableEnabled: true, + }; + + it('identifies a queried filter and reports its expression', () => { + expect(isQueryExpressionFilter(queried)).toBe(true); + expect(getFilterExpression(queried)).toBe('ServiceName'); + }); + + it('rejects a static-list filter, which names no column', () => { + expect(isQueryExpressionFilter(staticList)).toBe(false); + expect(getFilterExpression(staticList)).toBeUndefined(); + }); + }); + + describe('getFilterBroadcastTarget', () => { + const filter = ( + overrides: Partial, + ): QueryExpressionDashboardFilter => ({ + id: 'f1', + type: 'QUERY_EXPRESSION', + name: 'Service', + expression: 'ServiceName', + source: 'logs', + ...overrides, + }); + + it('reports the expression and scope for a broadcasting filter', () => { + expect(getFilterBroadcastTarget(filter({}))).toEqual({ + expression: 'ServiceName', + appliesToSourceIds: undefined, + }); + expect( + getFilterBroadcastTarget(filter({ appliesToSourceIds: ['logs'] })), + ).toEqual({ expression: 'ServiceName', appliesToSourceIds: ['logs'] }); + }); + + it('returns undefined when broadcasting is off', () => { + expect( + getFilterBroadcastTarget( + filter({ isBroadcastEnabled: false, appliesToSourceIds: ['logs'] }), + ), + ).toBeUndefined(); + }); + + it('returns undefined for a static-list filter, which has no column', () => { + expect( + getFilterBroadcastTarget({ + id: 'f2', + type: 'STATIC_LIST', + name: 'Environment', + options: ['prod'], + isBroadcastEnabled: false, + isVariableEnabled: true, + }), + ).toBeUndefined(); + }); + }); + describe('isFilterVariableEnabled', () => { it('treats a missing flag as disabled', () => { expect(isFilterVariableEnabled({})).toBe(false); @@ -1511,7 +1609,9 @@ describe('filters', () => { }); describe('getDashboardVariableFilters', () => { - const filter = (overrides: Partial): DashboardFilter => ({ + const filter = ( + overrides: Partial, + ): QueryExpressionDashboardFilter => ({ id: 'f1', type: 'QUERY_EXPRESSION', name: 'Service', @@ -1579,7 +1679,9 @@ describe('filters', () => { }); describe('getDashboardVariableDeclarations', () => { - const filter = (overrides: Partial): DashboardFilter => ({ + const filter = ( + overrides: Partial, + ): QueryExpressionDashboardFilter => ({ id: 'f1', type: 'QUERY_EXPRESSION', name: 'Service', @@ -1656,6 +1758,25 @@ describe('filters', () => { ).toEqual([{ name: 'svc', expression: 'ServiceName' }]); }); + // The falsiness of `expression` is what makes `$__filter($name)` report + // that the expression has to be passed explicitly, so it must stay + // undefined rather than becoming an empty string. + it('declares a static-list filter with no expression', () => { + expect( + getDashboardVariableDeclarations([ + { + id: 'f1', + type: 'STATIC_LIST', + name: 'Environment', + options: ['prod', 'staging', 'dev'], + isBroadcastEnabled: false, + isVariableEnabled: true, + variableName: 'env', + }, + ]), + ).toEqual([{ name: 'env', expression: undefined }]); + }); + it('keeps the declarations in filter order', () => { expect( getDashboardVariableDeclarations([ @@ -1677,8 +1798,8 @@ describe('filters', () => { describe('validateVariableName', () => { const variableFilter = ( - overrides: Partial, - ): DashboardFilter => ({ + overrides: Partial, + ): QueryExpressionDashboardFilter => ({ id: 'f1', type: 'QUERY_EXPRESSION', name: 'Service', diff --git a/packages/common-utils/src/__tests__/types.test.ts b/packages/common-utils/src/__tests__/types.test.ts index 7669c3ef13..12bbd38d5f 100644 --- a/packages/common-utils/src/__tests__/types.test.ts +++ b/packages/common-utils/src/__tests__/types.test.ts @@ -10,6 +10,8 @@ import { DashboardSchema, DerivedColumnSchema, MetricFormulaSchema, + PresetDashboard, + PresetDashboardFilterSchema, SavedChartConfigSchema, } from '@/types'; @@ -558,6 +560,200 @@ describe('DashboardFilterSchema variable fields', () => { }); }); +// `DashboardFilterSchema` is a discriminated union, so most per-type rules are +// structural and belong here. Only option *uniqueness* needs a refinement +// (`validateDashboardFilterOptions`), covered in `dashboardValidation.test.ts`. +describe('DashboardFilterSchema static-list variant', () => { + const staticFilter = { + id: 'f1', + type: 'STATIC_LIST' as const, + name: 'Environment', + options: ['prod', 'staging', 'dev'], + isBroadcastEnabled: false, + isVariableEnabled: true, + variableName: 'env', + }; + + it('parses a static-list filter and preserves the authored option order', () => { + const parsed = DashboardFilterSchema.parse(staticFilter); + + expect(parsed).toEqual(staticFilter); + expect(parsed.type === 'STATIC_LIST' && parsed.options).toEqual([ + 'prod', + 'staging', + 'dev', + ]); + }); + + it('rejects an unknown filter type at the discriminator', () => { + const result = DashboardFilterSchema.safeParse({ + ...staticFilter, + type: 'STATIC', + }); + + expect(result.success).toBe(false); + expect(result.error?.issues[0]).toMatchObject({ + code: 'invalid_union_discriminator', + path: ['type'], + }); + }); + + it.each([ + ['a missing option list', { options: undefined }], + ['an empty option list', { options: [] }], + ['an empty option', { options: ['prod', ''] }], + ])('rejects %s', (_label, overrides) => { + expect( + DashboardFilterSchema.safeParse({ ...staticFilter, ...overrides }) + .success, + ).toBe(false); + }); + + // Bounds mirror `VariableFilterValueSchema`, so a selection can always hold + // whatever the option list offers. + it('rejects an option longer than a selection could carry', () => { + expect( + DashboardFilterSchema.safeParse({ + ...staticFilter, + options: ['a'.repeat(10001)], + }).success, + ).toBe(false); + expect( + DashboardFilterSchema.safeParse({ + ...staticFilter, + options: ['a'.repeat(10000)], + }).success, + ).toBe(true); + }); + + it('rejects more options than a selection could carry', () => { + const options = Array.from({ length: 1001 }, (_, i) => `option-${i}`); + + expect( + DashboardFilterSchema.safeParse({ ...staticFilter, options }).success, + ).toBe(false); + expect( + DashboardFilterSchema.safeParse({ + ...staticFilter, + options: options.slice(0, 1000), + }).success, + ).toBe(true); + }); + + // Variable-only by construction: with no expression there is nothing to + // broadcast into a WHERE clause, so the modes are literals rather than + // optional booleans. + it.each([ + ['broadcast enabled', { isBroadcastEnabled: true }], + ['broadcast unset', { isBroadcastEnabled: undefined }], + ['variables disabled', { isVariableEnabled: false }], + ['variables unset', { isVariableEnabled: undefined }], + ])('rejects a static filter with %s', (_label, overrides) => { + const result = DashboardFilterSchema.safeParse({ + ...staticFilter, + ...overrides, + }); + + expect(result.success).toBe(false); + expect(result.error?.issues[0].path).toEqual([Object.keys(overrides)[0]]); + }); + + // Not strict, on purpose: these variants parse unvalidated Mongo documents, + // where a key left behind by an older version must not make template export + // throw. The external API's variants *are* strict and reject it — see + // `dashboards.int.test.ts`. + it('strips a query-expression field rather than rejecting it', () => { + const parsed = DashboardFilterSchema.parse({ + ...staticFilter, + expression: 'ServiceName', + source: 'source-1', + where: "ServiceName = 'api'", + appliesToSourceIds: ['source-1'], + }); + + expect(parsed).toEqual(staticFilter); + }); +}); + +describe('DashboardFilterSchema query-expression variant', () => { + const queryFilter = { + id: 'f1', + type: 'QUERY_EXPRESSION' as const, + name: 'Service', + expression: 'ServiceName', + source: 'source-1', + }; + + it.each(['expression', 'source'] as const)( + 'requires a non-empty %s', + field => { + for (const value of [undefined, '']) { + expect( + DashboardFilterSchema.safeParse({ ...queryFilter, [field]: value }) + .success, + ).toBe(false); + } + }, + ); + + it('strips an option list, which this type has no use for', () => { + expect( + DashboardFilterSchema.parse({ ...queryFilter, options: ['prod'] }), + ).toEqual(queryFilter); + }); + + // Unlike the static variant these stay optional booleans: a filter written + // before either field existed broadcasts, and must keep parsing. + it('accepts a filter carrying neither mode flag', () => { + expect(DashboardFilterSchema.parse(queryFilter)).toEqual(queryFilter); + }); +}); + +// Preset dashboards are broadcast-only, so the preset schema extends the +// query-expression variant rather than the union — a static filter is +// unrepresentable rather than rejected by a refinement. +describe('PresetDashboardFilterSchema', () => { + const presetFilter = { + id: 'f1', + type: 'QUERY_EXPRESSION' as const, + name: 'Service', + expression: 'ServiceName', + source: 'source-1', + presetDashboard: PresetDashboard.Services, + }; + + it('accepts a query-expression preset filter', () => { + expect(PresetDashboardFilterSchema.parse(presetFilter)).toEqual( + presetFilter, + ); + }); + + it('rejects a static-list preset filter', () => { + const result = PresetDashboardFilterSchema.safeParse({ + id: 'f1', + type: 'STATIC_LIST', + name: 'Environment', + options: ['prod'], + isBroadcastEnabled: false, + isVariableEnabled: true, + presetDashboard: PresetDashboard.Services, + }); + + expect(result.success).toBe(false); + expect(result.error?.issues[0].path).toEqual(['type']); + }); + + // The Mongoose model requires both, so the schema has to as well. + it.each(['expression', 'source'] as const)('requires %s', field => { + expect( + PresetDashboardFilterSchema.safeParse({ + ...presetFilter, + [field]: undefined, + }).success, + ).toBe(false); + }); +}); + describe('DashboardFilterValueSchema', () => { it('accepts the legacy condition-carrying entries', () => { for (const entry of [ diff --git a/packages/common-utils/src/__tests__/utils.test.ts b/packages/common-utils/src/__tests__/utils.test.ts index 55d678c63e..ee8b0c8c33 100644 --- a/packages/common-utils/src/__tests__/utils.test.ts +++ b/packages/common-utils/src/__tests__/utils.test.ts @@ -1267,8 +1267,15 @@ describe('utils', () => { source: 'Logs', }, ]); - expect(template.filters?.[2].appliesToSourceIds).toBeUndefined(); - expect(template.filters?.[3].appliesToSourceIds).toBeUndefined(); + // Explicitly absent rather than an empty array, which the import would + // read as "matches no tiles". Both are queried filters, pinned above. + for (const index of [2, 3]) { + const filter = template.filters![index]; + expect(filter.type).toBe('QUERY_EXPRESSION'); + expect( + filter.type === 'QUERY_EXPRESSION' && filter.appliesToSourceIds, + ).toBeUndefined(); + } }); // Variable settings are dashboard-local — unlike `source` and @@ -1324,6 +1331,74 @@ describe('utils', () => { ]); }); + // A `STATIC_LIST` filter references nothing in the workspace at all, so it + // must survive export untouched — in particular `source` must stay absent + // rather than being stamped with the empty string the name lookup returns + // for an unresolvable id, which the re-import would then reject. + it('should export a static-list filter with no source', () => { + const sources: TSource[] = [ + { + id: 'source1', + name: 'Logs', + connection: 'connection1', + kind: SourceKind.Log, + from: { databaseName: 'db1', tableName: 'logs_table' }, + timestampValueExpression: 'Timestamp', + defaultTableSelectExpression: '', + }, + ]; + + const staticFilter = { + id: 'filter-static', + type: 'STATIC_LIST' as const, + name: 'Environment', + options: ['prod', 'staging', 'dev'], + isBroadcastEnabled: false as const, + isVariableEnabled: true as const, + variableName: 'env', + }; + + const dashboard: z.infer = { + id: 'dashboard1', + name: 'Static Filter Dashboard', + tags: [], + tiles: [], + filters: [ + staticFilter, + { + id: 'filter-queried', + type: 'QUERY_EXPRESSION', + name: 'Service', + expression: 'ServiceName', + source: 'source1', + }, + ], + }; + + const template = convertToDashboardTemplate(dashboard, sources); + + expect(template.filters).toEqual([ + staticFilter, + { + id: 'filter-queried', + type: 'QUERY_EXPRESSION', + name: 'Service', + expression: 'ServiceName', + source: 'Logs', + }, + ]); + // No `source` key at all, rather than one holding the empty string the + // name lookup returns for an unresolvable id. + expect('source' in template.filters![0]).toBe(false); + + // And back: the import path carries the filter through verbatim, so a + // round-trip through the template format is lossless. + expect( + convertToDashboardDocument({ ...template, filters: template.filters }) + .filters?.[0], + ).toEqual(staticFilter); + }); + it('should convert a dashboard without filters to a dashboard template', () => { const dashboard: z.infer = { id: 'dashboard1', diff --git a/packages/common-utils/src/__tests__/variables.test.ts b/packages/common-utils/src/__tests__/variables.test.ts index 9b7618a3b1..ee11d4d7bf 100644 --- a/packages/common-utils/src/__tests__/variables.test.ts +++ b/packages/common-utils/src/__tests__/variables.test.ts @@ -1942,3 +1942,83 @@ describe('macros naming an unknown variable', () => { expect(warnings[0]).toContain('no meaning in a Lucene expression'); }); }); + +// A `STATIC_LIST` dashboard filter declares a variable with no `expression`: +// its values are authored, not queried, so there is no column to filter on. +// Every reference form still works except the one-argument `$__filter`, which +// has nothing to render — this is what the reference layer owes that filter +// type, so pin it. +describe('a variable with no expression', () => { + const ENV = variable('env', ['prod']); + const EMPTY_ENV = variable('env', []); + + describe('$__filter($name)', () => { + // The check sits before the empty-selection shortcut on purpose, so the + // author is told regardless of what happens to be selected. + it.each([ + ['with a selection', ENV], + ['with nothing selected', EMPTY_ENV], + ])('throws %s', (_label, env) => { + expect(() => substituteVariables('WHERE $__filter($env)', [env])).toThrow( + "Macro '$__filter($env)' requires the variable's filter expression, which " + + 'is not available — pass it explicitly, e.g. $__filter(, $env).', + ); + }); + + it('is reported as an editor error rather than a warning', () => { + const { errors, warnings } = validateVariableReferencesInTemplate( + 'WHERE $__filter($env)', + [ENV], + ); + + expect(errors).toEqual([ + "Macro '$__filter($env)' requires the variable's filter expression, which " + + 'is not available — pass it explicitly, e.g. $__filter(, $env).', + ]); + expect(warnings).toEqual([]); + }); + }); + + it('expands the two-argument $__filter form', () => { + expect( + substituteVariables('WHERE $__filter(ServiceName, $env)', [ENV]), + ).toBe("WHERE (ServiceName IN ('prod'))"); + }); + + it('expands $__conditionalAll', () => { + expect( + substituteVariables( + 'WHERE $__conditionalAll(ServiceName IN ($env), $env)', + [ENV], + ), + ).toBe("WHERE (ServiceName IN ('prod'))"); + expect( + substituteVariables( + 'WHERE $__conditionalAll(ServiceName IN ($env), $env)', + [EMPTY_ENV], + ), + ).toBe("WHERE (1=1 /** no values selected for variable 'env' */)"); + }); + + it.each(['$env', '${env}', '${env:csv}'])('expands %s', reference => { + expect( + substituteVariables(`WHERE ServiceName IN (${reference})`, [ENV]), + ).toBe( + reference === '${env:csv}' + ? 'WHERE ServiceName IN (prod)' + : "WHERE ServiceName IN ('prod')", + ); + }); + + it('accepts the guarded and macro forms without complaint', () => { + for (const template of [ + '$__filter(ServiceName, $env)', + '$__conditionalAll(ServiceName IN ($env), $env)', + ]) { + expect(validateVariableReferencesInTemplate(template, [ENV])).toEqual({ + errors: [], + warnings: [], + }); + } + }); +}); diff --git a/packages/common-utils/src/core/utils.ts b/packages/common-utils/src/core/utils.ts index b117dd2661..90c57faa30 100644 --- a/packages/common-utils/src/core/utils.ts +++ b/packages/common-utils/src/core/utils.ts @@ -6,6 +6,7 @@ import { z } from 'zod'; export { default as objectHash } from 'object-hash'; +import { isStaticListFilter } from '@/filters'; import { isBuilderSavedChartConfig, isRawSqlSavedChartConfig } from '@/guards'; import { MacroExpansionError, MalformedMacroArgsError } from '@/macroErrors'; import { @@ -700,12 +701,16 @@ export function convertToDashboardTemplate( input: DashboardFilter, sources: TSource[], ): DashboardFilter => { - const filter = DashboardFilterSchema.strip().parse(structuredClone(input)); + const filter = DashboardFilterSchema.parse(structuredClone(input)); + + // A static filter references nothing in the workspace + if (isStaticListFilter(filter)) return filter; + // Extract name from source or default to '' if not found filter.source = - sources.find(source => source.id === input.source)?.name ?? ''; - if (input.appliesToSourceIds?.length) { - const remapped = input.appliesToSourceIds + sources.find(source => source.id === filter.source)?.name ?? ''; + if (filter.appliesToSourceIds?.length) { + const remapped = filter.appliesToSourceIds .map(id => sources.find(source => source.id === id)?.name) .filter((name): name is string => !!name && name.length > 0); filter.appliesToSourceIds = remapped.length > 0 ? remapped : undefined; diff --git a/packages/common-utils/src/dashboardFilterValues.ts b/packages/common-utils/src/dashboardFilterValues.ts index 132aae6c65..02be2797cb 100644 --- a/packages/common-utils/src/dashboardFilterValues.ts +++ b/packages/common-utils/src/dashboardFilterValues.ts @@ -1,8 +1,10 @@ import { FilterState, filtersToQuery, + getFilterExpression, getFilterVariableName, isFilterVariableEnabled, + isStaticListFilter, parseQuery, } from '@/filters'; import { @@ -113,17 +115,19 @@ export function serializeDashboardFilterValues(input: { * has one, otherwise its SQL expression. */ export function filterSelectionKey( - filter: Pick< - DashboardFilter, - 'name' | 'expression' | 'variableName' | 'isVariableEnabled' - >, + filter: DashboardFilter, ): | { kind: 'variable'; name: string } | { kind: 'expression'; expression: string } { + if (isStaticListFilter(filter)) { + return { kind: 'variable', name: getFilterVariableName(filter) ?? '' }; + } + if (isFilterVariableEnabled(filter)) { const name = getFilterVariableName(filter); if (name) return { kind: 'variable', name }; } + return { kind: 'expression', expression: filter.expression }; } @@ -134,10 +138,7 @@ export function filterSelectionKey( * the same filter, including when it holds no values. */ export function resolveFilterSelection( - filter: Pick< - DashboardFilter, - 'name' | 'expression' | 'variableName' | 'isVariableEnabled' - >, + filter: DashboardFilter, parsed: Pick & { byVariable: ReadonlyMap; }, @@ -152,5 +153,7 @@ export function resolveFilterSelection( }; } } - return parsed.byExpression[filter.expression]; + const expression = getFilterExpression(filter); + if (expression == null) return undefined; + return parsed.byExpression[expression]; } diff --git a/packages/common-utils/src/dashboardValidation.ts b/packages/common-utils/src/dashboardValidation.ts index 4ba26dcf2b..8a88bf999e 100644 --- a/packages/common-utils/src/dashboardValidation.ts +++ b/packages/common-utils/src/dashboardValidation.ts @@ -271,6 +271,38 @@ export function validateDashboardFilterFieldGating< }); } +type FilterForOptionsValidation = { + name: string; + options?: string[]; +}; + +/** Validates uniqueness of options within each filter. */ +export function validateDashboardFilterOptionUniqueness< + T extends FilterForOptionsValidation, +>( + filters: T[], + ctx: z.RefinementCtx, + paths?: { filtersPath?: (string | number)[] }, +): void { + const filtersPath = paths?.filtersPath ?? ['filters']; + + filters.forEach((filter, filterIdx) => { + if (!filter.options?.length) return; + const seen = new Set(); + for (const option of filter.options) { + if (seen.has(option)) { + ctx.addIssue({ + code: z.ZodIssueCode.custom, + message: `Filter "${filter.name}" repeats the option "${option}"; options must be unique`, + path: [...filtersPath, filterIdx, 'options'], + }); + return; + } + seen.add(option); + } + }); +} + // Structural because the internal and external chart-config shapes differ // (internal `seriesReturnType: 'ratio'` vs external `asRatio: true`); only // these fields decide formula validity. `select` stays loosely typed — a diff --git a/packages/common-utils/src/filters.ts b/packages/common-utils/src/filters.ts index aff93e099b..1444171395 100644 --- a/packages/common-utils/src/filters.ts +++ b/packages/common-utils/src/filters.ts @@ -787,6 +787,8 @@ export function validateDashboardFilterQueries( ).map(declaration => ({ ...declaration, values: [] })); for (const filter of filters) { + // A static filter has no values query to validate. + if (isStaticListFilter(filter)) continue; const where = filter.where ?? ''; if (!where.trim()) continue; const language = filter.whereLanguage ?? 'sql'; @@ -844,6 +846,42 @@ export function isFilterVariableEnabled(filter: { return filter.isVariableEnabled === true; } +/** The discriminant every dashboard-filter shape carries. */ +export type DashboardFilterKind = DashboardFilter['type']; + +/** Type guard for QUERY_EXPRESSION type filters. */ +export function isQueryExpressionFilter< + T extends { type: DashboardFilterKind }, +>(filter: T): filter is Extract { + return filter.type === 'QUERY_EXPRESSION'; +} + +/** Type guard for STATIC_LIST type filters. */ +export function isStaticListFilter( + filter: T, +): filter is Extract { + return filter.type === 'STATIC_LIST'; +} + +/** The SQL expression associated with the filter, if any. */ +export function getFilterExpression( + filter: DashboardFilter, +): string | undefined { + return isQueryExpressionFilter(filter) ? filter.expression : undefined; +} + +/** What the filter broadcasts, or undefined when it cannot broadcast. */ +export function getFilterBroadcastTarget( + filter: DashboardFilter, +): { expression: string; appliesToSourceIds?: string[] } | undefined { + if (!isFilterBroadcastEnabled(filter) || isStaticListFilter(filter)) + return undefined; + return { + expression: filter.expression, + appliesToSourceIds: filter.appliesToSourceIds, + }; +} + /** * Whether a filter does anything at all with the value it collects — broadcast * it as a condition, expose it as `$variableName`, or both. @@ -899,11 +937,12 @@ export function getDashboardVariableFilters( } /** Minimal projection of fields necessary to extract the variables a dashboard declares. */ -export type FilterForVariableDeclaration = Pick< - DashboardFilter, - 'name' | 'expression' -> & - Partial>; +export type FilterForVariableDeclaration = { + name: string; + expression?: string; + variableName?: string; + isVariableEnabled?: boolean; +}; /** The variables a dashboard declares, in filter order. */ export function getDashboardVariableDeclarations( diff --git a/packages/common-utils/src/types.ts b/packages/common-utils/src/types.ts index 35f418fa41..08edb19a25 100644 --- a/packages/common-utils/src/types.ts +++ b/packages/common-utils/src/types.ts @@ -1892,7 +1892,8 @@ export const DashboardContainerSchema = z.object({ export type DashboardContainer = z.infer; -export const DashboardFilterType = z.enum(['QUERY_EXPRESSION']); +/** Type of dashboard filter, determining how its dropdown values are populated. */ +export const DashboardFilterType = z.enum(['QUERY_EXPRESSION', 'STATIC_LIST']); /** Allowed variable names for dashboard filters. Alphanumeric + underscore, must start with a letter. */ export const DASHBOARD_VARIABLE_NAME_PATTERN = '[a-zA-Z][a-zA-Z0-9_]*'; @@ -1901,31 +1902,10 @@ export const DASHBOARD_VARIABLE_NAME_PATTERN_ANCHORED = new RegExp( ); export const DASHBOARD_VARIABLE_NAME_MAX_LENGTH = 64; -export const DashboardFilterSchema = z.object({ +/** Fields carried by every dashboard filter, whatever its type. */ +const dashboardFilterBaseSchema = z.object({ id: z.string(), - type: DashboardFilterType, name: z.string().min(1), - expression: z.string().min(1), - source: z.string().min(1), - sourceMetricType: z.nativeEnum(MetricsDataType).optional(), - where: z.string().optional(), - whereLanguage: SearchConditionTrimmedLanguageSchema, - // Sources this filter applies to. Undefined / missing means the filter - // applies to all tiles. - appliesToSourceIds: z.array(z.string().min(1)).optional(), - /** - * Whether the selected value is applied as a filter condition on matching - * tiles. Undefined / missing means ENABLED — every filter that predates this - * field broadcasts, and that must not change. Read it through - * `isFilterBroadcastEnabled` rather than defaulting at each call site. - */ - isBroadcastEnabled: z.boolean().optional(), - /** - * Whether the selected value is exposed to tile queries as `$variableName`. - * Undefined / missing means DISABLED. Ignored while the dashboard-variables - * feature is off. - */ - isVariableEnabled: z.boolean().optional(), /** * Token that tiles reference as `$variableName`. Defaults to the filter's display * name with illegal characters replaced by dashes (`deriveVariableName`). @@ -1938,15 +1918,69 @@ export const DashboardFilterSchema = z.object({ .optional(), }); +/** + * A filter whose dropdown values are queried from ClickHouse: `expression` + * names the column, `source` the table, and the selection can be broadcast + * into matching tiles' `WHERE` clauses. + */ +export const QueryExpressionDashboardFilterSchema = + dashboardFilterBaseSchema.extend({ + type: z.literal(DashboardFilterType.enum.QUERY_EXPRESSION), + expression: z.string().min(1), + source: z.string().min(1), + sourceMetricType: z.nativeEnum(MetricsDataType).optional(), + where: z.string().optional(), + whereLanguage: SearchConditionTrimmedLanguageSchema, + // Sources this filter applies to. Undefined / missing means the filter + // applies to all tiles. + appliesToSourceIds: z.array(z.string().min(1)).optional(), + /** + * Whether the selected value is applied as a filter condition on matching + * tiles. Undefined / missing means ENABLED — every filter that predates this + * field broadcasts, and that must not change. Read it through + * `isFilterBroadcastEnabled` rather than defaulting at each call site. + */ + isBroadcastEnabled: z.boolean().optional(), + /** + * Whether the selected value is exposed to tile queries as `$variableName`. + * Undefined / missing means DISABLED. Ignored while the dashboard-variables + * feature is off. + */ + isVariableEnabled: z.boolean().optional(), + }); + +/** A filter whose dropdown offers a hand-authored list. */ +export const StaticListDashboardFilterSchema = dashboardFilterBaseSchema.extend( + { + type: z.literal(DashboardFilterType.enum.STATIC_LIST), + options: z.array(z.string().min(1).max(10000)).min(1).max(1000), + isBroadcastEnabled: z.literal(false), + isVariableEnabled: z.literal(true), + }, +); + +export const DashboardFilterSchema = z.discriminatedUnion('type', [ + QueryExpressionDashboardFilterSchema, + StaticListDashboardFilterSchema, +]); + +export type QueryExpressionDashboardFilter = z.infer< + typeof QueryExpressionDashboardFilterSchema +>; +export type StaticListDashboardFilter = z.infer< + typeof StaticListDashboardFilterSchema +>; export type DashboardFilter = z.infer; export enum PresetDashboard { Services = 'services', } -export const PresetDashboardFilterSchema = DashboardFilterSchema.extend({ - presetDashboard: z.nativeEnum(PresetDashboard), -}); +/** Preset-dashboard filters are broadcast-only, and therefore do not support static value filters. */ +export const PresetDashboardFilterSchema = + QueryExpressionDashboardFilterSchema.extend({ + presetDashboard: z.nativeEnum(PresetDashboard), + }); export type PresetDashboardFilter = z.infer; From b777de7d1ce4d853e52983255f7cb5b408f419ef Mon Sep 17 00:00:00 2001 From: Drew Davis Date: Fri, 28 Aug 2026 09:58:48 -0400 Subject: [PATCH 2/3] fix: Fix tests and lint --- .../__tests__/dashboards/validation.test.ts | 8 +++--- .../src/mcp/tools/dashboards/validation.ts | 9 ++++--- .../__tests__/provisionDashboards.int.test.ts | 1 + packages/api/src/utils/zod.ts | 5 ++++ .../useQueriedDashboardFilterValues.test.tsx | 21 +++++++-------- .../src/__tests__/filters.test.ts | 26 +++++++++---------- .../src/__tests__/variables.test.ts | 12 +++++---- .../common-utils/src/dashboardFilterValues.ts | 2 +- scripts/ci/ratchet-baseline.json | 4 +-- 9 files changed, 49 insertions(+), 39 deletions(-) diff --git a/packages/api/src/mcp/__tests__/dashboards/validation.test.ts b/packages/api/src/mcp/__tests__/dashboards/validation.test.ts index 54eb2f8a0d..82a2be84b6 100644 --- a/packages/api/src/mcp/__tests__/dashboards/validation.test.ts +++ b/packages/api/src/mcp/__tests__/dashboards/validation.test.ts @@ -14,8 +14,8 @@ import { getTileVariableWarnings, } from '@/mcp/tools/dashboards/validation'; import type { - ExternalDashboardFilterWithId, ExternalDashboardTileWithId, + ExternalQueryExpressionFilterWithId, } from '@/utils/zod'; const connectionId = new mongoose.Types.ObjectId().toString(); @@ -448,8 +448,8 @@ describe('getRawSqlTileMacroHints', () => { // ─── Variable-enabled filters ──────────────────────────────────────────────── function makeFilter( - overrides: Partial = {}, -): ExternalDashboardFilterWithId { + overrides: Partial = {}, +): ExternalQueryExpressionFilterWithId { return { id: new mongoose.Types.ObjectId().toString(), type: 'QUERY_EXPRESSION', @@ -803,7 +803,7 @@ describe('getFilterVariableWarnings', () => { }); const endpointFilter = ( - overrides: Partial = {}, + overrides: Partial = {}, ) => makeFilter({ name: 'Endpoint', diff --git a/packages/api/src/mcp/tools/dashboards/validation.ts b/packages/api/src/mcp/tools/dashboards/validation.ts index 6d14bd7f04..7fc6f6bbde 100644 --- a/packages/api/src/mcp/tools/dashboards/validation.ts +++ b/packages/api/src/mcp/tools/dashboards/validation.ts @@ -15,8 +15,8 @@ import { TIME_RANGE_MACROS, } from '@hyperdx/common-utils/dist/macros'; import { - DashboardFilter, DisplayType, + QueryExpressionDashboardFilter, SavedChartConfig, SearchConditionLanguage, } from '@hyperdx/common-utils/dist/types'; @@ -151,9 +151,12 @@ export function getRawSqlTileMacroWarnings( return hints; } -/** Minimal projection needed to check a filter's dropdown-values query. */ +/** + * Minimal projection needed to check a filter's dropdown-values query. Static + * list filters have no dropdown query, so `where` / `whereLanguage` are optional. + */ export type FilterForVariableValidation = FilterForVariableDeclaration & - Partial>; + Partial>; /** * Non-blocking checks on the dashboard variables references in a filter's `where`. diff --git a/packages/api/src/tasks/provisionDashboards/__tests__/provisionDashboards.int.test.ts b/packages/api/src/tasks/provisionDashboards/__tests__/provisionDashboards.int.test.ts index b4fb380da4..3e17594813 100644 --- a/packages/api/src/tasks/provisionDashboards/__tests__/provisionDashboards.int.test.ts +++ b/packages/api/src/tasks/provisionDashboards/__tests__/provisionDashboards.int.test.ts @@ -1,3 +1,4 @@ +/* eslint-disable security/detect-non-literal-fs-filename */ import fs from 'fs'; import mongoose from 'mongoose'; import os from 'os'; diff --git a/packages/api/src/utils/zod.ts b/packages/api/src/utils/zod.ts index 947b889053..18685d6956 100644 --- a/packages/api/src/utils/zod.ts +++ b/packages/api/src/utils/zod.ts @@ -165,6 +165,11 @@ export type ExternalDashboardFilterWithId = z.infer< typeof externalDashboardFilterSchemaWithId >; +export type ExternalQueryExpressionFilterWithId = Extract< + ExternalDashboardFilterWithId, + { type: 'QUERY_EXPRESSION' } +>; + export const externalDashboardFilterSchema = z.discriminatedUnion('type', [ externalQueryExpressionFilterShape.omit({ id: true }).strict(), externalStaticListFilterShape.omit({ id: true }).strict(), diff --git a/packages/app/src/hooks/__tests__/useQueriedDashboardFilterValues.test.tsx b/packages/app/src/hooks/__tests__/useQueriedDashboardFilterValues.test.tsx index beffd09d99..249d775466 100644 --- a/packages/app/src/hooks/__tests__/useQueriedDashboardFilterValues.test.tsx +++ b/packages/app/src/hooks/__tests__/useQueriedDashboardFilterValues.test.tsx @@ -7,7 +7,6 @@ import { import { Metadata } from '@hyperdx/common-utils/dist/core/metadata'; import { FilterSelection } from '@hyperdx/common-utils/dist/dashboardFilterValues'; import { - DashboardFilter, MetricsDataType, QueryExpressionDashboardFilter, SourceKind, @@ -83,7 +82,7 @@ describe('useQueriedDashboardFilterValues', () => { }, ]; - const mockFilters: DashboardFilter[] = [ + const mockFilters: QueryExpressionDashboardFilter[] = [ { id: 'filter1', type: 'QUERY_EXPRESSION', @@ -315,7 +314,7 @@ describe('useQueriedDashboardFilterValues', () => { it('should group multiple filters from the same source', async () => { // Arrange - const sameSourceFilters: DashboardFilter[] = [ + const sameSourceFilters: QueryExpressionDashboardFilter[] = [ { id: 'filter1', type: 'QUERY_EXPRESSION', @@ -360,7 +359,7 @@ describe('useQueriedDashboardFilterValues', () => { it('should not group filters with different where clauses', async () => { // Arrange - const sameSourceFiltersDifferentWhere: DashboardFilter[] = [ + const sameSourceFiltersDifferentWhere: QueryExpressionDashboardFilter[] = [ { id: 'filter1', type: 'QUERY_EXPRESSION', @@ -453,7 +452,7 @@ describe('useQueriedDashboardFilterValues', () => { it('should filter out filters for sources that do not exist', async () => { // Arrange - const filtersWithInvalidSource: DashboardFilter[] = [ + const filtersWithInvalidSource: QueryExpressionDashboardFilter[] = [ ...mockFilters, { id: 'filter3', @@ -614,7 +613,7 @@ describe('useQueriedDashboardFilterValues', () => { isLoading: false, } as any); - const multiFilters: DashboardFilter[] = [ + const multiFilters: QueryExpressionDashboardFilter[] = [ { id: 'filter1', type: 'QUERY_EXPRESSION', @@ -709,7 +708,7 @@ describe('useQueriedDashboardFilterValues', () => { }, ]); - const filtersForSameSource: DashboardFilter[] = [ + const filtersForSameSource: QueryExpressionDashboardFilter[] = [ { id: 'filter1', type: 'QUERY_EXPRESSION', @@ -1177,7 +1176,7 @@ describe('useQueriedDashboardFilterValues', () => { }); it('still uses a single scan for many filters when one is selected', async () => { - const filters: DashboardFilter[] = [ + const filters: QueryExpressionDashboardFilter[] = [ ...envAndStatus, { id: 'filter3', @@ -1211,7 +1210,7 @@ describe('useQueriedDashboardFilterValues', () => { // Both definitions constrain the same column independently, so a third // dropdown's lookup is narrowed by both — not by whichever was written // into the constraint state last. - const filters: DashboardFilter[] = [ + const filters: QueryExpressionDashboardFilter[] = [ { ...envAndStatus[0], id: 'env-a' }, { ...envAndStatus[0], id: 'env-b' }, envAndStatus[1], @@ -1262,7 +1261,7 @@ describe('useQueriedDashboardFilterValues', () => { }); it('does not apply a selection from one source to filters on another source', async () => { - const filters: DashboardFilter[] = [ + const filters: QueryExpressionDashboardFilter[] = [ ...envAndStatus, { id: 'filter3', @@ -1297,7 +1296,7 @@ describe('useQueriedDashboardFilterValues', () => { it('does not apply a selection across metric types of the same source', async () => { // Same source, different metric type → different physical table, so the // constrained column need not exist there. - const filters: DashboardFilter[] = [ + const filters: QueryExpressionDashboardFilter[] = [ { id: 'gauge-filter', type: 'QUERY_EXPRESSION', diff --git a/packages/common-utils/src/__tests__/filters.test.ts b/packages/common-utils/src/__tests__/filters.test.ts index 74ab7ccaea..def391284c 100644 --- a/packages/common-utils/src/__tests__/filters.test.ts +++ b/packages/common-utils/src/__tests__/filters.test.ts @@ -27,6 +27,7 @@ import type { DashboardFilter, Filter, QueryExpressionDashboardFilter, + StaticListDashboardFilter, } from '@/types'; import { DASHBOARD_VARIABLE_NAME_MAX_LENGTH, @@ -1762,19 +1763,18 @@ describe('filters', () => { // that the expression has to be passed explicitly, so it must stay // undefined rather than becoming an empty string. it('declares a static-list filter with no expression', () => { - expect( - getDashboardVariableDeclarations([ - { - id: 'f1', - type: 'STATIC_LIST', - name: 'Environment', - options: ['prod', 'staging', 'dev'], - isBroadcastEnabled: false, - isVariableEnabled: true, - variableName: 'env', - }, - ]), - ).toEqual([{ name: 'env', expression: undefined }]); + const staticFilter: StaticListDashboardFilter = { + id: 'f1', + type: 'STATIC_LIST', + name: 'Environment', + options: ['prod', 'staging', 'dev'], + isBroadcastEnabled: false, + isVariableEnabled: true, + variableName: 'env', + }; + expect(getDashboardVariableDeclarations([staticFilter])).toEqual([ + { name: 'env', expression: undefined }, + ]); }); it('keeps the declarations in filter order', () => { diff --git a/packages/common-utils/src/__tests__/variables.test.ts b/packages/common-utils/src/__tests__/variables.test.ts index ee11d4d7bf..65f27af52a 100644 --- a/packages/common-utils/src/__tests__/variables.test.ts +++ b/packages/common-utils/src/__tests__/variables.test.ts @@ -1959,7 +1959,9 @@ describe('a variable with no expression', () => { ['with a selection', ENV], ['with nothing selected', EMPTY_ENV], ])('throws %s', (_label, env) => { - expect(() => substituteVariables('WHERE $__filter($env)', [env])).toThrow( + expect(() => + substituteVariablesSql('WHERE $__filter($env)', [env]), + ).toThrow( "Macro '$__filter($env)' requires the variable's filter expression, which " + 'is not available — pass it explicitly, e.g. $__filter(, $env).', ); @@ -1981,19 +1983,19 @@ describe('a variable with no expression', () => { it('expands the two-argument $__filter form', () => { expect( - substituteVariables('WHERE $__filter(ServiceName, $env)', [ENV]), + substituteVariablesSql('WHERE $__filter(ServiceName, $env)', [ENV]), ).toBe("WHERE (ServiceName IN ('prod'))"); }); it('expands $__conditionalAll', () => { expect( - substituteVariables( + substituteVariablesSql( 'WHERE $__conditionalAll(ServiceName IN ($env), $env)', [ENV], ), ).toBe("WHERE (ServiceName IN ('prod'))"); expect( - substituteVariables( + substituteVariablesSql( 'WHERE $__conditionalAll(ServiceName IN ($env), $env)', [EMPTY_ENV], ), @@ -2002,7 +2004,7 @@ describe('a variable with no expression', () => { it.each(['$env', '${env}', '${env:csv}'])('expands %s', reference => { expect( - substituteVariables(`WHERE ServiceName IN (${reference})`, [ENV]), + substituteVariablesSql(`WHERE ServiceName IN (${reference})`, [ENV]), ).toBe( reference === '${env:csv}' ? 'WHERE ServiceName IN (prod)' diff --git a/packages/common-utils/src/dashboardFilterValues.ts b/packages/common-utils/src/dashboardFilterValues.ts index 02be2797cb..825166de01 100644 --- a/packages/common-utils/src/dashboardFilterValues.ts +++ b/packages/common-utils/src/dashboardFilterValues.ts @@ -155,5 +155,5 @@ export function resolveFilterSelection( } const expression = getFilterExpression(filter); if (expression == null) return undefined; - return parsed.byExpression[expression]; + return new Map(Object.entries(parsed.byExpression)).get(expression); } diff --git a/scripts/ci/ratchet-baseline.json b/scripts/ci/ratchet-baseline.json index a90d74370a..cdfebb99c5 100644 --- a/scripts/ci/ratchet-baseline.json +++ b/scripts/ci/ratchet-baseline.json @@ -1,8 +1,8 @@ { "api": { - "as-any": 94, + "as-any": 91, "ts-ignore": 0, - "eslint-disable": 31 + "eslint-disable": 32 }, "app": { "as-any": 215, From b3cf03f5cb24b34f761000fbe754dcf47b33ce5e Mon Sep 17 00:00:00 2001 From: Drew Davis Date: Mon, 31 Aug 2026 20:15:49 -0400 Subject: [PATCH 3/3] fix: Prevent empty source/connection mappings during import --- packages/app/src/DBDashboardImportPage.tsx | 118 +++++-- .../__tests__/DBDashboardImportPage.test.tsx | 189 +++++++++- .../dashboard-template-import.spec.ts | 333 +++++++++++++++++- .../e2e/page-objects/DashboardImportPage.ts | 31 ++ packages/app/tests/e2e/utils/constants.ts | 3 + 5 files changed, 646 insertions(+), 28 deletions(-) diff --git a/packages/app/src/DBDashboardImportPage.tsx b/packages/app/src/DBDashboardImportPage.tsx index b4053ad27d..501517c52d 100644 --- a/packages/app/src/DBDashboardImportPage.tsx +++ b/packages/app/src/DBDashboardImportPage.tsx @@ -25,7 +25,10 @@ import { validateSavedFilterValues, validateSavedQuery, } from '@hyperdx/common-utils/dist/filters'; -import { isRawSqlSavedChartConfig } from '@hyperdx/common-utils/dist/guards'; +import { + displayTypeRequiresSource, + isRawSqlSavedChartConfig, +} from '@hyperdx/common-utils/dist/guards'; import { type DashboardFilter, type DashboardTemplate, @@ -386,29 +389,6 @@ function resolveAppliesToSources( }; } -const MappingFormStateSchema = z.object({ - dashboardName: z.string().min(1), - tags: z.array(z.string()), - /** A list of tile source mappings, ordered by input tile index */ - tileSourceMappings: z.array(z.string()), - /** A list of tile connection mappings, ordered by input tile index. Only applicable for RawSQL tiles */ - connectionMappings: z.array(z.string()), - /** A list of filter source mappings, ordered by input filter index */ - filterSourceMappings: z.array(z.string()).optional(), - /** - * Per-filter applies-to source mappings. Each entry is an array of mapped - * source IDs in the same order as the filter's `appliesToSourceIds` names - * from the template. Empty arrays mean "applies to all" after import. - */ - filterAppliesToSourceMappings: z.array(z.array(z.string())).optional(), - /** A list of onClick source mappings, ordered by input tile index */ - onClickSourceMappings: z.array(z.string()).optional(), - /** A list of onClick dashboard mappings, ordered by input tile index */ - onClickDashboardMappings: z.array(z.string()).optional(), -}); - -type MappingFormState = z.infer; - /** * The query-expression half of a template filter, or `undefined` for a static * one. Only a queried filter references a source or an applies-to list, and @@ -417,6 +397,86 @@ type MappingFormState = z.infer; const queriedTemplateFilter = (filter: DashboardFilter | undefined) => filter && isQueryExpressionFilter(filter) ? filter : undefined; +/** + * One mapping select's value. Mantine's Select writes `null` when the user + * clears a selection, so accept that and normalize it to ''. + */ +const MappingValueSchema = z + .string() + .nullish() + .transform(value => value ?? ''); + +/** + * The mapping form's schema for one specific template. It has to be built from + * the template rather than declared once because the form state is positional + * (one entry per input tile/filter). + */ +export function buildMappingFormSchema(input: DashboardTemplate) { + return z + .object({ + dashboardName: z.string().min(1, 'Enter a dashboard name'), + tags: z.array(z.string()), + /** A list of tile source mappings, ordered by input tile index */ + tileSourceMappings: z.array(MappingValueSchema), + /** A list of tile connection mappings, ordered by input tile index. Only applicable for RawSQL tiles */ + connectionMappings: z.array(MappingValueSchema), + /** A list of filter source mappings, ordered by input filter index */ + filterSourceMappings: z.array(MappingValueSchema).optional(), + /** + * Per-filter applies-to source mappings. Each entry is an array of mapped + * source IDs in the same order as the filter's `appliesToSourceIds` names + * from the template. Empty arrays mean "applies to all" after import. + */ + filterAppliesToSourceMappings: z + .array(z.array(MappingValueSchema)) + .optional(), + /** A list of onClick source mappings, ordered by input tile index */ + onClickSourceMappings: z.array(MappingValueSchema).optional(), + /** A list of onClick dashboard mappings, ordered by input tile index */ + onClickDashboardMappings: z.array(MappingValueSchema).optional(), + }) + .superRefine((data, ctx) => { + input.tiles.forEach((tile, idx) => { + const config = tile.config; + // Only require what the table actually offers a row for: a tile gets a + // source row when it declared a source, and markdown carries no + // meaningful source even when the template names one. + if ( + config.source && + displayTypeRequiresSource(config.displayType) && + !data.tileSourceMappings[idx] + ) { + ctx.addIssue({ + code: z.ZodIssueCode.custom, + path: ['tileSourceMappings', idx], + message: 'Select a source for this tile', + }); + } + // A raw SQL tile always gets a connection row, and its connection is + // not optional — leaving it unpicked used to throw on `connection!.id`. + if (isRawSqlSavedChartConfig(config) && !data.connectionMappings[idx]) { + ctx.addIssue({ + code: z.ZodIssueCode.custom, + path: ['connectionMappings', idx], + message: 'Select a connection for this tile', + }); + } + }); + + input.filters?.forEach((filter, idx) => { + if (isStaticListFilter(filter)) return; + if (data.filterSourceMappings?.[idx]) return; + ctx.addIssue({ + code: z.ZodIssueCode.custom, + path: ['filterSourceMappings', idx], + message: 'Select a source for this filter', + }); + }); + }); +} + +type MappingFormState = z.infer>; + export function Mapping({ input }: { input: DashboardTemplate }) { const router = useRouter(); const { data: sources } = useSources(); @@ -425,9 +485,12 @@ export function Mapping({ input }: { input: DashboardTemplate }) { const { data: existingTags } = api.useTags(); const [dashboardId] = useQueryState('dashboardId', parseAsString); + const formSchema = useMemo(() => buildMappingFormSchema(input), [input]); + const { handleSubmit, getFieldState, control, setValue, getValues } = useForm({ - resolver: zodResolver(MappingFormStateSchema), + resolver: zodResolver(formSchema), + mode: 'onChange', defaultValues: { dashboardName: input.name, tags: input.tags ?? [], @@ -440,9 +503,14 @@ export function Mapping({ input }: { input: DashboardTemplate }) { }, }); + // The template whose names have already been auto-mapped + const autoMappedInputRef = useRef(null); + // When the input changes, reset the form useEffect(() => { if (!input || !sources || !connections || !dashboards) return; + if (autoMappedInputRef.current === input) return; + autoMappedInputRef.current = input; const tileSourceMappings = input.tiles.map(tile => { const config = tile.config as SavedChartConfig; diff --git a/packages/app/src/__tests__/DBDashboardImportPage.test.tsx b/packages/app/src/__tests__/DBDashboardImportPage.test.tsx index f3c352131a..eab08c39ff 100644 --- a/packages/app/src/__tests__/DBDashboardImportPage.test.tsx +++ b/packages/app/src/__tests__/DBDashboardImportPage.test.tsx @@ -5,7 +5,7 @@ import { } from '@hyperdx/common-utils/dist/types'; import { act, fireEvent, screen, waitFor } from '@testing-library/react'; -import { Mapping } from '@/DBDashboardImportPage'; +import { buildMappingFormSchema, Mapping } from '@/DBDashboardImportPage'; // Sources/connections the import maps the template's source *names* onto. The // auto-mapping effect in `Mapping` matches by name (case-insensitive), so a @@ -135,6 +135,85 @@ const allTileTypesTemplate: DashboardTemplate = { ], } as DashboardTemplate; +// A template whose only filter names a source that doesn't exist in the +// workspace, so the auto-mapping effect can't resolve it and the mapping select +// stays empty. +const unresolvedFilterSourceTemplate = DashboardTemplateSchema.parse({ + version: '0.1.0', + name: 'Unresolved Filter Source', + tiles: [builderTile('tile-number', 'number', 'Metrics', 0)], + filters: [ + { + id: 'filter-machine', + type: 'QUERY_EXPRESSION', + name: 'Machine', + expression: "ResourceAttributes['service.instance.id']", + source: 'Some Source That Does Not Exist', + whereLanguage: 'sql', + }, + ], +}); + +const markdownTileWithSource = (id: string, source: string, y: number) => ({ + id, + x: 0, + y, + w: 24, + h: 1, + config: { + name: id, + displayType: 'markdown', + source, + where: '', + whereLanguage: 'sql' as const, + select: [], + markdown: '## Overview', + }, +}); + +// A source/connection name no mock resolves, so the auto-mapping effect leaves +// the corresponding select empty. +const MISSING = 'Does Not Exist In This Workspace'; + +const unresolvedTileSourceTemplate = DashboardTemplateSchema.parse({ + version: '0.1.0', + name: 'Unresolved Tile Source', + tiles: [builderTile('tile-number', 'number', MISSING, 0)], +}); + +const unresolvedConnectionTemplate = DashboardTemplateSchema.parse({ + version: '0.1.0', + name: 'Unresolved Connection', + tiles: [ + { + id: 'tile-rawsql', + x: 0, + y: 0, + w: 6, + h: 3, + config: { + name: 'tile-rawsql', + displayType: 'table', + configType: 'sql', + connection: MISSING, + sqlTemplate: 'SELECT 1', + source: 'Traces', + }, + }, + ], +}); + +// A markdown tile renders static content, so its source is not worth blocking +// the import over even when the template names one that no longer exists. +const markdownStaleSourceTemplate = DashboardTemplateSchema.parse({ + version: '0.1.0', + name: 'Markdown With Stale Source', + tiles: [ + markdownTileWithSource('tile-markdown', MISSING, 0), + builderTile('tile-number', 'number', 'Metrics', 1), + ], +}); + beforeEach(() => { mockMutateAsync.mockClear(); }); @@ -200,3 +279,111 @@ describe('Dashboard import - all tile types', () => { expect(payload.filters[0].source).toBe('src-metrics'); }); }); + +describe('Dashboard import - queried filter source is required', () => { + it('blocks the import and flags the filter row when its source is unmapped', async () => { + renderWithMantine(); + + const finish = await screen.findByRole('button', { + name: /finish import/i, + }); + await act(async () => { + fireEvent.click(finish); + }); + + await waitFor(() => + expect( + screen.getByText('Select a source for this filter'), + ).toBeInTheDocument(), + ); + expect(mockMutateAsync).not.toHaveBeenCalled(); + }); +}); + +describe('Dashboard import - tile mappings are required', () => { + it('blocks the import and flags the tile row when its source is unmapped', async () => { + renderWithMantine(); + + await act(async () => { + fireEvent.click( + await screen.findByRole('button', { name: /finish import/i }), + ); + }); + + await waitFor(() => + expect( + screen.getByText('Select a source for this tile'), + ).toBeInTheDocument(), + ); + expect(mockMutateAsync).not.toHaveBeenCalled(); + }); + + it('blocks the import and flags the connection row when a raw SQL connection is unmapped', async () => { + renderWithMantine(); + + await act(async () => { + fireEvent.click( + await screen.findByRole('button', { name: /finish import/i }), + ); + }); + + await waitFor(() => + expect( + screen.getByText('Select a connection for this tile'), + ).toBeInTheDocument(), + ); + // The tile's own source resolved, so only the connection is flagged. + expect( + screen.queryByText('Select a source for this tile'), + ).not.toBeInTheDocument(); + expect(mockMutateAsync).not.toHaveBeenCalled(); + }); + + it('imports a markdown tile whose declared source no longer exists', async () => { + renderWithMantine(); + + await act(async () => { + fireEvent.click( + await screen.findByRole('button', { name: /finish import/i }), + ); + }); + + await waitFor(() => expect(mockMutateAsync).toHaveBeenCalledTimes(1)); + expect( + screen.queryByText('Select a source for this tile'), + ).not.toBeInTheDocument(); + }); +}); + +describe('Dashboard import - cleared mapping selects', () => { + // Mantine's Select writes `null` into the field when a selection is cleared. + it('treats a cleared markdown tile source as unpicked', () => { + const result = buildMappingFormSchema( + markdownStaleSourceTemplate, + ).safeParse({ + dashboardName: 'Markdown With Stale Source', + tags: [], + tileSourceMappings: [null, 'src-metrics'], + connectionMappings: ['', ''], + }); + + expect(result.success).toBe(true); + expect(result.data?.tileSourceMappings[0]).toBe(''); + }); + + it('reports a cleared required tile source in the row instead of a type error', () => { + const result = buildMappingFormSchema( + unresolvedTileSourceTemplate, + ).safeParse({ + dashboardName: 'Unresolved Tile Source', + tags: [], + tileSourceMappings: [null], + connectionMappings: [''], + }); + + expect(result.success).toBe(false); + expect( + result.error?.issues.map(issue => [issue.path.join('.'), issue.message]), + ).toEqual([['tileSourceMappings.0', 'Select a source for this tile']]); + }); +}); diff --git a/packages/app/tests/e2e/features/dashboard-template-import.spec.ts b/packages/app/tests/e2e/features/dashboard-template-import.spec.ts index 5c24dcb553..34e9b5ae85 100644 --- a/packages/app/tests/e2e/features/dashboard-template-import.spec.ts +++ b/packages/app/tests/e2e/features/dashboard-template-import.spec.ts @@ -9,6 +9,7 @@ import { DashboardsListPage } from '../page-objects/DashboardsListPage'; import { getApiUrl, getSources } from '../utils/api-helpers'; import { expect, test } from '../utils/base-test'; import { + DEFAULT_CONNECTION_NAME, DEFAULT_LOGS_SOURCE_NAME, DEFAULT_METRICS_SOURCE_NAME, DEFAULT_TRACES_SOURCE_NAME, @@ -157,11 +158,13 @@ test.describe('Dashboard Template Import', { tag: ['@dashboard'] }, () => { await expect( dashboardImportPage.getImportSuccessNotification(), ).toBeVisible(); - await page.waitForURL(/\/dashboards\/.+/); + // Match the dashboard id, not just any /dashboards/ path: a looser + // pattern also matches /dashboards/import, so a slow client-side + // navigation slips through and fails on the heading instead. + await page.waitForURL(/\/dashboards\/[a-f0-9]{24}/); }); await test.step('Verify the new dashboard has the correct name', async () => { - await expect(page).toHaveURL(/\/dashboards\/.+/); await expect( dashboardPage.getDashboardHeading('.NET Runtime Metrics'), ).toBeVisible(); @@ -608,6 +611,332 @@ test.describe('Dashboard Template Import', { tag: ['@dashboard'] }, () => { }, ); + test( + 'should block the import until a queried filter has a source picked', + { tag: '@full-stack' }, + async ({ page }) => { + const ts = Date.now(); + const dashboardName = `E2E Import Filter Source Required ${ts}`; + const tileName = `Logs Number ${ts}`; + // The tile's source auto-resolves; the filter's does not, so its mapping + // select stays empty and the import must refuse to proceed. + const template = { + version: '0.1.0', + name: dashboardName, + tiles: [ + { + id: 'tile-1', + x: 0, + y: 0, + w: 6, + h: 4, + config: { + name: tileName, + source: DEFAULT_LOGS_SOURCE_NAME, + displayType: 'number', + granularity: 'auto', + select: [{ aggFn: 'count', valueExpression: '' }], + where: '', + whereLanguage: 'sql', + }, + }, + ], + filters: [ + { + id: 'filter-1', + type: 'QUERY_EXPRESSION', + name: 'Machine', + expression: 'ServiceName', + source: `Nonexistent Source ${ts}`, + whereLanguage: 'sql', + }, + ], + }; + + const templatePath = writeTempTemplate(template); + + await test.step('Upload the template and wait for mapping step', async () => { + await dashboardImportPage.gotoImport(); + await dashboardImportPage.uploadTemplateFile(templatePath); + await expect(dashboardImportPage.mappingStepHeading).toBeVisible(); + await expect(dashboardImportPage.dashboardNameInput).toHaveValue( + dashboardName, + ); + }); + + await test.step('Finish import is blocked and the filter row shows the error', async () => { + await dashboardImportPage.finishImportButton.click(); + await expect( + dashboardImportPage.getMappingRowError('Machine (Filter)'), + ).toBeVisible(); + await expect( + dashboardImportPage.getImportSuccessNotification(), + ).toBeHidden(); + await expect(page).toHaveURL(/\/dashboards\/import/); + }); + + await test.step('Picking a source clears the error and lets the import finish', async () => { + await dashboardImportPage.selectMapping( + 'Machine (Filter)', + 'Data Source', + DEFAULT_LOGS_SOURCE_NAME, + ); + await expect( + dashboardImportPage.getMappingRowError('Machine (Filter)'), + ).toBeHidden(); + + await dashboardImportPage.finishImportButton.click(); + await expect( + dashboardImportPage.getImportSuccessNotification(), + ).toBeVisible(); + await page.waitForURL(/\/dashboards\/[a-f0-9]{24}/); + }); + + await test.step('Verify the saved filter carries the picked source id', async () => { + const logSources = await getSources(page, 'log'); + const logsSourceId = logSources.find( + (s: { name: string }) => s.name === DEFAULT_LOGS_SOURCE_NAME, + ).id; + + const dashboardId = dashboardPage.getCurrentDashboardId(); + const dashboard = await fetchDashboardById(page, dashboardId); + expect(dashboard.filters).toHaveLength(1); + expect(dashboard.filters[0].source).toBe(logsSourceId); + }); + }, + ); + + test( + 'should block the import until required tile mappings are picked', + { tag: '@full-stack' }, + async ({ page }) => { + const ts = Date.now(); + const dashboardName = `E2E Import Tile Mappings Required ${ts}`; + const markdownTileName = `Notes ${ts}`; + const builderTileName = `Logs Number ${ts}`; + const sqlTileName = `Raw SQL ${ts}`; + // None of the three names resolve against the workspace, so every select + // starts empty. Only the builder tile's source and the raw SQL tile's + // connection are required — markdown renders static content. + const template = { + version: '0.1.0', + name: dashboardName, + tiles: [ + { + id: 'tile-markdown', + x: 0, + y: 0, + w: 24, + h: 1, + config: { + name: markdownTileName, + source: `Stale Markdown Source ${ts}`, + displayType: 'markdown', + select: [], + where: '', + whereLanguage: 'sql', + markdown: '## Notes', + }, + }, + { + id: 'tile-builder', + x: 0, + y: 1, + w: 6, + h: 4, + config: { + name: builderTileName, + source: `Nonexistent Source ${ts}`, + displayType: 'number', + granularity: 'auto', + select: [{ aggFn: 'count', valueExpression: '' }], + where: '', + whereLanguage: 'sql', + }, + }, + { + id: 'tile-sql', + x: 6, + y: 1, + w: 6, + h: 4, + config: { + name: sqlTileName, + displayType: 'table', + configType: 'sql', + connection: `Nonexistent Connection ${ts}`, + sqlTemplate: 'SELECT 1', + }, + }, + ], + }; + + const templatePath = writeTempTemplate(template); + + await test.step('Upload the template and wait for mapping step', async () => { + await dashboardImportPage.gotoImport(); + await dashboardImportPage.uploadTemplateFile(templatePath); + await expect(dashboardImportPage.mappingStepHeading).toBeVisible(); + await expect(dashboardImportPage.dashboardNameInput).toHaveValue( + dashboardName, + ); + }); + + await test.step('Finish import is blocked and only the required rows show errors', async () => { + await dashboardImportPage.finishImportButton.click(); + await expect( + dashboardImportPage.getMappingRowError(builderTileName), + ).toBeVisible(); + await expect( + dashboardImportPage.getMappingRowError( + sqlTileName, + 'Data Connection', + ), + ).toBeVisible(); + // Markdown needs no source, so its row is not flagged. + await expect( + dashboardImportPage.getMappingRowError(markdownTileName), + ).toBeHidden(); + await expect( + dashboardImportPage.getImportSuccessNotification(), + ).toBeHidden(); + await expect(page).toHaveURL(/\/dashboards\/import/); + }); + + await test.step('Picking the tile source and connection lets the import finish', async () => { + await dashboardImportPage.selectMapping( + builderTileName, + 'Data Source', + DEFAULT_LOGS_SOURCE_NAME, + ); + await dashboardImportPage.selectMapping( + sqlTileName, + 'Data Connection', + DEFAULT_CONNECTION_NAME, + ); + + await dashboardImportPage.finishImportButton.click(); + await expect( + dashboardImportPage.getImportSuccessNotification(), + ).toBeVisible(); + await page.waitForURL(/\/dashboards\/[a-f0-9]{24}/); + }); + + await test.step('Verify the saved tiles carry the picked source and connection ids', async () => { + const logSources = await getSources(page, 'log'); + const logsSourceId = logSources.find( + (s: { name: string }) => s.name === DEFAULT_LOGS_SOURCE_NAME, + ).id; + + const dashboardId = dashboardPage.getCurrentDashboardId(); + const dashboard = await fetchDashboardById(page, dashboardId); + expect(dashboard.tiles).toHaveLength(3); + + const builderTile = dashboard.tiles.find( + (t: any) => t.config.name === builderTileName, + ); + expect(builderTile.config.source).toBe(logsSourceId); + + const sqlTile = dashboard.tiles.find( + (t: any) => t.config.name === sqlTileName, + ); + expect(sqlTile.config.connection).toMatch(/^[a-f0-9]{24}$/); + }); + }, + ); + + test( + 'should flag the row whose source was cleared, not only the rows it propagated to', + { tag: '@full-stack' }, + async ({ page }) => { + const ts = Date.now(); + const dashboardName = `E2E Import Cleared Source ${ts}`; + const firstTileName = `First Tile ${ts}`; + const secondTileName = `Second Tile ${ts}`; + // Both tiles name the same unresolvable source, so picking one cascades + // to the other — and so does clearing it. + const sharedSourceName = `Shared Source ${ts}`; + const template = { + version: '0.1.0', + name: dashboardName, + tiles: [firstTileName, secondTileName].map((name, i) => ({ + id: `tile-${i}`, + x: 0, + y: i * 4, + w: 6, + h: 4, + config: { + name, + source: sharedSourceName, + displayType: 'number', + granularity: 'auto', + select: [{ aggFn: 'count', valueExpression: '' }], + where: '', + whereLanguage: 'sql', + }, + })), + }; + + const templatePath = writeTempTemplate(template); + + await test.step('Upload the template and wait for mapping step', async () => { + await dashboardImportPage.gotoImport(); + await dashboardImportPage.uploadTemplateFile(templatePath); + await expect(dashboardImportPage.mappingStepHeading).toBeVisible(); + await expect(dashboardImportPage.dashboardNameInput).toHaveValue( + dashboardName, + ); + }); + + await test.step('Pick the first tile source and verify it propagates to the second', async () => { + await dashboardImportPage.selectMapping( + firstTileName, + 'Data Source', + DEFAULT_LOGS_SOURCE_NAME, + ); + await expect( + dashboardImportPage + .getMappingRow(secondTileName, 'Data Source') + .getByPlaceholder('Select a source'), + ).toHaveValue(DEFAULT_LOGS_SOURCE_NAME); + }); + + await test.step('Clearing it flags both rows, including the one that was cleared', async () => { + await dashboardImportPage.clearMapping( + firstTileName, + 'Data Source', + DEFAULT_LOGS_SOURCE_NAME, + ); + await expect( + dashboardImportPage.getMappingRowError(firstTileName), + ).toBeVisible(); + await expect( + dashboardImportPage.getMappingRowError(secondTileName), + ).toBeVisible(); + }); + + await test.step('Re-picking the source clears both errors and lets the import finish', async () => { + await dashboardImportPage.selectMapping( + firstTileName, + 'Data Source', + DEFAULT_LOGS_SOURCE_NAME, + ); + await expect( + dashboardImportPage.getMappingRowError(firstTileName), + ).toBeHidden(); + await expect( + dashboardImportPage.getMappingRowError(secondTileName), + ).toBeHidden(); + + await dashboardImportPage.finishImportButton.click(); + await expect( + dashboardImportPage.getImportSuccessNotification(), + ).toBeVisible(); + await page.waitForURL(/\/dashboards\/[a-f0-9]{24}/); + }); + }, + ); + test( 'should round-trip a dashboard through export and re-import', { tag: '@full-stack' }, diff --git a/packages/app/tests/e2e/page-objects/DashboardImportPage.ts b/packages/app/tests/e2e/page-objects/DashboardImportPage.ts index 8536d5e330..5a4d100013 100644 --- a/packages/app/tests/e2e/page-objects/DashboardImportPage.ts +++ b/packages/app/tests/e2e/page-objects/DashboardImportPage.ts @@ -137,6 +137,37 @@ export class DashboardImportPage { } } + /** + * Clear a mapping row's selection by re-clicking the option that is already + * selected (Mantine's Select deselects on a second click). + */ + async clearMapping( + inputName: string, + mappingType: MappingType, + selectedOptionName: string, + ) { + await this.getMappingRow(inputName, mappingType) + .getByPlaceholder('Select a source') + .click(); + await this.page + .getByRole('option', { name: selectedOptionName, exact: true }) + .click(); + } + + /** + * The validation error rendered on a mapping row whose mapping is required + * but left unpicked. The exact wording is asserted in the unit tests; here + * the row it belongs to is what matters. + */ + getMappingRowError( + inputName: string, + mappingType: MappingType = 'Data Source', + ) { + return this.getMappingRow(inputName, mappingType).getByText( + /^Select a (?:source|connection) for this (?:tile|filter)$/, + ); + } + getImportSuccessNotification() { return this.page.getByText('Import Successful!'); } diff --git a/packages/app/tests/e2e/utils/constants.ts b/packages/app/tests/e2e/utils/constants.ts index 704ea9256d..51fc7ec38f 100644 --- a/packages/app/tests/e2e/utils/constants.ts +++ b/packages/app/tests/e2e/utils/constants.ts @@ -4,6 +4,9 @@ export const DEFAULT_METRICS_SOURCE_NAME = 'E2E Metrics'; export const DEFAULT_LOGS_SOURCE_NAME = 'E2E Logs'; export const PROMQL_SOURCE_NAME = 'E2E PromQL'; +// The single ClickHouse connection seeded from `fixtures/e2e-fixtures.json`. +export const DEFAULT_CONNECTION_NAME = 'local'; + // Log source deliberately left without a correlated metric source, so tests can // exercise the "not correlated" paths (the search side panel's infrastructure // tab, and the Kubernetes dashboard's correlation warning).