feat: return the whole Cache from GET /layer/{layerName}/{cacheType} - #175
feat: return the whole Cache from GET /layer/{layerName}/{cacheType}#175razbroc wants to merge 7 commits into
Conversation
IMapProxyJsonDocument.caches was typed as a single Cache rather than a map of them, so every caches[name] lookup resolved to any through the Cache type's permissive index signature, and the casts around those lookups were unchecked. No behaviour change.
The endpoint returned only the Cache Source, so a consumer needing the Cache's
grids, format or sources had to fall back on the deprecated GET /layer/{name},
which has no Cache Type selection and cannot reach a Redis Cache at all.
The Cache is now spread verbatim under its resolved name, with the Cache Source
left in place at 'cache' so existing consumers are unaffected and the new fields
are additive. Cache Type resolution is unchanged.
readCacheType lifts the Cache Type out of an untrusted entry, so an entry that is
not an object or holds no Cache Source is the same 400 as any other Cache Type
mismatch instead of a 500. There is deliberately no notion of a malformed Cache:
nothing else about the entry is inspected.
Every Cache Source schema now requires only 'type' and permits additional properties, so a Cache Source missing an optional field no longer makes a valid response contract-invalid. getCacheResponse requires only cacheName and cache, documents sources, grids, format, upscale_tiles and minimize_meta_requests as optional, and permits additional properties at both levels: production redis Caches carry neither upscale_tiles nor minimize_meta_requests, so anything stricter would make real responses violate their own spec. geopackage was an accepted cacheType with no response schema. It is added to both the oneOf and the discriminator mapping - without the mapping entry a real geopackage response fails the contract regardless of tests. The response examples showed only the Cache Source and are updated to the whole Cache. The request schemas are separate objects and are untouched.
…Type exists The Cache Type mismatch answered 400 on master, with the message 'layer cache not found with requested cache type'. The status and the message disagreed, and the message was the honest half: when the Cache Type is in the enum but the Layer owns no Cache of that type, nothing about the request is malformed - the addressed Cache simply is not there. Every 'the addressed Cache is not there' case now answers alike: Layer absent, no Cache under the resolved name, and a Cache Type that cannot be confirmed. A Cache Type outside the enum still answers 400 from request validation, before the manager runs. The unreadable entry follows the mismatch to 404 rather than keeping a 400 of its own. Telling 'the Cache Type is confirmably something else' from 'the Cache Type could not be confirmed' would reintroduce the malformedness concept the design deliberately does not carry, and a 400 would blame the caller for the server's own corrupt configuration. The operator's signal stays the warn log carrying the offending entry. BEHAVIOUR CHANGE to a released endpoint: a consumer branching on 400 sees 404.
…e check in LayersManager
| format: string; | ||
| upscale_tiles?: number; | ||
| cache: ICacheSource; | ||
| cache?: ICacheSource; |
There was a problem hiding this comment.
Also the openapi.yaml is not sync with cache been optional:
Lines 356 to 363 in 2d959b1
There was a problem hiding this comment.
It should not be, and it is required again as of b0b88b4.
It only became optional so that requestedCache.cache?.type would type-check. caches is read straight off the mapproxy configuration and production holds entries that are not well formed Caches, so that read has to survive a missing Cache Source — but weakening the interface made every other call site pay for one untrusted read.
Restored readCacheType(cache: unknown) in common/utils.ts instead: it is the single place that reads a caches entry without trusting it, and it keeps IMapProxyCache.cache: ICacheSource required everywhere else. Behaviour is unchanged — an entry stating no Cache Type still answers 404 like any other Cache Type mismatch.
There was a problem hiding this comment.
Right — and this resolves itself with the above: cache is required again in IMapProxyCache, so getLayerResponse requiring cache is back in sync and needs no change. redocly lint passes.
(getCacheResponse, the schema for this endpoint, deliberately requires only cacheName and cache and allows additional properties — that one is about upscale_tiles/minimize_meta_requests, which real redis Caches genuinely lack.)
There was a problem hiding this comment.
Decide if you keep the cache optional or not
There was a problem hiding this comment.
Correction to my previous reply — I had reverted this to required, and that was wrong. Optional is the intended design, restored in e6b205d.
An entry of the configuration caches section is not guaranteed to be a well formed Cache: production holds entries that state no Cache Source. cache is therefore genuinely optional, and every read narrows through it rather than assuming it is there. Added a doc comment on the field saying so, so it does not read as an accident again.
There was a problem hiding this comment.
This one stands, then — with cache staying optional the openapi did need the fix, done in e6b205d.
getLayerResponse no longer requires cache. GET /layer/{name} returns the entry verbatim, so an entry with no Cache Source was producing a response that violated its own schema. redocly lint passes and the integration tests assert toSatisfyApiSpec.
getCacheResponse is left requiring cache on purpose: that endpoint only returns once the Cache Source has been confirmed to match the requested Cache Type, so it really is guaranteed there.
|
Great job! |
…t of logs - Restore `readCacheType`, the untrusted read of a `caches` entry, so `IMapProxyCache.cache` stays required. It was only made optional to let `?.` type-check against a possibly malformed entry, which weakened the type for every other call site and put it out of sync with the `getLayerResponse` openapi schema. - Drop `requestedCache` from the Cache Type mismatch warn log: a redis Cache Source carries a password. The operator signal is now `cacheName` and `foundCacheType`, which says the same thing without credentials. - Spread the Cache before `cacheName` so the resolved name always wins.
|
Thanks! Agreed on the direction, but I would rather not do it in this PR.
There is also a shape question worth settling first: Happy to open a follow-up ticket for it — the openapi The four inline comments are addressed in b0b88b4. |
|
Remove all the irrelevant comments that been added in this commit b0b88b4 |
| return layerName.endsWith('-redis'); | ||
| } | ||
|
|
||
| /** |
There was a problem hiding this comment.
Why we need this function- looks little bit over kill for this specific check, take a second look please.
An entry of the configuration's `caches` section is not guaranteed to be a
well formed Cache — production holds entries that state no Cache Source — so
`IMapProxyCache.cache` is optional by design and every read narrows through it.
`getLayerResponse` no longer requires `cache`, which is what the type has been
saying all along: `GET /layer/{name}` returns the entry verbatim, so a response
without a Cache Source was violating its own schema.
2 participants
Related issues: MAPCO-11508
Further information:
GET /layer/{layerName}/{cacheType}returned only the Cache Source — the storage backend nested undercache— so a consumer needing the Cache'sgrids,formatorsourceshad to fall back on the deprecatedGET /layer/{name}, which has no Cache Type selection and therefore cannot reach a Redis Cache at all.Three commits:
1.
fix:type the mapproxy caches section as a map of Caches — prefactor, no behaviour change.IMapProxyJsonDocument.cacheswas typed as a singleIMapProxyCache, so everycaches[name]lookup resolved toanythrough the Cache type's permissive index signature. Threeno-unsafe-member-accesssuppressions deleted.2.
feat:return the whole Cache — the Cache is spread verbatim under its resolved name, with the Cache Source left in place atcache, so existing consumers are unaffected and every new field is additive. Cache Type resolution is unchanged: a redis request still resolves to the-redissuffixed Cache, and the returnedcacheNameis the resolved name.readCacheTypelifts the Cache Type out of an untrusted entry, so an entry that is not an object, or holds no Cache Source, is now the same answer as any ordinary Cache Type mismatch rather than a 500. There is deliberately no notion of a malformed Cache — nothing else about the entry is inspected. Existing error message wording is preserved.3.
feat:loosen the Cache Source schemas, publish the geopackage schema — every Cache Source schema now requires onlytypeand permits additional properties;getCacheResponserequires onlycacheNameandcache, documents the five optional fields, and permits additional properties at both levels. This is forced by production data: redis Caches in the live configuration carry neitherupscale_tilesnorminimize_meta_requests, so anything stricter would make real responses violate their own spec.geopackagewas an acceptedcacheTypewith no response schema; it is added to both theoneOfand the discriminator mapping. Request schemas are separate objects and are untouched — insert validation is unchanged.4.⚠️ behaviour change to a released endpoint.
fix:answer 404 rather than 400 when no Cache of the requested Cache Type exists —The mismatch answered 400 on master, with the message
layer cache not found with requested cache type. The status and the message disagreed, and the message was the honest half: when the Cache Type is in the enum but the Layer owns no Cache of that type, nothing about the request is malformed — the addressed Cache simply is not there. Live on dev,GET /layer/TEST_LAYER-Orthophoto/s3returned 400 for a Layer whose Caches arefileandredis.Every "the addressed Cache is not there" case now answers alike — Layer absent, no Cache under the resolved name, and a Cache Type that cannot be confirmed. A Cache Type outside the enum still answers 400, from request validation, before the manager runs, so the endpoint keeps both statuses and the openapi document needs no change.
The unreadable entry follows the mismatch to 404 rather than keeping a 400 of its own: telling "the Cache Type is confirmably something else" from "the Cache Type could not be confirmed" would reintroduce the malformedness concept this design deliberately does not carry, and a 400 would blame the caller for the server's own corrupt configuration. The operator's signal stays the warn log carrying the offending entry.
A consumer branching on 400 for this endpoint will see 404 instead. Risk is low — the premise of this ticket is that consumers were pushed to the deprecated
GET /layer/{name}— but it is a real contract change and should be called out at release.Nine cases now covered at the HTTP seam, each asserting
toSatisfyApiSpecalongside the body: s3 whole Cache; redis resolving to the-redisCache and legitimately lackingupscale_tiles; unmodelled mapproxy options surviving verbatim at both levels; an entry that is not an object → 400; an entry with no Cache Source → 400; a genuine Cache Type mismatch → 400; an unknown Layer → 404; a Layer with no Cache under the resolved name → 404; acacheTypeoutside the enum → 400.The deprecated
GET /layer/{name}is not touched, and neither is the Layer-name validation it shares with the insert and update flows.Geopackage fixture and coverage follow in a second PR on this ticket.