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 index 44deada7cd155..251c00d4a75b4 100644 --- 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 index c734b5c153fb0..5b9520c487e31 100644 --- 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/x-pack/plugins/security_solution/public/detection_engine/rule_creation/logic/esql_validator.test.ts b/x-pack/plugins/security_solution/public/detection_engine/rule_creation/logic/esql_validator.test.ts index 07f14830d6a71..2fdd8cf8120d7 100644 --- a/x-pack/plugins/security_solution/public/detection_engine/rule_creation/logic/esql_validator.test.ts +++ b/x-pack/plugins/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/x-pack/plugins/security_solution/public/detection_engine/rule_creation/logic/esql_validator.ts b/x-pack/plugins/security_solution/public/detection_engine/rule_creation/logic/esql_validator.ts index 484f78c53f0e0..869e379c21aed 100644 --- a/x-pack/plugins/security_solution/public/detection_engine/rule_creation/logic/esql_validator.ts +++ b/x-pack/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 /(? { + // 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/x-pack/plugins/security_solution/public/detection_engine/rule_creation_ui/hooks/use_all_esql_rule_fields.test.ts b/x-pack/plugins/security_solution/public/detection_engine/rule_creation_ui/hooks/use_all_esql_rule_fields.test.ts index 377a1772a5ea0..1a13f0ff8e3a2 100644 --- a/x-pack/plugins/security_solution/public/detection_engine/rule_creation_ui/hooks/use_all_esql_rule_fields.test.ts +++ b/x-pack/plugins/security_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/x-pack/plugins/security_solution/public/detection_engine/rule_creation_ui/hooks/use_all_esql_rule_fields.ts b/x-pack/plugins/security_solution/public/detection_engine/rule_creation_ui/hooks/use_all_esql_rule_fields.ts index a67b990c88b80..80bfc364e5b51 100644 --- a/x-pack/plugins/security_solution/public/detection_engine/rule_creation_ui/hooks/use_all_esql_rule_fields.ts +++ b/x-pack/plugins/security_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(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 index 3258167eb50b7..bdaf656b9c986 100644 --- 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/x-pack/test/security_solution_cypress/cypress/e2e/detection_response/detection_engine/rule_creation/esql_rule.cy.ts b/x-pack/test/security_solution_cypress/cypress/e2e/detection_response/detection_engine/rule_creation/esql_rule.cy.ts index 1dad7edc63ab6..348133b1d2802 100644 --- a/x-pack/test/security_solution_cypress/cypress/e2e/detection_response/detection_engine/rule_creation/esql_rule.cy.ts +++ b/x-pack/test/security_solution_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 "` + ); + }); }); describe('ES|QL investigation fields', () => {