Fix invisible filters caused by missing index pattern #14131

Bargs · 2017-09-22T17:28:05Z

"invisible filters" occur when the mapping chain throws an error. If a single filter throws an error, the entire chain rejects. As a result, not even the valid filters appear in the filter bar because they never get added to the scope. However the filters still exist in app state and still get sent with each search request.

The most common error occurs when the filter's meta.index property points to a non-existing index pattern. Since this property is only used for looking up field formatters and it is not essential for a working filter, we now fall back on raw values instead of failing if the index pattern is not found. See the PR this one replaces for discussion about other solutions we tried and why we chose to go this route.

Bargs · 2017-09-22T17:33:45Z

src/ui/public/filter_bar/lib/map_script.js

-          value = filter.meta.formattedValue;
-        } else {
-          value = filter.script.script.params.value;
-          value = field.format.convert(value);


This else was pointless because map_phrase handles script filters that contain a params.value property (and appears before map_script in the mapping chain). I'm actually wondering if this map_script.js file is ever used. I dunno where meta.formattedValue would come from. Maybe @lukasolson has an idea?

Hmm, I think honestly when I added filter editors I updated the mapRange and mapPhrase functions to properly map scripted fields, so I think you're probably right... This might not ever get called.

@spalger it looks like the formattedValue stuff was added in this PR. Any insights here? Not sure if it's safe to delete or if mapRange and mapPhrase should also be checking for a meta.formattedValue prop?

Sorry, I really have no idea 😬

Bargs · 2017-09-22T17:34:39Z

@stacey-gammon I thought you might be interested in reviewing since you filed an issue about this at one point.

stacey-gammon

Couple minor comments that I'll leave up to you, but lgtm.

stacey-gammon · 2017-09-22T18:03:12Z

src/ui/public/filter_bar/lib/map_geo_bounding_box.js

+        : JSON.stringify(params.top_left);
+        const bottomRight = indexPattern
+         ? indexPattern.fields.byName[key].format.convert(params.bottom_right)
+         : JSON.stringify(params.bottom_right);


Should these be indented by two?

const bottomRight = indexPattern ? indexPattern.fields.byName[key].format.convert(params.bottom_right) : JSON.stringify(params.bottom_right);

Same below, I think the .thens are supposed to be indented by two because it's a continuation of the original line.

Fixed the ternary expressions, but the thens are a separate matter. This style guide rule was changed at some point but the code base was never reformatted en masse. I'm following the existing style in these files in order to avoid having inconsistent styles within a single module.

stacey-gammon · 2017-09-22T18:10:33Z

src/ui/public/filter_bar/lib/map_geo_bounding_box.js

        const params = filter.geo_bounding_box[key];
-        const topLeft = field.format.convert(params.top_left);
-        const bottomRight = field.format.convert(params.bottom_right);
+        const topLeft = indexPattern


I think a comment would be helpful:
indexPatterns are only used to find the field formatter for this field. An index pattern might not exist in some filters (... do you know how this can happen?), in which case we just display the raw value.

Up to you, but I found this code easy to read only after I read your comments on the PR, and a developer reading the code won't have that benefit.

Or maybe a comment for this getParams function at the top that says indexPattern is an optional parameter and used to format the field if given.

lukasolson

Added a couple of comments inline.

lukasolson · 2017-09-26T17:15:23Z

src/ui/public/filter_bar/lib/map_script.js

-          value = filter.meta.formattedValue;
-        } else {
-          value = filter.script.script.params.value;
-          value = field.format.convert(value);


Hmm, I think honestly when I added filter editors I updated the mapRange and mapPhrase functions to properly map scripted fields, so I think you're probably right... This might not ever get called.

lukasolson · 2017-09-26T17:23:58Z

src/ui/public/filter_bar/lib/map_geo_bounding_box.js

+        if (error instanceof SavedObjectNotFound) {
+          return getParams();
+        }
+        throw error;


What do we gain by throwing the error here?

I want the promise to reject in order to preserve the old behavior for errors that are not of type SavedObjectNotFound.

Bargs · 2017-09-27T20:46:35Z

@lukasolson updated and ready for another look. I removed map_script since I couldn't see a path through which it would ever be reached.

Bargs · 2017-09-29T17:51:39Z

jenkins, test this

Bargs · 2017-10-02T18:19:12Z

These tests ran fine locally, let's see if a rebase fixes it in CI.

lukasolson

LGTM

"invisible filters" occur when the mapping chain throws an error. If a single filter throws an error, the entire chain rejects. As a result, not even the valid filters appear in the filter bar because they never get added to the scope. However the filters still exist in app state and still get sent with each search request. The most common error occurs when the filter's meta.index property points to a non-existing index pattern. Since this property is only used for looking up field formatters and it is not essential for a working filter, we now fall back on raw values instead of failing if the index pattern is not found. See the PR this one replaces for discussion about other solutions we tried and why we chose to go this route.

…c#14131)" This reverts commit e09d7ad.

* Adds ability to change index pattern on import Signed-off-by: Tyler Smalley <tyler.smalley@elastic.co> * UI changes. Use a table in the modal grouped by index pattern id instead of multiple modals. * PR feedback * PR feedback * PR updates * Handle skip properly * Fix error when there were no existing index patterns * Tests for the new import conflict logic * Fix invisible filters caused by missing index pattern (#14131) "invisible filters" occur when the mapping chain throws an error. If a single filter throws an error, the entire chain rejects. As a result, not even the valid filters appear in the filter bar because they never get added to the scope. However the filters still exist in app state and still get sent with each search request. The most common error occurs when the filter's meta.index property points to a non-existing index pattern. Since this property is only used for looking up field formatters and it is not essential for a working filter, we now fall back on raw values instead of failing if the index pattern is not found. See the PR this one replaces for discussion about other solutions we tried and why we chose to go this route. * Show query and filter bars even when there's a linked search (#14212) The query bar used to be hidden in the presence of a linked search because unlike filters, queries didn't get merged when flattening a SearchSource hierarchy. That means a query in the query bar would override the query in the linked search. This is no longer the case. As of 6.0 we include all queries in the SearchSource hierarchy in the final request, so there's no longer any reason to hide the query bar. Since filters created via a Vis show up in the query bar when Kuery is selected, these filters now appear correctly even when there's a linked search in the vis editor. Previously when unlinking a saved search visualize would insert the query and filters from the saved search into app state before removing the SearchSource from the hierarcy. This posed a problem because combining two lucene query strings isn't as easy as combing two sets of filters. We decided this behavior was a bit counterintuitive anyway. If the user wants to unlink the saved search, they probably want to discard it, not combine it with their local changes. So I've also updated the unlinking functionality to discard the saved search. * limit wait time for baselayer (#14047) * adding scope appy back (#14269) * remove junk tests (#14191) * We are using the index pattern id now * Use the index pattern id here too * Use an isolated es env for these tests * Revert "Fix invisible filters caused by missing index pattern (#14131)" This reverts commit e09d7ad. * Revert "Show query and filter bars even when there's a linked search (#14212)" This reverts commit 3aee7c2. * Revert "limit wait time for baselayer (#14047)" This reverts commit 44a7107. * Revert "adding scope appy back (#14269)" This reverts commit 51b6b51. * Revert "remove junk tests (#14191)" This reverts commit f06c183. * Revert these

* Adds ability to change index pattern on import Signed-off-by: Tyler Smalley <tyler.smalley@elastic.co> * UI changes. Use a table in the modal grouped by index pattern id instead of multiple modals. * PR feedback * PR feedback * PR updates * Handle skip properly * Fix error when there were no existing index patterns * Tests for the new import conflict logic * Fix invisible filters caused by missing index pattern (elastic#14131) "invisible filters" occur when the mapping chain throws an error. If a single filter throws an error, the entire chain rejects. As a result, not even the valid filters appear in the filter bar because they never get added to the scope. However the filters still exist in app state and still get sent with each search request. The most common error occurs when the filter's meta.index property points to a non-existing index pattern. Since this property is only used for looking up field formatters and it is not essential for a working filter, we now fall back on raw values instead of failing if the index pattern is not found. See the PR this one replaces for discussion about other solutions we tried and why we chose to go this route. * Show query and filter bars even when there's a linked search (elastic#14212) The query bar used to be hidden in the presence of a linked search because unlike filters, queries didn't get merged when flattening a SearchSource hierarchy. That means a query in the query bar would override the query in the linked search. This is no longer the case. As of 6.0 we include all queries in the SearchSource hierarchy in the final request, so there's no longer any reason to hide the query bar. Since filters created via a Vis show up in the query bar when Kuery is selected, these filters now appear correctly even when there's a linked search in the vis editor. Previously when unlinking a saved search visualize would insert the query and filters from the saved search into app state before removing the SearchSource from the hierarcy. This posed a problem because combining two lucene query strings isn't as easy as combing two sets of filters. We decided this behavior was a bit counterintuitive anyway. If the user wants to unlink the saved search, they probably want to discard it, not combine it with their local changes. So I've also updated the unlinking functionality to discard the saved search. * limit wait time for baselayer (elastic#14047) * adding scope appy back (elastic#14269) * remove junk tests (elastic#14191) * We are using the index pattern id now * Use the index pattern id here too * Use an isolated es env for these tests * Revert "Fix invisible filters caused by missing index pattern (elastic#14131)" This reverts commit e09d7ad. * Revert "Show query and filter bars even when there's a linked search (elastic#14212)" This reverts commit 3aee7c2. * Revert "limit wait time for baselayer (elastic#14047)" This reverts commit 44a7107. * Revert "adding scope appy back (elastic#14269)" This reverts commit 51b6b51. * Revert "remove junk tests (elastic#14191)" This reverts commit f06c183. * Revert these

optical · 2017-11-12T19:41:05Z

I may be misunderstanding the labels on this ticket, but I assume this fix only applies to Kibana 6.0? Got stung by #7767 while running Kibana 5.5. Just wondering if this is fixed in the lastest 5.X release, or if it will ever be?

Bargs · 2017-11-13T22:39:40Z

@optical you're correct, this fix is only in 6.0+. It's unlikely we'll backport to 5.x. With 6.0 incoming we're only backporting fixes for high severity bugs at this point, to keep 5.x as stable as possible.

"invisible filters" occur when the mapping chain throws an error. If a single filter throws an error, the entire chain rejects. As a result, not even the valid filters appear in the filter bar because they never get added to the scope. However the filters still exist in app state and still get sent with each search request. The most common error occurs when the filter's meta.index property points to a non-existing index pattern. Since this property is only used for looking up field formatters and it is not essential for a working filter, we now fall back on raw values instead of failing if the index pattern is not found. See the PR this one replaces for discussion about other solutions we tried and why we chose to go this route.

* Adds ability to change index pattern on import Signed-off-by: Tyler Smalley <tyler.smalley@elastic.co> * UI changes. Use a table in the modal grouped by index pattern id instead of multiple modals. * PR feedback * PR feedback * PR updates * Handle skip properly * Fix error when there were no existing index patterns * Tests for the new import conflict logic * Fix invisible filters caused by missing index pattern (elastic#14131) "invisible filters" occur when the mapping chain throws an error. If a single filter throws an error, the entire chain rejects. As a result, not even the valid filters appear in the filter bar because they never get added to the scope. However the filters still exist in app state and still get sent with each search request. The most common error occurs when the filter's meta.index property points to a non-existing index pattern. Since this property is only used for looking up field formatters and it is not essential for a working filter, we now fall back on raw values instead of failing if the index pattern is not found. See the PR this one replaces for discussion about other solutions we tried and why we chose to go this route. * Show query and filter bars even when there's a linked search (elastic#14212) The query bar used to be hidden in the presence of a linked search because unlike filters, queries didn't get merged when flattening a SearchSource hierarchy. That means a query in the query bar would override the query in the linked search. This is no longer the case. As of 6.0 we include all queries in the SearchSource hierarchy in the final request, so there's no longer any reason to hide the query bar. Since filters created via a Vis show up in the query bar when Kuery is selected, these filters now appear correctly even when there's a linked search in the vis editor. Previously when unlinking a saved search visualize would insert the query and filters from the saved search into app state before removing the SearchSource from the hierarcy. This posed a problem because combining two lucene query strings isn't as easy as combing two sets of filters. We decided this behavior was a bit counterintuitive anyway. If the user wants to unlink the saved search, they probably want to discard it, not combine it with their local changes. So I've also updated the unlinking functionality to discard the saved search. * limit wait time for baselayer (elastic#14047) * adding scope appy back (elastic#14269) * remove junk tests (elastic#14191) * We are using the index pattern id now * Use the index pattern id here too * Use an isolated es env for these tests * Revert "Fix invisible filters caused by missing index pattern (elastic#14131)" This reverts commit e09d7ad. * Revert "Show query and filter bars even when there's a linked search (elastic#14212)" This reverts commit 3aee7c2. * Revert "limit wait time for baselayer (elastic#14047)" This reverts commit 44a7107. * Revert "adding scope appy back (elastic#14269)" This reverts commit 51b6b51. * Revert "remove junk tests (elastic#14191)" This reverts commit f06c183. * Revert these

"invisible filters" occur when the mapping chain throws an error. If a single filter throws an error, the entire chain rejects. As a result, not even the valid filters appear in the filter bar because they never get added to the scope. However the filters still exist in app state and still get sent with each search request. The most common error occurs when the filter's meta.index property points to a non-existing index pattern. Since this property is only used for looking up field formatters and it is not essential for a working filter, we now fall back on raw values instead of failing if the index pattern is not found. See the PR this one replaces for discussion about other solutions we tried and why we chose to go this route.

* Adds ability to change index pattern on import Signed-off-by: Tyler Smalley <tyler.smalley@elastic.co> * UI changes. Use a table in the modal grouped by index pattern id instead of multiple modals. * PR feedback * PR feedback * PR updates * Handle skip properly * Fix error when there were no existing index patterns * Tests for the new import conflict logic * Fix invisible filters caused by missing index pattern (elastic#14131) "invisible filters" occur when the mapping chain throws an error. If a single filter throws an error, the entire chain rejects. As a result, not even the valid filters appear in the filter bar because they never get added to the scope. However the filters still exist in app state and still get sent with each search request. The most common error occurs when the filter's meta.index property points to a non-existing index pattern. Since this property is only used for looking up field formatters and it is not essential for a working filter, we now fall back on raw values instead of failing if the index pattern is not found. See the PR this one replaces for discussion about other solutions we tried and why we chose to go this route. * Show query and filter bars even when there's a linked search (elastic#14212) The query bar used to be hidden in the presence of a linked search because unlike filters, queries didn't get merged when flattening a SearchSource hierarchy. That means a query in the query bar would override the query in the linked search. This is no longer the case. As of 6.0 we include all queries in the SearchSource hierarchy in the final request, so there's no longer any reason to hide the query bar. Since filters created via a Vis show up in the query bar when Kuery is selected, these filters now appear correctly even when there's a linked search in the vis editor. Previously when unlinking a saved search visualize would insert the query and filters from the saved search into app state before removing the SearchSource from the hierarcy. This posed a problem because combining two lucene query strings isn't as easy as combing two sets of filters. We decided this behavior was a bit counterintuitive anyway. If the user wants to unlink the saved search, they probably want to discard it, not combine it with their local changes. So I've also updated the unlinking functionality to discard the saved search. * limit wait time for baselayer (elastic#14047) * adding scope appy back (elastic#14269) * remove junk tests (elastic#14191) * We are using the index pattern id now * Use the index pattern id here too * Use an isolated es env for these tests * Revert "Fix invisible filters caused by missing index pattern (elastic#14131)" This reverts commit e09d7ad. * Revert "Show query and filter bars even when there's a linked search (elastic#14212)" This reverts commit 3aee7c2. * Revert "limit wait time for baselayer (elastic#14047)" This reverts commit 44a7107. * Revert "adding scope appy back (elastic#14269)" This reverts commit 51b6b51. * Revert "remove junk tests (elastic#14191)" This reverts commit f06c183. * Revert these

Bargs added :Discovery review v6.0.0 v6.1.0 v7.0.0 labels Sep 22, 2017

Bargs commented Sep 22, 2017

View reviewed changes

Bargs requested review from lukasolson and stacey-gammon September 22, 2017 17:34

stacey-gammon approved these changes Sep 22, 2017

View reviewed changes

lukasolson reviewed Sep 26, 2017

View reviewed changes

epixa added v6.0.0-rc2 and removed v6.0.0-rc2 labels Sep 28, 2017

Bargs added 5 commits October 2, 2017 14:11

Fallback on raw values instead of failing if index pattern is not found

5ff56e2

Update test

1e41623

Add clarifying comment

15c7162

Fixed indentation

8c7b198

Remove map_script which no longer seems to be used

32f54ef

Bargs force-pushed the invisibleFiltersTake2 branch from 351fa03 to 32f54ef Compare October 2, 2017 18:18

lukasolson approved these changes Oct 3, 2017

View reviewed changes

Bargs merged commit 0fd8289 into elastic:master Oct 3, 2017

chrisronline added a commit to chrisronline/kibana that referenced this pull request Oct 4, 2017

Revert "Fix invisible filters caused by missing index pattern (elasti…

9c89215

…c#14131)" This reverts commit e09d7ad.

psanz-estc mentioned this pull request Feb 8, 2021

Filter is applied but invisible when dashboard is imported #90569

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix invisible filters caused by missing index pattern #14131

Fix invisible filters caused by missing index pattern #14131

Bargs commented Sep 22, 2017

Bargs Sep 22, 2017

lukasolson Sep 26, 2017

Bargs Sep 26, 2017

spalger Sep 27, 2017

Bargs commented Sep 22, 2017

stacey-gammon left a comment

stacey-gammon Sep 22, 2017

Bargs Sep 27, 2017

stacey-gammon Sep 22, 2017

lukasolson left a comment

lukasolson Sep 26, 2017

lukasolson Sep 26, 2017

Bargs Sep 26, 2017

Bargs commented Sep 27, 2017

Bargs commented Sep 29, 2017

Bargs commented Oct 2, 2017

lukasolson left a comment

optical commented Nov 12, 2017

Bargs commented Nov 13, 2017

Fix invisible filters caused by missing index pattern #14131

Fix invisible filters caused by missing index pattern #14131

Conversation

Bargs commented Sep 22, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Bargs commented Sep 22, 2017

stacey-gammon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lukasolson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Bargs commented Sep 27, 2017

Bargs commented Sep 29, 2017

Bargs commented Oct 2, 2017

lukasolson left a comment

Choose a reason for hiding this comment

optical commented Nov 12, 2017

Bargs commented Nov 13, 2017