-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
Improve highlighting by using highlight_query with all_fields enabled #9671
Merged
Merged
Changes from 9 commits
Commits
Show all changes
10 commits
Select commit
Hold shift + click to select a range
4effa5a
Add all_fields to highlight query to improve highlighting
lukasolson 7191a07
Refactor highlighting and move out of _flatten
lukasolson 74aa281
Merge branch 'master' into highlight-all-fields
lukasolson 0a325af
Merge remote-tracking branch 'upstream/master' into highlight-all-fields
lukasolson 8081e24
Make changes as per @bargs' requests
lukasolson 4b49c8c
Add documentation about highlightAll setting
lukasolson 765ab01
Merge branch 'master' into highlight-all-fields
lukasolson a2accc5
Fix docs typo
lukasolson 264ac9e
Remove unused function
lukasolson 436f486
Remove unused code
lukasolson File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -58,13 +58,15 @@ import AbstractDataSourceProvider from './_abstract'; | |
import SearchRequestProvider from '../fetch/request/search'; | ||
import SegmentedRequestProvider from '../fetch/request/segmented'; | ||
import SearchStrategyProvider from '../fetch/strategy/search'; | ||
import { getHighlightRequestProvider } from '../../highlight'; | ||
|
||
export default function SearchSourceFactory(Promise, Private, config) { | ||
const SourceAbstract = Private(AbstractDataSourceProvider); | ||
const SearchRequest = Private(SearchRequestProvider); | ||
const SegmentedRequest = Private(SegmentedRequestProvider); | ||
const searchStrategy = Private(SearchStrategyProvider); | ||
const normalizeSortRequest = Private(NormalizeSortRequestProvider); | ||
const getHighlightRequest = getHighlightRequestProvider(config); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This might also be a left-over that can be removed. |
||
|
||
const forIp = Symbol('for which index pattern?'); | ||
|
||
|
@@ -93,6 +95,7 @@ export default function SearchSourceFactory(Promise, Private, config) { | |
'filter', | ||
'sort', | ||
'highlight', | ||
'highlightAll', | ||
'aggs', | ||
'from', | ||
'size', | ||
|
@@ -178,7 +181,6 @@ export default function SearchSourceFactory(Promise, Private, config) { | |
}); | ||
}; | ||
|
||
|
||
/****** | ||
* PRIVATE APIS | ||
******/ | ||
|
@@ -249,6 +251,7 @@ export default function SearchSourceFactory(Promise, Private, config) { | |
case 'index': | ||
case 'type': | ||
case 'id': | ||
case 'highlightAll': | ||
if (key && state[key] == null) { | ||
state[key] = val; | ||
} | ||
|
This file was deleted.
Oops, something went wrong.
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,69 @@ | ||
import expect from 'expect.js'; | ||
import highlightTags from '../highlight_tags'; | ||
import htmlTags from '../html_tags'; | ||
import getHighlightHtml from '../highlight_html'; | ||
|
||
describe('getHighlightHtml', function () { | ||
const text = '' + | ||
'Bacon ipsum dolor amet pork loin pork cow pig beef chuck ground round shankle sirloin landjaeger kevin ' + | ||
'venison sausage ribeye tongue. Chicken bacon ball tip pork. Brisket pork capicola spare ribs pastrami rump ' + | ||
'sirloin, t-bone ham shoulder jerky turducken bresaola. Chicken cow beef picanha. Picanha hamburger alcatra ' + | ||
'cupim. Salami capicola boudin pork belly shank picanha.'; | ||
|
||
it('should not modify text if highlight is empty', function () { | ||
expect(getHighlightHtml(text, undefined)).to.be(text); | ||
expect(getHighlightHtml(text, null)).to.be(text); | ||
expect(getHighlightHtml(text, [])).to.be(text); | ||
}); | ||
|
||
it('should preserve escaped text', function () { | ||
const highlights = ['<foo>']; | ||
const result = getHighlightHtml('<foo>', highlights); | ||
expect(result.indexOf('<foo>')).to.be(-1); | ||
expect(result.indexOf('<foo>')).to.be.greaterThan(-1); | ||
}); | ||
|
||
it('should highlight a single result', function () { | ||
const highlights = [ | ||
highlightTags.pre + 'hamburger' + highlightTags.post + ' alcatra cupim. Salami capicola boudin pork belly shank picanha.' | ||
]; | ||
const result = getHighlightHtml(text, highlights); | ||
expect(result.indexOf(htmlTags.pre + 'hamburger' + htmlTags.post)).to.be.greaterThan(-1); | ||
expect(result.split(htmlTags.pre + 'hamburger' + htmlTags.post).length).to.be(text.split('hamburger').length); | ||
}); | ||
|
||
it('should highlight multiple results', function () { | ||
const highlights = [ | ||
'kevin venison sausage ribeye tongue. ' + highlightTags.pre + 'Chicken' + highlightTags.post + ' bacon ball tip pork. Brisket ' + | ||
'pork capicola spare ribs pastrami rump sirloin, t-bone ham shoulder jerky turducken bresaola. ' + highlightTags.pre + | ||
'Chicken' + highlightTags.post + ' cow beef picanha. Picanha' | ||
]; | ||
const result = getHighlightHtml(text, highlights); | ||
expect(result.indexOf(htmlTags.pre + 'Chicken' + htmlTags.post)).to.be.greaterThan(-1); | ||
expect(result.split(htmlTags.pre + 'Chicken' + htmlTags.post).length).to.be(text.split('Chicken').length); | ||
}); | ||
|
||
it('should highlight multiple hits in a result', function () { | ||
const highlights = [ | ||
'Bacon ipsum dolor amet ' + highlightTags.pre + 'pork' + highlightTags.post + ' loin ' + | ||
'' + highlightTags.pre + 'pork' + highlightTags.post + ' cow pig beef chuck ground round shankle ' + | ||
'sirloin landjaeger', | ||
'kevin venison sausage ribeye tongue. Chicken bacon ball tip ' + | ||
'' + highlightTags.pre + 'pork' + highlightTags.post + '. Brisket ' + | ||
'' + highlightTags.pre + 'pork' + highlightTags.post + ' capicola spare ribs', | ||
'hamburger alcatra cupim. Salami capicola boudin ' + highlightTags.pre + 'pork' + highlightTags.post + ' ' + | ||
'belly shank picanha.' | ||
]; | ||
const result = getHighlightHtml(text, highlights); | ||
expect(result.indexOf(htmlTags.pre + 'pork' + htmlTags.post)).to.be.greaterThan(-1); | ||
expect(result.split(htmlTags.pre + 'pork' + htmlTags.post).length).to.be(text.split('pork').length); | ||
}); | ||
|
||
it('should accept an object and return a string containing its properties', function () { | ||
const obj = { foo: 1, bar: 2 }; | ||
const result = getHighlightHtml(obj, null); | ||
expect(result.indexOf('' + obj)).to.be(-1); | ||
expect(result.indexOf('foo')).to.be.greaterThan(-1); | ||
expect(result.indexOf('bar')).to.be.greaterThan(-1); | ||
}); | ||
}); |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,82 @@ | ||
import expect from 'expect.js'; | ||
import ngMock from 'ng_mock'; | ||
import getHighlightRequestProvider from '../highlight_request'; | ||
|
||
describe('getHighlightRequest', () => { | ||
const queryStringQuery = { query_string: { query: 'foo' } }; | ||
const rangeQuery = { range: { '@timestamp': { gte: 0, lte: 0 } } }; | ||
const boolQueryWithSingleCondition = { | ||
bool: { | ||
must: queryStringQuery, | ||
} | ||
}; | ||
const boolQueryWithMultipleConditions = { | ||
bool: { | ||
must: [ | ||
queryStringQuery, | ||
rangeQuery | ||
] | ||
} | ||
}; | ||
|
||
let config; | ||
let previousHighlightConfig; | ||
let previousAllFieldsConfig; | ||
let getHighlightRequest; | ||
|
||
beforeEach(ngMock.module('kibana')); | ||
beforeEach(ngMock.inject(function (_config_) { | ||
config = _config_; | ||
previousHighlightConfig = config.get('doc_table:highlight'); | ||
previousAllFieldsConfig = config.get('doc_table:highlight:all_fields'); | ||
})); | ||
|
||
afterEach(() => { | ||
config.set('doc_table:highlight', previousHighlightConfig); | ||
config.set('doc_table:highlight:all_fields', previousAllFieldsConfig); | ||
}); | ||
|
||
it('should be a function', () => { | ||
getHighlightRequest = getHighlightRequestProvider(config); | ||
expect(getHighlightRequest).to.be.a(Function); | ||
}); | ||
|
||
it('should add the all_fields param with query_string query without modifying original query', () => { | ||
getHighlightRequest = getHighlightRequestProvider(config); | ||
const request = getHighlightRequest(queryStringQuery); | ||
expect(request.fields['*']).to.have.property('highlight_query'); | ||
expect(request.fields['*'].highlight_query.query_string).to.have.property('all_fields'); | ||
expect(queryStringQuery.query_string).to.not.have.property('all_fields'); | ||
}); | ||
|
||
it('should add the all_fields param with bool query with single condition without modifying original query', () => { | ||
getHighlightRequest = getHighlightRequestProvider(config); | ||
const request = getHighlightRequest(boolQueryWithSingleCondition); | ||
expect(request.fields['*']).to.have.property('highlight_query'); | ||
expect(request.fields['*'].highlight_query.bool.must.query_string).to.have.property('all_fields'); | ||
expect(queryStringQuery.query_string).to.not.have.property('all_fields'); | ||
}); | ||
|
||
it('should add the all_fields param with bool query with multiple conditions without modifying original query', () => { | ||
getHighlightRequest = getHighlightRequestProvider(config); | ||
const request = getHighlightRequest(boolQueryWithMultipleConditions); | ||
expect(request.fields['*']).to.have.property('highlight_query'); | ||
expect(request.fields['*'].highlight_query.bool.must).to.have.length(boolQueryWithMultipleConditions.bool.must.length); | ||
expect(request.fields['*'].highlight_query.bool.must[0].query_string).to.have.property('all_fields'); | ||
expect(queryStringQuery.query_string).to.not.have.property('all_fields'); | ||
}); | ||
|
||
it('should return undefined if highlighting is turned off', () => { | ||
config.set('doc_table:highlight', false); | ||
getHighlightRequest = getHighlightRequestProvider(config); | ||
const request = getHighlightRequest(queryStringQuery); | ||
expect(request).to.be(undefined); | ||
}); | ||
|
||
it('should not add the highlight_query param if all_fields is turned off', () => { | ||
config.set('doc_table:highlight:all_fields', false); | ||
getHighlightRequest = getHighlightRequestProvider(config); | ||
const request = getHighlightRequest(queryStringQuery); | ||
expect(request.fields['*']).to.not.have.property('highlight_query'); | ||
}); | ||
}); |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This might also be a left-over that can be removed.