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

[GS] add search syntax support #83422

Merged
merged 30 commits into from
Nov 24, 2020

Conversation

pgayvallet
Copy link
Contributor

@pgayvallet pgayvallet commented Nov 16, 2020

Summary

Part of #74290
Fix #81397

What this PR does:

  • Change the globalSearch find API to now accept type and tag filter in addition to the search term
  • Add EUI's Query based search syntax support to the navigational search bar
  • Adapt the existing providers
  • Show icons for the saved objects results (as registered by the types)
  • Fix existing FTR test and add new one for this new feature

Screenshots

Screenshot 2020-11-18 at 22 55 23

Screenshot 2020-11-18 at 22 55 08

Screenshot 2020-11-18 at 22 54 21

Screenshot 2020-11-18 at 22 54 15

Checklist

Release note

Added advanced search syntax to the navigational search bar. It is now possible to filter results by type or by tag using the type:dashboard my term or tag:tag-name my term

@ryankeairns
Copy link
Contributor

I'm wondering if this a me thing (working from a loaner laptop and also had an error running Josh's sublinks PR but they look legit?), but I hit an error when bootstrapping this PR:

ERROR Error: Command failed with exit code 2: /Users/elastic/projects/kibana/node_modules/typescript/bin/tsc -b x-pack/tsconfig.refs.json --pretty
            x-pack/plugins/global_search/common/search_syntax/query_utils.test.ts:97:7 - error TS2345: Argument of type 'Map<string, string[]>' is not assignable to parameter of type 'Record<string, string[]>'.
              Index signature is missing in type 'Map<string, string[]>'.

            97       getAliasMap({})
                     ~~~~~~~~~~~~~~~

            x-pack/plugins/global_search/common/search_syntax/query_utils.test.ts:112:7 - error TS2345: Argument of type 'Map<string, string[]>' is not assignable to parameter of type 'Record<string, string[]>'.

            112       getAliasMap({
                      ~~~~~~~~~~~~~
            113         tag: ['tags'],
                ~~~~~~~~~~~~~~~~~~~~~~
            114       })
                ~~~~~~~~


            Found 2 errors.

@pgayvallet
Copy link
Contributor Author

@ryankeairns it's not you, the PR is not on a functioning state right now. Should be in a couple of days (only opened it for CI support, shouldn't have linked it to the issue so soon)

@pgayvallet pgayvallet requested a review from a team November 17, 2020 09:10
@pgayvallet
Copy link
Contributor Author

pgayvallet commented Nov 17, 2020

@ryankeairns @myasonik I gonna need your help here.

so, I added the syntax support and wired the type filter to our providers.

Then I tried to understand why queries such as type:application were not returning any results in the searchbar (when results are correctly returned by the GS API)... After a quick investigation, it seems that the EuiSelectableTemplateSitewide component we are using is actually performing some filtering on its own, based on the term currently typed in the searchbar.

When we were only searching by name/title, this was effectively working, as all results returned from GS did contain the search term either in their label or in their meta (see

const resultToOption = (result: GlobalSearchResult): EuiSelectableTemplateSitewideOption => {
)

However, with the advanced search syntax, this is no longer the case, as a term such as type:application is not present anywhere in the returned result's data, causing the component to exclude all the results provided by the GS API.

This is where I need you. We need to either find a way to disable the EuiSelectableTemplateSitewide internal filtering to let him just display all the results we are providing him with (which would make a lot of sense, as the filtering is already handled upstream by the GS API itself, this component should not perform any filtering logic), or, if disabling the filtering is not possible, use another component instead (or find any other workaround)

@pgayvallet pgayvallet added Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc v7.11.0 v8.0.0 labels Nov 17, 2020
@ryankeairns
Copy link
Contributor

Thanks for all your hard work here, Pierre!

@KOTungseth at this stage, it would likely be helpful to have a small section of the 7.11 Kibana docs devoted to using navigational search or add to what may already exist.

In particular, mentioning examples of searchable content and how this new search syntax can filter said content.

I'm happy to explain further and help write that up with your guidance.

Thanks!

Copy link
Contributor

@ryankeairns ryankeairns left a comment

Choose a reason for hiding this comment

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

In discussing what to do with tags and spaces in the rendered results UI, we came up with several ideas (more on that later) and would like to keep the left hand icon space open for the time being.

Specifically, we want to keep the solution/application icons, but use the empty icon for all other result types. The end result looking like this:

Screen Shot 2020-11-20 at 12 51 37 PM

@KOTungseth
Copy link
Contributor

Thanks for all your hard work here, Pierre!

@KOTungseth at this stage, it would likely be helpful to have a small section of the 7.11 Kibana docs devoted to using navigational search or add to what may already exist.

In particular, mentioning examples of searchable content and how this new search syntax can filter said content.

I'm happy to explain further and help write that up with your guidance.

Thanks!

I think this is a great idea! I've handed the Core UI docs over to @gchaps, so she'll be handling these docs from here on out.

Copy link
Contributor

@joshdover joshdover left a comment

Choose a reason for hiding this comment

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

Code & test coverage looks pretty good. Found one bug and a UX question.

One bug I've found:
If I tag a dashboard with a tag called true and then type tag:true into the search bar, no results appear. Same with false. Same with a tag name that is a number, such as 42. Probably worth a couple of unit tests to be sure we don't break this in the future. Note that this does work fine on the Tags UI filter bar in Stack Management.

/**
* @public
*/
export type GlobalSearchProviderFindParams = GlobalSearchFindParams;
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the purpose of this type? Do we expect this type to diverge in the future?

Copy link
Contributor Author

@pgayvallet pgayvallet Nov 21, 2020

Choose a reason for hiding this comment

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

It may, depending on how we decide to internally process the parameters from the service before forwarding them to providers. Also atm the service and provider types are all separated, so it was mostly for consistency.

export const getFieldValueMap = (query: Query) => {
const fieldMap = new Map<string, FilterValues>();

query.ast.clauses.forEach((clause) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Am I right in understanding that this only supports OR logic? Will we need to support AND or more complex logic in the future?

From a UX perspective, I actually find OR less useful than AND in this use case, however, I understand this emulates the Lucene query string behavior. I suspect that it's more common for users to want to use search filters to narrow down results to find a specific object. There is the use case of "I know it had either this tag or this one" but that seems less useful/common than the narrowing case. In general, I suspect users are trying to find a needle in a haystack. That said, I have 0 data to back this up 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Am I right in understanding that this only supports OR logic

That's right

Will we need to support AND or more complex logic in the future?

We might, but that would require changes in both the syntax we use (EUI Query syntax does not support AND for field terms, we will probably need to use/create our own syntax in that case), and in the SO find API. Also for some filters it doesn't make sense: type:(dashboard AND map).

See #74290 (comment) and the reply from alex for more context, but basically, we are not planning this short or mid term.

@pgayvallet
Copy link
Contributor Author

@joshdover

If I tag a dashboard with a tag called true and then type tag:true into the search bar, no results appear. Same with false. Same with a tag name that is a number, such as 42

Good catch. I was aware Query was parsing number, boolean and dates and I choose to not coerce them to string because we might have wanted to have non-string filters in the future. I guess I will just KISS for now and convert all terms to string.

@pgayvallet
Copy link
Contributor Author

pgayvallet commented Nov 22, 2020

@ryankeairns

Specifically, we want to keep the solution/application icons, but use the empty icon for all other result types

And, it's gone! 9a21e53

@joshdover

If I tag a dashboard with a tag called true and then type tag:true into the search bar, no results appear. Same with false. Same with a tag name that is a number, such as 42

Done in 829c8f0. I chose to handle this at the higher level (parseSearchParams) to be more future proof in case of potential non-string filter that we may add later.

@ryankeairns ryankeairns requested a review from a team November 23, 2020 14:32
Copy link
Contributor

@ryankeairns ryankeairns left a comment

Choose a reason for hiding this comment

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

Thanks for the icon change!

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
globalSearchBar 21 25 +4

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
lens 935.0KB 935.0KB +1.0B

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id before after diff
globalSearch 16.8KB 17.0KB +288.0B
globalSearchBar 28.5KB 33.2KB +4.6KB
globalSearchProviders 6.6KB 6.8KB +170.0B
lens 52.4KB 52.6KB +173.0B
savedObjectsTagging 44.7KB 44.7KB +71.0B
total +5.3KB

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

@pgayvallet pgayvallet merged commit 7d5fb8e into elastic:master Nov 24, 2020
pgayvallet added a commit to pgayvallet/kibana that referenced this pull request Nov 24, 2020
* add search syntax parsing logic

* fix ts types

* use type filter in providers

* move search syntax logic to the searchbar

* fix test plugin types

* fix test plugin types again

* use `onSearch` prop to disable internal component search

* add tag filter support

* add FTR tests

* move away from CI group 7

* fix unit tests

* add unit tests

* remove the API test suite

* Add icons to the SO results

* add test for unknown type / tag

* nits

* ignore case for the `type` filter

* Add syntax help text

* remove unused import

* hide icon for non-application results

* add tsdoc on query utils

* coerce known filter values to string

Co-authored-by: Ryan Keairns <[email protected]>
pgayvallet added a commit that referenced this pull request Nov 24, 2020
* [GS] add search syntax support (#83422)

* add search syntax parsing logic

* fix ts types

* use type filter in providers

* move search syntax logic to the searchbar

* fix test plugin types

* fix test plugin types again

* use `onSearch` prop to disable internal component search

* add tag filter support

* add FTR tests

* move away from CI group 7

* fix unit tests

* add unit tests

* remove the API test suite

* Add icons to the SO results

* add test for unknown type / tag

* nits

* ignore case for the `type` filter

* Add syntax help text

* remove unused import

* hide icon for non-application results

* add tsdoc on query utils

* coerce known filter values to string

Co-authored-by: Ryan Keairns <[email protected]>

* fix mappings for 7.x

Co-authored-by: Ryan Keairns <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc v7.11.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fix the GlobalSearchBar FTR test
9 participants