diff --git a/partner_geolocalize_usability/README.rst b/partner_geolocalize_usability/README.rst index 170e379..b47b1b6 100644 --- a/partner_geolocalize_usability/README.rst +++ b/partner_geolocalize_usability/README.rst @@ -21,7 +21,25 @@ It also modify the partner form vieww for a better understanding of the geolocat Configuration ============= -No configuration needed. +The OpenStreetMap Nominatim geocoding service requires a genuine, identifying +``User-Agent`` header with a valid contact address (see the `Nominatim usage +policy `_). The +default Odoo ``User-Agent`` is shared by many instances and gets rate-limited or +blocked (HTTP 429 "Too many requests"). + +To fix this, set the system parameter (Settings > Technical > System +Parameters): + +- **Key**: ``partner_geolocalize_usability.user_agent`` +- **Value**: a User-Agent identifying your deployment and a valid contact, + e.g. ``acme-crm/1.0 (contact: gis@acme.example)`` + +The module ships this parameter with a placeholder value; you MUST replace it +with your own, otherwise Nominatim will reject the requests. + +Requests to Nominatim are additionally throttled to one request per second to +comply with the usage policy, so mass geolocation of many partners is slow by +design (roughly one partner per second). Known issues / Roadmap ====================== diff --git a/partner_geolocalize_usability/__manifest__.py b/partner_geolocalize_usability/__manifest__.py index 20f0fe6..4ac0827 100644 --- a/partner_geolocalize_usability/__manifest__.py +++ b/partner_geolocalize_usability/__manifest__.py @@ -17,10 +17,11 @@ ], "qweb": [], "external_dependencies": { - "python": [], + "python": ["requests"], }, # always loaded "data": [ + "data/ir_config_parameter.xml", "views/res_partner.xml", ], # only loaded in demonstration mode diff --git a/partner_geolocalize_usability/data/ir_config_parameter.xml b/partner_geolocalize_usability/data/ir_config_parameter.xml new file mode 100644 index 0000000..d554af9 --- /dev/null +++ b/partner_geolocalize_usability/data/ir_config_parameter.xml @@ -0,0 +1,16 @@ + + + + + partner_geolocalize_usability.user_agent + SET-ME partner_geolocalize_usability (contact: you@example.invalid) + + diff --git a/partner_geolocalize_usability/models/__init__.py b/partner_geolocalize_usability/models/__init__.py index 608407e..d95c99a 100644 --- a/partner_geolocalize_usability/models/__init__.py +++ b/partner_geolocalize_usability/models/__init__.py @@ -1,2 +1,3 @@ +from . import base_geocoder from . import res_partner diff --git a/partner_geolocalize_usability/models/base_geocoder.py b/partner_geolocalize_usability/models/base_geocoder.py new file mode 100644 index 0000000..fdbfcaa --- /dev/null +++ b/partner_geolocalize_usability/models/base_geocoder.py @@ -0,0 +1,79 @@ +import logging +import time + +import requests + +from odoo import api, models + +_logger = logging.getLogger(__name__) + +# System parameter holding the User-Agent sent to Nominatim. +# Nominatim's usage policy requires a genuine, identifying User-Agent with a +# valid contact. The stock Odoo User-Agent is shared by thousands of instances +# and is rate-limited/blocked (HTTP 429), so we let each deployment set its own. +USER_AGENT_PARAM = "partner_geolocalize_usability.user_agent" + +# Placeholder shown until the deployment sets a real value. It intentionally +# does NOT identify any real service so an unconfigured instance fails loudly +# rather than silently reusing a banned generic agent. +USER_AGENT_PLACEHOLDER = ( + "SET-ME partner_geolocalize_usability (contact: you@example.invalid)" +) + +# Nominatim's public API allows at most 1 request per second. We wait slightly +# more than a second between calls to stay safely under the limit. +NOMINATIM_MIN_INTERVAL = 1.1 + + +class GeoCoder(models.AbstractModel): + _inherit = "base.geocoder" + + def _get_nominatim_user_agent(self): + """Return the configured User-Agent for Nominatim requests. + + Falls back to an obvious placeholder so a misconfigured instance is easy + to spot in logs (and so we never silently reuse Odoo's banned agent). + """ + return ( + self.env["ir.config_parameter"] + .sudo() + .get_param(USER_AGENT_PARAM, USER_AGENT_PLACEHOLDER) + ) + + @api.model + def _call_openstreetmap(self, addr, **kw): + """Query Nominatim with a configurable User-Agent and rate limiting. + + Overrides the stock implementation to fix two problems that cause + HTTP 429 ("Too many requests"): + + * the shared/generic Odoo User-Agent is blocked by Nominatim -> use a + deployment-specific one from a system parameter; + * bursts of requests exceed the 1 req/s policy -> throttle between calls. + """ + if not addr: + _logger.info("invalid address given") + return None + url = "https://nominatim.openstreetmap.org/search" + try: + headers = {"User-Agent": self._get_nominatim_user_agent()} + # Respect Nominatim's 1 req/s policy. Sleeping before the call keeps + # the throttle effective even when callers loop over many records. + time.sleep(NOMINATIM_MIN_INTERVAL) + response = requests.get( + url, headers=headers, params={"format": "json", "q": addr} + ) + _logger.info("openstreetmap nominatim service called") + if response.status_code != 200: + _logger.warning( + "Request to openstreetmap failed.\nCode: %s\nContent: %s", + response.status_code, + response.content, + ) + result = response.json() + except Exception as e: + self._raise_query_error(e) + if not result: + return None + geo = result[0] + return float(geo["lat"]), float(geo["lon"]) diff --git a/partner_geolocalize_usability/tests/__init__.py b/partner_geolocalize_usability/tests/__init__.py new file mode 100644 index 0000000..10fe9c8 --- /dev/null +++ b/partner_geolocalize_usability/tests/__init__.py @@ -0,0 +1 @@ +from . import test_base_geocoder diff --git a/partner_geolocalize_usability/tests/test_base_geocoder.py b/partner_geolocalize_usability/tests/test_base_geocoder.py new file mode 100644 index 0000000..96e140c --- /dev/null +++ b/partner_geolocalize_usability/tests/test_base_geocoder.py @@ -0,0 +1,92 @@ +from unittest.mock import MagicMock, patch + +from odoo.tests.common import TransactionCase + +from ..models.base_geocoder import ( + NOMINATIM_MIN_INTERVAL, + USER_AGENT_PARAM, + USER_AGENT_PLACEHOLDER, +) + +_MODULE = "odoo.addons.partner_geolocalize_usability.models.base_geocoder" + + +def _fake_response(payload, status_code=200): + response = MagicMock() + response.status_code = status_code + response.json.return_value = payload + return response + + +class TestBaseGeocoder(TransactionCase): + @classmethod + def setUpClass(cls): + super().setUpClass() + cls.geocoder = cls.env["base.geocoder"] + cls.Param = cls.env["ir.config_parameter"].sudo() + + def test_uses_configured_user_agent(self): + # A deployment-specific User-Agent must be sent to Nominatim. + self.Param.set_param(USER_AGENT_PARAM, "acme/1.0 (contact: x@acme.test)") + + with patch(f"{_MODULE}.time.sleep"), patch( + f"{_MODULE}.requests.get", + return_value=_fake_response([{"lat": "1.5", "lon": "2.5"}]), + ) as mocked_get: + result = self.geocoder._call_openstreetmap("some address") + + self.assertEqual(result, (1.5, 2.5)) + _, kwargs = mocked_get.call_args + self.assertEqual( + kwargs["headers"]["User-Agent"], "acme/1.0 (contact: x@acme.test)" + ) + + def test_falls_back_to_placeholder_user_agent(self): + # Without configuration, the obvious placeholder is used (never Odoo's + # banned generic agent). + self.Param.set_param(USER_AGENT_PARAM, False) + + with patch(f"{_MODULE}.time.sleep"), patch( + f"{_MODULE}.requests.get", + return_value=_fake_response([{"lat": "1.0", "lon": "2.0"}]), + ) as mocked_get: + self.geocoder._call_openstreetmap("addr") + + _, kwargs = mocked_get.call_args + self.assertEqual( + kwargs["headers"]["User-Agent"], USER_AGENT_PLACEHOLDER + ) + + def test_throttles_before_request(self): + # The 1 req/s policy is enforced by sleeping before each call. + self.Param.set_param(USER_AGENT_PARAM, "acme/1.0 (contact: x@acme.test)") + + with patch(f"{_MODULE}.time.sleep") as mocked_sleep, patch( + f"{_MODULE}.requests.get", + return_value=_fake_response([{"lat": "1.0", "lon": "2.0"}]), + ): + self.geocoder._call_openstreetmap("addr") + + mocked_sleep.assert_called_once_with(NOMINATIM_MIN_INTERVAL) + + def test_empty_result_returns_none(self): + # An unknown address (empty Nominatim result) must not raise IndexError. + self.Param.set_param(USER_AGENT_PARAM, "acme/1.0 (contact: x@acme.test)") + + with patch(f"{_MODULE}.time.sleep"), patch( + f"{_MODULE}.requests.get", return_value=_fake_response([]) + ): + result = self.geocoder._call_openstreetmap("nowhere at all") + + self.assertIsNone(result) + + def test_blank_address_returns_none_without_request(self): + # Empty address short-circuits: no HTTP call, no throttle. + with patch(f"{_MODULE}.time.sleep") as mocked_sleep, patch( + f"{_MODULE}.requests.get" + ) as mocked_get: + result = self.geocoder._call_openstreetmap("") + + self.assertIsNone(result) + mocked_get.assert_not_called() + mocked_sleep.assert_not_called()