From ba1e795b3f4341547d3acc4cf9df79646e2b412d Mon Sep 17 00:00:00 2001 From: Marco Liberati Date: Wed, 27 Jan 2021 14:23:47 +0100 Subject: [PATCH] [Lens] Fix indexpattern checks for missing references (#88840) Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> --- .../dimension_panel/reference_editor.test.tsx | 76 +++++++++++++++- .../dimension_panel/reference_editor.tsx | 42 ++++++--- .../indexpattern_datasource/indexpattern.tsx | 11 ++- .../public/indexpattern_datasource/mocks.ts | 78 +++++++++++++++- .../operations/definitions/index.ts | 4 +- .../operations/layer_helpers.test.ts | 89 +++++++++++++------ .../operations/layer_helpers.ts | 30 ++++--- .../public/indexpattern_datasource/utils.ts | 26 +++++- 8 files changed, 294 insertions(+), 62 deletions(-) diff --git a/x-pack/plugins/lens/public/indexpattern_datasource/dimension_panel/reference_editor.test.tsx b/x-pack/plugins/lens/public/indexpattern_datasource/dimension_panel/reference_editor.test.tsx index 0891dd27fcf17..ed1b695640922 100644 --- a/x-pack/plugins/lens/public/indexpattern_datasource/dimension_panel/reference_editor.test.tsx +++ b/x-pack/plugins/lens/public/indexpattern_datasource/dimension_panel/reference_editor.test.tsx @@ -13,7 +13,7 @@ import type { IUiSettingsClient, SavedObjectsClientContract, HttpSetup } from 'k import { IStorageWrapper } from 'src/plugins/kibana_utils/public'; import type { DataPublicPluginStart } from 'src/plugins/data/public'; import { OperationMetadata } from '../../types'; -import { createMockedIndexPattern } from '../mocks'; +import { createMockedIndexPattern, createMockedIndexPatternWithoutType } from '../mocks'; import { ReferenceEditor, ReferenceEditorProps } from './reference_editor'; import { insertOrReplaceColumn } from '../operations'; import { FieldSelect } from './field_select'; @@ -260,6 +260,48 @@ describe('reference editor', () => { ); }); + it("should show the sub-function as invalid if there's no field compatible with it", () => { + // This may happen for saved objects after changing the type of a field + wrapper = mount( + true, + }} + /> + ); + + const subFunctionSelect = wrapper + .find('[data-test-subj="indexPattern-reference-function"]') + .first(); + + expect(subFunctionSelect.prop('isInvalid')).toEqual(true); + expect(subFunctionSelect.prop('selectedOptions')).toEqual( + expect.arrayContaining([ + expect.objectContaining({ + 'data-test-subj': 'lns-indexPatternDimension-avg incompatible', + label: 'Average', + value: 'avg', + }), + ]) + ); + }); + it('should hide the function selector when using a field-only selection style', () => { wrapper = mount( { expect(fieldSelect.prop('markAllFieldsCompatible')).toEqual(true); }); + it('should show the FieldSelect as invalid if the selected field is missing', () => { + wrapper = mount( + true, + }} + /> + ); + + const fieldSelect = wrapper.find(FieldSelect); + expect(fieldSelect.prop('fieldIsInvalid')).toEqual(true); + expect(fieldSelect.prop('selectedField')).toEqual('missing'); + expect(fieldSelect.prop('selectedOperationType')).toEqual('avg'); + expect(fieldSelect.prop('incompleteOperation')).toBeUndefined(); + expect(fieldSelect.prop('markAllFieldsCompatible')).toEqual(false); + }); + it('should show the ParamEditor for functions that offer one', () => { wrapper = mount( value === incompleteInfo.operationType)!] + const selectedOption = incompleteOperation + ? [functionOptions.find(({ value }) => value === incompleteOperation)!] : column ? [functionOptions.find(({ value }) => value === column.operationType)!] : []; // If the operationType is incomplete, the user needs to select a field- so // the function is marked as valid. - const showOperationInvalid = !column && !Boolean(incompleteInfo?.operationType); + const showOperationInvalid = !column && !Boolean(incompleteOperation); // The field is invalid if the operation has been updated without a field, // or if we are in a field-only mode but empty state - const showFieldInvalid = - Boolean(incompleteInfo?.operationType) || (selectionStyle === 'field' && !column); + const showFieldInvalid = Boolean(incompleteOperation) || (selectionStyle === 'field' && !column); + // Check if the field still exists to protect from changes + const showFieldMissingInvalid = !currentIndexPattern.getFieldByName( + incompleteField ?? (column as FieldBasedIndexPatternColumn)?.sourceField + ); + + // what about a field changing type and becoming invalid? + // Let's say this change makes the indexpattern without any number field but the operation was set to a numeric operation. + // At this point the ComboBox will crash. + // Therefore check if the selectedOption is in functionOptions and in case fill it in as disabled option + const showSelectionFunctionInvalid = Boolean(selectedOption.length && selectedOption[0] == null); + if (showSelectionFunctionInvalid) { + const selectedOperationType = incompleteOperation || column.operationType; + const brokenFunctionOption = { + label: operationPanels[selectedOperationType].displayName, + value: selectedOperationType, + className: 'lnsIndexPatternDimensionEditor__operation', + 'data-test-subj': `lns-indexPatternDimension-${selectedOperationType} incompatible`, + }; + functionOptions.push(brokenFunctionOption); + selectedOption[0] = brokenFunctionOption; + } return (
@@ -216,7 +236,7 @@ export function ReferenceEditor(props: ReferenceEditorProps) { defaultMessage: 'Choose a sub-function', })} fullWidth - isInvalid={showOperationInvalid} + isInvalid={showOperationInvalid || showSelectionFunctionInvalid} > { @@ -258,11 +278,11 @@ export function ReferenceEditor(props: ReferenceEditorProps) { defaultMessage: 'Select a field', })} fullWidth - isInvalid={showFieldInvalid} + isInvalid={showFieldInvalid || showFieldMissingInvalid} labelAppend={labelAppend} > - (getErrorMessages(layer) ?? []).map((message) => ({ - shortMessage: '', // Not displayed currently - longMessage: message, - })) + (getErrorMessages(layer, state.indexPatterns[layer.indexPatternId]) ?? []).map( + (message) => ({ + shortMessage: '', // Not displayed currently + longMessage: message, + }) + ) ); // Single layer case, no need to explain more diff --git a/x-pack/plugins/lens/public/indexpattern_datasource/mocks.ts b/x-pack/plugins/lens/public/indexpattern_datasource/mocks.ts index d0cbcee61db6f..4aea9e8ac67a9 100644 --- a/x-pack/plugins/lens/public/indexpattern_datasource/mocks.ts +++ b/x-pack/plugins/lens/public/indexpattern_datasource/mocks.ts @@ -6,7 +6,83 @@ import { DragContextState } from '../drag_drop'; import { getFieldByNameFactory } from './pure_helpers'; -import type { IndexPattern } from './types'; +import type { IndexPattern, IndexPatternField } from './types'; + +export const createMockedIndexPatternWithoutType = ( + typeToFilter: IndexPatternField['type'] +): IndexPattern => { + const fields = [ + { + name: 'timestamp', + displayName: 'timestampLabel', + type: 'date', + aggregatable: true, + searchable: true, + }, + { + name: 'start_date', + displayName: 'start_date', + type: 'date', + aggregatable: true, + searchable: true, + }, + { + name: 'bytes', + displayName: 'bytes', + type: 'number', + aggregatable: true, + searchable: true, + }, + { + name: 'memory', + displayName: 'memory', + type: 'number', + aggregatable: true, + searchable: true, + }, + { + name: 'source', + displayName: 'source', + type: 'string', + aggregatable: true, + searchable: true, + esTypes: ['keyword'], + }, + { + name: 'unsupported', + displayName: 'unsupported', + type: 'geo', + aggregatable: true, + searchable: true, + }, + { + name: 'dest', + displayName: 'dest', + type: 'string', + aggregatable: true, + searchable: true, + esTypes: ['keyword'], + }, + { + name: 'scripted', + displayName: 'Scripted', + type: 'string', + searchable: true, + aggregatable: true, + scripted: true, + lang: 'painless', + script: '1234', + }, + ].filter(({ type }) => type !== typeToFilter); + return { + id: '1', + title: 'my-fake-index-pattern', + timeFieldName: 'timestamp', + hasRestrictions: false, + fields, + getFieldByName: getFieldByNameFactory(fields), + }; +}; export const createMockedIndexPattern = (): IndexPattern => { const fields = [ diff --git a/x-pack/plugins/lens/public/indexpattern_datasource/operations/definitions/index.ts b/x-pack/plugins/lens/public/indexpattern_datasource/operations/definitions/index.ts index 7dbc7d3b986a5..1cdaff53c5458 100644 --- a/x-pack/plugins/lens/public/indexpattern_datasource/operations/definitions/index.ts +++ b/x-pack/plugins/lens/public/indexpattern_datasource/operations/definitions/index.ts @@ -197,7 +197,7 @@ interface BaseOperationDefinitionProps { getErrorMessage?: ( layer: IndexPatternLayer, columnId: string, - indexPattern?: IndexPattern + indexPattern: IndexPattern ) => string[] | undefined; /* @@ -301,7 +301,7 @@ interface FieldBasedOperationDefinition { getErrorMessage: ( layer: IndexPatternLayer, columnId: string, - indexPattern?: IndexPattern + indexPattern: IndexPattern ) => string[] | undefined; } diff --git a/x-pack/plugins/lens/public/indexpattern_datasource/operations/layer_helpers.test.ts b/x-pack/plugins/lens/public/indexpattern_datasource/operations/layer_helpers.test.ts index c0e71dc1509e7..94cf13a5c50a4 100644 --- a/x-pack/plugins/lens/public/indexpattern_datasource/operations/layer_helpers.test.ts +++ b/x-pack/plugins/lens/public/indexpattern_datasource/operations/layer_helpers.test.ts @@ -2147,14 +2147,17 @@ describe('state_helpers', () => { it('should collect errors from metric-type operation definitions', () => { const mock = jest.fn().mockReturnValue(['error 1']); operationDefinitionMap.avg.getErrorMessage = mock; - const errors = getErrorMessages({ - indexPatternId: '1', - columnOrder: [], - columns: { - // @ts-expect-error invalid column - col1: { operationType: 'avg' }, + const errors = getErrorMessages( + { + indexPatternId: '1', + columnOrder: [], + columns: { + // @ts-expect-error invalid column + col1: { operationType: 'avg' }, + }, }, - }); + indexPattern + ); expect(mock).toHaveBeenCalled(); expect(errors).toHaveLength(1); }); @@ -2162,15 +2165,18 @@ describe('state_helpers', () => { it('should collect errors from reference-type operation definitions', () => { const mock = jest.fn().mockReturnValue(['error 1']); operationDefinitionMap.testReference.getErrorMessage = mock; - const errors = getErrorMessages({ - indexPatternId: '1', - columnOrder: [], - columns: { - col1: - // @ts-expect-error not statically analyzed - { operationType: 'testReference', references: [] }, + const errors = getErrorMessages( + { + indexPatternId: '1', + columnOrder: [], + columns: { + col1: + // @ts-expect-error not statically analyzed + { operationType: 'testReference', references: [] }, + }, }, - }); + indexPattern + ); expect(mock).toHaveBeenCalled(); expect(errors).toHaveLength(1); }); @@ -2184,24 +2190,55 @@ describe('state_helpers', () => { getErrorMessage: incompleteRef, }; - const errors = getErrorMessages({ - indexPatternId: '1', - columnOrder: [], - columns: { - col1: + const errors = getErrorMessages( + { + indexPatternId: '1', + columnOrder: [], + columns: { + col1: + // @ts-expect-error not statically analyzed + { operationType: 'testReference', references: [] }, + }, + incompleteColumns: { // @ts-expect-error not statically analyzed - { operationType: 'testReference', references: [] }, - }, - incompleteColumns: { - // @ts-expect-error not statically analyzed - col1: { operationType: 'testIncompleteReference' }, + col1: { operationType: 'testIncompleteReference' }, + }, }, - }); + indexPattern + ); expect(savedRef).not.toHaveBeenCalled(); expect(incompleteRef).toHaveBeenCalled(); expect(errors).toBeUndefined(); delete operationDefinitionMap.testIncompleteReference; }); + + it('should forward the indexpattern when available', () => { + const mock = jest.fn(); + operationDefinitionMap.testReference.getErrorMessage = mock; + getErrorMessages( + { + indexPatternId: '1', + columnOrder: [], + columns: { + col1: + // @ts-expect-error not statically analyzed + { operationType: 'testReference', references: [] }, + }, + }, + indexPattern + ); + expect(mock).toHaveBeenCalledWith( + { + indexPatternId: '1', + columnOrder: [], + columns: { + col1: { operationType: 'testReference', references: [] }, + }, + }, + 'col1', + indexPattern + ); + }); }); }); diff --git a/x-pack/plugins/lens/public/indexpattern_datasource/operations/layer_helpers.ts b/x-pack/plugins/lens/public/indexpattern_datasource/operations/layer_helpers.ts index d8244f3902a6e..10618cc754556 100644 --- a/x-pack/plugins/lens/public/indexpattern_datasource/operations/layer_helpers.ts +++ b/x-pack/plugins/lens/public/indexpattern_datasource/operations/layer_helpers.ts @@ -864,18 +864,24 @@ export function updateLayerIndexPattern( * - All column references are valid * - All prerequisites are met */ -export function getErrorMessages(layer: IndexPatternLayer): string[] | undefined { - const errors: string[] = []; - Object.entries(layer.columns).forEach(([columnId, column]) => { - // If we're transitioning to another operation, check for "new" incompleteColumns rather - // than "old" saved operation on the layer - const columnFinalRef = - layer.incompleteColumns?.[columnId]?.operationType || column.operationType; - const def = operationDefinitionMap[columnFinalRef]; - if (def.getErrorMessage) { - errors.push(...(def.getErrorMessage(layer, columnId) ?? [])); - } - }); +export function getErrorMessages( + layer: IndexPatternLayer, + indexPattern: IndexPattern +): string[] | undefined { + const errors: string[] = Object.entries(layer.columns) + .flatMap(([columnId, column]) => { + // If we're transitioning to another operation, check for "new" incompleteColumns rather + // than "old" saved operation on the layer + const columnFinalRef = + layer.incompleteColumns?.[columnId]?.operationType || column.operationType; + const def = operationDefinitionMap[columnFinalRef]; + + if (def.getErrorMessage) { + return def.getErrorMessage(layer, columnId, indexPattern); + } + }) + // remove the undefined values + .filter((v: string | undefined): v is string => v != null); return errors.length ? errors : undefined; } diff --git a/x-pack/plugins/lens/public/indexpattern_datasource/utils.ts b/x-pack/plugins/lens/public/indexpattern_datasource/utils.ts index 57cc4abeb723a..b5a4905a29738 100644 --- a/x-pack/plugins/lens/public/indexpattern_datasource/utils.ts +++ b/x-pack/plugins/lens/public/indexpattern_datasource/utils.ts @@ -7,9 +7,10 @@ import { DataType } from '../types'; import { IndexPattern, IndexPatternLayer } from './types'; import { DraggedField } from './indexpattern'; -import { +import type { BaseIndexPatternColumn, FieldBasedIndexPatternColumn, + ReferenceBasedIndexPatternColumn, } from './operations/definitions/column_types'; import { operationDefinitionMap, IndexPatternColumn } from './operations'; @@ -53,12 +54,29 @@ export function isColumnInvalid( if (!column) return; const operationDefinition = column.operationType && operationDefinitionMap[column.operationType]; - return !!( - operationDefinition.getErrorMessage && - operationDefinition.getErrorMessage(layer, columnId, indexPattern) + // check also references for errors + const referencesHaveErrors = + true && + 'references' in column && + Boolean(getReferencesErrors(layer, column, indexPattern).filter(Boolean).length); + + return ( + !!operationDefinition.getErrorMessage?.(layer, columnId, indexPattern) || referencesHaveErrors ); } +function getReferencesErrors( + layer: IndexPatternLayer, + column: ReferenceBasedIndexPatternColumn, + indexPattern: IndexPattern +) { + return column.references?.map((referenceId: string) => { + const referencedOperation = layer.columns[referenceId]?.operationType; + const referencedDefinition = operationDefinitionMap[referencedOperation]; + return referencedDefinition?.getErrorMessage?.(layer, referenceId, indexPattern); + }); +} + export function fieldIsInvalid(column: IndexPatternColumn | undefined, indexPattern: IndexPattern) { if (!column || !hasField(column)) { return false;