From 21391734709fb54d7690d233c80b8188dfb54f7a Mon Sep 17 00:00:00 2001 From: Brice Schaffner Date: Mon, 2 May 2022 06:16:26 +0200 Subject: [PATCH 01/10] Reduced the INFO log level output. --- app/__init__.py | 2 +- logging-cfg-local.yml | 6 +----- 2 files changed, 2 insertions(+), 6 deletions(-) diff --git a/app/__init__.py b/app/__init__.py index 9131f885..c837af5f 100644 --- a/app/__init__.py +++ b/app/__init__.py @@ -32,7 +32,7 @@ @app.before_request def log_route(): g.setdefault('request_started', time.time()) - route_logger.info('%s %s', request.method, request.path) + route_logger.debug('%s %s', request.method, request.path) @app.after_request diff --git a/logging-cfg-local.yml b/logging-cfg-local.yml index 846e4699..d1eb2db9 100644 --- a/logging-cfg-local.yml +++ b/logging-cfg-local.yml @@ -38,7 +38,6 @@ filters: - method - headers - remote_addr - - json formatters: standard: @@ -56,7 +55,6 @@ formatters: - flask_request_path - flask_request_method - flask_request_headers - - flask_request_json - flask_request_remote_addr remove_empty: True fmt: @@ -65,13 +63,11 @@ formatters: logger: name module: module function: funcName - process: process - thread: thread + worker_id: "%(process)d/%(thread)x" request: path: flask_request_path method: flask_request_method headers: flask_request_headers - data: flask_request_json remote: flask_request_remote_addr exc_info: exc_info message: message From 42911a39cc8d98deab2f81c94cc034ab93df4a5e Mon Sep 17 00:00:00 2001 From: Brice Schaffner Date: Mon, 2 May 2022 06:22:00 +0200 Subject: [PATCH 02/10] BGDIINF_SB-2347: Improved error message on invalid srid This also allow to improve the coding maintenance on the end to end test side. --- app/helpers/validation/height.py | 6 ++++-- app/settings.py | 2 ++ 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/app/helpers/validation/height.py b/app/helpers/validation/height.py index bb7c4a93..9d0cf047 100644 --- a/app/helpers/validation/height.py +++ b/app/helpers/validation/height.py @@ -3,6 +3,7 @@ from flask import abort from app.helpers.helpers import float_raise_nan +from app.settings import VALID_SRID def validate_lon_lat(lon, lat): @@ -24,9 +25,10 @@ def validate_lon_lat(lon, lat): def validate_sr(sr): - if sr not in (21781, 2056): + if sr not in VALID_SRID: abort( 400, - "Please provide a valid number for the spatial reference system model 21781 or 2056" + "Please provide a valid number for the spatial reference system model: " + f"{','.join(map(str, VALID_SRID))}" ) return sr diff --git a/app/settings.py b/app/settings.py index 328eb2bb..6b300150 100644 --- a/app/settings.py +++ b/app/settings.py @@ -14,3 +14,5 @@ PRELOAD_RASTER_FILES = strtobool(os.getenv('PRELOAD_RASTER_FILES', 'False')) TRAP_HTTP_EXCEPTIONS = True + +VALID_SRID = [21781, 2056] From 3291f3de921220cabcc84f52490f8d88d57464ef Mon Sep 17 00:00:00 2001 From: Brice Schaffner Date: Mon, 2 May 2022 08:37:46 +0200 Subject: [PATCH 03/10] BGDIINF_SB-2347: Fixed callback parameter responses The callback query parameter has two minor issues and was also not very efficient when used on the profile.json endpoint. The first issue was that if given to the profile.csv endpoint it would have transformed the CSV output by wrapping it with the value of callback and marked the output as javascript. However this was an invalid javascript output as CSV is not a javascript valid syntax. This bug has been introduced with the dockerization, before in this case the callback query was simply ignored. Now it is not ignored but returns a 400 HTTP status. The second minor issue, which was more a syntax sugar, was that using the callback query, a useless LF character was added at the end of the json data. The last issue was about efficiency especially used on a big data output (200 points). Although I did not measure the time penalty (so it might have been negligible), I change it to be more efficient and it make also IMHO the code cleaner. The issue was that the data in the response object is binary and due to the implicit call of Flask make_response for a JSON output the `\n` LF character was added. So using the `after_response` flask hook did introduced an unwanted `\n` see above but also the profile.json behavior see above as well, in this hook we had to decode and parse the binary output again and then to encode it again. So now without using this hook we change the output to javascript before encoding it with `make_response`. This is slightly more efficient and require lest IO and CPU. --- app/__init__.py | 8 -- app/routes.py | 32 ++++-- tests/unit_tests/test_height.py | 2 +- tests/unit_tests/test_profile.py | 161 +++++++++++++++++-------------- 4 files changed, 112 insertions(+), 91 deletions(-) diff --git a/app/__init__.py b/app/__init__.py index c837af5f..5336495c 100644 --- a/app/__init__.py +++ b/app/__init__.py @@ -35,14 +35,6 @@ def log_route(): route_logger.debug('%s %s', request.method, request.path) -@app.after_request -def wrap_in_callback_if_present(response): - if "callback" in request.args: - response.headers['Content-Type'] = 'application/javascript' - response.data = f'{request.args.get("callback")}({response.get_data(as_text=True)})' - return response - - # Add CORS Headers to all request @app.after_request def add_cors_header(response): diff --git a/app/routes.py b/app/routes.py index 70325e68..2e5f4aba 100644 --- a/app/routes.py +++ b/app/routes.py @@ -1,3 +1,4 @@ +import json import logging from shapely.geometry import Point @@ -71,20 +72,35 @@ def height_route(): alt = get_height(sr, lon, lat, georaster_utils) if alt is None: abort(400, f'Requested coordinate ({lon},{lat}) out of bounds in sr {sr}') - return {'height': str(alt)} + data = {'height': str(alt)} + if "callback" in request.args: + data = f'{request.args.get("callback")}({json.dumps(data, separators=(",", ":"))})' + response = make_response(data, 200, {'Content-Type': 'application/javascript'}) + else: + response = make_response(data) + return response @app.route('/profile.json', methods=['GET', 'POST']) def profile_json_route(): - return __get_profile_from_helper(True) + profile, status_code = _get_profile(True) + if "callback" in request.args: + data = f'{request.args.get("callback")}({json.dumps(profile, separators=(",", ":"))})' + response = make_response(data, {'Content-Type': 'application/javascript'}) + else: + response = make_response(jsonify(profile)) + return response, status_code @app.route('/profile.csv', methods=['GET', 'POST']) def profile_csv_route(): - return __get_profile_from_helper(False) + if "callback" in request.args: + abort(400, 'callback parameter not supported') + profile, status_code = _get_profile(False) + return str(profile), status_code, {'Content-Type': 'text/csv'} -def __get_profile_from_helper(output_to_json=True): +def _get_profile(output_to_json): linestring = profile_arg_validation.read_linestring() nb_points = profile_arg_validation.read_number_points() is_custom_nb_points = profile_arg_validation.read_is_custom_nb_points() @@ -122,18 +138,14 @@ def __get_profile_from_helper(output_to_json=True): output_to_json=output_to_json, georaster_utils=georaster_utils ) - if output_to_json: - response = jsonify(result) - else: - response = str(result) + # If profile calculation resulted in a lower number of point than requested (because there's no # need to add points closer to each other than the min resolution of 2m), we return HTTP 203 to # notify that nb_points couldn't be match. status_code = 200 if is_custom_nb_points and len(result) < nb_points: status_code = 203 - content_type = 'application/json' if output_to_json else 'text/csv' - return response, status_code, {'ContentType': content_type, 'Content-Type': content_type} + return result, status_code # if in debug, we add the route to the statistics page, otherwise it is not visible diff --git a/tests/unit_tests/test_height.py b/tests/unit_tests/test_height.py index 8bb2053f..39f20670 100644 --- a/tests/unit_tests/test_height.py +++ b/tests/unit_tests/test_height.py @@ -263,7 +263,7 @@ def test_height_lv03_with_callback_valid(self, mock_georaster_utils): } ) self.assertEqual(resp.content_type, 'application/javascript') - self.assertTrue('cb_({' in resp.get_data(as_text=True)) + self.assertEqual('cb_({"height":"568.2"})', resp.get_data(as_text=True)) @patch('app.routes.georaster_utils') def test_height_lv03_miss_northing(self, mock_georaster_utils): diff --git a/tests/unit_tests/test_profile.py b/tests/unit_tests/test_profile.py index 2cdbb43d..637da4b4 100644 --- a/tests/unit_tests/test_profile.py +++ b/tests/unit_tests/test_profile.py @@ -28,71 +28,63 @@ logger = logging.getLogger(__name__) -class TestProfile(unittest.TestCase): - # pylint: disable=too-many-public-methods +class TestProfileBase(unittest.TestCase): def setUp(self) -> None: service_alti.app.config['TESTING'] = True self.test_instance = service_alti.app.test_client() self.headers = DEFAULT_HEADERS - def __check_response(self, response, expected_status=200): + def check_response(self, response, expected_status=200): self.assertIsNotNone(response) self.assertEqual(response.status_code, expected_status, msg=response.get_data(as_text=True)) + def assert_response_contains(self, response, content): + self.assertTrue( + content in response.get_data(as_text=True), + msg=f"Response doesn't contain '{content}' : '{response.get_data(as_text=True)}'" + ) + + +class TestProfileJson(TestProfileBase): + # pylint: disable=too-many-public-methods + + def verify_point_is_present(self, response, point, msg="point not present"): + self.assertEqual(response.content_type, "application/json") + if len(point) != 2: + self.fail("Point must be a [x,y] point") + present = False + for profile_point in response.json: + if point[0] == profile_point['easting'] and point[1] == profile_point['northing']: + present = True + if not present: + self.fail(msg) + + def prepare_mock_and_test_json_profile(self, mock_georaster_utils, params, expected_status): + prepare_mock(mock_georaster_utils) + return self.get_json_profile(params=params, expected_status=expected_status) + def prepare_mock_and_test_post(self, mock_georaster_utils, body, expected_status): prepare_mock(mock_georaster_utils) response = self.post_with_body(body) - self.__check_response(response, expected_status) + self.check_response(response, expected_status) return response - def prepare_mock_and_test_csv_profile(self, mock_georaster_utils, params, expected_status): - prepare_mock(mock_georaster_utils) - response = self.get_csv_with_params(params) - self.__check_response(response, expected_status) - return response + def post_with_body(self, body): + return self.test_instance.post(ENDPOINT_FOR_JSON_PROFILE, data=body, headers=self.headers) - # pylint: disable=inconsistent-return-statements def get_json_profile(self, params, expected_status=200): # pylint: disable=broad-except try: response = self.test_instance.get( ENDPOINT_FOR_JSON_PROFILE, query_string=params, headers=self.headers ) - self.__check_response(response, expected_status) + self.check_response(response, expected_status) return response except Exception as e: logger.exception(e) self.fail(f'Call to test_instance failed: {e}') - - def get_csv_with_params(self, params): - return self.test_instance.get( - ENDPOINT_FOR_CSV_PROFILE, query_string=params, headers=self.headers - ) - - def post_with_body(self, body): - return self.test_instance.post(ENDPOINT_FOR_JSON_PROFILE, data=body, headers=self.headers) - - def prepare_mock_and_test_json_profile(self, mock_georaster_utils, params, expected_status): - prepare_mock(mock_georaster_utils) - return self.get_json_profile(params=params, expected_status=expected_status) - - def verify_point_is_present(self, response, point, msg="point not present"): - self.assertEqual(response.content_type, "application/json") - if len(point) != 2: - self.fail("Point must be a [x,y] point") - present = False - for profile_point in response.json: - if point[0] == profile_point['easting'] and point[1] == profile_point['northing']: - present = True - if not present: - self.fail(msg) - - def assert_response_contains(self, response, content): - self.assertTrue( - content in response.get_data(as_text=True), - msg=f"Response doesn't contain '{content}' : '{response.get_data(as_text=True)}'" - ) + return None @patch('app.routes.georaster_utils') def test_do_not_fail_when_no_origin(self, mock_georaster_utils): @@ -306,38 +298,6 @@ def test_profile_lv03_json_default_nb_points(self, mock_georaster_utils): self.assertGreaterEqual(len(resp.json), PROFILE_DEFAULT_AMOUNT_POINTS) self.assertGreaterEqual(PROFILE_MAX_AMOUNT_POINTS, len(resp.json)) - @patch('app.routes.georaster_utils') - def test_profile_lv03_csv_valid(self, mock_georaster_utils): - resp = self.prepare_mock_and_test_csv_profile( - mock_georaster_utils=mock_georaster_utils, - params={'geom': create_json(4, 21781)}, - expected_status=200 - ) - self.assertEqual(resp.content_type, 'text/csv') - - @patch('app.routes.georaster_utils') - def test_profile_lv03_cvs_wrong_geom(self, mock_georaster_utils): - resp = self.prepare_mock_and_test_csv_profile( - mock_georaster_utils=mock_georaster_utils, params={'geom': 'toto'}, expected_status=400 - ) - self.assert_response_contains(resp, 'Error loading geometry in JSON string') - - @patch('app.routes.georaster_utils') - def test_profile_lv03_csv_misspelled_shape(self, mock_georaster_utils): - resp = self.prepare_mock_and_test_csv_profile( - mock_georaster_utils=mock_georaster_utils, - params={'geom': LINESTRING_MISSPELLED_SHAPE}, - expected_status=400 - ) - self.assert_response_contains(resp, 'Error loading geometry in JSON string') - - resp = self.prepare_mock_and_test_csv_profile( - mock_georaster_utils=mock_georaster_utils, - params={'geom': LINESTRING_WRONG_SHAPE}, - expected_status=400 - ) - self.assert_response_contains(resp, 'Error converting JSON to Shape') - @patch('app.routes.georaster_utils') def test_profile_lv03_json_invalid_linestring(self, mock_georaster_utils): resp = self.prepare_mock_and_test_json_profile( @@ -455,3 +415,60 @@ def test_profile_all_old_elevation_models_are_returned(self, mock_georaster_util self.fail("All old elevation_models must be returned in alt for compatibility issue") if altitudes['DTM25'] != comb_value: self.fail("All values from all models should be taken from the new COMB layer") + + +class TestProfileCsv(TestProfileBase): + + def prepare_mock_and_test_csv_profile(self, mock_georaster_utils, params, expected_status): + prepare_mock(mock_georaster_utils) + response = self.get_csv_with_params(params) + self.check_response(response, expected_status) + return response + + def get_csv_with_params(self, params): + return self.test_instance.get( + ENDPOINT_FOR_CSV_PROFILE, query_string=params, headers=self.headers + ) + + @patch('app.routes.georaster_utils') + def test_profile_lv03_csv_valid(self, mock_georaster_utils): + resp = self.prepare_mock_and_test_csv_profile( + mock_georaster_utils=mock_georaster_utils, + params={'geom': create_json(4, 21781)}, + expected_status=200 + ) + self.assertEqual(resp.content_type, 'text/csv') + + @patch('app.routes.georaster_utils') + def test_profile_lv03_cvs_wrong_geom(self, mock_georaster_utils): + resp = self.prepare_mock_and_test_csv_profile( + mock_georaster_utils=mock_georaster_utils, params={'geom': 'toto'}, expected_status=400 + ) + self.assert_response_contains(resp, 'Error loading geometry in JSON string') + + @patch('app.routes.georaster_utils') + def test_profile_lv03_csv_misspelled_shape(self, mock_georaster_utils): + resp = self.prepare_mock_and_test_csv_profile( + mock_georaster_utils=mock_georaster_utils, + params={'geom': LINESTRING_MISSPELLED_SHAPE}, + expected_status=400 + ) + self.assert_response_contains(resp, 'Error loading geometry in JSON string') + + resp = self.prepare_mock_and_test_csv_profile( + mock_georaster_utils=mock_georaster_utils, + params={'geom': LINESTRING_WRONG_SHAPE}, + expected_status=400 + ) + self.assert_response_contains(resp, 'Error converting JSON to Shape') + + @patch('app.routes.georaster_utils') + def test_profile_lv03_csv_callback(self, mock_georaster_utils): + resp = self.prepare_mock_and_test_csv_profile( + mock_georaster_utils=mock_georaster_utils, + params={ + 'geom': create_json(4, 21781), 'callback': '_cb' + }, + expected_status=400 + ) + self.assert_response_contains(resp, 'callback parameter not supported') From 17698fdaa72e1abbd1bd5f4c30cae5c038a7f14e Mon Sep 17 00:00:00 2001 From: Brice Schaffner Date: Mon, 2 May 2022 15:01:01 +0200 Subject: [PATCH 04/10] BGDIINF_SB-2378: Corrected the CSV output The profile.csv endpoint did not returned a well csv formed format, but a python dictionary. This has been fixed also fixing the 203 answer code. --- app/routes.py | 19 +++++++++++++++++-- 1 file changed, 17 insertions(+), 2 deletions(-) diff --git a/app/routes.py b/app/routes.py index 2e5f4aba..19afa6ae 100644 --- a/app/routes.py +++ b/app/routes.py @@ -1,5 +1,7 @@ +import csv import json import logging +from io import StringIO from shapely.geometry import Point from werkzeug.exceptions import HTTPException @@ -97,7 +99,17 @@ def profile_csv_route(): if "callback" in request.args: abort(400, 'callback parameter not supported') profile, status_code = _get_profile(False) - return str(profile), status_code, {'Content-Type': 'text/csv'} + csv.register_dialect( + 'semi-colon', delimiter=';', quoting=csv.QUOTE_ALL, quotechar='"', lineterminator='\r\n' + ) + buffer = StringIO() + writer = csv.writer(buffer, dialect='semi-colon') + # write header + writer.writerow(profile['headers']) + writer.writerows(profile['rows']) + buffer.seek(0) + + return buffer.read(), status_code, {'Content-Type': 'text/csv'} def _get_profile(output_to_json): @@ -143,8 +155,11 @@ def _get_profile(output_to_json): # need to add points closer to each other than the min resolution of 2m), we return HTTP 203 to # notify that nb_points couldn't be match. status_code = 200 - if is_custom_nb_points and len(result) < nb_points: + if output_to_json and is_custom_nb_points and len(result) < nb_points: status_code = 203 + elif not output_to_json and is_custom_nb_points and len(result['rows']) < nb_points: + status_code = 203 + return result, status_code From 57d4c717d1192d3e95ead69faa4f7d7764d1aac7 Mon Sep 17 00:00:00 2001 From: Brice Schaffner Date: Wed, 4 May 2022 14:15:51 +0200 Subject: [PATCH 05/10] BGDIINF_SB-2381: Fixed boolean query parameters When adding a query parameter with =False it was treated as True. --- app/routes.py | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/app/routes.py b/app/routes.py index 19afa6ae..d1cd8c9c 100644 --- a/app/routes.py +++ b/app/routes.py @@ -1,6 +1,7 @@ import csv import json import logging +from distutils.util import strtobool from io import StringIO from shapely.geometry import Point @@ -122,7 +123,7 @@ def _get_profile(output_to_json): # param only_requested_points, which is flag that when set to True will make # the profile with only the given points in geom (no filling points) if 'only_requested_points' in request.args: - only_requested_points = bool(request.args.get('only_requested_points')) + only_requested_points = strtobool(request.args.get('only_requested_points')) else: only_requested_points = False @@ -130,12 +131,12 @@ def _get_profile(output_to_json): # there's not two points closer than what the resolution is) or if points are placed without # care for that. if 'smart_filling' in request.args: - smart_filling = bool(request.args.get('smart_filling')) + smart_filling = strtobool(request.args.get('smart_filling')) else: smart_filling = False if 'distinct_points' in request.args: - keep_points = bool(request.args.get('distinct_points')) + keep_points = strtobool(request.args.get('distinct_points')) else: keep_points = False From acd21d981b1234be70e927f757d31417244e4049 Mon Sep 17 00:00:00 2001 From: Brice Schaffner Date: Thu, 5 May 2022 11:35:04 +0200 Subject: [PATCH 06/10] BGDIINF_SB-2374: Fixed profile height When no offset is given in URL query parameter we need to disable smoothing by setting the offset to 3. This bug has been introduced with the migration to flask, before the default value was None. --- app/helpers/validation/profile.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/helpers/validation/profile.py b/app/helpers/validation/profile.py index 1d713f0c..8591df87 100644 --- a/app/helpers/validation/profile.py +++ b/app/helpers/validation/profile.py @@ -113,7 +113,7 @@ def read_spatial_reference(linestring): def read_offset(): # param offset, used for smoothing. define how many coordinates should be included # in the window used for smoothing. If value is zero smoothing is disabled. - offset = 3 + offset = 0 if 'offset' in request.args: offset = request.args.get('offset') if offset.isdigit(): From 651372e9291446d02b8f1d22d0ddc9a5b8ce9f95 Mon Sep 17 00:00:00 2001 From: Brice Schaffner Date: Thu, 5 May 2022 13:46:34 +0200 Subject: [PATCH 07/10] BGDIINF_SB-2238: Improved error handling - avoid 500 errors on bad request Avoiding 500 error when the request parameters are invalid. Only LineString and Point are supported as input geometry. Checking this avoid later crash. Removed also broad exception by using correct exception type. Point and LineString shape `is_valid` is a property and not a function and do not raise exception but simply return True or False. Before this kind of invalid shape object where caught later by trying to guess the srid. --- app/helpers/validation/profile.py | 27 ++++++++++++++++----------- tests/unit_tests/test_profile.py | 14 +++++++------- 2 files changed, 23 insertions(+), 18 deletions(-) diff --git a/app/helpers/validation/profile.py b/app/helpers/validation/profile.py index 8591df87..c1317247 100644 --- a/app/helpers/validation/profile.py +++ b/app/helpers/validation/profile.py @@ -14,6 +14,8 @@ logger = logging.getLogger(__name__) max_content_length = 32 * 1024 * 1024 # 32MB +PROFILE_VALID_GEOMETRY_TYPES = ['LineString', 'Point'] + def read_linestring(): # param geom, list of coordinates defining the line on which we want a profile @@ -30,22 +32,25 @@ def read_linestring(): if not linestring: abort(400, "No 'geom' given, cannot create a profile without coordinates") + try: geom = geojson.loads(linestring, object_hook=geojson.GeoJSON.to_instance) except ValueError as e: - logger.exception(e) - abort(400, "Error loading geometry in JSON string") + logger.error('Invalid "geom" parameter, it is not geojson: %s', e) + abort(400, "Invalid geom parameter, must be a GEOJSON") + + if geom.get('type') not in PROFILE_VALID_GEOMETRY_TYPES: + abort(400, f"geom parameter must be a {'/'.join(PROFILE_VALID_GEOMETRY_TYPES)} GEOJSON") + try: geom_to_shape = shape(geom) - # pylint: disable=broad-except - except Exception as e: - logger.exception(e) - abort(400, "Error converting JSON to Shape") - try: - geom_to_shape.is_valid - # pylint: disable=broad-except - except Exception: - abort(400, "Invalid Linestring syntax") + except ValueError as e: + logger.error("Failed to transformed GEOJSON to shape: %s", e) + abort(400, "Error converting GEOJSON to Shape") + + if not geom_to_shape.is_valid: + abort(400, f"Invalid {geom['type']}") + if len(geom_to_shape.coords) > PROFILE_MAX_AMOUNT_POINTS: abort( 413, diff --git a/tests/unit_tests/test_profile.py b/tests/unit_tests/test_profile.py index 637da4b4..d7ff25a0 100644 --- a/tests/unit_tests/test_profile.py +++ b/tests/unit_tests/test_profile.py @@ -171,7 +171,7 @@ def test_profile_lv03_layers_none(self, mock_georaster_utils): params={'geom': '{"type":"LineString","coordinates":[[0,0],[0,0],[0,0]]}'}, expected_status=400 ) - self.assert_response_contains(resp, "No 'sr' given and cannot be guessed from 'geom'") + self.assert_response_contains(resp, "Invalid LineString") def test_profile_lv03_layers_none2(self): resp = self.get_json_profile( @@ -212,7 +212,7 @@ def test_profile_lv03_json_wrong_geom(self, mock_georaster_utils): resp = self.prepare_mock_and_test_json_profile( mock_georaster_utils=mock_georaster_utils, params={'geom': 'toto'}, expected_status=400 ) - self.assert_response_contains(resp, 'Error loading geometry in JSON string') + self.assert_response_contains(resp, 'Invalid geom parameter, must be a GEOJSON') @patch('app.routes.georaster_utils') def test_profile_lv03_json_wrong_shape(self, mock_georaster_utils): @@ -221,7 +221,7 @@ def test_profile_lv03_json_wrong_shape(self, mock_georaster_utils): params={'geom': LINESTRING_WRONG_SHAPE}, expected_status=400 ) - self.assert_response_contains(resp, 'Error converting JSON to Shape') + self.assert_response_contains(resp, 'geom parameter must be a LineString/Point GEOJSON') @patch('app.routes.georaster_utils') def test_profile_lv03_json_nb_points(self, mock_georaster_utils): @@ -305,7 +305,7 @@ def test_profile_lv03_json_invalid_linestring(self, mock_georaster_utils): params={'geom': '{"type":"LineString","coordinates":[[550050,206550]]}'}, expected_status=400 ) - self.assert_response_contains(resp, 'Error converting JSON to Shape') + self.assert_response_contains(resp, 'Error converting GEOJSON to Shape') @patch('app.routes.georaster_utils') def test_profile_lv03_json_offset(self, mock_georaster_utils): @@ -444,7 +444,7 @@ def test_profile_lv03_cvs_wrong_geom(self, mock_georaster_utils): resp = self.prepare_mock_and_test_csv_profile( mock_georaster_utils=mock_georaster_utils, params={'geom': 'toto'}, expected_status=400 ) - self.assert_response_contains(resp, 'Error loading geometry in JSON string') + self.assert_response_contains(resp, 'Invalid geom parameter, must be a GEOJSON') @patch('app.routes.georaster_utils') def test_profile_lv03_csv_misspelled_shape(self, mock_georaster_utils): @@ -453,14 +453,14 @@ def test_profile_lv03_csv_misspelled_shape(self, mock_georaster_utils): params={'geom': LINESTRING_MISSPELLED_SHAPE}, expected_status=400 ) - self.assert_response_contains(resp, 'Error loading geometry in JSON string') + self.assert_response_contains(resp, 'Invalid geom parameter, must be a GEOJSON') resp = self.prepare_mock_and_test_csv_profile( mock_georaster_utils=mock_georaster_utils, params={'geom': LINESTRING_WRONG_SHAPE}, expected_status=400 ) - self.assert_response_contains(resp, 'Error converting JSON to Shape') + self.assert_response_contains(resp, 'geom parameter must be a LineString/Point GEOJSON') @patch('app.routes.georaster_utils') def test_profile_lv03_csv_callback(self, mock_georaster_utils): From 8ce235fa84d1c4a53fa22d8fcce0f3c1716190a9 Mon Sep 17 00:00:00 2001 From: Brice Schaffner Date: Thu, 5 May 2022 14:51:55 +0200 Subject: [PATCH 08/10] BGDIINF_SB-2238: Improved error message for invalid sr in profile endpoint. --- app/helpers/validation/__init__.py | 13 +++++++++++++ app/helpers/validation/height.py | 11 ----------- app/helpers/validation/profile.py | 7 ++----- app/routes.py | 2 +- tests/unit_tests/test_profile.py | 2 +- 5 files changed, 17 insertions(+), 18 deletions(-) diff --git a/app/helpers/validation/__init__.py b/app/helpers/validation/__init__.py index 5a7d85b3..6e716e15 100644 --- a/app/helpers/validation/__init__.py +++ b/app/helpers/validation/__init__.py @@ -2,7 +2,10 @@ from shapely.geometry import Polygon +from flask import abort + from app.helpers.helpers import float_raise_nan +from app.settings import VALID_SRID bboxes = { 2056: @@ -38,3 +41,13 @@ def srs_guesser(geom): sr = epsg break return sr + + +def validate_sr(sr): + if sr not in VALID_SRID: + abort( + 400, + "Please provide a valid number for the spatial reference system model: " + f"{', '.join(map(str, VALID_SRID))}" + ) + return sr diff --git a/app/helpers/validation/height.py b/app/helpers/validation/height.py index 9d0cf047..9d6f7a37 100644 --- a/app/helpers/validation/height.py +++ b/app/helpers/validation/height.py @@ -3,7 +3,6 @@ from flask import abort from app.helpers.helpers import float_raise_nan -from app.settings import VALID_SRID def validate_lon_lat(lon, lat): @@ -22,13 +21,3 @@ def validate_lon_lat(lon, lat): abort(400, "Please provide numerical values for the parameter 'northing'/'lat'") return lon, lat - - -def validate_sr(sr): - if sr not in VALID_SRID: - abort( - 400, - "Please provide a valid number for the spatial reference system model: " - f"{','.join(map(str, VALID_SRID))}" - ) - return sr diff --git a/app/helpers/validation/profile.py b/app/helpers/validation/profile.py index c1317247..9aabcc12 100644 --- a/app/helpers/validation/profile.py +++ b/app/helpers/validation/profile.py @@ -10,6 +10,7 @@ from app.helpers.profile_helpers import PROFILE_DEFAULT_AMOUNT_POINTS from app.helpers.profile_helpers import PROFILE_MAX_AMOUNT_POINTS from app.helpers.validation import srs_guesser +from app.helpers.validation import validate_sr logger = logging.getLogger(__name__) max_content_length = 32 * 1024 * 1024 # 32MB @@ -107,11 +108,7 @@ def read_spatial_reference(linestring): abort(400, "No 'sr' given and cannot be guessed from 'geom'") spatial_reference = sr - if spatial_reference not in (21781, 2056): - abort( - 400, - "Please provide a valid number for the spatial reference system model 21781 or 2056" - ) + validate_sr(spatial_reference) return spatial_reference diff --git a/app/routes.py b/app/routes.py index d1cd8c9c..59a12b94 100644 --- a/app/routes.py +++ b/app/routes.py @@ -22,8 +22,8 @@ from app.helpers.route import prefix_route from app.helpers.validation import bboxes from app.helpers.validation import srs_guesser +from app.helpers.validation import validate_sr from app.helpers.validation.height import validate_lon_lat -from app.helpers.validation.height import validate_sr # add route prefix from app.statistics.statistics import load_json from app.statistics.statistics import prepare_data diff --git a/tests/unit_tests/test_profile.py b/tests/unit_tests/test_profile.py index d7ff25a0..d43b5463 100644 --- a/tests/unit_tests/test_profile.py +++ b/tests/unit_tests/test_profile.py @@ -109,7 +109,7 @@ def test_profile_invalid_sr_json_valid(self, mock_georaster_utils): self.assert_response_contains( resp, "Please provide a valid number for the spatial reference " - "system model 21781 or 2056" + "system model: 21781, 2056" ) @patch('app.routes.georaster_utils') From 8c9239f8b9bb88d8cfdd3f923c0a7af82941851a Mon Sep 17 00:00:00 2001 From: Brice Schaffner Date: Wed, 4 May 2022 14:18:17 +0200 Subject: [PATCH 09/10] BGDIINF_SB-2381: Return 203 when we have more points than expected. --- app/routes.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/app/routes.py b/app/routes.py index 59a12b94..eede2007 100644 --- a/app/routes.py +++ b/app/routes.py @@ -154,11 +154,11 @@ def _get_profile(output_to_json): # If profile calculation resulted in a lower number of point than requested (because there's no # need to add points closer to each other than the min resolution of 2m), we return HTTP 203 to - # notify that nb_points couldn't be match. + # notify that nb_points couldn't be match. Smartfilling can result in more points as expected. status_code = 200 - if output_to_json and is_custom_nb_points and len(result) < nb_points: + if output_to_json and is_custom_nb_points and len(result) != nb_points: status_code = 203 - elif not output_to_json and is_custom_nb_points and len(result['rows']) < nb_points: + elif not output_to_json and is_custom_nb_points and len(result['rows']) != nb_points: status_code = 203 return result, status_code From eeea6aad0d96d6a77214342c9d1f8a651516a144 Mon Sep 17 00:00:00 2001 From: Brice Schaffner Date: Wed, 4 May 2022 15:35:23 +0200 Subject: [PATCH 10/10] BGDIINF_SB-2381: Added unittest and removed a duplicate one --- tests/unit_tests/test_profile.py | 31 +++++++++++++++++++++++-------- 1 file changed, 23 insertions(+), 8 deletions(-) diff --git a/tests/unit_tests/test_profile.py b/tests/unit_tests/test_profile.py index d43b5463..6a26197f 100644 --- a/tests/unit_tests/test_profile.py +++ b/tests/unit_tests/test_profile.py @@ -224,7 +224,18 @@ def test_profile_lv03_json_wrong_shape(self, mock_georaster_utils): self.assert_response_contains(resp, 'geom parameter must be a LineString/Point GEOJSON') @patch('app.routes.georaster_utils') - def test_profile_lv03_json_nb_points(self, mock_georaster_utils): + def test_profile_lv03_json_small_line(self, mock_georaster_utils): + + resp = self.prepare_mock_and_test_json_profile( + mock_georaster_utils=mock_georaster_utils, + params={'geom': LINESTRING_SMALL_LINE_LV03}, + expected_status=200 + ) + self.assertEqual(resp.content_type, 'application/json') + self.assertLessEqual(len(resp.json), PROFILE_DEFAULT_AMOUNT_POINTS) + + @patch('app.routes.georaster_utils') + def test_profile_lv03_json_nb_points_smart_filling(self, mock_georaster_utils): # as 150 is too much for this profile (distance between points will be smaller than 2m # resolution of the altitude model), the service will return 203 and a smaller amount of # points @@ -236,29 +247,33 @@ def test_profile_lv03_json_nb_points(self, mock_georaster_utils): expected_status=203 ) self.assertEqual(resp.content_type, 'application/json') - self.assertNotEqual(len(resp.json), 150) + self.assertLessEqual(len(resp.json), 150) @patch('app.routes.georaster_utils') - def test_profile_lv03_json_simplify_linestring(self, mock_georaster_utils): + def test_profile_lv03_json_fewer_nb_points_as_input_point(self, mock_georaster_utils): + input_points = 4 resp = self.prepare_mock_and_test_json_profile( mock_georaster_utils=mock_georaster_utils, params={ - 'geom': create_json(4, 21781), 'nb_points': '2' + 'geom': create_json(input_points, 21781), 'nb_points': '2' }, - expected_status=200 + expected_status=203 ) self.assertEqual(resp.content_type, 'application/json') + self.assertEqual(len(resp.json), input_points) @patch('app.routes.georaster_utils') - def test_profile_lv03_json_nb_points_smart_filling(self, mock_georaster_utils): + def test_profile_lv03_json_only_input_point(self, mock_georaster_utils): + input_points = 4 resp = self.prepare_mock_and_test_json_profile( mock_georaster_utils=mock_georaster_utils, params={ - 'geom': LINESTRING_SMALL_LINE_LV03, 'smart_filling': True, 'nbPoints': '150' + 'geom': create_json(input_points, 21781), 'only_requested_points': True }, - expected_status=203 + expected_status=200 ) self.assertEqual(resp.content_type, 'application/json') + self.assertEqual(len(resp.json), input_points) @patch('app.routes.georaster_utils') def test_profile_lv03_json_nb_points_wrong(self, mock_georaster_utils):