Skip to content

Commit

Permalink
[ES|QL] remove inaccurate values suggestions (elastic#189228)
Browse files Browse the repository at this point in the history
## 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 <[email protected]>
  • Loading branch information
drewdaemon and stratoula authored Jul 29, 2024
1 parent 35f6376 commit 70cad5e
Show file tree
Hide file tree
Showing 4 changed files with 87 additions and 28 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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<SuggestionRawDefinition[]> {
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 = [',', ':'];
Expand All @@ -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<SuggestionRawDefinition[]> {
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
Expand Down Expand Up @@ -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 " <suggest>"
return [];
}

// A new expression is considered either
// * just after a command name => i.e. ... | STATS <here>
// * or after a comma => i.e. STATS fieldA, <here>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
Original file line number Diff line number Diff line change
Expand Up @@ -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 ... "<here>"
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 ( <here> )
Expand Down

0 comments on commit 70cad5e

Please sign in to comment.