-
Notifications
You must be signed in to change notification settings - Fork 8.2k
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
[Maps] refactor feature id assignment (#51317)
* [Maps] refactor feature id assignment * add test case for falsy id value * review feedback
- Loading branch information
Showing
10 changed files
with
162 additions
and
79 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
57 changes: 57 additions & 0 deletions
57
x-pack/legacy/plugins/maps/public/layers/util/assign_feature_ids.js
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,57 @@ | ||
| /* | ||
| * 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 { FEATURE_ID_PROPERTY_NAME } from '../../../common/constants'; | ||
|
|
||
| let idCounter = 0; | ||
|
|
||
| function generateNumericalId() { | ||
| const newId = idCounter < Number.MAX_SAFE_INTEGER ? idCounter : 0; | ||
| idCounter = newId + 1; | ||
| return newId; | ||
| } | ||
|
|
||
| export function assignFeatureIds(featureCollection) { | ||
|
|
||
| // wrt https://github.com/elastic/kibana/issues/39317 | ||
| // In constrained resource environments, mapbox-gl may throw a stackoverflow error due to hitting the browser's recursion limit. This crashes Kibana. | ||
| // This error is thrown in mapbox-gl's quicksort implementation, when it is sorting all the features by id. | ||
| // This is a work-around to avoid hitting such a worst-case | ||
| // This was tested as a suitable work-around for mapbox-gl 0.54 | ||
| // The core issue itself is likely related to https://github.com/mapbox/mapbox-gl-js/issues/6086 | ||
|
|
||
| // This only shuffles the id-assignment, _not_ the features in the collection | ||
| // The reason for this is that we do not want to modify the feature-ordering, which is the responsiblity of the VectorSource#. | ||
| const ids = []; | ||
| for (let i = 0; i < featureCollection.features.length; i++) { | ||
| const id = generateNumericalId(); | ||
| ids.push(id); | ||
| } | ||
|
|
||
| const randomizedIds = _.shuffle(ids); | ||
| const features = []; | ||
| for (let i = 0; i < featureCollection.features.length; i++) { | ||
| const numericId = randomizedIds[i]; | ||
| const feature = featureCollection.features[i]; | ||
| features.push({ | ||
| type: 'Feature', | ||
| geometry: feature.geometry, // do not copy geometry, this object can be massive | ||
| properties: { | ||
| // preserve feature id provided by source so features can be referenced across fetches | ||
| [FEATURE_ID_PROPERTY_NAME]: feature.id == null ? numericId : feature.id, | ||
| // create new object for properties so original is not polluted with kibana internal props | ||
| ...feature.properties, | ||
| }, | ||
| id: numericId, // Mapbox feature state id, must be integer | ||
| }); | ||
| } | ||
|
|
||
| return { | ||
| type: 'FeatureCollection', | ||
| features | ||
| }; | ||
| } |
83 changes: 83 additions & 0 deletions
83
x-pack/legacy/plugins/maps/public/layers/util/assign_feature_ids.test.js
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,83 @@ | ||
| /* | ||
| * 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 { assignFeatureIds } from './assign_feature_ids'; | ||
| import { FEATURE_ID_PROPERTY_NAME } from '../../../common/constants'; | ||
|
|
||
| const featureId = 'myFeature1'; | ||
|
|
||
| test('should provide unique id when feature.id is not provided', () => { | ||
| const featureCollection = { | ||
| features: [ | ||
| { | ||
| properties: {} | ||
| }, | ||
| { | ||
| properties: {} | ||
| }, | ||
| ] | ||
| }; | ||
|
|
||
| const updatedFeatureCollection = assignFeatureIds(featureCollection); | ||
| const feature1 = updatedFeatureCollection.features[0]; | ||
| const feature2 = updatedFeatureCollection.features[1]; | ||
| expect(typeof feature1.id).toBe('number'); | ||
| expect(typeof feature2.id).toBe('number'); | ||
| expect(feature1.id).toBe(feature1.properties[FEATURE_ID_PROPERTY_NAME]); | ||
| expect(feature1.id).not.toBe(feature2.id); | ||
| }); | ||
|
|
||
| test('should preserve feature id when provided', () => { | ||
| const featureCollection = { | ||
| features: [ | ||
| { | ||
| id: featureId, | ||
| properties: {} | ||
| } | ||
| ] | ||
| }; | ||
|
|
||
| const updatedFeatureCollection = assignFeatureIds(featureCollection); | ||
| const feature1 = updatedFeatureCollection.features[0]; | ||
| expect(typeof feature1.id).toBe('number'); | ||
| expect(feature1.id).not.toBe(feature1.properties[FEATURE_ID_PROPERTY_NAME]); | ||
| expect(feature1.properties[FEATURE_ID_PROPERTY_NAME]).toBe(featureId); | ||
| }); | ||
|
|
||
| test('should preserve feature id for falsy value', () => { | ||
| const featureCollection = { | ||
| features: [ | ||
| { | ||
| id: 0, | ||
| properties: {} | ||
| } | ||
| ] | ||
| }; | ||
|
|
||
| const updatedFeatureCollection = assignFeatureIds(featureCollection); | ||
| const feature1 = updatedFeatureCollection.features[0]; | ||
| expect(typeof feature1.id).toBe('number'); | ||
| expect(feature1.id).not.toBe(feature1.properties[FEATURE_ID_PROPERTY_NAME]); | ||
| expect(feature1.properties[FEATURE_ID_PROPERTY_NAME]).toBe(0); | ||
| }); | ||
|
|
||
| test('should not modify original feature properties', () => { | ||
| const featureProperties = {}; | ||
| const featureCollection = { | ||
| features: [ | ||
| { | ||
| id: featureId, | ||
| properties: featureProperties | ||
| } | ||
| ] | ||
| }; | ||
|
|
||
| const updatedFeatureCollection = assignFeatureIds(featureCollection); | ||
| const feature1 = updatedFeatureCollection.features[0]; | ||
| expect(feature1.properties[FEATURE_ID_PROPERTY_NAME]).toBe(featureId); | ||
| expect(featureProperties).not.toHaveProperty(FEATURE_ID_PROPERTY_NAME); | ||
| }); | ||
|
|
Oops, something went wrong.