diff --git a/.changeset/dashboard-variable-builder-charts.md b/.changeset/dashboard-variable-builder-charts.md new file mode 100644 index 0000000000..8dab799fe7 --- /dev/null +++ b/.changeset/dashboard-variable-builder-charts.md @@ -0,0 +1,6 @@ +--- +'@hyperdx/app': patch +'@hyperdx/common-utils': patch +--- + +feat: Substitute dashboard variables in chart builder tiles diff --git a/packages/app/src/DBDashboardPage.tsx b/packages/app/src/DBDashboardPage.tsx index 02ffe8a631..a2ef34836f 100644 --- a/packages/app/src/DBDashboardPage.tsx +++ b/packages/app/src/DBDashboardPage.tsx @@ -144,6 +144,7 @@ import FullscreenPanelModal from '@/components/FullscreenPanelModal'; import ResourceTerraformPopover from '@/components/Iac/ResourceTerraformPopover'; import { PageHeader } from '@/components/PageHeader'; import { PageLayout } from '@/components/PageLayout'; +import { SqlVariablesProvider } from '@/components/SQLEditor/variableCompletions'; import { TimePicker } from '@/components/TimePicker'; import { parseTimeRangeInput } from '@/components/TimePicker/utils'; import { @@ -551,7 +552,7 @@ const Tile = forwardRef( // changes to `tileVariables`. const serializedTileVariables = useMemo( () => - !!variables && isRawSqlSavedChartConfig(chart.config) + !!variables && !isPromqlSavedChartConfig(chart.config) ? JSON.stringify(filterReferencedVariables(chart.config, variables)) : undefined, [chart.config, variables], @@ -646,6 +647,7 @@ const Tile = forwardRef( : undefined, sampleWeightExpression: getSampleWeightExpression(source), filters, + variables: tileVariables, metricTables: isMetricSource ? source.metricTables : undefined, }); } @@ -1605,23 +1607,28 @@ const EditTileModal = ({ {/* Isolate chart cross-syncing to this edit modal: the preview chart must not drive shadow tooltips on the dashboard tiles behind it. */} - { - onSave({ - ...chart, - config: config, - }); - }} - onClose={handleClose} - onDirtyChange={setHasUnsavedChanges} - isDashboardForm - autoRun - /> + {/* Offers the dashboard's variables as completions in every + expression input the editor renders. */} + + { + onSave({ + ...chart, + config: config, + }); + }} + onClose={handleClose} + onDirtyChange={setHasUnsavedChanges} + isDashboardForm + autoRun + /> + )} diff --git a/packages/app/src/components/ChartEditor/utils.ts b/packages/app/src/components/ChartEditor/utils.ts index 3b67934091..830f6c1865 100644 --- a/packages/app/src/components/ChartEditor/utils.ts +++ b/packages/app/src/components/ChartEditor/utils.ts @@ -6,11 +6,7 @@ import { isPromqlSavedChartConfig, isRawSqlSavedChartConfig, } from '@hyperdx/common-utils/dist/guards'; -import { - MACRO_SUGGESTIONS, - MacroSuggestion, - VARIABLE_MACRO_SUGGESTIONS, -} from '@hyperdx/common-utils/dist/macros'; +import { MACRO_SUGGESTIONS } from '@hyperdx/common-utils/dist/macros'; import { QUERY_PARAMS_BY_DISPLAY_TYPE } from '@hyperdx/common-utils/dist/rawSqlParams'; import { BuilderSavedChartConfig, @@ -30,76 +26,16 @@ import { SourceKind, TSource, } from '@hyperdx/common-utils/dist/types'; -import { - substituteVariables, - VARIABLE_FORMATS, - VariableFormat, -} from '@hyperdx/common-utils/dist/variables'; import { getStoredLanguage } from '@/components/SearchInput'; import { type SQLCompletion } from '@/components/SQLEditor/utils'; +import { + buildVariableCompletions, + toMacroCompletion, +} from '@/components/SQLEditor/variableCompletions'; import { ChartEditorFormState } from './types'; -/** What each `${name:format}` renders, for the completion's help text. */ -const VARIABLE_FORMAT_DESCRIPTIONS: Record = { - sqlstring: "Quoted and comma-separated, escaped for SQL. e.g. 'a', 'b', 'c'", - csv: 'Comma-separated and unquoted. Not SQL-escaped. e.g. a,b,c', - regex: 'A regex alternation. Regex escaped. e.g. (a|b|c)', - lucene: 'An OR of quoted terms, for Lucene inputs. e.g. ("a" OR "b" OR "c")', -}; - -/** What `snippet` expands to against the variable's current selection. */ -function describeVariableExpansion( - snippet: string, - variable: ChartVariable, -): string | undefined { - let expansion: string; - try { - expansion = substituteVariables(snippet, [variable]); - } catch { - return undefined; - } - return `Expands to: ${expansion === '' ? '(empty string)' : expansion}`; -} - -/** - * Completion help built as markup rather than a string, so `footnote` sits on - * its own line. - * - * CodeMirror turns a string `info` into a single text node, where a `\n` is - * subject to the inherited `white-space` and generally collapses to a space. - * A `Node` is rendered as given, so the break is structural. - */ -function completionInfo(description: string, footnote: string) { - return () => { - const dom = document.createElement('div'); - - const main = document.createElement('div'); - main.textContent = description; - dom.appendChild(main); - - const sub = document.createElement('div'); - sub.className = 'cm-completionInfo-footnote'; - sub.textContent = footnote; - dom.appendChild(sub); - - return dom; - }; -} - -const toMacroCompletion = ({ - name, - minArgs, - description, -}: MacroSuggestion): SQLCompletion => ({ - label: `$__${name}`, - apply: minArgs > 0 ? `$__${name}(` : `$__${name}`, - detail: 'macro', - info: description, - type: 'function', -}); - /** * Autocomplete entries offered on top of columns/keywords in the raw SQL * editor: query params, macros, and variables (when available). @@ -124,80 +60,10 @@ export function buildRawSqlCompletions({ }), ); - const macroCompletions = MACRO_SUGGESTIONS.map(toMacroCompletion); - - if (!variables?.length) { - return [...paramCompletions, ...macroCompletions]; - } - - const variableMacroCompletions = - VARIABLE_MACRO_SUGGESTIONS.map(toMacroCompletion); - - const variableCompletions = variables.flatMap((variable): SQLCompletion[] => { - const { name } = variable; - - /** A static description and an expansion preview given the current variable selections */ - const help = (snippet: string | undefined, description: string) => { - const expansion = - snippet == null - ? undefined - : describeVariableExpansion(snippet, variable); - return expansion ? completionInfo(description, expansion) : description; - }; - - return [ - ...(variable.expression - ? [ - { - label: `$__filter(${name})`, - apply: `$__filter(${name})`, - detail: 'variable filter', - info: help( - `$__filter(${name})`, - `Filters by the ${name} variable using its defined expression. Matches every row when no values are selected for the variable.`, - ), - type: 'function', - }, - ] - : []), - { - label: `$${name}`, - apply: `$${name}`, - detail: 'variable', - info: help( - `$${name}`, - `The selected values of ${name}, in the default sqlstring format. Has no valid empty state — prefer $__filter(, ${name}).`, - ), - type: 'variable', - }, - { - label: `\${${name}}`, - apply: `\${${name}}`, - detail: 'variable', - info: help( - `\${${name}}`, - `The same as $${name}, but delimited — use it when the reference runs into following word characters, as in \${${name}}_total.`, - ), - type: 'variable', - }, - ...VARIABLE_FORMATS.map((format): SQLCompletion => { - const reference = `\${${name}:${format}}`; - return { - label: reference, - apply: reference, - detail: 'variable', - info: help(reference, VARIABLE_FORMAT_DESCRIPTIONS[format]), - type: 'variable', - }; - }), - ]; - }); - return [ ...paramCompletions, - ...macroCompletions, - ...variableMacroCompletions, - ...variableCompletions, + ...MACRO_SUGGESTIONS.map(toMacroCompletion), + ...buildVariableCompletions(variables), ]; } diff --git a/packages/app/src/components/DBEditTimeChartForm/ChartActionBar.tsx b/packages/app/src/components/DBEditTimeChartForm/ChartActionBar.tsx index f4f238ce69..21ca394281 100644 --- a/packages/app/src/components/DBEditTimeChartForm/ChartActionBar.tsx +++ b/packages/app/src/components/DBEditTimeChartForm/ChartActionBar.tsx @@ -93,6 +93,7 @@ export function ChartActionBar({ disableKeywordAutocomplete onSubmit={onSubmit} label="ORDER BY" + enableVariables /> )} diff --git a/packages/app/src/components/DBEditTimeChartForm/ChartEditorControls.tsx b/packages/app/src/components/DBEditTimeChartForm/ChartEditorControls.tsx index e7e237530b..4c33a6e7d8 100644 --- a/packages/app/src/components/DBEditTimeChartForm/ChartEditorControls.tsx +++ b/packages/app/src/components/DBEditTimeChartForm/ChartEditorControls.tsx @@ -188,6 +188,7 @@ export function ChartEditorControls({ } onSubmit={onSubmit} label="Pattern Expression" + enableVariables /> {typeof select === 'string' && select.length > 0 && @@ -209,6 +210,7 @@ export function ChartEditorControls({ setValue('whereLanguage', lang) } showLabel={false} + enableVariables /> ) : displayType !== DisplayType.Search && Array.isArray(select) ? ( @@ -276,6 +278,7 @@ export function ChartEditorControls({ placeholder="SQL Columns" onSubmit={onSubmit} disableKeywordAutocomplete + enableVariables /> {displayType === DisplayType.Table && ( @@ -298,6 +301,7 @@ export function ChartEditorControls({ name="having" placeholder="SQL HAVING clause (ex. count() > 100)" onSubmit={onSubmit} + enableVariables /> @@ -430,6 +434,7 @@ export function ChartEditorControls({ } onSubmit={onSubmit} label="SELECT" + enableVariables /> )} diff --git a/packages/app/src/components/DBEditTimeChartForm/ChartSeriesEditor.tsx b/packages/app/src/components/DBEditTimeChartForm/ChartSeriesEditor.tsx index 9a887caff9..7df64347a1 100644 --- a/packages/app/src/components/DBEditTimeChartForm/ChartSeriesEditor.tsx +++ b/packages/app/src/components/DBEditTimeChartForm/ChartSeriesEditor.tsx @@ -409,6 +409,7 @@ export function ChartSeriesEditor({ name={`${namePrefix}valueExpression`} placeholder="SQL Column" onSubmit={onSubmit} + enableVariables /> )} @@ -439,6 +440,7 @@ export function ChartSeriesEditor({ showLabel={false} additionalSuggestions={attributeSuggestions} data-testid="series-where-input" + enableVariables /> @@ -464,6 +466,7 @@ export function ChartSeriesEditor({ placeholder="SQL Columns" disableKeywordAutocomplete onSubmit={onSubmit} + enableVariables /> {showHaving && ( @@ -479,6 +482,7 @@ export function ChartSeriesEditor({ placeholder="SQL HAVING clause (ex. count() > 100)" disableKeywordAutocomplete onSubmit={onSubmit} + enableVariables /> diff --git a/packages/app/src/components/DBEditTimeChartForm/EditTimeChartForm.tsx b/packages/app/src/components/DBEditTimeChartForm/EditTimeChartForm.tsx index b7497f4cf0..ebdd2364a5 100644 --- a/packages/app/src/components/DBEditTimeChartForm/EditTimeChartForm.tsx +++ b/packages/app/src/components/DBEditTimeChartForm/EditTimeChartForm.tsx @@ -13,7 +13,7 @@ import { displayTypeSupportsRawSqlAlerts, } from '@hyperdx/common-utils/dist/core/utils'; import { - isRawSqlChartConfig, + isPromqlChartConfig, isRawSqlSavedChartConfig, } from '@hyperdx/common-utils/dist/guards'; import { @@ -374,7 +374,7 @@ export default function EditTimeChartForm({ // Attach variables so that variable references can be validated and expanded in the preview const previewConfig = useMemo(() => { - if (queriedConfig == null || !isRawSqlChartConfig(queriedConfig)) { + if (queriedConfig == null || isPromqlChartConfig(queriedConfig)) { return queriedConfig; } return { diff --git a/packages/app/src/components/DBEditTimeChartForm/HeatmapSeriesEditor.tsx b/packages/app/src/components/DBEditTimeChartForm/HeatmapSeriesEditor.tsx index 3bbf37f693..eb94ec8f53 100644 --- a/packages/app/src/components/DBEditTimeChartForm/HeatmapSeriesEditor.tsx +++ b/packages/app/src/components/DBEditTimeChartForm/HeatmapSeriesEditor.tsx @@ -39,6 +39,7 @@ export function HeatmapSeriesEditor({ setValue('whereLanguage', lang) } showLabel={false} + enableVariables /> diff --git a/packages/app/src/components/DBEditTimeChartForm/__tests__/utils.test.ts b/packages/app/src/components/DBEditTimeChartForm/__tests__/utils.test.ts index 971bdf2d1b..0bb2820cd6 100644 --- a/packages/app/src/components/DBEditTimeChartForm/__tests__/utils.test.ts +++ b/packages/app/src/components/DBEditTimeChartForm/__tests__/utils.test.ts @@ -350,6 +350,122 @@ describe('buildSampleEventsConfig', () => { expect(result).not.toBeNull(); expect(result!.select).toBe(''); }); + + // The agg conditions leave `select` for `filters`, which is never scanned for + // variables, so they have to be expanded on the way out. + describe('dashboard variables', () => { + const configWithAggCondition = ( + aggCondition: string, + aggConditionLanguage: 'lucene' | 'sql', + values: string[], + ): ChartConfigWithDateRange => + ({ + ...builderConfig, + select: [ + { + aggFn: 'count', + aggCondition, + aggConditionLanguage, + valueExpression: '', + }, + ], + variables: [{ name: 'svc', expression: 'ServiceName', values }], + }) as ChartConfigWithDateRange; + + it('expands a Lucene reference in an agg condition to the selected values', () => { + const result = buildSampleEventsConfig( + configWithAggCondition('ServiceName:${svc:lucene}', 'lucene', [ + 'accounting', + ]), + logSource, + dateRange, + true, + ); + + expect(result!.filters).toEqual([ + { type: 'lucene', condition: 'ServiceName:("accounting")' }, + ]); + }); + + it('expands a Lucene reference to its empty state when nothing is selected', () => { + const result = buildSampleEventsConfig( + configWithAggCondition('ServiceName:${svc:lucene}', 'lucene', []), + logSource, + dateRange, + true, + ); + + expect(result!.filters).toEqual([ + { type: 'lucene', condition: 'ServiceName:("")' }, + ]); + }); + + it('expands $__filter in a SQL agg condition to the selected values', () => { + const result = buildSampleEventsConfig( + configWithAggCondition('$__filter(ServiceName, svc)', 'sql', [ + 'accounting', + ]), + logSource, + dateRange, + true, + ); + + expect(result!.filters).toEqual([ + { type: 'sql', condition: "(ServiceName IN ('accounting'))" }, + ]); + }); + + it('expands $__filter to its no-op form when nothing is selected', () => { + const result = buildSampleEventsConfig( + configWithAggCondition('$__filter(ServiceName, svc)', 'sql', []), + logSource, + dateRange, + true, + ); + + expect(result!.filters).toEqual([ + { + type: 'sql', + condition: "(1=1 /** no values selected for variable 'svc' */)", + }, + ]); + }); + + it('expands the chart-level where and consumes the variables', () => { + const config = configWithAggCondition('', 'lucene', ['accounting']); + const result = buildSampleEventsConfig( + { + ...config, + where: 'ServiceName IN ($svc)', + whereLanguage: 'sql', + } as ChartConfigWithDateRange, + logSource, + dateRange, + true, + ); + + expect(result!.where).toBe("ServiceName IN ('accounting')"); + // Cleared so the renderer doesn't substitute a second time + expect(result!.variables).toBeUndefined(); + }); + + it('leaves the condition as written when a macro names an unknown variable', () => { + const build = () => + buildSampleEventsConfig( + configWithAggCondition('$__filter(ServiceName, nope)', 'sql', [ + 'accounting', + ]), + logSource, + dateRange, + true, + ); + + expect(build).not.toThrow(); + expect(build()!.filters).toEqual([ + { type: 'sql', condition: '$__filter(ServiceName, nope)' }, + ]); + }); + }); }); // --------------------------------------------------------------------------- diff --git a/packages/app/src/components/DBEditTimeChartForm/utils.ts b/packages/app/src/components/DBEditTimeChartForm/utils.ts index de9ade05c0..01a3376f03 100644 --- a/packages/app/src/components/DBEditTimeChartForm/utils.ts +++ b/packages/app/src/components/DBEditTimeChartForm/utils.ts @@ -23,7 +23,10 @@ import { TSource, validateAlertScheduleOffsetMinutes, } from '@hyperdx/common-utils/dist/types'; -import { filterReferencedVariables } from '@hyperdx/common-utils/dist/variables'; +import { + filterReferencedVariables, + substituteChartConfigVariables, +} from '@hyperdx/common-utils/dist/variables'; import { convertToCategoricalChartConfig, @@ -147,7 +150,7 @@ export function computeDbTimeChartConfig( /** * Returns the dashboard variables a chart preview should use. - * - Builder and PromQL configs don't yet support variables, so they resolve to an empty set + * - PromQL configs don't yet support variables, so they resolve to an empty set * - Alerts always run with empty variable selections, so they resolve to each referenced variable with an empty `values` array. * - Otherwise, variables are filtered to only those referenced by the chart config. */ @@ -167,6 +170,21 @@ export function resolvePreviewVariables({ : referenced; } +/** + * Expand a builder config's variable references, falling back to the config as + * written when one of them can't be expanded (a malformed reference, or a macro + * naming a variable the dashboard doesn't declare). + */ +function expandVariablesOrLeaveRaw< + T extends Parameters[0], +>(config: T): T { + try { + return substituteChartConfigVariables(config); + } catch { + return config; + } +} + export function buildSampleEventsConfig( queriedConfig: ChartConfigWithDateRange | undefined, tableSource: TSource | undefined, @@ -182,8 +200,13 @@ export function buildSampleEventsConfig( return null; } + // The series' agg conditions become `filters` below, and `filters` is + // deliberately not scanned for variable references. So expand the variables + // here, building the filters in the sample events config below. + const config = expandVariablesOrLeaveRaw(queriedConfig); + return { - ...queriedConfig, + ...config, orderBy: [ { ordering: 'DESC' as const, @@ -202,7 +225,7 @@ export function buildSampleEventsConfig( tableSource.kind === SourceKind.Trace) && tableSource.defaultTableSelectExpression) || '', - filters: seriesToFilters(queriedConfig.select), + filters: seriesToFilters(config.select), filtersLogicalOperator: 'OR' as const, groupBy: undefined, granularity: undefined, diff --git a/packages/app/src/components/HeatmapSettingsDrawer.tsx b/packages/app/src/components/HeatmapSettingsDrawer.tsx index bc0d0a87d6..8bc07138cf 100644 --- a/packages/app/src/components/HeatmapSettingsDrawer.tsx +++ b/packages/app/src/components/HeatmapSettingsDrawer.tsx @@ -102,6 +102,7 @@ export default function HeatmapSettingsDrawer({ label="Value" error={form.formState.errors.value?.message} rules={{ required: true }} + enableVariables /> (null); @@ -177,6 +197,7 @@ export default function SQLInlineEditor({ identifiers, keywords: KEYWORDS_FOR_WHERE_OR_ORDER_BY, includeRegularFunctions: !disableKeywordAutocomplete, + additionalCompletions: variableCompletions, }); const queryHistoryList = autocompletion({ @@ -194,6 +215,7 @@ export default function SQLInlineEditor({ [ filteredFields, additionalSuggestions, + variableCompletions, disableKeywordAutocomplete, createHistoryList, hasNonEmptyValue, @@ -311,6 +333,9 @@ export default function SQLInlineEditor({ // Only apply expanded styling when multiline is enabled and focused const isExpanded = allowMultiline && isFocused; + + const isVariableWarningOnly = + variableIssues.errors.length === 0 && variableIssues.warnings.length > 0; const baseHeight = size === 'xs' ? 30 : 36; return ( @@ -325,7 +350,8 @@ export default function SQLInlineEditor({ shadow="none" className={cx( styles.paper, - error ? styles.error : undefined, + error || variableIssues.errors.length > 0 ? styles.error : undefined, + isVariableWarningOnly ? styles.warning : undefined, isExpanded ? styles.expanded : undefined, allowMultiline && !isExpanded ? styles.collapseFade : undefined, )} @@ -377,6 +403,7 @@ export default function SQLInlineEditor({ onClick={onClickCodeMirror} /> + {onLanguageChange != null && language != null && (
+ buildVariableCompletions(variables).map(completion => completion.label); + +/** The second line of a completion's help — what it expands to right now. */ +const footnoteOf = (variables: ChartVariable[], label: string) => { + const { info } = buildVariableCompletions(variables).find( + completion => completion.label === label, + ) ?? { info: undefined }; + if (typeof info !== 'function') return undefined; + const rendered = info(); + if (!(rendered instanceof HTMLElement)) return undefined; + return ( + rendered.querySelector('.cm-completionInfo-footnote')?.textContent ?? + undefined + ); +}; + +describe('buildVariableCompletions', () => { + it.each([ + ['off a dashboard', undefined], + ['on a dashboard that defines none', [] as ChartVariable[]], + ])('offers nothing %s', (_label, variables) => { + expect(buildVariableCompletions(variables)).toEqual([]); + }); + + it('offers the variable macros and every reference form', () => { + expect(labels([SERVICE])).toEqual( + expect.arrayContaining([ + '$__filter', + '$__conditionalAll', + '$__filter(service)', + '$service', + '${service}', + '${service:sqlstring}', + '${service:csv}', + '${service:regex}', + '${service:lucene}', + ]), + ); + }); + + it('withholds the macros that a chart builder input never expands', () => { + // Only the variable macros are substituted into builder expressions, so + // offering $__timeFilter and friends here would suggest SQL that reaches + // ClickHouse verbatim. + expect(labels([SERVICE])).not.toContain('$__timeFilter'); + expect(labels([SERVICE])).not.toContain('$__sourceTable'); + expect(labels([SERVICE])).not.toContain('$__filters'); + }); + + it('shows what each form expands to against the current selection', () => { + expect(footnoteOf([SERVICE], '$service')).toBe("Expands to: 'api', 'web'"); + expect(footnoteOf([SERVICE], '$__filter(service)')).toBe( + "Expands to: (toString(ServiceName) IN ('api', 'web'))", + ); + }); + + it('shows the empty-selection expansion when nothing is selected', () => { + const unselected: ChartVariable = { ...SERVICE, values: [] }; + expect(footnoteOf([unselected], '$service')).toBe('Expands to: NULL'); + expect(footnoteOf([unselected], '$__filter(service)')).toContain( + 'Expands to: (1=1', + ); + // The lucene form's empty term is itself a no-op once rendered to SQL. + expect(footnoteOf([unselected], '${service:lucene}')).toBe( + 'Expands to: ("")', + ); + }); +}); + +describe('buildLuceneVariableSuggestions', () => { + it.each([ + ['off a dashboard', undefined], + ['on a dashboard that defines none', [] as ChartVariable[]], + ])('offers nothing %s', (_label, variables) => { + expect(buildLuceneVariableSuggestions(variables)).toEqual([]); + }); + + it('offers the bare reference and nothing else', () => { + // No macros: they expand to SQL predicates a Lucene parser cannot read. + // No braced or explicit-format forms either — in a Lucene input the bare + // reference already renders in the lucene format. + expect(buildLuceneVariableSuggestions([SERVICE])).toEqual([ + { + value: '$service', + label: '$service', + description: + 'The selected values of service. Expands to: ("api" OR "web")', + }, + ]); + }); + + it('previews the empty selection as the term that drops out', () => { + expect( + buildLuceneVariableSuggestions([{ ...SERVICE, values: [] }])[0] + .description, + ).toContain('Expands to: ("")'); + }); +}); + +describe('expandLuceneVariablesForEnglishDisplay', () => { + const expand = (text: string, variables?: ChartVariable[]) => + expandLuceneVariablesForEnglishDisplay(text, variables); + + it.each([ + ['off a dashboard', undefined], + ['on a dashboard that defines none', [] as ChartVariable[]], + ])('returns the text unchanged %s', (_label, variables) => { + expect(expand('ServiceName:$service', variables)).toBe( + 'ServiceName:$service', + ); + }); + + it('expands a selected variable in the lucene format', () => { + expect(expand('ServiceName:$service', [SERVICE])).toBe( + 'ServiceName:("api" OR "web")', + ); + }); + + it('leaves an unselected variable as written', () => { + // `("")` reads as `'ServiceName' is ` once serialized to English, + // which is worse than naming the placeholder that has no value yet. + expect(expand('ServiceName:$service', [{ ...SERVICE, values: [] }])).toBe( + 'ServiceName:$service', + ); + }); + + it('expands only the variables that have a selection', () => { + expect( + expand('ServiceName:$service AND Env:$env', [ + SERVICE, + { name: 'env', values: [] }, + ]), + ).toBe('ServiceName:("api" OR "web") AND Env:$env'); + }); + + it('leaves unknown references and the variable macros alone', () => { + expect(expand('$nope AND $__filter(ServiceName, service)', [SERVICE])).toBe( + '$nope AND $__filter(ServiceName, service)', + ); + }); +}); diff --git a/packages/app/src/components/SQLEditor/variableCompletions.tsx b/packages/app/src/components/SQLEditor/variableCompletions.tsx new file mode 100644 index 0000000000..7c175c40ce --- /dev/null +++ b/packages/app/src/components/SQLEditor/variableCompletions.tsx @@ -0,0 +1,261 @@ +import { createContext, use, useCallback, useMemo } from 'react'; +import { + MacroSuggestion, + VARIABLE_MACRO_SUGGESTIONS, +} from '@hyperdx/common-utils/dist/macros'; +import { ChartVariable } from '@hyperdx/common-utils/dist/types'; +import { + substituteVariables, + VARIABLE_FORMATS, + VariableFormat, +} from '@hyperdx/common-utils/dist/variables'; + +import { type SQLCompletion } from './utils'; + +/** What each `${name:format}` renders, for the completion's help text. */ +const VARIABLE_FORMAT_DESCRIPTIONS: Record = { + sqlstring: "Quoted and comma-separated, escaped for SQL. e.g. 'a', 'b', 'c'", + csv: 'Comma-separated and unquoted. Not SQL-escaped. e.g. a,b,c', + regex: 'A regex alternation. Regex escaped. e.g. (a|b|c)', + lucene: 'An OR of quoted terms, for Lucene inputs. e.g. ("a" OR "b" OR "c")', +}; + +/** What `snippet` expands to against the variable's current selection. */ +function describeVariableExpansion( + snippet: string, + variable: ChartVariable, +): string | undefined { + let expansion: string; + try { + expansion = substituteVariables(snippet, [variable]); + } catch { + return undefined; + } + return `Expands to: ${expansion === '' ? '(empty string)' : expansion}`; +} + +/** + * Completion help built as markup rather than a string, so `footnote` sits on + * its own line. + * + * CodeMirror turns a string `info` into a single text node, where a `\n` is + * subject to the inherited `white-space` and generally collapses to a space. + * A `Node` is rendered as given, so the break is structural. + */ +function completionInfo(description: string, footnote: string) { + return () => { + const dom = document.createElement('div'); + + const main = document.createElement('div'); + main.textContent = description; + dom.appendChild(main); + + const sub = document.createElement('div'); + sub.className = 'cm-completionInfo-footnote'; + sub.textContent = footnote; + dom.appendChild(sub); + + return dom; + }; +} + +export const toMacroCompletion = ({ + name, + minArgs, + description, +}: MacroSuggestion): SQLCompletion => ({ + label: `$__${name}`, + apply: minArgs > 0 ? `$__${name}(` : `$__${name}`, + detail: 'macro', + info: description, + type: 'function', +}); + +/** Every reference form of one variable, each with its current expansion. */ +function referenceCompletions(variable: ChartVariable): SQLCompletion[] { + const { name } = variable; + + /** A static description and an expansion preview given the current selection */ + const help = (snippet: string, description: string) => { + const expansion = describeVariableExpansion(snippet, variable); + return expansion ? completionInfo(description, expansion) : description; + }; + + return [ + ...(variable.expression + ? [ + { + label: `$__filter(${name})`, + apply: `$__filter(${name})`, + detail: 'variable filter', + info: help( + `$__filter(${name})`, + `Filters by the ${name} variable using its defined expression. Matches every row when no values are selected for the variable.`, + ), + type: 'function', + }, + ] + : []), + { + label: `$${name}`, + apply: `$${name}`, + detail: 'variable', + info: help( + `$${name}`, + `The selected values of ${name}, in the default sqlstring format. Has no valid empty state — prefer $__filter(, ${name}).`, + ), + type: 'variable', + }, + { + label: `\${${name}}`, + apply: `\${${name}}`, + detail: 'variable', + info: help( + `\${${name}}`, + `The same as $${name}, but delimited — use it when the reference runs into following word characters, as in \${${name}}_total.`, + ), + type: 'variable', + }, + ...VARIABLE_FORMATS.map((format): SQLCompletion => { + const reference = `\${${name}:${format}}`; + return { + label: reference, + apply: reference, + detail: 'variable', + info: help(reference, VARIABLE_FORMAT_DESCRIPTIONS[format]), + type: 'variable', + }; + }), + ]; +} + +/** + * Auto-completions for the variables available to a query: the variable + * macros, then every reference form of each variable. + */ +export function buildVariableCompletions( + variables: ChartVariable[] | undefined, +): SQLCompletion[] { + if (!variables?.length) return []; + + return [ + ...VARIABLE_MACRO_SUGGESTIONS.map(toMacroCompletion), + ...variables.flatMap(referenceCompletions), + ]; +} + +/** One bare `$name` suggestion for a Lucene input. */ +export type LuceneVariableSuggestion = { + value: string; + label: string; + description: string; +}; + +/** Expand references the way a Lucene expression is expanded at query time. */ +const substituteLucene = (text: string, variables: ChartVariable[]) => + substituteVariables(text, variables, { + defaultFormat: 'lucene', + disableMacros: true, + }); + +/** + * Suggestions for a Lucene expression: the bare `$name` reference of each + * variable, and nothing else. + * + * The macros are deliberately absent — they expand to SQL predicates, and + * the lucene format empty state `("")` is safe without needing macros. + */ +export function buildLuceneVariableSuggestions( + variables: ChartVariable[] | undefined, +): LuceneVariableSuggestion[] { + return (variables ?? []).map(variable => { + const reference = `$${variable.name}`; + const expansion = substituteLucene(reference, [variable]); + return { + value: reference, + label: reference, + description: `The selected values of ${variable.name}. Expands to: ${expansion}`, + }; + }); +} + +/** + * Expand the dashboard variables in a Lucene expression, for display only — + * the input's plain-English summary describes the query that will really run, + * rather than naming the placeholders. + * + * Only variables with a selection are substituted. An empty one renders as + * `("")`, which the English serializer reads as `'field' is ` even + * though that form filters nothing; leaving the reference as written is the + * honest rendering of "no value chosen yet". + */ +export function expandLuceneVariablesForEnglishDisplay( + text: string, + variables: ChartVariable[] | undefined, +): string { + const selected = (variables ?? []).filter( + variable => variable.values.length > 0, + ); + return selected.length > 0 ? substituteLucene(text, selected) : text; +} + +/** Context providing in-scope dashboard variables for descendant inputs. */ +const SqlVariablesContext = createContext( + undefined, +); + +export function SqlVariablesProvider({ + variables, + children, +}: { + variables: ChartVariable[] | undefined; + children: React.ReactNode; +}) { + return ( + {children} + ); +} + +export type VariableSupportOptions = { enabled?: boolean }; + +/** + * The variables an input can use, or undefined when there are none to use. + * + * An empty scope is undefined here rather than `[]`: to an input there is no + * difference between a dashboard that declares no variables, one with the + * feature disabled, and somewhere that is not a dashboard at all — in each case + * nothing is substituted, so there is nothing to offer and nothing to check. + */ +export function useChartVariables({ + enabled = true, +}: VariableSupportOptions = {}): ChartVariable[] | undefined { + const variables = use(SqlVariablesContext); + return enabled && variables?.length ? variables : undefined; +} + +/** Variable completions for a SQL input inside a `SqlVariablesProvider`. */ +export function useVariableCompletions( + options?: VariableSupportOptions, +): SQLCompletion[] { + const variables = useChartVariables(options); + return useMemo(() => buildVariableCompletions(variables), [variables]); +} + +/** Variable suggestions for a Lucene input inside a `SqlVariablesProvider`. */ +export function useLuceneVariableSuggestions( + options?: VariableSupportOptions, +): LuceneVariableSuggestion[] { + const variables = useChartVariables(options); + return useMemo(() => buildLuceneVariableSuggestions(variables), [variables]); +} + +/** Expands variables in a Lucene expression for an input's English summary */ +export function useLuceneVariableEnglishExpander( + options?: VariableSupportOptions, +): (text: string) => string { + const variables = useChartVariables(options); + return useCallback( + (text: string) => expandLuceneVariablesForEnglishDisplay(text, variables), + [variables], + ); +} diff --git a/packages/app/src/components/SQLEditor/variableValidation.module.scss b/packages/app/src/components/SQLEditor/variableValidation.module.scss new file mode 100644 index 0000000000..30db348fdd --- /dev/null +++ b/packages/app/src/components/SQLEditor/variableValidation.module.scss @@ -0,0 +1,13 @@ +.indicator { + display: flex; + align-items: center; + flex-shrink: 0; + padding: 0 6px; + line-height: 1; + cursor: help; + + /* In the SQL editor, centers the icon on the collapsed input's first line so + it stays put when the editor expands to multiple lines. Elsewhere (the + Lucene input's right section) it just fills its host. */ + min-height: var(--editor-base-height, 100%); +} diff --git a/packages/app/src/components/SQLEditor/variableValidation.tsx b/packages/app/src/components/SQLEditor/variableValidation.tsx new file mode 100644 index 0000000000..7586df885a --- /dev/null +++ b/packages/app/src/components/SQLEditor/variableValidation.tsx @@ -0,0 +1,94 @@ +import { useMemo } from 'react'; +import { SearchConditionLanguage } from '@hyperdx/common-utils/dist/types'; +import { + validateVariableReferencesInTemplate, + VariableReferenceIssues, +} from '@hyperdx/common-utils/dist/variables'; +import { List, Text, Tooltip } from '@mantine/core'; +import { useDebouncedValue } from '@mantine/hooks'; +import { IconAlertTriangle } from '@tabler/icons-react'; + +import { + useChartVariables, + VariableSupportOptions, +} from './variableCompletions'; + +import styles from './variableValidation.module.scss'; + +const NO_ISSUES: VariableReferenceIssues = Object.freeze({ + errors: [], + warnings: [], +}); + +const VALIDATION_DEBOUNCE_MS = 500; + +export const hasVariableIssues = (issues: VariableReferenceIssues) => + issues.errors.length > 0 || issues.warnings.length > 0; + +/** + * Returns variable-reference issues in the given template. Returns nothing when no variables are in scope. + */ +export function useVariableValidation( + template: string, + { + enabled = true, + language = 'sql', + }: VariableSupportOptions & { + language?: SearchConditionLanguage; + } = {}, +): VariableReferenceIssues { + const variables = useChartVariables({ enabled }); + const [debouncedValue] = useDebouncedValue(template, VALIDATION_DEBOUNCE_MS); + + return useMemo(() => { + if (variables == null) return NO_ISSUES; + return validateVariableReferencesInTemplate(debouncedValue, variables, { + subject: 'This expression', + language, + }); + }, [debouncedValue, language, variables]); +} + +/** + * The alert icon an input shows when its expression misuses a variable. + */ +export function VariableIssueIndicator({ + issues, +}: { + issues: VariableReferenceIssues; +}) { + const { errors, warnings } = issues; + if (!hasVariableIssues(issues)) return null; + + const isError = errors.length > 0; + const messages = [...errors, ...warnings]; + + return ( + + {messages.map(message => ( + {message} + ))} + + ) + } + > + + + + + ); +} diff --git a/packages/app/src/components/SearchInput/AutocompleteInput.module.scss b/packages/app/src/components/SearchInput/AutocompleteInput.module.scss index 3539687b8f..2536b70f65 100644 --- a/packages/app/src/components/SearchInput/AutocompleteInput.module.scss +++ b/packages/app/src/components/SearchInput/AutocompleteInput.module.scss @@ -4,6 +4,13 @@ min-height: var(--autocomplete-base-height, 36px); } +/* The measured right section: an optional adornment, then the language switch */ +.rightSection { + display: flex; + align-items: center; + height: 100%; +} + .textarea { flex-grow: 1; resize: none; @@ -93,6 +100,13 @@ margin-right: 0.25rem; } +// What a suggestion means and, for a variable, what it expands to right now. +.suggestionDescription { + color: var(--color-text-muted); + font-size: 0.75rem; + margin-top: 0.125rem; +} + .belowSuggestions { border-top: 1px solid var(--color-border); padding: 0.5rem 0.75rem 0.25rem; diff --git a/packages/app/src/components/SearchInput/AutocompleteInput.tsx b/packages/app/src/components/SearchInput/AutocompleteInput.tsx index ce8c3bc9f8..91849581b3 100644 --- a/packages/app/src/components/SearchInput/AutocompleteInput.tsx +++ b/packages/app/src/components/SearchInput/AutocompleteInput.tsx @@ -16,11 +16,13 @@ export default function AutocompleteInput({ onChange, placeholder = 'Search your events for anything...', autocompleteOptions, + variableOptions, isLoadingValues, tokenInfo, size = 'sm', aboveSuggestions, belowSuggestions, + rightAdornment, showSuggestionsOnEmpty, suggestionsHeader = 'Properties', zIndex = 999, @@ -37,10 +39,13 @@ export default function AutocompleteInput({ placeholder?: string; size?: 'xs' | 'sm' | 'lg'; autocompleteOptions?: { value: string; label: string }[]; + variableOptions?: { value: string; label: string; description: string }[]; isLoadingValues?: boolean; tokenInfo?: TokenInfo; aboveSuggestions?: React.ReactNode; belowSuggestions?: React.ReactNode; + /** Rendered at the right edge of the input, left of the language switch. */ + rightAdornment?: React.ReactNode; showSuggestionsOnEmpty?: boolean; suggestionsHeader?: string; zIndex?: number; @@ -98,6 +103,23 @@ export default function AutocompleteInput({ [autocompleteOptions], ); + /** + * The `$var` fragment being typed at the end of the token, if any. A + * variable reference is completed within its token rather than replacing it, + * so `ServiceName:$sv` can become `ServiceName:$svc` instead of just `$svc`. + */ + const variableFragment = useMemo( + () => tokenInfo?.token.match(/\$[A-Za-z0-9_]*$/)?.[0], + [tokenInfo], + ); + + const suggestedVariables = useMemo(() => { + if (variableFragment == null || !variableOptions?.length) return []; + return variableOptions.filter(option => + option.value.startsWith(variableFragment), + ); + }, [variableFragment, variableOptions]); + const suggestedProperties = useMemo(() => { const token = tokenInfo?.token ?? ''; @@ -107,6 +129,18 @@ export default function AutocompleteInput({ return fuse.search(token).map(result => result.item); }, [tokenInfo, fuse, autocompleteOptions, showSuggestionsOnEmpty]); + // While a `$var` fragment is being typed, variables are the only useful + // suggestions, since no property name can match a token ending in `$…`. + const suggestions: { + value: string; + label: string; + description?: string; + isVariable?: boolean; + }[] = + suggestedVariables.length > 0 + ? suggestedVariables.map(option => ({ ...option, isVariable: true })) + : suggestedProperties; + const onSelectSearchHistory = (query: string) => { setSelectedQueryHistoryIndex(-1); onChange(query); // update inputText bar @@ -115,7 +149,7 @@ export default function AutocompleteInput({ onSubmit?.(); // search }; - const onAcceptSuggestion = (suggestion: string) => { + const onAcceptSuggestion = (suggestion: string, isVariable = false) => { setSelectedAutocompleteIndex(-1); if (value == null || !tokenInfo) { @@ -124,9 +158,15 @@ export default function AutocompleteInput({ return; } - // Replace the token at cursor with the suggestion + // Replace the token at cursor with the suggestion — except for a variable, + // which replaces only the `$var` fragment so anything the reference is + // scoped to (`ServiceName:`) survives. const tokens = [...tokenInfo.tokens]; - tokens[tokenInfo.index] = suggestion; + const currentToken = tokens[tokenInfo.index] ?? ''; + tokens[tokenInfo.index] = + isVariable && variableFragment != null + ? currentToken.slice(0, -variableFragment.length) + suggestion + : suggestion; const newValue = tokens.join(' '); // Place cursor right after the inserted suggestion @@ -151,7 +191,7 @@ export default function AutocompleteInput({ if (inputRef.current) { setInputWidth(inputRef.current.clientWidth); } - }, [language, onLanguageChange, inputRef]); + }, [language, onLanguageChange, rightAdornment, inputRef]); // Height including the 2px border from .textarea (1px top + 1px bottom) const baseHeight = size === 'xs' ? 30 : size === 'lg' ? 44 : 38; @@ -218,14 +258,13 @@ export default function AutocompleteInput({ // Autocomplete Navigation/Acceptance Keys if (e.key === 'Tab' && e.target instanceof HTMLTextAreaElement) { if ( - suggestedProperties.length > 0 && - selectedAutocompleteIndex < suggestedProperties.length && + suggestions.length > 0 && + selectedAutocompleteIndex < suggestions.length && selectedAutocompleteIndex >= 0 ) { e.preventDefault(); - onAcceptSuggestion( - suggestedProperties[selectedAutocompleteIndex].value, - ); + const selected = suggestions[selectedAutocompleteIndex]; + onAcceptSuggestion(selected.value, selected.isVariable); } } if ( @@ -233,14 +272,13 @@ export default function AutocompleteInput({ e.target instanceof HTMLTextAreaElement ) { if ( - suggestedProperties.length > 0 && - selectedAutocompleteIndex < suggestedProperties.length && + suggestions.length > 0 && + selectedAutocompleteIndex < suggestions.length && selectedAutocompleteIndex >= 0 ) { e.preventDefault(); - onAcceptSuggestion( - suggestedProperties[selectedAutocompleteIndex].value, - ); + const selected = suggestions[selectedAutocompleteIndex]; + onAcceptSuggestion(selected.value, selected.isVariable); } else { // Allow shift+enter to still create new lines if (!e.shiftKey) { @@ -256,12 +294,12 @@ export default function AutocompleteInput({ e.key === 'ArrowDown' && e.target instanceof HTMLTextAreaElement ) { - if (suggestedProperties.length > 0) { + if (suggestions.length > 0) { e.preventDefault(); setSelectedAutocompleteIndex( Math.min( selectedAutocompleteIndex + 1, - suggestedProperties.length - 1, + suggestions.length - 1, suggestionsLimit - 1, ), ); @@ -271,7 +309,7 @@ export default function AutocompleteInput({ e.key === 'ArrowUp' && e.target instanceof HTMLTextAreaElement ) { - if (suggestedProperties.length > 0) { + if (suggestions.length > 0) { e.preventDefault(); setSelectedAutocompleteIndex( Math.max(selectedAutocompleteIndex - 1, 0), @@ -281,12 +319,16 @@ export default function AutocompleteInput({ }} rightSectionWidth={rightSectionWidth} rightSection={ - language != null && onLanguageChange != null ? ( -
- + rightAdornment != null || + (language != null && onLanguageChange != null) ? ( +
+ {rightAdornment} + {language != null && onLanguageChange != null && ( + + )}
) : undefined } @@ -297,24 +339,26 @@ export default function AutocompleteInput({
{aboveSuggestions}
)}
- {suggestedProperties.length > 0 && ( + {suggestions.length > 0 && (
- {suggestionsHeader} + {suggestedVariables.length > 0 + ? 'Dashboard variables' + : suggestionsHeader} {isLoadingValues && ( )}
- {suggestedProperties.length > suggestionsLimit && ( + {suggestions.length > suggestionsLimit && (
(Showing Top {suggestionsLimit})
)}
- {suggestedProperties + {suggestions .slice(0, suggestionsLimit) - .map(({ value, label }, i) => ( + .map(({ value, label, description, isVariable }, i) => (
{ - onAcceptSuggestion(value); + onAcceptSuggestion(value, isVariable); }} > {label} + {description != null && ( +
+ {description} +
+ )}
))}
diff --git a/packages/app/src/components/SearchInput/SearchInputV2.tsx b/packages/app/src/components/SearchInput/SearchInputV2.tsx index 904b281f32..db1fde9ea0 100644 --- a/packages/app/src/components/SearchInput/SearchInputV2.tsx +++ b/packages/app/src/components/SearchInput/SearchInputV2.tsx @@ -9,6 +9,15 @@ import { genEnglishExplanation } from '@hyperdx/common-utils/dist/queryParser'; import { Group } from '@mantine/core'; import { IconBook } from '@tabler/icons-react'; +import { + useLuceneVariableEnglishExpander, + useLuceneVariableSuggestions, +} from '@/components/SQLEditor/variableCompletions'; +import { + hasVariableIssues, + useVariableValidation, + VariableIssueIndicator, +} from '@/components/SQLEditor/variableValidation'; import { ILanguageFormatter, useAutoCompleteOptions, @@ -46,6 +55,7 @@ export default function SearchInputV2({ queryHistoryType, dateRange, sourceId, + enableVariables = false, 'data-testid': dataTestId, ...props }: { @@ -60,6 +70,7 @@ export default function SearchInputV2({ queryHistoryType?: string; dateRange?: [Date, Date]; sourceId?: string; + enableVariables?: boolean; 'data-testid'?: string; } & UseControllerProps & TableConnectionChoice) { @@ -71,6 +82,21 @@ export default function SearchInputV2({ const ref = useRef(null); const [parsedEnglishQuery, setParsedEnglishQuery] = useState(''); + // Bare `$name` references only, no macros + const variableOptions = useLuceneVariableSuggestions({ + enabled: enableVariables, + }); + const expandVariablesForEnglish = useLuceneVariableEnglishExpander({ + enabled: enableVariables, + }); + const variableIssues = useVariableValidation( + value != null ? `${value}` : '', + { + enabled: enableVariables, + language: 'lucene', + }, + ); + const { options: autoCompleteOptions, isLoadingValues, @@ -90,14 +116,14 @@ export default function SearchInputV2({ useEffect(() => { if (tableConnection) { genEnglishExplanation({ - query: value, + query: expandVariablesForEnglish(value != null ? `${value}` : ''), tableConnection, metadata, }).then(q => { setParsedEnglishQuery(q); }); } - }, [value, tableConnection, metadata]); + }, [value, expandVariablesForEnglish, tableConnection, metadata]); useHotkeys( ['/', 's'], @@ -121,6 +147,7 @@ export default function SearchInputV2({ onChange={onChange} placeholder={placeholder} autocompleteOptions={autoCompleteOptions} + variableOptions={variableOptions} isLoadingValues={isLoadingValues} tokenInfo={tokenInfo} size={size} @@ -130,6 +157,11 @@ export default function SearchInputV2({ onSubmit={onSubmit} queryHistoryType={queryHistoryType} data-testid={dataTestId} + rightAdornment={ + hasVariableIssues(variableIssues) ? ( + + ) : undefined + } aboveSuggestions={ <>
Searching for:
diff --git a/packages/app/src/components/SearchInput/SearchWhereInput.tsx b/packages/app/src/components/SearchInput/SearchWhereInput.tsx index e4f60134b6..e405c7d77d 100644 --- a/packages/app/src/components/SearchInput/SearchWhereInput.tsx +++ b/packages/app/src/components/SearchInput/SearchWhereInput.tsx @@ -120,6 +120,8 @@ export type SearchWhereInputProps = { */ sourceId?: string; parentRef?: HTMLElement | null; + /** Whether the dashboard variables in scope apply to this expression. */ + enableVariables?: boolean; } & TableConnectionChoice & UseControllerProps; @@ -168,6 +170,7 @@ export default function SearchWhereInput({ languageName = `${name}Language`, sourceId, parentRef, + enableVariables = false, }: SearchWhereInputProps) { const [syntaxRefOpened, { open: openSyntaxRef, close: closeSyntaxRef }] = useDisclosure(false); @@ -243,6 +246,7 @@ export default function SearchWhereInput({ dateRange={dateRange} sourceId={sourceId} parentRef={parentRef} + enableVariables={enableVariables} /> ) : ( )} {enableHotkey && ( diff --git a/packages/app/src/components/SearchInput/__tests__/SearchWhereInput.test.tsx b/packages/app/src/components/SearchInput/__tests__/SearchWhereInput.test.tsx index eeb8a55404..059e4a064d 100644 --- a/packages/app/src/components/SearchInput/__tests__/SearchWhereInput.test.tsx +++ b/packages/app/src/components/SearchInput/__tests__/SearchWhereInput.test.tsx @@ -7,6 +7,7 @@ import { render, screen, waitFor } from '@testing-library/react'; import userEvent from '@testing-library/user-event'; import SearchWhereInput from '@/components/SearchInput/SearchWhereInput'; +import { SqlVariablesProvider } from '@/components/SQLEditor/variableCompletions'; function renderWithMantine(ui: React.ReactElement) { return render( @@ -35,16 +36,20 @@ const queryClient = new QueryClient({ // Test wrapper component that provides form context and query client function TestWrapper({ defaultLanguage = 'lucene', + defaultWhere = '', + supportsVariables, onSubmit, children, }: { defaultLanguage?: 'sql' | 'lucene'; + defaultWhere?: string; + supportsVariables?: boolean; onSubmit?: jest.Mock; children?: (props: { control: any }) => React.ReactNode; }) { const form = useForm({ defaultValues: { - where: '', + where: defaultWhere, whereLanguage: defaultLanguage, }, }); @@ -60,6 +65,7 @@ function TestWrapper({ name="where" onSubmit={onSubmit} enableHotkey + enableVariables={supportsVariables} /> )} @@ -199,4 +205,241 @@ describe('SearchWhereInput', () => { ).toBeInTheDocument(); }); }); + + describe('Dashboard variables in Lucene Mode', () => { + const variables = [ + { name: 'svc', values: ['api'], expression: 'ServiceName' }, + ]; + + const renderWithVariables = ( + inScope: { + name: string; + values: string[]; + expression?: string; + }[] = variables, + ) => { + renderWithMantine( + + + , + ); + return screen.getByPlaceholderText(/Search your events w\/ Lucene/i); + }; + + /** The "Searching for:" summary shown above the suggestions. */ + const searchingFor = () => + screen.getByText('Searching for:').nextElementSibling?.textContent ?? ''; + + it('suggests a bare reference, with what it expands to now', async () => { + const user = userEvent.setup(); + const input = renderWithVariables(); + + await user.type(input, 'ServiceName:$s'); + + expect(await screen.findByText('$svc')).toBeInTheDocument(); + expect( + screen.getByText(/The selected values of svc\. Expands to: \("api"\)/), + ).toBeInTheDocument(); + }); + + it('completes in place, keeping the field the reference is scoped to', async () => { + const user = userEvent.setup(); + const input = renderWithVariables(); + + await user.type(input, 'ServiceName:$s'); + await user.click(await screen.findByText('$svc')); + + await waitFor(() => expect(input).toHaveValue('ServiceName:$svc')); + }); + + it('does not offer the variable macros', async () => { + // They expand to SQL predicates, so they are unsupported in Lucene. + const user = userEvent.setup(); + const input = renderWithVariables(); + + await user.type(input, '$__'); + + await waitFor(() => + expect(screen.queryByText('$__filter')).not.toBeInTheDocument(), + ); + expect(screen.queryByText('$svc')).not.toBeInTheDocument(); + }); + + it('explains the query with the selected values, not the reference', async () => { + const user = userEvent.setup(); + const input = renderWithVariables(); + + await user.type(input, 'ServiceName:$svc'); + + await waitFor(() => + expect(searchingFor()).toBe('(ServiceName contains api)'), + ); + }); + + it('keeps the reference in the explanation while nothing is selected', async () => { + const user = userEvent.setup(); + const input = renderWithVariables([{ name: 'svc', values: [] }]); + + await user.type(input, 'ServiceName:$svc'); + + // An empty selection expands to `("")`, which reads as `is ` + // even though it filters nothing. + await waitFor(() => expect(searchingFor()).toContain('$svc')); + expect(searchingFor()).not.toContain('""'); + }); + + it('offers nothing to a field the variables do not apply to', async () => { + // A field the renderer never substitutes must not advertise a reference + // form that would reach ClickHouse verbatim. + const user = userEvent.setup(); + renderWithMantine( + + + , + ); + + await user.type( + screen.getByPlaceholderText(/Search your events w\/ Lucene/i), + 'ServiceName:$s', + ); + + await waitFor(() => + expect( + screen.queryByText('Dashboard variables'), + ).not.toBeInTheDocument(), + ); + expect(screen.queryByText('$svc')).not.toBeInTheDocument(); + }); + }); + + describe('Variable validation', () => { + const variables = [ + { name: 'svc', values: ['api'], expression: 'ServiceName' }, + ]; + + const renderWithVariables = ( + props: React.ComponentProps, + ) => + renderWithMantine( + + + , + ); + + /** The messages the indicator shows, or null when there is no indicator. */ + const issueMessages = () => + screen.queryByTestId('variable-validation')?.getAttribute('aria-label') ?? + null; + + it('warns that a SQL expression references a variable that does not exist', async () => { + renderWithVariables({ + defaultLanguage: 'sql', + defaultWhere: 'ServiceName IN ($srvice)', + }); + + await waitFor(() => + expect(issueMessages()).toBe( + 'This expression references unknown variable $srvice. Available variables: svc.', + ), + ); + }); + + it('warns that a Lucene expression references a variable that does not exist', async () => { + renderWithVariables({ + defaultLanguage: 'lucene', + defaultWhere: 'ServiceName:$srvice', + }); + + await waitFor(() => + expect(issueMessages()).toBe( + 'This expression references unknown variable $srvice. Available variables: svc.', + ), + ); + }); + + it('errors when a SQL reference is wrapped in quotes', async () => { + renderWithVariables({ + defaultLanguage: 'sql', + defaultWhere: "ServiceName = '$svc'", + }); + + await waitFor(() => + expect(issueMessages()).toContain('is wrapped in quotes'), + ); + }); + + it('leaves a quoted reference alone in Lucene, where each value is quoted anyway', async () => { + renderWithVariables({ + defaultLanguage: 'lucene', + defaultWhere: 'ServiceName:"$svc"', + }); + + await waitFor(() => expect(issueMessages()).toBeNull()); + }); + + it('says nothing without a variable context, where nothing is substituted', async () => { + // No provider: an expression on the search page or in a source form is + // never substituted, so a `$name` in it is just text. + renderWithMantine( + , + ); + + await waitFor(() => expect(issueMessages()).toBeNull()); + }); + + it('says nothing about a field the variables do not apply to', async () => { + renderWithVariables({ + defaultLanguage: 'sql', + defaultWhere: 'ServiceName IN ($srvice)', + supportsVariables: false, + }); + + await waitFor(() => expect(issueMessages()).toBeNull()); + }); + + it.each([ + // The dashboard variables feature is off, so nothing is substituted. + ['the feature is disabled', undefined], + // On, but no filter on this dashboard is exposed as a variable. + ['the dashboard declares none', []], + ])('says nothing when %s', async (_case, inScope) => { + renderWithMantine( + + + , + ); + + await waitFor(() => expect(issueMessages()).toBeNull()); + }); + + it('offers no completions when the dashboard declares none', async () => { + // The other half of the same prop: an empty scope has nothing to suggest + // either, so the two never disagree. + const user = userEvent.setup(); + renderWithMantine( + + + , + ); + + await user.type( + screen.getByPlaceholderText(/Search your events w\/ Lucene/i), + 'ServiceName:$s', + ); + + await waitFor(() => + expect( + screen.queryByText('Dashboard variables'), + ).not.toBeInTheDocument(), + ); + }); + }); }); diff --git a/packages/app/tests/e2e/components/ChartEditorComponent.ts b/packages/app/tests/e2e/components/ChartEditorComponent.ts index 85ee50e1c9..746841458c 100644 --- a/packages/app/tests/e2e/components/ChartEditorComponent.ts +++ b/packages/app/tests/e2e/components/ChartEditorComponent.ts @@ -79,6 +79,147 @@ export class ChartEditorComponent { await dismissSqlAutocomplete(this.page); } + /** + * The chart editor's root, in either place it renders: the dashboard's tile + * editor modal or the chart explorer page. Inputs that also exist outside it + * — most notably the dashboard's own search WHERE input, sitting behind the + * modal where the overlay swallows every click — must stay out of reach. + */ + private editorForm(): Locator { + return this.page.locator( + '[data-testid="tile-editor-form"], [data-testid="chart-explorer-form"]', + ); + } + + /** + * The editor renders one WHERE input per series (the series' agg condition) + * followed by the chart-level WHERE, and they share a placeholder and testid. + * `'series'` takes the first, `'chart'` the last — so `'series'` only + * addresses the first series, which is all the tests need so far. + */ + private whereInput(locator: Locator, scope: 'chart' | 'series'): Locator { + return scope === 'series' ? locator.first() : locator.last(); + } + + /** + * A whole WHERE input — its language switch, the SQL or Lucene editor, and + * anything the input renders beside them. Located from the language switch, + * which is the one part present in both languages and whichever state the + * editor is in. + */ + private whereRow(scope: 'chart' | 'series' = 'chart'): Locator { + return this.whereInput( + this.editorForm().getByTestId('where-language-switch'), + scope, + ).locator('xpath=..'); + } + + /** The warning icon a WHERE input shows about the variables it references. */ + whereVariableWarning(scope: 'chart' | 'series' = 'chart'): Locator { + return this.whereRow(scope).getByTestId('variable-validation'); + } + + /** + * What a WHERE input says about the dashboard variables its expression + * references, or '' when it flags nothing. + */ + async getWhereVariableWarning( + scope: 'chart' | 'series' = 'chart', + ): Promise { + const messages = await this.whereVariableWarning(scope).evaluateAll( + elements => + elements.map(element => element.getAttribute('aria-label') ?? ''), + ); + return messages.join(' '); + } + + /** + * Select SQL or Lucene on a WHERE input. Both inputs default to Lucene. + */ + async setWhereLanguage( + language: 'SQL' | 'Lucene', + scope: 'chart' | 'series' = 'chart', + ) { + // A completion popup left open by a prior editor can overlay the switch. + await dismissSqlAutocomplete(this.page); + const select = this.whereInput( + this.editorForm().getByTestId('where-language-switch'), + scope, + ).getByLabel('Query language'); + await select.click(); + await this.page + .getByRole('option', { name: language, exact: true }) + .click(); + } + + /** Focus a WHERE input and replace its contents with `expression`. */ + private async fillWhereEditor(expression: string, scope: 'chart' | 'series') { + // Located through the row rather than the placeholder, which CodeMirror + // drops as soon as there is content — so this can refill an input it has + // already filled once. + const editor = this.whereRow(scope).locator('.cm-content'); + await editor.click(); + await this.page.keyboard.press('ControlOrMeta+A'); + await this.page.keyboard.press('Delete'); + await this.page.keyboard.type(expression); + } + + /** + * Type a SQL WHERE clause into a WHERE input, replacing any existing + * contents. Switches the input to SQL first. + */ + async setSqlWhere(expression: string, scope: 'chart' | 'series' = 'chart') { + await this.setWhereLanguage('SQL', scope); + await this.fillWhereEditor(expression, scope); + await dismissSqlAutocomplete(this.page); + } + + /** + * Type into a WHERE input while it is in Lucene mode, where it renders as a + * plain textarea rather than CodeMirror. Leaves the suggestion dropdown open. + */ + async typeLuceneWhere(text: string, scope: 'chart' | 'series' = 'chart') { + const input = this.whereInput( + this.editorForm().getByPlaceholder(/Search your events w\/ Lucene/i), + scope, + ); + await input.click(); + await input.fill(text); + } + + /** + * Type `prefix` into a SQL WHERE input to open its autocomplete popup, and + * report what it offers plus the help panel of the highlighted suggestion. + * + * Empties the input and closes the popup before returning: the tooltip sits + * over the editor and would intercept the next interaction. + */ + async readWhereCompletions( + prefix: string, + scope: 'chart' | 'series' = 'chart', + ): Promise<{ labels: string[]; info: string }> { + await this.setWhereLanguage('SQL', scope); + await this.fillWhereEditor(prefix, scope); + + const popup = this.page.locator('.cm-tooltip-autocomplete'); + await popup.waitFor({ state: 'visible', timeout: 10000 }); + const labels = await this.sqlCompletionOptions().allInnerTexts(); + + const infoPanel = this.page.locator('.cm-completionInfo'); + const info = + (await infoPanel.count()) > 0 + ? (await infoPanel.innerText()).replace(/\s+/g, ' ').trim() + : ''; + + // The editor still has focus, so clear it from the keyboard rather than + // re-locating it: a non-empty editor no longer shows its placeholder. + await this.page.keyboard.press('ControlOrMeta+A'); + await this.page.keyboard.press('Backspace'); + await popup.waitFor({ state: 'hidden', timeout: 10000 }); + + return { labels, info }; + } + /** * Set a custom ORDER BY expression in the chart editor's ORDER BY input. * Available on the Table, Pie, and Bar display types. Clears any existing @@ -299,6 +440,20 @@ export class ChartEditorComponent { }); } + /** + * Expand the "Sample Matched Events" accordion in the preview panel. Safe to + * call when it is already open. The table only queries once expanded. + */ + async openSampleMatchedEvents() { + const control = this.page.getByRole('button', { + name: 'Sample Matched Events', + }); + await control.waitFor({ state: 'visible', timeout: 10000 }); + if ((await control.getAttribute('aria-expanded')) !== 'true') { + await control.click(); + } + } + /** CodeMirror content of the rendered "Generated SQL" preview. */ generatedSqlContent(): Locator { return this.page.getByTestId('chart-sql-preview').locator('.cm-content'); diff --git a/packages/app/tests/e2e/features/dashboard.spec.ts b/packages/app/tests/e2e/features/dashboard.spec.ts index 50b8c6d069..a7956d6ce9 100644 --- a/packages/app/tests/e2e/features/dashboard.spec.ts +++ b/packages/app/tests/e2e/features/dashboard.spec.ts @@ -1,4 +1,5 @@ import { DisplayType } from '@hyperdx/common-utils/dist/types'; +import { Locator } from '@playwright/test'; import { AlertsPage } from '../page-objects/AlertsPage'; import { DashboardPage } from '../page-objects/DashboardPage'; @@ -1470,6 +1471,21 @@ test.describe('Dashboard', { tag: ['@dashboard'] }, () => { }); }); + /** Add a variable-enabled `Service` filter exposed as `$svc`. */ + const addServiceVariable = async () => { + await dashboardPage.openEditFiltersModal(); + await dashboardPage.addFilterToDashboard( + 'Service', + DEFAULT_LOGS_SOURCE_NAME, + 'ServiceName', + undefined, + undefined, + { variableName: 'svc' }, + ); + await expect(dashboardPage.getFilterItemByName('Service')).toBeVisible(); + await dashboardPage.closeFiltersModal(); + }; + test.describe('Dashboard Variables in Raw SQL Tiles', () => { // Both `$__filter(, )` and a bare `$name` reference, so one // query exercises the macro and the plain-reference form together. @@ -1477,21 +1493,6 @@ test.describe('Dashboard', { tag: ['@dashboard'] }, () => { const VARIABLE_LINE_SQL = `SELECT toStartOfInterval(Timestamp, INTERVAL {intervalSeconds:Int64} SECOND) AS ts, count() AS count FROM default.e2e_otel_logs WHERE Timestamp >= fromUnixTimestamp64Milli({startDateMilliseconds:Int64}) AND Timestamp < fromUnixTimestamp64Milli({endDateMilliseconds:Int64}) AND $__filter(ServiceName, svc) GROUP BY ts ORDER BY ts ASC`; - /** Add a variable-enabled `Service` filter exposed as `$svc`. */ - const addServiceVariable = async () => { - await dashboardPage.openEditFiltersModal(); - await dashboardPage.addFilterToDashboard( - 'Service', - DEFAULT_LOGS_SOURCE_NAME, - 'ServiceName', - undefined, - undefined, - { variableName: 'svc' }, - ); - await expect(dashboardPage.getFilterItemByName('Service')).toBeVisible(); - await dashboardPage.closeFiltersModal(); - }; - test( 'substitutes selected variable values in the tile editor previews', { tag: '@full-stack' }, @@ -1826,6 +1827,373 @@ test.describe('Dashboard', { tag: ['@dashboard'] }, () => { ); }); + test.describe('Dashboard Variables in Chart Builder Tiles', () => { + /** + * Wait for a table tile's own query to land. Without this, a missing + * service reads as "the query excluded it" when the tile simply hasn't + * rendered yet — and a tile's first query can take a while when the whole + * spec is running in parallel. + */ + const expectTileRows = async (tile: Locator) => { + await expect(tile.locator('table tbody tr').first()).toBeVisible({ + timeout: 30000, + }); + }; + + test( + 'substitutes selected variable values in a builder tile', + { tag: '@full-stack' }, + async () => { + test.setTimeout(90000); + const chartName = `E2E Builder Variable Tile ${Date.now()}`; + + await test.step('Create a dashboard with a variable-enabled filter', async () => { + await dashboardPage.createNewDashboard(); + await addServiceVariable(); + await dashboardPage.clickFilterOption('Service', 'accounting'); + await dashboardPage.page.keyboard.press('Escape'); + }); + + await test.step('Add a builder table tile whose WHERE references the variable', async () => { + await dashboardPage.addTile(); + await expect(dashboardPage.chartEditor.nameInput).toBeVisible(); + await dashboardPage.chartEditor.waitForDataToLoad(); + await dashboardPage.chartEditor.setChartType(DisplayType.Table); + await dashboardPage.chartEditor.setChartName(chartName); + await dashboardPage.chartEditor.selectSource( + DEFAULT_LOGS_SOURCE_NAME, + ); + await dashboardPage.chartEditor.setGroupBy('ServiceName'); + await dashboardPage.chartEditor.setSqlWhere('ServiceName IN ($svc)'); + await dashboardPage.chartEditor.runQuery(false); + }); + + await test.step('Generated SQL expands the reference', async () => { + await dashboardPage.chartEditor.openGeneratedSql(); + await expect(async () => { + const sql = await dashboardPage.chartEditor.getGeneratedSqlText(); + expect(sql).toContain("ServiceName IN ('accounting')"); + expect(sql).not.toContain('$svc'); + }).toPass({ timeout: 15000 }); + }); + + await test.step('The preview table only shows the selected service', async () => { + const preview = dashboardPage.page.getByRole('dialog').first(); + await expect( + preview.getByTitle('accounting', { exact: true }), + ).toBeVisible({ timeout: 15000 }); + await expect(preview.getByTitle('ad', { exact: true })).toHaveCount( + 0, + ); + }); + + await test.step('The saved tile applies the same substitution', async () => { + await dashboardPage.saveTile(); + const tile = dashboardPage.getTiles().filter({ hasText: chartName }); + await expectTileRows(tile); + await expect( + tile.getByTitle('accounting', { exact: true }), + ).toBeVisible(); + await expect(tile.getByTitle('ad', { exact: true })).toHaveCount(0); + }); + + await test.step('Changing the selection re-renders the tile', async () => { + await dashboardPage.toggleFilterValue('Service', 'accounting'); + await dashboardPage.toggleFilterValue('Service', 'ad'); + + const tile = dashboardPage.getTiles().filter({ hasText: chartName }); + await expect(tile.getByTitle('ad', { exact: true })).toBeVisible({ + timeout: 15000, + }); + await expect( + tile.getByTitle('accounting', { exact: true }), + ).toHaveCount(0); + }); + }, + ); + + test( + 'expands the variable macros in a series agg condition', + { tag: '@full-stack' }, + async () => { + test.setTimeout(90000); + const chartName = `E2E Builder Macro Tile ${Date.now()}`; + + await test.step('Create a dashboard with a selected variable value', async () => { + await dashboardPage.createNewDashboard(); + await addServiceVariable(); + await dashboardPage.clickFilterOption('Service', 'accounting'); + await dashboardPage.page.keyboard.press('Escape'); + }); + + await test.step("Add a builder table tile using $__filter in its series' agg condition", async () => { + await dashboardPage.addTile(); + await expect(dashboardPage.chartEditor.nameInput).toBeVisible(); + await dashboardPage.chartEditor.waitForDataToLoad(); + await dashboardPage.chartEditor.setChartType(DisplayType.Table); + await dashboardPage.chartEditor.setChartName(chartName); + await dashboardPage.chartEditor.selectSource( + DEFAULT_LOGS_SOURCE_NAME, + ); + await dashboardPage.chartEditor.setGroupBy('ServiceName'); + await dashboardPage.chartEditor.setSqlWhere( + '$__filter(ServiceName, svc)', + 'series', + ); + await dashboardPage.chartEditor.runQuery(false); + }); + + await test.step('The macro expands to the selected values', async () => { + await dashboardPage.chartEditor.openGeneratedSql(); + await expect(async () => { + const sql = await dashboardPage.chartEditor.getGeneratedSqlText(); + expect(sql).toContain("IN ('accounting')"); + expect(sql).not.toContain('$__filter'); + }).toPass({ timeout: 15000 }); + }); + + await test.step('Sample Matched Events lists the rows the macro matches', async () => { + // The agg condition moves out of `select` into this preview's + // `filters`, so an unexpanded macro would reach ClickHouse verbatim + // and error instead of listing rows. + await dashboardPage.chartEditor.openSampleMatchedEvents(); + const sampleEvents = dashboardPage.page.getByTestId( + 'search-results-table', + ); + await expect( + sampleEvents.getByTestId(/^table-row-/).first(), + ).toBeVisible({ timeout: 30000 }); + await expect( + sampleEvents.getByText('accounting').first(), + ).toBeVisible(); + await expect( + dashboardPage.page.getByTestId('chart-error-state'), + ).toHaveCount(0); + }); + + await test.step('Clearing the selection keeps every row, rather than none', async () => { + // With nothing selected the macro renders a no-op predicate, so the + // tile falls back to showing every service. + await dashboardPage.saveTile(); + const tile = dashboardPage.getTiles().filter({ hasText: chartName }); + await expectTileRows(tile); + await expect( + tile.getByTitle('accounting', { exact: true }), + ).toBeVisible(); + await expect(tile.getByTitle('ad', { exact: true })).toHaveCount(0); + + await dashboardPage.toggleFilterValue('Service', 'accounting'); + await expect(tile.getByTitle('ad', { exact: true })).toBeVisible({ + timeout: 15000, + }); + await expect( + tile.getByTitle('accounting', { exact: true }), + ).toBeVisible(); + }); + }, + ); + + test( + 'autocompletes variables in the builder inputs, with their expansion', + { tag: '@full-stack' }, + async () => { + test.setTimeout(90000); + + await test.step('Open a builder tile on a dashboard with a selected variable', async () => { + await dashboardPage.createNewDashboard(); + await addServiceVariable(); + await dashboardPage.clickFilterOption('Service', 'accounting'); + await dashboardPage.page.keyboard.press('Escape'); + + await dashboardPage.addTile(); + await expect(dashboardPage.chartEditor.nameInput).toBeVisible(); + await dashboardPage.chartEditor.waitForDataToLoad(); + await dashboardPage.chartEditor.setChartType(DisplayType.Table); + await dashboardPage.chartEditor.selectSource( + DEFAULT_LOGS_SOURCE_NAME, + ); + }); + + await test.step('The Lucene WHERE offers the bare reference, and no macros', async () => { + // WHERE starts in Lucene mode, where only the bare form is valid — + // both macros expand to SQL predicates. + await dashboardPage.chartEditor.typeLuceneWhere('ServiceName:$s'); + const dropdown = dashboardPage.page.getByText('Dashboard variables'); + await expect(dropdown).toBeVisible({ timeout: 10000 }); + await expect( + dashboardPage.page.getByText('$svc', { exact: true }), + ).toBeVisible(); + await expect( + dashboardPage.page.getByText(/Expands to: \("accounting"\)/), + ).toBeVisible(); + + await dashboardPage.chartEditor.typeLuceneWhere('$__'); + await expect( + dashboardPage.page.getByText('$__filter', { exact: true }), + ).toHaveCount(0); + }); + + await test.step('The Lucene summary explains the selected values', async () => { + // "Searching for:" describes the query that will run, so it names + // the selection rather than the reference it was written with. + await dashboardPage.chartEditor.typeLuceneWhere('ServiceName:$svc'); + await expect( + dashboardPage.page.getByText(/ServiceName.*accounting/i), + ).toBeVisible({ timeout: 10000 }); + + // Leave the input empty for the SQL steps below. + await dashboardPage.chartEditor.typeLuceneWhere(''); + }); + + await test.step('A reference is offered, with what it expands to now', async () => { + const { labels, info } = + await dashboardPage.chartEditor.readWhereCompletions('$svc'); + expect(labels).toEqual( + expect.arrayContaining(['$svc', '${svc}', '${svc:csv}']), + ); + // The help describes the form and previews the current selection. + expect(info).toContain('The selected values of svc'); + expect(info).toContain("Expands to: 'accounting'"); + }); + + await test.step('The variable macros are offered, but no others', async () => { + const { labels } = + await dashboardPage.chartEditor.readWhereCompletions('$__'); + expect(labels).toEqual( + expect.arrayContaining([ + '$__filter', + '$__conditionalAll', + '$__filter(svc)', + ]), + ); + // A builder input only expands the variable macros; the raw SQL ones + // would reach ClickHouse verbatim. + expect(labels).not.toContain('$__timeFilter'); + expect(labels).not.toContain('$__sourceTable'); + }); + + await test.step("A series' agg condition offers the same completions", async () => { + const { labels, info } = + await dashboardPage.chartEditor.readWhereCompletions( + '$svc', + 'series', + ); + expect(labels).toContain('$svc'); + expect(info).toContain("Expands to: 'accounting'"); + }); + }, + ); + + test( + 'flags questionable variable references on the input that holds them', + { tag: '@full-stack' }, + async () => { + test.setTimeout(90000); + + await test.step('Open a builder tile on a dashboard with a variable', async () => { + await dashboardPage.createNewDashboard(); + await addServiceVariable(); + + await dashboardPage.addTile(); + await expect(dashboardPage.chartEditor.nameInput).toBeVisible(); + await dashboardPage.chartEditor.waitForDataToLoad(); + await dashboardPage.chartEditor.setChartType(DisplayType.Table); + await dashboardPage.chartEditor.selectSource( + DEFAULT_LOGS_SOURCE_NAME, + ); + }); + + /** + * Put `expression` in the WHERE input and assert what it says about the + * variables the expression references. Retried as a whole: CodeMirror + * occasionally drops a keystroke burst, and the check itself is + * debounced. + */ + const expectWhereWarning = async ( + expression: string, + assertWarning: (warning: string) => void, + ) => { + await expect(async () => { + await dashboardPage.chartEditor.setSqlWhere(expression); + assertWarning( + await dashboardPage.chartEditor.getWhereVariableWarning(), + ); + }).toPass({ timeout: 20000 }); + }; + + await test.step('An unknown variable is flagged, and the known ones named', async () => { + await expectWhereWarning('ServiceName IN ($srvice)', warning => { + expect(warning).toContain( + 'references unknown variable $srvice. Available variables: svc.', + ); + }); + // In the DOM is not enough — the icon shares the input's row with the + // editor itself, so it has to survive the layout. + await expect( + dashboardPage.chartEditor.whereVariableWarning(), + ).toBeVisible(); + }); + + await test.step('A bare reference is flagged for its empty state', async () => { + await expectWhereWarning('ServiceName IN ($svc)', warning => { + expect(warning).toContain('it renders as NULL'); + }); + }); + + await test.step('A quoted reference is flagged as already quoted', async () => { + await expectWhereWarning("ServiceName = '$svc'", warning => { + expect(warning).toContain('is wrapped in quotes'); + }); + }); + + await test.step('A correct $__filter usage is left alone', async () => { + await expectWhereWarning('$__filter(ServiceName, svc)', warning => { + expect(warning).toBe(''); + }); + }); + + await test.step('A Lucene reference is judged by the lucene format', async () => { + // Nothing wrong with either of these there: the format quotes each + // value itself and renders a term that drops out when unselected. + await dashboardPage.chartEditor.setWhereLanguage('Lucene'); + await dashboardPage.chartEditor.typeLuceneWhere('ServiceName:$svc'); + await expect(async () => { + expect( + await dashboardPage.chartEditor.getWhereVariableWarning(), + ).toBe(''); + }).toPass({ timeout: 10000 }); + + await dashboardPage.chartEditor.typeLuceneWhere( + 'ServiceName:$srvice', + ); + await expect(async () => { + expect( + await dashboardPage.chartEditor.getWhereVariableWarning(), + ).toContain('references unknown variable $srvice'); + }).toPass({ timeout: 10000 }); + // The Lucene input has no error affordance of its own, so the icon + // floats over the right edge of the textarea — check it is not + // clipped or covered. + await expect( + dashboardPage.chartEditor.whereVariableWarning(), + ).toBeVisible(); + + // The same macro the SQL step above accepted. Switching the language + // keeps the text, and here it is never expanded — it would be + // searched for as literal text, so it has to be called out. + await dashboardPage.chartEditor.typeLuceneWhere( + '$__filter(ServiceName, svc)', + ); + await expect(async () => { + expect( + await dashboardPage.chartEditor.getWhereVariableWarning(), + ).toContain('$__filter has no meaning in a Lucene expression'); + }).toPass({ timeout: 10000 }); + }); + }, + ); + }); + test( 'should deselect and hide the Custom aggregation function when switching to a metric source', { tag: '@full-stack' }, diff --git a/packages/common-utils/src/__tests__/queryParser.test.ts b/packages/common-utils/src/__tests__/queryParser.test.ts index ddfd1f7764..f420a2cadc 100644 --- a/packages/common-utils/src/__tests__/queryParser.test.ts +++ b/packages/common-utils/src/__tests__/queryParser.test.ts @@ -495,6 +495,20 @@ describe('CustomSchemaSQLSerializerV2 - json', () => { }, ); + // What the `lucene` variable format renders for an empty selection. It has + // to drop out of the predicate rather than match nothing, and it stays + // parenthesized for exactly that reason: the bare `ServiceName:""` below + // compares the column against the empty string instead. + it.each([ + ['ServiceName:("")', '(((1=1)))'], + ['("")', '(((1=1)))'], + ['ServiceName:""', "((ServiceName = ''))"], + ])('renders the empty lucene term %s as %s', async (lucene, expected) => { + expect(await new SearchQueryBuilder(lucene, serializer).build()).toBe( + expected, + ); + }); + it('correctly searches multi-column implicit field', async () => { const serializer = new CustomSchemaSQLSerializerV2({ metadata, diff --git a/packages/common-utils/src/__tests__/renderChartConfig.test.ts b/packages/common-utils/src/__tests__/renderChartConfig.test.ts index d2a04164b2..0e7e074497 100644 --- a/packages/common-utils/src/__tests__/renderChartConfig.test.ts +++ b/packages/common-utils/src/__tests__/renderChartConfig.test.ts @@ -3400,6 +3400,186 @@ describe('renderChartConfig', () => { }); }); + describe('dashboard variables', () => { + const configWithVariables: ChartConfigWithOptDateRange = { + displayType: DisplayType.Line, + connection: 'test-connection', + from: { databaseName: 'default', tableName: 'logs' }, + select: [ + { + aggFn: 'count', + valueExpression: '', + aggCondition: 'ServiceName IN ($service)', + aggConditionLanguage: 'sql', + }, + ], + groupBy: [{ valueExpression: 'ServiceName' }], + where: '$__filter(ServiceName, service)', + whereLanguage: 'sql', + having: 'count() > 0', + timestampValueExpression: 'timestamp', + dateRange: [new Date('2025-02-12'), new Date('2025-02-13')], + granularity: '5 minute', + variables: [{ name: 'service', values: ['api', 'web'] }], + }; + + it('expands references and variable macros in a builder config', async () => { + const sql = parameterizedQueryToSql( + await renderChartConfig(configWithVariables, mockMetadata, undefined), + ); + + expect(sql).toContain("(ServiceName IN ('api', 'web'))"); + expect(sql).toContain("countIf(ServiceName IN ('api', 'web'))"); + expect(sql).not.toContain('$service'); + expect(sql).not.toContain('$__filter'); + }); + + it('leaves references untouched when the config carries no variables', async () => { + const sql = parameterizedQueryToSql( + await renderChartConfig( + { ...configWithVariables, where: '$service', variables: undefined }, + mockMetadata, + undefined, + ), + ); + + expect(sql).toContain('$service'); + }); + + it('does not re-expand a selected value that looks like a reference', async () => { + const sql = parameterizedQueryToSql( + await renderChartConfig( + { + ...configWithVariables, + variables: [ + { name: 'service', values: ['$other'] }, + { name: 'other', values: ['nope'] }, + ], + }, + mockMetadata, + undefined, + ), + ); + + expect(sql).toContain("(ServiceName IN ('$other'))"); + expect(sql).not.toContain('nope'); + }); + + // A metric config is rewritten into CTEs by translateMetricChartConfig + // (single series) or split into one query per series (multi-series), and + // both read the config's expressions. Substitution therefore has to run + // *before* that rewriting for the expansions to reach the generated SQL at + // all — and exactly once, since the per-series branches recurse back + // through renderChartConfig. + const gaugeSeriesWithVariable = { + aggFn: 'avg' as const, + aggCondition: 'ServiceName IN ($service)', + aggConditionLanguage: 'sql' as const, + valueExpression: 'Value', + metricName: 'metric.alpha', + metricType: MetricsDataType.Gauge, + }; + + const metricConfigWithVariables: ChartConfigWithOptDateRange = { + displayType: DisplayType.Line, + connection: 'test-connection', + metricTables: { + gauge: 'otel_metrics_gauge', + histogram: 'otel_metrics_histogram', + sum: 'otel_metrics_sum', + summary: 'otel_metrics_summary', + 'exponential histogram': 'otel_metrics_exponential_histogram', + }, + from: { databaseName: 'default', tableName: '' }, + select: [gaugeSeriesWithVariable], + groupBy: [{ valueExpression: 'ServiceName' }], + where: '$__filter(ServiceName, service)', + whereLanguage: 'sql', + timestampValueExpression: 'TimeUnix', + dateRange: [new Date('2025-02-12'), new Date('2025-02-14')], + granularity: '1 minute', + variables: [{ name: 'service', values: ['api', 'web'] }], + }; + + it('expands references and macros in a single-series metric config', async () => { + const sql = parameterizedQueryToSql( + await renderChartConfig( + metricConfigWithVariables, + mockMetadata, + querySettings, + ), + ); + + // The WHERE macro and the series aggCondition, once each. + expect(sql.match(/ServiceName IN \('api', 'web'\)/g)).toHaveLength(2); + // Both land in the Source CTE's filter, which only happens when + // substitution runs before the metric translation builds that CTE — + // afterwards the CTE body is already rendered SQL text. + expect(sql).toMatch( + /FROM default\.otel_metrics_gauge[\s\S]*ServiceName IN \('api', 'web'\)[\s\S]*FROM Bucketed/, + ); + expect(sql).not.toContain('$service'); + expect(sql).not.toContain('$__filter'); + }); + + it('expands references in every branch of a multi-series metric config', async () => { + const sql = parameterizedQueryToSql( + await renderChartConfig( + { + ...metricConfigWithVariables, + select: [ + gaugeSeriesWithVariable, + { ...gaugeSeriesWithVariable, metricName: 'metric.beta' }, + { + ...gaugeSeriesWithVariable, + aggFn: 'sum', + metricName: 'metric.gamma', + metricType: MetricsDataType.Sum, + }, + ], + }, + mockMetadata, + querySettings, + ), + ); + + // Three branches (two gauge, one sum — a different physical table with + // its own CTE scaffolding), each carrying both expansions. + expect(sql.match(/UNION ALL/g)).toHaveLength(2); + expect(sql.match(/ServiceName IN \('api', 'web'\)/g)).toHaveLength(6); + expect(sql).toContain('FROM default.otel_metrics_sum'); + expect(sql).not.toContain('$service'); + expect(sql).not.toContain('$__filter'); + }); + + it('substitutes exactly once across a multi-series metric render', async () => { + const sql = parameterizedQueryToSql( + await renderChartConfig( + { + ...metricConfigWithVariables, + select: [ + gaugeSeriesWithVariable, + { ...gaugeSeriesWithVariable, metricName: 'metric.beta' }, + ], + // The per-series branches recurse through renderChartConfig, so a + // value that itself looks like a reference must not be expanded a + // second time on the way down. + variables: [ + { name: 'service', values: ['$other'] }, + { name: 'other', values: ['nope'] }, + ], + }, + mockMetadata, + querySettings, + ), + ); + + // Two branches × (WHERE macro + aggCondition), all left as written. + expect(sql.match(/ServiceName IN \('\$other'\)/g)).toHaveLength(4); + expect(sql).not.toContain('nope'); + }); + }); + // HDX-4371: a source with `timestampValueExpression = "EventDate, EventTime"` // should bucket on `EventTime` (the DateTime token), not on `EventDate` // (the partition-key Date). The WHERE clause keeps using both columns so diff --git a/packages/common-utils/src/__tests__/variables.test.ts b/packages/common-utils/src/__tests__/variables.test.ts index 7c2c324445..66cacd4a86 100644 --- a/packages/common-utils/src/__tests__/variables.test.ts +++ b/packages/common-utils/src/__tests__/variables.test.ts @@ -1,12 +1,14 @@ import { MalformedMacroArgsError } from '@/macroErrors'; -import type { ChartVariable } from '@/types'; +import type { BuilderChartConfig, ChartVariable } from '@/types'; import { filterReferencedVariables, formatVariableValues, getReferencedVariableNames, getVariableReferences, hasVariableMacro, + substituteChartConfigVariables, substituteVariables, + validateVariableReferencesInTemplate, } from '@/variables'; const variable = ( @@ -18,6 +20,18 @@ const variable = ( const SERVICE = variable('service', ['api', 'web'], 'ServiceName'); const EMPTY_SERVICE = variable('service', [], 'ServiceName'); +const builderConfig = ( + overrides: Partial = {}, +): BuilderChartConfig => ({ + select: 'count()', + from: { databaseName: 'default', tableName: 'logs' }, + where: '', + whereLanguage: 'sql', + timestampValueExpression: 'Timestamp', + connection: 'local', + ...overrides, +}); + describe('formatVariableValues', () => { describe('sqlstring', () => { it('renders NULL when nothing is selected', () => { @@ -72,8 +86,10 @@ describe('formatVariableValues', () => { }); describe('lucene', () => { - it('renders a match-all wildcard when nothing is selected', () => { - expect(formatVariableValues([], 'lucene')).toBe('*'); + it('renders an empty term when nothing is selected', () => { + // Parenthesized so it stays a no-op in a field-scoped position; see the + // queryParser test that pins `ServiceName:("")` to `1=1`. + expect(formatVariableValues([], 'lucene')).toBe('("")'); }); it('renders a single quoted term', () => { @@ -671,19 +687,334 @@ describe('filterReferencedVariables', () => { ).toEqual([]); }); - it('returns an empty array for a builder config even when its fields mention a variable', () => { + it('keeps the variables a builder config references, across every expression field', () => { expect( filterReferencedVariables( - { - select: 'count()', - from: { databaseName: 'default', tableName: 'logs' }, - where: 'ServiceName = $service', - whereLanguage: 'sql', - timestampValueExpression: 'Timestamp', - connection: 'local', - }, + builderConfig({ + select: [ + { aggFn: 'count', valueExpression: '', aggCondition: '$service' }, + ], + where: '', + having: 'count() > 0', + groupBy: [{ valueExpression: '$__filter(RegionName, region)' }], + orderBy: [{ valueExpression: '$env', ordering: 'DESC' }], + }), + variables, + ), + ).toEqual(variables); + }); + + it('returns an empty array when a builder config references none of them', () => { + expect( + filterReferencedVariables( + builderConfig({ where: 'ServiceName = $nope' }), variables, ), ).toEqual([]); }); }); + +describe('substituteChartConfigVariables', () => { + it('returns the config untouched when there is no variable context', () => { + const config = builderConfig({ where: 'ServiceName = $service' }); + expect(substituteChartConfigVariables(config)).toBe(config); + }); + + it('expands references in where and having, and consumes the variables', () => { + expect( + substituteChartConfigVariables( + builderConfig({ + where: 'ServiceName IN ($service)', + having: 'anyLast(Env) = $env', + variables: [SERVICE, variable('env', ['prod'])], + }), + ), + ).toMatchObject({ + where: "ServiceName IN ('api', 'web')", + having: "anyLast(Env) = 'prod'", + variables: undefined, + }); + }); + + it('expands a lucene where clause using the lucene format', () => { + expect( + substituteChartConfigVariables( + builderConfig({ + where: 'ServiceName:$service', + whereLanguage: 'lucene', + variables: [SERVICE], + }), + ).where, + ).toBe('ServiceName:("api" OR "web")'); + }); + + it('renders an empty lucene selection as a term that drops out', () => { + expect( + substituteChartConfigVariables( + builderConfig({ + where: 'ServiceName:$service', + whereLanguage: 'lucene', + variables: [EMPTY_SERVICE], + }), + ).where, + ).toBe('ServiceName:("")'); + }); + + it('leaves the variable macros alone in a lucene expression', () => { + // They expand to SQL, which a Lucene parser cannot read, so they are not + // supported there — and an unknown variable must not throw either. + const template = + '$__filter(ServiceName, service) $__conditionalAll(a, foo)'; + expect( + substituteChartConfigVariables( + builderConfig({ + where: template, + whereLanguage: 'lucene', + variables: [SERVICE], + }), + ).where, + ).toBe(template); + }); + + it('still expands the macros in a lucene chart’s SQL-language fields', () => { + // whereLanguage only governs `where`; `having` is SQL regardless. + expect( + substituteChartConfigVariables( + builderConfig({ + where: '', + whereLanguage: 'lucene', + having: '$__filter(ServiceName, service)', + variables: [SERVICE], + }), + ).having, + ).toBe("(ServiceName IN ('api', 'web'))"); + }); + + it('expands select value expressions and agg conditions', () => { + expect( + substituteChartConfigVariables( + builderConfig({ + select: [ + { + aggFn: 'count', + valueExpression: '', + // aggCondition defaults to lucene, like the renderer + aggCondition: 'ServiceName:$service', + }, + { + valueExpression: 'countIf(ServiceName IN ($service))', + }, + ], + variables: [SERVICE], + }), + ).select, + ).toEqual([ + { + aggFn: 'count', + valueExpression: '', + aggCondition: 'ServiceName:("api" OR "web")', + }, + { valueExpression: "countIf(ServiceName IN ('api', 'web'))" }, + ]); + }); + + it('expands group by and order by, in both their string and list forms', () => { + expect( + substituteChartConfigVariables( + builderConfig({ + groupBy: '$__conditionalAll(ServiceName, service)', + orderBy: [{ valueExpression: '$service', ordering: 'ASC' }], + variables: [SERVICE], + }), + ), + ).toMatchObject({ + groupBy: '(ServiceName)', + orderBy: [{ valueExpression: "'api', 'web'", ordering: 'ASC' }], + }); + }); + + it('leaves the non-variable macros alone', () => { + expect( + substituteChartConfigVariables( + builderConfig({ + where: '$__timeFilter(Timestamp) AND ServiceName IN ($service)', + variables: [SERVICE], + }), + ).where, + ).toBe("$__timeFilter(Timestamp) AND ServiceName IN ('api', 'web')"); + }); + + it('never re-scans an expansion, so a selected value cannot inject a reference', () => { + expect( + substituteChartConfigVariables( + builderConfig({ + where: 'ServiceName IN ($service)', + variables: [variable('service', ['$env']), variable('env', ['prod'])], + }), + ).where, + ).toBe("ServiceName IN ('$env')"); + }); + + it('renders an empty selection so the query stays valid', () => { + expect( + substituteChartConfigVariables( + builderConfig({ + where: '$__filter(ServiceName, service)', + variables: [EMPTY_SERVICE], + }), + ).where, + ).toBe("(1=1 /** no values selected for variable 'service' */)"); + }); +}); + +describe('validateVariableReferencesInTemplate', () => { + const validate = validateVariableReferencesInTemplate; + + it('says nothing about an expression with no references', () => { + expect(validate("ServiceName = 'api'", [SERVICE])).toEqual({ + errors: [], + warnings: [], + }); + }); + + it('warns about a reference to a variable that does not exist', () => { + const { errors, warnings } = validate('ServiceName IN ($srvice)', [ + SERVICE, + variable('env', ['prod']), + ]); + + expect(errors).toEqual([]); + expect(warnings).toEqual([ + 'SQL references unknown variable $srvice. Available variables: service, env.', + ]); + }); + + it('lists the available variables as (none) when a dashboard declares none', () => { + expect(validate('ServiceName IN ($service)', []).warnings).toEqual([ + 'SQL references unknown variable $service. Available variables: (none).', + ]); + }); + + it('names each unknown reference once, as written', () => { + expect(validate('$a = ${a} AND ${b:csv} = 1', [SERVICE]).warnings).toEqual([ + 'SQL references unknown variable $a, ${a}, ${b:csv}. Available variables: service.', + ]); + }); + + it('takes the sentence subject from the caller', () => { + expect( + validate('ServiceName IN ($srvice)', [SERVICE], { + subject: 'This expression', + }).warnings, + ).toEqual([ + 'This expression references unknown variable $srvice. Available variables: service.', + ]); + }); + + it('errors when a sqlstring reference is wrapped in quotes', () => { + const { errors } = validate("ServiceName = '$service'", [SERVICE]); + + expect(errors).toEqual([ + '$service is wrapped in quotes, but the default sqlstring format already quotes each value. Did you mean to use $__filter(, service) or ${service:csv} instead?', + ]); + }); + + it('warns that a bare reference renders as NULL before anything is selected', () => { + const { errors, warnings } = validate('ServiceName IN ($service)', [ + SERVICE, + ]); + + expect(errors).toEqual([]); + expect(warnings).toEqual([ + '$service has no valid empty-selection value — it renders as NULL before anything is selected. Prefer $__filter(, service) or $__conditionalAll(, service) so the query stays valid when no values are selected.', + ]); + }); + + it('accepts a reference guarded by its own variable macro', () => { + expect( + validate('$__filter(ServiceName IN ($service), service)', [SERVICE]), + ).toEqual({ errors: [], warnings: [] }); + }); + + it('accepts a format that has a valid empty state', () => { + expect(validate('match(ServiceName, ${service:regex})', [SERVICE])).toEqual( + { errors: [], warnings: [] }, + ); + }); + + describe('with no variable context at all', () => { + it('errors on a macro, which can only have been meant as one', () => { + expect(validate('$__filter(ServiceName, service)', undefined)).toEqual({ + errors: ['SQL uses $__filter, but no variables are available here.'], + warnings: [], + }); + }); + + it('only warns on a value reference, which may be literal text', () => { + expect(validate('ServiceName IN ($service)', undefined)).toEqual({ + errors: [], + warnings: [ + 'SQL references $service, but no variables are available here.', + ], + }); + }); + }); + + describe('a Lucene expression', () => { + it('still warns about a reference to a variable that does not exist', () => { + expect( + validate('ServiceName:$srvice', [SERVICE], { language: 'lucene' }) + .warnings, + ).toEqual([ + 'SQL references unknown variable $srvice. Available variables: service.', + ]); + }); + + it('accepts a bare reference: the lucene format has a valid empty state', () => { + expect( + validate('ServiceName:$service', [SERVICE], { language: 'lucene' }), + ).toEqual({ errors: [], warnings: [] }); + }); + + it('accepts a quoted reference: the lucene format quotes each value', () => { + expect( + validate('ServiceName:"$service"', [SERVICE], { language: 'lucene' }), + ).toEqual({ errors: [], warnings: [] }); + }); + + // The macros are never expanded here, so nothing downstream would say so. + it.each([ + '$__filter(ServiceName, service)', + '$__conditionalAll(ServiceName = 1, service)', + ])('errors on the macro %s, which is left as literal text', template => { + expect(validate(template, [SERVICE], { language: 'lucene' })).toEqual({ + errors: [ + `${template.slice(0, template.indexOf('('))} has no meaning in a Lucene expression — ` + + 'it is left as written and matched as literal text. Switch this input to SQL, ' + + 'or reference the variable directly, as in :$service.', + ], + warnings: [], + }); + }); + + it('reports a macro naming an unknown variable the same way', () => { + expect( + validate('$__filter(ServiceName, srvice)', [SERVICE], { + language: 'lucene', + }).errors, + ).toEqual([ + '$__filter has no meaning in a Lucene expression — it is left as written ' + + 'and matched as literal text. Switch this input to SQL, or reference the ' + + 'variable directly, as in :$srvice.', + ]); + }); + + it('leaves the same macro alone in a SQL expression, where it expands', () => { + expect( + validate('$__filter(ServiceName, service)', [SERVICE], { + language: 'sql', + }), + ).toEqual({ errors: [], warnings: [] }); + }); + }); +}); diff --git a/packages/common-utils/src/core/renderChartConfig.ts b/packages/common-utils/src/core/renderChartConfig.ts index 983cc61ea9..1c35ca8c23 100644 --- a/packages/common-utils/src/core/renderChartConfig.ts +++ b/packages/common-utils/src/core/renderChartConfig.ts @@ -53,6 +53,7 @@ import { SqlAstFilter, SQLInterval, } from '@/types'; +import { substituteChartConfigVariables } from '@/variables'; /** * Helper function to create a MetricName filter condition. @@ -2486,17 +2487,21 @@ export async function renderChartConfig( return renderRawSqlChartConfig(rawChartConfig, metadata); } + // Expand dashboard variables before anything reads the config's expressions, + // so metric translation and the CTEs it builds all see final SQL fragments. + const substitutedChartConfig = substituteChartConfigVariables(rawChartConfig); + // A metric chart with multiple series composes one query per series into a // single UNION ALL + pivot statement (each metric type needs its own CTE // scaffolding, and different types read different physical tables). The // per-series branches recurse through this function with a single select. if ( - isMetricChartConfig(rawChartConfig) && - Array.isArray(rawChartConfig.select) && - rawChartConfig.select.length > 1 + isMetricChartConfig(substitutedChartConfig) && + Array.isArray(substitutedChartConfig.select) && + substitutedChartConfig.select.length > 1 ) { return renderMultiSeriesMetricChartConfig( - rawChartConfig, + substitutedChartConfig, metadata, querySettings, ); @@ -2504,9 +2509,9 @@ export async function renderChartConfig( // metric types require more rewriting since we know more about the schema // but goes through the same generation process - const translatedChartConfig = isMetricChartConfig(rawChartConfig) - ? await translateMetricChartConfig(rawChartConfig, metadata) - : rawChartConfig; + const translatedChartConfig = isMetricChartConfig(substitutedChartConfig) + ? await translateMetricChartConfig(substitutedChartConfig, metadata) + : substitutedChartConfig; // Resolve the bucket column once for the whole render. A source with // `timestampValueExpression = "EventDate, EventTime"` should bucket on diff --git a/packages/common-utils/src/core/utils.ts b/packages/common-utils/src/core/utils.ts index 6c4a1bb2be..0ac541784f 100644 --- a/packages/common-utils/src/core/utils.ts +++ b/packages/common-utils/src/core/utils.ts @@ -36,12 +36,7 @@ import { TileTemplateSchema, TSource, } from '@/types'; -import { - getVariableReferences, - VARIABLE_FORMATS, - VariableFormat, - VariableReference, -} from '@/variables'; +import { validateVariableReferencesInTemplate } from '@/variables'; import { SkipIndexMetadata, TableMetadata } from './metadata'; @@ -1465,105 +1460,6 @@ export function validateRawSqlForAlert(chartConfig: RawSqlChartConfig): { return { errors, warnings }; } -/** `$a, $b` — deduplicated, in source order, for use in a message. */ -function formatReferenceList(references: VariableReference[]): string { - return [...new Set(references.map(reference => reference.raw))].join(', '); -} - -const isKnownFormat = (format: string): format is VariableFormat => - (VARIABLE_FORMATS as readonly string[]).includes(format); - -/** - * Checks on the dashboard variables a raw SQL template references. - * - * `chartConfig.variables` is tri-state and each state means something - * different here: `undefined` is "no variable context" (the chart explorer, or - * a dashboard with the feature flag off) where nothing is substituted at all; - * `[]` is a dashboard whose filters expose no variables. - * - * Only *value* references (`$name`, `${name}`, `${name:format}`) are inspected. - * The macro forms either expand correctly or throw, and those messages reach - * the user through `resolveRawSqlMacros` — except when there is no context at - * all, in which case they silently pass through and are reported here. - */ -function validateVariableReferences(chartConfig: RawSqlChartConfig): { - errors: string[]; - warnings: string[]; -} { - const errors: string[] = []; - const warnings: string[] = []; - - const references = getVariableReferences(chartConfig.sqlTemplate); - if (references.length === 0) return { errors, warnings }; - - const macroReferences = references.filter(r => r.kind === 'macro'); - const valueReferences = references.filter(r => r.kind !== 'macro'); - const { variables } = chartConfig; - - // Variables are not available on chart explorer (and maybe other contexts) - if (variables == null) { - // Macros are always an error, we assume no query will intentionally include them without variable context. - if (macroReferences.length > 0) { - errors.push( - `SQL uses ${formatReferenceList(macroReferences)}, but no variables are available here.`, - ); - } - - // $var and ${var} references only trigger a warning since they may be a literal the user means to keep. - if (valueReferences.length > 0) { - warnings.push( - `SQL references ${formatReferenceList(valueReferences)}, but no variables are available here.`, - ); - } - return { errors, warnings }; - } - - const knownVariableNames = new Set(variables.map(variable => variable.name)); - const available = - variables.length > 0 - ? variables.map(variable => variable.name).join(', ') - : '(none)'; - - const unknown = valueReferences.filter(r => !knownVariableNames.has(r.name)); - if (unknown.length > 0) { - warnings.push( - `SQL references unknown variable ${formatReferenceList(unknown)}. Available variables: ${available}.`, - ); - } - - // An unrecognized format throws during expansion, so it is already reported. - const resolved = valueReferences.filter( - r => - knownVariableNames.has(r.name) && - (r.format == null || isKnownFormat(r.format)), - ); - - const quoted = resolved.filter( - r => (r.format ?? 'sqlstring') === 'sqlstring' && r.inStringLiteral, - ); - if (quoted.length > 0) { - const [{ name }] = quoted; - errors.push( - `${formatReferenceList(quoted)} is wrapped in quotes, but the default sqlstring format already quotes each value. Did you mean to use $__filter(, ${name}) or \${${name}:csv} instead?`, - ); - } - - const unguarded = resolved.filter( - r => - (r.format ?? 'sqlstring') === 'sqlstring' && - !r.inStringLiteral && - r.guardedBy !== r.name, - ); - if (unguarded.length > 0) { - const [{ name }] = unguarded; - warnings.push( - `${formatReferenceList(unguarded)} has no valid empty-selection value — it renders as NULL before anything is selected. Prefer $__filter(, ${name}) or $__conditionalAll(, ${name}) so the query stays valid when no values are selected.`, - ); - } - - return { errors, warnings }; -} - /** * General-purpose raw SQL chart validation, surfaced in the chart editor * regardless of whether an alert is configured. @@ -1609,7 +1505,11 @@ export function validateRawSqlChartConfig( } } - const variableIssues = validateVariableReferences(chartConfig); + const variableIssues = validateVariableReferencesInTemplate( + chartConfig.sqlTemplate, + chartConfig.variables, + { subject: 'SQL', language: 'sql' }, + ); errors.push(...variableIssues.errors); warnings.push(...variableIssues.warnings); diff --git a/packages/common-utils/src/types.ts b/packages/common-utils/src/types.ts index c737524f81..99d74b00ea 100644 --- a/packages/common-utils/src/types.ts +++ b/packages/common-utils/src/types.ts @@ -1419,7 +1419,12 @@ export const WithClauseSchema = z.object({ // ensure the type system can catch more issues in the build pipeline. const BuilderChartConfigSchema = z.intersection( z.intersection(_ChartConfigSchema, SelectSQLStatementSchema), - z.object({ with: z.array(WithClauseSchema) }).partial(), + z + .object({ + with: z.array(WithClauseSchema), + variables: z.array(ChartVariableSchema), + }) + .partial(), ); export type BuilderChartConfig = z.infer; diff --git a/packages/common-utils/src/variables.ts b/packages/common-utils/src/variables.ts index c4467ec0a2..0531a8b95b 100644 --- a/packages/common-utils/src/variables.ts +++ b/packages/common-utils/src/variables.ts @@ -10,6 +10,9 @@ import { DASHBOARD_VARIABLE_NAME_PATTERN, DASHBOARD_VARIABLE_NAME_PATTERN_ANCHORED, SavedChartConfig, + SearchConditionLanguage, + SelectList, + SortSpecificationList, } from './types'; /** Rendering formats a reference can request via `${name:format}`. */ @@ -53,7 +56,7 @@ export function formatVariableValues( return values.join(','); case 'lucene': return values.length === 0 - ? '*' + ? '("")' : `(${values.map(value => `"${escapeLuceneValue(value)}"`).join(' OR ')})`; default: format satisfies never; // Unreachable @@ -316,6 +319,13 @@ export type VariableContext = { variables: ChartVariable[]; /** Format used by references that don't request one. */ defaultFormat: VariableFormat; + /** + * When true, `$__filter` and `$__conditionalAll` are left exactly as + * written. They expand to SQL predicates, so they have no meaning in a + * Lucene expression — expanding one there would splice SQL into a query + * that is about to be parsed as Lucene. + */ + disableMacros?: boolean; }; const sqlNoOp = (name: string) => @@ -436,7 +446,10 @@ export function expandVariableToken( } function substituteWithContext(input: string, ctx: VariableContext): string { - return scanTemplateTokens(input, VARIABLE_MACRO_NAMES) + // Unregistering the macro names is what leaves them verbatim: the scanner + // emits an unknown `$__x` as plain text, arguments and all. + const macroNames = ctx.disableMacros ? [] : VARIABLE_MACRO_NAMES; + return scanTemplateTokens(input, macroNames) .map(token => token.kind === 'text' ? token.text : expandVariableToken(token, ctx), ) @@ -454,9 +467,148 @@ function substituteWithContext(input: string, ctx: VariableContext): string { export function substituteVariables( input: string, variables: ChartVariable[], - { defaultFormat = 'sqlstring' }: { defaultFormat?: VariableFormat } = {}, + { + defaultFormat = 'sqlstring', + disableMacros, + }: { defaultFormat?: VariableFormat; disableMacros?: boolean } = {}, ): string { - return substituteWithContext(input, { variables, defaultFormat }); + return substituteWithContext(input, { + variables, + defaultFormat, + disableMacros, + }); +} + +// -- Chart builder configs -------------------------------------------------- + +/** + * The chart builder fields whose expressions may reference variables. Kept + * structural rather than tied to one config type so both runtime configs (which + * carry `variables`) and saved configs (which don't) can be walked. + */ +type BuilderVariableFields = { + select: SelectList; + where?: string; + whereLanguage?: SearchConditionLanguage; + having?: string; + havingLanguage?: SearchConditionLanguage; + groupBy?: SelectList; + orderBy?: SortSpecificationList; +}; + +/** + * Rewrites the given template. `language` is the language the renderer will parse + * that expression as, so a reference can be expanded in a matching format. + */ +type TemplateMapper = ( + template: string, + language: SearchConditionLanguage, +) => string; + +const mapSelectList = (list: SelectList, map: TemplateMapper): SelectList => + typeof list === 'string' + ? map(list, 'sql') + : list.map(column => ({ + ...column, + valueExpression: map( + column.valueExpression, + column.valueExpressionLanguage ?? 'sql', + ), + // Left absent when absent: the select union makes `aggCondition` + // required for some aggregations and optional for others. + ...(column.aggCondition + ? { + aggCondition: map( + column.aggCondition, + column.aggConditionLanguage ?? 'lucene', + ), + } + : {}), + })); + +const mapSortList = ( + list: SortSpecificationList, + map: TemplateMapper, +): SortSpecificationList => + typeof list === 'string' + ? map(list, 'sql') + : list.map(spec => ({ + ...spec, + // `renderSortSpecificationList` always renders items as SQL + valueExpression: map(spec.valueExpression, 'sql'), + })); + +/** + * Calls the given map function to rewrite every chart builder expression + * that may contain variable references, leaving the rest of the config untouched. + */ +function mapBuilderVariableTemplates( + config: T, + map: TemplateMapper, +): T { + return { + ...config, + select: mapSelectList(config.select, map), + ...(config.where + ? { where: map(config.where, config.whereLanguage ?? 'sql') } + : {}), + ...(config.having + ? { having: map(config.having, config.havingLanguage ?? 'sql') } + : {}), + ...(config.groupBy != null + ? { groupBy: mapSelectList(config.groupBy, map) } + : {}), + ...(config.orderBy != null + ? { orderBy: mapSortList(config.orderBy, map) } + : {}), + }; +} + +/** + * Expand variable references and the variable macros throughout a chart builder + * config, returning the config with `variables` consumed. `variables` being + * undefined means this is a no-op. + * + * Each expression is expanded for the language it will be parsed as. A Lucene + * expression renders values in the `lucene` format and gets no macros. + * + * Dropping `variables` from the result ensures that variables are never substituted + * twice, even when the config is passed through `substituteChartConfigVariables` + * recursively (eg. for CTEs or Metrics). + */ +export function substituteChartConfigVariables< + T extends BuilderVariableFields & { variables?: ChartVariable[] }, +>(config: T): T { + const { variables } = config; + if (variables == null) return config; + + const substituted = mapBuilderVariableTemplates( + config, + (template, language) => { + const isLucene = language === 'lucene'; + return substituteVariables(template, variables, { + defaultFormat: isLucene ? 'lucene' : 'sqlstring', + disableMacros: isLucene, + }); + }, + ); + + return { ...substituted, variables: undefined }; +} + +/** + * Every variable reference across a chart builder config's expressions. + * Never throws: it runs over saved configs that may be mid-edit or malformed. + */ +function getBuilderVariableReferences( + config: BuilderVariableFields, +): VariableReference[] { + const references: VariableReference[] = []; + mapBuilderVariableTemplates(config, template => { + references.push(...getVariableReferences(template)); + return template; + }); + return references; } /** One occurrence of a variable reference in a template. */ @@ -566,6 +718,125 @@ export function getVariableReferences(input: string): VariableReference[] { return references; } +/** `$a, $b` — deduplicated, in source order, for use in a message. */ +function formatReferenceList(references: VariableReference[]): string { + return [...new Set(references.map(reference => reference.raw))].join(', '); +} + +export type VariableReferenceIssues = { errors: string[]; warnings: string[] }; + +/** + * Checks on the dashboard variables a single expression references. + * + * `variables` is tri-state and each state means something different here: + * `undefined` is "no variable context" (the chart explorer, or a dashboard with + * the feature flag off) where nothing is substituted at all; `[]` is a dashboard + * whose filters expose no variables. + * + * Only *value* references (`$name`, `${name}`, `${name:format}`) are inspected. + * The macro forms either expand correctly or throw, and those messages reach + * the user through expansion — except in the two cases where expansion never + * runs and they would pass through silently: no context at all, and a Lucene + * expression. Both are reported here. + */ +export function validateVariableReferencesInTemplate( + template: string, + variables: ChartVariable[] | undefined, + { + subject = 'SQL', + language = 'sql', + }: { + /** The sentence subject of each message, e.g. `SQL references ...`. */ + subject?: string; + /** The language the renderer parses this template as. */ + language?: SearchConditionLanguage; + } = {}, +): VariableReferenceIssues { + const errors: string[] = []; + const warnings: string[] = []; + + const references = getVariableReferences(template); + if (references.length === 0) return { errors, warnings }; + + const macroReferences = references.filter(r => r.kind === 'macro'); + const valueReferences = references.filter(r => r.kind !== 'macro'); + + // Variables are not available on chart explorer (and maybe other contexts) + if (variables == null) { + // Macros are always an error, we assume no query will intentionally include them without variable context. + if (macroReferences.length > 0) { + errors.push( + `${subject} uses ${formatReferenceList(macroReferences)}, but no variables are available here.`, + ); + } + + // $var and ${var} references only trigger a warning since they may be a literal the user means to keep. + if (valueReferences.length > 0) { + warnings.push( + `${subject} references ${formatReferenceList(valueReferences)}, but no variables are available here.`, + ); + } + return { errors, warnings }; + } + + const knownVariableNames = new Set(variables.map(variable => variable.name)); + const available = + variables.length > 0 + ? variables.map(variable => variable.name).join(', ') + : '(none)'; + + const unknown = valueReferences.filter(r => !knownVariableNames.has(r.name)); + if (unknown.length > 0) { + warnings.push( + `${subject} references unknown variable ${formatReferenceList(unknown)}. Available variables: ${available}.`, + ); + } + + // Macros are not supported in lucene + if (language === 'lucene') { + if (macroReferences.length > 0) { + const [{ name }] = macroReferences; + errors.push( + `${formatReferenceList(macroReferences)} has no meaning in a Lucene expression — it is left as written and matched as literal text. Switch this input to SQL, or reference the variable directly, as in :$${name}.`, + ); + } + // The two checks below are specific to the `sqlstring` default format. + return { errors, warnings }; + } + + // An unrecognized format throws during expansion, so it is already reported. + const resolved = valueReferences.filter( + r => + knownVariableNames.has(r.name) && + (r.format == null || isVariableFormat(r.format)), + ); + + const quoted = resolved.filter( + r => (r.format ?? 'sqlstring') === 'sqlstring' && r.inStringLiteral, + ); + if (quoted.length > 0) { + const [{ name }] = quoted; + errors.push( + `${formatReferenceList(quoted)} is wrapped in quotes, but the default sqlstring format already quotes each value. Did you mean to use $__filter(, ${name}) or \${${name}:csv} instead?`, + ); + } + + const unguarded = resolved.filter( + r => + (r.format ?? 'sqlstring') === 'sqlstring' && + !r.inStringLiteral && + r.guardedBy !== r.name, + ); + if (unguarded.length > 0) { + const [{ name }] = unguarded; + warnings.push( + `${formatReferenceList(unguarded)} has no valid empty-selection value — it renders as NULL before anything is selected. Prefer $__filter(, ${name}) or $__conditionalAll(, ${name}) so the query stays valid when no values are selected.`, + ); + } + + return { errors, warnings }; +} + /** * Returns the names of every variable the template could reference. * Never throws: it runs over saved SQL that may be mid-edit or malformed. @@ -584,15 +855,28 @@ export function hasVariableMacro(input: string): boolean { }).some(token => token.kind === 'macro'); } -/** Returns the subset of `variables` that `config` actually references. */ +/** + * Returns the subset of `variables` that `config` actually references. + * + * Keying a tile's query on only these keeps it from re-running when an + * unrelated variable's selection changes. + */ export function filterReferencedVariables( config: ChartConfigWithOptDateRange | SavedChartConfig, variables: ChartVariable[], ): ChartVariable[] { - // Only Raw SQL configs can reference variables today. - if (!('configType' in config) || config.configType !== 'sql') { + let names: string[]; + if ('configType' in config && config.configType === 'sql') { + names = getReferencedVariableNames(config.sqlTemplate); + } else if ('configType' in config && config.configType === 'promql') { + // PromQL queries don't support variables yet. return []; + } else { + names = getBuilderVariableReferences(config).map( + reference => reference.name, + ); } - const referenced = new Set(getReferencedVariableNames(config.sqlTemplate)); + + const referenced = new Set(names); return variables.filter(variable => referenced.has(variable.name)); }