[Lens] Formula: add validation for multiple field/metrics #104092

dej611 · 2021-07-01T12:46:33Z

Summary

This PR adds one more validation case if the user passes too many fields/metrics to an operation.

Checklist

Delete any items that are not applicable to this PR.

Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support
Documentation was added for features that require explanation or tutorials
Unit or functional tests were updated or added to match the most common scenarios
Any UI touched in this PR is usable by keyboard only (learn more about keyboard accessibility)
Any UI touched in this PR does not create any new axe failures (run axe in browser: FF, Chrome)
If a plugin configuration key changed, check if it needs to be allowlisted in the cloud and added to the docker list
This renders correctly on smaller devices using a responsive layout. (You can test this in your browser)
This was checked for cross-browser compatibility

For maintainers

This was checked for breaking API changes and was labeled appropriately

elasticmachine · 2021-07-01T12:46:35Z

Pinging @elastic/kibana-app (Team:KibanaApp)

wylieconlon

While this logic appears to work for cases where the user has duplicate types, like sum(bytes, bytes) or moving_average(count(), sum()), it does not catch mixed types like sum(bytes, count()) which are also invalid.

Also, I think we should backport this 7.14 and mark this as a release_note:skip, I would mark it as a bugfix only if we ship the broken state to users.

dej611 · 2021-07-02T14:34:45Z

Add one more validation case, for the mixed mode in particular, and enriched tests with different type of parameters as well.

flash1293 · 2021-07-05T07:29:27Z

@elasticmachine merge upstream

flash1293

Didn't manage to break it, but I'm unsure about some things - left some questions.

flash1293 · 2021-07-05T08:28:54Z

x-pack/plugins/lens/public/indexpattern_datasource/operations/definitions/formula/validation.ts

@@ -633,6 +701,22 @@ function runFullASTValidation(
            })
          );
        } else {
+          const fields = variables.filter(


Which case is caught here?

Probably need to add some more comment context on all these branches.
In this specific branch it is checking that only a single field is passed to field-only operations.

I don't think so, this branch is in the if (nodeOperation.input === 'fullReference') block.

I don't see in which case it would become necessary - the only one is this, but it shows two error for the same problem:

You're right sorry. The check has to exclude the first argument.
This check is here to validate this scenario:

moving_average(average(bytes), bytes)

I'll refactor the logic to group both first argument check and all arguents check for field values.

flash1293 · 2021-07-05T08:30:24Z

x-pack/plugins/lens/public/indexpattern_datasource/operations/definitions/formula/validation.ts

@@ -621,6 +674,21 @@ function runFullASTValidation(
              locations: node.location ? [node.location] : [],
            })
          );
+        } else {
+          if (functions.length > 1) {


I'm not sure whether this is a future-proof way to check - the way our full reference operations are set up, they could theoretically accept multiple parameters. Should we check the requiredReferences here?

This code is generic enough to check functions in different position in the node, as moving_average( median( ...), median( ... ) ) as well as moving_average( median( ...), window = 7, shift="7d", median( ... ) ).

I could parametrize the threshold to be requiredReferences.filter( /* check for functions */).length here, but in general the formula code assumes only one argument to be a reference (to a field or operation) and it may be required to change in the future if we introduce operations with different assumptions.
The parametric change here is pretty easy and I'll push a fix on that.

…a into fix/formula-multiple-fields

flash1293 · 2021-07-06T09:51:43Z

x-pack/plugins/lens/public/indexpattern_datasource/operations/definitions/formula/validation.ts

+          if (functions.length > requiredFunctions) {
+            errors.push(
+              getMessageFromId({
+                messageId: 'tooManyFirstArguments',


The message still says a single ..., but I think it's fine for now - as you mentioned the parsing logic can't handle this either at the moment.

flash1293 · 2021-07-06T09:56:15Z

x-pack/plugins/lens/public/indexpattern_datasource/operations/definitions/formula/validation.ts

@@ -633,6 +701,22 @@ function runFullASTValidation(
            })
          );
        } else {
+          const fields = variables.filter(


I don't think so, this branch is in the if (nodeOperation.input === 'fullReference') block.

I don't see in which case it would become necessary - the only one is this, but it shows two error for the same problem:

…ple-fields

wylieconlon

On a meta level, I think this code is hard to deal with because of groupArgsByType. For example sum(bytes, 5) is probably worth throwing an error for, same with moving_average(sum(bytes), 5), but scanning through the code I'm not there's a simple validator for both. What I was hoping for was a simple test that would catch all of the "extra arguments" cases, like what we do for Math: if (node.args.length > positionalArguments.length) {. So if I can suggest a refactoring here, it would be that I'd iterate over each argument in position order instead of using the grouped arguments. This would simplify the code and also catch edge cases like the one I've mentioned above.

dej611 · 2021-07-07T09:17:17Z

On a meta level, I think this code is hard to deal with because of groupArgsByType. For example sum(bytes, 5) is probably worth throwing an error for, same with moving_average(sum(bytes), 5), but scanning through the code I'm not there's a simple validator for both. What I was hoping for was a simple test that would catch all of the "extra arguments" cases, like what we do for Math: if (node.args.length > positionalArguments.length) {. So if I can suggest a refactoring here, it would be that I'd iterate over each argument in position order instead of using the grouped arguments. This would simplify the code and also catch edge cases like the one I've mentioned above.

I think this makes sense over a general refactoring of the validation code, rather than this specific PR to fix some edge cases.

I've refactored the code now and added many more test cases with both positional and type-based issues and it all passes with the new implementation.

dej611 · 2021-07-07T09:17:29Z

@elasticmachine merge upstream

…a into fix/formula-multiple-fields

dej611 · 2021-07-08T09:23:38Z

@elasticmachine merge upstream

dej611 · 2021-07-09T13:15:39Z

@elasticmachine merge upstream

dej611 · 2021-07-12T08:36:33Z

@elasticmachine merge upstream

kibanamachine · 2021-07-12T10:46:07Z

💚 Build Succeeded

Metrics [docs]

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id	before	after	diff
`lens`	1.5MB	1.5MB	+4.1KB

History

💚 Build #137533 succeeded 7f571bf
💚 Build #137163 succeeded 07fb0a0
💚 Build #136791 succeeded aa4b7f6
💚 Build #136321 succeeded 9748083
💔 Build #136294 failed 950ff34

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

wylieconlon

Tested and LGTM. It's still missing the case where you do sum(bytes, 5) but this is worth getting into 7.14 and refactoring later as it significantly improves the semantics.

…4092) Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

kibanamachine · 2021-07-13T08:12:29Z

💚 Backport successful

Status	Branch	Result
✅	7.14
✅	7.x

The backport PRs will be merged automatically after passing CI.

…105395) Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> Co-authored-by: Marco Liberati <dej611@users.noreply.github.com>

…105396) Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> Co-authored-by: Marco Liberati <dej611@users.noreply.github.com>

…-png-pdf-report-type * 'master' of github.com:elastic/kibana: (292 commits) bring back KQL autocomplete in timeline + fix last updated (elastic#105380) [Maps] Change TOC pop-up wording to reflect filter change, not search bar change (elastic#105163) Updating urls to upstream elastic repo (elastic#105250) [Maps] Move new vector layer wizard card down (elastic#104797) Exclude registering the cases feature if not enabled (elastic#105292) [Uptime] Alerts - Monitor status alert - check monitor status by monitor.timespan (elastic#104541) updated UI copy (elastic#105184) Log a warning when documents of unknown types are detected during migration (elastic#105213) [Logs UI] Register log threshold rule as lifecycle rule (elastic#104341) [Ingest pipelines] add network direction processor (elastic#103436) [Console] Autocomplete definitions (manual backport) (elastic#105086) [Security Solution] User can make Exceptions for Memory protection alerts (elastic#102196) [Lens] Formula: add validation for multiple field/metrics (elastic#104092) Removing async from file upload and data visualizer plugins start lifecycle (elastic#105197) Fix error when validating the form with non blocking validations (elastic#103629) [ML] Fix "View by" swim lane with applied filter and sorting by score (elastic#105217) Update dependency @elastic/charts to v32 (elastic#104625) [CTI] shortens large numbers on Dashboard Link Panel (elastic#105269) [Security Solution][Endpoint][Host Isolation] Fixes bug to remove excess host metadata status toasts on non user initiated errors (elastic#105331) [Cases] Fix pushing alerts count on every push to external service (elastic#105030) ... # Conflicts: # x-pack/plugins/reporting/common/types.ts

dej611 added 2 commits July 1, 2021 14:43

🐛 Add validation for multiple field/metrics

a89059d

🔥 Remove debug comment

0dcc0b8

dej611 added release_note:fix Team:Visualizations Visualization editors, elastic-charts and infrastructure v8.0.0 Feature:Lens auto-backport Deprecated - use backport:version if exact versions are needed v7.15.0 labels Jul 1, 2021

dej611 requested a review from a team July 1, 2021 12:46

dej611 mentioned this pull request Jul 1, 2021

[Lens] Formula followups #101905

Closed

14 tasks

wylieconlon suggested changes Jul 1, 2021

View reviewed changes

👌 Added more validation cases

b50ecd5

dej611 added release_note:skip Skip the PR/issue when compiling release notes v7.14.0 and removed release_note:fix labels Jul 2, 2021

dej611 requested a review from wylieconlon July 2, 2021 14:34

Merge branch 'master' into fix/formula-multiple-fields

13eb57c

flash1293 reviewed Jul 5, 2021

View reviewed changes

dej611 added 3 commits July 5, 2021 12:26

👌 Integrate feedback

12521e6

Merge branch 'fix/formula-multiple-fields' of github.com:dej611/kiban…

950ff34

…a into fix/formula-multiple-fields

🐛 Fix missing ref case

9748083

flash1293 reviewed Jul 6, 2021

View reviewed changes

Merge remote-tracking branch 'upstream/master' into fix/formula-multi…

8aee877

…ple-fields

wylieconlon suggested changes Jul 6, 2021

View reviewed changes

Merge branch 'master' into fix/formula-multiple-fields

8c042c1

dej611 added 2 commits July 7, 2021 11:18

🐛 Fix more edges cases

2bc89e8

Merge branch 'fix/formula-multiple-fields' of github.com:dej611/kiban…

aa4b7f6

…a into fix/formula-multiple-fields

Merge branch 'master' into fix/formula-multiple-fields

07fb0a0

Merge branch 'master' into fix/formula-multiple-fields

7f571bf

dej611 requested a review from wylieconlon July 9, 2021 13:17

Merge branch 'master' into fix/formula-multiple-fields

d863ca4

wylieconlon approved these changes Jul 12, 2021

View reviewed changes

dej611 merged commit 26bc001 into elastic:master Jul 13, 2021

dej611 deleted the fix/formula-multiple-fields branch July 13, 2021 08:09

kibanamachine added a commit to kibanamachine/kibana that referenced this pull request Jul 13, 2021

[Lens] Formula: add validation for multiple field/metrics (elastic#10…

4e41708

…4092) Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

kibanamachine mentioned this pull request Jul 13, 2021

[7.14] [Lens] Formula: add validation for multiple field/metrics (#104092) #105395

Merged

kibanamachine added a commit to kibanamachine/kibana that referenced this pull request Jul 13, 2021

[Lens] Formula: add validation for multiple field/metrics (elastic#10…

9365bbe

…4092) Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

kibanamachine mentioned this pull request Jul 13, 2021

[7.x] [Lens] Formula: add validation for multiple field/metrics (#104092) #105396

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Lens] Formula: add validation for multiple field/metrics #104092

[Lens] Formula: add validation for multiple field/metrics #104092

dej611 commented Jul 1, 2021

elasticmachine commented Jul 1, 2021

wylieconlon left a comment

dej611 commented Jul 2, 2021

flash1293 commented Jul 5, 2021

flash1293 left a comment

flash1293 Jul 5, 2021

dej611 Jul 5, 2021

flash1293 Jul 6, 2021

dej611 Jul 6, 2021

flash1293 Jul 5, 2021

dej611 Jul 5, 2021

flash1293 Jul 6, 2021

flash1293 Jul 6, 2021

wylieconlon left a comment

dej611 commented Jul 7, 2021

dej611 commented Jul 7, 2021

dej611 commented Jul 8, 2021

dej611 commented Jul 9, 2021

dej611 commented Jul 12, 2021

kibanamachine commented Jul 12, 2021

wylieconlon left a comment

kibanamachine commented Jul 13, 2021

[Lens] Formula: add validation for multiple field/metrics #104092

[Lens] Formula: add validation for multiple field/metrics #104092

Conversation

dej611 commented Jul 1, 2021

Summary

Checklist

For maintainers

elasticmachine commented Jul 1, 2021

wylieconlon left a comment

Choose a reason for hiding this comment

dej611 commented Jul 2, 2021

flash1293 commented Jul 5, 2021

flash1293 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wylieconlon left a comment

Choose a reason for hiding this comment

dej611 commented Jul 7, 2021

dej611 commented Jul 7, 2021

dej611 commented Jul 8, 2021

dej611 commented Jul 9, 2021

dej611 commented Jul 12, 2021

kibanamachine commented Jul 12, 2021

💚 Build Succeeded

Metrics [docs]

Async chunks

History

wylieconlon left a comment

Choose a reason for hiding this comment

kibanamachine commented Jul 13, 2021

💚 Backport successful