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: https://github.com/mapbox/geojson-vt/pull/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 https://github.com/mapbox/vt-pbf/pull/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 https://github.com/mapbox/mapbox-gl-js/issues/4494#issuecomment-289756237, 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!

[mike-marcacci]

Well, this sure does go quite deep... Also, this is an amazingly well organized codebase, and oh man do the Flow annotations help me follow along.

It appears that both GeoJSON and VectorTile are treated as special across the codebase (in contrast to other source types) and both formats are actually used by different operations. It now appears to me that this issue extends beyond serialization/deserialization of the data. For example, rawTileData on both Tile and FeatureIndex is assumed to be a VectorTile buffer.

The fact that neither format is fully inclusive of the other (and thus a round-trip conversion is lossy) makes this a bit complicated. Duplicating all touch points to handle both formats is obviously undesirable as well... which leads me to suggest that perhaps the simplest solution is to internally redefine:

declare interface VectorTileFeature {
    extent: number;
    type: 1 | 2 | 3;
    id: number;
    properties: {[string]: string | number | boolean};

    loadGeometry(): Array<Array<Point>>;
    bbox(): [number, number, number, number];
    toGeoJSON(x: number, y: number, z: number): GeoJSONFeature;
}

to be:

type JSONValue = null | boolean | number | string | JSONValue[] | {[key: string]: JSONValue};

declare interface VectorTileFeature {
    extent: number;
    type: 1 | 2 | 3;
    id: string | number;
    properties: {[string]: JSONValue};

    loadGeometry(): Array<Array<Point>>;
    bbox(): [number, number, number, number];
    toGeoJSON(x: number, y: number, z: number): GeoJSONFeature;
}

I believe that this would represent everything from both specs. However, it would obviously make the internal representation differ from the over-the-wire spec, or require a change to the spec, which would likely be quite an undertaking.

Another solution might be to modify Tile and FeatureIndex on the main thread so that rawTileData is broken out into rawVectorTileData and rawGeobufData, then update Tile.querySourceFeatures, FeatureIndex.query, and any other touch points to convert to the necessary format from the available raw data. I'm not 100% sure where conditional styling takes place (I haven't really looked into it yet), but that would likely need to be updated as well.

In my deep-dive here I've actually found a way to work around for my exact use case, do this is no longer as critical as I thought. However, I'd still be more than willing to help with this.

@anandthakker thanks for the reply – do my observations here sound correct?

[anandthakker]

@mike-marcacci yes, I think your observations sound right.

The former approach will be more complicated than it might seem, because the existing implementation we're using of interface VectorTileFeature is from the protobuf-backed vector-tile-js, which expects data conforming to the vector tile spec (and thus no string feature ids).

In order to provide geojson data via the generalized interface you describe, we'd need a new VectorTileFeature implementation backed by geobuf data. That would be a bit more involved than a mere pass-through, as it would require transforming lng/lat coordinates (which is what the geobuf data has) to a specific tile's coordinate space.

With the latter approach -- modifying FeatureIndex to handle either rawVectorData or rawGeobufData -- I think the main difficulty would similarly be generalizing the coordinate-transforming code in FeatureIndex so that it properly handles lng/lat geobuf coordinates in the rawGeobufData case.

One advantage of the latter approach is that it would open up the possibility of using a single FeatureIndex on a GeoJSONSource, built only once, rather than having one for each tile. (But that would be a bit of a larger refactor.)

[hyperknot]

Some questions about id-s:

1. mapbox-gl-draw uses hat() to generate string id-s, yet queryRenderedFeatures only supports integers, isn't this in conflict?
2. Are there any performance benefits from adding root level uint ids to features, for example when updating sources using in setData()? If not I'd use string id's in properties.

[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