From 70cad5eb7567f229b12af6ff3ce9028d54567748 Mon Sep 17 00:00:00 2001 From: Drew Tate Date: Mon, 29 Jul 2024 08:33:42 -0600 Subject: [PATCH] [ES|QL] remove inaccurate values suggestions (#189228) ## Summary Today, we get a list of field names and functions where values should be suggested instead. Since we can't yet suggest values in most cases, this PR prevents any suggestions from being shown in a values context. ### Values suggestions hidden **Before** https://github.com/user-attachments/assets/ddf02092-4c61-4a80-8666-c9b4fca735be **After** https://github.com/user-attachments/assets/f1c50a8b-6bd1-4e44-b56f-68f8510e53f6 ### But not for index names However, index names are still suggested within quotes https://github.com/user-attachments/assets/a416220c-370b-4bb3-a1bc-c2d4bada18ca But not if a space is entered https://github.com/user-attachments/assets/83d3d1e4-b11b-4bdb-b250-c0f1575fe82f However, there were a few cases with quoted index names which I just couldn't find a good way to cover. They are recorded here https://github.com/elastic/kibana/pull/189228/files#diff-4a3b7269c26dc777be5c0b2a9a1d8f4b1897b7921f6d3e7a176defdf67e83fc6R910-R912 ### Checklist Delete any items that are not applicable to this PR. - [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: Stratoula Kalafateli --- .../src/autocomplete/autocomplete.test.ts | 23 ++++++ .../src/autocomplete/autocomplete.ts | 81 ++++++++++++------- .../src/autocomplete/types.ts | 7 +- .../src/shared/context.ts | 4 + 4 files changed, 87 insertions(+), 28 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 eceb0d87fe075..a19adb2674d6f 100644 --- a/packages/kbn-esql-validation-autocomplete/src/autocomplete/autocomplete.test.ts +++ b/packages/kbn-esql-validation-autocomplete/src/autocomplete/autocomplete.test.ts @@ -898,6 +898,29 @@ describe('autocomplete', () => { }); }); + describe('values suggestions', () => { + testSuggestions('FROM "a"', ['a', 'b'], undefined, 7, [ + , + [ + { name: 'a', hidden: false }, + { name: 'b', hidden: false }, + ], + ]); + testSuggestions('FROM " "', [], ' '); + // TODO — re-enable these tests when we can support this case + testSuggestions.skip('FROM " a"', [], undefined, 9); + testSuggestions.skip('FROM "foo b"', [], undefined, 11); + testSuggestions('FROM a | WHERE tags == " "', [], ' '); + testSuggestions('FROM a | WHERE tags == """ """', [], ' '); + testSuggestions('FROM a | WHERE tags == "a"', [], undefined, 25); + testSuggestions('FROM a | EVAL tags == " "', [], ' '); + testSuggestions('FROM a | EVAL tags == "a"', [], undefined, 24); + testSuggestions('FROM a | STATS tags == " "', [], ' '); + testSuggestions('FROM a | STATS tags == "a"', [], undefined, 25); + testSuggestions('FROM a | GROK "a" "%{WORD:firstWord}"', [], undefined, 16); + testSuggestions('FROM a | DISSECT "a" "%{WORD:firstWord}"', [], undefined, 19); + }); + describe('callbacks', () => { it('should send the fields query without the last command', async () => { const callbackMocks = createCustomCallbackMocks(undefined, undefined, undefined); diff --git a/packages/kbn-esql-validation-autocomplete/src/autocomplete/autocomplete.ts b/packages/kbn-esql-validation-autocomplete/src/autocomplete/autocomplete.ts index 45c0563ac833e..807bc32ba82b0 100644 --- a/packages/kbn-esql-validation-autocomplete/src/autocomplete/autocomplete.ts +++ b/packages/kbn-esql-validation-autocomplete/src/autocomplete/autocomplete.ts @@ -146,34 +146,36 @@ function getFinalSuggestions({ comma }: { comma?: boolean } = { comma: true }) { * @param text * @returns */ -function countBracketsUnclosed(bracketType: '(' | '[', text: string) { +function countBracketsUnclosed(bracketType: '(' | '[' | '"' | '"""', text: string) { const stack = []; - const closingBrackets = { '(': ')', '[': ']' }; - for (const char of text) { - if (char === bracketType) { - stack.push(bracketType); - } else if (char === closingBrackets[bracketType]) { + const closingBrackets = { '(': ')', '[': ']', '"': '"', '"""': '"""' }; + for (let i = 0; i < text.length; i++) { + const substr = text.substring(i, i + bracketType.length); + if (substr === closingBrackets[bracketType] && stack.length) { stack.pop(); + } else if (substr === bracketType) { + stack.push(bracketType); } } return stack.length; } -export async function suggest( - fullText: string, - offset: number, - context: EditorContext, - astProvider: AstProviderFn, - resourceRetriever?: ESQLCallbacks -): Promise { - const innerText = fullText.substring(0, offset); - - let finalText = innerText; - +/** + * This function attempts to correct the syntax of a partial query to make it valid. + * + * This is important because a syntactically-invalid query will not generate a good AST. + * + * @param _query + * @param context + * @returns + */ +function correctQuerySyntax(_query: string, context: EditorContext) { + let query = _query; // check if all brackets are closed, otherwise close them - const unclosedRoundBrackets = countBracketsUnclosed('(', finalText); - const unclosedSquaredBrackets = countBracketsUnclosed('[', finalText); - const unclosedBrackets = unclosedRoundBrackets + unclosedSquaredBrackets; + const unclosedRoundBrackets = countBracketsUnclosed('(', query); + const unclosedSquaredBrackets = countBracketsUnclosed('[', query); + const unclosedQuotes = countBracketsUnclosed('"', query); + const unclosedTripleQuotes = countBracketsUnclosed('"""', query); // if it's a comma by the user or a forced trigger by a function argument suggestion // add a marker to make the expression still valid const charThatNeedMarkers = [',', ':']; @@ -182,29 +184,48 @@ export async function suggest( // monaco.editor.CompletionTriggerKind['Invoke'] === 0 (context.triggerKind === 0 && unclosedRoundBrackets === 0) || (context.triggerCharacter === ' ' && - (isMathFunction(innerText, offset) || - isComma(innerText.trimEnd()[innerText.trimEnd().length - 1]))) + (isMathFunction(query, query.length) || isComma(query.trimEnd()[query.trimEnd().length - 1]))) ) { - finalText = `${innerText.substring(0, offset)}${EDITOR_MARKER}${innerText.substring(offset)}`; + query += EDITOR_MARKER; } + // if there are unclosed brackets, close them - if (unclosedBrackets) { + if (unclosedRoundBrackets || unclosedSquaredBrackets || unclosedQuotes) { for (const [char, count] of [ + ['"""', unclosedTripleQuotes], + ['"', unclosedQuotes], [')', unclosedRoundBrackets], [']', unclosedSquaredBrackets], ]) { if (count) { // inject the closing brackets - finalText += Array(count).fill(char).join(''); + query += Array(count).fill(char).join(''); } } } - const { ast } = await astProvider(finalText); + return query; +} + +export async function suggest( + fullText: string, + offset: number, + context: EditorContext, + astProvider: AstProviderFn, + resourceRetriever?: ESQLCallbacks +): Promise { + const innerText = fullText.substring(0, offset); + + const correctedQuery = correctQuerySyntax(innerText, context); + + const { ast } = await astProvider(correctedQuery); const astContext = getAstContext(innerText, ast, offset); // build the correct query to fetch the list of fields - const queryForFields = getQueryForFields(buildQueryUntilPreviousCommand(ast, finalText), ast); + const queryForFields = getQueryForFields( + buildQueryUntilPreviousCommand(ast, correctedQuery), + ast + ); const { getFieldsByType, getFieldsMap } = getFieldsByTypeRetriever( queryForFields, resourceRetriever @@ -511,6 +532,12 @@ async function getExpressionSuggestionsByType( const commandDef = getCommandDefinition(command.name); const { argIndex, prevIndex, lastArg, nodeArg } = extractArgMeta(command, node); + // TODO - this is a workaround because it was too difficult to handle this case in a generic way :( + if (commandDef.name === 'from' && node && isSourceItem(node) && /\s/.test(node.name)) { + // FROM " " + return []; + } + // A new expression is considered either // * just after a command name => i.e. ... | STATS // * or after a comma => i.e. STATS fieldA, diff --git a/packages/kbn-esql-validation-autocomplete/src/autocomplete/types.ts b/packages/kbn-esql-validation-autocomplete/src/autocomplete/types.ts index b4a6e62f24b8e..f431eb9a27145 100644 --- a/packages/kbn-esql-validation-autocomplete/src/autocomplete/types.ts +++ b/packages/kbn-esql-validation-autocomplete/src/autocomplete/types.ts @@ -62,6 +62,11 @@ export interface SuggestionRawDefinition { export interface EditorContext { /** The actual char that triggered the suggestion (1 single char) */ triggerCharacter?: string; - /** The type of trigger id. triggerKind = 0 is a programmatic trigger, while any other non-zero value is currently ignored. */ + /** + * monaco.editor.CompletionTriggerKind + * + * 0 is "Invoke" (user starts typing a word) + * 1 is "Trigger character" (user types a trigger character) + */ triggerKind: number; } diff --git a/packages/kbn-esql-validation-autocomplete/src/shared/context.ts b/packages/kbn-esql-validation-autocomplete/src/shared/context.ts index ce936b8fc1cdf..f9fd248228d0c 100644 --- a/packages/kbn-esql-validation-autocomplete/src/shared/context.ts +++ b/packages/kbn-esql-validation-autocomplete/src/shared/context.ts @@ -152,6 +152,10 @@ function isBuiltinFunction(node: ESQLFunction) { export function getAstContext(queryString: string, ast: ESQLAst, offset: number) { const { command, option, setting, node } = findAstPosition(ast, offset); if (node) { + if (node.type === 'literal' && node.literalType === 'string') { + // command ... "" + return { type: 'value' as const, command, node, option, setting }; + } if (node.type === 'function') { if (['in', 'not_in'].includes(node.name) && Array.isArray(node.args[1])) { // command ... a in ( )