feat(server): serve tiles only through the OGC surface (MAPCO-11483, MAPCO-11484, MAPCO-11485) - #9
Merged
Merged
Conversation
The two non-standard capabilities endpoints go. Service and map discovery are
the OGC landing page, /conformance and /collections, which cover the same ground
in standard form, and a map's TileJSON is
/collections/{map}/tiles/{tileMatrixSetId}?f=tilejson.
Both routes are unregistered, not merely unhandled: TestCapabilitiesRemoved asks
the router for every spelling either endpoint answered on -- /capabilities/:map_name
matched with and without an extension, because the handler split the extension
off the parameter itself rather than routing on it.
The map style endpoint survives this change (MAPCO-11485 removes it) and named
the capabilities document as its vector source, so it now names the OGC tileset
resource for the map's default scheme -- the same scheme the capabilities
document described. Without that it would have served a document pointing at a
404. The debug parameter is deliberately not carried into that URL: the OGC
surface has no debug layers, so it would name a parameter the target ignores.
Two pieces of machinery that existed only to build these responses go with them,
which is why this is one commit and not three:
- server.TileURLTemplate and its TileURL* constants. Their only non-test callers
were the two handlers; what remained was a type exercised solely by its own
test.
- server.Version. HandleCapabilities was its only reader, so it was left a
write-only package variable, still assigned by both entrypoints. Its comment
claimed "various Middleware" used it; none did.
The TileJSON package is untouched. All three members the review.md remediation
added -- TileJSON.CRS, TileJSON.TileMatrixSetID and VectorLayer.Fields -- are set
by server/ogc/tilejson.go and asserted by the OGC tests, so none of them was
capabilities-only. The ticket anticipated otherwise.
TestOGCMount's "the native routes are untouched" case asked /capabilities for a
200; a native tile route is what is left to show the OGC mount did not displace
the native surface. TestHostName and TestHTTPS used /capabilities as a sample
URL and now name routes that exist.
Removes the two endpoint blocks from the README and rewrites the paragraph in
docs/ogc-api-tiles.md that described /capabilities/{map}.json as the place a
map's default scheme is reported: that is the OGC tileset resource now, and
`?f=tilejson` is what adds Shigola's crs and tileMatrixSetId members.
The uri_prefix documentation in server/README.md described the prefix as
applying to "the capabilities endpoint responses". It applies to every URL the
server emits, which is what it now says -- and a typo in it is fixed on the way
past.
CHANGELOG.md keeps its historical entries: it is Tegola's inherited release
history, not a description of the current surface.
Coverage Report for CI Build 716Coverage decreased (-1.1%) to 47.123%Details
Uncovered Changes
Coverage Regressions1 previously-covered line in 1 file lost coverage.
Coverage Stats
💛 - Coveralls |
51ab3be deleted this file to re-home its debug subtest into handle_map_style_test.go, and took the other two subtests with it. That left MAPCO-11483's only acceptance criterion -- both capabilities endpoints return 404 -- with no executable assertion anywhere in the tree, and left a5cb2e5's commit message citing a test that no longer existed. The routes are unregistered rather than merely unhandled, and that is the property worth guarding: deleting a handler is a compile error, but unregistering one spelling of a route and missing another is silent. /capabilities/:map_name answered both with and without an extension, because the handler split the extension off the parameter itself rather than routing on it, so all three spellings are asked of the router. Verified by mutation: re-registering the two routes fails the subtest. The debug subtest is not restored -- TestHandleMapStyleDebug covers it, where it belongs. It is a property of the style endpoint, not evidence about the capabilities routes, which is why this file no longer claims it.
11f321e built the OGC source and then unbuilt it for the debug case, clearing the URL it had just assembled. An if/else yields one source instead, so neither branch is written twice. The debug predicate was also evaluated twice, the second time by reading a side effect of the first (debugQuery, populated under the same condition). One bool removes that coupling, and the query is now built where it is used. Records why the debug template is assembled with fmt rather than url.URL, which was previously unstated: url.URL escapes the {z}/{x}/{y} placeholders a Mapbox tile template has to carry literally, emitting them as %7Bz%7D.
a051be5 wrote this path as /collections/{map}/tiles/{tileMatrixSetId}, while the route table on the same page, and the section below it, use {collectionId}. A map name is a usable collection id, so both are correct; only one is consistent.
MAPCO-11483 removed server.Version with the /capabilities endpoint that read it, which took the service's only report of its own build with it. There is no way left to ask a running server which binary is answering. It comes back on the OGC surface rather than as the old package variable. The landing page gains a shigolaVersion member and the API definition gains info.x-shigola-version, both fed from a Version field on ogc.Config, which server.NewRouter fills from internal/build. That is strictly better than what was removed: server.Version was a write-only global each entrypoint had to remember to assign -- and the lambda entrypoint assigned it separately -- while a router built anywhere now reports the version without being told. Both members are extensions, deliberately: - OGC API - Common defines no version member on the landing page, so this one is named for the product. Calling it "version" would collide with any member the specification later defines there, and would read as the version of the API rather than of the server. - OpenAPI's info.version is the version of the API being described. It is fixed by the specification this surface implements and does not move when the binary is rebuilt, so the build sits beside it as an x- extension. Overwriting it would make a client pinning an API version track our release cadence. An empty Version omits both members rather than reporting "" as a version. info is copied before the extension is added: the parsed OpenAPI document is a package-level value shared by every request, so writing through it would leak one service's version into another's document. TestVersionReported covers that directly, along with both members and the empty case.
/maps/{map}/style.json and the mapbox/style package behind it go (MAPCO-11485).
Styling is a separate specification -- OGC API - Styles -- which this server
does not implement, so a generated Mapbox GL style was a non-standard surface on
a server whose scope is OGC API - Tiles. Clients bring their own style and point
its vector source at a tileset's TileJSON, which the docs now show.
Nothing replaces it, and that is the point: the generated style guessed layer
colours by hashing layer names, which is not a decision a tile server is better
placed to make than the client rendering the tiles.
Removed with it:
- mapbox/style, whose only importer was this handler. mapbox/tilejson stays --
the OGC surface serves it.
- gopkg.in/go-playground/colors.v1, which only the colour guessing used. It is
the one dependency this ticket drops; go.mod, go.sum and vendor/ follow.
- stringToColorHex and its test, for the same reason.
TestNativeRoutesRemoved asks the router for both spellings the handler answered
on. The route matched /maps/:map_name/style.json literally, but the handler split
the extension off the map_name parameter itself, so it is asked for /style too.
MAPCO-11484 extends that test to the tile routes, after which nothing under the
native map prefix is registered.
The capabilities removal test loses its subtest about the style document's
source: it asserted that the style did not point at a deleted capabilities URL,
and there is no style document left to assert about. The 404 guard it was added
for is untouched.
/maps/{map}/{z}/{x}/{y} and /maps/{map}/{layer}/{z}/{x}/{y} go, with the handler
behind them and the tile-cache middleware that wrapped them (MAPCO-11484).
Tiles are served only through the OGC collections surface, so a tile has exactly
one URL. Nothing is registered under /maps any longer, which is also the last
acceptance criterion of MAPCO-11485.
TestNativeRoutesRemoved asks the router for every spelling both routes answered
on -- with and without the layer segment, with and without the .pbf extension the
handler trimmed off the y parameter -- and then asks the OGC route for the same
tile, so the 404s read as a moved surface rather than a lost one.
The cache write moved with the caching, not just the code
------------------------------------------------------------------
The OGC handler already cached inline, so caching survives the middleware's
deletion untouched: TestTileCaching covers hit, miss and the Shigola-Cache
header on that path.
What did not survive automatically is the response-path property the middleware
existed to protect. It wrote to the cache after flushing the tile to the client;
the OGC handler wrote to the cache first, so a slow tier delayed a tile nothing
was waiting on. The write now happens after the response, behind an explicit
flush, in ogc.cacheAfterResponse -- net/http buffers a small tile until the
handler returns, so the flush is what makes the ordering mean anything. Verified
by mutation: restoring the old order fails TestFlushBeforeCacheWrite in both
Accept-Encoding cases, which is why that test moved to the OGC route rather than
being deleted with the middleware it used to drive.
The unflushable-writer warning moved with it, still firing once per process.
Metric label space
------------------------------------------------------------------
BREAKING for dashboards. The prometheus http "handler" label keeps the request's
actual value only for route variables named in `variables` (default
observeVars); everything else collapses to its ":name". None of the old defaults
-- :map_name, :layer_name, :z -- appears in an OGC route path, so without a
change every tile request collapsed to a single label value and the per-map and
per-zoom breakdown was silently gone.
:collection_id, :tile_matrix_set_id and :tile_matrix are added to the default.
:tile_row and :tile_col are deliberately left out, as :x and :y always were: a
label per tile is a cardinality explosion, not a metric. The old defaults stay,
because the same list also names the cache metrics' labels, which read from the
request context rather than the path -- dropping them would silently drop
map_name, layer_name and z from every cache metric.
A series carrying /maps/<map>/<layer>/<z>/:x/:y no longer exists. The equivalent
is /collections/<collectionId>/tiles/<tileMatrixSetId>/<tileMatrix>/:tile_row/:tile_col.
Two things this makes unreachable, deliberately recorded rather than removed
------------------------------------------------------------------
- `[[maps.params]]`. The per-layer tile route was the only caller that passed
parameters to Map.Encode; the OGC route passes none. The keys still validate,
so configs keep loading, but nothing reads them. config.go said "caching is
disabled for these maps" -- true when the route read them and its middleware
skipped cached query strings, and now actively misleading, so it says what is
actually the case.
- `?debug=true`. Only the native route honoured it, so atlas.Map.AddDebugLayers
now has no callers. The debug provider is still registrable, and the docs show
configuring those layers explicitly instead. AddDebugLayers is left in place:
it is atlas API, and deleting it is not this ticket's to do.
Also gone: server/errors.go. ErrMalformedTileTemplateURL was orphaned by
MAPCO-11483 -- it described the format of a native tile template URL, which is
now not a thing at all.
Tests that exercised middleware through a native URI (gzip, headers, CORS) now
do it through an OGC tile URI; they went through NewRouter, so the URI had to be
a registered route. CORSTest gained a `server.Headers = nil`: it asserts the CORS
defaults, which configured headers override, and it only ever passed because the
CORS cases lived in files the compiler ordered ahead of TestMiddlewareHeaders.
TestOGCMount's "the native routes are untouched" case became "a tile is served
under the mounted surface" -- there is no other surface left for the mount to
displace.
The user-facing docs carried a running commentary on the routes that used to exist -- a was/now table, "which has been removed", ticket keys -- which is history a reader coming to Shigola now has no use for. What a reader does need is what is true: the tile path is z/y/x, every request names a tiling scheme, there is no style document, there is no debug parameter, and configured map parameters are not read. Each of those statements stays; only the framing goes. Facts that were carried by the removal framing are re-attached where they belong -- the z/y/x order is worth checking against a client that assumes z/x/y, and `pbf` is accepted because it is the name in the TileJSON `format` member rather than because an older route used it. Also drops the same framing from the OpenAPI document's tile description, which is served to clients. Two things deliberately kept: - README's "What Shigola adds" still mentions Tegola's /maps/... routes. That section exists to say how this fork differs from upstream, and upstream does serve them -- it is not this server describing its own past. - Code comments keep their rationale and ticket references. That is design history for people editing the code, which this repo asks to be preserved, and no user reads it.
Three files were each enumerating paths this fork once answered on: capabilities_removed_test.go, native_routes_removed_test.go, and TestOGCMount's viewer case. Same shape in three places, and each one only as good as the paths whoever wrote it thought to list. They become one TestRouterSurface, in two halves: the OGC resources return 200, and everything else returns 404. Both halves are needed -- the 404 half alone would pass against a router that registered nothing at all. The point is the framing. "Anything outside the OGC surface is a 404" is a closed statement about the surface, so it also holds for whatever anyone adds at the root later, which none of the three files did. /capabilities, /maps/... and /viewer are instances of that rule now rather than the specification, which is why they no longer have a test each. Coverage of MAPCO-11483's acceptance criterion is unchanged: all three capabilities spellings are still asserted, including the .json one, which only mattered because that handler split the extension off its own parameter rather than routing on it. What the test guarantees, precisely: the response is a 404, not that no route is registered. A route registered with a handler that itself 404s passes. That is the right line -- a route serving nothing is not a surface -- but it is worth knowing, because it is what a naive mutation check gets wrong. Verified with a handler that actually serves: re-registering /capabilities, the style route, both tile routes and /viewer fails ten of the twelve paths. TestCORS moves to server_cors_test.go, beside the CORSTest helper it calls, rather than living in a file about removed routes.
The pattern was `-`, which is read as a path glob and matches a file literally named "-". Nothing is, so the file assigned ownership of nothing and no review was ever auto-requested -- the same effect as having no CODEOWNERS at all, but looking like it worked. `*` is what makes the one entry mean "the whole repository", which is what 105e9a1 intended.
TestRouterSurface's "nothing outside it answers" case listed twelve paths this fork used to serve -- /capabilities, /maps/..., /viewer -- and asserted each 404s. On a branch removing those routes that was evidence. On master it is not a test of this code: the router registers no catch-all, so any unregistered path 404s whatever we do, and nothing would lead anyone back to those paths for it to catch. It costs a reader twelve lines of dead surface to read past. What remains is the half that tests something: every resource NewRouter registers is reachable through it. That is not implied by anything -- a handler registered at a path with a typo, or never registered, is not a compile error, and the ogc package's own tests mount its routes themselves rather than through NewRouter, so this is the only place the assembled surface is exercised. Mutation-verified: dropping /tileMatrixSets from the OGC route table fails it. Consequence worth recording, since the tickets in this PR each have a 404 acceptance criterion: none of them now has an executable assertion. The behaviour is unchanged and follows from the routes being unregistered, which is visible in NewRouter -- but nothing tests it, deliberately, and the tickets say so.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Shigola now serves tiles through one surface. This PR removes the three non-standard HTTP surfaces that sat beside OGC API - Tiles — the capabilities endpoints, the map style endpoint, and the native
/maps/...tile routes — and puts back the one thing their removal cost: the service's report of its own build.After it, nothing is registered under
/maps, and every route the server answers on belongs to the OGC surface (plus/metrics).Jira
The three landed in that order deliberately: the style endpoint's vector source named a tile URL, so removing the style endpoint before the tile routes avoids an intermediate commit where the style document points at a 404.
Companion PR
(The earlier docs PR for MAPCO-11483, MapColonies/shigola-docs#4, is already merged.)
What changed, and why
The capabilities endpoints (
a5cb2e55,a051be5b). Service and map discovery are the OGC landing page,/conformanceand/collections; a map's TileJSON is/collections/{map}/tiles/{tileMatrixSetId}?f=tilejson.server.TileURLTemplateandserver.Versionwent with them as their only readers.The version came back (
e7fbb8e2). Removingserver.Versionleft no way to ask a running server which binary is answering. It returns on the OGC surface:shigolaVersionon the landing page,info.x-shigola-versionin the API definition, both fed from aVersionfield onogc.ConfigthatNewRouterfills frominternal/build. That is better than what was removed —server.Versionwas a write-only global each entrypoint had to remember to assign, and the lambda entrypoint assigned it separately, while a router built anywhere now reports the version without being told. Both members are extensions on purpose: OGC API - Common defines no version member, and OpenAPI'sinfo.versionis the version of the API, which does not move when the binary is rebuilt.The map style endpoint (
3ba96e62). Styling is a separate specification — OGC API - Styles — which this server does not implement, and the document it generated guessed layer colours by hashing layer names. Nothing replaces it; clients bring their own style pointed at a tileset's TileJSON.mapbox/stylegoes with it, and with it thegopkg.in/go-playground/colors.v1dependency — the one dependency this PR drops.The native tile routes (
c1ae7023). Both routes, the handler, and the tile-cache middleware. The OGC handler already cached inline, so caching survives untouched — but the response-path property the middleware existed to protect did not: it wrote to the cache after flushing the tile, while the OGC handler wrote first. That is fixed inogc.cacheAfterResponse, which writes after the response behind an explicit flush.Verification
go build -mod vendor ./...,gofmt -s -l .(excluding vendor) — cleango test -mod vendor -race ./...andCGO_ENABLED=0 go test -mod vendor ./...— greengo vet ./server/... ./config/... ./observability/...— one pre-existing finding (prometheus.gopasses a lock by value; present onmaster, tracked by MAPCO-11498)/tileMatrixSetsfrom the OGC route table failsTestRouterSurfaceTestFlushBeforeCacheWritein bothAccept-Encodingcasesshigola-docs:npm run build— passes, which is what catches a broken link or anchorCITE has not been run locally for this PR; CI runs it on
server/ogc/**changes, which this touches.Breaking changes
/maps/...,/capabilities...and/maps/{map}/style.jsonreturn 404. Two things a client has to get right on the OGC path: tile paths are z/y/x, and every request names a tiling scheme where the native routes served the map's default implicitly.The docs deliberately do not carry a was/now migration table (
93250d26). They describe what the surface is rather than what it replaced — the facts a client needs are attached to the routes they apply to, but there is no commentary on routes that used to exist. A reader upgrading gets those facts from the tile route's own description; a reader arriving fresh is not told about a surface they never used.Prometheus dashboards. No series carries
handler="/maps/<map>/<layer>/<z>/:x/:y"any more. The equivalent is/collections/<collectionId>/tiles/<tileMatrixSetId>/<tileMatrix>/:tile_row/:tile_col. The defaultvariableslist gained:collection_id,:tile_matrix_set_idand:tile_matrixso the per-map and per-zoom breakdown survives in the new vocabulary — without that, every tile request collapsed to one label value. The old names stay in the list because the same list also names the cache metrics' labels, which read from the request context rather than the path.[[maps.params]]is configurable but unread. The per-layer tile route was the only caller that passed query parameters toMap.Encode. Configs keep loading and the keys still validate; the startup log line changed from "caching is disabled for these maps" — true when the route read them — to saying they have no effect.?debug=trueis gone. Only the native tile route honoured it. Thedebugprovider is still registrable, and the docs show configuring those layers explicitly.Deliberately left out
atlas.Map.AddDebugLayersnow has no callers. It isatlasAPI rather than server, so deleting it is not any of these tickets' to do — worth its own.cache.ParseKey/cache.ParseKeyForGridare now exercised only by their own tests; the native cache middleware was their last caller. Same reasoning.go vetfinding above.Each of those three would make a reasonable follow-up ticket; none is filed.
Review follow-ups
A two-axis review of the branch (standards + spec) ran before the last three tickets and found three things, fixed in
9dc18543,0b01e5c4and66edfac9:TestCapabilitiesRemovedhad been deleted.51ab3beere-homed its debug subtest intohandle_map_style_test.goand took the other two subtests with it, leaving MAPCO-11483's one acceptance criterion with no executable assertion. Restored in9dc18543, and since folded intoTestRouterSurface(see below).11f321efassembled the OGC source URL and cleared it again for the debug case, and evaluated the debug predicate twice, the second time by reading a side effect of the first. (That code has since been deleted entirely with the endpoint.)docs/ogc-api-tiles.mdused{map}where the route table on the same page uses{collectionId}.On testing the removals (
5ab8d745,716fbf27)Three files were each enumerating paths this fork once answered on:
capabilities_removed_test.go,native_routes_removed_test.go, andTestOGCMount's viewer case.5ab8d745folded them into oneTestRouterSurface;716fbf27then dropped the enumeration entirely.There is deliberately no test that the removed routes 404, so each ticket's 404 acceptance criterion has no executable assertion. The reasoning: the router registers no catch-all, so any unregistered path 404s regardless of what we do — asserting it tests httptreemux, not this code, and nothing would lead anyone back to
/capabilitiesor/maps/...for such a test to catch. It would be twelve lines of dead surface for a reader ofmasterto read past. The behaviour is unchanged and visible inNewRouter, which registers onlyogcService.Routes().What
TestRouterSurfacedoes test is the half that isn't implied: every resourceNewRouterregisters is reachable through it. A handler registered at a path with a typo, or never registered, is not a compile error, and theogcpackage's own tests mount its routes themselves rather than throughNewRouter— so this is the only place the assembled surface is exercised. Mutation-verified by dropping/tileMatrixSetsfrom the route table.TestCORSmoved toserver_cors_test.go, beside theCORSTesthelper it calls.