From 3fcbed0bfbc9852f31e749e5682e33cff4cddccf Mon Sep 17 00:00:00 2001 From: Drew Tate Date: Thu, 11 Sep 2025 11:47:55 -0600 Subject: [PATCH 1/3] fix resource helper --- .../src/shared/resources_helpers.ts | 25 +++++++++++-------- 1 file changed, 15 insertions(+), 10 deletions(-) diff --git a/src/platform/packages/shared/kbn-esql-validation-autocomplete/src/shared/resources_helpers.ts b/src/platform/packages/shared/kbn-esql-validation-autocomplete/src/shared/resources_helpers.ts index 01151fec9ac14..abb245b2be479 100644 --- a/src/platform/packages/shared/kbn-esql-validation-autocomplete/src/shared/resources_helpers.ts +++ b/src/platform/packages/shared/kbn-esql-validation-autocomplete/src/shared/resources_helpers.ts @@ -8,15 +8,15 @@ */ import type { ESQLAstQueryExpression } from '@kbn/esql-ast'; -import { BasicPrettyPrinter, Builder, EDITOR_MARKER, EsqlQuery } from '@kbn/esql-ast'; +import { BasicPrettyPrinter, Builder, EDITOR_MARKER } from '@kbn/esql-ast'; import type { ESQLColumnData, ESQLFieldWithMetadata, ESQLPolicy, } from '@kbn/esql-ast/src/commands_registry/types'; -import type { ESQLCallbacks } from './types'; -import { getFieldsFromES, getCurrentQueryAvailableColumns } from './helpers'; import { expandEvals } from './expand_evals'; +import { getCurrentQueryAvailableColumns, getFieldsFromES } from './helpers'; +import type { ESQLCallbacks } from './types'; export const NOT_SUGGESTED_TYPES = ['unsupported']; @@ -101,8 +101,8 @@ export function getColumnsByTypeHelper( originalQueryText: string, resourceRetriever?: ESQLCallbacks ) { - const queryForFields = getQueryForFields(originalQueryText, query); - const root = EsqlQuery.fromSrc(queryForFields).ast; + const root = getQueryForFields(originalQueryText, query); + const queryForFields = BasicPrettyPrinter.print(root); const cacheColumns = async () => { if (!queryForFields) { @@ -195,11 +195,16 @@ export function getSourcesHelper(resourceRetriever?: ESQLCallbacks) { * Generally, this is the user's query up to the end of the previous command, but there * are special cases for multi-expression EVAL and FORK branches. * + * IMPORTANT: the AST nodes in the new query still reference locations in the original query text + * * @param queryString The original query string * @param commands * @returns */ -export function getQueryForFields(queryString: string, root: ESQLAstQueryExpression): string { +export function getQueryForFields( + queryString: string, + root: ESQLAstQueryExpression +): ESQLAstQueryExpression { const commands = root.commands; const lastCommand = commands[commands.length - 1]; if (lastCommand && lastCommand.name === 'fork' && lastCommand.args.length > 0) { @@ -215,7 +220,7 @@ export function getQueryForFields(queryString: string, root: ESQLAstQueryExpress */ const currentBranch = lastCommand.args[lastCommand.args.length - 1] as ESQLAstQueryExpression; const newCommands = commands.slice(0, -1).concat(currentBranch.commands.slice(0, -1)); - return BasicPrettyPrinter.print({ ...root, commands: newCommands }); + return { ...root, commands: newCommands }; } if (lastCommand && lastCommand.name === 'eval') { @@ -235,7 +240,7 @@ export function getQueryForFields(queryString: string, root: ESQLAstQueryExpress */ const expanded = expandEvals(commands); const newCommands = expanded.slice(0, endsWithComma ? undefined : -1); - return BasicPrettyPrinter.print({ ...root, commands: newCommands }); + return { ...root, commands: newCommands }; } } @@ -244,8 +249,8 @@ export function getQueryForFields(queryString: string, root: ESQLAstQueryExpress function buildQueryUntilPreviousCommand(root: ESQLAstQueryExpression) { if (root.commands.length === 1) { - return BasicPrettyPrinter.print({ ...root.commands[0] }); + return { ...root.commands[0] }; } else { - return BasicPrettyPrinter.print({ ...root, commands: root.commands.slice(0, -1) }); + return { ...root, commands: root.commands.slice(0, -1) }; } } From 6809d3c31fb398a6a7fd2e874a3921439a357ee3 Mon Sep 17 00:00:00 2001 From: Drew Tate Date: Thu, 11 Sep 2025 12:11:04 -0600 Subject: [PATCH 2/3] fix fn --- .../src/shared/resources_helpers.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/platform/packages/shared/kbn-esql-validation-autocomplete/src/shared/resources_helpers.ts b/src/platform/packages/shared/kbn-esql-validation-autocomplete/src/shared/resources_helpers.ts index abb245b2be479..36f9695b32d0e 100644 --- a/src/platform/packages/shared/kbn-esql-validation-autocomplete/src/shared/resources_helpers.ts +++ b/src/platform/packages/shared/kbn-esql-validation-autocomplete/src/shared/resources_helpers.ts @@ -249,7 +249,7 @@ export function getQueryForFields( function buildQueryUntilPreviousCommand(root: ESQLAstQueryExpression) { if (root.commands.length === 1) { - return { ...root.commands[0] }; + return { ...root, commands: [root.commands[0]] }; } else { return { ...root, commands: root.commands.slice(0, -1) }; } From d0014ed873d631c3ecbbbed922687687c13af739 Mon Sep 17 00:00:00 2001 From: Drew Tate Date: Thu, 11 Sep 2025 14:03:11 -0600 Subject: [PATCH 3/3] make cache keys case-sensitive --- .../src/shared/expand_evals.ts | 14 ++-- .../src/shared/resources_helpers.test.ts | 4 +- .../src/shared/resources_helpers.ts | 68 ++++++++----------- .../__tests__/column_caching.test.ts | 29 ++++++++ .../src/validation/helpers.ts | 6 +- 5 files changed, 72 insertions(+), 49 deletions(-) create mode 100644 src/platform/packages/shared/kbn-esql-validation-autocomplete/src/validation/__tests__/column_caching.test.ts diff --git a/src/platform/packages/shared/kbn-esql-validation-autocomplete/src/shared/expand_evals.ts b/src/platform/packages/shared/kbn-esql-validation-autocomplete/src/shared/expand_evals.ts index 07338095d16ef..fa23fe06b5b95 100644 --- a/src/platform/packages/shared/kbn-esql-validation-autocomplete/src/shared/expand_evals.ts +++ b/src/platform/packages/shared/kbn-esql-validation-autocomplete/src/shared/expand_evals.ts @@ -26,11 +26,15 @@ export function expandEvals(commands: ESQLCommand[]): ESQLCommand[] { if (command.name.toLowerCase() === 'eval') { for (const arg of command.args) { expanded.push( - Builder.command({ - name: 'eval', - args: [arg], - location: command.location, - }) + Builder.command( + { + name: 'eval', + args: [arg], + }, + { + location: command.location, + } + ) ); } } else { diff --git a/src/platform/packages/shared/kbn-esql-validation-autocomplete/src/shared/resources_helpers.test.ts b/src/platform/packages/shared/kbn-esql-validation-autocomplete/src/shared/resources_helpers.test.ts index b623e75087d0e..000ceb6b98f69 100644 --- a/src/platform/packages/shared/kbn-esql-validation-autocomplete/src/shared/resources_helpers.test.ts +++ b/src/platform/packages/shared/kbn-esql-validation-autocomplete/src/shared/resources_helpers.test.ts @@ -7,7 +7,7 @@ * License v3.0 only", or the "Server Side Public License, v 1". */ -import { parse, EDITOR_MARKER } from '@kbn/esql-ast'; +import { parse, EDITOR_MARKER, BasicPrettyPrinter } from '@kbn/esql-ast'; import { getQueryForFields } from './resources_helpers'; describe('getQueryForFields', () => { @@ -26,7 +26,7 @@ describe('getQueryForFields', () => { const result = getQueryForFields(query, root); - expect(result).toEqual(expected); + expect(BasicPrettyPrinter.print(result)).toEqual(expected); }; it('should return everything up till the last command', () => { diff --git a/src/platform/packages/shared/kbn-esql-validation-autocomplete/src/shared/resources_helpers.ts b/src/platform/packages/shared/kbn-esql-validation-autocomplete/src/shared/resources_helpers.ts index 36f9695b32d0e..c393789fae453 100644 --- a/src/platform/packages/shared/kbn-esql-validation-autocomplete/src/shared/resources_helpers.ts +++ b/src/platform/packages/shared/kbn-esql-validation-autocomplete/src/shared/resources_helpers.ts @@ -22,26 +22,6 @@ export const NOT_SUGGESTED_TYPES = ['unsupported']; const cache = new Map(); -// Function to check if a key exists in the cache, ignoring case -function checkCacheInsensitive(keyToCheck: string) { - for (const key of cache.keys()) { - if (key.toLowerCase() === keyToCheck.toLowerCase()) { - return true; // Or return the value associated with the key if needed: return cache.get(key); - } - } - return false; -} - -// Function to get a value from the cache, ignoring case -function getValueInsensitive(keyToCheck: string) { - for (const key of cache.keys()) { - if (key.toLowerCase() === keyToCheck.toLowerCase()) { - return cache.get(key); - } - } - return undefined; -} - /** * Given a query, this function will compute the available fields and cache them * for the next time the same query is used. @@ -53,18 +33,9 @@ async function cacheColumnsForQuery( getPolicies: () => Promise>, originalQueryText: string ) { - let cacheKey: string; - try { - cacheKey = BasicPrettyPrinter.print(query); - } catch { - // for some syntactically incorrect queries - // the printer will throw. They're incorrect - // anyways, so just move on — ANTLR errors will - // be reported. - return; - } + const cacheKey = originalQueryText.slice(query.location.min, query.location.max + 1); - const existsInCache = checkCacheInsensitive(cacheKey); + const existsInCache = cache.has(cacheKey); if (existsInCache) { // this is already in the cache return; @@ -74,7 +45,7 @@ async function cacheColumnsForQuery( ...query, commands: query.commands.slice(0, -1), }); - const fieldsAvailableAfterPreviousCommand = getValueInsensitive(queryBeforeCurrentCommand) ?? []; + const fieldsAvailableAfterPreviousCommand = cache.get(queryBeforeCurrentCommand) ?? []; const availableFields = await getCurrentQueryAvailableColumns( query.commands, @@ -102,15 +73,17 @@ export function getColumnsByTypeHelper( resourceRetriever?: ESQLCallbacks ) { const root = getQueryForFields(originalQueryText, query); - const queryForFields = BasicPrettyPrinter.print(root); + + // IMPORTANT: cache key can't be case-insensitive because column names are case-sensitive + const cacheKey = originalQueryText.slice(root.location.min, root.location.max + 1); const cacheColumns = async () => { - if (!queryForFields) { + if (!cacheKey) { return; } const getFields = async (queryToES: string) => { - const cached = getValueInsensitive(queryToES); + const cached = cache.get(queryToES); if (cached) { return cached as ESQLFieldWithMetadata[]; } @@ -121,7 +94,11 @@ export function getColumnsByTypeHelper( const subqueries = []; for (let i = 0; i < root.commands.length; i++) { - subqueries.push(Builder.expression.query(root.commands.slice(0, i + 1))); + subqueries.push( + Builder.expression.query(root.commands.slice(0, i + 1), { + location: { min: 0, max: root.commands[i].location.max }, + }) + ); } const getPolicies = async () => { @@ -142,7 +119,7 @@ export function getColumnsByTypeHelper( ): Promise => { const types = Array.isArray(expectedType) ? expectedType : [expectedType]; await cacheColumns(); - const cachedFields = getValueInsensitive(queryForFields); + const cachedFields = cache.get(cacheKey); return ( cachedFields?.filter(({ name, type }) => { const ts = Array.isArray(type) ? type : [type]; @@ -157,7 +134,7 @@ export function getColumnsByTypeHelper( }, getColumnMap: async (): Promise> => { await cacheColumns(); - const cachedFields = getValueInsensitive(queryForFields); + const cachedFields = cache.get(cacheKey); const cacheCopy = new Map(); cachedFields?.forEach((field) => cacheCopy.set(field.name, field)); return cacheCopy; @@ -220,7 +197,8 @@ export function getQueryForFields( */ const currentBranch = lastCommand.args[lastCommand.args.length - 1] as ESQLAstQueryExpression; const newCommands = commands.slice(0, -1).concat(currentBranch.commands.slice(0, -1)); - return { ...root, commands: newCommands }; + const newLocation = { min: 0, max: newCommands[newCommands.length - 1]?.location.max ?? 0 }; + return { ...root, commands: newCommands, location: newLocation }; } if (lastCommand && lastCommand.name === 'eval') { @@ -240,7 +218,8 @@ export function getQueryForFields( */ const expanded = expandEvals(commands); const newCommands = expanded.slice(0, endsWithComma ? undefined : -1); - return { ...root, commands: newCommands }; + const newLocation = { min: 0, max: newCommands[newCommands.length - 1]?.location.max ?? 0 }; + return { ...root, commands: newCommands, location: newLocation }; } } @@ -251,6 +230,13 @@ function buildQueryUntilPreviousCommand(root: ESQLAstQueryExpression) { if (root.commands.length === 1) { return { ...root, commands: [root.commands[0]] }; } else { - return { ...root, commands: root.commands.slice(0, -1) }; + const newCommands = root.commands.slice(0, -1); + const newLocation = { min: 0, max: newCommands[newCommands.length - 1]?.location.max ?? 0 }; + + return { + ...root, + commands: newCommands, + location: newLocation, + }; } } diff --git a/src/platform/packages/shared/kbn-esql-validation-autocomplete/src/validation/__tests__/column_caching.test.ts b/src/platform/packages/shared/kbn-esql-validation-autocomplete/src/validation/__tests__/column_caching.test.ts new file mode 100644 index 0000000000000..6d71c9bcf019f --- /dev/null +++ b/src/platform/packages/shared/kbn-esql-validation-autocomplete/src/validation/__tests__/column_caching.test.ts @@ -0,0 +1,29 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the "Elastic License + * 2.0", the "GNU Affero General Public License v3.0 only", and the "Server Side + * Public License v 1"; you may not use this file except in compliance with, at + * your election, the "Elastic License 2.0", the "GNU Affero General Public + * License v3.0 only", or the "Server Side Public License, v 1". + */ + +import { setup } from './helpers'; + +describe('column caching and casing', () => { + it('expression columns are cached case-sensitively', async () => { + const { expectErrors } = await setup(); + await expectErrors('FROM index | EVAL TrIm("") | EVAL `TrIm("")`', []); + await expectErrors('FROM index | EVAL TrIm("") | EVAL `TRIM("")`', [ + 'Unknown column "TRIM("")"', + ]); + }); + + it('case changes bust the cache', async () => { + const { expectErrors } = await setup(); + // first populate the cache with capitalized TRIM query + await expectErrors('FROM index | EVAL ABS(1) | EVAL `ABS(1)`', []); + + // change the case of the TRIM to trim, which should bust the cache and create an error + await expectErrors('FROM index | EVAL abs(1) | EVAL `ABS(1)`', ['Unknown column "ABS(1)"']); + }); +}); diff --git a/src/platform/packages/shared/kbn-esql-validation-autocomplete/src/validation/helpers.ts b/src/platform/packages/shared/kbn-esql-validation-autocomplete/src/validation/helpers.ts index 7296cfbeb6aa9..bb8f212584d08 100644 --- a/src/platform/packages/shared/kbn-esql-validation-autocomplete/src/validation/helpers.ts +++ b/src/platform/packages/shared/kbn-esql-validation-autocomplete/src/validation/helpers.ts @@ -60,7 +60,11 @@ export function getSubqueriesToValidate(rootCommands: ESQLCommand[]) { subsequences.push(expandedCommands.slice(0, i + 1)); } - return subsequences.map((subsequence) => Builder.expression.query(subsequence)); + return subsequences.map((subsequence) => + Builder.expression.query(subsequence, { + location: { min: 0, max: subsequence[subsequence.length - 1].location.max }, + }) + ); } /**