[Management] Allow wildcard anywhere in the search query by chrisronline · Pull Request #16109 · elastic/kibana

chrisronline · 2018-01-17T20:24:10Z

Fixes #16098

This PR addresses two things:

Ensures that queries like *o* properly match an index like foo. Before this PR, only ending wildcards did anything.
Addresses a bug where the number of results showing on the index pattern creation page was fewer than expected. This is because the proper limit was never passed into the ES query and it used the default limit (which I think is 10)

Testing

Ensure that using a wildcard in the search query, in places other than the end, match indices properly.

Tests

Tests have been updated to handle this scenario.

cjcenizal

Nice! I tested this in browser and it works. I have a couple small comments. Have you tested that MAX_SEARCH_SIZE has the expected result when you have a massive number of indices?

cjcenizal · 2018-01-18T00:42:54Z

+    });
+  });
+
+  it('should support queries with wildcards in various places again', () => {


What is this test verifying which the one before it does not?

It should be verifying that a wildcard not at the end of the query still works

cjcenizal · 2018-01-18T00:46:04Z

-    if (query.endsWith('*') && name.indexOf(query.substring(0, query.length - 1)) === 0) {
-      return true;
+
+    const regexQuery = query.startsWith('*')


How about extracting this logic into a helper function, and adding a couple light tests for it? I think it would improve readability of this code.

Sounds like the right move!

cjcenizal · 2018-01-18T00:48:01Z

+      allIndices: [{ name: 'kibana' }, { name: 'kibana2' }, { name: 'es' }],
+      exactMatchedIndices: [{ name: 'kibana' }, { name: 'kibana2' }],
+      partialMatchedIndices: [{ name: 'kibana' }, { name: 'kibana2' }],
+      visibleIndices: [{ name: 'kibana' }, { name: 'kibana2' }]


Something to think about in a later iteration, but I think we might be able to make these tests clearer if we pared the tests down to make assertions against just allIndices, exactMatchedIndices, partialMatchedIndices, and visibleIndices. This would result in more focused tests and I think it'd be easier to spot the relationships between the input and each specific type of output. It would result in more tests, however. What do you think?

Totally agree. Great suggestion!

chrisronline · 2018-01-18T15:13:50Z

@cjcenizal I haven't tested it like that. Rather, I've just decreased the number to something small, like 3 and it works as expected. Do you think it's necessary to test it like you're describing?

cjcenizal · 2018-01-18T16:49:40Z

I was just thinking of this part of the PR description:

Addresses a bug where the number of results showing on the index pattern creation page was fewer than expected. This is because the proper limit was never passed into the ES query and it used the default limit (which I think is 10)

Is there a way we can verify this behaves as expected now?

cjcenizal

🏇 Sweet! Just had a couple minor comments about test readability.

cjcenizal · 2018-01-18T18:01:18Z

+import { isQueryAMatch } from '../is_query_a_match';
+
+describe('isQueryAMatch', () => {
+  it('should handle straight up matches', () => {


"Handles" is a bit ambiguous to me. Can we rephrase/restructure these statements to assert the nature of the outcome? I'd suggest:

describe('returns true', () => { it('for an exact value') it('for a pattern with a trailing wildcard') it('for a pattern with a leading wildcard') it('for a pattern with a middle and trailing wildcard') it('for a pattern with a middle wildcard') it(`for a pattern that's only a wildcard`) }); describe('returns false', () => { it('for a non-matching pattern') });

cjcenizal · 2018-01-18T18:02:18Z

+
+  // This shouldn't be necessary but just used as a safety net
+  // so the page doesn't bust if the user types in some weird
+  // query that throws an exception when converting to a RegExp


Nice comment!

cjcenizal · 2018-01-18T18:25:20Z

-    ];
-
-    const result = getMatchedIndices(indices, matchedIndices, query, true);
+  describe('visibleIndices', () => {


Re our convo, I think these test would be easier to follow if we reverse the order of the "filtered" and "not filtered" assertions, so that the reader understands the baseline behavior first, and then can compare that with the "filtered" test to see how changing the last argument to false affects the output.

I also think being more verbose with the description would help make these tests clearer, e.g. by changing "should return unfiltered" to "returns all indices" and "should return filtered" to "excludes system indices".

chrisronline · 2018-01-18T19:00:13Z

Is there a way we can verify this behaves as expected now?

I just made a code change to handle and test this: ab6535d

Thoughts?

cjcenizal

🤜 💥 🤛 Awesome!

* Allow wildcard anywhere in the search query * PR feedback for tests * Update tests * Throw exception when missing required parameter

…6154) * Allow wildcard anywhere in the search query * PR feedback for tests * Update tests * Throw exception when missing required parameter

chrisronline · 2018-01-19T15:08:07Z

Backport:

6.x: #16154

* Allow wildcard anywhere in the search query * PR feedback for tests * Update tests * Throw exception when missing required parameter

Allow wildcard anywhere in the search query

fd753d4

chrisronline added review :Management v7.0.0 v6.2.0 labels Jan 17, 2018

chrisronline self-assigned this Jan 17, 2018

chrisronline requested review from bmcconaghy and cjcenizal January 17, 2018 20:24

cjcenizal reviewed Jan 18, 2018

View reviewed changes

PR feedback for tests

3496493

chrisronline mentioned this pull request Jan 18, 2018

Can't create index pattern containing * #16098

Closed

cjcenizal approved these changes Jan 18, 2018

View reviewed changes

chrisronline added 2 commits January 18, 2018 13:50

Update tests

bdc31f7

Throw exception when missing required parameter

ab6535d

cjcenizal approved these changes Jan 19, 2018

View reviewed changes

chrisronline merged commit 8864755 into elastic:master Jan 19, 2018

chrisronline mentioned this pull request Jan 19, 2018

[6.x] [Management] Allow wildcard anywhere in the search query (#16109) #16154

Merged

chrisronline deleted the fix/16098 branch January 19, 2018 14:14

Conversation

chrisronline commented Jan 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Testing

Tests

Uh oh!

cjcenizal left a comment

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!

chrisronline commented Jan 18, 2018

Uh oh!

cjcenizal commented Jan 18, 2018

Uh oh!

cjcenizal left a comment

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!

chrisronline commented Jan 18, 2018

Uh oh!

cjcenizal left a comment

Choose a reason for hiding this comment

Uh oh!

chrisronline commented Jan 19, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

chrisronline commented Jan 17, 2018 •

edited

Loading