Skip to content

Commit df2e76b

Browse files
[ES|QL] Fix nested editor marker leaks in autocomplete and introduce a shared autocomplete parsing boundary (#260618)
## Summary Closes #215186 **Disclaimer** : This PR restarts following the last comment on the issue. So I’m trying to clean up the ESQL commands as a consumers, without venturing into suicide missions. ------------------------------------------------------------------------------------------------------------------------------------- - Fixes the remaining editor marker leak for incomplete assignments in autocomplete: so nested marker nodes are also removed from the AST. - Introduces a shared autocomplete parsing boundary to centralize query repair, parsing, cursor-context extraction, and marker cleanup. - Removes redundant marker-aware checks from command/runtime paths where they are no longer needed. - Keeps the assignment-specific fallback in getAssignmentExpressionRoot(), since incomplete RHS still needs to behave like an empty expression in cases such as `RERANK ... ON col0 =`. --------- Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>
1 parent 1270467 commit df2e76b

29 files changed

Lines changed: 318 additions & 124 deletions

File tree

src/platform/packages/shared/kbn-esql-language/src/__tests__/commands/autocomplete.ts

Lines changed: 6 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,6 @@
1515
import { uniq } from 'lodash';
1616
import type { LicenseType } from '@kbn/licensing-types';
1717
import type { EsqlFieldType } from '@kbn/esql-types';
18-
import { Parser } from '@elastic/esql';
1918
import type { ESQLAstAllCommands } from '@elastic/esql/types';
2019
import type {
2120
ICommandCallbacks,
@@ -45,7 +44,7 @@ import { FunctionDefinitionTypes } from '../../commands/definitions/types';
4544
import { mockContext, getMockCallbacks } from './context_fixtures';
4645
import { getSafeInsertText } from '../../commands/definitions/utils';
4746
import { timeUnitsToSuggest } from '../../commands/definitions/constants';
48-
import { correctQuerySyntax, findAstPosition } from '../../commands/definitions/utils/ast';
47+
import { findAutocompleteAstPosition } from '../../language/shared/parse_for_autocomplete_query';
4948
import { FUNCTIONS_TO_IGNORE } from '../../commands/registry/eval/autocomplete';
5049
import { attachReplacementRanges } from '../../language/autocomplete/utils/prefix_range';
5150

@@ -87,24 +86,20 @@ export const suggest = async (
8786
) => Promise<ISuggestionItem[]>,
8887
offset?: number
8988
): Promise<ISuggestionItem[]> => {
90-
const innerText = query.substring(0, offset ?? query.length);
91-
const correctedQuery = correctQuerySyntax(innerText);
92-
const { root } = Parser.parse(correctedQuery, { withFormatting: true });
93-
const headerConstruction = root?.header?.find((cmd) => cmd.name === commandName);
94-
9589
const cursorPosition = offset ?? query.length;
90+
const { innerText, root, command } = findAutocompleteAstPosition(query, cursorPosition);
91+
const headerConstruction = root?.header?.find((cmd) => cmd.name === commandName);
92+
const targetCommand = headerConstruction ?? command;
9693

97-
const command = headerConstruction ?? findAstPosition(root, cursorPosition).command;
98-
99-
if (!command) {
94+
if (!targetCommand) {
10095
throw new Error(`${commandName.toUpperCase()} command not found in the parsed query`);
10196
}
10297

10398
const contextWithRoot = { ...context, rootAst: root };
10499

105100
const suggestions = await autocomplete(
106101
query,
107-
command,
102+
targetCommand,
108103
mockCallbacks,
109104
contextWithRoot,
110105
cursorPosition

src/platform/packages/shared/kbn-esql-language/src/commands/definitions/utils/ast.ts

Lines changed: 43 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@ import type {
1919
ESQLAstQueryExpression,
2020
} from '@elastic/esql/types';
2121
import { EDITOR_MARKER } from '../constants';
22+
import { TRAILING_COMMA_REGEX } from './shared';
2223

2324
export function isMarkerNode(node: ESQLAstItem | undefined): boolean {
2425
if (Array.isArray(node)) {
@@ -80,23 +81,61 @@ const removeMarkerNode = (node: ESQLAstExpression) => {
8081
});
8182
};
8283

83-
export function removeMarkerArgFromArgsList<T extends ESQLSingleAstItem | ESQLAstAllCommands>(
84+
function removeMarkerArgFromArgsList<T extends ESQLSingleAstItem | ESQLAstAllCommands>(
8485
node: T | undefined
8586
) {
8687
if (!node) {
8788
return;
8889
}
8990
if (node.type === 'command' || node.type === 'option' || node.type === 'function') {
90-
return {
91+
const cleanedNode = {
9192
...node,
93+
text: node.text.replace(EDITOR_MARKER, ''),
9294
args: node.args.filter(isNotMarkerNodeOrArray).map(mapToNonMarkerNode),
9395
};
96+
97+
if (cleanedNode.type === 'command' && 'expression' in cleanedNode && cleanedNode.expression) {
98+
cleanedNode.expression = isMarkerNode(cleanedNode.expression)
99+
? undefined
100+
: (mapToNonMarkerNode(cleanedNode.expression) as ESQLAstExpression);
101+
}
102+
103+
return cleanedNode;
94104
}
95105
return node;
96106
}
97107

98108
export function mapToNonMarkerNode(arg: ESQLAstItem): ESQLAstItem {
99-
return Array.isArray(arg) ? arg.filter(isNotMarkerNodeOrArray).map(mapToNonMarkerNode) : arg;
109+
if (Array.isArray(arg)) {
110+
return arg.filter(isNotMarkerNodeOrArray).map(mapToNonMarkerNode);
111+
}
112+
113+
if ('args' in arg) {
114+
return {
115+
...arg,
116+
text: arg.text.replace(EDITOR_MARKER, ''),
117+
args: arg.args.filter(isNotMarkerNodeOrArray).map(mapToNonMarkerNode) as typeof arg.args,
118+
} as typeof arg;
119+
}
120+
121+
if ('values' in arg) {
122+
return {
123+
...arg,
124+
text: arg.text.replace(EDITOR_MARKER, ''),
125+
values: arg.values
126+
.filter((value) => !isMarkerNode(value))
127+
.map((value) => mapToNonMarkerNode(value) as ESQLAstExpression) as typeof arg.values,
128+
} as typeof arg;
129+
}
130+
131+
if ('text' in arg && typeof arg.text === 'string' && arg.text.includes(EDITOR_MARKER)) {
132+
return {
133+
...arg,
134+
text: arg.text.replace(EDITOR_MARKER, ''),
135+
} as typeof arg;
136+
}
137+
138+
return arg;
100139
}
101140

102141
function cleanMarkerNode(node: ESQLSingleAstItem | undefined): ESQLSingleAstItem | undefined {
@@ -277,7 +316,6 @@ export function correctQuerySyntax(_query: string) {
277316
return query;
278317
}
279318

280-
const PROMQL_TRAILING_COMMA_REGEX = /,\s*$/;
281319
const PROMQL_TRAILING_COLON_REGEX = /:\s*$/;
282320

283321
/**
@@ -290,7 +328,7 @@ export function correctPromqlQuerySyntax(input: string): string {
290328

291329
if (
292330
!query.includes(EDITOR_MARKER) &&
293-
(PROMQL_TRAILING_COMMA_REGEX.test(query) || PROMQL_TRAILING_COLON_REGEX.test(query))
331+
(TRAILING_COMMA_REGEX.test(query) || PROMQL_TRAILING_COLON_REGEX.test(query))
294332
) {
295333
query += ` ${EDITOR_MARKER}`;
296334
}

src/platform/packages/shared/kbn-esql-language/src/commands/definitions/utils/autocomplete/expressions/operators/utils.ts

Lines changed: 1 addition & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,6 @@
99

1010
import { isList } from '@elastic/esql';
1111
import type { ESQLSingleAstItem } from '@elastic/esql/types';
12-
import { isMarkerNode } from '../../../ast';
1312
import { getOperatorSuggestion } from '../../../operators';
1413
import type { ISuggestionItem } from '../../../../../registry/types';
1514
import { logicalOperators } from '../../../../all_operators';
@@ -34,11 +33,7 @@ export function endsWithIsOrIsNotToken(innerText: string): boolean {
3433
}
3534

3635
export function isOperandMissing(operand: ESQLSingleAstItem | undefined): boolean {
37-
return (
38-
!operand ||
39-
isMarkerNode(operand) ||
40-
(operand?.type === 'unknown' && operand?.incomplete === true)
41-
);
36+
return !operand || (operand?.type === 'unknown' && operand?.incomplete === true);
4237
}
4338

4439
/** Returns true if we should suggest opening a list for the right operand */

src/platform/packages/shared/kbn-esql-language/src/commands/definitions/utils/autocomplete/expressions/positions/in_function.ts

Lines changed: 8 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -13,9 +13,7 @@ import { suggestForExpression } from '../suggestion_engine';
1313
import type { ExpressionContext } from '../types';
1414
import type { ISuggestionItem } from '../../../../../registry/types';
1515
import { buildExpressionFunctionParameterContext } from '../utils';
16-
17-
/** Matches comma followed by optional whitespace at end of text */
18-
const STARTING_NEW_PARAM_REGEX = /,\s*$/;
16+
import { TRAILING_COMMA_REGEX } from '../../../shared';
1917

2018
/** Suggests completions when cursor is inside a function call (e.g., CONCAT(field1, /)) */
2119
export async function suggestInFunction(ctx: ExpressionContext): Promise<ISuggestionItem[]> {
@@ -32,7 +30,12 @@ export async function suggestInFunction(ctx: ExpressionContext): Promise<ISugges
3230
} = ctx;
3331

3432
const functionExpression = expressionRoot as ESQLFunction;
35-
const paramContext = buildExpressionFunctionParameterContext(functionExpression, context);
33+
const startingNewParam = TRAILING_COMMA_REGEX.test(innerText);
34+
const paramContext = buildExpressionFunctionParameterContext(
35+
functionExpression,
36+
context,
37+
startingNewParam
38+
);
3639

3740
if (!paramContext) {
3841
return [];
@@ -69,7 +72,7 @@ function determineTargetExpression(
6972
innerText: string
7073
): ESQLSingleAstItem | undefined {
7174
const { args } = functionExpression;
72-
const startingNewParam = STARTING_NEW_PARAM_REGEX.test(innerText);
75+
const startingNewParam = TRAILING_COMMA_REGEX.test(innerText);
7376
const firstArgEmpty = isFirstArgumentEmpty(args, innerText);
7477

7578
const lastArg = args[args.length - 1];

src/platform/packages/shared/kbn-esql-language/src/commands/definitions/utils/autocomplete/expressions/utils.ts

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,6 @@ import { nullCheckOperators, inOperators } from '../../../all_operators';
1313
import type { ExpressionContext, FunctionParameterContext } from './types';
1414
import type { ICommandContext, ISuggestionItem } from '../../../../registry/types';
1515
import { getFunctionDefinition } from '../..';
16-
import { EDITOR_MARKER } from '../../../constants';
1716
import { resolveArgumentTypes } from '../../expressions';
1817
import type { SupportedDataType } from '../../../types';
1918
import {
@@ -63,7 +62,8 @@ export function matchesSpecialFunction(name: string, expected: SpecialFunctionNa
6362
*/
6463
export function buildExpressionFunctionParameterContext(
6564
fn: ESQLFunction,
66-
context?: ICommandContext
65+
context?: ICommandContext,
66+
shouldGetNextArgument = false
6767
): FunctionParameterContext | null {
6868
const fnDefinition = getFunctionDefinition(fn.name);
6969

@@ -76,7 +76,6 @@ export function buildExpressionFunctionParameterContext(
7676
unmappedFieldsStrategy: context?.unmappedFieldsStrategy,
7777
});
7878

79-
const shouldGetNextArgument = fn.text.includes(EDITOR_MARKER);
8079
let argIndex = Math.max(fn.args.length, 0);
8180
if (!shouldGetNextArgument && argIndex) {
8281
argIndex -= 1;

src/platform/packages/shared/kbn-esql-language/src/commands/definitions/utils/autocomplete/fields_list.ts

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@ import type { ICommandCallbacks, ICommandContext, ISuggestionItem } from '../../
1919
import { getAssignmentExpressionRoot } from '../expressions';
2020
import { suggestForExpression } from './expressions';
2121
import { withAutoSuggest } from './helpers';
22+
import { TRAILING_COMMA_REGEX } from '../shared';
2223
import type { ExpressionContextOptions } from './expressions/types';
2324

2425
const ENDS_WITH_WHITESPACE_REGEX = /\s$/;
@@ -54,7 +55,7 @@ export async function suggestFieldsList(
5455
const innerText = query.substring(0, cursorPosition);
5556
const lastField = fieldList[fieldList.length - 1];
5657

57-
const endsWithComma = /,\s*$/.test(innerText);
58+
const endsWithComma = TRAILING_COMMA_REGEX.test(innerText);
5859
const withinFunction =
5960
lastField && isFunctionExpression(lastField) && within(innerText.length, lastField);
6061
const startingNewExpression = endsWithComma && !withinFunction;

src/platform/packages/shared/kbn-esql-language/src/commands/definitions/utils/autocomplete/promql/query_position.ts

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@ import type {
1515
PromQLLabel,
1616
PromQLSelector,
1717
} from '@elastic/esql';
18+
import { TRAILING_COMMA_REGEX } from '../../shared';
1819
import type { PromQLFunctionParamType } from '../../../types';
1920
import type { CursorMatch, PromqlDetailedPosition } from './types';
2021
import {
@@ -41,7 +42,6 @@ import {
4142
} from '../../promql';
4243
import { promqlOperatorDefinitions } from '../../../generated/promql_operators';
4344

44-
const TRAILING_COMMA_WITH_SPACES_REGEX = /,\s*$/;
4545
const SELECTOR_DURATION_START_REGEX = /^\s*\[$/;
4646

4747
// Builds a regex pattern like `\+|\-|\*|==|and|or|...` from all binary operator definitions.
@@ -132,7 +132,7 @@ export function getQueryPosition(
132132
node.type === 'identifier' &&
133133
parent?.type === 'label-map' &&
134134
innermostFunc &&
135-
TRAILING_COMMA_WITH_SPACES_REGEX.test(textBeforeCursor);
135+
TRAILING_COMMA_REGEX.test(textBeforeCursor);
136136

137137
if (!skipLabel) {
138138
const labelPos = resolveLabelPosition(match, cursor, textBeforeCursor, getSignatureTypes());

src/platform/packages/shared/kbn-esql-language/src/commands/definitions/utils/expressions.test.ts

Lines changed: 41 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -6,14 +6,15 @@
66
* your election, the "Elastic License 2.0", the "GNU Affero General Public
77
* License v3.0 only", or the "Server Side Public License, v 1".
88
*/
9-
import { Parser } from '@elastic/esql';
9+
import { isAssignment, Parser } from '@elastic/esql';
1010
import type { SupportedDataType } from '../types';
1111
import { FunctionDefinitionTypes } from '../types';
1212
import type { ESQLColumnData } from '../../registry/types';
1313
import { Location } from '../../registry/types';
14-
import { buildPartialMatcher, getExpressionType } from './expressions';
14+
import { buildPartialMatcher, getAssignmentExpressionRoot, getExpressionType } from './expressions';
1515
import { setTestFunctions } from './test_functions';
1616
import { TIME_SYSTEM_PARAMS } from './literals';
17+
import { getAutocompleteCursorContext } from '../../../language/shared/parse_for_autocomplete_query';
1718

1819
describe('buildPartialMatcher', () => {
1920
it('should build a partial matcher', () => {
@@ -458,3 +459,41 @@ describe('getExpressionType', () => {
458459
);
459460
});
460461
});
462+
463+
describe('getAssignmentExpressionRoot', () => {
464+
it('returns undefined for an incomplete assignment after autocomplete parsing cleanup', () => {
465+
const query = 'FROM employees | EVAL total = ';
466+
const { astContext } = getAutocompleteCursorContext(query, query.length);
467+
468+
if (astContext.type !== 'expression') {
469+
throw new Error(`Expected expression context for query: ${query}`);
470+
}
471+
472+
const assignment = astContext.command.args[astContext.command.args.length - 1];
473+
474+
if (!assignment || !isAssignment(assignment)) {
475+
throw new Error(`Expected assignment expression for query: ${query}`);
476+
}
477+
478+
expect(getAssignmentExpressionRoot(assignment)).toBeUndefined();
479+
});
480+
481+
it('returns the RHS root for a complete assignment after autocomplete parsing cleanup', () => {
482+
const query = 'FROM employees | EVAL total = salary';
483+
const { astContext } = getAutocompleteCursorContext(query, query.length);
484+
485+
if (astContext.type !== 'expression') {
486+
throw new Error(`Expected expression context for query: ${query}`);
487+
}
488+
489+
const assignment = astContext.command.args[astContext.command.args.length - 1];
490+
491+
if (!assignment || !isAssignment(assignment)) {
492+
throw new Error(`Expected assignment expression for query: ${query}`);
493+
}
494+
495+
const root = getAssignmentExpressionRoot(assignment);
496+
497+
expect(root).toMatchObject({ type: 'column', name: 'salary' });
498+
});
499+
});

src/platform/packages/shared/kbn-esql-language/src/commands/definitions/utils/shared.ts

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,9 @@ import type { ESQLUserDefinedColumn, ICommandContext } from '../../registry/type
1414
import { getLastNonWhitespaceChar } from './autocomplete/helpers';
1515
import type { SupportedDataType } from '../types';
1616

17+
/** Matches a trailing comma (with optional whitespace) at end of text — used to detect "starting a new argument/expression" */
18+
export const TRAILING_COMMA_REGEX = /,\s*$/;
19+
1720
export const techPreviewLabel = i18n.translate(
1821
'kbn-esql-language.esql.autocomplete.techPreviewLabel',
1922
{

src/platform/packages/shared/kbn-esql-language/src/commands/registry/completion/autocomplete.ts

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,6 @@ import type {
1919
import { suggestForExpression } from '../../definitions/utils';
2020
import type { MapParameters } from '../../definitions/utils/autocomplete/map_expression';
2121
import { getCommandMapExpressionSuggestions } from '../../definitions/utils/autocomplete/map_expression';
22-
import { EDITOR_MARKER } from '../../definitions/constants';
2322
import {
2423
pipeCompleteItem,
2524
assignCompletionItem,
@@ -85,7 +84,7 @@ function getPosition(
8584
return { position: CompletionPosition.AFTER_COMMAND };
8685
}
8786

88-
const expressionRoot = prompt?.text !== EDITOR_MARKER ? prompt : undefined;
87+
const expressionRoot = prompt;
8988

9089
// (function, literal, or existing column) - handle as primaryExpression
9190
if (isFunctionExpression(expressionRoot) || isLiteral(prompt) || isExistingColumn) {

0 commit comments

Comments
 (0)