From 420ccffcd63f8f46073cf454592f6ce1b3aba653 Mon Sep 17 00:00:00 2001 From: Nathan Reese Date: Thu, 16 Apr 2020 19:22:02 -0600 Subject: [PATCH] [Maps] fix term join agg key collision (#63324) * [Maps] fix term join agg key collision * fix tslint and jest errors * fix join functional test * revert LayerDescriptor union and cast to VectorLayerDescriptor instead * move getJoinKey out of constants and into its own file Co-authored-by: Elastic Machine --- .../plugins/maps/common/get_join_key.ts | 8 + .../common/migrations/join_agg_key.test.ts | 140 ++++++++++++++++++ .../maps/common/migrations/join_agg_key.ts | 121 +++++++++++++++ x-pack/legacy/plugins/maps/migrations.js | 9 ++ x-pack/plugins/maps/common/constants.ts | 8 +- x-pack/plugins/maps/common/get_join_key.ts | 22 +++ .../public/layers/joins/inner_join.test.js | 2 +- .../sources/es_agg_source/es_agg_source.js | 4 +- .../sources/es_term_source/es_term_source.js | 17 +-- .../es_term_source/es_term_source.test.js | 15 +- .../api_integration/apis/maps/migrations.js | 2 +- x-pack/test/functional/apps/maps/joins.js | 2 +- .../functional/apps/maps/mapbox_styles.js | 8 +- 13 files changed, 325 insertions(+), 33 deletions(-) create mode 100644 x-pack/legacy/plugins/maps/common/get_join_key.ts create mode 100644 x-pack/legacy/plugins/maps/common/migrations/join_agg_key.test.ts create mode 100644 x-pack/legacy/plugins/maps/common/migrations/join_agg_key.ts create mode 100644 x-pack/plugins/maps/common/get_join_key.ts diff --git a/x-pack/legacy/plugins/maps/common/get_join_key.ts b/x-pack/legacy/plugins/maps/common/get_join_key.ts new file mode 100644 index 000000000000..004f12ca08d2 --- /dev/null +++ b/x-pack/legacy/plugins/maps/common/get_join_key.ts @@ -0,0 +1,8 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the Elastic License; + * you may not use this file except in compliance with the Elastic License. + */ + +// eslint-disable-next-line @kbn/eslint/no-restricted-paths +export * from '../../../../plugins/maps/common/get_join_key'; diff --git a/x-pack/legacy/plugins/maps/common/migrations/join_agg_key.test.ts b/x-pack/legacy/plugins/maps/common/migrations/join_agg_key.test.ts new file mode 100644 index 000000000000..d92bf0654143 --- /dev/null +++ b/x-pack/legacy/plugins/maps/common/migrations/join_agg_key.test.ts @@ -0,0 +1,140 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the Elastic License; + * you may not use this file except in compliance with the Elastic License. + */ + +import { LAYER_TYPE } from '../constants'; +import { migrateJoinAggKey } from './join_agg_key'; + +describe('migrateJoinAggKey', () => { + const joins = [ + { + leftField: 'machine.os', + right: { + id: '9055b4aa-136a-4b6d-90ab-9f94ccfe5eb5', + indexPatternTitle: 'kibana_sample_data_logs', + term: 'machine.os.keyword', + metrics: [ + { + type: 'avg', + field: 'bytes', + }, + { + type: 'count', + }, + ], + whereQuery: { + query: 'bytes > 10000', + language: 'kuery', + }, + indexPatternRefName: 'layer_1_join_0_index_pattern', + }, + }, + { + leftField: 'machine.os', + right: { + id: '9a7f4e71-9500-4512-82f1-b7eaee3d87ff', + indexPatternTitle: 'kibana_sample_data_logs', + term: 'machine.os.keyword', + whereQuery: { + query: 'bytes < 10000', + language: 'kuery', + }, + metrics: [ + { + type: 'avg', + field: 'bytes', + }, + ], + indexPatternRefName: 'layer_1_join_1_index_pattern', + }, + }, + ]; + + test('Should handle missing layerListJSON attribute', () => { + const attributes = { + title: 'my map', + }; + expect(migrateJoinAggKey({ attributes })).toEqual({ + title: 'my map', + }); + }); + + test('Should migrate vector styles from legacy join agg key to new join agg key', () => { + const layerListJSON = JSON.stringify([ + { + type: LAYER_TYPE.VECTOR, + joins, + style: { + properties: { + fillColor: { + type: 'DYNAMIC', + options: { + color: 'Blues', + colorCategory: 'palette_0', + field: { + name: + '__kbnjoin__avg_of_bytes_groupby_kibana_sample_data_logs.machine.os.keyword', + origin: 'join', + }, + fieldMetaOptions: { + isEnabled: true, + sigma: 3, + }, + type: 'ORDINAL', + }, + }, + lineColor: { + type: 'DYNAMIC', + options: { + color: 'Blues', + colorCategory: 'palette_0', + field: { + name: '__kbnjoin__count_groupby_kibana_sample_data_logs.machine.os.keyword', + origin: 'join', + }, + fieldMetaOptions: { + isEnabled: true, + sigma: 3, + }, + type: 'ORDINAL', + }, + }, + lineWidth: { + type: 'DYNAMIC', + options: { + color: 'Blues', + colorCategory: 'palette_0', + field: { + name: 'mySourceField', + origin: 'source', + }, + fieldMetaOptions: { + isEnabled: true, + sigma: 3, + }, + type: 'ORDINAL', + }, + }, + }, + }, + }, + ]); + const attributes = { + title: 'my map', + layerListJSON, + }; + const { layerListJSON: migratedLayerListJSON } = migrateJoinAggKey({ attributes }); + const migratedLayerList = JSON.parse(migratedLayerListJSON!); + expect(migratedLayerList[0].style.properties.fillColor.options.field.name).toBe( + '__kbnjoin__avg_of_bytes__9055b4aa-136a-4b6d-90ab-9f94ccfe5eb5' + ); + expect(migratedLayerList[0].style.properties.lineColor.options.field.name).toBe( + '__kbnjoin__count__9055b4aa-136a-4b6d-90ab-9f94ccfe5eb5' + ); + expect(migratedLayerList[0].style.properties.lineWidth.options.field.name).toBe( + 'mySourceField' + ); + }); +}); diff --git a/x-pack/legacy/plugins/maps/common/migrations/join_agg_key.ts b/x-pack/legacy/plugins/maps/common/migrations/join_agg_key.ts new file mode 100644 index 000000000000..29661aedb550 --- /dev/null +++ b/x-pack/legacy/plugins/maps/common/migrations/join_agg_key.ts @@ -0,0 +1,121 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the Elastic License; + * you may not use this file except in compliance with the Elastic License. + */ + +import _ from 'lodash'; +import { + AGG_DELIMITER, + AGG_TYPE, + FIELD_ORIGIN, + JOIN_FIELD_NAME_PREFIX, + LAYER_TYPE, + VECTOR_STYLES, +} from '../constants'; +import { getJoinAggKey } from '../get_join_key'; +import { + AggDescriptor, + JoinDescriptor, + LayerDescriptor, + VectorLayerDescriptor, +} from '../descriptor_types'; +import { MapSavedObjectAttributes } from '../../../../../plugins/maps/common/map_saved_object_type'; + +const GROUP_BY_DELIMITER = '_groupby_'; + +function getLegacyAggKey({ + aggType, + aggFieldName, + indexPatternTitle, + termFieldName, +}: { + aggType: AGG_TYPE; + aggFieldName?: string; + indexPatternTitle: string; + termFieldName: string; +}): string { + const metricKey = + aggType !== AGG_TYPE.COUNT ? `${aggType}${AGG_DELIMITER}${aggFieldName}` : aggType; + return `${JOIN_FIELD_NAME_PREFIX}${metricKey}${GROUP_BY_DELIMITER}${indexPatternTitle}.${termFieldName}`; +} + +function parseLegacyAggKey(legacyAggKey: string): { aggType: AGG_TYPE; aggFieldName?: string } { + const groupBySplit = legacyAggKey + .substring(JOIN_FIELD_NAME_PREFIX.length) + .split(GROUP_BY_DELIMITER); + const metricKey = groupBySplit[0]; + const metricKeySplit = metricKey.split(AGG_DELIMITER); + return { + aggType: metricKeySplit[0] as AGG_TYPE, + aggFieldName: metricKeySplit.length === 2 ? metricKeySplit[1] : undefined, + }; +} + +export function migrateJoinAggKey({ + attributes, +}: { + attributes: MapSavedObjectAttributes; +}): MapSavedObjectAttributes { + if (!attributes || !attributes.layerListJSON) { + return attributes; + } + + const layerList: LayerDescriptor[] = JSON.parse(attributes.layerListJSON); + layerList.forEach((layerDescriptor: LayerDescriptor) => { + if ( + layerDescriptor.type === LAYER_TYPE.VECTOR || + layerDescriptor.type === LAYER_TYPE.BLENDED_VECTOR + ) { + const vectorLayerDescriptor = layerDescriptor as VectorLayerDescriptor; + + if ( + !vectorLayerDescriptor.style || + !vectorLayerDescriptor.joins || + vectorLayerDescriptor.joins.length === 0 + ) { + return; + } + + const legacyJoinFields = new Map(); + vectorLayerDescriptor.joins.forEach((joinDescriptor: JoinDescriptor) => { + _.get(joinDescriptor, 'right.metrics', []).forEach((aggDescriptor: AggDescriptor) => { + const legacyAggKey = getLegacyAggKey({ + aggType: aggDescriptor.type, + aggFieldName: aggDescriptor.field, + indexPatternTitle: _.get(joinDescriptor, 'right.indexPatternTitle', ''), + termFieldName: _.get(joinDescriptor, 'right.term', ''), + }); + // The legacy getAggKey implemenation has a naming collision bug where + // aggType, aggFieldName, indexPatternTitle, and termFieldName would result in the identical aggKey. + // The VectorStyle implemenation used the first matching join descriptor + // so, in the event of a name collision, the first join descriptor will be used here as well. + if (!legacyJoinFields.has(legacyAggKey)) { + legacyJoinFields.set(legacyAggKey, joinDescriptor); + } + }); + }); + + Object.keys(vectorLayerDescriptor.style.properties).forEach(key => { + const style: any = vectorLayerDescriptor.style!.properties[key as VECTOR_STYLES]; + if (_.get(style, 'options.field.origin') === FIELD_ORIGIN.JOIN) { + const joinDescriptor = legacyJoinFields.get(style.options.field.name); + if (joinDescriptor) { + const { aggType, aggFieldName } = parseLegacyAggKey(style.options.field.name); + // Update legacy join agg key to new join agg key + style.options.field.name = getJoinAggKey({ + aggType, + aggFieldName, + rightSourceId: joinDescriptor.right.id!, + }); + } + } + }); + } + }); + + return { + ...attributes, + layerListJSON: JSON.stringify(layerList), + }; +} diff --git a/x-pack/legacy/plugins/maps/migrations.js b/x-pack/legacy/plugins/maps/migrations.js index 6a1f5bc93749..a8e69eef7a02 100644 --- a/x-pack/legacy/plugins/maps/migrations.js +++ b/x-pack/legacy/plugins/maps/migrations.js @@ -11,6 +11,7 @@ import { moveApplyGlobalQueryToSources } from './common/migrations/move_apply_gl import { addFieldMetaOptions } from './common/migrations/add_field_meta_options'; import { migrateSymbolStyleDescriptor } from './common/migrations/migrate_symbol_style_descriptor'; import { migrateUseTopHitsToScalingType } from './common/migrations/scaling_type'; +import { migrateJoinAggKey } from './common/migrations/join_agg_key'; export const migrations = { map: { @@ -57,5 +58,13 @@ export const migrations = { attributes: attributesPhase2, }; }, + '7.8.0': doc => { + const attributes = migrateJoinAggKey(doc); + + return { + ...doc, + attributes, + }; + }, }, }; diff --git a/x-pack/plugins/maps/common/constants.ts b/x-pack/plugins/maps/common/constants.ts index 9808a62852a9..a4006732224c 100644 --- a/x-pack/plugins/maps/common/constants.ts +++ b/x-pack/plugins/maps/common/constants.ts @@ -1,9 +1,3 @@ -/* - * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one - * or more contributor license agreements. Licensed under the Elastic License; - * you may not use this file except in compliance with the Elastic License. - */ - /* * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one * or more contributor license agreements. Licensed under the Elastic License; @@ -75,6 +69,7 @@ export enum FIELD_ORIGIN { SOURCE = 'source', JOIN = 'join', } +export const JOIN_FIELD_NAME_PREFIX = '__kbnjoin__'; export const SOURCE_DATA_ID_ORIGIN = 'source'; export const META_ID_ORIGIN_SUFFIX = 'meta'; @@ -132,6 +127,7 @@ export enum DRAW_TYPE { POLYGON = 'POLYGON', } +export const AGG_DELIMITER = '_of_'; export enum AGG_TYPE { AVG = 'avg', COUNT = 'count', diff --git a/x-pack/plugins/maps/common/get_join_key.ts b/x-pack/plugins/maps/common/get_join_key.ts new file mode 100644 index 000000000000..f1ee95126b9a --- /dev/null +++ b/x-pack/plugins/maps/common/get_join_key.ts @@ -0,0 +1,22 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the Elastic License; + * you may not use this file except in compliance with the Elastic License. + */ + +import { AGG_DELIMITER, AGG_TYPE, JOIN_FIELD_NAME_PREFIX } from './constants'; + +// function in common since its needed by migration +export function getJoinAggKey({ + aggType, + aggFieldName, + rightSourceId, +}: { + aggType: AGG_TYPE; + aggFieldName?: string; + rightSourceId: string; +}) { + const metricKey = + aggType !== AGG_TYPE.COUNT ? `${aggType}${AGG_DELIMITER}${aggFieldName}` : aggType; + return `${JOIN_FIELD_NAME_PREFIX}${metricKey}__${rightSourceId}`; +} diff --git a/x-pack/plugins/maps/public/layers/joins/inner_join.test.js b/x-pack/plugins/maps/public/layers/joins/inner_join.test.js index 65c37860ffa1..f197a67becfa 100644 --- a/x-pack/plugins/maps/public/layers/joins/inner_join.test.js +++ b/x-pack/plugins/maps/public/layers/joins/inner_join.test.js @@ -35,7 +35,7 @@ const leftJoin = new InnerJoin( }, mockSource ); -const COUNT_PROPERTY_NAME = '__kbnjoin__count_groupby_kibana_sample_data_logs.geo.dest'; +const COUNT_PROPERTY_NAME = '__kbnjoin__count__d3625663-5b34-4d50-a784-0d743f676a0c'; describe('joinPropertiesToFeature', () => { it('Should add join property to features in feature collection', () => { diff --git a/x-pack/plugins/maps/public/layers/sources/es_agg_source/es_agg_source.js b/x-pack/plugins/maps/public/layers/sources/es_agg_source/es_agg_source.js index c6197f137f21..58c56fe32f76 100644 --- a/x-pack/plugins/maps/public/layers/sources/es_agg_source/es_agg_source.js +++ b/x-pack/plugins/maps/public/layers/sources/es_agg_source/es_agg_source.js @@ -7,16 +7,14 @@ import { i18n } from '@kbn/i18n'; import { AbstractESSource } from '../es_source'; import { esAggFieldsFactory } from '../../fields/es_agg_field'; - import { + AGG_DELIMITER, AGG_TYPE, COUNT_PROP_LABEL, COUNT_PROP_NAME, FIELD_ORIGIN, } from '../../../../common/constants'; -export const AGG_DELIMITER = '_of_'; - export class AbstractESAggSource extends AbstractESSource { constructor(descriptor, inspectorAdapters) { super(descriptor, inspectorAdapters); diff --git a/x-pack/plugins/maps/public/layers/sources/es_term_source/es_term_source.js b/x-pack/plugins/maps/public/layers/sources/es_term_source/es_term_source.js index 826197cc4fec..cb07bb0e7d2e 100644 --- a/x-pack/plugins/maps/public/layers/sources/es_term_source/es_term_source.js +++ b/x-pack/plugins/maps/public/layers/sources/es_term_source/es_term_source.js @@ -7,15 +7,14 @@ import _ from 'lodash'; import { i18n } from '@kbn/i18n'; -import { DEFAULT_MAX_BUCKETS_LIMIT, FIELD_ORIGIN, AGG_TYPE } from '../../../../common/constants'; +import { AGG_TYPE, DEFAULT_MAX_BUCKETS_LIMIT, FIELD_ORIGIN } from '../../../../common/constants'; +import { getJoinAggKey } from '../../../../common/get_join_key'; import { ESDocField } from '../../fields/es_doc_field'; -import { AbstractESAggSource, AGG_DELIMITER } from '../es_agg_source'; +import { AbstractESAggSource } from '../es_agg_source'; import { getField, addFieldToDSL, extractPropertiesFromBucket } from '../../util/es_agg_utils'; const TERMS_AGG_NAME = 'join'; -const FIELD_NAME_PREFIX = '__kbnjoin__'; -const GROUP_BY_DELIMITER = '_groupby_'; const TERMS_BUCKET_KEYS_TO_IGNORE = ['key', 'doc_count']; export function extractPropertiesMap(rawEsData, countPropertyName) { @@ -64,11 +63,11 @@ export class ESTermSource extends AbstractESAggSource { } getAggKey(aggType, fieldName) { - const metricKey = - aggType !== AGG_TYPE.COUNT ? `${aggType}${AGG_DELIMITER}${fieldName}` : aggType; - return `${FIELD_NAME_PREFIX}${metricKey}${GROUP_BY_DELIMITER}${ - this._descriptor.indexPatternTitle - }.${this._termField.getName()}`; + return getJoinAggKey({ + aggType, + aggFieldName: fieldName, + rightSourceId: this._descriptor.id, + }); } getAggLabel(aggType, fieldName) { diff --git a/x-pack/plugins/maps/public/layers/sources/es_term_source/es_term_source.test.js b/x-pack/plugins/maps/public/layers/sources/es_term_source/es_term_source.test.js index b6cd3b670d3c..14eb39180a6b 100644 --- a/x-pack/plugins/maps/public/layers/sources/es_term_source/es_term_source.test.js +++ b/x-pack/plugins/maps/public/layers/sources/es_term_source/es_term_source.test.js @@ -32,33 +32,32 @@ const metricExamples = [ describe('getMetricFields', () => { it('should override name and label of count metric', async () => { const source = new ESTermSource({ + id: '1234', indexPatternTitle: indexPatternTitle, term: termFieldName, }); const metrics = source.getMetricFields(); - expect(metrics[0].getName()).toEqual('__kbnjoin__count_groupby_myIndex.myTermField'); + expect(metrics[0].getName()).toEqual('__kbnjoin__count__1234'); expect(await metrics[0].getLabel()).toEqual('Count of myIndex'); }); it('should override name and label of sum metric', async () => { const source = new ESTermSource({ + id: '1234', indexPatternTitle: indexPatternTitle, term: termFieldName, metrics: metricExamples, }); const metrics = source.getMetricFields(); - expect(metrics[0].getName()).toEqual( - '__kbnjoin__sum_of_myFieldGettingSummed_groupby_myIndex.myTermField' - ); + expect(metrics[0].getName()).toEqual('__kbnjoin__sum_of_myFieldGettingSummed__1234'); expect(await metrics[0].getLabel()).toEqual('my custom label'); - expect(metrics[1].getName()).toEqual('__kbnjoin__count_groupby_myIndex.myTermField'); + expect(metrics[1].getName()).toEqual('__kbnjoin__count__1234'); expect(await metrics[1].getLabel()).toEqual('Count of myIndex'); }); }); describe('extractPropertiesMap', () => { - const minPropName = - '__kbnjoin__min_of_avlAirTemp_groupby_kibana_sample_data_ky_avl.kytcCountyNmbr'; + const minPropName = '__kbnjoin__min_of_avlAirTemp__1234'; const responseWithNumberTypes = { aggregations: { join: { @@ -81,7 +80,7 @@ describe('extractPropertiesMap', () => { }, }, }; - const countPropName = '__kbnjoin__count_groupby_kibana_sample_data_ky_avl.kytcCountyNmbr'; + const countPropName = '__kbnjoin__count__1234'; let propertiesMap; beforeAll(() => { diff --git a/x-pack/test/api_integration/apis/maps/migrations.js b/x-pack/test/api_integration/apis/maps/migrations.js index c4587530e160..cd575899118a 100644 --- a/x-pack/test/api_integration/apis/maps/migrations.js +++ b/x-pack/test/api_integration/apis/maps/migrations.js @@ -41,7 +41,7 @@ export default function({ getService }) { type: 'index-pattern', }, ]); - expect(resp.body.migrationVersion).to.eql({ map: '7.7.0' }); + expect(resp.body.migrationVersion).to.eql({ map: '7.8.0' }); expect(resp.body.attributes.layerListJSON.includes('indexPatternRefName')).to.be(true); }); }); diff --git a/x-pack/test/functional/apps/maps/joins.js b/x-pack/test/functional/apps/maps/joins.js index 89a6c6ea82e5..4b36109a4de9 100644 --- a/x-pack/test/functional/apps/maps/joins.js +++ b/x-pack/test/functional/apps/maps/joins.js @@ -9,7 +9,7 @@ import _ from 'lodash'; import { MAPBOX_STYLES } from './mapbox_styles'; -const JOIN_PROPERTY_NAME = '__kbnjoin__max_of_prop1_groupby_meta_for_geo_shapes*.shape_name'; +const JOIN_PROPERTY_NAME = '__kbnjoin__max_of_prop1__855ccb86-fe42-11e8-8eb2-f2801f1b9fd1'; const EXPECTED_JOIN_VALUES = { alpha: 10, bravo: 3, diff --git a/x-pack/test/functional/apps/maps/mapbox_styles.js b/x-pack/test/functional/apps/maps/mapbox_styles.js index 508a019db176..63bfc331d888 100644 --- a/x-pack/test/functional/apps/maps/mapbox_styles.js +++ b/x-pack/test/functional/apps/maps/mapbox_styles.js @@ -27,7 +27,7 @@ export const MAPBOX_STYLES = { 'case', [ '==', - ['feature-state', '__kbnjoin__max_of_prop1_groupby_meta_for_geo_shapes*.shape_name'], + ['feature-state', '__kbnjoin__max_of_prop1__855ccb86-fe42-11e8-8eb2-f2801f1b9fd1'], null, ], 2, @@ -39,7 +39,7 @@ export const MAPBOX_STYLES = { 'to-number', [ 'feature-state', - '__kbnjoin__max_of_prop1_groupby_meta_for_geo_shapes*.shape_name', + '__kbnjoin__max_of_prop1__855ccb86-fe42-11e8-8eb2-f2801f1b9fd1', ], ], 12, @@ -97,7 +97,7 @@ export const MAPBOX_STYLES = { 'case', [ '==', - ['feature-state', '__kbnjoin__max_of_prop1_groupby_meta_for_geo_shapes*.shape_name'], + ['feature-state', '__kbnjoin__max_of_prop1__855ccb86-fe42-11e8-8eb2-f2801f1b9fd1'], null, ], 2, @@ -109,7 +109,7 @@ export const MAPBOX_STYLES = { 'to-number', [ 'feature-state', - '__kbnjoin__max_of_prop1_groupby_meta_for_geo_shapes*.shape_name', + '__kbnjoin__max_of_prop1__855ccb86-fe42-11e8-8eb2-f2801f1b9fd1', ], ], 12,