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.

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

Error handler should not capture exceptions in DEBUG mode #1151

Closed
silentninja opened this issue Mar 9, 2022 · 12 comments · Fixed by #1953
Closed

Error handler should not capture exceptions in DEBUG mode #1151

silentninja opened this issue Mar 9, 2022 · 12 comments · Fixed by #1953
Assignees
Labels
good first issue Everything in "Help wanted", PLUS being relatively easy and straightforward to implement. help wanted Community contributors can implement this type: enhancement New feature or request work: backend Related to Python, Django, and simple SQL
Milestone

Comments

@silentninja
Copy link
Contributor

silentninja commented Mar 9, 2022

Description

Based on the conclusion from the discussion.

Improve backend's debugability via two changes:

  1. when backend is in debug-mode, add a pretty-printed stacktrace alongside the current error JSON;
  2. stop wrapping unhandled exceptions in 4999 errors.

For some context, we're catching backend exceptions and wrapping them in custom errors so as to provide a uniform error interface to API clients. As it is now it has detrimental effects on debugging, because the resulting error messages don't include stacktraces. Above changes aim to address that.

@silentninja silentninja added type: bug Something isn't working work: backend Related to Python, Django, and simple SQL status: draft labels Mar 9, 2022
@silentninja silentninja changed the title Error handler should not capture exceptions instead should print the stacktrace. Error handler should not capture exceptions instead should print the stacktrace in DEBUG mode Mar 10, 2022
@silentninja silentninja changed the title Error handler should not capture exceptions instead should print the stacktrace in DEBUG mode Error handler should not capture exceptions in DEBUG mode Mar 10, 2022
@dmos62
Copy link
Contributor

dmos62 commented Mar 15, 2022

Made changes to top post to include changes decided upon in linked discussion.

@dmos62 dmos62 added help wanted Community contributors can implement this ready Ready for implementation type: enhancement New feature or request good first issue Everything in "Help wanted", PLUS being relatively easy and straightforward to implement. and removed status: draft type: bug Something isn't working labels Mar 15, 2022
@vorakunal
Copy link

May I work on this issue?

@kgodey kgodey added status: started and removed ready Ready for implementation labels Apr 7, 2022
@kgodey
Copy link
Contributor

kgodey commented Apr 7, 2022

Go ahead @vorakunal, thanks!

@kgodey
Copy link
Contributor

kgodey commented Apr 29, 2022

@vorakunal Are you still working on this?

@kgodey kgodey modified the milestones: [06.2] 2022-04 improvements, [08.1] 2022-05 improvements May 2, 2022
@kgodey kgodey added ready Ready for implementation and removed status: started labels May 6, 2022
@kgodey
Copy link
Contributor

kgodey commented May 6, 2022

Unassigned due to inactivity.

@kgodey kgodey modified the milestones: [08.1] 2022-05 improvements, High Priority Jun 1, 2022
@kgodey kgodey removed this from the High Priority milestone Jul 19, 2022
@aadityasinha-dotcom
Copy link

Can I work on this issue?

@kgodey
Copy link
Contributor

kgodey commented Jul 24, 2022

Yes, go ahead @aadityasinha-dotcom. Thanks for your interest!

@kgodey kgodey added status: started and removed ready Ready for implementation labels Jul 24, 2022
@kgodey kgodey added ready Ready for implementation and removed status: started labels Sep 1, 2022
@DavidKratochvila
Copy link

I'd like to try to solve this issue, if it's still available.

@kgodey
Copy link
Contributor

kgodey commented Oct 11, 2022

Sure, go ahead @DavidKratochvila

@IyadElwy
Copy link
Contributor

@kgodey @silentninja
I'm currently looking into this issue.
I've tried a couple of things but here's an example of an error before and after my changes.

Before:

[
    {
        "code": 4004,
        "message": "CSRF Failed: CSRF token missing or incorrect.",
        "details": {
            "exception": "CSRF Failed: CSRF token missing or incorrect."
        }
    }
]

After:

[
    {
        "code": 4004,
        "message": "CSRF Failed: CSRF token missing or incorrect.",
        "details": {
            "exception": "CSRF Failed: CSRF token missing or incorrect."
        },
        "stacktrace": [
            "  File \"/usr/local/lib/python3.9/site-packages/rest_framework/views.py\", line 497, in dispatch",
            "    self.initial(request, *args, **kwargs)",
            "  File \"/usr/local/lib/python3.9/site-packages/rest_framework/views.py\", line 414, in initial",
            "    self.perform_authentication(request)",
            "  File \"/usr/local/lib/python3.9/site-packages/rest_framework/views.py\", line 324, in perform_authentication",
            "    request.user",
            "  File \"/usr/local/lib/python3.9/site-packages/rest_framework/request.py\", line 227, in user",
            "    self._authenticate()",
            "  File \"/usr/local/lib/python3.9/site-packages/rest_framework/request.py\", line 380, in _authenticate",
            "    user_auth_tuple = authenticator.authenticate(self)",
            "  File \"/usr/local/lib/python3.9/site-packages/rest_framework/authentication.py\", line 130, in authenticate",
            "    self.enforce_csrf(request)",
            "  File \"/usr/local/lib/python3.9/site-packages/rest_framework/authentication.py\", line 148, in enforce_csrf",
            "    raise exceptions.PermissionDenied('CSRF Failed: %s' % reason)",
            "rest_framework.exceptions.PermissionDenied: CSRF Failed: CSRF token missing or incorrect."
        ]
    }
]

This way, when running on Debug mode the developer gets a lot more to work with, which was the intended goal after all. I'm still working on making it better but would appreciate anyone telling me if I'm on the right track and or giving any feedback in general.

@dmos62 dmos62 added status: started and removed ready Ready for implementation labels Nov 18, 2022
@IyadElwy
Copy link
Contributor

IyadElwy commented Nov 18, 2022

You could assign me to the issue and I'll continue working on it.
I think a good addition to the stacktrace would be numbering so I added line numbers for a better visualization.

[
    {
        "code": 4004,
        "message": "CSRF Failed: CSRF token missing or incorrect.",
        "details": {
            "exception": "CSRF Failed: CSRF token missing or incorrect."
        },
        "stacktrace": [
            "1. File \"/usr/local/lib/python3.9/site-packages/rest_framework/views.py\", line 497, in dispatch",
            "2. self.initial(request, *args, **kwargs)",
            "3. File \"/usr/local/lib/python3.9/site-packages/rest_framework/views.py\", line 414, in initial",
            "4. self.perform_authentication(request)",
            "5. File \"/usr/local/lib/python3.9/site-packages/rest_framework/views.py\", line 324, in perform_authentication",
            "6. request.user",
            "7. File \"/usr/local/lib/python3.9/site-packages/rest_framework/request.py\", line 227, in user",
            "8. self._authenticate()",
            "9. File \"/usr/local/lib/python3.9/site-packages/rest_framework/request.py\", line 380, in _authenticate",
            "10. user_auth_tuple = authenticator.authenticate(self)",
            "11. File \"/usr/local/lib/python3.9/site-packages/rest_framework/authentication.py\", line 130, in authenticate",
            "12. self.enforce_csrf(request)",
            "13. File \"/usr/local/lib/python3.9/site-packages/rest_framework/authentication.py\", line 148, in enforce_csrf",
            "14. raise exceptions.PermissionDenied('CSRF Failed: %s' % reason)",
            "15. rest_framework.exceptions.PermissionDenied: CSRF Failed: CSRF token missing or incorrect."
        ]
    }
]

I added a Draft PR so that anyone could add comments, etc.

@silentninja
Copy link
Contributor Author

silentninja commented Nov 18, 2022

@IyadElwy That looks pretty neat and you are certainly on the right track. Thanks! for the PR, I will take a look at it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Everything in "Help wanted", PLUS being relatively easy and straightforward to implement. help wanted Community contributors can implement this type: enhancement New feature or request work: backend Related to Python, Django, and simple SQL
Projects
No open projects
Development

Successfully merging a pull request may close this issue.

7 participants