[maps] fix Map orphans sources on layer deletion (#159067)

Fixes https://github.com/elastic/kibana/issues/158133

Sources where not getting removed because `return` was used instead of
`continue` in `for...in` loop. This caused the function to return
instead of processing remaining sources.

---------

Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>

packages/kbn-mapbox-gl/index.ts

@@ -13,6 +13,7 @@ import type {
   GeoJSONSource,
   VectorTileSource,
   RasterTileSource,
+  SourceSpecification,
   StyleSpecification,
   MapEvent,
   MapOptions,
@@ -45,6 +46,7 @@ export { maplibregl };
 export type {
   Map,
   LayerSpecification,
+  SourceSpecification,
   StyleSpecification,
   Source,
   GeoJSONSource,

x-pack/plugins/maps/public/connected_components/mb_map/mb_map.tsx

@@ -43,7 +43,7 @@ import {
 import { getCanAccessEmsFonts, getGlyphs, getKibanaFontsGlyphUrl } from './glyphs';
 import { syncLayerOrder } from './sort_layers';
 
-import { removeOrphanedSourcesAndLayers } from './utils';
+import { removeOrphanedSourcesAndLayers } from './remove_orphaned';
 import { RenderToolTipContent } from '../../classes/tooltips/tooltip_property';
 import { TileStatusTracker } from './tile_status_tracker';
 import { DrawFeatureControl } from './draw_control/draw_feature_control';

x-pack/plugins/maps/public/connected_components/mb_map/remove_orphaned.test.ts

@@ -5,12 +5,23 @@
  * 2.0.
  */
 
-import { removeOrphanedSourcesAndLayers } from './utils';
+/* eslint-disable max-classes-per-file */
+
+import type {
+  Map as MbMap,
+  LayerSpecification,
+  SourceSpecification,
+  StyleSpecification,
+} from '@kbn/mapbox-gl';
+import { removeOrphanedSourcesAndLayers } from './remove_orphaned';
 import { SPATIAL_FILTERS_LAYER_ID } from '../../../common/constants';
 import _ from 'lodash';
+import { ILayer } from '../../classes/layers/layer';
 
 class MockMbMap {
-  constructor(style) {
+  private _style: StyleSpecification;
+
+  constructor(style: StyleSpecification) {
     this._style = _.cloneDeep(style);
   }
 
@@ -18,26 +29,11 @@ class MockMbMap {
     return _.cloneDeep(this._style);
   }
 
-  moveLayer(mbLayerId, nextMbLayerId) {
-    const indexOfLayerToMove = this._style.layers.findIndex((layer) => {
-      return layer.id === mbLayerId;
-    });
-
-    const layerToMove = this._style.layers[indexOfLayerToMove];
-    this._style.layers.splice(indexOfLayerToMove, 1);
-
-    const indexOfNextLayer = this._style.layers.findIndex((layer) => {
-      return layer.id === nextMbLayerId;
-    });
-
-    this._style.layers.splice(indexOfNextLayer, 0, layerToMove);
-  }
-
-  removeSource(sourceId) {
+  removeSource(sourceId: string) {
     delete this._style.sources[sourceId];
   }
 
-  removeLayer(layerId) {
+  removeLayer(layerId: string) {
     const layerToRemove = this._style.layers.findIndex((layer) => {
       return layer.id === layerId;
     });
@@ -46,14 +42,15 @@ class MockMbMap {
 }
 
 class MockLayer {
-  constructor(layerId, mbSourceIds, mbLayerIdsToSource) {
+  private readonly _mbSourceIds: string[];
+  private readonly _mbLayerIdsToSource: Array<{ id: string; source: string }>;
+
+  constructor(mbSourceIds: string[], mbLayerIdsToSource: Array<{ id: string; source: string }>) {
     this._mbSourceIds = mbSourceIds;
     this._mbLayerIdsToSource = mbLayerIdsToSource;
-    this._layerId = layerId;
-  }
-  getId() {
-    return this._layerId;
   }
+
+  // Custom interface used in getMockStyle for populating maplibre style with layers and sources
   getMbSourceIds() {
     return this._mbSourceIds;
   }
@@ -61,58 +58,55 @@ class MockLayer {
     return this._mbLayerIdsToSource;
   }
 
-  getMbLayerIds() {
-    return this._mbLayerIdsToSource.map(({ id }) => id);
-  }
-
-  ownsMbLayerId(mbLayerId) {
+  // ILayer interface
+  ownsMbLayerId(mbLayerId: string) {
     return this._mbLayerIdsToSource.some((mbLayerToSource) => {
       return mbLayerToSource.id === mbLayerId;
     });
   }
 
-  ownsMbSourceId(mbSourceId) {
+  ownsMbSourceId(mbSourceId: string) {
     return this._mbSourceIds.some((id) => mbSourceId === id);
   }
 }
 
-function getMockStyle(orderedMockLayerList) {
+function getMockStyle(orderedMockLayerList: MockLayer[]) {
   const mockStyle = {
+    version: 8 as 8,
     sources: {},
     layers: [],
-  };
+  } as StyleSpecification;
 
   orderedMockLayerList.forEach((mockLayer) => {
     mockLayer.getMbSourceIds().forEach((mbSourceId) => {
-      mockStyle.sources[mbSourceId] = {};
+      mockStyle.sources[mbSourceId] = {} as unknown as SourceSpecification;
     });
     mockLayer.getMbLayersIdsToSource().forEach(({ id, source }) => {
       mockStyle.layers.push({
-        id: id,
-        source: source,
-      });
+        id,
+        source,
+      } as unknown as LayerSpecification);
     });
   });
 
   return mockStyle;
 }
 
-function makeSingleSourceMockLayer(layerId) {
+function makeSingleSourceMockLayer(layerId: string) {
+  const source1 = layerId + '_source1';
   return new MockLayer(
-    layerId,
-    [layerId],
+    [source1],
     [
-      { id: layerId + '_fill', source: layerId },
-      { id: layerId + '_line', source: layerId },
+      { id: source1 + '_fill', source: source1 },
+      { id: source1 + '_line', source: source1 },
     ]
   );
 }
 
-function makeMultiSourceMockLayer(layerId) {
+function makeMultiSourceMockLayer(layerId: string) {
   const source1 = layerId + '_source1';
   const source2 = layerId + '_source2';
   return new MockLayer(
-    layerId,
     [source1, source2],
     [
       { id: source1 + '_fill', source: source1 },
@@ -136,14 +130,18 @@ describe('removeOrphanedSourcesAndLayers', () => {
     const currentStyle = getMockStyle(currentLayerList);
     const mockMbMap = new MockMbMap(currentStyle);
 
-    removeOrphanedSourcesAndLayers(mockMbMap, nextLayerList, spatialFilterLayer);
+    removeOrphanedSourcesAndLayers(
+      mockMbMap as unknown as MbMap,
+      nextLayerList as unknown as ILayer[],
+      spatialFilterLayer as unknown as ILayer
+    );
     const removedStyle = mockMbMap.getStyle();
 
     const nextStyle = getMockStyle(nextLayerList);
     expect(removedStyle).toEqual(nextStyle);
   });
 
-  test('should remove foo and bar layer (multisource)', async () => {
+  test('should remove foo and bar layer (multisource)', () => {
     const bazLayer = makeMultiSourceMockLayer('baz');
     const fooLayer = makeMultiSourceMockLayer('foo');
     const barLayer = makeMultiSourceMockLayer('bar');
@@ -154,14 +152,18 @@ describe('removeOrphanedSourcesAndLayers', () => {
     const currentStyle = getMockStyle(currentLayerList);
     const mockMbMap = new MockMbMap(currentStyle);
 
-    removeOrphanedSourcesAndLayers(mockMbMap, nextLayerList, spatialFilterLayer);
+    removeOrphanedSourcesAndLayers(
+      mockMbMap as unknown as MbMap,
+      nextLayerList as unknown as ILayer[],
+      spatialFilterLayer as unknown as ILayer
+    );
     const removedStyle = mockMbMap.getStyle();
 
     const nextStyle = getMockStyle(nextLayerList);
     expect(removedStyle).toEqual(nextStyle);
   });
 
-  test('should not remove anything', async () => {
+  test('should not remove anything', () => {
     const bazLayer = makeSingleSourceMockLayer('baz');
     const fooLayer = makeSingleSourceMockLayer('foo');
     const barLayer = makeSingleSourceMockLayer('bar');
@@ -172,30 +174,63 @@ describe('removeOrphanedSourcesAndLayers', () => {
     const currentStyle = getMockStyle(currentLayerList);
     const mockMbMap = new MockMbMap(currentStyle);
 
-    removeOrphanedSourcesAndLayers(mockMbMap, nextLayerList, spatialFilterLayer);
+    removeOrphanedSourcesAndLayers(
+      mockMbMap as unknown as MbMap,
+      nextLayerList as unknown as ILayer[],
+      spatialFilterLayer as unknown as ILayer
+    );
     const removedStyle = mockMbMap.getStyle();
 
     const nextStyle = getMockStyle(nextLayerList);
     expect(removedStyle).toEqual(nextStyle);
   });
 
-  test('should not remove spatial filter layer and sources when spatialFilterLayer is provided', async () => {
+  test('should not remove spatial filter layer and sources when spatialFilterLayer is provided', () => {
     const styleWithSpatialFilters = getMockStyle([spatialFilterLayer]);
     const mockMbMap = new MockMbMap(styleWithSpatialFilters);
 
-    removeOrphanedSourcesAndLayers(mockMbMap, [], spatialFilterLayer);
+    removeOrphanedSourcesAndLayers(
+      mockMbMap as unknown as MbMap,
+      [],
+      spatialFilterLayer as unknown as ILayer
+    );
     expect(mockMbMap.getStyle()).toEqual(styleWithSpatialFilters);
   });
 
-  test('should not remove mapbox gl draw layers and sources', async () => {
+  test('should remove sources after spatial filter layer', () => {
+    const barLayer = makeMultiSourceMockLayer('bar');
+
+    const currentLayerList = [spatialFilterLayer, barLayer];
+
+    const currentStyle = getMockStyle(currentLayerList);
+    const mockMbMap = new MockMbMap(currentStyle);
+
+    removeOrphanedSourcesAndLayers(
+      mockMbMap as unknown as MbMap,
+      [],
+      spatialFilterLayer as unknown as ILayer
+    );
+    const removedStyle = mockMbMap.getStyle();
+
+    expect(Object.keys(removedStyle.sources)).toEqual([
+      'SPATIAL_FILTERS_LAYER_ID_source1',
+      'SPATIAL_FILTERS_LAYER_ID_source2',
+    ]);
+  });
+
+  test('should not remove mapbox gl draw layers and sources', () => {
     const fooLayer = makeMultiSourceMockLayer('foo');
     const layerList = [fooLayer];
 
     const currentStyle = getMockStyle(layerList);
-    currentStyle.layers.push({ id: 'gl-draw-points' });
+    currentStyle.layers.push({ id: 'gl-draw-points' } as unknown as LayerSpecification);
     const mockMbMap = new MockMbMap(currentStyle);
 
-    removeOrphanedSourcesAndLayers(mockMbMap, layerList, spatialFilterLayer);
+    removeOrphanedSourcesAndLayers(
+      mockMbMap as unknown as MbMap,
+      layerList as unknown as ILayer[],
+      spatialFilterLayer as unknown as ILayer
+    );
     expect(mockMbMap.getStyle()).toEqual(currentStyle);
   });
 });

x-pack/plugins/maps/public/connected_components/mb_map/remove_orphaned.ts

@@ -19,7 +19,6 @@ export function removeOrphanedSourcesAndLayers(
     return;
   }
 
-  const mbLayerIdsToRemove: string[] = [];
   mbStyle.layers.forEach((mbLayer) => {
     // ignore mapbox layers from spatial filter layer
     if (spatialFilterLayer.ownsMbLayerId(mbLayer.id)) {
@@ -35,26 +34,23 @@ export function removeOrphanedSourcesAndLayers(
       return layer.ownsMbLayerId(mbLayer.id);
     });
     if (!targetLayer) {
-      mbLayerIdsToRemove.push(mbLayer.id);
+      mbMap.removeLayer(mbLayer.id);
     }
   });
-  mbLayerIdsToRemove.forEach((mbLayerId) => mbMap.removeLayer(mbLayerId));
 
-  const mbSourcesToRemove = [];
   for (const mbSourceId in mbStyle.sources) {
     if (mbStyle.sources.hasOwnProperty(mbSourceId)) {
       // ignore mapbox sources from spatial filter layer
       if (spatialFilterLayer.ownsMbSourceId(mbSourceId)) {
-        return;
+        continue;
       }
 
       const targetLayer = layerList.find((layer) => {
         return layer.ownsMbSourceId(mbSourceId);
       });
       if (!targetLayer) {
-        mbSourcesToRemove.push(mbSourceId);
+        mbMap.removeSource(mbSourceId);
       }
     }
   }
-  mbSourcesToRemove.forEach((mbSourceId) => mbMap.removeSource(mbSourceId));
 }