[ML] Fix to not add configs with names which would result in nesting conflicts (#37212) (#37240)

Two group-by or aggregation names could result in conflicting nested fields which would return an error for pivot previews. For example, two names like responsetime and responsetime.avg are not allowed.

This PR fixes the issue by extending the tests whether a configuration is allowed to be added to the list of group-by and aggregation configurations. If a conflict is detected, a toast notification gets triggered and the configuration won't be added.

x-pack/plugins/ml/public/data_frame/components/define_pivot/define_pivot_form.test.tsx

@@ -7,8 +7,14 @@
 import { shallow } from 'enzyme';
 import React from 'react';
 
-import { KibanaContext } from '../../common';
+import {
-import { DefinePivotForm } from './define_pivot_form';
+  KibanaContext,
+  PivotAggsConfigDict,
+  PivotGroupByConfigDict,
+  PIVOT_SUPPORTED_AGGS,
+  PIVOT_SUPPORTED_GROUP_BY_AGGS,
+} from '../../common';
+import { DefinePivotForm, isAggNameConflict } from './define_pivot_form';
 
 // workaround to make React.memo() work with enzyme
 jest.mock('react', () => {
@@ -46,3 +52,72 @@ describe('Data Frame: <DefinePivotForm />', () => {
     expect(wrapper).toMatchSnapshot();
   });
 });
+
+describe('Data Frame: isAggNameConflict()', () => {
+  test('detect aggregation name conflicts', () => {
+    const aggList: PivotAggsConfigDict = {
+      'the-agg-name': {
+        agg: PIVOT_SUPPORTED_AGGS.AVG,
+        field: 'the-field-name',
+        aggName: 'the-agg-name',
+        dropDownName: 'the-dropdown-name',
+      },
+      'the-namespaced-agg-name.namespace': {
+        agg: PIVOT_SUPPORTED_AGGS.AVG,
+        field: 'the-field-name',
+        aggName: 'the-namespaced-agg-name.namespace',
+        dropDownName: 'the-dropdown-name',
+      },
+    };
+
+    const groupByList: PivotGroupByConfigDict = {
+      'the-group-by-agg-name': {
+        agg: PIVOT_SUPPORTED_GROUP_BY_AGGS.TERMS,
+        field: 'the-field-name',
+        aggName: 'the-group-by-agg-name',
+        dropDownName: 'the-dropdown-name',
+      },
+      'the-namespaced-group-by-agg-name.namespace': {
+        agg: PIVOT_SUPPORTED_GROUP_BY_AGGS.TERMS,
+        field: 'the-field-name',
+        aggName: 'the-namespaced-group-by-agg-name.namespace',
+        dropDownName: 'the-dropdown-name',
+      },
+    };
+
+    // no conflict, completely different name, no namespacing involved
+    expect(isAggNameConflict('the-other-agg-name', aggList, groupByList)).toBe(false);
+    // no conflict, completely different name and no conflicting namespace
+    expect(isAggNameConflict('the-other-agg-name.namespace', aggList, groupByList)).toBe(false);
+
+    // exact match conflict on aggregation name
+    expect(isAggNameConflict('the-agg-name', aggList, groupByList)).toBe(true);
+    // namespace conflict with `the-agg-name` aggregation
+    expect(isAggNameConflict('the-agg-name.namespace', aggList, groupByList)).toBe(true);
+
+    // exact match conflict on group-by name
+    expect(isAggNameConflict('the-group-by-agg-name', aggList, groupByList)).toBe(true);
+    // namespace conflict with `the-group-by-agg-name` group-by
+    expect(isAggNameConflict('the-group-by-agg-name.namespace', aggList, groupByList)).toBe(true);
+
+    // exact match conflict on namespaced agg name
+    expect(isAggNameConflict('the-namespaced-agg-name.namespace', aggList, groupByList)).toBe(true);
+    // no conflict, same base agg name but different namespace
+    expect(isAggNameConflict('the-namespaced-agg-name.namespace2', aggList, groupByList)).toBe(
+      false
+    );
+    // namespace conflict because the new agg name is base name of existing nested field
+    expect(isAggNameConflict('the-namespaced-agg-name', aggList, groupByList)).toBe(true);
+
+    // exact match conflict on namespaced group-by name
+    expect(
+      isAggNameConflict('the-namespaced-group-by-agg-name.namespace', aggList, groupByList)
+    ).toBe(true);
+    // no conflict, same base group-by name but different namespace
+    expect(
+      isAggNameConflict('the-namespaced-group-by-agg-name.namespace2', aggList, groupByList)
+    ).toBe(false);
+    // namespace conflict because the new group-by name is base name of existing nested field
+    expect(isAggNameConflict('the-namespaced-group-by-agg-name', aggList, groupByList)).toBe(true);
+  });
+});

x-pack/plugins/ml/public/data_frame/components/define_pivot/define_pivot_form.tsx

@@ -66,6 +66,101 @@ export function getDefaultPivotState(kibanaContext: KibanaContextValue): DefineP
     valid: false,
   };
 }
+export function isAggNameConflict(
+  aggName: AggName,
+  aggList: PivotAggsConfigDict,
+  groupByList: PivotGroupByConfigDict
+) {
+  if (aggList[aggName] !== undefined) {
+    toastNotifications.addDanger(
+      i18n.translate('xpack.ml.dataframe.definePivot.aggExistsErrorMessage', {
+        defaultMessage: `An aggregation configuration with the name '{aggName}' already exists.`,
+        values: { aggName },
+      })
+    );
+    return true;
+  }
+
+  if (groupByList[aggName] !== undefined) {
+    toastNotifications.addDanger(
+      i18n.translate('xpack.ml.dataframe.definePivot.groupByExistsErrorMessage', {
+        defaultMessage: `A group by configuration with the name '{aggName}' already exists.`,
+        values: { aggName },
+      })
+    );
+    return true;
+  }
+
+  let conflict = false;
+
+  // check the new aggName against existing aggs and groupbys
+  const aggNameSplit = aggName.split('.');
+  let aggNameCheck: string;
+  aggNameSplit.forEach(aggNamePart => {
+    aggNameCheck = aggNameCheck === undefined ? aggNamePart : `${aggNameCheck}.${aggNamePart}`;
+    if (aggList[aggNameCheck] !== undefined || groupByList[aggNameCheck] !== undefined) {
+      toastNotifications.addDanger(
+        i18n.translate('xpack.ml.dataframe.definePivot.nestedConflictErrorMessage', {
+          defaultMessage: `Couldn't add configuration '{aggName}' because of a nesting conflict with '{aggNameCheck}'.`,
+          values: { aggName, aggNameCheck },
+        })
+      );
+      conflict = true;
+    }
+  });
+
+  if (conflict) {
+    return true;
+  }
+
+  // check all aggs against new aggName
+  conflict = Object.keys(aggList).some(aggListName => {
+    const aggListNameSplit = aggListName.split('.');
+    let aggListNameCheck: string;
+    return aggListNameSplit.some(aggListNamePart => {
+      aggListNameCheck =
+        aggListNameCheck === undefined ? aggListNamePart : `${aggListNameCheck}.${aggListNamePart}`;
+      if (aggListNameCheck === aggName) {
+        toastNotifications.addDanger(
+          i18n.translate('xpack.ml.dataframe.definePivot.nestedAggListConflictErrorMessage', {
+            defaultMessage: `Couldn't add configuration '{aggName}' because of a nesting conflict with '{aggListName}'.`,
+            values: { aggName, aggListName },
+          })
+        );
+        return true;
+      }
+      return false;
+    });
+  });
+
+  if (conflict) {
+    return true;
+  }
+
+  // check all group-bys against new aggName
+  conflict = Object.keys(groupByList).some(groupByListName => {
+    const groupByListNameSplit = groupByListName.split('.');
+    let groupByListNameCheck: string;
+    return groupByListNameSplit.some(groupByListNamePart => {
+      groupByListNameCheck =
+        groupByListNameCheck === undefined
+          ? groupByListNamePart
+          : `${groupByListNameCheck}.${groupByListNamePart}`;
+      if (groupByListNameCheck === aggName) {
+        toastNotifications.addDanger(
+          i18n.translate('xpack.ml.dataframe.definePivot.nestedGroupByListConflictErrorMessage', {
+            defaultMessage: `Couldn't add configuration '{aggName}' because of a nesting conflict with '{groupByListName}'.`,
+            values: { aggName, groupByListName },
+          })
+        );
+        return true;
+      }
+      return false;
+    });
+  });
+
+  return conflict;
+}
 
 interface Props {
   overrides?: DefinePivotExposedState;
@@ -111,23 +206,24 @@ export const DefinePivotForm: SFC<Props> = React.memo(({ overrides = {}, onChang
     const config: PivotGroupByConfig = groupByOptionsData[label];
     const aggName: AggName = config.aggName;
 
-    if (groupByList[aggName] === undefined) {
+    if (isAggNameConflict(aggName, aggList, groupByList)) {
-      groupByList[aggName] = config;
+      return;
-      setGroupByList({ ...groupByList });
-    } else {
-      toastNotifications.addDanger(
-        i18n.translate('xpack.ml.dataframe.definePivot.groupByExistsErrorMessage', {
-          defaultMessage: `A group by configuration with the name '{aggName}' already exists.`,
-          values: { aggName },
-        })
-      );
     }
+
+    groupByList[aggName] = config;
+    setGroupByList({ ...groupByList });
   };
 
   const updateGroupBy = (previousAggName: AggName, item: PivotGroupByConfig) => {
-    delete groupByList[previousAggName];
+    const groupByListWithoutPrevious = { ...groupByList };
-    groupByList[item.aggName] = item;
+    delete groupByListWithoutPrevious[previousAggName];
-    setGroupByList({ ...groupByList });
+
+    if (isAggNameConflict(item.aggName, aggList, groupByListWithoutPrevious)) {
+      return;
+    }
+
+    groupByListWithoutPrevious[item.aggName] = item;
+    setGroupByList({ ...groupByListWithoutPrevious });
   };
 
   const deleteGroupBy = (aggName: AggName) => {
@@ -143,21 +239,22 @@ export const DefinePivotForm: SFC<Props> = React.memo(({ overrides = {}, onChang
     const config: PivotAggsConfig = aggOptionsData[label];
     const aggName: AggName = config.aggName;
 
-    if (aggList[aggName] === undefined) {
+    if (isAggNameConflict(aggName, aggList, groupByList)) {
-      aggList[aggName] = config;
+      return;
-      setAggList({ ...aggList });
-    } else {
-      toastNotifications.addDanger(
-        i18n.translate('xpack.ml.dataframe.definePivot.aggExistsErrorMessage', {
-          defaultMessage: `An aggregation configuration with the name '{aggName}' already exists.`,
-          values: { aggName },
-        })
-      );
     }
+
+    aggList[aggName] = config;
+    setAggList({ ...aggList });
   };
 
   const updateAggregation = (previousAggName: AggName, item: PivotAggsConfig) => {
-    delete aggList[previousAggName];
+    const aggListWithoutPrevious = { ...aggList };
+    delete aggListWithoutPrevious[previousAggName];
+
+    if (isAggNameConflict(item.aggName, aggListWithoutPrevious, groupByList)) {
+      return;
+    }
+
     aggList[item.aggName] = item;
     setAggList({ ...aggList });
   };