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

Fix issue with overwritten filters #17713

Merged
merged 3 commits into from
Apr 17, 2018

Conversation

timroes
Copy link
Contributor

@timroes timroes commented Apr 16, 2018

Fixes #17644

This fixes overwriting filters on the search source in courier request handler, and thus losing all filters saved with the visualization.

After talking to @spalger we decided, that we don't start passing filters/queries explicitly for now in this place, without having a clear idea about how the removal of inheritance in search source would work in all of Kibana. Search source is currently built around inheritance, and breaking it in just one tiny part (the courier request handler) we cannot assure, that anything would still work properly.

So instead we are doing the very "easy" solution, by just inheriting another time per request from the searchSource and apply the timeRange filter to that search source. We still need two more modifications for this to work:

  • We need to mirror the history of the inherited search source with the original one, so that spy panel will still work, since spy panel will read out of searchSource, but the request will actually be written into requestSearchSource's history. This works by mirroring both. @spalger in contrast to our discussions on Friday, I haven't put this in a general way into the inherit method, since this a) would have cluttered the SearchSource class even more for just this one small usage of it, that b) can be removed as soon as the inspector (Replace spy panels by Inspector #16387) is implemented, since it's currently the only feature using history at all. So I rather decided to leave it in the courier request handler, and thus this simple defineProperty works, without needing to take care of overwriting the property if inheritance should change, etc.
  • We need to call the parents search source's onRequestStart handlers, since these will be used to properly trigger preflight requests (e.g. determine the interval for date histograms with auto interval). If we don't trigger the searchSource handlers only the requestSearchSource handlers, the preflight requests attached to vis won't be triggered properly. This needs to be configurable by a flag, since we would otherwise trigger parent handlers also for the postflight requests, since they inherit from requestSearchSource, but we don't want one preflight request trigger per postflight request.

cc @stacey-gammon (for information about the postpone of explicit filter/query passing - also feel free to review, if you have the time :))

@timroes timroes added bug Fixes for quality problems that affect the customer experience Feature:Visualizations Generic visualization features (in case no more specific feature label is available) v7.0.0 v6.3.0 labels Apr 16, 2018
@elasticmachine
Copy link
Contributor

💔 Build Failed

@timroes timroes force-pushed the requesthandler-refactoring branch from 7c7d487 to fd86ca1 Compare April 16, 2018 08:55
@timroes timroes requested review from ppisljar and spalger April 16, 2018 09:07
@elasticmachine
Copy link
Contributor

💔 Build Failed

@timroes
Copy link
Contributor Author

timroes commented Apr 16, 2018

Jenkins, test this - the saved search was missing in the test case?

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

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.

LGTM

Copy link
Contributor

@spalger spalger left a comment

Choose a reason for hiding this comment

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

LGTM

@nyurik
Copy link
Contributor

nyurik commented Apr 17, 2018

Seems like we need to add some tests to both timelion & vega to check for this.

@timroes
Copy link
Contributor Author

timroes commented Apr 17, 2018

Since timelion and Vega doesn't support this at all, there is at the moment no need to add tests for it :) (see #17719)

@timroes timroes merged commit 2d46bbe into elastic:master Apr 17, 2018
@timroes timroes deleted the requesthandler-refactoring branch April 17, 2018 06:20
@timroes timroes mentioned this pull request Apr 17, 2018
timroes added a commit to timroes/kibana that referenced this pull request Apr 17, 2018
* Fix issue with overwritten filters

* Add tests

* Only add timerange filter predicate if index exists
timroes added a commit that referenced this pull request Apr 17, 2018
* Fix issue with overwritten filters

* Add tests

* Only add timerange filter predicate if index exists
@stacey-gammon
Copy link
Contributor

@nyurik - @timroes is right but I have a test that will cover this case here: #17688 and I am also working on adding more timelion/vega tests here: #17703 (that last one is still in progress).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Fixes for quality problems that affect the customer experience Feature:Visualizations Generic visualization features (in case no more specific feature label is available) v6.3.0 v7.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Filters applied to visualizations are lost when viewing on a dashboard
6 participants