Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Include more details in the ApiConnectionException message #316

Merged
merged 10 commits into from
Feb 17, 2023
Merged

Include more details in the ApiConnectionException message #316

merged 10 commits into from
Feb 17, 2023

Conversation

Andy-Grigg
Copy link
Contributor

@Andy-Grigg Andy-Grigg commented Feb 14, 2023 •
edited
Loading

Closes #313

Improves the message included in an ApiConnectionException to include all the information that is passed into the exception constructor, instead of just the response from the server.

Also adds the URL that resulted in the exception, resulting in a stack trace that looks like this:

Traceback (most recent call last):
  File "C:\Users\<username>\AppData\Roaming\JetBrains\PyCharm2022.2\scratches\scratch_5.py", line 6, in <module>
    conn = ApiClientFactory(api_url=url).with_credentials(**creds).connect()
  File "C:\git\pyansys\openapi-common\src\ansys\openapi\common\_session.py", line 213, in with_credentials
    if self.__test_connection():
  File "C:\git\pyansys\openapi-common\src\ansys\openapi\common\_session.py", line 317, in __test_connection
    raise ApiConnectionException(
ansys.openapi.common._exceptions.ApiConnectionException: Request url 'http://hostname/invalid/path' failed with reason 404: Not Found.
The following was returned by the server:
<!DOCTYPE html>
<html>
...

In some cases the content returned by the server would have been enough to figure out the cause of the issue, but not always. We now always raise an exception with at least the following:

  • url
  • status code
  • reason phrase

If any content was returned by the server, it is also included in the exception.

@codecov
Copy link

codecov bot commented Feb 14, 2023 •
edited
Loading

Codecov Report

Merging #316 (13b1d73) into main (3d23058) will increase coverage by 0.13%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #316      +/-   ##
==========================================
+ Coverage   94.34%   94.47%   +0.13%     
==========================================
  Files           8        8              
  Lines         796      797       +1     
==========================================
+ Hits          751      753       +2     
+ Misses         45       44       -1
Impacted Files Coverage Δ
src/ansys/openapi/common/_exceptions.py 97.61% <100.00%> (+0.05%) ⬆️
src/ansys/openapi/common/_session.py 88.13% <100.00%> (+0.56%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

da1910
da1910 approved these changes Feb 15, 2023
View reviewed changes
Copy link
Collaborator

@da1910 da1910 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks as expected. Only thing we might want to consider is including the actual failing request/response as properties of the exception, things like headers are likely to be helpful with debugging auth issues.

@ludovicsteinbach
Copy link
Contributor

Renaming message to content is a breaking change that will affect private packages

@Andy-Grigg
Copy link
Contributor Author

Renaming message to content is a breaking change that will affect private packages

That's a very good point. We could leave it as 'message', but it will be a bit confusing that it essentially repurposes the base exception keyword argument.

We might not have a choice though. I might argue that there are no actual packages that are using ApiConnectionException right now (BomAnalytics uses ApiException, am I missing anything?), but that's not really good enough if we're obeying our versioning rules.

@Andy-Grigg
Copy link
Contributor Author

Changes should now be non-breaking. I haven't added the actual response object to the exception itself, my rationale being that it's another arg (that in reality could replace 3 of them, but not if we want a non-breaking change), and that if you're actually trying to fix the issue, it's trivial to just step up one level in the call stack to see what the problem is.

@Andy-Grigg
Copy link
Contributor Author

After further discussion/thinking, we've decided to go ahead with breaking changes, and the response object is now the only thing passed to the exception. Extraction of relevant information is now handled in the exception itself.

ludovicsteinbach
ludovicsteinbach approved these changes Feb 17, 2023
View reviewed changes
Copy link
Contributor

@ludovicsteinbach ludovicsteinbach left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me

@Andy-Grigg Andy-Grigg added this pull request to the merge queue Feb 17, 2023
Merged via the queue into main with commit 1d42d0a Feb 17, 2023
@Andy-Grigg Andy-Grigg deleted the feat/313-improve-ApiConnectionException-message branch February 17, 2023 13:15
@Andy-Grigg Andy-Grigg mentioned this pull request Feb 20, 2023
Merged
Andy-Grigg added a commit that referenced this pull request Feb 20, 2023
@Andy-Grigg
Include more details in the ApiConnectionException message (#316)
8423aa4 
* Include more details in the ApiConnectionException message

* Content might be empty

* Make changes non-breaking

* Use response object

* Check exception repr

* Run black

* Fix integration test

* Test initial response handler
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ludovicsteinbach ludovicsteinbach ludovicsteinbach approved these changes

@da1910 da1910 Awaiting requested review from da1910

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Bug located in ApiConnectionException
3 participants
@Andy-Grigg @ludovicsteinbach @da1910