[GS] add search syntax support #83422

pgayvallet · 2020-11-16T13:39:54Z

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

Checklist

Documentation was added for features that require explanation or tutorials
Unit or functional tests were updated or added to match the most common scenarios

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 · 2020-11-16T16:24:57Z

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 · 2020-11-16T16:57:49Z

@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)

…ch-syntax

pgayvallet · 2020-11-17T09:10:27Z

@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

kibana/x-pack/plugins/global_search_bar/public/components/search_bar.tsx

Line 65 in a12bbdb

    
           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)

…ch-syntax

pgayvallet · 2020-11-18T17:56:18Z

x-pack/test/plugin_functional/test_suites/global_search/index.ts

-  // See https://github.com/elastic/kibana/issues/81397
-  describe.skip('GlobalSearch API', function () {
-    this.tags('ciGroup7');
-    loadTestFile(require.resolve('./global_search_api'));


This suite do not really make any sense now that we have real FTR tests against the searchbar.

Plus, it was broken on the server-side due to rxjs incompatibility issues between the test plugin and kibana's (similar to #61652)

Should we close #81397 then?

Yea, the issue is referenced and will be closed by this PR

pgayvallet · 2020-11-18T20:28:53Z

x-pack/plugins/global_search_bar/public/components/search_bar.tsx

        </EuiHeaderSectionItemButton>
      }
      searchProps={{
+        onSearch: () => undefined,


This is the required hack to disable the internal filtering of the component. Note that until elastic/eui#4277 is resolved, this disables the highlight of the term on the search results.

Maybe add a code comment TODO so that we don't forget about it and if we do, there's a chance someone will notice the TODO in the future...

pgayvallet · 2020-11-18T20:29:25Z

x-pack/plugins/global_search_bar/public/components/search_bar.tsx

      }}
      popoverProps={{
+        'data-test-subj': 'nav-search-popover',
+        panelClassName: 'navSearch__panel',


There is no panelDataTestSubject property, so I need to add a custom class for FTR tests.

Why do you need the popover panel specifically? It seems like you're mostly testing for existence of the popover, so shouldn't the popover data-test-subj give you the same?

Yea, that's misleading. the popover element is actually a wrapper around the input, not in the panel that opens on focus. (the 'data-test-subj': 'nav-search-popover', properties applies to the element that wraps the input, not the popover container that appears). From what I saw, there is noway to set a data-test-subj for any of the 'root' elements of the panel.

Created an issue in EUI: elastic/eui#4293

Maybe add a TODO to change this to a data-test-subj prop when that issue is closed?

pgayvallet · 2020-11-18T20:36:51Z

x-pack/plugins/global_search_bar/public/components/search_bar.tsx

+          ? rawParams.filters.tags.map(
+              (tagName) => taggingApi.ui.getTagIdFromName(tagName) ?? '__unknown__'
+            )


I started by filtering out the unknown tags, but the effect was that searching for a non existing tag was returning results (as the tag was excluded from the resulting search request. The simplest trick I found to address that was to add an invalid id instead.

pgayvallet · 2020-11-18T21:42:57Z

x-pack/test/functional/page_objects/navigational_search.ts

+      // results are emitted in multiple batches. Each individual batch causes a re-render of
+      // the component, causing the current elements to become stale. We can't perform DOM access
+      // without heavy flakiness in this situation.
+      // there is NO ui indication of any kind to detect when all the emissions are done,
+      // so we are forced to fallback to awaiting a given amount of time once the first options are displayed.
+      await delay(waitUntil);


Self explanatory comment. I can't rely on the UI at all to known when all the results are loaded: there are no indication that the searchbar did not received all the results yet, so I'm forced to use a good old delay here.

I will run that against the flaky runner to be sure than the 3000ms is enough.

Should we add a data-UI-render-complete prop to the popover or something so that we can rely on that in the test instead of a delay?

I wouldn't be able to use that from a FTR test. it's a user test, I would need something a human could see from the UI (or at least a class of something in the DOM). But the fact that a user do not have the info of when all the results are actually displayed in a smell IMHO.

Wait, isn't the Elastic logo that indicator? It stays spinning (as a loading indicator) while search is happening.

Nope. There was a PR that added a delay before displaying that indicator, so now we can't use it for that anymore. (#78879)

pgayvallet · 2020-11-20T07:53:53Z

@ryankeairns Thanks. I merged the PR.

To all: I created issues for all the remaining points / discussions of this PR and linked them to #74290.

This PR should be considered in its final state (regarding its scope). Any further increase in the functionalities should be listed, and done, as follow-ups in #74290.

ryankeairns · 2020-11-20T12:58:18Z

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!

ryankeairns

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:

KOTungseth · 2020-11-20T19:51:38Z

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.

joshdover

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.

joshdover · 2020-11-20T21:47:51Z

x-pack/plugins/global_search/common/types.ts

+/**
+ * @public
+ */
+export type GlobalSearchProviderFindParams = GlobalSearchFindParams;


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

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.

x-pack/plugins/global_search_bar/public/search_syntax/query_utils.ts

joshdover · 2020-11-20T22:38:52Z

x-pack/plugins/global_search_bar/public/search_syntax/query_utils.ts

+export const getFieldValueMap = (query: Query) => {
+  const fieldMap = new Map<string, FilterValues>();
+
+  query.ast.clauses.forEach((clause) => {


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 😄

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 · 2020-11-21T08:53:09Z

@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.

…ch-syntax

pgayvallet · 2020-11-22T15:55:52Z

@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

Thanks for the icon change!

…ch-syntax

kibanamachine · 2020-11-24T09:42:27Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 6322554

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

💚 Build #89479 succeeded 829c8f0
💚 Build #89163 succeeded c38c529
💔 Build #89162 failed 45424b4
💚 Build #89089 succeeded baa2013
💚 Build #88641 succeeded b28c441

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

* 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 <contactryank@gmail.com>

* [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 <contactryank@gmail.com> * fix mappings for 7.x Co-authored-by: Ryan Keairns <contactryank@gmail.com>

add search syntax parsing logic

ac7a5b8

pgayvallet added 3 commits November 17, 2020 08:10

Merge remote-tracking branch 'upstream/master' into kbn-74290-gs-sear…

16ab001

…ch-syntax

fix ts types

46f6543

use type filter in providers

a12bbdb

pgayvallet requested a review from a team November 17, 2020 09:10

pgayvallet mentioned this pull request Nov 17, 2020

[GS] Advanced search syntax to fine tune results returned #74290

Closed

3 tasks

pgayvallet added 2 commits November 17, 2020 11:25

move search syntax logic to the searchbar

d4582d3

fix test plugin types

b1b1af4

pgayvallet added Team:Core Platform Core services: plugins, logging, config, saved objects, http, ES client, i18n, etc t// v7.11.0 v8.0.0 labels Nov 17, 2020

pgayvallet added 9 commits November 17, 2020 12:24

Merge remote-tracking branch 'upstream/master' into kbn-74290-gs-sear…

7b59a53

…ch-syntax

fix test plugin types again

334c0f2

Merge remote-tracking branch 'upstream/master' into kbn-74290-gs-sear…

3e77eb3

…ch-syntax

use onSearch prop to disable internal component search

952c76b

add tag filter support

ef76562

add FTR tests

db0d913

move away from CI group 7

b2e661e

fix unit tests

063a96d

add unit tests

37d1dd3

pgayvallet added the release_note:roadmap label Nov 18, 2020

pgayvallet added 3 commits November 18, 2020 16:45

remove the API test suite

47cd1e1

Add icons to the SO results

bee03aa

add test for unknown type / tag

b28c441

pgayvallet mentioned this pull request Nov 18, 2020

Integrate tags to navigational search results #81846

Closed

nits

515743e

pgayvallet commented Nov 18, 2020

View reviewed changes

pgayvallet mentioned this pull request Nov 20, 2020

[GS] allow type 'alias' for search syntax #83889

Open

remove unused import

c38c529

myasonik mentioned this pull request Nov 20, 2020

[EuiPopover] has no way to pass data-test-subj to panel elastic/eui#4293

Closed

ryankeairns self-requested a review November 20, 2020 18:45

ryankeairns requested changes Nov 20, 2020

View reviewed changes

joshdover reviewed Nov 20, 2020

View reviewed changes

pgayvallet added 4 commits November 22, 2020 16:06

Merge remote-tracking branch 'upstream/master' into kbn-74290-gs-sear…

194f13b

…ch-syntax

hide icon for non-application results

9a21e53

add tsdoc on query utils

35e3fbd

coerce known filter values to string

829c8f0

pgayvallet requested review from joshdover and ryankeairns November 22, 2020 15:56

ryankeairns requested a review from a team November 23, 2020 14:32

ryankeairns approved these changes Nov 23, 2020

View reviewed changes

joshdover approved these changes Nov 23, 2020

View reviewed changes

joshdover mentioned this pull request Nov 24, 2020

Add application deep links to global search #83380

Merged

8 tasks

Merge remote-tracking branch 'upstream/master' into kbn-74290-gs-sear…

6322554

…ch-syntax

pgayvallet merged commit 7d5fb8e into elastic:master Nov 24, 2020

pgayvallet mentioned this pull request Nov 24, 2020

[7.x] [GS] add search syntax support (#83422) #84186

Merged

This was referenced Dec 9, 2020

Add saved object tags to integrations with Kibana saved objects elastic/integrations#327

Closed

[Discuss] Documentation for Tags and Navigational Search #85630

Closed

bhavyarm mentioned this pull request Jan 5, 2021

Kibana needs to tells users to use "quotes" while searching for tags if necessary #87402

Open

[GS] add search syntax support #83422

[GS] add search syntax support #83422

Uh oh!

Conversation

pgayvallet commented Nov 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Screenshots

Checklist

Release note

Uh oh!

ryankeairns commented Nov 16, 2020

Uh oh!

pgayvallet commented Nov 16, 2020

Uh oh!

pgayvallet commented Nov 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pgayvallet Nov 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pgayvallet Nov 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pgayvallet commented Nov 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ryankeairns commented Nov 20, 2020

Uh oh!

ryankeairns left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

KOTungseth commented Nov 20, 2020

Uh oh!

joshdover left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pgayvallet Nov 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pgayvallet commented Nov 21, 2020

Uh oh!

pgayvallet commented Nov 22, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

pgayvallet commented Nov 16, 2020 •

edited

Loading

pgayvallet commented Nov 17, 2020 •

edited

Loading

pgayvallet Nov 20, 2020 •

edited

Loading

pgayvallet Nov 20, 2020 •

edited

Loading

pgayvallet commented Nov 20, 2020 •

edited

Loading

ryankeairns left a comment •

edited

Loading

pgayvallet Nov 21, 2020 •

edited

Loading

pgayvallet commented Nov 22, 2020 •

edited

Loading