diff --git a/libraries/geolocation.py b/libraries/geolocation.py index 10fb6fe..dd74036 100644 --- a/libraries/geolocation.py +++ b/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,7 +128,7 @@ 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 @@ -74,15 +136,28 @@ def forward_geocode_place( 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)) diff --git a/libraries/test_geolocation.py b/libraries/test_geolocation.py new file mode 100644 index 0000000..4fd74e0 --- /dev/null +++ b/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() diff --git a/tests/e2e/test_map.py b/tests/e2e/test_map.py index 8a6386a..9c92ec9 100644 --- a/tests/e2e/test_map.py +++ b/tests/e2e/test_map.py @@ -1,4 +1,10 @@ +from threading import Event +from unittest.mock import Mock, patch + import pytest +from django.core.cache import cache + +from libraries import geolocation pytestmark = [pytest.mark.e2e, pytest.mark.django_db(transaction=True)] @@ -81,3 +87,79 @@ def test_map_list_view_shows_libraries( page.wait_for_timeout(2000) assert list_container.inner_html().strip() != "" + + +def test_proximity_filter_deduplicates_concurrent_map_and_list_geocoding( + live_server, + page, + mock_external_apis, + single_library, +) -> None: + """Verify browser proximity filtering shares one server geocoding operation. + Exercises concurrent map/list HTTP requests through the live Django server.""" + cache.clear() + follower_waiting = Event() + real_wait_for_result = geolocation._wait_for_forward_geocode_result + + def delayed_geocode(*args: object, **kwargs: object) -> Mock: + """Hold the elected geocoder until the concurrent request is waiting. + Makes browser-triggered server request overlap deterministic.""" + assert follower_waiting.wait(timeout=5) + return Mock(latitude=43.7696, longitude=11.2558) + + def observed_wait_for_result( + *, cache_key: str, timeout_seconds: int + ) -> tuple[float, float] | None: + """Record when the concurrent request joins the in-flight lookup. + Delegates result polling to the production cache-backed helper.""" + follower_waiting.set() + return real_wait_for_result( + cache_key=cache_key, + timeout_seconds=timeout_seconds, + ) + + with ( + patch("libraries.geolocation.Nominatim") as mocked_nominatim, + patch( + "libraries.geolocation._wait_for_forward_geocode_result", + side_effect=observed_wait_for_result, + ), + ): + mocked_nominatim.return_value.geocode.side_effect = delayed_geocode + page.goto(f"{live_server.url}/map/") + page.locator("#id_near").fill("Florence") + + with page.expect_response( + lambda response: ( + "/map/libraries.geojson?" in response.url + and "near=Florence" in response.url + ), + timeout=15000, + ) as map_response_info: + with page.expect_response( + lambda response: ( + "/map/libraries/list/?" in response.url + and "near=Florence" in response.url + ), + timeout=15000, + ) as list_response_info: + page.get_by_role("button", name="Apply filters").click() + + map_response = map_response_info.value + list_response = list_response_info.value + map_payload = map_response.json() + list_html = list_response.text() + + assert map_response.status == 200 + assert list_response.status == 200 + assert map_payload["meta"]["location_resolution_failed"] is False + assert map_payload["meta"]["center"] == { + "lat": 43.7696, + "lng": 11.2558, + } + assert single_library.name in list_html + assert "Could not resolve" not in list_html + mocked_nominatim.assert_called_once() + mocked_nominatim.return_value.geocode.assert_called_once() + + page.get_by_text(single_library.name).wait_for(state="visible", timeout=10000)