Skip to content

fix geocode cache key ignoring the geojson variant - #137

Open
munzzyy wants to merge 1 commit into
FoggedLens:masterfrom
munzzyy:fix/geocode-cache-key-geojson
Open

fix geocode cache key ignoring the geojson variant#137
munzzyy wants to merge 1 commit into
FoggedLens:masterfrom
munzzyy:fix/geocode-cache-key-geojson

Conversation

@munzzyy

@munzzyy munzzyy commented Aug 3, 2026

Copy link
Copy Markdown

/geocode and /geocode/multi both go through NominatimClient.geocodePhrase, and they ask for different things. The single endpoint passes includeGeoJson: true so the map can draw the city boundary; the multi endpoint passes false. Both then look the result up under the same cache key:

const cacheKey = `geocode:${query}`;

So whichever endpoint asks first wins that query for the next 24 hours. If /geocode/multi gets there first, a later /geocode for the same place is served the cached entry, which has no geojson on it, and no Nominatim call is made to go get it. Map.vue reads result.geojson to set the boundary polygon and to work out the zoom level, so the search quietly lands on the place with no boundary drawn.

The parameter was added in #109; the cache key is from back when there was only one variant.

The fix puts the variant in the key. I also added a test for the cached path, since there wasn't one.

Verifying

api/services/NominatimClient.test.ts stubs fetch and counts the upstream calls. On master:

$ bun test services/NominatimClient.test.ts
45 |
46 |     await client.geocodePhrase(query, false);
47 |     const withGeoJson = await client.geocodePhrase(query, true);
48 |
49 |     expect(urls.length).toBe(2);
                             ^
error: expect(received).toBe(expected)

Expected: 2
Received: 1

      at <anonymous> (.../api/services/NominatimClient.test.ts:49:25)
(fail) NominatimClient.geocodePhrase > does not serve a cached geojson-free result to a caller that asked for geojson [21.03ms]

 1 pass
 1 fail
 2 expect() calls
Ran 2 tests across 1 file. [271.00ms]

One upstream call instead of two, because the second request came back from the cache.

With the fix, the whole api suite:

$ bun test
bun test v1.3.14 (0d9b296a)

 7 pass
 0 fail
 15 expect() calls
Ran 7 tests across 2 files. [225.00ms]

The second test in the file asks for the same variant twice and still expects a single upstream call, so this isn't just turning the cache off.

One thing worth flagging: the disk store lives at a fixed /tmp/nominatim-cache and outlives the test process, so the tests generate a query string they haven't cached before rather than assuming an empty cache. Happy to swap that for an injectable cache if you'd rather have the seam.

geocodePhrase gained an includeGeoJson parameter in FoggedLens#109, but the cache key
still only used the query. Whichever endpoint asked first won the cache
entry for 24 hours, so /geocode could return a result with no geojson when
/geocode/multi had already cached that query.

@kawacukennedy kawacukennedy left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice catch — this is a real bug. /geocode (single, via geocodeSingleResult) and /geocode/multi both flow through geocodePhrase, but the single endpoint requests includeGeoJson: true (so the new map can draw the boundary and derive zoom from result.geojson), while multi requests false. Under the old key both variants collided for 24h, so whichever endpoint asked first could starve the other of geojson.

The fix is correct

Keying by variant (geojson vs plain) is exactly the right granularity. I verified it against the route in app.ts/server.ts — both endpoints call geocodePhrase(query, includeGeoJson) with different values, so the two variants are genuinely different responses that must not share a cache entry.

Tests are well-designed

  • First test proves the regression: requesting the plain variant then the geojson variant produces two upstream calls (urls.length === 2), the second carrying polygon_geojson=1 and returning geojson.
  • Second test guards against the obvious over-correction: same variant twice still hits the cache (one call), so you haven't just disabled caching.
  • Using a unique query per run works around the fixed /tmp/nominatim-cache disk path persisting between runs — reasonable given the current no-seam cache setup you flagged.

One small thing, not blocking: the disk cache at /tmp/nominatim-cache is shared across the whole API and survives restarts, so the "unique query" strategy (and the geojson keying) depends on the cache never being empty-cold in an unintended way. If you ever add an injectable cache seam (you mentioned you'd be happy to), that would let these tests control TTLs and make them fully deterministic. Happy to help take that on in a follow-up.

Good, focused fix — this resolves the map landmarks/zoom issue cleanly.

@munzzyy

munzzyy commented Aug 30, 2026

Copy link
Copy Markdown
Author

thanks for looking it over. the cache seam is still on my list once this lands, happy if you beat me to it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants