From 4d2b06d40d9b6cd36ca2b953436f7f5442676b7f Mon Sep 17 00:00:00 2001 From: Moritz Schott Date: Wed, 31 Jul 2024 15:23:11 +0200 Subject: [PATCH 01/10] fix: reoder exception handling to correctly parse a broken response --- CHANGELOG.md | 1 + ohsome/clients.py | 34 +++++++++++++++++----------------- ohsome/test/test_exceptions.py | 28 ++++++++++++++++++++++++++++ 3 files changed, 46 insertions(+), 17 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 15ff609..1b5bb75 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,7 @@ ### Fixed +- Ordering of exception handling to correctly parse a broken response - Assert that the expected columns are also present if the result is empty ## 0.3.2 diff --git a/ohsome/clients.py b/ohsome/clients.py index 4053e33..0db6bcc 100644 --- a/ohsome/clients.py +++ b/ohsome/clients.py @@ -15,7 +15,7 @@ import shapely from requests import Session from requests.adapters import HTTPAdapter -from requests.exceptions import RetryError +from requests.exceptions import RetryError, JSONDecodeError from urllib3 import Retry from ohsome import OhsomeException, OhsomeResponse @@ -363,6 +363,22 @@ def _handle_request(self) -> OhsomeResponse: response=e.response, ) + except (ValueError, JSONDecodeError) as e: + if response: + error_code, message = extract_error_message_from_invalid_json( + response.text + ) + else: + message = str(e) + error_code = None + ohsome_exception = OhsomeException( + message=message, + url=self._url, + error_code=error_code, + params=self._parameters, + response=response, + ) + except requests.exceptions.RequestException as e: if isinstance(e, RetryError): # retry one last time without retries, this will raise the original error instead of a cryptic retry @@ -386,22 +402,6 @@ def _handle_request(self) -> OhsomeResponse: error_code=440, ) - except ValueError as e: - if response: - error_code, message = extract_error_message_from_invalid_json( - response.text - ) - else: - message = str(e) - error_code = None - ohsome_exception = OhsomeException( - message=message, - url=self._url, - error_code=error_code, - params=self._parameters, - response=response, - ) - except AttributeError: ohsome_exception = OhsomeException( message=f"Seems like {self._url} is not a valid endpoint.", diff --git a/ohsome/test/test_exceptions.py b/ohsome/test/test_exceptions.py index 786a05e..d2805b0 100644 --- a/ohsome/test/test_exceptions.py +++ b/ohsome/test/test_exceptions.py @@ -43,6 +43,34 @@ def test_timeout_error(base_client): ) +def test_borken_response_timeout_error(base_client): + """ + Test whether an OhsomeException is raised, if the ohsome API contains a JSONDecodeError + :return: + """ + + bboxes = "8.67066,49.41423,8.68177,49.4204" + time = "2010-01-01/2011-01-01/P1Y" + fltr = "building=* and type:way" + timeout = 30 + + client = base_client + with pytest.raises(ohsome.OhsomeException) as e_info: + with responses.RequestsMock() as rsps: + rsps.post( + "https://api.ohsome.org/v1/elements/geometry", + body=b'{\n "attribution" : {\n "url" : "https://ohsome.org/copyrights",\n "text" : "\xc2\xa9 OpenStreetMap contributors"\n },\n "apiVersion" : "1.10.3",\n "type" : "FeatureCollection",\n "features" : [{\n "timestamp" : "2024-07-31T10:37:31.603661767",\n "status" : 413,\n "message" : "The given query is too large in respect to the given timeout. Please use a smaller region and/or coarser time period.",\n "requestUrl" : "https://api.ohsome.org/v1/elements/geometry"\n}', + ) + client.elements.geometry.post( + bboxes=bboxes, time=time, filter=fltr, timeout=timeout + ) + assert ( + "The given query is too large in respect to the given timeout. Please use a smaller region and/or coarser " + "time period." in e_info.value.message + ) + assert e_info.value.error_code == 413 + + @pytest.mark.vcr def test_invalid_url(): """ From 6fc7b035ebf56fb46b9ce27b97200798f2d00e8e Mon Sep 17 00:00:00 2001 From: Moritz Schott Date: Wed, 7 Aug 2024 18:06:14 +0200 Subject: [PATCH 02/10] fix typo Co-authored-by: Matthias (~talfus-laddus) <83658582+matthiasschaub@users.noreply.github.com> --- ohsome/test/test_exceptions.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ohsome/test/test_exceptions.py b/ohsome/test/test_exceptions.py index d2805b0..b30e7f0 100644 --- a/ohsome/test/test_exceptions.py +++ b/ohsome/test/test_exceptions.py @@ -43,7 +43,7 @@ def test_timeout_error(base_client): ) -def test_borken_response_timeout_error(base_client): +def test_broken_response_timeout_error(base_client): """ Test whether an OhsomeException is raised, if the ohsome API contains a JSONDecodeError :return: From 9dbcb894918853cc77aab4665a0fefee8a3b7606 Mon Sep 17 00:00:00 2001 From: Moritz Schott Date: Wed, 7 Aug 2024 18:06:55 +0200 Subject: [PATCH 03/10] improve test method documentation Co-authored-by: Matthias (~talfus-laddus) <83658582+matthiasschaub@users.noreply.github.com> --- ohsome/test/test_exceptions.py | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/ohsome/test/test_exceptions.py b/ohsome/test/test_exceptions.py index b30e7f0..37a4577 100644 --- a/ohsome/test/test_exceptions.py +++ b/ohsome/test/test_exceptions.py @@ -44,10 +44,7 @@ def test_timeout_error(base_client): def test_broken_response_timeout_error(base_client): - """ - Test whether an OhsomeException is raised, if the ohsome API contains a JSONDecodeError - :return: - """ + """Test whether an OhsomeException is raised in case of a JSONDecodeError.""" bboxes = "8.67066,49.41423,8.68177,49.4204" time = "2010-01-01/2011-01-01/P1Y" From 486de1845dde2d2b079eb996cf4449f7c8ba0a6d Mon Sep 17 00:00:00 2001 From: "dependabot[bot]" <49699333+dependabot[bot]@users.noreply.github.com> Date: Mon, 2 Sep 2024 13:41:36 +0200 Subject: [PATCH 04/10] build(deps-dev): bump notebook from 7.2.1 to 7.2.2 in the pip group (#166) Bumps the pip group with 1 update: [notebook](https://github.com/jupyter/notebook). Updates `notebook` from 7.2.1 to 7.2.2 - [Release notes](https://github.com/jupyter/notebook/releases) - [Changelog](https://github.com/jupyter/notebook/blob/@jupyter-notebook/tree@7.2.2/CHANGELOG.md) - [Commits](https://github.com/jupyter/notebook/compare/@jupyter-notebook/tree@7.2.1...@jupyter-notebook/tree@7.2.2) --- updated-dependencies: - dependency-name: notebook dependency-type: indirect dependency-group: pip ... Signed-off-by: dependabot[bot] Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> --- poetry.lock | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/poetry.lock b/poetry.lock index 700ca98..07f389a 100644 --- a/poetry.lock +++ b/poetry.lock @@ -1598,13 +1598,13 @@ files = [ [[package]] name = "notebook" -version = "7.2.1" +version = "7.2.2" description = "Jupyter Notebook - A web-based notebook environment for interactive computing" optional = false python-versions = ">=3.8" files = [ - {file = "notebook-7.2.1-py3-none-any.whl", hash = "sha256:f45489a3995746f2195a137e0773e2130960b51c9ac3ce257dbc2705aab3a6ca"}, - {file = "notebook-7.2.1.tar.gz", hash = "sha256:4287b6da59740b32173d01d641f763d292f49c30e7a51b89c46ba8473126341e"}, + {file = "notebook-7.2.2-py3-none-any.whl", hash = "sha256:c89264081f671bc02eec0ed470a627ed791b9156cad9285226b31611d3e9fe1c"}, + {file = "notebook-7.2.2.tar.gz", hash = "sha256:2ef07d4220421623ad3fe88118d687bc0450055570cdd160814a59cf3a1c516e"}, ] [package.dependencies] From 62d86d851a7bdc572d2fd1b6e833e065eba5bfce Mon Sep 17 00:00:00 2001 From: Moritz Schott Date: Tue, 3 Sep 2024 21:21:30 +0200 Subject: [PATCH 05/10] move raise of exception to right locatoin my adapting the mock --- ohsome/test/test_exceptions.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ohsome/test/test_exceptions.py b/ohsome/test/test_exceptions.py index 37a4577..f840987 100644 --- a/ohsome/test/test_exceptions.py +++ b/ohsome/test/test_exceptions.py @@ -219,7 +219,7 @@ def test_exception_connection_reset(base_client): """ with patch( - "requests.Response.raise_for_status", + "requests.sessions.Session.post", MagicMock( side_effect=RequestException( "This request was failed on purpose without response!" From a0ac8e5b4d8ec3b2692d04a462657e62064a01c9 Mon Sep 17 00:00:00 2001 From: Moritz Schott Date: Tue, 3 Sep 2024 21:22:55 +0200 Subject: [PATCH 06/10] make URL optional for response and add typing to helper function --- ohsome/helper.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/ohsome/helper.py b/ohsome/helper.py index a34e4f9..ec5bc60 100644 --- a/ohsome/helper.py +++ b/ohsome/helper.py @@ -6,7 +6,7 @@ import datetime import json import re -from typing import Tuple, Union, List +from typing import Tuple, Union, List, Optional import geopandas as gpd import numpy as np @@ -245,12 +245,12 @@ def format_list_parameters(parameters: dict) -> dict: return parameters -def find_groupby_names(url): +def find_groupby_names(url: Optional[str]) -> List[str]: """ Get the groupBy names :return: """ - return [name.strip("/") for name in url.split("groupBy")[1:]] + return [name.strip("/") for name in url.split("groupBy")[1:]] if url else [] def extract_error_message_from_invalid_json(responsetext: str) -> Tuple[int, str]: From 3d3bd1c2f0d9175e848f87fe8725a7df64f56561 Mon Sep 17 00:00:00 2001 From: Moritz Schott Date: Tue, 3 Sep 2024 21:27:39 +0200 Subject: [PATCH 07/10] remove unsued attributes from ohsome response (debatable and, if strict is a major breaking change) --- ohsome/clients.py | 2 +- ohsome/response.py | 6 ++---- ohsome/test/conftest.py | 2 +- ohsome/test/test_response.py | 2 +- 4 files changed, 5 insertions(+), 7 deletions(-) diff --git a/ohsome/clients.py b/ohsome/clients.py index 0db6bcc..0ff62e6 100644 --- a/ohsome/clients.py +++ b/ohsome/clients.py @@ -416,7 +416,7 @@ def _handle_request(self) -> OhsomeResponse: ohsome_exception.log(self.log_dir) raise ohsome_exception - return OhsomeResponse(response, url=self._url, params=self._parameters) + return OhsomeResponse(data=data, url=self._url) def _format_parameters(self, params): """ diff --git a/ohsome/response.py b/ohsome/response.py index 4301475..259929a 100644 --- a/ohsome/response.py +++ b/ohsome/response.py @@ -16,12 +16,10 @@ class OhsomeResponse: """Contains the response of the request to the ohsome API""" - def __init__(self, response=None, url=None, params=None): + def __init__(self, data: dict, url: str = None): """Initialize the OhsomeResponse class.""" - self.response = response + self.data = data self.url = url - self.parameters = params - self.data = response.json() def as_dataframe( self, multi_index: Optional[bool] = True, explode_tags: Optional[tuple] = () diff --git a/ohsome/test/conftest.py b/ohsome/test/conftest.py index 2b7ee16..bf19e53 100644 --- a/ohsome/test/conftest.py +++ b/ohsome/test/conftest.py @@ -116,4 +116,4 @@ def dummy_ohsome_response() -> OhsomeResponse: ) response = Response() response._content = test_gdf.to_json().encode() - return OhsomeResponse(response=response) + return OhsomeResponse(data=response.json()) diff --git a/ohsome/test/test_response.py b/ohsome/test/test_response.py index fa3c3c5..6d01e70 100644 --- a/ohsome/test/test_response.py +++ b/ohsome/test/test_response.py @@ -561,7 +561,7 @@ def test_explode_tags_present_on_empty_result(): '{"attribution":{"url":"https://ohsome.org/copyrights","text":"© OpenStreetMap contributors"},' '"apiVersion":"1.10.1","type":"FeatureCollection","features":[]}' ).encode() - computed_df = OhsomeResponse(response=response).as_dataframe( + computed_df = OhsomeResponse(data=response.json()).as_dataframe( explode_tags=("some_key", "some_other_key") ) From eeab650b2fc6322d9b517812e6d6bc753c59bcc3 Mon Sep 17 00:00:00 2001 From: Moritz Schott Date: Tue, 3 Sep 2024 21:28:53 +0200 Subject: [PATCH 08/10] move try except to separate functions --- ohsome/clients.py | 94 ++++++++++++++++++++++++----------------------- 1 file changed, 49 insertions(+), 45 deletions(-) diff --git a/ohsome/clients.py b/ohsome/clients.py index 0ff62e6..81be036 100644 --- a/ohsome/clients.py +++ b/ohsome/clients.py @@ -13,7 +13,7 @@ import pandas as pd import requests import shapely -from requests import Session +from requests import Session, Response from requests.adapters import HTTPAdapter from requests.exceptions import RetryError, JSONDecodeError from urllib3 import Retry @@ -332,21 +332,55 @@ def _handle_request(self) -> OhsomeResponse: Handles request to ohsome API :return: """ - ohsome_exception = None - response = None + try: + response = self._post_request() + self._check_response(response) + data = self._get_response_data(response) + except OhsomeException as ohsome_exception: + if self.log: + ohsome_exception.log(self.log_dir) + raise ohsome_exception + + return OhsomeResponse(data=data, url=self._url) + + def _post_request(self) -> Response: try: response = self._session().post(url=self._url, data=self._parameters) - response.raise_for_status() - response.json() + except requests.exceptions.ConnectionError as e: + raise OhsomeException( + message="Connection Error: Query could not be sent. Make sure there are no network " + f"problems and that the ohsome API URL {self._url} is valid.", + url=self._url, + params=self._parameters, + response=e.response, + ) + except requests.exceptions.RequestException as e: + if isinstance(e, RetryError): + # retry one last time without retries, this will raise the original error instead of a cryptic retry + # error (or succeed) + self._OhsomeBaseClient__session = None + self._OhsomeBaseClient__retry = False + self._handle_request() + + raise OhsomeException( + message=str(e), + url=self._url, + params=self._parameters, + response=e.response, + ) + return response + def _check_response(self, response: Response) -> None: + try: + response.raise_for_status() except requests.exceptions.HTTPError as e: try: error_message = e.response.json()["message"] except json.decoder.JSONDecodeError: error_message = f"Invalid URL: Is {self._url} valid?" - ohsome_exception = OhsomeException( + raise OhsomeException( message=error_message, url=self._url, params=self._parameters, @@ -354,15 +388,17 @@ def _handle_request(self) -> OhsomeResponse: response=e.response, ) - except requests.exceptions.ConnectionError as e: - ohsome_exception = OhsomeException( - message="Connection Error: Query could not be sent. Make sure there are no network " - f"problems and that the ohsome API URL {self._url} is valid.", + except KeyboardInterrupt: + raise OhsomeException( + message="Keyboard Interrupt: Query was interrupted by the user.", url=self._url, params=self._parameters, - response=e.response, + error_code=440, ) + def _get_response_data(self, response: Response) -> dict: + try: + return response.json() except (ValueError, JSONDecodeError) as e: if response: error_code, message = extract_error_message_from_invalid_json( @@ -371,53 +407,21 @@ def _handle_request(self) -> OhsomeResponse: else: message = str(e) error_code = None - ohsome_exception = OhsomeException( + raise OhsomeException( message=message, url=self._url, error_code=error_code, params=self._parameters, response=response, ) - - except requests.exceptions.RequestException as e: - if isinstance(e, RetryError): - # retry one last time without retries, this will raise the original error instead of a cryptic retry - # error (or succeed) - self._OhsomeBaseClient__session = None - self._OhsomeBaseClient__retry = False - self._handle_request() - - ohsome_exception = OhsomeException( - message=str(e), - url=self._url, - params=self._parameters, - response=e.response, - ) - - except KeyboardInterrupt: - ohsome_exception = OhsomeException( - message="Keyboard Interrupt: Query was interrupted by the user.", - url=self._url, - params=self._parameters, - error_code=440, - ) - except AttributeError: - ohsome_exception = OhsomeException( + raise OhsomeException( message=f"Seems like {self._url} is not a valid endpoint.", url=self._url, error_code=404, params=self._parameters, ) - # If there has been an error and logging is enabled, write it to file - if ohsome_exception: - if self.log: - ohsome_exception.log(self.log_dir) - raise ohsome_exception - - return OhsomeResponse(data=data, url=self._url) - def _format_parameters(self, params): """ Check and format parameters of the query From 139cdef01ae8971fa4a1642dd11238dcee47a6e6 Mon Sep 17 00:00:00 2001 From: Moritz Schott Date: Tue, 3 Sep 2024 21:32:38 +0200 Subject: [PATCH 09/10] move keyboard interrup to correct location --- ohsome/clients.py | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-) diff --git a/ohsome/clients.py b/ohsome/clients.py index 81be036..d64e461 100644 --- a/ohsome/clients.py +++ b/ohsome/clients.py @@ -347,6 +347,13 @@ def _handle_request(self) -> OhsomeResponse: def _post_request(self) -> Response: try: response = self._session().post(url=self._url, data=self._parameters) + except KeyboardInterrupt: + raise OhsomeException( + message="Keyboard Interrupt: Query was interrupted by the user.", + url=self._url, + params=self._parameters, + error_code=440, + ) except requests.exceptions.ConnectionError as e: raise OhsomeException( message="Connection Error: Query could not be sent. Make sure there are no network " @@ -388,14 +395,6 @@ def _check_response(self, response: Response) -> None: response=e.response, ) - except KeyboardInterrupt: - raise OhsomeException( - message="Keyboard Interrupt: Query was interrupted by the user.", - url=self._url, - params=self._parameters, - error_code=440, - ) - def _get_response_data(self, response: Response) -> dict: try: return response.json() From a6206d580d639f64a196cc61315f5f2183053379 Mon Sep 17 00:00:00 2001 From: Moritz Schott Date: Tue, 3 Sep 2024 21:37:50 +0200 Subject: [PATCH 10/10] update CHANGELOG.md --- CHANGELOG.md | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 1b5bb75..7ac47e4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,14 @@ - Ordering of exception handling to correctly parse a broken response - Assert that the expected columns are also present if the result is empty +### Removed + +- unused attributes `response` and `parameters`/`params` from `OhsomeResponse` + +### Added + +- init variable `data` to `OhsomeResponse` + ## 0.3.2 ### Fixed