[Lens] Fix indexpattern checks for missing references #88840

dej611 · 2021-01-20T14:18:07Z

Summary

This PR addresses a regression on the indexpattern checks on the workspace side of the Lens editor.
For some operations, like the terms one, the indexpattern argument was required to perform a full check, and while this was correctly propagated on the dimension panel side, it was omitted when checking in the workspace.

This PR fixes this by adding the indexpattern as argument (when available).
Also a test to check for correct propagation has been added.

Added

While working in the same area for the full reference operation fix, I've added also a fix for the #89141 bug.

If field is no longer valid it highlights it in the reference editor:

Also the full reference operation is now validating against its internal sub-function as well and showing a warning sign in the panel:

In case for some reason a saved chart gets into a state where there's a missing field and its saved operation is no longer valid, both fields gets highlighted now:

Checklist

Unit or functional tests were updated or added to match the most common scenarios

elasticmachine · 2021-01-20T16:46:40Z

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

wylieconlon

Tested on the same example as before and it worked as expected.

wylieconlon · 2021-01-20T22:53:05Z

x-pack/plugins/lens/public/indexpattern_datasource/operations/layer_helpers.ts

+      // If we're transitioning to another operation, check for "new" incompleteColumns rather
+      // than "old" saved operation on the layer
+      const columnFinalRef =
+        layer.incompleteColumns?.[columnId]?.operationType || column.operationType;
+      const def = operationDefinitionMap[columnFinalRef];


This chunk could cause a merge conflict with my other open PR, but it should be fine to merge as-is and then fix.

But this is unchanged from master

Ok, checked your PR #88916 together with this one and it works as expected in all the various scenarios. So I'll skip to refactor this and approve your so the merge will provide a fix for both issues.

wylieconlon · 2021-01-20T22:53:42Z

x-pack/plugins/lens/public/indexpattern_datasource/operations/layer_helpers.ts

+  indexPattern?: IndexPattern
+): string[] | undefined {
+  const errors: string[] = Object.entries(layer.columns)
+    .flatMap(([columnId, column]) => {


So in terms of style, you're using flatMap and then filter as opposed to forEach- seems fine.

I was just hoping to use a simple .filter(Boolean) at the end, but Typescript wasn't happy about that., therefore I had to god for an explicit typeguard.

flash1293

Looks like in x-pack/plugins/lens/public/indexpattern_datasource/operations/definitions/index.ts the index pattern parameter for getErrorMessage is still marked as optional, it seems like we pass it in everywhere now so we can make it required, right?

dej611 · 2021-01-21T10:34:53Z

Not all definitions require the indexPattern as argument, I think it has been designed on the minimal set of arguments to work in general.
It could be imposed and just discard when not used.

flash1293 · 2021-01-21T10:41:47Z

@dej611 I meant this ? in particular:

kibana/x-pack/plugins/lens/public/indexpattern_datasource/operations/definitions/index.ts

Line 200 in 86789da

indexPattern?: IndexPattern

The consumers don't have to use it, but they can be sure it will always be available if they need it.

dej611 · 2021-01-25T10:12:33Z

@elasticmachine merge upstream

flash1293 · 2021-01-25T12:01:21Z

@elasticmachine merge upstream

flash1293

Tested and LGTM - I also checked the case we talked about (problem in a "hidden" dimension ) and it's not correctly propagated. I will open a separate issue for this

dej611 · 2021-01-25T17:37:21Z

@elasticmachine merge upstream

dej611 · 2021-01-25T18:39:42Z

I've added a couple more fixes in the area while debugging this PR.

dej611 · 2021-01-26T08:45:02Z

@elasticmachine merge upstream

flash1293

I tested and it seems like the error is shown too often:

Create vis with "moving average of percentile of val"
Change "val" field to type string
Go back to Lens (shows as broken as expected)
Change oepration to "moving average of unique count of val.keyword"
Still shown as error

Even on recreating this configuration, the dimension is shown as error.

Uncaught error in the logs:

Uncaught (in promise) TypeError: Cannot read property 'getErrorMessage' of undefined
    at utils.ts:43
    at Array.map (<anonymous>)
    at isColumnInvalid (utils.ts:38)
    at dimension_panel.tsx:32
    at Object.useMemo (kbn-ui-shared-deps.js:375)
    at useMemo (kbn-ui-shared-deps.js:353)
    at IndexPatternDimensionTrigger (dimension_panel.tsx:32)
    at ua (kbn-ui-shared-deps.js:375)
    at Ga (kbn-ui-shared-deps.js:375)
    at qa (kbn-ui-shared-deps.js:375)

flash1293 · 2021-01-26T09:39:24Z

x-pack/plugins/lens/public/indexpattern_datasource/utils.ts

-    operationDefinition.getErrorMessage &&
-    operationDefinition.getErrorMessage(layer, columnId, indexPattern)
+  // check also references for errors
+  const referencesHaveErrors = (column as ReferenceBasedIndexPatternColumn).references


nit: You don't need to cast here, 'references' in column && will do it in a type safe way

flash1293 · 2021-01-26T09:40:04Z

x-pack/plugins/lens/public/indexpattern_datasource/utils.ts

-  return !!(
-    operationDefinition.getErrorMessage &&
-    operationDefinition.getErrorMessage(layer, columnId, indexPattern)
+  // check also references for errors


Fine for now, but note: Once we introduce formula, we have to extend this to follow references through multiple columns, not just a single "hop"

I've refactor that to be in a distinct function, so we could easily change it later.

dej611 · 2021-01-26T10:50:06Z

@flash1293 you are quite right. There was a missing .length there (the errors array was correctly empty). This should be fixed right now.

kibanamachine · 2021-01-26T17:58:48Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 4ae7fb4

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`	851.7KB	853.3KB	+1.6KB

History

💔 Build #101043 failed ad51514
💔 Build #101007 failed 16c5e8a
💔 Build #100824 failed 726f085
💚 Build #100808 succeeded 9314ff7
💛 Build #100677 was flaky 92cbb3b

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

wylieconlon

I manually tested both cases that you're fixing, and I'm glad you've added some unit tests for these cases. So LGTM!

outdated

…y-tests * 'master' of github.com:elastic/kibana: (276 commits) [Telemetry] Settings Collector: redact sensitive reported values (elastic#88675) [CI] Combines Jest test jobs (elastic#85850) [Upgrade Assistant] Migrate server to new es-js client (elastic#89207) Migrate maps_legacy, maps_oss, region_map, and tile_map plugions to TS projects (elastic#89351) [Vega Docs] Add experimental flag on the vega maps title (elastic#89402) Increase the time needed to locate the save viz toast (elastic#89301) [Enterprise Search] Add links to doc links service (elastic#89260) Fixed regex bug in Safari (elastic#89399) [Lens] Fix indexpattern checks for missing references (elastic#88840) [Lens] Clean up usage collector (elastic#89109) update apm index pattern (elastic#89395) [APM] Upgrade ES client (elastic#86594) Enable v2 so migrations, disable in FTR tests (elastic#89297) [Search Sessions] Make search session indicator UI opt-in, refactor per-app capabilities (elastic#88699) Cleanup OSS code from visualizations wizard (elastic#89092) [APM] Optimize API test order (elastic#88654) Rename conversion function, extract to module scope and add tests. (elastic#89018) [core.logging] Add ops logs to the KP logging system (elastic#88070) chore(NA): improve ts build refs performance on kbn bootstrap (elastic#89333) skip flaky suite (elastic#89379) ... # Conflicts: # x-pack/plugins/index_lifecycle_management/public/application/sections/edit_policy/components/timeline/timeline.tsx # x-pack/test/accessibility/config.ts

…ana into task-manager/shift-on-trend * 'task-manager/shift-on-trend' of github.com:gmmorris/kibana: (74 commits) [Metrics UI] Fix Host Overview boxes in Host Detail page (elastic#89299) [Telemetry] Settings Collector: redact sensitive reported values (elastic#88675) [CI] Combines Jest test jobs (elastic#85850) [Upgrade Assistant] Migrate server to new es-js client (elastic#89207) Migrate maps_legacy, maps_oss, region_map, and tile_map plugions to TS projects (elastic#89351) [Vega Docs] Add experimental flag on the vega maps title (elastic#89402) Increase the time needed to locate the save viz toast (elastic#89301) [Enterprise Search] Add links to doc links service (elastic#89260) Fixed regex bug in Safari (elastic#89399) [Lens] Fix indexpattern checks for missing references (elastic#88840) [Lens] Clean up usage collector (elastic#89109) update apm index pattern (elastic#89395) [APM] Upgrade ES client (elastic#86594) Enable v2 so migrations, disable in FTR tests (elastic#89297) [Search Sessions] Make search session indicator UI opt-in, refactor per-app capabilities (elastic#88699) Cleanup OSS code from visualizations wizard (elastic#89092) [APM] Optimize API test order (elastic#88654) Rename conversion function, extract to module scope and add tests. (elastic#89018) [core.logging] Add ops logs to the KP logging system (elastic#88070) chore(NA): improve ts build refs performance on kbn bootstrap (elastic#89333) ...

wylieconlon · 2021-01-28T21:11:40Z

This PR was somehow missing a backport. Fixing now.

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

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

dej611 added 2 commits January 20, 2021 15:02

🐛 Forward the indexpattern when possible for checks

89f6cb4

✅ Add test to check indexpattern propagation

ada6d2a

dej611 added v8.0.0 release_note:skip Skip the PR/issue when compiling release notes Feature:Lens v7.12.0 labels Jan 20, 2021

dej611 marked this pull request as ready for review January 20, 2021 16:46

dej611 requested a review from a team January 20, 2021 16:46

dej611 added the Team:Visualizations Visualization editors, elastic-charts and infrastructure label Jan 20, 2021

wylieconlon approved these changes Jan 20, 2021

View reviewed changes

flash1293 reviewed Jan 21, 2021

View reviewed changes

🔧 Make indexPattern mandatory for operations checks

3732196

Merge branch 'master' into fix/88786

5281956

Merge branch 'master' into fix/88786

92cbb3b

flash1293 approved these changes Jan 25, 2021

View reviewed changes

kibanamachine and others added 4 commits January 25, 2021 12:37

Merge branch 'master' into fix/88786

9314ff7

🐛 Fix the missing warning on full reference broken operations

f1e1d81

🐛 Fix the broken reference editor for missing fields

650469a

Merge branch 'fix/88786' of github.com:dej611/kibana into fix/88786

726f085

dej611 requested a review from wylieconlon January 25, 2021 18:39

Merge branch 'master' into fix/88786

16c5e8a

flash1293 previously requested changes Jan 26, 2021

View reviewed changes

dej611 added 2 commits January 26, 2021 11:48

♻️ Refactor reference check

c188539

Merge branch 'fix/88786' of github.com:dej611/kibana into fix/88786

ad51514

🐛 Referenced operation can be without validation

4ae7fb4

wylieconlon approved these changes Jan 27, 2021

View reviewed changes

dej611 merged commit ba1e795 into elastic:master Jan 27, 2021

dej611 deleted the fix/88786 branch January 27, 2021 13:23

flash1293 mentioned this pull request Jan 28, 2021

[Lens] Errors in hidden dimensions not shown correctly #89141

Closed

wylieconlon mentioned this pull request Jan 28, 2021

[7.x] [Lens] Fix indexpattern checks for missing references (#88840) #89638

Merged

wylieconlon pushed a commit to wylieconlon/kibana that referenced this pull request Jan 28, 2021

[Lens] Fix indexpattern checks for missing references (elastic#88840)

9c696cf

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Lens] Fix indexpattern checks for missing references #88840

[Lens] Fix indexpattern checks for missing references #88840

dej611 commented Jan 20, 2021 •

edited

Loading

elasticmachine commented Jan 20, 2021

wylieconlon left a comment

wylieconlon Jan 20, 2021

dej611 Jan 21, 2021

dej611 Jan 27, 2021

wylieconlon Jan 20, 2021

dej611 Jan 21, 2021

flash1293 left a comment

dej611 commented Jan 21, 2021

flash1293 commented Jan 21, 2021

dej611 commented Jan 25, 2021

flash1293 commented Jan 25, 2021

flash1293 left a comment

dej611 commented Jan 25, 2021

dej611 commented Jan 25, 2021

dej611 commented Jan 26, 2021

flash1293 left a comment

flash1293 Jan 26, 2021

flash1293 Jan 26, 2021

dej611 Jan 26, 2021

dej611 commented Jan 26, 2021

kibanamachine commented Jan 26, 2021

wylieconlon left a comment

wylieconlon commented Jan 28, 2021

[Lens] Fix indexpattern checks for missing references #88840

[Lens] Fix indexpattern checks for missing references #88840

Conversation

dej611 commented Jan 20, 2021 • edited Loading

Summary

Checklist

elasticmachine commented Jan 20, 2021

wylieconlon 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

flash1293 left a comment

Choose a reason for hiding this comment

dej611 commented Jan 21, 2021

flash1293 commented Jan 21, 2021

dej611 commented Jan 25, 2021

flash1293 commented Jan 25, 2021

flash1293 left a comment

Choose a reason for hiding this comment

dej611 commented Jan 25, 2021

dej611 commented Jan 25, 2021

dej611 commented Jan 26, 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

dej611 commented Jan 26, 2021

kibanamachine commented Jan 26, 2021

💚 Build Succeeded

Metrics [docs]

Async chunks

History

wylieconlon left a comment

Choose a reason for hiding this comment

wylieconlon commented Jan 28, 2021

dej611 commented Jan 20, 2021 •

edited

Loading