fix geocode cache key ignoring the geojson variant - #137
Open
munzzyy wants to merge 1 commit into
Open
Conversation
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.
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
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.
/geocodeand/geocode/multiboth go throughNominatimClient.geocodePhrase, and they ask for different things. The single endpoint passesincludeGeoJson: trueso the map can draw the city boundary; the multi endpoint passesfalse. Both then look the result up under the same cache key:So whichever endpoint asks first wins that query for the next 24 hours. If
/geocode/multigets there first, a later/geocodefor the same place is served the cached entry, which has nogeojsonon it, and no Nominatim call is made to go get it.Map.vuereadsresult.geojsonto 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.tsstubsfetchand counts the upstream calls. On master:One upstream call instead of two, because the second request came back from the cache.
With the fix, the whole
apisuite: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-cacheand 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.