From 443fd5ba3525d37e80aaf112643d9c2820983d11 Mon Sep 17 00:00:00 2001 From: Davis McPhee Date: Wed, 26 Feb 2025 12:26:06 -0400 Subject: [PATCH] [Field Formatters] Fix field format `convert()` return value (#211342) ## Summary It was [pointed out](https://github.com/elastic/kibana/pull/209824#discussion_r1955981955) during a recent review that the field format `convert()` value does some odd casting. It looks like this probably isn't necessary since `converter` should never be undefined, and we can just return the result directly. ### Checklist - [ ] Any text added follows [EUI's writing guidelines](https://elastic.github.io/eui/#/guidelines/writing), uses sentence case text and includes [i18n support](https://github.com/elastic/kibana/blob/main/src/platform/packages/shared/kbn-i18n/README.md) - [ ] [Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html) was added for features that require explanation or tutorials - [ ] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios - [ ] If a plugin configuration key changed, check if it needs to be allowlisted in the cloud and added to the [docker list](https://github.com/elastic/kibana/blob/main/src/dev/build/tasks/os_packages/docker_generator/resources/base/bin/kibana-docker) - [ ] This was checked for breaking HTTP API changes, and any breaking changes have been approved by the breaking-change committee. The `release_note:breaking` label should be applied in these situations. - [ ] [Flaky Test Runner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1) was used on any tests changed - [ ] The PR description includes the appropriate Release Notes section, and the correct `release_note:*` label is applied per the [guidelines](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process) --------- Co-authored-by: Elastic Machine --- .../components/summary_column/summary_column.test.tsx | 11 ++++++++++- .../logs/components/summary_column/utils.tsx | 4 +--- .../components/data_types/logs/service_name_cell.tsx | 4 +--- .../shared/field_formats/common/field_format.ts | 11 ++--------- 4 files changed, 14 insertions(+), 16 deletions(-) diff --git a/src/platform/packages/shared/kbn-discover-contextual-components/src/data_types/logs/components/summary_column/summary_column.test.tsx b/src/platform/packages/shared/kbn-discover-contextual-components/src/data_types/logs/components/summary_column/summary_column.test.tsx index 93e484145be70..53e90a72d0baa 100644 --- a/src/platform/packages/shared/kbn-discover-contextual-components/src/data_types/logs/components/summary_column/summary_column.test.tsx +++ b/src/platform/packages/shared/kbn-discover-contextual-components/src/data_types/logs/components/summary_column/summary_column.test.tsx @@ -22,6 +22,7 @@ import { sharePluginMock } from '@kbn/share-plugin/public/mocks'; import { coreMock as corePluginMock } from '@kbn/core/public/mocks'; import { DataTableRecord, buildDataTableRecord } from '@kbn/discover-utils'; import { dataViewMock } from '@kbn/discover-utils/src/__mocks__/data_view'; +import type { IFieldFormatsRegistry } from '@kbn/field-formats-plugin/common'; jest.mock('@elastic/eui', () => ({ ...jest.requireActual('@elastic/eui'), @@ -46,7 +47,15 @@ const getSummaryProps = ( isDetails: false, row: record, dataView: dataViewMock, - fieldFormats: fieldFormatsMock, + fieldFormats: { + ...fieldFormatsMock, + getDefaultInstance: jest + .fn() + .mockImplementation((...params: Parameters) => ({ + ...fieldFormatsMock.getDefaultInstance(...params), + convert: jest.fn().mockImplementation((t: string) => String(t)), + })), + }, setCellProps: () => {}, closePopover: () => {}, density: DataGridDensity.COMPACT, diff --git a/src/platform/packages/shared/kbn-discover-contextual-components/src/data_types/logs/components/summary_column/utils.tsx b/src/platform/packages/shared/kbn-discover-contextual-components/src/data_types/logs/components/summary_column/utils.tsx index ecc9a67eb7636..bda5a286c6633 100644 --- a/src/platform/packages/shared/kbn-discover-contextual-components/src/data_types/logs/components/summary_column/utils.tsx +++ b/src/platform/packages/shared/kbn-discover-contextual-components/src/data_types/logs/components/summary_column/utils.tsx @@ -181,9 +181,7 @@ export const createResourceFields = ({ return { name, rawValue: resourceDoc[name], - // TODO: formatFieldValue doesn't actually return a string in certain circumstances, change - // this line below once it does. - value: typeof value === 'string' ? value : `${value}`, + value, ResourceBadge: getResourceBadgeComponent(name, core, share), Icon: getResourceBadgeIcon(name, resourceDoc), }; diff --git a/src/platform/plugins/shared/discover/public/components/data_types/logs/service_name_cell.tsx b/src/platform/plugins/shared/discover/public/components/data_types/logs/service_name_cell.tsx index 48dca722785e4..85caa41cb3d1e 100644 --- a/src/platform/plugins/shared/discover/public/components/data_types/logs/service_name_cell.tsx +++ b/src/platform/plugins/shared/discover/public/components/data_types/logs/service_name_cell.tsx @@ -58,9 +58,7 @@ export const getServiceNameCell = onFilter={actions.addFilter} icon={getIcon} rawValue={serviceNameValue} - // TODO: formatFieldValue doesn't actually return a string in certain circumstances, change - // this line below once it does. - value={typeof value === 'string' ? value : `${value}`} + value={value} property={serviceNameField} core={core} share={share} diff --git a/src/platform/plugins/shared/field_formats/common/field_format.ts b/src/platform/plugins/shared/field_formats/common/field_format.ts index d920e5b88f079..5892533c9cdfa 100644 --- a/src/platform/plugins/shared/field_formats/common/field_format.ts +++ b/src/platform/plugins/shared/field_formats/common/field_format.ts @@ -117,14 +117,7 @@ export abstract class FieldFormat { contentType: FieldFormatsContentType = DEFAULT_CONTEXT_TYPE, options?: HtmlContextTypeOptions | TextContextTypeOptions ): string { - const converter = this.getConverterFor(contentType); - - if (converter) { - return converter.call(this, value, options); - } - - // TODO: should be "return `${value}`;", but might be a breaking change - return value as string; + return this.getConverterFor(contentType).call(this, value, options); } /** @@ -140,7 +133,7 @@ export abstract class FieldFormat { this.convertObject = this.setupContentType(); } - return this.convertObject[contentType]; + return this.convertObject[contentType] ?? this.convertObject.text; } /**