geoJSON source's updateData throws with undefined properties

[RivkaMandelbaum]

maplibre-gl-js version: 5.13.0

browser: Chrome

Steps to Trigger Behavior

1. In the JSBin linked below, click anywhere to trigger updateData with an undefined property.

More generally this issue occurs when using updateData on a GeoJSON source and any of the feature's properties are undefined.

Link to Demonstration

https://jsbin.com/hiwawahida/edit?html,console,output

Expected Behavior

Using updateData updates the features, working with undefined features just like setData does.

This should work because the GeoJSON properties types allow it. The fact that maplibre is using MVT which don't allow undefined properties under the hood should be considered an implementation detail, and maplibre should be responsible for stripping out invalid properties so that this case doesn't throw. This would match the behavior in the setData case. More detail on the reason this broke below.

Actual Behavior

Using updateData with an undefined property results in an uncaught error (unknown feature value).

Console error:

at index.js:361:15
    at Mu (index.js:339:44)
    at wc.readFields (index.js:42:13)
    at new Eu (index.js:307:13)
    at Fu (index.js:385:23)
    at wc.readFields (index.js:42:13)
    at new ku (index.js:374:27)
    at wf.loadVTLayers (feature_index.ts:122:19)
    at ee.shouldReloadTile (geojson_source.ts:519:48)
    at Ie.reload (tile_manager.ts:282:91)

This is because we're hitting the case in mapbox/vector-tile's readValueMessage where the value read is null. In the 2.0.4 dependency, this case throws. Check out the code here at index.js line 360. Maplibre upgraded to this dependency in 5.6.2 via this PR #6131. The previous version, 1.3.1, did not throw when encountering a null value (see https://www.npmjs.com/package/@mapbox/vector-tile/v/1.3.1?activeTab=code lib/vectortilelayer.js line 50).

At a higher level, this likely happens because of the difference in how the "data" case is handled here vs the "diff" case (corresponding to setData vs updateData).

[HarelM]

Thanks for taking the time to report this issue and the investigation around it.
Cc: @wayofthefuture @lucaswoj @mwilsnd
There has been a lot of changes recently around geojson so it might not be just this package update.
Can you help finding the last version this used to work?
@mwilsnd can you check if this might be some code changes you in vt-pbf somehow (although I doubt it).

[wayofthefuture]

It is almost definitely #6131

See comment: #6131 (comment)

Undefined is currently not allowed.

We resolved this issue by wrapping the props into a helper function:

export function copyPropertiesForGeoJson(currentProps = {}, addProps = {}) {
    const props = {};

    for (const key in currentProps) {
        const value = currentProps[key];
        if (value !== null && value !== undefined) {
            props[key] = value;
        }
    }
    for (const key in addProps) {
        const value = addProps[key];
        if (value !== null && value !== undefined) {
            props[key] = value;
        }
    }

    return props;
}

[mwilsnd]

I don't see any obvious changes to vt-pbf that would cause this. The change to mpabox/vector-tile-js was very deliberate and throws explicitly when encountering undefined properties. Check the specific commit that added this guard. The real question is where we should be filtering for undefined at. vt-pbf is a very specific tool closer to the implementation details surrounding MVTs so I'd lean towards filtering undefined properties out inside MapLibre itself.

[HarelM]

Sounds right to me. We can always move that down the stack.

[bradymadden97]

@mwilsnd I believe it was this commit that switched the dependency on @mapbox/vector-tile-js in vt-pbf. from v1.x to v2.x, which included the new throwing behavior. maplibre/vt-pbf@1f70a88#diff-7ae45ad102eab3b6d7e7896acd08c427a9b25b346470d7bc6507b6481575d519R24

Then #6131 introduced the above into this repo.

I do want to highlight the discrepancy from OP between setData and updateData:

At a higher level, this likely happens because of the difference in how the "data" case is handled here vs the "diff" case (corresponding to setData vs updateData).

This behavior does not occur with setData, and I think at a minimum maplibre should be consistent with its filtering behavior between set and update.

[HarelM]

Let's give @mwilsnd sometime to look into it, I'm sure he'll come up with valuable input.

[mwilsnd]

Alright, based on what I've read all of the following are true:

• vt-pbf explicitly supports the encoding of undefined property values
• Versions of vector-tile-js prior to 2.0.4 coerced undefined values to null
• The mapbox vector tile specification does not support null or undefined values [1], [2]
• The GeoJSON specification supports null values [1]

So, this has technically been a problem with how GeoJSON is processed by MapLibre for quite a while. It wasn't until vector-tile-js was updated to properly enforce the specification for value types that this broke. Currently, MapLibre encodes GeoJSON into a MVT protobuf. That data is then parsed back into MVT form by vector-tile-js inside the FeatureIndex, where the new rigid enforcement of the specification breaks down. To properly fix this, MapLibre's GeoJSON processing should be revised to no longer depend on vector tile protobuf parsing. Even MLTs lack support for null values, so switching internal processing to that format is not viable either.

Currently, this encoding and decoding round trip is required for a processed GeoJSON tile to cross the web worker boundary. I'm going to look into a more bespoke serialization scheme in an attempt to sidestep any limitations imposed by the vector tile specification.