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

Fix Enterprise Search document search bug in indices without position data indexed #148397

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 6 additions & 3 deletions x-pack/plugins/enterprise_search/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -49,11 +49,14 @@ To debug Kea state in-browser, Kea recommends [Redux Devtools](https://v2.keajs.

Documentation: https://www.elastic.co/guide/en/kibana/current/development-tests.html#_unit_testing

Jest tests can be run directly from the `x-pack/plugins/enterprise_search` folder. This also works for any subfolders or subcomponents.
Jest tests can be run from the root kibana directory, however, since the tests take so long to run you will likely want to apply the appropriate Jest configuration file to test only your changes. For example:
- `x-pack/plugins/enterprise_search/common/jest.config.js`
- `x-pack/plugins/enterprise_search/public/jest.config.js`
- `x-pack/plugins/enterprise_search/server/jest.config.js`

```bash
yarn test:jest
yarn test:jest --watch
yarn test:jest --config {YOUR_JEST_CONFIG_FILE}
yarn test:jest --config {YOUR_JEST_CONFIG_FILE} --watch
```

Unfortunately coverage collection does not work as automatically, and requires using our handy jest.sh script if you want to run tests on a specific file or folder and only get coverage numbers for that file or folder:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,28 @@ describe('fetchSearchResults lib function', () => {
expect(mockClient.asCurrentUser.search).toHaveBeenCalledWith({
from: DEFAULT_FROM_VALUE,
index: indexName,
q: JSON.stringify(query),
q: query,
size: ENTERPRISE_SEARCH_DOCUMENTS_DEFAULT_DOC_COUNT,
});
});

it('should escape quotes in queries and return results with hits', async () => {
mockClient.asCurrentUser.search.mockImplementation(
() => regularSearchResultsResponse as SearchResponseBody
);

await expect(
fetchSearchResults(
mockClient as unknown as IScopedClusterClient,
indexName,
'"yellow banana"'
)
).resolves.toEqual(regularSearchResultsResponse);

expect(mockClient.asCurrentUser.search).toHaveBeenCalledWith({
from: DEFAULT_FROM_VALUE,
index: indexName,
q: '\\"yellow banana\\"',
size: ENTERPRISE_SEARCH_DOCUMENTS_DEFAULT_DOC_COUNT,
});
});
Expand Down Expand Up @@ -120,7 +141,7 @@ describe('fetchSearchResults lib function', () => {
expect(mockClient.asCurrentUser.search).toHaveBeenCalledWith({
from: DEFAULT_FROM_VALUE,
index: indexName,
q: JSON.stringify(query),
q: query,
size: ENTERPRISE_SEARCH_DOCUMENTS_DEFAULT_DOC_COUNT,
});
});
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ export const fetchSearchResults = async (
from,
index: indexName,
size,
...(!!query ? { q: JSON.stringify(query) } : {}),
...(!!query ? { q: query.replace(/"/g, '\\"') } : {}),
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm good with this, but should we do any follow-up to ensure the query string is sanitized further?

Copy link
Member Author

Choose a reason for hiding this comment

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

@TattdCodeMonkey That's a great question. Did you have anything specific in mind?

@sphilipse seemed to think that was sufficient because you can't really inject anything into the query at that point plus it's already behind authorization. I really dislike the idea of ad-hoc manual safety String mutations for several reasons. We could consider adding a new issue to re-do the search form so that Stringify doesn't force everything to use multi-match, but I'm not really sure what the priority is. WDYT?

Copy link
Contributor

Choose a reason for hiding this comment

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

@kderusso The call is a query_string query, right? Could you describe how the multi-word query fails due to the index not supporting multi-match, e.g. show the error message? Maybe we can fix it in a different way.

My concern is putting the expression in quotes forces a phrase match, which differs from the actual full text matching experience (OR semantics by default). But I agree this is low priority.

Copy link
Member Author

Choose a reason for hiding this comment

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

@demjened Can you please help me better understand your concern here? JSON.stringify was quoting the query and forcing a multi-match query to be run. This removes this default behavior.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, I thought query was already coming in quoted from the UI. But then why the need for .replace()? Can't we just pass q: query as-is?

Copy link
Member

Choose a reason for hiding this comment

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

Users can still input a quote in the front-end, which will force a multi-match if we don't escape them.

Quick question: do we also need to escape single quotes, or only double quotes?

Copy link
Member Author

Choose a reason for hiding this comment

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

@sphilipse I have verified that single quotes don't need to be escaped, using them doesn't error.

@demjened If we use double quotes it will force the multi-match phrase query and error. Escaping them solves this.

});
return results;
};