[Detections] EQL query validation: Separate syntax errors into a own error type (#10181) #190149
Conversation
e40pud
added
release_note:skip
|
/ci |
|
/ci |
|
/ci |
|
Pinging @elastic/security-solution (Team: SecuritySolution) |
|
Pinging @elastic/security-detection-engine (Team:Detection Engine) |
|
@elasticmachine merge upstream |
Flaky Test Runner Stats🎉 All tests passed! - kibana-flaky-test-suite-runner#6754[✅] x-pack/test/security_solution_api_integration/test_suites/detections_response/detection_engine/rule_execution_logic/trial_license_complete_tier/configs/ess.config.ts: 100/100 tests passed. |
There was a problem hiding this comment.
Overall this looks great! The cypress tests demonstrating this behavior are fantastic, but it would be great to have some unit tests showing how this code handles different types of responses, since that's been expanded/changed significantly with this PR.
Approving because those tests should be quick and easy, but please request another review if you want/need it.
| } | ||
| ) | ||
| ); | ||
| if (isParsingErrorResponse(response)) { |
There was a problem hiding this comment.
Since this function is growing in complexity, it's probably time to add some unit tests around it (more for documentation of behavior than anything else); maybe extract a function that receives a response and test that? That would also allow us to document the shape of response(s) we handle.
type EqlValidationTransformer = (response: unknown) => {
valid: boolean;
error?: { code: EQL_ERROR_CODES; messages?: string[]; error?: Error };
};There was a problem hiding this comment.
Great point! I will add tests for this functionality
💛 Build succeeded, but was flaky
Failed CI StepsMetrics [docs]Async chunks
History
To update your PR or re-run it, just comment with: cc @e40pud |
|
Looks like cypress DE tests are flaky. I created a separate test PR to check whether cypress tests are unstable on main branch and same tests are flaky. |
kibanamachine
added
v8.16.0
backport:skip
Flaky Test Runner Stats🟠 Some tests failed. - kibana-flaky-test-suite-runner#6766[❌] Security Solution Detection Engine - Cypress: 58/100 tests passed. |
Summary
Addresses https://github.com/elastic/security-team/issues/10181
This PR is a refactoring of EQL query validator:
parsing_exception,verification_exceptionandmapping_exceptionas the same error of typeERR_INVALID_EQL. After these changes we will split these errors into separate ones: syntax (parsing_exception), invalid EQL (verification_exceptionandmapping_exception; can be split in future if needed)MISSING_DATA_SOURCE. Beforedata.search.search<EqlSearchStrategyRequest, EqlSearchStrategyResponse>()call would throw an exception in case data source does not exist and we would handle it as a failed request and show an error toast (see relevant ticket [Security Solution] Rule's EQL query validation fails unexpectedly when fields missing #178611). After these changes we would not show a toast and handle missing data source error as other EQL validation errors - showing an error message in the EQL query bar.This will allow us to distinguish between different types of EQL validation errors and will help to decide on whether certain errors are blocking during the rule creation/editing flow (#180407).
Checklist
Delete any items that are not applicable to this PR.