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

[Query Parsing] Use unknownFilters as part of the search term #186432

Closed
wants to merge 5 commits into from

Conversation

tsullivan
Copy link
Member

@tsullivan tsullivan commented Jun 18, 2024

Summary

Closes #184496

Issues fixed:

  1. In the Saved Object Finder, it was impossible to use a simple field clause for tagged content, such as tag:foo. The syntax was restricted to use an array, i.e: tag:(foo).
  2. In the Saved Object Finder and in the Global Search Bar, term clauses containing a : were interpreted as field clause queries.

It was noticed in this work that Global Search doesn't support the must_not match query. That is: -tag:foo will search for content tagged with foo instead of excluding it. This problem isn't evident in the Saved Objects Finder. A fix for that will have to come in a separate PR.

Release Note

Fixed an issue with the Global Search Bar where it was not possible to search for saved objects that contain a colon character.

Checklist

  • Fix saved object picker
  • Fix global search bar
  • Unit or functional tests were updated or added to match the most common scenarios

@tsullivan tsullivan force-pushed the fix/184496 branch 2 times, most recently from cf00805 to fb1db31 Compare June 19, 2024 00:05
@tsullivan
Copy link
Member Author

/ci

@tsullivan tsullivan force-pushed the fix/184496 branch 3 times, most recently from 016eb51 to 66c5e8c Compare July 2, 2024 00:50
@tsullivan tsullivan marked this pull request as ready for review July 2, 2024 01:04
@tsullivan tsullivan requested a review from a team as a code owner July 2, 2024 01:04
@tsullivan tsullivan added release_note:fix Team:SharedUX Team label for AppEx-SharedUX (formerly Global Experience) labels Jul 2, 2024
@elasticmachine
Copy link
Contributor

Pinging @elastic/appex-sharedux (Team:SharedUX)

@tsullivan tsullivan removed the request for review from TinaHeiligers July 2, 2024 01:09
@tsullivan
Copy link
Member Author

/ci

@kibana-ci
Copy link
Collaborator

💛 Build succeeded, but was flaky

Failed CI Steps

Metrics [docs]

Page load bundle

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

id before after diff
globalSearchBar 28.6KB 28.7KB +23.0B

History

  • 💔 Build #216595 failed fb1db3104b2d9882153249b217fdfc43f150b282

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

@tsullivan tsullivan marked this pull request as draft July 2, 2024 03:05
@tsullivan tsullivan removed the request for review from lukasolson July 2, 2024 18:39
@tsullivan
Copy link
Member Author

/ci

@tsullivan tsullivan marked this pull request as ready for review July 2, 2024 18:39
@tsullivan tsullivan requested a review from a team as a code owner July 2, 2024 18:39
const query = Query.parse('tag:tag-1 unknown:worlds some search');

expect(parseQuery(query, [])).toEqual({
queryText: 'some search unknown:worlds',
Copy link
Member

Choose a reason for hiding this comment

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

is it OK to invert the search?

Copy link
Member Author

Choose a reason for hiding this comment

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

@afharo It looks like the order of terms doesn't matter

image

Copy link
Member

Choose a reason for hiding this comment

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

FMI, do we support double-quote searches here?

Copy link
Contributor

@pgayvallet pgayvallet left a comment

Choose a reason for hiding this comment

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

In the Saved Object Finder and in the Global Search Bar, term clauses containing a : were interpreted as field clause queries.

I would just have documented that : is a special characters and that special characters must be escaped by double quoting the whole term. You're entering the rabit hole doing what you did there ihmo

@tsullivan
Copy link
Member Author

@pgayvallet

I would just have documented that : is a special characters and that special characters must be escaped by double quoting the whole term. You're entering the rabit hole doing what you did there ihmo

I understand the concern that this issue didn't entirely seem like something we need to solve in code. From my point of view, it would be better to solve the issue with better UX rather than documenting a limitation, and I don't know if we have an appropriate place in our documentation that users would easily find or would fit this kind of message.

Also, there didn't seem to be a big reason to treat : to be treated as a special character. What should be treated as special are just the key prefixes: tag: and type:. IMO the issue #184496 was filed based on the possible fact that when users type : into the search bar and aren't using one of the key prefixes, they do that because they are searching for a saved object with : in the name.

Finally, I think that having a bad UX in a search bar is an especially bad thing for Elastic, since we are a search-based company, getting this right should be our forte.

@elasticmachine
Copy link
Contributor

💛 Build succeeded, but was flaky

Failed CI Steps

Test Failures

  • [job] [logs] FTR Configs #42 / Rule execution logic API Detection Engine - Execution logic @ess @serverless @serverlessQA Threat match type rules timestamp override and fallback timestamp should create alert using a timestamp override and timestamp fallback enabled on events first code path execution

Metrics [docs]

Page load bundle

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

id before after diff
globalSearchBar 28.6KB 28.7KB +60.0B
savedObjectsManagement 19.8KB 20.1KB +338.0B
total +398.0B

History

@pgayvallet
Copy link
Contributor

pgayvallet commented Jul 4, 2024

@tsullivan

lso, there didn't seem to be a big reason to treat : to be treated as a special character. What should be treated as special are just the key prefixes: tag: and type:

Then maybe we could try to fix it upstream, especially given EUI is an internally maintained lib, instead of trying to "monkeypatch" that on top of the already existing syntax?

It's just that in my experience, that kind of approach can become very painful in term of maintaince, so I wanted to share my concerns, but if we still think it's the best approach, I won't object or anything.

Finally, I think that having a bad UX in a search bar is an especially bad thing for Elastic, since we are a search-based company, getting this right should be our forte.

I absolutely agree

@tsullivan
Copy link
Member Author

Then maybe we could try to fix it upstream, especially given EUI is an internally maintained lib, instead of trying to "monkeypatch" that on top of the already existing syntax?

This sounds good to me. I will place this PR in Draft mode for now.

@tsullivan tsullivan marked this pull request as draft July 4, 2024 17:29
@tsullivan
Copy link
Member Author

Closing for now, as the fix in the EUI repo will look very different than what was started here.

@tsullivan tsullivan closed this Jul 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release_note:fix Team:SharedUX Team label for AppEx-SharedUX (formerly Global Experience)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Choose a source dialog box returns unexpected results when searching data views with colon
5 participants