From dfd7d87ca9bbbd1839bc5a64b34f763b6267499c Mon Sep 17 00:00:00 2001 From: Drew Davis Date: Thu, 20 Aug 2026 15:34:26 -0400 Subject: [PATCH] feat: Accept variable-keyed dashboard filter values --- .changeset/dashboard-filter-value-format.md | 7 + packages/api/openapi.json | 51 ++- .../__tests__/dashboards.int.test.ts | 102 +++++ .../src/routers/external-api/v2/dashboards.ts | 36 +- packages/api/src/utils/zod.ts | 16 +- packages/app/src/DBDashboardPage.tsx | 10 +- packages/app/src/dashboard.ts | 4 +- .../app/src/hooks/useDashboardFilters.tsx | 20 +- .../__tests__/dashboardFilterValues.test.ts | 397 ++++++++++++++++++ .../src/__tests__/filters.test.ts | 86 ++++ .../common-utils/src/__tests__/types.test.ts | 76 ++++ .../common-utils/src/dashboardFilterValues.ts | 156 +++++++ packages/common-utils/src/filters.ts | 30 +- packages/common-utils/src/types.ts | 18 +- 14 files changed, 985 insertions(+), 24 deletions(-) create mode 100644 .changeset/dashboard-filter-value-format.md create mode 100644 packages/common-utils/src/__tests__/dashboardFilterValues.test.ts create mode 100644 packages/common-utils/src/dashboardFilterValues.ts diff --git a/.changeset/dashboard-filter-value-format.md b/.changeset/dashboard-filter-value-format.md new file mode 100644 index 0000000000..acb003b102 --- /dev/null +++ b/.changeset/dashboard-filter-value-format.md @@ -0,0 +1,7 @@ +--- +'@hyperdx/common-utils': patch +'@hyperdx/api': patch +'@hyperdx/app': patch +--- + +feat: Accept variable-keyed dashboard filter values diff --git a/packages/api/openapi.json b/packages/api/openapi.json index 32e7653e7d..7d8ed1c040 100644 --- a/packages/api/openapi.json +++ b/packages/api/openapi.json @@ -834,6 +834,17 @@ "description": "Query language for the where clause." }, "SavedFilterValue": { + "description": "A single saved dashboard filter selection. Either a rendered SQL condition, or a selection addressed by the name of the dashboard variable it belongs to.\n", + "oneOf": [ + { + "$ref": "#/components/schemas/SqlSavedFilterValue" + }, + { + "$ref": "#/components/schemas/VariableSavedFilterValue" + } + ] + }, + "SqlSavedFilterValue": { "type": "object", "required": [ "condition" @@ -845,16 +856,54 @@ "sql" ], "default": "sql", - "description": "Filter type. Currently only \"sql\" is supported.", + "description": "Filter type.", "example": "sql" }, "condition": { "type": "string", + "maxLength": 10000, "description": "SQL filter condition. For example use expressions in the form \"column IN ('value')\".", "example": "ServiceName IN ('hdx-oss-dev-api')" } } }, + "VariableSavedFilterValue": { + "type": "object", + "required": [ + "type", + "name", + "values" + ], + "properties": { + "type": { + "type": "string", + "enum": [ + "variable" + ], + "description": "Filter type.", + "example": "variable" + }, + "name": { + "type": "string", + "minLength": 1, + "maxLength": 1024, + "description": "The variableName of the dashboard variable this selection belongs to. Only allowed for variable-enabled filters.\n", + "example": "service" + }, + "values": { + "type": "array", + "maxItems": 1000, + "description": "Selected values", + "items": { + "type": "string", + "maxLength": 10000 + }, + "example": [ + "hdx-oss-dev-api" + ] + } + } + }, "MetricDataType": { "type": "string", "enum": [ 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 35e5c8a662..a3b5f942cb 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 @@ -7095,6 +7095,108 @@ describe('External API v2 Dashboards - new format', () => { }); }); + describe('savedFilterValues', () => { + const sqlValue = { type: 'sql', condition: "Env IN ('prod')" }; + const variableValue = { + type: 'variable', + name: 'svc', + values: ['accounting', 'frontend'], + }; + + it('should persist and return a variable-keyed value verbatim', async () => { + const response = await authRequest('post', BASE_URL) + .send( + createMockDashboard(traceSource._id.toString(), { + savedFilterValues: [variableValue], + }), + ) + .expect(200); + + expect(response.body.data.savedFilterValues).toEqual([variableValue]); + + const dashboardInDb = await Dashboard.findById( + response.body.data.id, + ).lean(); + expect(dashboardInDb?.savedFilterValues).toEqual([variableValue]); + }); + + it('should accept a variable-keyed value with no values selected', async () => { + const emptySelection = { type: 'variable', name: 'svc', values: [] }; + const response = await authRequest('post', BASE_URL) + .send( + createMockDashboard(traceSource._id.toString(), { + savedFilterValues: [emptySelection], + }), + ) + .expect(200); + + expect(response.body.data.savedFilterValues).toEqual([emptySelection]); + }); + + // The regression this format change exists to prevent: GET returns whatever + // is stored, so a write schema that only accepted the sql shape would make a + // dashboard holding a variable value un-updatable by echoing its own body. + it('should accept a mixed array echoed straight back from GET', async () => { + const created = await authRequest('post', BASE_URL) + .send( + createMockDashboardWithIds(traceSource._id.toString(), { + savedFilterValues: [sqlValue, variableValue], + }), + ) + .expect(200); + + const fetched = await authRequest( + 'get', + `${BASE_URL}/${created.body.data.id}`, + ).expect(200); + expect(fetched.body.data.savedFilterValues).toEqual([ + sqlValue, + variableValue, + ]); + + const echoed = await authRequest( + 'put', + `${BASE_URL}/${created.body.data.id}`, + ) + .send(fetched.body.data) + .expect(200); + expect(echoed.body.data.savedFilterValues).toEqual([ + sqlValue, + variableValue, + ]); + }); + + it('should return 400 for a variable-keyed value missing name', async () => { + await authRequest('post', BASE_URL) + .send( + createMockDashboard(traceSource._id.toString(), { + savedFilterValues: [{ type: 'variable', values: ['a'] }], + }), + ) + .expect(400); + }); + + it('should return 400 for a variable-keyed value missing values', async () => { + await authRequest('post', BASE_URL) + .send( + createMockDashboard(traceSource._id.toString(), { + savedFilterValues: [{ type: 'variable', name: 'svc' }], + }), + ) + .expect(400); + }); + + it('should return 400 for an unknown saved filter value type', async () => { + await authRequest('post', BASE_URL) + .send( + createMockDashboard(traceSource._id.toString(), { + savedFilterValues: [{ type: 'nonsense', name: 'svc', values: [] }], + }), + ) + .expect(400); + }); + }); + describe('DELETE /:id', () => { it('should delete a dashboard', async () => { const dashboard = await createTestDashboard(); diff --git a/packages/api/src/routers/external-api/v2/dashboards.ts b/packages/api/src/routers/external-api/v2/dashboards.ts index a61485c8fc..a6ffcc93b7 100644 --- a/packages/api/src/routers/external-api/v2/dashboards.ts +++ b/packages/api/src/routers/external-api/v2/dashboards.ts @@ -52,6 +52,14 @@ const EXTERNAL_DASHBOARD_PROJECTION = { * enum: [sql, lucene] * description: Query language for the where clause. * SavedFilterValue: + * description: > + * A single saved dashboard filter selection. Either a rendered SQL + * condition, or a selection addressed by the name of the dashboard + * variable it belongs to. + * oneOf: + * - $ref: '#/components/schemas/SqlSavedFilterValue' + * - $ref: '#/components/schemas/VariableSavedFilterValue' + * SqlSavedFilterValue: * type: object * required: [condition] * properties: @@ -59,12 +67,38 @@ const EXTERNAL_DASHBOARD_PROJECTION = { * type: string * enum: [sql] * default: sql - * description: Filter type. Currently only "sql" is supported. + * description: Filter type. * example: "sql" * condition: * type: string + * maxLength: 10000 * description: SQL filter condition. For example use expressions in the form "column IN ('value')". * example: "ServiceName IN ('hdx-oss-dev-api')" + * VariableSavedFilterValue: + * type: object + * required: [type, name, values] + * properties: + * type: + * type: string + * enum: [variable] + * description: Filter type. + * example: "variable" + * name: + * type: string + * minLength: 1 + * maxLength: 1024 + * description: > + * The variableName of the dashboard variable this selection + * belongs to. Only allowed for variable-enabled filters. + * example: "service" + * values: + * type: array + * maxItems: 1000 + * description: Selected values + * items: + * type: string + * maxLength: 10000 + * example: ["hdx-oss-dev-api"] * MetricDataType: * type: string * enum: [sum, gauge, histogram, summary, exponential histogram] diff --git a/packages/api/src/utils/zod.ts b/packages/api/src/utils/zod.ts index de776bf03b..3dd6acff01 100644 --- a/packages/api/src/utils/zod.ts +++ b/packages/api/src/utils/zod.ts @@ -24,6 +24,7 @@ import { validateAlertChannelSelection, validateAlertScheduleOffsetMinutes, validateAlertThresholdMax, + VariableFilterValueSchema, WebhookService, zAlertChannel, zAlertChannels, @@ -155,10 +156,17 @@ export type ExternalDashboardFilter = z.infer< typeof externalDashboardFilterSchema >; -export const externalDashboardSavedFilterValueSchema = z.object({ - type: z.literal('sql').optional().default('sql'), - condition: z.string().max(10000), -}); +/** + * One entry in a dashboard's `savedFilterValues`: either a rendered SQL + * predicate, or a selection addressed by the dashboard variable it belongs to. + */ +export const externalDashboardSavedFilterValueSchema = z.union([ + z.object({ + type: z.literal('sql').optional().default('sql'), + condition: z.string().max(10000), + }), + VariableFilterValueSchema.strict(), +]); // ================================ // Dashboards (new format) diff --git a/packages/app/src/DBDashboardPage.tsx b/packages/app/src/DBDashboardPage.tsx index 2559e0766a..b2c1005fee 100644 --- a/packages/app/src/DBDashboardPage.tsx +++ b/packages/app/src/DBDashboardPage.tsx @@ -179,7 +179,9 @@ import SearchWhereInput, { getStoredLanguage, } from './components/SearchInput/SearchWhereInput'; import { Tags } from './components/Tags'; -import useDashboardFilters from './hooks/useDashboardFilters'; +import useDashboardFilters, { + filterQueriesParser, +} from './hooks/useDashboardFilters'; import { useDashboardRefresh } from './hooks/useDashboardRefresh'; import { useIsVariablesEnabled } from './hooks/useIsVariablesEnabled'; import useTileSelection from './hooks/useTileSelection'; @@ -1829,10 +1831,8 @@ function DBDashboardPage({ presetConfig }: { presetConfig?: Dashboard }) { whereLanguageParser, ); // Get raw filter queries from URL (not processed by hook) - const [rawFilterQueries] = useQueryState( - 'filters', - parseAsJsonEncoded(), - ); + const [rawFilterQueries] = useQueryState('filters', filterQueriesParser); + // Toggle for overlaying alert firing/recovery markers on tile charts. // Ephemeral view state (URL param), not persisted on the dashboard. const [showAlertAnnotations, setShowAlertAnnotations] = useQueryState( diff --git a/packages/app/src/dashboard.ts b/packages/app/src/dashboard.ts index c4f027a7ff..b3ba2484e2 100644 --- a/packages/app/src/dashboard.ts +++ b/packages/app/src/dashboard.ts @@ -3,7 +3,7 @@ import { parseAsJson, useQueryState } from 'nuqs'; import { DashboardContainer, DashboardFilter, - Filter, + DashboardFilterValue, resolveChartPaletteToken, SavedChartConfig, SearchConditionLanguage, @@ -38,7 +38,7 @@ export type Dashboard = { filters?: DashboardFilter[]; savedQuery?: string | null; savedQueryLanguage?: SearchConditionLanguage | null; - savedFilterValues?: Filter[]; + savedFilterValues?: DashboardFilterValue[]; containers?: DashboardContainer[]; createdAt?: string; updatedAt?: string; diff --git a/packages/app/src/hooks/useDashboardFilters.tsx b/packages/app/src/hooks/useDashboardFilters.tsx index d44535eed9..d53dd732f0 100644 --- a/packages/app/src/hooks/useDashboardFilters.tsx +++ b/packages/app/src/hooks/useDashboardFilters.tsx @@ -9,13 +9,23 @@ import { import { ChartVariable, DashboardFilter, + DashboardFilterValue, Filter, } from '@hyperdx/common-utils/dist/types'; import { parseQuery } from '@/searchFilters'; import { parseAsJsonEncoded } from '@/utils/queryParsers'; -const filterQueriesParser = parseAsJsonEncoded(); +export const filterQueriesParser = parseAsJsonEncoded(); + +/** + * Narrow the persisted entries to the ones the expression-keyed reader + * understands. Variable-keyed entries are not read or written here yet. + */ +const expressionKeyedEntries = ( + entries: DashboardFilterValue[] | null | undefined, +): Filter[] => + (entries ?? []).filter((entry): entry is Filter => entry.type !== 'variable'); /** * Whether a filter definition broadcasts its selected value onto a tile @@ -40,7 +50,9 @@ const useDashboardFilters = (filters: DashboardFilter[]) => { const setFilterValue = useCallback( (expression: string, values: string[]) => { setFilterQueries(prev => { - const { filters: filterValues } = parseQuery(prev ?? []); + const { filters: filterValues } = parseQuery( + expressionKeyedEntries(prev), + ); if (values.length === 0) { delete filterValues[expression]; } else { @@ -66,7 +78,9 @@ const useDashboardFilters = (filters: DashboardFilter[]) => { filtersByExpression, variables, } = useMemo(() => { - const { filters: parsedFilters } = parseQuery(filterQueries ?? []); + const { filters: parsedFilters } = parseQuery( + expressionKeyedEntries(filterQueries), + ); const valuesForExistingFilters: FilterState = {}; const knownExpressions = new Set(filters.map(f => f.expression)); const ignored: string[] = []; diff --git a/packages/common-utils/src/__tests__/dashboardFilterValues.test.ts b/packages/common-utils/src/__tests__/dashboardFilterValues.test.ts new file mode 100644 index 0000000000..7346bf941f --- /dev/null +++ b/packages/common-utils/src/__tests__/dashboardFilterValues.test.ts @@ -0,0 +1,397 @@ +import { + filterSelectionKey, + parseDashboardFilterValues, + resolveFilterSelection, + serializeDashboardFilterValues, +} from '@/dashboardFilterValues'; +import { FilterState, filtersToQuery } from '@/filters'; +import type { DashboardFilter, DashboardFilterValue } from '@/types'; + +const filter = (overrides: Partial = {}): DashboardFilter => ({ + id: 'f1', + type: 'QUERY_EXPRESSION', + name: 'Service', + expression: 'ServiceName', + source: 'logs', + ...overrides, +}); + +const included = (...values: (string | boolean)[]) => ({ + included: new Set(values), + excluded: new Set(), +}); + +describe('dashboardFilterValues', () => { + describe('parseDashboardFilterValues', () => { + it('returns empty buckets for a missing / empty array', () => { + for (const input of [undefined, []]) { + const parsed = parseDashboardFilterValues(input); + expect(parsed.byExpression).toEqual({}); + expect(parsed.byVariable.size).toBe(0); + expect(parsed.passthrough).toEqual([]); + } + }); + + it('splits a mixed array into both addressing schemes', () => { + const parsed = parseDashboardFilterValues([ + { type: 'sql', condition: "Env IN ('prod')" }, + { type: 'variable', name: 'svc', values: ['accounting'] }, + ]); + + expect(parsed.byExpression).toEqual({ Env: included('prod') }); + expect(Array.from(parsed.byVariable)).toEqual([['svc', ['accounting']]]); + expect(parsed.passthrough).toEqual([]); + }); + + it('handles a variable-only array', () => { + const parsed = parseDashboardFilterValues([ + { type: 'variable', name: 'svc', values: ['a', 'b'] }, + { type: 'variable', name: 'env', values: [] }, + ]); + + expect(parsed.byExpression).toEqual({}); + expect(Array.from(parsed.byVariable)).toEqual([ + ['svc', ['a', 'b']], + ['env', []], + ]); + }); + + it('handles a sql-only array, merging entries on one expression', () => { + const parsed = parseDashboardFilterValues([ + { type: 'sql', condition: "ServiceName IN ('a')" }, + { type: 'sql', condition: "ServiceName NOT IN ('b')" }, + ]); + + expect(parsed.byExpression).toEqual({ + ServiceName: { included: new Set(['a']), excluded: new Set(['b']) }, + }); + expect(parsed.byVariable.size).toBe(0); + }); + + it('keeps the first of two entries claiming the same variable name', () => { + const parsed = parseDashboardFilterValues([ + { type: 'variable', name: 'svc', values: ['first'] }, + { type: 'variable', name: 'svc', values: ['second'] }, + ]); + + expect(Array.from(parsed.byVariable)).toEqual([['svc', ['first']]]); + }); + + it('routes non-sql entries to passthrough', () => { + const lucene: DashboardFilterValue = { + type: 'lucene', + condition: 'ServiceName:"api"', + }; + const sqlAst: DashboardFilterValue = { + type: 'sql_ast', + operator: '=', + left: 'ServiceName', + right: 'api', + }; + + const parsed = parseDashboardFilterValues([ + lucene, + sqlAst, + { type: 'sql', condition: "Env IN ('prod')" }, + ]); + + expect(parsed.passthrough).toEqual([lucene, sqlAst]); + expect(parsed.byExpression).toEqual({ Env: included('prod') }); + }); + + it('drops a sql entry it can extract nothing from, rather than carrying it', () => { + // Carrying these would grow the URL without bound: a rebuild re-emits + // every declared filter through `filtersToQuery`, so a filter whose + // expression cannot survive that round trip would append a fresh dead + // copy on every write. `parseQuery` extracts nothing from the first + // (top-level comparison operator, not an IN clause) or the second (empty). + const parsed = parseDashboardFilterValues([ + { type: 'sql', condition: "ServiceName = 'api'" }, + { type: 'sql', condition: ' ' }, + { type: 'sql', condition: "Env IN ('prod')" }, + ]); + + expect(parsed.passthrough).toEqual([]); + expect(parsed.byExpression).toEqual({ Env: included('prod') }); + }); + }); + + describe('serializeDashboardFilterValues', () => { + it('returns an empty array for empty input', () => { + expect(serializeDashboardFilterValues({})).toEqual([]); + }); + + it('orders legacy entries, then variable entries, then passthrough', () => { + const passthrough: DashboardFilterValue = { + type: 'lucene', + condition: 'Level:"error"', + }; + + expect( + serializeDashboardFilterValues({ + byExpression: { Env: included('prod'), Region: included('us') }, + byVariable: new Map([ + ['svc', ['accounting']], + ['team', ['platform']], + ]), + passthrough: [passthrough], + }), + ).toEqual([ + { type: 'sql', condition: "Env IN ('prod')" }, + { type: 'sql', condition: "Region IN ('us')" }, + { type: 'variable', name: 'svc', values: ['accounting'] }, + { type: 'variable', name: 'team', values: ['platform'] }, + passthrough, + ]); + }); + + it('omits empty selections from both schemes', () => { + expect( + serializeDashboardFilterValues({ + byExpression: { + Env: included(), + Region: included('us'), + }, + byVariable: new Map([ + ['svc', []], + ['team', ['platform']], + ]), + }), + ).toEqual([ + { type: 'sql', condition: "Region IN ('us')" }, + { type: 'variable', name: 'team', values: ['platform'] }, + ]); + }); + + it('emits legacy entries byte-identically to filtersToQuery', () => { + const byExpression: FilterState = { + ServiceName: { + included: new Set(["it's", 'back\\slash', 'a,b']), + excluded: new Set(['nope']), + }, + Latency: { + included: new Set(), + excluded: new Set(), + range: { min: 1, max: 2 }, + }, + }; + + expect(serializeDashboardFilterValues({ byExpression })).toEqual( + filtersToQuery(byExpression, { stringifyKeys: false }), + ); + }); + }); + + describe('round-tripping values', () => { + // Values chosen to exercise SQL escaping, the comma splitter, the + // date-expression unwrap, and boolean coercion on the legacy path. + const AWKWARD_VALUES = [ + "it's", + 'back\\slash', + "escaped\\'quote", + 'a,b', + 'close)paren', + 'open(paren', + 'toString(', + "parseDateTime64BestEffort('x', 9)", + 'true', + 'TRUE', + '', + ' ', + 'ünïcødé 🎉', + 'x'.repeat(5000), + ]; + + it.each(AWKWARD_VALUES.map(v => [JSON.stringify(v), v]))( + 'preserves %s exactly on the variable path', + (_label, value) => { + const entries = serializeDashboardFilterValues({ + byVariable: new Map([['svc', [value]]]), + }); + expect(entries).toEqual([ + { type: 'variable', name: 'svc', values: [value] }, + ]); + + // The URL carries the JSON encoding of the array, so assert the value + // survives that too rather than only the in-memory hop. + const parsed = parseDashboardFilterValues( + JSON.parse(JSON.stringify(entries)), + ); + expect(parsed.byVariable.get('svc')).toEqual([value]); + expect(parsed.byExpression).toEqual({}); + expect(parsed.passthrough).toEqual([]); + }, + ); + + it.each(AWKWARD_VALUES.map(v => [JSON.stringify(v), v]))( + 'preserves %s on the legacy path too, when it wrote the SQL itself', + (_label, value) => { + // Every value the app emits is quoted and escaped by `filtersToQuery`, + // and the parser reverses that exactly — including for `true` and for a + // value shaped like a date wrapper. The legacy path's known lossiness is + // confined to SQL text written by something *else* (see below), which + // matters because that is the only kind the new format cannot express. + const entries = serializeDashboardFilterValues({ + byExpression: { ServiceName: included(value) }, + }); + const parsed = parseDashboardFilterValues( + JSON.parse(JSON.stringify(entries)), + ); + expect(parsed.byExpression.ServiceName?.included).toEqual( + new Set([value]), + ); + }, + ); + + // The differences between the two formats, asserted so the variable + // format's divergence reads as intentional rather than accidental. Both + // require a hand-written (or link-builder-written) legacy entry, since the + // app's own writes are always quoted. + it('documents an unquoted true in a legacy entry becoming a boolean', () => { + const parsed = parseDashboardFilterValues([ + { type: 'sql', condition: 'ServiceName IN (true)' }, + ]); + expect(parsed.byExpression.ServiceName.included).toEqual(new Set([true])); + + // Re-emitted unquoted, so `toString(ServiceName) IN (true)` is a type + // error against a String column. The variable format has no coercion step + // at all, so the same selection stays the string 'true'. + expect(serializeDashboardFilterValues(parsed)).toEqual([ + { type: 'sql', condition: 'ServiceName IN (true)' }, + ]); + expect( + serializeDashboardFilterValues({ + byVariable: new Map([['svc', ['true']]]), + }), + ).toEqual([{ type: 'variable', name: 'svc', values: ['true'] }]); + }); + + it('documents a legacy date-column entry losing its value wrapper', () => { + // The unwrap exists so a date column's condition parses back to the plain + // literal; a caller that wrote the wrapper itself does not get it back. + const parsed = parseDashboardFilterValues([ + { + type: 'sql', + condition: "Timestamp IN (parseDateTime64BestEffort('x', 9))", + }, + ]); + expect(parsed.byExpression.Timestamp.included).toEqual(new Set(['x'])); + expect(serializeDashboardFilterValues(parsed)).toEqual([ + { type: 'sql', condition: "Timestamp IN ('x')" }, + ]); + }); + }); + + describe('filterSelectionKey', () => { + it('keys a variable-enabled filter by its variable name', () => { + expect( + filterSelectionKey( + filter({ isVariableEnabled: true, variableName: 'svc' }), + ), + ).toEqual({ kind: 'variable', name: 'svc' }); + }); + + it('keys a filter with no variable by its expression', () => { + expect(filterSelectionKey(filter())).toEqual({ + kind: 'expression', + expression: 'ServiceName', + }); + expect(filterSelectionKey(filter({ isVariableEnabled: false }))).toEqual({ + kind: 'expression', + expression: 'ServiceName', + }); + }); + + it('falls back to the name derived from the display name', () => { + expect( + filterSelectionKey( + filter({ + name: 'Total Requests', + isVariableEnabled: true, + variableName: ' ', + }), + ), + ).toEqual({ kind: 'variable', name: 'Total_Requests' }); + }); + + it('keys by expression when nothing usable can be derived', () => { + expect( + filterSelectionKey(filter({ name: '环境', isVariableEnabled: true })), + ).toEqual({ kind: 'expression', expression: 'ServiceName' }); + }); + }); + + describe('resolveFilterSelection', () => { + const variableFilter = filter({ + isVariableEnabled: true, + variableName: 'svc', + }); + + it('prefers a variable entry over a legacy one for the same filter', () => { + const parsed = parseDashboardFilterValues([ + { type: 'sql', condition: "ServiceName IN ('legacy')" }, + { type: 'variable', name: 'svc', values: ['new'] }, + ]); + + expect(resolveFilterSelection(variableFilter, parsed)).toEqual( + included('new'), + ); + }); + + it('treats an explicitly empty variable entry as a selection', () => { + const parsed = parseDashboardFilterValues([ + { type: 'sql', condition: "ServiceName IN ('legacy')" }, + { type: 'variable', name: 'svc', values: [] }, + ]); + + expect(resolveFilterSelection(variableFilter, parsed)).toEqual( + included(), + ); + }); + + it('falls back to the expression entry for back-compat', () => { + const parsed = parseDashboardFilterValues([ + { type: 'sql', condition: "ServiceName IN ('legacy')" }, + ]); + + expect(resolveFilterSelection(variableFilter, parsed)).toEqual( + included('legacy'), + ); + }); + + it('reads a non-variable filter by expression only', () => { + const parsed = parseDashboardFilterValues([ + { type: 'variable', name: 'svc', values: ['new'] }, + { type: 'sql', condition: "ServiceName IN ('legacy')" }, + ]); + + expect(resolveFilterSelection(filter(), parsed)).toEqual( + included('legacy'), + ); + }); + + it('returns undefined when nothing addresses the filter', () => { + const parsed = parseDashboardFilterValues([ + { type: 'variable', name: 'other', values: ['x'] }, + { type: 'sql', condition: "Env IN ('prod')" }, + ]); + + expect(resolveFilterSelection(variableFilter, parsed)).toBeUndefined(); + expect(resolveFilterSelection(filter(), parsed)).toBeUndefined(); + }); + + it('resolves two filters sharing an expression independently', () => { + const parsed = parseDashboardFilterValues([ + { type: 'sql', condition: "ServiceName IN ('plain')" }, + { type: 'variable', name: 'svc', values: ['variable'] }, + ]); + + expect(resolveFilterSelection(variableFilter, parsed)).toEqual( + included('variable'), + ); + expect(resolveFilterSelection(filter({ id: 'f2' }), parsed)).toEqual( + included('plain'), + ); + }); + }); +}); diff --git a/packages/common-utils/src/__tests__/filters.test.ts b/packages/common-utils/src/__tests__/filters.test.ts index e09244ff35..4482afcc0a 100644 --- a/packages/common-utils/src/__tests__/filters.test.ts +++ b/packages/common-utils/src/__tests__/filters.test.ts @@ -4,6 +4,7 @@ import { filterStateToPredicate, filtersToQuery, getDashboardVariableDeclarations, + getDashboardVariableFilters, getFilterVariableName, getPendingFilterValuesVariables, hasFilterEffect, @@ -586,6 +587,23 @@ describe('filters', () => { { index: 3, language: 'sql', condition: 'broken = = =' }, ]); }); + + it('ignores variable-keyed values, which carry no condition to validate', () => { + expect( + validateSavedFilterValues([ + { type: 'variable', name: 'svc', values: ['a', 'b'] }, + ]), + ).toEqual([]); + }); + + it('still reports the index of an invalid value after a variable one', () => { + expect( + validateSavedFilterValues([ + { type: 'variable', name: 'svc', values: ['a'] }, + { type: 'sql', condition: 'broken = = =' }, + ]), + ).toEqual([{ index: 1, language: 'sql', condition: 'broken = = =' }]); + }); }); describe('validateSavedQuery', () => { @@ -1492,6 +1510,74 @@ describe('filters', () => { }); }); + describe('getDashboardVariableFilters', () => { + const filter = (overrides: Partial): DashboardFilter => ({ + id: 'f1', + type: 'QUERY_EXPRESSION', + name: 'Service', + expression: 'ServiceName', + source: 'logs', + ...overrides, + }); + + it('returns nothing for a dashboard with no filters', () => { + expect(getDashboardVariableFilters(undefined)).toEqual([]); + expect(getDashboardVariableFilters([])).toEqual([]); + }); + + it('skips filters that do not expose a variable', () => { + expect( + getDashboardVariableFilters([ + filter({ id: 'broadcast-only', isVariableEnabled: false }), + filter({ id: 'unset', name: 'Env', expression: 'Env' }), + ]), + ).toEqual([]); + }); + + it('skips a filter whose display name derives nothing usable', () => { + expect( + getDashboardVariableFilters([ + filter({ name: '环境', isVariableEnabled: true }), + ]), + ).toEqual([]); + }); + + it('pairs each variable-enabled filter with the name it answers to', () => { + const explicit = filter({ isVariableEnabled: true, variableName: 'svc' }); + const derived = filter({ + id: 'f2', + name: 'Total Requests', + expression: 'Env', + isVariableEnabled: true, + }); + + expect(getDashboardVariableFilters([explicit, derived])).toEqual([ + { filter: explicit, name: 'svc' }, + { filter: derived, name: 'Total_Requests' }, + ]); + }); + + it('keeps the first of two filters claiming the same name', () => { + const first = filter({ + id: 'a', + isVariableEnabled: true, + variableName: 'svc', + }); + + expect( + getDashboardVariableFilters([ + first, + filter({ + id: 'b', + expression: 'Other', + isVariableEnabled: true, + variableName: 'svc', + }), + ]), + ).toEqual([{ filter: first, name: 'svc' }]); + }); + }); + describe('getDashboardVariableDeclarations', () => { const filter = (overrides: Partial): DashboardFilter => ({ id: 'f1', diff --git a/packages/common-utils/src/__tests__/types.test.ts b/packages/common-utils/src/__tests__/types.test.ts index 12e4289129..7669c3ef13 100644 --- a/packages/common-utils/src/__tests__/types.test.ts +++ b/packages/common-utils/src/__tests__/types.test.ts @@ -6,6 +6,8 @@ import { ColorConditionSchema, DASHBOARD_VARIABLE_NAME_MAX_LENGTH, DashboardFilterSchema, + DashboardFilterValueSchema, + DashboardSchema, DerivedColumnSchema, MetricFormulaSchema, SavedChartConfigSchema, @@ -556,6 +558,80 @@ describe('DashboardFilterSchema variable fields', () => { }); }); +describe('DashboardFilterValueSchema', () => { + it('accepts the legacy condition-carrying entries', () => { + for (const entry of [ + { type: 'sql', condition: "ServiceName IN ('api')" }, + { type: 'lucene', condition: 'ServiceName:"api"' }, + { type: 'sql_ast', operator: '=', left: 'ServiceName', right: 'api' }, + ]) { + expect(DashboardFilterValueSchema.safeParse(entry).success).toBe(true); + } + }); + + it('accepts a variable-keyed entry, including with no values selected', () => { + expect( + DashboardFilterValueSchema.safeParse({ + type: 'variable', + name: 'svc', + values: ['accounting'], + }).success, + ).toBe(true); + expect( + DashboardFilterValueSchema.safeParse({ + type: 'variable', + name: 'svc', + values: [], + }).success, + ).toBe(true); + }); + + it('requires a non-empty name and a values array', () => { + for (const entry of [ + { type: 'variable', values: ['a'] }, + { type: 'variable', name: '', values: ['a'] }, + { type: 'variable', name: 'svc' }, + { type: 'variable', name: 'svc', values: 'a' }, + { type: 'variable', name: 'svc', values: [1] }, + { type: 'variable', name: 'a'.repeat(1025), values: ['a'] }, + ]) { + expect(DashboardFilterValueSchema.safeParse(entry).success).toBe(false); + } + }); + + it.each([ + ['one the variable-name grammar would reject', 'not a token'], + ['far longer than the variable-name limit', 'a'.repeat(65)], + ['at the entry limit', 'a'.repeat(1024)], + ])('accepts a name that is %s', (_label, name) => { + // Both the grammar and the 64-character limit are enforced on the filter + // *definition*. Enforcing either here would make a selection written by a + // looser client unparseable, and an unmatched selection is meant to survive + // as an orphan rather than be dropped. The entry's own cap only bounds input. + expect( + DashboardFilterValueSchema.safeParse({ + type: 'variable', + name, + values: ['a'], + }).success, + ).toBe(true); + }); + + it('is accepted by DashboardSchema.savedFilterValues alongside sql entries', () => { + const result = DashboardSchema.safeParse({ + id: 'd1', + name: 'Dashboard', + tiles: [], + tags: [], + savedFilterValues: [ + { type: 'sql', condition: "Env IN ('prod')" }, + { type: 'variable', name: 'svc', values: ['accounting'] }, + ], + }); + expect(result.success).toBe(true); + }); +}); + describe('MetricFormulaSchema', () => { it('parses an expression-only formula', () => { const result = MetricFormulaSchema.safeParse({ diff --git a/packages/common-utils/src/dashboardFilterValues.ts b/packages/common-utils/src/dashboardFilterValues.ts new file mode 100644 index 0000000000..132aae6c65 --- /dev/null +++ b/packages/common-utils/src/dashboardFilterValues.ts @@ -0,0 +1,156 @@ +import { + FilterState, + filtersToQuery, + getFilterVariableName, + isFilterVariableEnabled, + parseQuery, +} from '@/filters'; +import { + DashboardFilter, + DashboardFilterValue, + Filter, + VariableFilterValue, +} from '@/types'; + +/** + * A dashboards filter/variable state is persisted as an array of entries in one of two + * addressing schemes: + * + * - legacy, keyed by SQL expression: `{ type: 'sql', condition: "E IN ('a')" }` + * - variable-keyed: `{ type: 'variable', name: 'svc', values: ['a'] }` + * + * Expression keying cannot represent two filters that share an `expression`, + * and has nowhere to put a selection for a filter with no expression at all. + * Variable keying fixes both, but only exists for variable-enabled filters, so + * both schemes are accepted when reading state, and state is written in + * variable-keyed format when possible, falling back to expression-keyed + * format for filters that are not variable-enabled. + */ + +/** One filter's selection: what a `FilterState` holds per key. */ +export type FilterSelection = FilterState[string]; + +export type ParsedDashboardFilterValues = { + /** Selections addressed by SQL expression, parsed via `parseQuery`. */ + byExpression: FilterState; + /** Selections addressed by dashboard variable name. */ + byVariable: Map; + /** + * Non-`sql` entries (`lucene`, `sql_ast`), carried verbatim so a write doesn't + * destroy them. + */ + passthrough: DashboardFilterValue[]; +}; + +const isVariableEntry = ( + entry: DashboardFilterValue, +): entry is VariableFilterValue => entry.type === 'variable'; + +/** Split a raw entry array into the two addressing schemes + passthrough. */ +export function parseDashboardFilterValues( + entries: DashboardFilterValue[] | undefined, +): ParsedDashboardFilterValues { + const byVariable = new Map(); + const passthrough: DashboardFilterValue[] = []; + const sqlEntries: Filter[] = []; + + for (const entry of entries ?? []) { + if (isVariableEntry(entry)) { + // There shouldn't be duplicate names, but if there are the first wins. + if (!byVariable.has(entry.name)) { + byVariable.set(entry.name, entry.values); + } + continue; + } + + if (entry.type !== 'sql') { + passthrough.push(entry); + continue; + } + + // `parseQuery` is lenient: it extracts the clauses it recognizes and drops + // the rest, so an entry it understands nothing of contributes no key here. + sqlEntries.push(entry); + } + + return { + // Parsed as one batch rather than merged per entry, so multiple entries on + // one expression combine exactly as they do today. + byExpression: parseQuery(sqlEntries).filters, + byVariable, + passthrough, + }; +} + +/** + * Inverse of `parseDashboardFilterValues`. + * + * Ordering is legacy entries, then variable entries, then passthrough — output + * is deterministic to avoid unnecessary churn in URL query params. + */ +export function serializeDashboardFilterValues(input: { + byExpression?: FilterState; + byVariable?: ReadonlyMap; + passthrough?: DashboardFilterValue[]; +}): DashboardFilterValue[] { + const entries: DashboardFilterValue[] = [ + // Re-use existing filter query rendering for legacy/expression-keyed format + ...filtersToQuery(input.byExpression ?? {}, { stringifyKeys: false }), + ]; + + for (const [name, values] of input.byVariable ?? []) { + if (values.length === 0) continue; // Empty selections are omitted. + entries.push({ type: 'variable', name, values }); + } + + entries.push(...(input.passthrough ?? [])); + + return entries; +} + +/** + * The identity a filter's selection is stored under: its variable name when it + * has one, otherwise its SQL expression. + */ +export function filterSelectionKey( + filter: Pick< + DashboardFilter, + 'name' | 'expression' | 'variableName' | 'isVariableEnabled' + >, +): + | { kind: 'variable'; name: string } + | { kind: 'expression'; expression: string } { + if (isFilterVariableEnabled(filter)) { + const name = getFilterVariableName(filter); + if (name) return { kind: 'variable', name }; + } + return { kind: 'expression', expression: filter.expression }; +} + +/** + * Resolve one filter's selection out of a parsed entry array. + * + * A variable-keyed entry wins over an expression-keyed entry if they address + * the same filter, including when it holds no values. + */ +export function resolveFilterSelection( + filter: Pick< + DashboardFilter, + 'name' | 'expression' | 'variableName' | 'isVariableEnabled' + >, + parsed: Pick & { + byVariable: ReadonlyMap; + }, +): FilterSelection | undefined { + const key = filterSelectionKey(filter); + if (key.kind === 'variable') { + const values = parsed.byVariable.get(key.name); + if (values) { + return { + included: new Set(values), + excluded: new Set(), + }; + } + } + return parsed.byExpression[filter.expression]; +} diff --git a/packages/common-utils/src/filters.ts b/packages/common-utils/src/filters.ts index 7b1822d986..052a0caa1d 100644 --- a/packages/common-utils/src/filters.ts +++ b/packages/common-utils/src/filters.ts @@ -7,6 +7,7 @@ import { DASHBOARD_VARIABLE_NAME_MAX_LENGTH, DASHBOARD_VARIABLE_NAME_PATTERN_ANCHORED, DashboardFilter, + DashboardFilterValue, Filter, } from '@/types'; import { @@ -700,9 +701,11 @@ export function isValidFilterCondition( * * Empty / whitespace-only conditions are treated as valid (they're no-ops at * query time, not errors), as are structurally-validated `sql_ast` filters. + * Variable-keyed entries carry no condition text at all, so there is nothing to + * validate and they are skipped by the same `type` guard. */ export function validateSavedFilterValues( - filters: Filter[], + filters: DashboardFilterValue[], ): SavedFilterValueIssue[] { const issues: SavedFilterValueIssue[] = []; filters.forEach((filter, index) => { @@ -874,11 +877,14 @@ export type DashboardVariableDeclaration = Pick< 'name' | 'expression' >; -/** The variables a dashboard declares, in filter order. */ -export function getDashboardVariableDeclarations( +/** + * The variable-enabled filters a dashboard declares, paired with the name each + * one answers to, in filter order. + */ +export function getDashboardVariableFilters( filters: DashboardFilter[] | undefined, -): DashboardVariableDeclaration[] { - const declarations: DashboardVariableDeclaration[] = []; +): { filter: DashboardFilter; name: string }[] { + const results: { filter: DashboardFilter; name: string }[] = []; const takenNames = new Set(); for (const filter of filters ?? []) { @@ -889,10 +895,20 @@ export function getDashboardVariableDeclarations( if (!name || takenNames.has(name)) continue; takenNames.add(name); - declarations.push({ name, expression: filter.expression }); + results.push({ filter, name }); } - return declarations; + return results; +} + +/** The variables a dashboard declares, in filter order. */ +export function getDashboardVariableDeclarations( + filters: DashboardFilter[] | undefined, +): DashboardVariableDeclaration[] { + return getDashboardVariableFilters(filters).map(({ filter, name }) => ({ + name, + expression: filter.expression, + })); } export type ResolvedFilterValuesQuery = { diff --git a/packages/common-utils/src/types.ts b/packages/common-utils/src/types.ts index 5a14c12825..ad3100a085 100644 --- a/packages/common-utils/src/types.ts +++ b/packages/common-utils/src/types.ts @@ -1046,6 +1046,22 @@ export const FilterSchema = z.union([ export type Filter = z.infer; +export const VariableFilterValueSchema = z.object({ + type: z.literal('variable'), + name: z.string().min(1).max(1024), + values: z.array(z.string().max(10000)).max(1000), +}); + +export type VariableFilterValue = z.infer; + +/** One entry in a dashboard's `filters=` param / `savedFilterValues`. */ +export const DashboardFilterValueSchema = z.union([ + FilterSchema, + VariableFilterValueSchema, +]); + +export type DashboardFilterValue = z.infer; + // -------------------------- // TAGS // -------------------------- @@ -1893,7 +1909,7 @@ export const DashboardSchema = z.object({ filters: z.array(DashboardFilterSchema).optional(), savedQuery: z.string().nullable().optional(), savedQueryLanguage: SearchConditionLanguageSchema.nullable().optional(), - savedFilterValues: z.array(FilterSchema).optional(), + savedFilterValues: z.array(DashboardFilterValueSchema).optional(), containers: z .array(DashboardContainerSchema) .max(DASHBOARD_MAX_CONTAINERS)