[Maps] Add back removed logic copying feature properties for injected data #49400
Conversation
…rior to modification
kindsun
added
[Deprecated-Use Team:Presentation]Team:Geo
|
Pinging @elastic/kibana-gis (Team:Geo) |
💚 Build Succeeded |
There was a problem hiding this comment.
getInjectedData gets called any time syncLayerWithMB is called and syncLayerWithMB gets called any time layer state changes so putting the new properties in getInjectedData will cause a lot of new objects to get created often. cc @thomasneirynck
How about moving the fix further upstream and copy the properties prior creating the layer, or clean the properties prior to indexing?
There was a problem hiding this comment.
This isn't the right fix imo, for the reasons @nreese mentions.
- Syncing style should not require rewriting data.
- The concept for InjectedData seems to be in the wrong place altogether:
InjectedDatadoes not belong onAbstractLayerif onlyVectorLayeris using it. - This PR is a spot-fix that provides an exceptional code-path for setting the visibility of a feature. Why would this knowledge have to bleed down?
imo, GeoJsonFileSource should correctly implement the same contract that all vector sources implement.
Specifically, GeoJsonFileSource should implement getGeoJsonWithMeta. In this method, it can then return a copy of the original source data, similar to how ES-source is dealing with the exact same issue and duplicating the properties. The concept of __injectedData could move down there as well (if we need it at all).
💚 Build Succeeded |
|
After following up with an offline conversation, I've made a few changes that should address the feedback on this issue. I've backed out the introduction of the
While backing out
I believe this should address the above concerns but feel free to let me know if you see anything else! |
| @@ -125,6 +127,22 @@ export class GeojsonFileSource extends AbstractVectorSource { | |||
| ); | |||
| } | |||
|
|
|||
| async getGeoJsonWithMeta() { | |||
There was a problem hiding this comment.
I'd add a safety check here to check on the existence of __featureCollection. This source could be created by a client who's passing in a descriptor without the __featureCollection property, especially now we have embeddables (e.g. consider an embeddable-client just injecting data with a GeoJsonSource and misconfigurin the descriptor). We don't want this call to throw.
You could also enforce that there's always an empty featureCollection for the __featureCollection property in the GeoJsonFileSource#constructor .
There was a problem hiding this comment.
You could also enforce that there's always an empty featureCollection for the __featureCollection property in the GeoJsonFileSource#constructor .
I like that. Done!
…imally to a geojson object with an empty features array
| static createDescriptor(geoJson, name) { | ||
| // Wrap feature as feature collection if needed | ||
| let featureCollection; | ||
| if (geoJson.type === 'FeatureCollection') { |
There was a problem hiding this comment.
Need to check that geoJson is not null here and else if statement
💚 Build Succeeded |
💚 Build Succeeded |
… data (elastic#49400) * Add back removed logic copying feature properties for injected data prior to modification * Back out injected data and assign featureCollection to sourceDescriptor with __ prefix * Review feedback. Ensure __featureCollection is always initialized minimally to a geojson object with an empty features array * Check for null/undefined
… data (elastic#49400) * Add back removed logic copying feature properties for injected data prior to modification * Back out injected data and assign featureCollection to sourceDescriptor with __ prefix * Review feedback. Ensure __featureCollection is always initialized minimally to a geojson object with an empty features array * Check for null/undefined
… data (#49400) (#49699) * Add back removed logic copying feature properties for injected data prior to modification * Back out injected data and assign featureCollection to sourceDescriptor with __ prefix * Review feedback. Ensure __featureCollection is always initialized minimally to a geojson object with an empty features array * Check for null/undefined
… data (#49400) (#49698) * Add back removed logic copying feature properties for injected data prior to modification * Back out injected data and assign featureCollection to sourceDescriptor with __ prefix * Review feedback. Ensure __featureCollection is always initialized minimally to a geojson object with an empty features array * Check for null/undefined
Fixes a regression introduced by #47158. Logic specific to the Maps app leaks into the parsed GeoJSON object prior to indexing, causing it to be unnecessarily indexed:
This PR adds back the removed logic but in a different location to work with the new InjectedData workflow. The field no longer appears in the indexed data: