From e1edec02d01e5b0ca34972d6d040f84ded91925f Mon Sep 17 00:00:00 2001 From: Sergej-Vlasov <37613182+Sergej-Vlasov@users.noreply.github.com> Date: Wed, 21 Feb 2024 17:16:57 +0200 Subject: [PATCH] DashboardScene: Validate variable name in scenes variable editor (#82415) * add variable name validation * adjust variable name validation logic * move variable name validation logic to model * add tests for onValidateVariableName * extract variable name validation itest into separate describe --- .../settings/VariablesEditView.test.tsx | 44 +++++++++++++++++++ .../settings/VariablesEditView.tsx | 42 +++++++++++++++++- .../settings/variables/VariableEditorForm.tsx | 36 ++++++++++++--- .../settings/variables/utils.ts | 3 ++ 4 files changed, 117 insertions(+), 8 deletions(-) diff --git a/public/app/features/dashboard-scene/settings/VariablesEditView.test.tsx b/public/app/features/dashboard-scene/settings/VariablesEditView.test.tsx index 8f67633d5a5..02d0232e89d 100644 --- a/public/app/features/dashboard-scene/settings/VariablesEditView.test.tsx +++ b/public/app/features/dashboard-scene/settings/VariablesEditView.test.tsx @@ -17,6 +17,7 @@ import { SceneGridLayout, VizPanel, AdHocFiltersVariable, + SceneVariableState, } from '@grafana/scenes'; import { mockDataSource } from 'app/features/alerting/unified/mocks'; import { LegacyVariableQueryEditor } from 'app/features/variables/editor/LegacyVariableQueryEditor'; @@ -206,6 +207,49 @@ describe('VariablesEditView', () => { }); }); + describe('Variables name validation', () => { + let variableView: VariablesEditView; + let variable1: SceneVariableState; + let variable2: SceneVariableState; + + beforeAll(async () => { + const result = await buildTestScene(); + variableView = result.variableView; + + const variables = variableView.getVariables(); + variable1 = variables[0].state; + variable2 = variables[1].state; + }); + + it('should not return error on same name and key', () => { + expect(variableView.onValidateVariableName(variable1.name, variable1.key)[0]).toBe(false); + }); + + it('should not return error if name is unique', () => { + expect(variableView.onValidateVariableName('unique_variable_name', variable1.key)[0]).toBe(false); + }); + + it('should return error if global variable name is used', () => { + expect(variableView.onValidateVariableName('__', variable1.key)[0]).toBe(true); + }); + + it('should not return error if global variable name is used not at the beginning ', () => { + expect(variableView.onValidateVariableName('test__', variable1.key)[0]).toBe(false); + }); + + it('should return error if name is empty', () => { + expect(variableView.onValidateVariableName('', variable1.key)[0]).toBe(true); + }); + + it('should return error if non word characters are used', () => { + expect(variableView.onValidateVariableName('-', variable1.key)[0]).toBe(true); + }); + + it('should return error if variable name is taken', () => { + expect(variableView.onValidateVariableName(variable2.name, variable1.key)[0]).toBe(true); + }); + }); + describe('Dashboard Variables dependencies', () => { let variableView: VariablesEditView; let dashboard: DashboardScene; diff --git a/public/app/features/dashboard-scene/settings/VariablesEditView.tsx b/public/app/features/dashboard-scene/settings/VariablesEditView.tsx index 84fe03a8f93..c6c97d9655b 100644 --- a/public/app/features/dashboard-scene/settings/VariablesEditView.tsx +++ b/public/app/features/dashboard-scene/settings/VariablesEditView.tsx @@ -12,7 +12,13 @@ import { EditListViewSceneUrlSync } from './EditListViewSceneUrlSync'; import { DashboardEditView, DashboardEditViewState, useDashboardEditPageNav } from './utils'; import { VariableEditorForm } from './variables/VariableEditorForm'; import { VariableEditorList } from './variables/VariableEditorList'; -import { EditableVariableType, getVariableDefault, getVariableScene } from './variables/utils'; +import { + EditableVariableType, + RESERVED_GLOBAL_VARIABLE_NAME_REGEX, + WORD_CHARACTERS_REGEX, + getVariableDefault, + getVariableScene, +} from './variables/utils'; export interface VariablesEditViewState extends DashboardEditViewState { editIndex?: number | undefined; } @@ -168,6 +174,29 @@ export class VariablesEditView extends SceneObjectBase i public onGoBack = () => { this.setState({ editIndex: undefined }); }; + + public onValidateVariableName = (name: string, key: string | undefined): [true, string] | [false, null] => { + let errorText = null; + if (!RESERVED_GLOBAL_VARIABLE_NAME_REGEX.test(name)) { + errorText = "Template names cannot begin with '__', that's reserved for Grafana's global variables"; + } + + if (!WORD_CHARACTERS_REGEX.test(name)) { + errorText = 'Only word characters are allowed in variable names'; + } + + const variable = this.getVariableSet().getByName(name)?.state; + + if (variable && variable.key !== key) { + errorText = 'Variable with the same name already exists'; + } + + if (errorText) { + return [true, errorText]; + } + + return [false, null]; + }; } function VariableEditorSettingsListView({ model }: SceneComponentProps) { @@ -190,6 +219,7 @@ function VariableEditorSettingsListView({ model }: SceneComponentProps ); } @@ -218,6 +248,7 @@ interface VariableEditorSettingsEditViewProps { onTypeChange: (variableType: EditableVariableType) => void; onGoBack: () => void; onDelete: (variableName: string) => void; + onValidateVariableName: (name: string, key: string | undefined) => [true, string] | [false, null]; } function VariableEditorSettingsView({ @@ -228,6 +259,7 @@ function VariableEditorSettingsView({ onTypeChange, onGoBack, onDelete, + onValidateVariableName, }: VariableEditorSettingsEditViewProps) { const parentTab = pageNav.children!.find((p) => p.active)!; parentTab.parentItem = pageNav; @@ -240,7 +272,13 @@ function VariableEditorSettingsView({ return ( - + ); } diff --git a/public/app/features/dashboard-scene/settings/variables/VariableEditorForm.tsx b/public/app/features/dashboard-scene/settings/variables/VariableEditorForm.tsx index 4f0ab76a584..d04e228acc4 100644 --- a/public/app/features/dashboard-scene/settings/variables/VariableEditorForm.tsx +++ b/public/app/features/dashboard-scene/settings/variables/VariableEditorForm.tsx @@ -1,5 +1,5 @@ import { css } from '@emotion/css'; -import React, { FormEvent } from 'react'; +import React, { FormEvent, useCallback, useState } from 'react'; import { useAsyncFn } from 'react-use'; import { lastValueFrom } from 'rxjs'; @@ -24,23 +24,44 @@ interface VariableEditorFormProps { onTypeChange: (type: EditableVariableType) => void; onGoBack: () => void; onDelete: (variableName: string) => void; + onValidateVariableName: (name: string, key: string | undefined) => [true, string] | [false, null]; } - -export function VariableEditorForm({ variable, onTypeChange, onGoBack, onDelete }: VariableEditorFormProps) { +export function VariableEditorForm({ + variable, + onTypeChange, + onGoBack, + onDelete, + onValidateVariableName, +}: VariableEditorFormProps) { const styles = useStyles2(getStyles); - const { name, type, label, description, hide } = variable.useState(); + const [nameError, setNameError] = useState(null); + const { name, type, label, description, hide, key } = variable.useState(); const EditorToRender = isEditableVariableType(type) ? getVariableEditor(type) : undefined; const [runQueryState, onRunQuery] = useAsyncFn(async () => { await lastValueFrom(variable.validateAndUpdate!()); }, [variable]); - const onVariableTypeChange = (option: SelectableValue) => { if (option.value) { onTypeChange(option.value); } }; - const onNameBlur = (e: FormEvent) => variable.setState({ name: e.currentTarget.value }); + const onNameChange = useCallback( + (e: FormEvent) => { + const [, errorMessage] = onValidateVariableName(e.currentTarget.value, key); + if (nameError !== errorMessage) { + setNameError(errorMessage); + } + }, + [key, nameError, onValidateVariableName] + ); + + const onNameBlur = (e: FormEvent) => { + if (!nameError) { + variable.setState({ name: e.currentTarget.value }); + } + }; + const onLabelBlur = (e: FormEvent) => variable.setState({ label: e.currentTarget.value }); const onDescriptionBlur = (e: FormEvent) => variable.setState({ description: e.currentTarget.value }); @@ -64,10 +85,13 @@ export function VariableEditorForm({ variable, onTypeChange, onGoBack, onDelete description="The name of the template variable. (Max. 50 characters)" placeholder="Variable name" defaultValue={name ?? ''} + onChange={onNameChange} onBlur={onNameBlur} testId={selectors.pages.Dashboard.Settings.Variables.Edit.General.generalNameInputV2} maxLength={VariableNameConstraints.MaxSize} required + invalid={!!nameError} + error={nameError} />