From e1c0cef15dd7f771a621db0230bf4cddcf10815b Mon Sep 17 00:00:00 2001 From: Drew Tate Date: Wed, 23 Oct 2024 07:49:38 -0600 Subject: [PATCH] [ES|QL] Create validation errors for unknown parameters (#197334) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Summary Follow-up from https://github.com/elastic/kibana/pull/195989. We discussed as a team and decided to show validation errors when an unknown variable is used as an argument to subsequent functions. **Before** Screenshot 2024-10-22 at 1 41 08 PM **After** Screenshot 2024-10-22 at 1 41 29 PM --- .../src/shared/helpers.ts | 2 +- .../validation/__tests__/functions.test.ts | 396 ++++++++++-------- 2 files changed, 226 insertions(+), 172 deletions(-) diff --git a/packages/kbn-esql-validation-autocomplete/src/shared/helpers.ts b/packages/kbn-esql-validation-autocomplete/src/shared/helpers.ts index 31c2c01a11404..2392a44814997 100644 --- a/packages/kbn-esql-validation-autocomplete/src/shared/helpers.ts +++ b/packages/kbn-esql-validation-autocomplete/src/shared/helpers.ts @@ -465,7 +465,7 @@ export function checkFunctionArgMatchesDefinition( const wrappedTypes: Array<(typeof validHit)['type']> = Array.isArray(validHit.type) ? validHit.type : [validHit.type]; - return wrappedTypes.some((ct) => ct === argType || ct === 'null' || ct === 'unknown'); + return wrappedTypes.some((ct) => ct === argType || ct === 'null'); } if (arg.type === 'inlineCast') { const lowerArgType = argType?.toLowerCase(); diff --git a/packages/kbn-esql-validation-autocomplete/src/validation/__tests__/functions.test.ts b/packages/kbn-esql-validation-autocomplete/src/validation/__tests__/functions.test.ts index a3934c1a35627..ec0fbe5395334 100644 --- a/packages/kbn-esql-validation-autocomplete/src/validation/__tests__/functions.test.ts +++ b/packages/kbn-esql-validation-autocomplete/src/validation/__tests__/functions.test.ts @@ -505,67 +505,41 @@ describe('function validation', () => { ['Invalid option ["foo"] for test. Supported options: ["ASC", "DESC"].'] ); }); - }); - describe('command/option support', () => { - it('validates command support', async () => { + it('validates values of type unknown', async () => { setTestFunctions([ { - name: 'eval_fn', + name: 'test1', type: 'eval', description: '', supportedCommands: ['eval'], signatures: [ { - params: [], - returnType: 'keyword', - }, - ], - }, - { - name: 'stats_fn', - type: 'agg', - description: '', - supportedCommands: ['stats'], - signatures: [ - { - params: [], - returnType: 'keyword', - }, - ], - }, - { - name: 'row_fn', - type: 'eval', - description: '', - supportedCommands: ['row'], - signatures: [ - { - params: [], + params: [{ name: 'arg1', type: 'keyword' }], returnType: 'keyword', }, ], }, { - name: 'where_fn', + name: 'test2', type: 'eval', description: '', - supportedCommands: ['where'], + supportedCommands: ['eval'], signatures: [ { - params: [], + params: [{ name: 'arg1', type: 'keyword' }], returnType: 'keyword', }, ], }, { - name: 'sort_fn', + name: 'test3', type: 'eval', description: '', - supportedCommands: ['sort'], + supportedCommands: ['eval'], signatures: [ { - params: [], + params: [{ name: 'arg1', type: 'long' }], returnType: 'keyword', }, ], @@ -573,155 +547,235 @@ describe('function validation', () => { ]); const { expectErrors } = await setup(); + await expectErrors( + `FROM a_index + | EVAL foo = TEST1(1.) + | EVAL TEST2(foo) + | EVAL TEST3(foo)`, + [ + 'Argument of [test1] must be [keyword], found value [1.] type [double]', + 'Argument of [test2] must be [keyword], found value [foo] type [unknown]', + 'Argument of [test3] must be [long], found value [foo] type [unknown]', + ] + ); + }); - await expectErrors('FROM a_index | EVAL EVAL_FN()', []); - await expectErrors('FROM a_index | SORT SORT_FN()', []); - await expectErrors('FROM a_index | STATS STATS_FN()', []); - await expectErrors('ROW ROW_FN()', []); - await expectErrors('FROM a_index | WHERE WHERE_FN()', []); + describe('command/option support', () => { + it('validates command support', async () => { + setTestFunctions([ + { + name: 'eval_fn', + type: 'eval', + description: '', + supportedCommands: ['eval'], + signatures: [ + { + params: [], + returnType: 'keyword', + }, + ], + }, + { + name: 'stats_fn', + type: 'agg', + description: '', + supportedCommands: ['stats'], + signatures: [ + { + params: [], + returnType: 'keyword', + }, + ], + }, + { + name: 'row_fn', + type: 'eval', + description: '', + supportedCommands: ['row'], + signatures: [ + { + params: [], + returnType: 'keyword', + }, + ], + }, + { + name: 'where_fn', + type: 'eval', + description: '', + supportedCommands: ['where'], + signatures: [ + { + params: [], + returnType: 'keyword', + }, + ], + }, + { + name: 'sort_fn', + type: 'eval', + description: '', + supportedCommands: ['sort'], + signatures: [ + { + params: [], + returnType: 'keyword', + }, + ], + }, + ]); - await expectErrors('FROM a_index | EVAL SORT_FN()', [ - 'EVAL does not support function sort_fn', - ]); - await expectErrors('FROM a_index | SORT STATS_FN()', [ - 'SORT does not support function stats_fn', - ]); - await expectErrors('FROM a_index | STATS ROW_FN()', [ - 'At least one aggregation function required in [STATS], found [ROW_FN()]', - 'STATS does not support function row_fn', - ]); - await expectErrors('ROW WHERE_FN()', ['ROW does not support function where_fn']); - await expectErrors('FROM a_index | WHERE EVAL_FN()', [ - 'WHERE does not support function eval_fn', - ]); - }); + const { expectErrors } = await setup(); - it('validates option support', async () => { - setTestFunctions([ - { - name: 'supports_by_option', - type: 'eval', - description: '', - supportedCommands: ['eval'], - supportedOptions: ['by'], - signatures: [ - { - params: [], - returnType: 'keyword', - }, - ], - }, - { - name: 'does_not_support_by_option', - type: 'eval', - description: '', - supportedCommands: ['eval'], - supportedOptions: [], - signatures: [ - { - params: [], - returnType: 'keyword', - }, - ], - }, + await expectErrors('FROM a_index | EVAL EVAL_FN()', []); + await expectErrors('FROM a_index | SORT SORT_FN()', []); + await expectErrors('FROM a_index | STATS STATS_FN()', []); + await expectErrors('ROW ROW_FN()', []); + await expectErrors('FROM a_index | WHERE WHERE_FN()', []); - { - name: 'agg_fn', - type: 'agg', - description: '', - supportedCommands: ['stats'], - supportedOptions: [], - signatures: [ - { - params: [], - returnType: 'keyword', - }, - ], - }, - ]); + await expectErrors('FROM a_index | EVAL SORT_FN()', [ + 'EVAL does not support function sort_fn', + ]); + await expectErrors('FROM a_index | SORT STATS_FN()', [ + 'SORT does not support function stats_fn', + ]); + await expectErrors('FROM a_index | STATS ROW_FN()', [ + 'At least one aggregation function required in [STATS], found [ROW_FN()]', + 'STATS does not support function row_fn', + ]); + await expectErrors('ROW WHERE_FN()', ['ROW does not support function where_fn']); + await expectErrors('FROM a_index | WHERE EVAL_FN()', [ + 'WHERE does not support function eval_fn', + ]); + }); - const { expectErrors } = await setup(); + it('validates option support', async () => { + setTestFunctions([ + { + name: 'supports_by_option', + type: 'eval', + description: '', + supportedCommands: ['eval'], + supportedOptions: ['by'], + signatures: [ + { + params: [], + returnType: 'keyword', + }, + ], + }, + { + name: 'does_not_support_by_option', + type: 'eval', + description: '', + supportedCommands: ['eval'], + supportedOptions: [], + signatures: [ + { + params: [], + returnType: 'keyword', + }, + ], + }, - await expectErrors('FROM a_index | STATS AGG_FN() BY SUPPORTS_BY_OPTION()', []); - await expectErrors('FROM a_index | STATS AGG_FN() BY DOES_NOT_SUPPORT_BY_OPTION()', [ - 'STATS BY does not support function does_not_support_by_option', - ]); + { + name: 'agg_fn', + type: 'agg', + description: '', + supportedCommands: ['stats'], + supportedOptions: [], + signatures: [ + { + params: [], + returnType: 'keyword', + }, + ], + }, + ]); + + const { expectErrors } = await setup(); + + await expectErrors('FROM a_index | STATS AGG_FN() BY SUPPORTS_BY_OPTION()', []); + await expectErrors('FROM a_index | STATS AGG_FN() BY DOES_NOT_SUPPORT_BY_OPTION()', [ + 'STATS BY does not support function does_not_support_by_option', + ]); + }); }); - }); - describe('nested functions', () => { - it('supports deep nesting', async () => { - setTestFunctions([ - { - name: 'test', - type: 'eval', - description: '', - supportedCommands: ['eval'], - signatures: [ - { - params: [{ name: 'arg1', type: 'keyword' }], - returnType: 'integer', - }, - ], - }, - { - name: 'test2', - type: 'eval', - description: '', - supportedCommands: ['eval'], - signatures: [ - { - params: [{ name: 'arg1', type: 'integer' }], - returnType: 'keyword', - }, - ], - }, - ]); + describe('nested functions', () => { + it('supports deep nesting', async () => { + setTestFunctions([ + { + name: 'test', + type: 'eval', + description: '', + supportedCommands: ['eval'], + signatures: [ + { + params: [{ name: 'arg1', type: 'keyword' }], + returnType: 'integer', + }, + ], + }, + { + name: 'test2', + type: 'eval', + description: '', + supportedCommands: ['eval'], + signatures: [ + { + params: [{ name: 'arg1', type: 'integer' }], + returnType: 'keyword', + }, + ], + }, + ]); - const { expectErrors } = await setup(); + const { expectErrors } = await setup(); - await expectErrors('FROM a_index | EVAL TEST(TEST2(TEST(TEST2(1))))', []); - }); + await expectErrors('FROM a_index | EVAL TEST(TEST2(TEST(TEST2(1))))', []); + }); - it("doesn't allow nested aggregation functions", async () => { - setTestFunctions([ - { - name: 'agg_fn', - type: 'agg', - description: '', - supportedCommands: ['stats'], - signatures: [ - { - params: [{ name: 'arg1', type: 'keyword' }], - returnType: 'keyword', - }, - ], - }, - { - name: 'scalar_fn', - type: 'eval', - description: '', - supportedCommands: ['stats'], - signatures: [ - { - params: [{ name: 'arg1', type: 'keyword' }], - returnType: 'keyword', - }, - ], - }, - ]); + it("doesn't allow nested aggregation functions", async () => { + setTestFunctions([ + { + name: 'agg_fn', + type: 'agg', + description: '', + supportedCommands: ['stats'], + signatures: [ + { + params: [{ name: 'arg1', type: 'keyword' }], + returnType: 'keyword', + }, + ], + }, + { + name: 'scalar_fn', + type: 'eval', + description: '', + supportedCommands: ['stats'], + signatures: [ + { + params: [{ name: 'arg1', type: 'keyword' }], + returnType: 'keyword', + }, + ], + }, + ]); - const { expectErrors } = await setup(); + const { expectErrors } = await setup(); - await expectErrors('FROM a_index | STATS AGG_FN(AGG_FN(""))', [ - 'Aggregate function\'s parameters must be an attribute, literal or a non-aggregation function; found [AGG_FN("")] of type [keyword]', - ]); - // @TODO — enable this test when we have fixed this bug - // await expectErrors('FROM a_index | STATS AGG_FN(SCALAR_FN(AGG_FN("")))', [ - // 'No nested aggregation functions.', - // ]); - }); + await expectErrors('FROM a_index | STATS AGG_FN(AGG_FN(""))', [ + 'Aggregate function\'s parameters must be an attribute, literal or a non-aggregation function; found [AGG_FN("")] of type [keyword]', + ]); + // @TODO — enable this test when we have fixed this bug + // await expectErrors('FROM a_index | STATS AGG_FN(SCALAR_FN(AGG_FN("")))', [ + // 'No nested aggregation functions.', + // ]); + }); - // @TODO — test function aliases + // @TODO — test function aliases + }); }); });