[ML] Adding additional runtime mapping checks#94760
[ML] Adding additional runtime mapping checks#94760jgowdyelastic merged 12 commits intoelastic:masterfrom
Conversation
|
@elasticmachine merge upstream |
|
Pinging @elastic/ml-ui (:ml) |
...lugins/ml/public/application/jobs/new_job/common/job_creator/util/filter_runtime_mappings.ts
Show resolved
Hide resolved
|
@elasticmachine merge upstream |
|
@elasticmachine merge upstream |
| // return all nested keys in the object | ||
| // most will not be fields, but better to catch everything | ||
| // and not accidentally remove a used runtime field. | ||
| if (typeof val === 'object' && val !== null) { |
There was a problem hiding this comment.
you can use isPopulatedObject instead
| return fields; | ||
| } | ||
|
|
||
| function findFieldsInQuery(obj: Record<string, any>) { |
There was a problem hiding this comment.
If I'm not mistaken we have some Query type provided by the data plugin, but otherwise Record<string, object | null> would be better
| function isLowerCase(str: string) { | ||
| return /^[a-z]+$/.test(str); | ||
| } |
There was a problem hiding this comment.
If a string contains a number or some special charts it will return false. Have you considered something like example below?
| function isLowerCase(str: string) { | |
| return /^[a-z]+$/.test(str); | |
| } | |
| function isLowerCase(str: string) { | |
| return !/[A-Z]+/.test(str) | |
| } |
|
Tested and LGTM |
peteharverson
left a comment
There was a problem hiding this comment.
Tested latest changes and LGTM
pheyos
left a comment
There was a problem hiding this comment.
Great to see functional tests coming as part of this PR 🎉
It would be good to add some validation messages, I've left two comments with examples, but they apply to most of the validations.
Also, I'd suggest to add two additional test cases to validate that the users ML_UNAUTHORIZED and ML_VIEWER are not allowed to call this endpoint.
|
@elasticmachine merge upstream |
💚 Build Succeeded
Metrics [docs]Async chunks
History
To update your PR or re-run it, just comment with: |
* [ML] Adding additional runtime mapping checks * adding functional test for datafeed preview * renaming findFieldsInAgg * updating query check * always use runtime mappings if present in agg field exists check * changes based on review * updating tests based on review * fixing permission check on endpoint and test Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
* [ML] Adding additional runtime mapping checks * adding functional test for datafeed preview * renaming findFieldsInAgg * updating query check * always use runtime mappings if present in agg field exists check * changes based on review * updating tests based on review * fixing permission check on endpoint and test Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> Co-authored-by: James Gowdy <jgowdy@elastic.co>
Adds runtime mappings to queries used for determining the index's time range.
Corrects the on save runtime mappings filter to also look in the
queryobject as well as theaggregationobject for used field names.Adds some functional tests for the datafeed preview endpoint.
Related to #91534 #91168
Unit or functional tests were updated or added to match the most common scenarios
This was checked for breaking API changes and was labeled appropriately