Refactor into two packages

[fawick]

This is with regard to maplibre/maputnik#184:

/cc @lukasmartinelli

I'd like to propose to refactor this repository so that it consists of main package and a non-mainpackage so that the tile server can be vendored and used in other Go projects.

I'd be happy to contribute to the necessary effort if @consbio and @brendan-ward are interested.

[brendan-ward]

@fawick @lukasmartinelli 👍

This sounds like a good idea, and maputnik looks like a great project. I'd be happy to help do this.

Before we begin, it would be good to have an understanding of how we want to split mbtileserver into packages to best support this.

Splitting out mbtiles.go into its own package seems like an easy first step; this is very self contained and provides the core interface to mbtiles files.

However, I'm unclear on how you would want what is now in main.go split out. @fawick would you mind identifying the core capabilities that are lurking in main.go that are most useful as a library for use in other applications?

At a high level, we have:

• CLI related operations
• HTTP server operations
• in-memory caching
• things that help format the responses with the tile data (could theoretically be split out from the HTTP handlers themselves)

[fawick]

Thank your being open to this, @brendan-ward, that means a lot! 👍

I agree that the type Mbtiles and its methods are a good place to start. It makes sense to put that into a dedicated package. I'd also move the import of go-sqlite3 from main.go to there as its the only user of sqlite. That package would then already take care of the most important use case for other users: Being able to read tiles from a Mbtiles server.
Ages ago I wrote a MBTiles implementation for storing raster tiles from mapnik: https://github.com/fawick/go-mapnik/blob/master/maptiles/mbtilesgenerator.go#L16. That code contains members to create a new MBTiles DB and its schema, I could sponsor that code for the new package that is about to be created here, too. That way you'd have an mbtiles packages that could read from and write to the DB.

Another idea would be some convenience methods that wrap Mbtiles.ReadTile et al., like this

// WriteTileTo reads a tile with coordinates z, x, and y from tileset and writes it to w. 
// The first error encountered is returned. If no error occurs, the returned error will be nil.
func (tileset * Mbtiles) WriteTileTo(w io.Writer, z uint8, x uint64, y uint64) 

In the same vein I'd remove the calls logrus.Error and logrus.Errorf. Returning the error is enough and whether to handle them via logging should be a choise of the package user.

As for main.go: I believe the CLI is specific to mbtileserver and should remain in package main. Same goes for TLS support.

I'd refactor the HTTP operations into an own package that only uses net/http.Handler to lift the hard requirement to labstack/echo. That way a user could use any HTTP router / framework. For the main package of mbtileserver I'd stick with echo, as you have probably chosen it for a reason.

I have no good idea for the caching, yet. Maybe it can be implemented as a middleware as well? The caching part of GetTile would be a http.Handler and the cacheGetter would then call the net/http.Handler from above. and cache the body of the response.

What do you think?

[fawick]

I started to work on this here: https://github.com/fawick/mbtileserver/tree/splitIntoPackages (removed)

Would you prefer me to submit atomic PRs after each step or just one PR in the end? Personally, I'd prefer the first, but it's your call.

[brendan-ward]

Thanks for starting on this @fawick !

I'd like atomic PRs; that will help us refine as we go. Don't feel like you have to do it all - I'm happy to jump in and do this too.

I wonder if we shouldn't upframe this a little bit. Let me know your thoughts on this:

• new repo for mbtiles I/O. (I'd prefer this under an organization account rather than personal account. Using consbio for org is an option. Creating a new org is also an option)
• include that as a library used by mbtileserver, which could provide basic HTTP operations in an http package under mbtileserver. This would include the response handlers for both regular tiles and ArcGIS type tile services.
• main.go here could then use CLI and web framework of choice for starting server and muxing requests?

This seems like it would give us better separation of concerns between a repo intended to be an I/O library for mbtiles, a library for formatting HTTP responses for tile / ArcGIS endpoints, and an executable that runs a server.

[brendan-ward]

Re: caching. Our use of groupcache is total overkill for how we are using it here.

I like the idea of a handling this in the application rather than the library (cache the response data rather than the tile data), perhaps as middleware.

[fawick]

Great, it looks like we have pretty much the same understanding on how to proceed.
I'm happy to bootstrap all of this and shape it in a way so I can use it easily it maputnik/desktop.

Only one comment: For the moment I'd rather keep development in this repo until things are fleshed out. That would mean to keep the mbtiles package here for the moment. Moving that package to another repo later is easy.

I already have a commit that refactors mbtiles.go into its own package and starts using it in main.go. This is going to be my first PR which I create in a moment.

[fawick]

Thanks for your reviews on the PRs #41 and #42. I've included the requested changes.

Further on, I am currenty adding a type ServiceSet to package handlers, which will be providing the http.Handler. This will probably take a few days as I can only do this in my limited personal time.

I understand that the GZIP compression is currently done with labstack/echo/middleware.Gzip. I could not get anything about this out of mbtiles-spec, so I wonder: Is the GZIP compression mandatory for vector tiles? If so, I was considering to use https://github.com/NYTimes/gziphandler instead, as it provides a method function that is compatible to the API we aim for in package handlers.

PS: gorilla/handlers has one, too.

[fawick]

Okay, scratch that question, I read the code more closely and saw that the GZIP middleware is not applied to the GET of the tile itself.

[fawick]

@brendan-ward Thank you very much for your reviews and your many great suggestions to make the code better.
With #50 now being merged, the next thing I should probably start working on are the ArcGIS endpoints. Would you agree that these should also be in package handlers, or is it better to start a new package for them?

I do not have access to an ArcGIS installation (and I have never worked with ArcGIS), so I am unsure how to do an manual integration test for these endpoints. Can you recommend some tool for that? Would they work with, say, QuantumGIS?

Finally, I have some additional exported methods for package handlers in mind, that would allow to only install some of the endpoints. For example, in maplibre/maputnik#148 we only need the endpoints for serving the TileJSON and the tiles, but not the templates.

Another thing probably would be to start (re)implementing a caching middleware in the main package.

[brendan-ward]

@fawick Thank YOU for doing all the work to make this better!!

The arcgis endpoints can go into handlers/arcgis.go since they are handlers, but I suppose for use in maputnik/editor we wouldn't necessarily want those endpoints available by default? I'm not sure exactly what you have in mind for conditionally installing endpoints, but maybe that would work here as well?

The arcgis endpoints cold also go into a dedicated package under handlers handlers/arcgis if that seems like a better way of isolating them.

For testing them, I always just hit the endpoints directly (manually) to make sure they respond as expected. There is no need to try and consume them from another application. This is where I really need to learn how to write tests in Go that exercise HTTP endpoints, just haven't had the time.

Do you want me to run with updating those? They seem orthogonal to your needs for integration with maputnik/editor.

I think next up should be your conditional installation of endpoints.

Is the caching middleware of the tiles something that would be at all useful for maputnik/editor? Seems to me like it would be useful there, and it would just be up to the consumer of the library to choose to use it. There might be a really simple, in-memory implementation that we could use here (controlling write locks would probably be hardest part). My original use of groupcache was probably orders of magnitude more complex than we actually needed.

If we are open to swapping out one 3rd party dependency for another, these look promising but would need investigation:

• go-cache
• freecache

[fawick]

I'd be grateful and glad if you like to take a shot at the ArcGIS handlers, as I relly know next to nothing about ArcGIS and am only guessing the meaning and use cases of these endpoint from the current implementation.

I can start on the smaller endpoint set. I will implement this as a function very similar to ServiceSet.Handler, but leaving out the /map endpoint and the service lister endpoint. I even consider making the return value an *http.ServeMuxand then calling this method from Handlerto simply add the maps and the lister again and return the extended http.ServeMux as the http.Handler.

For the caching middleware: I am inclined to put the caching middleware into the main package, and even basing it on labstack/echo and groupcache but with the request URL path as the key. But I am without preferences for the 3rdparty cache package. The main reason I suggested the caching was that I did not want to take that feature away from mbtileserver, as you had probably added for a good reason in the first case. For maputnik/editor (and for another project with mbiles and handler) I do not plan to use any caching.

What do you think?

[fawick]

@brendan-ward Thank you very much again for being so open and for all the collaboration on this!