query*Features should preserve non-integer feature IDs

[danvk]

Here's a jsbin demonstrating the issue:
https://jsbin.com/bafewapela/edit?html,console,output

The GeoJSON spec states that features may have a top-level id property:

If a feature has a commonly used identifier, that identifier should be included as a member of the feature object with the name "id".

However, when I have a source with such a feature:

var markers = {
    "type": "FeatureCollection",
    "features": [{
        "id": "my-feature-id",  // <--- feature has an ID
        "type": "Feature",
        "properties": {
            "marker-symbol": "theatre",
        },
        "geometry": {
            "type": "Point",
            "coordinates": [-77.038659, 38.931567]
        }
    }]
};

add it to the map and then get features back via queryRenderedFeatures, the id is gone. The properties and geometry properties are there, but id is not. It would be really helpful if this special property were preserved.

The workaround is to duplicate IDs into properties.id before adding features to the map, but this creates extra work and contradicts the GeoJSON spec.

mapbox-gl-js version: 0.19.1

[jfirebaugh]

Upstream PR which this depends on: mapbox/geojson-vt#60.

[mourner]

This was fixed on the geojson-vt side, but now ids don't survive the transfer through vt-pbf. We'll probably want to switch to something like Geobuf.

[lucaswoj]

@mourner I'm excited to (finally!) make geobuf part of GL JS!!

Have you considered transferring the feature ids through a specially named property, like $id?

[mourner]

@lucaswoj we could do this, but this feels kinda hacky and fragile to me (e.g. what if the data has both id and a property $id?). It might be good enough as a stopgap though.

[lucaswoj]

what if the data has both id and a property $id

Then our $id filtering will break anyway!

[anandthakker]

@mourner see mapbox/vt-pbf#8

[mourner]

@anandthakker thanks! I was referring to the vector-tile-spec limitation of having ids as uints which John pointed out in the PR, so the original issue would still be present as string ids are not encoded.

[anandthakker]

Ahhh, this problem goes deeper than I realized -- I read too quickly 😄. And anyway, the geobuf thing sounds like a much better solution, ultimately!

[anandthakker]

Note that per #4494 (comment), polygon layers manifest this problem more severely, crashing (due to pbf parsing problems) rather than simply stripping the id. This may indicate an upstream bug in vt-pbf

[gmaclennan]

The crashing was introduced by a change in v0.33.0. Prior to that polygon layers would simply strip the id but continue to work.

[gmaclennan]

@anandthakker just thinking about this a little more. It's your choice, but I'm wondering if this makes sense to be the same issue? query*Features() has always ignored non-integer ids, and it seems non-trivial to fix. Crashing when adding GeoJSON polygons with non-integer ids is a related bug, but it is a new bug introduced by a recent change, and seems like it needs a fix outside the fix to this bug. At least the bug label should be added here, since this part is not a new feature request.

[anandthakker]

good point @gmaclennan - I think you're right. I'll reopen 4944 to track the crash. Thanks for following up / setting me straight!

[mike-marcacci]

Hi there! It's looking like the lack of string ID support is going to be a blocker for me shortly, and I'm definitely willing to work on a PR to get this fixed. I just want to double check: is the plan here still to swap in geobuf for the current encoding scheme when communicating between the main js thread and the worker?

[mike-marcacci]

So, today's the day I need to start working on this – I'm going to try swapping in geobuf for communication to the web worker, and I'll submit a PR if I get it working, but I would definitely appreciate an early nudge in a better direction or confirmation that this is still the best strategy to fixing this.

[anandthakker]

Hey @mike-marcacci -- sorry for the delay! Confirming that using geobufinstead of vt-pbf for transferring geojson data back to the main thread does seem like our most promising option. A PR attempting this would be most welcome!

[asheemmamoowala]

Are there any performance benefits from adding root level uint ids to features, for example when updating sources using in setData()?

@hyperknot there are no performance benefits, but the Map#featureState APIs only work with uint ids, so there is the performance benefit if using that option.

[aliir74]

Why mapbox didn't support string ids? I read comments on this issue but don't understand why id forced to be number?
For preventing from brute force I need some random string id not integer.

[bloigge]

I am confused as well. For example it makes things unnecessarily difficult if you are working with geojson WFS data hosted from a geoserver where the id comes as a string in the format <layer-name>.<serial-id>. It would be awesome if mapbox-gl could preserve non-integer feature IDs on geojson features

[strech345]

For me it is also important, because we use ubid for buildings which is a string. To use states we need to use this as id.

[karen1au]

Is there any update on the type for feature.id ? I'm using uuid (string) as well so allowing string would keep things consistent.

[pgoshulak]

Is there any update on the type for feature.id ? I'm using uuid (string) as well so allowing string would keep things consistent.

Bump

Same, our project uses String type ids everywhere for our polygons but in our Mapbox components we can't use these :(

[jeanpul]

Is there any update on the type for feature.id ? I'm using uuid (string) as well so allowing string would keep things consistent.

Bump

Same, our project uses String type ids everywhere for our polygons but in our Mapbox components we can't use these :(

Same here, I'm using mapbox-gl-draw that returns IDs on feature but then removed when using setData

[asheemmamoowala]

While this is not directly supported now, starting with v1.7.0 you can use promoteId (ref: #8987) to use a string-type feature property as the identifier for feature-state operations

[mingjunlu]

@asheemmamoowala Thank you! Using promoteId solved my problem.

[karen1au]

@asheemmamoowala thanks for replying! I'm trying to use promoteId with my geojson type source, however after adding that, the feature simply doesn't render on screen anymore... am I missing anything??

this.map.addSource(sector.id, { type: 'geojson', promoteId: sector.id, data: { type: 'Feature', geometry: { type: 'Polygon', coordinates: sector.boundaries.coordinates } } })

[mingjunlu]

@karen1au does this work?

this.map.addSource(sector.id, {
  type: 'geojson',
  promoteId: 'id',
  data: {
    type: 'Feature',
    geometry: {
      type: 'Polygon',
      coordinates: sector.boundaries.coordinates
    },
    properties: {
      id: sector.id
    }
  }
});

[chriszrc]

@mingjunlu that works, but I think @karen1au is maybe wondering (and me too), if there's a way to use the feature.id property rather than the feature.properties.id

[hmendezm]

Hi, @chriszrc Do you find the way to use the feature.Id as promoted? I do not have Id in properties and I do not have control over the data. Also, I want to avoid adding the id in properties because of performance.
best
hmendez

[chriszrc]

@hmendezm unfortunately no. I moved the id to the properties, and then it worked. I never was able to have it work from feature.id

[hmendezm]

ohk. Thanks @chriszrc. in the case, I have to do the same. sucks

[felix-ht]

Even tho promoteId works, this is pretty clunky to use. So allowing for strings in ids directly would be much nicer!

[sienki-jenki]

Bumping. Any update on that? When string id will be preserved without any additional config like promoteId?

[jaybo]

Bumping. I run into this on an annual basis, and each time I can't believe this incompatibility with the GeoJSON spec hasn't been fixed yet! I'd have to say it's my major pain point using Mapbox.

[michaeljfazio]

How is this still a thing....

[michaelabela]

Bumping this. Running into major headaches not being able to use strings as feature ids.