digidem / mapeo-map-server

Offline map style and tile server
MIT License
5 stars 2 forks source link

fix: return proper responses for get tile endpoint #62

Closed achou11 closed 2 years ago

achou11 commented 2 years ago

Fixes #57

Notes:

gmaclennan commented 2 years ago

Hi @achou11, I haven't done a full review (currently on a flight), but my initial thought about this is way we are handling the mapbox access token.

As far as I know (worth double-checking in the Mapbox API docs and doing some test requests), when you request a Style or a TileJSON with an access token, then the tile template URL that is returned already has the access token included as part of the template URL. I don't think any code in the server needs to know about the access token: we just store the tile template URL (which may or may not include the access token) and then the rest of the code does not need to be concerned about it.

achou11 commented 2 years ago

@gmaclennan boarding my flight now but did a quick test of fetching a style and the response doesn't seem to embed the access token in the template urls.

the reason for handling the access token is because the routes need to be able to handle the token since the mapbox client (whether from js, java, etc) will append the access token as part of the query string in its request to whatever asset url is provided (the asset url usually will not have the access token from what I've seen). since the request is coming to us first, we need to extract and reappend it to the upstream url

achou11 commented 2 years ago

I don't think any code in the server needs to know about the access token: we just store the tile template URL (which may or may not include the access token) and then the rest of the code does not need to be concerned about it.

yeah that's correct. the only relevant handling of the access token we need to do is append it to whatever template url we're using to make an upstream request. see previous comment for why we need to deal with an access token at all in the first place

achou11 commented 2 years ago

Note to self: update the PRs so that we persist the access token when creating a style (POST /styles), so that the template urls will include that token already. this way we can support creating different mapbox styles that use different access tokens.

When requesting the tile source, the tiles urls will have the access token appended to them already, so no additional handling needed

achou11 commented 2 years ago

Follow up needed:

the GET tile endpoint should include information about the upstream error when a 4XX error occurs. The map server will return a 404 to the client in this case, but it's helpful for it to have access to the upstream error that results in that

EDIT: Mostly addressed via https://github.com/digidem/mapeo-map-server/pull/62/commits/2e4540ed8b92375dc4033ac3c58c0e388ef5616e (could be more specific info instead of http code + http status message)