Skip to content
This repository was archived by the owner on Jun 13, 2023. It is now read-only.

Adding support for exception handlers#335

Merged
maorlx merged 7 commits intomasterfrom
fastapi_error_handlers_support-EP-6197
Mar 15, 2021
Merged

Adding support for exception handlers#335
maorlx merged 7 commits intomasterfrom
fastapi_error_handlers_support-EP-6197

Conversation

@maorlx
Copy link
Copy Markdown
Contributor

@maorlx maorlx commented Mar 14, 2021

No description provided.

@maorlx maorlx requested a review from a team as a code owner March 14, 2021 12:50
@maorlx maorlx requested a review from Alon-Katz March 14, 2021 12:50
Returns None if not found.
"""
try:
if not request.scope:
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

When will a request wont have a scope? and can this create an issue with us unable to collect the trace in this case?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

well it shouldn't happen as its a part of the ASGI interface. Anyway, we check it so we will be failsafe (at this unexpected scenario it will be fine to collect a trace)

json.dumps(loop.run_until_complete(request.json()))
)
except json.decoder.JSONDecodeError:
pass
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

DecodeError might happen only if the json was empty? if not maybe we should log this error as well?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

well it was actually with log error before and it was spammy (every empty body caused this log)

Comment thread epsagon/wrappers/fastapi.py Outdated
epsagon_scope[SCOPE_IGNORE_REQUEST] = True

except Exception as exception: # pylint: disable=W0703
return original_handler(*args, **kwargs)
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Seems like most of the catch blocks are doing the same call and catching all exceptions, possible to create a bigger try catch block? Also this method (_fastapi_handler) gets to much big and to many returns, we should try to split it into smaller methods and this "# pylint: disable=too-many-return-statements" might not be needed then

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I will try to split it, but some of the except Exception adds some logic (print_debug, warning, setting some var names)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

splitted the function ;)

@maorlx maorlx merged commit 9923a9d into master Mar 15, 2021
@maorlx maorlx deleted the fastapi_error_handlers_support-EP-6197 branch March 15, 2021 13:48
@enoodle
Copy link
Copy Markdown
Contributor

enoodle commented Mar 15, 2021

🎉 This PR is included in version 1.64.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants