[Hotfix] Clear saved query crashes kibana on Discover in some cases by Dosant · Pull Request #63554 · elastic/kibana

Dosant · 2020-04-15T09:20:43Z

Summary

This hot fixes bug in Discover when clearing "Saved query". #63505
Bug is reproduced reliably in any environment if preferred query language in uiSettings is different from saved query language in localStorage. :

To reproduce:

Assuming KQL is default language
Go to discover and save a query with KQL
Clear the query
Switch to lucene
Reload the page (lucene should be in the query bar)
Apply the saved query (the one with KQL)
Clear query
Browser is frozen

The bug causing the loop is happening on integration level between discover and search_bar src/plugins/data/public/ui/search_bar/lib/use_saved_query.ts. When clearing saved_query search_bar tried to switch query to value from uiSettings, but discover then updated it to the value from localStorage. Then there is a loop condition caused by this and this has to be fixed separately in a generic way. I'll create a tech-debt ticket.

I don't believe this is a proper fix. It is just the simplest and with the least amount of risk one.
This is the commit with the actual fix: 12c9c56

TODO:

Add e2e test in this pr
Create a tech debt ticket for a connected_search_bar to not cause infinite loops depending on inputs - this would be a proper fix. We need to increase test coverage focused on redundant renderings. Statefull <SearchBar/> hardening #63675

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
This was checked for keyboard-only and screenreader accessibility
This renders correctly on smaller devices using a responsive layout. (You can test this in your browser
This was checked for cross-browser compatibility, including a check against IE11

For maintainers

This was checked for breaking API changes and was labeled appropriately

Dosant · 2020-04-15T09:25:20Z

src/legacy/core_plugins/kibana/public/discover/np_ready/angular/discover.js

-      //reset filters and query string, remove savedQuery from state
+      // remove savedQueryId from state
      const state = {
        ...appStateContainer.getState(),


This what was causing a "loop". Since as a hot fix I just moved localStorage.get('kibana.userQueryLanguage') into search_bar logic, this additional logic is not needed anymore.
query and filters are anyway synced outside of this

If I'd leave this code as is - the loop still will be fixed just by other changes. See the commit: 12c9c56

Dosant · 2020-04-15T11:00:21Z

test/functional/apps/discover/_saved_queries.js

+
+      // fails: bug in discover https://github.com/elastic/kibana/issues/63561
+      // unskip this test when bug is fixed
+      it.skip('changing language removes saved query', async () => {


Noticed different bug, a bit related but with lower severity. created an issue: #63561

elasticmachine · 2020-04-15T11:22:17Z

Pinging @elastic/kibana-app-arch (Team:AppArch)

kertal

Code LGTM from KibanaApp/Discover side, tested locally in Chrome 80, no more endless looping, thx!

mattkime · 2020-04-15T17:09:15Z

I'm having some trouble producing the problem and I'm therefore uncertain if this fixes it. @Dosant perhaps we should pair, seems like it should be easy to reproduce.

mattkime · 2020-04-15T17:53:42Z

Was able to reproduce the problem and verify that this fixes the problem. Thanks @Dosant

…astic#63554) * actual hotfix * clean up redundant code * add functional test

…3554) (#63628) * actual hotfix * add functional test

…3554) (#63627) * actual hotfix * add functional test

kibanamachine · 2020-04-16T11:21:27Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 6895a6f

History

💚 Build #40912 succeeded 6895a6f
💚 Build #40888 succeeded eb0c7a0

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

* master: (56 commits) [i18n] Update CODEOWNERS (elastic#63354) add platform team definition of done (elastic#59993) [SIEM] move away from Joi for importing/exporting timeline (elastic#62125) Fix discover preserve url (elastic#63580) [alerting] Adds an alertServices mock and uses it in siem, monitoring and uptime (elastic#63489) Closes elastic#63109 for Service Map by resetting edges styles for the selected node (elastic#63655) MIgrated index_header to react (elastic#63490) Index pattern management UI -> TypeScript and New Platform Ready (indexed_fields_table) (elastic#63364) [SIEM] [Cases] Insert timeline and reporters/tags in table bug fixes (elastic#63642) [Reporting] Make usable default element positions (elastic#63191) [Reporting] Switch Serverside Config Wrapper to NP (elastic#62500) [Reporting] Add "warning" status as an alternate type of completed job (elastic#63498) Split action types into own page (elastic#63516) [Lens] Only show copy on save for previously saved docs (elastic#63535) Update README.md (elastic#63622) Bugfix clear saved query crashes kibana on Discover in some cases (elastic#63554) Add uptime CODEOWNER entries. (elastic#63616) [ML] Extract apiDoc params from the schema definitions (elastic#62933) Fix alerting documentation encryption key requirement (elastic#63512) Fix CODEOWNERS and sass lint paths (elastic#63552) ...

Dosant added 2 commits April 15, 2020 11:03

actual hotfix

12c9c56

clean up redundant code

eb0c7a0

Dosant mentioned this pull request Apr 15, 2020

Clear saved query crashes kibana on Discover in some cases #63505

Closed

Dosant commented Apr 15, 2020

View reviewed changes

add functional test

6895a6f

Dosant commented Apr 15, 2020

View reviewed changes

Dosant added bug Fixes for quality problems that affect the customer experience release_note:skip Skip the PR/issue when compiling release notes Team:AppArch v7.7.0 v8.0.0 labels Apr 15, 2020

Dosant marked this pull request as ready for review April 15, 2020 11:22

Dosant requested a review from a team April 15, 2020 11:22

Dosant requested a review from a team as a code owner April 15, 2020 11:22

Dosant requested review from kertal, lizozom, mattkime and ppisljar April 15, 2020 11:23

kertal approved these changes Apr 15, 2020

View reviewed changes

kertal added the v7.8.0 label Apr 15, 2020

mattkime approved these changes Apr 15, 2020

View reviewed changes

Dosant merged commit c57297c into elastic:master Apr 15, 2020

Dosant added a commit to Dosant/kibana that referenced this pull request Apr 15, 2020

Bugfix clear saved query crashes kibana on Discover in some cases (el…

601e25a

…astic#63554) * actual hotfix * clean up redundant code * add functional test

Dosant mentioned this pull request Apr 15, 2020

[7.x] Bugfix clear saved query crashes kibana on Discover in some cases (#63554) #63627

Merged

Dosant added a commit to Dosant/kibana that referenced this pull request Apr 15, 2020

Bugfix clear saved query crashes kibana on Discover in some cases (el…

1c249de

…astic#63554) * actual hotfix * clean up redundant code * add functional test

Dosant mentioned this pull request Apr 15, 2020

[7.7] Bugfix clear saved query crashes kibana on Discover in some cases (#63554) #63628

Merged

mattkime pushed a commit that referenced this pull request Apr 15, 2020

Bugfix clear saved query crashes kibana on Discover in some cases (#6…

e8fcb49

…3554) (#63628) * actual hotfix * add functional test

mattkime pushed a commit that referenced this pull request Apr 15, 2020

Bugfix clear saved query crashes kibana on Discover in some cases (#6…

24abdd4

…3554) (#63627) * actual hotfix * add functional test

kertal added the Feature:Discover Discover Application label Apr 17, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Hotfix] Clear saved query crashes kibana on Discover in some cases#63554

[Hotfix] Clear saved query crashes kibana on Discover in some cases#63554
Dosant merged 3 commits intoelastic:masterfrom
Dosant:dev/discover-clear-saved-query-hotfix

Dosant commented Apr 15, 2020 •

edited

Loading

Uh oh!

Dosant Apr 15, 2020 •

edited

Loading

Uh oh!

Dosant Apr 15, 2020

Uh oh!

elasticmachine commented Apr 15, 2020

Uh oh!

kertal left a comment

Uh oh!

mattkime commented Apr 15, 2020 •

edited

Loading

Uh oh!

mattkime commented Apr 15, 2020

Uh oh!

kibanamachine commented Apr 16, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Conversation

Dosant commented Apr 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Checklist

For maintainers

Uh oh!

Dosant Apr 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Dosant Apr 15, 2020

Choose a reason for hiding this comment

Uh oh!

elasticmachine commented Apr 15, 2020

Uh oh!

kertal left a comment

Choose a reason for hiding this comment

Uh oh!

mattkime commented Apr 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mattkime commented Apr 15, 2020

Uh oh!

kibanamachine commented Apr 16, 2020

💚 Build Succeeded

History

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Dosant commented Apr 15, 2020 •

edited

Loading

Dosant Apr 15, 2020 •

edited

Loading

mattkime commented Apr 15, 2020 •

edited

Loading