[8.x] [ES|QL] Improve variable and field name handling (#195149) (#19…

…5936) # Backport This will backport the following commits from `main` to `8.x`: - [[ES|QL] Improve variable and field name handling (#195149)](#195149)  ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sqren/backport)  Co-authored-by: Drew Tate <[email protected]>
elastic · Oct 11, 2024 · 7b9ace8 · 7b9ace8
1 parent 5ebef44
commit 7b9ace8
Show file tree

Hide file tree

Showing 12 changed files with 417 additions and 162 deletions.
diff --git a/packages/kbn-esql-validation-autocomplete/index.ts b/packages/kbn-esql-validation-autocomplete/index.ts
@@ -62,7 +62,6 @@ export {
   isLiteralItem,
   isTimeIntervalItem,
   isAssignment,
-  isExpression,
   isAssignmentComplete,
   isSingleItem,
 } from './src/shared/helpers';

diff --git a/...esql-validation-autocomplete/src/autocomplete/__tests__/autocomplete.command.sort.test.ts b/...esql-validation-autocomplete/src/autocomplete/__tests__/autocomplete.command.sort.test.ts
@@ -62,6 +62,36 @@ describe('autocomplete.suggest', () => {
             },
           ].map(attachTriggerCommand)
         );
+
+        await assertSuggestions(
+          'from a | sort `keywordField`/',
+          [
+            {
+              filterText: '`keywordField`',
+              text: '`keywordField`, ',
+            },
+            {
+              filterText: '`keywordField`',
+              text: '`keywordField` | ',
+            },
+            {
+              filterText: '`keywordField`',
+              text: '`keywordField` ASC',
+            },
+            {
+              filterText: '`keywordField`',
+              text: '`keywordField` DESC',
+            },
+            {
+              filterText: '`keywordField`',
+              text: '`keywordField` NULLS FIRST',
+            },
+            {
+              filterText: '`keywordField`',
+              text: '`keywordField` NULLS LAST',
+            },
+          ].map(attachTriggerCommand)
+        );
       });
       it('suggests subsequent column after comma', async () => {
         const { assertSuggestions } = await setup();

diff --git a/packages/kbn-esql-validation-autocomplete/src/autocomplete/autocomplete.test.ts b/packages/kbn-esql-validation-autocomplete/src/autocomplete/autocomplete.test.ts
@@ -1196,28 +1196,39 @@ describe('autocomplete', () => {
         );
 
         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 | ${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(
+          testSuggestions(
             `FROM a | ${commandName} \`foo.bar\`/`,
-            ['foo.bar, ', 'foo.bar | '],
+            ['`foo.bar`, ', '`foo.bar` | '],
             undefined,
-            [[{ name: 'foo.bar', type: 'double' }]]
+            [
+              [
+                { name: 'foo.bar', type: 'double' },
+                { name: 'baz', type: 'date' }, // added so that we get a comma suggestion
+              ],
+            ]
           );
-          testSuggestions.skip(
-            `FROM a | ${commandName} \`foo\`.\`bar\`/`,
-            ['foo.bar, ', 'foo.bar | '],
+          testSuggestions(
+            `FROM a | ${commandName} \`foo\`\`\`\`bar\`\`baz\`/`,
+            ['`foo````bar``baz`, ', '`foo````bar``baz` | '],
             undefined,
-            [[{ name: 'foo.bar', type: 'double' }]]
+            [
+              [
+                { name: 'foo``bar`baz', type: 'double' },
+                { name: 'baz', type: 'date' }, // added so that we get a comma suggestion
+              ],
+            ]
           );
-          testSuggestions.skip(`FROM a | ${commandName} \`any#Char$Field\`/`, [
+          testSuggestions(`FROM a | ${commandName} \`any#Char$Field\`/`, [
             '`any#Char$Field`, ',
             '`any#Char$Field` | ',
           ]);
+          // @todo enable this test when we can use AST to support this case
+          testSuggestions.skip(
+            `FROM a | ${commandName} \`foo\`.\`bar\`/`,
+            ['`foo`.`bar`, ', '`foo`.`bar` | '],
+            undefined,
+            [[{ name: 'foo.bar', type: 'double' }]]
+          );
         });
 
         // Subsequent fields

diff --git a/packages/kbn-esql-validation-autocomplete/src/autocomplete/autocomplete.ts b/packages/kbn-esql-validation-autocomplete/src/autocomplete/autocomplete.ts
@@ -83,7 +83,7 @@ import {
   TRIGGER_SUGGESTION_COMMAND,
   getAddDateHistogramSnippet,
 } from './factories';
-import { EDITOR_MARKER, SINGLE_BACKTICK, METADATA_FIELDS } from '../shared/constants';
+import { EDITOR_MARKER, METADATA_FIELDS } from '../shared/constants';
 import { getAstContext, removeMarkerArgFromArgsList } from '../shared/context';
 import {
   buildQueryUntilPreviousCommand,
@@ -1215,11 +1215,7 @@ async function getFieldsOrFunctionsSuggestions(
       filteredVariablesByType.some((v) => ALPHANUMERIC_REGEXP.test(v))
     ) {
       for (const variable of filteredVariablesByType) {
-        // remove backticks if present
-        const sanitizedVariable = variable.startsWith(SINGLE_BACKTICK)
-          ? variable.slice(1, variable.length - 1)
-          : variable;
-        const underscoredName = sanitizedVariable.replace(ALPHANUMERIC_REGEXP, '_');
+        const underscoredName = variable.replace(ALPHANUMERIC_REGEXP, '_');
         const index = filteredFieldsByType.findIndex(
           ({ label }) => underscoredName === label || `_${underscoredName}_` === label
         );

diff --git a/packages/kbn-esql-validation-autocomplete/src/autocomplete/factories.ts b/packages/kbn-esql-validation-autocomplete/src/autocomplete/factories.ts
@@ -171,7 +171,7 @@ export const buildFieldsDefinitions = (fields: string[]): SuggestionRawDefinitio
 export const buildVariablesDefinitions = (variables: string[]): SuggestionRawDefinition[] =>
   variables.map((label) => ({
     label,
-    text: label,
+    text: getSafeInsertText(label),
     kind: 'Variable',
     detail: i18n.translate(
       'kbn-esql-validation-autocomplete.esql.autocomplete.variableDefinition',

diff --git a/packages/kbn-esql-validation-autocomplete/src/definitions/types.ts b/packages/kbn-esql-validation-autocomplete/src/definitions/types.ts
@@ -172,7 +172,7 @@ export interface CommandBaseDefinition {
       name: string;
       type: string;
       optional?: boolean;
-      innerTypes?: string[];
+      innerTypes?: Array<SupportedDataType | 'any' | 'policy'>;
       values?: string[];
       valueDescriptions?: string[];
       constantOnly?: boolean;

diff --git a/packages/kbn-esql-validation-autocomplete/src/shared/constants.ts b/packages/kbn-esql-validation-autocomplete/src/shared/constants.ts
@@ -12,7 +12,7 @@ export const EDITOR_MARKER = 'marker_esql_editor';
 export const TICKS_REGEX = /^`{1}|`{1}$/g;
 export const DOUBLE_TICKS_REGEX = /``/g;
 export const SINGLE_TICK_REGEX = /`/g;
-export const SINGLE_BACKTICK = '`';
 export const DOUBLE_BACKTICK = '``';
+export const SINGLE_BACKTICK = '`';
 
 export const METADATA_FIELDS = ['_version', '_id', '_index', '_source', '_ignored'];
diff --git a/packages/kbn-esql-validation-autocomplete/src/shared/esql_types.ts b/packages/kbn-esql-validation-autocomplete/src/shared/esql_types.ts
@@ -83,8 +83,6 @@ export const compareTypesWithLiterals = (
     return b === 'timeInterval';
   if (b === 'time_literal' || b === 'time_duration' || b === 'date_period')
     return a === 'timeInterval';
-  if (a === 'time_literal') return b === 'time_duration';
-  if (b === 'time_literal') return a === 'time_duration';
 
   return false;
 };
diff --git a/packages/kbn-esql-validation-autocomplete/src/shared/helpers.ts b/packages/kbn-esql-validation-autocomplete/src/shared/helpers.ts
@@ -48,7 +48,7 @@ import type { ESQLRealField, ESQLVariable, ReferenceMaps } from '../validation/t
 import { removeMarkerArgFromArgsList } from './context';
 import { isNumericDecimalType } from './esql_types';
 import type { ReasonTypes } from './types';
-import { EDITOR_MARKER } from './constants';
+import { DOUBLE_TICKS_REGEX, EDITOR_MARKER, SINGLE_BACKTICK } from './constants';
 import type { EditorContext } from '../autocomplete/types';
 
 export function nonNullable<T>(v: T): v is NonNullable<T> {
@@ -99,10 +99,6 @@ export function isAssignmentComplete(node: ESQLFunction | undefined) {
   return Boolean(assignExpression && Array.isArray(assignExpression) && assignExpression.length);
 }
 
-export function isExpression(arg: ESQLAstItem): arg is ESQLFunction {
-  return isFunctionItem(arg) && arg.name !== '=';
-}
-
 export function isIncompleteItem(arg: ESQLAstItem): boolean {
   return !arg || (!Array.isArray(arg) && arg.incomplete);
 }
@@ -228,7 +224,7 @@ export function getCommandOption(optionName: CommandOptionsDefinition['name']) {
   );
 }
 
-function compareLiteralType(argType: string, item: ESQLLiteral) {
+function doesLiteralMatchParameterType(argType: FunctionParameterType, item: ESQLLiteral) {
   if (item.literalType === 'null') {
     return true;
   }
@@ -249,7 +245,7 @@ function compareLiteralType(argType: string, item: ESQLLiteral) {
   }
 
   // date-type parameters accept string literals because of ES auto-casting
-  return ['string', 'date', 'date', 'date_period'].includes(argType);
+  return ['string', 'date', 'date_period'].includes(argType);
 }
 
 /**
@@ -259,13 +255,7 @@ export function getColumnForASTNode(
   column: ESQLColumn,
   { fields, variables }: Pick<ReferenceMaps, 'fields' | 'variables'>
 ): ESQLRealField | ESQLVariable | undefined {
-  const columnName = getQuotedColumnName(column);
-  return (
-    getColumnByName(columnName, { fields, variables }) ||
-    // It's possible columnName has backticks "`fieldName`"
-    // so we need to access the original name as well
-    getColumnByName(column.name, { fields, variables })
-  );
+  return getColumnByName(column.parts.join('.'), { fields, variables });
 }
 
 /**
@@ -275,6 +265,11 @@ export function getColumnByName(
   columnName: string,
   { fields, variables }: Pick<ReferenceMaps, 'fields' | 'variables'>
 ): ESQLRealField | ESQLVariable | undefined {
+  // TODO this doesn't cover all escaping scenarios... the best thing to do would be
+  // to use the AST column node parts array, but in some cases the AST node isn't available.
+  if (columnName.startsWith(SINGLE_BACKTICK) && columnName.endsWith(SINGLE_BACKTICK)) {
+    columnName = columnName.slice(1, -1).replace(DOUBLE_TICKS_REGEX, SINGLE_BACKTICK);
+  }
   return fields.get(columnName) || variables.get(columnName)?.[0];
 }
 
@@ -445,7 +440,7 @@ export function checkFunctionArgMatchesDefinition(
     return true;
   }
   if (arg.type === 'literal') {
-    const matched = compareLiteralType(argType as string, arg);
+    const matched = doesLiteralMatchParameterType(argType, arg);
     return matched;
   }
   if (arg.type === 'function') {
@@ -549,16 +544,6 @@ export function isVariable(
   return Boolean(column && 'location' in column);
 }
 
-/**
- * This will return the name without any quotes.
- *
- * E.g. "`bytes`" will become "bytes"
- *
- * @param column
- * @returns
- */
-export const getUnquotedColumnName = (column: ESQLColumn) => column.name;
-
 /**
  * This returns the name with any quotes that were present.
  *
@@ -577,17 +562,16 @@ export function getColumnExists(
   column: ESQLColumn,
   { fields, variables }: Pick<ReferenceMaps, 'fields' | 'variables'>
 ) {
-  const namesToCheck = [getUnquotedColumnName(column), getQuotedColumnName(column)];
-
-  for (const name of namesToCheck) {
-    if (fields.has(name) || variables.has(name)) {
-      return true;
-    }
+  const columnName = column.parts.join('.');
+  if (fields.has(columnName) || variables.has(columnName)) {
+    return true;
+  }
 
-    // TODO — I don't see this fuzzy searching in lookupColumn... should it be there?
-    if (Boolean(fuzzySearch(name, fields.keys()) || fuzzySearch(name, variables.keys()))) {
-      return true;
-    }
+  // TODO — I don't see this fuzzy searching in lookupColumn... should it be there?
+  if (
+    Boolean(fuzzySearch(columnName, fields.keys()) || fuzzySearch(columnName, variables.keys()))
+  ) {
+    return true;
   }
 
   return false;