mapbox / vector-tile-spec

Mapbox Vector Tile specification
https://www.mapbox.com/vector-tiles/specification/
Other
906 stars 210 forks source link

Add byte array to Values message #142

Open flippmoke opened 5 years ago

flippmoke commented 5 years ago

We currently do not have a way to properly encode byte arrays as the value of an attribute. Ideally, this would not be an allowed type but I can see in some rare cases where this might be useful. I would like to propose adding bytes as the 8th value message and specifically state that it may be ignored in the decoder.

I know that from an data format standpoint that protobuffers do not have any difference between bytes and strings, however, I have come to realize that in some situations such as the python protobuf libraries it attempts to enforce a text encoding type on the values as they are set via the protobuf library. Additionally, it would be ideal for a client to know if there is a non standard string format that it could be ignored for situations where a bytes field is being used.

Thoughts?

/cc @ericfischer @joto

joto commented 5 years ago

Is this a change you want to make to the v2 spec?

e-n-f commented 5 years ago

I think I had argued for custom attribute types that would include a MIME type or com.whatever-style identifier as well as the blob, but I agree, it is useful to distinguish strings of text from binary blobs.

My only objection to adding this is that we are still in the same trap we were in before, where too many libraries will throw exceptions when they find unexpected messages in the protobuf instead of cleanly passing them through. Until that is solved somehow, it will be safer to encode binary data as base64 or something similar than to use a new type.

flippmoke commented 5 years ago

@joto no to the v3 spec

joto commented 5 years ago

@flippmoke Shouldn't we then leave the Values alone and backwards compatible and only allow the "binary type" as a new type in the stuctured value encoding?

flippmoke commented 5 years ago

@joto I went brain dead on the solution here this morning -- and yes we should.