Elasticsearch: Fix bucket script variable duplication in UI (#32705)

* Elasticsearch: Fix bucket script variable duplication

* Generate names based on previous value & fix list key

* Fix typos

Co-authored-by: Piotr Jamróz <pm.jamroz@gmail.com>

Co-authored-by: Piotr Jamróz <pm.jamroz@gmail.com>

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<Props> = ({ value, pr
           `}
         >
           {value.pipelineVariables!.map((pipelineVar, index) => (
-            <Fragment key={pipelineVar.name}>
+            // 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.
+            <Fragment key={uniqueId('es-bs-')}>
               <div
                 className={css`
                   display: grid;

public/app/plugins/datasource/elasticsearch/components/QueryEditor/MetricAggregationsEditor/SettingsEditor/BucketScriptSettingsEditor/state/reducer.test.ts

@@ -9,16 +9,32 @@ import {
 import { reducer } from './reducer';
 
 describe('BucketScript Settings Reducer', () => {
-  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<PipelineVariable[]>()
       .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', () => {

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));

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');
+  });
+});

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}`;

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: {