From c9474270d1d513784fb21d61603567adfc66f1aa Mon Sep 17 00:00:00 2001 From: Nathan Reese Date: Mon, 26 Jun 2023 09:27:28 -0600 Subject: [PATCH] [maps] fix geojson layer with joins and no left source matches stuck in loading state (#160222) closes https://github.com/elastic/kibana/issues/156630 --------- Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com> --- .../geojson_vector_layer.test.ts | 166 ++++++++++++++++++ .../geojson_vector_layer.tsx | 17 ++ .../mvt_vector_layer/mvt_vector_layer.tsx | 5 + .../layers/vector_layer/vector_layer.tsx | 7 +- .../components/geo_index_pattern_select.tsx | 7 +- .../add_layer_panel/view.tsx | 1 + 6 files changed, 194 insertions(+), 9 deletions(-) create mode 100644 x-pack/plugins/maps/public/classes/layers/vector_layer/geojson_vector_layer/geojson_vector_layer.test.ts diff --git a/x-pack/plugins/maps/public/classes/layers/vector_layer/geojson_vector_layer/geojson_vector_layer.test.ts b/x-pack/plugins/maps/public/classes/layers/vector_layer/geojson_vector_layer/geojson_vector_layer.test.ts new file mode 100644 index 0000000000000..6717a04bc80d0 --- /dev/null +++ b/x-pack/plugins/maps/public/classes/layers/vector_layer/geojson_vector_layer/geojson_vector_layer.test.ts @@ -0,0 +1,166 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the Elastic License + * 2.0; you may not use this file except in compliance with the Elastic License + * 2.0. + */ + +import { SOURCE_DATA_REQUEST_ID } from '../../../../../common/constants'; +import { VectorLayerDescriptor } from '../../../../../common/descriptor_types'; +import { InnerJoin } from '../../../joins/inner_join'; +import { IJoinSource } from '../../../sources/join_sources'; +import { IVectorSource } from '../../../sources/vector_source'; +import { GeoJsonVectorLayer } from './geojson_vector_layer'; + +const joinDataRequestId = 'join_source_a0b0da65-5e1a-4967-9dbe-74f24391afe2'; +const mockJoin = { + hasCompleteConfig: () => { + return true; + }, + getSourceDataRequestId: () => { + return joinDataRequestId; + }, + getRightJoinSource: () => { + return {} as unknown as IJoinSource; + }, +} as unknown as InnerJoin; + +describe('isLayerLoading', () => { + test('should return true when source loading has not started', () => { + const layer = new GeoJsonVectorLayer({ + customIcons: [], + layerDescriptor: {}, + source: {} as unknown as IVectorSource, + }); + expect(layer.isLayerLoading(1)).toBe(true); + }); + + test('Should return true when source data request is pending', () => { + const layer = new GeoJsonVectorLayer({ + customIcons: [], + layerDescriptor: { + __dataRequests: [ + { + data: {}, + dataId: SOURCE_DATA_REQUEST_ID, + dataRequestMetaAtStart: {}, + dataRequestToken: Symbol(), + }, + ], + }, + source: {} as unknown as IVectorSource, + }); + expect(layer.isLayerLoading(1)).toBe(true); + }); + + describe('no joins', () => { + test('Should return false when source data request is finished', () => { + const layer = new GeoJsonVectorLayer({ + customIcons: [], + layerDescriptor: { + __dataRequests: [ + { + data: {}, + dataId: SOURCE_DATA_REQUEST_ID, + dataRequestMeta: {}, + dataRequestMetaAtStart: undefined, + dataRequestToken: undefined, + }, + ], + }, + source: {} as unknown as IVectorSource, + }); + expect(layer.isLayerLoading(1)).toBe(false); + }); + }); + + describe('joins', () => { + describe('source data loaded with no features', () => { + test('should return false when join loading has not started', () => { + const layer = new GeoJsonVectorLayer({ + customIcons: [], + joins: [mockJoin], + layerDescriptor: { + __dataRequests: [ + { + data: { + features: [], + }, + dataId: SOURCE_DATA_REQUEST_ID, + dataRequestMeta: {}, + dataRequestMetaAtStart: undefined, + dataRequestToken: undefined, + }, + ], + } as unknown as VectorLayerDescriptor, + source: {} as unknown as IVectorSource, + }); + expect(layer.isLayerLoading(1)).toBe(false); + }); + }); + + describe('source data loaded with features', () => { + const sourceDataRequestDescriptorWithFeatures = { + data: { + features: [{}], + }, + dataId: SOURCE_DATA_REQUEST_ID, + dataRequestMeta: {}, + dataRequestMetaAtStart: undefined, + dataRequestToken: undefined, + }; + + test('should return true when join loading has not started', () => { + const layer = new GeoJsonVectorLayer({ + customIcons: [], + joins: [mockJoin], + layerDescriptor: { + __dataRequests: [sourceDataRequestDescriptorWithFeatures], + } as unknown as VectorLayerDescriptor, + source: {} as unknown as IVectorSource, + }); + expect(layer.isLayerLoading(1)).toBe(true); + }); + + test('should return true when join data request is pending', () => { + const layer = new GeoJsonVectorLayer({ + customIcons: [], + joins: [mockJoin], + layerDescriptor: { + __dataRequests: [ + sourceDataRequestDescriptorWithFeatures, + { + dataId: joinDataRequestId, + dataRequestMetaAtStart: {}, + dataRequestToken: Symbol(), + }, + ], + } as unknown as VectorLayerDescriptor, + source: {} as unknown as IVectorSource, + }); + expect(layer.isLayerLoading(1)).toBe(true); + }); + + test('should return false when join data request is finished', () => { + const layer = new GeoJsonVectorLayer({ + customIcons: [], + joins: [mockJoin], + layerDescriptor: { + __dataRequests: [ + sourceDataRequestDescriptorWithFeatures, + { + data: {}, + dataId: joinDataRequestId, + dataRequestMeta: {}, + dataRequestMetaAtStart: undefined, + dataRequestToken: undefined, + }, + ], + } as unknown as VectorLayerDescriptor, + source: {} as unknown as IVectorSource, + }); + expect(layer.isLayerLoading(1)).toBe(false); + }); + }); + }); +}); diff --git a/x-pack/plugins/maps/public/classes/layers/vector_layer/geojson_vector_layer/geojson_vector_layer.tsx b/x-pack/plugins/maps/public/classes/layers/vector_layer/geojson_vector_layer/geojson_vector_layer.tsx index cb36405e7f367..826318f7e6d7f 100644 --- a/x-pack/plugins/maps/public/classes/layers/vector_layer/geojson_vector_layer/geojson_vector_layer.tsx +++ b/x-pack/plugins/maps/public/classes/layers/vector_layer/geojson_vector_layer/geojson_vector_layer.tsx @@ -60,6 +60,23 @@ export class GeoJsonVectorLayer extends AbstractVectorLayer { return layerDescriptor; } + isLayerLoading(zoom: number) { + const isSourceLoading = super.isLayerLoading(zoom); + if (isSourceLoading) { + return true; + } + + // Do not check join loading status when there are no source features. Why? + // syncMeta short circuits join loading when there are no source features + // because there is no reason to fetch join results when there is nothing to join with + const featureCollection = this._getSourceFeatureCollection(); + if (!featureCollection || featureCollection?.features?.length === 0) { + return false; + } + + return this._isLoadingJoins(); + } + _isTiled(): boolean { // Uses untiled maplibre source 'geojson' return false; diff --git a/x-pack/plugins/maps/public/classes/layers/vector_layer/mvt_vector_layer/mvt_vector_layer.tsx b/x-pack/plugins/maps/public/classes/layers/vector_layer/mvt_vector_layer/mvt_vector_layer.tsx index 4be734ab3be2a..208e8c4acdb07 100644 --- a/x-pack/plugins/maps/public/classes/layers/vector_layer/mvt_vector_layer/mvt_vector_layer.tsx +++ b/x-pack/plugins/maps/public/classes/layers/vector_layer/mvt_vector_layer/mvt_vector_layer.tsx @@ -74,6 +74,11 @@ export class MvtVectorLayer extends AbstractVectorLayer { this._source = args.source as IMvtVectorSource; } + isLayerLoading(zoom: number) { + const isSourceLoading = super.isLayerLoading(zoom); + return isSourceLoading ? true : this._isLoadingJoins(); + } + _isTiled(): boolean { // Uses tiled maplibre source 'vector' return true; diff --git a/x-pack/plugins/maps/public/classes/layers/vector_layer/vector_layer.tsx b/x-pack/plugins/maps/public/classes/layers/vector_layer/vector_layer.tsx index ae75aebba5929..519137f5379f4 100644 --- a/x-pack/plugins/maps/public/classes/layers/vector_layer/vector_layer.tsx +++ b/x-pack/plugins/maps/public/classes/layers/vector_layer/vector_layer.tsx @@ -259,12 +259,7 @@ export class AbstractVectorLayer extends AbstractLayer implements IVectorLayer { return this.getValidJoins().length > 0; } - isLayerLoading(zoom: number) { - const isSourceLoading = super.isLayerLoading(zoom); - if (isSourceLoading) { - return true; - } - + _isLoadingJoins() { return this.getValidJoins().some((join) => { const joinDataRequest = this.getDataRequest(join.getSourceDataRequestId()); return !joinDataRequest || joinDataRequest.isLoading(); diff --git a/x-pack/plugins/maps/public/components/geo_index_pattern_select.tsx b/x-pack/plugins/maps/public/components/geo_index_pattern_select.tsx index 34a6f8b74bded..b3c6034a66e9c 100644 --- a/x-pack/plugins/maps/public/components/geo_index_pattern_select.tsx +++ b/x-pack/plugins/maps/public/components/geo_index_pattern_select.tsx @@ -106,7 +106,8 @@ export function GeoIndexPatternSelect(props: Props) { } const IndexPatternSelect = getIndexPatternSelectComponent(); - const error = !hasGeoFields + const isDataViewInvalid = !props.dataView ? false : !hasGeoFields; + const error = isDataViewInvalid ? i18n.translate('xpack.maps.noGeoFieldInIndexPattern.message', { defaultMessage: 'Data view does not contain any geospatial fields', }) @@ -116,9 +117,9 @@ export function GeoIndexPatternSelect(props: Props) { <> {_renderNoIndexPatternWarning()} - + { disabled={isDisabled || isLoading} isLoading={isLoading} onClick={addLayersAndClose} + iconSide="right" > {ADD_LAYER_STEP_SECONDARY_ACTION_BUTTON_LABEL}