[Lens] Formula: add validation for multiple field/metrics (#104092) (#105395)

Co-authored-by: Kibana Machine <[email protected]>

Co-authored-by: Marco Liberati <[email protected]>

Author: kibanamachine

x-pack/plugins/lens/public/indexpattern_datasource/operations/definitions/formula/formula.test.tsx

@@ -35,7 +35,7 @@ const operationDefinitionMap: Record<string, GenericOperationDefinition> = {
     }),
   } as unknown) as GenericOperationDefinition,
   terms: { input: 'field' } as GenericOperationDefinition,
-  sum: { input: 'field' } as GenericOperationDefinition,
+  sum: { input: 'field', filterable: true } as GenericOperationDefinition,
   last_value: { input: 'field' } as GenericOperationDefinition,
   max: { input: 'field' } as GenericOperationDefinition,
   count: ({
@@ -928,6 +928,63 @@ invalid: "
       ).toEqual(['The operation average does not accept any parameter']);
     });
 
+    it('returns an error if first argument type is passed multiple times', () => {
+      const formulas = [
+        'average(bytes, bytes)',
+        "sum(bytes, kql='category.keyword: *', bytes)",
+        'moving_average(average(bytes), average(bytes))',
+        "moving_average(average(bytes), kql='category.keyword: *', average(bytes))",
+        'moving_average(average(bytes, bytes), count())',
+        'moving_average(moving_average(average(bytes, bytes), count(), count()))',
+      ];
+      for (const formula of formulas) {
+        expect(
+          formulaOperation.getErrorMessage!(
+            getNewLayerWithFormula(formula),
+            'col1',
+            indexPattern,
+            operationDefinitionMap
+          )
+        ).toEqual(
+          expect.arrayContaining([
+            expect.stringMatching(
+              /The operation (moving_average|average|sum) in the Formula requires a single (field|metric), found:/
+            ),
+          ])
+        );
+      }
+    });
+
+    it('returns an error if a function received an argument of the wrong argument type in any position', () => {
+      const formulas = [
+        'average(bytes, count())',
+        "sum(bytes, kql='category.keyword: *', count(), count())",
+        'average(bytes, bytes + 1)',
+        'average(count(), bytes)',
+        'moving_average(average(bytes), bytes)',
+        'moving_average(bytes, bytes)',
+        'moving_average(average(bytes), window=7, bytes)',
+        'moving_average(window=7, bytes)',
+        "moving_average(kql='category.keyword: *', bytes)",
+      ];
+      for (const formula of formulas) {
+        expect(
+          formulaOperation.getErrorMessage!(
+            getNewLayerWithFormula(formula),
+            'col1',
+            indexPattern,
+            operationDefinitionMap
+          )
+        ).toEqual(
+          expect.arrayContaining([
+            expect.stringMatching(
+              /The operation (moving_average|average|sum) in the Formula does not support (metric|field) parameters, found:/
+            ),
+          ])
+        );
+      }
+    });
+
     it('returns an error if the parameter passed to an operation is of the wrong type', () => {
       expect(
         formulaOperation.getErrorMessage!(
@@ -1087,6 +1144,14 @@ invalid: "
           )
         ).toEqual([`The first argument for ${fn} should be a field name. Found no field`]);
       }
+      expect(
+        formulaOperation.getErrorMessage!(
+          getNewLayerWithFormula(`sum(kql='category.keyword: *')`),
+          'col1',
+          indexPattern,
+          operationDefinitionMap
+        )
+      ).toEqual([`The first argument for sum should be a field name. Found category.keyword: *`]);
     });
 
     it("returns a clear error when there's a missing function for a fullReference operation", () => {

x-pack/plugins/lens/public/indexpattern_datasource/operations/definitions/formula/util.ts

@@ -431,7 +431,7 @@ ${'`square(last_value(length))`'}
   },
 };
 
-export function isMathNode(node: TinymathAST) {
+export function isMathNode(node: TinymathAST | string) {
   return isObject(node) && node.type === 'function' && tinymathFunctions[node.name];
 }
 

x-pack/plugins/lens/public/indexpattern_datasource/operations/definitions/formula/validation.ts

@@ -7,7 +7,7 @@
 
 import { isObject, partition } from 'lodash';
 import { i18n } from '@kbn/i18n';
-import { parse, TinymathLocation } from '@kbn/tinymath';
+import { parse, TinymathLocation, TinymathVariable } from '@kbn/tinymath';
 import type { TinymathAST, TinymathFunction, TinymathNamedArgument } from '@kbn/tinymath';
 import { esKuery, esQuery } from '../../../../../../../../src/plugins/data/public';
 import {
@@ -63,6 +63,19 @@ interface ValidationErrors {
     message: string;
     type: {};
   };
+  tooManyFirstArguments: {
+    message: string;
+    type: {
+      operation: string;
+      type: string;
+      text: string;
+      supported?: number;
+    };
+  };
+  wrongArgument: {
+    message: string;
+    type: { operation: string; text: string; type: string };
+  };
 }
 
 type ErrorTypes = keyof ValidationErrors;
@@ -276,6 +289,25 @@ function getMessageFromId<K extends ErrorTypes>({
         defaultMessage: 'Use only one of kql= or lucene=, not both',
       });
       break;
+    case 'tooManyFirstArguments':
+      message = i18n.translate('xpack.lens.indexPattern.formulaOperationTooManyFirstArguments', {
+        defaultMessage:
+          'The operation {operation} in the Formula requires a {supported, plural, one {single} other {supported}} {type}, found: {text}',
+        values: {
+          operation: out.operation,
+          text: out.text,
+          type: out.type,
+          supported: out.supported || 1,
+        },
+      });
+      break;
+    case 'wrongArgument':
+      message = i18n.translate('xpack.lens.indexPattern.formulaOperationwrongArgument', {
+        defaultMessage:
+          'The operation {operation} in the Formula does not support {type} parameters, found: {text}',
+        values: { operation: out.operation, text: out.text, type: out.type },
+      });
+      break;
     // case 'mathRequiresFunction':
     //   message = i18n.translate('xpack.lens.indexPattern.formulaMathRequiresFunctionLabel', {
     //     defaultMessage; 'The function {name} requires an Elasticsearch function',
@@ -531,14 +563,16 @@ function runFullASTValidation(
     } else {
       if (nodeOperation.input === 'field') {
         if (shouldHaveFieldArgument(node)) {
-          if (!isFirstArgumentValidType(firstArg, 'variable')) {
+          if (!isArgumentValidType(firstArg, 'variable')) {
             if (isMathNode(firstArg)) {
               errors.push(
                 getMessageFromId({
                   messageId: 'wrongFirstArgument',
                   values: {
                     operation: node.name,
-                    type: 'field',
+                    type: i18n.translate('xpack.lens.indexPattern.formulaFieldValue', {
+                      defaultMessage: 'field',
+                    }),
                     argument: `math operation`,
                   },
                   locations: node.location ? [node.location] : [],
@@ -550,7 +584,9 @@ function runFullASTValidation(
                   messageId: 'wrongFirstArgument',
                   values: {
                     operation: node.name,
-                    type: 'field',
+                    type: i18n.translate('xpack.lens.indexPattern.formulaFieldValue', {
+                      defaultMessage: 'field',
+                    }),
                     argument:
                       getValueOrName(firstArg) ||
                       i18n.translate('xpack.lens.indexPattern.formulaNoFieldForOperation', {
@@ -561,6 +597,25 @@ function runFullASTValidation(
                 })
               );
             }
+          } else {
+            // If the first argument is valid proceed with the other arguments validation
+            const fieldErrors = validateFieldArguments(node, variables, {
+              isFieldOperation: true,
+              firstArg,
+            });
+            if (fieldErrors.length) {
+              errors.push(...fieldErrors);
+            }
+          }
+          const functionErrors = validateFunctionArguments(node, functions, 0, {
+            isFieldOperation: true,
+            type: i18n.translate('xpack.lens.indexPattern.formulaFieldValue', {
+              defaultMessage: 'field',
+            }),
+            firstArgValidation: false,
+          });
+          if (functionErrors.length) {
+            errors.push(...functionErrors);
           }
         } else {
           // Named arguments only
@@ -602,16 +657,20 @@ function runFullASTValidation(
       if (nodeOperation.input === 'fullReference') {
         // What about fn(7 + 1)? We may want to allow that
         // In general this should be handled down the Esaggs route rather than here
-        if (
-          !isFirstArgumentValidType(firstArg, 'function') ||
-          (isMathNode(firstArg) && validateMathNodes(firstArg, missingVariablesSet).length)
-        ) {
+        const isFirstArgumentNotValid = Boolean(
+          !isArgumentValidType(firstArg, 'function') ||
+            (isMathNode(firstArg) && validateMathNodes(firstArg, missingVariablesSet).length)
+        );
+        // First field has a special handling
+        if (isFirstArgumentNotValid) {
           errors.push(
             getMessageFromId({
               messageId: 'wrongFirstArgument',
               values: {
                 operation: node.name,
-                type: 'operation',
+                type: i18n.translate('xpack.lens.indexPattern.formulaOperationValue', {
+                  defaultMessage: 'operation',
+                }),
                 argument:
                   getValueOrName(firstArg) ||
                   i18n.translate('xpack.lens.indexPattern.formulaNoOperation', {
@@ -622,6 +681,21 @@ function runFullASTValidation(
             })
           );
         }
+        // Check for multiple function passed
+        const requiredFunctions = nodeOperation.requiredReferences
+          ? nodeOperation.requiredReferences.length
+          : 1;
+        const functionErrors = validateFunctionArguments(node, functions, requiredFunctions, {
+          isFieldOperation: false,
+          firstArgValidation: isFirstArgumentNotValid,
+          type: i18n.translate('xpack.lens.indexPattern.formulaMetricValue', {
+            defaultMessage: 'metric',
+          }),
+        });
+        if (functionErrors.length) {
+          errors.push(...functionErrors);
+        }
+
         if (!canHaveParams(nodeOperation) && namedArguments.length) {
           errors.push(
             getMessageFromId({
@@ -633,6 +707,14 @@ function runFullASTValidation(
             })
           );
         } else {
+          // check for fields passed at any position
+          const fieldErrors = validateFieldArguments(node, variables, {
+            isFieldOperation: false,
+            firstArg,
+          });
+          if (fieldErrors.length) {
+            errors.push(...fieldErrors);
+          }
           const argumentsErrors = validateNameArguments(
             node,
             nodeOperation,
@@ -736,7 +818,7 @@ export function hasFunctionFieldArgument(type: string) {
   return !['count'].includes(type);
 }
 
-export function isFirstArgumentValidType(arg: TinymathAST, type: TinymathNodeTypes['type']) {
+export function isArgumentValidType(arg: TinymathAST | string, type: TinymathNodeTypes['type']) {
   return isObject(arg) && arg.type === type;
 }
 
@@ -812,3 +894,109 @@ export function validateMathNodes(root: TinymathAST, missingVariableSet: Set<str
   });
   return errors;
 }
+
+function validateFieldArguments(
+  node: TinymathFunction,
+  variables: Array<string | number | TinymathVariable>,
+  { isFieldOperation, firstArg }: { isFieldOperation: boolean; firstArg: TinymathAST }
+) {
+  const fields = variables.filter(
+    (arg) => isArgumentValidType(arg, 'variable') && !isMathNode(arg)
+  );
+  const errors = [];
+  if (isFieldOperation && (fields.length > 1 || (fields.length === 1 && fields[0] !== firstArg))) {
+    errors.push(
+      getMessageFromId({
+        messageId: 'tooManyFirstArguments',
+        values: {
+          operation: node.name,
+          type: i18n.translate('xpack.lens.indexPattern.formulaFieldValue', {
+            defaultMessage: 'field',
+          }),
+          supported: 1,
+          text: (fields as TinymathVariable[]).map(({ text }) => text).join(', '),
+        },
+        locations: node.location ? [node.location] : [],
+      })
+    );
+  }
+  if (!isFieldOperation && fields.length) {
+    errors.push(
+      getMessageFromId({
+        messageId: 'wrongArgument',
+        values: {
+          operation: node.name,
+          text: (fields as TinymathVariable[]).map(({ text }) => text).join(', '),
+          type: i18n.translate('xpack.lens.indexPattern.formulaFieldValue', {
+            defaultMessage: 'field',
+          }),
+        },
+        locations: node.location ? [node.location] : [],
+      })
+    );
+  }
+  return errors;
+}
+
+function validateFunctionArguments(
+  node: TinymathFunction,
+  functions: TinymathFunction[],
+  requiredFunctions: number = 0,
+  {
+    isFieldOperation,
+    firstArgValidation,
+    type,
+  }: { isFieldOperation: boolean; firstArgValidation: boolean; type: string }
+) {
+  const errors = [];
+  // For math operation let the native operation run its own validation
+  const [esOperations, mathOperations] = partition(functions, (arg) => !isMathNode(arg));
+  if (esOperations.length > requiredFunctions) {
+    if (isFieldOperation) {
+      errors.push(
+        getMessageFromId({
+          messageId: 'wrongArgument',
+          values: {
+            operation: node.name,
+            text: (esOperations as TinymathFunction[]).map(({ text }) => text).join(', '),
+            type: i18n.translate('xpack.lens.indexPattern.formulaMetricValue', {
+              defaultMessage: 'metric',
+            }),
+          },
+          locations: node.location ? [node.location] : [],
+        })
+      );
+    } else {
+      errors.push(
+        getMessageFromId({
+          messageId: 'tooManyFirstArguments',
+          values: {
+            operation: node.name,
+            type,
+            supported: requiredFunctions,
+            text: (esOperations as TinymathFunction[]).map(({ text }) => text).join(', '),
+          },
+          locations: node.location ? [node.location] : [],
+        })
+      );
+    }
+  }
+  // full reference operation have another way to handle math operations
+  if (
+    isFieldOperation &&
+    ((!firstArgValidation && mathOperations.length) || mathOperations.length > 1)
+  ) {
+    errors.push(
+      getMessageFromId({
+        messageId: 'wrongArgument',
+        values: {
+          operation: node.name,
+          type,
+          text: (mathOperations as TinymathFunction[]).map(({ text }) => text).join(', '),
+        },
+        locations: node.location ? [node.location] : [],
+      })
+    );
+  }
+  return errors;
+}