-
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
Changes from 3 commits
4effa5a
7191a07
74aa281
0a325af
8081e24
4b49c8c
765ab01
a2accc5
264ac9e
436f486
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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?'); | ||
|
||
|
@@ -178,6 +180,11 @@ export default function SearchSourceFactory(Promise, Private, config) { | |
}); | ||
}; | ||
|
||
SearchSource.prototype.highlightRequest = function highlightRequest() { | ||
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. I'm probably missing something here, but where is this ever called? 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. Oops, good point, that should have been removed. |
||
return this._flatten().then(({ body }) => { | ||
this.highlight(getHighlightRequest(body.query)); | ||
}); | ||
}; | ||
|
||
/****** | ||
* PRIVATE APIS | ||
|
This file was deleted.
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); | ||
}); | ||
}); |
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'); | ||
}); | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,31 +1,7 @@ | ||
import 'ui/highlight/highlight_tags'; | ||
import _ from 'lodash'; | ||
import angular from 'angular'; | ||
import uiModules from 'ui/modules'; | ||
import getHighlightHtml from './highlight_html'; | ||
import getHighlightRequestProvider from './highlight_request'; | ||
|
||
const module = uiModules.get('kibana'); | ||
|
||
module.filter('highlight', function (highlightTags) { | ||
return function (formatted, highlight) { | ||
if (typeof formatted === 'object') formatted = angular.toJson(formatted); | ||
|
||
_.each(highlight, function (section) { | ||
section = _.escape(section); | ||
|
||
// Strip out the highlight tags to compare against the formatted string | ||
const untagged = section | ||
.split(highlightTags.pre).join('') | ||
.split(highlightTags.post).join(''); | ||
|
||
// Replace all highlight tags with proper html tags | ||
const tagged = section | ||
.split(highlightTags.pre).join('<mark>') | ||
.split(highlightTags.post).join('</mark>'); | ||
|
||
// Replace all instances of the untagged string with the properly tagged string | ||
formatted = formatted.split(untagged).join(tagged); | ||
}); | ||
|
||
return formatted; | ||
}; | ||
}); | ||
export default { | ||
getHighlightHtml, | ||
getHighlightRequestProvider | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,29 @@ | ||
import _ from 'lodash'; | ||
import angular from 'angular'; | ||
import highlightTags from './highlight_tags'; | ||
import htmlTags from './html_tags'; | ||
|
||
export default function getHighlightHtml(fieldValue, highlights) { | ||
let highlightHtml = (typeof fieldValue === 'object') | ||
? angular.toJson(fieldValue) | ||
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. Could this just be 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. I can't remember exactly why this was put in here, but if I remember correctly, 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. Fair enough, not a big deal |
||
: fieldValue; | ||
|
||
_.each(highlights, function (highlight) { | ||
const escapedHighlight = _.escape(highlight); | ||
|
||
// Strip out the highlight tags to compare against the field text | ||
const untaggedHighlight = escapedHighlight | ||
.split(highlightTags.pre).join('') | ||
.split(highlightTags.post).join(''); | ||
|
||
// Replace all highlight tags with proper html tags | ||
const taggedHighlight = escapedHighlight | ||
.split(highlightTags.pre).join(htmlTags.pre) | ||
.split(highlightTags.post).join(htmlTags.post); | ||
|
||
// Replace all instances of the untagged string with the properly tagged string | ||
highlightHtml = highlightHtml.split(untaggedHighlight).join(taggedHighlight); | ||
}); | ||
|
||
return highlightHtml; | ||
} |
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.