fix(response): reoder exception handling to correctly parse a broken …

…response (#164) split exception handling into three parts dedicated to the kind of exception that could happen Co-authored-by: Matthias (~talfus-laddus) <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
GIScience · Sep 9, 2024 · b27c519 · b27c519
1 parent af13807
commit b27c519
Show file tree

Hide file tree

Showing 8 changed files with 84 additions and 49 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -4,8 +4,17 @@
 
 ### Fixed
 
+- 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

diff --git a/ohsome/clients.py b/ohsome/clients.py
@@ -13,9 +13,9 @@
 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
+from requests.exceptions import RetryError, JSONDecodeError
 from urllib3 import Retry
 
 from ohsome import OhsomeException, OhsomeResponse
@@ -332,37 +332,36 @@ def _handle_request(self) -> OhsomeResponse:
         Handles request to ohsome API
         :return:
         """
-        ohsome_exception = None
-        response = None
 
         try:
-            response = self._session().post(url=self._url, data=self._parameters)
-            response.raise_for_status()
-            response.json()
+            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
 
-        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?"
+        return OhsomeResponse(data=data, url=self._url)
 
-            ohsome_exception = OhsomeException(
-                message=error_message,
+    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=e.response.status_code,
-                response=e.response,
+                error_code=440,
             )
-
         except requests.exceptions.ConnectionError as e:
-            ohsome_exception = OhsomeException(
+            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
@@ -371,53 +370,57 @@ def _handle_request(self) -> OhsomeResponse:
                 self._OhsomeBaseClient__retry = False
                 self._handle_request()
 
-            ohsome_exception = OhsomeException(
+            raise OhsomeException(
                 message=str(e),
                 url=self._url,
                 params=self._parameters,
                 response=e.response,
             )
+        return response
 
-        except KeyboardInterrupt:
-            ohsome_exception = OhsomeException(
-                message="Keyboard Interrupt: Query was interrupted by the user.",
+    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?"
+
+            raise OhsomeException(
+                message=error_message,
                 url=self._url,
                 params=self._parameters,
-                error_code=440,
+                error_code=e.response.status_code,
+                response=e.response,
             )
 
-        except ValueError as e:
+    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(
                     response.text
                 )
             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 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(response, url=self._url, params=self._parameters)
-
     def _format_parameters(self, params):
         """
         Check and format parameters of the query

diff --git 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]:

diff --git 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
@@ -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_exceptions.py b/ohsome/test/test_exceptions.py
@@ -43,6 +43,31 @@ def test_timeout_error(base_client):
     )
 
 
+def test_broken_response_timeout_error(base_client):
+    """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"
+    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():
     """
@@ -194,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!"

diff --git 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")
     )
 

diff --git a/poetry.lock b/poetry.lock