mojodna / tilelive-http

An HTTP source for tilelive
MIT License
12 stars 10 forks source link

Request gzip content-encoding #13

Closed gmaclennan closed 8 years ago

gmaclennan commented 8 years ago

Some tile servers ignore 'accept-encoding' and return gzip encoded content whatever. In that case tilelive-http does not decompress the data, and leads to hours of head-scratching.

mojodna commented 8 years ago

Good call. Published in v0.11.2.

gmaclennan commented 8 years ago

@jingsam I'm very sorry that this change caused you such a headache. It does seem to be a breaking change and should probably be a major version bump @mojodna

As to whether this change is "wrong"...

At v0.11.1 (i.e. before this change) tilelive-http did not request gzip encoding, and should always have received ungzipped (uncompressed) tiles from a well-behaved server - by default the request module it uses does not seem to send any accept-encoding header. Here is the relevant part of the HTTP/1.1 spec. This was the case with any raster tiles - returned tiles are not gzipped.

Unfortunately the Mapbox Vector tile API ignores the accept-encoding header and always sends responses with content-encoding: gzip. Most http clients (all browsers) will decompress anything sent with that header. However, the request module seems to ignore that header because it has not said it will accept gzip encoding.

If Mapbox were to fix their server to properly respond to the accept-encoding header, you would get decompressed vector tiles here. This change kind of patches over the change, and it does make sense to be requesting gzipped tiles in general.

It is all a bit of a mess though because I understand completely when there are other parts of the toolchain that depend on vector tiles being gzipped. I came across this bug in the first place because I was trying to use vector tiles downloaded with tilelive-http with the mapbox-gl-js library, which throws a strange error when encountering gzip compressed tiles.

Perhaps the solution is to make this change opt-in, or make it a major version bump? If Mapbox change their API endpoint that will change the need for this, but it would also break @jingsam 's code.

jingsam commented 8 years ago

@gmaclennan First, tilelive modules should alway return compressed tiles. This is specified in tilelive API

// z, x, y is XYZ
Tilesource.prototype.getTile = function(z, x, y, callback) {
    // when initialization is incomplete, this will fail always.

    // obtains tile and calls callback:
    function(err, tile, options) {
        // err is set when the tile does not exist or when retrieval failed.
        // If the tile does not exist and that's OK, the error message should
        // explicitly read 'Tile does not exist' in order to be handled correctly
        // by tilelive.copy.
        // otherwise, tile is a buffer containing the compressed image data
    }
};

If you want to follow HTTP spec, that's fine. But you should compress tiles again before returning from getTile.

I definitely know why mapbox server does not properly respond to the accept-encoding header. Because the compression of tiles happens at storage level, not at transfer level, so the server can not determine to compress or not by the accept-encoding header. This has been discussed here https://github.com/mapbox/vector-tile-spec/issues/27. Let's check out another similar module called node-tilejson. From here you can see she did not sending accept-encoding header or try to decompress tiles.

As for your use case, using tilelive-http as the backend of mapbox-gl-js, that is not intended for the usage of tilelive modules. Browsers can properly handle compression problem, why you need request module to request tiles in browser side? Tilelive modules is not intend for serve tiles directly, but for the generating and storage of tiles. With the your change, not only my code but all of tilelive related code would be broken, except yours.

gmaclennan commented 8 years ago

Thanks for this @jingsam and I agree with you. I wish I had found that issue earlier and it would have saved me messing things up for you (and lots of wasted time myself).

I added an additional commit to your PR with an update to the README explaining this in case anyone else hits this problem.

@mojodna are you able to merge the PR and release another version reversing my misguided change? Thanks and sorry for the trouble.