Merge pull request #347 from agessaman/fix/geocode-language

Geocoding: name places in the bot's language, with an opt-out

CHANGELOG.md

@@ -40,6 +40,7 @@ semantic versioning.
 
 ### Fixed
 
+- Place names in `wx`, `gwx` and other location replies are in the bot's `[Localization] language` (falling back to English) instead of each place's local language: `gwx Tokyo` named the place `千代田区, JP` rather than `Chiyoda, JP`, which also took more of the message. Nominatim was never told a language. `[Localization] place_name_language` changes it: `local` keeps each place's own name as before, and a language code picks another language. Reverse lookups are now cached per language, so addresses cached before in local languages are looked up again (unless `local` is set).
 - Reloading the config (from the web viewer, the `reload` command or the admin API) no longer fails when any value contains a bare `%`, such as `% RH` in an `[MqttWeather] json_template`, which `config.ini.example` documents as fine. The reload check expanded `%` interpolation on every value, although those values are read raw and startup accepts the same file, so every reload was rejected and changes saved from the web viewer only took effect after a restart. Such values are now named in a warning and the reload goes ahead.
 - The Telegram bridge no longer logs "Failed to stop service" at shutdown when it is enabled without an `api_token`. It disabled itself before setting up the state `stop()` uses, so stopping it raised `AttributeError`. The Discord bridge had the same gap when neither aiohttp nor requests is installed.
 - `trace` no longer gives up on a reply that is on its way. Each attempt listened only for its own tag, so when the first trace's reply arrived after its timeout, the retry had already replaced it and the reply was dropped; on a mesh where repeaters wait a second or more before forwarding, a three-hop trace always failed. A reply to any attempt now counts until the last attempt times out, and one that arrives during the retry delay ends the trace without sending again. The timeout defaults are now 2.0 s base and 1.5 s per hop (were 1.0 s and 0.5 s, about 2.5 s for three hops); a config that sets them keeps its values.

config.ini.example

@@ -419,6 +419,14 @@ banned_users =
 # If translation file is missing or key not found, falls back to English
 language = en
 
+# Language for place names from geocoding (wx, gwx and other location replies)
+#   (empty) - follow language above, falling back to English where OpenStreetMap
+#             has no name in it ("Chiyoda, JP")
+#   local   - each place's own name ("千代田区, JP"), as before this setting existed
+#   en, de, ... - that language, whatever language replies are in
+# Default: empty
+place_name_language =
+
 # Path to translation files directory (relative to bot root)
 # Default: translations/
 translation_path = translations/

modules/location.py

@@ -19,6 +19,7 @@ import requests
 
 from .region_capitals import REGION_DEFAULT_NOTE, region_capital_query
 from .utils import (
+    _reverse_cache_key,
     abbreviate_location,
     geocode_city_sync,
     geocode_zipcode_sync,
@@ -469,7 +470,7 @@ def _is_country_token(text: str) -> bool:
 
 def _address_from_result(bot: Any, lat: float, lon: float, timeout: int) -> dict:
     db = getattr(bot, "db_manager", None)
-    reverse_cache_key = f"reverse_{lat}_{lon}"
+    reverse_cache_key = _reverse_cache_key(bot, lat, lon)
     if db is not None:
         cached = db.get_cached_json(reverse_cache_key, "geolocation")
         if cached:

modules/utils.py

@@ -754,6 +754,53 @@ def get_nominatim_geocoder(user_agent: str = "meshcore-bot", timeout: int = 10)
     return Nominatim(user_agent=user_agent, timeout=timeout)
 
 
+_LANGUAGE_RE = re.compile(r"[A-Za-z]{2,3}(?:[-_][A-Za-z0-9]{2,8})*")
+
+
+def _language_tag(value: Any) -> Optional[str]:
+    """*value* as a language tag ("de", "pt-BR"), or None when it is not one."""
+    if isinstance(value, str) and _LANGUAGE_RE.fullmatch(value.strip()):
+        return value.strip()
+    return None
+
+
+def _nominatim_language(bot: Any) -> Union[str, bool]:
+    """The languages Nominatim names places in, as its ``language`` argument.
+
+    [Localization] place_name_language picks them: empty follows
+    [Localization] language, ``local`` asks for each place's own name (False:
+    Nominatim's default), and a language tag names that language. A language
+    other than English gets English as its fallback, since Nominatim has no
+    name in most languages for most places and would otherwise fall back to
+    the local one ("千代田区" for Tokyo).
+    """
+    config = getattr(bot, "config", None)
+    try:
+        choice = config.get("Localization", "place_name_language", fallback="") if config is not None else ""
+        if isinstance(choice, str) and choice.strip().lower() == "local":
+            return False
+        language = _language_tag(choice)
+        if language is None:
+            fallback = config.get("Localization", "language", fallback="en") if config is not None else "en"
+            language = _language_tag(fallback) or "en"
+    except Exception:
+        return "en"
+    if language.lower().split("-")[0].split("_")[0] == "en":
+        return language
+    return f"{language},en"
+
+
+def _reverse_cache_key(bot: Any, lat: Any, lon: Any) -> str:
+    """Cache key for a reverse lookup's address; the names in it depend on the language.
+
+    Local names keep the key used before languages were passed, whose entries are local names.
+    """
+    language = _nominatim_language(bot)
+    if language is False:
+        return f"reverse_{lat}_{lon}"
+    return f"reverse_{language}_{lat}_{lon}"
+
+
 async def rate_limited_nominatim_geocode(
     bot: Any, query: Union[str, Mapping[str, str]], timeout: int = 10
 ) -> Optional[Any]:
@@ -770,13 +817,13 @@ async def rate_limited_nominatim_geocode(
     if not hasattr(bot, 'nominatim_rate_limiter'):
         # Fallback if rate limiter not initialized
         geolocator = get_nominatim_geocoder(timeout=timeout)
-        return await asyncio.to_thread(geolocator.geocode, query, timeout=timeout)
+        return await asyncio.to_thread(geolocator.geocode, query, timeout=timeout, language=_nominatim_language(bot))
 
     # Atomically reserve a slot shared with synchronous worker-thread callers.
     await bot.nominatim_rate_limiter.wait_and_request()
 
     geolocator = get_nominatim_geocoder(timeout=timeout)
-    return await asyncio.to_thread(geolocator.geocode, query, timeout=timeout)
+    return await asyncio.to_thread(geolocator.geocode, query, timeout=timeout, language=_nominatim_language(bot))
 
 
 async def rate_limited_nominatim_reverse(bot: Any, coordinates: str, timeout: int = 10) -> Optional[Any]:
@@ -793,13 +840,13 @@ async def rate_limited_nominatim_reverse(bot: Any, coordinates: str, timeout: in
     if not hasattr(bot, 'nominatim_rate_limiter'):
         # Fallback if rate limiter not initialized
         geolocator = get_nominatim_geocoder(timeout=timeout)
-        return await asyncio.to_thread(geolocator.reverse, coordinates, timeout=timeout)
+        return await asyncio.to_thread(geolocator.reverse, coordinates, timeout=timeout, language=_nominatim_language(bot))
 
     # Atomically reserve a slot shared with synchronous worker-thread callers.
     await bot.nominatim_rate_limiter.wait_and_request()
 
     geolocator = get_nominatim_geocoder(timeout=timeout)
-    return await asyncio.to_thread(geolocator.reverse, coordinates, timeout=timeout)
+    return await asyncio.to_thread(geolocator.reverse, coordinates, timeout=timeout, language=_nominatim_language(bot))
 
 
 def rate_limited_nominatim_geocode_sync(
@@ -818,13 +865,13 @@ def rate_limited_nominatim_geocode_sync(
     if not hasattr(bot, 'nominatim_rate_limiter'):
         # Fallback if rate limiter not initialized
         geolocator = get_nominatim_geocoder(timeout=timeout)
-        return geolocator.geocode(query, timeout=timeout)
+        return geolocator.geocode(query, timeout=timeout, language=_nominatim_language(bot))
 
     # Reserve the slot before the request, not after: this runs on worker threads.
     bot.nominatim_rate_limiter.wait_and_request_sync()
 
     geolocator = get_nominatim_geocoder(timeout=timeout)
-    return geolocator.geocode(query, timeout=timeout)
+    return geolocator.geocode(query, timeout=timeout, language=_nominatim_language(bot))
 
 
 def rate_limited_nominatim_reverse_sync(bot: Any, coordinates: str, timeout: int = 10) -> Optional[Any]:
@@ -841,13 +888,13 @@ def rate_limited_nominatim_reverse_sync(bot: Any, coordinates: str, timeout: int
     if not hasattr(bot, 'nominatim_rate_limiter'):
         # Fallback if rate limiter not initialized
         geolocator = get_nominatim_geocoder(timeout=timeout)
-        return geolocator.reverse(coordinates, timeout=timeout)
+        return geolocator.reverse(coordinates, timeout=timeout, language=_nominatim_language(bot))
 
     # Reserve the slot before the request, not after: this runs on worker threads.
     bot.nominatim_rate_limiter.wait_and_request_sync()
 
     geolocator = get_nominatim_geocoder(timeout=timeout)
-    return geolocator.reverse(coordinates, timeout=timeout)
+    return geolocator.reverse(coordinates, timeout=timeout, language=_nominatim_language(bot))
 
 
 _GeocodeSteps = Generator[tuple[str, Any], Any, Any]
@@ -1032,7 +1079,7 @@ def _geocode_city_steps(bot: Any, city: str, default_state: Optional[str],
                 address_info = None
                 if include_address_info:
                     # Check cache for reverse geocoding result
-                    reverse_cache_key = f"reverse_{lat}_{lon}"
+                    reverse_cache_key = _reverse_cache_key(bot, lat, lon)
                     cached_address = bot.db_manager.get_cached_json(reverse_cache_key, "geolocation")
                     if cached_address:
                         address_info = cached_address
@@ -1067,7 +1114,7 @@ def _geocode_city_steps(bot: Any, city: str, default_state: Optional[str],
                 address_info = None
                 if include_address_info:
                     # Check cache for reverse geocoding result
-                    reverse_cache_key = f"reverse_{lat}_{lon}"
+                    reverse_cache_key = _reverse_cache_key(bot, lat, lon)
                     cached_address = bot.db_manager.get_cached_json(reverse_cache_key, "geolocation")
                     if cached_address:
                         address_info = cached_address
@@ -1100,7 +1147,7 @@ def _geocode_city_steps(bot: Any, city: str, default_state: Optional[str],
                 address_info = None
                 if include_address_info:
                     # Check cache for reverse geocoding result
-                    reverse_cache_key = f"reverse_{lat}_{lon}"
+                    reverse_cache_key = _reverse_cache_key(bot, lat, lon)
                     cached_address = bot.db_manager.get_cached_json(reverse_cache_key, "geolocation")
                     if cached_address:
                         address_info = cached_address
@@ -1176,7 +1223,7 @@ def _geocode_city_steps(bot: Any, city: str, default_state: Optional[str],
                     address_info = None
                     if include_address_info:
                         # Check cache for reverse geocoding result
-                        reverse_cache_key = f"reverse_{lat}_{lon}"
+                        reverse_cache_key = _reverse_cache_key(bot, lat, lon)
                         cached_address = bot.db_manager.get_cached_json(reverse_cache_key, "geolocation")
                         if cached_address:
                             address_info = cached_address
@@ -1211,7 +1258,7 @@ def _geocode_city_steps(bot: Any, city: str, default_state: Optional[str],
                 address_info = None
                 if include_address_info:
                     # Check cache for reverse geocoding result
-                    reverse_cache_key = f"reverse_{lat}_{lon}"
+                    reverse_cache_key = _reverse_cache_key(bot, lat, lon)
                     cached_address = bot.db_manager.get_cached_json(reverse_cache_key, "geolocation")
                     if cached_address:
                         address_info = cached_address
@@ -1235,7 +1282,7 @@ def _geocode_city_steps(bot: Any, city: str, default_state: Optional[str],
             address_info = None
             if include_address_info:
                 # Check cache for reverse geocoding result
-                reverse_cache_key = f"reverse_{lat}_{lon}"
+                reverse_cache_key = _reverse_cache_key(bot, lat, lon)
                 cached_address = bot.db_manager.get_cached_json(reverse_cache_key, "geolocation")
                 if cached_address:
                     address_info = cached_address

tests/test_utils_geocoding.py

@@ -363,7 +363,7 @@ class TestRateLimitedNominatimGeocode:
         async def wait_and_request():
             events.append("reserve")
 
-        def geocode(query, *, timeout):
+        def geocode(query, *, timeout, language):
             events.append("geocode")
             entered.set()
             if not release.wait(0.5):
@@ -424,7 +424,7 @@ class TestRateLimitedNominatimReverse:
         mock_loc = _make_location()
         mock_geocoder = Mock()
 
-        def reverse(coordinates, *, timeout):
+        def reverse(coordinates, *, timeout, language):
             entered.set()
             if not release.wait(0.5):
                 raise AssertionError("event loop did not run while geocoder was blocked")

tests/unit/test_geocode_language.py

@@ -0,0 +1,118 @@
+"""Nominatim names places in the bot's [Localization] language, and reverse lookups are cached per language."""
+
+import configparser
+from unittest.mock import Mock, patch
+
+import pytest
+
+from modules import utils
+from modules.utils import (
+    _nominatim_language,
+    _reverse_cache_key,
+    rate_limited_nominatim_geocode,
+    rate_limited_nominatim_geocode_sync,
+    rate_limited_nominatim_reverse,
+    rate_limited_nominatim_reverse_sync,
+)
+
+
+def _bot(language=None, limiter=True, place_names=None):
+    config = configparser.ConfigParser()
+    localization = {}
+    if language is not None:
+        localization["language"] = language
+    if place_names is not None:
+        localization["place_name_language"] = place_names
+    if localization:
+        config.read_dict({"Localization": localization})
+    bot = Mock(spec=["config", "nominatim_rate_limiter"] if limiter else ["config"])
+    bot.config = config
+    if limiter:
+        bot.nominatim_rate_limiter = Mock()
+        bot.nominatim_rate_limiter.wait_and_request = Mock(side_effect=lambda: _done())
+    return bot
+
+
+async def _done():
+    return None
+
+
+@pytest.mark.parametrize(("language", "expected"), [
+    (None, "en"),          # no [Localization] section
+    ("de", "de,en"),
+    ("en-GB", "en-GB"),
+    (" pt-BR ", "pt-BR,en"),
+    ("zh_Hant", "zh_Hant,en"),
+    ("", "en"),
+    ("en;q=0.5,de", "en"),  # not a language tag: never forwarded to Nominatim
+])
+def test_language_comes_from_localization(language, expected):
+    assert _nominatim_language(_bot(language)) == expected
+
+
+@pytest.mark.parametrize(("language", "place_names", "expected"), [
+    ("de", "", "de,en"),        # empty follows [Localization] language
+    ("de", "local", False),     # each place's own name, as before
+    ("de", " LOCAL ", False),
+    ("de", "en", "en"),
+    ("en", "fr", "fr,en"),
+    ("de", "not a tag!", "de,en"),
+])
+def test_place_name_language_overrides(language, place_names, expected):
+    assert _nominatim_language(_bot(language, place_names=place_names)) == expected
+
+
+def test_local_names_reuse_the_cache_key_from_before():
+    assert _reverse_cache_key(_bot("de", place_names="local"), 35.68, 139.76) == "reverse_35.68_139.76"
+
+
+def test_language_without_a_usable_config():
+    assert _nominatim_language(object()) == "en"
+    bot = Mock()  # config.get returns a Mock, not a string
+    assert _nominatim_language(bot) == "en"
+
+
+def test_reverse_cache_key_includes_the_language():
+    assert _reverse_cache_key(_bot("de"), 35.68, 139.76) == "reverse_de,en_35.68_139.76"
+    assert _reverse_cache_key(_bot(), 35.68, 139.76) == "reverse_en_35.68_139.76"
+
+
+@pytest.mark.parametrize("limiter", [True, False])
+def test_sync_lookups_pass_the_language(limiter):
+    geolocator = Mock()
+    bot = _bot("fr", limiter=limiter)
+    with patch.object(utils, "get_nominatim_geocoder", return_value=geolocator):
+        rate_limited_nominatim_geocode_sync(bot, "Tokyo", timeout=5)
+        rate_limited_nominatim_reverse_sync(bot, "35.68, 139.76", timeout=5)
+    geolocator.geocode.assert_called_once_with("Tokyo", timeout=5, language="fr,en")
+    geolocator.reverse.assert_called_once_with("35.68, 139.76", timeout=5, language="fr,en")
+
+
+@pytest.mark.asyncio
+@pytest.mark.parametrize("limiter", [True, False])
+async def test_async_lookups_pass_the_language(limiter):
+    geolocator = Mock()
+    bot = _bot("fr", limiter=limiter)
+    with patch.object(utils, "get_nominatim_geocoder", return_value=geolocator):
+        await rate_limited_nominatim_geocode(bot, "Tokyo", timeout=5)
+        await rate_limited_nominatim_reverse(bot, "35.68, 139.76", timeout=5)
+    geolocator.geocode.assert_called_once_with("Tokyo", timeout=5, language="fr,en")
+    geolocator.reverse.assert_called_once_with("35.68, 139.76", timeout=5, language="fr,en")
+
+
+def test_a_cached_address_from_before_is_not_reused():
+    """Addresses cached under the old language-less key were in each place's local language."""
+    from modules.location import _address_from_result
+
+    bot = _bot("en")
+    cache = {"reverse_35.68_139.76": {"city": "千代田区", "country_code": "jp"}}
+    bot.db_manager = Mock()
+    bot.db_manager.get_cached_json = Mock(side_effect=lambda key, _kind: cache.get(key))
+    reverse = Mock()
+    reverse.raw = {"address": {"city": "Chiyoda", "country_code": "jp"}}
+    with patch("modules.location.rate_limited_nominatim_reverse_sync", return_value=reverse) as lookup:
+        address = _address_from_result(bot, 35.68, 139.76, timeout=5)
+    lookup.assert_called_once()
+    assert address["city"] == "Chiyoda"
+    bot.db_manager.cache_json.assert_called_once()
+    assert bot.db_manager.cache_json.call_args.args[0] == "reverse_en_35.68_139.76"