From a87b3d3123ffc57a34f747112da2d6adaaf572b8 Mon Sep 17 00:00:00 2001 From: Joe Reuter Date: Tue, 29 Jun 2021 10:19:50 +0200 Subject: [PATCH] [Lens] Do not persist time zone (#102735) --- .../indexpattern_datasource/loader.test.ts | 2 +- .../definitions/date_histogram.test.tsx | 135 +----------------- .../operations/definitions/date_histogram.tsx | 48 ++----- .../operations/layer_helpers.test.ts | 11 +- .../embeddable/lens_embeddable_factory.ts | 15 +- .../server/migrations/common_migrations.ts | 36 ++++- .../saved_object_migrations.test.ts | 92 ++++++++++++ .../migrations/saved_object_migrations.ts | 18 ++- .../plugins/lens/server/migrations/types.ts | 65 +++++++++ 9 files changed, 248 insertions(+), 174 deletions(-) diff --git a/x-pack/plugins/lens/public/indexpattern_datasource/loader.test.ts b/x-pack/plugins/lens/public/indexpattern_datasource/loader.test.ts index 192f3d3c0c5359..d58d9e593e101d 100644 --- a/x-pack/plugins/lens/public/indexpattern_datasource/loader.test.ts +++ b/x-pack/plugins/lens/public/indexpattern_datasource/loader.test.ts @@ -770,7 +770,7 @@ describe('loader', () => { label: 'My hist', operationType: 'date_histogram', params: { - interval: '1d', + interval: 'm', }, sourceField: 'timestamp', }, diff --git a/x-pack/plugins/lens/public/indexpattern_datasource/operations/definitions/date_histogram.test.tsx b/x-pack/plugins/lens/public/indexpattern_datasource/operations/definitions/date_histogram.test.tsx index 28325c8d2b6016..3b7554d8110f85 100644 --- a/x-pack/plugins/lens/public/indexpattern_datasource/operations/definitions/date_histogram.test.tsx +++ b/x-pack/plugins/lens/public/indexpattern_datasource/operations/definitions/date_histogram.test.tsx @@ -176,30 +176,6 @@ describe('date_histogram', () => { }); expect(column.params.interval).toEqual('auto'); }); - - it('should create column object with restrictions', () => { - const column = dateHistogramOperation.buildColumn({ - layer: { columns: {}, columnOrder: [], indexPatternId: '' }, - indexPattern: createMockedIndexPattern(), - field: { - name: 'timestamp', - displayName: 'timestampLabel', - type: 'date', - esTypes: ['date'], - aggregatable: true, - searchable: true, - aggregationRestrictions: { - date_histogram: { - agg: 'date_histogram', - time_zone: 'UTC', - calendar_interval: '1y', - }, - }, - }, - }); - expect(column.params.interval).toEqual('1y'); - expect(column.params.timeZone).toEqual('UTC'); - }); }); describe('toEsAggsFn', () => { @@ -223,7 +199,7 @@ describe('date_histogram', () => { ); }); - it('should not use normalized es interval for rollups', () => { + it('should use restricted time zone and omit use normalized es interval for rollups', () => { const esAggsFn = dateHistogramOperation.toEsAggsFn( layer.columns.col1 as DateHistogramIndexPatternColumn, 'col1', @@ -271,6 +247,7 @@ describe('date_histogram', () => { arguments: expect.objectContaining({ interval: ['42w'], field: ['timestamp'], + time_zone: ['UTC'], useNormalizedEsInterval: [false], }), }) @@ -320,114 +297,6 @@ describe('date_histogram', () => { }); }); - describe('transfer', () => { - it('should adjust interval and time zone params if that is necessary due to restrictions', () => { - const transferedColumn = dateHistogramOperation.transfer!( - { - dataType: 'date', - isBucketed: true, - label: '', - operationType: 'date_histogram', - sourceField: 'dateField', - params: { - interval: 'd', - }, - }, - { - title: '', - id: '', - hasRestrictions: true, - fields: [ - { - name: 'dateField', - displayName: 'dateField', - type: 'date', - aggregatable: true, - searchable: true, - aggregationRestrictions: { - date_histogram: { - agg: 'date_histogram', - time_zone: 'CET', - calendar_interval: 'w', - }, - }, - }, - ], - getFieldByName: getFieldByNameFactory([ - { - name: 'dateField', - displayName: 'dateField', - type: 'date', - aggregatable: true, - searchable: true, - aggregationRestrictions: { - date_histogram: { - agg: 'date_histogram', - time_zone: 'CET', - calendar_interval: 'w', - }, - }, - }, - ]), - } - ); - expect(transferedColumn).toEqual( - expect.objectContaining({ - params: { - interval: 'w', - timeZone: 'CET', - }, - }) - ); - }); - - it('should retain interval', () => { - const transferedColumn = dateHistogramOperation.transfer!( - { - dataType: 'date', - isBucketed: true, - label: '', - operationType: 'date_histogram', - sourceField: 'dateField', - params: { - interval: '20s', - }, - }, - { - title: '', - id: '', - hasRestrictions: false, - fields: [ - { - name: 'dateField', - displayName: 'dateField', - type: 'date', - aggregatable: true, - searchable: true, - }, - ], - getFieldByName: getFieldByNameFactory([ - { - name: 'dateField', - displayName: 'dateField', - type: 'date', - aggregatable: true, - searchable: true, - }, - ]), - } - ); - expect(transferedColumn).toEqual( - expect.objectContaining({ - params: { - interval: '20s', - timeZone: undefined, - }, - }) - ); - }); - }); - describe('param editor', () => { it('should render current value', () => { const updateLayerSpy = jest.fn(); diff --git a/x-pack/plugins/lens/public/indexpattern_datasource/operations/definitions/date_histogram.tsx b/x-pack/plugins/lens/public/indexpattern_datasource/operations/definitions/date_histogram.tsx index a645079c7b9f8f..0091c43601202b 100644 --- a/x-pack/plugins/lens/public/indexpattern_datasource/operations/definitions/date_histogram.tsx +++ b/x-pack/plugins/lens/public/indexpattern_datasource/operations/definitions/date_histogram.tsx @@ -45,7 +45,6 @@ export interface DateHistogramIndexPatternColumn extends FieldBasedIndexPatternC operationType: 'date_histogram'; params: { interval: string; - timeZone?: string; }; } @@ -106,12 +105,6 @@ export const dateHistogramOperation: OperationDefinition< }, getDefaultLabel: (column, indexPattern) => getSafeName(column.sourceField, indexPattern), buildColumn({ field }, columnParams) { - let interval = columnParams?.interval ?? autoInterval; - let timeZone: string | undefined; - if (field.aggregationRestrictions && field.aggregationRestrictions.date_histogram) { - interval = restrictedInterval(field.aggregationRestrictions) as string; - timeZone = field.aggregationRestrictions.date_histogram.time_zone; - } return { label: field.displayName, dataType: 'date', @@ -120,8 +113,7 @@ export const dateHistogramOperation: OperationDefinition< isBucketed: true, scale: 'interval', params: { - interval, - timeZone, + interval: columnParams?.interval ?? autoInterval, }, }; }, @@ -135,28 +127,6 @@ export const dateHistogramOperation: OperationDefinition< (!newField.aggregationRestrictions || newField.aggregationRestrictions.date_histogram) ); }, - transfer: (column, newIndexPattern) => { - const newField = newIndexPattern.getFieldByName(column.sourceField); - - if (newField?.aggregationRestrictions?.date_histogram) { - const restrictions = newField.aggregationRestrictions.date_histogram; - - return { - ...column, - params: { - ...column.params, - timeZone: restrictions.time_zone, - // TODO this rewrite logic is simplified - if the current interval is a multiple of - // the restricted interval, we could carry it over directly. However as the current - // UI does not allow to select multiples of an interval anyway, this is not included yet. - // If the UI allows to pick more complicated intervals, this should be re-visited. - interval: restrictedInterval(newField.aggregationRestrictions) as string, - }, - }; - } - - return column; - }, onFieldChange: (oldColumn, field) => { return { ...oldColumn, @@ -166,14 +136,24 @@ export const dateHistogramOperation: OperationDefinition< }, toEsAggsFn: (column, columnId, indexPattern) => { const usedField = indexPattern.getFieldByName(column.sourceField); + let timeZone: string | undefined; + let interval = column.params?.interval ?? autoInterval; + if ( + usedField && + usedField.aggregationRestrictions && + usedField.aggregationRestrictions.date_histogram + ) { + interval = restrictedInterval(usedField.aggregationRestrictions) as string; + timeZone = usedField.aggregationRestrictions.date_histogram.time_zone; + } return buildExpressionFunction('aggDateHistogram', { id: columnId, enabled: true, schema: 'segment', field: column.sourceField, - time_zone: column.params.timeZone, + time_zone: timeZone, useNormalizedEsInterval: !usedField?.aggregationRestrictions?.date_histogram, - interval: column.params.interval, + interval, drop_partials: false, min_doc_count: 0, extended_bounds: JSON.stringify({}), @@ -244,7 +224,7 @@ export const dateHistogramOperation: OperationDefinition< id="xpack.lens.indexPattern.dateHistogram.restrictedInterval" defaultMessage="Interval fixed to {intervalValue} due to aggregation restrictions." values={{ - intervalValue: currentColumn.params.interval, + intervalValue: restrictedInterval(field!.aggregationRestrictions), }} /> ) : ( 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 9eedae6d82d430..ef1de43ad06ed8 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 @@ -25,6 +25,7 @@ import { documentField } from '../document_field'; import { getFieldByNameFactory } from '../pure_helpers'; import { generateId } from '../../id_generator'; import { createMockedFullReference, createMockedManagedReference } from './mocks'; +import { IndexPatternColumn, OperationDefinition } from './definitions'; import { TinymathAST } from 'packages/kbn-tinymath'; jest.mock('../operations'); @@ -2642,7 +2643,15 @@ describe('state_helpers', () => { }); }); + // no operation currently requires this check, faking a transfer function to check whether the generic logic works it('should rewrite column params if that is necessary due to restrictions', () => { + operationDefinitionMap.date_histogram.transfer = ((oldColumn) => ({ + ...oldColumn, + params: { + ...oldColumn.params, + interval: 'w', + }, + })) as OperationDefinition['transfer']; const layer: IndexPatternLayer = { columnOrder: ['col1', 'col2'], columns: { @@ -2666,10 +2675,10 @@ describe('state_helpers', () => { ...layer.columns.col1, params: { interval: 'w', - timeZone: 'CET', }, }, }); + delete operationDefinitionMap.date_histogram.transfer; }); it('should remove operations referencing fields with wrong field types', () => { diff --git a/x-pack/plugins/lens/server/embeddable/lens_embeddable_factory.ts b/x-pack/plugins/lens/server/embeddable/lens_embeddable_factory.ts index ddc822f37b95bf..4f21378cc81155 100644 --- a/x-pack/plugins/lens/server/embeddable/lens_embeddable_factory.ts +++ b/x-pack/plugins/lens/server/embeddable/lens_embeddable_factory.ts @@ -8,8 +8,11 @@ import { EmbeddableRegistryDefinition } from 'src/plugins/embeddable/server'; import { SerializableState } from '../../../../../src/plugins/kibana_utils/common'; import { DOC_TYPE } from '../../common'; -import { commonRenameOperationsForFormula } from '../migrations/common_migrations'; -import { LensDocShapePre712 } from '../migrations/types'; +import { + commonRemoveTimezoneDateHistogramParam, + commonRenameOperationsForFormula, +} from '../migrations/common_migrations'; +import { LensDocShape713, LensDocShapePre712 } from '../migrations/types'; export const lensEmbeddableFactory = (): EmbeddableRegistryDefinition => { return { @@ -24,6 +27,14 @@ export const lensEmbeddableFactory = (): EmbeddableRegistryDefinition => { attributes: migratedLensState, } as unknown) as SerializableState; }, + '7.14.0': (state) => { + const lensState = (state as unknown) as { attributes: LensDocShape713 }; + const migratedLensState = commonRemoveTimezoneDateHistogramParam(lensState.attributes); + return ({ + ...lensState, + attributes: migratedLensState, + } as unknown) as SerializableState; + }, }, }; }; diff --git a/x-pack/plugins/lens/server/migrations/common_migrations.ts b/x-pack/plugins/lens/server/migrations/common_migrations.ts index 85055e471bac95..db19de7fd9c07f 100644 --- a/x-pack/plugins/lens/server/migrations/common_migrations.ts +++ b/x-pack/plugins/lens/server/migrations/common_migrations.ts @@ -6,7 +6,13 @@ */ import { cloneDeep } from 'lodash'; -import { LensDocShapePre712, OperationTypePre712, LensDocShapePost712 } from './types'; +import { + LensDocShapePre712, + OperationTypePre712, + LensDocShapePost712, + LensDocShape713, + LensDocShape714, +} from './types'; export const commonRenameOperationsForFormula = ( attributes: LensDocShapePre712 @@ -44,3 +50,31 @@ export const commonRenameOperationsForFormula = ( ); return newAttributes as LensDocShapePost712; }; + +export const commonRemoveTimezoneDateHistogramParam = ( + attributes: LensDocShape713 +): LensDocShape714 => { + const newAttributes = cloneDeep(attributes); + const datasourceLayers = newAttributes.state.datasourceStates.indexpattern.layers || {}; + (newAttributes as LensDocShapePost712).state.datasourceStates.indexpattern.layers = Object.fromEntries( + Object.entries(datasourceLayers).map(([layerId, layer]) => { + return [ + layerId, + { + ...layer, + columns: Object.fromEntries( + Object.entries(layer.columns).map(([columnId, column]) => { + if (column.operationType === 'date_histogram' && 'params' in column) { + const copy = { ...column, params: { ...column.params } }; + delete copy.params.timeZone; + return [columnId, copy]; + } + return [columnId, column]; + }) + ), + }, + ]; + }) + ); + return newAttributes as LensDocShapePost712; +}; diff --git a/x-pack/plugins/lens/server/migrations/saved_object_migrations.test.ts b/x-pack/plugins/lens/server/migrations/saved_object_migrations.test.ts index 5478d86e9b14c7..9daae1d184ab63 100644 --- a/x-pack/plugins/lens/server/migrations/saved_object_migrations.test.ts +++ b/x-pack/plugins/lens/server/migrations/saved_object_migrations.test.ts @@ -852,4 +852,96 @@ describe('Lens migrations', () => { validate(result2); }); }); + + describe('7.14.0 remove time zone from date histogram', () => { + const context = ({ log: { warning: () => {} } } as unknown) as SavedObjectMigrationContext; + const example = { + type: 'lens', + id: 'mocked-saved-object-id', + attributes: { + savedObjectId: '1', + title: 'MyRenamedOps', + description: '', + visualizationType: 'lnsXY', + state: { + datasourceStates: { + indexpattern: { + layers: { + '2': { + columns: { + '3': { + label: '@timestamp', + dataType: 'date', + operationType: 'date_histogram', + sourceField: '@timestamp', + isBucketed: true, + scale: 'interval', + params: { interval: 'auto', timeZone: 'Europe/Berlin' }, + }, + '4': { + label: '@timestamp', + dataType: 'date', + operationType: 'date_histogram', + sourceField: '@timestamp', + isBucketed: true, + scale: 'interval', + params: { interval: 'auto' }, + }, + '5': { + label: '@timestamp', + dataType: 'date', + operationType: 'my_unexpected_operation', + isBucketed: true, + scale: 'interval', + params: { timeZone: 'do not delete' }, + }, + }, + columnOrder: ['3', '4', '5'], + incompleteColumns: {}, + }, + }, + }, + }, + visualization: { + title: 'Empty XY chart', + legend: { isVisible: true, position: 'right' }, + valueLabels: 'hide', + preferredSeriesType: 'bar_stacked', + layers: [ + { + layerId: '5ab74ddc-93ca-44e2-9857-ecf85c86b53e', + accessors: [ + '5fea2a56-7b73-44b5-9a50-7f0c0c4f8fd0', + 'e5efca70-edb5-4d6d-a30a-79384066987e', + '7ffb7bde-4f42-47ab-b74d-1b4fd8393e0f', + ], + position: 'top', + seriesType: 'bar_stacked', + showGridlines: false, + xAccessor: '2e57a41e-5a52-42d3-877f-bd211d903ef8', + }, + ], + }, + query: { query: '', language: 'kuery' }, + filters: [], + }, + }, + }; + + it('should remove time zone param from date histogram', () => { + const result = migrations['7.14.0'](example, context) as ReturnType< + SavedObjectMigrationFn + >; + const layers = Object.values(result.attributes.state.datasourceStates.indexpattern.layers); + expect(layers.length).toBe(1); + const columns = Object.values(layers[0].columns); + expect(columns.length).toBe(3); + expect(columns[0].operationType).toEqual('date_histogram'); + expect((columns[0] as { params: {} }).params).toEqual({ interval: 'auto' }); + expect(columns[1].operationType).toEqual('date_histogram'); + expect((columns[1] as { params: {} }).params).toEqual({ interval: 'auto' }); + expect(columns[2].operationType).toEqual('my_unexpected_operation'); + expect((columns[2] as { params: {} }).params).toEqual({ timeZone: 'do not delete' }); + }); + }); }); diff --git a/x-pack/plugins/lens/server/migrations/saved_object_migrations.ts b/x-pack/plugins/lens/server/migrations/saved_object_migrations.ts index ba7004ba67a958..efcd6e2e6f3420 100644 --- a/x-pack/plugins/lens/server/migrations/saved_object_migrations.ts +++ b/x-pack/plugins/lens/server/migrations/saved_object_migrations.ts @@ -15,8 +15,11 @@ import { } from 'src/core/server'; import { Query, Filter } from 'src/plugins/data/public'; import { PersistableFilter } from '../../common'; -import { LensDocShapePost712, LensDocShapePre712 } from './types'; -import { commonRenameOperationsForFormula } from './common_migrations'; +import { LensDocShapePost712, LensDocShapePre712, LensDocShape713, LensDocShape714 } from './types'; +import { + commonRenameOperationsForFormula, + commonRemoveTimezoneDateHistogramParam, +} from './common_migrations'; interface LensDocShapePre710 { visualizationType: string | null; @@ -400,6 +403,16 @@ const renameOperationsForFormula: SavedObjectMigrationFn< }; }; +const removeTimezoneDateHistogramParam: SavedObjectMigrationFn = ( + doc +) => { + const newDoc = cloneDeep(doc); + return { + ...newDoc, + attributes: commonRemoveTimezoneDateHistogramParam(newDoc.attributes), + }; +}; + export const migrations: SavedObjectMigrationMap = { '7.7.0': removeInvalidAccessors, // The order of these migrations matter, since the timefield migration relies on the aggConfigs @@ -410,4 +423,5 @@ export const migrations: SavedObjectMigrationMap = { '7.12.0': transformTableState, '7.13.0': renameOperationsForFormula, '7.13.1': renameOperationsForFormula, // duplicate this migration in case a broken by value panel is added to the library + '7.14.0': removeTimezoneDateHistogramParam, }; diff --git a/x-pack/plugins/lens/server/migrations/types.ts b/x-pack/plugins/lens/server/migrations/types.ts index 38e079ff380516..035e1a86b86f8f 100644 --- a/x-pack/plugins/lens/server/migrations/types.ts +++ b/x-pack/plugins/lens/server/migrations/types.ts @@ -87,3 +87,68 @@ export interface LensDocShapePost712 { filters: Filter[]; }; } + +export type LensDocShape713 = Omit & { + state: Omit & { + datasourceStates: { + indexpattern: Omit< + LensDocShapePost712['state']['datasourceStates']['indexpattern'], + 'layers' + > & { + layers: Record< + string, + Omit< + LensDocShapePost712['state']['datasourceStates']['indexpattern']['layers'][string], + 'columns' + > & { + columns: Record< + string, + | { + operationType: OperationTypePost712; + } + | { + operationType: 'date_histogram'; + params: { + interval: string; + timeZone?: string; + }; + } + >; + } + >; + }; + }; + }; +}; + +export type LensDocShape714 = Omit & { + state: Omit & { + datasourceStates: { + indexpattern: Omit< + LensDocShapePost712['state']['datasourceStates']['indexpattern'], + 'layers' + > & { + layers: Record< + string, + Omit< + LensDocShapePost712['state']['datasourceStates']['indexpattern']['layers'][string], + 'columns' + > & { + columns: Record< + string, + | { + operationType: OperationTypePost712; + } + | { + operationType: 'date_histogram'; + params: { + interval: string; + }; + } + >; + } + >; + }; + }; + }; +};