-
Notifications
You must be signed in to change notification settings - Fork 8.6k
[Discover][Logs profile] Fix missing search highlights #260056
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
Changes from all commits
903f11f
a4a4e12
f8a7d63
c26f565
d016668
1e16540
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
This file was deleted.
This file was deleted.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,95 @@ | ||
| /* | ||
| * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one | ||
| * or more contributor license agreements. Licensed under the "Elastic License | ||
| * 2.0", the "GNU Affero General Public License v3.0 only", and the "Server Side | ||
| * Public License v 1"; you may not use this file except in compliance with, at | ||
| * your election, the "Elastic License 2.0", the "GNU Affero General Public | ||
| * License v3.0 only", or the "Server Side Public License, v 1". | ||
| */ | ||
|
|
||
| import { escapeAndPreserveHighlightTags, getHighlightedFieldValue } from './highlight_utils'; | ||
|
|
||
| // Must match the html tags defined in @kbn/field-formats-plugin (html_tags.ts) | ||
| const PRE = '<mark class="ffSearch__highlight">'; | ||
| const POST = '</mark>'; | ||
|
|
||
| // Must match the tags defined in @kbn/field-formats-plugin (highlight_tags.ts) | ||
| const ES_PRE = '@kibana-highlighted-field@'; | ||
| const ES_POST = '@/kibana-highlighted-field@'; | ||
|
|
||
| describe('escapeAndPreserveHighlightTags', () => { | ||
| it('escapes HTML when there are no highlight tags', () => { | ||
| expect(escapeAndPreserveHighlightTags('<hello>world</hello>')).toBe( | ||
| '<hello>world</hello>' | ||
| ); | ||
| }); | ||
|
|
||
| it('preserves highlight wrappers while escaping the content', () => { | ||
| expect(escapeAndPreserveHighlightTags(`${PRE}<hello>${POST}`)).toBe( | ||
| `${PRE}<hello>${POST}` | ||
| ); | ||
| }); | ||
|
|
||
| it('preserves multiple highlight regions', () => { | ||
| expect(escapeAndPreserveHighlightTags(`${PRE}hello${POST} + ${PRE}world${POST}`)).toBe( | ||
| `${PRE}hello${POST} + ${PRE}world${POST}` | ||
| ); | ||
| }); | ||
|
|
||
| it('escapes plain <mark> tags that do not match the highlight format', () => { | ||
| expect(escapeAndPreserveHighlightTags('<mark><hello></mark>')).toBe( | ||
| '<mark><hello></mark>' | ||
| ); | ||
| }); | ||
|
|
||
| it('escapes ES highlight tags as plain text (not converted)', () => { | ||
| expect(escapeAndPreserveHighlightTags(`${ES_PRE}test${ES_POST}`)).toBe( | ||
| `${ES_PRE}test${ES_POST}` | ||
| ); | ||
| }); | ||
| }); | ||
|
|
||
| describe('getHighlightedFieldValue', () => { | ||
| it('escapes the field value when no highlights are provided', () => { | ||
| expect(getHighlightedFieldValue('<hello>', undefined)).toBe('<hello>'); | ||
| }); | ||
|
|
||
| it('escapes the field value when highlights is an empty array', () => { | ||
| expect(getHighlightedFieldValue('<hello>', [])).toBe('<hello>'); | ||
| }); | ||
|
|
||
| it('replaces matching text with highlighted version from a single snippet', () => { | ||
| expect( | ||
| getHighlightedFieldValue('This is a test message', [ | ||
| `This is a ${ES_PRE}test${ES_POST} message`, | ||
| ]) | ||
| ).toBe(`This is a ${PRE}test${POST} message`); | ||
| }); | ||
|
|
||
| it('handles multiple highlight regions within a single snippet', () => { | ||
| expect( | ||
| getHighlightedFieldValue('hello world', [`${ES_PRE}hello${ES_POST} ${ES_PRE}world${ES_POST}`]) | ||
| ).toBe(`${PRE}hello${POST} ${PRE}world${POST}`); | ||
| }); | ||
|
|
||
| it('applies highlights from multiple snippets for multi-valued fields', () => { | ||
| expect( | ||
| getHighlightedFieldValue('error in service A, warning in service B', [ | ||
| `${ES_PRE}error${ES_POST} in service A`, | ||
| `${ES_PRE}warning${ES_POST} in service B`, | ||
| ]) | ||
| ).toBe(`${PRE}error${POST} in service A, ${PRE}warning${POST} in service B`); | ||
| }); | ||
|
|
||
| it('escapes HTML in the field value while preserving highlight tags', () => { | ||
| expect(getHighlightedFieldValue('<b>test</b>', [`${ES_PRE}<b>test</b>${ES_POST}`])).toBe( | ||
| `${PRE}<b>test</b>${POST}` | ||
| ); | ||
| }); | ||
|
|
||
| it('returns escaped value when highlights do not match field text', () => { | ||
| expect(getHighlightedFieldValue('no match here', [`${ES_PRE}other text${ES_POST}`])).toBe( | ||
| 'no match here' | ||
| ); | ||
| }); | ||
| }); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,87 @@ | ||
| /* | ||
| * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one | ||
| * or more contributor license agreements. Licensed under the "Elastic License | ||
| * 2.0", the "GNU Affero General Public License v3.0 only", and the "Server Side | ||
| * Public License v 1"; you may not use this file except in compliance with, at | ||
| * your election, the "Elastic License 2.0", the "GNU Affero General Public | ||
| * License v3.0 only", or the "Server Side Public License, v 1". | ||
| */ | ||
|
|
||
| import { escape } from 'lodash'; | ||
|
|
||
| // TODO: Remove these duplicated utils when we have a proper way to access the highlight tags | ||
| // or when we have a proper HTML field formatters | ||
| // Related issue for field formatters: https://github.com/elastic/kibana/issues/259286 | ||
|
|
||
| // Duplicated from @kbn/field-formats-plugin because packages cannot depend on plugins. | ||
| const HTML_HIGHLIGHT_PRE_TAG = '<mark class="ffSearch__highlight">'; | ||
| const HTML_HIGHLIGHT_POST_TAG = '</mark>'; | ||
|
|
||
| const ES_HIGHLIGHT_PRE_TAG = '@kibana-highlighted-field@'; | ||
| const ES_HIGHLIGHT_POST_TAG = '@/kibana-highlighted-field@'; | ||
|
|
||
| /** | ||
| * Escapes HTML in a string while preserving field-format highlight <mark> tags. | ||
| * Used for values already processed by formatFieldValue / getHighlightHtml (e.g. resource badges). | ||
| */ | ||
| export function escapeAndPreserveHighlightTags(value: string): string { | ||
| if (!value.includes(HTML_HIGHLIGHT_PRE_TAG)) { | ||
| return escape(value); | ||
| } | ||
|
|
||
| return value | ||
| .split(HTML_HIGHLIGHT_PRE_TAG) | ||
| .map((segment, index) => { | ||
| if (index === 0) return escape(segment); | ||
|
|
||
| const postTagIndex = segment.indexOf(HTML_HIGHLIGHT_POST_TAG); | ||
| if (postTagIndex === -1) return escape(segment); | ||
|
|
||
| const highlighted = segment.substring(0, postTagIndex); | ||
| const rest = segment.substring(postTagIndex + HTML_HIGHLIGHT_POST_TAG.length); | ||
|
|
||
| return HTML_HIGHLIGHT_PRE_TAG + escape(highlighted) + HTML_HIGHLIGHT_POST_TAG + escape(rest); | ||
| }) | ||
| .join(''); | ||
| } | ||
|
|
||
| /** | ||
| * Merges ES highlight snippets into a field value, producing safe HTML with <mark> tags. | ||
| * Replicates the logic of getHighlightHtml from @kbn/field-formats-plugin, which packages | ||
| * cannot import directly. | ||
|
Contributor
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. Do we need to duplicate the field formats logic here? The components which use this should already have access to the field formats plugin. Can we not just use
Contributor
Author
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. We tried using Additionally, this component applies custom log-level coloring via The duplication is minimal (just the core split/join logic from
Contributor
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. Thanks for clarifying. I think the fact we need to do this points to an issue around the OTel docs logic, but it doesn't seem to add new risk, so I won't block on it. But for clarity, you'll still need to provide a |
||
| * | ||
| * Each snippet in the highlights array is the full field value with ES highlight tags | ||
| * around matched terms. The function iterates over all snippets (handling multi-valued | ||
| * fields), strips the ES tags to find the matching text, and replaces those occurrences | ||
| * in the escaped field value with properly tagged <mark> elements. | ||
| */ | ||
| export function getHighlightedFieldValue( | ||
| fieldValue: string, | ||
| highlights: string[] | undefined | ||
| ): string { | ||
| if (!highlights?.length) { | ||
| return escape(fieldValue); | ||
| } | ||
|
|
||
| let result = escape(fieldValue); | ||
|
|
||
| for (const highlight of highlights) { | ||
| const escapedHighlight = escape(highlight); | ||
|
|
||
| const untaggedHighlight = escapedHighlight | ||
| .split(ES_HIGHLIGHT_PRE_TAG) | ||
| .join('') | ||
| .split(ES_HIGHLIGHT_POST_TAG) | ||
| .join(''); | ||
|
|
||
| const taggedHighlight = escapedHighlight | ||
| .split(ES_HIGHLIGHT_PRE_TAG) | ||
| .join(HTML_HIGHLIGHT_PRE_TAG) | ||
| .split(ES_HIGHLIGHT_POST_TAG) | ||
| .join(HTML_HIGHLIGHT_POST_TAG); | ||
|
|
||
| result = result.split(untaggedHighlight).join(taggedHighlight); | ||
| } | ||
|
|
||
| return result; | ||
| } | ||
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.
I'm a bit confused about the purpose of this function. Values run through an HTML formatter with
formatFieldValueshould already be HTML escaped since that's it's purpose. Can you help me understand why it's needed?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.
escapeAndPreserveHighlightTagswas introduced in#253210 as a defense measure since the values are rendered via
dangerouslySetInnerHTML. My PR didn't introduce it, only updated it to handle multiple highlights (it was previously limited to two mark tags). I don't think any logic changed since then, so I kept that fix wherever it was needed still.We can work on removing these temporary fixes once #259286 is completed.
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.
Thanks for explaining. I'm still not sure this function actually does anything tbh, but it doesn't seem to introduce new risk, so I'll leave it up to you folks to decide.