[Security Solution] Use AST utils from @kbn/esql-ast for ES|QL rule t…

…ype query parsing (elastic#9282) (elastic#189780) ## Summary With these changes we utilise AST based utils to do ES|QL query validation. This allows us to recognise and display syntax errors. Syntax errors have higher priority than the rest of the validation errors. Validation errors priorities from top to bottom: 1. Syntax error 2. Missing metadata for non-aggregating queries 3. Missing data source and/or data fields 4. Missing `_id` column requested for non-aggregating queries via metadata operator These priorities define the sequence in which we display errors to the user. If there are several errors detected, that the one with higher priority will be shown. https://github.com/user-attachments/assets/cef88c60-b0a4-413e-885a-b619773fd853 ### 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 - [x] [Flaky Test Runner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1) was used on any tests changed * [Integration tests](https://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/6681) (100 ESS, 100 Serverless) * [Cypress DE tests](https://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/6712) (100 ESS, 100 Serverless) --------- Co-authored-by: kibanamachine <[email protected]>
mikecote · Aug 7, 2024 · d3d5a7c · d3d5a7c
1 parent 2328efb
commit d3d5a7c
Show file tree

Hide file tree

Showing 8 changed files with 152 additions and 45 deletions.
diff --git a/packages/kbn-securitysolution-utils/src/esql/compute_if_esql_query_aggregating.ts b/packages/kbn-securitysolution-utils/src/esql/compute_if_esql_query_aggregating.ts
@@ -6,11 +6,18 @@
  * Side Public License, v 1.
  */
 
+import { ESQLAst, getAstAndSyntaxErrors } from '@kbn/esql-ast';
+
+export const isAggregatingQuery = (ast: ESQLAst): boolean => {
+  return ast.some((astItem) => astItem.type === 'command' && astItem.name === 'stats');
+};
+
 /**
  * compute if esqlQuery is aggregating/grouping, i.e. using STATS...BY command
  * @param esqlQuery
  * @returns boolean
  */
 export const computeIsESQLQueryAggregating = (esqlQuery: string): boolean => {
-  return /\|\s+stats\s/i.test(esqlQuery);
+  const { ast } = getAstAndSyntaxErrors(esqlQuery);
+  return isAggregatingQuery(ast);
 };
diff --git a/packages/kbn-securitysolution-utils/tsconfig.json b/packages/kbn-securitysolution-utils/tsconfig.json
@@ -12,7 +12,8 @@
   ],
   "kbn_references": [
     "@kbn/i18n",
-    "@kbn/esql-utils"
+    "@kbn/esql-utils",
+    "@kbn/esql-ast"
   ],
   "exclude": [
     "target/**/*",

diff --git a/...gins/security_solution/public/detection_engine/rule_creation/logic/esql_validator.test.ts b/...gins/security_solution/public/detection_engine/rule_creation/logic/esql_validator.test.ts
@@ -5,82 +5,117 @@
  * 2.0.
  */
 
+import { getAstAndSyntaxErrors } from '@kbn/esql-ast';
 import { parseEsqlQuery, computeHasMetadataOperator } from './esql_validator';
 
-import { computeIsESQLQueryAggregating } from '@kbn/securitysolution-utils';
+import { isAggregatingQuery } from '@kbn/securitysolution-utils';
 
-jest.mock('@kbn/securitysolution-utils', () => ({ computeIsESQLQueryAggregating: jest.fn() }));
+jest.mock('@kbn/securitysolution-utils', () => ({ isAggregatingQuery: jest.fn() }));
 
-const computeIsESQLQueryAggregatingMock = computeIsESQLQueryAggregating as jest.Mock;
+const isAggregatingQueryMock = isAggregatingQuery as jest.Mock;
+
+const getQeryAst = (query: string) => {
+  const { ast } = getAstAndSyntaxErrors(query);
+  return ast;
+};
 
 describe('computeHasMetadataOperator', () => {
   it('should be false if query does not have operator', () => {
-    expect(computeHasMetadataOperator('from test*')).toBe(false);
-    expect(computeHasMetadataOperator('from test* [metadata]')).toBe(false);
-    expect(computeHasMetadataOperator('from test* [metadata id]')).toBe(false);
-    expect(computeHasMetadataOperator('from metadata*')).toBe(false);
-    expect(computeHasMetadataOperator('from test* | keep metadata')).toBe(false);
-    expect(computeHasMetadataOperator('from test* | eval x="[metadata _id]"')).toBe(false);
+    expect(computeHasMetadataOperator(getQeryAst('from test*'))).toBe(false);
+    expect(computeHasMetadataOperator(getQeryAst('from test* [metadata]'))).toBe(false);
+    expect(computeHasMetadataOperator(getQeryAst('from test* [metadata id]'))).toBe(false);
+    expect(computeHasMetadataOperator(getQeryAst('from metadata*'))).toBe(false);
+    expect(computeHasMetadataOperator(getQeryAst('from test* | keep metadata'))).toBe(false);
+    expect(computeHasMetadataOperator(getQeryAst('from test* | eval x="[metadata _id]"'))).toBe(
+      false
+    );
   });
   it('should be true if query has operator', () => {
-    expect(computeHasMetadataOperator('from test* metadata _id')).toBe(true);
-    expect(computeHasMetadataOperator('from test* metadata _id, _index')).toBe(true);
-    expect(computeHasMetadataOperator('from test* metadata _index, _id')).toBe(true);
-    expect(computeHasMetadataOperator('from test*  metadata _id ')).toBe(true);
-    expect(computeHasMetadataOperator('from test*  metadata _id | limit 10')).toBe(true);
+    expect(computeHasMetadataOperator(getQeryAst('from test* metadata _id'))).toBe(true);
+    expect(computeHasMetadataOperator(getQeryAst('from test* metadata _id, _index'))).toBe(true);
+    expect(computeHasMetadataOperator(getQeryAst('from test* metadata _index, _id'))).toBe(true);
+    expect(computeHasMetadataOperator(getQeryAst('from test*  metadata _id '))).toBe(true);
+    expect(computeHasMetadataOperator(getQeryAst('from test*  metadata _id | limit 10'))).toBe(
+      true
+    );
     expect(
-      computeHasMetadataOperator(`from packetbeat* metadata 
+      computeHasMetadataOperator(
+        getQeryAst(`from packetbeat* metadata
 
         _id
         | limit 100`)
+      )
     ).toBe(true);
 
     // still validates deprecated square bracket syntax
-    expect(computeHasMetadataOperator('from test* [metadata _id]')).toBe(true);
-    expect(computeHasMetadataOperator('from test* [metadata _id, _index]')).toBe(true);
-    expect(computeHasMetadataOperator('from test* [metadata _index, _id]')).toBe(true);
-    expect(computeHasMetadataOperator('from test* [ metadata _id ]')).toBe(true);
-    expect(computeHasMetadataOperator('from test* [   metadata _id] ')).toBe(true);
-    expect(computeHasMetadataOperator('from test* [ metadata _id] | limit 10')).toBe(true);
+    expect(computeHasMetadataOperator(getQeryAst('from test* [metadata _id]'))).toBe(true);
+    expect(computeHasMetadataOperator(getQeryAst('from test* [metadata _id, _index]'))).toBe(true);
+    expect(computeHasMetadataOperator(getQeryAst('from test* [metadata _index, _id]'))).toBe(true);
+    expect(computeHasMetadataOperator(getQeryAst('from test* [ metadata _id ]'))).toBe(true);
+    expect(computeHasMetadataOperator(getQeryAst('from test* [   metadata _id] '))).toBe(true);
+    expect(computeHasMetadataOperator(getQeryAst('from test* [ metadata _id] | limit 10'))).toBe(
+      true
+    );
     expect(
-      computeHasMetadataOperator(`from packetbeat* [metadata 
+      computeHasMetadataOperator(
+        getQeryAst(`from packetbeat* [metadata
 
         _id ]
         | limit 100`)
+      )
     ).toBe(true);
   });
 });
 
 describe('parseEsqlQuery', () => {
   it('returns isMissingMetadataOperator true when query is not aggregating and does not have metadata operator', () => {
-    computeIsESQLQueryAggregatingMock.mockReturnValueOnce(false);
+    isAggregatingQueryMock.mockReturnValueOnce(false);
 
     expect(parseEsqlQuery('from test*')).toEqual({
+      errors: [],
       isEsqlQueryAggregating: false,
       isMissingMetadataOperator: true,
     });
   });
 
   it('returns isMissingMetadataOperator false when query is not aggregating and has metadata operator', () => {
-    computeIsESQLQueryAggregatingMock.mockReturnValueOnce(false);
+    isAggregatingQueryMock.mockReturnValueOnce(false);
 
     expect(parseEsqlQuery('from test* metadata _id')).toEqual({
+      errors: [],
       isEsqlQueryAggregating: false,
       isMissingMetadataOperator: false,
     });
   });
 
   it('returns isMissingMetadataOperator false when query is aggregating', () => {
-    computeIsESQLQueryAggregatingMock.mockReturnValue(true);
+    isAggregatingQueryMock.mockReturnValue(true);
 
     expect(parseEsqlQuery('from test*')).toEqual({
+      errors: [],
       isEsqlQueryAggregating: true,
       isMissingMetadataOperator: false,
     });
 
     expect(parseEsqlQuery('from test* metadata _id')).toEqual({
+      errors: [],
       isEsqlQueryAggregating: true,
       isMissingMetadataOperator: false,
     });
   });
+
+  it('returns error when query is syntactically invalid', () => {
+    isAggregatingQueryMock.mockReturnValueOnce(false);
+
+    expect(parseEsqlQuery('aaa bbbb ssdasd')).toEqual({
+      errors: expect.arrayContaining([
+        expect.objectContaining({
+          message:
+            "SyntaxError: mismatched input 'aaa' expecting {'explain', 'from', 'meta', 'metrics', 'row', 'show'}",
+        }),
+      ]),
+      isEsqlQueryAggregating: false,
+      isMissingMetadataOperator: true,
+    });
+  });
 });
diff --git a/...k/plugins/security_solution/public/detection_engine/rule_creation/logic/esql_validator.ts b/...k/plugins/security_solution/public/detection_engine/rule_creation/logic/esql_validator.ts
@@ -7,8 +7,11 @@
 
 import { isEmpty } from 'lodash';
 import type { QueryClient } from '@tanstack/react-query';
-import { computeIsESQLQueryAggregating } from '@kbn/securitysolution-utils';
+import { isAggregatingQuery } from '@kbn/securitysolution-utils';
 
+import type { ESQLAst } from '@kbn/esql-ast';
+import { getAstAndSyntaxErrors } from '@kbn/esql-ast';
+import { isColumnItem, isOptionItem } from '@kbn/esql-validation-autocomplete';
 import { KibanaServices } from '../../../common/lib/kibana';
 
 import type { ValidationError, ValidationFunc } from '../../../shared_imports';
@@ -21,6 +24,7 @@ export type FieldType = 'string';
 
 export enum ERROR_CODES {
   INVALID_ESQL = 'ERR_INVALID_ESQL',
+  INVALID_SYNTAX = 'ERR_INVALID_SYNTAX',
   ERR_MISSING_ID_FIELD_FROM_RESULT = 'ERR_MISSING_ID_FIELD_FROM_RESULT',
 }
 
@@ -34,11 +38,52 @@ const constructValidationError = (error: Error) => {
   };
 };
 
+const constructSyntaxError = (error: Error) => {
+  return {
+    code: ERROR_CODES.INVALID_SYNTAX,
+    message: error?.message
+      ? i18n.esqlValidationErrorMessage(error.message)
+      : i18n.ESQL_VALIDATION_UNKNOWN_ERROR,
+    error,
+  };
+};
+
+const getMetadataOption = (ast: ESQLAst) => {
+  const fromCommand = ast.find((astItem) => astItem.type === 'command' && astItem.name === 'from');
+
+  if (!fromCommand?.args) {
+    return undefined;
+  }
+
+  // Check whether the `from` command has `metadata` operator
+  for (const fromArg of fromCommand.args) {
+    if (isOptionItem(fromArg) && fromArg.name === 'metadata') {
+      return fromArg;
+    }
+  }
+
+  return undefined;
+};
+
 /**
  * checks whether query has metadata _id operator
  */
-export const computeHasMetadataOperator = (esqlQuery: string) => {
-  return /(?<!\|[\s\S.]*)\s*metadata[\s\S.]*_id[\s\S.]*/i.test(esqlQuery?.split('|')?.[0]);
+export const computeHasMetadataOperator = (ast: ESQLAst) => {
+  // Check whether the `from` command has `metadata` operator
+  const metadataOption = getMetadataOption(ast);
+  if (!metadataOption) {
+    return false;
+  }
+
+  // Check whether the `metadata` operator has `_id` argument
+  const idColumnItem = metadataOption.args.find(
+    (fromArg) => isColumnItem(fromArg) && fromArg.name === '_id'
+  );
+  if (!idColumnItem) {
+    return false;
+  }
+
+  return true;
 };
 
 /**
@@ -61,7 +106,12 @@ export const esqlValidator = async (
     const queryClient = (customData.value as { queryClient: QueryClient | undefined })?.queryClient;
 
     const services = KibanaServices.get();
-    const { isEsqlQueryAggregating, isMissingMetadataOperator } = parseEsqlQuery(query);
+    const { isEsqlQueryAggregating, isMissingMetadataOperator, errors } = parseEsqlQuery(query);
+
+    // Check if there are any syntax errors
+    if (errors.length) {
+      return constructSyntaxError(new Error(errors[0].message));
+    }
 
     if (isMissingMetadataOperator) {
       return {
@@ -97,11 +147,14 @@ export const esqlValidator = async (
  * - if it's non aggregation query it must have metadata operator
  */
 export const parseEsqlQuery = (query: string) => {
-  const isEsqlQueryAggregating = computeIsESQLQueryAggregating(query);
+  const { ast, errors } = getAstAndSyntaxErrors(query);
+
+  const isEsqlQueryAggregating = isAggregatingQuery(ast);
 
   return {
+    errors,
     isEsqlQueryAggregating,
     // non-aggregating query which does not have [metadata], is not a valid one
-    isMissingMetadataOperator: !isEsqlQueryAggregating && !computeHasMetadataOperator(query),
+    isMissingMetadataOperator: !isEsqlQueryAggregating && !computeHasMetadataOperator(ast),
   };
 };
diff --git a/..._solution/public/detection_engine/rule_creation_ui/hooks/use_all_esql_rule_fields.test.ts b/..._solution/public/detection_engine/rule_creation_ui/hooks/use_all_esql_rule_fields.test.ts
@@ -12,11 +12,9 @@ import { getESQLQueryColumns } from '@kbn/esql-utils';
 import { useAllEsqlRuleFields } from './use_all_esql_rule_fields';
 
 import { createQueryWrapperMock } from '../../../common/__mocks__/query_wrapper';
-import { parseEsqlQuery } from '../../rule_creation/logic/esql_validator';
+import { computeIsESQLQueryAggregating } from '@kbn/securitysolution-utils';
 
-jest.mock('../../rule_creation/logic/esql_validator', () => ({
-  parseEsqlQuery: jest.fn(),
-}));
+jest.mock('@kbn/securitysolution-utils', () => ({ computeIsESQLQueryAggregating: jest.fn() }));
 
 jest.mock('@kbn/esql-utils', () => {
   return {
@@ -25,7 +23,7 @@ jest.mock('@kbn/esql-utils', () => {
   };
 });
 
-const parseEsqlQueryMock = parseEsqlQuery as jest.Mock;
+const computeIsESQLQueryAggregatingMock = computeIsESQLQueryAggregating as jest.Mock;
 const getESQLQueryColumnsMock = getESQLQueryColumns as jest.Mock;
 
 const { wrapper } = createQueryWrapperMock();
@@ -61,7 +59,7 @@ describe.skip('useAllEsqlRuleFields', () => {
           : mockEsqlDatatable.columns
       )
     );
-    parseEsqlQueryMock.mockReturnValue({ isEsqlQueryAggregating: false });
+    computeIsESQLQueryAggregatingMock.mockReturnValue(false);
   });
 
   it('should return loading true when esql fields still loading', () => {
@@ -104,7 +102,7 @@ describe.skip('useAllEsqlRuleFields', () => {
   });
 
   it('should return index pattern fields concatenated with ES|QL fields when ES|QL query is non-aggregating', async () => {
-    parseEsqlQueryMock.mockReturnValue({ isEsqlQueryAggregating: false });
+    computeIsESQLQueryAggregatingMock.mockReturnValue(false);
 
     const { result, waitFor } = renderHook(
       () =>
@@ -127,7 +125,7 @@ describe.skip('useAllEsqlRuleFields', () => {
   });
 
   it('should return only ES|QL fields when ES|QL query is aggregating', async () => {
-    parseEsqlQueryMock.mockReturnValue({ isEsqlQueryAggregating: true });
+    computeIsESQLQueryAggregatingMock.mockReturnValue(true);
 
     const { result, waitFor } = renderHook(
       () =>
@@ -149,7 +147,7 @@ describe.skip('useAllEsqlRuleFields', () => {
 
   it('should deduplicate index pattern fields and ES|QL fields when fields have same name', async () => {
     //  getESQLQueryColumnsMock.mockClear();
-    parseEsqlQueryMock.mockReturnValue({ isEsqlQueryAggregating: false });
+    computeIsESQLQueryAggregatingMock.mockReturnValue(false);
 
     const { result, waitFor } = renderHook(
       () =>

diff --git a/...urity_solution/public/detection_engine/rule_creation_ui/hooks/use_all_esql_rule_fields.ts b/...urity_solution/public/detection_engine/rule_creation_ui/hooks/use_all_esql_rule_fields.ts
@@ -13,7 +13,7 @@ import useDebounce from 'react-use/lib/useDebounce';
 import { useQuery } from '@tanstack/react-query';
 
 import { useKibana } from '@kbn/kibana-react-plugin/public';
-import { parseEsqlQuery } from '../../rule_creation/logic/esql_validator';
+import { computeIsESQLQueryAggregating } from '@kbn/securitysolution-utils';
 
 import { getEsqlQueryConfig } from '../../rule_creation/logic/get_esql_query_config';
 
@@ -89,8 +89,8 @@ export const useAllEsqlRuleFields: UseAllEsqlRuleFields = ({ esqlQuery, indexPat
   const [debouncedEsqlQuery, setDebouncedEsqlQuery] = useState<string | undefined>(undefined);
   const { fields: esqlFields, isLoading } = useEsqlFields(debouncedEsqlQuery);
 
-  const { isEsqlQueryAggregating } = useMemo(
-    () => parseEsqlQuery(debouncedEsqlQuery ?? ''),
+  const isEsqlQueryAggregating = useMemo(
+    () => computeIsESQLQueryAggregating(debouncedEsqlQuery ?? ''),
     [debouncedEsqlQuery]
   );
 

diff --git a/x-pack/plugins/security_solution/tsconfig.json b/x-pack/plugins/security_solution/tsconfig.json
@@ -208,6 +208,8 @@
     "@kbn/core-theme-browser",
     "@kbn/integration-assistant-plugin",
     "@kbn/avc-banner",
+    "@kbn/esql-ast",
+    "@kbn/esql-validation-autocomplete",
     "@kbn/config",
   ]
 }
diff --git a/...ion_cypress/cypress/e2e/detection_response/detection_engine/rule_creation/esql_rule.cy.ts b/...ion_cypress/cypress/e2e/detection_response/detection_engine/rule_creation/esql_rule.cy.ts
@@ -180,6 +180,17 @@ describe(
 
         cy.get(ESQL_QUERY_BAR).contains('Error validating ES|QL');
       });
+
+      it('shows syntax error when query is syntactically invalid - prioritizing it over missing metadata operator error', function () {
+        const invalidNonAggregatingQuery = 'from auditbeat* | limit 5 test';
+        selectEsqlRuleType();
+        fillEsqlQueryBar(invalidNonAggregatingQuery);
+        getDefineContinueButton().click();
+
+        cy.get(ESQL_QUERY_BAR).contains(
+          `Error validating ES|QL: "SyntaxError: extraneous input 'test' expecting <EOF>"`
+        );
+      });
     });
 
     describe('ES|QL investigation fields', () => {