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.

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

Fixes error when filters agg filters are a query_string query #43310

Merged
merged 2 commits into from
Aug 15, 2019

Conversation

Bargs
Copy link
Contributor

@Bargs Bargs commented Aug 14, 2019

Summary

Fixes #43188

In #37287 I updated the filters agg code to assume that filters are always in our new query format {query: string | object, language: string}. While this is true, I failed to account for the fact that when a lucene query is a query_string query, we transform the JSON into a simple string. This led to an issue where the value in the filter agg input (now a simple string) was out of sync with the label being sent to ES (stringified query_string JSON).

You could reproduce this in two situations:

  1. An old saved vis had a filter agg with a query_string query. We used to store the raw query DSL instead of the simple lucene query string in older versions of Kibana. To fix this scenario I added a migration for 7.3.1 that will transform query_string queries into simple lucene query strings in the filters agg. This way we can always expect filters aggs to have the same format no matter what version they're from.

  2. You literally type in a query_string query into the filters agg input. The query_bar_input component would transform this for you automatically into a simple lucene query string (which is what we want), but it failed to fire the onChange handler afterwards so the actual model wouldn't get updated. I fixed this scenario by adding a check in componentDidUpdate that tests whether our parsing/serializing rules would modify the new query and if so, calls the onChange.

Checklist

Use strikethroughs to remove checklist items you don't feel are applicable to this PR.

- [ ] This was checked for cross-browser compatibility, including a check against IE11
- [ ] 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

For maintainers

- [ ] This was checked for breaking API changes and was labeled appropriately
- [ ] This includes a feature addition or change that requires a release note and was labeled appropriately

@Bargs Bargs added release_note:fix Team:Visualizations Visualization editors, elastic-charts and infrastructure v8.0.0 v7.3.1 labels Aug 14, 2019
@Bargs Bargs requested a review from lukeelmers August 14, 2019 19:36
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-app

@@ -503,6 +540,7 @@ export const migrations = {
'7.0.1': removeDateHistogramTimeZones,
'7.2.0': doc => executeMigrations720(doc),
'7.3.0': executeMigrations730,
'7.3.1': executeVisualizationMigrations731,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This version number will need to be bumped if we miss 7.3.1

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

Copy link
Member

@lukeelmers lukeelmers left a comment

Choose a reason for hiding this comment

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

LGTM, confirmed this fixes #43188 (but not #43189)

Tested locally (Chrome OSX) and verified that the migration transforms the saved object as expected.

I pushed an update to this PR with a unit test for the migration.

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
blocker release_note:fix Team:Visualizations Visualization editors, elastic-charts and infrastructure v7.3.1 v7.4.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[esaggs] > Cannot read property 'doc_count' of undefined
3 participants