Layer configuration should have some semantic checks for min and max zooms

[jerluc]

Describe the bug When layers are configured in Tilenol, we should do some simple semantic checks to at least ensure that the layers are actually accessible (likely around here somewhere), e.g. checking that the min and max zoom numbers fall within the absolute min and max zooms acceptable by the server itself.

To Reproduce Steps to reproduce the behavior:

1. Configure Tilenol with a layer whose max zoom is insanely large, e.g. 1000
2. Run the server
3. Try to request at z = 1000
4. See that you can't access it

[brian-dellabetta]

Hi Jeremy, feel free to assign this to me and I'll take a look

[jerluc]

Thanks for taking a look @brian-dellabetta! Let me know if run into any issues getting started

[brian-dellabetta]

Just getting familiar with it for now -- this is pretty neat. I set it up with elastic search, seeded the data, and I can see tiles but I'm not seeing any heights for the buildings. Maybe something's off with the provided tilenol.yml? I see height fields in the data but no height_ft, though the issue persists after removing the _ft suffixes: https://github.com/StationA/tilenol/blob/master/examples/elasticsearch/tilenol.yml#L19

I'm running ./target/tilenol run -x -f examples/elasticsearch/tilenol.yml. Mutually exclusive to the issue, but I'd like to see it all in action before adding any code.

[jerluc]

Good catch @brian-dellabetta . Yeah it looks like in a recent commit I had changed the field name that's used to set the fill-extrusion-height in the examples/index.html file to height, but never updated the Elasticsearch example config.

It should work however by removing the _ft from only the left-hand side of the config file (this is the GeoJSON property name that the right-hand side gets mapped to in the response), e.g.

        ...
        sourceFields:
          area_sqft: building.area_sqft
          height: building.height_ft

[brian-dellabetta]

ok cool, it was

  sourceFields:
    height: height

i can create a separate MR for this

[brian-dellabetta]

@jerluc couple questions for you:

1) server.go provides default min and max zoom values for the server, such that it returns an InvalidRequestError if z resides outside of these bounds. These are the "absolute min and max zooms acceptable by the server itself" you're talking about, correct? The comment that they're default values is confusing me, makes it sound like they're configurable and only used if the config file doesn't provide min/max zoom values.

2) We have a design choice in terms of sanitization (modify the faulty config to be within the absolute min/max bounds and continue running the server) vs validation (log the error and stop tilenol completely). The latter seems to be more in line with how you've set up the main function, by panicking if any of the ConfigOpts fail, but that implementation could cause other users' servers to fail to startup just by upgrading their tilenol version. I still think latter option but want to get a simple :+1: / :-1: from you first.

[jerluc]

@brian-dellabetta it's probably confusing because that's the source of this particular bug ;)

1. IMO it makes sense that we'd want the absolute min/max zoom levels that are configurable in the layer config to match the server's request-time MinZoom / MaxZoom. The core of the problem is the fact that today you could technically configure a layer via tilenol.yml such that it's stated max zoom level is e.g. 25, but because the request-time check that occurs, you could never really access any tiles beyond MaxZoom.
2. I think it makes sense to consider a tilenol layer configuration that sets its maximum zoom level to something above the MaxZoom as something erroneous, and should probably fail at boot (much like other configuration sanity checks we do). Currently this value is set at 22, which should be sufficient for most practical applications (according to OpenStreetMap's wiki, a zoom level of 20 is already 0.149 meters per pixel, so 22 seems like more than enough).

[brian-dellabetta]

Sounds good! Makes sense to me