be more flexible with responses coming through repo_auth_custom_exception_handler #659
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
✅ All tests successful. No failed tests found @@ Coverage Diff @@
## main #659 +/- ##
=======================================
Coverage 91.58% 91.58%
=======================================
Files 628 628
Lines 16698 16698
=======================================
Hits 15293 15293
Misses 1405 1405
Flags with carried forward coverage won't be shown. Click here to find out more.
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
✅ All tests successful. No failed tests found. Additional details and impacted files@@ Coverage Diff @@
## main #659 +/- ##
===========================================
Coverage 95.94000 95.94000
===========================================
Files 806 806
Lines 18015 18015
===========================================
Hits 17285 17285
Misses 730 730
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
| if ( | ||
| response is not None | ||
| and hasattr(response, "status_code") | ||
| and response.status_code == 401 | ||
| and hasattr(response, "data") | ||
| ): |
There was a problem hiding this comment.
these hasattrs are making me think we're sometimes getting something that is a totally different type than we're expecting. i am confused
the KeyErrors in sentry were just about that detail key, were you also seeing errors about status_code and data not even existing? the link in the PR description is just to the project, not a specific issue
There was a problem hiding this comment.
you right - I think lots of types of exceptions are flowing through here but this is an overreaction, to make it fully bomb-proof, so that this type of issue can't arise again. It's doing the safe checks for every element of the object that we check - just because I was thinking, what if I just patch this KeyError with detail and another similar issue arises with this block.
What do you think - should I pull it back to just the KeyError, or should I leave this super cautious logic?
There was a problem hiding this comment.
Added some in-line comments to reflect my thoughts here, but going to merge so that the issue can be fixed.
Codecov ReportAll modified and coverable lines are covered by tests ✅ ✅ All tests successful. No failed tests found. 📢 Thoughts on this report? Let us know! |
Codecov ReportAll modified and coverable lines are covered by tests ✅
✅ All tests successful. No failed tests found. @@ Coverage Diff @@
## main #659 +/- ##
=======================================
Coverage 91.58% 91.58%
=======================================
Files 628 628
Lines 16698 16698
=======================================
Hits 15293 15293
Misses 1405 1405
Flags with carried forward coverage won't be shown. Click here to find out more.
📣 Codecov offers a browser extension for seamless coverage viewing on GitHub. Try it in Chrome or Firefox today! |
nora-codecov
force-pushed
the
nora/support-bug-2
branch
from
474dc21 to
13f7e58
Compare
nora-codecov
force-pushed
the
nora/support-bug-2
branch
from
13f7e58 to
f900bb9
Compare
Purpose/Motivation
This code introduced an issue when it was added, sentry entry here. lots of responses were getting a
KeyErrorbecause they didn't have the format that was expected.What does this PR do?
Makes the logic in the function much more cautious, does several checks instead of making assumptions