Prevent duplicate Nominatim requests during proximity searches

libraries/geolocation.py

@@ -1,16 +1,24 @@
 from __future__ import annotations
 
+import hashlib
 import math
+import time
 from collections.abc import Sequence
+from functools import partial
 from typing import Any
 
 from django.core.cache import cache
 from django.core.files.uploadedfile import UploadedFile
+from geopy.adapters import RequestsAdapter
 from geopy.exc import GeocoderServiceError, GeocoderTimedOut, GeocoderUnavailable
 from geopy.geocoders import Nominatim
 from PIL import ExifTags, Image, UnidentifiedImageError
 
 FORWARD_GEOCODE_CACHE_TIMEOUT_SECONDS = 60 * 60 * 6
+FORWARD_GEOCODE_FAILURE_CACHE_TIMEOUT_SECONDS = 60
+FORWARD_GEOCODE_LOCK_TIMEOUT_PADDING_SECONDS = 1
+FORWARD_GEOCODE_WAIT_INTERVAL_SECONDS = 0.25
+_FORWARD_GEOCODE_FAILURE_CACHE_VALUE = "unresolved"
 
 
 def _normalize_gps_reference(value: Any) -> str:
@@ -28,9 +36,63 @@ def _normalize_gps_reference(value: Any) -> str:
 def _build_forward_geocode_cache_key(*, place_query: str, country_code: str | None) -> str:
     """Build a stable cache key for forward geocoding lookups.
     Keeps repeated place searches fast while respecting rate limits."""
-    normalized_query = place_query.strip().lower()
-    normalized_country = (country_code or "").strip().lower()
-    return f"forward-geocode:{normalized_country}:{normalized_query}"
+    normalized_query = " ".join(place_query.split()).casefold()
+    normalized_country = (country_code or "").strip().casefold()
+    lookup_identity = f"{normalized_country}:{normalized_query}"
+    lookup_digest = hashlib.sha256(lookup_identity.encode("utf-8")).hexdigest()
+    return f"forward-geocode:{lookup_digest}"
+
+
+def _get_cached_forward_geocode_result(
+    *, cache_key: str
+) -> tuple[bool, tuple[float, float] | None]:
+    """Read a successful or failed forward-geocode cache entry.
+    Distinguishes a cached failure from a cache miss for single-flight callers."""
+    cached_value = cache.get(cache_key)
+    if cached_value == _FORWARD_GEOCODE_FAILURE_CACHE_VALUE:
+        return True, None
+    if (
+        isinstance(cached_value, tuple)
+        and len(cached_value) == 2
+        and all(isinstance(value, float) for value in cached_value)
+    ):
+        return True, cached_value
+    return False, None
+
+
+def _cache_forward_geocode_failure(*, cache_key: str) -> None:
+    """Cache an unresolved forward-geocode result for a short period.
+    Prevents immediate retries while preserving the caller's keyword fallback."""
+    cache.set(
+        cache_key,
+        _FORWARD_GEOCODE_FAILURE_CACHE_VALUE,
+        FORWARD_GEOCODE_FAILURE_CACHE_TIMEOUT_SECONDS,
+    )
+
+
+def _wait_for_forward_geocode_result(
+    *, cache_key: str, timeout_seconds: int
+) -> tuple[float, float] | None:
+    """Wait for the active forward-geocode caller to publish its result.
+    Returns None if the single-flight window ends without a cached outcome."""
+    wait_timeout_seconds = max(
+        timeout_seconds + FORWARD_GEOCODE_LOCK_TIMEOUT_PADDING_SECONDS,
+        FORWARD_GEOCODE_LOCK_TIMEOUT_PADDING_SECONDS,
+    )
+    deadline = time.monotonic() + wait_timeout_seconds
+
+    while time.monotonic() < deadline:
+        cache_hit, cached_result = _get_cached_forward_geocode_result(
+            cache_key=cache_key
+        )
+        if cache_hit:
+            return cached_result
+        time.sleep(FORWARD_GEOCODE_WAIT_INTERVAL_SECONDS)
+
+    cache_hit, cached_result = _get_cached_forward_geocode_result(
+        cache_key=cache_key
+    )
+    return cached_result if cache_hit else None
 
 
 def _dms_to_decimal(values: Sequence[Any], reference: str) -> float | None:
@@ -66,23 +128,36 @@ def forward_geocode_place(
 ) -> tuple[float, float] | None:
     """Forward geocode a place string into latitude and longitude.
     Returns None when no usable coordinates are resolved."""
-    normalized_query = place_query.strip()
+    normalized_query = " ".join(place_query.split())
     if not normalized_query:
         return None
 
     cache_key = _build_forward_geocode_cache_key(
         place_query=normalized_query,
         country_code=country_code,
     )
-    cached_coordinates = cache.get(cache_key)
-    if (
-        isinstance(cached_coordinates, tuple)
-        and len(cached_coordinates) == 2
-        and all(isinstance(value, float) for value in cached_coordinates)
-    ):
-        return cached_coordinates
+    cache_hit, cached_result = _get_cached_forward_geocode_result(
+        cache_key=cache_key
+    )
+    if cache_hit:
+        return cached_result
 
-    geolocator = Nominatim(user_agent=user_agent, timeout=timeout_seconds)
+    lock_timeout_seconds = max(
+        timeout_seconds + FORWARD_GEOCODE_LOCK_TIMEOUT_PADDING_SECONDS,
+        FORWARD_GEOCODE_LOCK_TIMEOUT_PADDING_SECONDS,
+    )
+    lock_key = f"{cache_key}:lock"
+    if not cache.add(lock_key, True, lock_timeout_seconds):
+        return _wait_for_forward_geocode_result(
+            cache_key=cache_key,
+            timeout_seconds=timeout_seconds,
+        )
+
+    geolocator = Nominatim(
+        user_agent=user_agent,
+        timeout=timeout_seconds,
+        adapter_factory=partial(RequestsAdapter, max_retries=0),
+    )
     geocode_kwargs: dict[str, Any] = {
         "exactly_one": True,
         "language": "en",
@@ -96,14 +171,17 @@ def forward_geocode_place(
     try:
         location = geolocator.geocode(normalized_query, **geocode_kwargs)
     except (GeocoderServiceError, GeocoderTimedOut, GeocoderUnavailable, ValueError):
+        _cache_forward_geocode_failure(cache_key=cache_key)
         return None
 
     if location is None:
+        _cache_forward_geocode_failure(cache_key=cache_key)
         return None
 
     latitude = getattr(location, "latitude", None)
     longitude = getattr(location, "longitude", None)
     if not isinstance(latitude, (float, int)) or not isinstance(longitude, (float, int)):
+        _cache_forward_geocode_failure(cache_key=cache_key)
         return None
 
     coordinates = (float(latitude), float(longitude))

libraries/test_geolocation.py

@@ -0,0 +1,183 @@
+from collections.abc import Iterator
+from concurrent.futures import ThreadPoolExecutor
+from threading import Event
+from unittest.mock import Mock, patch
+
+import pytest
+from django.core.cache import cache
+from django.db import connections
+from geopy.exc import GeocoderRateLimited
+
+from libraries import geolocation
+from libraries.geolocation import forward_geocode_place
+
+pytestmark = pytest.mark.django_db(transaction=True)
+
+
+@pytest.fixture(autouse=True)
+def clear_forward_geocode_cache() -> Iterator[None]:
+    """Clear geocoding cache entries around every test.
+    Keeps single-flight and negative-cache scenarios isolated."""
+    cache.clear()
+    yield
+    cache.clear()
+
+
+@patch("libraries.geolocation.Nominatim")
+def test_forward_geocode_reuses_cache_for_normalized_query(
+    mocked_nominatim: Mock,
+) -> None:
+    """Verify equivalent place queries share one successful cache entry.
+    Normalizes whitespace, casing, and country codes before cache lookup."""
+    mocked_location = Mock(latitude=51.5074, longitude=-0.1278)
+    mocked_nominatim.return_value.geocode.return_value = mocked_location
+
+    first_result = forward_geocode_place(
+        place_query="  Central   London  ",
+        user_agent="book-corners-tests",
+        timeout_seconds=5,
+        country_code=" GB ",
+    )
+    second_result = forward_geocode_place(
+        place_query="central london",
+        user_agent="book-corners-tests",
+        timeout_seconds=5,
+        country_code="gb",
+    )
+
+    assert first_result == (51.5074, -0.1278)
+    assert second_result == first_result
+    mocked_nominatim.assert_called_once()
+    mocked_nominatim.return_value.geocode.assert_called_once_with(
+        "Central London",
+        exactly_one=True,
+        language="en",
+        addressdetails=False,
+        country_codes="gb",
+    )
+
+
+@patch("libraries.geolocation.Nominatim")
+def test_forward_geocode_disables_transport_retries(
+    mocked_nominatim: Mock,
+) -> None:
+    """Verify Nominatim uses an HTTP adapter with retries disabled.
+    Prevents one rate-limited operation from producing repeated requests."""
+    mocked_nominatim.return_value.geocode.return_value = None
+
+    forward_geocode_place(
+        place_query="London",
+        user_agent="book-corners-tests",
+        timeout_seconds=5,
+    )
+
+    adapter_factory = mocked_nominatim.call_args.kwargs["adapter_factory"]
+    adapter = adapter_factory(proxies=None, ssl_context=None)
+    try:
+        https_adapter = adapter.session.get_adapter("https://")
+        assert https_adapter.max_retries.total == 0
+    finally:
+        adapter.session.close()
+
+
+@patch("libraries.geolocation.Nominatim")
+def test_concurrent_forward_geocode_calls_share_one_upstream_operation(
+    mocked_nominatim: Mock,
+) -> None:
+    """Verify concurrent callers share one in-flight geocoding operation.
+    Both callers receive the successful coordinates published through cache."""
+    geocode_started = Event()
+    release_geocode = Event()
+    follower_waiting = Event()
+
+    real_wait_for_result = geolocation._wait_for_forward_geocode_result
+
+    def delayed_geocode(*args: object, **kwargs: object) -> Mock:
+        """Hold the mocked upstream call until both callers contend.
+        Makes the single-flight lock behavior deterministic."""
+        geocode_started.set()
+        assert release_geocode.wait(timeout=2)
+        return Mock(latitude=51.5074, longitude=-0.1278)
+
+    def observed_wait_for_result(
+        *, cache_key: str, timeout_seconds: int
+    ) -> tuple[float, float] | None:
+        """Record when a caller waits for the elected geocoder.
+        Delegates polling to the real cache-backed wait helper."""
+        follower_waiting.set()
+        return real_wait_for_result(
+            cache_key=cache_key,
+            timeout_seconds=timeout_seconds,
+        )
+
+    def threaded_forward_geocode(
+        *, place_query: str, country_code: str
+    ) -> tuple[float, float] | None:
+        """Run forward geocoding and close the worker's database connections.
+        Prevents thread-local cache connections from leaking into test teardown."""
+        try:
+            return forward_geocode_place(
+                place_query=place_query,
+                user_agent="book-corners-tests",
+                timeout_seconds=5,
+                country_code=country_code,
+            )
+        finally:
+            connections.close_all()
+
+    mocked_nominatim.return_value.geocode.side_effect = delayed_geocode
+
+    with patch(
+        "libraries.geolocation._wait_for_forward_geocode_result",
+        side_effect=observed_wait_for_result,
+    ):
+        with ThreadPoolExecutor(max_workers=2) as executor:
+            first_future = executor.submit(
+                threaded_forward_geocode,
+                place_query="Central London",
+                country_code="GB",
+            )
+            assert geocode_started.wait(timeout=2)
+            second_future = executor.submit(
+                threaded_forward_geocode,
+                place_query=" central   london ",
+                country_code="gb",
+            )
+            assert follower_waiting.wait(timeout=2)
+            release_geocode.set()
+
+            first_result = first_future.result(timeout=2)
+            second_result = second_future.result(timeout=2)
+
+    assert first_result == (51.5074, -0.1278)
+    assert second_result == first_result
+    mocked_nominatim.assert_called_once()
+    mocked_nominatim.return_value.geocode.assert_called_once()
+
+
+@patch("libraries.geolocation.Nominatim")
+def test_rate_limited_forward_geocode_is_cached_briefly(
+    mocked_nominatim: Mock,
+) -> None:
+    """Verify HTTP rate limiting creates a short-lived failure cache entry.
+    Immediate matching requests use fallback without another upstream call."""
+    mocked_nominatim.return_value.geocode.side_effect = GeocoderRateLimited(
+        "HTTP 429",
+        retry_after=30,
+    )
+
+    first_result = forward_geocode_place(
+        place_query="London",
+        user_agent="book-corners-tests",
+        timeout_seconds=5,
+    )
+    second_result = forward_geocode_place(
+        place_query="london",
+        user_agent="book-corners-tests",
+        timeout_seconds=5,
+    )
+
+    assert first_result is None
+    assert second_result is None
+    mocked_nominatim.assert_called_once()
+    mocked_nominatim.return_value.geocode.assert_called_once()