fix: replace Nominatim reverse geocoding with pg-nearest-city#7287
fix: replace Nominatim reverse geocoding with pg-nearest-city#7287munzzyy wants to merge 1 commit into
Conversation
Project.set_country_info made a blocking requests.get call to the public Nominatim server inside an async request path. Swap it for pg-nearest-city, which resolves the nearest city and country from the project's own PostGIS database instead of a remote HTTP call. set_country_info is now async and reads location.country_name to keep the existing long-form country naming convention. Removed OSM_NOMINATIM_SERVER_URL since it's no longer read anywhere. Fixes hotosm#6905
github-actions
Bot
added
scope: infrastructure
scope: backend
dependencies
|
|
Looked into the backend test failure since it's blocking on a red X. Both run attempts fail before any test actually executes. Attempt 1 was That's the The fork PR token isn't carrying package-write to the org registry, and it's separate from anything in the diff - #7282 (also a fork PR) hit the same approval gate on attempt 1. flake8/black pass locally on the touched files per the PR description, and SonarCloud's quality gate on this PR is green. Happy to rebase/retrigger if there's a token or workflow change on the CI side, just flagging that a plain retry won't fix this one since it's failing at the same push step both times. |
1 participant
What type of PR is this? (check all applicable)
Related Issue
Fixes #6905
Describe this PR
Project.set_country_inforeverse-geocodes a project's centroid by making a blockingrequests.getcall to the public Nominatim server, inside an async FastAPI request path. That's an external dependency for something that doesn't need one, it blocks the event loop while it waits on the network, and it adds load to a server the OSM community donates for free.This swaps that call for
pg-nearest-city, the local PostGIS-backed reverse geocoder HOTOSM already built for Field-TM. It resolves the nearest city (and its country) straight from the database with a spatial query, no outbound HTTP, no external service.Changes:
pg-nearest-city>=2.0.0topyproject.toml(pulls inpsycopgas a dependency;libpqis already present in the runtime image viapostgresql-client, so nothing extra needed there).requires-pythonnarrowed from>=3.9,<=3.11to>=3.10,<=3.11sincepg-nearest-cityneeds 3.10+. Doesn't change anything for actual deploys, the Docker image already runs 3.10.set_country_infois nowasyncand queriesAsyncNearestCitywith the project's centroid instead of hitting Nominatim. It readslocation.country_name(the full country name from the package's dataset) rather thanlocation.country(an ISO-style code likeUSA), specifically to avoid breaking existing country filters/charts, which expect long names. For the UK test fixture this still resolves to"United Kingdom", matching the old Nominatim-based expectation exactly.OSM_NOMINATIM_SERVER_URLfrombackend/config.py,example.env, and the three AWS ECS env hcl files, since it's no longer read anywhere.ST_Coverson the country polygon before finding the nearest city within it), which is what fixes the cross-border mismatch reported in this issue's thread (a Nepal AOI near the India border resolving to the wrong country). I checked the actual query pg-nearest-city runs and confirmed it scopes the nearest-city search to the country whose polygon covers the point, so it shouldn't reintroduce the "closest city regardless of border" bug.AsyncNearestCityalready handles this itself: on first connection it checks for itscountry/geocodingtables, and if they're missing it auto-imports the bundled compressed CSVs (guarded by a Postgres advisory lock so concurrent requests don't race each other). So the first project create/update after this ships will take longer (the package's docs say 30s-4m), everything after that is fast. No separate migration or deploy step needed for that part.Alternative Approaches Considered
The issue thread raised using ISO country codes instead of country names for the whole
countryfield, to sidestep the Nominatim-vs-pg-nearest-city naming mismatch entirely. That's a bigger change (DB column, API responses, frontend filters all currently key off the long name), so I left it out of this PR and just picked the field (country_name) that matches the existing naming convention as closely as pg-nearest-city allows. Happy to follow up separately if that's still wanted.I also didn't add a data-backfill migration for existing projects'
countryvalues (the repo has precedent for this, seemigrations/versions/7937dae319b5_.pyfrom 2020). Existing projects already havecountryset from Nominatim and this PR only affects new lookups going forward, so I don't think a backfill is required for this fix, but flagging it in case reviewers disagree.Review Guide
backend/models/postgis/project.py:Project.set_country_infois the core change.backend/services/project_admin_service.pyand theupdatemethod inproject.py: both call sites nowawaitthe method.tests/api/unit/models/postgis/test_project.py: replaced the Nominatim-mocking test with one that patchesAsyncNearestCityand assertscountrygets set fromlocation.country_name, plus a new test for the lookup-failure path (make sure it doesn't blow up project creation, just leavescountryunset).Testing done
pg_nearest_citypackage (installed it locally, read the real source, not just the README) to confirm the class names,query()signature, and theLocationdataclass fields (countryvscountry_name) before writing the integration.uv lockresolves cleanly with the new dependency.flake8andblack --checkpass on all touched files.backend/models/postgis/project.pyandbackend/services/project_admin_service.pyimport cleanly with all real dependencies installed, and thatset_country_infois genuinely a coroutine now.Project.set_country_infomethod (mockingAsyncNearestCitythe same way the test file does) and confirmed both the success and failure paths work.tests/api/unit/,tests/api/integration/) locally. It needs the docker-compose Postgres/PostGIS stack, which isn't available in my environment. Deferring to CI for that part.Environment variable changes
Removed
OSM_NOMINATIM_SERVER_URLeverywhere it was referenced -backend/config.py,example.env, and the three AWS terragrunt hcl files underscripts/aws/infra/*/purgeable/. It's no longer read anywhere in the backend. If there's an infra-side reason to keep the var defined regardless, happy to revert that part.