Added option to override the output layer name

[thomersch]

An attempt on building the issue discussed in #94.

What I did here is adding the possibility to specify an additional name for each maps.layer. If none is specified, it is ignored and generated from provider_layer as it was. Existing configurations are unaffected this way.

Because the data source name was also used as the output name in the tile, I changed the MVTLayer method to have to be able to take distinct strings.

I tried to make the changes as simple as possible, I hope this is acceptable for you. If you have any feedback, I'd love to hear it.

[ARolek]

@thomersch thanks for the PR. I have been looking through it and I have a few comments:

• I noticed you're expanding out the mvt.Provider interface to include both the providerLayerName and the layerName. I would like to keep the MVTLayer() function signature as tight as possible. An alternative way to approach this would be storing the user configured layerName on the layer struct when the provider is setup when NewProvider() is invoked. Then inside the MVTLayer() method we can check for an user defined layerName to use or default to the providerLayerName. This allows us to not expand the MVTLayer() signature and get the same result.
• We should use providerLayerName as the variable name pointing to the providerLayer (current convention). We can use layerName for the user defined name.

Let me know what you think.

[gdey]

@thomersch @ARolek

Though the PR looks good overall, I agree with @ARolek that we don't need to change the MVTLayer() function signature. Passing a mapping from ProviderLayerName to OutputLayerName as a config would be better. Adding a new field to the layer struct for the output name.

Also, the LayersNames() function signature should change. LayersNames is used to populate the /capabilities endpoint, and to help people know which provider provides which providerLayer names. Since we are splitting the providerLayer name from the tile name, we need to provide this mapping and make the /capabilities end point aware of the change. I would recommend changing the interface from LayerNames()[]string to LayerNames()map[string]string with the key of the map being the ProviderLayer name and the value being the output layer name.

[thomersch]

Thanks for the valuable feedback :) Totally agree with you that that MVTLayer shouldn't get too bloated.

Nevertheless I think the one of the problems is that there is no separation of concerns: The MVTLayer implementation handles database functionality and output in one method. Though I am not sure how to solve this nicely.

An alternative way to approach this would be storing the user configured layerName on the layer struct when the provider is setup when NewProvider() is invoked.

I've looked into it, but as far as I see, NewProvider gets only the provider configuration. This would mean that the layer name override needs to be defined in the provider section, which would intermingle map layer and provider configuration. I don't think this is desirable.

What I would suggest is decoupling this and handling the layer name outside, after the layer assembly step. I will push some changes shortly.

Since we are splitting the providerLayer name from the tile name, we need to provide this mapping and make the /capabilities end point aware of the change.

This is already reflected and rather clear in the capabilities call:

"layers": [
    {
        "maxzoom": 14,
        "minzoom": 11,
        "name": "roads",
        "provider_layer": "roads",
        "tiles": [
            "http://localhost:6768/maps/foobar/roads/{z}/{x}/{y}.pbf"
        ]
    },
    {
        "maxzoom": 10,
        "minzoom": 8,
        "name": "roads",
        "provider_layer": "roads_gen0",
        "tiles": [
            "http://localhost:6768/maps/foobar/roads/{z}/{x}/{y}.pbf"
        ]
    }
]

@gdey Do you think we need to make this more explicit?

We should use providerLayerName as the variable name pointing to the providerLayer (current convention). We can use layerName for the user defined name.

@ARolek I don't think I quite understand: Do you mean inside server.Layer?

[ARolek]

@thomersch enjoying the discussion here. It's nice to get some feedback and review on the design thus far.

I've looked into it, but as far as I see, NewProvider gets only the provider configuration. This would mean that the layer name override needs to be defined in the provider section, which would intermingle map layer and provider configuration. I don't think this is desirable.

Data providers are passed a map[string]interface{} with various config params. We designed this to be pretty flexible as different data providers are going to require completely different config params. For reference, the PostGIS data provider does setup a postgis.layer here. I do think your approach to setting the name after the mvt.Layer is returned from the provider is much cleaner than my suggestion and does not require any modification to the MVTProvider interface.

Regarding the /capabilities endpoint, I'm interested to know how a client side styling tool will handle this (are you familiar with Maputnik)? Having two layers in the /capabilities endpoint might break it. Part of me also thinks that we should remove the providerLayer attribute from the /capabilities endpoints AND combine the layer zoom levels when they have the same name. It seems to me we're exposing server side concepts to the client. The only time I see this being a bit strange is if there is a gap in zoom levels (i.e. 3-5 for roads and then 10-15 for roads_gen0).

The newest solution you have proposed does look pretty good. We would need some additional error checking during the config parsing to make sure that no two layers with the same name have overlapping zoom. This check would also address #81.

[thomersch]

Regarding the /capabilities endpoint, I'm interested to know how a client side styling tool will handle this (are you familiar with Maputnik)? Having two layers in the /capabilities endpoint might break it.

I checked Maputnik, but it doesn't even call /capabilities, but looks only at the tiles, so there should be no problem so far.

We would need some additional error checking during the config parsing to make sure that no two layers with the same name have overlapping zoom.

Good idea, will try to find some time in the next days.

[ARolek]

I checked Maputnik, but it doesn't even call /capabilities, but looks only at the tiles, so there should be no problem so far.

Adding a tegola source to Maputnik requires a link to a TopoJSON endpoint describing the map. Since tegola can support multiple maps we have a server level /capabilities endpoint that describes all maps, and and individual /capabilities/:map_name.json endpoints for each map. The /capabilities/:map_name.json endpoints return TopoJSON which Maputnik reads. Here's an example endpoint of a Bonn OSM extract that can be added to Maputnik as a Source:

http://tegola.cfapps.io/capabilities/bonn_osm.json

I just want to make sure that if we were to return two layers with the same name but different zoom levels this would not break Maputnik (or any other client side renderer). I'm also still considering combining layers with the same name and combining their min / max zoom levels. For example:

name: roads
minZoom: 1
maxZoom: 5

name: roads
minZoom: 7
maxZoom: 12

would become:

name: roads
minZoom: 1
maxZoom: 12

Note the gap at zoom level 6.

[ARolek]

@thomersch This has been implemented in the v0.4.0 branch. I leveraged a lot of your ideas and implemented a bunch of our discussion. Thanks for the help on this feature.