From dffe0b571899b2ed0c71ee9f090095311d4d2b55 Mon Sep 17 00:00:00 2001 From: Drew Tate Date: Thu, 10 Oct 2024 09:33:55 -0600 Subject: [PATCH] [ES|QL] only suggest pipe at the end of the field list (#195679) ## Summary Close https://github.com/elastic/kibana/issues/191100 ### Improvements 1. You no longer get a comma suggestion when you're out of fields... https://github.com/user-attachments/assets/3ed3617b-99e2-44a5-917e-294b98f16ef4 2. Fixed https://github.com/elastic/kibana/issues/191100 ### Checklist - [x] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios --------- Co-authored-by: Elastic Machine --- .../src/autocomplete/autocomplete.test.ts | 83 +++++++++++++++---- .../src/autocomplete/autocomplete.ts | 38 +++++---- 2 files changed, 88 insertions(+), 33 deletions(-) diff --git a/packages/kbn-esql-validation-autocomplete/src/autocomplete/autocomplete.test.ts b/packages/kbn-esql-validation-autocomplete/src/autocomplete/autocomplete.test.ts index 84779f1dd36b5..a0a4a359c5ff6 100644 --- a/packages/kbn-esql-validation-autocomplete/src/autocomplete/autocomplete.test.ts +++ b/packages/kbn-esql-validation-autocomplete/src/autocomplete/autocomplete.test.ts @@ -387,6 +387,23 @@ describe('autocomplete', () => { '```````````````````````````````round(doubleField) + 1```````````````` + 1```````` + 1```` + 1`` + 1`', ] ); + + it('should not suggest already-used fields and variables', async () => { + const { suggest: suggestTest } = await setup(); + const getSuggestions = async (query: string) => + (await suggestTest(query)).map((value) => value.text); + + expect(await getSuggestions('from a_index | EVAL foo = 1 | KEEP /')).toContain('foo'); + expect(await getSuggestions('from a_index | EVAL foo = 1 | KEEP foo, /')).not.toContain( + 'foo' + ); + expect(await getSuggestions('from a_index | EVAL foo = 1 | KEEP /')).toContain( + 'doubleField' + ); + expect( + await getSuggestions('from a_index | EVAL foo = 1 | KEEP doubleField, /') + ).not.toContain('doubleField'); + }); }); } @@ -1111,11 +1128,14 @@ describe('autocomplete', () => { ]); }); - describe('KEEP ', () => { + describe.each(['KEEP', 'DROP'])('%s ', (commandName) => { // KEEP field - testSuggestions('FROM a | KEEP /', getFieldNamesByType('any').map(attachTriggerCommand)); testSuggestions( - 'FROM a | KEEP d/', + `FROM a | ${commandName} /`, + getFieldNamesByType('any').map(attachTriggerCommand) + ); + testSuggestions( + `FROM a | ${commandName} d/`, getFieldNamesByType('any') .map((text) => ({ text, @@ -1124,11 +1144,11 @@ describe('autocomplete', () => { .map(attachTriggerCommand) ); testSuggestions( - 'FROM a | KEEP doubleFiel/', + `FROM a | ${commandName} doubleFiel/`, getFieldNamesByType('any').map(attachTriggerCommand) ); testSuggestions( - 'FROM a | KEEP doubleField/', + `FROM a | ${commandName} doubleField/`, ['doubleField, ', 'doubleField | '] .map((text) => ({ text, @@ -1141,7 +1161,7 @@ describe('autocomplete', () => { // Let's get funky with the field names testSuggestions( - 'FROM a | KEEP @timestamp/', + `FROM a | ${commandName} @timestamp/`, ['@timestamp, ', '@timestamp | '] .map((text) => ({ text, @@ -1150,10 +1170,15 @@ describe('autocomplete', () => { })) .map(attachTriggerCommand), undefined, - [[{ name: '@timestamp', type: 'date' }]] + [ + [ + { name: '@timestamp', type: 'date' }, + { name: 'utc_stamp', type: 'date' }, + ], + ] ); testSuggestions( - 'FROM a | KEEP foo.bar/', + `FROM a | ${commandName} foo.bar/`, ['foo.bar, ', 'foo.bar | '] .map((text) => ({ text, @@ -1162,26 +1187,34 @@ describe('autocomplete', () => { })) .map(attachTriggerCommand), undefined, - [[{ name: 'foo.bar', type: 'double' }]] + [ + [ + { name: 'foo.bar', type: 'double' }, + { name: 'baz', type: 'date' }, + ], + ] ); describe('escaped field names', () => { // This isn't actually the behavior we want, but this test is here // to make sure no weird suggestions start cropping up in this case. - testSuggestions('FROM a | KEEP `foo.bar`/', ['foo.bar'], undefined, [ + testSuggestions(`FROM a | ${commandName} \`foo.bar\`/`, ['foo.bar'], undefined, [ [{ name: 'foo.bar', type: 'double' }], ]); // @todo re-enable these tests when we can use AST to support this case - testSuggestions.skip('FROM a | KEEP `foo.bar`/', ['foo.bar, ', 'foo.bar | '], undefined, [ - [{ name: 'foo.bar', type: 'double' }], - ]); testSuggestions.skip( - 'FROM a | KEEP `foo`.`bar`/', + `FROM a | ${commandName} \`foo.bar\`/`, ['foo.bar, ', 'foo.bar | '], undefined, [[{ name: 'foo.bar', type: 'double' }]] ); - testSuggestions.skip('FROM a | KEEP `any#Char$Field`/', [ + testSuggestions.skip( + `FROM a | ${commandName} \`foo\`.\`bar\`/`, + ['foo.bar, ', 'foo.bar | '], + undefined, + [[{ name: 'foo.bar', type: 'double' }]] + ); + testSuggestions.skip(`FROM a | ${commandName} \`any#Char$Field\`/`, [ '`any#Char$Field`, ', '`any#Char$Field` | ', ]); @@ -1189,12 +1222,28 @@ describe('autocomplete', () => { // Subsequent fields testSuggestions( - 'FROM a | KEEP doubleField, dateFiel/', + `FROM a | ${commandName} doubleField, dateFiel/`, getFieldNamesByType('any') .filter((s) => s !== 'doubleField') .map(attachTriggerCommand) ); - testSuggestions('FROM a | KEEP doubleField, dateField/', ['dateField, ', 'dateField | ']); + testSuggestions(`FROM a | ${commandName} doubleField, dateField/`, [ + 'dateField, ', + 'dateField | ', + ]); + + // out of fields + testSuggestions( + `FROM a | ${commandName} doubleField, dateField/`, + ['dateField | '], + undefined, + [ + [ + { name: 'doubleField', type: 'double' }, + { name: 'dateField', type: 'date' }, + ], + ] + ); }); }); }); diff --git a/packages/kbn-esql-validation-autocomplete/src/autocomplete/autocomplete.ts b/packages/kbn-esql-validation-autocomplete/src/autocomplete/autocomplete.ts index 2433f5d496521..6f9fb66a8c715 100644 --- a/packages/kbn-esql-validation-autocomplete/src/autocomplete/autocomplete.ts +++ b/packages/kbn-esql-validation-autocomplete/src/autocomplete/autocomplete.ts @@ -627,7 +627,7 @@ async function getExpressionSuggestionsByType( literals: argDef.constantOnly, }, { - ignoreFields: isNewExpression + ignoreColumns: isNewExpression ? command.args.filter(isColumnItem).map(({ name }) => name) : [], } @@ -656,10 +656,15 @@ async function getExpressionSuggestionsByType( })); } - return [ - { ...pipeCompleteItem, text: ' | ' }, - { ...commaCompleteItem, text: ', ' }, - ].map((s) => ({ + const finalSuggestions = [{ ...pipeCompleteItem, text: ' | ' }]; + if (fieldSuggestions.length > 1) + // when we fix the editor marker, this should probably be checked against 0 instead of 1 + // this is because the last field in the AST is currently getting removed (because it contains + // the editor marker) so it is not included in the ignored list which is used to filter out + // existing fields above. + finalSuggestions.push({ ...commaCompleteItem, text: ', ' }); + + return finalSuggestions.map((s) => ({ ...s, filterText: fragment, text: fragment + s.text, @@ -1176,15 +1181,15 @@ async function getFieldsOrFunctionsSuggestions( }, { ignoreFn = [], - ignoreFields = [], + ignoreColumns = [], }: { ignoreFn?: string[]; - ignoreFields?: string[]; + ignoreColumns?: string[]; } = {} ): Promise { const filteredFieldsByType = pushItUpInTheList( (await (fields - ? getFieldsByType(types, ignoreFields, { + ? getFieldsByType(types, ignoreColumns, { advanceCursor: commandName === 'sort', openSuggestions: commandName === 'sort', }) @@ -1195,7 +1200,10 @@ async function getFieldsOrFunctionsSuggestions( const filteredVariablesByType: string[] = []; if (variables) { for (const variable of variables.values()) { - if (types.includes('any') || types.includes(variable[0].type)) { + if ( + (types.includes('any') || types.includes(variable[0].type)) && + !ignoreColumns.includes(variable[0].name) + ) { filteredVariablesByType.push(variable[0].name); } } @@ -1515,7 +1523,7 @@ async function getListArgsSuggestions( fields: true, variables: anyVariables, }, - { ignoreFields: [firstArg.name, ...otherArgs.map(({ name }) => name)] } + { ignoreColumns: [firstArg.name, ...otherArgs.map(({ name }) => name)] } )) ); } @@ -1875,18 +1883,16 @@ async function getOptionArgsSuggestions( * for a given fragment of text in a generic way. A good example is * a field name. * - * When typing a field name, there are three scenarios + * When typing a field name, there are 2 scenarios * - * 1. user hasn't begun typing + * 1. field name is incomplete (includes the empty string) * KEEP / - * - * 2. user is typing a partial field name * KEEP fie/ * - * 3. user has typed a complete field name + * 2. field name is complete * KEEP field/ * - * This function provides a framework for handling all three scenarios in a clean way. + * This function provides a framework for detecting and handling both scenarios in a clean way. * * @param innerText - the query text before the current cursor position * @param isFragmentComplete — return true if the fragment is complete