feature: geolocation utils extracted from backend-client #296
Conversation
WalkthroughThe changes in this pull request involve updates to the project's dependencies and the introduction of new functionality related to geolocation and IP validation. The Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## dev #296 +/- ##
==========================================
- Coverage 47.19% 45.72% -1.48%
==========================================
Files 31 32 +1
Lines 3244 3353 +109
==========================================
+ Hits 1531 1533 +2
- Misses 1713 1820 +107 ☔ View full report in Codecov by Sentry. 🚨 Try these New Features:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (2)
ovos_utils/location.py (2)
70-71: Simplify timezone assignment by removing redundant checksSince the 'timezone' key is not present in the initial 'location' dictionary, the condition
if "timezone" not in location:will always be true. You can simplify the code by directly assigning the timezone.Apply this diff to simplify the code:
-if "timezone" not in location: - location["timezone"] = get_timezone(lon=lon, lat=lat) +location["timezone"] = get_timezone(lon=lon, lat=lat)This applies to both instances in the code at lines 70-71 and 118-121.
Also applies to: 118-121
27-28: Refactor API request logic into a utility function to enhance maintainabilityBoth
get_geolocationandget_reverse_geolocationfunctions perform API requests with similar error handling needs. Consider refactoring the request and response parsing into a utility function to reduce code duplication and improve maintainability.Example:
def nominatim_api_get(url, params): headers = {"User-Agent": "OVOS/1.0"} response = requests.get(url, params=params, headers=headers) if response.status_code == 200: try: return response.json() except ValueError: # handle JSON decoding error return None else: # handle HTTP error return NoneThen update your functions accordingly:
# In get_geolocation results = nominatim_api_get(url, {"q": location, "format": "json", "limit": 1}) if results: data = results[0] # proceed with data else: # handle error return None # In get_reverse_geolocation details = nominatim_api_get(url, {"lat": lat, "lon": lon, "format": "json"}) if details and 'address' in details: address = details['address'] # proceed with address else: # handle error return NoneAlso applies to: 87-88
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
ovos_utils/location.py(1 hunks)requirements/requirements.txt(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- requirements/requirements.txt
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (1)
ovos_utils/location.py (1)
1-155: Consider architectural improvements for reliabilityThe module heavily depends on external APIs without fallback mechanisms.
Consider these improvements:
- Add configuration management for API endpoints and timeouts:
NOMINATIM_BASE_URL = "https://nominatim.openstreetmap.org" IP_API_BASE_URL = "https://ip-api.com" REQUEST_TIMEOUT = 10 # seconds
- Implement fallback mechanisms:
- Alternative geolocation providers
- Local timezone database fallback
- Caching failed requests to prevent repeated failures
- Add retry mechanism for transient failures:
from tenacity import retry, stop_after_attempt, wait_exponential @retry(stop=stop_after_attempt(3), wait=wait_exponential(multiplier=1, min=4, max=10)) def _make_api_request(url, params, headers): response = requests.get(url, params=params, headers=headers, timeout=REQUEST_TIMEOUT) response.raise_for_status() return response.json()
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (2)
ovos_utils/location.py (2)
19-20: Avoid exposing raw error messagesThe error message from the original exception is directly exposed in the raised ValueError. This could potentially expose implementation details.
Consider sanitizing the error message:
- raise ValueError(f"Invalid coordinates: {str(e)}") + raise ValueError("Invalid coordinates: Values must be valid numbers within range")🧰 Tools
🪛 Ruff
20-20: Within an
exceptclause, raise exceptions withraise ... from errorraise ... from Noneto distinguish them from errors in exception handling(B904)
25-32: Update docstring return typeThe docstring incorrectly states that the return type is 'str', but the function returns a dictionary or None.
Update the return type in the docstring:
Returns: - str: JSON structure with lookup results + dict or None: Dictionary containing location details if found, None otherwise
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
ovos_utils/location.py(1 hunks)
🧰 Additional context used
🪛 Ruff
ovos_utils/location.py
2-2: requests.exceptions.RequestException imported but unused
Remove unused import
(F401)
2-2: requests.exceptions.Timeout imported but unused
Remove unused import
(F401)
20-20: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
167-167: Redefinition of unused get_ip_geolocation from line 148
(F811)
🔇 Additional comments (2)
ovos_utils/location.py (2)
109-111:
112-139:
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (1)
ovos_utils/location.py (1)
121-121: Remove redundant address lookupThe address is already retrieved and checked at line 114:
- address = details.get("address")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
ovos_utils/location.py(1 hunks)
🧰 Additional context used
🪛 Ruff
ovos_utils/location.py
22-22: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
🔇 Additional comments (4)
ovos_utils/location.py (4)
157-202: LGTM! Well-implemented IP geolocation functionality
The implementation includes:
- Proper IP validation
- Good error handling
- Comprehensive response processing
1-8: 🛠️ Refactor suggestion
Import request exceptions for better error handling
Add exception imports from requests for better error handling of network-related issues:
import ipaddress
import requests
+from requests.exceptions import RequestException, Timeout
from timezonefinder import TimezoneFinderLikely invalid or redundant comment.
78-80:
Remove incorrect usage of calculated_postcode as state code
The state code is incorrectly falling back to calculated_postcode:
- "code": tags.get("state_code") or
- details["calculated_postcode"] or "",
+ "code": tags.get("state_code") or "",Likely invalid or redundant comment.
56-58:
Add error handling for OSM ID lookup
The code assumes osm_id and osm_type exist in the response. Add proper error handling:
- details = requests.get(url, params={"osmid": data['osm_id'], "osmtype": data['osm_type'][0].upper(),
- "format": "json"},
- headers={"User-Agent": "OVOS/1.0"}).json()
+ if 'osm_id' not in data or 'osm_type' not in data:
+ raise ValueError("Missing OSM ID or type in response")
+ response = _make_nominatim_request(url,
+ params={"osmid": data['osm_id'],
+ "osmtype": data['osm_type'][0].upper(),
+ "format": "json"},
+ headers={"User-Agent": "OVOS/1.0"})
+ if response.status_code != 200:
+ raise ValueError(f"OSM details lookup failed: status code {response.status_code}")
+ details = response.json()Likely invalid or redundant comment.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (3)
ovos_utils/location.py (3)
12-41: Consider caching timezone lookupsThe function performs well but could benefit from caching since timezone data is static for given coordinates. Consider adding the
@timed_lru_cachedecorator as used in other functions.+@timed_lru_cache(seconds=3600) # Cache for 1 hour since timezone data is static def get_timezone(lat: float, lon: float) -> Dict[str, str]:
105-125: Consider extracting location object construction to a helper functionThe nested dictionary construction is complex and could benefit from being extracted into a helper function for better maintainability and reusability.
Consider creating a helper function like:
def _build_location_dict(address: str, city: Dict, coordinates: Dict) -> Dict[str, Any]: return { "address": address, "city": city, "coordinate": coordinates }
251-265: Consider extracting data structure constructionSimilar to earlier suggestions, the nested dictionary construction could benefit from helper functions.
Consider creating helper functions:
def _build_region_dict(code: str, name: str, country: Dict) -> Dict[str, Any]: return {"code": code, "name": name, "country": country} def _build_city_dict(code: str, name: str, state: Dict) -> Dict[str, Any]: return {"code": code, "name": name, "state": state}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
ovos_utils/location.py(1 hunks)
🧰 Additional context used
🪛 Ruff
ovos_utils/location.py
64-64: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
87-87: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
154-154: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
241-241: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
🔇 Additional comments (3)
ovos_utils/location.py (3)
201-216: LGTM! Well-implemented helper function
The function is focused, well-documented, and handles errors appropriately.
238-240: Ensure HTTPS usage for IP API calls
While the code uses HTTPS, it's worth noting that the free tier of ip-api.com only supports HTTP. Consider either:
- Documenting this limitation
- Using the pro version for HTTPS support
- Switching to a different service that supports HTTPS in the free tier
112-114:
Remove incorrect state code fallback to postcode
The state code should not fall back to calculated_postcode as these are different types of data.
- "code": tags.get("state_code") or
- details["calculated_postcode"] or "",
+ "code": tags.get("state_code") or "",Likely invalid or redundant comment.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (2)
ovos_utils/network_utils.py (1)
20-34: Consider enhancing the IP validation documentation and handlingThe implementation is clean and correct, using the standard library's
ipaddressmodule. Consider these enhancements:def is_valid_ip(ip: str) -> bool: """ Validate an IP address. + + Validates both IPv4 and IPv6 addresses using the ipaddress module. Args: - ip (str): The IP address to validate. + ip (str): The IP address to validate (IPv4 or IPv6). + + Examples: + >>> is_valid_ip("192.168.1.1") + True + >>> is_valid_ip("2001:db8::1") + True + >>> is_valid_ip("256.256.256.256") + False Returns: bool: True if the IP is valid, False otherwise. """ + if ip is None: + return False try: ipaddress.ip_address(ip) return True except ValueError: return Falseovos_utils/geolocation.py (1)
222-223: Consider using HTTPS for API requests to enhance securityThe request to the IP geolocation service is made over HTTP, which may expose sensitive data over the network. If the API supports HTTPS, consider switching to HTTPS to improve security.
Verify if the API supports HTTPS and update the URL accordingly:
- response = requests.get(f"http://ip-api.com/json/{ip}", + response = requests.get(f"https://ip-api.com/json/{ip}", params={"fields": fields}, timeout=timeout)If HTTPS is not available for the free API, consider using an alternative service that supports secure connections to protect user data.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
ovos_utils/geolocation.py(1 hunks)ovos_utils/network_utils.py(2 hunks)
🧰 Additional context used
🪛 Ruff
ovos_utils/geolocation.py
63-63: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
86-86: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
153-153: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
225-225: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
🔇 Additional comments (2)
ovos_utils/network_utils.py (2)
1-1: LGTM: Clean import addition
The ipaddress standard library import is appropriately placed and properly grouped with other standard library imports.
20-34: Verify integration with geolocation functionality
Let's verify that this function's interface aligns with its usage in the geolocation module.
✅ Verification successful
Based on the results, I can now generate the final response since I have sufficient information about the integration between is_valid_ip and the geolocation functionality.
Integration with geolocation functionality is properly aligned
The verification shows clean integration:
- Function signature matches usage:
is_valid_ip(ip: str) -> boolaligns with its usage in geolocation.py - Error handling is consistent: geolocation.py properly checks the return value and raises a ValueError with a descriptive message
- Import is correctly defined:
from ovos_utils.network_utils import get_external_ip, is_valid_ip
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check how is_valid_ip is used in geolocation.py and verify alignment
# Find the geolocation file and check usage
echo "Checking for geolocation.py file..."
fd -t f "geolocation.py$"
# Check how is_valid_ip is used
echo "Checking is_valid_ip usage..."
rg -A 5 "is_valid_ip"
# Look for any error handling patterns around IP validation
echo "Checking error handling patterns..."
ast-grep --pattern 'try {
$$$
is_valid_ip($$$)
$$$
} catch'
Length of output: 1460
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (1)
ovos_utils/geolocation.py (1)
105-105: Offer assistance for implementing language supportThere's a TODO comment indicating that language support is needed for handling official names reported in various languages. Implementing this feature will enhance the localization capabilities of the geolocation service.
Would you like assistance in implementing multilingual support for official names? I can help develop a solution or open a GitHub issue to track this task.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
ovos_utils/geolocation.py(1 hunks)
🧰 Additional context used
🪛 Ruff
ovos_utils/geolocation.py
65-65: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
88-88: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
155-155: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
244-244: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
ovos_utils/geolocation.py (4)
106-106: Address TODO comment for language supportThe TODO comment indicates missing language support for official names. Would you like me to help implement multi-language support for official names?
99-105: Simplify nested dictionary accessConsider using a helper function to handle the nested dictionary access with fallbacks. This would make the code more readable and maintainable.
+ def get_nested_value(data: dict, *keys: str, default: str = "") -> str: + """Helper function to safely get nested dictionary values with multiple fallbacks.""" + for key_list in keys: + result = data + for k in key_list.split('.'): + result = result.get(k, {}) if isinstance(result, dict) else {} + if result and not isinstance(result, dict): + return result + return default - place_type = details.get("extratags", {}).get("linked_place") or details.get("category") or data.get( - "type") or data.get("class") + place_type = get_nested_value(details, + "extratags.linked_place", + "category", + "type", + "class")
171-176: Simplify city name resolution logicConsider extracting the city name resolution logic into a helper function for better maintainability:
+ def resolve_city_name(address: dict) -> str: + """Helper function to resolve city name from address fields.""" + city_fields = ['city', 'village', 'town', 'hamlet', 'county'] + return next((address.get(field) for field in city_fields if address.get(field)), "") - "name": address.get("city") or - address.get("village") or - address.get("town") or - address.get("hamlet") or - address.get("county") or "", + "name": resolve_city_name(address),
230-238: Simplify language normalization logicConsider extracting the language normalization logic into a helper function:
+ def normalize_ip_api_language(lang: str) -> str: + """Normalize language code for ip-api.com.""" + supported_langs = {"en", "de", "es", "fr", "ja", "ru"} + special_cases = {"pt": "pt-BR", "zh": "zh-CN"} + + lang = standardize_lang_tag(lang).split("-")[0] + if lang in special_cases: + return special_cases[lang] + return lang if lang in supported_langs else "en" - lang = standardize_lang_tag(lang).split("-")[0] - if lang not in ["en", "de", "es", "pt", "fr", "ja", "zh", "ru"]: - LOG.warning(f"Language unsupported by ip-api.com ({lang}), defaulting to english") - lang = "en" - elif lang == "pt": - lang = "pt-BR" - elif lang == "zh": - lang = "zh-CN" + normalized_lang = normalize_ip_api_language(lang) + if normalized_lang != lang: + LOG.warning(f"Language unsupported by ip-api.com ({lang}), using {normalized_lang}")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
ovos_utils/geolocation.py(1 hunks)
🧰 Additional context used
🪛 Ruff
ovos_utils/geolocation.py
66-66: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
89-89: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
157-157: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
246-246: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
🔇 Additional comments (3)
ovos_utils/geolocation.py (3)
28-37: Reuse TimezoneFinder instance for better performance
The previous review comment about reusing the TimezoneFinder instance is still valid. Creating a new instance on every function call is resource-intensive.
63-64: Update User-Agent headers for Nominatim compliance
The previous review comment about updating User-Agent headers to comply with Nominatim's usage policy is still valid.
242-245: Security Concern: Use HTTPS for IP geolocation
The previous review comment about using a geolocation service that supports HTTPS is still valid.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (4)
ovos_utils/geolocation.py (4)
69-95: Consider using a session for multiple requestsThe function makes two separate requests to Nominatim. Using a session would improve performance and connection reuse.
+ session = requests.Session() + session.headers.update({"User-Agent": "OVOS/1.0", "Accept-Language": lang}) + try: - response = requests.get(url, params={"q": location, "format": "json", "limit": 1}, - headers={"User-Agent": "OVOS/1.0", "Accept-Language": lang}, timeout=timeout) + response = session.get(url, params={"q": location, "format": "json", "limit": 1}, + timeout=timeout)
204-206: Simplify coordinate handling and move type conversion earlierThe type conversion and coordinate fallback logic could be simplified and validated earlier.
+ lat = float(details.get("lat") or lat) + lon = float(details.get("lon") or lon) location["timezone"] = get_timezone( - lat=float(details.get("lat") or lat), - lon=float(details.get("lon") or lon)) + lat=lat, lon=lon)
237-244: Simplify language normalization logicThe language normalization could be simplified using a mapping dictionary.
+ LANG_MAPPING = { + "pt": "pt-BR", + "zh": "zh-CN" + } lang = standardize_lang_tag(lang).split("-")[0] if lang not in ["en", "de", "es", "pt", "fr", "ja", "zh", "ru"]: LOG.warning(f"Language unsupported by ip-api.com ({lang}), defaulting to english") lang = "en" - elif lang == "pt": - lang = "pt-BR" - elif lang == "zh": - lang = "zh-CN" + else: + lang = LANG_MAPPING.get(lang, lang)
263-277: Make response structure consistent with other geolocation functionsThe response structure differs slightly from other geolocation functions (missing 'address' field).
return {"city": city_data, "coordinate": coordinate_data, - "timezone": timezone_data} + "timezone": timezone_data, + "address": f"{data['city']}, {data['regionName']}, {data['country']}"}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
ovos_utils/geolocation.py(1 hunks)
🔇 Additional comments (1)
ovos_utils/geolocation.py (1)
16-49: LGTM! Well-structured timezone lookup implementation.
The implementation is robust with proper coordinate validation, efficient resource handling through lazy loading, and comprehensive error handling.
Summary by CodeRabbit
New Features
Chores
langcodespackage and addedtimezonefinderto project dependencies.Bug Fixes