Raise exceptions in ohsome client instead of returning None in case of failed ohsome API query #29

matthiasschaub · 2021-05-19T08:06:38Z

Corresponding issue

Closes #23
Closes #22

Checklist

I have updated my branch to main (e.g. through git rebase main)
Rebase or squash commits in a reasonable manner
My code follows the style guide and was checked with pre-commit before committing
I have commented my code
I have added sufficient unit and integration tests
I have updated the CHANGELOG.md
@joker234 Review check_iso_time
@joker234 Review test_ohsome_client.py
@matthiasschaub fix failing test https://jenkins.ohsome.org/job/ohsome-quality-analyst/job/PR-29/15/display/redirect

workers/ohsome_quality_analyst/ohsome/client.py

workers/tests/integrationtests/fixtures/vcr_cassettes/test_indicator_tags_ratio.json

joker234

Thanks for dealing with this issue. I left some suggestions, all in all a good change 👍

joker234 · 2021-05-19T15:09:48Z

workers/tests/unittests/test_ohsome_client.py

-                200, content=self.invalid_geojson
+                200,
+                content=self.invalid_geojson,
+                request=httpx.Request("POST", "mock.org"),


Changed URL to http://www.example.org/

joker234 · 2021-05-19T15:10:06Z

workers/tests/unittests/test_ohsome_client.py

+            mock_request.return_value = httpx.Response(
+                200,
+                content=self.valid_geojson,
+                request=httpx.Request("POST", "mock.org"),


What is the page "mock.org" doing in here? I assume you want to query another page here.

Actually this will make a request to a website. It just create the object which is needed by the httpx client to make a request: https://www.python-httpx.org/api/#request

I substitude mock.org with "url" to make the generic naming more clear.

Changed URL to http://www.example.org/

joker234 · 2021-05-19T15:10:13Z

workers/tests/unittests/test_ohsome_client.py

+            mock_request.return_value = httpx.Response(
+                400,
+                content=self.invalid_geojson,
+                request=httpx.Request("POST", "mock.org"),


Changed URL to http://www.example.org/

workers/ohsome_quality_analyst/ohsome/client.py

joker234 · 2021-05-20T14:05:26Z

workers/ohsome_quality_analyst/ohsome/client.py

+    try:
+        resp.raise_for_status()  # Raise for response status codes 4xx and 5xx
+    except httpx.HTTPStatusError:
+        logging.error("Query ohsome API failed!")
+        raise


Currently, these errors are returned to the user just as “Internal Server Error 500”. It might be useful to give the user a better error message.

Can we provide more context then stating that quering the ohsome API failed since response status code could be any of 4xx and 5xx? Should we state more clearly that the reponse code has bee returend from the ohsome API?

Does commit b390b84 adresses the problem you raised here?

workers/tests/integrationtests/fixtures/vcr_cassettes/test_indicator_tags_ratio.json

workers/ohsome_quality_analyst/ohsome/client.py

matthiasschaub · 2021-05-25T06:57:17Z

workers/ohsome_quality_analyst/ohsome/client.py

+    return data
+
+
+def check_iso_time(time: str) -> bool:


I started to implement this function to validate the time parameter before it is used as query parameter with the ohsome API. But since the ISO-8601 conform timestring(s) can be quite versatile it can only be tested rudimentary with the built-in stdlib. An alternative is to use a library such as dateutils to validate the timestring. Another one is to forgo it and pass it to the ohsome API in the hope to catch invalid timestring through the response of the ohsome API.

@joker234 I would be happy to hear your thoughts on that.

I'll review the implemented suggestion, but in general I would say you shouldn't deal with that. The ohsome API only covers part of the ISO 8601 implementation. I would suggest to catch the exception/error of the ohsome API and deal with that properly. In general I would never try to deal with time parsing manually, but use existing, well-maintained libraries like dateutil.

Maybe @tyrasd has an additional opinion on the general matter if it's reasonable to check the time input in your own code or if the ohsome API should deal with that.

I like the idea to catch invalid timestrings through the response of ohsome API.

While playing around with the ohsome API I found out two different response related to wrong use of the time parameter:

{ "timestamp": "2021-05-31T07:33:31.206528", "status": 400, "message": "You need to give at least two timestamps or a time interval for this resource.", "requestUrl": "https://api.ohsome.org/v1/contributions/count?bboxes=8.67%2C49.39%2C8.71%2C49.42&filter=type%3Away%20and%20natural%3D*&format=json&time=20140101" } { "timestamp": "2021-05-31T07:36:40.661092", "status": 400, "message": "The provided time parameter is not ISO-8601 conform.", "requestUrl": "https://api.ohsome.org/v1/contributions/count?bboxes=8.67%2C49.39%2C8.71%2C49.42&filter=type%3Away%20and%20natural%3D*&format=json&time=20140101%2C2015010" }

@tyrasd are there more cases that should be covered? I would be happy to here your thoughts on that.

I removed functions for testing the time parameter.
The code in line 43 will raise an exception if the ohsome response will return status code 400 when the time parameter is invalid.

try: resp.raise_for_status() # Raise for response status codes 4xx and 5xx except httpx.HTTPStatusError as error: raise OhsomeApiError( "Querying the ohsome API failed!" + error.response.json()["message"] ) from error

Which will lead to following Error:

Traceback (most recent call last): File "/home/matthias/work/oqt/test.py", line 7, in <module> resp.raise_for_status() # Raise for response status codes 4xx and 5xx File "/home/matthias/.cache/pypoetry/virtualenvs/ohsome-quality-analyst-JhxvPu0L-py3.9/lib/python3.9/site-packages/httpx/_models.py", line 1105, in raise_for_status raise HTTPStatusError(message, request=request, response=self) httpx.HTTPStatusError: 400 Client Error: for url: https://api.ohsome.org/v1/contributions/count?bboxes=8.67%2C49.39%2C8.71%2C49.42&filter=type%3Away%20and%20natural%3D*&format=json&time=20140101%2C2015010 For more information check: https://httpstatuses.com/400 The above exception was the direct cause of the following exception: Traceback (most recent call last): File "/home/matthias/work/oqt/test.py", line 10, in <module> raise OhsomeApiError("Querying the ohsome API failed! " + _) from error ohsome_quality_analyst.utils.exceptions.OhsomeApiError: Querying the ohsome API failed! The provided time parameter is not ISO-8601 conform.

joker234

Made some suggestions.
Additional ToDos (added to checklist):

Review check_iso_time
Review test_ohsome_client.py
fix failing test

workers/ohsome_quality_analyst/ohsome/client.py

joker234 · 2021-05-28T13:09:23Z

workers/tests/unittests/test_ohsome_client.py

+        # self.assertTrue(ohsome_client.check_iso_time("2014-01-01/2018-01-01/P1Y"))
+
+    def test_build_url(self) -> None:
+        ohsome_api = OHSOME_API.rstrip("/")


I'm not a huge fan of using variables from outside of the test classes. You can keep it like that, but I would have defined the OHSOME_API variable separately in the test class.

Disadvantage: if you change the default value, you have to change it in the tests as well, which you will recognize, because the tests will run ;)
Advantage: no one can accidentally change the default value of this variable. E.g. because the person runs the tests with the OHSOME_API environment variable anyway

Yes, I see your point. I removed the dependecy on the env and rely on hardcoded URL for now.

workers/ohsome_quality_analyst/ohsome/client.py

joker234 · 2021-05-28T13:49:30Z

workers/ohsome_quality_analyst/ohsome/client.py

+    return data
+
+
+def check_iso_time(time: str) -> bool:


I'll review the implemented suggestion, but in general I would say you shouldn't deal with that. The ohsome API only covers part of the ISO 8601 implementation. I would suggest to catch the exception/error of the ohsome API and deal with that properly. In general I would never try to deal with time parsing manually, but use existing, well-maintained libraries like dateutil.

Maybe @tyrasd has an additional opinion on the general matter if it's reasonable to check the time input in your own code or if the ohsome API should deal with that.

workers/ohsome_quality_analyst/ohsome/client.py

workers/tests/unittests/test_ohsome_client.py

Raise exceptions in ohsome client instead of returning None in case of failed ohsome API query.

Co-authored-by: Johannes Visintini <johannes.visintini@heigit.org>

Improve error message by adding a custom Exception for the ohsome client module and additional context in text form.

Co-authored-by: Johannes Visintini <johannes.visintini@heigit.org>

joker234 · 2021-06-08T17:26:40Z

workers/ohsome_quality_analyst/ohsome/client.py

+        resp.raise_for_status()
+    except httpx.HTTPStatusError as error:
+        raise OhsomeApiError(
+            "Querying the ohsome API failed!" + error.response.json()["message"]


Didn't try this out, but I would add spacing between the two strings, e.g.:

Suggested change

"Querying the ohsome API failed!" + error.response.json()["message"]

"Querying the ohsome API failed: " + error.response.json()["message"]

matthiasschaub added the code quality Related to our standards for 'good' code label May 19, 2021

matthiasschaub self-assigned this May 19, 2021

matthiasschaub added the bug Something isn't working label May 19, 2021

matthiasschaub force-pushed the ohsome-client-exception branch from c82664e to 759f46c Compare May 19, 2021 08:09

matthiasschaub commented May 19, 2021

View reviewed changes

workers/ohsome_quality_analyst/ohsome/client.py Outdated Show resolved Hide resolved

matthiasschaub commented May 19, 2021

View reviewed changes

workers/ohsome_quality_analyst/ohsome/client.py Outdated Show resolved Hide resolved

matthiasschaub force-pushed the ohsome-client-exception branch 3 times, most recently from d869a02 to d802dbe Compare May 19, 2021 14:47

matthiasschaub requested a review from joker234 May 19, 2021 14:47

matthiasschaub changed the title ~~🚧 Raise exceptions in ohsome client instead of returning None in case of failed ohsome API query~~ Raise exceptions in ohsome client instead of returning None in case of failed ohsome API query May 19, 2021

joker234 reviewed May 19, 2021

View reviewed changes

workers/tests/integrationtests/fixtures/vcr_cassettes/test_indicator_tags_ratio.json Outdated Show resolved Hide resolved

joker234 added this to the Release 0.4.0 milestone May 20, 2021

joker234 requested changes May 20, 2021

View reviewed changes

joker234 reviewed May 20, 2021

View reviewed changes

workers/ohsome_quality_analyst/ohsome/client.py Outdated Show resolved Hide resolved

matthiasschaub force-pushed the ohsome-client-exception branch from 0c26995 to b165c08 Compare May 23, 2021 09:26

matthiasschaub marked this pull request as draft May 24, 2021 11:27

matthiasschaub commented May 25, 2021

View reviewed changes

matthiasschaub marked this pull request as ready for review May 25, 2021 12:11

matthiasschaub requested a review from joker234 May 25, 2021 12:11

joker234 reviewed May 27, 2021

View reviewed changes

joker234 assigned joker234 and unassigned joker234 May 27, 2021

joker234 reviewed May 28, 2021

View reviewed changes

matthiasschaub force-pushed the ohsome-client-exception branch from 635b08f to 9127f4a Compare May 31, 2021 10:28

matthiasschaub marked this pull request as draft June 1, 2021 07:47

matthiasschaub force-pushed the ohsome-client-exception branch 2 times, most recently from a0724b1 to 8f242a2 Compare June 1, 2021 14:23

matthiasschaub added 2 commits June 7, 2021 13:38

Raise exceptions instead of returning None

51bfba2

Raise exceptions in ohsome client instead of returning None in case of failed ohsome API query.

Recreate VCR cassette

b2aa1a3

Substitute removesuffix with rstrip

0277e5c

Co-authored-by: Johannes Visintini <johannes.visintini@heigit.org>

matthiasschaub force-pushed the ohsome-client-exception branch from 8f242a2 to 65ac79f Compare June 7, 2021 11:38

matthiasschaub marked this pull request as ready for review June 8, 2021 08:29

matthiasschaub requested a review from joker234 June 8, 2021 08:29

matthiasschaub and others added 17 commits June 8, 2021 10:32

Add build_data_dict function to ohsome client

060b0aa

Add unittests for build_data_dict of ohsome client

82dd813

Improve comment

0b040e9

Co-authored-by: Johannes Visintini <johannes.visintini@heigit.org>

Remove duplicated log message

759d9e0

Co-authored-by: Johannes Visintini <johannes.visintini@heigit.org>

Update CHANGELOG.md

a0f88e6

Co-authored-by: Johannes Visintini <johannes.visintini@heigit.org>

Add unittests for build_url function

8c77eb5

Add custom exception for ohsome client

58cac7e

Improve error message by adding a custom Exception for the ohsome client module and additional context in text form.

Minor spelling fix

e523da7

Co-authored-by: Johannes Visintini <johannes.visintini@heigit.org>

Minor changes to comments and names

09c7c94

Remove check and tests for time parameter

12fa5f8

Improve OhsomeApiError by adding msg of resp

e39205d

Update Changelog

44761ce

Reset VCR cassette to version in main

883664a

Revert removing empty line

35e5c2a

Remove dependency on env for ohsome client tests

eb0bd52

Add comment on time parameter validation

41fb8b8

Add test for invalid ohsome api response

b56d65f

matthiasschaub force-pushed the ohsome-client-exception branch from 1028de4 to b56d65f Compare June 8, 2021 08:33

joker234 approved these changes Jun 8, 2021

View reviewed changes

matthiasschaub merged commit 8bb4f0c into main Jun 9, 2021

matthiasschaub deleted the ohsome-client-exception branch June 9, 2021 07:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Raise exceptions in ohsome client instead of returning None in case of failed ohsome API query #29

Raise exceptions in ohsome client instead of returning None in case of failed ohsome API query #29

matthiasschaub commented May 19, 2021 •

edited

joker234 left a comment

joker234 May 19, 2021

matthiasschaub Jun 7, 2021

joker234 May 19, 2021

matthiasschaub May 25, 2021

matthiasschaub May 31, 2021

joker234 May 19, 2021

matthiasschaub Jun 7, 2021

joker234 May 20, 2021

matthiasschaub May 23, 2021

matthiasschaub May 25, 2021 •

edited

matthiasschaub May 25, 2021

joker234 May 28, 2021

matthiasschaub May 31, 2021 •

edited

matthiasschaub Jun 7, 2021

joker234 left a comment

joker234 May 28, 2021

matthiasschaub Jun 8, 2021

joker234 May 28, 2021

joker234 Jun 8, 2021

	"Querying the ohsome API failed!" + error.response.json()["message"]
	"Querying the ohsome API failed: " + error.response.json()["message"]

Raise exceptions in ohsome client instead of returning None in case of failed ohsome API query #29

Raise exceptions in ohsome client instead of returning None in case of failed ohsome API query #29

Conversation

matthiasschaub commented May 19, 2021 • edited

Corresponding issue

Checklist

joker234 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matthiasschaub May 25, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matthiasschaub May 31, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joker234 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matthiasschaub commented May 19, 2021 •

edited

matthiasschaub May 25, 2021 •

edited

matthiasschaub May 31, 2021 •

edited