Validate duplicate stops in stylesheets. #4498
Conversation
| } | ||
| seenStops[stop[0]] = stop[1]; | ||
| return true; | ||
| }); |
There was a problem hiding this comment.
Since we don't actually use the returned value of every, it makes more sense to use a for ... of loop here.
|
Also, what's the earlier bug you mentioned and should it be reported as well? |
|
See discussion on #4107 and mapbox/mapbox-gl-style-spec#411: we tried adding this validation, but reverted it when it became clear that styles with duplicate stops were somewhat widespread and it would be difficult to migrate them without incrementing the style spec version number. Do we want to revisit that decision? |
|
On a second thought, we should probably fix the infinite loop problem in the binary search implementation rather then avoiding the situation with validation then. cc @kronick |
Prior to mapbox/mapbox-gl-function#35, duplicate stops wouldn't trigger the infinite loop in my styles. It landed in mapbox-gl-js as part of this merge. Specifically: - "mapbox-gl-function": "mapbox/mapbox-gl-function#41c6724e2bbd7bd1eb5991451bbf118b7d02b525",
+ "mapbox-gl-function": "mapbox/mapbox-gl-function#4f829622413f336080d3c710244c251421c0ec30",Just to confirm, should binarySearchForIndex() be updated to only pick the first stop when there's duplicates? |
This seems like a change that'd need to wait for a semver major version bump. There are several other validation strict-ening changes we'd probably want to make at once. (i.e. disallowing arbitrary properties on the style root object) |
|
I agree. It should fix this problem in the short term if we change A newer version of the spec can enforce the prevention of duplicate keys. |
Otherwise, duplicate pairs for a stop in a style can cause binarySearchForIndex() to enter an infinite loop. This didn't immediately cause issues because of an earlier bug in mapbox-gl-function that was introduced between 0.32.1 and 0.33 as part of this rebase with the mapbox-gl-function library.