Repository: mapbox/mapbox-sdk-py
Pull request #96 (2015-12-14 18:57:00)

Validation
The primary aim of this PR is to clean up the the validation of inputs and make sure they raise a `MapboxValidationError`. This allows the CLI to catch know validation errors and expose them as clean command line help, rather than python stack traces.

It's fully tested and working but needs review.

* I'm not convinced that the exception naming is all that great
* I implemented most of the logic in the the service classes rather than in the validation module which makes me wonder why have a validation module at all.


Patch:
diff --git a/mapbox/encoding.py b/mapbox/encoding.py
index a027b38..0674d51 100644
--- a/mapbox/encoding.py
+++ b/mapbox/encoding.py
@@ -1,5 +1,6 @@
 import json
 
+from .errors import InvalidFeatureError
 from .polyline.codec import PolylineCodec
 
 
@@ -12,7 +13,7 @@ def _geom_points(geom):
         for position in geom['coordinates']:
             yield tuple(position)
     else:
-        raise ValueError(
+        raise InvalidFeatureError(
             "Unsupported geometry type:{0}".format(geom['type']))
 
 
@@ -42,8 +43,9 @@ def read_points(features):
                 yield pt
 
         else:
-            raise ValueError("Unknown object: Not a GeoJSON Point feature or "
-                             "an object with __geo_interface__:\n{0}".format(feature))
+            raise InvalidFeatureError(
+                "Unknown object: Not a GeoJSON Point feature or "
+                "an object with __geo_interface__:\n{0}".format(feature))
 
 
 def encode_waypoints(features, min_limit=None, max_limit=None, precision=6):
@@ -55,11 +57,13 @@ def encode_waypoints(features, min_limit=None, max_limit=None, precision=6):
               for lon, lat in read_points(features)]
 
     if min_limit is not None and len(coords) < min_limit:
-        raise ValueError("Not enough features to encode waypoints, "
-                         "need at least {0}".format(min_limit))
+        raise InvalidFeatureError(
+            "Not enough features to encode waypoints, "
+            "need at least {0}".format(min_limit))
     if max_limit is not None and len(coords) > max_limit:
-        raise ValueError("Too many features to encode waypoints, "
-                         "need at most {0}".format(max_limit))
+        raise InvalidFeatureError(
+            "Too many features to encode waypoints, "
+            "need at most {0}".format(max_limit))
 
     return ';'.join(coords)
 
diff --git a/mapbox/errors.py b/mapbox/errors.py
new file mode 100644
index 0000000..659072d
--- /dev/null
+++ b/mapbox/errors.py
@@ -0,0 +1,43 @@
+
+class ValidationError(ValueError):
+    pass
+
+
+class InvalidPlaceTypeError(ValidationError):
+    pass
+
+
+class InvalidProfileError(ValidationError):
+    pass
+
+
+class InvalidFeatureError(ValidationError):
+    pass
+
+
+class HTTPError(ValidationError):
+    pass
+
+
+class InvalidCoordError(ValidationError):
+    pass
+
+
+class InputSizeError(ValidationError):
+    pass
+
+
+class ImageSizeError(ValidationError):
+    pass
+
+
+class TokenError(ValidationError):
+    pass
+
+
+class InvalidParameterError(ValidationError):
+    pass
+
+
+class FileIOError(ValidationError):
+    pass
diff --git a/mapbox/services/base.py b/mapbox/services/base.py
index d898201..73d7a04 100644
--- a/mapbox/services/base.py
+++ b/mapbox/services/base.py
@@ -7,6 +7,7 @@ import os
 import requests
 
 from .. import __version__
+from mapbox import errors
 
 
 class Service:
@@ -39,7 +40,7 @@ class Service:
         Token contains base64 encoded json object with username"""
         token = self.session.params.get('access_token')
         if not token:
-            raise ValueError(
+            raise errors.TokenError(
                 "session does not have a valid access_token param")
         data = token.split('.')[1]
         # replace url chars and add padding
@@ -48,14 +49,14 @@ class Service:
         try:
             return json.loads(base64.b64decode(data).decode('utf-8'))['u']
         except (ValueError, KeyError):
-            raise ValueError("access_token does not contain username")
+            raise errors.TokenError(
+                "access_token does not contain username")
 
     def handle_http_error(self, response, custom_messages=None,
                           raise_for_status=False):
         if not custom_messages:
             custom_messages = {}
         if response.status_code in custom_messages.keys():
-            raise requests.exceptions.HTTPError(
-                custom_messages[response.status_code])
+            raise errors.HTTPError(custom_messages[response.status_code])
         if raise_for_status:
             response.raise_for_status()
diff --git a/mapbox/services/directions.py b/mapbox/services/directions.py
index d08a7dd..7e813fd 100644
--- a/mapbox/services/directions.py
+++ b/mapbox/services/directions.py
@@ -2,6 +2,7 @@ from uritemplate import URITemplate
 
 from mapbox.encoding import encode_waypoints
 from mapbox.services.base import Service
+from mapbox import errors
 
 
 class Directions(Service):
@@ -9,13 +10,32 @@ class Directions(Service):
     def __init__(self, access_token=None):
         self.baseuri = 'https://api.mapbox.com/v4/directions'
         self.session = self.get_session(access_token)
+        self.valid_profiles = ['mapbox.driving',
+                               'mapbox.cycling',
+                               'mapbox.walking']
+        self.valid_instruction_formats = ['text', 'html']
+        self.valid_geom_encoding = ['geojson', 'polyline', 'false']
 
     def _validate_profile(self, profile):
-        valid_profiles = ['mapbox.driving', 'mapbox.cycling', 'mapbox.walking']
-        if profile not in valid_profiles:
-            raise ValueError("{} is not a valid profile".format(profile))
+        if profile not in self.valid_profiles:
+            raise errors.InvalidProfileError(
+                "{0} is not a valid profile".format(profile))
         return profile
 
+    def _validate_geom_encoding(self, geom_encoding):
+        if geom_encoding is not None and \
+           geom_encoding not in self.valid_geom_encoding:
+            raise errors.InvalidParameterError(
+                "{0} is not a valid geometry encoding".format(geom_encoding))
+        return geom_encoding
+
+    def _validate_instruction_format(self, instruction_format):
+        if instruction_format is not None and \
+           instruction_format not in self.valid_instruction_formats:
+            raise errors.InvalidParameterError(
+                "{0} is not a valid instruction format".format(instruction_format))
+        return instruction_format
+
     def directions(
             self,
             features,
@@ -26,6 +46,8 @@ class Directions(Service):
             steps=None):
         """Request directions for waypoints encoded as GeoJSON features."""
         profile = self._validate_profile(profile)
+        instructions = self._validate_instruction_format(instructions)
+        geometry = self._validate_geom_encoding(geometry)
         waypoints = encode_waypoints(features, precision=6,
                                      min_limit=2, max_limit=30)
 
diff --git a/mapbox/services/distance.py b/mapbox/services/distance.py
index 247b71e..ec39229 100644
--- a/mapbox/services/distance.py
+++ b/mapbox/services/distance.py
@@ -2,6 +2,7 @@ from uritemplate import URITemplate
 
 from mapbox.encoding import encode_coordinates_json
 from mapbox.services.base import Service
+from mapbox.errors import InvalidProfileError
 
 
 class Distance(Service):
@@ -9,11 +10,11 @@ class Distance(Service):
     def __init__(self, access_token=None):
         self.baseuri = 'https://api.mapbox.com/distances/v1/mapbox'
         self.session = self.get_session(access_token)
+        self.valid_profiles = ['driving', 'cycling', 'walking']
 
     def _validate_profile(self, profile):
-        valid_profiles = ['driving', 'cycling', 'walking']
-        if profile not in valid_profiles:
-            raise ValueError("{} is not a valid profile".format(profile))
+        if profile not in self.valid_profiles:
+            raise InvalidProfileError("{0} is not a valid profile".format(profile))
         return profile
 
     def distances(self, features, profile='driving'):
diff --git a/mapbox/services/geocoding.py b/mapbox/services/geocoding.py
index da01349..0806119 100644
--- a/mapbox/services/geocoding.py
+++ b/mapbox/services/geocoding.py
@@ -1,10 +1,7 @@
 # mapbox
 from uritemplate import URITemplate
 from .base import Service
-
-
-class InvalidPlaceTypeError(KeyError):
-    pass
+from mapbox.errors import InvalidPlaceTypeError
 
 
 class Geocoder(Service):
diff --git a/mapbox/services/mapmatching.py b/mapbox/services/mapmatching.py
index cdeb1c6..87127ca 100644
--- a/mapbox/services/mapmatching.py
+++ b/mapbox/services/mapmatching.py
@@ -3,6 +3,7 @@ import json
 from uritemplate import URITemplate
 
 from mapbox.services.base import Service
+from mapbox import errors
 
 
 class MapMatcher(Service):
@@ -14,21 +15,25 @@ class MapMatcher(Service):
     def _validate_profile(self, profile):
         valid_profiles = ['mapbox.driving', 'mapbox.cycling', 'mapbox.walking']
         if profile not in valid_profiles:
-            raise ValueError("{} is not a valid profile".format(profile))
+            raise errors.InvalidProfileError(
+                "{0} is not a valid profile".format(profile))
         return profile
 
-    def match(self, feature, gps_precision=None, profile='mapbox.driving'):
-        profile = self._validate_profile(profile)
-
+    def _validate_feature(self, val):
         # validate single feature with linestring geometry up to 100 pts
         try:
-            assert feature['type'] == 'Feature'
-            assert feature['geometry']['type'] == 'LineString'
-            assert len(feature['geometry']['coordinates']) <= 100
+            assert val['type'] == 'Feature'
+            assert val['geometry']['type'] == 'LineString'
+            assert len(val['geometry']['coordinates']) <= 100
         except (TypeError, KeyError, AssertionError):
-            raise ValueError("feature must have LineString geometry "
-                             "with <= 100 points")
+            raise errors.InvalidFeatureError(
+                "Feature must have LineString geometry with <= 100 points")
+        return val
+
+    def match(self, feature, gps_precision=None, profile='mapbox.driving'):
+        profile = self._validate_profile(profile)
 
+        feature = self._validate_feature(feature)
         geojson_line_feature = json.dumps(feature)
 
         uri = URITemplate('%s/{profile}.json' % self.baseuri).expand(
diff --git a/mapbox/services/static.py b/mapbox/services/static.py
index ca3bd48..71b52a4 100644
--- a/mapbox/services/static.py
+++ b/mapbox/services/static.py
@@ -3,6 +3,7 @@ import json
 from uritemplate import URITemplate
 
 from mapbox.services.base import Service
+from mapbox import errors
 
 
 class Static(Service):
@@ -11,14 +12,44 @@ class Static(Service):
         self.baseuri = 'https://api.mapbox.com/v4'
         self.session = self.get_session(access_token)
 
+    def _validate_lat(self, val):
+        if val < -85.0511 or val > 85.0511:
+            raise errors.InvalidCoordError(
+                "Latitude must be between -85.0511 and 85.0511")
+        return val
+
+    def _validate_lon(self, val):
+        if val < -180 or val > 180:
+            raise errors.InvalidCoordError(
+                "Longitude must be between -180 and 180")
+        return val
+
+    def _validate_image_size(self, val):
+        if not (1 < val < 1280):
+            raise errors.ImageSizeError(
+                "Image height and width must be between 1 and 1280")
+        return val
+
+    def _validate_overlay(self, val):
+        if len(val) > 4087:  # limit is 4096 minus the 'geojson()'
+            raise errors.InputSizeError(
+                "GeoJSON is too large for the static maps API, "
+                "must be less than 4096 characters")
+        return val
+
     def image(self, mapid, lon=None, lat=None, z=None, features=None,
               width=600, height=600, image_format='png256', sort_keys=False):
 
         if lon and lat and z:
             auto = False
+            lat = self._validate_lat(lat)
+            lon = self._validate_lon(lon)
         else:
             auto = True
 
+        width = self._validate_image_size(width)
+        height = self._validate_image_size(height)
+
         values = dict(
             mapid=mapid,
             lon=str(lon),
@@ -34,9 +65,7 @@ class Static(Service):
                                            separators=(',', ':'),
                                            sort_keys=sort_keys)
 
-            if len(values['overlay']) > 4087:  # limit is 4096 minus the 'geojson()'
-                raise ValueError("geojson is too large for the static maps API, "
-                                 "must be less than 4096 characters")
+            self._validate_overlay(values['overlay'])
 
             if auto:
                 uri = URITemplate(
@@ -48,7 +77,8 @@ class Static(Service):
                     self.baseuri).expand(**values)
         else:
             if auto:
-                raise ValueError("Must provide features if lat, lon, z are None")
+                raise errors.InvalidCoordError(
+                    "Must provide features if lat, lon, z are None")
 
             # No overlay
             uri = URITemplate(
diff --git a/mapbox/services/uploads.py b/mapbox/services/uploads.py
index 6afdf97..8761623 100644
--- a/mapbox/services/uploads.py
+++ b/mapbox/services/uploads.py
@@ -1,9 +1,11 @@
 # mapbox
+import os
 
 from boto3.session import Session as boto3_session
 from uritemplate import URITemplate
 
 from .base import Service
+from mapbox import errors
 
 
 class Uploader(Service):
@@ -38,7 +40,9 @@ class Uploader(Service):
         resp = self.session.get(uri)
         self.handle_http_error(
             resp,
-            custom_messages={404: "Token does not have upload scope"})
+            custom_messages={
+                401: "Token is not authorized",
+                404: "Token does not have upload scope"})
         return resp
 
     def stage(self, filepath, creds=None):
@@ -57,6 +61,9 @@ class Uploader(Service):
             region_name="us-east-1")
 
         s3 = session.resource('s3')
+        if not os.path.exists(filepath):
+            raise errors.FileIOError(
+                "{0} does not exist".format(filepath))
         with open(filepath, 'rb') as data:
             res = s3.Object(creds['bucket'], creds['key']).put(Body=data)
 


Test patch:
diff --git a/tests/test_base.py b/tests/test_base.py
index ee3a068..70c58fb 100644
--- a/tests/test_base.py
+++ b/tests/test_base.py
@@ -57,7 +57,7 @@ def test_custom_messages():
 
     assert service.handle_http_error(response) is None
 
-    with pytest.raises(requests.exceptions.HTTPError) as exc:
+    with pytest.raises(mapbox.errors.HTTPError) as exc:
         assert service.handle_http_error(response, custom_messages={401: "error"})
         assert exc.value.message == 'error'
 
diff --git a/tests/test_directions.py b/tests/test_directions.py
index 3436a39..2765fce 100644
--- a/tests/test_directions.py
+++ b/tests/test_directions.py
@@ -59,8 +59,8 @@ def test_directions_geojson():
 
 def test_invalid_profile():
     with pytest.raises(ValueError):
-        _ = mapbox.Directions(access_token='pk.test').directions(
-                None, profile='bogus')
+        mapbox.Directions(access_token='pk.test').directions(
+            None, profile='bogus')
 
 
 @responses.activate
@@ -74,9 +74,22 @@ def test_direction_params():
         body="not important, only testing URI templating", status=200,
         content_type='application/json')
 
-    res = mapbox.Directions(access_token='pk.test').directions(points,
-                                                          alternatives=False,
-                                                          instructions='html',
-                                                          geometry='polyline',
-                                                          steps=False)
+    res = mapbox.Directions(access_token='pk.test').directions(
+        points,
+        alternatives=False,
+        instructions='html',
+        geometry='polyline',
+        steps=False)
     assert res.status_code == 200
+
+
+def test_invalid_geom_encoding():
+    service = mapbox.Directions(access_token='pk.test')
+    with pytest.raises(mapbox.errors.ValidationError):
+        service._validate_geom_encoding('wkb')
+
+
+def test_invalid_instruction_format():
+    service = mapbox.Directions(access_token='pk.test')
+    with pytest.raises(mapbox.errors.ValidationError):
+        service._validate_instruction_format('markdown')
diff --git a/tests/test_geocoder.py b/tests/test_geocoder.py
index 0f256ce..f2ce150 100644
--- a/tests/test_geocoder.py
+++ b/tests/test_geocoder.py
@@ -110,7 +110,7 @@ def test_validate_place_types_err():
     try:
         mapbox.Geocoder()._validate_place_types(('address', 'bogus'))
     except mapbox.InvalidPlaceTypeError as err:
-        assert str(err) == "'bogus'"
+        assert str(err) == "bogus"
 
 
 def test_validate_place_types():
diff --git a/tests/test_staticmaps.py b/tests/test_staticmaps.py
index 046368b..4e582d4 100644
--- a/tests/test_staticmaps.py
+++ b/tests/test_staticmaps.py
@@ -100,6 +100,25 @@ def test_staticmap_auto_nofeatures(points):
 
 
 def test_staticmap_featurestoolarge(points):
-    with pytest.raises(ValueError):
-        mapbox.Static(access_token='pk.test').image(
-            'mapbox.satellite', -61.7, 12.1, 12, points * 100)
+    service = mapbox.Static(access_token='pk.test')
+    with pytest.raises(mapbox.errors.ValidationError):
+        service._validate_overlay(json.dumps(points * 100))
+
+def test_staticmap_imagesize():
+    service = mapbox.Static(access_token='pk.test')
+    with pytest.raises(mapbox.errors.ValidationError):
+        service._validate_image_size(0)
+    with pytest.raises(mapbox.errors.ValidationError):
+        service._validate_image_size(2000)
+
+def test_latlon():
+    service = mapbox.Static(access_token='pk.test')
+    assert -179.0 == service._validate_lon(-179.0)
+    assert -85.0 == service._validate_lat(-85.0)
+
+def test_lon_invalid():
+    service = mapbox.Static(access_token='pk.test')
+    with pytest.raises(mapbox.errors.ValidationError):
+        service._validate_lat(-86.0)
+    with pytest.raises(mapbox.errors.ValidationError):
+        service._validate_lon(-181.0)
diff --git a/tests/test_upload.py b/tests/test_upload.py
index d3b5ff3..eedb66d 100644
--- a/tests/test_upload.py
+++ b/tests/test_upload.py
@@ -2,6 +2,7 @@ import base64
 import json
 
 import responses
+import pytest
 
 import mapbox
 import mapbox.services.uploads
@@ -237,3 +238,25 @@ def test_upload(monkeypatch):
     assert res.status_code == 201
     job = res.json()
     assert job['tileset'] == "{0}.test1".format(username)
+
+@responses.activate
+def test_upload_doesnotexist():
+    # Credentials
+    query_body = """
+       {{"key": "_pending/{username}/key.test",
+         "accessKeyId": "ak.test",
+         "bucket": "tilestream-tilesets-production",
+         "url": "https://tilestream-tilesets-production.s3.amazonaws.com/_pending/{username}/key.test",
+         "secretAccessKey": "sak.test",
+         "sessionToken": "st.test"}}""".format(username=username)
+
+    responses.add(
+        responses.GET,
+        'https://api.mapbox.com/uploads/v1/{0}/credentials?access_token={1}'.format(username, access_token),
+        match_querystring=True,
+        body=query_body, status=200,
+        content_type='application/json')
+
+    service = mapbox.Uploader(access_token=access_token)
+    with pytest.raises(mapbox.errors.ValidationError):
+        service.upload('tests/doesnotexist.gml', 'test-dne')