Skip to content
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

[7.x] [Lens] Do not persist time zone (#102735) #103620

Merged
merged 1 commit into from
Jun 29, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -770,7 +770,7 @@ describe('loader', () => {
label: 'My hist',
operationType: 'date_histogram',
params: {
interval: '1d',
interval: 'm',
},
sourceField: 'timestamp',
},
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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', () => {
Expand All @@ -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',
Expand Down Expand Up @@ -271,6 +247,7 @@ describe('date_histogram', () => {
arguments: expect.objectContaining({
interval: ['42w'],
field: ['timestamp'],
time_zone: ['UTC'],
useNormalizedEsInterval: [false],
}),
})
Expand Down Expand Up @@ -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();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,6 @@ export interface DateHistogramIndexPatternColumn extends FieldBasedIndexPatternC
operationType: 'date_histogram';
params: {
interval: string;
timeZone?: string;
};
}

Expand Down Expand Up @@ -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',
Expand All @@ -120,8 +113,7 @@ export const dateHistogramOperation: OperationDefinition<
isBucketed: true,
scale: 'interval',
params: {
interval,
timeZone,
interval: columnParams?.interval ?? autoInterval,
},
};
},
Expand All @@ -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,
Expand All @@ -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<AggFunctionsMapping['aggDateHistogram']>('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({}),
Expand Down Expand Up @@ -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),
}}
/>
) : (
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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');
Expand Down Expand Up @@ -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<IndexPatternColumn, 'field'>['transfer'];
const layer: IndexPatternLayer = {
columnOrder: ['col1', 'col2'],
columns: {
Expand All @@ -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', () => {
Expand Down
15 changes: 13 additions & 2 deletions x-pack/plugins/lens/server/embeddable/lens_embeddable_factory.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand All @@ -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;
},
},
};
};
36 changes: 35 additions & 1 deletion x-pack/plugins/lens/server/migrations/common_migrations.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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;
};
Loading