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

Guard against empty and undefined index pattern arrays passed to QueryBar #24607

Merged
merged 2 commits into from
Nov 30, 2018

Conversation

Bargs
Copy link
Contributor

@Bargs Bargs commented Oct 25, 2018

Fixes #24572

The angular implementation of the QueryBar would use the default index pattern if the consumer of the component didn't pass any in. AFAIK we don't have an existing non-angular service for grabbing the default index pattern. I looked into creating one briefly, but as I was thinking about it I decided this probably wasn't the appropriate solution anyway. An index pattern is required for autocomplete suggestions (other than recent searches). If no pattern is passed in, we simply don't provide those suggestions. This seems like the simplest and most logical solution to me.

@Bargs Bargs added review v7.0.0 v6.5.0 Team:Visualizations Visualization editors, elastic-charts and infrastructure labels Oct 25, 2018
@Bargs Bargs added the v6.6.0 label Oct 25, 2018
@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@lukasolson
Copy link
Member

Hmm, so I've been thinking about this and I'm thinking we're going to need to eventually build a service anyway for getting the default index pattern, aren't we? Otherwise how will we choose which index pattern loads up by default in certain places? I'm open to discussion about whether or not we want to have a default altogether.

I'm also wondering if this will also affect the rewrite of the filter bar. If you remember we had quite a few issues come up because we weren't using a default index pattern when one wasn't provided to the directive. I'm afraid these same issues might come up if we move forward this way.

@lukasolson lukasolson closed this Oct 30, 2018
@lukasolson
Copy link
Member

Closed by accident.

@lukasolson lukasolson reopened this Oct 30, 2018
@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@Bargs
Copy link
Contributor Author

Bargs commented Oct 31, 2018

@lukasolson offering suggestions for the default just doesn't make much sense to me, it seems worse than offering no suggestions at all. Let's say you're on a dashboard that only has TSVB visualizations targeting a non-default index pattern. You're going to get a bunch of non-sensical and confusing suggestions. I didn't get a chance to review #16235, otherwise I would have offered the same feedback there.

I'm also wondering if this will also affect the rewrite of the filter bar. If you remember we had quite a few issues come up because we weren't using a default index pattern when one wasn't provided to the directive. I'm afraid these same issues might come up if we move forward this way.

Couldn't we take the same approach there that I've taken here? If there is no index pattern, provide no suggestions. That would be my preference for the same reasons as above.

@lukasolson
Copy link
Member

Couldn't we take the same approach there that I've taken here? If there is no index pattern, provide no suggestions. That would be my preference for the same reasons as above.

The problem here is that TSVB/Timelion do support filters, and users had complained in the past when the filter bar wouldn't show up when just a TSVB or Timelion vis was on a dashboard and the filter bar didn't show up.

@Bargs
Copy link
Contributor Author

Bargs commented Oct 31, 2018

users had complained in the past when the filter bar wouldn't show up when just a TSVB or Timelion vis was on a dashboard and the filter bar didn't show up.

What I'm saying is the filter bar would show up, as would the add filter button, but we just wouldn't show any suggestions in the add filter form.

Copy link
Contributor

@timroes timroes left a comment

Choose a reason for hiding this comment

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

Tested in Chrome (Linux) fixes the issue. Code LGTM

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@Bargs
Copy link
Contributor Author

Bargs commented Nov 27, 2018

Rebased this for another fresh CI run since it's been awhile. @lukasolson how are you feeling about this PR?

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

Copy link
Member

@lukasolson lukasolson left a comment

Choose a reason for hiding this comment

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

Changes LGTM and checked it out and works fine. We'll have to make sure to update the filter bar to have the same behavior, because it seems odd that they work differently as of this PR.

@LeeDr LeeDr added v6.5.3 and removed v6.5.2 labels Nov 29, 2018
@LeeDr
Copy link
Contributor

LeeDr commented Nov 29, 2018

This isn't making it into v6.5.2 (unless it's a really serious blocker). I bumped it to v6.5.3, but I'm not sure if really should be backported to 6.5 branch at all.

@Bargs
Copy link
Contributor Author

Bargs commented Nov 30, 2018

Unless @timroes disagrees I don't think it needs to go into 6.5.3 either. It only has that label because it was originally slated for 6.5.0 and people kept bumping the label one patch version after each release.

@Bargs Bargs merged commit 6e42dca into elastic:master Nov 30, 2018
Bargs added a commit to Bargs/kibana that referenced this pull request Nov 30, 2018
…yBar (elastic#24607)

* guard against empty and undefined index pattern arrays

* fix merge issues
Bargs added a commit that referenced this pull request Nov 30, 2018
…yBar (#24607) (#26487)

* guard against empty and undefined index pattern arrays

* fix merge issues
@rayafratkina
Copy link
Contributor

Removed 6.5.3 label per discussion above

@markov00
Copy link
Member

markov00 commented Feb 5, 2019

@Bargs as we are adding KQL support to TSVB on 6.7 can you backport this PR?

@Bargs
Copy link
Contributor Author

Bargs commented Feb 5, 2019

@markov00 it should already be in 6.7 since I originally back ported this to 6.6 #26487

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

Successfully merging this pull request may close these issues.

8 participants