Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

match_all query disappears when typed into Lucene query bar #62194

Merged
merged 5 commits into from
Apr 6, 2020
Merged

match_all query disappears when typed into Lucene query bar #62194

merged 5 commits into from
Apr 6, 2020

Conversation

alexwizp
Copy link
Contributor

@alexwizp alexwizp commented Apr 1, 2020 •
edited
Loading

Closes: #52115

This PR reverts #14644 and move that logic into the migration scripts.

Summary

Summarize your PR. If it involves visual changes include a screenshot or gif.

Checklist

Delete any items that are not applicable to this PR.

For maintainers

@alexwizp alexwizp requested a review from VladLasitsa April 1, 2020 16:56
@elasticmachine
Copy link
Contributor

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

alexwizp
alexwizp commented Apr 2, 2020
View reviewed changes
src/legacy/core_plugins/kibana/migrations/migrations.js
@@ -60,6 +63,7 @@ function migrateIndexPattern(doc) {

export const migrations = {
dashboard: {
'6.7.2': migrateMatchAllQuery,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure that version '6.7.2' is a correct version, anyway this PR will be delivered only for versions >7.8 so it's not so important

@VladLasitsa VladLasitsa self-requested a review April 2, 2020 09:54
VladLasitsa
VladLasitsa approved these changes Apr 2, 2020
View reviewed changes
Copy link
Contributor

@VladLasitsa VladLasitsa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@alexwizp alexwizp requested a review from lukasolson April 2, 2020 10:09
@alexwizp alexwizp self-assigned this Apr 2, 2020
@alexwizp alexwizp marked this pull request as ready for review April 2, 2020 11:40
@alexwizp alexwizp requested a review from a team April 2, 2020 11:40
@alexwizp alexwizp requested a review from a team as a code owner April 2, 2020 11:40
@alexwizp alexwizp added the review label Apr 3, 2020
@alexwizp
Copy link
Contributor Author

alexwizp commented Apr 3, 2020

@elasticmachine merge upstream

flash1293
flash1293 reviewed Apr 3, 2020
View reviewed changes
Copy link
Contributor

@flash1293 flash1293 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't have saved searches the same problem? They are also using the searchsource json. I'm not sure about the maps app (was it around that early to have this problem as well?)

@alexwizp
Copy link
Contributor Author

alexwizp commented Apr 4, 2020

@flash1293 probably you are right, we should also do it for 'search' saved object type

ppisljar
ppisljar approved these changes Apr 6, 2020
View reviewed changes
Copy link
Member

@ppisljar ppisljar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

code LGTM

@alexwizp alexwizp requested a review from flash1293 April 6, 2020 11:36
@alexwizp
Copy link
Contributor Author

alexwizp commented Apr 6, 2020

@elasticmachine merge upstream

flash1293
flash1293 reviewed Apr 6, 2020
View reviewed changes
Copy link
Contributor

@flash1293 flash1293 left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@alexwizp I tested this by manually changing the query in the search source json of a saved search to query: { match_all: {} } and it got migrated fine.

However, I'm wondering about the following case:

  • User creates a saved search in 6.6
  • User upgrades to 7.4
  • The 7.4 migration kicks in doing unrelated stuff
  • User upgrades to 7.7
  • The 6.7.2 migration won't run anymore because Kibana thinks it has already done this

It looks like we should do your migration in 7.7.0 instead to make sure it runs even if the user had a "broken" saved search in between.

And if we do this, we should probably also consider the following case (not as relevant, because pretty edge-casey):

  • User creates a saved search in 6.6
  • User upgrades to 7.4 and visits the saved search
  • Now a on-the-fly migration will kick in, changing query: { match_all: {} } to query: { query: { match_all: {} }, language: 'lucene' } (that's the migrateLegacyQuery function)
  • User saves the saved search
  • User upgrades to 7.7
  • This migration will run, but the migration removing match_all won't work because it's only looking for the legacy structure.

In the latter case it might be even fine to keep it because the users saw the strange query and still saved. But the first case should definitely be handled correctly.

@alexwizp
Copy link
Contributor Author

alexwizp commented Apr 6, 2020

@flash1293 This pull request is a revert of one our old issue that was relevant when user migrated from version 5 -> 6. Honestly not sure should we support that cases or not but I decided to add this script to the very youngest version in our migration scripts.

For me it looks like if user decided to update Kibana from version 5 -> 7.7 this script will be executed for all cases.
From the other hand if visualization will be created in version 6 it will be created without query: { match_all: {} } . It is also correct

@alexwizp alexwizp requested a review from flash1293 April 6, 2020 13:59
@kibanamachine
Copy link
Contributor

💚 Build Succeeded

History

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

flash1293
flash1293 approved these changes Apr 6, 2020
View reviewed changes
Copy link
Contributor

@flash1293 flash1293 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Discussed offline with @alexwizp, tested and works as intended. LGTM 👍

@alexwizp alexwizp merged commit fa661c0 into elastic:master Apr 6, 2020
@alexwizp alexwizp mentioned this pull request Apr 6, 2020
Merged
alexwizp added a commit to alexwizp/kibana that referenced this pull request Apr 6, 2020
@alexwizp @elasticmachine
match_all query disappears when typed into Lucene query bar (elastic#…
b26652d 
…62194)

* match_all query disappears when typed into Lucene query bar

Closes: elastic#52115

* add migrations for searh savedobject type

Co-authored-by: Elastic Machine <[email protected]>
alexwizp added a commit that referenced this pull request Apr 6, 2020
@alexwizp @elasticmachine
match_all query disappears when typed into Lucene query bar (#62194) (#…
42083f6 
…62626)

* match_all query disappears when typed into Lucene query bar

Closes: #52115

* add migrations for searh savedobject type

Co-authored-by: Elastic Machine <[email protected]>

Co-authored-by: Elastic Machine <[email protected]>
@lukeelmers lukeelmers mentioned this pull request Jun 3, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@VladLasitsa VladLasitsa VladLasitsa approved these changes

@ppisljar ppisljar ppisljar approved these changes

@flash1293 flash1293 flash1293 approved these changes

@lukasolson lukasolson Awaiting requested review from lukasolson

Assignees

@alexwizp alexwizp

Labels
Feature:KQL KQL release_note:fix review v7.8.0 v8.0.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

match_all query disappears when typed into Lucene query bar
6 participants
@alexwizp @elasticmachine @kibanamachine @flash1293 @ppisljar @VladLasitsa