Repository: geopy/geopy (python)

Issue:
KeyError when trying to lookup up timezone with reverse_timezone
I'm getting the following error when running the following code (with the correct usernames):

```
import json
from geopy.geocoders import Nominatim, GeoNames

lat = 40.6997716
lon = -73.9753359

geolocator = Nominatim(user_agent="test")
tz_lookup = GeoNames(user_agent="test", username="test")
tz = tz_lookup.reverse_timezone((lat, lon))
print tz
```

```
Traceback (most recent call last):
  File "set_timezone.py", line 9, in <module>
    tz = tz_lookup.reverse_timezone((lat, lon))
  File "/usr/local/lib/python2.7/dist-packages/geopy/geocoders/geonames.py", line 301, in reverse_timezone
    return self._parse_json_timezone(response)
  File "/usr/local/lib/python2.7/dist-packages/geopy/geocoders/geonames.py", line 304, in _parse_json_timezone
    return from_timezone_name(response["timezoneId"], raw=response)
KeyError: 'timezoneId'
```

Pull request:
Fix GeoNames reverse_timezone error handling
This PR improves error handling for reverse_timezone methods of GoogleV3 and GeoNames.

Specifically:
1. GoogleV3's reverse_timezone now returns `None` instead of raising KeyError for uncovered points (e.g. Antarctica);
2. GeoNames's reverse_timezone now uses `pytz.FixedOffset` timezone for uncovered points (mentioned in the updated docs);
3. GeoNames's reverse_timezone didn't perform error checking (this is the main cause of #338).

Closes #338 

Patch:
diff --git a/geopy/geocoders/geonames.py b/geopy/geocoders/geonames.py
index 49c76e2..e5ff0cc 100644
--- a/geopy/geocoders/geonames.py
+++ b/geopy/geocoders/geonames.py
@@ -3,13 +3,19 @@ import warnings
 from geopy.compat import urlencode
 from geopy.exc import (
     ConfigurationError,
+    GeocoderAuthenticationFailure,
     GeocoderInsufficientPrivileges,
     GeocoderQueryError,
+    GeocoderQuotaExceeded,
     GeocoderServiceError,
 )
 from geopy.geocoders.base import DEFAULT_SENTINEL, Geocoder
 from geopy.location import Location
-from geopy.timezone import ensure_pytz_is_installed, from_timezone_name
+from geopy.timezone import (
+    ensure_pytz_is_installed,
+    from_fixed_gmt_offset,
+    from_timezone_name,
+)
 from geopy.util import logger
 
 __all__ = ("GeoNames", )
@@ -238,6 +244,7 @@ class GeoNames(Geocoder):
                 '`%s` find_nearby_type is not supported by geopy' % find_nearby_type
             )
 
+        logger.debug("%s.reverse: %s", self.__class__.__name__, url)
         return self._parse_json(
             self._call_geocoder(url, timeout=timeout),
             exactly_one
@@ -267,6 +274,10 @@ class GeoNames(Geocoder):
         """
         Find the timezone for a point in `query`.
 
+        GeoNames always returns a timezone: if the point being queried
+        doesn't have an assigned Olson timezone id, a ``pytz.FixedOffset``
+        timezone is used to produce the :class:`geopy.timezone.Timezone`.
+
         .. versionadded:: 1.18.0
 
         :param query: The coordinates for which you want a timezone.
@@ -295,25 +306,45 @@ class GeoNames(Geocoder):
 
         url = "?".join((self.api_timezone, urlencode(params)))
 
-        logger.debug("%s.timezone: %s", self.__class__.__name__, url)
-        response = self._call_geocoder(url, timeout=timeout)
+        logger.debug("%s.reverse_timezone: %s", self.__class__.__name__, url)
+        return self._parse_json_timezone(
+            self._call_geocoder(url, timeout=timeout)
+        )
 
-        return self._parse_json_timezone(response)
+    def _raise_for_error(self, body):
+        err = body.get('status')
+        if err:
+            code = err['value']
+            message = err['message']
+            # http://www.geonames.org/export/webservice-exception.html
+            if message.startswith("user account not enabled to use"):
+                raise GeocoderInsufficientPrivileges(message)
+            if code == 10:
+                raise GeocoderAuthenticationFailure(message)
+            if code in (18, 19, 20):
+                raise GeocoderQuotaExceeded(message)
+            raise GeocoderServiceError(message)
 
     def _parse_json_timezone(self, response):
-        return from_timezone_name(response["timezoneId"], raw=response)
+        self._raise_for_error(response)
+
+        timezone_id = response.get("timezoneId")
+        if timezone_id is None:
+            # Sometimes (e.g. for Antarctica) GeoNames doesn't return
+            # a `timezoneId` value, but it returns GMT offsets.
+            # Apparently GeoNames always returns these offsets -- for
+            # every single point on the globe.
+            raw_offset = response["rawOffset"]
+            return from_fixed_gmt_offset(raw_offset, raw=response)
+        else:
+            return from_timezone_name(timezone_id, raw=response)
 
     def _parse_json(self, doc, exactly_one):
         """
         Parse JSON response body.
         """
         places = doc.get('geonames', [])
-        err = doc.get('status', None)
-        if err and 'message' in err:
-            if err['message'].startswith("user account not enabled to use"):
-                raise GeocoderInsufficientPrivileges(err['message'])
-            else:
-                raise GeocoderServiceError(err['message'])
+        self._raise_for_error(doc)
         if not len(places):
             return None
 
diff --git a/geopy/geocoders/googlev3.py b/geopy/geocoders/googlev3.py
index 5359f1d..5459d62 100644
--- a/geopy/geocoders/googlev3.py
+++ b/geopy/geocoders/googlev3.py
@@ -329,6 +329,11 @@ class GoogleV3(Geocoder):
            Use :meth:`GoogleV3.reverse_timezone` instead. This method
            will be removed in geopy 2.0.
 
+        .. versionchanged:: 1.18.1
+           Previously a :class:`KeyError` was raised for a point without
+           an assigned Olson timezone id (e.g. for Antarctica).
+           Now this method returns None for such requests.
+
         :param location: The coordinates for which you want a timezone.
         :type location: :class:`geopy.point.Point`, list or tuple of (latitude,
             longitude), or string as "%(latitude)s, %(longitude)s"
@@ -350,7 +355,7 @@ class GoogleV3(Geocoder):
             exception. Set this only if you wish to override, on this call
             only, the value set during the geocoder's initialization.
 
-        :rtype: pytz timezone. See :func:`pytz.timezone`.
+        :rtype: ``None`` or pytz timezone. See :func:`pytz.timezone`.
         """
 
         warnings.warn('%(cls)s.timezone method is deprecated in favor of '
@@ -359,7 +364,10 @@ class GoogleV3(Geocoder):
                       'instead of just pytz timezone. This method will '
                       'be removed in geopy 2.0.' % dict(cls=type(self).__name__),
                       DeprecationWarning, stacklevel=2)
-        return self.reverse_timezone(location, at_time, timeout).pytz_timezone
+        timezone = self.reverse_timezone(location, at_time, timeout)
+        if timezone is None:
+            return None
+        return timezone.pytz_timezone
 
     def reverse_timezone(self, query, at_time=None, timeout=DEFAULT_SENTINEL):
         """
@@ -367,6 +375,11 @@ class GoogleV3(Geocoder):
 
         .. versionadded:: 1.18.0
 
+        .. versionchanged:: 1.18.1
+           Previously a :class:`KeyError` was raised for a point without
+           an assigned Olson timezone id (e.g. for Antarctica).
+           Now this method returns None for such requests.
+
         :param query: The coordinates for which you want a timezone.
         :type query: :class:`geopy.point.Point`, list or tuple of (latitude,
             longitude), or string as "%(latitude)s, %(longitude)s"
@@ -382,7 +395,7 @@ class GoogleV3(Geocoder):
             exception. Set this only if you wish to override, on this call
             only, the value set during the geocoder's initialization.
 
-        :rtype: :class:`geopy.timezone.Timezone`
+        :rtype: ``None`` or :class:`geopy.timezone.Timezone`
         """
         ensure_pytz_is_installed()
 
@@ -398,17 +411,24 @@ class GoogleV3(Geocoder):
             params['key'] = self.api_key
         url = "?".join((self.tz_api, urlencode(params)))
 
-        logger.debug("%s.timezone: %s", self.__class__.__name__, url)
-        response = self._call_geocoder(url, timeout=timeout)
+        logger.debug("%s.reverse_timezone: %s", self.__class__.__name__, url)
+        return self._parse_json_timezone(
+            self._call_geocoder(url, timeout=timeout)
+        )
 
+    def _parse_json_timezone(self, response):
         status = response.get('status')
         if status != 'OK':
             self._check_status(status)
 
-        return self._parse_json_timezone(response)
-
-    def _parse_json_timezone(self, response):
-        return from_timezone_name(response["timeZoneId"], raw=response)
+        timezone_id = response.get("timeZoneId")
+        if timezone_id is None:
+            # Google returns `status: ZERO_RESULTS` for uncovered
+            # points (e.g. for Antarctica), so there's nothing
+            # meaningful to be returned as the `raw` response,
+            # hence we return `None`.
+            return None
+        return from_timezone_name(timezone_id, raw=response)
 
     def _normalize_timezone_at_time(self, at_time):
         if at_time is None:
diff --git a/geopy/timezone.py b/geopy/timezone.py
index 1fd2322..efd570b 100644
--- a/geopy/timezone.py
+++ b/geopy/timezone.py
@@ -8,6 +8,11 @@ except ImportError:
     pytz_available = False
 
 
+__all__ = (
+    "Timezone",
+)
+
+
 def ensure_pytz_is_installed():
     if not pytz_available:
         raise ImportError(
@@ -33,6 +38,12 @@ def from_timezone_name(timezone_name, raw=None):
     return Timezone(pytz_timezone, raw)
 
 
+def from_fixed_gmt_offset(gmt_offset_hours, raw=None):
+    ensure_pytz_is_installed()
+    pytz_timezone = pytz.FixedOffset(gmt_offset_hours * 60)
+    return Timezone(pytz_timezone, raw)
+
+
 class Timezone(object):
     """
     Contains a parsed response for a timezone request, which is


Test patch:
diff --git a/test/geocoders/geonames.py b/test/geocoders/geonames.py
index 88ffd24..06c9a81 100644
--- a/test/geocoders/geonames.py
+++ b/test/geocoders/geonames.py
@@ -1,11 +1,12 @@
 # -*- coding: UTF-8 -*-
 import unittest
+import uuid
 
 import pytz
 
 from geopy import Point
 from geopy.compat import u
-from geopy.exc import GeocoderQueryError
+from geopy.exc import GeocoderAuthenticationFailure, GeocoderQueryError
 from geopy.geocoders import GeoNames
 from test.geocoders.util import GeocoderTestBase, env
 
@@ -34,7 +35,10 @@ class GeoNamesTestCase(GeocoderTestBase):
 
     def reverse_timezone_run(self, payload, expected):
         timezone = self._make_request(self.geocoder.reverse_timezone, **payload)
-        self.assertEqual(timezone.pytz_timezone, expected)
+        if expected is None:
+            self.assertIsNone(timezone)
+        else:
+            self.assertEqual(timezone.pytz_timezone, expected)
         return timezone
 
     def test_unicode_name(self):
@@ -65,6 +69,13 @@ class GeoNamesTestCase(GeocoderTestBase):
         )
         self.assertIn("Times Square", location.address)
 
+    def test_geocode_empty_response(self):
+        self.geocode_run(
+            {"query": "sdlahaslkhdkasldhkjsahdlkash"},
+            {},
+            expect_failure=True,
+        )
+
     def test_reverse_nearby_place_name_raises_for_feature_code(self):
         with self.assertRaises(ValueError):
             self.reverse_run(
@@ -158,3 +169,46 @@ class GeoNamesTestCase(GeocoderTestBase):
             america_new_york,
         )
         self.assertEqual(timezone.raw['countryCode'], 'US')
+
+    def test_reverse_timezone_unknown(self):
+        self.reverse_timezone_run(
+            # Geonames doesn't return `timezoneId` for Antarctica,
+            # but it provides GMT offset which can be used
+            # to create a FixedOffset pytz timezone.
+            {"query": "89.0, 1.0"},
+            pytz.UTC,
+        )
+        self.reverse_timezone_run(
+            {"query": "89.0, 80.0"},
+            pytz.FixedOffset(5 * 60),
+        )
+
+
+class GeoNamesInvalidAccountTestCase(GeocoderTestBase):
+
+    @classmethod
+    def setUpClass(cls):
+        cls.geocoder = GeoNames(username="geopy-not-existing-%s" % uuid.uuid4())
+
+    def reverse_timezone_run(self, payload, expected):
+        timezone = self._make_request(self.geocoder.reverse_timezone, **payload)
+        if expected is None:
+            self.assertIsNone(timezone)
+        else:
+            self.assertEqual(timezone.pytz_timezone, expected)
+        return timezone
+
+    def test_geocode(self):
+        with self.assertRaises(GeocoderAuthenticationFailure):
+            self.geocode_run(
+                {"query": "moscow"},
+                {},
+                expect_failure=True,
+            )
+
+    def test_reverse_timezone(self):
+        with self.assertRaises(GeocoderAuthenticationFailure):
+            self.reverse_timezone_run(
+                {"query": "40.6997716, -73.9753359"},
+                None,
+            )
diff --git a/test/geocoders/googlev3.py b/test/geocoders/googlev3.py
index e5ede6c..9c94f87 100644
--- a/test/geocoders/googlev3.py
+++ b/test/geocoders/googlev3.py
@@ -21,7 +21,10 @@ class GoogleV3TestCase(GeocoderTestBase):
 
     def reverse_timezone_run(self, payload, expected):
         timezone = self._make_request(self.geocoder.reverse_timezone, **payload)
-        self.assertEqual(timezone.pytz_timezone, expected)
+        if expected is None:
+            self.assertIsNone(timezone)
+        else:
+            self.assertEqual(timezone.pytz_timezone, expected)
 
         # `timezone` method is deprecated, but we still support it.
         with warnings.catch_warnings(record=True) as w:
@@ -30,7 +33,10 @@ class GoogleV3TestCase(GeocoderTestBase):
                 payload['location'] = payload['query']
                 del payload['query']
             pytz_timezone = self._make_request(self.geocoder.timezone, **payload)
-            self.assertEqual(pytz_timezone, expected)
+            if expected is None:
+                self.assertIsNone(pytz_timezone)
+            else:
+                self.assertEqual(pytz_timezone, expected)
             self.assertLess(0, len(w))
 
         return timezone
@@ -251,6 +257,13 @@ class GoogleV3TestCase(GeocoderTestBase):
         with self.assertRaises(exc.GeocoderQueryError):
             self.geocoder.reverse_timezone(self.new_york_point, "eek")
 
+    def test_reverse_timezone_unknown(self):
+        self.reverse_timezone_run(
+            # Google doesn't return a timezone for Antarctica.
+            {"query": "89.0, 1.0"},
+            None,
+        )
+
     def test_geocode_bounds(self):
         """
         GoogleV3.geocode check bounds restriction
diff --git a/test/test_timezone.py b/test/test_timezone.py
index 02662c9..6c63093 100644
--- a/test/test_timezone.py
+++ b/test/test_timezone.py
@@ -1,3 +1,4 @@
+import datetime
 import pickle
 import unittest
 
@@ -5,12 +6,13 @@ import pytz
 import pytz.tzinfo
 
 from geopy.compat import u
-from geopy.timezone import Timezone, from_timezone_name
+from geopy.timezone import Timezone, from_fixed_gmt_offset, from_timezone_name
 
 
 class TimezoneTestCase(unittest.TestCase):
 
-    timezone_name = u("Europe/Moscow")
+    timezone_gmt_offset_hours = 3.0
+    timezone_name = u("Europe/Moscow")  # a DST-less timezone
 
     def test_create_from_timezone_name(self):
         raw = dict(foo="bar")
@@ -18,6 +20,19 @@ class TimezoneTestCase(unittest.TestCase):
 
         self.assertEqual(tz.raw['foo'], 'bar')
         self.assertIsInstance(tz.pytz_timezone, pytz.tzinfo.BaseTzInfo)
+        self.assertIsInstance(tz.pytz_timezone, datetime.tzinfo)
+
+    def test_create_from_fixed_gmt_offset(self):
+        raw = dict(foo="bar")
+        tz = from_fixed_gmt_offset(self.timezone_gmt_offset_hours, raw)
+
+        self.assertEqual(tz.raw['foo'], 'bar')
+        # pytz.FixedOffset is not an instanse of pytz.tzinfo.BaseTzInfo.
+        self.assertIsInstance(tz.pytz_timezone, datetime.tzinfo)
+
+        olson_tz = pytz.timezone(self.timezone_name)
+        dt = datetime.datetime.utcnow()
+        self.assertEqual(tz.pytz_timezone.utcoffset(dt), olson_tz.utcoffset(dt))
 
     def test_create_from_pytz_timezone(self):
         pytz_timezone = pytz.timezone(self.timezone_name)