From 317909f0c96b1c19bc2e578bae8aa11fd87a8670 Mon Sep 17 00:00:00 2001 From: Giordano Ricci Date: Tue, 6 Apr 2021 13:16:23 +0100 Subject: [PATCH] Elasticsearch: Fix bucket script variable duplication in UI (#32705) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Elasticsearch: Fix bucket script variable duplication * Generate names based on previous value & fix list key * Fix typos Co-authored-by: Piotr Jamróz Co-authored-by: Piotr Jamróz --- .../BucketScriptSettingsEditor/index.tsx | 10 ++++++- .../state/reducer.test.ts | 22 ++++++++++++--- .../state/reducer.ts | 4 +-- .../BucketScriptSettingsEditor/utils.test.ts | 27 +++++++++++++++++++ .../BucketScriptSettingsEditor/utils.ts | 9 ++++++- .../MetricAggregationsEditor/utils.ts | 7 +++-- 6 files changed, 70 insertions(+), 9 deletions(-) create mode 100644 public/app/plugins/datasource/elasticsearch/components/QueryEditor/MetricAggregationsEditor/SettingsEditor/BucketScriptSettingsEditor/utils.test.ts diff --git a/public/app/plugins/datasource/elasticsearch/components/QueryEditor/MetricAggregationsEditor/SettingsEditor/BucketScriptSettingsEditor/index.tsx b/public/app/plugins/datasource/elasticsearch/components/QueryEditor/MetricAggregationsEditor/SettingsEditor/BucketScriptSettingsEditor/index.tsx index 3a5e6d2eb8c..4fddaabbe5f 100644 --- a/public/app/plugins/datasource/elasticsearch/components/QueryEditor/MetricAggregationsEditor/SettingsEditor/BucketScriptSettingsEditor/index.tsx +++ b/public/app/plugins/datasource/elasticsearch/components/QueryEditor/MetricAggregationsEditor/SettingsEditor/BucketScriptSettingsEditor/index.tsx @@ -15,6 +15,7 @@ import { } from './state/actions'; import { SettingField } from '../SettingField'; import { BucketScript, MetricAggregation } from '../../aggregations'; +import { uniqueId } from 'lodash'; interface Props { value: BucketScript; @@ -55,7 +56,14 @@ export const BucketScriptSettingsEditor: FunctionComponent = ({ value, pr `} > {value.pipelineVariables!.map((pipelineVar, index) => ( - + // index as a key doesn't work here since removing an element + // in the middle of the list, will cause the next element to obtain the same key as the removed one. + // this will cause react to "drop" the last element of the list instead of the just removed one, + // and the default value for the input won't match the model as the DOM won't get updated. + // using pipelineVar.name is not an option since it might be duplicated by the user. + // generating a unique key on every render, while is probably not the best solution in terms of performance + // ensures the UI is in a correct state. We might want to optimize this if we see perf issue in the future. +
{ - it('Should correctly add new pipeline variable', () => { - const expectedPipelineVar: PipelineVariable = { + it('Should correctly add new pipeline variables', () => { + const var1: PipelineVariable = { name: 'var1', pipelineAgg: '', }; + const var2: PipelineVariable = { + name: 'var2', + pipelineAgg: '', + }; + + const var3: PipelineVariable = { + name: 'var3', + pipelineAgg: '', + }; + reducerTester() .givenReducer(reducer, []) .whenActionIsDispatched(addPipelineVariable()) - .thenStateShouldEqual([expectedPipelineVar]); + .thenStateShouldEqual([var1]) + .whenActionIsDispatched(addPipelineVariable()) + .thenStateShouldEqual([var1, var2]) + .whenActionIsDispatched(removePipelineVariable(0)) + .thenStateShouldEqual([var2]) + .whenActionIsDispatched(addPipelineVariable()) + .thenStateShouldEqual([var2, var3]); }); it('Should correctly remove pipeline variables', () => { diff --git a/public/app/plugins/datasource/elasticsearch/components/QueryEditor/MetricAggregationsEditor/SettingsEditor/BucketScriptSettingsEditor/state/reducer.ts b/public/app/plugins/datasource/elasticsearch/components/QueryEditor/MetricAggregationsEditor/SettingsEditor/BucketScriptSettingsEditor/state/reducer.ts index 3541ae52d51..6b8b5b7c1a4 100644 --- a/public/app/plugins/datasource/elasticsearch/components/QueryEditor/MetricAggregationsEditor/SettingsEditor/BucketScriptSettingsEditor/state/reducer.ts +++ b/public/app/plugins/datasource/elasticsearch/components/QueryEditor/MetricAggregationsEditor/SettingsEditor/BucketScriptSettingsEditor/state/reducer.ts @@ -1,5 +1,5 @@ import { PipelineVariable } from '../../../aggregations'; -import { defaultPipelineVariable } from '../utils'; +import { defaultPipelineVariable, generatePipelineVariableName } from '../utils'; import { PipelineVariablesAction, REMOVE_PIPELINE_VARIABLE, @@ -11,7 +11,7 @@ import { export const reducer = (state: PipelineVariable[] = [], action: PipelineVariablesAction) => { switch (action.type) { case ADD_PIPELINE_VARIABLE: - return [...state, defaultPipelineVariable()]; + return [...state, defaultPipelineVariable(generatePipelineVariableName(state))]; case REMOVE_PIPELINE_VARIABLE: return state.slice(0, action.payload.index).concat(state.slice(action.payload.index + 1)); diff --git a/public/app/plugins/datasource/elasticsearch/components/QueryEditor/MetricAggregationsEditor/SettingsEditor/BucketScriptSettingsEditor/utils.test.ts b/public/app/plugins/datasource/elasticsearch/components/QueryEditor/MetricAggregationsEditor/SettingsEditor/BucketScriptSettingsEditor/utils.test.ts new file mode 100644 index 00000000000..55671f9f690 --- /dev/null +++ b/public/app/plugins/datasource/elasticsearch/components/QueryEditor/MetricAggregationsEditor/SettingsEditor/BucketScriptSettingsEditor/utils.test.ts @@ -0,0 +1,27 @@ +import { generatePipelineVariableName } from './utils'; + +describe('generatePipelineVariableName', () => { + it('Correctly generates a new name', () => { + // when we have no previous name, the generated one should be `var1` + expect(generatePipelineVariableName([])).toBe('var1'); + + expect( + generatePipelineVariableName([ + { + name: 'var1', + pipelineAgg: '', + }, + ]) + ).toBe('var2'); + + // It should only generate nemes based on variables named `var{n}` + expect( + generatePipelineVariableName([ + { + name: 'something1', + pipelineAgg: '', + }, + ]) + ).toBe('var1'); + }); +}); diff --git a/public/app/plugins/datasource/elasticsearch/components/QueryEditor/MetricAggregationsEditor/SettingsEditor/BucketScriptSettingsEditor/utils.ts b/public/app/plugins/datasource/elasticsearch/components/QueryEditor/MetricAggregationsEditor/SettingsEditor/BucketScriptSettingsEditor/utils.ts index a3169a07366..2f57c25a26d 100644 --- a/public/app/plugins/datasource/elasticsearch/components/QueryEditor/MetricAggregationsEditor/SettingsEditor/BucketScriptSettingsEditor/utils.ts +++ b/public/app/plugins/datasource/elasticsearch/components/QueryEditor/MetricAggregationsEditor/SettingsEditor/BucketScriptSettingsEditor/utils.ts @@ -1,3 +1,10 @@ import { PipelineVariable } from '../../aggregations'; -export const defaultPipelineVariable = (): PipelineVariable => ({ name: 'var1', pipelineAgg: '' }); +export const defaultPipelineVariable = (name: string): PipelineVariable => ({ name, pipelineAgg: '' }); + +/** + * Given an array of pipeline variables generates a new unique pipeline variable name in the form of `var{n}`. + * The value for `n` is calculated based on the variables names in pipelineVars matching `var{n}`. + */ +export const generatePipelineVariableName = (pipelineVars: PipelineVariable[]): string => + `var${Math.max(0, ...pipelineVars.map((v) => parseInt(v.name.match('^var(\\d+)$')?.[1] || '0', 10))) + 1}`; diff --git a/public/app/plugins/datasource/elasticsearch/components/QueryEditor/MetricAggregationsEditor/utils.ts b/public/app/plugins/datasource/elasticsearch/components/QueryEditor/MetricAggregationsEditor/utils.ts index 7c3a1aafc4c..6872c41ec44 100644 --- a/public/app/plugins/datasource/elasticsearch/components/QueryEditor/MetricAggregationsEditor/utils.ts +++ b/public/app/plugins/datasource/elasticsearch/components/QueryEditor/MetricAggregationsEditor/utils.ts @@ -5,7 +5,10 @@ import { MetricAggregation, PipelineMetricAggregationType, } from './aggregations'; -import { defaultPipelineVariable } from './SettingsEditor/BucketScriptSettingsEditor/utils'; +import { + defaultPipelineVariable, + generatePipelineVariableName, +} from './SettingsEditor/BucketScriptSettingsEditor/utils'; export const metricAggregationConfig: MetricsConfiguration = { count: { @@ -182,7 +185,7 @@ export const metricAggregationConfig: MetricsConfiguration = { supportsInlineScript: false, hasMeta: false, defaults: { - pipelineVariables: [defaultPipelineVariable()], + pipelineVariables: [defaultPipelineVariable(generatePipelineVariableName([]))], }, }, raw_document: {