From eaf6aea98f489bcedef89d09224d8fcc16caf494 Mon Sep 17 00:00:00 2001 From: Josh Hunt Date: Tue, 2 Aug 2022 10:15:25 +0100 Subject: [PATCH] Typed variables pt1: Use discriminated union for variable model (#52981) * wip * make diff easier to read * Update template_srv getVariables to return new TypedVariableModel * update VariableType to use the type from TypedVariableModel * tidy things up --- .betterer.results | 17 +- .../grafana-data/src/types/templateVars.ts | 154 ++++++++++++++- .../src/services/templateSrv.ts | 4 +- .../dashboard/state/DashboardMigrator.ts | 25 ++- public/app/features/plugins/datasource_srv.ts | 4 +- .../features/templating/template_srv.mock.ts | 8 +- .../app/features/templating/template_srv.ts | 16 +- public/app/features/variables/guard.ts | 3 + .../query/QueryVariableEditor.test.tsx | 5 +- .../testing/datasourceVariableBuilder.ts | 6 +- .../variables/state/sharedReducer.test.ts | 4 +- public/app/features/variables/types.ts | 178 +++++------------- 12 files changed, 263 insertions(+), 161 deletions(-) diff --git a/.betterer.results b/.betterer.results index b561c1e0935..d049b0568bb 100644 --- a/.betterer.results +++ b/.betterer.results @@ -751,6 +751,10 @@ exports[`better eslint`] = { [0, 0, 0, "Unexpected any. Specify a different type.", "1"], [0, 0, 0, "Unexpected any. Specify a different type.", "2"] ], + "packages/grafana-data/src/types/templateVars.ts:5381": [ + [0, 0, 0, "Unexpected any. Specify a different type.", "0"], + [0, 0, 0, "Unexpected any. Specify a different type.", "1"] + ], "packages/grafana-data/src/types/trace.ts:5381": [ [0, 0, 0, "Unexpected any. Specify a different type.", "0"] ], @@ -5726,6 +5730,9 @@ exports[`better eslint`] = { [0, 0, 0, "Unexpected any. Specify a different type.", "3"], [0, 0, 0, "Unexpected any. Specify a different type.", "4"] ], + "public/app/features/templating/template_srv.mock.ts:5381": [ + [0, 0, 0, "Do not use any type assertions.", "0"] + ], "public/app/features/templating/template_srv.test.ts:5381": [ [0, 0, 0, "Unexpected any. Specify a different type.", "0"], [0, 0, 0, "Unexpected any. Specify a different type.", "1"], @@ -6275,13 +6282,9 @@ exports[`better eslint`] = { "public/app/features/variables/types.ts:5381": [ [0, 0, 0, "Unexpected any. Specify a different type.", "0"], [0, 0, 0, "Unexpected any. Specify a different type.", "1"], - [0, 0, 0, "Do not use any type assertions.", "2"], - [0, 0, 0, "Do not use any type assertions.", "3"], - [0, 0, 0, "Unexpected any. Specify a different type.", "4"], - [0, 0, 0, "Unexpected any. Specify a different type.", "5"], - [0, 0, 0, "Unexpected any. Specify a different type.", "6"], - [0, 0, 0, "Unexpected any. Specify a different type.", "7"], - [0, 0, 0, "Unexpected any. Specify a different type.", "8"] + [0, 0, 0, "Unexpected any. Specify a different type.", "2"], + [0, 0, 0, "Unexpected any. Specify a different type.", "3"], + [0, 0, 0, "Unexpected any. Specify a different type.", "4"] ], "public/app/features/variables/utils.ts:5381": [ [0, 0, 0, "Unexpected any. Specify a different type.", "0"], diff --git a/packages/grafana-data/src/types/templateVars.ts b/packages/grafana-data/src/types/templateVars.ts index 5ac807d16be..f723d20a341 100644 --- a/packages/grafana-data/src/types/templateVars.ts +++ b/packages/grafana-data/src/types/templateVars.ts @@ -1,7 +1,159 @@ -export type VariableType = 'query' | 'adhoc' | 'constant' | 'datasource' | 'interval' | 'textbox' | 'custom' | 'system'; +import { LoadingState } from './data'; +import { DataSourceRef } from './query'; +export type VariableType = TypedVariableModel['type']; + +/** @deprecated Use TypedVariableModel instead */ export interface VariableModel { type: VariableType; name: string; label?: string; } + +export type TypedVariableModel = + | QueryVariableModel + | AdHocVariableModel + | ConstantVariableModel + | DataSourceVariableModel + | IntervalVariableModel + | TextBoxVariableModel + | CustomVariableModel + | UserVariableModel + | OrgVariableModel + | DashboardVariableModel; + +export enum VariableRefresh { + never, // removed from the UI + onDashboardLoad, + onTimeRangeChanged, +} + +export enum VariableSort { + disabled, + alphabeticalAsc, + alphabeticalDesc, + numericalAsc, + numericalDesc, + alphabeticalCaseInsensitiveAsc, + alphabeticalCaseInsensitiveDesc, +} + +export enum VariableHide { + dontHide, + hideLabel, + hideVariable, +} + +export interface AdHocVariableFilter { + key: string; + operator: string; + value: string; + condition: string; +} + +export interface AdHocVariableModel extends BaseVariableModel { + type: 'adhoc'; + datasource: DataSourceRef | null; + filters: AdHocVariableFilter[]; +} + +export interface VariableOption { + selected: boolean; + text: string | string[]; + value: string | string[]; + isNone?: boolean; +} + +export interface IntervalVariableModel extends VariableWithOptions { + type: 'interval'; + auto: boolean; + auto_min: string; + auto_count: number; + refresh: VariableRefresh; +} + +export interface CustomVariableModel extends VariableWithMultiSupport { + type: 'custom'; +} + +export interface DataSourceVariableModel extends VariableWithMultiSupport { + type: 'datasource'; + regex: string; + refresh: VariableRefresh; +} + +export interface QueryVariableModel extends VariableWithMultiSupport { + type: 'query'; + datasource: DataSourceRef | null; + definition: string; + sort: VariableSort; + queryValue?: string; + query: any; + regex: string; + refresh: VariableRefresh; +} + +export interface TextBoxVariableModel extends VariableWithOptions { + type: 'textbox'; + originalQuery: string | null; +} + +export interface ConstantVariableModel extends VariableWithOptions { + type: 'constant'; +} + +export interface VariableWithMultiSupport extends VariableWithOptions { + multi: boolean; + includeAll: boolean; + allValue?: string | null; +} + +export interface VariableWithOptions extends BaseVariableModel { + current: VariableOption; + options: VariableOption[]; + query: string; +} + +export interface DashboardProps { + name: string; + uid: string; + toString: () => string; +} + +export interface DashboardVariableModel extends SystemVariable {} + +export interface OrgProps { + name: string; + id: number; + toString: () => string; +} + +export interface OrgVariableModel extends SystemVariable {} + +export interface UserProps { + login: string; + id: number; + email?: string; + toString: () => string; +} + +export interface UserVariableModel extends SystemVariable {} + +export interface SystemVariable string }> extends BaseVariableModel { + type: 'system'; + current: { value: TProps }; +} + +export interface BaseVariableModel extends VariableModel { + name: string; + label?: string; + id: string; + rootStateKey: string | null; + global: boolean; + hide: VariableHide; + skipUrlSync: boolean; + index: number; + state: LoadingState; + error: any | null; + description: string | null; +} diff --git a/packages/grafana-runtime/src/services/templateSrv.ts b/packages/grafana-runtime/src/services/templateSrv.ts index b0ac664e15e..502e9de2f9a 100644 --- a/packages/grafana-runtime/src/services/templateSrv.ts +++ b/packages/grafana-runtime/src/services/templateSrv.ts @@ -1,4 +1,4 @@ -import { VariableModel, ScopedVars, TimeRange } from '@grafana/data'; +import { ScopedVars, TimeRange, TypedVariableModel } from '@grafana/data'; /** * Via the TemplateSrv consumers get access to all the available template variables @@ -11,7 +11,7 @@ export interface TemplateSrv { /** * List the dashboard variables */ - getVariables(): VariableModel[]; + getVariables(): TypedVariableModel[]; /** * Replace the values within the target string. See also {@link InterpolateFunction} diff --git a/public/app/features/dashboard/state/DashboardMigrator.ts b/public/app/features/dashboard/state/DashboardMigrator.ts index 8e7e7b9a990..11abda3f6d0 100644 --- a/public/app/features/dashboard/state/DashboardMigrator.ts +++ b/public/app/features/dashboard/state/DashboardMigrator.ts @@ -51,7 +51,7 @@ import { migrateMultipleStatsAnnotationQuery, migrateMultipleStatsMetricsQuery, } from '../../../plugins/datasource/cloudwatch/migrations/dashboardMigrations'; -import { VariableHide } from '../../variables/types'; +import { ConstantVariableModel, TextBoxVariableModel, VariableHide } from '../../variables/types'; import { DashboardModel } from './DashboardModel'; import { PanelModel } from './PanelModel'; @@ -621,18 +621,27 @@ export class DashboardMigrator { } if (oldVersion < 27) { - for (const variable of this.dashboard.templating.list) { + this.dashboard.templating.list = this.dashboard.templating.list.map((variable) => { if (!isConstant(variable)) { - continue; + return variable; } - if (variable.hide === VariableHide.dontHide || variable.hide === VariableHide.hideLabel) { - variable.type = 'textbox'; + const newVariable: ConstantVariableModel | TextBoxVariableModel = { + ...variable, + }; + + newVariable.current = { selected: true, text: newVariable.query ?? '', value: newVariable.query ?? '' }; + newVariable.options = [newVariable.current]; + + if (newVariable.hide === VariableHide.dontHide || newVariable.hide === VariableHide.hideLabel) { + return { + ...newVariable, + type: 'textbox', + }; } - variable.current = { selected: true, text: variable.query ?? '', value: variable.query ?? '' }; - variable.options = [variable.current]; - } + return newVariable; + }); } if (oldVersion < 28) { diff --git a/public/app/features/plugins/datasource_srv.ts b/public/app/features/plugins/datasource_srv.ts index 41a4fc7b4a2..32cc1b0deef 100644 --- a/public/app/features/plugins/datasource_srv.ts +++ b/public/app/features/plugins/datasource_srv.ts @@ -24,8 +24,6 @@ import { instanceSettings as expressionInstanceSettings, } from 'app/features/expressions/ExpressionDatasource'; -import { isDataSource } from '../variables/guard'; - import { importDataSourcePlugin } from './plugin_loader'; export class DatasourceSrv implements DataSourceService { @@ -246,7 +244,7 @@ export class DatasourceSrv implements DataSourceService { if (filters.variables) { for (const variable of this.templateSrv.getVariables()) { - if (!isDataSource(variable) || variable.multi || variable.includeAll) { + if (variable.type !== 'datasource' || variable.multi || variable.includeAll) { continue; } const dsName = variable.current.value === 'default' ? this.defaultName : variable.current.value; diff --git a/public/app/features/templating/template_srv.mock.ts b/public/app/features/templating/template_srv.mock.ts index 0d7e41f8ad4..70ab70fb988 100644 --- a/public/app/features/templating/template_srv.mock.ts +++ b/public/app/features/templating/template_srv.mock.ts @@ -1,4 +1,4 @@ -import { ScopedVars, TimeRange, VariableModel } from '@grafana/data'; +import { ScopedVars, TimeRange, TypedVariableModel } from '@grafana/data'; import { TemplateSrv } from '@grafana/runtime'; import { variableRegex } from '../variables/utils'; @@ -15,14 +15,16 @@ export class TemplateSrvMock implements TemplateSrv { private regex = variableRegex; constructor(private variables: Record) {} - getVariables(): VariableModel[] { + getVariables(): TypedVariableModel[] { return Object.keys(this.variables).map((key) => { return { type: 'custom', name: key, label: key, }; - }); + // TODO: we remove this type assertion in a later PR + // eslint-disable-next-line @typescript-eslint/consistent-type-assertions + }) as TypedVariableModel[]; } replace(target?: string, scopedVars?: ScopedVars, format?: string | Function): string { diff --git a/public/app/features/templating/template_srv.ts b/public/app/features/templating/template_srv.ts index 7a06912ddfc..4bb9cab8aef 100644 --- a/public/app/features/templating/template_srv.ts +++ b/public/app/features/templating/template_srv.ts @@ -1,13 +1,19 @@ import { escape, isString, property } from 'lodash'; -import { deprecationWarning, ScopedVars, TimeRange } from '@grafana/data'; +import { + deprecationWarning, + ScopedVars, + TimeRange, + AdHocVariableFilter, + AdHocVariableModel, + TypedVariableModel, +} from '@grafana/data'; import { getDataSourceSrv, setTemplateSrv, TemplateSrv as BaseTemplateSrv } from '@grafana/runtime'; import { variableAdapters } from '../variables/adapters'; import { ALL_VARIABLE_TEXT, ALL_VARIABLE_VALUE } from '../variables/constants'; import { isAdHoc } from '../variables/guard'; import { getFilteredVariables, getVariables, getVariableWithName } from '../variables/state/selectors'; -import { AdHocVariableFilter, AdHocVariableModel, VariableModel } from '../variables/types'; import { variableRegex } from '../variables/utils'; import { FormatOptions, formatRegistry, FormatRegistryID } from './formatRegistry'; @@ -56,8 +62,10 @@ export class TemplateSrv implements BaseTemplateSrv { return this.getVariables(); } - getVariables(): VariableModel[] { - return this.dependencies.getVariables(); + getVariables(): TypedVariableModel[] { + // TODO: we remove this type assertion in a later PR + // eslint-disable-next-line @typescript-eslint/consistent-type-assertions + return this.dependencies.getVariables() as TypedVariableModel[]; } updateIndex() { diff --git a/public/app/features/variables/guard.ts b/public/app/features/variables/guard.ts index 5d8eecababe..f6097fc5f35 100644 --- a/public/app/features/variables/guard.ts +++ b/public/app/features/variables/guard.ts @@ -28,14 +28,17 @@ import { DataSourceVariableModel, } from './types'; +/** @deprecated use a if (model.type === "query") type narrowing check instead */ export const isQuery = (model: VariableModel): model is QueryVariableModel => { return model.type === 'query'; }; +/** @deprecated use a if (model.type === "adhoc") type narrowing check instead */ export const isAdHoc = (model: VariableModel): model is AdHocVariableModel => { return model.type === 'adhoc'; }; +/** @deprecated use a if (model.type === "constant") type narrowing check instead */ export const isConstant = (model: VariableModel): model is ConstantVariableModel => { return model.type === 'constant'; }; diff --git a/public/app/features/variables/query/QueryVariableEditor.test.tsx b/public/app/features/variables/query/QueryVariableEditor.test.tsx index 21bd94127ad..76b274fe4bf 100644 --- a/public/app/features/variables/query/QueryVariableEditor.test.tsx +++ b/public/app/features/variables/query/QueryVariableEditor.test.tsx @@ -9,17 +9,18 @@ import { DataSourceType } from 'app/features/alerting/unified/utils/datasource'; import { NEW_VARIABLE_ID } from '../constants'; import { LegacyVariableQueryEditor } from '../editor/LegacyVariableQueryEditor'; import { KeyedVariableIdentifier } from '../state/types'; -import { VariableModel } from '../types'; +import { QueryVariableModel } from '../types'; import { Props, QueryVariableEditorUnConnected } from './QueryVariableEditor'; import { initialQueryVariableModelState } from './reducer'; const setupTestContext = (options: Partial) => { - const variableDefaults: Partial = { rootStateKey: 'key' }; + const variableDefaults: Partial = { rootStateKey: 'key' }; const extended = { VariableQueryEditor: LegacyVariableQueryEditor, dataSource: {} as unknown as DataSourceApi, }; + const defaults: Props = { variable: { ...initialQueryVariableModelState, ...variableDefaults }, initQueryVariableEditor: jest.fn(), diff --git a/public/app/features/variables/shared/testing/datasourceVariableBuilder.ts b/public/app/features/variables/shared/testing/datasourceVariableBuilder.ts index db4c9be4ed6..28847c872ae 100644 --- a/public/app/features/variables/shared/testing/datasourceVariableBuilder.ts +++ b/public/app/features/variables/shared/testing/datasourceVariableBuilder.ts @@ -1,8 +1,10 @@ -import { DataSourceVariableModel, VariableRefresh } from 'app/features/variables/types'; +import { DataSourceVariableModel, QueryVariableModel, VariableRefresh } from 'app/features/variables/types'; import { MultiVariableBuilder } from './multiVariableBuilder'; -export class DatasourceVariableBuilder extends MultiVariableBuilder { +export class DatasourceVariableBuilder< + T extends DataSourceVariableModel | QueryVariableModel +> extends MultiVariableBuilder { withRefresh(refresh: VariableRefresh) { this.variable.refresh = refresh; return this; diff --git a/public/app/features/variables/state/sharedReducer.test.ts b/public/app/features/variables/state/sharedReducer.test.ts index 5e6e10d2811..fd795ed798a 100644 --- a/public/app/features/variables/state/sharedReducer.test.ts +++ b/public/app/features/variables/state/sharedReducer.test.ts @@ -37,7 +37,7 @@ describe('sharedReducer', () => { it('then state should be correct', () => { const model: any = { name: 'name from model', - type: 'type from model', + type: 'query', current: undefined, }; @@ -47,7 +47,7 @@ describe('sharedReducer', () => { global: true, index: 0, name: 'name from model', - type: 'type from model' as unknown as VariableType, + type: 'query', current: {} as unknown as VariableOption, }; diff --git a/public/app/features/variables/types.ts b/public/app/features/variables/types.ts index 890df93e8a3..3b74f7914be 100644 --- a/public/app/features/variables/types.ts +++ b/public/app/features/variables/types.ts @@ -4,11 +4,56 @@ import { BusEventWithPayload, DataQuery, DataSourceJsonData, - DataSourceRef, LoadingState, QueryEditorProps, - VariableModel as BaseVariableModel, - VariableType, + BaseVariableModel, + VariableHide, +} from '@grafana/data'; +export { + /** @deprecated Import from @grafana/data instead */ + VariableRefresh, + /** @deprecated Import from @grafana/data instead */ + VariableSort, + /** @deprecated Import from @grafana/data instead */ + VariableHide, + /** @deprecated Import from @grafana/data instead */ + AdHocVariableFilter, + /** @deprecated Import from @grafana/data instead */ + AdHocVariableModel, + /** @deprecated Import from @grafana/data instead */ + VariableOption, + /** @deprecated Import from @grafana/data instead */ + IntervalVariableModel, + /** @deprecated Import from @grafana/data instead */ + CustomVariableModel, + /** @deprecated Import from @grafana/data instead */ + DataSourceVariableModel, + /** @deprecated Import from @grafana/data instead */ + QueryVariableModel, + /** @deprecated Import from @grafana/data instead */ + TextBoxVariableModel, + /** @deprecated Import from @grafana/data instead */ + ConstantVariableModel, + /** @deprecated Import from @grafana/data instead */ + VariableWithMultiSupport, + /** @deprecated Import from @grafana/data instead */ + VariableWithOptions, + /** @deprecated Import from @grafana/data instead */ + DashboardProps, + /** @deprecated Import from @grafana/data instead */ + DashboardVariableModel, + /** @deprecated Import from @grafana/data instead */ + OrgProps, + /** @deprecated Import from @grafana/data instead */ + OrgVariableModel, + /** @deprecated Import from @grafana/data instead */ + UserProps, + /** @deprecated Import from @grafana/data instead */ + UserVariableModel, + /** @deprecated Import from @grafana/data instead */ + SystemVariable, + /** @deprecated Import from @grafana/data instead */ + BaseVariableModel as VariableModel, } from '@grafana/data'; import { TemplateSrv } from '@grafana/runtime'; @@ -20,133 +65,12 @@ export enum TransactionStatus { Completed = 'Completed', } -export enum VariableRefresh { - never, // removed from the UI - onDashboardLoad, - onTimeRangeChanged, -} - -export enum VariableHide { - dontHide, - hideLabel, - hideVariable, -} - -export enum VariableSort { - disabled, - alphabeticalAsc, - alphabeticalDesc, - numericalAsc, - numericalDesc, - alphabeticalCaseInsensitiveAsc, - alphabeticalCaseInsensitiveDesc, -} - -export interface VariableOption { - selected: boolean; - text: string | string[]; - value: string | string[]; - isNone?: boolean; -} - -export interface AdHocVariableFilter { - key: string; - operator: string; - value: string; - condition: string; -} - -export interface AdHocVariableModel extends VariableModel { - datasource: DataSourceRef | null; - filters: AdHocVariableFilter[]; -} - -export interface IntervalVariableModel extends VariableWithOptions { - auto: boolean; - auto_min: string; - auto_count: number; - refresh: VariableRefresh; -} - -export interface CustomVariableModel extends VariableWithMultiSupport {} - -export interface DataSourceVariableModel extends VariableWithMultiSupport { - regex: string; - refresh: VariableRefresh; -} - -export interface QueryVariableModel extends DataSourceVariableModel { - datasource: DataSourceRef | null; - definition: string; - sort: VariableSort; - queryValue?: string; - query: any; -} - -export interface TextBoxVariableModel extends VariableWithOptions { - originalQuery: string | null; -} - -export interface ConstantVariableModel extends VariableWithOptions {} - -export interface VariableWithMultiSupport extends VariableWithOptions { - multi: boolean; - includeAll: boolean; - allValue?: string | null; -} - -export interface VariableWithOptions extends VariableModel { - current: VariableOption; - options: VariableOption[]; - query: string; -} - -export interface DashboardProps { - name: string; - uid: string; - toString: () => string; -} - -export interface DashboardVariableModel extends SystemVariable {} - -export interface OrgProps { - name: string; - id: number; - toString: () => string; -} - -export interface OrgVariableModel extends SystemVariable {} - -export interface UserProps { - login: string; - id: number; - email?: string; - toString: () => string; -} - -export interface UserVariableModel extends SystemVariable {} - -export interface SystemVariable string }> extends VariableModel { - current: { value: TProps }; -} - -export interface VariableModel extends BaseVariableModel { - id: string; - rootStateKey: string | null; - global: boolean; - hide: VariableHide; - skipUrlSync: boolean; - index: number; - state: LoadingState; - error: any | null; - description: string | null; -} - -export const initialVariableModelState: VariableModel = { +export const initialVariableModelState: BaseVariableModel = { id: NEW_VARIABLE_ID, rootStateKey: null, name: '', - type: '' as unknown as VariableType, + // TODO: in a later PR, remove type and type this object to Partial + type: 'query', global: false, index: -1, hide: VariableHide.dontHide,