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

Add Checks to Dashboard Context #13182

Merged
merged 4 commits into from
Jul 28, 2017
Merged

Conversation

simianhacker
Copy link
Member

This PR fixes #13181 by adding checks to the values before adding queries to the dashboard context filter. The bug was originally introduced in #12624.

This also fixes elastic/beats#4777

@simianhacker simianhacker added blocker v6.0.0 v6.0.0-beta1 Feature:Visualizations Generic visualization features (in case no more specific feature label is available) labels Jul 28, 2017
@epixa
Copy link
Contributor

epixa commented Jul 28, 2017

@simianhacker Can you add a unit test to this to ensure it doesn't regress in the future? This bug is pretty subtle and has a huge impact

@epixa
Copy link
Contributor

epixa commented Jul 28, 2017

I've confirmed this does fix the beats dashboard issue

@Bargs
Copy link
Contributor

Bargs commented Jul 28, 2017

Adding these checks definitely makes dashboard_context more robust and fixes these particular symptoms, but the root issue here is that the query property in the new-style query object is undefined in the first place. It should never be undefined.

This line seems to be the root of all our trouble. It wraps the query even if the query does not exist (as would be the case with old dashboards, Timelion saved visualizations, etc). If we just wrap that line with a conditional, all the errors go away:

if (!_.isUndefined(this.searchSource.getOwn('query'))) {
  this.searchSource.set('query', migrateLegacyQuery(this.searchSource.getOwn('query')));
}

@simianhacker
Copy link
Member Author

Thanks @Bargs, I added that fix as well.

Copy link
Contributor

@Bargs Bargs left a comment

Choose a reason for hiding this comment

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

LGTM

@simianhacker simianhacker merged commit b678b4b into elastic:master Jul 28, 2017
simianhacker added a commit that referenced this pull request Jul 28, 2017
* Fixes #13181 - Check values before adding them to filter

* Adding tests

* Adding check to make sure query is never undefined
simianhacker added a commit that referenced this pull request Jul 28, 2017
* Fixes #13181 - Check values before adding them to filter

* Adding tests

* Adding check to make sure query is never undefined
@simianhacker
Copy link
Member Author

Back ported to 6.0 with 9ee0ca8
Back ported to 6.x with 31b3ec2

@simianhacker simianhacker deleted the fix-13181 branch April 17, 2024 14:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
blocker Feature:Visualizations Generic visualization features (in case no more specific feature label is available) review v6.0.0-beta1 v6.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Dashboard context returns invalid filter Broken Metricbeat dashboards
4 participants