Replace outdated vector tile fixtures

[anandthakker]

While working on #11489, I discovered that at least one (and probably more) of our vector tile unit test fixtures is out of date.

Specifically, https://github.com/mapbox/mapbox-gl-native/blob/553efa38e3591ce62863d4d74222710f8e3c2337/test/fixtures/map/offline/0-0-0.vector.pbf (originally added in #2721) is a v1 vector tile that appears to include a linestring with a ClosePath command. The v1 spec did not disallow this, but it causes vtzero to throw an exception ("expected command 1 but got 7").

We should:

• For most of our unit tests, update the vector tiles we're using as test fixtures to v2 tiles.
• Add some v1 tiles to https://github.com/mapbox/mvt-fixtures, and then add a unit test specifically targeting v1 compatibility

[jfirebaugh]

We released offline before v2 vector tiles (#2444 (comment)), so it's definitely possible (and supported) to have an offline database with v1 tiles in it.

[anandthakker]

Yeah -- my suggestion here is just that our normal unit test fixtures be v2 tiles (since that probably accounts for the large majority of use in practice), and that we separate v1 compatibility into its own, explicit test(s)

[anandthakker]

A separate question is whether we need to make sure we can continue to handle tiles exhibiting the specific issue (the spurious closepath command) that prompted this.

My understanding is that in general, vtzero does attempt to be flexible in decoding tiles, leaving strict validation up to the client. cc @springmeyer @joto

[joto]

The fixtures in https://github.com/mapbox/mvt-fixtures have separate fields already describing the tiles as "valid" according to the v1 or v2 spec. So tiles that have different validity in v1/v2 could be easily integrated.

But I think the real problem is a different one here: There basically is no v1 spec. The so-called v1 spec isn't much more than a .proto file with some comments. So just from the spec it is unclear what is allowed and what not. The ClosePath you mentioned isn't disallowed in v1, but I'd never have thought to interpret the spec in a way that would allow this. So basically the implementation(s) we (or others) have (or had at some point) define what's allowed and what not. That's difficult to implement/test.

My understanding is that in general, vtzero does attempt to be flexible in decoding tiles, leaving strict validation up to the client.

That is not really true. Vtzero is strict in some places where it matters and where the strictness is cheap to check. "where it matters" in this case means: Where we want to and can give the user of vtzero assurances about the data, we want to do this. So for instance the spec says that a layer MUST contain a name field and that check is reasonably cheap, so we check for that so that the application using vtzero doesn't have to check for it themselves. On the other hand the spec also says that polygons must be valid. Vtzero doesn't check for that, because the implementation is really expensive and would add a huge burden to software that might not need that check.

Now back to the v1 vs. v2 question: An earlier version of vtzero had a flag that would allow making checks more or less strict. This was removed (mapbox/vtzero#23) to keep the code simpler and vtzero easier to use. Where vtzero keeps the checks is where it needs them to make sure there are no ambiguities or, even worse, possible attacks (for instance by opening up the application to a buffer overflow).

We could add the "strict" vs. "non-strict" mode back in, the "strict" mode would only read v2 tiles and the "non-strict" mode also v1 tiles. But, as I said, it is difficult to do because we don't know what all the v1 tiles actually look like without the spec. And the result might well be that everybody uses the "non-strict" mode, because nobody wants users complaining about non-working vector tiles, which makes the "strict" mode kind of useless. Or we can make the code more backwards-compatible by just always allowing things like the ClosePath on LineStrings (possibly setting some error flag somewhere). In this specific case I can't think of any problems this would result in except making the code a bit more complex. If you think you needs this and this is the way to go, please open issues on vtzero for each of those specific cases where you want vtzero to be backwards compatible.

[anandthakker]

That is not really true. Vtzero is strict in some places where it matters and where the strictness is cheap to check.

👍 thanks for clarifying this @joto. I suspect that in practice, it's really just invalid geometries coming from old v1 tiles that we would need to worry about, and I think it sounds fine to deal with such backwards compatibility issues on a case-by-case basis.

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[stale]

This issue has been automatically detected as stale because it has not had recent activity and will be archived. Thank you for your contributions.