Skip to content

Commit

Permalink
[Lens] Formula: add validation for multiple field/metrics (elastic#10…
Browse files Browse the repository at this point in the history 
…4092) (elastic#105395)

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

Co-authored-by: Marco Liberati <dej611@users.noreply.github.com>
  • Loading branch information
@kibanamachine @dej611
kibanamachine and dej611 committed Jul 13, 2021
1 parent dc4c4c9 commit 966d682
Show file tree
Hide file tree
Showing 3 changed files with 265 additions and 12 deletions.
67 changes: 66 additions & 1 deletion ...ugins/lens/public/indexpattern_datasource/operations/definitions/formula/formula.test.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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: ({
Expand Down Expand Up @@ -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!(
Expand Down Expand Up @@ -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", () => {
Expand Down
2 changes: 1 addition & 1 deletion x-pack/plugins/lens/public/indexpattern_datasource/operations/definitions/formula/util.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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];
}

Expand Down
208 changes: 198 additions & 10 deletions .../plugins/lens/public/indexpattern_datasource/operations/definitions/formula/validation.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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',
Expand Down Expand Up @@ -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] : [],
Expand All @@ -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', {
Expand All @@ -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
Expand Down Expand Up @@ -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', {
Expand All @@ -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({
Expand All @@ -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,
Expand Down Expand Up @@ -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;
}

Expand Down Expand Up @@ -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;
}

0 comments on commit 966d682

Please sign in to comment.