From 876bae6b56e573eaa7dedcbb455d044e91e8b828 Mon Sep 17 00:00:00 2001 From: Mario de Frutos Date: Wed, 31 Jan 2018 12:05:27 +0100 Subject: [PATCH 01/11] Going red with bad request parameters --- .../cartodb_services/test/test_mapboxgeocoder.py | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/server/lib/python/cartodb_services/test/test_mapboxgeocoder.py b/server/lib/python/cartodb_services/test/test_mapboxgeocoder.py index 37bbc05..910da0a 100644 --- a/server/lib/python/cartodb_services/test/test_mapboxgeocoder.py +++ b/server/lib/python/cartodb_services/test/test_mapboxgeocoder.py @@ -34,3 +34,18 @@ class MapboxGeocoderTestCase(unittest.TestCase): place = self.geocoder.geocode(searchtext='New York', country='us') assert place + + def test_empty_request(self): + place = self.geocoder.geocode(searchtext='', country=None, city=None, state_province=None) + + assert place == [] + + def test_empty_search_text_request(self): + place = self.geocoder.geocode(searchtext=' ', country='us', city=None, state_province="") + + assert place == [] + + def test_unknown_place_request(self): + place = self.geocoder.geocode(searchtext='[unknown]', country='ch', state_province=None, city=None) + + assert place == [] From 850dc09a4f0a351b1cc23cf23d8b782ff639cdcb Mon Sep 17 00:00:00 2001 From: Mario de Frutos Date: Wed, 31 Jan 2018 12:07:54 +0100 Subject: [PATCH 02/11] Going green with empty responses --- .../python/cartodb_services/cartodb_services/mapbox/geocoder.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/server/lib/python/cartodb_services/cartodb_services/mapbox/geocoder.py b/server/lib/python/cartodb_services/cartodb_services/mapbox/geocoder.py index 5be6da0..6addc81 100644 --- a/server/lib/python/cartodb_services/cartodb_services/mapbox/geocoder.py +++ b/server/lib/python/cartodb_services/cartodb_services/mapbox/geocoder.py @@ -39,7 +39,7 @@ class MapboxGeocoder(Traceable): def _parse_geocoder_response(self, response): json_response = json.loads(response) - if json_response: + if json_response and json_response[ENTRY_FEATURES]: feature = json_response[ENTRY_FEATURES][0] return self._extract_lng_lat_from_feature(feature) From 3583cc6f47ce6782332f6ada8aa8065e6a980f2b Mon Sep 17 00:00:00 2001 From: Mario de Frutos Date: Wed, 31 Jan 2018 12:11:07 +0100 Subject: [PATCH 03/11] Going green with empty requests that leads to 404 responses --- .../cartodb_services/cartodb_services/mapbox/geocoder.py | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/server/lib/python/cartodb_services/cartodb_services/mapbox/geocoder.py b/server/lib/python/cartodb_services/cartodb_services/mapbox/geocoder.py index 6addc81..caca851 100644 --- a/server/lib/python/cartodb_services/cartodb_services/mapbox/geocoder.py +++ b/server/lib/python/cartodb_services/cartodb_services/mapbox/geocoder.py @@ -60,7 +60,10 @@ class MapboxGeocoder(Traceable): @qps_retry(qps=10) def geocode(self, searchtext, city=None, state_province=None, country=None): - address = [searchtext] + if not searchtext: + return [] + else: + address = [searchtext] if city: address.append(city) if state_province: From 56c90cc541053cf1e06d77c04c13701c3866bf97 Mon Sep 17 00:00:00 2001 From: Mario de Frutos Date: Wed, 31 Jan 2018 12:12:52 +0100 Subject: [PATCH 04/11] Bump version to 0.16.3 --- server/lib/python/cartodb_services/setup.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/server/lib/python/cartodb_services/setup.py b/server/lib/python/cartodb_services/setup.py index b5658e6..5808e49 100644 --- a/server/lib/python/cartodb_services/setup.py +++ b/server/lib/python/cartodb_services/setup.py @@ -10,7 +10,7 @@ from setuptools import setup, find_packages setup( name='cartodb_services', - version='0.16.2', + version='0.16.3', description='CartoDB Services API Python Library', From 3484cce88b39e4f384ba72c2e2cd617277c1c809 Mon Sep 17 00:00:00 2001 From: Mario de Frutos Date: Wed, 31 Jan 2018 12:13:02 +0100 Subject: [PATCH 05/11] Update NEWS.md --- NEWS.md | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/NEWS.md b/NEWS.md index 5b4cc73..27a542a 100644 --- a/NEWS.md +++ b/NEWS.md @@ -1,3 +1,8 @@ +January 29th, 2018 +================== +* Version `0.16.3` of the python library + * Fix for Mapbox geocoder to handle empty requests and empty responses + January 29th, 2018 ================== * Version `0.30.1` of server side From 2f8dbbb5dc31e2c59f98593799843d0cce7a765c Mon Sep 17 00:00:00 2001 From: Mario de Frutos Date: Wed, 31 Jan 2018 12:16:13 +0100 Subject: [PATCH 06/11] Dont raise exception when empty params passed to HERE geocoder --- .../python/cartodb_services/cartodb_services/here/geocoder.py | 2 +- .../lib/python/cartodb_services/test/test_heremapsgeocoder.py | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/server/lib/python/cartodb_services/cartodb_services/here/geocoder.py b/server/lib/python/cartodb_services/cartodb_services/here/geocoder.py index 6cd0a7b..29166c6 100644 --- a/server/lib/python/cartodb_services/cartodb_services/here/geocoder.py +++ b/server/lib/python/cartodb_services/cartodb_services/here/geocoder.py @@ -70,7 +70,7 @@ class HereMapsGeocoder(Traceable): if value: params[key] = value if not params: - raise NoGeocodingParams() + return [] return self._execute_geocode(params) def _execute_geocode(self, params): diff --git a/server/lib/python/cartodb_services/test/test_heremapsgeocoder.py b/server/lib/python/cartodb_services/test/test_heremapsgeocoder.py index daf813d..4cdaef5 100644 --- a/server/lib/python/cartodb_services/test/test_heremapsgeocoder.py +++ b/server/lib/python/cartodb_services/test/test_heremapsgeocoder.py @@ -128,8 +128,8 @@ class HereMapsGeocoderTestCase(unittest.TestCase): def test_geocode_address_with_no_params(self, req_mock): req_mock.register_uri('GET', HereMapsGeocoder.PRODUCTION_GEOCODE_JSON_URL, text=self.GOOD_RESPONSE) - with self.assertRaises(NoGeocodingParams): - self.geocoder.geocode() + result = self.geocoder.geocode() + self.assertEqual(result, []) def test_geocode_address_empty_response(self, req_mock): req_mock.register_uri('GET', HereMapsGeocoder.PRODUCTION_GEOCODE_JSON_URL, From 2bbd6bac9112f461c274cbf2660cbbe192ae4f7f Mon Sep 17 00:00:00 2001 From: Mario de Frutos Date: Wed, 31 Jan 2018 12:25:18 +0100 Subject: [PATCH 07/11] Going red with non empty request string --- .../python/cartodb_services/test/test_heremapsgeocoder.py | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/server/lib/python/cartodb_services/test/test_heremapsgeocoder.py b/server/lib/python/cartodb_services/test/test_heremapsgeocoder.py index 4cdaef5..70f573e 100644 --- a/server/lib/python/cartodb_services/test/test_heremapsgeocoder.py +++ b/server/lib/python/cartodb_services/test/test_heremapsgeocoder.py @@ -131,6 +131,12 @@ class HereMapsGeocoderTestCase(unittest.TestCase): result = self.geocoder.geocode() self.assertEqual(result, []) + def test_geocode_address_with_non_empty_string_params(self, req_mock): + req_mock.register_uri('GET', HereMapsGeocoder.PRODUCTION_GEOCODE_JSON_URL, + text=self.GOOD_RESPONSE) + result = self.geocoder.geocode(searchtext=" ", city=None, state=" ", country=" ") + self.assertEqual(result, []) + def test_geocode_address_empty_response(self, req_mock): req_mock.register_uri('GET', HereMapsGeocoder.PRODUCTION_GEOCODE_JSON_URL, text=self.EMPTY_RESPONSE) From 544e0fa7639fea2165f598ba8f2b10fa11d5eafc Mon Sep 17 00:00:00 2001 From: Mario de Frutos Date: Wed, 31 Jan 2018 12:25:35 +0100 Subject: [PATCH 08/11] Going green with non empty request strings --- .../python/cartodb_services/cartodb_services/here/geocoder.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/server/lib/python/cartodb_services/cartodb_services/here/geocoder.py b/server/lib/python/cartodb_services/cartodb_services/here/geocoder.py index 29166c6..6c3b81c 100644 --- a/server/lib/python/cartodb_services/cartodb_services/here/geocoder.py +++ b/server/lib/python/cartodb_services/cartodb_services/here/geocoder.py @@ -67,7 +67,7 @@ class HereMapsGeocoder(Traceable): def geocode(self, **kwargs): params = {} for key, value in kwargs.iteritems(): - if value: + if value and value.strip(): params[key] = value if not params: return [] From 07f00cc0ae5619ac25e0f022b3341edaaab0f3e7 Mon Sep 17 00:00:00 2001 From: Mario de Frutos Date: Wed, 31 Jan 2018 13:17:36 +0100 Subject: [PATCH 09/11] Added tests for the client/channel part of google --- .../test/test_google_client_factory.py | 20 +++++++++++++------ .../test/test_googlegeocoder.py | 15 ++++++++++++++ 2 files changed, 29 insertions(+), 6 deletions(-) diff --git a/server/lib/python/cartodb_services/test/test_google_client_factory.py b/server/lib/python/cartodb_services/test/test_google_client_factory.py index f09da5a..7fd001b 100644 --- a/server/lib/python/cartodb_services/test/test_google_client_factory.py +++ b/server/lib/python/cartodb_services/test/test_google_client_factory.py @@ -18,10 +18,10 @@ class GoogleMapsClientFactoryTestCase(unittest.TestCase): GoogleMapsClientFactory.clients = {} def test_consecutive_calls_with_same_params_return_same_client(self): - id = 'any_id' + client_id = 'any_id' key = base64.b64encode('any_key') - client1 = GoogleMapsClientFactory.get(id, key) - client2 = GoogleMapsClientFactory.get(id, key) + client1 = GoogleMapsClientFactory.get(client_id, key) + client2 = GoogleMapsClientFactory.get(client_id, key) self.assertEqual(client1, client2) def test_consecutive_calls_with_different_key_return_different_clients(self): @@ -29,11 +29,11 @@ class GoogleMapsClientFactoryTestCase(unittest.TestCase): This requirement is important for security reasons as well as not to cache a wrong key accidentally. """ - id = 'any_id' + client_id = 'any_id' key1 = base64.b64encode('any_key') key2 = base64.b64encode('another_key') - client1 = GoogleMapsClientFactory.get(id, key1) - client2 = GoogleMapsClientFactory.get(id, key2) + client1 = GoogleMapsClientFactory.get(client_id, key1) + client2 = GoogleMapsClientFactory.get(client_id, key2) self.assertNotEqual(client1, client2) def test_consecutive_calls_with_different_ids_return_different_clients(self): @@ -59,3 +59,11 @@ class GoogleMapsClientFactoryTestCase(unittest.TestCase): def test_credentials_with_underscores_can_be_valid(self): client = GoogleMapsClientFactory.get('yet_another_dummy_client_id', 'Ola_k_ase___') self.assertIsInstance(client, googlemaps.Client) + + def test_invalid_credentials(self): + with self.assertRaises(InvalidGoogleCredentials): + GoogleMapsClientFactory.get('dummy_client_id', 'lalala') + + def test_credentials_with_channel(self): + client = GoogleMapsClientFactory.get('yet_another_dummy_client_id', 'Ola_k_ase___', 'channel') + self.assertIsInstance(client, googlemaps.Client) diff --git a/server/lib/python/cartodb_services/test/test_googlegeocoder.py b/server/lib/python/cartodb_services/test/test_googlegeocoder.py index ecbaa7b..bf56a60 100644 --- a/server/lib/python/cartodb_services/test/test_googlegeocoder.py +++ b/server/lib/python/cartodb_services/test/test_googlegeocoder.py @@ -118,3 +118,18 @@ class GoogleGeocoderTestCase(unittest.TestCase): searchtext='Calle Eloy Gonzalo 27', city='Madrid', country='España') + + def test_client_data_extraction(self, req_mock): + client_id, channel = self.geocoder.parse_client_id('dummy_client_id') + self.assertEqual(client_id, 'dummy_client_id') + self.assertEqual(channel, None) + + def test_client_data_extraction_with_client_parameter(self, req_mock): + client_id, channel = self.geocoder.parse_client_id('client=gme-test') + self.assertEqual(client_id, 'gme-test') + self.assertEqual(channel, None) + + def test_client_data_extraction_with_client_and_channel_parameter(self, req_mock): + client_id, channel = self.geocoder.parse_client_id('client=gme-test&channel=testchannel') + self.assertEqual(client_id, 'gme-test') + self.assertEqual(channel, 'testchannel') From 02cb4862f9da26bf22f6c05c6665f1b404a4ce99 Mon Sep 17 00:00:00 2001 From: Mario de Frutos Date: Wed, 31 Jan 2018 13:18:09 +0100 Subject: [PATCH 10/11] Update NEWS.md --- NEWS.md | 3 +++ 1 file changed, 3 insertions(+) diff --git a/NEWS.md b/NEWS.md index 27a542a..1ab552a 100644 --- a/NEWS.md +++ b/NEWS.md @@ -2,6 +2,9 @@ January 29th, 2018 ================== * Version `0.16.3` of the python library * Fix for Mapbox geocoder to handle empty requests and empty responses + * Remove raising an exception when non parameters are passed to the HERE geocoder + * Fix for HERE geocoder with non empty requests + * Added more coverage to the google geocoder credentials parse logic January 29th, 2018 ================== From 6ae8aa45fd561944587518b7ddf639683a652389 Mon Sep 17 00:00:00 2001 From: Mario de Frutos Date: Wed, 31 Jan 2018 16:13:20 +0100 Subject: [PATCH 11/11] CR changes --- NEWS.md | 2 +- .../cartodb_services/mapbox/geocoder.py | 14 +++++++------- 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/NEWS.md b/NEWS.md index 1ab552a..bf9629f 100644 --- a/NEWS.md +++ b/NEWS.md @@ -1,4 +1,4 @@ -January 29th, 2018 +January 31th, 2018 ================== * Version `0.16.3` of the python library * Fix for Mapbox geocoder to handle empty requests and empty responses diff --git a/server/lib/python/cartodb_services/cartodb_services/mapbox/geocoder.py b/server/lib/python/cartodb_services/cartodb_services/mapbox/geocoder.py index caca851..ceba105 100644 --- a/server/lib/python/cartodb_services/cartodb_services/mapbox/geocoder.py +++ b/server/lib/python/cartodb_services/cartodb_services/mapbox/geocoder.py @@ -60,14 +60,14 @@ class MapboxGeocoder(Traceable): @qps_retry(qps=10) def geocode(self, searchtext, city=None, state_province=None, country=None): - if not searchtext: - return [] - else: + if searchtext and searchtext.strip(): address = [searchtext] - if city: - address.append(city) - if state_province: - address.append(state_province) + if city: + address.append(city) + if state_province: + address.append(state_province) + else: + return [] country = [country] if country else None